Back to Foundry

Unsafe OZ ERC721 mint

crates/lint/docs/unsafe-oz-erc721-mint.md

1.8.111.5 KB
Original Source

Unsafe OZ ERC721 mint

Severity: Med ID: unsafe-oz-erc721-mint

Flags calls that resolve to ERC721._mint, which credits a token without checking that the recipient can receive it.

What it does

Reports a call whose callee resolves to a function named _mint declared in a contract named exactly ERC721, ERC721Upgradeable, ERC721Consecutive or ERC721ConsecutiveUpgradeable whose source comes from an OpenZeppelin package path, wherever that contract sits in the caller's inheritance chain, or to a user _mint override that transitively delegates to one of those (the capped/pausable pattern forwarding through super._mint): direct calls dispatch to the override, but the path still reaches the unchecked base. The first two canonical contracts declare the unchecked _mint, and most extensions (ERC721Enumerable, ...) inherit it, so resolution lands on the base; the v4 Consecutive extensions are the exception, they override _mint with a construction guard that forwards to the base through super._mint, still without a receiver check (in v5 they override _update instead, and their names match nothing). The plain _mint(to, id), the qualified ERC721._mint(to, id) and super._mint(to, id) forms are all covered. Exact names keep a safe override clean even when the overriding contract's name contains the ERC721 substring, and the provenance requirement keeps a local contract reusing a canonical name out of scope; a vendored OpenZeppelin copy under a path that does not name OpenZeppelin is not recognized.

This mirrors Aderyn's unsafe-oz-erc721-mint detector, with a resolution-based check instead of its name-and-import heuristic: Aderyn flags any identifier named _mint in a file that imports an openzeppelin path and whose contract lists a direct ERC721* base, which misses indirect inheritance and depends on the import path. Resolving the callee also keeps ERC20._mint and unrelated local _mint functions out of scope.

Three exemptions:

  • calls to _safeMint are the recommended fix and never fire;
  • calls made inside the canonical wrapper, a _safeMint declared in ERC721/ERC721Upgradeable itself, which legitimately calls _mint next to its receiver check. A user-defined _safeMint override stays analyzed: it can call _mint directly without any check;
  • calls made inside a user _mint override: the override is the mint primitive itself, super._mint there is delegation, and _safeMint there would re-enter the override through the virtual dispatch. A delegating override reports at its call sites instead, unless every successful path either proves the recipient code-less or runs a reverting receiver check after the delegation, matching the ordering of canonical _safeMint.

The recognized guards are a closed set, because a hook call that merely appears somewhere in a condition proves nothing about whether the revert depends on its answer. An override is safe when it requires the recipient to be code-less, or, after the delegated mint establishes ownership, reverts on refusal after asking onERC721Received of the recipient itself and:

  • require or assert holds the comparison hook(to) == selector as its whole condition, or the account short circuit to.code.length == 0 || hook(to) == selector;
  • or if (hook(to) != selector) takes a branch that always reverts, or if (hook(to) == selector) has an else that does. A branch reverts only when nothing before its revert can leave the function keeping the token: a return, an assembly block, an internal helper that transitively reaches assembly, an unresolved internal function-pointer call, a try or a switch all end the reading there;
  • or one of those sits in the branch a test that the recipient carries code (> 0, != 0, >= 1, or their mirrors) dedicates to contracts, an account needing no hook;
  • or one of those is reached through a same-frame function or modifier, provided the callee's guard runs before any of its own possible exits. A code-less proof needs only the recipient parameter, while a callback helper needs both the recipient and token, the way OpenZeppelin factors _checkOnERC721Received out of _safeMint; the helper call must itself follow the delegated mint. A cross-contract call does not count because the hook then sees the helper contract as its caller rather than the minting contract. A virtual callee does not count either: an override may replace its body, and the one analyzed here is the statically resolved declaration. A helper carrying modifiers is rejected conservatively because a modifier may skip its placeholder, and a helper or modifier that reassigns a bound identity is rejected because its later check may name a different value from the one its caller retains. For modifier guards themselves, the expansion order is followed so an inner assembly exit can bypass an outer tail guard; a callback in a prefix is too early, while a code-less prefix or callback tail may cover the body.

The body is read in statement order. A code-less proof may precede a delegation, since an account needs no receiver callback. A callback may not: OpenZeppelin establishes ownership before invoking the recipient, which may inspect ownerOf, its balance, or reenter during the hook. A pre-mint callback can therefore accept state that the same recipient would reject after the mint. A callback executed after a delegation covers the pending mint, the revert undoing it, unless a statement in between may leave the function successfully: in super._mint(to, id); if (id == 0) return; require(hook(to, id) == selector) token zero walks out unchecked. Once a post-mint callback or code-less observation has discharged that mint, later state-changing work cannot retroactively invalidate it; the same work does invalidate a code-less observation intended to cover another mint later. A guard expression that can transitively reach assembly cannot clear a pending mint either, because an assembly return in another argument may leave before the builtin reverts. The accepted branch of a pre-mint if guard remains uncovered for a later mint, even when the refusing branch always reverts. A guard behind a condition of its own clears a pending mint only when every branch holds one, and nothing inside a loop body is credited, since it may run zero times. The branch a to.code.length test dedicates to accounts needs no guard and satisfies the mints already made or made later on that path, an account accepting the token either way, so the usual skip stays exempt on both sides of the mint and in both polarities.

The callback guard covers the recipient and the token it names, read from the arguments bound to the callee's first and second parameters, matched by name for named arguments. An override handing the base any other address reports, and so does one asking the hook about another token than the delegated one, the hook's third parameter: a recipient may accept one token and refuse another. The same identities must survive every recursive unsafe override target; an intermediate override forwarding tokenId + 1 or another recipient prevents an outer post-mint callback guard from exempting its callers. A code-less-recipient proof needs only the recipient identity, so a computed or remapped token remains covered. Identity is by variable, not by value, so reassigning an identity between the delegation and callback reports: super._mint(to, tokenId); tokenId = tokenId + 1; require(hook(to, tokenId) == selector) asks about a token other than the one minted, though both statements spell tokenId. The reassignment counts wherever it runs between those operations, a bare statement or embedded in an if condition (if ((tokenId = tokenId + 1) > 0) {}, if (tokenId++ > 0) {}), the condition running whichever branch is taken. Inline assembly is opaque to the variable visitor and conservatively retires coverage too; a fresh callback after it may cover only a mint that already ran with the resulting identity, while a code-less proof may still cover a later mint. A reassignment before both the mint and guard is fine, both reading the new value; one after a callback has covered its mint is fine too. Since the check is by variable and does not read values, a reassignment that changes nothing, tokenId = tokenId or a whole-width mask, still reports for a callback guard, and so does one confined to a branch that reverts, both conservative. The recipient keeps its identity behind parentheses, payable(...) and casts to an address or contract type; a truncating cast such as address(uint160(uint8(...))) usually mints a different address than the checked one and reports. The hook is matched on its (address, address, uint256, bytes) shape and must resolve to an externally callable declaration of a non-library contract: an onERC721Received attached by using ... for address runs in the minting contract without asking the recipient anything. The answer the hook is compared against must be 0x150b7a02, spelled, converted without losing selector bytes or changing their alignment, held by a constant, or named by a selector member that itself resolves to the receiver hook; spelled on a same-name function of another shape, .selector is a different value and does not exempt. An immutable or a state variable does not exempt either, its value being unknown here.

Everything else reports, including an override that merely reads the recipient's .code without restricting it to code-less addresses, a callback that precedes the mint (even when its refusal reverts), a hook whose answer is discarded, stored in a local, returned as a bool by a helper, or wrapped in a try whose catch may swallow the refusal, a hook a second operand can short circuit past, a hook riding in the revert message, one asked of an address derived from the recipient rather than of the recipient, one guarded inside a loop body that may never run, one whose refusal only returns, one whose guard lives in a virtual callee, and an exiting branch taken on acceptance instead of refusal. Following an answer across statements would take a dataflow analysis this detector does not run, which also reports, for callback guards, a delegation whose token is not a plain variable, there being no name to match the guard against. A mutable state variable is rejected conservatively as the token identity: an intervening call, including one inside a recursively delegated override, can reenter and change it after the mint reads it but before the later callback guard does.

Two shapes stay out of reach, both resting on a mint the type checker cannot resolve to a _mint declaration. An override that reimplements the mint itself, assigning ownership without delegating to the OpenZeppelin base, never resolves to the unchecked _mint and is not reported, even though it locks a token just the same. A delegation reached through an internal function pointer (function(address, uint256) internal fp = ERC721._mint; fp(to, id);) resolves to no function id, so it is invisible the same way delegatecall and assembly-dispatched calls are; a call to _mint in real code is direct, so this stays a documented limit rather than a checked path.

Why is this bad?

ERC721._mint assigns the token without calling onERC721Received on the recipient. Minting to a contract that does not implement the receiver interface permanently locks the token. _safeMint performs the check and reverts instead.

Example

Bad

solidity
function mint(address to, uint256 id) external {
    _mint(to, id);
}

Good

solidity
function mint(address to, uint256 id) external {
    _safeMint(to, id);
}