Skip to content

Recover a Braavos 2FA account with the recovery phrase - #552

Closed
l-henri wants to merge 2 commits into
feat/braavos-deploymentfrom
feat/braavos-2fa-recovery
Closed

l-henri wants to merge 2 commits into
feat/braavos-deploymentfrom
feat/braavos-2fa-recovery

Conversation

@l-henri

@l-henri l-henri commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #551. Stacked on #550; retarget down the stack as it merges. The last rung of the Braavos arc.

Problem

A Braavos account with a strong signer (2FA/3FA) was view-only with no way forward from the phrase. Braavos exposes a phrase-only escape hatch — a deferred removal of all strong signers — and this wires it into the read-only Home flag from #546.

Solution

  • Platform (braavos-recovery.ts): submits the one transaction a strong-signer account accepts from the stark key alone — a single self-call to deferred_remove_signers, empty calldata, no paymaster, fee bounded by the account's on-chain cap (MAX_ETD_FEE_V3). It deliberately bypasses the strong-signer refusal that guards every other submission, mirroring the account contract's own carve-out, and refuses anything that is not a deployed strong-signer Braavos account with the wallet's stark key registered. After the account's execution time delay (default 4 days) the account treats the strong signers as removed, so Sign for stark-only Braavos accounts #548's signing checks start passing with no further transaction.
  • Cancel is intentionally not offered from Bramble: the carve-out covers only the removal request, so cancelling needs the 2FA device. The pending copy says exactly that rather than showing a button that would fail at signing. (This is the one place I diverged from the "cancel action" specified — the protocol doesn't allow a phrase-only cancel.)
  • Background: privileged reviewBraavosRecovery (fee + delay) and recoverBraavosAccount (submit) handlers; the signability verdict now carries the recovery state so the flag needs no extra round trip.
  • Home flag renders all three states: read-only + "Recover account", read-only mid-removal with the delay and the device-cancel note (buttons stay greyed), and ordinary once signable. The pure view states the delay rather than a live countdown, since a ticking countdown would need the wall clock in the render layer.

Verification

  • Platform unit tests: strong-signer requirement, "no 2FA to remove" refusal, on-chain fee cap, pending/delay state read.
  • Background: privileged review + submit handlers (and the key-boundary catalog).
  • UI: the recover button, the pending delay + device note, no button for a non-phrase-recoverable restriction.
  • Mainnet, through the wallet's own review/submit path: braavos-matrix/4 accepted the removal request (tx 0x1706142958afa1a978e291cfa159aa22e12e5c72e13834644830fc0aff73564); it now reads pending, usable after the four-day delay, and should flip to signable on its own when that elapses.
  • LC_ALL=en_US.UTF-8 bun run check passes end to end.

🤖 Generated with Claude Code


Update: #553 (Ready guardian recovery) stacks on this and reuses the same Home flag + verdict recovery field, now with a stage for Ready's two-step escape. The pending copy here shows an absolute UTC datetime per review feedback.

@mossbot55 mossbot55 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wires the Braavos phrase-only 2FA removal (deferred_remove_signers) into the read-only Home flag. I read all 19 changed files at 5e193821 and ran the checks below.

Two things I think should land before merge, both detailed inline:

  1. Home does not refresh the signability verdict after a successful submission, and the new message was not added to the two refresh registries, so the pending state is not shown and the Recover account button stays tappable, allowing a second, separately paid removal request.
  2. The submission does not follow the indeterminate-write contract the other chain-write lanes use, so a lost broadcast reply or a broadcast-hash mismatch reads as an ordinary retryable error instead of a no-blind-retry response with an Activity record.

Also inline, non-blocking: the submit path has no test, the fee bound is weaker than the invoke lane's, and two comments are stale or inaccurate.

Containment of the signing carve-out looks sound: the estimate and the executed call are both the fixed deferred_remove_signers self-call, requireRecoverableBraavos rejects non-Braavos accounts, unknown classes, no-signer and non-strong-signer states, and the session check fails closed. The wire validators and the privileged-message and key-boundary registries were extended consistently.

Note this is the top of the stack (#543 -> #546 -> #548 -> #550 -> #552); #550 carries an outstanding blocking finding of mine, so the base is not yet ready either.

Checks: fresh worktree at 5e193821, bun 1.3.3, bun install --frozen-lockfile; focused suites for the new platform, background and UI tests pass; LC_ALL=en_US.UTF-8 bun run check exits 0 with every workspace green.

Time: 35m 20s. Approx. API cost: $0.47.

"success",
button,
);
await deps.wallet.loadStatus().then(deps.render);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repaints with no refresh reason, so the signability verdict is not re-probed after the action. deps.render is the entrypoint render and its optional refreshReason reaches hydration only when present; hydrate.ts:307 forces the signability cache only for refreshReason === "action", and signability-cache.ts otherwise returns the cached verdict for 300000 ms. wallet.recoverBraavosAccount is also missing from MUTATING_MESSAGES and CHAIN_SETTLING_MESSAGES in app/sync/runtime-refresh.ts (it was added to WALLET_OPS_HANDLERS, KEY_BOUNDARY_MESSAGES, runtimeRpcFor and SECRET_MATERIAL_FILES but not those two), so no "action" refresh is scheduled either.

Net effect: right after a successful submission Home repaints from the cached pre-submit verdict (recovery.pendingUntil === null), so readOnlyNotice renders the Recover account button again and the Removing 2FA ... usable on ... state is not shown. The scene renderer replaces the DOM and rebinds, so the closure's armed flag also resets: the user can review and submit a second deferred_remove_signers request and pay a second fee. Nothing in the submit path refuses it because requireRecoverableBraavos still answers strong-signer while a request is pending. Adding wallet.recoverBraavosAccount to both sets (as wallet.transfer and wallet.deployAccount are) delivers the forced probe the cache's own doc promises for a completed action.

tip: "0x0",
});
const transactionHash = acceptedRpcWriteHash(submitted.transaction_hash);
if (signedHash === undefined || BigInt(signedHash) !== BigInt(transactionHash)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reimplements the broadcast-hash check with acceptedRpcWriteHash plus a plain WalletError, while every other submission lane calls matchingAcceptedRpcWriteHash(submitted.transaction_hash, signedHash) and lets it throw IndeterminateRpcWriteError (provider.ts:128; callers rpc-submit.ts:308, deployment.ts:248, declare.ts:128, multisig-invoke.ts:227). A mismatch after a broadcast means the node may have accepted a write, so it must not read as a determinate failure. Using the shared helper also validates that the signed hash is a non-zero field element.

return { error: "account recovery unavailable" };
}
const authorized = captureSessionAuthorization(deps.session);
const submitted = await deps.runtimeRpc.submitBraavosRecovery({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleRecoverBraavosAccount has no indeterminate-write handling. Every other chain-write lane catches IndeterminateRpcWriteError and returns submissionUnknownResponse(...) so the panel never offers a blind re-send and Activity records the possibly-landed hash (wallet-ops.ts:370, swap-ops.ts:346, strk20-wallet-execution.ts:302, the multisig lanes). Here a lost broadcast reply (the mismatch above is one path) propagates through the message router as { error }, so bindRecoverAccount shows Couldn't start recovery. Try again. and no Activity entry is written. Please add the same catch/response, passing the signed hash.

skipValidate: true,
tip: "0x0",
});
const spend = reviewedSpendFor(estimate, "standard");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, but the bound is weaker than the invoke lane's. reviewedSpendFor never returns null: when the resource bounds cannot be read it falls back to the node's headline overall_fee, which can understate what the signed bounds authorize. The invoke lane (rpc-submit.ts settledResourceBounds) instead uses authorizedSpendFor and refuses when it returns null, so the figure compared to the reviewed fee is always the signed-max spend. Matching that here keeps the on-chain 100 STRK cap from being the only ceiling.


const signHash = async (_hash: string) => ({ publicKey: PUBLIC_KEY, r: "0x1", s: "0x2" });

describe("braavos recovery", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No test exercises submitBraavosRecovery: this suite imports only the state read and reviewBraavosRecovery, and the background and key-boundary suites stub the submit. The submit path holds the mandatory reviewed-fee bound, the authorizedSigner wrapper with the session re-check, the signed-hash/accepted-hash match, and the post-wait expire_at read-back, so it is the riskiest new code. A focused test covering a fee over the reviewed figure and a broadcast hash that does not match the signed hash would close the gap.

import { deploymentKnownFromPrivacy, type PrivacyDisclosureState } from "../privacy/privacy-model";
import { homeTabs, unifiedHomeBalances, visiblePrivateHomeBalances } from "./home-tabs";

// Whole days a delay spans, for the recovery copy. Takes the delay it is

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment: the days helper it describes lives in bindRecoverAccount in home-ui.ts; nothing in this file matches it. formatUtcInstant's own doc follows at line 41.


/**
* Quotes the Braavos recovery request for its review card: the fee, and the
* delay after which the removal takes effect. Privileged; the phrase key signs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor doc inaccuracy: both the review and submit estimates call estimateInvokeFee with skipValidate: true (braavos-recovery.ts:139 and :178), and with that flag starknet.js builds an empty signature and never calls the signer, so no query-version estimate is signed on the review path. Worth rewording so the comment matches what runs.

l-henri and others added 2 commits October 6, 2026 11:31
A Braavos account with a strong signer (2FA/3FA) was view-only with no
way forward from the phrase. Braavos exposes a phrase-only escape hatch,
and this wires it into the read-only Home flag.

A new platform module submits the one transaction such an account accepts
from the stark key alone: a single self-call to deferred_remove_signers,
empty calldata, no paymaster, and a fee bounded by the account's on-chain
cap (MAX_ETD_FEE_V3). It deliberately bypasses the strong-signer refusal
that guards every other submission, mirroring the account contract's own
carve-out, and it refuses anything that is not a deployed strong-signer
Braavos account whose stark key is registered. After the account's
execution time delay (default four days) the account treats the strong
signers as removed, so the existing signing checks start passing with no
further transaction; cancelling during the window needs the 2FA device,
which the copy says because the carve-out does not extend to the cancel.

The background exposes privileged review and submit handlers; the
signability verdict carries the recovery state (pending expiry, or the
delay a fresh request would take) so the Home flag renders all three
states: read-only with a Recover account button, read-only mid-removal
with the delay and the device-cancel note, and ordinary once the account
becomes signable. The verdict and flag follow #548's checks, which
already flip a strong-signer account to signable the moment its removal
delay expires.

Proven on mainnet through the wallet's own review/submit path:
braavos-matrix/4 (simulated 2FA) accepted the removal request (tx
0x1706142958afa1a978e291cfa159aa22e12e5c72e13834644830fc0aff73564) and
reads pending, usable after the four-day delay.

Fixes #551.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The pending read-only flag stated the delay in days; show the exact
moment the account becomes usable instead — "Usable on 09/10/26 at
13h28 UTC". Formatted with pure civil-from-epoch integer arithmetic so
the render layer reads no wall clock.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@derek-flossman

Copy link
Copy Markdown
Collaborator

Closing in favour of #559, which brings this change into v0.10.5 with your commits unchanged (only an import-line rebase conflict resolved), together with the fixes from four independent senior reviews as separate labelled commits, so the whole stack ships with one review. Thanks, Henri!

derek-flossman added a commit that referenced this pull request Oct 6, 2026
…vos-stack

v0.10.5: Ready/Braavos stack (#546, #548, #550, #552, #553) with review fixes, plus audit fix
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.

3 participants