Validation logic in `validateVariablePoolHasEnoughLiquidity()` is inaccurate with the introduction of virtual Accounting in AAVE3.1

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/CapsLibrary.sol#L67-L72

Vulnerability details

Description

In the new AAVE v3.1 upgrade (which is scheduled to be deployed in the few next weeks to follow, (source: Forum see last comment by bgd labs) (source: activated governance proposal ) the way the pools in aave handels the internal accounting has changed, instead of relying on underlying.balanceOf() the pools now use virtual accounting to calculate the cash available for withdrawals. This will lead in some cases for the pools internal balance to have less liquidity than the actual balance of the underlying asset. The upgrade have already been audited, approved and will be executed on chain in the next few weeks.

please also note: that the changes are already merged in main in the aave repo source

impact

This could lead to eventhough the borrower has taken a loan the user wouldn't be able to withdraw the funds in the withdraw() function because the aave withdrawal cal will revert, as the aave protocol wouldn't have enough cash in the virtualBalance. This makes the check in validateVariablePoolHasEnoughLiquidity() and opens up a DoS vector for the protocol.

Proof Of Concept

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/CapsLibrary.sol#L67-L72

    function validateVariablePoolHasEnoughLiquidity(State storage state, uint256 amount) public view {
        uint256 liquidity = state.data.underlyingBorrowToken.balanceOf(address(state.data.variablePool));
        if (liquidity < amount) {
            revert Errors.NOT_ENOUGH_BORROW_ATOKEN_LIQUIDITY(liquidity, amount);
        }
    }

As we can see the protocol still relies on underlying.balanceOf() to check if the aave protocol have enough liquidity.

With the 3.1 upgrade the aave protocol, this check has become obsolete as it doesn't reflect the actual withdrawable balance from the protocol, this balance could be instead querried using the new aave Pool.getVirtualUnderlyingBalance()
Please see the main github repo of aave:

https://github.com/aave-dao/aave-v3-origin/blob/3375062ea20a46eccdc55dbc4b68740ad140bf64/src/core/contracts/protocol/pool/Pool.sol#L489C1-L493C4

  function getVirtualUnderlyingBalance(
    address asset
  ) external view virtual override returns (uint128) {
    return _reserves[asset].virtualUnderlyingBalance;
  }

Recomendation

We recommend that the protocol considers using the virtualBalance instead of underlying.balanceOf() to check if the aave protocol have enough liquidity for withdrawals

Assessed type

Invalid Validation

[hansfriese]

QA at most

[c4-judge]

hansfriese marked the issue as not a duplicate

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-a

[aliX40]

Dear judge @hansfriese , Thanks for your work judging the contest! I think this issue should be a valid medium, those are my arguments: I first start by giving more description on the aave upgrade: The whole way how aave does the token accounting has changed, the protocol doesn't use erc20.balanceOf() at all, instead it has changed its implementation to track the accounting using state variable virtualBalance (which can be known in other protocols as internal accounting). After the upgrade, the aave when withdrawing usdc from ausdc would check the virtualBalance and not usdc.balanceOf(aToken), knowing that the virtualBalance would be an approximation of available liquidity and it is first guaranteed to be always bigger than curent usdc ballance, thanks to roudning in fovour of the protocol, and accumulation of unaccounted for tokens recieved through user mistakes and donations.

Secondly using the virtual balance to check for liquidity availablity, adds a lot stability to the size protocol, as erc20.balanceOf() is manipulatable through donations attacks, which could lead to the checks not failing and users with borrowed funds wouldn't be able to withdraw their funds to usdc

Lastly also known that the likelbility is high of this happening and the fact that other issues with exactly the same impact and really similar scenarios and conditions of happening (e.g Sandwich attack on loan fulfillment will temporarily prevent users from accessing their borrowed funds issue#152). I think this should be a medium severity bug

[hansfriese]

I agree it is a good suggestion. However, as noted in the ReadMe, the Size protocol is expected to interact with AAVE v3. Although this can be revised in future development, it is unfair to classify it as a valid Medium risk.