feat(lint): add unsafe-oz-erc721-mint detector#15551
Conversation
Flags calls that resolve to a function named _mint declared in a non-library contract whose name contains ERC721, wherever it sits in the caller's inheritance chain: the plain _mint(to, id), the qualified ERC721._mint(to, id) and super._mint(to, id) forms. Overload sets are filtered by arity, so a call that can only dispatch to a same-name local overload of a non-ERC721 contract stays clean. Calls to _safeMint and calls made inside a _safeMint implementation (the wrapper itself) are exempt. Part of foundry-rs#14381.
0xMars42
requested review from
0xrusowsky,
DaniPopes,
figtracer,
grandizzy,
mablr,
mattsse and
stevencartavia
as code owners
…type checker The detector judged the whole name-resolution set of a _mint call, so a contract that safely overrides _mint was flagged for the base it hides, and reconstructing dispatch by hand missed overload selection by argument type. Take the resolved callee from type_of_expr instead: it is the single function the type checker selected, so override shadowing, super._mint(...) and overload selection by argument type are already accounted for. _mint(to, id) that dispatches to the inherited ERC721._mint(address, uint256) is flagged, while a same-arity local overload (_mint(address, bytes)) and a safe override are not.
…onical safe wrapper Two review findings: - the _safeMint exemption suppressed every function named _safeMint, including a user override that calls _mint directly without a receiver check. Only exempt a _safeMint declared in ERC721/ERC721Upgradeable (the canonical wrapper); a user-defined _safeMint override stays analyzed. - contains(ERC721) on the declaring contract name flagged a safe override just because the contract name held the substring. Match the exact OZ base names instead; extensions inherit _mint rather than redeclare it, so resolution still lands on the canonical base.
A `_mint` override delegating to the base (`super._mint(to, id)` behind a guard, the capped/pausable pattern) was reported, and the suggested `_safeMint` replacement is wrong there: it re-enters the override through the virtual dispatch. The override is part of the mint primitive itself, so the receiver check belongs to its callers; exempt it like the canonical `_safeMint` wrapper. `super._mint` from any other function still reports.
Stock OZ v4 ERC721Consecutive and ERC721ConsecutiveUpgradeable override `_mint` with a construction guard that forwards to the base through `super._mint`, still without the receiver check, so a consumer inheriting them resolves `_mint` to that override and went unflagged. The canonical list now carries the two names; in v5 the Consecutive extension overrides `_update` instead, so they match nothing there. The docs and comments claiming every extension inherits `_mint` are corrected accordingly.
|
9b0bfa3 extends the canonical list to the v4 Consecutive extensions. Stock OZ v4 |
Two scope fixes to what counts as an unsafe `_mint` target: - A user `_mint` override that transitively delegates to the canonical unchecked `_mint` (the capped/pausable pattern forwarding through `super._mint`) went unreported at its call sites, even though the path still reaches the base without a receiver check. Such overrides are now judged recursively and their direct calls report; the `super._mint` inside the override itself stays exempt. - The canonical contracts were identified by exact name alone, so a local contract reusing a name like `ERC721ConsecutiveUpgradeable` was flagged without any OpenZeppelin import. The declaration's source must now come from an OpenZeppelin package path, applied to the `_safeMint` wrapper exemption as well. The canonical testdata mirrors move under an `openzeppelin-contracts` auxiliary path; a vendored copy under a path that does not name OpenZeppelin is not recognized, which the docs state. Covers the delegating override, its `_safeMint` clean path and the same-name local contract in the testdata.
…mint A `_mint` override that performs the receiver check itself before delegating, an `onERC721Received` call or an `address.code` inspection, is a safe wrapper like the canonical `_safeMint`, but the delegation judgment classified it as an unsafe target and reported its callers. The override body scan now also looks for the check markers: a call whose resolved declaration is named `onERC721Received`, or a `code` member read whose base is typed as an address, so a user field named `code` does not count. An override carrying either marker is not an unsafe target. Covers a wrapper with the full check and one with the code inspection alone in the testdata; a delegating override without a check still reports at its call sites.
…z-erc721-mint The check markers accepted any `.code` inspection in a `_mint` override, so an `address(this).code` construction guard passed for a receiver check and the override's callers went unreported while the path to the unchecked base stayed open for arbitrary recipients. Both markers are now tied to the recipient, the override's first address-typed parameter: the `.code` base and the `onERC721Received` call receiver must mention it. The construction-guard repro is in the testdata and its callers report; the recipient-checking wrappers stay clean.
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
Found one blocker:
crates/lint/src/sol/med/unsafe_oz_erc721_mint.rs:176: the OpenZeppelin provenance check still accepts any path containingopenzeppelin. A local or third-party path such assrc/not-openzeppelin/ERC721.solcan satisfy the check and make an unrelated same-name_mintlook canonical. Please match actual package/path components such as@openzeppelinoropenzeppelin-contracts, and add a negative fixture for a misleading path containingopenzeppelin.
The provenance check used `path.to_lowercase().contains("openzeppelin")`,
which also accepts a same-name contract under a path that merely contains
the substring, for example `src/not-openzeppelin/ERC721.sol`, making an
unrelated local `_mint` look like the canonical OpenZeppelin one. Match a
full path component against the package roots (`@openzeppelin`,
`openzeppelin-contracts`, `openzeppelin-contracts-upgradeable`) instead.
Adds a negative fixture: a local `ERC721` under `not-openzeppelin/` whose
`_mint` stays out of scope.
|
Fixed in |
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
The previous OpenZeppelin provenance blocker is fixed, but there is still a false negative in the delegating override handling.
At crates/lint/src/sol/med/unsafe_oz_erc721_mint.rs:253, any read of the recipient's .code is treated as proof that the override performs a receiver check. An override can read to.code.length into a local, ignore it, and then call super._mint(...); direct callers would skip the diagnostic even though the unchecked mint path remains. Please require the recipient code check to actually guard/control the mint or receiver-call path, rather than treating any .code read as sufficient.
Any read of the recipient's `.code` counted as the receiver check, so an override could do `uint256 len = to.code.length; len;` and then `super._mint(to, tokenId)` and pass for a safe wrapper while arbitrary contract recipients still reached the unchecked base. What makes a delegating override safe is that the recipient's refusal reverts the mint. Since a hook call that merely appears inside a condition says nothing about whether the revert depends on its answer, the guards are now a closed set: `require`/`assert` holding `hook(to) == selector` as its whole condition, or the account short circuit `to.code.length == 0 || hook(to) == selector`; an `if` whose reverting branch is the one a refusal takes; either of those under a test that the recipient carries code; and either of those reached through a function or a modifier the recipient itself is handed to, as `_checkOnERC721Received` is. The guard may follow the delegation, a revert undoing the mint wherever it sits. A branch reverts only when nothing before its `revert` can leave the function keeping the token. A `virtual` callee does not credit a guard. The guard covers the recipient it names, read from the argument bound to the callee's first parameter. The hook is matched on its `(address, address, uint256, bytes)` shape, and the answer compared against it must be the accepting selector. Reported now, each with a fixture: a hook a second operand can short circuit past, one riding in the revert message, one asked of an address derived from the recipient, one inside a loop body that may never run, a refusal that only returns, returns before reverting, or escapes through assembly, a guard behind a `virtual` callee, an accepting answer that is not provably the selector, a second mint to an unguarded recipient through a sibling base, an exiting branch taken on acceptance rather than refusal, and an answer discarded, stored in a local, returned as a `bool` by a helper, or wrapped in a `try` whose `catch` may swallow it.
|
Fixed in A guard is Reported now, each with a fixture: a refusal that only returns or escapes through assembly, a guard behind a |
mablr
left a comment
mablr
left a comment
There was a problem hiding this comment.
Please check the following FNs, maybe not all worth fixing.
| recipient: hir::VariableId, | ||
| seen: &mut Vec<FunctionId>, | ||
| ) -> bool { | ||
| stmts.iter().any(|stmt| stmt_reverts_on_refusal(gcx, hir, stmt, recipient, seen)) |
There was a problem hiding this comment.
This can miss an unsafe path because any ignores statement order. For example, super._mint(to, id); if (id == 0) return; require(hook(to) == selector); leaves token 0 minted without a callback, but the later guard exempts every caller. An expected-warning UI fixture confirms the diagnostic is missing; the proof needs to account for successful exits before a guard.
| seen: &mut Vec<FunctionId>, | ||
| ) -> bool { | ||
| let parameters = hir.function(function_id).parameters; | ||
| for (index, arg) in args.exprs().enumerate() { |
There was a problem hiding this comment.
Named helper arguments are bound by source position here, not parameter name. _check({actual: to, checked: checker}) can therefore analyze to as the checked parameter even though runtime checks checker, suppressing the unsafe mint warning. The mint collector already handles named binding correctly; helper calls need the same mapping.
| return false; | ||
| } | ||
| let ExprKind::Member(receiver, _) = &callee.peel_parens().kind else { return false }; | ||
| is_exactly_recipient(receiver, recipient) |
There was a problem hiding this comment.
The callback is tied only to the recipient, not to the delegated token. A wrapper can check onERC721Received(..., tokenId + 1, ...) and then super._mint(to, tokenId); a receiver may accept the former and refuse the latter, yet callers are suppressed. The adversarial UI fixture confirms this missing diagnostic, so the callback arguments need correlation with each delegated mint.
| ExprKind::Member(base, member) => { | ||
| member.as_str() == "selector" | ||
| && matches!(&base.peel_parens().kind, ExprKind::Member(_, hook) | ||
| if hook.as_str() == "onERC721Received") |
There was a problem hiding this comment.
Any member spelled onERC721Received.selector is accepted without resolving its signature or value. Comparing the real hook result with IWrongSelector.onERC721Received.selector (for a zero-argument function) therefore suppresses the warning even though that selector is not 0x150b7a02; the fixture reproduces the false negative. Please resolve/validate the selector rather than matching member names.
| if let ExprKind::Call(callee, args, _) = &expr.kind | ||
| && matches!( | ||
| &callee.peel_parens().kind, | ||
| ExprKind::Type(..) | ExprKind::Ident([hir::Res::Item(hir::ItemId::Contract(_)), ..]) |
There was a problem hiding this comment.
Unwrapping every one-argument type conversion treats lossy casts as recipient-preserving. super._mint(address(uint160(uint8(uint160(to)))), id) is considered a mint to to, although it usually targets a different truncated address that was never checked. The expected-warning fixture is missed; only value-preserving address/interface casts should be unwrapped.
| let expr = expr.peel_parens(); | ||
| let ExprKind::Call(callee, ..) = &expr.kind else { return false }; | ||
| let Some(function_id) = resolved_callee(gcx, expr) else { return false }; | ||
| if !is_receiver_hook(hir, function_id) { |
There was a problem hiding this comment.
This shape does not prove an external callback was made to the recipient. A using FakeReceiverCheck for address library can provide an internal onERC721Received(address,address,uint256,bytes) that always returns the selector; to.onERC721Received(...) then exempts the wrapper without calling to at all. The UI fixture confirms the miss; library/internal extension functions need to be excluded.
4 participants
Motivation
unsafe-oz-erc721-mintis an unchecked item of #14381.ERC721._mintassigns a token without callingonERC721Receivedon the recipient, so minting to a contract that does not implement the receiver interface permanently locks the token;_safeMintperforms the check.Solution
New
Medseverity late pass (Medium is the item's section in #14381; Aderyn classifies it Low). It flags a call whose callee resolves to a function named_mintdeclared in a non-library contract whose name containsERC721, wherever that contract sits in the caller's linearized bases:_mint(to, id),ERC721._mint(to, id)andsuper._mint(to, id)are all covered, including from modifiers and constructors.This is a resolution-based check rather than Aderyn's heuristic (
aderyn_core/src/detect/low/unsafe_oz_erc721_mint.rsflags any identifier named_mintin a file importing anopenzeppelinpath whose contract lists a directERC721*base): resolving the callee covers indirect inheritance, drops the import-path dependency, and keepsERC20._mintand unrelated local_mintfunctions out of scope.Precision details:
_mint(to)that can only dispatch to a local_mint(address)overload of a non-ERC721 contract is not flagged, while the two-argument call to the inherited base still is;ERC721(ERC721Lib._mint): the unchecked_mintlives in OpenZeppelin'sERC721contract;_safeMintand calls made inside a function named_safeMint(the wrapper implementation, which legitimately calls_mintnext to its receiver check) never fire.Known limitation: only function bodies are visited, so a
_mintcall in a state variable initializer is not analyzed. This is unreachable with OpenZeppelin's_mint, which returns nothing and cannot appear in an initializer.Test
testdata/UnsafeOzErc721Mint.soluses a minimal ERC721 mirror (unchecked_mint, checking_safeMint) plus ERC721Upgradeable, ERC20, a same-arity local overload, anERC721Liblibrary decoy and a standalone_mint. Firing forms: plain,super., qualified, indirect inheritance two levels up, the upgradeable variant, a two-argument call next to a one-argument local overload, and calls from a modifier and a constructor. Clean forms:_safeMint, an overriding_safeMintimplementation calling_mint,ERC20._mint, the arity-mismatched overload call, the library decoy and the standalone_mint.Running the detector over the repo's forge integration fixtures (the root
testdata/directory) produces no findings.Part of #14381.