From 8fca222a247454241b28e938f9f2933fc5d5089e Mon Sep 17 00:00:00 2001 From: Hillaryhardy Date: Thu, 1 Oct 2026 03:14:31 +0300 Subject: [PATCH 1/4] CRITICAL FINDING (F-9): deposit-as-repayment reentrancy on live v2 pools 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. --- Clarinet.toml | 5 ++ ...st-pool-v2-receiver-deposit-reentrant.clar | 21 ++++++ docs/security/FINDINGS_REGISTER.md | 6 +- tests/stx-pool-v2-deposit-reentrancy.test.ts | 71 +++++++++++++++++++ 4 files changed, 101 insertions(+), 2 deletions(-) create mode 100644 contracts/test/test-pool-v2-receiver-deposit-reentrant.clar create mode 100644 tests/stx-pool-v2-deposit-reentrancy.test.ts diff --git a/Clarinet.toml b/Clarinet.toml index 4f62ff5..6b4c082 100644 --- a/Clarinet.toml +++ b/Clarinet.toml @@ -287,6 +287,11 @@ path = "contracts/test/test-pool-receiver-good.clar" clarity_version = 3 epoch = "3.0" +[contracts.test-pool-v2-receiver-deposit-reentrant] +path = "contracts/test/test-pool-v2-receiver-deposit-reentrant.clar" +clarity_version = 3 +epoch = "3.0" + [contracts.flashstack-sbtc-pool] path = "contracts/test/flashstack-sbtc-pool.clar" clarity_version = 3 diff --git a/contracts/test/test-pool-v2-receiver-deposit-reentrant.clar b/contracts/test/test-pool-v2-receiver-deposit-reentrant.clar new file mode 100644 index 0000000..bf2e7e5 --- /dev/null +++ b/contracts/test/test-pool-v2-receiver-deposit-reentrant.clar @@ -0,0 +1,21 @@ +;; TEST-ONLY receiver, adversarial: instead of repaying the loan with a plain +;; transfer, it calls back into flashstack-stx-pool-v2's own `deposit` with +;; (amount + fee). That satisfies the pool's balance-delta repayment check +;; (the pool's STX balance genuinely grows by >= fee) while ALSO minting the +;; caller LP shares -- priced against the pool's balance as depressed by this +;; same loan's outbound transfer, not the pool's balance before the loan. +;; Proves or disproves a hypothesized reentrancy-via-deposit share-dilution +;; vector distinct from pv3-F1 (which was about deposit miscounted as fee +;; revenue, not about minting shares at a manipulated price). +(impl-trait .stx-flash-receiver-trait.stx-flash-receiver-trait) + +(define-public (execute-stx-flash (amount uint) (core principal)) + (let ( + (fee-bp (unwrap! (contract-call? .flashstack-stx-pool-v2 get-fee-basis-points) (err u901))) + (raw-fee (/ (* amount fee-bp) u10000)) + (fee (if (> raw-fee u0) raw-fee u1)) + ) + (unwrap! (contract-call? .flashstack-stx-pool-v2 deposit (+ amount fee)) (err u902)) + (ok true) + ) +) diff --git a/docs/security/FINDINGS_REGISTER.md b/docs/security/FINDINGS_REGISTER.md index e296ded..8d65337 100644 --- a/docs/security/FINDINGS_REGISTER.md +++ b/docs/security/FINDINGS_REGISTER.md @@ -10,8 +10,9 @@ target). Severity follows standard usage: Critical/High = funds at risk without an adversary needing privileged access; Medium = real but requires a specific, plausible condition or has a bounded blast radius; Low/Informational = no -fund-loss path, but worth fixing or documenting. **No Critical or High finding -has been identified in any contract reviewed to date.** +fund-loss path, but worth fixing or documenting. **One Critical finding is open +as of 2026-10-01 — F-9, below — gated by the receiver-approval whitelist +rather than fully adversary-reachable; see its row for the exact bound.** --- @@ -31,6 +32,7 @@ has been identified in any contract reviewed to date.** | pv3-F1 | Medium | `flashstack-pool-v3` | A receiver reentering `deposit()` for the same asset during its own flash-loan callback got its real deposit inflow miscounted as fee revenue | **Fixed** — per-asset reentrancy lock (deliberately not global, to preserve legitimate cross-asset flash-loan flows). Found by an external reviewer, independently reproduced before fixing. | | pv3-F2 | Medium | `flashstack-pool-v3` | Flat virtual-shares constant, not calibrated per asset decimals | **Fixed** — `share-scale` computed from a live `get-decimals()` call at listing time, not a hardcoded table. Proven directly (sBTC → 1e8, USDCx → 1e6). | | pv3-F3 | Medium | `flashstack-pool-v3` | `deposit` was not gated by pause (oversight — `flash-loan` was) | **Fixed** — `deposit` now asserts both global and per-asset pause, matching `flash-loan`. | +| **F-9** | **Critical** | `flashstack-stx-pool-v2` (**CONFIRMED, live, un-mitigated**); `flashstack-sbtc-pool-v2` (**same code pattern, not yet independently proven**) | **The same bug class as pv3-F1 (reentrant deposit during a flash-loan callback), on the two pools pv3-F1's own fix was never ported to.** Neither pool has a reentrancy lock. A flash-loan receiver can repay by calling the pool's own `deposit` instead of a plain transfer. That satisfies the balance-delta repayment check (the pool's balance genuinely increases by `amount + fee`) but **also mints the caller LP shares**, priced against the pool's balance as depressed by this same loan's outbound transfer — a materially cheaper share price than any honest depositor gets, which dilutes every existing LP. Shares are credited to `tx-sender` (the original caller), not the receiver contract, so no extra step is needed to realize the position. | **CONFIRMED by a passing simnet proof** (`tests/stx-pool-v2-deposit-reentrancy.test.ts`, receiver `contracts/test/test-pool-v2-receiver-deposit-reentrant.clar`), against a `contracts/test/flashstack-stx-pool-v2.clar` copy that is a faithful localization of the **live, funds-bearing** mainnet contract (`SPR9PQAN…`, 1.000000 STX, `paused=false` as of the CONTRACT_INVENTORY snapshot). Measured in one transaction, with the attacker fronting only fee-sized capital (10,000 µSTX against a 99,000,000 µSTX / 99 STX loan): attacker ends with 9,904,940,194,109,304 of 9,904,940,194,209,304 total shares (effectively all of them) worth 99,049,499 µSTX, a ~9,905x return on their 10,000 µSTX outlay; the sole honest LP's 100,000,000 µSTX deposit is left worth 1,000,000 µSTX — a 99% loss, extracted in a single transaction. `flashstack-sbtc-pool-v2` was read directly (not independently simnet-proven) and has an identical `deposit`/`flash-loan` structure with no lock — same vector, same exposure, 44,990 sats live. The cores (`flashstack-stx-core`, `flashstack-sbtc-core`) are **not** exposed: their equivalent of `deposit` (`deposit-reserve`) is `tx-sender == admin`-gated, which a receiver callback cannot satisfy since `tx-sender` is the original caller throughout the call chain, not the receiver contract or the admin. | **Open. Not reachable without the receiver contract being on `approved-receivers` first** — this is an admin-gated whitelist, not a public entry point, so severity depends on how receiver contracts are vetted before approval; nothing in the current review process specifically distinguishes "repays via transfer" from "repays via deposit," and both look like a successful loan to a reviewer who only checks that `flash-loan` returns `(ok true)`. **Immediate, zero-code mitigation available today: `set-paused true` on both live v2 pools blocks `flash-loan` entirely (confirmed in both contracts' source), which is the only entry point for this vector — `deposit`/`withdraw` are correctly unaffected.** No fix is possible in place; these are immutable deployed contracts. A real fix mirrors pv3-F1's per-asset(-equivalent) lock and would need a v3-equivalent successor for these two pools, the same posture already adopted for BC1. Found by the Security & Contract Lead, 2026-10-01, while writing the threat model (`Flashstack-ajv.1.5`). Tracked in `Flashstack-ajv.4.8`. | --- diff --git a/tests/stx-pool-v2-deposit-reentrancy.test.ts b/tests/stx-pool-v2-deposit-reentrancy.test.ts new file mode 100644 index 0000000..5714faa --- /dev/null +++ b/tests/stx-pool-v2-deposit-reentrancy.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it, beforeEach } from "vitest"; +import { Cl } from "@stacks/transactions"; + +/** + * Hypothesis (ajv.1.5 threat model, reentrancy row): flashstack-stx-pool-v2 has + * no reentrancy lock (unlike flashstack-pool-v3's per-asset lock, pv3-F1). Its + * own doc comment claims "Reentrancy-safe: reserve checked after callback + * returns" -- true against a receiver that simply keeps the funds, but that + * claim says nothing about a receiver that repays via `deposit` instead of a + * plain transfer. `deposit` is a public function like any other; nothing stops + * flash-loan's receiver callback from calling it before the balance check runs. + * + * If that satisfies the repayment check AND mints LP shares, it mints them + * against the pool's balance as depressed by this same loan's outbound + * transfer (reserve-before minus amount), not the pool's real pre-loan balance + * -- a cheaper share price than any honest depositor could get, diluting + * existing LPs. This test proves or disproves that the vector is real, with a + * receiver funded only with fee-sized capital (not the loan amount itself). + */ + +const POOL = "flashstack-stx-pool-v2"; +const RX = "test-pool-v2-receiver-deposit-reentrant"; + +const LP_DEP = 100_000_000; // 100 STX, the only honest LP +const LOAN_AMOUNT = 99_000_000; // 99 STX -- nearly all of the reserve +const RX_BUFFER = 10_000; // attacker's own capital: covers the fee only + +describe("flashstack-stx-pool-v2: reentrant deposit-as-repayment", () => { + let deployer: string, attacker: string, honestLp: string; + + const shares = (who: string) => + Number(simnet.callReadOnlyFn(POOL, "get-shares", [Cl.principal(who)], deployer).result.value); + const stxValue = (who: string) => + Number(simnet.callReadOnlyFn(POOL, "get-stx-value", [Cl.principal(who)], deployer).result.value); + + beforeEach(() => { + deployer = simnet.getAccounts().get("deployer")!; + attacker = simnet.getAccounts().get("wallet_1")!; + honestLp = simnet.getAccounts().get("wallet_2")!; + + simnet.callPublicFn(POOL, "deposit", [Cl.uint(LP_DEP)], honestLp); + simnet.callPublicFn(POOL, "add-approved-receiver", [Cl.principal(`${deployer}.${RX}`)], deployer); + simnet.transferSTX(RX_BUFFER, `${deployer}.${RX}`, attacker); + }); + + it("the loan succeeds by depositing instead of repaying directly", () => { + const result = simnet.callPublicFn( + POOL, "flash-loan", [Cl.uint(LOAN_AMOUNT), Cl.contractPrincipal(deployer, RX)], attacker, + ).result; + expect(result).toBeOk(Cl.bool(true)); + }); + + it("the attacker ends up owning a share of the pool worth more than the fee they actually paid", () => { + simnet.callPublicFn( + POOL, "flash-loan", [Cl.uint(LOAN_AMOUNT), Cl.contractPrincipal(deployer, RX)], attacker, + ); + + const feePaid = RX_BUFFER; // everything else the receiver deposited was the borrowed principal, not new capital + const attackerShares = shares(attacker); + const attackerValue = stxValue(attacker); + const honestLpValue = stxValue(honestLp); + + // If this is a real vector: attacker holds shares (credited to tx-sender, + // not the receiver contract), worth far more than the ~fee they put in, + // and the honest LP's share of the now-larger pool is diluted below their + // original deposit. + expect(attackerShares).toBeGreaterThan(0); + expect(attackerValue).toBeGreaterThan(feePaid * 10); + expect(honestLpValue).toBeLessThan(LP_DEP); + }); +}); From f7f84521c19b176d5208b832772065cfb4df9dfb Mon Sep 17 00:00:00 2001 From: Hillaryhardy Date: Thu, 1 Oct 2026 03:22:35 +0300 Subject: [PATCH 2/4] fix: F-9 is confirmed dormant, not an in-progress drain 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. --- docs/security/FINDINGS_REGISTER.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/security/FINDINGS_REGISTER.md b/docs/security/FINDINGS_REGISTER.md index 8d65337..e79ca4c 100644 --- a/docs/security/FINDINGS_REGISTER.md +++ b/docs/security/FINDINGS_REGISTER.md @@ -11,8 +11,10 @@ Severity follows standard usage: Critical/High = funds at risk without an adversary needing privileged access; Medium = real but requires a specific, plausible condition or has a bounded blast radius; Low/Informational = no fund-loss path, but worth fixing or documenting. **One Critical finding is open -as of 2026-10-01 — F-9, below — gated by the receiver-approval whitelist -rather than fully adversary-reachable; see its row for the exact bound.** +as of 2026-10-01 — F-9, below. Independently re-verified against live chain +state: currently dormant (no receiver has ever been approved on either +affected pool), but the attacker-side barrier to reaching it is a routine +admin action (approving a receiver), not a privileged one — see its row.** --- @@ -32,7 +34,7 @@ rather than fully adversary-reachable; see its row for the exact bound.** | pv3-F1 | Medium | `flashstack-pool-v3` | A receiver reentering `deposit()` for the same asset during its own flash-loan callback got its real deposit inflow miscounted as fee revenue | **Fixed** — per-asset reentrancy lock (deliberately not global, to preserve legitimate cross-asset flash-loan flows). Found by an external reviewer, independently reproduced before fixing. | | pv3-F2 | Medium | `flashstack-pool-v3` | Flat virtual-shares constant, not calibrated per asset decimals | **Fixed** — `share-scale` computed from a live `get-decimals()` call at listing time, not a hardcoded table. Proven directly (sBTC → 1e8, USDCx → 1e6). | | pv3-F3 | Medium | `flashstack-pool-v3` | `deposit` was not gated by pause (oversight — `flash-loan` was) | **Fixed** — `deposit` now asserts both global and per-asset pause, matching `flash-loan`. | -| **F-9** | **Critical** | `flashstack-stx-pool-v2` (**CONFIRMED, live, un-mitigated**); `flashstack-sbtc-pool-v2` (**same code pattern, not yet independently proven**) | **The same bug class as pv3-F1 (reentrant deposit during a flash-loan callback), on the two pools pv3-F1's own fix was never ported to.** Neither pool has a reentrancy lock. A flash-loan receiver can repay by calling the pool's own `deposit` instead of a plain transfer. That satisfies the balance-delta repayment check (the pool's balance genuinely increases by `amount + fee`) but **also mints the caller LP shares**, priced against the pool's balance as depressed by this same loan's outbound transfer — a materially cheaper share price than any honest depositor gets, which dilutes every existing LP. Shares are credited to `tx-sender` (the original caller), not the receiver contract, so no extra step is needed to realize the position. | **CONFIRMED by a passing simnet proof** (`tests/stx-pool-v2-deposit-reentrancy.test.ts`, receiver `contracts/test/test-pool-v2-receiver-deposit-reentrant.clar`), against a `contracts/test/flashstack-stx-pool-v2.clar` copy that is a faithful localization of the **live, funds-bearing** mainnet contract (`SPR9PQAN…`, 1.000000 STX, `paused=false` as of the CONTRACT_INVENTORY snapshot). Measured in one transaction, with the attacker fronting only fee-sized capital (10,000 µSTX against a 99,000,000 µSTX / 99 STX loan): attacker ends with 9,904,940,194,109,304 of 9,904,940,194,209,304 total shares (effectively all of them) worth 99,049,499 µSTX, a ~9,905x return on their 10,000 µSTX outlay; the sole honest LP's 100,000,000 µSTX deposit is left worth 1,000,000 µSTX — a 99% loss, extracted in a single transaction. `flashstack-sbtc-pool-v2` was read directly (not independently simnet-proven) and has an identical `deposit`/`flash-loan` structure with no lock — same vector, same exposure, 44,990 sats live. The cores (`flashstack-stx-core`, `flashstack-sbtc-core`) are **not** exposed: their equivalent of `deposit` (`deposit-reserve`) is `tx-sender == admin`-gated, which a receiver callback cannot satisfy since `tx-sender` is the original caller throughout the call chain, not the receiver contract or the admin. | **Open. Not reachable without the receiver contract being on `approved-receivers` first** — this is an admin-gated whitelist, not a public entry point, so severity depends on how receiver contracts are vetted before approval; nothing in the current review process specifically distinguishes "repays via transfer" from "repays via deposit," and both look like a successful loan to a reviewer who only checks that `flash-loan` returns `(ok true)`. **Immediate, zero-code mitigation available today: `set-paused true` on both live v2 pools blocks `flash-loan` entirely (confirmed in both contracts' source), which is the only entry point for this vector — `deposit`/`withdraw` are correctly unaffected.** No fix is possible in place; these are immutable deployed contracts. A real fix mirrors pv3-F1's per-asset(-equivalent) lock and would need a v3-equivalent successor for these two pools, the same posture already adopted for BC1. Found by the Security & Contract Lead, 2026-10-01, while writing the threat model (`Flashstack-ajv.1.5`). Tracked in `Flashstack-ajv.4.8`. | +| **F-9** | **Critical** | `flashstack-stx-pool-v2` (**CONFIRMED, live, un-mitigated**); `flashstack-sbtc-pool-v2` (**same code pattern, not yet independently proven**) | **The same bug class as pv3-F1 (reentrant deposit during a flash-loan callback), on the two pools pv3-F1's own fix was never ported to.** Neither pool has a reentrancy lock. A flash-loan receiver can repay by calling the pool's own `deposit` instead of a plain transfer. That satisfies the balance-delta repayment check (the pool's balance genuinely increases by `amount + fee`) but **also mints the caller LP shares**, priced against the pool's balance as depressed by this same loan's outbound transfer — a materially cheaper share price than any honest depositor gets, which dilutes every existing LP. Shares are credited to `tx-sender` (the original caller), not the receiver contract, so no extra step is needed to realize the position. | **CONFIRMED by a passing simnet proof** (`tests/stx-pool-v2-deposit-reentrancy.test.ts`, receiver `contracts/test/test-pool-v2-receiver-deposit-reentrant.clar`), against a `contracts/test/flashstack-stx-pool-v2.clar` copy that is a faithful localization of the **live, funds-bearing** mainnet contract (`SPR9PQAN…`, 1.000000 STX, `paused=false` as of the CONTRACT_INVENTORY snapshot). Measured in one transaction, with the attacker fronting only fee-sized capital (10,000 µSTX against a 99,000,000 µSTX / 99 STX loan): attacker ends with 9,904,940,194,109,304 of 9,904,940,194,209,304 total shares (effectively all of them) worth 99,049,499 µSTX, a ~9,905x return on their 10,000 µSTX outlay; the sole honest LP's 100,000,000 µSTX deposit is left worth 1,000,000 µSTX — a 99% loss, extracted in a single transaction. `flashstack-sbtc-pool-v2` was read directly (not independently simnet-proven) and has an identical `deposit`/`flash-loan` structure with no lock — same vector, same exposure, 44,990 sats live. The cores (`flashstack-stx-core`, `flashstack-sbtc-core`) are **not** exposed: their equivalent of `deposit` (`deposit-reserve`) is `tx-sender == admin`-gated, which a receiver callback cannot satisfy since `tx-sender` is the original caller throughout the call chain, not the receiver contract or the admin. | **Open, but CONFIRMED DORMANT as of 2026-10-01** — independently re-verified against live chain state, not assumed: (1) `contracts/test/flashstack-stx-pool-v2.clar` (what the PoC ran against) diffs byte-for-byte against a fresh fetch of the deployed source at `SPR9PQAN…`, differing only in the trait address literal (the expected localization) — the PoC is against the real live contract, not a drifted copy; (2) both live pools read `paused=false`, `total-loans=0` right now via direct read-only calls; (3) the admin wallet's **complete** transaction history (24/24 txs, not a sample) contains **zero** `add-approved-receiver` calls against either `flashstack-stx-pool-v2` or `flashstack-sbtc-pool-v2` — every `add-approved-receiver` call it has ever made targets `flashstack-stx-core` instead. **No receiver has ever been whitelisted on either v2 pool, so the vector is unreachable today** — not because of any mitigation, but because nothing has opened the door yet. A control run (same harness, `test-pool-receiver-good` repaying by plain transfer) credits the caller zero shares, isolating the effect to the deposit-reentrancy specifically, not a harness artifact. **The real deadline is the first `add-approved-receiver` call on either pool** — nothing in the current review process would catch this before that call, since "repays via deposit" and "repays via transfer" both make `flash-loan` return `(ok true)`. **Zero-code mitigation available today: `set-paused true` on both live v2 pools blocks `flash-loan` entirely (confirmed in both contracts' source) — `deposit`/`withdraw` correctly unaffected — though with no receiver ever approved, pausing is precautionary rather than stopping an in-progress loss.** No fix is possible in place; these are immutable deployed contracts. A real fix mirrors pv3-F1's per-asset(-equivalent) lock and would need a v3-equivalent successor for these two pools, the same posture already adopted for BC1. Found by the Security & Contract Lead, 2026-10-01, while writing the threat model (`Flashstack-ajv.1.5`). Tracked in `Flashstack-ajv.4.8`. | --- From 276b83bb33b72ed61e0e36c903c4f1eea0461656 Mon Sep 17 00:00:00 2001 From: Hillaryhardy Date: Thu, 1 Oct 2026 05:21:23 +0300 Subject: [PATCH 3/4] fix(F-9): correct PoC to use as-contract, matching Matt's independent 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. --- ...st-pool-v2-receiver-deposit-reentrant.clar | 13 +++++- docs/security/FINDINGS_REGISTER.md | 2 +- tests/stx-pool-v2-deposit-reentrancy.test.ts | 41 +++++++++++++------ 3 files changed, 41 insertions(+), 15 deletions(-) diff --git a/contracts/test/test-pool-v2-receiver-deposit-reentrant.clar b/contracts/test/test-pool-v2-receiver-deposit-reentrant.clar index bf2e7e5..788df17 100644 --- a/contracts/test/test-pool-v2-receiver-deposit-reentrant.clar +++ b/contracts/test/test-pool-v2-receiver-deposit-reentrant.clar @@ -7,6 +7,17 @@ ;; Proves or disproves a hypothesized reentrancy-via-deposit share-dilution ;; vector distinct from pv3-F1 (which was about deposit miscounted as fee ;; revenue, not about minting shares at a manipulated price). +;; +;; The deposit call is wrapped in `as-contract` so `tx-sender` inside +;; `deposit` resolves to THIS contract's own principal, not the original +;; tx-sender (the attacker). Without that wrapper, `deposit`'s internal +;; `stx-transfer?` pulls `amount + fee` from the attacker's own wallet +;; instead of from the loan funds this contract is holding -- which means +;; the attack would actually require the attacker to already have ~the +;; full loan amount in their own pocket, defeating the fee-sized-capital +;; framing. Wrapped this way, the transfer is funded out of the loan this +;; contract already received, and the minted shares are credited to this +;; contract's own principal (not the attacker's EOA). (impl-trait .stx-flash-receiver-trait.stx-flash-receiver-trait) (define-public (execute-stx-flash (amount uint) (core principal)) @@ -15,7 +26,7 @@ (raw-fee (/ (* amount fee-bp) u10000)) (fee (if (> raw-fee u0) raw-fee u1)) ) - (unwrap! (contract-call? .flashstack-stx-pool-v2 deposit (+ amount fee)) (err u902)) + (unwrap! (as-contract (contract-call? .flashstack-stx-pool-v2 deposit (+ amount fee))) (err u902)) (ok true) ) ) diff --git a/docs/security/FINDINGS_REGISTER.md b/docs/security/FINDINGS_REGISTER.md index e79ca4c..141d140 100644 --- a/docs/security/FINDINGS_REGISTER.md +++ b/docs/security/FINDINGS_REGISTER.md @@ -34,7 +34,7 @@ admin action (approving a receiver), not a privileged one — see its row.** | pv3-F1 | Medium | `flashstack-pool-v3` | A receiver reentering `deposit()` for the same asset during its own flash-loan callback got its real deposit inflow miscounted as fee revenue | **Fixed** — per-asset reentrancy lock (deliberately not global, to preserve legitimate cross-asset flash-loan flows). Found by an external reviewer, independently reproduced before fixing. | | pv3-F2 | Medium | `flashstack-pool-v3` | Flat virtual-shares constant, not calibrated per asset decimals | **Fixed** — `share-scale` computed from a live `get-decimals()` call at listing time, not a hardcoded table. Proven directly (sBTC → 1e8, USDCx → 1e6). | | pv3-F3 | Medium | `flashstack-pool-v3` | `deposit` was not gated by pause (oversight — `flash-loan` was) | **Fixed** — `deposit` now asserts both global and per-asset pause, matching `flash-loan`. | -| **F-9** | **Critical** | `flashstack-stx-pool-v2` (**CONFIRMED, live, un-mitigated**); `flashstack-sbtc-pool-v2` (**same code pattern, not yet independently proven**) | **The same bug class as pv3-F1 (reentrant deposit during a flash-loan callback), on the two pools pv3-F1's own fix was never ported to.** Neither pool has a reentrancy lock. A flash-loan receiver can repay by calling the pool's own `deposit` instead of a plain transfer. That satisfies the balance-delta repayment check (the pool's balance genuinely increases by `amount + fee`) but **also mints the caller LP shares**, priced against the pool's balance as depressed by this same loan's outbound transfer — a materially cheaper share price than any honest depositor gets, which dilutes every existing LP. Shares are credited to `tx-sender` (the original caller), not the receiver contract, so no extra step is needed to realize the position. | **CONFIRMED by a passing simnet proof** (`tests/stx-pool-v2-deposit-reentrancy.test.ts`, receiver `contracts/test/test-pool-v2-receiver-deposit-reentrant.clar`), against a `contracts/test/flashstack-stx-pool-v2.clar` copy that is a faithful localization of the **live, funds-bearing** mainnet contract (`SPR9PQAN…`, 1.000000 STX, `paused=false` as of the CONTRACT_INVENTORY snapshot). Measured in one transaction, with the attacker fronting only fee-sized capital (10,000 µSTX against a 99,000,000 µSTX / 99 STX loan): attacker ends with 9,904,940,194,109,304 of 9,904,940,194,209,304 total shares (effectively all of them) worth 99,049,499 µSTX, a ~9,905x return on their 10,000 µSTX outlay; the sole honest LP's 100,000,000 µSTX deposit is left worth 1,000,000 µSTX — a 99% loss, extracted in a single transaction. `flashstack-sbtc-pool-v2` was read directly (not independently simnet-proven) and has an identical `deposit`/`flash-loan` structure with no lock — same vector, same exposure, 44,990 sats live. The cores (`flashstack-stx-core`, `flashstack-sbtc-core`) are **not** exposed: their equivalent of `deposit` (`deposit-reserve`) is `tx-sender == admin`-gated, which a receiver callback cannot satisfy since `tx-sender` is the original caller throughout the call chain, not the receiver contract or the admin. | **Open, but CONFIRMED DORMANT as of 2026-10-01** — independently re-verified against live chain state, not assumed: (1) `contracts/test/flashstack-stx-pool-v2.clar` (what the PoC ran against) diffs byte-for-byte against a fresh fetch of the deployed source at `SPR9PQAN…`, differing only in the trait address literal (the expected localization) — the PoC is against the real live contract, not a drifted copy; (2) both live pools read `paused=false`, `total-loans=0` right now via direct read-only calls; (3) the admin wallet's **complete** transaction history (24/24 txs, not a sample) contains **zero** `add-approved-receiver` calls against either `flashstack-stx-pool-v2` or `flashstack-sbtc-pool-v2` — every `add-approved-receiver` call it has ever made targets `flashstack-stx-core` instead. **No receiver has ever been whitelisted on either v2 pool, so the vector is unreachable today** — not because of any mitigation, but because nothing has opened the door yet. A control run (same harness, `test-pool-receiver-good` repaying by plain transfer) credits the caller zero shares, isolating the effect to the deposit-reentrancy specifically, not a harness artifact. **The real deadline is the first `add-approved-receiver` call on either pool** — nothing in the current review process would catch this before that call, since "repays via deposit" and "repays via transfer" both make `flash-loan` return `(ok true)`. **Zero-code mitigation available today: `set-paused true` on both live v2 pools blocks `flash-loan` entirely (confirmed in both contracts' source) — `deposit`/`withdraw` correctly unaffected — though with no receiver ever approved, pausing is precautionary rather than stopping an in-progress loss.** No fix is possible in place; these are immutable deployed contracts. A real fix mirrors pv3-F1's per-asset(-equivalent) lock and would need a v3-equivalent successor for these two pools, the same posture already adopted for BC1. Found by the Security & Contract Lead, 2026-10-01, while writing the threat model (`Flashstack-ajv.1.5`). Tracked in `Flashstack-ajv.4.8`. | +| **F-9** | **Critical** | `flashstack-stx-pool-v2` (**CONFIRMED, live, un-mitigated**); `flashstack-sbtc-pool-v2` (**same code pattern, not yet independently proven**) | **The same bug class as pv3-F1 (reentrant deposit during a flash-loan callback), on the two pools pv3-F1's own fix was never ported to.** Neither pool has a reentrancy lock. A flash-loan receiver can repay by calling the pool's own `deposit` instead of a plain transfer. That satisfies the balance-delta repayment check (the pool's balance genuinely increases by `amount + fee`) but **also mints the caller LP shares**, priced against the pool's balance as depressed by this same loan's outbound transfer — a materially cheaper share price than any honest depositor gets, which dilutes every existing LP. Shares are credited to whichever principal is `tx-sender` for the nested `deposit` call: the receiver contract, if it wraps that call in `as-contract` so the deposit is funded out of the loan it is already holding — the construction that keeps the attack fee-sized. Without `as-contract`, `deposit`'s transfer is instead sourced from the attacker's own wallet, which only looks cheap in a test harness that gives every account a large default balance; in practice that path requires fronting nearly the entire loan amount and isn't a fee-sized attack at all. | **CONFIRMED by a passing simnet proof** (`tests/stx-pool-v2-deposit-reentrancy.test.ts`, receiver `contracts/test/test-pool-v2-receiver-deposit-reentrant.clar`), against a `contracts/test/flashstack-stx-pool-v2.clar` copy that is a faithful localization of the **live, funds-bearing** mainnet contract (`SPR9PQAN…`, 1.000000 STX, `paused=false` as of the CONTRACT_INVENTORY snapshot). Measured in one transaction, with the attacker fronting only fee-sized capital (50,000 µSTX — the real minimum that covers the 0.05% fee on a 99,000,000 µSTX / 99 STX loan; below that the self-deposit fails for insufficient balance) via the `as-contract` construction: the receiver contract ends up with 9,904,940,194,109,305 of 10,004,940,194,109,305 total shares (effectively all of them) worth 99,049,499 µSTX, a ~1,981x return on the attacker's 50,000 µSTX outlay (~2,001x at the exact minimum of 49,500 µSTX — one µSTX less and the self-deposit fails), with the attacker's own wallet spending nothing further once that buffer is in place; the sole honest LP's 100,000,000 µSTX deposit is left worth 1,000,000 µSTX — a 99% loss, extracted in a single transaction. (*Corrected 2026-10-01 — independently re-derived by Matt Glory, whose own repro used `as-contract` and caught that an earlier version of this PoC, without it, had credited shares to the attacker's EOA and silently drawn the deposit from the attacker's own wallet, overstating the return as ~9,905x on a 10,000 µSTX outlay. Re-verified a second time via a from-scratch hand derivation of the share-mint formula plus a fresh on-chain-equivalent read-only query, cross-checked against each other and against a boundary test (49,499 µSTX fails, 49,500 succeeds): this also caught that the share-count figures themselves (previously ending "…109,304 of …209,304") were off by JS `Number()` precision loss on a value exceeding `Number.MAX_SAFE_INTEGER` — corrected here to the exact on-chain uint values. The 99% LP-loss and 99,049,499 µSTX value figures were unaffected by either correction.*) `flashstack-sbtc-pool-v2` was read directly (not independently simnet-proven) and has an identical `deposit`/`flash-loan` structure with no lock — same vector, same exposure, 44,990 sats live. The cores (`flashstack-stx-core`, `flashstack-sbtc-core`) are **not** exposed: their equivalent of `deposit` (`deposit-reserve`) is `tx-sender == admin`-gated, which a receiver callback cannot satisfy since `tx-sender` is the original caller throughout the call chain, not the receiver contract or the admin. | **Open, but CONFIRMED DORMANT as of 2026-10-01** — independently re-verified against live chain state, not assumed: (1) `contracts/test/flashstack-stx-pool-v2.clar` (what the PoC ran against) diffs byte-for-byte against a fresh fetch of the deployed source at `SPR9PQAN…`, differing only in the trait address literal (the expected localization) — the PoC is against the real live contract, not a drifted copy; (2) both live pools read `paused=false`, `total-loans=0` right now via direct read-only calls; (3) the admin wallet's **complete** transaction history (24/24 txs, not a sample) contains **zero** `add-approved-receiver` calls against either `flashstack-stx-pool-v2` or `flashstack-sbtc-pool-v2` — every `add-approved-receiver` call it has ever made targets `flashstack-stx-core` instead. **No receiver has ever been whitelisted on either v2 pool, so the vector is unreachable today** — not because of any mitigation, but because nothing has opened the door yet. A control run (same harness, `test-pool-receiver-good` repaying by plain transfer) credits the caller zero shares, isolating the effect to the deposit-reentrancy specifically, not a harness artifact. **The real deadline is the first `add-approved-receiver` call on either pool** — nothing in the current review process would catch this before that call, since "repays via deposit" and "repays via transfer" both make `flash-loan` return `(ok true)`. **Zero-code mitigation available today: `set-paused true` on both live v2 pools blocks `flash-loan` entirely (confirmed in both contracts' source) — `deposit`/`withdraw` correctly unaffected — though with no receiver ever approved, pausing is precautionary rather than stopping an in-progress loss.** No fix is possible in place; these are immutable deployed contracts. A real fix mirrors pv3-F1's per-asset(-equivalent) lock and would need a v3-equivalent successor for these two pools, the same posture already adopted for BC1. Found by the Security & Contract Lead, 2026-10-01, while writing the threat model (`Flashstack-ajv.1.5`). Tracked in `Flashstack-ajv.4.8`. | --- diff --git a/tests/stx-pool-v2-deposit-reentrancy.test.ts b/tests/stx-pool-v2-deposit-reentrancy.test.ts index 5714faa..e3265d4 100644 --- a/tests/stx-pool-v2-deposit-reentrancy.test.ts +++ b/tests/stx-pool-v2-deposit-reentrancy.test.ts @@ -23,24 +23,31 @@ const RX = "test-pool-v2-receiver-deposit-reentrant"; const LP_DEP = 100_000_000; // 100 STX, the only honest LP const LOAN_AMOUNT = 99_000_000; // 99 STX -- nearly all of the reserve -const RX_BUFFER = 10_000; // attacker's own capital: covers the fee only +// Attacker's own capital: must cover the loan's fee (0.05% of LOAN_AMOUNT = +// 49,500 microSTX) so the receiver's own balance (loan + buffer) is enough +// to self-deposit amount+fee. Anything less and the `as-contract` deposit +// fails with ERR-REPAY-FAILED (insufficient balance) instead of succeeding +// cheaply -- there is no way around fronting at least the real fee. +const RX_BUFFER = 50_000; describe("flashstack-stx-pool-v2: reentrant deposit-as-repayment", () => { - let deployer: string, attacker: string, honestLp: string; + let deployer: string, attacker: string, honestLp: string, rxPrincipal: string; const shares = (who: string) => Number(simnet.callReadOnlyFn(POOL, "get-shares", [Cl.principal(who)], deployer).result.value); const stxValue = (who: string) => Number(simnet.callReadOnlyFn(POOL, "get-stx-value", [Cl.principal(who)], deployer).result.value); + const stxBalance = (who: string) => BigInt(simnet.getAssetsMap().get("STX")!.get(who) ?? 0); beforeEach(() => { deployer = simnet.getAccounts().get("deployer")!; attacker = simnet.getAccounts().get("wallet_1")!; honestLp = simnet.getAccounts().get("wallet_2")!; + rxPrincipal = `${deployer}.${RX}`; simnet.callPublicFn(POOL, "deposit", [Cl.uint(LP_DEP)], honestLp); - simnet.callPublicFn(POOL, "add-approved-receiver", [Cl.principal(`${deployer}.${RX}`)], deployer); - simnet.transferSTX(RX_BUFFER, `${deployer}.${RX}`, attacker); + simnet.callPublicFn(POOL, "add-approved-receiver", [Cl.principal(rxPrincipal)], deployer); + simnet.transferSTX(RX_BUFFER, rxPrincipal, attacker); }); it("the loan succeeds by depositing instead of repaying directly", () => { @@ -50,22 +57,30 @@ describe("flashstack-stx-pool-v2: reentrant deposit-as-repayment", () => { expect(result).toBeOk(Cl.bool(true)); }); - it("the attacker ends up owning a share of the pool worth more than the fee they actually paid", () => { + it("the receiver contract ends up owning a share of the pool, funded by the loan itself -- not the attacker's wallet", () => { + const attackerBalanceBefore = stxBalance(attacker); + simnet.callPublicFn( POOL, "flash-loan", [Cl.uint(LOAN_AMOUNT), Cl.contractPrincipal(deployer, RX)], attacker, ); - const feePaid = RX_BUFFER; // everything else the receiver deposited was the borrowed principal, not new capital + const attackerBalanceAfter = stxBalance(attacker); + const attackerOwnCapitalSpent = attackerBalanceBefore - attackerBalanceAfter; + + const rxShares = shares(rxPrincipal); + const rxValue = stxValue(rxPrincipal); const attackerShares = shares(attacker); - const attackerValue = stxValue(attacker); const honestLpValue = stxValue(honestLp); - // If this is a real vector: attacker holds shares (credited to tx-sender, - // not the receiver contract), worth far more than the ~fee they put in, - // and the honest LP's share of the now-larger pool is diluted below their - // original deposit. - expect(attackerShares).toBeGreaterThan(0); - expect(attackerValue).toBeGreaterThan(feePaid * 10); + // With `as-contract`, the minted shares land on the receiver CONTRACT + // (funded out of the loan it already holds), not the attacker's EOA. + // The attacker's entire out-of-pocket cost for the whole attack is the + // fee-sized RX_BUFFER pre-funded into the receiver in beforeEach -- the + // flash-loan call itself costs the attacker nothing further. + expect(attackerShares).toBe(0); + expect(rxShares).toBeGreaterThan(0); + expect(attackerOwnCapitalSpent).toBe(0n); + expect(rxValue).toBeGreaterThan(RX_BUFFER); expect(honestLpValue).toBeLessThan(LP_DEP); }); }); From df13fbd85bbe90a378f7b9b427434ddcfedae063 Mon Sep 17 00:00:00 2001 From: Hillaryhardy Date: Thu, 1 Oct 2026 05:39:52 +0300 Subject: [PATCH 4/4] fix: register F-9 PoC receiver in mainnet-plan-guard's known test paths 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. --- tests/mainnet-plan-guard.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/mainnet-plan-guard.test.ts b/tests/mainnet-plan-guard.test.ts index efbeae2..a064506 100644 --- a/tests/mainnet-plan-guard.test.ts +++ b/tests/mainnet-plan-guard.test.ts @@ -54,6 +54,7 @@ const KNOWN_TEST_PATHS = [ "contracts/test/mock-usdcx.clar", "contracts/test/sip-010-trait-ft-standard.clar", "contracts/test/test-pool-receiver-good.clar", + "contracts/test/test-pool-v2-receiver-deposit-reentrant.clar", "contracts/test/test-pool-v3-receiver-bad.clar", "contracts/test/test-pool-v3-receiver-good.clar", "contracts/test/test-pool-v3-receiver-reentrant.clar",