The fee-on-transfer bug that locks funds forever (with a working PoC)

#security #solidity #web3 #smartcontracts

Last week I found this pattern in a real audit. I can't name the protocol — it's under
responsible-disclosure until the program confirms a fix — but the bug itself is a generic pattern
that shows up constantly, and it's worth understanding because most scanners miss it entirely.

The setup

Any contract that does this is at risk:

function topUp(address token, uint256 amount) external {
    IERC20(token).transferFrom(msg.sender, address(this), amount);
    accounted[token] += amount;   // <- the bug is right here
}

Looks fine, right? transferFrom moves amount tokens, and you record amount in your internal
ledger. Standard accounting.

Except: transferFrom doesn't guarantee the contract received amount.

If token charges a transfer fee (fee-on-transfer tokens, deflationary tokens, reflection tokens —
all real, all still in production on mainnet today), the contract's actual balance goes up by
amount - fee, while accounted[token] goes up by the full amount. Your internal books just
started lying to you.

Why it matters

The mismatch sits quietly until someone tries to pay out based on the accounted value:

function withdraw(address token, address to) external {
    uint256 owed = accounted[token];
    accounted[token] = 0;
    IERC20(token).transfer(to, owed);   // reverts: contract doesn't have `owed` tokens
}

withdraw reverts. Every time. Forever. The contract's real balance is permanently less than what
it thinks it owes, so the transfer call always fails with "transfer amount exceeds balance." There's
no retry that fixes it — the accounted number never shrinks to match reality on its own.

Result: the deposited funds are stuck in the contract with no recovery path. Not "hard to get
back" — actually, structurally, permanently unreachable.

Proof, not theory

I don't like reporting "this looks vulnerable" without running it. Here's a self-contained Foundry
PoC — a minimal contract reproducing the exact pattern, plus a 5% fee-on-transfer token mock:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import { Test, console2 } from "forge-std/Test.sol";

contract FeeOnTransferToken {
    mapping(address => uint256) public balanceOf;
    mapping(address => mapping(address => uint256)) public allowance;
    uint256 constant FEE_BPS = 500; // 5%

    constructor(uint256 supply) { balanceOf[msg.sender] = supply; }
    function approve(address s, uint256 a) external returns (bool) { allowance[msg.sender][s] = a; return true; }
    function transferFrom(address from, address to, uint256 amount) external returns (bool) {
        allowance[from][msg.sender] -= amount;
        uint256 fee = (amount * FEE_BPS) / 10_000;
        balanceOf[from] -= amount;
        balanceOf[to] += amount - fee;   // receiver gets LESS than `amount`
        return true;
    }
}

contract VulnerableVault {
    mapping(address => uint256) public accounted;
    function topUp(address token, uint256 amount) external {
        FeeOnTransferToken(token).transferFrom(msg.sender, address(this), amount);
        accounted[token] += amount;
    }
    function withdraw(address token, address to) external {
        uint256 owed = accounted[token];
        accounted[token] = 0;
        require(FeeOnTransferToken(token).balanceOf(address(this)) >= owed, "insufficient");
        // (real code would use safeTransfer here — omitted, same failure either way)
    }
}

contract FeeOnTransferPoC is Test {
    function test_FundsLockedForever() public {
        VulnerableVault vault = new VulnerableVault();
        FeeOnTransferToken token = new FeeOnTransferToken(1_000_000e18);

        token.approve(address(vault), 1_000e18);
        vault.topUp(address(token), 1_000e18);

        uint256 accounted = vault.accounted(address(token));
        uint256 realBalance = token.balanceOf(address(vault));

        console2.log("accounted:", accounted);      // 1000e18 — what the vault THINKS it has
        console2.log("real balance:", realBalance);  // 950e18  — what it ACTUALLY has

        assertEq(accounted, 1_000e18);
        assertEq(realBalance, 950e18);

        vm.expectRevert("insufficient");
        vault.withdraw(address(token), address(this)); // always reverts, forever
    }
}

Run it:

[PASS] test_FundsLockedForever() (gas: 87211)
Logs:
  accounted:    1000000000000000000000
  real balance: 950000000000000000000

1000 requested, 950 actually received, and the vault will try to pay out 1000 every single time —
forever failing.

The fix

Never trust the parameter you passed to transferFrom. Measure what actually arrived:

function topUp(address token, uint256 amount) external {
    uint256 before = IERC20(token).balanceOf(address(this));
    IERC20(token).transferFrom(msg.sender, address(this), amount);
    uint256 received = IERC20(token).balanceOf(address(this)) - before;
    accounted[token] += received;   // credit what you actually got
}

One extra balanceOf call. That's the entire fix.

Why scanners miss this

Static analyzers flag unchecked-transfer (missing return-value checks) constantly — that's a
different, much shallower bug. This one requires understanding that transferFrom's declared
amount and the received amount can diverge, and tracing that divergence all the way to a
permanent-lock outcome several functions later. It's a reasoning bug, not a pattern-match bug —
which is exactly why an LLM-assisted, hand-verified review catches it and a pure static scan usually
doesn't.

I do fast, affordable Solidity security reviews with a zero-false-positive pipeline (every flag is
hand-verified before it reaches you — see the proof). If
you're shipping a vault, a router, or anything that moves other people's tokens: let's talk.

DEV Community