feat(mobile): swap execute screen with passkey gate - #512
Conversation
|
@collinsezedike is attempting to deploy a commit to the miracle656's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@collinsezedike Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Miracle656
left a comment
There was a problem hiding this comment.
There's real craft in this one — separating signing from submitting so the UI can say what the user is actually waiting on is a genuine improvement over the web's single swapping state, the Soroswap→SDEX pathPaymentStrictSend fallback is the right resilience move, and refreshing an expired quote before submit rather than failing is a nice touch.
I have to block it on a security issue though, and I want to explain it properly because the root cause is a platform difference that's easy to miss when porting.
🚨 Blocker: the signing key is stored unencrypted
const [secret, credId] = await Promise.all([
AsyncStorage.getItem('veil_signer_secret'),
AsyncStorage.getItem('invisible_wallet_key_id'),
])AsyncStorage is not encrypted on either platform. On Android it's a plaintext file in the app's data directory; on iOS it's an unencrypted file in the app container. It ends up in unencrypted device backups, and it's readable on a rooted or jailbroken device, by any process running as the app's user, and by forensic extraction tools.
And this key isn't decorative — it signs and broadcasts the swap:
const signer = Keypair.fromSecret(signerSecret)
const assembled = SorobanRpc.assembleTransaction(built, sim).build()
assembled.sign(signer)Anyone who reads that string can execute swaps against the account without ever touching the device again.
It needs to be expo-secure-store, which is backed by the iOS Keychain and the Android Keystore:
import * as SecureStore from 'expo-secure-store'
const secret = await SecureStore.getItemAsync('veil_signer_secret', {
requireAuthentication: true,
})Note that requireAuthentication: true makes the OS itself gate retrieval behind biometrics — which is strictly stronger than the current arrangement, and I'll come back to why in a second.
Whatever writes this key elsewhere in the app needs the same treatment, otherwise the read just fails. Worth grepping for veil_signer_secret across the mobile tree before you push.
🚨 Blocker: the passkey gate isn't load-bearing
async function authorizeWithPasskey(credentialId: string): Promise<void> {
...
const result = await Passkey.authenticate({ ... })
if (!result) throw new Error('Passkey authentication was cancelled.')
}The assertion is checked for truthiness and then discarded. It isn't verified, and it isn't bound to the transaction being signed. The signing path doesn't consume it at all — it goes straight to Keypair.fromSecret(...). So the prompt proves a human was present on that screen, and nothing more. Combined with the storage issue above, an attacker holding the secret skips the prompt entirely.
Your comment is honest that this is an intent confirmation rather than authentication, and I appreciate that. The problem is that a reader will reasonably assume a biometric prompt in front of a signing operation is protecting the signing operation.
Two ways to make it real, in ascending order of correctness:
- Short term — use
SecureStorewithrequireAuthentication: trueas above. The biometric check then actually gates key retrieval at the OS level instead of running beside it. - Properly — this is an invisible wallet: the passkey is supposed to be the signer, via the contract's
__check_auth, rather than sitting next to a locally-stored Stellar key. The SDK already exposessignAuthEntryfor exactly this, and it's the pathfrontend/walletuses. A locally-held secret with a passkey prompt beside it inverts the architecture the project is built on.
I'd accept (1) to unblock, but I'd like to understand why (2) wasn't the route — if there's a reason the SDK path doesn't work on RN yet, that's very much worth knowing, and it may be its own issue.
Also
Math.random() for the WebAuthn challenge:
// expo-crypto is not required; Math.random is fine for a presence check
for (let i = 0; i < challengeBytes.length; i++) {
challengeBytes[i] = Math.floor(Math.random() * 256)
}Math.random() is not cryptographically secure. The reasoning holds only as long as the challenge is never verified — so this is fine today precisely because of the previous point, which isn't a comfortable place to be. If you adopt option (2), this must become expo-crypto's getRandomBytes(32). I'd just change it now.
rpId: 'veil.app' — this is the third different app identity in the open queue: #508 uses app.veil.xyz / xyz.veil.wallet, your #514 uses veil.app / app.veil.mobile. I'm settling on one canonical pair and will confirm on #514. Don't change it here until I do.
Conflicts with main — app/swap.tsx is an add/add conflict (main already has a Soroswap-quoting swap screen) and package.json conflicts too. Your version supersedes it, so take yours for swap.tsx, but please rebase so the diff is honest about what it replaces.
Depends on #514 — react-native-passkey can't run in Expo Go, so this needs the dev-client work in your #514 to land first. You add the dependency in both PRs; drop it here once #514 is in.
919 lines in one file — not blocking, but getStoredKeys / authorizeWithPasskey / signAndSubmitSoroban / fetchSoroswapQuote are all reusable and every other screen that signs will want them. lib/ (top level, not app/lib/) is the right home. Extracting them would also make them unit-testable, which matters a lot for the signing path.
Happy to re-review quickly once the storage change is in — the rest of this is good work and I don't want it stuck.
2843cd5 to
08fcf72
Compare
|
Thanks for the detailed review. Storage - moved Challenge entropy -
Rebase - done. Our
Regarding option (2) - using the SDK's |
|
One more thing I should have caught before pushing: grepping The web wallet writes it in |
|
Thanks for this — there's real substance in the execute path, and the biometric reasoning is documented carefully. Closing it though, because the file it rewrites has been replaced underneath it and the branch now duplicates several pieces that landed separately. The base moved
What's now duplicated
|
Summary
eact-native-passkey) separate from \submitting\ (network broadcast), unlike the web's single \swapping\ state
eact-native-passkey, @react-native-async-storage/async-storage, and @soroswap/sdk\ to \rontend/mobile/package.json\
Test plan
Resolves #474