Repository navigation
Conversation
The recovery phrase now lives in a Keychain item (iOS, userPresence access control) or a user-authenticated Keystore key (Android, biometric or device credential) that the Secure Enclave or StrongBox/TEE releases only after the user unlocks it. A Keychain or Keystore dump by a sandbox-escaped process, as in the FomoPeek attack, no longer yields the seed. Devices without a lock keep the plain store; the wallet does not force a passcode. Every seed read is now the check, so the lock screen, the resume check and the explicit biometric prompts before sending, signing and viewing the phrase are gone. Wormhole addresses come from a per-wallet address book built once from the seed, so discovery, receive addresses and self-send checks never read it. Nullifiers are cached in memory, so a balance refresh reads the seed only for transfers it has not seen. Rust wipes the mnemonic in every derivation entry point and derives the address book from one seed stretch.
|
This will need to be rebased and testing will be extensive and only on device, so can't do this right now Set to draft |
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT-6.1 Sol
Verdict (advisory): Request changes
Reviewed base 77058785a8f64897f5797a503af0a78a1961a04d and head bf140948ee8af24dda8d1ec69698812148d2fbe8. Four blocking findings remain:
-
[P1] Authenticate the address book before using it for deposits. wormhole_address_book.dart:80–85 accepts addresses directly from an unauthenticated app-support JSON file. The changed Receive screen uses these values for the address, QR and checkphrase. Under the PR's stated sandbox-escape threat model, an attacker can replace an external address with their own, restart the app, and redirect subsequent deposits without unlocking the seed. A review probe confirmed that a substituted valid SS58 address is returned by
receiveAddress()with zero seed reads. Derive/verify deposit addresses against the protected seed, or authenticate the book against a trusted integrity root before displaying them. -
[P1] Require authentication before destructive iOS reset. reset_confirmation_screen.dart:23–24 now goes straight to logout.
SeedVault.deleteAll()only deletes items; it never reads the protected seed. The pinned Darwin plugin callsSecItemDelete, and Apple's access-control implementation applies user presence to decryption while allowing deletion unconditionally. With the app lock also removed, someone holding an unlocked iPhone can erase all local recovery phrases without Face ID or the passcode. Restore an explicit authentication step, or perform an authenticated seed access before starting logout. -
[P2] Finish authentication before mutating wallet/session state. The newly interactive deletion at seed_vault.dart:133–136 is called after
SettingsService.removeWallet()has persisted the reduced account list and switched the active account. Cancelling Android's first protected-store prompt therefore reports failure but leaves the wallet removed and its seed orphaned; a probe reproduced exactly this state. Reset has the same ordering problem:SubstrateService.logout()disposes the encrypted services beforeclearAll()prompts, and a cancelled reset leaves their retained provider instances unusable. Authenticate before changing accounts or disposing services, and cover cancellation with regression tests. -
[P2] Handle PIN-only Android 9/10 devices before selecting the protected store. seed_vault.dart:80–83 admits API 28/29 devices based on
isDeviceSupported(), which returns true for a PIN alone. In pinned flutter_secure_storage 10.3.4, those API levels usesetUserAuthenticationValidityDurationSeconds(-1)and a biometric-only CryptoObject prompt; device-credential fallback is enabled only at API 30+. Android's API documentation confirms this legacy per-operation mode requires biometrics. New wallet creation/import fails on PIN-only Android 9/10, while existing-wallet migration fails and leaves the plain seed accessible after the old authentication gates have been removed. Use a compatible credential-protected implementation, or explicitly retain the plain-store authentication gate on unsupported configurations.
Validation: git diff --check passed; all 41 changed Dart files passed formatting; 44 focused SDK tests, 17 focused mobile tests, all 287 cold-wallet tests, and three review probes passed. The probes assert the defective behaviors described above. The Rust address-book equivalence test passed with SDKROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX15.2.sdk cargo test --locked test_wormhole_address_book_matches_single_derivation. Workspace analysis was terminated at the required 10-second limit. Physical iOS/Android authentication flows remain untested here. GitHub's Analyze job passed; the latest dependency-cooldown job failed because the bypass label lacks a Cooldown-bypass-reason: line in the PR body.
Why
A malicious App Store app with an iOS kernel exploit can escape the sandbox, decrypt Keychain items and read other apps' files. We would not currently be 100% safe from such an attack.
What changes
Seed at rest (
SeedVault, wrapped bySettingsService)SecAccessControl(userPresence). The Secure Enclave releases the item key only after Face ID, Touch ID or the passcode.AndroidOptions.biometric(strong biometric or device credential), user-authenticated AES key, StrongBox where available,resetOnError: falseso an error never wipes the seed.SeedAccessCancelled.SeedVaultmarks where an app-level cryptographic password would wrap the seed.One check, not two.
LocalAuthService,LocalAuthController,AuthWrapper, the resume check and the seven explicit prompts (send, encrypted send, multisig propose/confirm/create, reset, recovery phrase) are gone. The seed read is the check. Cold-wallet accounts never read a seed, so they never prompt.Wormhole without the seed (
WormholeAddressBook)ownsAddress, the receive address and cache discards use the book.getUnspentUtxostakes a nullifier resolver instead of secrets.EncryptedAccountServicecaches nullifiers in memory and reads the seed at most once per load, only for transfers it has not seen. Empty or unchanged wallets never read it.Rust.
derive_wormhole_addresses(seed wiped on return); the mnemonic is wiped ingenerate_derived_keypairandderive_wormhole. Dart zeroesKeypair.secretKeyafter signing.Housekeeping. flutter_secure_storage ^10.3.1 (sdk, cold wallet).
local_authanddevice_info_plusmoved into the sdk. DeadSubstrateService.queryUserBalanceremoved. App-support file helpers shared between the three services that scan that directory.Consequences to know
setInvalidatedByBiometricEnrollment(true)). iOS items survive re-enrollment but not passcode removal. Recovery is re-import from the phrase. This is the one place Android is stricter than the userPresence choice on iOS.AppLifecycleManagerignores that viaseedAccessInProgressso it does not refresh everything on resume.Tests
seed_vault_test.dart(store selection, migration, idempotence, coalescing, cancellation mapping) and new encrypted-account tests asserting zero seed reads for load/receive/discard without transfers and one read with an in-memory cache afterwards.flutter analyzeclean in both packages.Needs a device before release
containsKeyon the protected item is silent;readprompts; a cancelled prompt maps toSeedAccessCancelled(details-128). Migration from an existing wallet moves the item without a prompt.