fix(wallet): prevent multi-wallet global wipe - #1014
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughChangesWallet authorization flows
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The PR tightens wallet enumeration and recovery authorization before destructive resets; no actionable merge-blocking risk remains, so it is merge-ready after normal checks and review. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant WalletsViewModel
participant SwiftDashSDKHost
participant Keychain
participant DWRecoverModelMnemonic
WalletsViewModel->>SwiftDashSDKHost: request strict persisted mnemonics
SwiftDashSDKHost->>Keychain: enumerate wallet IDs and read mnemonics
Keychain-->>SwiftDashSDKHost: complete wallet set or read error
SwiftDashSDKHost-->>WalletsViewModel: return mnemonics or fail
WalletsViewModel->>DWRecoverModelMnemonic: validate recovery phrase across wallet set
DWRecoverModelMnemonic-->>WalletsViewModel: authorize or deny destructive flow
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Automated deep review (10 finder angles + adversarial verification). The core fail-closed logic is sound: normalization is applied on both sides of every comparison, the strict enumeration genuinely closes the partial-read hole, and no fail-open path was found in the changed code. But the routing change creates one real functional regression, plus several smaller issues. Inline comments cover the findings anchored to changed lines; findings in files this PR doesn't touch are below. A follow-up commit on this branch applies fixes for most of these.
Findings outside the diff:
-
Removal dead-end /
removeWalletguard scope (WalletsViewModel.swift:322): whenremovalRoutesToFullResetreturnsfalsefor the sole rendered row (distinct-phrase wallet stored only on the other network, or any keychain failure),beginRemoveopens the per-wallet remove sheet, the user's phrase verifies (recoveryPhraseMatchesderives locally, no keychain), and thenremoveWalletrefuses with "Cannot remove the last wallet here." — a false message, after walking the user through phrase entry. The reset flow holding the strong accept-phrase override is only presented when routing returnstrue, so the documented "global emergency override" is unreachable exactly when routing denies it. Repro: wallet A on mainnet → switch to testnet (mirror persists A) → add distinct-phrase wallet B → remove testnet-A → switch back to mainnet → try to remove A. Also, the last-wallet guard fires only whenwalletId == activeWalletId()(a UserDefaults registry read); a sole non-active row would be hard-deleted viadeleteWalletFromSDKwith none of the reset teardown. -
WalletsScreen.swift:249-251comments are now stale: "The last remaining wallet routes to the existing full reset flow…" — post-PR, routing can returnfalsefor the last rendered wallet, so the comments (and theremoveWalletprecondition doc atWalletsViewModel.swift:309-310) no longer state what the code does. -
Pre-existing, surfaced because this PR's doc claims set-wide authorization for "the global wipe": two phrase-less routes reach
DWSwiftDashSDKWalletWiper.wipeWallet(removingPin:)with no set-wide check — (a) the lock-screen wipe offered after 6 failed PIN attempts (DWLockScreenViewController.m:172-238, unauthenticated, alert copy says "erase this wallet" singular while erasing every wallet on both networks); (b) "Reset Wallet (Debug)" (SecurityMenuViewModel.swift:121-127) is appended with no#ifgate and its tap path skipsviewModel.authenticate, so a Release-build user can trigger an immediate global wipe with no PIN and no phrase. The wiper boundary itself accepts no authorization evidence — a new entry point added tomorrow silently regains the vulnerability; longer-term, set-wide authorization belongs in a single boundary in front of the wiper. -
WalletsViewModel.hasSingleWallet(:128) now has zero consumers yet preserves verbatim the "rendered row count = last-wallet test" this PR declares unsafe — worth deleting before a future caller reaches for it.
🤖 Generated with Claude Code
| /// last-wallet test. Prove from Keychain ground truth that every stored | ||
| /// wallet id resolves to the same recovery phrase as the target. Any | ||
| /// enumeration/read failure returns false (fail closed). | ||
| func removalRoutesToFullReset(walletId: Data) -> Bool { |
There was a problem hiding this comment.
Routing regression — sole rendered wallet can dead-end. false from this method no longer implies "≥2 removable wallets on this network": it is also returned for the sole rendered row when a distinct-phrase wallet exists only on the other network, or when the strict enumeration throws. In those states the screen opens the per-wallet remove sheet, the phrase verifies, and removeWallet then refuses ("Cannot remove the last wallet here.") — a guaranteed dead-end with a false error, and the reset flow carrying the strong accept-phrase override is unreachable. Suggest refusing up front in beginRemove with honest copy when routing is false and no sibling row exists.
| let entries = try SwiftDashSDKHost.strictlyPersistedMnemonics() | ||
| guard let target = entries.first(where: { $0.walletId == walletId }) else { return false } | ||
| let targetMnemonic = Mnemonic.normalizePhrase(target.mnemonic) | ||
| return !targetMnemonic.isEmpty && entries.allSatisfy { |
There was a problem hiding this comment.
Reuse: this normalize-and-compare-all predicate now exists three times (here, canWipeWithPhrase, canResetPinWithPhrase) with already-divergent guard/logging styles (do/catch+log here, silent try? there). The Remove-routing gate and the wipe-authorization gate are two halves of one policy — divergence between them is exactly the bug class this PR closes. Suggest one host-level helper next to strictlyPersistedMnemonics() (e.g. allStoredMnemonicsMatch(phrase:)), consumed by both.
| @objc(isWalletEmpty) | ||
| func isWalletEmpty() -> Bool { | ||
| guard let entries = try? SwiftDashSDKHost.strictlyPersistedMnemonics(), | ||
| entries.count == 1 else { return false } |
There was a problem hiding this comment.
Three issues in this gate:
-
Wrong-network emptiness (residual hole): the surviving arm certifies emptiness from
SwiftDashSDKWalletState.shared.balance— active-network state. Single stored id created while on testnet, seed also holds mainnet DASH: testnet balance 0 + syncDone → plain-"wipe" deletes the only copy of a mainnet-funded seed with no phrase. Consider requiringWalletEnvironment.isMainnetas well. -
False refusal reason: the id-count gate is intentionally conservative (correctly so — don't relax it to phrase-count), but the network-switch mirror permanently stores a second id for the same seed (
SwiftDashSDKHost.recoverPersistedWalletre-stores under the new network's id; nothing prunes it), so any single-seed user who ever toggled networks is refused here — and the alert then claims "This wallet is not empty or sync has not finished", which is false. The caller should distinguish the multi-id refusal and say so. -
Efficiency/secret hygiene: this materializes every mnemonic String via N secret-returning keychain reads just to evaluate
count == 1, and does so before the cheap in-memory balance/sync checks.WalletStorage.listWalletIdsWithMnemonic()is attributes-only, throws on failure, and exactly implements the documented "exactly one stored wallet id" gate.
| func canWipeWithPhrase(_ phrase: String) -> Bool { | ||
| let typed = Mnemonic.normalizePhrase(phrase) | ||
| guard !typed.isEmpty else { return false } | ||
| guard let entries = try? SwiftDashSDKHost.strictlyPersistedMnemonics(), |
There was a problem hiding this comment.
Swallowed error + false user-facing copy. try? collapses three distinct causes — keychain read failure, a second distinct stored wallet, and a genuinely wrong phrase — into one false, surfaced as "Recovery phrase doesn't match" (DWRecoverViewController.m:217) with no log line (contrast removalRoutesToFullReset, which logs the same failure; the pre-PR lenient path also logged enumeration failures). In the wipe contexts not pre-gated by the Wallets screen (jailbreak-check wipe, setup recover-with-wallet), a multi-wallet user typing a phrase that correctly matches one wallet is told it doesn't match — they'll conclude their backup is wrong. Suggest: do/catch with logging, and a distinct "multiple wallets stored" alert (with the accept-phrase override hint) when the phrase matches ≥1 but not all.
| /// wallet's BIP32/BIP44 xpub comparison — because BIP39 phrase→seed is | ||
| /// deterministic, but derives no key material. No persisted SDK mnemonic | ||
| /// ⇒ false (fail-closed; the strong accept-phrase still allows a wipe). | ||
| /// Authorizes the GLOBAL wipe with a recovery phrase (C6-D): the typed |
There was a problem hiding this comment.
Nit (CLAUDE.md guardrail #6): the rewritten doc comment re-commits the plan-stage label "C6-D" (here and in DWRecoverModel.m:53), which is defined nowhere in the repo — the comment rewrite was the moment to drop it.
| that route. | ||
|
|
||
| Recovery-phrase authorization inside the global wipe is set-wide: the typed | ||
| phrase must match every strictly readable stored mnemonic (multiple |
There was a problem hiding this comment.
Two spec-accuracy issues in this paragraph:
-
"must match every strictly readable stored mnemonic" reads as subset-matching (unreadable entries exempt), but the code refuses outright when any entry is unreadable — and "strictly readable" is defined nowhere. A future implementer "fixing" the code to match this text would reopen the partially-readable-keychain hole this PR closes. State explicitly: any unreadable entry denies authorization.
-
"authorization inside the global wipe" / "the global emergency override" over-claims: enforcement lives at the entry points this PR touched, not in
DWSwiftDashSDKWalletWiper— and phrase-less routes to the wiper remain (lock-screen wipe after 6 failed PINs; Reset Wallet (Debug), currently Release-visible). The doc should name where enforcement lives and enumerate the unphrased routes, or the enforcement should move to the wiper boundary.
- refuse a sole rendered wallet's removal up front (honest copy) instead of walking the user through phrase verification into a guaranteed refusal when reset routing is denied (distinct cross-network wallet / keychain failure) - make removeWallet's last-wallet bail independent of the active-wallet registry so a stale registry can never hard-delete the only rendered wallet - restrict the plain-"wipe" shortcut to mainnet: a zero testnet balance cannot prove the seed's mainnet balance is empty - explain wipe denials honestly: new alerts for "phrase matches one wallet of several" and the multi-id / testnet shortcut refusals, instead of the false "Recovery phrase doesn't match" / "not empty" copy - log keychain failures in the wipe gates (do/catch) instead of silent try? - consolidate the set-wide phrase predicate into SwiftDashSDKHost helpers (allStoredMnemonicsMatch / anyStoredMnemonicMatches / persistedWalletIdCount) consumed by routing, wipe auth, and forgot-PIN - count wallet ids attributes-only in isWalletEmpty (no mnemonic secret reads just to count), cheap in-memory checks first - compile-gate Reset Wallet (Debug) out of Release (#if DEBUG || DASH_TESTNET) - delete unused hasSingleWallet (the row-count last-wallet test this PR declares unsafe), refresh stale routing comments, drop C6-D plan labels - DASHSYNC_KEY_MIGRATION.md: state that unreadable entries deny outright, document where enforcement lives and the remaining phrase-less wiper routes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Applied fixes for the review findings in 6ffdfda (canonical dashpay arm64 simulator build passes). Not addressed by that commit, by design: the lock-screen phrase-less wipe route (product decision — now documented in DASHSYNC_KEY_MIGRATION.md along with a TODO(wipe-auth) to move set-wide authorization to a single boundary in front of the wiper). 🤖 Generated with Claude Code |
2 participants
Summary
wipeshortcut only when exactly one wallet ID is storedRoot cause
The Wallets screen treated its rendered row count as the complete wallet inventory, but the global reset deletes SDK mnemonics across networks, including wallets not loaded in the current view. The recovery flow also accepted a phrase matching any persisted wallet, and its zero-balance shortcut checked only the active wallet. Together, those paths could authorize deleting a distinct second wallet.
Safety behavior
Destructive authorization now fails closed if wallet enumeration or any mnemonic read fails. Multiple network-scoped IDs remain valid when they all resolve to the same recovery phrase.
Verification
dashpayarm64 simulator build passedswiftc -parseplutil -lintgit diff --checkSummary by CodeRabbit
Security Improvements
Bug Fixes