diff --git a/.changelog/lint-underlying-var-numeric-cast.md b/.changelog/lint-underlying-var-numeric-cast.md new file mode 100644 index 0000000000000..e15764c6519c8 --- /dev/null +++ b/.changelog/lint-underlying-var-numeric-cast.md @@ -0,0 +1,5 @@ +--- +forge-lint: patch +--- + +Fixed a false positive in `arbitrary-send-erc20` where a `permit`/`transferFrom` owner correlated through a numeric cast round-trip (e.g. `address(uint160(rawToken))`) was no longer recognized as the same variable, a regression from the `sol/analysis` consolidation. The peeling now only applies to casts that cannot truncate an address value, so amount/fee correlation in flash-loan repayment tracking stays conservative. diff --git a/crates/lint/src/sol/analysis/exprs.rs b/crates/lint/src/sol/analysis/exprs.rs index fe84998e53fd3..b556bcf665981 100644 --- a/crates/lint/src/sol/analysis/exprs.rs +++ b/crates/lint/src/sol/analysis/exprs.rs @@ -113,6 +113,22 @@ pub fn is_address_like_cast(callee: &Expr<'_>) -> bool { is_address_cast(callee) || is_contract_cast(callee) } +/// `uintN(..)` / `intN(..)` cast head at least as wide as `address` (20 bytes), or a `bytes(..)` +/// cast head - the non-address-like casts that still legitimately wrap an underlying address +/// value (e.g. `address(uint160(rawAddr))`). The width floor matters: peeling through a narrower +/// cast (e.g. `uint8`) would treat a value-truncating round-trip as identity-preserving, which is +/// unsound for any caller trying to prove two expressions reference the same value. +pub fn is_numeric_or_bytes_cast(callee: &Expr<'_>) -> bool { + match &callee.peel_parens().kind { + ExprKind::Type(hir::Type { + kind: TypeKind::Elementary(ElementaryType::Int(size) | ElementaryType::UInt(size)), + .. + }) => size.bytes() >= 20, + ExprKind::Type(hir::Type { kind: TypeKind::Elementary(ElementaryType::Bytes), .. }) => true, + _ => false, + } +} + /// `address(this)`, `payable(this)`, `IFoo(this)`, `IFoo(address(this))`, or bare `this`. pub fn is_address_self(expr: &Expr<'_>) -> bool { let expr = expr.peel_parens(); @@ -138,6 +154,26 @@ pub fn underlying_var(expr: &Expr<'_>) -> Option { } } +/// Like [`underlying_var`], but also looks through `uintN(x)`, `intN(x)` and `bytes(x)` cast +/// heads. Only for callers that correlate two independently-cast references to the *same* +/// address-typed variable (e.g. matching a `permit` owner against a later `transferFrom` `from`) - +/// NOT a general-purpose replacement for `underlying_var`, since peeling an arbitrary numeric cast +/// chain can silently accept a value-truncating round-trip (`uint160 -> uint8 -> uint160`) as if +/// it were identity-preserving, which is exactly what a recipient/target-tracking lint like +/// `unsafe-oz-erc721-mint` must NOT do. +pub fn underlying_var_through_numeric_casts(expr: &Expr<'_>) -> Option { + match &expr.peel_parens().kind { + ExprKind::Ident(reses) => reses.iter().find_map(Res::as_variable), + ExprKind::Call(callee, args, _) + if is_address_like_cast(callee) || is_numeric_or_bytes_cast(callee) => + { + args.exprs().next().and_then(underlying_var_through_numeric_casts) + } + ExprKind::Payable(inner) => underlying_var_through_numeric_casts(inner), + _ => None, + } +} + /// The local (non-state) variable a bare identifier refers to. pub fn lhs_local_var(hir: &hir::Hir<'_>, lhs: &Expr<'_>) -> Option { let ExprKind::Ident(reses) = &lhs.peel_parens().kind else { return None }; diff --git a/crates/lint/src/sol/high/arbitrary_send_erc20.rs b/crates/lint/src/sol/high/arbitrary_send_erc20.rs index 2690a6cc9efb4..396aaa89b7c65 100644 --- a/crates/lint/src/sol/high/arbitrary_send_erc20.rs +++ b/crates/lint/src/sol/high/arbitrary_send_erc20.rs @@ -7,7 +7,7 @@ use crate::{ arg_for_param, branch_always_exits, expr_is_address, function_ids, is_address_like_cast, is_address_self, is_address_type, is_elementary, is_msg_sender, is_require_or_assert, loop_update, modifier_prefix, receiver_contract_id, - state_lhs_vars, tuple_elems, underlying_var, + state_lhs_vars, tuple_elems, underlying_var, underlying_var_through_numeric_casts, }, }, }; @@ -263,8 +263,8 @@ impl<'gcx> Analyzer<'gcx> { for ¶m in modifier.parameters { // A fact about a rewritten parameter says nothing about the caller's variable. if !a.written.contains(¶m) - && let Some(caller) = - arg_for_param(&self.gcx.hir, modifier, param, &m.args).and_then(underlying_var) + && let Some(caller) = arg_for_param(&self.gcx.hir, modifier, param, &m.args) + .and_then(underlying_var_through_numeric_casts) && self.is_safe_target(caller) { if a.state.safe_vars.contains(¶m) { @@ -329,7 +329,7 @@ impl<'gcx> Analyzer<'gcx> { Rhs { safe: self.is_safe(rhs), is_self: self.is_self_expr(rhs), - alias: underlying_var(rhs).map(|v| self.canonical(v)), + alias: underlying_var_through_numeric_casts(rhs).map(|v| self.canonical(v)), sum: sum_operands(rhs), } } @@ -360,7 +360,7 @@ impl<'gcx> Analyzer<'gcx> { fn assign_lhs(&mut self, lhs: &Expr<'_>, rhs: Option<&Expr<'_>>) { // Writing `cfg.token` drops permits keyed on that field. if let ExprKind::Member(base, ident) = &lhs.peel_parens().kind - && let Some(base) = underlying_var(base) + && let Some(base) = underlying_var_through_numeric_casts(base) { let key = TokenKey::Field(self.canonical(base), ident.name); self.state.permits.retain(|p| p.token != key); @@ -405,7 +405,7 @@ impl<'gcx> Analyzer<'gcx> { self.state = after_lhs.meet(&self.state); } else if op.kind == eq { for (a, b) in [(lhs, rhs), (rhs, lhs)] { - if let Some(v) = underlying_var(b) + if let Some(v) = underlying_var_through_numeric_casts(b) && self.is_safe_target(v) { if self.is_safe(a) { @@ -465,12 +465,14 @@ impl<'gcx> Analyzer<'gcx> { } Some(PermitRecord { token: self.canonical_key(token_key(token)?), - owner: self.canonical(underlying_var(owner)?), + owner: self.canonical(underlying_var_through_numeric_casts(owner)?), }) } fn permit_covers(&self, sink: &Sink<'_>) -> bool { - let (Some(token), Some(owner)) = (sink.token, underlying_var(sink.from)) else { + let (Some(token), Some(owner)) = + (sink.token, underlying_var_through_numeric_casts(sink.from)) + else { return false; }; self.state.permits.contains(&PermitRecord { @@ -489,7 +491,8 @@ impl<'gcx> Analyzer<'gcx> { /// Consumes one pending repayment matched by a sink pulling `amount + fee` from the flash-loan /// receiver back to `address(this)`. fn consume_repayment(&mut self, sink: &Sink<'_>) -> bool { - let (Some(from), Some(TokenKey::Var(token))) = (underlying_var(sink.from), sink.token) + let (Some(from), Some(TokenKey::Var(token))) = + (underlying_var_through_numeric_casts(sink.from), sink.token) else { return false; }; @@ -725,11 +728,13 @@ fn sum_operands(expr: &Expr<'_>) -> Option<(VariableId, VariableId)> { /// `token` or `cfg.token` receiver key, through casts and `payable(..)`. fn token_key(expr: &Expr<'_>) -> Option { - if let Some(v) = underlying_var(expr) { + if let Some(v) = underlying_var_through_numeric_casts(expr) { return Some(TokenKey::Var(v)); } match &expr.peel_parens().kind { - ExprKind::Member(base, ident) => Some(TokenKey::Field(underlying_var(base)?, ident.name)), + ExprKind::Member(base, ident) => { + Some(TokenKey::Field(underlying_var_through_numeric_casts(base)?, ident.name)) + } _ => None, } } @@ -772,8 +777,8 @@ fn match_flash_loan_call<'gcx>(gcx: Gcx<'gcx>, expr: &Expr<'gcx>) -> Option Analyzer<'gcx> { } _ => false, }), - ExprKind::Call(callee, args, _) if is_cast(callee) => { + ExprKind::Call(callee, args, _) + if is_address_like_cast(callee) || is_numeric_or_bytes_cast(callee) => + { args.exprs().next().is_some_and(|arg| self.is_trusted_target_inner(arg, depth)) } ExprKind::Payable(inner) => self.is_trusted_target_inner(inner, depth), @@ -140,7 +143,7 @@ impl<'gcx> Analyzer<'gcx> { } fn assign_expr(&mut self, lhs: &'gcx Expr<'gcx>, rhs: Option<&'gcx Expr<'gcx>>) { - if let Some(var) = underlying_var(lhs) { + if let Some(var) = underlying_var_through_numeric_casts(lhs) { self.assign(var, rhs.is_some_and(|rhs| self.is_trusted_target(rhs))); } } @@ -199,7 +202,7 @@ impl<'gcx> Analyzer<'gcx> { } else if op.kind == eq { for (safe, candidate) in [(lhs, rhs), (rhs, lhs)] { if self.is_trusted_target(safe) - && let Some(var) = underlying_var(candidate) + && let Some(var) = underlying_var_through_numeric_casts(candidate) && self.is_trusted_fact_target(var) { self.safe_vars.insert(var); @@ -382,7 +385,7 @@ impl<'gcx> Visit<'gcx> for Analyzer<'gcx> { } ExprKind::Delete(target) => { // `delete` zeroes the target, and the zero address is trusted. - if let Some(var) = underlying_var(target) { + if let Some(var) = underlying_var_through_numeric_casts(target) { self.assign(var, true); } self.walk_expr(expr) @@ -392,33 +395,6 @@ impl<'gcx> Visit<'gcx> for Analyzer<'gcx> { } } -/// The variable a bare identifier refers to, looking through parens, `payable(...)` and -/// address-like or numeric casts. -fn underlying_var(expr: &Expr<'_>) -> Option { - match &expr.peel_parens().kind { - ExprKind::Ident(reses) => reses.iter().find_map(Res::as_variable), - ExprKind::Call(callee, args, _) if is_cast(callee) => { - args.exprs().next().and_then(underlying_var) - } - ExprKind::Payable(inner) => underlying_var(inner), - _ => None, - } -} - -/// `address(..)`, `IFoo(..)`, `uintN(..)`, `intN(..)` or `bytes(..)` cast head. -fn is_cast(callee: &Expr<'_>) -> bool { - is_address_like_cast(callee) - || matches!( - &callee.peel_parens().kind, - ExprKind::Type(hir::Type { - kind: TypeKind::Elementary( - ElementaryType::Int(_) | ElementaryType::UInt(_) | ElementaryType::Bytes - ), - .. - }) - ) -} - /// The expression returned by a non-virtual, non-overriding, parameterless helper whose body is a /// single `return ;` or ` = ;` (optionally followed by a bare `return;`). fn no_arg_helper_return<'gcx>( @@ -440,7 +416,8 @@ fn no_arg_helper_return<'gcx>( StmtKind::Return(Some(expr)) => Some(expr), StmtKind::Expr(expr) => match &expr.peel_parens().kind { ExprKind::Assign(lhs, None, rhs) - if func.returns.len() == 1 && underlying_var(lhs) == Some(func.returns[0]) => + if func.returns.len() == 1 + && underlying_var_through_numeric_casts(lhs) == Some(func.returns[0]) => { Some(rhs) } @@ -471,7 +448,7 @@ fn modifier_safe_vars<'gcx>( .iter() .filter_map(|¶m| { let arg = arg_for_param(&gcx.hir, modifier, param, &invocation.args)?; - Some((param, underlying_var(arg)?)) + Some((param, underlying_var_through_numeric_casts(arg)?)) }) .collect(); if bindings.is_empty() { diff --git a/crates/lint/testdata/ArbitrarySendErc20.sol b/crates/lint/testdata/ArbitrarySendErc20.sol index bdea5de25defb..73d93c358464f 100644 --- a/crates/lint/testdata/ArbitrarySendErc20.sol +++ b/crates/lint/testdata/ArbitrarySendErc20.sol @@ -475,6 +475,36 @@ contract ArbitrarySendErc20 { token.transferFrom(from, to, a); } + // `from` round-tripped through a numeric cast (address -> uint160 -> address) must still + // resolve back to the same underlying variable so the permit correlates with the pull. + function okPermitNumericCastFrom( + address from, + address to, + uint256 a, + uint256 deadline, + uint8 v, + bytes32 r, + bytes32 s + ) public { + token.permit(address(uint160(from)), address(this), a, deadline, v, r, s); + token.transferFrom(address(uint160(from)), to, a); + } + + // Same numeric-cast round-trip, but the pull uses a *different* raw variable - must still warn. + function badPermitNumericCastFromMismatch( + address from, + address other_, + address to, + uint256 a, + uint256 deadline, + uint8 v, + bytes32 r, + bytes32 s + ) public { + token.permit(address(uint160(from)), address(this), a, deadline, v, r, s); + token.transferFrom(address(uint160(other_)), to, a); //~WARN: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + } + // ERC721 same-named methods must NOT trigger this lint. function okErc721TransferFrom(address from, address to, uint256 id) public { nft.transferFrom(from, to, id); @@ -600,6 +630,45 @@ contract ArbitrarySendErc20 { token.transferFrom(address(receiver), address(this), other); //~WARN: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` } + // The callback commits to a truncated amount (via a narrowing cast); the pull-back still + // claims the full untruncated `amount + fee`. Peeling the numeric cast for amount/fee must + // stay narrow (no cast-peeling at all), or this would be wrongly treated as matching. + function badFlashLoanCallbackAmountTruncated( + IERC3156FlashBorrower receiver, + uint256 amount, + uint256 fee, + bytes calldata data + ) public { + receiver.onFlashLoan(msg.sender, address(token), uint160(amount), fee, data); + token.transferFrom(address(receiver), address(this), amount + fee); //~WARN: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + } + + // Same hazard, mirrored: full callback amount, but the pull-back sums a locally truncated + // stand-in for the fee. + function badFlashLoanPullFeeTruncated( + IERC3156FlashBorrower receiver, + uint256 amount, + uint256 fee, + bytes calldata data + ) public { + receiver.onFlashLoan(msg.sender, address(token), amount, fee, data); + token.transferFrom(address(receiver), address(this), amount + uint256(uint160(fee))); //~WARN: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + } + + // Same hazard again, but through the `sum_of` local-alias fallback rather than a direct + // `amount + fee` expression: the pull-back passes a truncated stand-in for the local that + // holds the real sum. + function badFlashLoanSumOfLocalTruncated( + IERC3156FlashBorrower receiver, + uint256 amount, + uint256 fee, + bytes calldata data + ) public { + receiver.onFlashLoan(msg.sender, address(token), amount, fee, data); + uint256 total = amount + fee; + token.transferFrom(address(receiver), address(this), uint256(uint160(total))); //~WARN: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + } + // Second pull-back after the obligation has been consumed. function badFlashLoanDoublePull( IERC3156FlashBorrower receiver, diff --git a/crates/lint/testdata/ArbitrarySendErc20.stderr b/crates/lint/testdata/ArbitrarySendErc20.stderr index 75715184387ec..b9f6d95ab820f 100644 --- a/crates/lint/testdata/ArbitrarySendErc20.stderr +++ b/crates/lint/testdata/ArbitrarySendErc20.stderr @@ -222,6 +222,14 @@ LL │ … token.transferFrom(owner, to, a); │ ╰ help: https://getfoundry.sh/forge/linting/arbitrary-send-erc20 +warning[arbitrary-send-erc20]: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + ╭▸ ROOT/testdata/ArbitrarySendErc20.sol:LL:CC + │ +LL │ … token.transferFrom(address(uint160(other_)), to, a); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/arbitrary-send-erc20 + warning[arbitrary-send-erc20]: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` ╭▸ ROOT/testdata/ArbitrarySendErc20.sol:LL:CC │ @@ -286,6 +294,30 @@ LL │ … token.transferFrom(address(receiver), address(this), amount + fee │ ╰ help: https://getfoundry.sh/forge/linting/arbitrary-send-erc20 +warning[arbitrary-send-erc20]: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + ╭▸ ROOT/testdata/ArbitrarySendErc20.sol:LL:CC + │ +LL │ … token.transferFrom(address(receiver), address(this), amount + uint256(uint160(fee))); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/arbitrary-send-erc20 + +warning[arbitrary-send-erc20]: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + ╭▸ ROOT/testdata/ArbitrarySendErc20.sol:LL:CC + │ +LL │ … token.transferFrom(address(receiver), address(this), uint256(uint160(total))); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/arbitrary-send-erc20 + +warning[arbitrary-send-erc20]: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + ╭▸ ROOT/testdata/ArbitrarySendErc20.sol:LL:CC + │ +LL │ … token.transferFrom(address(receiver), address(this), amount + fee); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/arbitrary-send-erc20 + warning[arbitrary-send-erc20]: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` ╭▸ ROOT/testdata/ArbitrarySendErc20.sol:LL:CC │ diff --git a/crates/lint/testdata/ControlledDelegatecall.sol b/crates/lint/testdata/ControlledDelegatecall.sol index 85d02b7be6843..823dfe9b843f0 100644 --- a/crates/lint/testdata/ControlledDelegatecall.sol +++ b/crates/lint/testdata/ControlledDelegatecall.sol @@ -236,6 +236,13 @@ contract ControlledDelegatecall { (ok,) = address(uint160(0x000000000000000000000000000000000000dEaD)).delegatecall(data); } + // A narrowing cast (uint8) inside an otherwise-trusted numeric chain stops the peel, even + // though the whole expression is a provably-constant zero address. Accepted false positive: + // rejecting narrowing casts is what keeps a genuinely truncating chain from being trusted. + function delegateToNarrowedConstant(bytes calldata data) external returns (bool ok) { + (ok,) = address(uint160(uint8(0))).delegatecall(data); //~WARN: delegatecall target is not provably trusted + } + function delegateToDeleted(address target, bytes calldata data) external returns (bool ok) { address localTarget = target; delete localTarget; diff --git a/crates/lint/testdata/ControlledDelegatecall.stderr b/crates/lint/testdata/ControlledDelegatecall.stderr index 257563abff192..06578580b7e1a 100644 --- a/crates/lint/testdata/ControlledDelegatecall.stderr +++ b/crates/lint/testdata/ControlledDelegatecall.stderr @@ -174,6 +174,14 @@ LL │ (ok,) = localTarget.delegatecall(data); │ ╰ help: https://getfoundry.sh/forge/linting/controlled-delegatecall +warning[controlled-delegatecall]: delegatecall target is not provably trusted + ╭▸ ROOT/testdata/ControlledDelegatecall.sol:LL:CC + │ +LL │ (ok,) = address(uint160(uint8(0))).delegatecall(data); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/controlled-delegatecall + warning[controlled-delegatecall]: delegatecall target is not provably trusted ╭▸ ROOT/testdata/ControlledDelegatecall.sol:LL:CC │