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

[H-1] Depositing any fee on transfer (FoT) token into the Treasury will make the Treasury::getTotalValue() and Treasury::getBalance(FoT address) have wrong values

joro

Summary

Treasury::deposit function allows anyone to deposit a fee on transfer or rebasing token into the Treasury and badly influence the Treasury::_totalValue as well as the particular balance of the deposited FoT or rebasing token.

Vulnerability Details

When you're interacting with standard ERC20 tokens, the transferFrom function transfers the exact amount specified from the sender to the contract. However, fee-on-transfer tokens deduct a fee during the transfer process, meaning the actual amount received by the contract will be less than the amount specified in the transferFrom call. In this code from the Treasury::deposit function:

IERC20(token).transferFrom(msg.sender, address(this), amount);
_balances[token] += amount;
_totalValue += amount;

_balances[token] and _totalValue are incremented by the full amount, even though the contract received less due to the fee. This leads to inaccurate internal accounting.

Impact

  • Impact: High, as it makes important variables like Treasury::_totalValue and Treasury::_balances have wrong values

  • Likelihood: High, since every time a FoT (or rebasing) token is deposited into the treasury the variables become more disconnected from reality

PoC

  1. Create a Mock token to replicate fee on transfer behaviour, add this code into /contracts/mocks/core/tokens:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract MockFeeOnTransferToken is ERC20 {
uint256 public feeBasisPoints; // Fee in basis points (e.g., 100 = 1%)
​
constructor(
string memory name,
string memory symbol,
uint256 initialSupply,
uint256 _feeBasisPoints
) ERC20(name, symbol) {
feeBasisPoints = _feeBasisPoints;
_mint(msg.sender, initialSupply);
}
​
function mint(address account, uint256 amount) external {
_mint(account, amount);
}
​
function _update(
address sender,
address recipient,
uint256 amount
) internal virtual override {
if (sender == address(0)) {
// Minting: no fee
super._update(sender, recipient, amount);
} else if (recipient == address(0)) {
// Burning: no fee
super._update(sender, recipient, amount);
} else {
// Transfer: apply fee
uint256 fee = (amount * feeBasisPoints) / 10000;
uint256 amountAfterFee = amount - fee;
​
// Transfer the net amount to recipient
super._update(sender, recipient, amountAfterFee);
​
// Burn the fee by transferring to address(0)
super._update(sender, address(0), fee);
}
}
}

2 . Add this to the beforeEach of /test/unit/core/collectors/Treasury.test.js in order to have the Mock fee token deployed and ready for the next step

// Deploy fee mock token for PoC
const FeeMockToken = await ethers.getContractFactory("MockFeeOnTransferToken");
feeToken = await FeeMockToken.deploy("Test Fee Token", "Fee", ethers.parseEther("1000000"), 100);
await feeToken.waitForDeployment();
await feeToken.mint(user1.address, ethers.parseEther("1000"));
await feeToken.mint(user2.address, ethers.parseEther("1000"));

3 . To validate the internal accounting is wrong for such tokens - add this test into /test/unit/core/collectors/Treasury.test.js under the Deposits describe section:

it("should demonstrate miscalculation of internal balances with fee-on-transfer tokens", async function () {
const depositAmount = ethers.parseEther("1000");
​
// Approve the treasury to spend user1's tokens
await feeToken.connect(user1).approve(treasury.getAddress(), depositAmount);
​
// Perform the deposit
await treasury.connect(user1).deposit(feeToken.getAddress(), depositAmount);
​
// Calculate the actual received amount (1% fee deducted) (using bigint math)
const fee = depositAmount * 1n / 100n; // 1% fee (1000 * 0.01 = 10 tokens)
const expectedReceived = depositAmount - fee; // 990 tokens
​
// Check actual contract balance after deposit
const actualTreasuryBalance = await feeToken.balanceOf(treasury.getAddress());
​
// Verify the contract received less due to the fee
expect(actualTreasuryBalance).to.equal(expectedReceived);
​
// Check internal accounting (which should be incorrect if not handling fee-on-transfer correctly)
const storedBalance = await treasury.getBalance(feeToken.getAddress());
const totalValue = await treasury.getTotalValue();
​
// Assert that the internal accounting incorrectly matches the full deposit amount instead of the received amount
expect(storedBalance).to.not.equal(actualTreasuryBalance);
expect(totalValue).to.not.equal(actualTreasuryBalance);
});

Tools Used

  1. Manual Review

Recommendations

To ensure _totalValue and _balances[token] reflects the actual amount received by the contract, you can compare the contract's balance before and after the transfer:

uint256 balanceBefore = IERC20(token).balanceOf(address(this));
IERC20(token).transferFrom(msg.sender, address(this), amount);
uint256 balanceAfter = IERC20(token).balanceOf(address(this));
​
uint256 receivedAmount = balanceAfter - balanceBefore;
_balances[token] += receivedAmount;
_totalValue += receivedAmount;
​
emit Deposited(token, receivedAmount);
Updates

Lead Judging Commences

inallhonesty Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Treasury::deposit increments _totalValue regardless of the token, be it malicious, different decimals, FoT etc.

Treasury::deposit increments _balances[token] with amount, not taking FoT or rebasing into account

Support

FAQs

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