code423n4 commented 1 year ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/aave/AaveStrategy.sol#L263-L267

Vulnerability details

Summary

There is no control for liquidity squeeze that is typical for lending markets and can routinely happen in Aave pools.

Withdrawals exceeding available funds will be reverted, which will block any strategy call's upstream functionality that now tries to withdraw what is needed based solely on the aToken balance and fails if this call reverts.

Vulnerability Detail

A part of lending pool funds is routinely temporary locked, being lent out. Withdrawal requests that exceed the amount of free funds available will be failed, even for big pools there is no guarantees that given amount can be withdrawn.

This way, as an example, a rebalancing that includes many strategies will fail when one of them have even small liquidity shortage.

Impact

Rebalancing will be frozen until Aave pool liquidity returns. Corresponding Aave pool cannot be manually removed from Vault allocation.

Code Snippet

AaveStrategy's _withdraw() fails when lendingPool.withdraw() reverts:

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/aave/AaveStrategy.sol#L263-L267

@>          uint256 obtainedWrapped = lendingPool.withdraw(
                address(wrappedNative),
                toWithdraw,
                address(this)
            );

The toWithdraw is determined solely based on what is needed and the current strategy's aToken balance:

    function _currentBalance() internal view override returns (uint256 amount) {
        (amount, , , , , ) = lendingPool.getUserAccountData(address(this));
        uint256 queued = wrappedNative.balanceOf(address(this));
        uint256 claimableRewards = compoundAmount();
        return amount + queued + claimableRewards;
    }

    function _withdraw(
        address to,
        uint256 amount
    ) internal override nonReentrant {
        uint256 available = _currentBalance();
        require(available >= amount, "AaveStrategy: amount not valid");

        uint256 queued = wrappedNative.balanceOf(address(this));
        if (amount > queued) {
            compound("");

            uint256 toWithdraw = amount - queued;

While Aave v2 revert withdraw() calls when the amount is less than user balance, but exceeds the liquidity available:

https://github.com/aave/protocol-v2/blob/6f57232358af0fd41d9dcf9309d7a8c0b9aa3912/contracts/protocol/lendingpool/DefaultReserveInterestRateStrategy.sol#L132

availableLiquidity = availableLiquidity.add(liquidityAdded).sub(liquidityTaken);

https://github.com/aave/protocol-v2/blob/baeb455fad42d3160d571bd8d3a795948b72dd85/contracts/protocol/libraries/logic/ReserveLogic.sol#L233-L235

    require(vars.newLiquidityRate <= type(uint128).max, Errors.RL_LIQUIDITY_RATE_OVERFLOW);
    require(vars.newStableRate <= type(uint128).max, Errors.RL_STABLE_BORROW_RATE_OVERFLOW);
    require(vars.newVariableRate <= type(uint128).max, Errors.RL_VARIABLE_BORROW_RATE_OVERFLOW);

Tool used

Manual Review

Recommendation

Consider using try-catch approach for Aave withdrawal, for example:

  try lendingPool.withdraw(
      address(wrappedNative),
      toWithdraw,
      address(this)
      ) {
      ...
  } catch {
      return 0;
  }

Assessed type

Other

c4-pre-sort commented 1 year ago

minhquanym marked the issue as low quality report

minhquanym commented 1 year ago

QA

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)