CRITICAL FINDING (F-9): deposit-as-repayment reentrancy on live v2 pools - #83
Merged
Merged
Conversation
flashstack-stx-pool-v2 and flashstack-sbtc-pool-v2 have no reentrancy lock. A flash-loan receiver can repay by calling the pool's own deposit() instead of transferring funds back. That satisfies the balance-delta repayment check (the pool's balance genuinely grows by amount+fee) but also mints the caller LP shares, priced against the pool's balance as depressed by this same loan's own outbound transfer -- a far cheaper price than any honest depositor gets. Shares land on tx-sender (the original caller), not the receiver contract. Same bug class as pv3-F1, never checked against the two pools that predate pool-v3 and are actually live with real funds. Proven in simnet (tests/stx-pool-v2-deposit-reentrancy.test.ts, receiver contracts/test/test-pool-v2-receiver-deposit-reentrant.clar) against the localized copy of the live flashstack-stx-pool-v2 contract (SPR9PQAN..., 1.000000 STX, paused=false): honest LP deposits 100 STX; attacker fronts only 0.01 STX (fee-sized capital), borrows 99 STX, and ends up owning effectively 100% of the pool's shares. The honest LP's 100 STX position is left worth 1 STX -- 99% extracted in a single transaction. flashstack-sbtc-pool-v2 was read directly (contracts/flashstack-sbtc-pool-v2.clar:84-163) and has an identical deposit/flash-loan structure with no lock -- same vector, 44,990 sats live, not yet independently simnet-proven. The two cores (flashstack-stx-core, flashstack-sbtc-core) are NOT exposed: their deposit-reserve equivalent is tx-sender==admin gated, which a receiver callback cannot satisfy. Bounded by the receiver-approval whitelist (flash-loan checks the receiver principal, not tx-sender, so any already-approved contract can be invoked by anyone) -- but nothing in the current review process distinguishes "repays via transfer" from "repays via deposit." Immediate, zero-code mitigation: set-paused true on both live v2 pools blocks flash-loan entirely (the only entry point for this vector); deposit/withdraw correctly stay open. No in-place fix is possible on these immutable contracts -- a real fix needs a v3-equivalent successor with a per-asset lock, mirroring pv3-F1 and the BC1 pattern. Full write-up in docs/security/FINDINGS_REGISTER.md (F-9). Tracked in Flashstack-ajv.4.8. Full suite: 260 passed, 3 skipped / 263 total across 26 files -- no regressions.
Independent re-verification against live chain state before escalating: fresh Hiro fetch confirms the PoC ran against the real deployed flashstack-stx-pool-v2 source (diffs only in the expected address localization); both live pools read paused=false, total-loans=0 right now; the admin wallet's complete transaction history (24/24 txs) shows zero add-approved-receiver calls against either v2 pool. No receiver has ever been whitelisted, so the vector is real but unreachable today -- not mitigated, just not yet opened. A control run with an honest receiver (repay by plain transfer) credits zero shares, isolating the effect to the deposit-reentrancy mechanism rather than a test-harness artifact.
… repro The original PoC's receiver called deposit() without as-contract, so tx-sender stayed the attacker throughout the nested call. That meant deposit's transfer silently pulled amount+fee from the attacker's own wallet (masked by Clarinet's 100M STX default simnet balance) instead of from the loan the receiver was holding, and credited shares to the attacker's EOA. Wrapping the deposit call in as-contract fixes both: the transfer is funded by the loan itself, and shares land on the receiver contract -- the economically realistic construction, and the one Matt's own from-scratch reproduction used. Corrected numbers (independently re-derived three ways: hand arithmetic against the share-mint formula, a fresh on-chain-equivalent read, and a boundary test pinning the exact minimum capital at 49,500 uSTX): receiver ends with 9,904,940,194,109,305 of 10,004,940,194,109,305 total shares worth 99,049,499 uSTX, a ~1,981x return on a 50,000 uSTX buffer. Honest LP's 99% loss is unchanged. FINDINGS_REGISTER.md F-9 updated to match, with a note on what was wrong and why.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
tests/mainnet-plan-guard.test.ts pins the exact set of contracts/test/ files clarinet's mainnet deploy plan would publish (D6, Flashstack-ajv.6.6). The new F-9 receiver, test-pool-v2-receiver-deposit-reentrant.clar, is test-only infrastructure and belongs on that list like its siblings.
mattglory
approved these changes
Oct 1, 2026
mattglory
left a comment
mattglory
left a comment
Owner
There was a problem hiding this comment.
Independently re-verified the core claims rather than reviewing the diff on its own:
- Construction:
as-contract-wrappeddeposit(amount + fee), fee pulled live viaget-fee-basis-points, matches my own from-scratch repro of this vector. - Numbers: hand-verified the fee math — 99,000,000 µSTX loan × 5bp = 49,500 µSTX, and 49,500 × 2001 = 99,049,500, consistent with the "~2,001× at the exact minimum" figure and the 99,049,499 µSTX result (1 µSTX off from redemption rounding at that boundary, as documented).
- Share-count correction: the 1-unit discrepancy between our two repros is explained by
Number()precision loss aboveMAX_SAFE_INTEGERon a ~9.9 quadrillion-unit value. Your figures (ending…109,305) are correct; mine weren't — thanks for tracking that down. - Dormancy claim: re-checked independently, not taken on the PR description — confirmed.
- CI: Build, CodeQL ×2, Dependency Audit, Test Smart Contracts, Vercel all green.
Clarinet.tomlandmainnet-plan-guard.test.tsentries correctly placed and alphabetized.
Not folding in verify-f9-independently as a separate PoC — this PR's register entry already documents the independent cross-check and the as-contract correction, so a second near-duplicate fixture (and another mainnet-plan-guard entry) wouldn't add verification value, just surface area.
Approved.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
flashstack-stx-pool-v2andflashstack-sbtc-pool-v2have no reentrancy lock. A flash-loan receiver can "repay" by calling the pool's owndeposit()(wrapped inas-contract, so the deposit is funded from the loan itself) instead of a plain transfer. That satisfies the balance-delta repayment check but also mints the caller LP shares, priced against the pool's balance as depressed by the same loan's own outbound transfer — a far cheaper share price than any honest depositor gets, diluting every existing LP. Same bug class aspv3-F1, never ported to these two pools, which predatepool-v3and are live with real funds.flashstack-stx-pool-v2contract (SPR9PQAN…).add-approved-receivercalls against either v2 pool — nobody is whitelisted to trigger it today.set-paused trueon both live v2 pools (confirmed via freshget-statsreads —paused=true, balances unchanged,flash-loanblocked,deposit/withdrawcorrectly still open).Full detail in
docs/security/FINDINGS_REGISTER.md(F-9) andFlashstack-ajv.4.8.What's in this PR
tests/stx-pool-v2-deposit-reentrancy.test.ts+contracts/test/test-pool-v2-receiver-deposit-reentrant.clar: the simnet PoC, corrected to useas-contract(shares land on the receiver contract, funded by the loan itself — matching the economically realistic attack, and @mattglory's independent repro).docs/security/FINDINGS_REGISTER.md: F-9 entry with the corrected, independently-verified numbers.Test plan
npx vitest run tests/stx-pool-v2-deposit-reentrancy.test.tspassesverify-f9-independentlyif usefulNext steps (tracked separately, not in this PR)
No fix is possible in place — these are immutable deployed contracts. A real fix mirrors
pv3-F1's per-asset lock and needs a v3-equivalent successor for these two pools, same posture as BC1.🤖 Generated with Claude Code