Sturdy contest - IllIllI's results

Lines of code

https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/LidoVault.sol#L139-L142

Vulnerability details

Impact

Users will lose all of their collateral if there is an issue with the transfer

Proof of Concept

_withdrawFromYieldPool() which is called by GeneralVault.withdrawCollateral() checks for the failure of the low level call after the function returns.

File: smart-contracts/LidoVault.sol   #1

139         // send ETH to user
140         (bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');
141         return receivedETHAmount;
142         require(sent, Errors.VT_COLLATERAL_WITHDRAW_INVALID);

https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/LidoVault.sol#L139-L142

Code that occurs after a return statement is code that is never executed, so if the low level call fails, the transaction will not revert, and the user's funds will remain locked in the LidoVault

Tools Used

Code inspection

Recommended Mitigation Steps

Move the require() to before the return statement

Lines of code

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L105-L110 https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L129-L136

Vulnerability details

Impact

Yields will not be able to be distributed to lenders because attempts to do so will revert

Proof of Concept

The processYield() function loops overall of the extra rewards and transfers them

File: smart-contracts/ConvexCurveLPVault.sol   #1

105       uint256 extraRewardsLength = IConvexBaseRewardPool(baseRewardPool).extraRewardsLength();
106       for (uint256 i = 0; i < extraRewardsLength; i++) {
107         address _extraReward = IConvexBaseRewardPool(baseRewardPool).extraRewards(i);
108         address _rewardToken = IRewards(_extraReward).rewardToken();
109         _transferYield(_rewardToken);
110       }

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L105-L110

There is no guarantee that the tokens involved will be efficient in their use of gas, and there are no upper bounds on the number of extra rewards:

    function extraRewardsLength() external view returns (uint256) {
        return extraRewards.length;
    }


    function addExtraReward(address _reward) external returns(bool){
        require(msg.sender == rewardManager, "!authorized");
        require(_reward != address(0),"!reward setting");


        extraRewards.push(_reward);
        return true;
    }

https://github.com/convex-eth/platform/blob/main/contracts/contracts/BaseRewardPool.sol#L105-L115

Even if not every extra reward token has a balance, an attacker can sprinkle each one with dust, forcing a transfer by this function

_getAssetYields() has a similar issue:

File: smart-contracts/YieldManager.sol   #X

129       AssetYield[] memory assetYields = _getAssetYields(exchangedAmount);
130       for (uint256 i = 0; i < assetYields.length; i++) {
131         if (assetYields[i].amount > 0) {
132           uint256 _amount = _convertToStableCoin(assetYields[i].asset, assetYields[i].amount);
133           // 3. deposit Yield to pool for suppliers
134           _depositYield(assetYields[i].asset, _amount);
135         }
136       }

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L129-L136

Tools Used

Code inspection

Recommended Mitigation Steps

Include an offset and length as is done in YieldManager.distributeYield()

Summary

Low Risk Issues

Total: 12 instances over 10 issues

Non-critical Issues

Total: 28 instances over 13 issues

Low Risk Issues

1. Slippage of 1% is too strict

The GeneralVault assumes that st tokens are always 1:1 redeemable and therefore the slippage should be negligable. This currently is not the case, specifically for LIDO where the depegging is >4%: https://twitter.com/LidoFinance/status/1525129266689622016 I've marked this as 'Low' rather than 'Medium' because funds can still be withdrawn as LIDO directly, and converted at a later point

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

117      // Withdraw from vault, it will convert stAsset to asset and send to user
118      // Ex: In Lido vault, it will return ETH or stETH to user
119      uint256 withdrawAmount = _withdrawFromYieldPool(_asset, _amountToWithdraw, _to);
120  
121      if (_amount == type(uint256).max) {
122        uint256 decimal = IERC20Detailed(_asset).decimals();
123        _amount = _amountToWithdraw.mul(this.pricePerShare()).div(10**decimal);
124      }
125:     require(withdrawAmount >= _amount.percentMul(99_00), Errors.VT_WITHDRAW_AMOUNT_MISMATCH);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L117-L125

2. Mistaken null values cause distributeYield() to revert

There are no null checks in the registerAsset() function, so admins can mistakenly pass 0x0 to that function, which will cause the for-loop to revert when that asset is reached. I've marked this as 'Low' rather than 'Medium' since the admin can work around it by using the _offset and _count input arguments to the function.

There is 1 instance of this issue:

File: smart-contracts/YieldManager.sol   #1

118    function distributeYield(uint256 _offset, uint256 _count) external onlyAdmin {
119      // 1. convert from asset to exchange token via uniswap
120      for (uint256 i = 0; i < _count; i++) {
121        address asset = _assetsList[_offset + i];
122:       require(asset != address(0), Errors.UL_INVALID_INDEX);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L118-L122

3. Can't remove old assets

There is no way to remove old assets added by calls to registerAsset(). A disgruntled admin, before their access is revoked, can add a lot of assets and regularly sprinkle them with dust, so the new admins have to submit multiple calls to distributeYield() with different offsets and counts, to avoid the dust and possible reversion due to running out of gas

There is 1 instance of this issue:

File: smart-contracts/YieldManager.sol   #1

118    function distributeYield(uint256 _offset, uint256 _count) external onlyAdmin {
119      // 1. convert from asset to exchange token via uniswap
120      for (uint256 i = 0; i < _count; i++) {
121        address asset = _assetsList[_offset + i];
122        require(asset != address(0), Errors.UL_INVALID_INDEX);
123        uint256 _amount = IERC20Detailed(asset).balanceOf(address(this));
124        _convertAssetToExchangeToken(asset, _amount);
125:     }

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L118-L125

4. Missing checks for approve()'s return status

Some tokens, such as Tether (USDT) return false rather than reverting if the approval fails. Use OpenZeppelin's safeApprove(), which reverts if there's a failure, instead

There is 1 instance of this issue:

File: smart-contracts/YieldManager.sol   #1

221:     IERC20(_asset).approve(_lendingPool, _amount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L221

5. Move ETH constant to child contract

All of the functions in the GeneralVault require 0x0 when referring to Ether, not the constant here. Having it here will lead to mistakes down the line. It's only used by CurveswapAdapter, so it only needs to be there (it currently is also defined there).

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

47:    address constant ETH = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L47

6. Unsafe casts and usage of IERC20Detailed

The GeneralVault is meant to be general, i.e. not specific to Lido or Curve, and therefore should not assume that the asset will always be IERC20Detailed (not all ERC20 contracts define decimals() since it's optional in the spec). Use safeDecimals() instead

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

122:       uint256 decimal = IERC20Detailed(_asset).decimals();

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L122

7. Unused receive() function will lock Ether in contract

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

There is 1 instance of this issue:

File: smart-contracts/LidoVault.sol   #1

24:     receive() external payable {}

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L24

8. safeApprove() is deprecated

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead

There are 3 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol   #1

141:      IERC20(curveLPToken).safeApprove(convexBooster, _amount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L141

File: smart-contracts/ConvexCurveLPVault.sol   #2

146:      IERC20(internalAssetToken).safeApprove(address(_addressesProvider.getLendingPool()), _amount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L146

File: smart-contracts/LidoVault.sol   #3

102:      IERC20(LIDO).safeApprove(address(_addressesProvider.getLendingPool()), assetAmount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L102

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

There is 1 instance of this issue:

File: smart-contracts/ConvexCurveLPVault.sol   #1

41:       curveLPToken = _lpToken;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L41

10. Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

77:       // Ex: if user deposit 100ETH, this will deposit 100ETH to Lido and receive 100stETH TODO No Lido

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L77

Non-critical Issues

1. override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings

There are 2 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol   #1

154:    function _getWithdrawalAmount(address _asset, uint256 _amount)

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L154

File: smart-contracts/LidoVault.sol   #2

109:    function _getWithdrawalAmount(address _asset, uint256 _amount)

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L109

2. public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

There are 3 instances of this issue:

File: smart-contracts/CollateralAdapter.sol   #1

35:     function initialize(ILendingPoolAddressesProvider _provider) public initializer {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/CollateralAdapter.sol#L35

File: smart-contracts/GeneralVault.sol   #2

61:     function initialize(ILendingPoolAddressesProvider _provider) public initializer {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L61

File: smart-contracts/YieldManager.sol   #3

60:     function initialize(ILendingPoolAddressesProvider _provider) public initializer {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L60

3. constants should be defined rather than using magic numbers

There are 6 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol

/// @audit 0xF403C135812408BFbE8713b5A23a04b3D48AAE31
40:       convexBooster = 0xF403C135812408BFbE8713b5A23a04b3D48AAE31;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L40

File: smart-contracts/GeneralVault.sol

/// @audit 99_00
125:      require(withdrawAmount >= _amount.percentMul(99_00), Errors.VT_WITHDRAW_AMOUNT_MISMATCH);

/// @audit 30_00
167:      require(_fee <= 30_00, Errors.VT_FEE_TOO_BIG);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L125

File: smart-contracts/LidoVault.sol

/// @audit 200
48:         200

/// @audit 1e18
73:       return 1e18;

/// @audit 200
136:          200

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L48

4. Redundant cast

The type of the variable is the same as the type to which the variable is being cast

There is 1 instance of this issue:

File: smart-contracts/LidoVault.sol   #1

140:        (bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L140

5. Missing event for critical parameter change

There are 2 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol   #1

37      function setConfiguration(address _lpToken, uint256 _poolId) external onlyAdmin {
38        require(internalAssetToken == address(0), Errors.VT_INVALID_CONFIGURATION);
39    
40        convexBooster = 0xF403C135812408BFbE8713b5A23a04b3D48AAE31;
41        curveLPToken = _lpToken;
42        convexPoolId = _poolId;
43        SturdyInternalAsset _interalToken = new SturdyInternalAsset(
44          string(abi.encodePacked('Sturdy ', IERC20Detailed(_lpToken).symbol())),
45          string(abi.encodePacked('c', IERC20Detailed(_lpToken).symbol())),
46          IERC20Detailed(_lpToken).decimals()
47        );
48        internalAssetToken = address(_interalToken);
49:     }

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L37-L49

File: smart-contracts/YieldManager.sol   #2

64      function setExchangeToken(address _token) external onlyAdmin {
65        require(_token != address(0), Errors.VT_INVALID_CONFIGURATION);
66        _exchangeToken = _token;
67:     }

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L64-L67

6. Use a more recent version of solidity

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

There are 3 instances of this issue:

File: smart-contracts/GeneralVault.sol   #1

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L2

File: smart-contracts/LidoVault.sol   #2

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L2

File: smart-contracts/YieldManager.sol   #3

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L2

7. Use a more recent version of solidity

Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>) Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)

There is 1 instance of this issue:

File: smart-contracts/ConvexCurveLPVault.sol   #1

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L2

8. Variable names that consist of all capital letters should be reserved for const/immutable variables

If the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).

There is 1 instance of this issue:

File: smart-contracts/LidoVault.sol   #1

127:      address LIDO = _addressesProvider.getAddress('LIDO');

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L127

9. NatSpec is incomplete

There are 2 instances of this issue:

File: smart-contracts/GeneralVault.sol   #1

/// @audit Missing: '@return'
134      * @param _amount The amount to be withdrawn
135      */
136     function withdrawOnLiquidation(address _asset, uint256 _amount)
137       external
138       virtual
139:      returns (uint256)

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L134-L139

File: smart-contracts/YieldManager.sol   #2

/// @audit Missing: '@return'
104      * @param _tokenOut The address of token being received
105      */
106:    function getCurvePool(address _tokenIn, address _tokenOut) external view returns (address) {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L104-L106

10. Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

There are 4 instances of this issue:

File: smart-contracts/GeneralVault.sol   #1

24:     event ProcessYield(address indexed collateralAsset, uint256 yieldAmount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L24

File: smart-contracts/GeneralVault.sol   #2

25:     event DepositCollateral(address indexed collateralAsset, address indexed from, uint256 amount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L25

File: smart-contracts/GeneralVault.sol   #3

26:     event WithdrawCollateral(address indexed collateralAsset, address indexed to, uint256 amount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L26

File: smart-contracts/GeneralVault.sol   #4

27:     event SetTreasuryInfo(address indexed treasuryAddress, uint256 fee);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L27

11. Consider allowing the passing of a referral code

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

81       ILendingPool(_addressesProvider.getLendingPool()).deposit(
82         _stAsset,
83         _stAssetAmount,
84         msg.sender,
85         0
86:      );

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L81-L86

12. Remove commented out code

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

144    // /**
145    //  * @dev Convert an `amount` of asset used as collateral to swappable asset on liquidation.
146    //  * @param _amountIn The amount of collateral asset
147    //  */
148:   // function convertOnLiquidation(address _assetOut, uint256 _amountIn) external virtual {}

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L144-L148

13. Consider two-phase ownership transfer

Consider adding a two-phase transfer, where the current owner nominates the next owner, and the next owner has to call accept*() to become the new owner. This prevents passing the ownership to an account that is unable to use it.

There is 1 instance of this issue:

File: smart-contracts/YieldManager.sol   #1

26:  contract YieldManager is VersionedInitializable, Ownable {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L26

Summary

Gas Optimizations

Total: 61 instances over 17 issues

Gas Optimizations

1. Add require() for asset address checks before doing the exchange

The code below should verify that the address is either 0x0 or the LIDO address, in order to prevent wasting gas by doing all of the operations between this point and the actual check done in _depositToYieldPool()

There is 1 instance of this issue:

File: smart-contracts/LidoVault.sol   #1

109    function _getWithdrawalAmount(address _asset, uint256 _amount)
110      internal
111      view
112      override
113      returns (address, uint256)
114    {
115      // In this vault, return same amount of asset.
116      return (_addressesProvider.getAddress('LIDO'), _amount);
117:   }

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L109-L117

2. Add an unregisterAsset() function

By unregistering and setting a mapping field to 0, you'll be getting a Gsreset gas refund (2900 gas). If most registerAsset() calls are paired with unregisterAsset() calls, transactions will be cheaper

There is 1 instance of this issue:

File: smart-contracts/YieldManager.sol   #1

73     function registerAsset(address _asset) external onlyAdmin {
74       _assetsList[_assetsCount] = _asset;
75       _assetsCount = _assetsCount + 1;
76:    }

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L73-L76

3. Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot

There is 1 instance of this issue:

File: smart-contracts/CollateralAdapter.sol   #1

27      mapping(address => address) internal _assetToVaults;
28      // External collateral asset -> internal collateral asset
29:     mapping(address => address) internal _collateralAssets;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/CollateralAdapter.sol#L27-L29

4. State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read. Less obvious fixes/optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

There are 7 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol

/// @audit convexBooster
142:      IConvexBooster(convexBooster).deposit(convexPoolId, _amount, true);

/// @audit curveLPToken
138:      TransferHelper.safeTransferFrom(curveLPToken, msg.sender, address(this), _amount);

/// @audit curveLPToken
141:      IERC20(curveLPToken).safeApprove(convexBooster, _amount);

/// @audit internalAssetToken
146:      IERC20(internalAssetToken).safeApprove(address(_addressesProvider.getLendingPool()), _amount);

/// @audit internalAssetToken
148:      return (internalAssetToken, _amount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L142

File: smart-contracts/YieldManager.sol

/// @audit _exchangeToken
202:      address _pool = _curvePools[_exchangeToken][_tokenOut];

/// @audit _exchangeToken
207:        _exchangeToken,

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L202

5. internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

There are 6 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol

205:    function _processTreasury(address _asset, uint256 _yieldAmount) internal returns (uint256) {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L205

File: smart-contracts/LidoVault.sol

154:    function _processTreasury(uint256 _yieldAmount) internal returns (uint256) {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L154

File: smart-contracts/YieldManager.sol

142:    function _getAssetYields(uint256 _totalYieldAmount) internal view returns (AssetYield[] memory) {

178:    function _convertAssetToExchangeToken(address asset, uint256 amount) internal {

195     function _convertToStableCoin(address _tokenOut, uint256 _amount)
196       internal
197:      returns (uint256 receivedAmount)

219:    function _depositYield(address _asset, uint256 _amount) internal {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L142

6. <array>.length should not be looked up in every loop of a for-loop

The overheads outlined below are PER LOOP, excluding the first loop

• storage arrays incur a Gwarmaccess (100 gas)
• memory arrays use MLOAD (3 gas)
• calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

There is 1 instance of this issue:

File: smart-contracts/YieldManager.sol   #1

130:      for (uint256 i = 0; i < assetYields.length; i++) {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L130

7. Not using the named return variables when a function returns, wastes deployment gas

There is 1 instance of this issue:

File: smart-contracts/YieldManager.sol   #1

200:        return _amount;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L200

8. Use a more recent version of solidity

Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath Use a solidity version of at least 0.8.2 to get compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

There are 5 instances of this issue:

File: smart-contracts/CollateralAdapter.sol

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/CollateralAdapter.sol#L2

File: smart-contracts/ConvexCurveLPVault.sol

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L2

File: smart-contracts/GeneralVault.sol

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L2

File: smart-contracts/LidoVault.sol

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L2

File: smart-contracts/YieldManager.sol

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L2

9. Using > 0 costs more gas than != 0 when used on a uint in a require() statement

This change saves 6 gas per instance

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

179:      require(yieldStAsset > 0, Errors.VT_PROCESS_YIELD_INVALID);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L179

10. It costs more gas to initialize variables to zero than to let the default of zero be applied

There are 5 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol

106:      for (uint256 i = 0; i < extraRewardsLength; i++) {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L106

File: smart-contracts/GeneralVault.sol

218:      for (uint256 i = 0; i < length; i++) {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L218

File: smart-contracts/YieldManager.sol

120:      for (uint256 i = 0; i < _count; i++) {

130:      for (uint256 i = 0; i < assetYields.length; i++) {

156:      for (uint256 i = 0; i < length; i++) {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L120

11. internal functions not called by the contract should be removed to save deployment gas

If the functions are required by an interface, the contract should inherit from that interface and use the override keyword

There are 2 instances of this issue:

File: smart-contracts/GeneralVault.sol   #1

204:    function _getAssetYields(uint256 _WETHAmount) internal view returns (AssetYield[] memory) {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L204

File: smart-contracts/GeneralVault.sol   #2

235:    function _depositYield(address _asset, uint256 _amount) internal {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L235

12. ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 6 gas PER LOOP

There are 5 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol

106:      for (uint256 i = 0; i < extraRewardsLength; i++) {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L106

File: smart-contracts/GeneralVault.sol

218:      for (uint256 i = 0; i < length; i++) {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L218

File: smart-contracts/YieldManager.sol

120:      for (uint256 i = 0; i < _count; i++) {

130:      for (uint256 i = 0; i < assetYields.length; i++) {

156:      for (uint256 i = 0; i < length; i++) {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L120

13. Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

There are 5 instances of this issue:

File: smart-contracts/CollateralAdapter.sol

22:     uint256 public constant VAULT_REVISION = 0x1;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/CollateralAdapter.sol#L22

File: smart-contracts/GeneralVault.sol

55:     uint256 public constant VAULT_REVISION = 0x1;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L55

File: smart-contracts/YieldManager.sol

41:     uint256 public constant VAULT_REVISION = 0x1;

48:     uint256 public constant UNISWAP_FEE = 10000; // 1%

49:     uint256 public constant SLIPPAGE = 500; // 5%

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L41

14. Duplicated require()/revert() checks should be refactored to a modifier or function

Saves deployment costs

There are 2 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol   #1

101:      require(_token == _tokenFromConvex, Errors.VT_INVALID_CONFIGURATION);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L101

File: smart-contracts/YieldManager.sol   #2

203:      require(_pool != address(0), Errors.VT_INVALID_CONFIGURATION);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L203

15. Empty blocks should be removed or emit something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})

There are 6 instances of this issue:

File: smart-contracts/GeneralVault.sol

153:    function processYield() external virtual {}

158:    function pricePerShare() external view virtual returns (uint256) {}

246:    {}

255:    ) internal virtual returns (uint256) {}

265:    {}

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L153

File: smart-contracts/LidoVault.sol

24:     receive() external payable {}

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L24

16. Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 9 instances of this issue:

File: smart-contracts/CollateralAdapter.sol

43      function addCollateralAsset(
44        address _externalAsset,
45        address _internalAsset,
46        address _acceptVault
47:     ) external onlyAdmin {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/CollateralAdapter.sol#L43-L47

File: smart-contracts/ConvexCurveLPVault.sol

37:     function setConfiguration(address _lpToken, uint256 _poolId) external onlyAdmin {

87:     function processYield() external override onlyAdmin {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L37

File: smart-contracts/GeneralVault.sol

165:    function setTreasuryInfo(address _treasury, uint256 _fee) external onlyAdmin {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L165

File: smart-contracts/LidoVault.sol

30:     function processYield() external override onlyAdmin {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L30

File: smart-contracts/YieldManager.sol

64:     function setExchangeToken(address _token) external onlyAdmin {

73:     function registerAsset(address _asset) external onlyAdmin {

92      function setCurvePool(
93        address _tokenIn,
94        address _tokenOut,
95        address _pool
96:     ) external onlyAdmin {

118:    function distributeYield(uint256 _offset, uint256 _count) external onlyAdmin {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L64

17. public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public and can save gas by doing so.

There are 3 instances of this issue:

File: smart-contracts/CollateralAdapter.sol   #1

35:     function initialize(ILendingPoolAddressesProvider _provider) public initializer {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/CollateralAdapter.sol#L35

File: smart-contracts/GeneralVault.sol   #2

61:     function initialize(ILendingPoolAddressesProvider _provider) public initializer {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L61

File: smart-contracts/YieldManager.sol   #3

60:     function initialize(ILendingPoolAddressesProvider _provider) public initializer {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L60