security(audit): expire-stream griefing via paused top-up + deactivated DAO state gaps [F-1, F-2, F-3]#7
Open
Majormaxx wants to merge 1 commit into
Conversation
… 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).
|
@Majormaxx is attempting to deploy a commit to the dev_jaytee's projects Team on Vercel. A member of the Team first needs to authorize it. |
jayteemoney
added a commit
that referenced
this pull request
May 19, 2026
H-1 (Akanimoh, PR #5): stream-factory had no reactivate-dao - deactivated DAOs were permanently locked out of the registry and their name burned in dao-names forever. Added reactivate-dao public function with ERR-DAO-ALREADY-ACTIVE (err u508). F-1 (Majormaxx, PR #7): top-up-stream on a paused stream allowed sender to extend end-block indefinitely with minimum-valid top-ups, permanently griefing expire-stream. Added paused-state guard before the end-block check. F-2 (Majormaxx, PR #7): update-dao-name had no is-active check, letting a deactivated DAO release its locked name via map-delete and enabling name squatting. Added ERR-DAO-INACTIVE (err u507) and is-active guard. F-3 (Majormaxx, PR #7): track-stream had no is-active check, letting a deactivated DAO inflate its own analytics post-deactivation. Same ERR-DAO-INACTIVE guard. Error-code numbering: ERR-DAO-INACTIVE=u507 (Majormaxx, 2 call sites), ERR-DAO-ALREADY-ACTIVE=u508 (renumbered from u507 to avoid collision). Test suite: 113 -> 125 passing tests (+12 covering all four fixes plus updated L-10 test for new error ordering).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Independent Security Audit — v1.0.0-rc1
Reviewer: Majormaxx
Date: May 18, 2026
Scope:
stream-manager.clar,stream-factory.clarMethod: Full independent review of both contracts. All prior audit PRs (#2–#6) and SECURITY_REVIEW.md reviewed before beginning to avoid duplication. Focused on fix-interaction effects, factory state-consistency, and the
expire-streamrecovery path.Test run:
npm test— 119/119 passing (113 original + 6 new)Findings
F-1 —
top-up-streamon paused stream enables permanentexpire-streamgriefingSeverity: Low-Medium | Function:
top-up-stream| Lines: 686–693 (stream-manager.clar)Root cause. The M-1 fix (dannyy2000) added
expire-streamas a permissionless recovery path for paused streams whoseend-blockhas passed. The L-10 fix (Zachyo) blocked top-ups on streams wherestacks-block-height >= end-block. Neither fix covered the case where both conditions are approaching simultaneously: a sender who has paused a live stream can calltop-up-streamrepeatedly just beforeend-blockarrives, each call extendingend-blockby⌊amount × PRECISION / rate⌋blocks.expire-streamrequires:With
end-blockperpetually ahead, this condition never satisfies.Cost to attacker. The L-8 zero-extension guard enforces a minimum top-up of
⌈rate / PRECISION⌉tokens per call. However, every topped-up token enters the sender's own escrow and is fully recoverable viacancel-stream. The net out-of-pocket cost to indefinitely blockexpire-streamis zero.Recipient impact. The recipient retains full access to earned tokens via
claim(frozen atpaused-at-block). What they lose is the ability to force a terminal state and recover the sender's unearned refund portion via the permissionlessexpire-streampath.Fix. One assert added in
top-up-streamafter the CANCELLED/DEPLETED state guards:This guard is ordered before the end-block check (
ERR-STREAM-ENDED), so both live-paused and expired-paused streams returnERR-STREAM-PAUSED (u203). The pre-existing L-10 regression test expectedu207for the expired-paused case and has been updated tou203with an explanatory comment. Senders who legitimately want to extend a stream while it is paused must callresume-streamfirst — this is a reasonable constraint, as topping up a frozen stream is semantically inconsistent.F-2 — Deactivated DAO can rename itself, freeing its locked name for third-party registration
Severity: Low | Function:
update-dao-name| Lines: 99–124 (stream-factory.clar)Root cause.
register-daowrites to two maps atomically:daos(principal → record) anddao-names(name string → principal).deactivate-daosetsis-active: falseon thedaosrecord but does not touchdao-names. The intent is a soft-delete: the name stays reserved, the slot stays consumed.update-dao-namereads thedaosrecord via(unwrap! (map-get? daos caller) ERR-DAO-NOT-FOUND)but does not assertis-active. A deactivated DAO can therefore call it, triggering:The
map-deletemakes the original name available. Any other principal can then callregister-dao("Acme DAO")and take it.Reproduction path:
Fix. New constant
ERR-DAO-INACTIVE (err u507)plus anis-activeguard at the top ofupdate-dao-name:F-3 — Deactivated DAO can call
track-streamand inflate on-chain analyticsSeverity: Low | Function:
track-stream| Lines: 152–183 (stream-factory.clar)Root cause. Structurally identical to F-2.
track-streamreads(unwrap! (map-get? daos caller) ERR-DAO-NOT-FOUND)without checkingis-active. A deactivated DAO can calltrack-streamto incrementtotal-streams-createdandtotal-depositedon its own record.is-registered-daoreturnsfalsefor deactivated DAOs, so external callers correctly see the DAO as inactive. However, the underlying analytics fields remain writable, creating a discrepancy: the DAO appears inactive to observers yet can inflate its on-chain track record — relevant for any governance system, grant program, or reputation layer that reads raw factory data.Fix. Same
ERR-DAO-INACTIVEguard applied totrack-stream:Changes
contracts/stream-manager.clarSTATUS-PAUSEDguard intop-up-streamcontracts/stream-factory.clarERR-DAO-INACTIVE (u507)constant +is-activeguards inupdate-dao-nameandtrack-streamtests/stream-manager.test.tstests/stream-factory.test.tsupdate-dao-nameandtrack-streamdescribe blocksauditors-report/security-audit-report-by-majormaxx.mdTest Coverage
stream-manager.test.ts — top-up-stream describe block:
should reject top-up on a paused stream with ERR-STREAM-PAUSED— direct rejectionshould allow top-up after resuming a paused stream— valid resume-then-top-up path preservedshould not allow top-up to bypass expire-stream on a paused stream near end-block— end-to-end: create stream → mine 50 blocks → pause → assert top-up rejected → mine past end-block → assert expire-stream succeedsstream-factory.test.ts — update-dao-name and track-stream describe blocks:
should reject update-dao-name for deactivated DAO with ERR-DAO-INACTIVEshould not free a name when deactivated DAO rename is blocked— asserts third party cannot register the original nameshould reject track-stream for deactivated DAO with ERR-DAO-INACTIVEshould not update DAO stats when track-stream is blocked after deactivation— assertstotal-streams-createdandtotal-depositedremain at zeroConfirmations
The following prior findings were independently verified as correctly implemented in v1.0.0-rc1:
deposit * PRECISION >= durationcorrectly enforcedamount * PRECISION >= ratecorrectly enforcedcurrent-block >= start-blockcorrectly assertedstacks-block-height < end-blockcorrectly asserted (now preceded by F-1 guard)Verdict
Three findings — one Low-Medium, two Low. All fixed. No critical or high vulnerabilities found. Core streaming math, token conservation invariants, and the
contract-callerauthorization model are sound. F-1 closes the last known interaction gap between the M-1 and L-10 fixes. F-2 and F-3 complete thedeactivate-daostate-lock that the factory's soft-delete pattern implied but did not enforce.The contracts are ready for mainnet with these fixes applied.