Skip to content

fix(#1182): guardian and recovery-key paths share one recovery slot - #1224

Closed
Anambraboi-1 wants to merge 1 commit into
Miracle656:mainfrom
Anambraboi-1:fix/issue-1182-one-recovery-at-a-time
Closed

Anambraboi-1 wants to merge 1 commit into
Miracle656:mainfrom
Anambraboi-1:fix/issue-1182-one-recovery-at-a-time

Conversation

@Anambraboi-1

Copy link
Copy Markdown
Contributor

Closes #1182.

Decision: keep two paths, give them one slot

The wallet keeps both recovery systems. They answer different "who vouches for me" questions, and each has a client built on it:

Guardian Recovery key
Entrypoints set_guardian → initiate_recovery → complete_recovery set_recovery_key → request_recovery → finalize_recovery
Timelock 3 days 7 days
Who vouches A party the owner trusts (another account, a friend, a custodian) SEP-30 recovery servers that re-verify the owner's identities
Used by Web docs' key-rotation flow Mobile app/recover.tsx

Removing either one would break whichever client is built on it. Nobody chose for them to run blind to each other, though, so they now share one recovery slot. The reasoning is written in the module doc at the top of contracts/invisible_wallet/src/recovery.rs. The two delays are unchanged, because changing them is a separate decision.

What changed (contract)

  • No race. The only code that writes RecoveryPending or RecoveryKeyPending is recovery::claim_slot, and it refuses while either key is set. So at most one recovery can be pending.

  • Distinguishable errors. There are two new codes, and each names the system holding the slot:

    • GuardianRecoveryPending = 28
    • RecoveryKeyRequestPending = 29

    Starting a recovery in either path returns whichever one applies. complete_recovery / finalize_recovery called while the other system holds the slot also return it, instead of RecoveryNotPending. RecoveryNotPending (14) now means nothing is pending in either system. RecoveryAlreadyPending (13) is no longer returned, but it stays in the enum so clients can still decode errors from older wallets.

  • Unambiguous cancel. cancel_recovery and cancel_recovery_request both clear whatever is pending in either system. Both storage keys are removed, so after either succeeds nothing is counting down. The cancel event now carries the RecoverySource it cancelled.

  • Visibility. The new view get_pending_recovery() -> Option<PendingRecoveryStatus> returns the source, the new key and the unlock time.

  • Signers are still replaced, deliberately. Both paths go through recovery::install_recovered_signer, which keeps the reset and explains why. Recovery is only needed when the owner can't authorise with any registered signer; an owner with one working signer would cancel or rotate_signer instead. That means every existing signer is lost or compromised. Merging would let a stolen device keep a seat on the recovered wallet, and would make recovery unable to evict exactly the key it most needs to evict.

  • What recovery does not touch (documented, not changed): the guardian, the recovery key, allowances, spend limits and session keys. Session keys sit in temporary storage keyed by id and can't be enumerated, so they survive recovery until their own expiry or until the recovered signer revokes them. (The issue text says session keys are wiped; on the current code they are not.) Making recovery invalidate them would need an epoch counter on the session-key ACL, which touches the signing path. I left that for a separate issue.

Tests

contracts/invisible_wallet/src/recovery_slot_test.rs adds 12 tests on a wallet with three signers, a guardian and a recovery key:

  • The race, both orders. Initiate both, then finalise each. guardian_first_blocks_recovery_key_and_wins_alone and recovery_key_first_blocks_guardian_and_wins_alone: the second start is refused with the other system's code, finalising through the wrong entrypoint names the holder, and only the first system's key is ever installed, even after both delays have passed.
  • Cross-cancel. Each cancel function cancels the other system's recovery, and the signers are untouched.
  • Signers replaced on both paths, including recovery to a key that was already a signer.
  • Bounded exhaustive check. no_sequence_of_operations_leaves_two_recoveries_pending runs every sequence of three operations (512 sequences) drawn from {initiate, request, complete, finalize, cancel, cancel_request, wait 3d, wait 7d}. After every step it reads raw storage and asserts that the two pending keys are never both set, that get_pending_recovery agrees with storage, and that the signer set is either untouched or exactly one of the two proposed keys.

None of the existing tests were changed or weakened.

cargo test for the wallet: 102 passed locally before I rebased onto current main. After the rebase the only conflict was two mod lines in lib.rs, but I haven't re-run the suite locally on the rebased commit, so CI is the check for that. Mobile jest lib/__tests__/recovery.test.ts: 25 passed, including a new case for codes 28/29.

Clients and docs

  • frontend/mobile/lib/recovery.ts translates 28 and 29 into user-facing messages. 13, 14 and 15 are kept for older wallets.
  • frontend/docs/pages/sdk-errors.mdx gets corrected rows for 13/14 and new rows for 28/29.
  • frontend/docs/pages/security/key-rotation.mdx gets a "One recovery at a time" section explaining the signer reset, plus guidance for owners of existing wallets.

Wallets that already hold money

Deployed wallets keep the old behaviour. There is no upgrade path; only wallets created by a factory initialised with the new WASM hash get this. Owners of existing wallets should be told:

  1. Both recovery paths can be pending on your wallet at the same time.
  2. If you're alerted to a recovery you didn't start, call both cancel_recovery and cancel_recovery_request. A RecoveryNotPending from one of them is harmless; it only means that path was idle.
  3. On your wallet, RecoveryAlreadyPending / RecoveryNotPending don't tell you which path they refer to.
  4. Either recovery, if it completes, replaces all of your signers with the single recovered key. Additional devices have to be re-added afterwards.

Funds are not at risk from upgrading, because nothing migrates. An owner who wants the new behaviour has to create a new wallet from the new factory and move funds across.

contracts/expected-hashes.json

Not updated yet. I don't have Docker locally, so I can't produce the canonical hash. Once CI's reproducible-build job prints the new invisible_wallet.wasm hash, I'll push a follow-up commit to this PR updating that one line. The other artifacts should be unchanged, because the factory takes the wallet hash as an init argument rather than embedding it. Please don't merge before that commit lands.

…ery slot

The wallet keeps both recovery systems: the guardian (3-day timelock) and the
recovery key (7-day, used by mobile's SEP-30 flow). They now share one
pending-recovery slot instead of running blind to each other:

- initiate_recovery / request_recovery refuse while either system has a
  recovery pending, with GuardianRecoveryPending (28) or
  RecoveryKeyRequestPending (29) naming the system that holds the slot.
- complete_recovery / finalize_recovery called for the wrong system report
  which system holds the slot rather than RecoveryNotPending.
- cancel_recovery and cancel_recovery_request both clear whatever is pending,
  so after either succeeds nothing is counting down.
- get_pending_recovery() reports the pending system, key and unlock time.

Recovery still replaces every signer, now deliberately and documented in
recovery::install_recovered_signer: recovery is only needed when no
registered signer can authorise, so every existing signer is lost or
compromised, and merging would let a stolen device survive recovery.

Wallets already deployed keep the old behaviour; docs tell their owners to
call both cancel functions.
@vercel

vercel Bot commented Oct 21, 2026

Copy link
Copy Markdown

@Anambraboi-1 is attempting to deploy a commit to the miracle656's projects Team on Vercel.

A member of the Team first needs to authorize it.

@Anambraboi-1

Copy link
Copy Markdown
Contributor Author

Opened from the wrong account by mistake; reopening from the correct fork.

@Anambraboi-1
Anambraboi-1 deleted the fix/issue-1182-one-recovery-at-a-time branch October 21, 2026 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A wallet has two recovery systems and they cannot see each other

1 participant