Summary

The AaveDIVAWrapperCore contract is designed to interact with the DIVA Protocol and Aave V3, enabling users to create, manage, and remove liquidity from contingent pools using wrapped tokens (wTokens) as proxy collateral. During the audit, a critical vulnerability was identified in the _removeLiquidity function, where the return values of transferFrom calls are ignored. Although the contract uses SafeERC20 for IERC20Metadata, the transferFrom calls do not handle potential failures, which could lead to silent failures and potential exploitation.

Vulnerability Details

Unchecked Return Values for transferFrom

In the _removeLiquidity function, the contract transfers short and long position tokens from the user to the contract using transferFrom:

The transferFrom function in ERC20 tokens returns a boolean value indicating whether the transfer was successful. However, the contract does not check this return value. If the transfer fails (e.g., due to insufficient allowance or balance), the contract will continue execution as if the transfer succeeded, leading to incorrect state changes and potential vulnerabilities.

The comment in the code states:

While this assumption may hold true for standard OpenZeppelin ERC20 implementations, it is not a safe practice to ignore the return value of transferFrom. Non-standard ERC20 tokens or future changes to the token implementation could result in silent failures.

Impact

• High Severity: Ignoring the return value of transferFrom can result in silent failures, where the contract assumes tokens have been transferred even if the transfer failed. This can allow users to remove liquidity without providing the required position tokens, leading to inconsistencies in the pool's state.

• Potential Exploit: An attacker could exploit this vulnerability to remove liquidity without transferring the required tokens, potentially draining funds from the contract or causing financial losses to other users.

Tools Used

• Manual Code Review

Recommendations

1. Explicitly Check transferFrom Return Values:
   To ensure that the transfer is successful, explicitly check the return value of transferFrom and revert the transaction if the transfer fails. This approach provides clear feedback and prevents silent failures.

   Update the transferFrom calls as follows:

   if (!_shortTokenContract.transferFrom(

   msg.sender /** from */,

   address(this) /** to */,

   _positionTokenAmountToRemove

   )) {

   revert ShortTokenTransferFailed();

   }

   ​

   if (!_longTokenContract.transferFrom(

   msg.sender /** from */,

   address(this) /** to */,

   _positionTokenAmountToRemove

   )) {

   revert LongTokenTransferFailed();

   }

   Define custom errors for these reverts:

   error ShortTokenTransferFailed();

   error LongTokenTransferFailed();

2. Remove Assumptions About Token Behavior:
   Avoid making assumptions about the behavior of external tokens. Even if the tokens are currently standard ERC20 implementations, future changes or non-standard tokens could introduce unexpected behavior.