Aave DIVA Wrapper

Summary

The _removeLiquidity function in `AaveDIVAWrapperCore` contract contains an issue related to insufficient validation of user balances (_userBalanceShort and _userBalanceLong) when _positionTokenAmount equals type(uint256).max. This can lead to silent failures, incorrect behavior, or unintended consequences, as zero balances are not explicitly handled before assigning a value to _positionTokenAmountToRemove.

https://github.com/Cyfrin/2025-01-diva/blob/main/contracts/src/AaveDIVAWrapperCore.sol#L223

Vulnerability Details

Issue

When _positionTokenAmount is set to type(uint256).max, the function calculates _positionTokenAmountToRemove as the lesser of _userBalanceShort and _userBalanceLong. However, if both _userBalanceShort and _userBalanceLong are 0, _positionTokenAmountToRemove is also assigned 0 without proper validation.

Root Cause

The absence of validation for non-zero user balances before assigning _positionTokenAmountToRemove introduces the following issues:

1. Silent Failure:

   • If both balances are zero, _positionTokenAmountToRemove becomes 0, and the subsequent token transfer operations (via transferFrom) are effectively skipped. However, this does not revert the transaction, leaving the user unaware of the failure.

2. Unexpected Behavior:

   • The function proceeds with a zero _positionTokenAmountToRemove, which may affect downstream logic, such as incorrectly interacting with the DIVA Protocol or _redeemWTokenPrivate.

3. Potential Exploit Vector:

   • While no immediate exploit is evident in this specific implementation, downstream logic assuming _positionTokenAmountToRemove > 0 could lead to unintended consequences.

Impact

1. User Experience:

   • Users with zero balances for both tokens may call this function, expecting a meaningful result, but the function silently completes without reverting or transferring any tokens.

2. State Integrity:

   • Although unlikely, if downstream logic does not properly handle zero _positionTokenAmountToRemove, it could lead to unintended contract state changes or incorrect outcomes.

3. Gas Wastage:

   • Users may incur unnecessary gas costs for a transaction that achieves no meaningful result due to unvalidated zero balances.

Tools Used

1. Manual Code Review

2. Logical Testing and Scenario Analysis

Recommendations

1. Add Validation for Non-Zero Balances:

   • Before assigning _positionTokenAmountToRemove, ensure that the user's balances for both _shortTokenContract and _longTokenContract are greater than 0:

1. Revert if _positionTokenAmountToRemove is 0:

• After determining _positionTokenAmountToRemove, validate that it is greater than 0 to ensure meaningful execution:

1. Early Exit on Zero Input Balances:

• Add an early check for zero balances before proceeding to avoid unnecessary gas consumption:

1. Enhance User Feedback:

• Provide clear error messages when a user attempts to call the function with insufficient balances to improve user experience.