Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changelog/lint-underlying-var-numeric-cast.md
Original file line number Diff line number Diff line change
@@ -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.
36 changes: 36 additions & 0 deletions crates/lint/src/sol/analysis/exprs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,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();
Expand All @@ -119,6 +135,26 @@ pub fn underlying_var(expr: &Expr<'_>) -> Option<VariableId> {
}
}

/// 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<VariableId> {
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<VariableId> {
let ExprKind::Ident(reses) = &lhs.peel_parens().kind else { return None };
Expand Down
31 changes: 18 additions & 13 deletions crates/lint/src/sol/high/arbitrary_send_erc20.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, modifier_prefix, receiver_contract_id, state_lhs_vars,
tuple_elems, underlying_var,
tuple_elems, underlying_var, underlying_var_through_numeric_casts,
},
},
};
Expand Down Expand Up @@ -266,8 +266,8 @@ impl<'hir> Analyzer<'hir> {
for &param in modifier.parameters {
// A fact about a rewritten parameter says nothing about the caller's variable.
if !a.written.contains(&param)
&& let Some(caller) =
arg_for_param(self.hir, modifier, param, &m.args).and_then(underlying_var)
&& let Some(caller) = arg_for_param(self.hir, modifier, param, &m.args)
.and_then(underlying_var_through_numeric_casts)
&& self.is_safe_target(caller)
{
if a.state.safe_vars.contains(&param) {
Expand Down Expand Up @@ -332,7 +332,7 @@ impl<'hir> Analyzer<'hir> {
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),
}
}
Expand Down Expand Up @@ -363,7 +363,7 @@ impl<'hir> Analyzer<'hir> {
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);
Expand Down Expand Up @@ -408,7 +408,7 @@ impl<'hir> Analyzer<'hir> {
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) {
Expand Down Expand Up @@ -467,12 +467,14 @@ impl<'hir> Analyzer<'hir> {
}
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 {
Expand All @@ -491,7 +493,8 @@ impl<'hir> Analyzer<'hir> {
/// 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;
};
Expand Down Expand Up @@ -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<TokenKey> {
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,
}
}
Expand Down Expand Up @@ -776,8 +781,8 @@ fn match_flash_loan_call<'hir>(
return None;
}
Some(PendingRepayment {
receiver: underlying_var(recv)?,
token: underlying_var(a[1])?,
receiver: underlying_var_through_numeric_casts(recv)?,
token: underlying_var_through_numeric_casts(a[1])?,
amount: underlying_var(a[2])?,
fee: underlying_var(a[3])?,
})
Expand Down
51 changes: 14 additions & 37 deletions crates/lint/src/sol/high/controlled_delegatecall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ use crate::{
analysis::{
arg_for_param, branch_always_exits, count_placeholders, do_while_user_stmts,
expr_is_address, function_ids, has_side_effect, is_address_like_cast,
is_loop_termination_if, is_require_or_assert, stmts_before_placeholder,
stmts_break_or_continue, tuple_elems, unique, var_is_address_like,
is_loop_termination_if, is_numeric_or_bytes_cast, is_require_or_assert,
stmts_before_placeholder, stmts_break_or_continue, tuple_elems,
underlying_var_through_numeric_casts, unique, var_is_address_like,
},
},
};
Expand All @@ -17,8 +18,8 @@ use solar::{
sema::{
Gcx,
hir::{
self, ElementaryType, Expr, ExprKind, FunctionKind, ItemId, LoopSource, Res, Stmt,
StmtKind, TypeKind, VariableId, Visit,
self, Expr, ExprKind, FunctionKind, ItemId, LoopSource, Res, Stmt, StmtKind,
VariableId, Visit,
},
},
};
Expand Down Expand Up @@ -107,7 +108,9 @@ impl<'hir> Analyzer<'hir> {
}
_ => 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),
Expand Down Expand Up @@ -143,7 +146,7 @@ impl<'hir> Analyzer<'hir> {
}

fn assign_expr(&mut self, lhs: &'hir Expr<'hir>, rhs: Option<&'hir Expr<'hir>>) {
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)));
}
}
Expand Down Expand Up @@ -202,7 +205,7 @@ impl<'hir> Analyzer<'hir> {
} 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);
Expand Down Expand Up @@ -383,7 +386,7 @@ impl<'hir> Visit<'hir> for Analyzer<'hir> {
}
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)
Expand All @@ -393,33 +396,6 @@ impl<'hir> Visit<'hir> for Analyzer<'hir> {
}
}

/// The variable a bare identifier refers to, looking through parens, `payable(...)` and
/// address-like or numeric casts.
fn underlying_var(expr: &Expr<'_>) -> Option<VariableId> {
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 <expr>;` or `<ret> = <expr>;` (optionally followed by a bare `return;`).
fn no_arg_helper_return<'hir>(
Expand All @@ -441,7 +417,8 @@ fn no_arg_helper_return<'hir>(
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)
}
Expand Down Expand Up @@ -473,7 +450,7 @@ fn modifier_safe_vars<'hir>(
.iter()
.filter_map(|&param| {
let arg = arg_for_param(hir, modifier, param, &invocation.args)?;
Some((param, underlying_var(arg)?))
Some((param, underlying_var_through_numeric_casts(arg)?))
})
.collect();
if bindings.is_empty() {
Expand Down
69 changes: 69 additions & 0 deletions crates/lint/testdata/ArbitrarySendErc20.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down
32 changes: 32 additions & 0 deletions crates/lint/testdata/ArbitrarySendErc20.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading
Loading