QA Report

[code423n4]

Low

1. Use Two-Step Transfer Pattern for Access Controls

Impact

Contracts implementing access control's, e.g. owner, should consider implementing a Two-Step Transfer pattern. Otherwise it's possible that the role mistakenly transfers ownership to the wrong address, resulting in a loss of the role

Proof of Concept

Comptroller.sol::_changeAdmin()

    function _changeAdmin(address newAdmin) external {  // @audit low: address(0) check
        require(msg.sender == admin, "Only admin can change the admin");  
        admin = newAdmin;
    }

Recommendation

Consider adding a pendingOwner where the new owner will have to accept the ownership. This two-step process is already implemented in CToken.sol.

Example from CToken.sol

    /**
      * @notice Begins transfer of admin rights. The newPendingAdmin must call `_acceptAdmin` to finalize the transfer.
      * @dev Admin function to begin change of admin. The newPendingAdmin must call `_acceptAdmin` to finalize the transfer.
      * @param newPendingAdmin New pending admin.
      * @return uint 0=success, otherwise a failure (see ErrorReporter.sol for details)
      */
    function _setPendingAdmin(address payable newPendingAdmin) external returns (uint) {
        // Check caller = admin
        if (msg.sender != admin) {
            return fail(Error.UNAUTHORIZED, FailureInfo.SET_PENDING_ADMIN_OWNER_CHECK);
        }

        // Save current value, if any, for inclusion in log
        address oldPendingAdmin = pendingAdmin;

        // Store pendingAdmin with value newPendingAdmin
        pendingAdmin = newPendingAdmin;

        // Emit NewPendingAdmin(oldPendingAdmin, newPendingAdmin)
        emit NewPendingAdmin(oldPendingAdmin, newPendingAdmin);

        return uint(Error.NO_ERROR);
    }

    /**
      * @notice Accepts transfer of admin rights. msg.sender must be pendingAdmin
      * @dev Admin function for pending admin to accept role and update admin
      * @return uint 0=success, otherwise a failure (see ErrorReporter.sol for details)
      */
    function _acceptAdmin() external returns (uint) {
        // Check caller is pendingAdmin and pendingAdmin ≠ address(0)
        if (msg.sender != pendingAdmin || msg.sender == address(0)) {
            return fail(Error.UNAUTHORIZED, FailureInfo.ACCEPT_ADMIN_PENDING_ADMIN_CHECK);
        }

        // Save current values for inclusion in log
        address oldAdmin = admin;
        address oldPendingAdmin = pendingAdmin;

        // Store admin with value pendingAdmin
        admin = pendingAdmin;

        // Clear the pending value
        pendingAdmin = address(0);

        emit NewAdmin(oldAdmin, admin);
        emit NewPendingAdmin(oldPendingAdmin, pendingAdmin);

        return uint(Error.NO_ERROR);
    }

2. Unspecific Compiler Version Pragma

Impact

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

Proof of Concept

  CErc20.sol::1 => pragma solidity ^0.5.16;
  CEther.sol::1 => pragma solidity ^0.5.16;
  CNft.sol::2 => pragma solidity ^0.8.0;
  CNftPriceOracle.sol::2 => pragma solidity ^0.8.0;
  CToken.sol::1 => pragma solidity ^0.5.16;
  Comptroller.sol::1 => pragma solidity ^0.5.16;
  ERC1155Enumerable.sol::2 => pragma solidity ^0.8.0;
  PriceOracleImplementation.sol::1 => pragma solidity ^0.5.16;
  UniswapV2PriceOracle.sol::2 => pragma solidity ^0.8.0;

Recommendation

Avoid floating pragmas for non-library contracts. It is recommended to pin to a concrete compiler version.

3. Unsafe ERC20 Operations

Impact

The return value of an external transfer/transferFrom call is not checked

Proof of Concept

  CErc20.sol::138 => token.transfer(admin, balance);
  CErc20.sol::174 => token.transferFrom(from, address(this), amount);
  CErc20.sol::209 => token.transfer(to, amount);
  CEther.sol::167 => to.transfer(amount);
  Comptroller.sol::1256 => comp.transfer(user, amount);

Recommendation

Use SafeERC20, or ensure that the transfer/transferFrom return value is checked.

4. Missing checks for address(0) when assigning values to address state variables.

Impact

Setters of address type parameters should include a zero-address check otherwise contract functionality may become inaccessible or tokens burnt forever.

Proof of Concept

Comptroller.sol::_changeAdmin(), Comptroller.sol::_setBorrowCapGuardian(), Comptroller.sol::_setPauseGuardian().

Recommendation

Add address(0) checks.

5. Invalid documentation regardin upper-bounds error checks.

Impact

Across the contracts, there are several areas which refer to the constant value -1 in documentation, but, in fact, refers to uint(-1), which is just the maximum value of a uint (2^256 - 1). This could result in improper assumptions about functionality and lead to misunderstanding developer intent.

Proof of Concept

/* Fail if repayAmount = -1 */
if (repayAmount == uint(-1)) {
    return (fail(Error.INVALID_CLOSE_AMOUNT_REQUESTED, FailureInfo.LIQUIDATE_CLOSE_AMOUNT_IS_UINT_MAX), 0);
}

Recommendation

Ensure documentation is correct to avoid potential confusion.

6. Incorrect use of "infinite" in documentation.

Incorrect usage of the word "infinite". uint(-1) does not mean infinite, like it says in the comments, but it just means 2^256 - 1.

Proof of Concept

CToken.sol#L80,CToken.sol#L153, CToken.sol#L167.

Check @audit tags.

/* Get the allowance, infinite for the account owner */ // @audit technically not infinite. its 2^256 - 1.
uint startingAllowance = 0;
if (spender == src) {
    startingAllowance = uint(-1); // @audit uint(-1) = 2^256 - 1

 * @param amount The number of tokens that are approved (-1 means infinite) // @audit not infinite, uint(-1) = 2^256 - 1.

 * @return The number of tokens allowed to be spent (-1 means infinite) // @audit not infinite, uint(-1) = 2^256 - 1.

Recommendation

Fix comments.

Non-Critical

1. Natspec is incomplete

Missing @return: Comptroller.sol#L395-L402, Comptroller.sol#L442-L448, CToken.sol#L221-L224, CToken.sol#L377-L381.

Recommendation

Add @return's.

2. Declare uint as uint256.

Recommendation

To favor explicitness, all instances of uint should be declared as uint256.

Tools used

c4udit, manual, slither.