Sturdy contest - WatchPug's results

Lines of code

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

Vulnerability details

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

require(withdrawAmount >= _amount.percentMul(99_00), Errors.VT_WITHDRAW_AMOUNT_MISMATCH);

The GeneralVault.sol contract comes with a on-chain slippage/loss control to ensure the output amount is no more than 1% less of the requested amount.

This can be a problem when the wrapped asset in underlying protocol is now trading at a discount or loss.

For example, if Lido's setETH is trading at a more than 1% of discount on Curve (which is the case at the moment of writing), trying to withdraw as ETH will always fail with error: VT_WITHDRAW_AMOUNT_MISMATCH.

Since GeneralVault is the super class for all kinds of vaults, when the underlying protocol is suffering a loss of > 1% or the wrapped token is trading at a discount > 1% for a long period of time, this can be major problem that pervents all users from withdrawing.

In essence, causing the funds to be frozen in the contract.

Recommendation

Consider adding a minAmountOut as slippage control and allow the user to decide the minimum acceptable amount in outToken.

Lines of code

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

Vulnerability details

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

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);
  } else {
    // Case of stETH withdraw request from user, so directly send
    require(_asset == LIDO, Errors.VT_COLLATERAL_WITHDRAW_INVALID);
    IERC20(LIDO).safeTransfer(_to, _amount);
  }
  return _amount;
}

When calling LidoVault#withdrawCollateral(), LidoVault#_withdrawFromYieldPool() will called internally.

At L140, when the _to address is a smart contract that can not receive ETH, the transfer will fail and the local bool variable sent will be false.

There is a check to revert the transaction when send == false at L142.

However, L142 and L141 are in a wrong order. As a result, the user will lose funds as L141 returned and completed the transaction before the check at L142.

Recommendation

Change to:

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;
  } else {
    // Case of stETH withdraw request from user, so directly send
    require(_asset == LIDO, Errors.VT_COLLATERAL_WITHDRAW_INVALID);
    IERC20(LIDO).safeTransfer(_to, _amount);
  }
  return _amount;
}

Lines of code

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

Vulnerability details

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

if (_amount == type(uint256).max) {
    uint256 decimal = IERC20Detailed(_asset).decimals();
    _amount = _amountToWithdraw.mul(this.pricePerShare()).div(10**decimal);
}

Per the comment:

The asset address for collateral * _asset = 0x0000000000000000000000000000000000000000 means to use ETH as collateral

However, if the user set amount as type(uint256).max and _asset is set as the native token at the same time, the transaction will always fail at L122 because IERC20Detailed(_asset).decimals() will revert.

Recommendation

Change to:

if (_amount == type(uint256).max && _asset != address(0)) {

[N] Outdated compiler version

It's a best practice to use the latest compiler version.

The specified minimum compiler version is quite old. Older compilers might be susceptible to some bugs. We recommend changing the solidity version pragma to the latest version to enforce the use of an up to date compiler.

List of known compiler bugs and their severity can be found here: https://etherscan.io/solcbuginfo

[L] Critical operations should emit events

Across the contracts, there are certain critical operations that change critical values that affect the users of the protocol.

It's a best practice for these setter functions to emit events to record these changes on-chain for off-chain monitors/tools/interfaces to register the updates and react if necessary.

Instances include:

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

  function setConfiguration(address _lpToken, uint256 _poolId) external onlyAdmin {
    require(internalAssetToken == address(0), Errors.VT_INVALID_CONFIGURATION);

    convexBooster = 0xF403C135812408BFbE8713b5A23a04b3D48AAE31;
    curveLPToken = _lpToken;
    convexPoolId = _poolId;
    SturdyInternalAsset _interalToken = new SturdyInternalAsset(
      string(abi.encodePacked('Sturdy ', IERC20Detailed(_lpToken).symbol())),
      string(abi.encodePacked('c', IERC20Detailed(_lpToken).symbol())),
      IERC20Detailed(_lpToken).decimals()
    );
    internalAssetToken = address(_interalToken);
  }

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

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

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[M-1] ++i is more efficient than i++

Using ++i is more gas efficient than i++, especially in for loops.

For example:

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

    for (uint256 i = 0; i < extraRewardsLength; i++) {
      address _extraReward = IConvexBaseRewardPool(baseRewardPool).extraRewards(i);
      address _rewardToken = IRewards(_extraReward).rewardToken();
      _transferYield(_rewardToken);
    }

Can be changed to:

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

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

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

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

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

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

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

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

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

[M-2] Setting uint256 variables to 0 is redundant

For example:

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

    for (uint256 i = 0; i < extraRewardsLength; i++) {
      address _extraReward = IConvexBaseRewardPool(baseRewardPool).extraRewards(i);
      address _rewardToken = IRewards(_extraReward).rewardToken();
      _transferYield(_rewardToken);
    }

Can be changed to:

        for (uint256 i; i < extraRewardsLength; i++) {

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

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

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

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

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

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

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

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

[S-3] Avoid unnecessary arithmetic operations in SafeMath can save gas

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

    if (stAssetBalance >= aTokenBalance) return stAssetBalance.sub(aTokenBalance);

    return 0;

Recommendation

    if (stAssetBalance > aTokenBalance) return stAssetBalance - aTokenBalance;

    return 0;

[S-4] Avoid repeated arithmetic operations in for loop can save gas

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

    for (uint256 i = 0; i < length; i++) {
      assetYields[i].asset = assets[i];
      if (i != length - 1) {
        // Distribute wethAmount based on percent of asset volume
        assetYields[i].amount = _WETHAmount.percentMul(
          volumes[i].mul(PercentageMath.PERCENTAGE_FACTOR).div(totalVolume)
        );
        extraWETHAmount = extraWETHAmount.sub(assetYields[i].amount);
      } else {
        // without calculation, set remained extra amount
        assetYields[i].amount = extraWETHAmount;
      }
    }

Comparing to i != length - 1, i < length can save 3 gas.

And length - 1 is being recalculated each in the for loop, which is unnecessary.

Recommendation

    for (uint256 i = 0; i < length; i++) {
      assetYields[i].asset = assets[i];
      if (i < length) {
        // Distribute wethAmount based on percent of asset volume
        assetYields[i].amount = _WETHAmount.percentMul(
          volumes[i].mul(PercentageMath.PERCENTAGE_FACTOR).div(totalVolume)
        );
        extraWETHAmount = extraWETHAmount.sub(assetYields[i].amount);
      } else {
        // without calculation, set remained extra amount
        assetYields[i].amount = extraWETHAmount;
      }
    }

Similiar issue also exists in:

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

[S-5] Cache array length in for loops can save gas

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.

Instances include:

• YieldManager.sol#distributeYield()

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

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

[S-6] Cache external call results can save gas

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

SturdyInternalAsset _interalToken = new SturdyInternalAsset(
    string(abi.encodePacked('Sturdy ', IERC20Detailed(_lpToken).symbol())),
    string(abi.encodePacked('c', IERC20Detailed(_lpToken).symbol())),
    IERC20Detailed(_lpToken).decimals()
);

IERC20Detailed(_lpToken).symbol() can be cached and reused to save gas.

Recommendation

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

string memory _symbol = IERC20Detailed(_lpToken).symbol();
SturdyInternalAsset _interalToken = new SturdyInternalAsset(
    string(abi.encodePacked('Sturdy ', _symbol)),
    string(abi.encodePacked('c', _symbol)),
    IERC20Detailed(_lpToken).decimals()
);

[S-7] Avoid unnecessary external call can save gas

Every call to an external contract costs a decent amount of gas. For optimization of gas usage, external call results should be reused if possible.

For example:

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

    if (_amount == type(uint256).max) {
      uint256 decimal = IERC20Detailed(_asset).decimals();
      _amount = _amountToWithdraw.mul(this.pricePerShare()).div(10**decimal);
    }

Can be changed to:

    if (_amount == type(uint256).max) {
      uint256 decimal = IERC20Detailed(_asset).decimals();
      _amount = _amountToWithdraw.mul(1e18).div(10**decimal);
    }