Repository navigation
fix(#1182): guardian and recovery-key paths share one recovery slot - #1224
Closed
Anambraboi-1 wants to merge 1 commit into
Closed
Anambraboi-1 wants to merge 1 commit into
Anambraboi-1 wants to merge 1 commit into
Conversation
…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.
|
@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. |
Contributor
Author
|
Opened from the wrong account by mistake; reopening from the correct fork. |
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 #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:
set_guardian→initiate_recovery→complete_recoveryset_recovery_key→request_recovery→finalize_recoveryapp/recover.tsxRemoving 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
RecoveryPendingorRecoveryKeyPendingisrecovery::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 = 28RecoveryKeyRequestPending = 29Starting a recovery in either path returns whichever one applies.
complete_recovery/finalize_recoverycalled while the other system holds the slot also return it, instead ofRecoveryNotPending.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_recoveryandcancel_recovery_requestboth 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 theRecoverySourceit 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 orrotate_signerinstead. 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.rsadds 12 tests on a wallet with three signers, a guardian and a recovery key:guardian_first_blocks_recovery_key_and_wins_aloneandrecovery_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.no_sequence_of_operations_leaves_two_recoveries_pendingruns 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, thatget_pending_recoveryagrees 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 testfor the wallet: 102 passed locally before I rebased onto currentmain. After the rebase the only conflict was twomodlines inlib.rs, but I haven't re-run the suite locally on the rebased commit, so CI is the check for that. Mobilejest lib/__tests__/recovery.test.ts: 25 passed, including a new case for codes 28/29.Clients and docs
frontend/mobile/lib/recovery.tstranslates 28 and 29 into user-facing messages. 13, 14 and 15 are kept for older wallets.frontend/docs/pages/sdk-errors.mdxgets corrected rows for 13/14 and new rows for 28/29.frontend/docs/pages/security/key-rotation.mdxgets 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:
cancel_recoveryandcancel_recovery_request. ARecoveryNotPendingfrom one of them is harmless; it only means that path was idle.RecoveryAlreadyPending/RecoveryNotPendingdon't tell you which path they refer to.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.jsonNot 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.wasmhash, 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 aninitargument rather than embedding it. Please don't merge before that commit lands.