Skip to content

fix(lint): restore numeric-cast peeling for underlying_var in arbitrary-send-erc20 - #16632

Open
gomesalexandre wants to merge 1 commit into
foundry-rs:masterfrom
gomesalexandre:fix_underlying_var_numeric_cast
Open

fix(lint): restore numeric-cast peeling for underlying_var in arbitrary-send-erc20#16632
gomesalexandre wants to merge 1 commit into
foundry-rs:masterfrom
gomesalexandre:fix_underlying_var_numeric_cast

Conversation

@gomesalexandre

Copy link
Copy Markdown
Contributor

underlying_var() (crates/lint/src/sol/analysis/exprs.rs) only peels address-like casts (address(x), IFoo(x)) after the sol/analysis consolidation (#16615). Before that refactor, arbitrary_send_erc20.rs had its own copy of this logic that ALSO peeled numeric casts (uintN/intN/bytes), specifically to correlate a permit() owner with a later transferFrom()'s from across a cast round-trip like address(uint160(rawToken)). The refactor silently dropped that:

IERC20 token = IERC20(address(uint160(rawToken)));
token.permit(from, address(this), amt, deadline, v, r, s);
token.transferFrom(from, address(this), amt);   // guarded, but now flagged - underlying_var can't
                                                  // resolve address(uint160(rawToken)) back to rawToken

controlled_delegatecall.rs still 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 (including arbitrary-send-erc20) onto the weaker version.

Fix

Adds underlying_var_through_numeric_casts() alongside the original underlying_var() (left untouched). The new function peels uintN/intN casts that are at least 20 bytes wide (i.e. can hold a full address without truncation), plus bytes(..).

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's TruncatedRecipientNft test - that test specifically checks that a uint160 -> uint8 -> uint160 truncating cast chain must NOT be treated as identity-preserving. Caught via the full UI suite (not just the target fixture), confirmed against unfixed master that 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, 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() - a width floor that's sound for addresses is not sound for arbitrary uint256 values. 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 two badFlashLoan*Truncated fixtures below, which reproduce exactly that.

Also consolidates controlled_delegatecall.rs's private underlying_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::Bytes is dynamic bytes, not bytesN, and this diff doesn't add FixedBytes handling. This gap existed identically before the diff too (the pre-refactor code's bytes(..) branch matched dynamic Bytes, which doesn't apply to a real address(bytes20(x)) cast either - matching a valid bytesN cast head would need new logic). Codex specifically cautioned against a naive "accept all FixedBytes >= 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 in controlled-delegatecall, since rejecting narrowing casts is exactly what keeps a genuinely truncating chain from being wrongly trusted.

Testing

  • Real red-before/green-after: confirmed the false-positive on unfixed master via token.permit/token.transferFrom correlated 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.
  • Full lint UI fixture suite: 97/97 passing, including UnsafeOzErc721Mint.sol and ControlledDelegatecall.sol with zero unintended diff on either's .stderr snapshot (confirmed non-regressive on both prior consumers of the duplicated/shared logic).
  • Added 6 new fixture cases to 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 in ControlledDelegatecall.sol documenting the accepted narrowing-cast conservative false positive.
  • forge-lint unit tests pass.
  • Clippy clean (-D warnings). cargo fmt --check clean on stable - nightly (used by this repo's CI) wasn't available in this environment to verify against.
  • Two rounds of synchronous Codex adversarial review: first pass found the flash-loan amount/fee false-negative (High) and the bytes20 gap (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).

…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.
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

✅ Changelog found

The deterministic check will validate the changed entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant