design: settle upgradeability and pausing — non-upgradeable, create_stream-only pause - #37
Merged
Merged
Conversation
…tream-only pause Answers #33, which is architecture.md open questions 4 and 5 and threat-model T1 (the highest-severity entry in the model) and T5. Three decisions that turned out to be one: 1. Non-upgradeable. No upgrade function, so no key to compromise or be coerced into using. Only a contract can replace its own Wasm, so an absent function is permanent immutability rather than a policy needing enforcement. 2. create_stream is the only pausable entry point. withdraw, cancel, approve_milestone and TTL extension never are, so a pause cannot reach a stream that already exists. It auto-expires after 30 days and can be renounced. 3. cancel on a non-cancelable stream is permitted with the sender's AND the recipient's authorization. One auth rule, no new entry point, no new settlement math. The threat model's fallback recommendation — upgradeable behind a multisig and a timelock — is withdrawn as wrong, and the reason is arithmetic rather than principle. A timelock protects only what a user can withdraw during it, and a stream's defining property is that most of the money isn't withdrawable yet. Against a four-year vest, a 30-day timelock lets a perfectly attentive recipient rescue 2.1% if the attacker announces early — and the attacker picks when to announce. Gated tranches can't be rescued at all; a recipient inside a cliff rescues nothing. A timelock long enough to actually protect a stream must outlast the stream, which is non-upgradeability plus a key someone can be compelled to use. Decision 3 exists because of decision 1. Migration off an immutable contract means each stream unwinds and re-creates, and a non-cancelable stream could not unwind at all. That dead end is identical to T3's — approver vanishes, nobody can move the funds — and two unrelated causes producing one dead end was the signal that the dead end was the defect. cancelable=false now means the sender cannot cancel unilaterally, not that nobody can. The recipient's guarantee is unchanged; they gained an option requiring their own signature. Honest limit recorded rather than glossed: mutual cancel makes stranded funds movable without deciding who deserves them. Rule 4 still returns the unapproved tranche to the sender, so a recipient who did the work may reasonably refuse. T3 goes Unmitigated -> Partially mitigated, not Mitigated. The strongest case for pausing withdrawals is stronger than T5 originally allowed and is now recorded: the contract's token balance is pooled, so an accrual bug over-crediting one stream pays it out of another's deposit. It is answered by an invariant rather than a key — assert payout <= total - withdrawn per stream, one comparison on an entry already loaded. This is only sound because #32 made `total` the measured balance delta rather than the requested amount. Also noted in behaviour.md and SECURITY.md: the conservation invariant would NOT catch cross-stream drain, since it is a closure check across the whole contract and balances either way. Same lesson as the day-20 scenario in PR #26, in a new place. Two consequences found by tracing dependents rather than by looking for them: - ttl-strategy.md proposed storing tunable TTL thresholds "mutable by whatever admin/governance process the upgradeability question settles". That process is now none, so the thresholds must be derived from max_ttl() at call time — which that document already recommended as better practice anyway. - architecture.md's "there is no global admin over user funds" needed amending rather than deleting. A pauser role now exists; it has no power over funds in any existing stream. Precision beats a clean sentence that has quietly stopped being true. Prior art is cited where I could verify it and marked TODO(maintainer) where I could not — the web tooling was unavailable during this work, and a Sablier citation from memory is not good enough for a doc a grant application might lean on. Closes #33. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KZDKaiY2UJbvbZCKeqy4cg
3 tasks
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.
Closes #33 — architecture.md open questions 4 and 5, and threat-model T1 (the highest-severity entry in the model) and T5.
Three decisions, which turned out to be one
create_streamis the only pausable entry point.withdraw,cancel,approve_milestone, and TTL extension never are — a pause cannot reach a stream that already exists. It auto-expires after 30 days and can be renounced.cancelon a non-cancelable stream is permitted with the sender's and the recipient's authorization. One authorization rule. Not a new entry point, not a new settlement rule.The timelock recommendation was wrong, on arithmetic
T1 originally offered a second acceptable answer — upgradeable behind a multisig plus a timelock long enough for recipients to exit. That half is withdrawn. It borrows a vault pattern that doesn't survive contact with streaming: a timelock protects only what a user can withdraw during it, and a stream's defining property is that most of the money isn't withdrawable yet.
Four-year vest, attacker picks the announcement day, recipient reacts perfectly:
A timelock long enough to actually protect a stream has to outlast the stream — which is non-upgradeability plus a key someone can be compelled to use. Gated tranches can't be rescued at all, and a recipient inside a cliff rescues nothing.
Decision 3 exists because of decision 1
Migration off an immutable contract means each stream unwinds and re-creates. A non-cancelable stream couldn't unwind at all — and that dead end is identical to T3's (approver vanishes, nobody can move the funds). Two unrelated causes producing one dead end was the signal that the dead end was the defect.
cancelable = falsenow means the sender cannot cancel unilaterally, not nobody can cancel. The recipient's guarantee is unchanged — they gained an option that requires their own signature.Stated as a limit rather than a win: this makes stranded funds movable without deciding who deserves them. Rule 4 still returns the unapproved tranche to the sender, so a recipient who did the work is being asked to sign it away and may reasonably refuse. T3 moves Unmitigated → Partially mitigated, not Mitigated.
The best case for pausing withdrawals, and the invariant that answers it
T5's original sentence survives but was under-argued. The real counter-case: the contract's token balance is pooled, so an accrual bug over-crediting stream A pays it out of B's deposit — a race where fast recipients drain slow ones.
Answered by an invariant rather than a key: assert
payout <= total - withdrawnper stream, one comparison on an entry already loaded. This is only sound because #32 madetotalthe measured balance delta rather than the requested amount.Recorded in
behaviour.mdandSECURITY.md: the conservation invariant would not catch cross-stream drain — it's a closure check across the whole contract and balances either way. Same lesson as the day-20 scenario in #26, in a new place.Two consequences found by tracing dependents
ttl-strategy.mdproposed storing tunable TTL thresholds "mutable by whatever admin/governance process the upgradeability question settles." That process is now none — so thresholds derive frommax_ttl()at call time, which that document already recommended as better practice.architecture.md's "there is no global admin over user funds" needed amending rather than deleting. A pauser exists; it has no power over funds in any existing stream. Precision beats a clean sentence that has quietly stopped being true.Acceptance criteria
architecture.mdopen questions 4 and 5 replaced by the decisionsthreat-model.mdT1 and T5 updated; T1's second recommendation amended as wrongSECURITY.mdscope updated — a new class 6 for attacks on the limits themselvesOn prior art
Cited where I could verify it, marked
TODO(maintainer)where I couldn't. The web tooling was unavailable throughout this work, and a Sablier citation from memory isn't good enough for a document a grant application might lean on.Verification
markdownlint-cli2clean (17 files, 0 issues). All internal links and anchors resolved with a local checker — the only hit is the pre-existing GitHub-relative path inPULL_REQUEST_TEMPLATE.md, which resolves correctly on GitHub. Timelock table computed in Python, not estimated.Next in Track A: nothing — this was the last of #17 → #32 → #33. Tracks B and C on #34 remain unclaimed.