Aave DIVA Wrapper
DIVA
HardhatDeFi
15,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

DoS in removeLiquidity when token recipients differ

holydevoti0n

Summary

Users will be unable to remove liquidity if the long and short position token holders are different addresses, effectively locking their funds in the contract.

Vulnerability Details

The protocol fully supports sending tokens to different addresses - it's actually a key feature since you might want to give the long position to one party and the short to another.

But here's where it breaks: When trying to remove liquidity, the wrapper assumes both tokens are held by the same address:

https://github.com/Cyfrin/2025-01-diva/blob/1b6543768c341c2334cdff87b6dd627ee2f62c89/contracts/src/AaveDIVAWrapperCore.sol#L213-L230

function _removeLiquidity(
bytes32 _poolId,
uint256 _positionTokenAmount,
address _recipient
) internal returns (uint256) {
// ...
// Gets long and short balances from msg.sender only
uint256 _userBalanceShort = _shortTokenContract.balanceOf(msg.sender);
uint256 _userBalanceLong = _longTokenContract.balanceOf(msg.sender);
// ...
// @audit-issue: Both transfers are from msg.sender
_shortTokenContract.transferFrom(msg.sender, address(this), _positionTokenAmountToRemove);
_longTokenContract.transferFrom(msg.sender, address(this), _positionTokenAmountToRemove);
}

The issue arises because:

  1. The function assumes both long and short tokens are held by msg.sender

  2. The transferFrom calls both use msg.sender as the source

  3. There's no way to specify different addresses for long and short-token holders

The root cause is that the wrapper contract forces both position tokens to come from the same address (msg.sender), while DIVA protocol itself supports adding liquidity for different addresses:

https://github.com/Cyfrin/2025-01-diva/blob/1b6543768c341c2334cdff87b6dd627ee2f62c89/contracts/src/AaveDIVAWrapperCore.sol#L167-L172

function _addLiquidity(
bytes32 _poolId,
uint256 _collateralAmount,
@> address _longRecipient, // @audit long and short address can differ
@> address _shortRecipient
) internal {
...
}

So you can split the tokens on creation/add, but you can't remove liquidity if you do. The wrapper forces both transfers to come from msg.sender, with no way to specify different addresses.

Impact

Users can't remove liquidity if long and short tokens are held by different addresses.

Tools Used

Manual Review and DIVA contract.

https://github.com/divaprotocol/diva-protocol-v1/blob/1263828cd5c5b2192e876edef90444448e66176d/contracts/facets/LiquidityFacet.sol

Recommendations

Add support for different token holders in removeLiquidity:

function removeLiquidity(
bytes32 _poolId,
uint256 _positionTokenAmount,
address _longTokenHolder, // New param
address _shortTokenHolder, // New param
address _recipient
) external returns (uint256) {
_shortTokenContract.transferFrom(_shortTokenHolder, address(this), amount);
_longTokenContract.transferFrom(_longTokenHolder, address(this), amount);
// ... rest of logic
}
Updates

Lead Judging Commences

bube Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[Invalid] Inconsistent recipient addresses

The `addLiquidity` allows the short and long recipients to be different addresses. Then if a given user has only one of the position tokens, he calls `redeemPositionToken` to redeem position token amount, if this user has amount of the both token types, he can call `removeLiquidity` and in that way an equal amount of short and long tokens will be burned.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.