Sturdy contest - hyh's results

Lines of code

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

Vulnerability details

Currently the ETH withdrawing transfer result control is unreachable in _withdrawFromYieldPool as the function returns early and address.call result remains unchecked.

If the call weren't successful for any reason the user funds end up frozen in the contract as the deposit's state is updated, while funds aren't delivered in this case.

Setting the severity to high as that's a violation of the logic of the system and funds freeze impact for the collateral depositor.

Proof of Concept

_withdrawFromYieldPool doesn't check ETH sending call success as the function now exit beforehand:

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

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

This way if call be unsuccessful for any reason the corresponding user's funds will be frozen within the contract as withdrawCollateral will go through and the withdrawAmount will be accounted as successfully withdrawn from the lending pool:

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

    // withdraw from lendingPool, it will convert user's aToken to stAsset
    uint256 _amountToWithdraw = ILendingPool(_addressesProvider.getLendingPool()).withdrawFrom(
      _stAsset,
      _stAssetAmount,
      msg.sender,
      address(this)
    );

    // Withdraw from vault, it will convert stAsset to asset and send to user
    // Ex: In Lido vault, it will return ETH or stETH to user
    uint256 withdrawAmount = _withdrawFromYieldPool(_asset, _amountToWithdraw, _to);

Recommended Mitigation Steps

Consider enabling require check by switching the lines:

      // 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/ConvexCurveLPVault.sol#L74-L82

Vulnerability details

Now there are no checks for the amounts to be transferred via _transferYield and _processTreasury. As reward token list is external and an arbitrary token can end up there, in the case when such token doesn't allow for zero amount transfers, the reward retrieval can become unavailable.

I.e. processYield() can be fully blocked for even an extended period, with some low probability, which cannot be controlled otherwise as pool reward token list is external.

Setting the severity to medium as reward gathering is a base functionality for the system and its availability is affected.

Proof of Concept

_transferYield proceeds with sending the amounts to treasury and yieldManager without checking:

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

    // transfer to treasury
    if (_vaultFee > 0) {
      uint256 treasuryAmount = _processTreasury(_asset, yieldAmount);
      yieldAmount = yieldAmount.sub(treasuryAmount);
    }

    // transfer to yieldManager
    address yieldManager = _addressesProvider.getAddress('YIELD_MANAGER');
    TransferHelper.safeTransfer(_asset, yieldManager, yieldAmount);

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

  function _processTreasury(address _asset, uint256 _yieldAmount) internal returns (uint256) {
    uint256 treasuryAmount = _yieldAmount.percentMul(_vaultFee);
    IERC20(_asset).safeTransfer(_treasuryAddress, treasuryAmount);
    return treasuryAmount;
  }

The incentive token can be arbitrary. Some ERC20 do not allow zero amounts to be sent:

https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers

In a situation of such a token added to reward list and zero incentive amount earned the whole processYield call will revert, making reward gathering unavailable until either such token be removed from pool's reward token list or some non-zero reward amount be earned. Both are external processes and aren’t controllable.

Recommended Mitigation Steps

Consider running the transfers in _transferYield only when yieldAmount is positive:

+	if (yieldAmount > 0) {
	    // transfer to treasury
	    if (_vaultFee > 0) {
	      uint256 treasuryAmount = _processTreasury(_asset, yieldAmount);
	      yieldAmount = yieldAmount.sub(treasuryAmount);
	    }

	    // transfer to yieldManager
	    address yieldManager = _addressesProvider.getAddress('YIELD_MANAGER');
	    TransferHelper.safeTransfer(_asset, yieldManager, yieldAmount);
+  }

Lines of code

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

Vulnerability details

distributeYield uses Uniswap swaps via _convertAssetToExchangeToken and Curve swaps via _convertToStableCoin. UniswapAdapter and CurveswapAdapter do use Oracle for price estimation, but distributeYield calls use hard coded 5% SLIPPAGE, which is wide enough to make sandwich attacks economically viable, specifically for Uniswap case, where price manipulation can be less costly.

As a result trades converting assets to _exchangeToken can happen at a manipulated price and end up receiving fewer _exchangeTokens than current market price dictates.

Placing severity to medium as impact here is a partial fund loss conditional only on big enough asset amount to be swapped: sandwich attacks are common and can be counted to happen almost always as long as economic viability is present.

Proof of Concept

_convertAssetToExchangeToken and _convertToStableCoin use SLIPPAGE for DEX output control:

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

  function _convertAssetToExchangeToken(address asset, uint256 amount) internal {
    UniswapAdapter.swapExactTokensForTokens(
      _addressesProvider,
      asset,
      _exchangeToken,
      amount,
      UNISWAP_FEE,
      SLIPPAGE
    );
  }

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

    receivedAmount = CurveswapAdapter.swapExactTokensForTokens(
      _addressesProvider,
      _pool,
      _exchangeToken,
      _tokenOut,
      _amount,
      SLIPPAGE
    );

SLIPPAGE is hard coded to be 5%, which is excessively wide and can be exploited whenever the asset amount is big enough:

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

  uint256 public constant SLIPPAGE = 500; // 5%

Recommended Mitigation Steps

Consider adding slippage argument to the distributeYield, so it can be tuned each time accordingly to the current market conditions.

1. Core configuration can be reset many times (low)

If LP token and curve pool be switched in-between system operations, the corresponding assets invested can be frozen within old pool. Users will not be able to run most of the system functionality after that.

Introducing two functions, initialization and switching, is one of the best ways to handle such a dependency. Initialization one should be immutable (for example, via initializer modifier), while pool switching one should ensure no funds are left in the old pool and all the corresponding variables are reset/redirected.

Proof of Concept

It is now one function that can be run multiple times with a variety of system breaking outcomes:

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);
  }

Recommended Mitigation Steps

_lpToken and _poolId needs to be set only once on initialization.

The switching requires asset removal from the old pool and reinvesting to the new one.

I.e. consider introducing independent initialization and switching logic, while making initialization to be able to run only once.

2. Misspell param comment (non-critical)

Consider fixing the typo in _amount description:

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

   * @param _amount The mount of asset