Skip to content

docs: audit scope packet for prospective auditors; fix stale audit brief - #65

Open
mattglory wants to merge 5 commits into
mainfrom
audit-scope-packet
Open

mattglory wants to merge 5 commits into
mainfrom
audit-scope-packet

Conversation

@mattglory

Copy link
Copy Markdown
Owner

Summary

Adds docs/security/AUDIT_SCOPE.md, an external-facing scoping packet for a prospective Clarity auditor, and fixes stale statements in docs/AUDIT_BRIEF.md and the README that I found while writing it. Docs only.

The packet

Scope in two tiers: Tier 1 flashstack-pool-v3 + its trait (505 lines), Tier 2 the four BC1 successors (+1,096), quoted separately. It states the language versions, the properties the design declares load-bearing, evidence to date, and an explicit "what is not done" section: no professional audit yet, pool-v3 not testnet-staged, and the canonical file is not what clarinet check compiles (D6).

Every figure was checked on 2026-09-21:

  • Deployment status of all six contracts, on both FlashStack mainnet principals and the testnet deployer. Only flashstack-stx-core-v2 exists anywhere (testnet).
  • Epoch 4.0 live on mainnet since burn height 960,230 (GET /v2/pox).
  • pool-v3's canonical file vs its test copy: differ only in a 4-line header and two use-trait lines.
  • Test counts: 221 across 20 files; 29 in the two pool-v3 files.
  • Branch protection read from the GitHub API.

Drift found and fixed

  • AUDIT_BRIEF.md named two audit firms and cited ROADMAP.md for it. The ROADMAP names neither. The firm isn't chosen; corrected.
  • Items 1-3 described the findings register as missing, CI gating as unrestored, and branch protection as unconfirmed. All three are done; re-verified, including that enforce_admins is off.
  • "the 38 contracts checked above" referred to nothing; replaced with the caveat that the clarinet check total isn't a coverage metric.
  • README said 176 tests in two places; the suite is 221.

For the reviewer

Please check the Tier 1 / Tier 2 split is what you'd send. Whether Tier 2 belongs in the first quote is a judgement call I made for the reader's benefit, not a fact.

🤖 Generated with Claude Code

Adds docs/security/AUDIT_SCOPE.md, an external-facing packet: scope in two
tiers (pool-v3 + trait = 505 lines; the four BC1 successors add 1,096),
language versions, the properties the design declares load-bearing,
evidence to date, and an explicit "what is not done" section (no
professional audit yet; pool-v3 not testnet-staged; canonical file not what
clarinet check compiles). Every figure was checked against the repo or the
live chain on 2026-09-21, including deployment status of all six contracts
on both mainnet principals and testnet, epoch 4.0 being live since burn
height 960,230, and the canonical-vs-copy diff for pool-v3.

Fixes drift found while writing it:
- AUDIT_BRIEF named two audit firms and cited ROADMAP.md for it; the
  ROADMAP names neither. The firm is not chosen. Corrected.
- AUDIT_BRIEF items 1-3 described the findings register as missing, CI
  gating as unrestored, and branch protection as unconfirmed. All three are
  done; re-verified and updated, including that enforce_admins is off.
- AUDIT_BRIEF cited "the 38 contracts checked above"; replaced with the
  caveat that the clarinet-check total is not a coverage metric.
- README said 176 tests in two places; the suite is 221 across 20 files.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Sep 21, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
web Ready Ready Preview Sep 21, 2026 11:36am UTC

Request Review

mattglory and others added 2 commits September 21, 2026 09:42
Found while checking #65 before review:

- Reviewed-state hash was 737c659; main has since taken #62 and #63. None
  touched contracts/, tests/ or Clarinet.toml, but the hash is now e8e2497,
  with clarinet check (0 errors) and the suite (221/221) re-run on that
  tree after merging main into the branch.
- "testnet 404" was ambiguous; it is 404 at the staging deployer.
- Clarity 6 claim: epoch 4.0 being live was verified, but not that Clarity 6
  became deployable at that epoch. Confirmed against the stacks-core 4.0.0
  release notes and now cited. Added, after verifying it against the full
  tx history of both deployer principals (latest burn heights 960,197 and
  960,216, below 960,230), that no FlashStack contract has been deployed on
  Clarity 6 yet -- context an auditor will want about the toolchain.

Verified: every relative link resolves and is tracked in git; design-doc
sections 7 and 13 exist under the titles cited.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@unixwhisperer unixwhisperer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked every figure independently rather than reading past them — the packet is external-facing, so I ran the claims rather than trusting them. Almost all of it holds up, and the "what is not done" section is the best thing in it. Two concrete defects to fix before it goes out, then the split question you actually asked.

The Tier 1 / Tier 2 split — yes, I'd send it, with one reframe

Tier 1 is right. flashstack-pool-v3 + its trait is the only contract targeting Clarity 6 / epoch 4.0, it's the generic funds-bearing surface, it carries all three internal findings, and it's pre-TVL. 505 lines is a coherent quotable unit, and the trait belongs with it — it's the callback interface an attacker implements.

Tier 2 is undersold, in a way that could cost you. Calling it "the four BC1 successors" frames it as optional extras. But two of the four are pools, and I checked: flashstack-stx-pool-v3 and flashstack-sbtc-pool-v3 both carry VIRTUAL-SHARES — the same first-depositor inflation defence the packet lists under "where we would most want scrutiny" as a Tier 1 property. The share-math surface spans both tiers.

So an auditor who quotes Tier 1 alone, finds a share-math bug in pool-v3, and hands it back has no obligation to tell you whether it reproduces in the two v3 pools. I'd say that outright in the packet:

Tier 2 includes two pools that share Tier 1's virtual-shares and pause design. A Tier 1 finding may or may not reproduce there, and we would want that stated either way.

That turns Tier 2 from "more contracts" into "the blast radius of a Tier 1 finding" — a much stronger reason for a firm to quote both, and a stronger reason for you to buy both.

Minor: Tier 2 mixes two designs — cores (reserve model, no LP shares, 454 lines) and pools (LP shares, 642 lines). A one-line sub-split lets a firm price them separately.

Two things to fix before this goes out

1. The §6a citation claims more than §6a does. Evidence to date says flashstack-stx-core-v2 was staged "including the two-step-admin happy path and the negative case, verified against the live testnet API (§6a, §6b)". But §6a's own staleness note says that evidence predates a92fb8e, used the propose-to-self sequence with no read-only assertions, and that "a re-run against the current script is required before this document can be treated as current BC1 evidence, not optional". So the packet cites as evidence a section that describes itself as insufficient — to an auditor, exactly the wrong first impression.

§6b is rigorous and stands as written. Suggest narrowing to: negative case proven on chain with a second key (§6b); happy-path transactions succeeded and the end state is correct, but the intermediate assertions were only added afterwards and are pending a re-run (§6a). Fix it here rather than waiting — #66 gives you --steps=admin but the run still needs your key, and the packet shouldn't block on that.

2. The README badge still says 176. README.md:6Tests-176%20Passing. You fixed both prose mentions; the badge is the most-read number in the repo, and D2's own remediation row names "README (badge, Security bullet, Quick start)" as the three places. One line.

(FLASH_LOAN_INVARIANT.md:147 also says 176, but it's explicitly dated "as of 2026-09-16", so it's a historical statement and fine.)

One asymmetry worth closing

AUDIT_BRIEF.md (internal) now says branch protection requires code-owner review and both checks, and that enforce_admins is off so an admin can bypass. AUDIT_SCOPE.md (external) states only the first half. For a packet describing controls to an auditor, the bypass is the material half, and stating it reads as candour — which is what the "what is not done" section is already buying you.

I can't verify the protection values myself: GET /branches/main/protection still 404s for my token (admin-only, ajv.2.5). I'm taking your read on the values and commenting only on which half appears where.

One inconsistency the auditor will find anyway

flashstack-sbtc-core-v2 names its proposal function set-admin; the other four use transfer-admin. All five use accept-admin.

flashstack-pool-v3        transfer-admin accept-admin
flashstack-stx-pool-v3    transfer-admin accept-admin
flashstack-sbtc-pool-v3   transfer-admin accept-admin
flashstack-stx-core-v2    transfer-admin accept-admin
flashstack-sbtc-core-v2   set-admin      accept-admin

The scrutiny list says "two-step admin transfer" as if it's one uniform thing. All five are undeployed, so unifying is free now; if you'd rather not, say it in the packet, because an ops runbook written against one name fails against the other.

Verified, no action needed

  • Line counts: 483 / 22 / 230 / 224 / 306 / 336. Tier 1 = 505, Tier 2 = 1,096. Both add up.
  • Clarity/epoch per contract from Clarinet.toml: pool-v3 6 / 4.0, the other four 3 / 3.0. Matches the table.
  • Epoch 4.0 at burn height 960,230GET /v2/pox, Epoch40 start_height 960230.
  • "Neither deployer has transacted since epoch 4.0 activated" — true, and closer than you may realise: last tx for SP20XD46… is burn height 960,197, for SPR9PQAN… 960,216. Fourteen and thirty-three blocks under the line. Worth keeping the sentence, since it dates fast.
  • All five Tier 1/2 contracts 404 at both mainnet principals; flashstack-pool-v3 404 at the testnet staging deployer, flashstack-stx-core-v2 200 there.
  • pool-v3 canonical vs contracts/test/ copy: a 4-line header comment and two use-trait lines. Exactly as described.
  • 29 tests across flashstack-pool-v3.test.ts + pool-v3-hillary-review.test.ts; 221 across 20 files overall.
  • CI: clarinet check then npm test, on push and PR to main; jobs named Test Smart Contracts and Build Frontend, matching the check names in the brief.
  • Three receivers excluded from Clarinet.toml, each with a comment saying clarinet check fails and why. The internal brief's description is accurate.

Heads-up on one number: #66 takes the suite to 230 across 21 files. Whichever of these merges second should carry the update.

Requesting changes for the §6a wording and the badge only — everything else above is a suggestion or a note.

mattglory and others added 2 commits September 21, 2026 12:33
…pools)

Hillary's requested changes, all verified before applying:

- Narrowed the §6a citation. §6a's own staleness note says its run predates
  the intermediate assertions; the packet now cites §6b for the negative
  case (proven with a second key) and says §6a's happy path succeeded but is
  superseded pending a re-run.
- README badge said 176 (README.md:6); now 221. ROADMAP.md said 176 in two
  places (D2 lists ROADMAP x2); also 221.
- Tier 2 reframed as the blast radius of a Tier 1 finding, with her
  suggested sub-split: 2a cores (454 lines, no LP shares) and 2b pools (642).
- Stated the enforce_admins-off bypass in the external packet, matching the
  internal brief.
- Stated the set-admin (sbtc-core-v2) vs transfer-admin naming difference.

Verifying her Tier 2 claim ("share Tier 1's pause design") turned up a real
finding: the two v3 pools' deposit checks only amount > 0 while flash-loan is
pause-gated -- pv3-F3's shape, fixed in pool-v3 only. The v3 pools are the v2
pool sources plus the two-step admin change (14 differing lines each), and I
read the DEPLOYED mainnet source of both v2 pools: same gap, so it is on live,
immutable contracts. Added as F-8 (Low proposed, for the Security Lead to
confirm) and corrected F-6, which said migrating to v2 was the fix. Also
stated in the packet that the two v3 pools are directly tested only for the
two-step admin transfer.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mattglory

Copy link
Copy Markdown
Owner Author

Pushed 71f96a3. Both defects fixed, your suggestions taken, and checking one of them turned up a real finding.

Your two defects

  1. §6a citation narrowed as you suggested: negative case proven with a second key (§6b); §6a's happy path succeeded but predates the intermediate assertions and is superseded pending the re-run (§6c).
  2. README badge 176 → 221. ROADMAP.md said 176 in two places (D2 lists ROADMAP ×2), fixed too.

Your suggestions: Tier 2 reframed as the blast radius of a Tier 1 finding, using your wording, with the 2a cores (454) / 2b pools (642) sub-split. The enforce_admins bypass is now in the external packet. The set-admin vs transfer-admin difference is stated; all five are undeployed, so unifying is still free if you'd rather.

New: F-8. I checked "the pools share Tier 1's pause design" before writing it, and it isn't true. flashstack-stx-pool-v3 and flashstack-sbtc-pool-v3 deposit asserts only amount > 0; flash-loan is gated. That is pv3-F3's shape, fixed only in pool-v3. The v3 pools are the v2 pool sources plus the two-step admin (14 differing lines each, all admin), so I read the deployed mainnet source of both v2 pools: same gap. It is on live, immutable contracts. I filed it as F-8 (Low proposed, matching F-6; severity and ID are your call) and corrected F-6, whose mitigation said migrating to v2 was the fix. The packet also now says the two v3 pools are tested directly only for the two-step admin.

Question for you: fix the two v3 pools before the audit? They're undeployed, so it's (asserts! (not (var-get paused)) ERR-PAUSED) in deposit, a test per pool, and the contracts/test/ copies. I'd say yes, as a separate PR from you.

Suite count: #66 takes it to 230/21, so whichever merges second should carry the update to the README, ROADMAP and packet.

@mattglory

Copy link
Copy Markdown
Owner Author

Taking the F-8 fix for the two v3 pools myself, so please don't start it in parallel. Plan: (asserts! (not (var-get paused)) ERR-PAUSED) in deposit for both, the contracts/test/ copies in the same change, a test per pool (deposit blocked when paused, withdraw NOT blocked, unpause restores deposit), and the F-8 / packet updates. It will be a separate PR for your review, since contracts are yours. Shout if you already have it in progress.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants