RCN BasicCosigner Smart Contract Security Audit

Coinfabrik security audit’s team was asked to audit the BasicCosigner contract. This contract is part of a Loan system where multiple contracts interact, where the main contract is NanoLoanEngine. Particularly, this is an example implementation of a loan cosigner, which serves as a guarantee for the lender. In the first part, we will give a summary of our discoveries and then, the details of our findings.

The contract is at https://github.com/ripio/rcn-network/blob/master/contracts/examples/BasicCosigner.sol. The audit is based on the commit d206c4f1e95ac5c9a06defcbdc899b7fedb18f7c, and updated to reflect changes at fc63852b012fcad68dd174418d48d4d6740e15cc.

Detailed findings

Critical severity

NanoLoanEngine transfer doesn’t allow the lender to call the claim function safely

There is a transfer call to the engine in the claim function:

function claim(uint256 index) public returns (bool) {
   Liability storage liability = liabilities[index];
   require(engine.getCosigner(index) == address(this));
   require(isDefaulted(index));
   require(!liability.claimed);
   require(liability.coverage > 0);
   require(engine.getLender(index) == msg.sender);
   liability.claimed = true;
   engine.addInterest(index);
   uint debt = safeSubtract(safeAdd(engine.getAmount(index), engine.getInterest(index)), engine.getPaid(index));
   uint amount = safeMult(debt, liability.coverage) / 100;
   require(engine.transfer(index, this));
   require(token.transfer(msg.sender, safeMult(getOracleRate(index), amount)));
   return true;
}

This function is used to transfer ownership of the loan. It only allows the lender or someone approved by the lender to do so:

function transfer(uint index, address to) public returns (bool) {
   Loan storage loan = loans[index];
   require(loan.status != Status.destroyed);
   require(msg.sender == loan.lender || msg.sender == loan.approvedTransfer);
   require(to != address(0));
   Transfer(index, loan.lender, to);
   loan.lender = to;
   loan.approvedTransfer = address(0);
   return true;
}

In the case the sender is neither the lender nor someone approved, this function will always fail.

The alternative usage of the function is that the lender has to call approveTransfer before calling claim. RCN team confirmed that this is the expected workflow of the function. There is a possible attack using in this way.

The cosigner owner tries to put a transferLoan call to himself between approveTransfer and claim calls. Although these functions are called consecutively, the attacker can spam the transaction. This spam mechanism is a bit expensive, but it may result in a very good return for the cosigner owner as he saves the compensations. Note that the original lender cannot call claim after this, as he is not the lender of the loan anymore.

Update: This was fixed by the RCN team by locking the transfer of the loan until the lender calls claim at commit 70955c54e42fbecbb23783f7e8375c028ba1ad5f.

Minor severity

Solidity warnings

Multiple function qualifier warnings are emitted by the solidity compiler at compile time. Consider fixing these as they may occlude other more important warnings. Also, consider bumping up the required compiler version as more specific function mutability qualifiers have been added recently.

The Token Interface isn’t ERC20 standard.

Token Interface:

contract Token {
	function transfer(address _to, uint _value) returns (bool success);
	function transferFrom(address _from, address _to, uint256 _value) returns (bool success);
	function allowance(address _owner, address _spender) constant returns (uint256 remaining);
	function approve(address _spender, uint256 _value) returns (bool success);
	function increaseApproval (address _spender, uint _addedValue) public returns (bool success);
	function getCosigner(uint index) constant returns (address);
}

Consider using something like this:

contract Token {
   function totalSupply() constant returns (uint totalSupply);
   function balanceOf(address _owner) constant returns (uint balance);
   function transfer(address _to, uint _value) returns (bool success);
   function transferFrom(address _from, address _to, uint _value) returns (bool success);
   function approve(address _spender, uint _value) returns (bool success);
   function allowance(address _owner, address _spender) constant returns (uint remaining);
}

The Token interface seems to be used in place of a standard token interface. This isn’t a good idea in code that is meant to be an implementation example unless there’s a good reason for it. In which case, it should be properly documented in the example.

This was fixed in commit fc63852b012fcad68dd174418d48d4d6740e15cc.

Enhancements

Documentation

Documentation in the form of comments and/or a separate specification would greatly increase the auditability of the contract.

Testing

There are a few functions in this contract that may require higher than average amounts of gas. Crafting test cases not only improves the quality of the code, but it can even help to save gas.

Conclusion

The code quality is good but requires more documentation since it is was hard to infer the intention behind some functions, it would prevent misunderstandings and greatly improve auditability. 

Sever issues pointed were fixed by RCN team.