diff --git a/docs/concord-soft-ban-audit.md b/docs/concord-soft-ban-audit.md index 239258ff40..8f284085ff 100644 --- a/docs/concord-soft-ban-audit.md +++ b/docs/concord-soft-ban-audit.md @@ -26,6 +26,9 @@ test. | [V7](#v7) | Banlist rank rule diverges from Armada | Medium | — | — | | [V8](#v8) | A soft ban revokes no read access and no live invite | Medium | Yes | Yes (Refounding) | | [V9](#v9) | The base-rekey plane is writable by every member | Low | Yes | Yes | +| [V10](#v10) | Stranded recovery: either broken, or a removal bypass | **Critical** | Yes | — | +| [V11](#v11) | Voice rooms are key-gated, not roster-gated | High | Yes | Yes (Refounding) | +| [V12](#v12) | Typing indicators are not ban-filtered | Low | Yes | Yes | The two structural causes worth naming up front, because most of the list collapses into them: @@ -216,8 +219,86 @@ valid wraps there. Authorization happens after the blobs are scanned, so a flood a locator scan per blob on every revision tick. Bounded work per wrap and no correctness impact; listed for completeness. +## V10 — Stranded recovery: either broken, or a removal bypass + +**Critical, and it forks — one of the two halves is true and both are bad.** +*Read:* `ConcordStrandedRecovery`, `AccountConcordActions.recoverStrandedConcordCommunities`, +`AccountConcordActions.mintConcordInvite`. + +`ConcordStrandedRecovery.isStranded` / `mergeForward` take only `(entry, bundle)`. There is **no +banlist check and no check that we were legitimately re-keyed** — the entire test is "the bundle at +my stored `inviteRef` sits at a higher epoch than I do". The unlock token lives in the link +fragment, which an ex-member keeps forever. So whether a removed member walks back in with the new +root depends *only* on whether the bundle at that coordinate ever advances an epoch. + +In Amethyst it never does: `mintConcordInvite` mints a **fresh link signer per mint**, so nothing +re-publishes at an existing coordinate, and `refoundConcordCommunity` does not re-mint or revoke +anything. Two consequences, and they are the fork: + +- **If nothing re-mints** — today's behaviour — then stranded recovery never fires *for anyone*. + That makes it dead code, and the cure `drainConcordRekeys`' own KDoc points to for "a BAN-holder + can evict anyone (the owner included) by omission" does not exist. An owner evicted by a rogue + admin has no way back. +- **If anything re-mints at a stable coordinate** — which is what CORD-05's design describes ("the + community keeps publishing its bundle at that same addressable coordinate, re-minted at the + current epoch"), so plausibly Armada in a cross-client community — then every removed member who + joined through a still-live link auto-recovers the new root on the 15-minute sweep, and + re-announces a Guestbook join so they look current again. **Refounding, the only hard removal, + is silently undone.** + +Note also that `refoundConcordCommunity` never revokes the invite links the removed member created +or joined through, even though `ControlEntityKind.INVITE_REVOKED` exists and `classifyInvite` +already honors it. + +**Fix direction.** Decide the intended semantics first — this needs a spec answer, not a patch. +Then: gate `mergeForward` on not being banned in the epoch we are merging *from*, have the +Refounding revoke the removed members' links, and either implement re-minting (so legitimate +recovery works) or drop the mechanism and give evicted owners a different route. + +## V11 — Voice rooms are key-gated, not roster-gated + +**High.** *Read:* `ConcordBrokerToken`, CORD-07 §2. + +A member proves voice-room membership by signing a NIP-98 kind-27235 request with the channel's +**derived voice signer key**, whose pubkey is the SFU room name. The broker is stateless and holds +no community secret, so it cannot consult the Control Plane and has no idea a banlist exists. A +banned member keeps that key until a Refounding, so they can join the voice room and stay in it. +Nothing on the client side can evict them — kicking them from the UI does not kick them from the SFU. + +This is the one place where a ban fails *audibly*, in real time, in front of everyone. Worth ranking +above its technical severity for that reason alone. + +## V12 — Typing indicators are not ban-filtered + +**Low.** *Read:* `ConcordCommunitySession.ingestTyping`. + +`ingestTyping` checks the rumor is a typing heartbeat, is bound to the channel/epoch, and is not our +own — and nothing else. A banned member (or any fresh npub holding the channel key, see V5) shows +in the "… is typing" row indefinitely. Cheap to fix and user-visible: the promise a ban makes is +that the member disappears, and here they do not. + --- +## What was NOT examined + +This audit is bounded by what was opened. Checked and found sound: the wrap/seal envelope (no author +impersonation — `rumor.pubKey == seal.pubKey` and `rumor.verifyId()`), Concord chat edits +(`Note.latestConcordEdit` is author-gated, so a member cannot rewrite someone else's message), and +self-unban (V2). + +Not looked at at all: + +- **Private channels** (CORD-03 derived keys) — key delivery on grant, and channel-scoped rekey. + Note that no channel-scoped rekey *receive* path appears to exist: `drainConcordRekeys` handles + `ROOT_SCOPE` only, and `entry.privateChannels` is carried forward but never populated by a + delivery path. If that is right, the only removal Amethyst can perform is a full-community + Refounding — which is exactly what V4 makes expensive. +- **In-plane reactions and deletes** — the edit path is author-gated; the delete path was not read. +- **Guestbook kicks** (kind 3309) — the builder documents a KICK-bit + rank rule; the receive side + was not verified against it. +- Unread counts and notification triggers, media/upload references from messages, the NIP-53 nests + overlap, and the desktop client's Concord paths. + ## Suggested order 1. **V4** — cheapest, not consensus-affecting, and it protects the remedy every other fix depends on.