Sturdy contest - 0xf15ers's results

Lines of code

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

Vulnerability details

Title

ETH transefer is not checked properly

Impact

Function will return successfully even if the transfer of ETH failed

Proof of Concept

in smart-contracts/LidoVault.sol#L141-L142 the function returns before checking that the ETH is sent properly. This may cause the function successfully returning even if the ETH transfer failed and eventually causes fund loss for the user.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Check that the ETH transfer is successful before function returns

      // Before
      return receivedETHAmount;
      require(sent, Errors.VT_COLLATERAL_WITHDRAW_INVALID);

      // After
      require(sent, Errors.VT_COLLATERAL_WITHDRAW_INVALID);
      return receivedETHAmount;

1. Event DepositCollateral uses _amount as parameter which may be wrong while depositing ether

in smart-contracts/GeneralVault.sol#L88 the event DepositCollateral uses _amount as parameter which may be wrong. For eg. in case of depositing ETH (implementation in smart-contracts/LidoVault.sol#L79-L104) the actual amount is taken from msg.value rather than user provided _amount. This will lead to reporting a false amount in the Events if the user calls the function with different _amount than the amount of ETH provided(i.e. msg.value).

Recommended solution

use _stAssetAmount in the event rather than _amount to use correct avount of asset recieved in the function. Also use stAsset instead of _asset to be sure the actual asset deposited is being reported.

 emit DepositCollateral(_stAsset, msg.sender, _stAssetAmount);

The event WithdrawCollateral is also called with _amount in smart-contracts/GeneralVault.sol#L127 which is correct for now across all implementations, but its better to change it to _stAssetAmount to make sure it stays correct for future implementations too.

1. checking a != 0 is cheaper than a > 0 for uint

• In GeneralVault.sol#L170, yieldStAsset > 0 can be replaced with yieldStAsset != 0 since it is uint.

2. Prefix increment can be used in loops instead of postfix increment as it is cheaper

  // Before
            for (uint256 i = 0; i < _count; i++) {
                    ......
        }
  // After
        for (uint256 i = 0; i < _count; ++i) {
        }

3. Data.length is calculated for each iteration in loop, this can be cached to save gas

• In YieldManager.sol#L130 assetYields.length can be cached to save gas

  // Before
        for (uint256 i = 0; i < assetYields.length; i++) {
            ...
        }
  // After
        uint len = assetYields.length;
        for (uint256 i = 0; i < len; ++i) {
        }