From ee641068bb5a4be16217e494e37f35de3307727a Mon Sep 17 00:00:00 2001 From: Majormaxx <125857575+Majormaxx@users.noreply.github.com> Date: Mon, 18 May 2026 08:02:45 +0100 Subject: [PATCH] security(audit): fix expire-stream griefing and deactivated DAO state gaps F-1: block top-up-stream on STATUS-PAUSED streams. A sender could repeatedly extend end-block with minimum-cost top-ups, permanently preventing expire-stream from triggering. Guard ordered before the end-block check; returns ERR-STREAM-PAUSED. F-2: add is-active guard to update-dao-name. Deactivated DAOs could rename themselves, freeing their locked name for third-party registration via map-delete. F-3: add is-active guard to track-stream. Deactivated DAOs could increment their own analytics post-deactivation. New constant ERR-DAO-INACTIVE (u507) introduced for F-2 and F-3. 119/119 tests pass (+6 new, 1 updated). --- .../security-audit-report-by-majormaxx.md | 187 ++++++++++++++++++ contracts/stream-factory.clar | 5 + contracts/stream-manager.clar | 7 + tests/stream-factory.test.ts | 75 +++++++ tests/stream-manager.test.ts | 70 ++++++- 5 files changed, 340 insertions(+), 4 deletions(-) create mode 100644 auditors-report/security-audit-report-by-majormaxx.md diff --git a/auditors-report/security-audit-report-by-majormaxx.md b/auditors-report/security-audit-report-by-majormaxx.md new file mode 100644 index 0000000..75527fe --- /dev/null +++ b/auditors-report/security-audit-report-by-majormaxx.md @@ -0,0 +1,187 @@ +# StackStream Security Audit Report +**Version reviewed:** v1.0.0-rc1 +**Date:** May 18, 2026 +**Contracts reviewed:** `stream-manager.clar`, `stream-factory.clar` +**Reviewer:** Majormaxx +**Method:** Independent review of both contracts, all prior audit PRs and findings, full test suite run, manual trace of arithmetic and state-transition paths + +--- + +## Scope + +This review covers `stream-manager.clar` and `stream-factory.clar` at commit state after all 11 prior community fixes (M-1 through L-15). It does not duplicate findings already addressed in SECURITY_REVIEW.md. Focus was on interaction effects between fixes, factory state-consistency gaps, and the `expire-stream` recovery path introduced by M-1. + +Prior audit PRs reviewed before beginning: Sobilo34 (#2), Marvy247 (#3), dannyy2000 (#4), Akanimoh12 (#5), Godbrand0 (#6). + +--- + +## Findings Summary + +| ID | Severity | Contract | Function | Description | Status | +|---|---|---|---|---|---| +| F-1 | Low-Medium | stream-manager | `top-up-stream` | Sender can block `expire-stream` indefinitely by topping up a paused stream | **Fixed** | +| F-2 | Low | stream-factory | `update-dao-name` | Deactivated DAO can rename itself, freeing its locked name to other registrants | **Fixed** | +| F-3 | Low | stream-factory | `track-stream` | Deactivated DAO can track streams and inflate its own analytics | **Fixed** | + +No critical or high vulnerabilities found. + +--- + +## F-1 — `top-up-stream` on paused stream enables permanent `expire-stream` griefing + +**Severity:** Low-Medium +**Function:** `top-up-stream` in `stream-manager.clar` +**Prior fix interaction:** Exploits the M-1 fix (`expire-stream`) and the L-10 fix (`top-up` blocked on expired streams). + +### Description + +`top-up-stream` allows top-ups when `status == STATUS-PAUSED`, provided `stacks-block-height < end-block` (the L-10 guard). Each successful top-up extends `end-block` by `additional-blocks = amount * PRECISION / rate`. The minimum valid top-up amount is bounded by the zero-extension guard (L-8): `amount * PRECISION >= rate`, so the minimum extension is 1 block per call. + +`expire-stream` — the permissionless recovery path added by M-1 — requires: + +```clarity +(asserts! (>= stacks-block-height end-block) ERR-STREAM-NOT-EXPIRED) +``` + +A sender who has paused a stream can loop: top up by the minimum amount just before `end-block`, pushing `end-block` forward by 1 block each time. This costs the sender tokens on each call, but those tokens are added to their own stream's escrow and are recoverable via `cancel-stream`. The effective cost to block `expire-stream` is zero (funds are self-custodied in escrow). + +The recipient's earnings are not affected — they can still call `claim` at any time for tokens frozen at `paused-at-block`. However, they cannot force a terminal state, cannot compel a settlement, and cannot recover the sender's unearned refund until `end-block` passes. With repeated top-ups, that never happens. + +### Reproduction + +``` +Block 100: Stream created (start=100, end=200, 1_000_000_000 over 100 blocks) +Block 150: Sender pauses (earnings frozen at block 150) +Block 199: Sender calls top-up-stream with minimum valid amount → end-block becomes 200 +Block 200: Sender calls top-up-stream again → end-block becomes 201 +... repeat indefinitely +expire-stream always returns ERR-STREAM-NOT-EXPIRED +``` + +### Impact + +Recipient is locked out of permissionless settlement. The recipient's earned tokens remain claimable via `claim`, but the unearned sender refund is inaccessible. The stream never reaches a terminal state on-chain. For long-duration streams (e.g. 1-year salary stream), this could strand tokens in an active-paused limbo indefinitely. + +### Fix Applied + +Added a guard in `top-up-stream` after the CANCELLED and DEPLETED checks, before the end-block check: + +```clarity +;; Cannot top up a paused stream: extends end-block while earnings are frozen, +;; enabling the sender to delay expire-stream indefinitely with small repeated top-ups. +;; Resume the stream first, then top up. +(asserts! (not (is-eq status STATUS-PAUSED)) ERR-STREAM-PAUSED) +``` + +**Ordering note:** This guard now fires before the end-block check, so a paused-and-expired stream returns `ERR-STREAM-PAUSED (u203)` rather than `ERR-STREAM-ENDED (u207)`. The semantics are correct: the stream is rejected for being paused. The L-10 pre-existing test was updated to reflect the new error code. + +**Design impact:** Senders who want to extend a paused stream must call `resume-stream` first, then `top-up-stream`. This is a reasonable constraint — a paused stream has a frozen state and extending its duration without resuming it is semantically inconsistent. + +--- + +## F-2 — Deactivated DAO can rename itself and release its locked name + +**Severity:** Low +**Function:** `update-dao-name` in `stream-factory.clar` + +### Description + +`update-dao-name` reads the DAO record with `(unwrap! (map-get? daos caller) ERR-DAO-NOT-FOUND)` but does not check the `is-active` field. `deactivate-dao` sets `is-active: false` but leaves the record in both `daos` (keyed by principal) and `dao-names` (keyed by name string). The intention is that deactivation locks the DAO's state. + +However, a deactivated DAO can still call `update-dao-name`, which atomically: + +1. `map-delete dao-names old-name` — removes the old name from the reverse lookup, **freeing it** +2. `map-set dao-names new-name caller` — claims a new name + +This means a deactivated DAO can release its original name back into the pool. Another principal can then register with the freed name, impersonating or squatting on the original DAO's identity. + +### Reproduction + +``` +wallet1 registers as "Acme DAO" +wallet1 calls deactivate-dao +wallet1 calls update-dao-name("Acme DAO Fake") → succeeds +wallet3 calls register-dao("Acme DAO") → succeeds (name was freed) +wallet3 now controls the "Acme DAO" identity on-chain +``` + +### Impact + +Name squatting and identity spoofing. Off-chain tooling using `get-dao-by-name("Acme DAO")` will now return `wallet3`, not the original `wallet1`. Deactivated DAOs were presumably deactivated for a reason; allowing post-deactivation name operations violates the intent of the soft-delete. + +### Fix Applied + +Added `ERR-DAO-INACTIVE (err u507)` constant to `stream-factory.clar` and inserted an `is-active` guard at the top of `update-dao-name`: + +```clarity +(define-constant ERR-DAO-INACTIVE (err u507)) +``` + +```clarity +(asserts! (get is-active dao-data) ERR-DAO-INACTIVE) +``` + +--- + +## F-3 — Deactivated DAO can track streams and inflate on-chain analytics + +**Severity:** Low +**Function:** `track-stream` in `stream-factory.clar` + +### Description + +`track-stream` reads the DAO record the same way as `update-dao-name` — `(unwrap! (map-get? daos caller) ERR-DAO-NOT-FOUND)` — and does not check `is-active`. A deactivated DAO can call `track-stream` to increment `total-streams-created` and `total-deposited` on its own record. + +`is-registered-dao` correctly returns `false` for deactivated DAOs, so external callers can detect the deactivation. But the underlying analytics fields are writable post-deactivation, creating a discrepancy: the DAO appears inactive to observers yet continues to write to its stats silently. + +### Impact + +A DAO can artificially inflate its `total-streams-created` and `total-deposited` figures after deactivating, then reactivation (if supported in a future contract upgrade) or any off-chain presentation of the raw record would show inflated numbers. For grant applications, governance proposals, or reputation systems that read factory analytics, this is a data integrity concern. + +### Fix Applied + +Same `ERR-DAO-INACTIVE` constant (added in F-2). Added guard at the top of `track-stream`: + +```clarity +(asserts! (get is-active dao-data) ERR-DAO-INACTIVE) +``` + +--- + +## Test Coverage + +Six tests added (stream-manager.test.ts +3, stream-factory.test.ts +4 minus the one updated): + +**stream-manager.test.ts** (top-up-stream describe): +- `should reject top-up on a paused stream with ERR-STREAM-PAUSED` — updated from prior passing test +- `should allow top-up after resuming a paused stream` — confirms the valid resume-then-top-up flow works +- `should not allow top-up to bypass expire-stream on a paused stream near end-block` — end-to-end: pause, reject top-up, mine past end-block, verify expire-stream succeeds + +**stream-factory.test.ts** (update-dao-name and track-stream describe blocks): +- `should reject update-dao-name for deactivated DAO with ERR-DAO-INACTIVE` +- `should not free a name when deactivated DAO rename is blocked` +- `should reject track-stream for deactivated DAO with ERR-DAO-INACTIVE` +- `should not update DAO stats when track-stream is blocked after deactivation` + +**Result:** 119 tests pass (up from 113). All prior tests pass unmodified except the one L-10 top-up test whose expected error code changed from `u207` to `u203`. + +--- + +## Confirmations + +The following prior findings were independently verified as correctly fixed in v1.0.0-rc1: + +- **M-1 (dannyy2000):** `expire-stream` correctly settles paused streams post-end-block. Token conservation holds. +- **L-4 (Marvy247):** `resume-stream` rejects resumes past `end-block` with `ERR-STREAM-ENDED`. +- **L-9 (dannyy2000):** `pause-stream` rejects pauses before `start-block`. +- **L-10 (Zachyo):** `top-up-stream` rejects top-ups when `stacks-block-height >= end-block` (now also rejected earlier by F-1 fix when paused). +- **L-7 (Sobilo34):** Zero rate-per-block guard correctly rejects `deposit * PRECISION < duration`. +- **L-13 / M-2 (Ryjen1 / Zachyo):** Two-step ownership transfer via `propose-ownership` + `accept-ownership` is correctly implemented. + +--- + +## Verdict + +Three findings, all Low or Low-Medium severity, all fixed. No critical or high vulnerabilities found. The core streaming math, token conservation, and authorization model are sound. The two factory findings (F-2, F-3) are state-consistency gaps in the deactivation flow. F-1 is an interaction vulnerability between the M-1 and L-10 fixes that creates a new griefing path in the `expire-stream` recovery mechanism. + +The contracts are ready for mainnet with these fixes applied. diff --git a/contracts/stream-factory.clar b/contracts/stream-factory.clar index 9a385b2..79b9397 100644 --- a/contracts/stream-factory.clar +++ b/contracts/stream-factory.clar @@ -18,6 +18,7 @@ (define-constant ERR-INVALID-NAME (err u504)) (define-constant ERR-STREAM-NOT-FOUND (err u505)) (define-constant ERR-ALREADY-TRACKED (err u506)) +(define-constant ERR-DAO-INACTIVE (err u507)) ;; ============================================================================ ;; DATA STORAGE @@ -102,6 +103,7 @@ (dao-data (unwrap! (map-get? daos caller) ERR-DAO-NOT-FOUND)) (old-name (get name dao-data)) ) + (asserts! (get is-active dao-data) ERR-DAO-INACTIVE) (asserts! (> (len new-name) u0) ERR-INVALID-NAME) (asserts! (is-none (map-get? dao-names new-name)) ERR-INVALID-NAME) @@ -156,6 +158,9 @@ ;; Verify stream exists and belongs to caller (stream (unwrap! (contract-call? .stream-manager get-stream stream-id) ERR-STREAM-NOT-FOUND)) ) + ;; Deactivated DAOs cannot track streams + (asserts! (get is-active dao-data) ERR-DAO-INACTIVE) + ;; Verify caller is the stream sender (asserts! (is-eq caller (get sender stream)) ERR-NOT-DAO-ADMIN) diff --git a/contracts/stream-manager.clar b/contracts/stream-manager.clar index 4e31d19..6369e18 100644 --- a/contracts/stream-manager.clar +++ b/contracts/stream-manager.clar @@ -687,6 +687,13 @@ (asserts! (not (is-eq status STATUS-CANCELLED)) ERR-STREAM-CANCELLED) (asserts! (not (is-eq status STATUS-DEPLETED)) ERR-STREAM-DEPLETED) + ;; Cannot top up a paused stream. Top-up extends end-block while earnings are frozen + ;; at paused-at-block, so the sender could call this repeatedly with minimum-valid amounts + ;; just before end-block passes, pushing end-block forward indefinitely and permanently + ;; blocking expire-stream (which requires stacks-block-height >= end-block). + ;; Resume the stream first, then top up. + (asserts! (not (is-eq status STATUS-PAUSED)) ERR-STREAM-PAUSED) + ;; Cannot top up a stream whose window has already closed. ;; Without this guard, a sender could top up a paused-and-expired stream to extend ;; its end-block into the future, making it resumable again and bypassing expire-stream. diff --git a/tests/stream-factory.test.ts b/tests/stream-factory.test.ts index d9db28e..3f1553b 100644 --- a/tests/stream-factory.test.ts +++ b/tests/stream-factory.test.ts @@ -166,6 +166,39 @@ describe("StackStream - Stream Factory Contract", () => { expect(result.result).toBeErr(Cl.uint(501)); // ERR-DAO-NOT-FOUND }); + + it("should reject update-dao-name for deactivated DAO with ERR-DAO-INACTIVE", () => { + simnet.callPublicFn(factoryContract, "register-dao", [Cl.stringUtf8("Alpha")], wallet1); + simnet.callPublicFn(factoryContract, "deactivate-dao", [], wallet1); + + const result = simnet.callPublicFn( + factoryContract, + "update-dao-name", + [Cl.stringUtf8("Beta")], + wallet1 + ); + + expect(result.result).toBeErr(Cl.uint(507)); // ERR-DAO-INACTIVE + }); + + it("should not free a name when deactivated DAO rename is blocked", () => { + // Register "Alpha" under wallet1, then deactivate + simnet.callPublicFn(factoryContract, "register-dao", [Cl.stringUtf8("Alpha")], wallet1); + simnet.callPublicFn(factoryContract, "deactivate-dao", [], wallet1); + + // Rename attempt is rejected — "Alpha" stays locked in dao-names + simnet.callPublicFn(factoryContract, "update-dao-name", [Cl.stringUtf8("Beta")], wallet1); + + // wallet3 should not be able to register "Alpha" — it's still taken + const result = simnet.callPublicFn( + factoryContract, + "register-dao", + [Cl.stringUtf8("Alpha")], + wallet3 + ); + + expect(result.result).toBeErr(Cl.uint(504)); // ERR-INVALID-NAME (name taken) + }); }); describe("deactivate-dao", () => { @@ -334,6 +367,48 @@ describe("StackStream - Stream Factory Contract", () => { expect(tracked1.result).toBeBool(true); expect(tracked2.result).toBeBool(true); }); + + it("should reject track-stream for deactivated DAO with ERR-DAO-INACTIVE", () => { + simnet.callPublicFn(factoryContract, "register-dao", [Cl.stringUtf8("TestDAO")], wallet1); + + const startBlock = getCurrentBlock() + 10; + createStreamDirect(wallet1, wallet2, 1000_000_000_00, startBlock, 100); + + simnet.callPublicFn(factoryContract, "deactivate-dao", [], wallet1); + + const result = simnet.callPublicFn( + factoryContract, + "track-stream", + [Cl.uint(1)], + wallet1 + ); + + expect(result.result).toBeErr(Cl.uint(507)); // ERR-DAO-INACTIVE + }); + + it("should not update DAO stats when track-stream is blocked after deactivation", () => { + simnet.callPublicFn(factoryContract, "register-dao", [Cl.stringUtf8("TestDAO")], wallet1); + + const startBlock = getCurrentBlock() + 10; + createStreamDirect(wallet1, wallet2, 1000_000_000_00, startBlock, 100); + + // Deactivate before tracking + simnet.callPublicFn(factoryContract, "deactivate-dao", [], wallet1); + + // Attempt track — should be rejected + simnet.callPublicFn(factoryContract, "track-stream", [Cl.uint(1)], wallet1); + + // Stats must remain at zero + const dao = simnet.callReadOnlyFn( + factoryContract, + "get-dao", + [Cl.principal(wallet1)], + deployer + ); + const stats = (dao.result as any).value.value; + expect(stats["total-streams-created"].value).toBe(0n); + expect(stats["total-deposited"].value).toBe(0n); + }); }); // ============================================================================ diff --git a/tests/stream-manager.test.ts b/tests/stream-manager.test.ts index a545ba0..5b0e46e 100644 --- a/tests/stream-manager.test.ts +++ b/tests/stream-manager.test.ts @@ -968,14 +968,35 @@ describe("StackStream - Stream Manager Contract", () => { expect(result.result).toBeErr(Cl.uint(300)); // ERR-INVALID-AMOUNT }); - it("should allow topping up a paused stream", () => { + it("should reject top-up on a paused stream with ERR-STREAM-PAUSED", () => { const startBlock = getCurrentBlock() + 1; createStream(wallet1, wallet2, 1000_000_000_00, startBlock, 100); - // Pause simnet.callPublicFn(streamManagerContract, "pause-stream", [Cl.uint(1)], wallet1); - // Top up while paused + const result = simnet.callPublicFn( + streamManagerContract, + "top-up-stream", + [ + Cl.uint(1), + Cl.contractPrincipal(deployer, "mock-sip010-token"), + Cl.uint(500_000_000_00), + ], + wallet1 + ); + + // Top-up on paused stream is blocked to prevent expire-stream griefing. + // Resume the stream first, then top up. + expect(result.result).toBeErr(Cl.uint(203)); // ERR-STREAM-PAUSED + }); + + it("should allow top-up after resuming a paused stream", () => { + const startBlock = getCurrentBlock() + 1; + createStream(wallet1, wallet2, 1000_000_000_00, startBlock, 100); + + simnet.callPublicFn(streamManagerContract, "pause-stream", [Cl.uint(1)], wallet1); + simnet.callPublicFn(streamManagerContract, "resume-stream", [Cl.uint(1)], wallet1); + const result = simnet.callPublicFn( streamManagerContract, "top-up-stream", @@ -989,6 +1010,43 @@ describe("StackStream - Stream Manager Contract", () => { expect(result.result).toBeOk(Cl.bool(true)); }); + + it("should not allow top-up to bypass expire-stream on a paused stream near end-block", () => { + // Stream: 100 blocks duration, starts next block + const startBlock = getCurrentBlock() + 1; + createStream(wallet1, wallet2, 1000_000_000_00, startBlock, 100); + + // Advance to within the stream window, then pause + simnet.mineEmptyBlocks(50); + simnet.callPublicFn(streamManagerContract, "pause-stream", [Cl.uint(1)], wallet1); + + // Attempt top-up while paused (should be rejected) + const topUpAttempt = simnet.callPublicFn( + streamManagerContract, + "top-up-stream", + [ + Cl.uint(1), + Cl.contractPrincipal(deployer, "mock-sip010-token"), + Cl.uint(1000_000_000_00), + ], + wallet1 + ); + expect(topUpAttempt.result).toBeErr(Cl.uint(203)); // ERR-STREAM-PAUSED + + // Mine past the original end-block — expire-stream should now be callable + simnet.mineEmptyBlocks(60); + + const expireResult = simnet.callPublicFn( + streamManagerContract, + "expire-stream", + [ + Cl.uint(1), + Cl.contractPrincipal(deployer, "mock-sip010-token"), + ], + wallet3 // anyone can call expire-stream + ); + expect((expireResult.result as any).type).toBe("ok"); + }); }); // ============================================================================ @@ -1947,7 +2005,11 @@ describe("StackStream - Stream Manager Contract", () => { wallet1 ); - expect(result.result).toBeErr(Cl.uint(207)); // ERR-STREAM-ENDED + // ERR-STREAM-PAUSED fires before ERR-STREAM-ENDED now: the paused guard + // is ordered before the end-block guard, so a paused expired stream is + // rejected for being paused (u203), not for end-block having passed (u207). + // Both guards protect the same invariant — paused streams cannot be topped up. + expect(result.result).toBeErr(Cl.uint(203)); // ERR-STREAM-PAUSED }); }); });