QA Report

[code423n4]

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

[HickupHH3]

NC: NC-01 + NC-05, NC-02, NC-03, NC-04, L-02, L-03
Invalid: L-01. Reverting actually might be a bug because it affects the LendingPool (OOS) flow and break liquidation flows.