Skip to content

fix: queue intents until unlocked and bind send to source account (M10, L10)#582

Open
n13 wants to merge 3 commits into
mainfrom
fix/m10-l10-intent-gating-send-account
Open

fix: queue intents until unlocked and bind send to source account (M10, L10)#582
n13 wants to merge 3 commits into
mainfrom
fix/m10-l10-intent-gating-send-account

Conversation

@n13

@n13 n13 commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

Addresses findings M10 and L10 of the 2026-07-22 mobile wallet security audit.

M10 — intents consumed beneath the lock overlay

Deep-link (/pay, /account) and notification (transaction, multisig proposal) intents were acted on by HomeScreen's listeners while the lock overlay was up, so e.g. a https://www.quantus.com/pay?to=…&amount=… link pushed an attacker pre-filled InputAmountScreen while the app was locked — the first thing the user saw after unlock.

  • All intent handlers in home_screen.dart now require localAuthProvider.isAuthenticated before consuming.
  • Intents arriving while locked simply stay in their StateProviders (queued) and are drained when isAuthenticated flips to true (new listener in HomeScreen.initState).
  • The cold-start retry on activeAccountProvider now drains all pending intents, not just transaction intents.

L10 — source account re-resolved at submit time

RegularSendStrategy was account-less and re-resolved the signing account at submit time (SettingsService().getActiveRegularAccount()), so an account switch between review and confirm (e.g. via a proposal-notification tap) signed from the new account.

  • RegularSendStrategy now captures the source Account at flow start (matching EncryptedSendStrategy/MultisigProposeStrategy) and uses it for fee estimation, balance validation and submission. Callers (home send button, payment intent, shared-address sheet) pass the account in; payment/shared-entry flows require an active regular account.
  • New effectiveMaxBalanceProviderFamily(accountId) binds spendable-balance validation to the captured account instead of the active account.
  • New sendFlowActiveProvider counts send-flow root screens (SelectRecipientScreen/InputAmountScreen). While a send is in flight, intents that navigate or switch the active account (payment, shared-account, proposal) are deferred and drained when the flow leaves the stack — a notification tap can no longer hijack an in-flight send.

Verification

  • dart analyze in mobile-app/: no issues.
  • flutter test test/unit: 222/222 passed.

Notes

  • The multisig-propose and encrypted flows already carried their source account in the strategy; only the regular send needed the L10 change.
  • Transaction-detail intents are gated on unlock but not deferred mid-send (they only show an informational sheet and don't switch accounts).

n13 added 2 commits July 23, 2026 17:31
…0, L10)

M10: deep-link and notification intents were consumed by HomeScreen's
listeners even while the lock overlay was up, pushing attacker-crafted
screens underneath it. Intent handlers now require
localAuthProvider.isAuthenticated; intents arriving while locked stay
queued in their providers and are drained on unlock.

L10: RegularSendStrategy re-resolved the source account at submit time,
so an account switch between review and confirm signed from the new
account. The strategy now captures the source account at flow start and
uses it for fee estimation, balance validation and submission. Intents
that navigate or switch the active account are also deferred while a
send flow is on the stack, so a notification tap can't hijack an
in-flight send.
@n13

n13 commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

🔍 Review — M10, L10 (intent gating & send-account binding)

Verdict: 🟡 Approve with non-blocking comments

The core fixes are correct and well-targeted: intent handlers now require isAuthenticated, so nothing is pushed under the live lock overlay (M10), and RegularSendStrategy captures its source Account at flow start and uses it for fee estimation, balance validation, self-send guard and submission (L10). The sendFlowActiveProvider deferral counter is balanced and leak-free. A handful of non-blocking behavior/UX/test-coverage items are worth addressing before or shortly after merge.

What it does

  • Gates all four HomeScreen intent handlers (_onTransactionIntent, _onPaymentIntent, _onSharedIntent, _onProposalIntent) on _isUnlocked; deferred intents stay queued in their StateProviders and are drained by new localAuthProvider and sendFlowActiveProvider listeners plus the widened activeAccountProvider cold-start retry (home_screen.dart:82-93).
  • Binds RegularSendStrategy to a captured Account (regular_send_strategy.dart:27-34,49-50,62-63,115), replacing the old submit-time re-resolution via SettingsService().getActiveRegularAccount() and activeAccountProvider.
  • Adds sendFlowActiveProvider (int counter incremented/decremented in SelectRecipientScreen/InputAmountScreen init/dispose) so payment/shared/proposal intents are deferred while a send is in flight (send_providers.dart:13, input_amount_screen.dart:67,82,107, select_recipient_screen.dart).
  • Replaces effectiveMaxBalanceProvider with effectiveMaxBalanceProviderFamily(accountId) bound to the captured account (send_providers.dart:19-35).

Strengths

  • Root cause correctly identified. AuthWrapper is a Stack sibling over the live Navigator (app.dart:55), rendered whenever !isAuthenticated (auth_wrapper.dart:19). Gating on the same isAuthenticated flag is the right invariant, and because the overlay is not a route on the Navigator, draining/navigating on unlock does not race the overlay's own dismissal.
  • Gate cannot be bypassed. Grep of all four intent providers confirms the only consumers are HomeScreen's listeners; deep_link_service/transaction_service merely produce (state = …). No second unguarded consumer exists.
  • Queued intents are preserved, not dropped. Every deferred handler returns before setting provider.state = null, so the payload survives until conditions allow it (unlock, account load, send-flow exit). The re-drain listeners are idempotent.
  • L10 binding is complete. Base re-resolved the account in three places (sourceAccountId, estimateFee, submit); all now use the captured account, so a mid-flow switch (even one triggered by a proposal-notification tap) cannot change the signer, fee basis, or self-send guard.
  • Counter is symmetric and leak-safe. Increment in initState / decrement in dispose, with the notifier cached in a late final so dispose never touches ref. Terminal/review screens are pushed on top of InputAmountScreen (review_send_screen.dart:74), so the counter stays >0 through confirm/success and only drains after the flow fully unwinds — no premature drain over the success screen.

Findings

  1. [non-blocking] Balance-error fallback silently changed. Old effectiveMaxBalanceProvider read balanceProvider, whose error branch returns AsyncValue.data(_cachedBalance) (wallet_providers.dart:207-209). The new effectiveMaxBalanceProviderFamily reads effectiveBalanceProviderFamily, whose error branch propagates AsyncValue.error (wallet_providers.dart:153-154). In input_amount_screen.dart:190 the max-sendable is computed as ...spendableBalanceProvider).value ?? BigInt.zero, so on a transient balance-query error mid-send the Max button and the amount <= spendable check now see 0 instead of the last-good cached balance — a transient RPC hiccup can block an otherwise-valid send. For the normal (data) case the two are identical, so this is a side-effect of the provider swap rather than an intended change. Consider a cached/last-good fallback for the captured account, or confirm 0-on-error is desired.

  2. [non-blocking] Payment intent silently dropped for non-regular active account. _onPaymentIntent consumes the intent (paymentIntentProvider.state = null) and then, if active is! RegularAccount, returns with only quantusDebugPrint (home_screen.dart:126-130). The user sees nothing, and because the intent is already consumed the subsequent activeAccountProvider re-drain can't recover it after they switch to a regular account. Contrast with the active == null path, which defers. Consider deferring (don't null the state) until a regular account is active, or surfacing a toast. Same shape in shared_address_action_sheet._sendToAddress (shared_address_action_sheet.dart:64-69): it returns before Navigator.pop, leaving the sheet open so the "Send" tap appears dead.

  3. [non-blocking] isVisuallyLocked is outside the gate. _isUnlocked is isAuthenticated only; when authenticated-but-visually-locked (the privacy overlay during the 5s background grace, auth_wrapper.dart:23) an intent would be consumed under the privacy overlay. In practice the resume path (checkAuthentication) clears the visual lock or forces re-auth, and the grace window is intentional, so this is very likely not exploitable — but a one-line comment noting the deliberate choice would help future readers.

  4. [non-blocking] No tests added. This is a security remediation with non-trivial new logic (four gated handlers, a deferral counter with init/dispose symmetry, account capture) and the diff touches zero test files. dart analyze clean + 222 pre-existing tests do not exercise the new gate/deferral/binding. A couple of widget/unit tests (intent-while-locked stays queued then drains on unlock; intent-while-send-in-flight defers then drains on flow exit; submit signs from captured account after a switch) would materially reduce regression risk.

  5. [nit] Inconsistent debug logging. home_screen.dart uses quantusDebugPrint while shared_address_action_sheet.dart:65 uses raw debugPrint for the analogous "cannot send regular transfers" case.

  6. [nit] Two simultaneously-queued intents interleave awkwardly. If both a payment and a proposal intent are queued, one _drainPendingIntents pass runs _onPaymentIntent (pushes InputAmountScreen) and then _onProposalIntent (switches active account + shows the proposal sheet), because the counter isn't incremented until InputAmountScreen.initState runs on the next frame. L10 still holds (the strategy already captured the account), so this is cosmetic, but the UX is messy. Not worth blocking.

Verification

Stated verification (dart analyze: no issues; flutter test test/unit: 222/222) is accurate as far as it goes and the PR-body claims check out against the diff — the three RegularSendStrategy() call sites are all updated (grep found no un-migrated site), the old effectiveMaxBalanceProvider symbol is fully removed with its sole consumer repointed, and the "already carry their account" claim for EncryptedSendStrategy/MultisigProposeStrategy is correct (home_screen.dart:372,402). The gap is that verification is entirely regression-suite based: none of the 222 tests are new, so the actual M10/L10 behaviors (gate, deferral, account binding) are unverified by automated tests (Finding 4). Manual verification of the deep-link-while-locked and switch-account-mid-send scenarios is not described in the PR body.


🤖 AI-assisted review generated with Claude Code

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.

1 participant