Sturdy contest - robee's results

Does not validate the input fee parameter

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

Code instance:

    GeneralVault.setTreasuryInfo (_fee)

safeApprove of openZeppelin is deprecated

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.

Code instances:

    Deprecated safeApprove in ConvexCurveLPVault.sol line 140: IERC20(curveLPToken).safeApprove(convexBooster, _amount);
    Deprecated safeApprove in ConvexCurveLPVault.sol line 145: IERC20(internalAssetToken).safeApprove(address(_addressesProvider.getLendingPool()), _amount);
    Deprecated safeApprove in LidoVault.sol line 101: IERC20(LIDO).safeApprove(address(_addressesProvider.getLendingPool()), assetAmount);
    Deprecated safeApprove in YieldManager.sol line 220: IERC20(_asset).approve(_lendingPool, _amount);

Not verified input

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

Code instances:

    GeneralVault.sol.withdrawCollateral _to
    YieldManager.sol.setCurvePool _pool
    YieldManager.sol.registerAsset _asset
    GeneralVault.sol.withdrawOnLiquidation _asset
    GeneralVault.sol.withdrawCollateral _asset

Treasury may be address(0)

Make sure the treasury is not address(0).
    
    

Code instance:

    GeneralVault.sol.setTreasuryInfo _treasury

Use safe math for solidity version <8

You should use safe math for solidity version <8 since there is no default over/under flow check it suchversions of solidity.

Code instances:

    The contract LidoVault.sol doesn't use safe math and is of solidity version < 8
    The contract CollateralAdapter.sol doesn't use safe math and is of solidity version < 8
    The contract ConvexCurveLPVault.sol doesn't use safe math and is of solidity version < 8

Init frontrun

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:

Code instances:

    GeneralVault.sol, initialize, 61
    CollateralAdapter.sol, initialize, 35
    YieldManager.sol, initialize, 60

Named return issue

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

Code instance:

    YieldManager.sol, _convertToStableCoin

Never used parameters

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

Code instances:

    ConvexCurveLPVault.sol: function _getWithdrawalAmount parameter _asset isn't used. (_getWithdrawalAmount is internal)
    GeneralVault.sol: function _withdrawFromYieldPool parameter _asset isn't used. (_withdrawFromYieldPool is internal)
    GeneralVault.sol: function _withdrawFromYieldPool parameter _amount isn't used. (_withdrawFromYieldPool is internal)
    GeneralVault.sol: function _getWithdrawalAmount parameter _asset isn't used. (_getWithdrawalAmount is internal)
    GeneralVault.sol: function _getWithdrawalAmount parameter _amount isn't used. (_getWithdrawalAmount is internal)

Open TODOs

Open TODOs can hint at programming or architectural errors that still need to be fixed. These files has open TODOs:

Code instance:

Open TODO in GeneralVault.sol line 76 : // Ex: if user deposit 100ETH, this will deposit 100ETH to Lido and receive 100stETH TODO No Lido

Check transfer receiver is not 0 to avoid burned money

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Code instances:

    https://github.com/code-423n4/2022-05-sturdy/tree/main/smart-contracts/LidoVault.sol#L146
    https://github.com/code-423n4/2022-05-sturdy/tree/main/smart-contracts/ConvexCurveLPVault.sol#L138
    https://github.com/code-423n4/2022-05-sturdy/tree/main/smart-contracts/ConvexCurveLPVault.sol#L74
    https://github.com/code-423n4/2022-05-sturdy/tree/main/smart-contracts/LidoVault.sol#L98
    https://github.com/code-423n4/2022-05-sturdy/tree/main/smart-contracts/ConvexCurveLPVault.sol#L207

Never used parameters

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

Code instances:

    ConvexCurveLPVault.sol: function _getWithdrawalAmount parameter _asset isn't used. (_getWithdrawalAmount is internal)
    GeneralVault.sol: function _withdrawFromYieldPool parameter _asset isn't used. (_withdrawFromYieldPool is internal)
    GeneralVault.sol: function _withdrawFromYieldPool parameter _amount isn't used. (_withdrawFromYieldPool is internal)
    GeneralVault.sol: function _getWithdrawalAmount parameter _asset isn't used. (_getWithdrawalAmount is internal)
    GeneralVault.sol: function _getWithdrawalAmount parameter _amount isn't used. (_getWithdrawalAmount is internal)

Open TODOs

Open TODOs can hint at programming or architectural errors that still need to be fixed. These files has open TODOs:

Code instance:

Open TODO in GeneralVault.sol line 76 : // Ex: if user deposit 100ETH, this will deposit 100ETH to Lido and receive 100stETH TODO No Lido

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instances:

    https://github.com/code-423n4/2022-05-sturdy/tree/main/smart-contracts/GeneralVault.sol#L165
    https://github.com/code-423n4/2022-05-sturdy/tree/main/smart-contracts/YieldManager.sol#L92
    https://github.com/code-423n4/2022-05-sturdy/tree/main/smart-contracts/YieldManager.sol#L64
    https://github.com/code-423n4/2022-05-sturdy/tree/main/smart-contracts/ConvexCurveLPVault.sol#L37

State variables that could be set immutable

In the following files there are state variables that could be set immutable to save gas.

Code instances:

    _addressesProvider in CollateralAdapter.sol
    _addressesProvider in GeneralVault.sol
    _addressesProvider in YieldManager.sol

Caching array length can save gas

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++) { ... }

Code instance:

    YieldManager.sol, assetYields, 130

Prefix increments are cheaper than postfix increments

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:

Code instances:

    change to prefix increment and unchecked: YieldManager.sol, i, 156
    change to prefix increment and unchecked: ConvexCurveLPVault.sol, i, 106
    change to prefix increment and unchecked: GeneralVault.sol, i, 218
    change to prefix increment and unchecked: YieldManager.sol, i, 120
    change to prefix increment and unchecked: YieldManager.sol, i, 130

Unnecessary index init

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:

Code instances:

    YieldManager.sol, 120
    ConvexCurveLPVault.sol, 106
    GeneralVault.sol, 218
    YieldManager.sol, 130
    YieldManager.sol, 156

Consider inline the following functions to save gas

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)

Code instances

    GeneralVault.sol, getRevision, { return VAULT_REVISION; }
    CollateralAdapter.sol, getRevision, { return VAULT_REVISION; }
    YieldManager.sol, getRevision, { return VAULT_REVISION; }

Inline one time use functions

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

Code instances:

    LidoVault.sol, _processTreasury
    GeneralVault.sol, _getWithdrawalAmount
    GeneralVault.sol, _depositToYieldPool
    ConvexCurveLPVault.sol, _processTreasury
    YieldManager.sol, _convertToStableCoin

Upgrade pragma to at least 0.8.4

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

The advantages of versions 0.8.* over <0.8.0 are:

    1. Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
    2. Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
    3. Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
    4. Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Code instances:

    YieldManager.sol
    GeneralVault.sol
    ConvexCurveLPVault.sol
    LidoVault.sol
    CollateralAdapter.sol