From 2fce59fb6a4b6cb65698ad056bd75374fa8943fd Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Wed, 15 Jul 2026 16:10:12 +0200 Subject: [PATCH 1/9] feat(lint): detect balance reentrancy Extend the existing reentrancy analysis to track locals derived from address(this).balance and report reentrant calls followed by stale balance guards. Register the high-severity lint and add documentation plus focused true- and false-path UI coverage. --- crates/lint/README.md | 1 + crates/lint/docs/reentrancy-balance.md | 49 ++++ crates/lint/src/sol/high/mod.rs | 4 +- crates/lint/src/sol/high/reentrancy.rs | 276 +++++++++++++++++- crates/lint/testdata/ReentrancyBalance.sol | 107 +++++++ crates/lint/testdata/ReentrancyBalance.stderr | 40 +++ 6 files changed, 473 insertions(+), 4 deletions(-) create mode 100644 crates/lint/docs/reentrancy-balance.md create mode 100644 crates/lint/testdata/ReentrancyBalance.sol create mode 100644 crates/lint/testdata/ReentrancyBalance.stderr diff --git a/crates/lint/README.md b/crates/lint/README.md index 223c37a0f4523..6e3ddf8f0eb1e 100644 --- a/crates/lint/README.md +++ b/crates/lint/README.md @@ -17,6 +17,7 @@ It helps enforce best practices and improve code quality within Foundry projects - `controlled-delegatecall`: Flags `delegatecall` calls whose target is not provably trusted. - `encode-packed-collision`: Flags `abi.encodePacked()` calls with multiple dynamic-type arguments (`string`, `bytes`, dynamic arrays) that can produce hash collisions. - `rtlo`: Flags Unicode bidirectional override characters ("Trojan Source", CVE-2021-42574) that can hide malicious code. + - `reentrancy-balance`: Flags reentrant calls between saving `address(this).balance` and checking the current balance against that stale value. - `reentrancy-eth`: Flags uncapped ETH-transferring low-level calls followed by writes to state that was read before the call. - `unprotected-initializer`: Upgradeable initializers should not be callable on the implementation contract. - **Medium Severity:** diff --git a/crates/lint/docs/reentrancy-balance.md b/crates/lint/docs/reentrancy-balance.md new file mode 100644 index 0000000000000..85ba01a8dabb5 --- /dev/null +++ b/crates/lint/docs/reentrancy-balance.md @@ -0,0 +1,49 @@ +# Reentrancy through stale contract balance checks + +**Severity**: `High` +**ID**: `reentrancy-balance` + +Flags reentrant external calls between saving `address(this).balance` and checking the current +contract balance against that saved value. + +## What it does + +Warns when a public or external entry point saves `address(this).balance` in a local value, performs +an external call that can forward enough gas to re-enter, and then uses the saved value together +with a fresh `address(this).balance` read in a `require`, `assert`, or reverting branch. Local +arithmetic derived from the saved balance is tracked, and internal helper calls and modifiers are +analyzed when their bodies are available. + +This detector intentionally covers native ETH held by the current contract. It does not report +token balances, balances of other addresses, view or static calls, calls with a concrete gas cap, +checks on mutually exclusive paths, baselines overwritten after the call, or expressions that do +not compare a fresh contract-balance read with the stale local value. + +## Why is this bad? + +A callback can re-enter the function several times before any invocation reaches its balance +check. Nested invocations can therefore share the same pre-call balance and make one payment appear +to satisfy several operations. A non-strict inequality does not prevent the attack because the +saved baseline, rather than the comparison operator, is stale. + +## Example + +### Bad + +```solidity +function mint(IPayer payer, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + payer.pay(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + _mint(msg.sender, amount); +} +``` + +### Good + +```solidity +function mint(uint256 amount) external payable nonReentrant { + require(msg.value >= amount, "insufficient payment"); + _mint(msg.sender, amount); +} +``` diff --git a/crates/lint/src/sol/high/mod.rs b/crates/lint/src/sol/high/mod.rs index 963e2ab705d1d..127b3dbcdcfc3 100644 --- a/crates/lint/src/sol/high/mod.rs +++ b/crates/lint/src/sol/high/mod.rs @@ -17,7 +17,7 @@ use controlled_delegatecall::CONTROLLED_DELEGATECALL; use encode_packed_collision::ENCODE_PACKED_COLLISION; use incorrect_exp::INCORRECT_EXP; use incorrect_shift::INCORRECT_SHIFT; -use reentrancy::{REENTRANCY_ETH, REENTRANCY_NO_ETH}; +use reentrancy::{REENTRANCY_BALANCE, REENTRANCY_ETH, REENTRANCY_NO_ETH}; use rtlo::RTLO; use unchecked_calls::{ERC20_UNCHECKED_TRANSFER, UNCHECKED_CALL}; use unprotected_initializer::UNPROTECTED_INITIALIZER; @@ -29,7 +29,7 @@ register_lints!( (EncodedPackedCollision, late, (ENCODE_PACKED_COLLISION)), (IncorrectExp, late, (INCORRECT_EXP)), (IncorrectShift, early, (INCORRECT_SHIFT)), - (ReentrancyEth, late, (REENTRANCY_ETH, REENTRANCY_NO_ETH)), + (ReentrancyEth, late, (REENTRANCY_BALANCE, REENTRANCY_ETH, REENTRANCY_NO_ETH)), (UncheckedCall, early, (UNCHECKED_CALL)), (UncheckedTransferERC20, late, (ERC20_UNCHECKED_TRANSFER)), (UnprotectedInitializer, late, (UNPROTECTED_INITIALIZER)), diff --git a/crates/lint/src/sol/high/reentrancy.rs b/crates/lint/src/sol/high/reentrancy.rs index 48326b52d95d2..eeb3111b46e1e 100644 --- a/crates/lint/src/sol/high/reentrancy.rs +++ b/crates/lint/src/sol/high/reentrancy.rs @@ -3,7 +3,10 @@ use crate::{ linter::{LateLintPass, LintContext}, sol::{ Severity, SolLint, - analysis::helper_cache::{DEFAULT_HELPER_ANALYSIS_CACHE_LIMIT, HelperAnalysisCache}, + analysis::{ + helper_cache::{DEFAULT_HELPER_ANALYSIS_CACHE_LIMIT, HelperAnalysisCache}, + primitives::is_require_or_assert, + }, }, }; use solar::{ @@ -22,6 +25,13 @@ use solar::{ }; use std::collections::{BTreeSet, HashMap, HashSet}; +declare_forge_lint!( + REENTRANCY_BALANCE, + Severity::High, + "reentrancy-balance", + "external call can be reentered before a stale contract balance is checked" +); + declare_forge_lint!( REENTRANCY_ETH, Severity::High, @@ -76,6 +86,8 @@ fn is_entry_point(func: &hir::Function<'_>) -> bool { struct FlowState { state_reads: BTreeSet, pending_calls: Vec, + balance_locals: BTreeSet, + pending_balance_calls: Vec, } #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -85,6 +97,12 @@ struct PendingCall { state_reads: BTreeSet, } +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +struct PendingBalanceCall { + span: Span, + stale_locals: BTreeSet, +} + #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] enum ReentrantCallKind { Eth, @@ -113,6 +131,20 @@ impl FlowState { }); } } + + fn push_balance_call(&mut self, span: Span) { + if self.balance_locals.is_empty() { + return; + } + + if let Some(existing) = self.pending_balance_calls.iter_mut().find(|call| call.span == span) + { + existing.stale_locals.extend(self.balance_locals.iter().copied()); + } else { + self.pending_balance_calls + .push(PendingBalanceCall { span, stale_locals: self.balance_locals.clone() }); + } + } } struct Analyzer<'ctx, 's, 'c, 'hir> { @@ -120,12 +152,14 @@ struct Analyzer<'ctx, 's, 'c, 'hir> { gcx: Gcx<'hir>, hir: &'hir hir::Hir<'hir>, emitted: HashSet, + emitted_balance: HashSet, call_stack: Vec, inline_cache: HelperAnalysisCache, recursive_cut_frontiers: HashMap>, direct_internal_calls: HashMap>, reentrancy_eth_enabled: bool, reentrancy_no_eth_enabled: bool, + reentrancy_balance_enabled: bool, } #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -149,17 +183,21 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { gcx, hir, emitted: HashSet::new(), + emitted_balance: HashSet::new(), call_stack: Vec::new(), inline_cache: HelperAnalysisCache::new(DEFAULT_HELPER_ANALYSIS_CACHE_LIMIT), recursive_cut_frontiers: HashMap::new(), direct_internal_calls: HashMap::new(), reentrancy_eth_enabled: ctx.is_lint_enabled(REENTRANCY_ETH.id), reentrancy_no_eth_enabled: ctx.is_lint_enabled(REENTRANCY_NO_ETH.id), + reentrancy_balance_enabled: ctx.is_lint_enabled(REENTRANCY_BALANCE.id), } } const fn has_enabled_lints(&self) -> bool { - self.reentrancy_eth_enabled || self.reentrancy_no_eth_enabled + self.reentrancy_eth_enabled + || self.reentrancy_no_eth_enabled + || self.reentrancy_balance_enabled } fn analyze_callable( @@ -230,6 +268,9 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { StmtKind::DeclSingle(var_id) => { if let Some(init) = self.hir.variable(var_id).initializer { self.analyze_expr(init, state); + if self.reentrancy_balance_enabled { + self.update_balance_local(state, var_id, Some(init), false); + } } true } @@ -266,6 +307,11 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } StmtKind::If(cond, then_stmt, else_stmt) => { self.analyze_expr(cond, state); + if self.reentrancy_balance_enabled + && (branch_reverts(then_stmt) || else_stmt.is_some_and(branch_reverts)) + { + self.emit_balance_calls(cond, state); + } let mut then_state = state.clone(); let then_falls_through = self.analyze_stmt(then_stmt, placeholder, &mut then_state); @@ -328,6 +374,11 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { if !written_vars.is_empty() { self.emit_pending_calls(state, &written_vars); } + if self.reentrancy_balance_enabled + && let Some(var_id) = lhs_local_var(self.hir, lhs) + { + self.update_balance_local(state, var_id, Some(rhs), op.is_some()); + } } ExprKind::Delete(inner) => { self.analyze_lhs_indices(inner, state); @@ -335,6 +386,11 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { if !written_vars.is_empty() { self.emit_pending_calls(state, &written_vars); } + if self.reentrancy_balance_enabled + && let Some(var_id) = lhs_local_var(self.hir, inner) + { + self.update_balance_local(state, var_id, None, false); + } } ExprKind::Unary(op, inner) if matches!( @@ -362,6 +418,13 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.analyze_expr(arg, state); } + if self.reentrancy_balance_enabled + && is_require_or_assert(callee) + && let Some(cond) = args.exprs().next() + { + self.emit_balance_calls(cond, state); + } + for func_id in resolved_function_ids(callee) { self.analyze_internal_call(func_id, state); } @@ -370,6 +433,11 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { { state.push_call(expr.span, kind); } + if self.reentrancy_balance_enabled + && is_balance_reentrant_call(self.gcx, self.hir, callee, args, *opts) + { + state.push_balance_call(expr.span); + } } ExprKind::Binary(lhs, _, rhs) => { self.analyze_expr(lhs, state); @@ -715,6 +783,46 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } } + fn emit_balance_calls(&mut self, guard: &'hir hir::Expr<'hir>, state: &FlowState) { + for call in &state.pending_balance_calls { + if self.emitted_balance.contains(&call.span) + || !guard_has_stale_balance_comparison(guard, &call.stale_locals) + { + continue; + } + + self.ctx.emit(&REENTRANCY_BALANCE, call.span); + self.emitted_balance.insert(call.span); + } + } + + fn update_balance_local( + &self, + state: &mut FlowState, + var_id: VariableId, + value: Option<&'hir hir::Expr<'hir>>, + reads_old_value: bool, + ) { + let balance_dependent = (reads_old_value && state.balance_locals.contains(&var_id)) + || value.is_some_and(|value| { + contains_self_balance(value) || expr_depends_on_locals(value, &state.balance_locals) + }); + + for call in &mut state.pending_balance_calls { + let stale = (reads_old_value && call.stale_locals.contains(&var_id)) + || value.is_some_and(|value| expr_depends_on_locals(value, &call.stale_locals)); + call.stale_locals.remove(&var_id); + if stale { + call.stale_locals.insert(var_id); + } + } + + state.balance_locals.remove(&var_id); + if balance_dependent { + state.balance_locals.insert(var_id); + } + } + fn reentrant_call_kind( &self, callee: &'hir hir::Expr<'hir>, @@ -737,6 +845,8 @@ impl FlowState { fn clear(&mut self) { self.state_reads.clear(); self.pending_calls.clear(); + self.balance_locals.clear(); + self.pending_balance_calls.clear(); } fn merge(&mut self, other: &Self) { @@ -752,6 +862,155 @@ impl FlowState { self.pending_calls.push(call.clone()); } } + self.balance_locals.extend(other.balance_locals.iter().copied()); + for call in &other.pending_balance_calls { + if let Some(existing) = + self.pending_balance_calls.iter_mut().find(|existing| existing.span == call.span) + { + existing.stale_locals.extend(call.stale_locals.iter().copied()); + } else { + self.pending_balance_calls.push(call.clone()); + } + } + } +} + +fn is_balance_reentrant_call<'hir>( + gcx: Gcx<'hir>, + hir: &'hir hir::Hir<'hir>, + callee: &'hir hir::Expr<'hir>, + args: &CallArgs<'hir>, + opts: Option<&hir::CallOptions<'hir>>, +) -> bool { + if !call_options_forward_all_gas(opts) { + return false; + } + + match &callee.peel_parens().kind { + ExprKind::Member(receiver, member) => match member.name { + kw::Call | kw::Callcode | kw::Delegatecall => is_address_like(gcx, hir, receiver), + kw::Staticcall => false, + _ => external_member_call_can_reenter(gcx, hir, receiver, member.name, args), + }, + _ => external_function_pointer_can_reenter(gcx, hir, callee, args), + } +} + +fn call_options_forward_all_gas(opts: Option<&hir::CallOptions<'_>>) -> bool { + opts.and_then(|opts| opts.args.iter().find(|opt| opt.name.name == kw::Gas)) + .is_none_or(|gas| gas_option_forwards_all(&gas.value)) +} + +fn branch_reverts(stmt: &hir::Stmt<'_>) -> bool { + match &stmt.kind { + StmtKind::Revert(_) => true, + StmtKind::Block(block) | StmtKind::UncheckedBlock(block) => { + block.stmts.iter().any(|stmt| branch_reverts(stmt)) + } + StmtKind::If(_, then_stmt, Some(else_stmt)) => { + branch_reverts(then_stmt) && branch_reverts(else_stmt) + } + _ => false, + } +} + +fn guard_has_stale_balance_comparison( + expr: &hir::Expr<'_>, + stale_locals: &BTreeSet, +) -> bool { + match &expr.peel_parens().kind { + ExprKind::Binary(lhs, op, rhs) => { + let is_comparison = matches!( + op.kind, + BinOpKind::Lt + | BinOpKind::Le + | BinOpKind::Gt + | BinOpKind::Ge + | BinOpKind::Eq + | BinOpKind::Ne + ); + (is_comparison + && contains_self_balance(expr) + && expr_depends_on_locals(expr, stale_locals)) + || guard_has_stale_balance_comparison(lhs, stale_locals) + || guard_has_stale_balance_comparison(rhs, stale_locals) + } + ExprKind::Unary(_, inner) | ExprKind::Payable(inner) => { + guard_has_stale_balance_comparison(inner, stale_locals) + } + ExprKind::Ternary(cond, true_expr, false_expr) => { + guard_has_stale_balance_comparison(cond, stale_locals) + || guard_has_stale_balance_comparison(true_expr, stale_locals) + || guard_has_stale_balance_comparison(false_expr, stale_locals) + } + _ => false, + } +} + +fn contains_self_balance(expr: &hir::Expr<'_>) -> bool { + let expr = expr.peel_parens(); + if is_self_balance(expr) { + return true; + } + + match &expr.kind { + ExprKind::Unary(_, inner) | ExprKind::Payable(inner) => contains_self_balance(inner), + ExprKind::Binary(lhs, _, rhs) => contains_self_balance(lhs) || contains_self_balance(rhs), + ExprKind::Ternary(cond, true_expr, false_expr) => { + contains_self_balance(cond) + || contains_self_balance(true_expr) + || contains_self_balance(false_expr) + } + _ => false, + } +} + +fn is_self_balance(expr: &hir::Expr<'_>) -> bool { + matches!( + &expr.peel_parens().kind, + ExprKind::Member(base, member) + if member.as_str() == "balance" && is_self_address(base) + ) +} + +fn is_self_address(expr: &hir::Expr<'_>) -> bool { + match &expr.peel_parens().kind { + ExprKind::Payable(inner) => is_self_address(inner), + ExprKind::Call(callee, args, opts) + if opts.is_none() && is_address_type_expr(callee) && args.exprs().count() == 1 => + { + args.exprs().next().is_some_and(is_self_address) + } + _ => is_this(expr), + } +} + +fn expr_depends_on_locals(expr: &hir::Expr<'_>, locals: &BTreeSet) -> bool { + match &expr.peel_parens().kind { + ExprKind::Ident(reses) => reses.iter().any( + |res| matches!(res, Res::Item(ItemId::Variable(var_id)) if locals.contains(var_id)), + ), + ExprKind::Unary(_, inner) | ExprKind::Payable(inner) => { + expr_depends_on_locals(inner, locals) + } + ExprKind::Binary(lhs, _, rhs) => { + expr_depends_on_locals(lhs, locals) || expr_depends_on_locals(rhs, locals) + } + ExprKind::Ternary(cond, true_expr, false_expr) => { + expr_depends_on_locals(cond, locals) + || expr_depends_on_locals(true_expr, locals) + || expr_depends_on_locals(false_expr, locals) + } + ExprKind::Call(callee, args, opts) + if opts.is_none() + && matches!( + callee.peel_parens().kind, + ExprKind::Type(_) | ExprKind::TypeCall(_) + ) => + { + args.exprs().any(|arg| expr_depends_on_locals(arg, locals)) + } + _ => false, } } @@ -1223,6 +1482,19 @@ fn resolved_function_ids<'hir>( }) } +fn lhs_local_var(hir: &hir::Hir<'_>, lhs: &hir::Expr<'_>) -> Option { + if let ExprKind::Ident(reses) = &lhs.peel_parens().kind { + for res in *reses { + if let Res::Item(ItemId::Variable(var_id)) = res + && !hir.variable(*var_id).kind.is_state() + { + return Some(*var_id); + } + } + } + None +} + fn state_write_lhs_vars(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> Vec { let mut vars = Vec::new(); collect_state_write_lhs_vars(hir, expr, &mut vars); diff --git a/crates/lint/testdata/ReentrancyBalance.sol b/crates/lint/testdata/ReentrancyBalance.sol new file mode 100644 index 0000000000000..4b8ab3a4fc931 --- /dev/null +++ b/crates/lint/testdata/ReentrancyBalance.sol @@ -0,0 +1,107 @@ +//@compile-flags: --only-lint reentrancy-balance + +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.18; + +interface IReentrancyBalanceCallback { + function pay() external; + function observe() external view returns (uint256); +} + +contract ReentrancyBalance { + error InsufficientPayment(); + + function requireAfterCall(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function assertAfterLowLevelCall(address target, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + (bool ok,) = target.call(""); //~WARN: external call can be reentered before a stale contract balance is checked + require(ok, "call failed"); + assert(address(this).balance - balanceBefore >= amount); + } + + function revertingBranchAfterCall(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + if (address(this).balance < balanceBefore + amount) { + revert InsufficientPayment(); + } + } + + function derivedBaseline(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + uint256 minimumBalance = balanceBefore + amount; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= minimumBalance, "insufficient payment"); + } + + function callThroughHelper(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + invoke(callback); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function checkBeforeInteraction(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + callback.pay(); + } + + function overwrittenBaseline(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + callback.pay(); + balanceBefore = address(this).balance; + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function mutuallyExclusivePaths( + IReentrancyBalanceCallback callback, + uint256 amount, + bool saveBalance + ) external { + uint256 balanceBefore; + if (saveBalance) { + balanceBefore = address(this).balance; + } else { + callback.pay(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + } + + function otherAddressBalance( + IReentrancyBalanceCallback callback, + address account, + uint256 amount + ) external { + uint256 balanceBefore = account.balance; + callback.pay(); + require(account.balance >= balanceBefore + amount, "insufficient payment"); + } + + function viewCallCannotReenter(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + callback.observe(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function concreteGasCap(address target, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + (bool ok,) = target.call{gas: 2_300}(""); + require(ok, "call failed"); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function noPostCallBalanceCheck(IReentrancyBalanceCallback callback) external { + uint256 balanceBefore = address(this).balance; + callback.pay(); + require(balanceBefore > 0, "empty balance"); + } + + function invoke(IReentrancyBalanceCallback callback) internal { + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + } +} diff --git a/crates/lint/testdata/ReentrancyBalance.stderr b/crates/lint/testdata/ReentrancyBalance.stderr new file mode 100644 index 0000000000000..bb94b52d2930f --- /dev/null +++ b/crates/lint/testdata/ReentrancyBalance.stderr @@ -0,0 +1,40 @@ +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ (bool ok,) = target.call(""); + │ ━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + From 47e428b7f63ad335fbf545d0650722cf454a4456 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Wed, 15 Jul 2026 17:18:08 +0200 Subject: [PATCH 2/9] fix(lint): harden balance reentrancy Track native self-balance values across casts, tuples, helper boundaries, operand ordering, and loop control flow. Resolve external calls semantically while preserving view, static, storage, and stipend-capped exclusions. --- crates/lint/docs/reentrancy-balance.md | 20 +- crates/lint/src/sol/high/reentrancy.rs | 801 ++++++++++++------ crates/lint/testdata/ReentrancyBalance.sol | 223 ++++- crates/lint/testdata/ReentrancyBalance.stderr | 152 ++++ 4 files changed, 939 insertions(+), 257 deletions(-) diff --git a/crates/lint/docs/reentrancy-balance.md b/crates/lint/docs/reentrancy-balance.md index 85ba01a8dabb5..5487d5274abf1 100644 --- a/crates/lint/docs/reentrancy-balance.md +++ b/crates/lint/docs/reentrancy-balance.md @@ -9,15 +9,17 @@ contract balance against that saved value. ## What it does Warns when a public or external entry point saves `address(this).balance` in a local value, performs -an external call that can forward enough gas to re-enter, and then uses the saved value together -with a fresh `address(this).balance` read in a `require`, `assert`, or reverting branch. Local -arithmetic derived from the saved balance is tracked, and internal helper calls and modifiers are -analyzed when their bodies are available. - -This detector intentionally covers native ETH held by the current contract. It does not report -token balances, balances of other addresses, view or static calls, calls with a concrete gas cap, -checks on mutually exclusive paths, baselines overwritten after the call, or expressions that do -not compare a fresh contract-balance read with the stale local value. +an external call that can forward enough gas to re-enter, and then compares the saved value with a +fresh contract-balance read in a `require`, `assert`, or exiting branch. Casts, tuple assignments, +derived locals, fresh post-call balance locals, internal helper parameters and returns, and +modifiers are tracked when their bodies are available. + +This detector intentionally covers native ETH held by the current contract. It is not the later +token `balanceOf(address)` detector with a similar name. It does not report token balances, +balances of other addresses, mutable state or storage baselines, view or static calls, calls capped +at the 2,300-gas stipend or less, checks on directly mutually exclusive branches, baselines +overwritten after the call, or expressions that do not compare a fresh contract-balance read with +the stale local value. ## Why is this bad? diff --git a/crates/lint/src/sol/high/reentrancy.rs b/crates/lint/src/sol/high/reentrancy.rs index eeb3111b46e1e..a74af45b7ba7a 100644 --- a/crates/lint/src/sol/high/reentrancy.rs +++ b/crates/lint/src/sol/high/reentrancy.rs @@ -5,10 +5,11 @@ use crate::{ Severity, SolLint, analysis::{ helper_cache::{DEFAULT_HELPER_ANALYSIS_CACHE_LIMIT, HelperAnalysisCache}, - primitives::is_require_or_assert, + primitives::{branch_always_exits, is_require_or_assert}, }, }, }; +use alloy_primitives::U256; use solar::{ ast::{ BinOpKind, DataLocation, ElementaryType, LitKind, StateMutability, StrKind, TypeSize, @@ -25,6 +26,8 @@ use solar::{ }; use std::collections::{BTreeSet, HashMap, HashSet}; +const REENTRANCY_GAS_STIPEND: u64 = 2_300; + declare_forge_lint!( REENTRANCY_BALANCE, Severity::High, @@ -60,7 +63,7 @@ impl<'hir> LateLintPass<'hir> for ReentrancyEth { let Some(body) = func.body else { return }; - let mut analyzer = Analyzer::new(ctx, gcx, hir); + let mut analyzer = Analyzer::new(ctx, gcx, hir, func.span); if !analyzer.has_enabled_lints() { return; } @@ -153,13 +156,17 @@ struct Analyzer<'ctx, 's, 'c, 'hir> { hir: &'hir hir::Hir<'hir>, emitted: HashSet, emitted_balance: HashSet, + entry_function_span: Span, call_stack: Vec, - inline_cache: HelperAnalysisCache, + inline_cache: HelperAnalysisCache, recursive_cut_frontiers: HashMap>, direct_internal_calls: HashMap>, reentrancy_eth_enabled: bool, reentrancy_no_eth_enabled: bool, reentrancy_balance_enabled: bool, + balance_only_analysis: bool, + call_balance_values: HashMap>, + return_collectors: Vec, } #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -167,23 +174,55 @@ struct InlineCallKey { func_id: FunctionId, /// First active function that can cut recursion from this callee. recursive_cut: Option, + balance_only: bool, state: FlowState, } +#[derive(Clone, Debug)] +struct InlineCallResult { + state: FlowState, + returns: Vec, +} + #[derive(Clone, Debug, PartialEq, Eq, Hash)] struct RecursiveFrontierKey { func_id: FunctionId, active_call_stack: Vec, } +#[derive(Clone, Debug, Default)] +struct BalanceValue { + balance_dependent: bool, + stale_calls: HashSet, +} + +#[derive(Clone, Copy, Debug)] +enum BalanceQuery { + Any, + Current(Span), + Stale(Span), +} + +#[derive(Debug)] +struct ReturnCollector { + func_id: FunctionId, + values: Vec, +} + impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { - fn new(ctx: &'ctx LintContext<'s, 'c>, gcx: Gcx<'hir>, hir: &'hir hir::Hir<'hir>) -> Self { + fn new( + ctx: &'ctx LintContext<'s, 'c>, + gcx: Gcx<'hir>, + hir: &'hir hir::Hir<'hir>, + entry_function_span: Span, + ) -> Self { Self { ctx, gcx, hir, emitted: HashSet::new(), emitted_balance: HashSet::new(), + entry_function_span, call_stack: Vec::new(), inline_cache: HelperAnalysisCache::new(DEFAULT_HELPER_ANALYSIS_CACHE_LIMIT), recursive_cut_frontiers: HashMap::new(), @@ -191,6 +230,9 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { reentrancy_eth_enabled: ctx.is_lint_enabled(REENTRANCY_ETH.id), reentrancy_no_eth_enabled: ctx.is_lint_enabled(REENTRANCY_NO_ETH.id), reentrancy_balance_enabled: ctx.is_lint_enabled(REENTRANCY_BALANCE.id), + balance_only_analysis: false, + call_balance_values: HashMap::new(), + return_collectors: Vec::new(), } } @@ -237,10 +279,12 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { return self.analyze_modifier_chain(modifiers, index + 1, body, state); }; + self.seed_balance_parameters(modifier_func, &modifier.args, state); self.call_stack.push(modifier_id); let falls_through = self.analyze_block(modifier_body, Some((modifiers, index + 1, body)), state); self.call_stack.pop(); + self.clear_function_balance_locals(modifier_id, state); falls_through } @@ -274,7 +318,14 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } true } - StmtKind::DeclMulti(_, expr) | StmtKind::Expr(expr) => { + StmtKind::DeclMulti(vars, expr) => { + self.analyze_expr(expr, state); + if self.reentrancy_balance_enabled { + self.update_balance_vars(state, vars.iter().copied(), expr); + } + true + } + StmtKind::Expr(expr) => { self.analyze_expr(expr, state); true } @@ -293,6 +344,9 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { if let Some(expr) = expr { self.analyze_expr(expr, state); } + if self.reentrancy_balance_enabled { + self.record_return(expr, state); + } false } StmtKind::Break | StmtKind::Continue => false, @@ -300,15 +354,28 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { let before_loop = state.clone(); let mut body_state = state.clone(); self.analyze_block(block, placeholder, &mut body_state); + // One bounded second iteration exposes loop-carried balance checks while leaving + // the established ETH and no-ETH analysis unchanged. + let second_iteration = self.reentrancy_balance_enabled.then(|| { + let mut second_iteration = body_state.balance_only(); + self.analyze_with_only_balance(|this| { + this.analyze_block(block, placeholder, &mut second_iteration); + }); + second_iteration + }); state.clear(); state.merge(&before_loop); state.merge(&body_state); + if let Some(second_iteration) = second_iteration { + state.merge_balance(&second_iteration); + } true } StmtKind::If(cond, then_stmt, else_stmt) => { self.analyze_expr(cond, state); if self.reentrancy_balance_enabled - && (branch_reverts(then_stmt) || else_stmt.is_some_and(branch_reverts)) + && (branch_stops_current_path(then_stmt) + || else_stmt.is_some_and(branch_stops_current_path)) { self.emit_balance_calls(cond, state); } @@ -374,10 +441,8 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { if !written_vars.is_empty() { self.emit_pending_calls(state, &written_vars); } - if self.reentrancy_balance_enabled - && let Some(var_id) = lhs_local_var(self.hir, lhs) - { - self.update_balance_local(state, var_id, Some(rhs), op.is_some()); + if self.reentrancy_balance_enabled { + self.update_balance_assignment(state, lhs, rhs, op.is_some()); } } ExprKind::Delete(inner) => { @@ -408,14 +473,32 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.analyze_expr(inner, state); } ExprKind::Call(callee, args, opts) => { - self.analyze_expr(callee, state); + let mut operands = Vec::with_capacity(1 + args.len() + usize::from(opts.is_some())); + operands.push(*callee); if let Some(opts) = opts { for opt in opts.args { - self.analyze_expr(&opt.value, state); + operands.push(&opt.value); } } for arg in args.exprs() { - self.analyze_expr(arg, state); + operands.push(arg); + } + + let before_operands = state.clone(); + for operand in &operands { + self.analyze_expr(operand, state); + } + // Solidity does not specify operand evaluation order. Reversing the operands + // covers both relative orders for each pair without changing the shared + // reentrancy analysis. + if self.reentrancy_balance_enabled && operands.len() > 1 { + let mut reverse_state = before_operands.balance_only(); + self.analyze_with_only_balance(|this| { + for operand in operands.iter().rev() { + this.analyze_expr(operand, &mut reverse_state); + } + }); + state.merge_balance(&reverse_state); } if self.reentrancy_balance_enabled @@ -425,8 +508,9 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.emit_balance_calls(cond, state); } - for func_id in resolved_function_ids(callee) { - self.analyze_internal_call(func_id, state); + if let Some(func_id) = self.resolved_internal_function_id(callee) { + let returns = self.analyze_internal_call(func_id, args, state); + self.merge_call_balance_values(expr.span, returns); } if !state.state_reads.is_empty() && let Some(kind) = self.reentrant_call_kind(callee, args, *opts) @@ -498,35 +582,107 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } } - fn analyze_internal_call(&mut self, func_id: FunctionId, state: &mut FlowState) { - if self.call_stack.contains(&func_id) { - return; + fn analyze_internal_call( + &mut self, + func_id: FunctionId, + args: &CallArgs<'hir>, + state: &mut FlowState, + ) -> Vec { + if self.call_stack.contains(&func_id) + || self.hir.function(func_id).span == self.entry_function_span + { + return Vec::new(); } let func = self.hir.function(func_id); - let Some(body) = func.body else { return }; + let Some(body) = func.body else { return Vec::new() }; + + if self.reentrancy_balance_enabled { + self.seed_balance_parameters(func, args, state); + } let key = InlineCallKey { func_id, recursive_cut: self.first_recursive_cut(func_id), + balance_only: self.balance_only_analysis, state: state.clone(), }; if self.inline_cache.is_in_progress(&key) { - return; + self.clear_function_balance_locals(func_id, state); + return Vec::new(); } if let Some(cached) = self.inline_cache.get(&key) { - *state = cached.clone(); - return; + let cached = cached.clone(); + *state = + if self.balance_only_analysis { cached.state.balance_only() } else { cached.state }; + return cached.returns; } let mut after = state.clone(); self.inline_cache.start(key.clone()); + if self.reentrancy_balance_enabled { + self.return_collectors.push(ReturnCollector { + func_id, + values: vec![BalanceValue::default(); func.returns.len()], + }); + } self.call_stack.push(func_id); - self.analyze_callable(func, body, &mut after); + let falls_through = self.analyze_callable(func, body, &mut after); self.call_stack.pop(); - self.inline_cache.finish(key, after.clone()); + let returns = if self.reentrancy_balance_enabled { + if falls_through { + self.record_return(None, &after); + } + self.return_collectors.pop().expect("return collector is active").values + } else { + Vec::new() + }; + self.clear_function_balance_locals(func_id, &mut after); + if self.balance_only_analysis { + after = after.balance_only(); + } + + self.inline_cache + .finish(key, InlineCallResult { state: after.clone(), returns: returns.clone() }); *state = after; + returns + } + + fn resolved_internal_function_id(&self, callee: &'hir hir::Expr<'hir>) -> Option { + match &callee.peel_parens().kind { + ExprKind::Ident(_) => {} + ExprKind::Member(base, _) if is_super(base) => {} + _ => return None, + } + let ty = self.gcx.type_of_expr(callee.peel_parens().id)?; + let TyKind::Fn(function) = ty.kind else { return None }; + if function.is_internal() { function.function_id } else { None } + } + + fn merge_call_balance_values(&mut self, span: Span, values: Vec) { + let stored = self.call_balance_values.entry(span).or_default(); + if stored.len() < values.len() { + stored.resize_with(values.len(), BalanceValue::default); + } + for (stored, value) in stored.iter_mut().zip(values) { + stored.balance_dependent |= value.balance_dependent; + stored.stale_calls.extend(value.stale_calls); + } + } + + fn analyze_with_only_balance(&mut self, f: impl FnOnce(&mut Self) -> T) -> T { + let reentrancy_eth_enabled = self.reentrancy_eth_enabled; + let reentrancy_no_eth_enabled = self.reentrancy_no_eth_enabled; + let balance_only_analysis = self.balance_only_analysis; + self.reentrancy_eth_enabled = false; + self.reentrancy_no_eth_enabled = false; + self.balance_only_analysis = true; + let result = f(self); + self.reentrancy_eth_enabled = reentrancy_eth_enabled; + self.reentrancy_no_eth_enabled = reentrancy_no_eth_enabled; + self.balance_only_analysis = balance_only_analysis; + result } fn first_recursive_cut(&mut self, func_id: FunctionId) -> Option { @@ -677,7 +833,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { for arg in args.exprs() { self.collect_direct_internal_calls_expr(arg, calls); } - for func_id in resolved_function_ids(callee) { + if let Some(func_id) = self.resolved_internal_function_id(callee) { calls.insert(func_id); } } @@ -786,7 +942,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { fn emit_balance_calls(&mut self, guard: &'hir hir::Expr<'hir>, state: &FlowState) { for call in &state.pending_balance_calls { if self.emitted_balance.contains(&call.span) - || !guard_has_stale_balance_comparison(guard, &call.stale_locals) + || !self.guard_has_stale_balance_comparison(guard, call, state) { continue; } @@ -796,21 +952,120 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } } - fn update_balance_local( + fn guard_has_stale_balance_comparison( &self, + expr: &'hir hir::Expr<'hir>, + call: &PendingBalanceCall, + state: &FlowState, + ) -> bool { + match &expr.peel_parens().kind { + ExprKind::Binary(lhs, op, rhs) => { + let is_comparison = matches!( + op.kind, + BinOpKind::Lt + | BinOpKind::Le + | BinOpKind::Gt + | BinOpKind::Ge + | BinOpKind::Eq + | BinOpKind::Ne + ); + let current_locals = state + .balance_locals + .difference(&call.stale_locals) + .copied() + .collect::>(); + (is_comparison + && self.expr_depends_on_balance( + expr, + ¤t_locals, + BalanceQuery::Current(call.span), + ) + && self.expr_depends_on_balance( + expr, + &call.stale_locals, + BalanceQuery::Stale(call.span), + )) + || self.guard_has_stale_balance_comparison(lhs, call, state) + || self.guard_has_stale_balance_comparison(rhs, call, state) + } + ExprKind::Unary(_, inner) | ExprKind::Payable(inner) => { + self.guard_has_stale_balance_comparison(inner, call, state) + } + ExprKind::Ternary(cond, true_expr, false_expr) => { + self.guard_has_stale_balance_comparison(cond, call, state) + || self.guard_has_stale_balance_comparison(true_expr, call, state) + || self.guard_has_stale_balance_comparison(false_expr, call, state) + } + ExprKind::Call(callee, args, opts) + if opts.is_none() + && matches!( + callee.peel_parens().kind, + ExprKind::Type(_) | ExprKind::TypeCall(_) + ) => + { + args.exprs().any(|arg| self.guard_has_stale_balance_comparison(arg, call, state)) + } + _ => false, + } + } + + fn update_balance_local( + &mut self, state: &mut FlowState, var_id: VariableId, value: Option<&'hir hir::Expr<'hir>>, reads_old_value: bool, ) { - let balance_dependent = (reads_old_value && state.balance_locals.contains(&var_id)) - || value.is_some_and(|value| { - contains_self_balance(value) || expr_depends_on_locals(value, &state.balance_locals) - }); + let value = value.map(|value| self.balance_dependency(value, state)).unwrap_or_default(); + self.update_balance_local_with_value(state, var_id, &value, reads_old_value); + } + + fn update_balance_assignment( + &mut self, + state: &mut FlowState, + lhs: &'hir hir::Expr<'hir>, + rhs: &'hir hir::Expr<'hir>, + reads_old_value: bool, + ) { + if let ExprKind::Tuple(lhs_values) = &lhs.peel_parens().kind { + let values = self.balance_dependencies(rhs, state); + for (lhs, value) in lhs_values.iter().zip(values.iter()) { + if let Some(var_id) = lhs.and_then(|lhs| lhs_local_var(self.hir, lhs)) { + self.update_balance_local_with_value(state, var_id, value, false); + } + } + } else if let Some(var_id) = lhs_local_var(self.hir, lhs) { + self.update_balance_local(state, var_id, Some(rhs), reads_old_value); + } + } + + fn update_balance_vars( + &mut self, + state: &mut FlowState, + vars: impl Iterator>, + value: &'hir hir::Expr<'hir>, + ) { + let values = self.balance_dependencies(value, state); + for (var_id, value) in vars.zip(values.iter()) { + if let Some(var_id) = var_id { + self.update_balance_local_with_value(state, var_id, value, false); + } + } + } + + fn update_balance_local_with_value( + &self, + state: &mut FlowState, + var_id: VariableId, + value: &BalanceValue, + reads_old_value: bool, + ) { + let balance_dependent = + value.balance_dependent || (reads_old_value && state.balance_locals.contains(&var_id)); for call in &mut state.pending_balance_calls { - let stale = (reads_old_value && call.stale_locals.contains(&var_id)) - || value.is_some_and(|value| expr_depends_on_locals(value, &call.stale_locals)); + let stale = value.stale_calls.contains(&call.span) + || (reads_old_value && call.stale_locals.contains(&var_id)); call.stale_locals.remove(&var_id); if stale { call.stale_locals.insert(var_id); @@ -823,6 +1078,160 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } } + fn balance_dependencies( + &self, + expr: &'hir hir::Expr<'hir>, + state: &FlowState, + ) -> Vec { + match &expr.peel_parens().kind { + ExprKind::Tuple(exprs) => exprs + .iter() + .map(|expr| { + expr.map(|expr| self.balance_dependency(expr, state)).unwrap_or_default() + }) + .collect(), + ExprKind::Call(_, _, _) => self + .call_balance_values + .get(&expr.span) + .cloned() + .unwrap_or_else(|| vec![self.balance_dependency(expr, state)]), + _ => vec![self.balance_dependency(expr, state)], + } + } + + fn balance_dependency(&self, expr: &'hir hir::Expr<'hir>, state: &FlowState) -> BalanceValue { + let balance_dependent = + self.expr_depends_on_balance(expr, &state.balance_locals, BalanceQuery::Any); + let stale_calls = state + .pending_balance_calls + .iter() + .filter(|call| { + self.expr_depends_on_balance( + expr, + &call.stale_locals, + BalanceQuery::Stale(call.span), + ) + }) + .map(|call| call.span) + .collect(); + BalanceValue { balance_dependent, stale_calls } + } + + fn expr_depends_on_balance( + &self, + expr: &'hir hir::Expr<'hir>, + locals: &BTreeSet, + query: BalanceQuery, + ) -> bool { + let expr = expr.peel_parens(); + if is_self_balance(expr) { + return !matches!(query, BalanceQuery::Stale(_)); + } + + match &expr.kind { + ExprKind::Ident(reses) => reses.iter().any( + |res| matches!(res, Res::Item(ItemId::Variable(var_id)) if locals.contains(var_id)), + ), + ExprKind::Unary(_, inner) | ExprKind::Payable(inner) => { + self.expr_depends_on_balance(inner, locals, query) + } + ExprKind::Binary(lhs, _, rhs) => { + self.expr_depends_on_balance(lhs, locals, query) + || self.expr_depends_on_balance(rhs, locals, query) + } + ExprKind::Ternary(cond, true_expr, false_expr) => { + self.expr_depends_on_balance(cond, locals, query) + || self.expr_depends_on_balance(true_expr, locals, query) + || self.expr_depends_on_balance(false_expr, locals, query) + } + ExprKind::Call(callee, args, opts) + if opts.is_none() + && matches!( + callee.peel_parens().kind, + ExprKind::Type(_) | ExprKind::TypeCall(_) + ) => + { + args.exprs().any(|arg| self.expr_depends_on_balance(arg, locals, query)) + } + ExprKind::Call(_, _, _) => { + self.call_balance_values.get(&expr.span).is_some_and(|values| { + values.iter().any(|value| match query { + BalanceQuery::Any => value.balance_dependent, + BalanceQuery::Current(call) => { + value.balance_dependent && !value.stale_calls.contains(&call) + } + BalanceQuery::Stale(call) => value.stale_calls.contains(&call), + }) + }) + } + _ => false, + } + } + + fn seed_balance_parameters( + &mut self, + func: &'hir hir::Function<'hir>, + args: &CallArgs<'hir>, + state: &mut FlowState, + ) { + if !self.reentrancy_balance_enabled { + return; + } + let values = func + .parameters + .iter() + .enumerate() + .map(|(index, ¶m)| { + let value = argument_for_parameter(self.hir, args, func.parameters, index) + .map(|arg| self.balance_dependency(arg, state)) + .unwrap_or_default(); + (param, value) + }) + .collect::>(); + for (param, value) in values { + self.update_balance_local_with_value(state, param, &value, false); + } + } + + fn record_return(&mut self, expr: Option<&'hir hir::Expr<'hir>>, state: &FlowState) { + let Some(func_id) = self.return_collectors.last().map(|collector| collector.func_id) else { + return; + }; + let func = self.hir.function(func_id); + let values = if let Some(expr) = expr { + self.balance_dependencies(expr, state) + } else { + func.returns + .iter() + .map(|var_id| { + let balance_dependent = state.balance_locals.contains(var_id); + let stale_calls = state + .pending_balance_calls + .iter() + .filter(|call| call.stale_locals.contains(var_id)) + .map(|call| call.span) + .collect(); + BalanceValue { balance_dependent, stale_calls } + }) + .collect() + }; + let collector = self.return_collectors.last_mut().expect("return collector is active"); + for (stored, value) in collector.values.iter_mut().zip(values) { + stored.balance_dependent |= value.balance_dependent; + stored.stale_calls.extend(value.stale_calls); + } + } + + fn clear_function_balance_locals(&self, func_id: FunctionId, state: &mut FlowState) { + let belongs_to_function = |var_id: &VariableId| { + self.hir.variable(*var_id).parent == Some(ItemId::Function(func_id)) + }; + state.balance_locals.retain(|var_id| !belongs_to_function(var_id)); + for call in &mut state.pending_balance_calls { + call.stale_locals.retain(|var_id| !belongs_to_function(var_id)); + } + } + fn reentrant_call_kind( &self, callee: &'hir hir::Expr<'hir>, @@ -873,95 +1282,113 @@ impl FlowState { } } } + + fn balance_only(&self) -> Self { + Self { + balance_locals: self.balance_locals.clone(), + pending_balance_calls: self.pending_balance_calls.clone(), + ..Self::default() + } + } + + fn merge_balance(&mut self, other: &Self) { + self.balance_locals.extend(other.balance_locals.iter().copied()); + for call in &other.pending_balance_calls { + if let Some(existing) = + self.pending_balance_calls.iter_mut().find(|existing| existing.span == call.span) + { + existing.stale_locals.extend(call.stale_locals.iter().copied()); + } else { + self.pending_balance_calls.push(call.clone()); + } + } + } } fn is_balance_reentrant_call<'hir>( gcx: Gcx<'hir>, hir: &'hir hir::Hir<'hir>, callee: &'hir hir::Expr<'hir>, - args: &CallArgs<'hir>, + _args: &CallArgs<'hir>, opts: Option<&hir::CallOptions<'hir>>, ) -> bool { - if !call_options_forward_all_gas(opts) { + if !call_options_allow_reentrancy(hir, opts) { return false; } match &callee.peel_parens().kind { - ExprKind::Member(receiver, member) => match member.name { - kw::Call | kw::Callcode | kw::Delegatecall => is_address_like(gcx, hir, receiver), - kw::Staticcall => false, - _ => external_member_call_can_reenter(gcx, hir, receiver, member.name, args), - }, - _ => external_function_pointer_can_reenter(gcx, hir, callee, args), - } -} - -fn call_options_forward_all_gas(opts: Option<&hir::CallOptions<'_>>) -> bool { - opts.and_then(|opts| opts.args.iter().find(|opt| opt.name.name == kw::Gas)) - .is_none_or(|gas| gas_option_forwards_all(&gas.value)) -} - -fn branch_reverts(stmt: &hir::Stmt<'_>) -> bool { - match &stmt.kind { - StmtKind::Revert(_) => true, - StmtKind::Block(block) | StmtKind::UncheckedBlock(block) => { - block.stmts.iter().any(|stmt| branch_reverts(stmt)) + ExprKind::Member(receiver, _) if is_contract_receiver(gcx, receiver) => { + external_call_can_reenter(gcx, callee) } - StmtKind::If(_, then_stmt, Some(else_stmt)) => { - branch_reverts(then_stmt) && branch_reverts(else_stmt) + ExprKind::Member(receiver, member) + if is_address_like(gcx, hir, receiver) + && matches!( + member.name, + kw::Call | kw::Callcode | kw::Delegatecall | kw::Staticcall + ) => + { + member.name != kw::Staticcall } - _ => false, + ExprKind::Member(receiver, _) if is_super(receiver) => false, + _ => external_call_can_reenter(gcx, callee), } } -fn guard_has_stale_balance_comparison( +fn call_options_allow_reentrancy(hir: &hir::Hir<'_>, opts: Option<&hir::CallOptions<'_>>) -> bool { + let Some(gas) = opts.and_then(|opts| opts.args.iter().find(|opt| opt.name.name == kw::Gas)) + else { + return true; + }; + let mut seen = BTreeSet::new(); + concrete_gas_cap(hir, &gas.value, &mut seen) + .is_none_or(|gas| gas > U256::from(REENTRANCY_GAS_STIPEND)) +} + +fn concrete_gas_cap( + hir: &hir::Hir<'_>, expr: &hir::Expr<'_>, - stale_locals: &BTreeSet, -) -> bool { + seen: &mut BTreeSet, +) -> Option { match &expr.peel_parens().kind { - ExprKind::Binary(lhs, op, rhs) => { - let is_comparison = matches!( - op.kind, - BinOpKind::Lt - | BinOpKind::Le - | BinOpKind::Gt - | BinOpKind::Ge - | BinOpKind::Eq - | BinOpKind::Ne - ); - (is_comparison - && contains_self_balance(expr) - && expr_depends_on_locals(expr, stale_locals)) - || guard_has_stale_balance_comparison(lhs, stale_locals) - || guard_has_stale_balance_comparison(rhs, stale_locals) - } - ExprKind::Unary(_, inner) | ExprKind::Payable(inner) => { - guard_has_stale_balance_comparison(inner, stale_locals) - } - ExprKind::Ternary(cond, true_expr, false_expr) => { - guard_has_stale_balance_comparison(cond, stale_locals) - || guard_has_stale_balance_comparison(true_expr, stale_locals) - || guard_has_stale_balance_comparison(false_expr, stale_locals) + ExprKind::Lit(lit) => match lit.kind { + LitKind::Number(value) => Some(value), + _ => None, + }, + ExprKind::Ident(reses) => { + let var_id = unique(reses.iter().filter_map(|res| match res { + Res::Item(ItemId::Variable(var_id)) => Some(*var_id), + _ => None, + }))?; + let var = hir.variable(var_id); + if !var.is_constant() || !seen.insert(var_id) { + return None; + } + concrete_gas_cap(hir, var.initializer?, seen) } - _ => false, + ExprKind::Call(callee, args, opts) + if opts.is_none() + && matches!( + callee.peel_parens().kind, + ExprKind::Type(_) | ExprKind::TypeCall(_) + ) + && args.exprs().count() == 1 => + { + concrete_gas_cap(hir, args.exprs().next()?, seen) + } + _ => None, } } -fn contains_self_balance(expr: &hir::Expr<'_>) -> bool { - let expr = expr.peel_parens(); - if is_self_balance(expr) { - return true; - } - - match &expr.kind { - ExprKind::Unary(_, inner) | ExprKind::Payable(inner) => contains_self_balance(inner), - ExprKind::Binary(lhs, _, rhs) => contains_self_balance(lhs) || contains_self_balance(rhs), - ExprKind::Ternary(cond, true_expr, false_expr) => { - contains_self_balance(cond) - || contains_self_balance(true_expr) - || contains_self_balance(false_expr) +fn branch_stops_current_path(stmt: &hir::Stmt<'_>) -> bool { + match &stmt.kind { + StmtKind::Break | StmtKind::Continue => true, + StmtKind::Block(block) | StmtKind::UncheckedBlock(block) => { + block.stmts.iter().any(branch_stops_current_path) } - _ => false, + StmtKind::If(_, then_stmt, Some(else_stmt)) => { + branch_stops_current_path(then_stmt) && branch_stops_current_path(else_stmt) + } + _ => branch_always_exits(stmt), } } @@ -985,35 +1412,6 @@ fn is_self_address(expr: &hir::Expr<'_>) -> bool { } } -fn expr_depends_on_locals(expr: &hir::Expr<'_>, locals: &BTreeSet) -> bool { - match &expr.peel_parens().kind { - ExprKind::Ident(reses) => reses.iter().any( - |res| matches!(res, Res::Item(ItemId::Variable(var_id)) if locals.contains(var_id)), - ), - ExprKind::Unary(_, inner) | ExprKind::Payable(inner) => { - expr_depends_on_locals(inner, locals) - } - ExprKind::Binary(lhs, _, rhs) => { - expr_depends_on_locals(lhs, locals) || expr_depends_on_locals(rhs, locals) - } - ExprKind::Ternary(cond, true_expr, false_expr) => { - expr_depends_on_locals(cond, locals) - || expr_depends_on_locals(true_expr, locals) - || expr_depends_on_locals(false_expr, locals) - } - ExprKind::Call(callee, args, opts) - if opts.is_none() - && matches!( - callee.peel_parens().kind, - ExprKind::Type(_) | ExprKind::TypeCall(_) - ) => - { - args.exprs().any(|arg| expr_depends_on_locals(arg, locals)) - } - _ => false, - } -} - fn is_uncapped_value_call( hir: &hir::Hir<'_>, callee: &hir::Expr<'_>, @@ -1042,7 +1440,7 @@ fn is_no_eth_reentrant_call<'hir>( gcx: Gcx<'hir>, hir: &'hir hir::Hir<'hir>, callee: &'hir hir::Expr<'hir>, - args: &CallArgs<'hir>, + _args: &CallArgs<'hir>, opts: Option<&hir::CallOptions<'hir>>, ) -> bool { if call_sends_eth(hir, opts) { @@ -1050,12 +1448,20 @@ fn is_no_eth_reentrant_call<'hir>( } match &callee.peel_parens().kind { - ExprKind::Member(receiver, member) => match member.name { - kw::Call | kw::Callcode | kw::Delegatecall => is_address_like(gcx, hir, receiver), - kw::Staticcall => false, - _ => external_member_call_can_reenter(gcx, hir, receiver, member.name, args), - }, - _ => external_function_pointer_can_reenter(gcx, hir, callee, args), + ExprKind::Member(receiver, _) if is_contract_receiver(gcx, receiver) => { + external_call_can_reenter(gcx, callee) + } + ExprKind::Member(receiver, member) + if is_address_like(gcx, hir, receiver) + && matches!( + member.name, + kw::Call | kw::Callcode | kw::Delegatecall | kw::Staticcall + ) => + { + member.name != kw::Staticcall + } + ExprKind::Member(receiver, _) if is_super(receiver) => false, + _ => external_call_can_reenter(gcx, callee), } } @@ -1065,119 +1471,35 @@ fn call_sends_eth(hir: &hir::Hir<'_>, opts: Option<&hir::CallOptions<'_>>) -> bo }) } -fn external_member_call_can_reenter<'hir>( - gcx: Gcx<'hir>, - hir: &'hir hir::Hir<'hir>, - receiver: &'hir hir::Expr<'hir>, - member: solar::interface::Symbol, - args: &CallArgs<'hir>, -) -> bool { - if is_super(receiver) { - return false; - } - - let Some(receiver_ty) = expr_ty(gcx, hir, receiver) else { return false }; - gcx.members_of(receiver_ty, base_item_source(hir, receiver), base_contract(hir, receiver)) - .filter(|candidate| candidate.name == member) - .any(|candidate| match (candidate.res, candidate.ty.kind) { - (Some(Res::Item(ItemId::Function(function_id))), _) => { - let function = hir.function(function_id); - is_externally_callable(function) - && args_match_function(gcx, hir, args, function.parameters) - && function.mutates_state() - } - (_, TyKind::Fn(function)) => { - is_externally_callable_fn_kind(function.kind) - && args_match_types(gcx, hir, args, function.parameters) - && !matches!( - function.state_mutability, - StateMutability::Pure | StateMutability::View - ) - } - _ => false, - }) -} - -fn external_function_pointer_can_reenter<'hir>( - gcx: Gcx<'hir>, - hir: &'hir hir::Hir<'hir>, - callee: &'hir hir::Expr<'hir>, - args: &CallArgs<'hir>, -) -> bool { - let Some(ty) = expr_ty(gcx, hir, callee) else { return false }; +fn external_call_can_reenter<'hir>(gcx: Gcx<'hir>, callee: &'hir hir::Expr<'hir>) -> bool { + let Some(ty) = gcx.type_of_expr(callee.peel_parens().id) else { return false }; let TyKind::Fn(function) = ty.kind else { return false }; - function.kind == TyFnKind::External - && args_match_types(gcx, hir, args, function.parameters) + is_externally_callable_fn_kind(function.kind) && !matches!(function.state_mutability, StateMutability::Pure | StateMutability::View) } -const fn is_externally_callable(func: &hir::Function<'_>) -> bool { - matches!(func.visibility, Visibility::Public | Visibility::External) -} - const fn is_externally_callable_fn_kind(kind: TyFnKind) -> bool { matches!(kind, TyFnKind::External | TyFnKind::Declaration | TyFnKind::DelegateCall) } -fn args_match_function<'hir>( - gcx: Gcx<'hir>, - hir: &'hir hir::Hir<'hir>, +fn argument_for_parameter<'hir>( + hir: &hir::Hir<'hir>, args: &CallArgs<'hir>, - params: &'hir [VariableId], -) -> bool { - if args.len() != params.len() { - return false; - } - + params: &[VariableId], + index: usize, +) -> Option<&'hir hir::Expr<'hir>> { match args.kind { - CallArgsKind::Unnamed(exprs) => exprs.iter().zip(params).all(|(arg, ¶m)| { - let param = hir.variable(param); - let param_ty = - gcx.type_of_hir_ty(¶m.ty).with_loc_if_ref_opt(gcx, param.data_location); - arg_matches_type(gcx, hir, arg, param_ty) - }), - CallArgsKind::Named(named_args) => named_args.iter().all(|arg| { - params - .iter() - .copied() - .find(|¶m| { - hir.variable(param).name.is_some_and(|name| name.name == arg.name.name) - }) - .is_some_and(|param| { - let param = hir.variable(param); - let param_ty = - gcx.type_of_hir_ty(¶m.ty).with_loc_if_ref_opt(gcx, param.data_location); - arg_matches_type(gcx, hir, &arg.value, param_ty) - }) - }), - } -} - -fn args_match_types<'hir>( - gcx: Gcx<'hir>, - hir: &'hir hir::Hir<'hir>, - args: &CallArgs<'hir>, - params: &'hir [Ty<'hir>], -) -> bool { - if args.len() != params.len() { - return false; - } - - match args.kind { - CallArgsKind::Unnamed(exprs) => { - exprs.iter().zip(params).all(|(arg, ¶m)| arg_matches_type(gcx, hir, arg, param)) + CallArgsKind::Unnamed(exprs) => exprs.get(index), + CallArgsKind::Named(named_args) => { + let name = hir.variable(*params.get(index)?).name?; + named_args.iter().find(|arg| arg.name.name == name.name).map(|arg| &arg.value) } - CallArgsKind::Named(_) => false, } } -fn arg_matches_type<'hir>( - gcx: Gcx<'hir>, - hir: &'hir hir::Hir<'hir>, - arg: &'hir hir::Expr<'hir>, - param_ty: Ty<'hir>, -) -> bool { - expr_ty(gcx, hir, arg).is_some_and(|arg_ty| arg_ty.convert_implicit_to(param_ty, gcx)) +fn is_contract_receiver<'hir>(gcx: Gcx<'hir>, expr: &'hir hir::Expr<'hir>) -> bool { + gcx.type_of_expr(expr.peel_parens().id) + .is_some_and(|ty| matches!(ty.peel_refs().kind, TyKind::Contract(_))) } fn is_address_like<'hir>( @@ -1469,19 +1791,6 @@ fn gas_option_forwards_all(expr: &hir::Expr<'_>) -> bool { ) } -fn resolved_function_ids<'hir>( - callee: &'hir hir::Expr<'hir>, -) -> impl Iterator + 'hir { - let reses = match &callee.peel_parens().kind { - ExprKind::Ident(reses) => *reses, - _ => &[], - }; - reses.iter().filter_map(|res| match res { - Res::Item(ItemId::Function(func_id)) => Some(*func_id), - _ => None, - }) -} - fn lhs_local_var(hir: &hir::Hir<'_>, lhs: &hir::Expr<'_>) -> Option { if let ExprKind::Ident(reses) = &lhs.peel_parens().kind { for res in *reses { diff --git a/crates/lint/testdata/ReentrancyBalance.sol b/crates/lint/testdata/ReentrancyBalance.sol index 4b8ab3a4fc931..fc36f73625d7b 100644 --- a/crates/lint/testdata/ReentrancyBalance.sol +++ b/crates/lint/testdata/ReentrancyBalance.sol @@ -6,15 +6,40 @@ pragma solidity ^0.8.18; interface IReentrancyBalanceCallback { function pay() external; function observe() external view returns (uint256); + function amount() external returns (uint256); +} + +interface IReentrancyBalanceSameNames { + function call(bytes calldata data) external; + function delegatecall(bytes calldata data) external; + function staticcall(bytes calldata data) external; +} + +interface IReentrancyBalanceToken { + function balanceOf(address account) external view returns (uint256); +} + +interface IReentrancyBalanceViewSameName { + function staticcall(bytes calldata data) external view returns (bytes memory); } contract ReentrancyBalance { error InsufficientPayment(); + uint256 private storedBalance; + + modifier checkAfter(uint256 balanceBefore, uint256 amount) { + _; + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + function requireAfterCall(IReentrancyBalanceCallback callback, uint256 amount) external { - uint256 balanceBefore = address(this).balance; + uint256 balanceBefore = payable(address(this)).balance; callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked - require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + require( + address(payable(address(this))).balance >= balanceBefore + amount, + "insufficient payment" + ); } function assertAfterLowLevelCall(address target, uint256 amount) external { @@ -39,12 +64,134 @@ contract ReentrancyBalance { require(address(this).balance >= minimumBalance, "insufficient payment"); } + function castAndFreshLocal(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = uint256(address(this).balance); + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + uint256 balanceAfter = uint256(address(this).balance); + require(balanceAfter >= balanceBefore + amount, "insufficient payment"); + } + + function tupleDeclaration(IReentrancyBalanceCallback callback, uint256 amount) external { + (uint256 balanceBefore, uint256 expected) = (address(this).balance, amount); + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + expected, "insufficient payment"); + } + + function tupleAssignment(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore; + uint256 expected; + (balanceBefore, expected) = (address(this).balance, amount); + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + expected, "insufficient payment"); + } + + function returnedBaseline(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = currentBalance(); + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + checkBalance(balanceBefore, amount); + } + + function modifierParameter(IReentrancyBalanceCallback callback, uint256 amount) + external + checkAfter(address(this).balance, amount) + { + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + } + + function argumentEvaluationOrder( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + uint256 balanceBefore = address(this).balance; + consume( + callback.amount(), //~WARN: external call can be reentered before a stale contract balance is checked + balanceBefore = address(this).balance + ); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function gasCaps( + IReentrancyBalanceCallback callback, + uint256 amount, + uint256 gasAmount + ) external { + uint256 balanceBefore = address(this).balance; + callback.pay{gas: 2_300}(); + callback.pay{gas: 100_000}(); //~WARN: external call can be reentered before a stale contract balance is checked + callback.pay{gas: gasAmount}(); //~WARN: external call can be reentered before a stale contract balance is checked + callback.pay{gas: gasleft() - 1}(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function sameNamedMethods(IReentrancyBalanceSameNames callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + callback.call(""); //~WARN: external call can be reentered before a stale contract balance is checked + callback.delegatecall(""); //~WARN: external call can be reentered before a stale contract balance is checked + callback.staticcall(""); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function viewSameNamedMethod( + IReentrancyBalanceViewSameName callback, + uint256 amount + ) external { + uint256 balanceBefore = address(this).balance; + callback.staticcall(""); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function exitingBranches( + IReentrancyBalanceCallback callback, + uint256 amount, + bool useReturn + ) external { + uint256 balanceBefore = address(this).balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + if (useReturn ? address(this).balance < balanceBefore + amount : false) return; + } + + function plainRevert(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + if (address(this).balance < balanceBefore + amount) revert(); + } + + function loopCarriedCheck(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + for (uint256 i; i < 2; ++i) { + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + } + } + + function continueGuard(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + for (uint256 i; i < 2; ++i) { + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + if (address(this).balance < balanceBefore + amount) continue; + } + } + + function breakGuard(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + for (uint256 i; i < 2; ++i) { + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + if (address(this).balance < balanceBefore + amount) break; + } + } + function callThroughHelper(IReentrancyBalanceCallback callback, uint256 amount) external { uint256 balanceBefore = address(this).balance; invoke(callback); require(address(this).balance >= balanceBefore + amount, "insufficient payment"); } + function recursiveHelper(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + recurse(callback, 1); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + function checkBeforeInteraction(IReentrancyBalanceCallback callback, uint256 amount) external { uint256 balanceBefore = address(this).balance; require(address(this).balance >= balanceBefore + amount, "insufficient payment"); @@ -58,6 +205,12 @@ contract ReentrancyBalance { require(address(this).balance >= balanceBefore + amount, "insufficient payment"); } + function stateBaseline(IReentrancyBalanceCallback callback, uint256 amount) external { + storedBalance = address(this).balance; + callback.pay(); + require(address(this).balance >= storedBalance + amount, "insufficient payment"); + } + function mutuallyExclusivePaths( IReentrancyBalanceCallback callback, uint256 amount, @@ -82,6 +235,27 @@ contract ReentrancyBalance { require(account.balance >= balanceBefore + amount, "insufficient payment"); } + function tokenBalance( + IReentrancyBalanceCallback callback, + IReentrancyBalanceToken token, + uint256 amount + ) external { + uint256 balanceBefore = token.balanceOf(address(this)); + callback.pay(); + require( + token.balanceOf(address(this)) >= balanceBefore + amount, "insufficient payment" + ); + } + + function userDefinedBalanceName( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + uint256 balance = amount; + callback.pay(); + require(address(this).balance >= balance, "insufficient payment"); + } + function viewCallCannotReenter(IReentrancyBalanceCallback callback, uint256 amount) external { uint256 balanceBefore = address(this).balance; callback.observe(); @@ -104,4 +278,49 @@ contract ReentrancyBalance { function invoke(IReentrancyBalanceCallback callback) internal { callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked } + + function currentBalance() internal view returns (uint256) { + return address(this).balance; + } + + function checkBalance(uint256 balanceBefore, uint256 amount) internal view { + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function consume(uint256, uint256) internal pure {} + + function recurse(IReentrancyBalanceCallback callback, uint256 depth) internal { + if (depth > 0) recurse(callback, depth - 1); + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + } +} + +contract ReentrancyBalanceBase { + function invokeBase(IReentrancyBalanceCallback callback) internal virtual { + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + } +} + +contract ReentrancyBalanceDerived is ReentrancyBalanceBase { + function selectedOverride(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + invokeBase(callback); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function selectedSuper(IReentrancyBalanceCallback callback, uint256 amount) external { + uint256 balanceBefore = address(this).balance; + super.invokeBase(callback); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function invokeBase(IReentrancyBalanceCallback) internal override {} +} + +contract ReentrancyBalanceConstructor { + constructor(IReentrancyBalanceCallback callback, uint256 amount) { + uint256 balanceBefore = address(this).balance; + callback.pay(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } } diff --git a/crates/lint/testdata/ReentrancyBalance.stderr b/crates/lint/testdata/ReentrancyBalance.stderr index bb94b52d2930f..5cdaa6aa52392 100644 --- a/crates/lint/testdata/ReentrancyBalance.stderr +++ b/crates/lint/testdata/ReentrancyBalance.stderr @@ -38,3 +38,155 @@ LL │ callback.pay(); │ ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.amount(), + │ ━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay{gas: 100_000}(); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay{gas: gasAmount}(); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay{gas: gasleft() - 1}(); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.call(""); + │ ━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.delegatecall(""); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.staticcall(""); + │ ━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + From 65bf0620d9ed3c8046f25bc2dd61de783929383b Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Thu, 16 Jul 2026 17:52:28 +0200 Subject: [PATCH 3/9] fix(lint): address reentrancy reviews Require fresh and stale balances on opposite comparison operands, model standard state-lock guards across effective entry points, restore bounded recursive entry analysis, and account for value-call gas stipends. --- crates/lint/docs/reentrancy-balance.md | 8 +- crates/lint/src/sol/high/reentrancy.rs | 424 ++++++++++++++++-- crates/lint/testdata/ReentrancyBalance.sol | 209 ++++++++- crates/lint/testdata/ReentrancyBalance.stderr | 80 +++- crates/lint/testdata/ReentrancyNoEth.sol | 11 + crates/lint/testdata/ReentrancyNoEth.stderr | 8 + 6 files changed, 702 insertions(+), 38 deletions(-) diff --git a/crates/lint/docs/reentrancy-balance.md b/crates/lint/docs/reentrancy-balance.md index 5487d5274abf1..5357ad5974aeb 100644 --- a/crates/lint/docs/reentrancy-balance.md +++ b/crates/lint/docs/reentrancy-balance.md @@ -17,9 +17,11 @@ modifiers are tracked when their bodies are available. This detector intentionally covers native ETH held by the current contract. It is not the later token `balanceOf(address)` detector with a similar name. It does not report token balances, balances of other addresses, mutable state or storage baselines, view or static calls, calls capped -at the 2,300-gas stipend or less, checks on directly mutually exclusive branches, baselines -overwritten after the call, or expressions that do not compare a fresh contract-balance read with -the stale local value. +at a total callee budget of 2,300 gas or less after accounting for the value-transfer stipend, +checks on directly mutually exclusive branches, baselines overwritten after the call, or +expressions that do not compare a fresh contract-balance read with the stale local value. It also +recognizes standard state-lock `nonReentrant` modifiers when the same lock guards every mutable +entry point on each concrete deployment of the function. ## Why is this bad? diff --git a/crates/lint/src/sol/high/reentrancy.rs b/crates/lint/src/sol/high/reentrancy.rs index a74af45b7ba7a..dce8e55f5e81a 100644 --- a/crates/lint/src/sol/high/reentrancy.rs +++ b/crates/lint/src/sol/high/reentrancy.rs @@ -63,7 +63,7 @@ impl<'hir> LateLintPass<'hir> for ReentrancyEth { let Some(body) = func.body else { return }; - let mut analyzer = Analyzer::new(ctx, gcx, hir, func.span); + let mut analyzer = Analyzer::new(ctx, gcx, hir, func); if !analyzer.has_enabled_lints() { return; } @@ -91,6 +91,7 @@ struct FlowState { pending_calls: Vec, balance_locals: BTreeSet, pending_balance_calls: Vec, + invalidated_balance_guards: BTreeSet, } #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -156,7 +157,6 @@ struct Analyzer<'ctx, 's, 'c, 'hir> { hir: &'hir hir::Hir<'hir>, emitted: HashSet, emitted_balance: HashSet, - entry_function_span: Span, call_stack: Vec, inline_cache: HelperAnalysisCache, recursive_cut_frontiers: HashMap>, @@ -167,6 +167,8 @@ struct Analyzer<'ctx, 's, 'c, 'hir> { balance_only_analysis: bool, call_balance_values: HashMap>, return_collectors: Vec, + active_balance_guards: Vec, + balance_reentry_lock: Option, } #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -175,9 +177,13 @@ struct InlineCallKey { /// First active function that can cut recursion from this callee. recursive_cut: Option, balance_only: bool, + active_balance_guards: Vec, state: FlowState, } +type ModifierContinuation<'hir> = + (&'hir [hir::Modifier<'hir>], usize, hir::Block<'hir>, Option); + #[derive(Clone, Debug)] struct InlineCallResult { state: FlowState, @@ -203,6 +209,12 @@ enum BalanceQuery { Stale(Span), } +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum LockValue { + Bool(bool), + Number(U256), +} + #[derive(Debug)] struct ReturnCollector { func_id: FunctionId, @@ -214,25 +226,29 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { ctx: &'ctx LintContext<'s, 'c>, gcx: Gcx<'hir>, hir: &'hir hir::Hir<'hir>, - entry_function_span: Span, + entry: &'hir hir::Function<'hir>, ) -> Self { + let reentrancy_balance_enabled = ctx.is_lint_enabled(REENTRANCY_BALANCE.id); Self { ctx, gcx, hir, emitted: HashSet::new(), emitted_balance: HashSet::new(), - entry_function_span, call_stack: Vec::new(), inline_cache: HelperAnalysisCache::new(DEFAULT_HELPER_ANALYSIS_CACHE_LIMIT), recursive_cut_frontiers: HashMap::new(), direct_internal_calls: HashMap::new(), reentrancy_eth_enabled: ctx.is_lint_enabled(REENTRANCY_ETH.id), reentrancy_no_eth_enabled: ctx.is_lint_enabled(REENTRANCY_NO_ETH.id), - reentrancy_balance_enabled: ctx.is_lint_enabled(REENTRANCY_BALANCE.id), + reentrancy_balance_enabled, balance_only_analysis: false, call_balance_values: HashMap::new(), return_collectors: Vec::new(), + active_balance_guards: Vec::new(), + balance_reentry_lock: reentrancy_balance_enabled + .then(|| balance_reentry_lock(gcx, hir, entry)) + .flatten(), } } @@ -281,8 +297,12 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.seed_balance_parameters(modifier_func, &modifier.args, state); self.call_stack.push(modifier_id); - let falls_through = - self.analyze_block(modifier_body, Some((modifiers, index + 1, body)), state); + let balance_guard = self + .reentrancy_balance_enabled + .then(|| standard_reentrancy_guard_lock(self.hir, modifier_func)) + .flatten(); + let continuation = Some((modifiers, index + 1, body, balance_guard)); + let falls_through = self.analyze_block(modifier_body, continuation, state); self.call_stack.pop(); self.clear_function_balance_locals(modifier_id, state); falls_through @@ -291,7 +311,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { fn analyze_block( &mut self, block: hir::Block<'hir>, - placeholder: Option<(&'hir [hir::Modifier<'hir>], usize, hir::Block<'hir>)>, + placeholder: Option>, state: &mut FlowState, ) -> bool { for stmt in block.stmts { @@ -305,7 +325,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { fn analyze_stmt( &mut self, stmt: &'hir hir::Stmt<'hir>, - placeholder: Option<(&'hir [hir::Modifier<'hir>], usize, hir::Block<'hir>)>, + placeholder: Option>, state: &mut FlowState, ) -> bool { match stmt.kind { @@ -419,13 +439,25 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { any_falls_through } StmtKind::Placeholder => { - if let Some((modifiers, index, body)) = placeholder { - self.analyze_modifier_chain(modifiers, index, body, state) + if let Some((modifiers, index, body, balance_guard)) = placeholder { + if let Some(lock_var) = balance_guard { + state.invalidated_balance_guards.remove(&lock_var); + self.active_balance_guards.push(lock_var); + } + let falls_through = self.analyze_modifier_chain(modifiers, index, body, state); + if balance_guard.is_some() { + self.active_balance_guards.pop(); + } + falls_through } else { true } } - StmtKind::AssemblyBlock(_) | StmtKind::Switch(_) | StmtKind::Err(_) => true, + StmtKind::AssemblyBlock(_) | StmtKind::Switch(_) => { + state.invalidated_balance_guards.extend(self.active_balance_guards.iter().copied()); + true + } + StmtKind::Err(_) => true, } } @@ -440,6 +472,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { let written_vars = state_write_lhs_vars(self.hir, lhs); if !written_vars.is_empty() { self.emit_pending_calls(state, &written_vars); + self.invalidate_balance_guards(state, &written_vars); } if self.reentrancy_balance_enabled { self.update_balance_assignment(state, lhs, rhs, op.is_some()); @@ -450,6 +483,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { let written_vars = state_write_lhs_vars(self.hir, inner); if !written_vars.is_empty() { self.emit_pending_calls(state, &written_vars); + self.invalidate_balance_guards(state, &written_vars); } if self.reentrancy_balance_enabled && let Some(var_id) = lhs_local_var(self.hir, inner) @@ -467,12 +501,14 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { let written_vars = state_write_lhs_vars(self.hir, inner); if !written_vars.is_empty() { self.emit_pending_calls(state, &written_vars); + self.invalidate_balance_guards(state, &written_vars); } } ExprKind::Unary(_, inner) => { self.analyze_expr(inner, state); } ExprKind::Call(callee, args, opts) => { + let uses_delegate_context = call_uses_delegate_context(self.gcx, callee); let mut operands = Vec::with_capacity(1 + args.len() + usize::from(opts.is_some())); operands.push(*callee); if let Some(opts) = opts { @@ -519,9 +555,15 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } if self.reentrancy_balance_enabled && is_balance_reentrant_call(self.gcx, self.hir, callee, args, *opts) + && !self.balance_guard_blocks_call(state, callee) { state.push_balance_call(expr.span); } + if uses_delegate_context { + state + .invalidated_balance_guards + .extend(self.active_balance_guards.iter().copied()); + } } ExprKind::Binary(lhs, _, rhs) => { self.analyze_expr(lhs, state); @@ -588,9 +630,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { args: &CallArgs<'hir>, state: &mut FlowState, ) -> Vec { - if self.call_stack.contains(&func_id) - || self.hir.function(func_id).span == self.entry_function_span - { + if self.call_stack.contains(&func_id) { return Vec::new(); } @@ -605,6 +645,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { func_id, recursive_cut: self.first_recursive_cut(func_id), balance_only: self.balance_only_analysis, + active_balance_guards: self.active_balance_guards.clone(), state: state.clone(), }; if self.inline_cache.is_in_progress(&key) { @@ -975,16 +1016,23 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { .copied() .collect::>(); (is_comparison - && self.expr_depends_on_balance( - expr, + && ((self.expr_depends_on_balance( + lhs, ¤t_locals, BalanceQuery::Current(call.span), - ) - && self.expr_depends_on_balance( - expr, + ) && self.expr_depends_on_balance( + rhs, &call.stale_locals, BalanceQuery::Stale(call.span), - )) + )) || (self.expr_depends_on_balance( + rhs, + ¤t_locals, + BalanceQuery::Current(call.span), + ) && self.expr_depends_on_balance( + lhs, + &call.stale_locals, + BalanceQuery::Stale(call.span), + )))) || self.guard_has_stale_balance_comparison(lhs, call, state) || self.guard_has_stale_balance_comparison(rhs, call, state) } @@ -1248,6 +1296,25 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } None } + + fn invalidate_balance_guards(&self, state: &mut FlowState, written_vars: &[VariableId]) { + state.invalidated_balance_guards.extend( + written_vars + .iter() + .filter(|var_id| self.active_balance_guards.contains(var_id)) + .copied(), + ); + } + + fn balance_guard_blocks_call(&self, state: &FlowState, callee: &'hir hir::Expr<'hir>) -> bool { + !call_uses_delegate_context(self.gcx, callee) + && self.balance_reentry_lock.is_some_and(|reentry_lock| { + self.active_balance_guards.iter().any(|lock_var| { + *lock_var == reentry_lock + && !state.invalidated_balance_guards.contains(lock_var) + }) + }) + } } impl FlowState { @@ -1256,6 +1323,7 @@ impl FlowState { self.pending_calls.clear(); self.balance_locals.clear(); self.pending_balance_calls.clear(); + self.invalidated_balance_guards.clear(); } fn merge(&mut self, other: &Self) { @@ -1272,6 +1340,7 @@ impl FlowState { } } self.balance_locals.extend(other.balance_locals.iter().copied()); + self.invalidated_balance_guards.extend(other.invalidated_balance_guards.iter().copied()); for call in &other.pending_balance_calls { if let Some(existing) = self.pending_balance_calls.iter_mut().find(|existing| existing.span == call.span) @@ -1287,12 +1356,14 @@ impl FlowState { Self { balance_locals: self.balance_locals.clone(), pending_balance_calls: self.pending_balance_calls.clone(), + invalidated_balance_guards: self.invalidated_balance_guards.clone(), ..Self::default() } } fn merge_balance(&mut self, other: &Self) { self.balance_locals.extend(other.balance_locals.iter().copied()); + self.invalidated_balance_guards.extend(other.invalidated_balance_guards.iter().copied()); for call in &other.pending_balance_calls { if let Some(existing) = self.pending_balance_calls.iter_mut().find(|existing| existing.span == call.span) @@ -1335,13 +1406,19 @@ fn is_balance_reentrant_call<'hir>( } fn call_options_allow_reentrancy(hir: &hir::Hir<'_>, opts: Option<&hir::CallOptions<'_>>) -> bool { - let Some(gas) = opts.and_then(|opts| opts.args.iter().find(|opt| opt.name.name == kw::Gas)) - else { + let Some(opts) = opts else { return true }; + let Some(gas) = opts.args.iter().find(|opt| opt.name.name == kw::Gas) else { return true; }; + let may_transfer_value = opts + .args + .iter() + .find(|opt| opt.name.name == sym::value) + .is_some_and(|value| !is_zero_value(hir, &value.value)); let mut seen = BTreeSet::new(); - concrete_gas_cap(hir, &gas.value, &mut seen) - .is_none_or(|gas| gas > U256::from(REENTRANCY_GAS_STIPEND)) + concrete_gas_cap(hir, &gas.value, &mut seen).is_none_or(|gas| { + gas > U256::from(REENTRANCY_GAS_STIPEND) || (may_transfer_value && !gas.is_zero()) + }) } fn concrete_gas_cap( @@ -1392,6 +1469,303 @@ fn branch_stops_current_path(stmt: &hir::Stmt<'_>) -> bool { } } +fn standard_reentrancy_guard_lock( + hir: &hir::Hir<'_>, + modifier: &hir::Function<'_>, +) -> Option { + if !matches!(modifier.kind, hir::FunctionKind::Modifier) || !modifier.modifiers.is_empty() { + return None; + } + let body = modifier.body?; + if body.stmts.iter().map(count_modifier_placeholders).sum::() != 1 { + return None; + } + let mut placeholders = body + .stmts + .iter() + .enumerate() + .filter(|(_, stmt)| matches!(stmt.kind, StmtKind::Placeholder)); + let (placeholder_index, _) = placeholders.next()?; + debug_assert!(placeholders.next().is_none()); + + let mut seen = BTreeSet::new(); + let (lock_var, entered) = + guard_activation_from_stmts(hir, &body.stmts[..placeholder_index], &mut seen)?; + let mut seen = BTreeSet::new(); + let (restored_var, restored) = + guard_restoration_from_stmt(hir, body.stmts.get(placeholder_index + 1)?, &mut seen)?; + (lock_var == restored_var && entered != restored).then_some(lock_var) +} + +fn count_modifier_placeholders(stmt: &hir::Stmt<'_>) -> usize { + match stmt.kind { + StmtKind::Placeholder => 1, + StmtKind::Block(block) | StmtKind::UncheckedBlock(block) | StmtKind::Loop(block, _) => { + block.stmts.iter().map(count_modifier_placeholders).sum() + } + StmtKind::If(_, then_stmt, else_stmt) => { + count_modifier_placeholders(then_stmt) + + else_stmt.map_or(0, count_modifier_placeholders) + } + StmtKind::Try(try_stmt) => try_stmt + .clauses + .iter() + .flat_map(|clause| clause.block.stmts) + .map(count_modifier_placeholders) + .sum(), + StmtKind::AssemblyBlock(_) | StmtKind::Switch(_) | StmtKind::Err(_) => 2, + StmtKind::DeclSingle(_) + | StmtKind::DeclMulti(_, _) + | StmtKind::Emit(_) + | StmtKind::Revert(_) + | StmtKind::Return(_) + | StmtKind::Break + | StmtKind::Continue + | StmtKind::Expr(_) => 0, + } +} + +fn balance_reentry_lock<'hir>( + gcx: Gcx<'hir>, + hir: &'hir hir::Hir<'hir>, + entry: &'hir hir::Function<'hir>, +) -> Option { + let entry_id = hir.function_ids().find(|&id| std::ptr::eq(hir.function(id), entry))?; + let defining_contract = entry.contract?; + reentrancy_guard_locks(hir, entry).into_iter().find(|&lock_var| { + let mut has_effective_deployment = false; + for contract_id in hir.contract_ids() { + let contract = hir.contract(contract_id); + if !contract.can_be_deployed() + || contract.is_abstract() + || !contract.linearized_bases.contains(&defining_contract) + { + continue; + } + + let interface = gcx.interface_functions(contract_id); + let entry_is_effective = interface.iter().any(|function| function.id == entry_id) + || contract.fallback == Some(entry_id) + || contract.receive == Some(entry_id); + if !entry_is_effective { + continue; + } + has_effective_deployment = true; + + let ordinary_entries_are_guarded = interface.iter().all(|function| { + let function = hir.function(function.id); + matches!(function.state_mutability, StateMutability::Pure | StateMutability::View) + || function_has_reentrancy_guard(hir, function, lock_var) + }); + let special_entries_are_guarded = + [contract.fallback, contract.receive].into_iter().flatten().all(|function_id| { + function_has_reentrancy_guard(hir, hir.function(function_id), lock_var) + }); + if !ordinary_entries_are_guarded || !special_entries_are_guarded { + return false; + } + } + has_effective_deployment + }) +} + +fn reentrancy_guard_locks(hir: &hir::Hir<'_>, function: &hir::Function<'_>) -> Vec { + function + .modifiers + .iter() + .filter(|modifier| modifier.args.exprs().next().is_none()) + .filter_map(|modifier| modifier.id.as_function()) + .filter_map(|modifier_id| standard_reentrancy_guard_lock(hir, hir.function(modifier_id))) + .collect() +} + +fn function_has_reentrancy_guard( + hir: &hir::Hir<'_>, + function: &hir::Function<'_>, + lock_var: VariableId, +) -> bool { + reentrancy_guard_locks(hir, function).contains(&lock_var) +} + +fn guard_activation_from_stmts( + hir: &hir::Hir<'_>, + stmts: &[hir::Stmt<'_>], + seen: &mut BTreeSet, +) -> Option<(VariableId, LockValue)> { + let (activation, prefix) = stmts.split_last()?; + if let Some((lock_var, entered)) = state_lock_assignment(hir, activation) { + return prefix + .iter() + .any(|stmt| stmt_rejects_lock_value(hir, stmt, lock_var, entered)) + .then_some((lock_var, entered)); + } + + let helper_id = simple_internal_call(activation)?; + if !seen.insert(helper_id) { + return None; + } + let helper = hir.function(helper_id); + let result = if helper.modifiers.is_empty() { + guard_activation_from_stmts(hir, helper.body?.stmts, seen) + } else { + None + }; + seen.remove(&helper_id); + result +} + +fn guard_restoration_from_stmt( + hir: &hir::Hir<'_>, + stmt: &hir::Stmt<'_>, + seen: &mut BTreeSet, +) -> Option<(VariableId, LockValue)> { + if let Some(restoration) = state_lock_assignment(hir, stmt) { + return Some(restoration); + } + + let helper_id = simple_internal_call(stmt)?; + if !seen.insert(helper_id) { + return None; + } + let helper = hir.function(helper_id); + let body = helper.modifiers.is_empty().then_some(helper.body?)?; + let result = match body.stmts { + [stmt] => state_lock_assignment(hir, stmt), + _ => None, + }; + seen.remove(&helper_id); + result +} + +fn simple_internal_call(stmt: &hir::Stmt<'_>) -> Option { + let StmtKind::Expr(expr) = stmt.kind else { return None }; + let ExprKind::Call(callee, args, opts) = &expr.peel_parens().kind else { return None }; + if opts.is_some() || args.exprs().next().is_some() { + return None; + } + let ExprKind::Ident(reses) = &callee.peel_parens().kind else { return None }; + unique(reses.iter().filter_map(|res| match res { + Res::Item(ItemId::Function(func_id)) => Some(*func_id), + _ => None, + })) +} + +fn state_lock_assignment( + hir: &hir::Hir<'_>, + stmt: &hir::Stmt<'_>, +) -> Option<(VariableId, LockValue)> { + let StmtKind::Expr(expr) = stmt.kind else { return None }; + let ExprKind::Assign(lhs, None, rhs) = &expr.peel_parens().kind else { return None }; + let lock_var = direct_state_var(hir, lhs)?; + Some((lock_var, constant_lock_value(hir, rhs)?)) +} + +fn direct_state_var(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> Option { + let ExprKind::Ident(reses) = &expr.peel_parens().kind else { return None }; + unique(reses.iter().filter_map(|res| match res { + Res::Item(ItemId::Variable(var_id)) if hir.variable(*var_id).kind.is_state() => { + Some(*var_id) + } + _ => None, + })) +} + +fn stmt_rejects_lock_value( + hir: &hir::Hir<'_>, + stmt: &hir::Stmt<'_>, + lock_var: VariableId, + entered: LockValue, +) -> bool { + match stmt.kind { + StmtKind::Expr(expr) => { + let ExprKind::Call(callee, args, _) = &expr.peel_parens().kind else { return false }; + is_require_or_assert(callee) + && args.exprs().next().is_some_and(|condition| { + eval_lock_condition(hir, condition, lock_var, entered) == Some(false) + }) + } + StmtKind::If(condition, then_stmt, else_stmt) => { + let condition = eval_lock_condition(hir, condition, lock_var, entered); + (condition == Some(true) && branch_always_exits(then_stmt)) + || (condition == Some(false) && else_stmt.is_some_and(branch_always_exits)) + } + _ => false, + } +} + +fn constant_lock_value(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> Option { + eval_lock_value_inner(hir, expr, None, None, &mut BTreeSet::new()) +} + +fn eval_lock_condition( + hir: &hir::Hir<'_>, + expr: &hir::Expr<'_>, + lock_var: VariableId, + entered: LockValue, +) -> Option { + match eval_lock_value_inner(hir, expr, Some(lock_var), Some(entered), &mut BTreeSet::new())? { + LockValue::Bool(value) => Some(value), + LockValue::Number(_) => None, + } +} + +fn eval_lock_value_inner( + hir: &hir::Hir<'_>, + expr: &hir::Expr<'_>, + lock_var: Option, + entered: Option, + seen: &mut BTreeSet, +) -> Option { + match &expr.peel_parens().kind { + ExprKind::Lit(lit) => match lit.kind { + LitKind::Bool(value) => Some(LockValue::Bool(value)), + LitKind::Number(value) => Some(LockValue::Number(value)), + _ => None, + }, + ExprKind::Ident(reses) => { + let var_id = unique(reses.iter().filter_map(|res| match res { + Res::Item(ItemId::Variable(var_id)) => Some(*var_id), + _ => None, + }))?; + if Some(var_id) == lock_var { + return entered; + } + let var = hir.variable(var_id); + if !var.is_constant() || !seen.insert(var_id) { + return None; + } + let value = eval_lock_value_inner(hir, var.initializer?, lock_var, entered, seen); + seen.remove(&var_id); + value + } + ExprKind::Unary(op, inner) if op.kind == UnOpKind::Not => { + let LockValue::Bool(value) = + eval_lock_value_inner(hir, inner, lock_var, entered, seen)? + else { + return None; + }; + Some(LockValue::Bool(!value)) + } + ExprKind::Binary(lhs, op, rhs) if matches!(op.kind, BinOpKind::Eq | BinOpKind::Ne) => { + let lhs = eval_lock_value_inner(hir, lhs, lock_var, entered, seen)?; + let rhs = eval_lock_value_inner(hir, rhs, lock_var, entered, seen)?; + Some(LockValue::Bool(if op.kind == BinOpKind::Eq { lhs == rhs } else { lhs != rhs })) + } + _ => None, + } +} + +fn call_uses_delegate_context(gcx: Gcx<'_>, callee: &hir::Expr<'_>) -> bool { + if let ExprKind::Member(_, member) = &callee.peel_parens().kind + && matches!(member.name, kw::Callcode | kw::Delegatecall) + { + return true; + } + gcx.type_of_expr(callee.peel_parens().id).is_some_and( + |ty| matches!(ty.kind, TyKind::Fn(function) if function.kind == TyFnKind::DelegateCall), + ) +} + fn is_self_balance(expr: &hir::Expr<'_>) -> bool { matches!( &expr.peel_parens().kind, diff --git a/crates/lint/testdata/ReentrancyBalance.sol b/crates/lint/testdata/ReentrancyBalance.sol index fc36f73625d7b..ca2a98bbf7edb 100644 --- a/crates/lint/testdata/ReentrancyBalance.sol +++ b/crates/lint/testdata/ReentrancyBalance.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.18; interface IReentrancyBalanceCallback { - function pay() external; + function pay() external payable; function observe() external view returns (uint256); function amount() external returns (uint256); } @@ -44,7 +44,7 @@ contract ReentrancyBalance { function assertAfterLowLevelCall(address target, uint256 amount) external { uint256 balanceBefore = address(this).balance; - (bool ok,) = target.call(""); //~WARN: external call can be reentered before a stale contract balance is checked + (bool ok,) = target.call(""); require(ok, "call failed"); assert(address(this).balance - balanceBefore >= amount); } @@ -123,6 +123,26 @@ contract ReentrancyBalance { require(address(this).balance >= balanceBefore + amount, "insufficient payment"); } + function valueStipendGasCap( + IReentrancyBalanceCallback callback, + uint256 amount + ) external payable { + uint256 balanceBefore = address(this).balance; + callback.pay{value: 1, gas: 2_300}(); //~WARN: external call can be reentered before a stale contract balance is checked + callback.pay{value: amount, gas: 2_300}(); //~WARN: external call can be reentered before a stale contract balance is checked + callback.pay{value: 0, gas: 2_300}(); + callback.pay{value: 1, gas: 0}(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function sameOperandBalanceDependencies( + IReentrancyBalanceCallback callback + ) external { + uint256 balanceBefore = address(this).balance; + callback.pay(); + require(address(this).balance + balanceBefore > 0, "empty balance"); + } + function sameNamedMethods(IReentrancyBalanceSameNames callback, uint256 amount) external { uint256 balanceBefore = address(this).balance; callback.call(""); //~WARN: external call can be reentered before a stale contract balance is checked @@ -324,3 +344,188 @@ contract ReentrancyBalanceConstructor { require(address(this).balance >= balanceBefore + amount, "insufficient payment"); } } + +contract ReentrancyBalanceGuarded { + uint256 private locked; + + modifier nonReentrant() { + require(locked == 0, "reentrant"); + locked = 1; + _; + locked = 0; + } + + function guarded( + IReentrancyBalanceCallback callback, + uint256 amount + ) external nonReentrant { + uint256 balanceBefore = address(this).balance; + callback.pay(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } +} + +contract ReentrancyBalanceHelperGuarded { + error ReentrantCall(); + + uint256 private constant NOT_ENTERED = 1; + uint256 private constant ENTERED = 2; + uint256 private status = NOT_ENTERED; + + modifier nonReentrant() { + nonReentrantBefore(); + _; + nonReentrantAfter(); + } + + function guarded( + IReentrancyBalanceCallback callback, + uint256 amount + ) external nonReentrant { + uint256 balanceBefore = address(this).balance; + callback.pay(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function nonReentrantBefore() private { + if (status == ENTERED) revert ReentrantCall(); + status = ENTERED; + } + + function nonReentrantAfter() private { + status = NOT_ENTERED; + } +} + +contract ReentrancyBalanceLaterGuard { + uint256 private locked; + + modifier onlyOwner() { + _; + } + + modifier nonReentrant() { + require(locked == 0, "reentrant"); + locked = 1; + _; + locked = 0; + } + + function guarded( + IReentrancyBalanceCallback callback, + uint256 amount + ) external onlyOwner nonReentrant { + uint256 balanceBefore = address(this).balance; + callback.pay(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } +} + +contract ReentrancyBalanceInvalidatedGuard { + uint256 private locked; + + modifier nonReentrant() { + require(locked == 0, "reentrant"); + locked = 1; + _; + locked = 0; + } + + function unlocksBeforeCall( + IReentrancyBalanceCallback callback, + uint256 amount + ) external nonReentrant { + uint256 balanceBefore = address(this).balance; + locked = 0; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } +} + +contract ReentrancyBalanceNameOnlyGuard { + modifier nonReentrant() { + _; + } + + function guarded( + IReentrancyBalanceCallback callback, + uint256 amount + ) external nonReentrant { + uint256 balanceBefore = address(this).balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } +} + +contract ReentrancyBalanceCrossFunctionGuard { + uint256 private locked; + + modifier nonReentrant() { + require(locked == 0, "reentrant"); + locked = 1; + _; + locked = 0; + } + + function guarded( + IReentrancyBalanceCallback callback, + uint256 amount + ) external nonReentrant { + uint256 balanceBefore = address(this).balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function unguarded( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + uint256 balanceBefore = address(this).balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } +} + +contract ReentrancyBalanceDelegateInvalidatesGuard { + uint256 private locked; + + modifier nonReentrant() { + require(locked == 0, "reentrant"); + locked = 1; + _; + locked = 0; + } + + function guarded( + address target, + IReentrancyBalanceCallback callback, + uint256 amount + ) external nonReentrant { + uint256 balanceBefore = address(this).balance; + (bool ok,) = target.delegatecall(""); //~WARN: external call can be reentered before a stale contract balance is checked + require(ok, "delegatecall failed"); + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } +} + +contract ReentrancyBalanceMultiplePlaceholderGuard { + uint256 private locked; + + modifier nonReentrantTwice() { + require(locked == 0, "reentrant"); + locked = 1; + _; + locked = 0; + _; + } + + function guarded( + IReentrancyBalanceCallback callback, + uint256 amount + ) external nonReentrantTwice { + uint256 balanceBefore = address(this).balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } +} diff --git a/crates/lint/testdata/ReentrancyBalance.stderr b/crates/lint/testdata/ReentrancyBalance.stderr index 5cdaa6aa52392..a4e774732fd78 100644 --- a/crates/lint/testdata/ReentrancyBalance.stderr +++ b/crates/lint/testdata/ReentrancyBalance.stderr @@ -6,14 +6,6 @@ LL │ callback.pay(); │ ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance -warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked - ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC - │ -LL │ (bool ok,) = target.call(""); - │ ━━━━━━━━━━━━━━━ - │ - ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance - warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC │ @@ -102,6 +94,22 @@ LL │ callback.pay{gas: gasleft() - 1}(); │ ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay{value: 1, gas: 2_300}(); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay{value: amount, gas: 2_300}(); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC │ @@ -190,3 +198,59 @@ LL │ callback.pay(); │ ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ (bool ok,) = target.delegatecall(""); + │ ━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + diff --git a/crates/lint/testdata/ReentrancyNoEth.sol b/crates/lint/testdata/ReentrancyNoEth.sol index 0f96da069a6b3..0ef1430fffd28 100644 --- a/crates/lint/testdata/ReentrancyNoEth.sol +++ b/crates/lint/testdata/ReentrancyNoEth.sol @@ -121,6 +121,17 @@ contract ReentrancyNoEth { balances[msg.sender] = 0; } + function recursiveEntryPoint(uint256 depth, address target) public { + if (depth != 0) { + uint256 snapshot = locked; + recursiveEntryPoint(depth - 1, target); + locked = snapshot; + } else { + (bool ok,) = target.call(""); //~WARN: external call can be reentered before `locked` is updated + require(ok, "call failed"); + } + } + constructor(IHook hook) { uint256 amount = balances[msg.sender]; hook.notify(amount); diff --git a/crates/lint/testdata/ReentrancyNoEth.stderr b/crates/lint/testdata/ReentrancyNoEth.stderr index 3d61fec7c6136..2bd37011d0662 100644 --- a/crates/lint/testdata/ReentrancyNoEth.stderr +++ b/crates/lint/testdata/ReentrancyNoEth.stderr @@ -78,3 +78,11 @@ LL │ delete balances[hook.key()]; │ ╰ help: https://getfoundry.sh/forge/linting/reentrancy-no-eth +warning[reentrancy-no-eth]: external call can be reentered before `locked` is updated + ╭▸ ROOT/testdata/ReentrancyNoEth.sol:LL:CC + │ +LL │ (bool ok,) = target.call(""); + │ ━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-no-eth + From e14c09e7e89ca93b363e689722e9a117b7ca6ae2 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Thu, 16 Jul 2026 22:33:11 +0200 Subject: [PATCH 4/9] fix(lint): preserve balance flow facts Carry stale-balance comparison and boolean path provenance through helper returns, compound assignments, and control-flow joins. This avoids false positives on complementary conditions without hiding reachable callback paths. --- crates/lint/src/sol/high/reentrancy.rs | 419 +++++++++++++++++- crates/lint/testdata/ReentrancyBalance.sol | 185 ++++++++ crates/lint/testdata/ReentrancyBalance.stderr | 88 ++++ 3 files changed, 672 insertions(+), 20 deletions(-) diff --git a/crates/lint/src/sol/high/reentrancy.rs b/crates/lint/src/sol/high/reentrancy.rs index dce8e55f5e81a..607a21420c987 100644 --- a/crates/lint/src/sol/high/reentrancy.rs +++ b/crates/lint/src/sol/high/reentrancy.rs @@ -24,7 +24,7 @@ use solar::{ ty::{TyFnKind, TyKind}, }, }; -use std::collections::{BTreeSet, HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; const REENTRANCY_GAS_STIPEND: u64 = 2_300; @@ -90,8 +90,11 @@ struct FlowState { state_reads: BTreeSet, pending_calls: Vec, balance_locals: BTreeSet, + balance_local_paths: BTreeMap, + balance_comparison_locals: BTreeMap>, pending_balance_calls: Vec, invalidated_balance_guards: BTreeSet, + path_predicates: PathPredicates, } #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -107,6 +110,9 @@ struct PendingBalanceCall { stale_locals: BTreeSet, } +type PathPredicates = BTreeMap; +type PathAlternatives = BTreeSet; + #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] enum ReentrantCallKind { Eth, @@ -137,16 +143,25 @@ impl FlowState { } fn push_balance_call(&mut self, span: Span) { - if self.balance_locals.is_empty() { + let stale_locals = self + .balance_locals + .iter() + .filter(|var_id| { + self.balance_local_paths + .get(var_id) + .is_some_and(|paths| paths_compatible_with(paths, &self.path_predicates)) + }) + .copied() + .collect::>(); + if stale_locals.is_empty() { return; } if let Some(existing) = self.pending_balance_calls.iter_mut().find(|call| call.span == span) { - existing.stale_locals.extend(self.balance_locals.iter().copied()); + existing.stale_locals.extend(stale_locals); } else { - self.pending_balance_calls - .push(PendingBalanceCall { span, stale_locals: self.balance_locals.clone() }); + self.pending_balance_calls.push(PendingBalanceCall { span, stale_locals }); } } } @@ -178,6 +193,7 @@ struct InlineCallKey { recursive_cut: Option, balance_only: bool, active_balance_guards: Vec, + parameter_predicates: Vec>, state: FlowState, } @@ -199,12 +215,13 @@ struct RecursiveFrontierKey { #[derive(Clone, Debug, Default)] struct BalanceValue { balance_dependent: bool, + balance_paths: PathAlternatives, stale_calls: HashSet, + stale_comparisons: Vec, } #[derive(Clone, Copy, Debug)] enum BalanceQuery { - Any, Current(Span), Stale(Span), } @@ -386,9 +403,16 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { state.clear(); state.merge(&before_loop); state.merge(&body_state); + let mut path_predicates = common_path_predicates( + &before_loop.path_predicates, + &body_state.path_predicates, + ); if let Some(second_iteration) = second_iteration { + path_predicates = + common_path_predicates(&path_predicates, &second_iteration.path_predicates); state.merge_balance(&second_iteration); } + state.path_predicates = path_predicates; true } StmtKind::If(cond, then_stmt, else_stmt) => { @@ -401,10 +425,21 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } let mut then_state = state.clone(); - let then_falls_through = self.analyze_stmt(then_stmt, placeholder, &mut then_state); - let mut else_state = state.clone(); - let else_falls_through = if let Some(else_stmt) = else_stmt { + let predicate = self + .reentrancy_balance_enabled + .then(|| path_predicate(self.hir, cond)) + .flatten(); + let then_reachable = + predicate.is_none_or(|predicate| then_state.constrain_path(predicate)); + let else_reachable = predicate + .is_none_or(|(var_id, value)| else_state.constrain_path((var_id, !value))); + let then_falls_through = + then_reachable && self.analyze_stmt(then_stmt, placeholder, &mut then_state); + + let else_falls_through = if !else_reachable { + false + } else if let Some(else_stmt) = else_stmt { self.analyze_stmt(else_stmt, placeholder, &mut else_state) } else { true @@ -417,6 +452,15 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { if else_falls_through { state.merge(&else_state); } + state.path_predicates = match (then_falls_through, else_falls_through) { + (true, true) => common_path_predicates( + &then_state.path_predicates, + &else_state.path_predicates, + ), + (true, false) => then_state.path_predicates, + (false, true) => else_state.path_predicates, + (false, false) => PathPredicates::new(), + }; then_falls_through || else_falls_through } @@ -424,6 +468,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.analyze_expr(&try_stmt.expr, state); let mut merged = FlowState::default(); + let mut path_predicates = None; let mut any_falls_through = false; for clause in try_stmt.clauses { let mut clause_state = state.clone(); @@ -431,11 +476,19 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.analyze_block(clause.block, placeholder, &mut clause_state); if falls_through { merged.merge(&clause_state); + path_predicates = Some(match path_predicates { + Some(path_predicates) => common_path_predicates( + &path_predicates, + &clause_state.path_predicates, + ), + None => clause_state.path_predicates.clone(), + }); any_falls_through = true; } } *state = merged; + state.path_predicates = path_predicates.unwrap_or_default(); any_falls_through } StmtKind::Placeholder => { @@ -474,6 +527,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.emit_pending_calls(state, &written_vars); self.invalidate_balance_guards(state, &written_vars); } + forget_path_predicates(state, local_write_lhs_vars(self.hir, lhs)); if self.reentrancy_balance_enabled { self.update_balance_assignment(state, lhs, rhs, op.is_some()); } @@ -485,6 +539,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.emit_pending_calls(state, &written_vars); self.invalidate_balance_guards(state, &written_vars); } + forget_path_predicates(state, local_write_lhs_vars(self.hir, inner)); if self.reentrancy_balance_enabled && let Some(var_id) = lhs_local_var(self.hir, inner) { @@ -503,6 +558,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.emit_pending_calls(state, &written_vars); self.invalidate_balance_guards(state, &written_vars); } + forget_path_predicates(state, local_write_lhs_vars(self.hir, inner)); } ExprKind::Unary(_, inner) => { self.analyze_expr(inner, state); @@ -588,14 +644,38 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.analyze_expr(cond, state); let mut true_state = state.clone(); - self.analyze_expr(true_expr, &mut true_state); - let mut false_state = state.clone(); - self.analyze_expr(false_expr, &mut false_state); + let predicate = self + .reentrancy_balance_enabled + .then(|| path_predicate(self.hir, cond)) + .flatten(); + let true_reachable = + predicate.is_none_or(|predicate| true_state.constrain_path(predicate)); + let false_reachable = predicate + .is_none_or(|(var_id, value)| false_state.constrain_path((var_id, !value))); + if true_reachable { + self.analyze_expr(true_expr, &mut true_state); + } + if false_reachable { + self.analyze_expr(false_expr, &mut false_state); + } state.clear(); - state.merge(&true_state); - state.merge(&false_state); + if true_reachable { + state.merge(&true_state); + } + if false_reachable { + state.merge(&false_state); + } + state.path_predicates = match (true_reachable, false_reachable) { + (true, true) => common_path_predicates( + &true_state.path_predicates, + &false_state.path_predicates, + ), + (true, false) => true_state.path_predicates, + (false, true) => false_state.path_predicates, + (false, false) => PathPredicates::new(), + }; } ExprKind::Array(exprs) => { for expr in *exprs { @@ -640,12 +720,25 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { if self.reentrancy_balance_enabled { self.seed_balance_parameters(func, args, state); } + let parameter_predicates = if self.reentrancy_balance_enabled { + func.parameters + .iter() + .enumerate() + .map(|(index, _)| { + argument_for_parameter(self.hir, args, func.parameters, index) + .and_then(|arg| path_predicate(self.hir, arg)) + }) + .collect::>() + } else { + Vec::new() + }; let key = InlineCallKey { func_id, recursive_cut: self.first_recursive_cut(func_id), balance_only: self.balance_only_analysis, active_balance_guards: self.active_balance_guards.clone(), + parameter_predicates: parameter_predicates.clone(), state: state.clone(), }; if self.inline_cache.is_in_progress(&key) { @@ -671,7 +764,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { let falls_through = self.analyze_callable(func, body, &mut after); self.call_stack.pop(); - let returns = if self.reentrancy_balance_enabled { + let mut returns = if self.reentrancy_balance_enabled { if falls_through { self.record_return(None, &after); } @@ -679,6 +772,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } else { Vec::new() }; + remap_return_paths(self.hir, func_id, func.parameters, ¶meter_predicates, &mut returns); self.clear_function_balance_locals(func_id, &mut after); if self.balance_only_analysis { after = after.balance_only(); @@ -708,7 +802,9 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } for (stored, value) in stored.iter_mut().zip(values) { stored.balance_dependent |= value.balance_dependent; + stored.balance_paths.extend(value.balance_paths); stored.stale_calls.extend(value.stale_calls); + extend_unique(&mut stored.stale_comparisons, value.stale_comparisons); } } @@ -1053,6 +1149,17 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { { args.exprs().any(|arg| self.guard_has_stale_balance_comparison(arg, call, state)) } + ExprKind::Call(_, _, _) => { + self.call_balance_values.get(&expr.peel_parens().span).is_some_and(|values| { + values.iter().any(|value| value.stale_comparisons.contains(&call.span)) + }) + } + ExprKind::Ident(reses) => reses.iter().any(|res| { + matches!(res, Res::Item(ItemId::Variable(var_id)) if state + .balance_comparison_locals + .get(var_id) + .is_some_and(|calls| calls.contains(&call.span))) + }), _ => false, } } @@ -1110,6 +1217,16 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { ) { let balance_dependent = value.balance_dependent || (reads_old_value && state.balance_locals.contains(&var_id)); + let mut balance_paths = value.balance_paths.clone(); + let mut stale_comparisons = value.stale_comparisons.clone(); + if reads_old_value { + if let Some(old_paths) = state.balance_local_paths.get(&var_id) { + balance_paths.extend(old_paths.iter().cloned()); + } + if let Some(old_comparisons) = state.balance_comparison_locals.get(&var_id) { + extend_unique(&mut stale_comparisons, old_comparisons.iter().copied()); + } + } for call in &mut state.pending_balance_calls { let stale = value.stale_calls.contains(&call.span) @@ -1121,8 +1238,14 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } state.balance_locals.remove(&var_id); + state.balance_local_paths.remove(&var_id); + state.balance_comparison_locals.remove(&var_id); if balance_dependent { state.balance_locals.insert(var_id); + state.balance_local_paths.insert(var_id, balance_paths); + } + if !stale_comparisons.is_empty() { + state.balance_comparison_locals.insert(var_id, stale_comparisons); } } @@ -1148,8 +1271,8 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } fn balance_dependency(&self, expr: &'hir hir::Expr<'hir>, state: &FlowState) -> BalanceValue { - let balance_dependent = - self.expr_depends_on_balance(expr, &state.balance_locals, BalanceQuery::Any); + let balance_paths = self.expr_balance_paths(expr, state); + let balance_dependent = !balance_paths.is_empty(); let stale_calls = state .pending_balance_calls .iter() @@ -1162,7 +1285,76 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { }) .map(|call| call.span) .collect(); - BalanceValue { balance_dependent, stale_calls } + let stale_comparisons = state + .pending_balance_calls + .iter() + .filter(|call| self.guard_has_stale_balance_comparison(expr, call, state)) + .map(|call| call.span) + .collect(); + BalanceValue { balance_dependent, balance_paths, stale_calls, stale_comparisons } + } + + fn expr_balance_paths( + &self, + expr: &'hir hir::Expr<'hir>, + state: &FlowState, + ) -> PathAlternatives { + let expr = expr.peel_parens(); + if is_self_balance(expr) { + return [state.path_predicates.clone()].into_iter().collect(); + } + + match &expr.kind { + ExprKind::Ident(reses) => { + let mut paths = PathAlternatives::new(); + for var_id in reses.iter().filter_map(|res| match res { + Res::Item(ItemId::Variable(var_id)) => Some(var_id), + _ => None, + }) { + if let Some(local_paths) = state.balance_local_paths.get(var_id) { + paths.extend(constrain_paths(local_paths, &state.path_predicates)); + } + } + paths + } + ExprKind::Unary(_, inner) | ExprKind::Payable(inner) => { + self.expr_balance_paths(inner, state) + } + ExprKind::Binary(lhs, _, rhs) => { + let mut paths = self.expr_balance_paths(lhs, state); + paths.extend(self.expr_balance_paths(rhs, state)); + paths + } + ExprKind::Ternary(cond, true_expr, false_expr) => { + let mut paths = self.expr_balance_paths(cond, state); + paths.extend(self.expr_balance_paths(true_expr, state)); + paths.extend(self.expr_balance_paths(false_expr, state)); + paths + } + ExprKind::Call(callee, args, opts) + if opts.is_none() + && matches!( + callee.peel_parens().kind, + ExprKind::Type(_) | ExprKind::TypeCall(_) + ) => + { + let mut paths = PathAlternatives::new(); + for arg in args.exprs() { + paths.extend(self.expr_balance_paths(arg, state)); + } + paths + } + ExprKind::Call(_, _, _) => { + let mut paths = PathAlternatives::new(); + if let Some(values) = self.call_balance_values.get(&expr.span) { + for value in values { + paths.extend(constrain_paths(&value.balance_paths, &state.path_predicates)); + } + } + paths + } + _ => PathAlternatives::new(), + } } fn expr_depends_on_balance( @@ -1204,7 +1396,6 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { ExprKind::Call(_, _, _) => { self.call_balance_values.get(&expr.span).is_some_and(|values| { values.iter().any(|value| match query { - BalanceQuery::Any => value.balance_dependent, BalanceQuery::Current(call) => { value.balance_dependent && !value.stale_calls.contains(&call) } @@ -1253,20 +1444,31 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { .iter() .map(|var_id| { let balance_dependent = state.balance_locals.contains(var_id); + let balance_paths = + state.balance_local_paths.get(var_id).cloned().unwrap_or_default(); let stale_calls = state .pending_balance_calls .iter() .filter(|call| call.stale_locals.contains(var_id)) .map(|call| call.span) .collect(); - BalanceValue { balance_dependent, stale_calls } + let stale_comparisons = + state.balance_comparison_locals.get(var_id).cloned().unwrap_or_default(); + BalanceValue { + balance_dependent, + balance_paths, + stale_calls, + stale_comparisons, + } }) .collect() }; let collector = self.return_collectors.last_mut().expect("return collector is active"); for (stored, value) in collector.values.iter_mut().zip(values) { stored.balance_dependent |= value.balance_dependent; + stored.balance_paths.extend(value.balance_paths); stored.stale_calls.extend(value.stale_calls); + extend_unique(&mut stored.stale_comparisons, value.stale_comparisons); } } @@ -1275,6 +1477,9 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.hir.variable(*var_id).parent == Some(ItemId::Function(func_id)) }; state.balance_locals.retain(|var_id| !belongs_to_function(var_id)); + state.balance_local_paths.retain(|var_id, _| !belongs_to_function(var_id)); + state.balance_comparison_locals.retain(|var_id, _| !belongs_to_function(var_id)); + state.path_predicates.retain(|var_id, _| !belongs_to_function(var_id)); for call in &mut state.pending_balance_calls { call.stale_locals.retain(|var_id| !belongs_to_function(var_id)); } @@ -1322,8 +1527,11 @@ impl FlowState { self.state_reads.clear(); self.pending_calls.clear(); self.balance_locals.clear(); + self.balance_local_paths.clear(); + self.balance_comparison_locals.clear(); self.pending_balance_calls.clear(); self.invalidated_balance_guards.clear(); + self.path_predicates.clear(); } fn merge(&mut self, other: &Self) { @@ -1340,6 +1548,11 @@ impl FlowState { } } self.balance_locals.extend(other.balance_locals.iter().copied()); + merge_balance_local_paths(&mut self.balance_local_paths, &other.balance_local_paths); + merge_comparison_locals( + &mut self.balance_comparison_locals, + &other.balance_comparison_locals, + ); self.invalidated_balance_guards.extend(other.invalidated_balance_guards.iter().copied()); for call in &other.pending_balance_calls { if let Some(existing) = @@ -1355,14 +1568,22 @@ impl FlowState { fn balance_only(&self) -> Self { Self { balance_locals: self.balance_locals.clone(), + balance_local_paths: self.balance_local_paths.clone(), + balance_comparison_locals: self.balance_comparison_locals.clone(), pending_balance_calls: self.pending_balance_calls.clone(), invalidated_balance_guards: self.invalidated_balance_guards.clone(), + path_predicates: self.path_predicates.clone(), ..Self::default() } } fn merge_balance(&mut self, other: &Self) { self.balance_locals.extend(other.balance_locals.iter().copied()); + merge_balance_local_paths(&mut self.balance_local_paths, &other.balance_local_paths); + merge_comparison_locals( + &mut self.balance_comparison_locals, + &other.balance_comparison_locals, + ); self.invalidated_balance_guards.extend(other.invalidated_balance_guards.iter().copied()); for call in &other.pending_balance_calls { if let Some(existing) = @@ -1374,6 +1595,112 @@ impl FlowState { } } } + + fn constrain_path(&mut self, (var_id, value): (VariableId, bool)) -> bool { + match self.path_predicates.get(&var_id) { + Some(existing) => *existing == value, + None => { + self.path_predicates.insert(var_id, value); + true + } + } + } +} + +fn path_predicate(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> Option<(VariableId, bool)> { + match &expr.peel_parens().kind { + ExprKind::Ident(reses) => { + let var_id = unique(reses.iter().filter_map(|res| match res { + Res::Item(ItemId::Variable(var_id)) if !hir.variable(*var_id).kind.is_state() => { + Some(*var_id) + } + _ => None, + }))?; + Some((var_id, true)) + } + ExprKind::Unary(op, inner) if op.kind == UnOpKind::Not => { + path_predicate(hir, inner).map(|(var_id, value)| (var_id, !value)) + } + _ => None, + } +} + +fn common_path_predicates(lhs: &PathPredicates, rhs: &PathPredicates) -> PathPredicates { + lhs.iter() + .filter(|(var_id, value)| rhs.get(var_id) == Some(*value)) + .map(|(var_id, value)| (*var_id, *value)) + .collect() +} + +fn paths_compatible(lhs: &PathPredicates, rhs: &PathPredicates) -> bool { + lhs.iter().all(|(var_id, value)| rhs.get(var_id).is_none_or(|other| other == value)) +} + +fn paths_compatible_with(paths: &PathAlternatives, active: &PathPredicates) -> bool { + paths.iter().any(|path| paths_compatible(path, active)) +} + +fn constrain_paths(paths: &PathAlternatives, active: &PathPredicates) -> PathAlternatives { + paths + .iter() + .filter(|path| paths_compatible(path, active)) + .map(|path| { + let mut constrained = path.clone(); + constrained.extend(active.iter().map(|(var_id, value)| (*var_id, *value))); + constrained + }) + .collect() +} + +fn remap_return_paths( + hir: &hir::Hir<'_>, + func_id: FunctionId, + parameters: &[VariableId], + parameter_predicates: &[Option<(VariableId, bool)>], + values: &mut [BalanceValue], +) { + for value in values { + value.balance_paths = value + .balance_paths + .iter() + .filter_map(|path| { + let mut path = path.clone(); + for (¶meter, &argument) in parameters.iter().zip(parameter_predicates) { + let Some(parameter_value) = path.remove(¶meter) else { continue }; + let Some((argument_var, argument_value)) = argument else { continue }; + let mapped_value = + if parameter_value { argument_value } else { !argument_value }; + if path.get(&argument_var).is_some_and(|existing| *existing != mapped_value) { + return None; + } + path.insert(argument_var, mapped_value); + } + path.retain(|var_id, _| { + hir.variable(*var_id).parent != Some(ItemId::Function(func_id)) + }); + Some(path) + }) + .collect(); + value.balance_dependent = !value.balance_paths.is_empty(); + } +} + +fn merge_balance_local_paths( + stored: &mut BTreeMap, + other: &BTreeMap, +) { + for (var_id, paths) in other { + stored.entry(*var_id).or_default().extend(paths.iter().cloned()); + } +} + +fn merge_comparison_locals( + stored: &mut BTreeMap>, + other: &BTreeMap>, +) { + for (var_id, comparisons) in other { + extend_unique(stored.entry(*var_id).or_default(), comparisons.iter().copied()); + } } fn is_balance_reentrant_call<'hir>( @@ -2178,6 +2505,52 @@ fn lhs_local_var(hir: &hir::Hir<'_>, lhs: &hir::Expr<'_>) -> Option None } +fn local_write_lhs_vars(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> Vec { + let mut vars = Vec::new(); + collect_local_write_lhs_vars(hir, expr, &mut vars); + vars +} + +fn collect_local_write_lhs_vars( + hir: &hir::Hir<'_>, + expr: &hir::Expr<'_>, + vars: &mut Vec, +) { + match &expr.kind { + ExprKind::Ident(reses) => { + for &res in *reses { + if let Res::Item(ItemId::Variable(var_id)) = res + && !hir.variable(var_id).kind.is_state() + { + push_unique(vars, var_id); + } + } + } + ExprKind::Tuple(exprs) => { + for expr in exprs.iter().copied().flatten() { + collect_local_write_lhs_vars(hir, expr, vars); + } + } + _ => {} + } +} + +fn forget_path_predicates(state: &mut FlowState, vars: impl IntoIterator) { + for var_id in vars { + state.path_predicates.remove(&var_id); + for paths in state.balance_local_paths.values_mut() { + *paths = paths + .iter() + .map(|path| { + let mut path = path.clone(); + path.remove(&var_id); + path + }) + .collect(); + } + } +} + fn state_write_lhs_vars(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> Vec { let mut vars = Vec::new(); collect_state_write_lhs_vars(hir, expr, &mut vars); @@ -2220,3 +2593,9 @@ fn push_unique(items: &mut Vec, item: T) { items.push(item); } } + +fn extend_unique(items: &mut Vec, values: impl IntoIterator) { + for value in values { + push_unique(items, value); + } +} diff --git a/crates/lint/testdata/ReentrancyBalance.sol b/crates/lint/testdata/ReentrancyBalance.sol index ca2a98bbf7edb..821f9939f9922 100644 --- a/crates/lint/testdata/ReentrancyBalance.sol +++ b/crates/lint/testdata/ReentrancyBalance.sol @@ -245,6 +245,171 @@ contract ReentrancyBalance { } } + function sequentialComplementaryPaths( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline + ) external { + uint256 balanceBefore; + if (takeBaseline) balanceBefore = address(this).balance; + if (!takeBaseline) callback.pay(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function sequentialSamePath( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline + ) external { + uint256 balanceBefore; + if (takeBaseline) balanceBefore = address(this).balance; + if (takeBaseline) callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function conditionalBaselineUnconditionalCall( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline + ) external { + uint256 balanceBefore; + if (takeBaseline) balanceBefore = address(this).balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function reassignedPredicateStillWarns( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline + ) external { + uint256 balanceBefore; + if (takeBaseline) balanceBefore = address(this).balance; + takeBaseline = false; + if (!takeBaseline) callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function compoundBaselineStillWarns( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + uint256 balanceBefore = address(this).balance; + balanceBefore += amount; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore, "insufficient payment"); + } + + function loopMutationStillWarns( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline + ) external { + if (!takeBaseline) return; + uint256 balanceBefore = address(this).balance; + while (takeBaseline) { + takeBaseline = false; + } + if (!takeBaseline) callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function secondLoopIterationMutationStillWarns( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline, + bool firstIteration + ) external { + if (!takeBaseline) return; + if (!firstIteration) return; + uint256 balanceBefore = address(this).balance; + for (uint256 i; i < 2; ++i) { + if (firstIteration) firstIteration = false; + else takeBaseline = false; + } + if (!takeBaseline) callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function ternaryMutationStillWarns( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline + ) external { + if (!takeBaseline) return; + uint256 balanceBefore = address(this).balance; + takeBaseline ? (takeBaseline = false) : false; + if (!takeBaseline) callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function tryClauseMutationStillWarns( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline + ) external { + if (!takeBaseline) return; + uint256 balanceBefore = address(this).balance; + try callback.observe() returns (uint256) { + takeBaseline = false; + } catch { + takeBaseline = false; + } + if (!takeBaseline) callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function helperBaselineComplementaryPath( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline + ) external { + uint256 balanceBefore = conditionalBaseline(takeBaseline); + if (!takeBaseline) callback.pay(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function helperBaselineSamePath( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline + ) external { + uint256 balanceBefore = conditionalBaseline(takeBaseline); + if (takeBaseline) callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function comparisonReturnedByHelper( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + uint256 balanceBefore = address(this).balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require( + isPaid(address(this).balance, balanceBefore, amount), "insufficient payment" + ); + } + + function helperComparisonCompoundAssignment( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + uint256 balanceBefore = address(this).balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + bool paid = isPaid(address(this).balance, balanceBefore, amount); + paid &= amount > 0; + require(paid, "insufficient payment"); + } + + function nonComparisonReturnedByHelper( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + uint256 balanceBefore = address(this).balance; + callback.pay(); + require(hasEnoughCombinedBalance(address(this).balance, balanceBefore, amount)); + } + function otherAddressBalance( IReentrancyBalanceCallback callback, address account, @@ -307,6 +472,26 @@ contract ReentrancyBalance { require(address(this).balance >= balanceBefore + amount, "insufficient payment"); } + function isPaid( + uint256 current, + uint256 balanceBefore, + uint256 amount + ) internal pure returns (bool) { + return current >= balanceBefore + amount; + } + + function hasEnoughCombinedBalance( + uint256 current, + uint256 balanceBefore, + uint256 amount + ) internal pure returns (bool) { + return current + balanceBefore >= amount; + } + + function conditionalBaseline(bool takeBaseline) internal view returns (uint256 balanceBefore) { + if (takeBaseline) balanceBefore = address(this).balance; + } + function consume(uint256, uint256) internal pure {} function recurse(IReentrancyBalanceCallback callback, uint256 depth) internal { diff --git a/crates/lint/testdata/ReentrancyBalance.stderr b/crates/lint/testdata/ReentrancyBalance.stderr index a4e774732fd78..821c1238145d0 100644 --- a/crates/lint/testdata/ReentrancyBalance.stderr +++ b/crates/lint/testdata/ReentrancyBalance.stderr @@ -190,6 +190,94 @@ LL │ callback.pay(); │ ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ if (takeBaseline) callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ if (!takeBaseline) callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ if (!takeBaseline) callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ if (!takeBaseline) callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ if (!takeBaseline) callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ if (!takeBaseline) callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ if (takeBaseline) callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC │ From ba7bed00f70fd88a7c029d4ab04bc6c76fb5e785 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Fri, 17 Jul 2026 01:12:38 +0200 Subject: [PATCH 5/9] fix(lint): track self address aliases Preserve address(this) provenance through locals, tuples, helper and modifier parameters, and branch-correlated call paths. Invalidate aliases on observable writes and assembly reassignment. --- crates/lint/src/sol/high/reentrancy.rs | 224 +++++++++++++++--- crates/lint/testdata/ReentrancyBalance.sol | 101 ++++++++ crates/lint/testdata/ReentrancyBalance.stderr | 40 ++++ 3 files changed, 337 insertions(+), 28 deletions(-) diff --git a/crates/lint/src/sol/high/reentrancy.rs b/crates/lint/src/sol/high/reentrancy.rs index 607a21420c987..db52eb163d41d 100644 --- a/crates/lint/src/sol/high/reentrancy.rs +++ b/crates/lint/src/sol/high/reentrancy.rs @@ -89,6 +89,7 @@ fn is_entry_point(func: &hir::Function<'_>) -> bool { struct FlowState { state_reads: BTreeSet, pending_calls: Vec, + self_address_local_paths: BTreeMap, balance_locals: BTreeSet, balance_local_paths: BTreeMap, balance_comparison_locals: BTreeMap>, @@ -108,6 +109,7 @@ struct PendingCall { struct PendingBalanceCall { span: Span, stale_locals: BTreeSet, + paths: PathAlternatives, } type PathPredicates = BTreeMap; @@ -156,12 +158,14 @@ impl FlowState { if stale_locals.is_empty() { return; } + let paths = [self.path_predicates.clone()].into_iter().collect::(); if let Some(existing) = self.pending_balance_calls.iter_mut().find(|call| call.span == span) { existing.stale_locals.extend(stale_locals); + existing.paths.extend(paths); } else { - self.pending_balance_calls.push(PendingBalanceCall { span, stale_locals }); + self.pending_balance_calls.push(PendingBalanceCall { span, stale_locals, paths }); } } } @@ -347,18 +351,23 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { ) -> bool { match stmt.kind { StmtKind::DeclSingle(var_id) => { - if let Some(init) = self.hir.variable(var_id).initializer { + let init = self.hir.variable(var_id).initializer; + if let Some(init) = init { self.analyze_expr(init, state); if self.reentrancy_balance_enabled { self.update_balance_local(state, var_id, Some(init), false); } } + if self.reentrancy_balance_enabled { + self.update_self_address_local(state, var_id, init); + } true } StmtKind::DeclMulti(vars, expr) => { self.analyze_expr(expr, state); if self.reentrancy_balance_enabled { self.update_balance_vars(state, vars.iter().copied(), expr); + self.update_self_address_vars(state, vars.iter().copied(), expr); } true } @@ -508,6 +517,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } StmtKind::AssemblyBlock(_) | StmtKind::Switch(_) => { state.invalidated_balance_guards.extend(self.active_balance_guards.iter().copied()); + state.self_address_local_paths.clear(); true } StmtKind::Err(_) => true, @@ -530,6 +540,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { forget_path_predicates(state, local_write_lhs_vars(self.hir, lhs)); if self.reentrancy_balance_enabled { self.update_balance_assignment(state, lhs, rhs, op.is_some()); + self.update_self_address_assignment(state, lhs, rhs, op.is_some()); } } ExprKind::Delete(inner) => { @@ -544,6 +555,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { && let Some(var_id) = lhs_local_var(self.hir, inner) { self.update_balance_local(state, var_id, None, false); + self.update_self_address_local(state, var_id, None); } } ExprKind::Unary(op, inner) @@ -559,6 +571,11 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.invalidate_balance_guards(state, &written_vars); } forget_path_predicates(state, local_write_lhs_vars(self.hir, inner)); + if self.reentrancy_balance_enabled + && let Some(var_id) = lhs_local_var(self.hir, inner) + { + self.update_self_address_local(state, var_id, None); + } } ExprKind::Unary(_, inner) => { self.analyze_expr(inner, state); @@ -1116,18 +1133,22 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { lhs, ¤t_locals, BalanceQuery::Current(call.span), + state, ) && self.expr_depends_on_balance( rhs, &call.stale_locals, BalanceQuery::Stale(call.span), + state, )) || (self.expr_depends_on_balance( rhs, ¤t_locals, BalanceQuery::Current(call.span), + state, ) && self.expr_depends_on_balance( lhs, &call.stale_locals, BalanceQuery::Stale(call.span), + state, )))) || self.guard_has_stale_balance_comparison(lhs, call, state) || self.guard_has_stale_balance_comparison(rhs, call, state) @@ -1175,6 +1196,78 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.update_balance_local_with_value(state, var_id, &value, reads_old_value); } + fn update_self_address_local( + &self, + state: &mut FlowState, + var_id: VariableId, + value: Option<&hir::Expr<'_>>, + ) { + let paths = value.map(|value| self_address_paths(value, state)).unwrap_or_default(); + self.update_self_address_local_with_paths(state, var_id, paths); + } + + fn update_self_address_local_with_paths( + &self, + state: &mut FlowState, + var_id: VariableId, + paths: PathAlternatives, + ) { + state.self_address_local_paths.remove(&var_id); + if !paths.is_empty() { + state.self_address_local_paths.insert(var_id, paths); + } + } + + fn update_self_address_assignment( + &self, + state: &mut FlowState, + lhs: &hir::Expr<'_>, + rhs: &hir::Expr<'_>, + reads_old_value: bool, + ) { + if let ExprKind::Tuple(lhs_values) = &lhs.peel_parens().kind { + let values = self.self_address_dependencies(rhs, state); + for (lhs, paths) in lhs_values.iter().zip(values) { + if let Some(var_id) = lhs.and_then(|lhs| lhs_local_var(self.hir, lhs)) { + self.update_self_address_local_with_paths(state, var_id, paths); + } + } + } else if let Some(var_id) = lhs_local_var(self.hir, lhs) { + if reads_old_value { + self.update_self_address_local_with_paths(state, var_id, PathAlternatives::new()); + } else { + self.update_self_address_local(state, var_id, Some(rhs)); + } + } + } + + fn update_self_address_vars( + &self, + state: &mut FlowState, + vars: impl Iterator>, + value: &hir::Expr<'_>, + ) { + for (var_id, paths) in vars.zip(self.self_address_dependencies(value, state)) { + if let Some(var_id) = var_id { + self.update_self_address_local_with_paths(state, var_id, paths); + } + } + } + + fn self_address_dependencies( + &self, + expr: &hir::Expr<'_>, + state: &FlowState, + ) -> Vec { + match &expr.peel_parens().kind { + ExprKind::Tuple(exprs) => exprs + .iter() + .map(|expr| expr.map(|expr| self_address_paths(expr, state)).unwrap_or_default()) + .collect(), + _ => vec![self_address_paths(expr, state)], + } + } + fn update_balance_assignment( &mut self, state: &mut FlowState, @@ -1281,6 +1374,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { expr, &call.stale_locals, BalanceQuery::Stale(call.span), + state, ) }) .map(|call| call.span) @@ -1300,8 +1394,9 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { state: &FlowState, ) -> PathAlternatives { let expr = expr.peel_parens(); - if is_self_balance(expr) { - return [state.path_predicates.clone()].into_iter().collect(); + let self_balance_paths = self_balance_paths(expr, state); + if !self_balance_paths.is_empty() { + return self_balance_paths; } match &expr.kind { @@ -1362,10 +1457,21 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { expr: &'hir hir::Expr<'hir>, locals: &BTreeSet, query: BalanceQuery, + state: &FlowState, ) -> bool { let expr = expr.peel_parens(); - if is_self_balance(expr) { - return !matches!(query, BalanceQuery::Stale(_)); + let self_balance_paths = self_balance_paths(expr, state); + if !self_balance_paths.is_empty() { + return match query { + BalanceQuery::Current(call) => state + .pending_balance_calls + .iter() + .find(|pending| pending.span == call) + .is_some_and(|pending| { + path_alternatives_compatible(&self_balance_paths, &pending.paths) + }), + BalanceQuery::Stale(_) => false, + }; } match &expr.kind { @@ -1373,16 +1479,16 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { |res| matches!(res, Res::Item(ItemId::Variable(var_id)) if locals.contains(var_id)), ), ExprKind::Unary(_, inner) | ExprKind::Payable(inner) => { - self.expr_depends_on_balance(inner, locals, query) + self.expr_depends_on_balance(inner, locals, query, state) } ExprKind::Binary(lhs, _, rhs) => { - self.expr_depends_on_balance(lhs, locals, query) - || self.expr_depends_on_balance(rhs, locals, query) + self.expr_depends_on_balance(lhs, locals, query, state) + || self.expr_depends_on_balance(rhs, locals, query, state) } ExprKind::Ternary(cond, true_expr, false_expr) => { - self.expr_depends_on_balance(cond, locals, query) - || self.expr_depends_on_balance(true_expr, locals, query) - || self.expr_depends_on_balance(false_expr, locals, query) + self.expr_depends_on_balance(cond, locals, query, state) + || self.expr_depends_on_balance(true_expr, locals, query, state) + || self.expr_depends_on_balance(false_expr, locals, query, state) } ExprKind::Call(callee, args, opts) if opts.is_none() @@ -1391,7 +1497,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { ExprKind::Type(_) | ExprKind::TypeCall(_) ) => { - args.exprs().any(|arg| self.expr_depends_on_balance(arg, locals, query)) + args.exprs().any(|arg| self.expr_depends_on_balance(arg, locals, query, state)) } ExprKind::Call(_, _, _) => { self.call_balance_values.get(&expr.span).is_some_and(|values| { @@ -1421,14 +1527,17 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { .iter() .enumerate() .map(|(index, ¶m)| { - let value = argument_for_parameter(self.hir, args, func.parameters, index) - .map(|arg| self.balance_dependency(arg, state)) - .unwrap_or_default(); - (param, value) + let argument = argument_for_parameter(self.hir, args, func.parameters, index); + let value = + argument.map(|arg| self.balance_dependency(arg, state)).unwrap_or_default(); + let self_address_paths = + argument.map(|arg| self_address_paths(arg, state)).unwrap_or_default(); + (param, value, self_address_paths) }) .collect::>(); - for (param, value) in values { + for (param, value, self_address_paths) in values { self.update_balance_local_with_value(state, param, &value, false); + self.update_self_address_local_with_paths(state, param, self_address_paths); } } @@ -1477,6 +1586,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.hir.variable(*var_id).parent == Some(ItemId::Function(func_id)) }; state.balance_locals.retain(|var_id| !belongs_to_function(var_id)); + state.self_address_local_paths.retain(|var_id, _| !belongs_to_function(var_id)); state.balance_local_paths.retain(|var_id, _| !belongs_to_function(var_id)); state.balance_comparison_locals.retain(|var_id, _| !belongs_to_function(var_id)); state.path_predicates.retain(|var_id, _| !belongs_to_function(var_id)); @@ -1526,6 +1636,7 @@ impl FlowState { fn clear(&mut self) { self.state_reads.clear(); self.pending_calls.clear(); + self.self_address_local_paths.clear(); self.balance_locals.clear(); self.balance_local_paths.clear(); self.balance_comparison_locals.clear(); @@ -1547,6 +1658,10 @@ impl FlowState { self.pending_calls.push(call.clone()); } } + merge_balance_local_paths( + &mut self.self_address_local_paths, + &other.self_address_local_paths, + ); self.balance_locals.extend(other.balance_locals.iter().copied()); merge_balance_local_paths(&mut self.balance_local_paths, &other.balance_local_paths); merge_comparison_locals( @@ -1559,6 +1674,7 @@ impl FlowState { self.pending_balance_calls.iter_mut().find(|existing| existing.span == call.span) { existing.stale_locals.extend(call.stale_locals.iter().copied()); + existing.paths.extend(call.paths.iter().cloned()); } else { self.pending_balance_calls.push(call.clone()); } @@ -1567,6 +1683,7 @@ impl FlowState { fn balance_only(&self) -> Self { Self { + self_address_local_paths: self.self_address_local_paths.clone(), balance_locals: self.balance_locals.clone(), balance_local_paths: self.balance_local_paths.clone(), balance_comparison_locals: self.balance_comparison_locals.clone(), @@ -1578,6 +1695,10 @@ impl FlowState { } fn merge_balance(&mut self, other: &Self) { + merge_balance_local_paths( + &mut self.self_address_local_paths, + &other.self_address_local_paths, + ); self.balance_locals.extend(other.balance_locals.iter().copied()); merge_balance_local_paths(&mut self.balance_local_paths, &other.balance_local_paths); merge_comparison_locals( @@ -1590,6 +1711,7 @@ impl FlowState { self.pending_balance_calls.iter_mut().find(|existing| existing.span == call.span) { existing.stale_locals.extend(call.stale_locals.iter().copied()); + existing.paths.extend(call.paths.iter().cloned()); } else { self.pending_balance_calls.push(call.clone()); } @@ -1601,6 +1723,9 @@ impl FlowState { Some(existing) => *existing == value, None => { self.path_predicates.insert(var_id, value); + for paths in self.self_address_local_paths.values_mut() { + *paths = constrain_paths(paths, &self.path_predicates); + } true } } @@ -1636,6 +1761,10 @@ fn paths_compatible(lhs: &PathPredicates, rhs: &PathPredicates) -> bool { lhs.iter().all(|(var_id, value)| rhs.get(var_id).is_none_or(|other| other == value)) } +fn path_alternatives_compatible(lhs: &PathAlternatives, rhs: &PathAlternatives) -> bool { + lhs.iter().any(|lhs| rhs.iter().any(|rhs| paths_compatible(lhs, rhs))) +} + fn paths_compatible_with(paths: &PathAlternatives, active: &PathPredicates) -> bool { paths.iter().any(|path| paths_compatible(path, active)) } @@ -2093,23 +2222,41 @@ fn call_uses_delegate_context(gcx: Gcx<'_>, callee: &hir::Expr<'_>) -> bool { ) } -fn is_self_balance(expr: &hir::Expr<'_>) -> bool { - matches!( - &expr.peel_parens().kind, - ExprKind::Member(base, member) - if member.as_str() == "balance" && is_self_address(base) - ) +fn self_balance_paths(expr: &hir::Expr<'_>, state: &FlowState) -> PathAlternatives { + let ExprKind::Member(base, member) = &expr.peel_parens().kind else { + return PathAlternatives::new(); + }; + if member.as_str() != "balance" { + return PathAlternatives::new(); + } + self_address_paths(base, state) } -fn is_self_address(expr: &hir::Expr<'_>) -> bool { +fn self_address_paths(expr: &hir::Expr<'_>, state: &FlowState) -> PathAlternatives { match &expr.peel_parens().kind { - ExprKind::Payable(inner) => is_self_address(inner), + ExprKind::Ident(reses) => { + if is_this(expr) { + return [state.path_predicates.clone()].into_iter().collect(); + } + let mut paths = PathAlternatives::new(); + for var_id in reses.iter().filter_map(|res| match res { + Res::Item(ItemId::Variable(var_id)) => Some(var_id), + _ => None, + }) { + if let Some(local_paths) = state.self_address_local_paths.get(var_id) { + paths.extend(constrain_paths(local_paths, &state.path_predicates)); + } + } + paths + } + ExprKind::Payable(inner) => self_address_paths(inner, state), ExprKind::Call(callee, args, opts) if opts.is_none() && is_address_type_expr(callee) && args.exprs().count() == 1 => { - args.exprs().next().is_some_and(is_self_address) + args.exprs().next().map(|arg| self_address_paths(arg, state)).unwrap_or_default() } - _ => is_this(expr), + _ if is_this(expr) => [state.path_predicates.clone()].into_iter().collect(), + _ => PathAlternatives::new(), } } @@ -2548,6 +2695,27 @@ fn forget_path_predicates(state: &mut FlowState, vars: impl IntoIterator= balanceBefore + amount, "insufficient payment"); } + modifier checkSelfAfter(address payable self, uint256 balanceBefore, uint256 amount) { + _; + require(self.balance >= balanceBefore + amount, "insufficient payment"); + } + function requireAfterCall(IReentrancyBalanceCallback callback, uint256 amount) external { uint256 balanceBefore = payable(address(this)).balance; callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked @@ -71,6 +76,95 @@ contract ReentrancyBalance { require(balanceAfter >= balanceBefore + amount, "insufficient payment"); } + function selfAddressAlias(IReentrancyBalanceCallback callback, uint256 amount) external { + address payable self = payable(address(this)); + uint256 balanceBefore = self.balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(self.balance >= balanceBefore + amount, "insufficient payment"); + } + + function reassignedSelfAddressAlias( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + address payable self = payable(address(this)); + uint256 balanceBefore = self.balance; + self = payable(address(callback)); + callback.pay(); + require(self.balance >= balanceBefore + amount, "insufficient payment"); + } + + function conditionallyReassignedSelfAddressAlias( + IReentrancyBalanceCallback callback, + uint256 amount, + bool retarget + ) external { + address payable self = payable(address(this)); + uint256 balanceBefore = self.balance; + if (retarget) { + self = payable(address(callback)); + callback.pay(); + } + require(self.balance >= balanceBefore + amount, "insufficient payment"); + } + + function conditionallyRetargetedAfterCall( + IReentrancyBalanceCallback callback, + uint256 amount, + bool retarget + ) external { + address payable self = payable(address(this)); + uint256 balanceBefore = self.balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + if (retarget) self = payable(address(callback)); + require(self.balance >= balanceBefore + amount, "insufficient payment"); + } + + function tupleSelfAddressAlias(IReentrancyBalanceCallback callback, uint256 amount) external { + (address payable self, uint256 expected) = (payable(address(this)), amount); + uint256 balanceBefore = self.balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(self.balance >= balanceBefore + expected, "insufficient payment"); + } + + function tupleReassignedSelfAddressAlias( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + address payable self = payable(address(this)); + uint256 balanceBefore = self.balance; + (self, amount) = (payable(address(callback)), amount); + callback.pay(); + require(self.balance >= balanceBefore + amount, "insufficient payment"); + } + + function assemblyReassignedSelfAddressAlias( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + address payable self = payable(address(this)); + uint256 balanceBefore = self.balance; + assembly { + self := caller() + } + callback.pay(); + require(self.balance >= balanceBefore + amount, "insufficient payment"); + } + + function helperSelfAddressAlias(IReentrancyBalanceCallback callback, uint256 amount) external { + address payable self = payable(address(this)); + uint256 balanceBefore = self.balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + checkSelfBalance(self, balanceBefore, amount); + } + + function modifierSelfAddressAlias(IReentrancyBalanceCallback callback, uint256 amount) + external + checkSelfAfter(payable(address(this)), address(this).balance, amount) + { + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + } + function tupleDeclaration(IReentrancyBalanceCallback callback, uint256 amount) external { (uint256 balanceBefore, uint256 expected) = (address(this).balance, amount); callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked @@ -472,6 +566,13 @@ contract ReentrancyBalance { require(address(this).balance >= balanceBefore + amount, "insufficient payment"); } + function checkSelfBalance(address payable self, uint256 balanceBefore, uint256 amount) + internal + view + { + require(self.balance >= balanceBefore + amount, "insufficient payment"); + } + function isPaid( uint256 current, uint256 balanceBefore, diff --git a/crates/lint/testdata/ReentrancyBalance.stderr b/crates/lint/testdata/ReentrancyBalance.stderr index 821c1238145d0..711023ffd2333 100644 --- a/crates/lint/testdata/ReentrancyBalance.stderr +++ b/crates/lint/testdata/ReentrancyBalance.stderr @@ -62,6 +62,46 @@ LL │ callback.pay(); │ ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC │ From 8654028dc2a0530f485e789e79eb140639b2cee9 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Fri, 17 Jul 2026 15:53:01 +0200 Subject: [PATCH 6/9] fix(lint): track balance expression paths Propagate path constraints and self-address provenance through short-circuit expressions and helper returns so balance reentrancy diagnostics follow feasible paths. --- crates/lint/src/sol/high/reentrancy.rs | 193 ++++++++++++++---- crates/lint/testdata/ReentrancyBalance.sol | 109 ++++++++++ crates/lint/testdata/ReentrancyBalance.stderr | 32 +++ 3 files changed, 296 insertions(+), 38 deletions(-) diff --git a/crates/lint/src/sol/high/reentrancy.rs b/crates/lint/src/sol/high/reentrancy.rs index db52eb163d41d..9b8dad1424355 100644 --- a/crates/lint/src/sol/high/reentrancy.rs +++ b/crates/lint/src/sol/high/reentrancy.rs @@ -220,6 +220,7 @@ struct RecursiveFrontierKey { struct BalanceValue { balance_dependent: bool, balance_paths: PathAlternatives, + self_address_paths: PathAlternatives, stale_calls: HashSet, stale_comparisons: Vec, } @@ -638,6 +639,40 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { .extend(self.active_balance_guards.iter().copied()); } } + ExprKind::Binary(lhs, op, rhs) + if self.reentrancy_balance_enabled + && matches!(op.kind, BinOpKind::And | BinOpKind::Or) => + { + self.analyze_expr(lhs, state); + + let rhs_outcome = op.kind == BinOpKind::And; + let mut short_state = state.clone(); + let short_reachable = + constrain_boolean_outcome(self.hir, lhs, !rhs_outcome, &mut short_state); + let mut rhs_state = state.clone(); + let rhs_reachable = + constrain_boolean_outcome(self.hir, lhs, rhs_outcome, &mut rhs_state); + if rhs_reachable { + self.analyze_expr(rhs, &mut rhs_state); + } + + state.clear(); + if short_reachable { + state.merge(&short_state); + } + if rhs_reachable { + state.merge(&rhs_state); + } + state.path_predicates = match (short_reachable, rhs_reachable) { + (true, true) => common_path_predicates( + &short_state.path_predicates, + &rhs_state.path_predicates, + ), + (true, false) => short_state.path_predicates, + (false, true) => rhs_state.path_predicates, + (false, false) => PathPredicates::new(), + }; + } ExprKind::Binary(lhs, _, rhs) => { self.analyze_expr(lhs, state); self.analyze_expr(rhs, state); @@ -820,6 +855,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { for (stored, value) in stored.iter_mut().zip(values) { stored.balance_dependent |= value.balance_dependent; stored.balance_paths.extend(value.balance_paths); + stored.self_address_paths.extend(value.self_address_paths); stored.stale_calls.extend(value.stale_calls); extend_unique(&mut stored.stale_comparisons, value.stale_comparisons); } @@ -1113,6 +1149,14 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { state: &FlowState, ) -> bool { match &expr.peel_parens().kind { + ExprKind::Binary(lhs, op, rhs) if matches!(op.kind, BinOpKind::And | BinOpKind::Or) => { + if self.guard_has_stale_balance_comparison(lhs, call, state) { + return true; + } + let mut rhs_state = state.clone(); + constrain_boolean_outcome(self.hir, lhs, op.kind == BinOpKind::And, &mut rhs_state) + && self.guard_has_stale_balance_comparison(rhs, call, &rhs_state) + } ExprKind::Binary(lhs, op, rhs) => { let is_comparison = matches!( op.kind, @@ -1202,7 +1246,9 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { var_id: VariableId, value: Option<&hir::Expr<'_>>, ) { - let paths = value.map(|value| self_address_paths(value, state)).unwrap_or_default(); + let paths = value + .and_then(|value| self.self_address_dependencies(value, state).into_iter().next()) + .unwrap_or_default(); self.update_self_address_local_with_paths(state, var_id, paths); } @@ -1262,12 +1308,51 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { match &expr.peel_parens().kind { ExprKind::Tuple(exprs) => exprs .iter() - .map(|expr| expr.map(|expr| self_address_paths(expr, state)).unwrap_or_default()) + .map(|expr| { + expr.map(|expr| self.self_address_path(expr, state)).unwrap_or_default() + }) .collect(), - _ => vec![self_address_paths(expr, state)], + ExprKind::Call(_, _, _) => self + .call_balance_values + .get(&expr.span) + .map(|values| values.iter().map(|value| value.self_address_paths.clone()).collect()) + .unwrap_or_else(|| vec![self.self_address_path(expr, state)]), + _ => vec![self.self_address_path(expr, state)], } } + fn self_address_path(&self, expr: &hir::Expr<'_>, state: &FlowState) -> PathAlternatives { + let expr = expr.peel_parens(); + if let ExprKind::Call(_, _, _) = expr.kind + && let Some(value) = + self.call_balance_values.get(&expr.span).and_then(|values| values.first()) + { + return constrain_paths(&value.self_address_paths, &state.path_predicates); + } + match &expr.kind { + ExprKind::Payable(inner) => self.self_address_path(inner, state), + ExprKind::Call(callee, args, opts) + if opts.is_none() && is_address_type_expr(callee) && args.exprs().count() == 1 => + { + args.exprs() + .next() + .map(|arg| self.self_address_path(arg, state)) + .unwrap_or_default() + } + _ => self_address_paths(expr, state), + } + } + + fn self_balance_paths(&self, expr: &hir::Expr<'_>, state: &FlowState) -> PathAlternatives { + let ExprKind::Member(base, member) = &expr.peel_parens().kind else { + return PathAlternatives::new(); + }; + if member.as_str() != "balance" { + return PathAlternatives::new(); + } + self.self_address_path(base, state) + } + fn update_balance_assignment( &mut self, state: &mut FlowState, @@ -1366,6 +1451,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { fn balance_dependency(&self, expr: &'hir hir::Expr<'hir>, state: &FlowState) -> BalanceValue { let balance_paths = self.expr_balance_paths(expr, state); let balance_dependent = !balance_paths.is_empty(); + let self_address_paths = self.self_address_path(expr, state); let stale_calls = state .pending_balance_calls .iter() @@ -1385,7 +1471,13 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { .filter(|call| self.guard_has_stale_balance_comparison(expr, call, state)) .map(|call| call.span) .collect(); - BalanceValue { balance_dependent, balance_paths, stale_calls, stale_comparisons } + BalanceValue { + balance_dependent, + balance_paths, + self_address_paths, + stale_calls, + stale_comparisons, + } } fn expr_balance_paths( @@ -1394,7 +1486,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { state: &FlowState, ) -> PathAlternatives { let expr = expr.peel_parens(); - let self_balance_paths = self_balance_paths(expr, state); + let self_balance_paths = self.self_balance_paths(expr, state); if !self_balance_paths.is_empty() { return self_balance_paths; } @@ -1460,7 +1552,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { state: &FlowState, ) -> bool { let expr = expr.peel_parens(); - let self_balance_paths = self_balance_paths(expr, state); + let self_balance_paths = self.self_balance_paths(expr, state); if !self_balance_paths.is_empty() { return match query { BalanceQuery::Current(call) => state @@ -1531,7 +1623,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { let value = argument.map(|arg| self.balance_dependency(arg, state)).unwrap_or_default(); let self_address_paths = - argument.map(|arg| self_address_paths(arg, state)).unwrap_or_default(); + argument.map(|arg| self.self_address_path(arg, state)).unwrap_or_default(); (param, value, self_address_paths) }) .collect::>(); @@ -1555,6 +1647,8 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { let balance_dependent = state.balance_locals.contains(var_id); let balance_paths = state.balance_local_paths.get(var_id).cloned().unwrap_or_default(); + let self_address_paths = + state.self_address_local_paths.get(var_id).cloned().unwrap_or_default(); let stale_calls = state .pending_balance_calls .iter() @@ -1566,6 +1660,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { BalanceValue { balance_dependent, balance_paths, + self_address_paths, stale_calls, stale_comparisons, } @@ -1576,6 +1671,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { for (stored, value) in collector.values.iter_mut().zip(values) { stored.balance_dependent |= value.balance_dependent; stored.balance_paths.extend(value.balance_paths); + stored.self_address_paths.extend(value.self_address_paths); stored.stale_calls.extend(value.stale_calls); extend_unique(&mut stored.stale_comparisons, value.stale_comparisons); } @@ -1750,6 +1846,26 @@ fn path_predicate(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> Option<(VariableI } } +fn constrain_boolean_outcome( + hir: &hir::Hir<'_>, + expr: &hir::Expr<'_>, + outcome: bool, + state: &mut FlowState, +) -> bool { + if let Some((var_id, value)) = path_predicate(hir, expr) { + return state.constrain_path((var_id, if outcome { value } else { !value })); + } + match &expr.peel_parens().kind { + ExprKind::Binary(lhs, op, rhs) + if (outcome && op.kind == BinOpKind::And) || (!outcome && op.kind == BinOpKind::Or) => + { + constrain_boolean_outcome(hir, lhs, outcome, state) + && constrain_boolean_outcome(hir, rhs, outcome, state) + } + _ => true, + } +} + fn common_path_predicates(lhs: &PathPredicates, rhs: &PathPredicates) -> PathPredicates { lhs.iter() .filter(|(var_id, value)| rhs.get(var_id) == Some(*value)) @@ -1789,31 +1905,42 @@ fn remap_return_paths( values: &mut [BalanceValue], ) { for value in values { - value.balance_paths = value - .balance_paths - .iter() - .filter_map(|path| { - let mut path = path.clone(); - for (¶meter, &argument) in parameters.iter().zip(parameter_predicates) { - let Some(parameter_value) = path.remove(¶meter) else { continue }; - let Some((argument_var, argument_value)) = argument else { continue }; - let mapped_value = - if parameter_value { argument_value } else { !argument_value }; - if path.get(&argument_var).is_some_and(|existing| *existing != mapped_value) { - return None; - } - path.insert(argument_var, mapped_value); - } - path.retain(|var_id, _| { - hir.variable(*var_id).parent != Some(ItemId::Function(func_id)) - }); - Some(path) - }) - .collect(); + value.balance_paths = + remap_paths(hir, func_id, parameters, parameter_predicates, &value.balance_paths); + value.self_address_paths = + remap_paths(hir, func_id, parameters, parameter_predicates, &value.self_address_paths); value.balance_dependent = !value.balance_paths.is_empty(); } } +fn remap_paths( + hir: &hir::Hir<'_>, + func_id: FunctionId, + parameters: &[VariableId], + parameter_predicates: &[Option<(VariableId, bool)>], + paths: &PathAlternatives, +) -> PathAlternatives { + paths + .iter() + .filter_map(|path| { + let mut path = path.clone(); + for (¶meter, &argument) in parameters.iter().zip(parameter_predicates) { + let Some(parameter_value) = path.remove(¶meter) else { continue }; + let Some((argument_var, argument_value)) = argument else { continue }; + let mapped_value = if parameter_value { argument_value } else { !argument_value }; + if path.get(&argument_var).is_some_and(|existing| *existing != mapped_value) { + return None; + } + path.insert(argument_var, mapped_value); + } + path.retain(|var_id, _| { + hir.variable(*var_id).parent != Some(ItemId::Function(func_id)) + }); + Some(path) + }) + .collect() +} + fn merge_balance_local_paths( stored: &mut BTreeMap, other: &BTreeMap, @@ -2222,16 +2349,6 @@ fn call_uses_delegate_context(gcx: Gcx<'_>, callee: &hir::Expr<'_>) -> bool { ) } -fn self_balance_paths(expr: &hir::Expr<'_>, state: &FlowState) -> PathAlternatives { - let ExprKind::Member(base, member) = &expr.peel_parens().kind else { - return PathAlternatives::new(); - }; - if member.as_str() != "balance" { - return PathAlternatives::new(); - } - self_address_paths(base, state) -} - fn self_address_paths(expr: &hir::Expr<'_>, state: &FlowState) -> PathAlternatives { match &expr.peel_parens().kind { ExprKind::Ident(reses) => { diff --git a/crates/lint/testdata/ReentrancyBalance.sol b/crates/lint/testdata/ReentrancyBalance.sol index 8492939a8f4c4..f469a99a39c2a 100644 --- a/crates/lint/testdata/ReentrancyBalance.sol +++ b/crates/lint/testdata/ReentrancyBalance.sol @@ -83,6 +83,46 @@ contract ReentrancyBalance { require(self.balance >= balanceBefore + amount, "insufficient payment"); } + function helperReturnedSelfAddressAlias( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + address payable self = currentSelf(); + uint256 balanceBefore = self.balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(self.balance >= balanceBefore + amount, "insufficient payment"); + } + + function helperReturnedSelfInlineBalance( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + uint256 balanceBefore = currentSelf().balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function helperReturnedSelfTupleAlias( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + address payable self; + (self,) = (currentSelf(), amount); + uint256 balanceBefore = self.balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(self.balance >= balanceBefore + amount, "insufficient payment"); + } + + function chainedHelperReturnedSelfAlias( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + address payable self = identity(currentSelf()); + uint256 balanceBefore = self.balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(self.balance >= balanceBefore + amount, "insufficient payment"); + } + function reassignedSelfAddressAlias( IReentrancyBalanceCallback callback, uint256 amount @@ -350,6 +390,67 @@ contract ReentrancyBalance { require(address(this).balance >= balanceBefore + amount, "insufficient payment"); } + function shortCircuitAndComplementaryPaths( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline + ) external { + uint256 balanceBefore; + takeBaseline && (balanceBefore = address(this).balance) > 0; + if (!takeBaseline) callback.pay(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function compoundShortCircuitAndComplementaryPaths( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline, + bool enabled + ) external { + uint256 balanceBefore; + takeBaseline && enabled && (balanceBefore = address(this).balance) > 0; + if (!takeBaseline) callback.pay(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function shortCircuitOrComplementaryPaths( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline + ) external { + uint256 balanceBefore; + takeBaseline || (balanceBefore = address(this).balance) > 0; + if (takeBaseline) callback.pay(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function shortCircuitGuardComplementaryPaths( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline + ) external { + uint256 balanceBefore = address(this).balance; + if (!takeBaseline) callback.pay(); + require( + !takeBaseline || address(this).balance >= balanceBefore + amount, + "insufficient payment" + ); + } + + function compoundShortCircuitGuardComplementaryPaths( + IReentrancyBalanceCallback callback, + uint256 amount, + bool takeBaseline, + bool enabled + ) external { + uint256 balanceBefore = address(this).balance; + if (!takeBaseline) callback.pay(); + require( + takeBaseline && enabled && address(this).balance >= balanceBefore + amount, + "insufficient payment" + ); + } + function sequentialSamePath( IReentrancyBalanceCallback callback, uint256 amount, @@ -562,6 +663,14 @@ contract ReentrancyBalance { return address(this).balance; } + function currentSelf() internal view returns (address payable) { + return payable(address(this)); + } + + function identity(address payable value) internal pure returns (address payable) { + return value; + } + function checkBalance(uint256 balanceBefore, uint256 amount) internal view { require(address(this).balance >= balanceBefore + amount, "insufficient payment"); } diff --git a/crates/lint/testdata/ReentrancyBalance.stderr b/crates/lint/testdata/ReentrancyBalance.stderr index 711023ffd2333..e03a636740982 100644 --- a/crates/lint/testdata/ReentrancyBalance.stderr +++ b/crates/lint/testdata/ReentrancyBalance.stderr @@ -102,6 +102,38 @@ LL │ callback.pay(); │ ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC │ From 9ce830e1891b64d3abb45100ed486f7f71afefc2 Mon Sep 17 00:00:00 2001 From: Mablr <59505383+mablr@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:08:43 +0200 Subject: [PATCH 7/9] fix(lint): complete balance reentrancy flow Track internal function-pointer targets through assignments and control-flow joins so helper callbacks remain visible to the balance analysis. Recognize standard lock modifiers whose sole placeholder is nested in an unconditional block. Amp-Thread-ID: https://ampcode.com/threads/T-019f896c-fd1a-745b-be37-de6016df1dbb Co-authored-by: Amp --- crates/lint/src/sol/high/reentrancy.rs | 84 ++++++++++++++++++---- crates/lint/testdata/ReentrancyBalance.sol | 58 +++++++++++++++ 2 files changed, 128 insertions(+), 14 deletions(-) diff --git a/crates/lint/src/sol/high/reentrancy.rs b/crates/lint/src/sol/high/reentrancy.rs index 9b8dad1424355..359cdf2ebcd8c 100644 --- a/crates/lint/src/sol/high/reentrancy.rs +++ b/crates/lint/src/sol/high/reentrancy.rs @@ -89,6 +89,7 @@ fn is_entry_point(func: &hir::Function<'_>) -> bool { struct FlowState { state_reads: BTreeSet, pending_calls: Vec, + internal_function_targets: BTreeMap>, self_address_local_paths: BTreeMap, balance_locals: BTreeSet, balance_local_paths: BTreeMap, @@ -355,6 +356,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { let init = self.hir.variable(var_id).initializer; if let Some(init) = init { self.analyze_expr(init, state); + self.update_internal_function_target(state, var_id, init); if self.reentrancy_balance_enabled { self.update_balance_local(state, var_id, Some(init), false); } @@ -518,6 +520,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { } StmtKind::AssemblyBlock(_) | StmtKind::Switch(_) => { state.invalidated_balance_guards.extend(self.active_balance_guards.iter().copied()); + state.internal_function_targets.clear(); state.self_address_local_paths.clear(); true } @@ -539,6 +542,13 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.invalidate_balance_guards(state, &written_vars); } forget_path_predicates(state, local_write_lhs_vars(self.hir, lhs)); + if let Some(var_id) = lhs_local_var(self.hir, lhs) { + if op.is_none() { + self.update_internal_function_target(state, var_id, rhs); + } else { + state.internal_function_targets.remove(&var_id); + } + } if self.reentrancy_balance_enabled { self.update_balance_assignment(state, lhs, rhs, op.is_some()); self.update_self_address_assignment(state, lhs, rhs, op.is_some()); @@ -552,6 +562,9 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.invalidate_balance_guards(state, &written_vars); } forget_path_predicates(state, local_write_lhs_vars(self.hir, inner)); + if let Some(var_id) = lhs_local_var(self.hir, inner) { + state.internal_function_targets.remove(&var_id); + } if self.reentrancy_balance_enabled && let Some(var_id) = lhs_local_var(self.hir, inner) { @@ -618,7 +631,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { self.emit_balance_calls(cond, state); } - if let Some(func_id) = self.resolved_internal_function_id(callee) { + for func_id in self.resolved_internal_function_ids(callee, state) { let returns = self.analyze_internal_call(func_id, args, state); self.merge_call_balance_values(expr.span, returns); } @@ -836,15 +849,39 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { returns } - fn resolved_internal_function_id(&self, callee: &'hir hir::Expr<'hir>) -> Option { + fn resolved_internal_function_ids( + &self, + callee: &'hir hir::Expr<'hir>, + state: &FlowState, + ) -> BTreeSet { + if let Some(var_id) = lhs_local_var(self.hir, callee) + && let Some(targets) = state.internal_function_targets.get(&var_id) + { + return targets.clone(); + } match &callee.peel_parens().kind { ExprKind::Ident(_) => {} ExprKind::Member(base, _) if is_super(base) => {} - _ => return None, + _ => return BTreeSet::new(), + } + let Some(ty) = self.gcx.type_of_expr(callee.peel_parens().id) else { + return BTreeSet::new(); + }; + let TyKind::Fn(function) = ty.kind else { return BTreeSet::new() }; + function.is_internal().then_some(function.function_id).flatten().into_iter().collect() + } + + fn update_internal_function_target( + &self, + state: &mut FlowState, + var_id: VariableId, + value: &'hir hir::Expr<'hir>, + ) { + let targets = self.resolved_internal_function_ids(value, state); + state.internal_function_targets.remove(&var_id); + if !targets.is_empty() { + state.internal_function_targets.insert(var_id, targets); } - let ty = self.gcx.type_of_expr(callee.peel_parens().id)?; - let TyKind::Fn(function) = ty.kind else { return None }; - if function.is_internal() { function.function_id } else { None } } fn merge_call_balance_values(&mut self, span: Span, values: Vec) { @@ -1023,7 +1060,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { for arg in args.exprs() { self.collect_direct_internal_calls_expr(arg, calls); } - if let Some(func_id) = self.resolved_internal_function_id(callee) { + for func_id in self.resolved_internal_function_ids(callee, &FlowState::default()) { calls.insert(func_id); } } @@ -1681,6 +1718,7 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { let belongs_to_function = |var_id: &VariableId| { self.hir.variable(*var_id).parent == Some(ItemId::Function(func_id)) }; + state.internal_function_targets.retain(|var_id, _| !belongs_to_function(var_id)); state.balance_locals.retain(|var_id| !belongs_to_function(var_id)); state.self_address_local_paths.retain(|var_id, _| !belongs_to_function(var_id)); state.balance_local_paths.retain(|var_id, _| !belongs_to_function(var_id)); @@ -1732,6 +1770,7 @@ impl FlowState { fn clear(&mut self) { self.state_reads.clear(); self.pending_calls.clear(); + self.internal_function_targets.clear(); self.self_address_local_paths.clear(); self.balance_locals.clear(); self.balance_local_paths.clear(); @@ -1754,6 +1793,12 @@ impl FlowState { self.pending_calls.push(call.clone()); } } + for (var_id, targets) in &other.internal_function_targets { + self.internal_function_targets + .entry(*var_id) + .or_default() + .extend(targets.iter().copied()); + } merge_balance_local_paths( &mut self.self_address_local_paths, &other.self_address_local_paths, @@ -1779,6 +1824,7 @@ impl FlowState { fn balance_only(&self) -> Self { Self { + internal_function_targets: self.internal_function_targets.clone(), self_address_local_paths: self.self_address_local_paths.clone(), balance_locals: self.balance_locals.clone(), balance_local_paths: self.balance_local_paths.clone(), @@ -1795,6 +1841,12 @@ impl FlowState { &mut self.self_address_local_paths, &other.self_address_local_paths, ); + for (var_id, targets) in &other.internal_function_targets { + self.internal_function_targets + .entry(*var_id) + .or_default() + .extend(targets.iter().copied()); + } self.balance_locals.extend(other.balance_locals.iter().copied()); merge_balance_local_paths(&mut self.balance_local_paths, &other.balance_local_paths); merge_comparison_locals( @@ -2063,13 +2115,7 @@ fn standard_reentrancy_guard_lock( if body.stmts.iter().map(count_modifier_placeholders).sum::() != 1 { return None; } - let mut placeholders = body - .stmts - .iter() - .enumerate() - .filter(|(_, stmt)| matches!(stmt.kind, StmtKind::Placeholder)); - let (placeholder_index, _) = placeholders.next()?; - debug_assert!(placeholders.next().is_none()); + let placeholder_index = body.stmts.iter().position(contains_unconditional_placeholder)?; let mut seen = BTreeSet::new(); let (lock_var, entered) = @@ -2080,6 +2126,16 @@ fn standard_reentrancy_guard_lock( (lock_var == restored_var && entered != restored).then_some(lock_var) } +fn contains_unconditional_placeholder(stmt: &hir::Stmt<'_>) -> bool { + match stmt.kind { + StmtKind::Placeholder => true, + StmtKind::Block(block) | StmtKind::UncheckedBlock(block) => { + block.stmts.iter().any(contains_unconditional_placeholder) + } + _ => false, + } +} + fn count_modifier_placeholders(stmt: &hir::Stmt<'_>) -> usize { match stmt.kind { StmtKind::Placeholder => 1, diff --git a/crates/lint/testdata/ReentrancyBalance.sol b/crates/lint/testdata/ReentrancyBalance.sol index f469a99a39c2a..720862ec276a6 100644 --- a/crates/lint/testdata/ReentrancyBalance.sol +++ b/crates/lint/testdata/ReentrancyBalance.sol @@ -340,6 +340,40 @@ contract ReentrancyBalance { require(address(this).balance >= balanceBefore + amount, "insufficient payment"); } + function callThroughFunctionPointer( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + uint256 balanceBefore = address(this).balance; + function(IReentrancyBalanceCallback) internal fn = invoke; + fn(callback); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function callThroughAssignedFunctionPointer( + IReentrancyBalanceCallback callback, + uint256 amount + ) external { + uint256 balanceBefore = address(this).balance; + function(IReentrancyBalanceCallback) internal fn; + fn = invoke; + fn(callback); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + + function callThroughJoinedFunctionPointer( + IReentrancyBalanceCallback callback, + uint256 amount, + bool callOut + ) external { + uint256 balanceBefore = address(this).balance; + function(IReentrancyBalanceCallback) internal fn; + if (callOut) fn = invoke; + else fn = doNothing; + fn(callback); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + function recursiveHelper(IReentrancyBalanceCallback callback, uint256 amount) external { uint256 balanceBefore = address(this).balance; recurse(callback, 1); @@ -704,6 +738,8 @@ contract ReentrancyBalance { function consume(uint256, uint256) internal pure {} + function doNothing(IReentrancyBalanceCallback) internal pure {} + function recurse(IReentrancyBalanceCallback callback, uint256 depth) internal { if (depth > 0) recurse(callback, depth - 1); callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked @@ -924,3 +960,25 @@ contract ReentrancyBalanceMultiplePlaceholderGuard { require(address(this).balance >= balanceBefore + amount, "insufficient payment"); } } + +contract ReentrancyBalanceNestedPlaceholderGuard { + uint256 private locked; + + modifier nonReentrant() { + require(locked == 0, "reentrant"); + locked = 1; + { + _; + } + locked = 0; + } + + function guarded( + IReentrancyBalanceCallback callback, + uint256 amount + ) external nonReentrant { + uint256 balanceBefore = address(this).balance; + callback.pay(); + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } +} From 2111d025190884c32df10a0919f5b2faf9d28cb3 Mon Sep 17 00:00:00 2001 From: Mablr <59505383+mablr@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:52:31 +0200 Subject: [PATCH 8/9] fix(lint): inspect nested guard activation Include statements before a nested modifier placeholder when recognizing reentrancy guards. This prevents an unlock inside the nested block from suppressing reentrancy-balance diagnostics. --- crates/lint/src/sol/high/reentrancy.rs | 35 ++++++++++++++++--- crates/lint/testdata/ReentrancyBalance.sol | 23 ++++++++++++ crates/lint/testdata/ReentrancyBalance.stderr | 8 +++++ 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/crates/lint/src/sol/high/reentrancy.rs b/crates/lint/src/sol/high/reentrancy.rs index 359cdf2ebcd8c..ffaa5b610f130 100644 --- a/crates/lint/src/sol/high/reentrancy.rs +++ b/crates/lint/src/sol/high/reentrancy.rs @@ -2115,17 +2115,35 @@ fn standard_reentrancy_guard_lock( if body.stmts.iter().map(count_modifier_placeholders).sum::() != 1 { return None; } - let placeholder_index = body.stmts.iter().position(contains_unconditional_placeholder)?; - + let mut activation_stmts = Vec::new(); + collect_stmts_before_unconditional_placeholder(body.stmts, &mut activation_stmts)?; let mut seen = BTreeSet::new(); - let (lock_var, entered) = - guard_activation_from_stmts(hir, &body.stmts[..placeholder_index], &mut seen)?; + let (lock_var, entered) = guard_activation_from_stmt_refs(hir, &activation_stmts, &mut seen)?; + let placeholder_index = body.stmts.iter().position(contains_unconditional_placeholder)?; let mut seen = BTreeSet::new(); let (restored_var, restored) = guard_restoration_from_stmt(hir, body.stmts.get(placeholder_index + 1)?, &mut seen)?; (lock_var == restored_var && entered != restored).then_some(lock_var) } +fn collect_stmts_before_unconditional_placeholder<'hir>( + stmts: &'hir [hir::Stmt<'hir>], + before: &mut Vec<&'hir hir::Stmt<'hir>>, +) -> Option<()> { + for stmt in stmts { + match stmt.kind { + StmtKind::Placeholder => return Some(()), + StmtKind::Block(block) | StmtKind::UncheckedBlock(block) + if contains_unconditional_placeholder(stmt) => + { + return collect_stmts_before_unconditional_placeholder(block.stmts, before); + } + _ => before.push(stmt), + } + } + None +} + fn contains_unconditional_placeholder(stmt: &hir::Stmt<'_>) -> bool { match stmt.kind { StmtKind::Placeholder => true, @@ -2230,6 +2248,15 @@ fn guard_activation_from_stmts( hir: &hir::Hir<'_>, stmts: &[hir::Stmt<'_>], seen: &mut BTreeSet, +) -> Option<(VariableId, LockValue)> { + let stmts = stmts.iter().collect::>(); + guard_activation_from_stmt_refs(hir, &stmts, seen) +} + +fn guard_activation_from_stmt_refs( + hir: &hir::Hir<'_>, + stmts: &[&hir::Stmt<'_>], + seen: &mut BTreeSet, ) -> Option<(VariableId, LockValue)> { let (activation, prefix) = stmts.split_last()?; if let Some((lock_var, entered)) = state_lock_assignment(hir, activation) { diff --git a/crates/lint/testdata/ReentrancyBalance.sol b/crates/lint/testdata/ReentrancyBalance.sol index 720862ec276a6..08a02be97809e 100644 --- a/crates/lint/testdata/ReentrancyBalance.sol +++ b/crates/lint/testdata/ReentrancyBalance.sol @@ -982,3 +982,26 @@ contract ReentrancyBalanceNestedPlaceholderGuard { require(address(this).balance >= balanceBefore + amount, "insufficient payment"); } } + +contract ReentrancyBalanceNestedPlaceholderUnlock { + uint256 private locked; + + modifier unlockedBeforeBody() { + require(locked == 0, "reentrant"); + locked = 1; + { + locked = 0; + _; + } + locked = 0; + } + + function vulnerable( + IReentrancyBalanceCallback callback, + uint256 amount + ) external unlockedBeforeBody { + uint256 balanceBefore = address(this).balance; + callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } +} diff --git a/crates/lint/testdata/ReentrancyBalance.stderr b/crates/lint/testdata/ReentrancyBalance.stderr index e03a636740982..91a6e7035276f 100644 --- a/crates/lint/testdata/ReentrancyBalance.stderr +++ b/crates/lint/testdata/ReentrancyBalance.stderr @@ -414,3 +414,11 @@ LL │ callback.pay(); │ ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + From 659d568f8c61075fa164c5c125466955abd73c48 Mon Sep 17 00:00:00 2001 From: Mablr <59505383+mablr@users.noreply.github.com> Date: Wed, 22 Jul 2026 20:01:13 +0200 Subject: [PATCH 9/9] fix(lint): track comparison paths Preserve normalized equality predicates in reentrancy balance path state so complementary == and != branches remain mutually exclusive. Invalidate those predicates when either local operand is reassigned. --- crates/lint/src/sol/high/reentrancy.rs | 122 ++++++++++++++---- crates/lint/testdata/ReentrancyBalance.sol | 37 ++++++ crates/lint/testdata/ReentrancyBalance.stderr | 16 +++ 3 files changed, 153 insertions(+), 22 deletions(-) diff --git a/crates/lint/src/sol/high/reentrancy.rs b/crates/lint/src/sol/high/reentrancy.rs index ffaa5b610f130..ad938a0f43a19 100644 --- a/crates/lint/src/sol/high/reentrancy.rs +++ b/crates/lint/src/sol/high/reentrancy.rs @@ -113,9 +113,40 @@ struct PendingBalanceCall { paths: PathAlternatives, } -type PathPredicates = BTreeMap; +type PathPredicates = BTreeMap; type PathAlternatives = BTreeSet; +#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] +enum PathPredicate { + Boolean(VariableId), + Equality(PredicateOperand, PredicateOperand), +} + +impl PathPredicate { + fn contains(self, var_id: VariableId) -> bool { + match self { + Self::Boolean(predicate_var) => predicate_var == var_id, + Self::Equality(lhs, rhs) => { + lhs.variable() == Some(var_id) || rhs.variable() == Some(var_id) + } + } + } +} + +#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] +enum PredicateOperand { + Variable(VariableId), + Number(U256), + Boolean(bool), +} + +impl PredicateOperand { + const fn variable(self) -> Option { + let Self::Variable(var_id) = self else { return None }; + Some(var_id) + } +} + #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] enum ReentrantCallKind { Eth, @@ -198,7 +229,7 @@ struct InlineCallKey { recursive_cut: Option, balance_only: bool, active_balance_guards: Vec, - parameter_predicates: Vec>, + parameter_predicates: Vec>, state: FlowState, } @@ -1723,7 +1754,15 @@ impl<'ctx, 's, 'c, 'hir> Analyzer<'ctx, 's, 'c, 'hir> { state.self_address_local_paths.retain(|var_id, _| !belongs_to_function(var_id)); state.balance_local_paths.retain(|var_id, _| !belongs_to_function(var_id)); state.balance_comparison_locals.retain(|var_id, _| !belongs_to_function(var_id)); - state.path_predicates.retain(|var_id, _| !belongs_to_function(var_id)); + state.path_predicates.retain(|predicate, _| { + !matches!(predicate, PathPredicate::Boolean(var_id) if belongs_to_function(var_id)) + && !matches!( + predicate, + PathPredicate::Equality(lhs, rhs) + if lhs.variable().is_some_and(|var_id| belongs_to_function(&var_id)) + || rhs.variable().is_some_and(|var_id| belongs_to_function(&var_id)) + ) + }); for call in &mut state.pending_balance_calls { call.stale_locals.retain(|var_id| !belongs_to_function(var_id)); } @@ -1866,11 +1905,11 @@ impl FlowState { } } - fn constrain_path(&mut self, (var_id, value): (VariableId, bool)) -> bool { - match self.path_predicates.get(&var_id) { + fn constrain_path(&mut self, (predicate, value): (PathPredicate, bool)) -> bool { + match self.path_predicates.get(&predicate) { Some(existing) => *existing == value, None => { - self.path_predicates.insert(var_id, value); + self.path_predicates.insert(predicate, value); for paths in self.self_address_local_paths.values_mut() { *paths = constrain_paths(paths, &self.path_predicates); } @@ -1880,7 +1919,7 @@ impl FlowState { } } -fn path_predicate(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> Option<(VariableId, bool)> { +fn path_predicate(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> Option<(PathPredicate, bool)> { match &expr.peel_parens().kind { ExprKind::Ident(reses) => { let var_id = unique(reses.iter().filter_map(|res| match res { @@ -1889,15 +1928,43 @@ fn path_predicate(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> Option<(VariableI } _ => None, }))?; - Some((var_id, true)) + Some((PathPredicate::Boolean(var_id), true)) } ExprKind::Unary(op, inner) if op.kind == UnOpKind::Not => { - path_predicate(hir, inner).map(|(var_id, value)| (var_id, !value)) + path_predicate(hir, inner).map(|(predicate, value)| (predicate, !value)) + } + ExprKind::Binary(lhs, op, rhs) if matches!(op.kind, BinOpKind::Eq | BinOpKind::Ne) => { + let mut lhs = predicate_operand(hir, lhs)?; + let mut rhs = predicate_operand(hir, rhs)?; + if lhs > rhs { + std::mem::swap(&mut lhs, &mut rhs); + } + Some((PathPredicate::Equality(lhs, rhs), op.kind == BinOpKind::Eq)) } _ => None, } } +fn predicate_operand(hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> Option { + match &expr.peel_parens().kind { + ExprKind::Ident(reses) => { + let var_id = unique(reses.iter().filter_map(|res| match res { + Res::Item(ItemId::Variable(var_id)) if !hir.variable(*var_id).kind.is_state() => { + Some(*var_id) + } + _ => None, + }))?; + Some(PredicateOperand::Variable(var_id)) + } + ExprKind::Lit(lit) => match lit.kind { + LitKind::Number(value) => Some(PredicateOperand::Number(value)), + LitKind::Bool(value) => Some(PredicateOperand::Boolean(value)), + _ => None, + }, + _ => None, + } +} + fn constrain_boolean_outcome( hir: &hir::Hir<'_>, expr: &hir::Expr<'_>, @@ -1943,7 +2010,7 @@ fn constrain_paths(paths: &PathAlternatives, active: &PathPredicates) -> PathAlt .filter(|path| paths_compatible(path, active)) .map(|path| { let mut constrained = path.clone(); - constrained.extend(active.iter().map(|(var_id, value)| (*var_id, *value))); + constrained.extend(active.iter().map(|(predicate, value)| (*predicate, *value))); constrained }) .collect() @@ -1953,7 +2020,7 @@ fn remap_return_paths( hir: &hir::Hir<'_>, func_id: FunctionId, parameters: &[VariableId], - parameter_predicates: &[Option<(VariableId, bool)>], + parameter_predicates: &[Option<(PathPredicate, bool)>], values: &mut [BalanceValue], ) { for value in values { @@ -1969,7 +2036,7 @@ fn remap_paths( hir: &hir::Hir<'_>, func_id: FunctionId, parameters: &[VariableId], - parameter_predicates: &[Option<(VariableId, bool)>], + parameter_predicates: &[Option<(PathPredicate, bool)>], paths: &PathAlternatives, ) -> PathAlternatives { paths @@ -1977,16 +2044,27 @@ fn remap_paths( .filter_map(|path| { let mut path = path.clone(); for (¶meter, &argument) in parameters.iter().zip(parameter_predicates) { - let Some(parameter_value) = path.remove(¶meter) else { continue }; - let Some((argument_var, argument_value)) = argument else { continue }; + let Some(parameter_value) = path.remove(&PathPredicate::Boolean(parameter)) else { + continue + }; + let Some((argument_predicate, argument_value)) = argument else { continue }; let mapped_value = if parameter_value { argument_value } else { !argument_value }; - if path.get(&argument_var).is_some_and(|existing| *existing != mapped_value) { + if path + .get(&argument_predicate) + .is_some_and(|existing| *existing != mapped_value) + { return None; } - path.insert(argument_var, mapped_value); + path.insert(argument_predicate, mapped_value); } - path.retain(|var_id, _| { - hir.variable(*var_id).parent != Some(ItemId::Function(func_id)) + path.retain(|predicate, _| { + !matches!(predicate, PathPredicate::Boolean(var_id) if hir.variable(*var_id).parent == Some(ItemId::Function(func_id))) + && !matches!( + predicate, + PathPredicate::Equality(lhs, rhs) + if lhs.variable().is_some_and(|var_id| hir.variable(var_id).parent == Some(ItemId::Function(func_id))) + || rhs.variable().is_some_and(|var_id| hir.variable(var_id).parent == Some(ItemId::Function(func_id))) + ) }); Some(path) }) @@ -2884,13 +2962,13 @@ fn collect_local_write_lhs_vars( fn forget_path_predicates(state: &mut FlowState, vars: impl IntoIterator) { for var_id in vars { - state.path_predicates.remove(&var_id); + state.path_predicates.retain(|predicate, _| !predicate.contains(var_id)); for paths in state.balance_local_paths.values_mut() { *paths = paths .iter() .map(|path| { let mut path = path.clone(); - path.remove(&var_id); + path.retain(|predicate, _| !predicate.contains(var_id)); path }) .collect(); @@ -2900,7 +2978,7 @@ fn forget_path_predicates(state: &mut FlowState, vars: impl IntoIterator= balanceBefore + amount, "insufficient payment"); } + function sequentialComplementaryComparisonPaths( + IReentrancyBalanceCallback callback, + uint256 amount, + uint256 mode + ) external { + uint256 balanceBefore = address(this).balance; + if (mode == 0) callback.pay(); + if (mode != 0) { + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + } + + function sequentialSameComparisonPath( + IReentrancyBalanceCallback callback, + uint256 amount, + uint256 mode + ) external { + uint256 balanceBefore = address(this).balance; + if (mode == 0) callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + if (0 == mode) { + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + } + + function reassignedComparisonPredicateStillWarns( + IReentrancyBalanceCallback callback, + uint256 amount, + uint256 mode + ) external { + uint256 balanceBefore = address(this).balance; + if (mode == 0) callback.pay(); //~WARN: external call can be reentered before a stale contract balance is checked + mode = 1; + if (mode != 0) { + require(address(this).balance >= balanceBefore + amount, "insufficient payment"); + } + } + function shortCircuitAndComplementaryPaths( IReentrancyBalanceCallback callback, uint256 amount, diff --git a/crates/lint/testdata/ReentrancyBalance.stderr b/crates/lint/testdata/ReentrancyBalance.stderr index 91a6e7035276f..700ff72f9f751 100644 --- a/crates/lint/testdata/ReentrancyBalance.stderr +++ b/crates/lint/testdata/ReentrancyBalance.stderr @@ -262,6 +262,22 @@ LL │ callback.pay(); │ ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ if (mode == 0) callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + +warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked + ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC + │ +LL │ if (mode == 0) callback.pay(); + │ ━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/reentrancy-balance + warning[reentrancy-balance]: external call can be reentered before a stale contract balance is checked ╭▸ ROOT/testdata/ReentrancyBalance.sol:LL:CC │