fix(lint): detect repeated flash-loan repayment pulls inside a loop

.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.

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<Vec<State>>,
+    /// 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<HashMap<PendingRepayment, u32>>,
     /// Every variable written on any path.
     written: HashSet<VariableId>,
     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<PendingRepayment> = 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<PendingRepayment, u32>| {
+            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)) {

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) {

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
    │