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

AaveDIVAWrapperCore Smart Contract Vulnerability: Silent Failure in Liquidity Removal

sidanaarmaan2003

AaveDIVAWrapperCore Vulnerability PoC and Report

Summary

The contract AaveDIVAWrapperCore._removeLiquidity in its current implementation ignores the return value of the transferFrom function call. This could lead to unintended behavior if the transferFrom fails silently. The issue arises from the assumption that transferFrom will always succeed, but since it returns a boolean indicating success, ignoring this return value could allow the contract to proceed with invalid or incomplete state changes.

Vulnerability Details

  • Vulnerable Contract: AaveDIVAWrapperCore

  • Function: _removeLiquidity

  • Line of Occurrence: Line 229 in AaveDIVAWrapperCore.sol

  • Description: The contract calls transferFrom on the _shortTokenContract token without verifying the success of the call. This could lead to a scenario where liquidity removal is not executed correctly, but the function continues to execute without reversion or error, leading to inconsistent contract state.

// Vulnerable code snippet
bool success = _shortTokenContract.transferFrom(msg.sender, address(this), _positionTokenAmountToRemove);

Impact

This vulnerability could result in the following potential impacts:

  • Inconsistent State: Liquidity might not be removed, but the contract continues its execution, leading to a mismatch between the state of the contract and the expected behavior.

  • Loss of Tokens: If the transferFrom fails silently and liquidity is not removed, the contract might hold the tokens indefinitely without proper liquidity management.

  • Security Risk: Attackers could exploit this by passing an amount that exceeds the available balance, causing the contract to fail silently and execute unintended logic.

Tools Used

  • Hardhat: Used to write and run the Solidity tests.

  • OpenZeppelin Contracts: Provides ERC20 implementations and other utilities.

  • Solidity: Used to write the smart contracts and PoC test.

Recommendations

  • Always Check Return Values: Ensure that the return value of critical functions such as transferFrom is checked. If it returns false, revert the transaction.

  • Use Safe Methods: Prefer safeTransferFrom or similar wrappers that ensure the transaction is successful before proceeding.

  • Improve Error Handling: Implement appropriate error handling and reverts in functions that depend on external contract calls to prevent silent failures.

Proof of Concept (PoC)

Solidity Test Suite

This PoC demonstrates the issue with AaveDIVAWrapperCore._removeLiquidity by simulating the failure of transferFrom and observing the results. The test suite uses Hardhat for testing.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
import "hardhat/console.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/utils/math/SafeMath.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
​
contract MockToken is IERC20 {
using SafeMath for uint256;
​
string public name = "MockToken";
string public symbol = "MTK";
uint8 public decimals = 18;
uint256 private _totalSupply;
mapping(address => uint256) private _balances;
mapping(address => mapping(address => uint256)) private _allowances;
​
constructor(uint256 totalSupply_) {
_totalSupply = totalSupply_;
_balances[msg.sender] = totalSupply_;
}
​
function totalSupply() external view override returns (uint256) {
return _totalSupply;
}
​
function balanceOf(address account) external view override returns (uint256) {
return _balances[account];
}
​
function transfer(address recipient, uint256 amount) external override returns (bool) {
_transfer(msg.sender, recipient, amount);
return true;
}
​
function allowance(address owner, address spender) external view override returns (uint256) {
return _allowances[owner][spender];
}
​
function approve(address spender, uint256 amount) external override returns (bool) {
_allowances[msg.sender][spender] = amount;
return true;
}
​
function transferFrom(address sender, address recipient, uint256 amount) external override returns (bool) {
uint256 allowedAmount = _allowances[sender][msg.sender];
require(allowedAmount >= amount, "ERC20: transfer amount exceeds allowance");
​
// Simulate transfer failure if the amount is too large
if (amount > 1000 * 10**18) {
return false; // Simulate failure on large transfers
}
​
_transfer(sender, recipient, amount);
_allowances[sender][msg.sender] = allowedAmount.sub(amount);
return true;
}
​
function _transfer(address sender, address recipient, uint256 amount) internal {
require(sender != address(0), "ERC20: transfer from the zero address");
require(recipient != address(0), "ERC20: transfer to the zero address");
​
uint256 senderBalance = _balances[sender];
require(senderBalance >= amount, "ERC20: transfer amount exceeds balance");
​
_balances[sender] = senderBalance.sub(amount);
_balances[recipient] = _balances[recipient].add(amount);
​
emit Transfer(sender, recipient, amount);
}
​
event Transfer(address indexed from, address indexed to, uint256 value);
}
​
contract AaveDIVAWrapperCore is Ownable {
IERC20 private _shortTokenContract;
​
constructor(address shortTokenAddress) {
_shortTokenContract = IERC20(shortTokenAddress);
}
​
function _removeLiquidity(uint256 _positionTokenAmountToRemove) external {
bool success = _shortTokenContract.transferFrom(msg.sender, address(this), _positionTokenAmountToRemove);
​
// Vulnerable code: Ignore return value of transferFrom
console.log("Liquidity Removed!");
}
​
function getShortTokenAddress() external view returns (address) {
return address(_shortTokenContract);
}
}
​
contract TestAaveDIVAWrapperCore {
AaveDIVAWrapperCore private wrapper;
MockToken private mockToken;
address private user;
​
constructor() {
user = address(0x1234567890abcdef1234567890abcdef12345678); // User's address
}
​
function beforeEach() public {
mockToken = new MockToken(10000 * 10**18); // 10000 tokens with 18 decimals
wrapper = new AaveDIVAWrapperCore(address(mockToken));
​
// Transfer tokens to user for testing
mockToken.transfer(user, 1000 * 10**18);
}
​
function testRemoveLiquidityWithoutRevert() public {
uint256 amountToRemove = 2000 * 10**18; // Simulate a failure as user does not have enough allowance
mockToken.approve(address(wrapper), amountToRemove);
​
// Call _removeLiquidity (which will fail due to large amount)
try wrapper._removeLiquidity(amountToRemove) {
assert(false, "Expected failure due to insufficient allowance or failure in transferFrom");
} catch Error(string memory reason) {
assert(keccak256(bytes(reason)) == keccak256(bytes("ERC20: transfer amount exceeds allowance")));
}
}
​
function testRemoveLiquidityWithFailure() public {
uint256 amountToRemove = 2000 * 10**18; // This amount is intentionally too large to trigger failure in transferFrom
mockToken.approve(address(wrapper), amountToRemove);
​
// Call _removeLiquidity (which should fail silently as the return value of transferFrom is ignored)
wrapper._removeLiquidity(amountToRemove);
​
uint256 contractBalanceAfter = mockToken.balanceOf(address(wrapper));
uint256 userBalanceAfter = mockToken.balanceOf(user);
​
// Ensure liquidity has not been transferred to the contract due to the failure
assert(contractBalanceAfter == 0, "Liquidity should not be transferred due to transferFrom failure");
assert(userBalanceAfter == 1000 * 10**18, "User balance should not change due to failed transfer");
}
}
Updates

Lead Judging Commences

bube Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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