Skip to content

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

Open
gomesalexandre wants to merge 1 commit into
foundry-rs:masterfrom
gomesalexandre:fix_flashloan_loop_repeated_pull
Open

fix(lint): detect repeated flash-loan repayment pulls inside a loop#16628
gomesalexandre wants to merge 1 commit into
foundry-rs:masterfrom
gomesalexandre:fix_flashloan_loop_repeated_pull

Conversation

@gomesalexandre

Copy link
Copy Markdown
Contributor

arbitrary-send-erc20 tracks flash-loan repayment licenses (one per onFlashLoan call) consumed by a matching transferFrom pull. The loop body is analyzed as a single static pass, so a repayment minted before a loop could license every pull the (single-pass) body happened to contain, even though that same pull re-executes every iteration against the one license at runtime - a real fund-drain false negative:

receiver.onFlashLoan(msg.sender, address(token), amount, fee, data);
for (uint256 i = 0; i < n; i++) {
    token.transferFrom(address(receiver), address(this), amount + fee); // was silently OK
}

Fix

Track repayment counts as they stood at each loop's entry (loop_repayment_floors), and refuse to consume a repayment inside a loop unless its count exceeds that entry floor, i.e. it was freshly minted since the loop was entered. Repayments a loop never touches at all pass through the eventual meet unchanged, so code after the loop still sees them.

Two related correctness issues surfaced during review and are fixed too:

  • consume_repayment now searches for an eligible matching key instead of taking the first HashMap-order match and checking the floor after, since amount + fee and fee + amount can both structurally match.
  • The floor mechanism is deliberately never touched by invalidate(), since that stack isn't snapshotted/restored around if/try branches the way State is - mutating it there would leak across sibling branches and turn a real vulnerability into a false negative.

Accepted trade-offs

Provably single-iteration loop shapes (do-while(false), while with an unconditional break) and a mid-loop reassign-then-remint of a repayment's own key variable are disclosed, accepted false positives - documented in code comments and pinned by dedicated fixtures - traded for never missing a real repeated-pull drain.

Testing

  • badFlashLoanLoopPull - mint outside a loop, pull inside it: now correctly warns (was silent before this fix).
  • okFlashLoanPerIterationMintAndPull - mint and pull both inside the same loop body: stays silent (legitimate per-iteration pattern, unaffected).
  • okFlashLoanPostLoopPullStillGuarded / okFlashLoanNestedUnrelatedLoop - a pull after a loop (or nested loops) that never touch the repayment stays silently licensed by the pre-loop mint.
  • badFlashLoanDoWhileFalseSingleIteration / badFlashLoanWhileBreakSingleIteration / badFlashLoanReassignThenRemintInLoop - the three accepted-trade-off cases above, pinned as intentional warnings.
  • Full crates/lint/testdata UI suite (97 fixtures, not just this file) passes.
  • .changelog/flashloan-loop-repeated-pull.md added per this repo's changelog requirement.

Went through four rounds of adversarial (Codex) review during development - each round's finding is reflected in the fixtures/comments above, not just fixed silently.

`arbitrary-send-erc20` tracks flash-loan repayment licenses (one per
`onFlashLoan` call) consumed by a matching `transferFrom` pull. The loop
body is analyzed as a single static pass, so a repayment minted before a
loop could license every pull the (single-pass) body happened to contain,
even though that same pull re-executes every iteration against the one
license at runtime - a real fund-drain false negative.

Fix: track repayment counts as they stood at each loop's entry
(`loop_repayment_floors`), and refuse to consume a repayment inside a
loop unless its count exceeds that entry floor, i.e. it was freshly
minted since the loop was entered. Repayments a loop never touches pass
through unaffected, so code after the loop still sees them.

Two related correctness issues surfaced during review and are also fixed:
- `consume_repayment` now searches for an eligible matching key instead
  of taking the first HashMap-order match and checking the floor after,
  since `amount + fee` and `fee + amount` can both structurally match.
- The floor mechanism is deliberately never touched by `invalidate()`,
  since that stack isn't snapshotted/restored around `if`/`try` branches
  the way `State` is - mutating it there would leak across sibling
  branches and turn a real vulnerability into a false negative.

Provably single-iteration loop shapes (`do-while(false)`, `while` with an
unconditional `break`) and a mid-loop reassign-then-remint of a
repayment's own key variable are accepted, disclosed false positives -
documented in comments and pinned by dedicated fixtures - traded for
never missing a real repeated-pull drain.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lh6V2uPTUqauqq45BM7m5k
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

✅ Changelog found

The deterministic check will validate the changed entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant