RCN Network Smart Contract Audit

Coinfabrik’s smart contract audit team was commissioned to conduct a security audit of the contracts for the Ripio RCN Network.

RCN Network is a P2P lending protocol based on smart contracts. The protocol connects lenders and borrowers from anywhere in the world.

The engine allows borrowers to create loan requests, and lenders to fulfill those requests. The loan can have a cosigner that will act as a guarantor. Loans are denominated in RCN Tokens, but Oracles can be defined to manage currency conversions.

Oracles are third-party contracts that can be called by the loan engine to determine the exchange rate between the loan’s currency and RCN tokens.

Summary

The audited repository is https://github.com/ripio/rcn-network, commit 3868e6470aae35a38d0acf7d443db311e435735d.

Reviewed files:

• NanoLoanEngine.sol: Loan’s Engine.
• utils\RpSafeMath.sol: Safe mathematical functions.
• interfaces\Token.sol: Standard interface for the RCN Network Token EIP-20 compatible.
• interfaces\Oracle.sol: Interface for the Oracles.
• examples\BasicOracle.sol: An example implementation of an Oracle.

The most important issue we found was the possibility of a recursive attack in case a borrower and Oracle work together to pay a loan before the interest was accounted, leaving the loan in an invalid state: pending amount would be non zero but the loan will not accept payments.

Details of our Findings

High severity

Invalid state in recursive attack to lend

The function lend in NanoLoanEngine.sol calls Oracle’s getRateFor. Since the Oracle is an external contract it can recursively call to NanoLoanEngine to cancel the debt before the interest is calculated, leaving the loan in an invalid state.

• NanoLoanEngine.lend() is called
  • loan status set to Status.lend
  • oracle.getRateFor() is called through NanoLoanEngine.getOracleRate()
    • Oracle call’s NanoLoanEngine.pay() paying the loan principal
      • loan status set to Status.paid
  • Inside lend function internalAddInterest() is called
    • Now getPendingAmount is non zero but loan status is set to paid, and cannot be paid and the lender will not recover the interest

We recommend to move interest calculation before making transfers:

    function lend(uint index) public returns (bool) {
        Loan storage loan = loans[index];
        require(loan.status == Status.initial);
        require(isApproved(index));

        loan.lender = msg.sender;
        loan.dueTime = safeAdd(block.timestamp, loan.duesIn);
        loan.interestTimestamp = block.timestamp;
        loan.status = Status.lent;

        if (loan.cancelableAt > 0) {
            internalAddInterest(index, safeAdd(block.timestamp, loan.cancelableAt));
        }

        uint256 rate = getOracleRate(index);
        require(token.transferFrom(msg.sender, loan.borrower, safeMult(loan.amount, rate)));

        if (loan.cosigner != address(0)) {
            require(token.transferFrom(msg.sender, loan.cosigner, safeMult(loan.cosignerFee, rate)));
        }

        Lent(index, loan.lender);
        return true;
    }

Fixed at 3030f25366f68501ba8d9f11932ccc7c4e275531.

Medium severity

Not using safe functions

Function calculateInterest in NanoLoanEngine does not use safeXX

    function calculateInterest(uint256 timeDelta, uint256 interestRate, uint256 amount) public returns (uint256 realDelta, uint256 interest) {
        interest = (100000 * timeDelta * amount) / interestRate;
        realDelta = (interest * interestRate) / (amount * 100000);
    }

Fixed at db868ac41d0f362bf7c85c9c561396877e71f382.

Not using visibility specifier

Some functions do not have a visibility specifier. We suggest to explicitly set visibility for every function. It is a warning in latest versions of the solidity compiler.

For example createLoan, getLoanConfig, getLoanState, etc.

Protection for unintended tokens

By mistake, a user can send tokens to a contract, and tokens will be frozen in the contract if it doesn’t have a mechanism to transfer a third party token.

We recommend implementing a similar function.

    event ClaimedTokens(address _token, address _to, uint _amount);

    function claimTokens(address _token, address _receiver, uint amount) onlyOwner {
        Token token = Token(_token);
        token.transfer(_receiver, amount);
        ClaimedTokens(_token, _receiver, amount);
    }

Enhancements

Improve documentation

The documentation is not good enough and does not follow Ethereum’s Natspec. The only documentation is in the createLoan method

	// _oracleContract: Address of the Oracle contract, must implement OracleInterface. 0x0 for no oracle
	// _cosigner: Responsable of the payment of the loan if the lender does not pay. 0x0 for no cosigner
	// _cosignerFee: absolute amount in currency
	// _interestRate: 100 000 / interest; ej 100 000 = 100 %; 10 000 000 = 1% (by second)
	function createLoan(Oracle _oracleContract, address _borrower, address _cosigner,

Add Unit Tests

We strongly recommend for the future to add unit tests. It helps to find issues earlier and prevent bugs after changes.

Notes

• Possible reentry attack in withdrawal (this is not an issue because the token in under control of the team):
  
  If the token is not under your control there’s a possibility of a reentry attack in withdrawal function. You should be a check for lenderBalance to be positive and reset the balance before making the transfer call.

    function withdrawal(uint index, address to) public returns (uint256) {
        Loan storage loan = loans[index];
        require(to != address(0));
        require(msg.sender == loan.lender);
        uint256 balance = loan.lenderBalance;
        if (balance > 0) {
            loan.lenderBalance = 0;
            require(token.transfer(to, balance));
        }
        return balance;
    }

The issue was fixed at 9816d288cd7a3362cc1a484eb7f455a7019d4b0c.

• For certain operations like lend or pay the person sending the transaction have to calculate the amount of RCN Tokens to approve. If the contract has to pay fees or use an Oracle to calculate conversion rates the amount to approve is complex to calculate.
  
  We suggest adding a constant function that calculates the number of tokens required to approve in order to complete the operation successfully.

Conclusion

The contracts are well written and well structured. The functions are simple and easy to follow. The only negative thing we found is the lack of documentation and unit tests.

References

• “Ripio Credit Network – Whitepaper”, https://ripiocredit.network/wp/RCN%20Whitepaper%20ENG.pdf
• “Ethereum Natural Specification Format”, https://github.com/ethereum/wiki/wiki/Ethereum-Natural-Specification-Format