ERC20 Patterns Through an Auditor's Lens: Receipt Tokens, Ownership, and Checks-Effects-Interactions

Given that ERC-20 is the blueprint for fungible tokens, it is not surprising that issues involving ERC20 are incredibly common in Web3 projects and applications. This doesn't mean that the bug is in the standard or in the ERC20.sol though. In this write-up I'll walk through two contracts that, superficially, look like simple ERC20 implementations: a token with a mint function, and a deposit/withdraw receipt-token system. I'll show what I look at on the first read and second reads, and discuss the design decisions of the developer that led to vulnerabilities.

Note, these aren't broken contracts. They're a chance to talk about how a careful ERC20 implementation should read.

Part 1 — Audit 1: "HackersToken" — The Minimal Mintable ERC20

The first engagement was a tiny ERC20 wrapper. The intent was to inherit from OpenZeppelin's ERC20, set an owner, and expose a mint function that only the owner can call.

pragma solidity ^0.8.13;

import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract HackersToken is ERC20 {

    address public owner;

    constructor() ERC20("Hackers Token", "HTK") {
        owner = msg.sender;
    }

    function mint(address _to, uint _amount) external {
        require(msg.sender == owner, "Not Owner");
        _mint(_to, _amount);
    }
}

My First Read

Upon reviewing this code, the structure is about as minimal as it gets, which is good. The OpenZeppelin ERC20 base provides _mint, _burn, transfer, transferFrom, approve, and allowance to HackersToken. The only thing the developer adds is an owner, set in the constructor, and a mint function protected by require(msg.sender == owner, "Not Owner").

The check for the owner is in the function rather than wrapped in an onlyOwner modifier. This is valid for a function that's called once. The mistake I've seen on other projects is the inverse — a modifier that looks like it does the check but in reality does nothing because of a typo such as require(msg.sender = owner, "Not Owner"). Here the require is present, the revert string is clear, and the owner state variable is publicly readable so anyone can verify on-chain who holds the keys.

But let's take a closer look:

The owner is set once and never changed. There's no transferOwnership. What if the deployer no longer has stewardship of the contract? If this contract is going to back real liquidity, I'd want to see either a transferOwnership(address newOwner) external onlyOwner (with require(newOwner != address(0))), or — better — inheritance from OpenZeppelin's Ownable where the functions are battle-tested.

There's also no supply cap. The owner can mint arbitrarily, forever. This is fine for an educational contract. But for a real token, mint-without-cap means the owner can dilute every other holder to zero in one transaction. The mitigation is either a hardcoded cap (require(totalSupply() + _amount <= MAX_SUPPLY)) or, again, OpenZeppelin's ERC20Capped extension.

Also, there's no event for ownership being set in the constructor. OpenZeppelin's Ownable emits OwnershipTransferred(address(0), msg.sender) on construction; this contract doesn't. From a forensics perspective, the lack of an event is a small audit comment, not a vulnerability — but it makes off-chain indexers and Etherscan less able to display the owner relationship.

The Test Harness

Let's look at the Hardhat sanity-checks:

// First transfer
await this.token.connect(user2).transfer(user3.address, FIRST_TRANSFER);

// Approval & Allowance test
await this.token.connect(user3).approve(user1.address, SECOND_TRANSFER);
expect(await this.token.allowance(user3.address, user1.address)).to.equal(SECOND_TRANSFER);

// Second transfer (transferFrom)
await this.token.connect(user1).transferFrom(user3.address, user1.address, SECOND_TRANSFER);

The test correctly verifies that transfer debits the sender and credits the recipient, that approve sets an allowance, and that transferFrom consumes the allowance. What it does not test — and this is the kind of negative test I always look for — is that an unauthorized account cannot mint. There is no expect(this.token.connect(user1).mint(...)).to.be.revertedWith("Not Owner"). If I were signing off on this contract, that test would be added.

Verdict

For an educational artifact, this contract is fine. For production, I'd recommend three changes: inherit from Ownable, add a supply cap (or a documented rationale for not having one), and add negative tests against the access control.

Part 2 — Audit 2: "TokensDepository" — Receipt Tokens, Reentrancy, and Checks-Effects-Interactions

The second engagement was more interesting. The idea was to build a deposit/withdraw system for three supported assets — AAVE, UNI, and WETH — where each deposit mints the user a "receipt token" (rAave, rUni, rWeth) that can later be redeemed for the underlying. This is the Aave-style pattern in miniature.

There are two contracts: a parameterized rToken that the depository owns, and the TokensDepository itself which mints and burns receipts.

The Receipt Token

contract rToken is ERC20 {

    address public owner;
    address public underlyingToken;

    modifier onlyOwner() {
        require(msg.sender == owner, "not owner");
        _;
    }

    constructor(address _underlyingToken, string memory _name, string memory _symbol)
    ERC20(_name, _symbol) {
        require(_underlyingToken != address(0), "wrong underlying");
        owner = msg.sender;
        underlyingToken = _underlyingToken;
    }

    function mint(address _to, uint256 _amount) external onlyOwner {
        _mint(_to, _amount);
    }

    function burn(address _to, uint256 _amount) external onlyOwner {
        _burn(_to, _amount);
    }
}

Upon reviewing this code, the access control pattern is correct: onlyOwner is a real modifier with a real require, the constructor rejects zero-address underlying tokens, and mint/burn are gated. The owner is set to msg.sender in the constructor — which means the depository, the contract that deploys the rToken, becomes the owner. That's intentional: users should never be able to mint or burn receipt tokens directly; only the depository should, in response to legitimate deposits and withdrawals.

The one thing I'd note here is that the burn function takes a _to parameter — semantically it's "burn from this address." Since it's onlyOwner-gated and only the depository ever calls it, this isn't an external vulnerability. But the naming is slightly misleading; _from would read more clearly. This is an audit comment, not a finding.

The Depository

contract TokensDepository {

    mapping(address => IERC20) public tokens;
    mapping(address => rToken) public receiptTokens;

    modifier isSupported(address _token) {
        require(address(tokens[_token]) != address(0), "token is not supported");
        _;
    }

    constructor(address _aave, address _uni, address _weth) {
        tokens[_aave] = IERC20(_aave);
        tokens[_uni] = IERC20(_uni);
        tokens[_weth] = IERC20(_weth);

        receiptTokens[_aave] = new rToken(_aave, "Receipt AAVE", "rAave");
        receiptTokens[_uni] = new rToken(_uni, "Receipt UNI", "rUni");
        receiptTokens[_weth] = new rToken(_weth, "Receipt WETH", "rWeth");
    }

    function deposit(address _token, uint256 _amount) external isSupported(_token) {
        bool success = tokens[_token].transferFrom(msg.sender, address(this), _amount);
        require(success, "transferFrom failed");
        receiptTokens[_token].mint(msg.sender, _amount);
    }

    function withdraw(address _token, uint256 _amount) external isSupported(_token) {
        receiptTokens[_token].burn(msg.sender, _amount);
        bool success = tokens[_token].transfer(msg.sender, _amount);
        require(success, "transfer failed");
    }
}

My First Read

Upon reviewing this code, the architecture is clean. The depository deploys the receipt tokens in its own constructor, which means there's no possible race where the depository is "linked to" a malicious pre-deployed receipt token — the receipt token is owned by the depository. The isSupported modifier prevents users from depositing arbitrary tokens not in the whitelist.

But upon closer inspection, the order of operations in withdraw is the part of this contract that caught my attention. Look at it again:

function withdraw(address _token, uint256 _amount) external isSupported(_token) {
    receiptTokens[_token].burn(msg.sender, _amount);   // STATE update
    bool success = tokens[_token].transfer(msg.sender, _amount);  // EXTERNAL call
    require(success, "transfer failed");
}

This is the Checks-Effects-Interactions pattern in textbook form. The "Effect" — burning the user's receipt tokens — happens before the "Interaction" — transferring the underlying tokens out to the user. If the developer had done it the other way around, the contract would be vulnerable to a reentrancy attack via any ERC20 with a transfer callback (e.g., ERC777, or any token with a fee-on-transfer or hook).

Let's walk through why this ordering matters. Imagine the inverse:

// HYPOTHETICAL VULNERABLE VERSION — DO NOT DEPLOY
function withdraw(address _token, uint256 _amount) external {
    bool success = tokens[_token].transfer(msg.sender, _amount);  // EXTERNAL call FIRST
    require(success);
    receiptTokens[_token].burn(msg.sender, _amount);               // STATE update LATER
}

If _token is an ERC777 — or any token that calls back into the recipient via tokensReceived — then during transfer, control passes to the attacker's contract before the receipt token has been burned. The attacker can reenter withdraw, see an unchanged receipt balance, and withdraw again. They can drain the depository while the receipt balance decremented only once.

The current contract is safe against this because burn is called first. The attacker can still reenter, but by the time control returns to their contract, their receipt balance has already been debited, so the second withdraw call will fail when burn tries to debit a balance that no longer exists.

Note: the standard AAVE, UNI, and WETH ERC20s do not have transfer callbacks, so reentrancy is not currently exploitable here even if the order were wrong. But the supported-token list could be expanded by a future deployment, and the safe pattern costs nothing.

The deposit Function Is Safe — But Pay Attention to Why

function deposit(address _token, uint256 _amount) external isSupported(_token) {
    bool success = tokens[_token].transferFrom(msg.sender, address(this), _amount);
    require(success, "transferFrom failed");
    receiptTokens[_token].mint(msg.sender, _amount);
}

A reasonable auditor's question might be: doesn't this violate CEI? The external transferFrom happens before the state update (the receipt token mint).

The answer is that CEI is a property about state the attacker could exploit by reentering, not a blind ordering rule. In deposit, the only state changes are (a) the depository's balance of the underlying goes up, and (b) the user gets receipt tokens. If an attacker reentered between those, what would they exploit? They'd have to call deposit or withdraw again. A nested deposit would just do another transferFrom — they pay again, they mint more receipts; no profit. A nested withdraw would call burn first — but they have no receipts yet (we haven't minted them), so it would revert.

The deeper reason deposit is safe is that transferFrom does not provide a callback to the sender for vanilla ERC20. The transferFrom function moves balances inside the token contract and emits a Transfer event; control does not pass back to the user's code mid-call. ERC777's tokensReceived hook fires on the recipient (here, the depository), not the sender, so even with a callback-enabled token there's no attacker-controlled code in the path.

That said — and this is the audit comment I'd write — if the supported asset list ever expands to a token with a true sender-side callback, this argument breaks. Inheriting from OZ's ReentrancyGuard and decorating deposit and withdraw with nonReentrant would be a cheap belt-and-suspenders.

"But Shouldn't the Contract Approve msg.sender Before Transferring?"

This is the question that almost every junior developer asks the first time they read this withdraw function, and the answer is no — and understanding why it's no is part of understanding ERC20.

approve is the mechanism by which an account permits a third party to spend its tokens via transferFrom. The flow is:

1. Alice (owner) calls approve(Bob, 100) on the token contract.
2. Bob (spender) calls token.transferFrom(Alice, Charlie, 100). The token contract checks Alice's allowance for Bob, debits Alice's balance, credits Charlie.

In the depository's withdraw, the depository owns the tokens — they are held at address(this). To move tokens that you own, you call transfer(to, amount). There is no spender, no allowance, no approval needed. The check is internal: the token contract verifies that the caller (the depository) has enough balance.

Approval would only be necessary if the depository wanted some other contract to be able to spend tokens on its behalf. That's not what withdraw does — withdraw is the depository sending its own tokens directly to the user. So approve is irrelevant here.

The confusion usually arises because msg.sender for the call to withdraw is the user — but inside withdraw, when the depository calls tokens[_token].transfer(...), the msg.sender to the token contract is the depository (address(this)). That context switch is exactly what makes the direct transfer work.

Part 3 — Closing Notes from the Auditor's Desk

The two contracts above are not the kind of thing that ends up on a hack post-mortem. They're the baseline — what a well-formed ERC20 implementation looks like. But the audit habits I exercised are the same ones I exercise reading the code that does end up on post-mortems:

I read access control modifiers character by character to make sure they actually contain a require. I check whether onlyOwner is hand-made or inherited from a standard library; if hand-made, I read every line of the parent. I look for transferOwnership and check whether it is gated. I check whether the token has a supply cap. I check whether mint and burn emit events and update the right balances. And whenever I see an external call, I trace the order: state update before, or state update after? If after, I check whether the called contract can possibly call back into us, and whether the state we haven't yet updated is something the attacker could profit from.

A clean ERC20 is the boring kind of clean. It looks like nothing happened. That's the goal.