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

Discrepancy with Fee-on-Transfer Tokens

TxGuard

Description

The Treasury contract incorrectly assumes that the full amount specified in deposit (via IERC20.transferFrom) is received by the contract. For tokens with transfer fees (e.g., a 1% fee where transferring 100 tokens delivers only 99), the contract receives less than the requested amount but records the full amount in _balances[token] and _totalValue. This creates a persistent mismatch between the contract’s internal state and its actual token balance.

Affected Code

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);
}
​
​

Root Cause

  • The contract does not verify the actual amount of tokens received after calling transferFrom.

  • Fee-on-transfer tokens deduct a percentage of the transfer amount (sent to a fee collector, burned, or redistributed), reducing the amount delivered to the contract.

Reproduction Steps

  1. Deploy a fee-on-transfer token (e.g., Token X with a 1% fee).

  2. Approve the Treasury contract to spend 100 Token X (100 * 10^18 units).

  3. Call deposit(TokenX, 100 * 10^18).

  4. Observe:

    • Contract receives 99 * 10^18 Token X (after 1% fee).

    • _balances[TokenX] records 100 * 10^18.

  5. Attempt withdraw(TokenX, 100 * 10^18, recipient):

    • Check passes (_balances[TokenX] >= 100 * 10^18).

    • Transfer fails (actual balance is only 99 * 10^18).

Severity

  • Likelihood: Medium (depends on use of fee-on-transfer tokens, which are less common but exist in the ecosystem).

  • Impact: High (prevents withdrawals, misrepresents funds, disrupts operations).

  • Overall: High.

Affected Components

  • _balances[token]: Overstates the token balance.

  • _totalValue: Overstates the total value held.

  • withdraw: Fails when attempting to transfer overstated amounts.

  • View functions (getBalance, getTotalValue): Return inaccurate data.

Impact Analysis

Operational Impact

  • Withdrawal Failures: Managers cannot withdraw the full recorded balance, halting fund distribution even when _balances[token] suggests sufficient funds.

  • Phantom Funds: The contract reports fictitious balances that cannot be accessed, accumulating with each deposit of fee-on-transfer tokens.

  • Allocation Misalignment: If allocations are based on reported balances, they may exceed actual holdings, risking insolvency.

Financial Impact

  • Stuck Funds: Small residual amounts (e.g., fee differences) remain unwithdrawable, tying up value indefinitely.

  • Trust Erosion: Users or external systems relying on getBalance or getTotalValue may make decisions based on inflated figures, leading to financial miscalculations.

Example Scenario

  • Initial State: Treasury holds 0 Token X.

  • Deposit: User deposits 100 Token X (1% fee).

    • Actual: 99 Token X received.

    • Recorded: _balances[TokenX] = 100.

  • Second Deposit: Another 100 Token X.

    • Actual: 198 Token X total.

    • Recorded: _balances[TokenX] = 200.

  • Withdrawal Attempt: Manager tries to withdraw 200 Token X.

    • Fails (only 198 available).

  • Partial Withdrawal: Withdraws 198 Token X.

    • Actual: 0 Token X left.

    • Recorded: _balances[TokenX] = 2 (phantom balance).

PoC

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import {Test, console} from "forge-std/Test.sol";
import {Treasury} from "../contracts/core/collectors/Treasury.sol";
import {MockERC20} from "../test/mocks/MockERC20.sol";
import {MockFeeToken} from "../test/mocks/MockFeeToken.sol";
​
contract TreasuryTest is Test {
Treasury public treasury;
MockERC20 public token;
MockERC20 public token2;
​
address admin = makeAddr("admin");
address manager = makeAddr("manager");
address allocator = makeAddr("allocator");
address allocator2 = makeAddr("allocator2");
address user = makeAddr("user");
address recipient = makeAddr("recipient");
​
uint256 constant INITIAL_BALANCE = 1000e18;
​
event FundsAllocated(address indexed recipient, uint256 amount);
event Deposited(address indexed token, uint256 amount);
event Withdrawn(
address indexed token,
uint256 amount,
address indexed recipient
);
​
function setUp() public {
// Deploy contracts
treasury = new Treasury(admin);
token = new MockERC20();
token2 = new MockERC20();
token2.setDecimals(6);
​
// Setup roles
vm.startPrank(admin);
treasury.grantRole(treasury.MANAGER_ROLE(), manager);
treasury.grantRole(treasury.ALLOCATOR_ROLE(), allocator);
treasury.grantRole(treasury.ALLOCATOR_ROLE(), allocator2);
vm.stopPrank();
​
// Setup initial token balances
token.mint(user, INITIAL_BALANCE);
token2.mint(user, 1000e6);
}
​
function testFeeOnTransferTokenVulnerability() public {
// Deploy our fee token
MockFeeToken feeToken = new MockFeeToken();
// Setup initial balance for user (1000 tokens)
uint256 depositAmount = 1000e18;
feeToken.mint(user, depositAmount);
vm.startPrank(user);
// Approve and deposit tokens
feeToken.approve(address(treasury), depositAmount);
// Log balances before deposit
console.log("User balance before deposit:", feeToken.balanceOf(user) / 1e18);
console.log("Treasury balance before deposit:", feeToken.balanceOf(address(treasury)) / 1e18);
// Perform deposit
treasury.deposit(address(feeToken), depositAmount);
// Calculate expected received amount (99% of deposit due to 1% fee)
uint256 expectedReceivedAmount = (depositAmount * 99) / 100;
// Log actual vs recorded balances
console.log("User balance after deposit:", feeToken.balanceOf(user) / 1e18);
console.log("Treasury actual balance:", feeToken.balanceOf(address(treasury)) / 1e18);
console.log("Treasury recorded balance:", treasury.getBalance(address(feeToken)) / 1e18);
vm.stopPrank();
// Verify the discrepancy
assertEq(
treasury.getBalance(address(feeToken)),
depositAmount,
"Treasury recorded balance should match deposit amount"
);
assertEq(
feeToken.balanceOf(address(treasury)),
expectedReceivedAmount,
"Treasury actual balance should be 99% of deposit"
);
// Try to withdraw the full recorded balance
vm.startPrank(manager);
vm.expectRevert(); // Should revert as trying to withdraw more than actual balance
treasury.withdraw(address(feeToken), depositAmount, recipient);
// Try to withdraw the actual balance
treasury.withdraw(address(feeToken), expectedReceivedAmount, recipient);
// Verify final state
assertEq(
feeToken.balanceOf(recipient),
(expectedReceivedAmount * 99) / 100, // Another 1% fee on withdrawal
"Recipient should receive 99% of the actual balance"
);
assertEq(
treasury.getBalance(address(feeToken)),
depositAmount - expectedReceivedAmount,
"Treasury recorded balance should be reduced by withdrawal amount"
);
assertEq(
feeToken.balanceOf(address(treasury)),
0,
"Treasury actual balance should be 0"
);
vm.stopPrank();
}
}
​
​

MockFeeToken:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
​
contract MockFeeToken is IERC20 {
string public constant name = "Mock Fee Token";
string public constant symbol = "FEE";
uint8 public constant decimals = 18;
uint256 public constant FEE_DENOMINATOR = 100;
uint256 public constant FEE_NUMERATOR = 1; // 1% fee
​
mapping(address => uint256) private _balances;
mapping(address => mapping(address => uint256)) private _allowances;
uint256 private _totalSupply;
​
function totalSupply() external view override returns (uint256) {
return _totalSupply;
}
​
function balanceOf(address account) external view override returns (uint256) {
return _balances[account];
}
​
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;
emit Approval(msg.sender, spender, amount);
return true;
}
​
function transfer(address to, uint256 amount) external override returns (bool) {
return _transfer(msg.sender, to, amount);
}
​
function transferFrom(address from, address to, uint256 amount) external override returns (bool) {
require(_allowances[from][msg.sender] >= amount, "ERC20: insufficient allowance");
_allowances[from][msg.sender] -= amount;
return _transfer(from, to, amount);
}
​
function mint(address to, uint256 amount) external {
_totalSupply += amount;
_balances[to] += amount;
emit Transfer(address(0), to, amount);
}
​
function _transfer(address from, address to, uint256 amount) internal returns (bool) {
require(from != address(0), "ERC20: transfer from the zero address");
require(to != address(0), "ERC20: transfer to the zero address");
require(_balances[from] >= amount, "ERC20: transfer amount exceeds balance");
​
uint256 fee = (amount * FEE_NUMERATOR) / FEE_DENOMINATOR;
uint256 actualAmount = amount - fee;
​
// Deduct full amount from sender
_balances[from] -= amount;
// Add actual amount (minus fee) to recipient
_balances[to] += actualAmount;
// Burn the fee by reducing total supply
_totalSupply -= fee;
​
emit Transfer(from, to, actualAmount);
emit Transfer(from, address(0), fee); // Emit transfer to zero address for fee burn
return true;
}
}
​

MockERC20:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract MockERC20 is ERC20 {
uint8 private _decimals = 18;
​
constructor() ERC20("Mock Token", "MOCK") {}
​
function mint(address to, uint256 amount) external {
_mint(to, amount);
}
​
function setDecimals(uint8 decimals_) external {
_decimals = decimals_;
}
​
function decimals() public view override returns (uint8) {
return _decimals;
}
​
function approve(address spender, uint256 amount) public virtual override returns (bool) {
_approve(_msgSender(), spender, amount);
return true;
}
}
​

Root Cause Analysis

The flaw stems from a design assumption that all ERC-20 tokens follow a standard where transferFrom(amount) delivers exactly amount to the recipient. While true for most tokens, fee-on-transfer tokens violate this by delivering less than the requested amount, and the contract does not account for this discrepancy.

Recommendations

Proposed Fix

Modify the deposit function to track the actual amount received by measuring the contract’s balance before and after the transfer:

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

Key Changes

  • Balance Check: Use balanceOf to calculate the exact amount received.

  • Accurate Updates: Adjust _balances and _totalValue with actualAmount instead of the requested amount.

  • Event Adjustment: Emit the actual amount deposited, ensuring transparency.

Considerations

  • Gas Cost: Adds two external calls (balanceOf), increasing gas usage slightly but ensuring correctness.

  • Edge Case: If a token’s balanceOf is manipulated (e.g., via self-destruct sending ETH or unexpected token transfers), the fix remains robust as it relies on the delta, not absolute values.

  • Withdrawals: No change needed, as transfer will revert if the actual balance is insufficient, aligning with the updated _balances.

Alternative Approaches

  1. Token Whitelist: Restrict deposits to a curated list of standard ERC-20 tokens without fees. (Limits flexibility.)

  2. SafeERC20: Use OpenZeppelin’s SafeERC20 library with safeTransferFrom to handle transfer quirks, though it doesn’t inherently fix fee discrepancies.

Additional Improvements

  • Documentation: Add a note in the contract warning about fee-on-transfer tokens and the implemented safeguard.

  • Testing: Include unit tests with a mock fee-on-transfer token to verify the fix.

Validation

Pre-Conditions

  • Token X with 1% fee: Transferring 100 units delivers 99 units.

  • Treasury deployed with initial balances at 0.

Post-Fix Behavior

  • Deposit 100 Token X:

    • Before: 0 Token X.

    • After: 99 Token X received.

    • Recorded: _balances[TokenX] = 99, _totalValue = 99.

  • Withdraw 99 Token X:

    • Succeeds, leaving _balances[TokenX] = 0.

  • Withdraw 100 Token X:

    • Reverts due to insufficient balance, as expected.

The fix ensures recorded balances match actual holdings, resolving the issue.

Updates

Lead Judging Commences

inallhonesty Lead Judge 4 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.