diff --git a/.changelog/flashloan-loop-repeated-pull.md b/.changelog/flashloan-loop-repeated-pull.md new file mode 100644 index 0000000000000..4e55abaa69400 --- /dev/null +++ b/.changelog/flashloan-loop-repeated-pull.md @@ -0,0 +1,5 @@ +--- +forge-lint: patch +--- + +Fixed the `arbitrary-send-erc20` lint failing to detect a repeated flash-loan repayment pull inside a loop: a single `onFlashLoan` callback minted before a loop could license every `transferFrom` the (single-pass) loop body happened to contain, even though that same sink re-executes every iteration against the one license at runtime. diff --git a/crates/lint/src/sol/high/arbitrary_send_erc20.rs b/crates/lint/src/sol/high/arbitrary_send_erc20.rs index aa794e273f101..83dbcea3a09fd 100644 --- a/crates/lint/src/sol/high/arbitrary_send_erc20.rs +++ b/crates/lint/src/sol/high/arbitrary_send_erc20.rs @@ -185,6 +185,14 @@ struct Analyzer<'hir> { state: State, /// States at `break`/`continue` of each enclosing loop, innermost last. loop_exits: Vec>, + /// Repayment counts as they stood at the entry of each enclosing loop's body, innermost + /// last. A repayment can only be consumed inside a loop if its count exceeds this floor, + /// i.e. it was freshly minted since that loop was entered - a repayment already pending + /// beforehand must not license a sink the single-pass body walk merely happens to contain, + /// since at runtime that same sink may re-execute every iteration against the one license. + /// Unlike `State`, this is never snapshotted or restored around `if`/`try` branches - see + /// `invalidate`'s comment on why it must never be mutated in place mid-analysis. + loop_repayment_floors: Vec>, /// Every variable written on any path. written: HashSet, hits: Vec<(Span, &'static SolLint)>, @@ -198,6 +206,7 @@ impl<'hir> Analyzer<'hir> { has_solady_lib, state: State::default(), loop_exits: Vec::new(), + loop_repayment_floors: Vec::new(), written: HashSet::new(), hits: Vec::new(), } @@ -325,6 +334,14 @@ impl<'hir> Analyzer<'hir> { s.sum_of.retain(|k, (a, b)| *k != v && *a != v && *b != v); s.permits.retain(|p| !p.token.touches(v) && p.owner != v); s.repayments.retain(|r, _| ![r.receiver, r.token, r.amount, r.fee].contains(&v)); + // Deliberately NOT purged here: `loop_repayment_floors` is shared, mutate-in-place + // analyzer state, not part of `State` - unlike everything above, it isn't snapshotted + // and restored around `if`/`try` branches. Purging a floor entry here would leak across + // sibling branches (e.g. a `then` that reassigns `v` would permanently drop the floor + // before the `else` branch, sharing the same loop, is ever analyzed), turning a real + // repeated-pull vulnerability into a false negative. Reassigning a repayment's own key + // variable mid-loop and re-minting is a rare enough shape that the resulting false + // positive (see `badFlashLoanReassignThenRemintInLoop`) is the safer trade-off. } fn eval_rhs(&self, rhs: Option<&Expr<'_>>) -> Rhs { @@ -498,11 +515,30 @@ impl<'hir> Analyzer<'hir> { if !self.is_self_expr(sink.to) { return false; } - let Some(rep) = self.state.repayments.keys().copied().find(|r| { - r.receiver == from - && r.token == token - && self.amount_matches(sink.amount, r.amount, r.fee) - }) else { + let candidates: Vec = self + .state + .repayments + .keys() + .copied() + .filter(|r| { + r.receiver == from + && r.token == token + && self.amount_matches(sink.amount, r.amount, r.fee) + }) + .collect(); + // `amount + fee` and `fee + amount` both match, so an outer (pre-loop) repayment and an + // in-loop fresh one can both structurally match the same sink. Prefer one this loop + // hasn't already spent its entry-floor allowance on, rather than an arbitrary HashMap + // iteration order picking the stale one and rejecting a genuinely fresh license. + let floor = self.loop_repayment_floors.last(); + let is_fresh = |r: &PendingRepayment, repayments: &HashMap| { + let count = repayments.get(r).copied().unwrap_or(0); + let floor = floor.and_then(|f| f.get(r)).copied().unwrap_or(0); + count > floor + }; + let Some(rep) = + candidates.into_iter().find(|r| is_fresh(r, &self.state.repayments)) + else { return false; }; match self.state.repayments.get_mut(&rep) { @@ -551,10 +587,26 @@ impl<'hir> Analyzer<'hir> { // all; `do-while` bodies run at least once. let baseline = (!matches!(source, LoopSource::DoWhile)).then(|| self.state.clone()); self.loop_exits.push(baseline.into_iter().collect()); + // The body is walked as a single static pass, but a real loop may run it many + // times. Record the repayment counts as they stand on entry so `consume_repayment` + // can refuse to spend a repayment that was already pending beforehand - such a + // license must not cover a sink the single-pass body walk merely happens to + // contain, since at runtime that sink may re-execute every iteration against the + // one license. A repayment minted (and consumed) inside this same body pass is + // unaffected, since its count exceeds the floor recorded here; repayments the body + // never touches at all pass through the eventual `meet` unchanged, so code after + // the loop still sees them. + // + // This has no notion of a loop shape that provably runs its body at most once + // (`do { .. } while (false)`, `while (cond) { ..; break; }`), so those flag a + // pull they can't actually repeat. Accepted: a narrow false positive on an + // otherwise-safe idiom, traded for never missing a real repeated-pull drain. + self.loop_repayment_floors.push(self.state.repayments.clone()); if self.visit_stmts(block.stmts) { let state = self.state.clone(); self.loop_exits.last_mut().expect("pushed above").push(state); } + self.loop_repayment_floors.pop().expect("pushed above"); let exits = self.loop_exits.pop().expect("pushed above"); let falls = !exits.is_empty(); if let Some(joined) = exits.into_iter().reduce(|a, b| a.meet(&b)) { diff --git a/crates/lint/testdata/ArbitrarySendErc20.sol b/crates/lint/testdata/ArbitrarySendErc20.sol index bdea5de25defb..3c6e6c12558b3 100644 --- a/crates/lint/testdata/ArbitrarySendErc20.sol +++ b/crates/lint/testdata/ArbitrarySendErc20.sol @@ -734,6 +734,125 @@ contract ArbitrarySendErc20 { token.transferFrom(x, to, a); } + // A single flash-loan callback must not license repeated pulls across loop iterations. + function badFlashLoanLoopPull( + IERC3156FlashBorrower receiver, + uint256 amount, + uint256 fee, + uint256 n, + bytes calldata data + ) public { + receiver.onFlashLoan(msg.sender, address(token), amount, fee, data); + for (uint256 i = 0; i < n; i++) { + token.transferFrom(address(receiver), address(this), amount + fee); //~WARN: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + } + } + + // Mint-and-consume both inside the same loop body is still safe on every iteration. + function okFlashLoanPerIterationMintAndPull( + IERC3156FlashBorrower receiver, + uint256 amount, + uint256 fee, + uint256 n, + bytes calldata data + ) public { + for (uint256 i = 0; i < n; i++) { + receiver.onFlashLoan(msg.sender, address(token), amount, fee, data); + token.transferFrom(address(receiver), address(this), amount + fee); + } + } + + // A pull *after* a loop that never touches the repayment is still safely licensed by the + // pre-loop mint - the loop-entry floor only restricts sinks reached *inside* that loop. + function okFlashLoanPostLoopPullStillGuarded( + IERC3156FlashBorrower receiver, + uint256 amount, + uint256 fee, + uint256 n, + bytes calldata data + ) public { + receiver.onFlashLoan(msg.sender, address(token), amount, fee, data); + for (uint256 i = 0; i < n; i++) { + unrelatedSideEffect(); + } + token.transferFrom(address(receiver), address(this), amount + fee); + } + + // Same shape, nested: an inner loop unrelated to the repayment must not strip the license + // from a pull that happens after both loops return. + function okFlashLoanNestedUnrelatedLoop( + IERC3156FlashBorrower receiver, + uint256 amount, + uint256 fee, + uint256 n, + uint256 m, + bytes calldata data + ) public { + receiver.onFlashLoan(msg.sender, address(token), amount, fee, data); + for (uint256 i = 0; i < n; i++) { + for (uint256 j = 0; j < m; j++) { + unrelatedSideEffect(); + } + } + token.transferFrom(address(receiver), address(this), amount + fee); + } + + // `do { ... } while (false)` provably runs exactly once, so the repeated-pull hazard the + // loop-entry floor guards against doesn't actually apply here - but the lint has no + // constant-condition reasoning to prove that, so it conservatively still flags this as if + // the license could be replayed. Accepted false positive on an otherwise-safe idiom, in + // exchange for never missing a real repeated-pull drain. + function badFlashLoanDoWhileFalseSingleIteration( + IERC3156FlashBorrower receiver, + uint256 amount, + uint256 fee, + bytes calldata data + ) public { + receiver.onFlashLoan(msg.sender, address(token), amount, fee, data); + do { + token.transferFrom(address(receiver), address(this), amount + fee); //~WARN: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + } while (false); + } + + // Same accepted trade-off, spelled as a `while` that always breaks on its first pass - also + // provably single-iteration, also unrecognised as such, also intentionally still flagged. + function badFlashLoanWhileBreakSingleIteration( + IERC3156FlashBorrower receiver, + uint256 amount, + uint256 fee, + bool cond, + bytes calldata data + ) public { + receiver.onFlashLoan(msg.sender, address(token), amount, fee, data); + while (cond) { + token.transferFrom(address(receiver), address(this), amount + fee); //~WARN: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + break; + } + } + + // Reassigning a repayment's own key variable mid-loop invalidates the stale pre-loop record, + // and the fresh `onFlashLoan` right after re-mints the same key - but the loop-entry floor + // isn't corrected for that, since doing so in-place would leak across sibling `if`/`try` + // branches sharing this loop (see the comment on `invalidate`). Accepted false positive on + // a rare shape, in exchange for the floor mechanism never producing a branch-sensitive + // false negative. + function badFlashLoanReassignThenRemintInLoop( + IERC3156FlashBorrower receiver, + uint256 amount, + uint256 fee, + uint256 n, + bytes calldata data + ) public { + receiver.onFlashLoan(msg.sender, address(token), amount, fee, data); + for (uint256 i = 0; i < n; i++) { + receiver = receiver; + receiver.onFlashLoan(msg.sender, address(token), amount, fee, data); + token.transferFrom(address(receiver), address(this), amount + fee); //~WARN: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + } + } + + function unrelatedSideEffect() internal {} + // -- MODIFIER BODY SINKS -- modifier pullBad(address from, address to, uint256 a) { diff --git a/crates/lint/testdata/ArbitrarySendErc20.stderr b/crates/lint/testdata/ArbitrarySendErc20.stderr index 75715184387ec..86d97a809325c 100644 --- a/crates/lint/testdata/ArbitrarySendErc20.stderr +++ b/crates/lint/testdata/ArbitrarySendErc20.stderr @@ -342,6 +342,38 @@ LL │ … token.transferFrom(x, to, a); │ ╰ help: https://getfoundry.sh/forge/linting/arbitrary-send-erc20 +warning[arbitrary-send-erc20]: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + ╭▸ ROOT/testdata/ArbitrarySendErc20.sol:LL:CC + │ +LL │ … token.transferFrom(address(receiver), address(this), amount + fee); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/arbitrary-send-erc20 + +warning[arbitrary-send-erc20]: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + ╭▸ ROOT/testdata/ArbitrarySendErc20.sol:LL:CC + │ +LL │ … token.transferFrom(address(receiver), address(this), amount + fee); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/arbitrary-send-erc20 + +warning[arbitrary-send-erc20]: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + ╭▸ ROOT/testdata/ArbitrarySendErc20.sol:LL:CC + │ +LL │ … token.transferFrom(address(receiver), address(this), amount + fee); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/arbitrary-send-erc20 + +warning[arbitrary-send-erc20]: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` + ╭▸ ROOT/testdata/ArbitrarySendErc20.sol:LL:CC + │ +LL │ … token.transferFrom(address(receiver), address(this), amount + fee); + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: https://getfoundry.sh/forge/linting/arbitrary-send-erc20 + warning[arbitrary-send-erc20]: `transferFrom` uses an arbitrary `from`; require it to equal `msg.sender` or `address(this)` ╭▸ ROOT/testdata/ArbitrarySendErc20.sol:LL:CC │