Sturdy contest - rotcivegaf's results

Lines of code

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

Vulnerability details

Impact

If the call fails, the refunds did not succeed and the caller will lose all funds of receivedETHAmount.

Proof of Concept

In the LidoVault what heredit from GeneralVault, the function withdrawCollateral recibe an address _asset and an address _to If _asset its address(0) what reference ETH and the _to its a contract performs a low-level .call inside the _withdrawFromYieldPool: (bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}(''); but does not check the return value if the call succeeded.

Tools Used

hardhat

Recommended Mitigation Steps

Switch the Line 142 to Line 141

Lines of code

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L75-L89 https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L79-L104 https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L131-L149

Vulnerability details

Impact

Possible lost value in depositCollateral function call

Proof of Concept

In call depositCollateral can will send value and the asset can be an ERC20(!= address(0)), if LidoVault and ConvexCurveLPVault contract receive this call the fouds will lost Also in LidoVault, L88, if send as asset ETH(== address(0)) and send more value than _amount(msg.value > _amount), the exedent will lost

Recommended Mitigation Steps

In GeneralVault, depositCollateral function:

• Check if the msg.value is zero when the _asset is ERC20(!= address(0))
• Check if the msg.value is equeal to _amount when the _asset ETH(== address(0))

function depositCollateral(address _asset, uint256 _amount) external payable virtual {
  if (_asset != address(0)) { // asset = ERC20
    require(msg.value == 0, <AN ERROR FROM Errors LIBRARY>);
  } else { // asset = ETH
    require(msg.value == _amount, <AN ERROR FROM Errors LIBRARY>);
  }

  // Deposit asset to vault and receive stAsset
  // Ex: if user deposit 100ETH, this will deposit 100ETH to Lido and receive 100stETH TODO No Lido
  (address _stAsset, uint256 _stAssetAmount) = _depositToYieldPool(_asset, _amount);

  // Deposit stAsset to lendingPool, then user will get aToken of stAsset
  ILendingPool(_addressesProvider.getLendingPool()).deposit(
    _stAsset,
    _stAssetAmount,
    msg.sender,
    0
  );

  emit DepositCollateral(_asset, msg.sender, _amount);
}

Also can remove the require(msg.value > 0, Errors.VT_COLLATERAL_DEPOSIT_REQUIRE_ETH); in LidoVault, L88

QA reports (low/non-critical)

Contest: Sturdy

Autor: Rotcivegaf

Scope:

• ConvexCurveLPVault.sol
• CollateralAdapter.sol
• GeneralVault.sol
• LidoVault.sol
• YieldManager.sol

Non-critical

[NC-01] Unused function parameter / Unused local variable

ConvexCurveLPVault.sol, L154

function _getWithdrawalAmount(address _asset, uint256 _amount)
                              ^------------^

GeneralVault.sol, L136

function withdrawOnLiquidation(address _asset, uint256 _amount)
                               ^------------^

LidoVault.sol, L91

LidoVault.sol:91:19: Warning: Unused local variable.
      (bool sent, bytes memory data) = LIDO.call{value: msg.value}('');
                  ^---------------^

LidoVault.sol, L109

function _getWithdrawalAmount(address _asset, uint256 _amount)
                              ^------------^

LidoVault.sol, L140

(bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');
            ^---------------^

[NC-02] GeneralVault contract to abstract contract

Change the GeneralVault contract to abstract contract to avoid the deploy by mistake and define the functions without implementation:

• L153: function processYield replace {} to ;
• L158: function pricePerShare replace {} to ;
• L242-L246: function _depositToYieldPool replace {} to ;
• L251-L255: function _withdrawFromYieldPool replace {} to ;
• L260-L265: function _getWithdrawalAmount replace {} to ;

The withdrawOnLiquidation(L136) function only returns the _amount parameter, avoid the implementation to avoid mistakes and that the inherited contract implements it

[NC-03] Missing documentation

In some functions/variables missing documentation:

• In CollateralAdapter: addCollateralAsset
• In ConvexCurveLPVault: processYield, _withdraw, withdrawOnLiquidation
• In GeneralVault: AssetYield, getRevision, _depositYield
• In YieldManager: AssetYield, setExchangeToken, getRevision

[NC-04]

In GeneralVault, the depositCollateral(L75) function: The safeApprove of _stAsset should call inside this function and just before deposit and no in the _depositToYieldPool

function depositCollateral(address _asset, uint256 _amount) external payable virtual {
  // Deposit asset to vault and receive stAsset
  // Ex: if user deposit 100ETH, this will deposit 100ETH to Lido and receive 100stETH TODO No Lido
  (address _stAsset, uint256 _stAssetAmount) = _depositToYieldPool(_asset, _amount);

  IERC20(_stAsset).safeApprove(address(_addressesProvider.getLendingPool()), _stAssetAmount);
  // Deposit stAsset to lendingPool, then user will get aToken of stAsset
  ILendingPool(_addressesProvider.getLendingPool()).deposit(
    _stAsset,
    _stAssetAmount,
    msg.sender,
    0
  );

  emit DepositCollateral(_asset, msg.sender, _amount);
}

Remember import SafeERC20 library and using SafeERC20 for IERC20;

[NC-05] Unused import

In CollateralAdapter, the import {ILendingPool} from '../../interfaces/ILendingPool.sol'; is unused In GeneralVault, the import {ILendingPool} from '../../interfaces/ILendingPool.sol'; is unused In YieldManager, the import {IPriceOracleGetter} from '../../interfaces/IPriceOracleGetter.sol';, import {ISwapRouter} from '../../interfaces/ISwapRouter.sol';, import {TransferHelper} from '../libraries/helpers/TransferHelper.sol'; are unused

Low

[L-01] withdrawOnLiquidation only returns _amount

The contract LidoVault heredit from GeneralVault the function withdrawOnLiquidation but dont reimplement it, an user can use this function by mistake esperando withdraw a collateral Implement it with a revert to avoid mistake calls

  function withdrawOnLiquidation(address, uint256)
    external
    virtual
    returns (uint256)
  {
    revert(<AN ERROR FROM Errors LIBRARY>);
  }

[L-02] OPEN TODO

GeneralVault.sol, L77: There is an open TODO and don't understand what it mean

[L-03] Comment function

GeneralVault.sol, L148: The function convertOnLiquidation its commented

Gas report

Contest: Sturdy

Autor: Rotcivegaf

Scope:

• ConvexCurveLPVault.sol
• CollateralAdapter.sol
• GeneralVault.sol
• LidoVault.sol
• YieldManager.sol

[G-01] Use function parameter

ConvexCurveLPVault.sol, L138 L141: Replace curveLPToken to _asset, this save the gas to SLOAD

[G-02] SLOAD gas optimization

ConvexCurveLPVault.sol, L44 L45: cache IERC20Detailed(_lpToken).symbol()) in a local variable to save gas of storage reads

[G-03] Make constant the convexBooster contract variable

ConvexCurveLPVault.sol, L27 L40: address public convexBooster; to address public constant convexBooster; and remove the set in setConfiguration function

[G-04] <var>++ or <var> += 1 to ++<var>

When the value of the post-loop increment/decrement is not stored or used in any calculations, the prefix increment/decrement operators (++<var>/--<var>) cost less gas PER LOOP than the postfix increment/decrement operators (<var>++/<var>--)

Use ++<var> rather than <var>++ to save gas

In ConvexCurveLPVault.sol, L106 In GeneralVault.sol, L218 In YieldManager.sol, L120, L130, L156

[G-05] Sum outside of for

In YieldManager.sol, L121: Move _offset + i outside the for to consume less gas

_count = _offset + _offset;

for (uint256 i = _offset; i < _count; i++) {
  address asset = _assetsList[i];
  require(asset != address(0), Errors.UL_INVALID_INDEX);
  uint256 _amount = IERC20Detailed(asset).balanceOf(address(this));
  _convertAssetToExchangeToken(asset, _amount);
}

[G-06] Caching length on for

In YieldManager.sol, L130: Caching the array length is more gas efficient

uint256 assetYieldsLength = assetYields.length;

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

[G-07] Sub outside of for

In YieldManager.sol, L158: Move length - 1 outside the for to consume less gas

uint256 lengthLessOne = length - 1;

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