Sturdy contest - cccz's results

Lines of code

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

Vulnerability details

Impact

In the _withdrawFromYieldPool function of the LidoVault contract, the require statement is placed after the return statement, which makes the function not check the return value of to.call, which may result in the user not receiving ether.

  function _withdrawFromYieldPool(
    address _asset,
    uint256 _amount,
    address _to
  ) internal override returns (uint256) {
    address LIDO = _addressesProvider.getAddress('LIDO');
    if (_asset == address(0)) {
      // Case of ETH withdraw request from user, so exchange stETH -> ETH via curve
      uint256 receivedETHAmount = CurveswapAdapter.swapExactTokensForTokens(
        _addressesProvider,
        _addressesProvider.getAddress('STETH_ETH_POOL'),
        LIDO,
        ETH,
        _amount,
        200
      );

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

Proof of Concept

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

The example contract below shows that the require statement does not take effect when the require statement comes after the return statement

pragma solidity 0.6.12;

contract A{
  uint256 public a;
  function convertA() external{
      a = _test();
  }
  function _test() internal returns (uint256) {
    return 1;
    require(false);
  }
}

Tools Used

None

Recommended Mitigation Steps

Put the require statement before the return statement

  function _withdrawFromYieldPool(
    address _asset,
    uint256 _amount,
    address _to
  ) internal override returns (uint256) {
    address LIDO = _addressesProvider.getAddress('LIDO');
    if (_asset == address(0)) {
      // Case of ETH withdraw request from user, so exchange stETH -> ETH via curve
      uint256 receivedETHAmount = CurveswapAdapter.swapExactTokensForTokens(
        _addressesProvider,
        _addressesProvider.getAddress('STETH_ETH_POOL'),
        LIDO,
        ETH,
        _amount,
        200
      );

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

Lines of code

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

Vulnerability details

Impact

In the depositCollateral function of the LidoVault contract, the user can choose to deposit Ether or LIDO. When the user chooses to deposit LIDO, the _depositToYieldPool function does not check msg.value == 0. When the user accidentally carries Ether in the transaction, the user will lost his Ether.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

When user deposits LIDO, check msg.value == 0.

    } else {
      // Case of stETH deposit from user, receive stETH from user
      require(_asset == LIDO && msg.value == 0, Errors.VT_COLLATERAL_DEPOSIT_INVALID);
      IERC20(LIDO).safeTransferFrom(msg.sender, address(this), _amount);
    }