fix(lint): restore numeric-cast peeling for underlying_var in arbitrary-send-erc20 - #16632
Open
gomesalexandre wants to merge 1 commit into
Open
fix(lint): restore numeric-cast peeling for underlying_var in arbitrary-send-erc20#16632gomesalexandre wants to merge 1 commit into
gomesalexandre wants to merge 1 commit into
Conversation
…ry-send-erc20 underlying_var() only peeled address-like casts (address(x), IFoo(x)) after the sol/analysis consolidation (foundry-rs#16615), losing the numeric-cast peeling (uintN/intN/bytes) arbitrary_send_erc20.rs's own copy of this logic used to have - a permit()/transferFrom() owner correlated through a cast round-trip like address(uint160(rawToken)) is no longer recognized as the same variable, producing a false positive on a High-severity lint. controlled_delegatecall.rs still carried its own private duplicate with the broader behavior, which is the evidence the drop was accidental. Adds underlying_var_through_numeric_casts(), a width-floored variant (>=20 bytes, i.e. can hold a full address without truncation) used only where two expressions are being correlated as the same address-typed value (permit owner, transferFrom from, token identity, flash-loan receiver/token). Value-typed correlation (flash-loan amount/fee, sum_operands, the sum_of lookup) stays on the original narrow underlying_var(), since a width floor that is sound for addresses is not sound for arbitrary uint256 values - an earlier draft of this fix that widened all call sites uniformly introduced a real false negative there, caught by a synchronous Codex review before this shipped. Consolidates controlled_delegatecall.rs's private underlying_var()/is_cast() duplicate into the shared helpers instead of leaving two copies to drift again.
gomesalexandre
marked this pull request as ready for review
September 4, 2026 15:45
gomesalexandre
requested review from
0xrusowsky,
DaniPopes,
figtracer,
grandizzy,
mablr and
mattsse
as code owners
September 4, 2026 15:45
Contributor
✅ Changelog foundThe deterministic check will validate the changed entry. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
underlying_var()(crates/lint/src/sol/analysis/exprs.rs) only peels address-like casts (address(x),IFoo(x)) after thesol/analysisconsolidation (#16615). Before that refactor,arbitrary_send_erc20.rshad its own copy of this logic that ALSO peeled numeric casts (uintN/intN/bytes), specifically to correlate apermit()owner with a latertransferFrom()'sfromacross a cast round-trip likeaddress(uint160(rawToken)). The refactor silently dropped that:controlled_delegatecall.rsstill carries its own private duplicate with the broader (numeric-cast-peeling) behavior - that's the evidence the drop from the shared helper was accidental, not deliberate: the refactor kept the narrower helper in the new shared module, let one caller opt out with a local duplicate, and silently moved every other consumer (includingarbitrary-send-erc20) onto the weaker version.Fix
Adds
underlying_var_through_numeric_casts()alongside the originalunderlying_var()(left untouched). The new function peelsuintN/intNcasts that are at least 20 bytes wide (i.e. can hold a full address without truncation), plusbytes(..).The width floor matters, and cost me an iteration: my first draft peeled through any numeric cast width, which broke
unsafe_oz_erc721_mint.rs'sTruncatedRecipientNfttest - that test specifically checks that auint160 -> uint8 -> uint160truncating cast chain must NOT be treated as identity-preserving. Caught via the full UI suite (not just the target fixture), confirmed against unfixedmasterthat it wasn't a pre-existing flake.underlying_var_through_numeric_casts()is used only where two expressions are correlated as the same address-typed value (permit owner,transferFromfrom, token identity, flash-loanreceiver/token). Value-typed correlation (flash-loanamount/fee,sum_operands, thesum_oflookup) stays on the original narrowunderlying_var()- a width floor that's sound for addresses is not sound for arbitraryuint256values. An earlier draft that widened every call site uniformly introduced a real false negative here (a truncated flash-loan callback amount silently treated as matching the untruncated repayment pull), caught by a synchronous Codex adversarial review before this shipped - see the twobadFlashLoan*Truncatedfixtures below, which reproduce exactly that.Also consolidates
controlled_delegatecall.rs's privateunderlying_var()/is_cast()duplicate into the shared helpers (deleting the duplicate) instead of leaving two copies to drift apart again, which is what caused this regression in the first place.Known, disclosed limitation (not introduced by this diff)
address(bytes20(x))still false-positives -ElementaryType::Bytesis dynamicbytes, notbytesN, and this diff doesn't addFixedByteshandling. This gap existed identically before the diff too (the pre-refactor code'sbytes(..)branch matched dynamicBytes, which doesn't apply to a realaddress(bytes20(x))cast either - matching a validbytesNcast head would need new logic). Codex specifically cautioned against a naive "accept allFixedBytes >= 20" fix here, since mixed fixed-byte/integer resizing has different padding semantics that can shift which 160 bits survive a chain - left out of scope rather than risk introducing an under-tested new correctness gap.Also disclosed and documented with a regression test: a narrowing cast inside an otherwise-trusted chain (
address(uint160(uint8(0)))) is a conservative false positive incontrolled-delegatecall, since rejecting narrowing casts is exactly what keeps a genuinely truncating chain from being wrongly trusted.Testing
masterviatoken.permit/token.transferFromcorrelated through a numeric-cast round-trip; confirmed the two flash-loan truncation false-negatives Codex found also reproduce against an earlier draft of this diff (widen-everywhere) but not against the final version.UnsafeOzErc721Mint.solandControlledDelegatecall.solwith zero unintended diff on either's.stderrsnapshot (confirmed non-regressive on both prior consumers of the duplicated/shared logic).ArbitrarySendErc20.sol: the false-positive fix (numeric-cast round-trip correctly correlates), a mismatched-round-trip regression check (still warns), 3 flash-loan truncation false-negative regression checks (callback-side truncation, pull-side sum truncation,sum_of-local-alias truncation), and 1 new fixture inControlledDelegatecall.soldocumenting the accepted narrowing-cast conservative false positive.forge-lintunit tests pass.-D warnings).cargo fmt --checkclean on stable - nightly (used by this repo's CI) wasn't available in this environment to verify against.bytes20gap (Medium), both addressed above; second pass on the revised diff found no blocking issues.receipts
no runtime UI change -
forge-lint's own test suite is the receipt for this PR (static-analysis fixture corpus, not a runnable app).