Sublime contest - robee's results

Handle

robee

Vulnerability details

You use safeApprove of openZeppelin although it's deprecated. (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38) You should change it to increase/decrease Allowance as OpenZeppilin says. This appears in the following locations in the code base:

    Deprecated safeApprove in CreditLine.sol line 646: IERC20(_collateralAsset).approve(_strategy, _amount); 
    Deprecated safeApprove in CreditLine.sol line 774: IERC20(_borrowAsset).approve(_defaultStrategy, _amount); 
    Deprecated safeApprove in Controller.sol line 195: IERC20(_token).safeApprove(onesplit, 0); 
    Deprecated safeApprove in Controller.sol line 196: IERC20(_token).safeApprove(onesplit, _amount); 
    Deprecated safeApprove in Strategy.sol line 41: // IERC20(token).approve(mcd_join_eth_a, uint256(-1)); 
    Deprecated safeApprove in SavingsAccount.sol line 171: IERC20(_token).safeApprove(_newStrategy, _tokensReceived); 
    Deprecated safeApprove in SavingsAccountUtil.sol line 60: IERC20(_token).safeApprove(_approveTo, _amount); 
    Deprecated safeApprove in AaveYield.sol line 293: IERC20(asset).approve(lendingPool, 0); 
    Deprecated safeApprove in AaveYield.sol line 294: IERC20(asset).approve(lendingPool, amount); 
    Deprecated safeApprove in AaveYield.sol line 303: IERC20(IWETHGateway(wethGateway).getAWETHAddress()).approve(wethGateway, amount); 
    Deprecated safeApprove in AaveYield.sol line 320: IERC20(aToken).approve(lendingPool, amount); 
    Deprecated safeApprove in CompoundYield.sol line 207: IERC20(asset).approve(cToken, 0); 
    Deprecated safeApprove in CompoundYield.sol line 208: IERC20(asset).approve(cToken, amount); 
    Deprecated safeApprove in YearnYield.sol line 206: IERC20(asset).approve(vault, 0); 
    Deprecated safeApprove in YearnYield.sol line 207: IERC20(asset).approve(vault, amount); 

Handle

robee

Vulnerability details

This issue is about arithmetic computation that could have been done more percise. The following are places in the codebase in which you multiplied after the divisions. Doing the multiplications at start lead to more accurate calculations. This is a list of places in the code that this appears (Solidity file, line number, actual line):

    CreditLine.sol, 450, uint256 _maxPossible = _totalCollateralToken.mul(_ratioOfPrices).div(creditLineConstants[_id].idealCollateralRatio).mul(10**30).div( 
    CreditLine.sol, 870, uint256 currentCollateralRatio = calculateTotalCollateralTokens(_id).mul(_ratioOfPrices).div(currentDebt).mul(10**30).div( 
    CreditLine.sol, 1046, _totalCollateralTokens.mul(uint256(10**30).sub(liquidatorRewardFraction)).div(10**30).mul(_ratioOfPrices).div(10**_decimals) 
    Pool.sol, 907, return _totalCollateralTokens.mul(_ratioOfPrices).div(10**_decimals).mul(uint256(10**30).sub(_fraction)).div(10**30); 

Handle

robee

Vulnerability details

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

    CreditLine.sol, initialize, 262
    Extension.sol, initialize, 59
    Pool.sol, initialize, 133
    PoolFactory.sol, initialize, 188
    Repayments.sol, initialize, 97
    PriceOracle.sol, initialize, 36
    SavingsAccount.sol, initialize, 58
    adminVerifier.sol, initialize, 28
    Verification.sol, initialize, 48
    AaveYield.sol, initialize, 84
    CompoundYield.sol, initialize, 53
    NoYield.sol, initialize, 41
    StrategyRegistry.sol, initialize, 33
    YearnYield.sol, initialize, 52

Handle

robee

Vulnerability details

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    ILendingPoolAddressesProvider.sol
    IUniswapV3Factory.sol
    Controller.sol
    Strategy.sol
    yVault.sol

Handle

robee

Vulnerability details

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

    change to prefix increment and unchecked: CreditLine.sol, _index, 484
    change to prefix increment and unchecked: CreditLine.sol, _index, 661
    change to prefix increment and unchecked: CreditLine.sol, _index, 735
    change to prefix increment and unchecked: CreditLine.sol, index, 888
    change to prefix increment and unchecked: CreditLine.sol, index, 955
    change to prefix increment and unchecked: SavingsAccount.sol, i, 286
    change to prefix increment and unchecked: SavingsAccount.sol, i, 464

Handle

robee

Vulnerability details

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    CreditLine.sol, 484
    CreditLine.sol, 661
    CreditLine.sol, 735
    CreditLine.sol, 888
    CreditLine.sol, 955
    SavingsAccount.sol, 286
    SavingsAccount.sol, 464

Handle

robee

Vulnerability details

Where there is onlyOwner or Initializer modifer, the reentrancy gaurd isn't necessary (unless you don't trust the owner or the deployer, which will lead to full security breakdown of the project and we believe this is not the case) This is a list we found of such occurrences:

    YearnYield.sol no need both nonReentrant and onlyOwner modifiers in emergencyWithdraw

Handle

robee

Vulnerability details

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory We recommend to change from: for (uint256 i=0; i<array.length; i++) { ... } to: uint len = array.length for (uint256 i=0; i<len; i++) { ... } These functions use not using prefix increments (++x) or not using the unchecked keyword:

    CreditLine.sol, _strategyList, 484
    CreditLine.sol, _strategyList, 661
    CreditLine.sol, _strategyList, 735
    CreditLine.sol, _strategyList, 888
    CreditLine.sol, _strategyList, 955
    SavingsAccount.sol, _strategyList, 286
    SavingsAccount.sol, _strategyList, 464

Handle

robee

Vulnerability details

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

    Solidity file: CreditLine.sol, In line 914, Require message length to shorten: 36, The message: Collateral ratio cant go below ideal
    Solidity file: CreditLine.sol, In line 1054, Require message length to shorten: 39, The message: CreditLine::receive invalid transaction
    Solidity file: Extension.sol, In line 71, Require message length to shorten: 40, The message: Repayments::onlyValidPool - Invalid Pool
    Solidity file: Extension.sol, In line 132, Require message length to shorten: 38, The message: Pool::voteOnExtension - Voting is over
    Solidity file: SavingsAccount.sol, In line 80, Require message length to shorten: 39, The message: SavingsAccount::initialize zero address
    Solidity file: SavingsAccount.sol, In line 396, Require message length to shorten: 36, The message: SavingsAccount::transfer zero amount
    Solidity file: SavingsAccount.sol, In line 430, Require message length to shorten: 40, The message: SavingsAccount::transferFrom zero amount
    Solidity file: AaveYield.sol, In line 146, Require message length to shorten: 34, The message: Invest: WETHGateway:: zero address
    Solidity file: AaveYield.sol, In line 241, Require message length to shorten: 34, The message: Asset address cannot be address(0)
    Solidity file: CompoundYield.sol, In line 163, Require message length to shorten: 34, The message: Asset address cannot be address(0)
    Solidity file: YearnYield.sol, In line 164, Require message length to shorten: 34, The message: Asset address cannot be address(0)

Handle

robee

Vulnerability details

There are places in the code (especially in for-each loops) that loads the same array element more than once. In such cases, only one array boundaries check should take place, and the rest are unnecessary. Therefore, this array element should be cached in a local variable and then be loaded again using this local variable, skipping the redundent second array boundaries check:

    CreditLine.sol, variable name: _strategyList times: 2 at: _withdrawBorrowAmount
    CreditLine.sol, variable name: _strategyList times: 2 at: _repayFromSavingsAccount
    CreditLine.sol, variable name: _strategyList times: 3 at: _transferCollateral
    SavingsAccount.sol, variable name: _strategyList times: 2 at: withdrawAll