coop/docs/nip4e-audit.md

NIP-4e Audit Report

Update: The P0 findings (C1, C2, C3) are now fixed — seals are always signed by the identity key, extract_rumor falls back to the device key, the DEVICE_GIFTWRAP subscription is created, and trashed wraps are retried on DeviceEvent::Set. The remaining findings (F1–F8, P2 items, UX bugs) are still open.

Scope: crates/nip4e/src/lib.rs (key announcement/sync), the send path (crates/chat/src/room.rs), the receive path (crates/chat/src/lib.rs), UI integration (crates/workspace), compared against the NIP-4e draft and PsstPsst's implementation (docs/protocols/nip4e.md + docs/protocols/nip17-extensions.md, plus rust-nostr's NIP-59 source to verify crypto mechanics).

Executive summary

The key-sync side (kinds 10044/4454/4455) is a reasonable sketch of the spec, but the actual message encryption is inverted and non-functional end-to-end:

• Sending seals/signs with the device encryption key (should be the identity key).
• Receiving unwraps gift wraps with the identity key (should be the device key when the wrap is addressed to your encryption key E).

Every NIP-4e message between two Coop clients fails at decryption; messages to/from PsstPsst fail in both directions. On top of that there are ~10 sync/UX bugs that make even key distribution unreliable.

1. Critical — the message path is broken (P0)

rust-nostr's GiftWrapBuilder::finalize_async(signer) (verified against upstream source):

• Seal content = nip44_encrypt(conversation_key(signer_sk, receiver_pk), rumor)
• Seal is signed by signer → seal.pubkey = signer_pk
• Gift wrap = nip44_encrypt(conversation_key(ephemeral_sk, receiver_pk), seal)
• Unwrap requires the receiver's secret matching receiver_pk; then rust-nostr rejects if rumor.pubkey != seal.pubkey ("sender mismatch").

NIP-4e draft says the sender keeps using their identity key (ecdh(b, E) — Bob's b) and only the target changes to E. So:

#	Finding	Location
C1	Send path signs the seal with the device key. select_signer returns the encryption signer, and GiftWrapBuilder::finalize_async(device_signer) signs the seal with E while rumor.pubkey = identity (Room::rumor uses current_user). Result: (a) receivers can't attribute the sender, (b) Coop's own rumor.pubkey != seal.pubkey check rejects it, (c) PsstPsst verifies the seal against the sender's identity key → invalid.	crates/chat/src/room.rs:482-495, 590-596, 643-646; rumor pubkey at room.rs:404-417
C2	Receive path unwraps with the identity key. handle_notifications captures the user signer and passes it to extract_rumor → nip44_decrypt(ecdh(a, wrap_pubkey)). A 4e wrap is encrypted to your E, so decryption always fails → "Failed to extract rumor" → trash. The correct unwrap key for 4e messages is the device key (ecdh(e, wrap_pubkey)), then ecdh(e, seal.pubkey) for the rumor — both succeed with the device signer in rust-nostr's unwrapper.	crates/chat/src/lib.rs:184, 252, 743-801
C3	The DEVICE_GIFTWRAP subscription is never created. The constant exists (state/src/constants.rs:24), its ID is used for EOSE matching (chat/src/lib.rs:190), but no code ever subscribe()s with it (presumably intended: #p = E filter on inbox relays). Only USER_GIFTWRAP (#p = A) exists.	crates/chat/src/lib.rs:190-191, 410-418

Consequences:

Path	Status
Coop → Coop (4e)	❌ Fails at receiver wrap-decryption (C2); even with a device-key unwrap it fails the sender-mismatch check (C1)
Coop → PsstPsst	❌ PsstPsst unwraps fine (wrap → E), but the seal signature doesn't verify against your identity key and there's no seal n tag → rejected
PsstPsst → Coop	❌ Wrap routed via identity p tag arrives, but Coop unwraps with the identity key → fail (C2)
Coop → self backup copy	❌ Same broken envelope, so cross-device history sync also fails
Non-4e (SignerKind::User)	✅ Works — the bug only bites when 4e is enabled, which is why it looks "half-baked" rather than fully broken

Note on interop targets: the draft wants seal signed by identity, content = ecdh(identity_sk, receiver_E) — which is exactly GiftWrapBuilder::finalize_async(user_signer) with receiver = E. PsstPsst's profile goes further: seal signed by identity plus an n tag with the sender's E, content encrypted with ecdh(sender_e, receiver_E). So minimal correctness (Coop↔Coop) = swap the signers; PsstPsst interop additionally requires implementing their split-key envelope (n tag, sender-e content key).

sequenceDiagram
    participant S as Sender (Coop)
    participant R as Receiver (Coop)
    Note over S: TODAY (broken)<br/>seal signed by device key Es<br/>wrap → Er, unwrap with identity a
    S->>R: gift wrap (→ Er)
    R->>R: nip44_decrypt(a, wrap) ✗ FAILS
    Note over S: CORRECT (draft)<br/>seal signed by identity As<br/>wrap → Er, unwrap with device e
    S->>R: gift wrap (→ Er)
    R->>R: nip44_decrypt(e, wrap) ✓<br/>nip44_decrypt(e, seal.pubkey) ✓

2. Key-sync bugs (P1)

#	Finding	Location
F1	No p-tag filter on 4455. wait_for_approval subscribes to all your 4455s (author=me, since=now). extract_encryption then tries to decrypt transfers meant for other devices → NIP-44 failure → DeviceEvent::Error → error notification. The approving device also picks up its own just-published 4455 and errors on it. Spec/PsstPsst: filter by p = own client key and ignore undecryptable responses.	nip4e/src/lib.rs:476-482, 163-165, 489-526
F2	No validation of the received key. After decrypting a 4455, the key is installed without comparing derived_pubkey to the current 10044 n value. A stale 4455 (pre-rotation) silently installs a dead key. PsstPsst: "accepts the key only when that public key equals the current kind 10044 n value".	nip4e/src/lib.rs:489-526
F3	since(Timestamp::now()) on both subscriptions. The approver misses all 4454s published while it was offline (the draft says Cope "looks for all 4454 events" — i.e. backfill). The requester likewise misses 4455s published while it was offline; recovery depends on LMDB having synced them. This is the main "works only when both apps are open at the same time" behavior.	nip4e/src/lib.rs:388-394, 476-482
F4	pending_request is never reset to false. After the first approval in a session, every subsequent device request is silently dropped (ask_for_approval early-returns).	nip4e/src/lib.rs:597-601 (no set_pending_request(false) anywhere)
F5	No deletion after success. The draft explicitly says the requester deletes its 4454 + the received 4455 (NIP-09). Coop leaves them on relays forever, and combined with F6 below, stale events get re-applied on later launches.	absent
F6	request() short-circuits on any old 4455 in the local DB (limit(1), no recency check, no decrypt-check before committing to it) — stale after rotation.	nip4e/src/lib.rs:414-422
F7	4454/4455 tag deviations vs PsstPsst profile: no n tag (which key is requested) or relay hints on 4454; no identity p tag on 4455; P tag in 4455 is the device key E instead of a freshly generated client key (c per the draft). The last one is mostly cosmetic (recipients read P either way) but links every transfer to your announcement.	nip4e/src/lib.rs:426-432, 559-565
F8	Self-request race. set_encryption races the 10044 echo against the local set_keys write: if the announcement you just published comes back before the DB save completes, get_keys fails → the master publishes a 4454 asking itself, then shows "You've requested an encryption key from another device" about its own profile.	nip4e/src/lib.rs:342-375 vs 335

3. Robustness / state management (P2)

• announcement_existed never resets and the notification loop resolves current_user once at startup (nip4e/src/lib.rs:144) — after an account switch, 10044/4454/4455 filtering uses the stale pubkey, and a new account without an announcement never gets NotSet.
• NotSet after a fixed 5s timer, not EOSE — slow relays produce false "no key" prompts (nip4e/src/lib.rs:269-283).
• get_announcement runs on every settings change (including theme changes) — each call pushes 2 more tasks into self.tasks (never drained, grows all session) and re-creates the wait_for_request subscription on every Set (nip4e/src/lib.rs:100-116, 295).
• Publish-before-save: create_encryption publishes 10044 then saves locally; if set_keys fails, the key exists only in memory while 10044 advertises it → permanent unlockable requests (nip4e/src/lib.rs:317-338).
• Unbounded processed_events HashSet in the 4e notification loop (chat's version prunes at 10k; this one doesn't).

4. Security considerations

• One-click approval with no out-of-band check. Anyone who can get a 4454 signed by your identity (i.e., any of your devices — fine) is approved by client-name alone. PsstPsst requires comparing an 8-hex-char authorization code derived from the requester's client key. Given this event transfers your master encryption key, an approval-code comparison is worth copying.
• backup() writes the bech32 secret key to a plaintext file (nip4e/src/lib.rs:222-240). Consider at least warning in the UI.
• Announcement::from falls back to val.pubkey when the n tag is missing/malformed (state/src/nip4e.rs:14-23) → a malformed 10044 makes deliver() believe 4e is available and produce hybrid messages signed with the device key but wrapped to the identity — worst of both worlds. PsstPsst treats a missing n as "not a 4e peer".
• Plaintext-encrypted device key sits in kind-30078 local DB events — local-only, acceptable.

5. UI/UX bugs

#	Finding	Location
U1	The "Retry" button is a key-rotation footgun. On NotSet, the notification's action calls set_announcement(Keys::generate()). If the NotSet was a false negative (F-timer) and an announcement does exist, clicking "Retry" silently rotates your encryption key, breaking decryption on all other devices until each re-requests. The label says retry; the code creates a new key.	workspace/src/lib.rs:130-140
U2	Inverted empty-check in restore: if !content.is_empty() { set_error("cannot be empty") } — errors when the input is not empty, then falls through and reports "invalid" for empty input.	workspace/src/dialogs/restore.rs:54-56
U3	Restore/reset republish a 10044 with a new key without warning that other devices will need to re-request (reset dialog warns about message loss — good — but restore doesn't warn at all).	workspace/src/lib.rs:466-473
U4	Every spurious decrypt failure from F1 surfaces as a persistent error notification.	workspace/src/lib.rs:152-154

6. What's already correct

• 10044 construction matches the draft (n tag, replaceable kind, extra client tag is harmless).
• 4454 tags (P, client) match the draft; request/response events are correctly signed by the identity key, so requests are authenticated.
• Client keys are generated per install and stored in the OS keyring — reasonable.
• Local key escrow (kind 30078, NIP-44 self-encrypted, d = "coop:device", never published) is a sound design; the IDENTIFIER value surviving the crate rename is fine (changing it would orphan stored keys).
• Dedup of relay echoes, opt-in nip4e: false default, and Auto mode degrading to the identity path when no announcement exists (this fallback is why normal DMs still work).

7. Recommended fix roadmap (ordered)

1. P0 — fix the envelope (C1+C2+C3), Coop↔Coop:
   • Send: always sign the seal with the identity signer (GiftWrapBuilder::finalize_async(user_signer) with receiver = announcement E). The device key is not needed to send.
   • Receive: create the DEVICE_GIFTWRAP subscription (#p = E) on the user's inbox relays; in extract_rumor, unwrap with the device signer for wraps addressed to E (fall back to identity for wraps addressed to A); re-process trash when DeviceEvent::Set arrives.
   • Keep rumor.pubkey = identity — the sender-mismatch check then passes by construction.
2. P0.5 — decide the interop target: if PsstPsst interop matters, adopt their split-key envelope (seal n tag = sender E, seal content encrypted with ecdh(sender_e, receiver_E), seal signed by identity). rust-nostr's builder can't express that split today — you'd build the seal manually.
3. P1 — sync hardening: add p-tag filters, validate decrypted key against the current 10044 n, replace since(now) with a startup backfill query, reset pending_request, delete 4454/4455 after success, ignore (don't error on) transfers for other devices, and save locally before publishing.
4. P2 — UX: fix the Retry button to actually retry (get_announcement) with a separate explicit "Create key" action; fix the inverted restore check; add an approval code comparison before sharing the key.
5. P3 — state hygiene: reset announcement_existed + re-resolve current_user on signer change, drive NotSet off EOSE, bound the task vec/dedup set.

Validation status: audit only, no code changed; the workspace compiled cleanly at the last cargo check. The rust-nostr NIP-59 behavior was verified against upstream master source (the workspace pins nostr-sdk to that git repo; the seal-content/signing behavior has been stable across recent versions).