Sturdy contest - sorrynotsorry's results

Judge has assessed an item in Issue #135 as High risk. The relevant finding follows:

Lines of code

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

Vulnerability details

Impact

At _withdrawFromYieldPool() ETH transfer return value is not checked as the return statement at line #141 breaks the return value checking.

Proof of Concept

  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; // <-------------- @audit-info 
      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;
  }

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

Tools Used

Manual Review

Recommended Mitigation Steps

Shift the return statement on line number:142

Judge has assessed an item in Issue #135 as Medium risk. The relevant finding follows:

QA (LOW & NON-CRITICAL)

• At GeneralVault.sol,the modifiers onlyAdmin and onlyYieldProcessor are throwing the same error string which might be confusing to identify the cause of error incase both are used in the same function.

• The project uses Solidity version 0.6.12. Using an old version prevents access to new Solidity security checks. However the current version is 0.8.13 with more benefits and less bugs.

• Any unused imports, inherited contracts, functions, parameters, variables, modifiers, events or return values should be removed (or used appropriately) after careful evaluation. This will not only reduce gas costs but improve readability and maintainability of the code. At GeneralVault.sol#L136 unused parameter: _asset, at LidoVault.sol#L91 unused local variable: bytes memory data, at LidoVault.sol#L109 unused parameter: address _asset, at LidoVault.sol#L140 unused local variable: bytes memory data, at ConvexCurveLPVault.sol#154 unused parameter: address _asset,

• At LidoVault.sol, the slippage is hardcoded to 200 inside swapExactTokensForTokens() which yields to %2 and it most likely will revert during volatile market conditions and it will not be possible to save economic value of the assets by not being able to withdraw ETH.

• ConvexCurveLPVault.sol uses abi.encodePacked(). Using abi.encodePacked() with multiple variable length arguments can, in certain situations, lead to a hash collision. Instead abi.encode() can be used. Reference Link

• At YieldManager.sol, setCurvePool() function, there is no address(0) check for the parameters.

• While swapping the assets, the contracts using deprecated safeApprove. Link

• At YieldManager.sol, distributeYield() function, an expensive loop is used by incrementing state_variable in a loop incurs a lot of gas because of expensive SSTOREs, which might lead to an out-of-gas and revert

function distributeYield(uint256 _offset, uint256 _count) external onlyAdmin {
    // 1. convert from asset to exchange token via uniswap
    for (uint256 i = 0; i < _count; i++) {
      address asset = _assetsList[_offset + i];
      require(asset != address(0), Errors.UL_INVALID_INDEX);
      uint256 _amount = IERC20Detailed(asset).balanceOf(address(this));
      _convertAssetToExchangeToken(asset, _amount);
    }
    uint256 exchangedAmount = IERC20Detailed(_exchangeToken).balanceOf(address(this));

    // 2. convert from exchange token to other stable assets via curve swap
    AssetYield[] memory assetYields = _getAssetYields(exchangedAmount);
    for (uint256 i = 0; i < assetYields.length; i++) {
      if (assetYields[i].amount > 0) {
        uint256 _amount = _convertToStableCoin(assetYields[i].asset, assetYields[i].amount);
        // 3. deposit Yield to pool for suppliers
        _depositYield(assetYields[i].asset, _amount);
      }
    }
  }

• At YieldManager.sol, _convertAssetToExchangeToken() function converts the assets to exchange token at the limits of the slippage. However, there is no oracle provided and the AMM prices are subject to manipulation. The team might consider to add a function parameter for minimum tokens to receive.