QA Report

[code423n4]

QA Report (Low Risk & Non-Critical)

• The whole project (including contest scope contracts) is built on 33 contracts, 23 libraries, 36 interfaces which have different solidity compiler ranges (0.4.0 to 0.7.6) referenced. This leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior.

• At IUniV3LpVault.sol#L105 Team can consider to remove the message // do we want a "decreaseLiquidityAndCollect" function? to avoid calling authorization / avoidShortfall funcs twice in multicall

• At Comptroller.sol constructor function, there is no zero-address validation on the admin parameter. This may accidentally set address to zero-address. Any methods using onlyAdmin modifier would be affected.

    constructor (address _admin) public {
        admin = _admin;
        _notEntered = true;
        _notEnteredInitialized = true;    
    }

• Floating Pragma used in Cerc20.sol, CErc20Immutable.sol, Comptroller.sol, CToken.sol, FlashLoan.sol, MasterPriceOracle.sol, UniV3LpVault.sol. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma (i.e. by not using ^) helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. Reference

• At Comptroller.sol#L418the require logic should not be less than "<", it should be "<=", else the borrowCap will never be reached. Recomendation as follows;

  require(nextTotalBorrows <= borrowCap, "market borrow cap reached");

  Reference

• Many methods are based on the value addressed from ErrorReporter.sol of which may the Struct values change and break the flow. So,the checks should not be upon conditions which are out of scope.

• The team may consider to remove self notes as // (No safe failures beyond this point)

• If the contracts will be refactored to at least 0.8.0, the team can consider to implicitly state the infinite allowance by using type(uint256).max instead of uint256(-1)

• In CErc20.sol the transfer function is called inside sweepToken method. However, this function has 2300 gas stipend and might fail if this was called to a smart contract due to it's receive/fallback complexity. The team can consider using the low level call method instead.

• The scoped contracts use approve() method which is prone to attack vectors as described here A potential fix includes preventing a call to approve if all the previous tokens are not spent through adding a check that the allowed balance is 0: require(allowed[msg.sender][_spender] == 0)

• At Comptroller.sol#L292 the require statement logic should be "<=" instead of "<" to reach the supplyCap

  require(nextTotalUnderlyingSupply <= supplyCap, "market supply cap reached");