bunker.finance contest - ellahi's results

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.

Gas

1. Cache Array Length Outside of Loop.

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

  CEther.sol::175 => bytes memory fullMessage = new bytes(bytes(message).length + 5);
  CEther.sol::178 => for (i = 0; i < bytes(message).length; i++) {
  CNft.sol::176 => for (uint256 i; i < vars.length; ++i) {
  CNftPriceOracle.sol::66 => for (uint256 i = 0; i < cNfts.length; ++i) {
  Comptroller.sol::591 => for (uint i = 0; i < assets.length; i++) {
  Comptroller.sol::928 => for (uint i = 0; i < allMarkets.length; i ++) {
  Comptroller.sol::1223 => for (uint i = 0; i < cTokens.length; i++) {
  Comptroller.sol::1229 => for (uint j = 0; j < holders.length; j++) {
  Comptroller.sol::1235 => for (uint j = 0; j < holders.length; j++) {
  Comptroller.sol::1240 => for (uint j = 0; j < holders.length; j++) {
  ERC1155Enumerable.sol::51 => for (uint256 i; i < ids.length; ++i) {
  UniswapV2PriceOracle.sol::42 => for (uint256 i = 0; i < pairs.length; ++i) {

Recommendation

Store the array’s length in a variable before the for-loop.

2. Use != 0 instead of > 0 for Unsigned Integer Comparison.

Impact

!= 0 is cheapear than > 0 when comparing unsigned integers in require statements.

Proof of Concept

  CNftPriceOracle.sol::63 => cNfts.length > 0 && cNfts.length == nftxTokens.length,
  CToken.sol::37 => require(initialExchangeRateMantissa > 0, "initial exchange rate must be greater than zero.");
  CToken.sol::619 => /* If redeemTokensIn > 0: */
  CToken.sol::620 => if (redeemTokensIn > 0) {
  Comptroller.sol::282 => if (shortfall > 0) {
  Comptroller.sol::302 => if (redeemTokens == 0 && redeemAmount > 0) {
  Comptroller.sol::353 => if (shortfall > 0) {
  Comptroller.sol::631 => if (nftBalance > 0) {
  Comptroller.sol::1097 => if (deltaBlocks > 0 && supplySpeed > 0) {
  Comptroller.sol::1100 => Double memory ratio = supplyTokens > 0 ? fraction(compAccrued, supplyTokens) : Double({mantissa: 0});
  Comptroller.sol::1106 => } else if (deltaBlocks > 0) {
  Comptroller.sol::1120 => if (deltaBlocks > 0 && borrowSpeed > 0) {
  Comptroller.sol::1123 => Double memory ratio = borrowAmount > 0 ? fraction(compAccrued, borrowAmount) : Double({mantissa: 0});
  Comptroller.sol::1129 => } else if (deltaBlocks > 0) {
  Comptroller.sol::1145 => if (supplierIndex.mantissa == 0 && supplyIndex.mantissa > 0) {
  Comptroller.sol::1169 => if (borrowerIndex.mantissa > 0) {
  Comptroller.sol::1187 => if (deltaBlocks > 0 && compSpeed > 0) {
  Comptroller.sol::1255 => if (amount > 0 && amount <= compRemaining) {
  UniswapV2PriceOracle.sol::26 => numPairObservations[pair] > 0 &&
  UniswapV2PriceOracle.sol::67 => reserve0 > 0 && reserve1 > 0,
  UniswapV2PriceOracle.sol::91 => reserve0 > 0,
  UniswapV2PriceOracle.sol::115 => reserve1 > 0,
  UniswapV2PriceOracle.sol::128 => require(length > 0, "UniswapV2PriceOracle: No observations.");
  UniswapV2PriceOracle.sol::149 => require(length > 0, "UniswapV2PriceOracle: No observations.");

Recommendation

Use != 0 instead of > 0.

3. Reduce the size of error messages (Long revert Strings).

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of Concept

  CErc20.sol::136 => require(address(token) != underlying, "CErc20::sweepToken: can not sweep underlying token");
  CErc20.sol::234 => require(msg.sender == admin, "only the admin may set the comp-like delegate");
  CNft.sol::24 => require(_underlying != address(0), "CNFT: Asset should not be address(0)");
  CNft.sol::25 => require(ComptrollerInterface(_comptroller).isComptroller(), "_comptroller is not a Comptroller contract");
  CNft.sol::52 => require(amounts[i] == 1, "CNFT: Amounts must be all 1s for non-ERC1155s.");
  CNft.sol::69 => require(buyPunkSuccess, "CNFT: Calling buyPunk was unsuccessful");
  CNft.sol::93 => require(borrower != liquidator, "CNFT: Liquidator cannot be borrower");
  CNft.sol::100 => require(seizeAmounts[i] == 1, "CNFT: Amounts must be all 1s for non-ERC1155s.");
  CNft.sol::124 => require(amounts[i] == 1, "CNFT: Amounts must be all 1s for non-ERC1155s.");
  CNft.sol::148 => require(transferPunkSuccess, "CNFT: Calling transferPunk was unsuccessful");
  CNft.sol::208 => require(msg.sender == underlying, "CNFT: This contract can only receive the underlying NFT");
  CNft.sol::209 => require(operator == address(this), "CNFT: Only the CNFT contract can be the operator");
  CNft.sol::279 => require(to != underlying, "CNFT: Cannot make an arbitrary call to underlying NFT");
  CNftPriceOracle.sol::64 => "CNftPriceOracle: `cNfts` and `nftxTokens` must have nonzero, equal lengths."
  CNftPriceOracle.sol::70 => "CNftPriceOracle: Cannot overwrite existing address mappings."
  CNftPriceOracle.sol::90 => "CNftPriceOracle: No NFTX token for cNFT."
  CToken.sol::32 => require(msg.sender == admin, "only admin may initialize the market");
  CToken.sol::33 => require(accrualBlockNumber == 0 && borrowIndex == 0, "market may only be initialized once");
  CToken.sol::37 => require(initialExchangeRateMantissa > 0, "initial exchange rate must be greater than zero.");
  CToken.sol::49 => require(err == uint(Error.NO_ERROR), "setting interest rate model failed");
  CToken.sol::271 => require(err == MathError.NO_ERROR, "borrowBalanceStored: borrowBalanceStoredInternal failed");
  CToken.sol::328 => require(err == MathError.NO_ERROR, "exchangeRateStored: exchangeRateStoredInternal failed");
  CToken.sol::542 => require(vars.mathErr == MathError.NO_ERROR, "MINT_NEW_TOTAL_SUPPLY_CALCULATION_FAILED");
  CToken.sol::545 => require(vars.mathErr == MathError.NO_ERROR, "MINT_NEW_ACCOUNT_BALANCE_CALCULATION_FAILED");
  CToken.sol::609 => require(redeemTokensIn == 0 || redeemAmountIn == 0, "one of redeemTokensIn or redeemAmountIn must be zero");
  CToken.sol::891 => require(vars.mathErr == MathError.NO_ERROR, "REPAY_BORROW_NEW_ACCOUNT_BORROW_BALANCE_CALCULATION_FAILED");
  CToken.sol::894 => require(vars.mathErr == MathError.NO_ERROR, "REPAY_BORROW_NEW_TOTAL_BALANCE_CALCULATION_FAILED");
  CToken.sol::986 => require(amountSeizeError == uint(Error.NO_ERROR), "LIQUIDATE_COMPTROLLER_CALCULATE_AMOUNT_SEIZE_FAILED");
  CToken.sol::1083 => require(amountSeizeError == uint(Error.NO_ERROR), "LIQUIDATE_COMPTROLLER_CALCULATE_AMOUNT_SEIZE_FAILED");
  CToken.sol::1093 => require(seizeTokens == 0, "LIQUIDATE_SEIZE_INCORRECT_NUM_NFTS");
  CToken.sol::1433 => require(totalReservesNew <= totalReserves, "reduce reserves unexpected underflow");
  Comptroller.sol::171 => require(oErr == 0, "exitMarket: getAccountSnapshot failed"); // semi-opaque error code
  Comptroller.sol::420 => require(borrowBalance >= repayAmount, "Can not repay more than the total borrow");
  Comptroller.sol::702 => require(cNftCollateral == address(nftMarket), "cNFT is from the wrong comptroller");
  Comptroller.sol::942 => require(msg.sender == admin || msg.sender == borrowCapGuardian, "only admin or borrow cap guardian can set borrow caps");
  Comptroller.sol::960 => require(msg.sender == admin, "only admin can set borrow cap guardian");
  Comptroller.sol::995 => require(markets[cAsset].isListed, "cannot pause a market that is not listed");
  Comptroller.sol::996 => require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause");
  Comptroller.sol::1009 => require(markets[address(cToken)].isListed, "cannot pause a market that is not listed");
  Comptroller.sol::1010 => require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause");
  Comptroller.sol::1019 => require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause");
  Comptroller.sol::1028 => require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause");
  Comptroller.sol::1037 => require(msg.sender == unitroller.admin(), "only unitroller admin can change brains");
  Comptroller.sol::1338 => require(address(nftMarket) == address(0), "nft collateral already initialized");
  Comptroller.sol::1339 => require(address(cNft) != address(0), "cannot initialize nft market to the 0 address");
  UniswapV2PriceOracle.sol::68 => "UniswapV2PriceOracle: Division by zero."
  UniswapV2PriceOracle.sol::92 => "UniswapV2PriceOracle: Division by zero."
  UniswapV2PriceOracle.sol::116 => "UniswapV2PriceOracle: Division by zero."
  UniswapV2PriceOracle.sol::128 => require(length > 0, "UniswapV2PriceOracle: No observations.");
  UniswapV2PriceOracle.sol::131 => require(length > 1, "UniswapV2PriceOracle: Only one observation.");
  UniswapV2PriceOracle.sol::137 => "UniswapV2PriceOracle: Bad TWAP time."
  UniswapV2PriceOracle.sol::149 => require(length > 0, "UniswapV2PriceOracle: No observations.");
  UniswapV2PriceOracle.sol::152 => require(length > 1, "UniswapV2PriceOracle: Only one observation.");
  UniswapV2PriceOracle.sol::158 => "UniswapV2PriceOracle: Bad TWAP time."

Recommendation

Shorten the revert strings to fit in 32 bytes.

4. public Functions can be external.

Impact

public functions that are never called by the contract should be declared external to save gas.

Proof of Concept

_setInterestRateModel(InterestRateModel) should be declared external:
	- CToken._setInterestRateModel(InterestRateModel) (contracts/CToken.sol#1452-1460)
enterMarkets(address[]) should be declared external:
	- Comptroller.enterMarkets(address[]) (contracts/Comptroller.sol#115-126)
getAccountLiquidity(address) should be declared external:
	- Comptroller.getAccountLiquidity(address) (contracts/Comptroller.sol#533-537)
getHypotheticalAccountLiquidity(address,address,uint256,uint256) should be declared external:
	- Comptroller.getHypotheticalAccountLiquidity(address,address,uint256,uint256) (contracts/Comptroller.sol#559-566)
_setPriceOracle(PriceOracle) should be declared external:
	- Comptroller._setPriceOracle(PriceOracle) (contracts/Comptroller.sol#741-757)
_setNftPriceOracle(NftPriceOracle) should be declared external:
	- Comptroller._setNftPriceOracle(NftPriceOracle) (contracts/Comptroller.sol#764-774)
_setPauseGuardian(address) should be declared external:
	- Comptroller._setPauseGuardian(address) (contracts/Comptroller.sol#977-992)
_setMintPaused(address,bool) should be declared external:
	- Comptroller._setMintPaused(address,bool) (contracts/Comptroller.sol#994-1006)
_setBorrowPaused(CToken,bool) should be declared external:
	- Comptroller._setBorrowPaused(CToken,bool) (contracts/Comptroller.sol#1008-1016)
_setTransferPaused(bool) should be declared external:
	- Comptroller._setTransferPaused(bool) (contracts/Comptroller.sol#1018-1025)
_setSeizePaused(bool) should be declared external:
	- Comptroller._setSeizePaused(bool) (contracts/Comptroller.sol#1027-1034)
_become(Unitroller) should be declared external:
	- Comptroller._become(Unitroller) (contracts/Comptroller.sol#1036-1039)
claimComp(address) should be declared external:
	- Comptroller.claimComp(address) (contracts/Comptroller.sol#1200-1202)
_grantComp(address,uint256) should be declared external:
	- Comptroller._grantComp(address,uint256) (contracts/Comptroller.sol#1270-1275)
_setCompSpeed(CToken,uint256) should be declared external:
	- Comptroller._setCompSpeed(CToken,uint256) (contracts/Comptroller.sol#1282-1285)
_setContributorCompSpeed(address,uint256) should be declared external:
	- Comptroller._setContributorCompSpeed(address,uint256) (contracts/Comptroller.sol#1292-1306)
getAllMarkets() should be declared external:
	- Comptroller.getAllMarkets() (contracts/Comptroller.sol#1313-1315)
safeTransferFrom(address,address,uint256,uint256,bytes) should be declared external:
	- CNft.safeTransferFrom(address,address,uint256,uint256,bytes) (contracts/CNft.sol#190-205)
initialize(string,address,bool,bool,address) should be declared external:
	- CNft.initialize(string,address,bool,bool,address) (contracts/CNft.sol#17-33)
onERC721Received(address,address,uint256,bytes) should be declared external:
	- CNft.onERC721Received(address,address,uint256,bytes) (contracts/CNft.sol#213-220)
onERC1155Received(address,address,uint256,uint256,bytes) should be declared external:
	- CNft.onERC1155Received(address,address,uint256,uint256,bytes) (contracts/CNft.sol#222-230)
onERC1155BatchReceived(address,address,uint256[],uint256[],bytes) should be declared external:
	- CNft.onERC1155BatchReceived(address,address,uint256[],uint256[],bytes) (contracts/CNft.sol#232-240)

Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-external

Recommendation

Declare visibility of functions above as external.

5. No need to initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

  CNft.sol::49 => uint256 totalAmount = 0;
  CNft.sol::97 => uint256 totalAmount = 0;
  CNft.sol::119 => uint256 totalAmount = 0;
  CNftPriceOracle.sol::66 => for (uint256 i = 0; i < cNfts.length; ++i) {
  CToken.sol::81 => uint startingAllowance = 0;
  Comptroller.sol::119 => for (uint i = 0; i < len; i++) {
  Comptroller.sol::199 => for (uint i = 0; i < len; i++) {
  Comptroller.sol::591 => for (uint i = 0; i < assets.length; i++) {
  Comptroller.sol::928 => for (uint i = 0; i < allMarkets.length; i ++) {
  Comptroller.sol::949 => for(uint i = 0; i < numMarkets; i++) {
  Comptroller.sol::1223 => for (uint i = 0; i < cTokens.length; i++) {
  Comptroller.sol::1229 => for (uint j = 0; j < holders.length; j++) {
  Comptroller.sol::1235 => for (uint j = 0; j < holders.length; j++) {
  Comptroller.sol::1240 => for (uint j = 0; j < holders.length; j++) {
  UniswapV2PriceOracle.sol::41 => uint256 numberUpdated = 0;
  UniswapV2PriceOracle.sol::42 => for (uint256 i = 0; i < pairs.length; ++i) {

Recommendation

Remove explicit zero initialization.

6. ++i costs less gas compared to i++ or i += 1

Impact

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

Proof of Concept

  Comptroller.sol::119 => for (uint i = 0; i < len; i++) {
  Comptroller.sol::199 => for (uint i = 0; i < len; i++) {
  Comptroller.sol::591 => for (uint i = 0; i < assets.length; i++) {
  Comptroller.sol::928 => for (uint i = 0; i < allMarkets.length; i ++) {
  Comptroller.sol::949 => for(uint i = 0; i < numMarkets; i++) {
  Comptroller.sol::1223 => for (uint i = 0; i < cTokens.length; i++) {
  Comptroller.sol::1229 => for (uint j = 0; j < holders.length; j++) {
  Comptroller.sol::1235 => for (uint j = 0; j < holders.length; j++) {
  Comptroller.sol::1240 => for (uint j = 0; j < holders.length; j++) {
  CEther.sol::178 => for (i = 0; i < bytes(message).length; i++) {

Recommendation

Use ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--.