superform-xyz/super-vaults

Arithmetic overflow bug in `maxDeposit` and `maxMint` functions in Aave V3 wrapper

Closed this issue · 1 comments

duckception commented

Hey, I discovered a bug in your AaveV3ERC4626Reinvest.sol contract that breaks the ERC-4626 standard, which specifies that the maxDeposit and maxMint functions must never revert. Those functions, however, are reverting in certain situations.

The issue at hand is that the wrapper does not take into account an edge case when a supply cap for an asset changes. If the supply cap is changed to a value lower than the current supply of the token in the pool, then the math breaks in the maxDeposit and maxMint functions resulting in a revert due to overflow.

These lines are to blame:

super-vaults/src/aave-v3/AaveV3ERC4626Reinvest.sol

Line 359 in afd00e0

return supplyCap - aToken.totalSupply();

super-vaults/src/aave-v3/AaveV3ERC4626Reinvest.sol

Line 384 in afd00e0

return convertToShares(supplyCap - aToken.totalSupply());

This situation occurred in your BTC wrapper on Avalanche chain, which rendered said functions unusable for around 12 days. This is precisely what happened:

  • The supply cap of BTC was set to 5800 BTC.
  • Total supply of BTC in the pool was around 4600 BTC when the supply cap for this asset was changed to 3500 BTC (see tx).
  • This change made the maxDeposit and maxMint functions to revert due to the math issue: (supplyCap - aToken.totalSupply()) -> (3500e8 BTC - 4600e8 BTC) -> OVERFLOW -> Revert.
  • These functions were unusable until enough BTC were withdrawn from the pool to make the condition supplyCap >= aToken.totalSupply() fulfilled, which happened around 12 days later.

I propose to add the following check just before the final return statements in the maxDeposit and maxMint functions to prevent any overflows in the future:

if (aToken.totalSupply() >= supplyCap)
    return 0;
shortdoom commented

Thank you! Another repository affected is yield-daddy from which some of super-vaults is forked.

I created a PR with your proposed fix. It makes sense. EIP doc disallows revert explicitly. And returning 0 in place of overflowing is fulfilling intended function of maxDeposit/Mint.

  • 👍1