Aave DIVA Wrapper

DIVA

HardhatDeFi

15,000 USDC

View results

Previous Next

Submission Details

Severity: low

Invalid

Missing amount check against the user's balance causes unnecessary reverts in functions `_removeLiquidity`, `_redeemWToken`, `_redeemPositionToken`

avoloder

Summary

Functions _removeLiquidity, _redeemPositionToken and _redeemWToken check if the token amount is equal to type(uint256).max to adjust the amount, but do not check the amount against the maximum withdrawable value which causes unnecessary reverts.

Vulnerability Details

There is a check to see if token_amount == type(uint256).max in these functions, which, if true, adjusts the token_amount to be equal to the user's withdrawable balance. The token_amount is provided by the user, so it is highly unlikely that the user will try to redeem, or remove the liquidity in the value of type(uint256).max. In every relevant function, there is a specific call to check what is the user's balance of a given token, for example in _redeemPositionToken

// Use the user's balance if `_positionTokenAmount` equals `type(uint256).max`.

uint256 _userBalance = _positionTokenContract.balanceOf(msg.sender);

uint256 _positionTokenAmountToRedeem = _positionTokenAmount;

if (_positionTokenAmount == type(uint256).max) {

_positionTokenAmountToRedeem = _userBalance;

}

This will lead to unnecessary reverts when the user tries to transfer more than it has on his/her balance, which is obvious and desired behaviour, but there is more to it. This holds for all three functions, however in the _removeLiquidity there is also a specific case where it is more reasonable to adjust tokenAmount to the user's balance if certain conditions are met. To remove liquidity, users need to send the same amount of long and short position tokens that will be burnt. The part of the function where the user's tokenAmount is adjusted to the balance is:

/**

* @dev See {IAaveDIVAWrapper-removeLiquidity}.

function _removeLiquidity(

bytes32 _poolId,

uint256 _positionTokenAmount,

address _recipient

) internal returns (uint256) {

... SNIPET ...

IERC20Metadata _shortTokenContract = IERC20Metadata(_pool.shortToken);

IERC20Metadata _longTokenContract = IERC20Metadata(_pool.longToken);

IERC20Metadata _collateralTokenContract = IERC20Metadata(_pool.collateralToken);

// Use the user's min short/long token balance if `_positionTokenAmount` equals `type(uint256).max`.

// That corresponds to the maximum amount that the user can remove from the pool.

uint256 _userBalanceShort = _shortTokenContract.balanceOf(msg.sender);

uint256 _userBalanceLong = _longTokenContract.balanceOf(msg.sender);

uint256 _positionTokenAmountToRemove = _positionTokenAmount;

if (_positionTokenAmount == type(uint256).max) {

_positionTokenAmountToRemove = _userBalanceShort > _userBalanceLong ? _userBalanceLong : _userBalanceShort;

}

// Transfer short and long tokens from user to this contract. Requires prior user approval on both tokens.

// No need to use `safeTransferFrom` here as short and long tokens in DIVA Protocol are standard ERC20 tokens

// using OpenZeppelin's ERC20 implementation.

_shortTokenContract.transferFrom(msg.sender /** from */, address(this) /** to */, _positionTokenAmountToRemove);

_longTokenContract.transferFrom(msg.sender /** from */, address(this) /** to */, _positionTokenAmountToRemove);

... SNIPET ...

}

The problem with unnecessary reverts occurs when the user holds both long and short position tokens, but in unequal amounts. For example, if User A has 100 LONG tokens and 50 SHORT tokens, and they try to remove 60 tokens, the function will revert. This happens because it will not be possible to transfer 60 SHORT tokens at line 229. To successfully remove liquidity, the user must either provide exactly 50 as the token amount or specify type(uint256).max. In the latter case, the token amount will be adjusted to match the user's minimum short/long balance. A more convenient use case for the user would be to automatically adjust the token amount to the maximum available withdrawable value if it exceeds either the LONG or SHORT amount, and not only if the user passes type(uint256).max as amount, which is unlikely.

POC

User adds liquidity and adds himself as a long receiver
User gets 100 Long tokens
User adds liquidity again to the same pool, however now as a short receiver
User gets 50 Short tokens
User tries to remove liquidity after some time, and wants to remove 70 tokens
The function reverts with the Insufficient balance

Impact

Low

Tools Used

Manual Review

Recommendations

Instead of checking whether the token amount provided in the arguments is equal to type(uint256).max in order to adjust it, first retrieve the user's minimum balance (which represents the maximum withdrawable amount in this case) and compare it with the token amount. If the token amount exceeds the maximum withdrawable balance, set the token amount to the minimum balance and proceed with removing the liquidity, rather than reverting.

So the function _removeLiquidity() would look like this: