Sturdy contest - Dravee's results

Lines of code

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

Vulnerability details

The LidoVault._withdrawFromYieldPool() function performs a low-level .call here:

File: LidoVault.sol
140:       (bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');
141:       return receivedETHAmount;
142:       require(sent, Errors.VT_COLLATERAL_WITHDRAW_INVALID);

It seems like lines 141 and 142 are swapped (L142 is an unreachable dead-code). Therefore, the return value from the call is never checked to see if the call succeeded. Furthermore, no address(0) checks are implemented so to might very well be == address(0) in the future of the solution, burning funds and still returning a success call even if the lines 141 and 142 are swapped as suggested.

Consider:

• Swapping L141 and 142
• Adding an address(0) check on address to

Table of Contents:

• [L-01] Add constructor initializers
• [L-02] Deprecated safeApprove() function
• [L-03] Deprecated approve() function
• [L-04] Add a timelock to critical functions
• [L-05] Lack of event emission after critical initialize() functions
• [N-01] Missing NatSpec comments
• [N-02] A magical number should be documented and explained. Use a constant instead
• [N-03] Hardcoded strings having multiple occurences should be declared as constant

[L-01] Add constructor initializers

As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”

Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”

Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:

smart-contracts/CollateralAdapter.sol:
  35:   function initialize(ILendingPoolAddressesProvider _provider) public initializer {

smart-contracts/GeneralVault.sol:
  61:   function initialize(ILendingPoolAddressesProvider _provider) public initializer {

smart-contracts/YieldManager.sol:
  60:   function initialize(ILendingPoolAddressesProvider _provider) public initializer {

[L-02] Deprecated safeApprove() function

Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219 (OpenZeppelin/openzeppelin-contracts#2219). The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.

As recommended by the OpenZeppelin comment, I suggest replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead:

smart-contracts/ConvexCurveLPVault.sol:
  141:     IERC20(curveLPToken).safeApprove(convexBooster, _amount);
  146:     IERC20(internalAssetToken).safeApprove(address(_addressesProvider.getLendingPool()), _amount);

smart-contracts/LidoVault.sol:
  102:     IERC20(LIDO).safeApprove(address(_addressesProvider.getLendingPool()), assetAmount);

[L-03] Deprecated approve() function

While safeApprove() in itself is deprecated, it is still better than approve which is subject to a known front-running attack. Consider using safeApprove instead (or better: safeIncreaseAllowance()/safeDecreaseAllowance()):

smart-contracts/YieldManager.sol:
  221:     IERC20(_asset).approve(_lendingPool, _amount);

[L-04] Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).

Consider adding a timelock to:

File: GeneralVault.sol
160:   /**
161:    * @dev Set treasury address and vault fee
162:    * @param _treasury The treasury address
163:    * @param _fee The vault fee which has more two decimals, ex: 100% = 100_00
164:    */
165:   function setTreasuryInfo(address _treasury, uint256 _fee) external onlyAdmin {
166:     require(_treasury != address(0), Errors.VT_TREASURY_INVALID);
167:     require(_fee <= 30_00, Errors.VT_FEE_TOO_BIG);
168:     _treasuryAddress = _treasury;
169:     _vaultFee = _fee;
170: 
171:     emit SetTreasuryInfo(_treasury, _fee);
172:   }

[L-05] Lack of event emission after critical initialize() functions

To record the init parameters for off-chain monitoring and transparency reasons, please consider emitting an event after the initialize() functions:

smart-contracts/CollateralAdapter.sol:
  35:   function initialize(ILendingPoolAddressesProvider _provider) public initializer {
  36      _addressesProvider = _provider;
  37    }

smart-contracts/GeneralVault.sol:
  61:   function initialize(ILendingPoolAddressesProvider _provider) public initializer {
  62      _addressesProvider = _provider;
  63    }

smart-contracts/YieldManager.sol:
  60:   function initialize(ILendingPoolAddressesProvider _provider) public initializer {
  61      _addressesProvider = _provider;
  62    }

[N-01] Missing NatSpec comments

The following comments are missing (see @audit tags):

smart-contracts/GeneralVault.sol:
  130    /**
  131     * @dev Withdraw an `amount` of asset used as collateral to user on liquidation.
  132     * @param _asset The asset address for collateral
  133     *  _asset = 0x0000000000000000000000000000000000000000 means to use ETH as collateral
  134:    * @param _amount The amount to be withdrawn //@audit missing @return uint256
  135     */
  136    function withdrawOnLiquidation(address _asset, uint256 _amount)
  137      external
  138      virtual
  139      returns (uint256)

smart-contracts/YieldManager.sol:
  101    /**
  102     * @dev Function to get Curve Pool address for the swap
  103     * @param _tokenIn The address of token being sent
  104:    * @param _tokenOut The address of token being received  //@audit missing @return address
  105     */
  106    function getCurvePool(address _tokenIn, address _tokenOut) external view returns (address) {

[N-02] A magical number should be documented and explained. Use a constant instead

smart-contracts/LidoVault.sol:
   42      uint256 receivedETHAmount = CurveswapAdapter.swapExactTokensForTokens(
   43        _addressesProvider,
   44        _addressesProvider.getAddress('STETH_ETH_POOL'),
   45        LIDO,
   46        ETH,
   47        yieldStETH,
   48:       200 //@audit magic number
   49      );

  130        uint256 receivedETHAmount = CurveswapAdapter.swapExactTokensForTokens(
  131          _addressesProvider,
  132          _addressesProvider.getAddress('STETH_ETH_POOL'),
  133          LIDO,
  134          ETH,
  135          _amount,
  136:         200 //@audit magic number
  137        );

I suggest using constant variables as this would make the code more maintainable and readable while costing nothing gas-wise (constants are replaced by their value at compile-time).

[N-03] Hardcoded strings having multiple occurences should be declared as constant

The following strings should be declared in a constant variable, which should be used instead:

• 'LIDO'
• 'STETH_ETH_POOL'
• 'WETH'

Occurences:

smart-contracts/LidoVault.sol:
   32:     address LIDO = _addressesProvider.getAddress('LIDO');
   44:       _addressesProvider.getAddress('STETH_ETH_POOL'),
   52:     address weth = _addressesProvider.getAddress('WETH');
   59:     emit ProcessYield(_addressesProvider.getAddress('WETH'), receivedETHAmount);
   66:     return _getYieldAmount(_addressesProvider.getAddress('LIDO'));
   84:     address LIDO = _addressesProvider.getAddress('LIDO');
   91:       (bool sent, bytes memory data) = LIDO.call{value: msg.value}('');
  116:     return (_addressesProvider.getAddress('LIDO'), _amount);
  127:     address LIDO = _addressesProvider.getAddress('LIDO');
  132:         _addressesProvider.getAddress('STETH_ETH_POOL'),
  156:     IERC20(_addressesProvider.getAddress('LIDO')).safeTransfer(_treasuryAddress, treasuryAmount);

Table of Contents:

• Caching storage values in memory
• Not using SafeMath can save gas on arithmetics operations that can't underflow/overflow
• A modifier used only once and not being inherited should be inlined to save gas
• Use of this instead of marking as public an external function
• > 0 is less efficient than != 0 for unsigned integers (with proof)
• Contract is Ownable but owner capabilites are not used
• An array's length should be cached to save gas in for-loops
• ++i costs less gas compared to i++ or i += 1
• No need to explicitly initialize variables with default values

Caching storage values in memory

The code can be optimized by minimising the number of SLOADs.

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.

See the @audit tags for details about the multiple SLOADs where a cached value should be used instead of SLOAD 2 and above:

smart-contracts/ConvexCurveLPVault.sol:
   93:     address _token = _addressesProvider.getAddress('CRV'); //@audit gas: SLOAD 1 (_addressesProvider)
   99:     _token = _addressesProvider.getAddress('CVX'); //@audit gas: SLOAD 2 (_addressesProvider)
  137:     require(_asset == curveLPToken, Errors.VT_COLLATERAL_DEPOSIT_INVALID); //@audit gas: SLOAD 1 (curveLPToken)
  138:     TransferHelper.safeTransferFrom(curveLPToken, msg.sender, address(this), _amount); //@audit gas: SLOAD 2 (curveLPToken)
  141:     IERC20(curveLPToken).safeApprove(convexBooster, _amount); //@audit gas: SLOAD 3 (curveLPToken) + SLOAD 1 (convexBooster)
  142:     IConvexBooster(convexBooster).deposit(convexPoolId, _amount, true); //@audit gas: SLOAD 2 (convexBooster)
  145:     SturdyInternalAsset(internalAssetToken).mint(address(this), _amount); //@audit gas: SLOAD 1 (internalAssetToken)
  146:     IERC20(internalAssetToken).safeApprove(address(_addressesProvider.getLendingPool()), _amount); //@audit gas: SLOAD 2 (internalAssetToken)
  148:     return (internalAssetToken, _amount); //@audit gas: SLOAD 3 (internalAssetToken)

smart-contracts/LidoVault.sol:
   32:     address LIDO = _addressesProvider.getAddress('LIDO'); //@audit gas: SLOAD 1 (_addressesProvider)
   43:       _addressesProvider, //@audit gas: SLOAD 2 (_addressesProvider)
   44:       _addressesProvider.getAddress('STETH_ETH_POOL'), //@audit gas: SLOAD 3 (_addressesProvider)
   52:     address weth = _addressesProvider.getAddress('WETH'); //@audit gas: SLOAD 4 (_addressesProvider)
   56:     address yieldManager = _addressesProvider.getAddress('YIELD_MANAGER'); //@audit gas: SLOAD 5 (_addressesProvider)
   59:     emit ProcessYield(_addressesProvider.getAddress('WETH'), receivedETHAmount); //@audit gas: SLOAD 6 (_addressesProvider)
   84:     address LIDO = _addressesProvider.getAddress('LIDO'); //@audit gas: SLOAD 1 (_addressesProvider)
  102:     IERC20(LIDO).safeApprove(address(_addressesProvider.getLendingPool()), assetAmount); //@audit gas: SLOAD 2 (_addressesProvider)
  127:     address LIDO = _addressesProvider.getAddress('LIDO'); //@audit gas: SLOAD 1 (_addressesProvider)
  131:         _addressesProvider, //@audit gas: SLOAD 2 (_addressesProvider)
  132:         _addressesProvider.getAddress('STETH_ETH_POOL'), //@audit gas: SLOAD 3 (_addressesProvider)

smart-contracts/YieldManager.sol:
   74:     _assetsList[_assetsCount] = _asset; //@audit gas: SLOAD 1 (_assetsCount)
   75:     _assetsCount = _assetsCount + 1; //@audit gas: SLOAD 2 (_assetsCount)
  199:     if (_tokenOut == _exchangeToken) { //@audit gas: SLOAD 1 (_exchangeToken)
  202:     address _pool = _curvePools[_exchangeToken][_tokenOut]; //@audit gas: SLOAD 2 (_exchangeToken)
  207:       _exchangeToken, //@audit gas: SLOAD 3 (_exchangeToken)

Not using SafeMath can save gas on arithmetics operations that can't underflow/overflow

When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), SafeMath isn't necessary:

File: GeneralVault.sol
196:     if (stAssetBalance >= aTokenBalance) return stAssetBalance.sub(aTokenBalance); //@audit gas: no need to use SafeMath here as it can't underflow. Replace "sub" with " - "

A modifier used only once and not being inherited should be inlined to save gas

Affected code:

smart-contracts/CollateralAdapter.sol:
  17:   modifier onlyAdmin() {
  47:   ) external onlyAdmin {

Use of this instead of marking as public an external function

Using this. is like making an expensive external call.

Consider marking pricePerShare() as public:

File: GeneralVault.sol
158:   function pricePerShare() external view virtual returns (uint256) {}
...
123:       _amount = _amountToWithdraw.mul(this.pricePerShare()).div(10**decimal); //@audit gas: "this" makes an external call. Consider marking "pricePerShare()" as "public" instead of "external" 

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

GeneralVault.sol:179:    require(yieldStAsset > 0, Errors.VT_PROCESS_YIELD_INVALID);
LidoVault.sol:88:      require(msg.value > 0, Errors.VT_COLLATERAL_DEPOSIT_REQUIRE_ETH);

Also, please enable the Optimizer.

Contract is Ownable but owner capabilites are not used

Reduce contract size by removing Ownable given that its functionalities are not used.

YieldManager.sol:13:import {Ownable} from '../../dependencies/openzeppelin/contracts/Ownable.sol';
YieldManager.sol:26:contract YieldManager is VersionedInitializable, Ownable {

An array's length should be cached to save gas in for-loops

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.

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

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

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

++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.

The same is also true for i--.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

ConvexCurveLPVault.sol:106:    for (uint256 i = 0; i < extraRewardsLength; i++) {
GeneralVault.sol:218:    for (uint256 i = 0; i < length; i++) {
YieldManager.sol:120:    for (uint256 i = 0; i < _count; i++) {
YieldManager.sol:130:    for (uint256 i = 0; i < assetYields.length; i++) {
YieldManager.sol:156:    for (uint256 i = 0; i < length; i++) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

ConvexCurveLPVault.sol:106:    for (uint256 i = 0; i < extraRewardsLength; i++) {
GeneralVault.sol:218:    for (uint256 i = 0; i < length; i++) {
YieldManager.sol:120:    for (uint256 i = 0; i < _count; i++) {
YieldManager.sol:130:    for (uint256 i = 0; i < assetYields.length; i++) {
YieldManager.sol:156:    for (uint256 i = 0; i < length; i++) {

I suggest removing explicit initializations for default values.