CP-14604: reject WalletConnect signing requests from accounts not granted to the session

[bogdandobritoiu]

Description

Ticket: CP-14604

Restores WalletConnect request-time session-account authorization that was lost in the vm-module migration (CP-8450 left a TODO, CP-8365/CP-8916 removed the legacy isAddressApproved check). Since then, a connected dApp could request signing with any account in the active wallet — including accounts the user never granted to that dApp's session.

Changes:

• isAccountApproved.ts: adds isAddressApproved(address, caip2ChainId, namespaces) — an address-based sibling of the existing (previously unused) helper. Checks the address against the session's granted accounts for the request's CAIP-2 namespace, any chain within the namespace, case-insensitive. isAccountApproved now delegates to it.
• ApprovalController.requestApproval: when signingData carries an account (the module-resolved signer — covers EVM tx, all EVM message signing, all 3 Solana methods, both Bitcoin methods) and the request is not in-app, the WC session for the request's topic is looked up and the request is rejected with providerErrors.unauthorized('Requested address is not authorized') (same error as pre-migration) before the approval screen opens.

Design notes:

• Enforced at the ApprovalController (single chokepoint on the exact address that will sign) instead of validateRequest, avoiding per-method param parsing that could drift from the VM modules.
• The !isInAppRequest(request) guard is load-bearing: WalletConnectService.getSession throws while the WC client is initializing, so in-app Send/Swap during app startup must not touch it. In-app + injected-browser requests ride CORE_MOBILE_TOPIC (per-launch uuid, not spoofable by a dApp) and keep their own account gating.
• Avalanche signing methods are knowingly not checked (their signingData carries no account): the avax namespace is only granted to Core-domain dApps and those methods always sign with the visible active account.
• Legit flows cannot be falsely rejected: the SVM module schemas require the signer account in request params (never fall back to the active account), and dApps only ever learn granted accounts (accountsChanged grants before it announces).

Screenshots/Videos

N/A — no UI change; unauthorized requests reject with an error toast before any approval screen.

Testing

iOS - 9341
Android - 9342

Dev Testing

Happy path (ticket steps — must behave as before):

1. Onboard one wallet, connect to jup.ag via WalletConnect with account 1
2. Switch to account 2 in Core, swap on jup.ag
3. Approval screen shows account 1 (the granted, requesting account), signs with no error, tx goes through

Reject path (new):

1. With a WC session granted only account 1, send a signing request targeting another account of the same wallet (e.g. replay a solana_signTransaction / eth_sendTransaction with account 2's address as signer)
2. Request is rejected with "Requested address is not authorized"; no approval screen opens

Regression: in-app Send/Swap/Stake, injected-browser dApp signing, Core web via WC (incl. BTC/AVAX flows), Ledger approvals — all unaffected paths.

QA Testing

Acceptance: a dApp can only trigger signing with accounts that are part of its WalletConnect session grant; requests for any other account are rejected with an explicit error and no approval prompt.

Checklist

• I have performed a self-review of my code
• I have verified the code works (unit-tested; device verification pending)
• I have included screenshots / videos of android and ios
• I have added testing steps
• I have added/updated necessary unit tests
• I have updated the documentation

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR restores request-time WalletConnect session account authorization so a connected dApp can only request signing using addresses that were explicitly granted to that session (closing a gap introduced during the vm-module migration).

Changes:

• Adds isAddressApproved(address, caip2ChainId, namespaces) to validate a signer address against a WC session’s granted accounts within the request’s namespace (case-insensitive; any chain in the namespace counts).
• Updates ApprovalController.requestApproval to reject non-in-app requests when signingData.account is not authorized for the WC session topic, returning providerErrors.unauthorized('Requested address is not authorized') before opening the approval UI.
• Adds unit tests covering both rejection and acceptance paths across SVM and EVM, plus guard behavior for in-app/injected-browser requests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts	Enforces WC session account authorization at the approval chokepoint using module-resolved signingData.account.
packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts	Adds tests ensuring unauthorized accounts are rejected early and authorized accounts proceed to the approval screen.
packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.ts	Introduces isAddressApproved and refactors isAccountApproved to delegate to it.
packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.test.ts	Adds focused tests for isAddressApproved across EVM + Solana and negative cases.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Coverage report ✅

2/2 packages passed thresholds
Thresholds are shown inline against each package baseline.

🟢 @avalabs/core-mobile

St.	Category	Percentage	Covered / Total
🟢	Statements	27.93% (+9.93% vs 18% ▲)	12384/44331
🟢	Branches	23.64% (+11.64% vs 12% ▲)	6100/25802
🟢	Functions	22.20% (+9.20% vs 13% ▲)	2274/10240
🟢	Lines	28.01% (+10.01% vs 18% ▲)	11860/42334

🟢 @avalabs/k2-alpine

St.	Category	Percentage	Covered / Total
🟢	Statements	8.49% (+6.49% vs 2% ▲)	304/3578
🟢	Branches	7.90% (+6.90% vs 1% ▲)	172/2177
🟢	Functions	4.99% (+3.99% vs 1% ▲)	41/821
🟢	Lines	7.82% (+5.82% vs 2% ▲)	257/3283

Artifacts and threshold sources

• @avalabs/core-mobile: summary core-mobile/coverage/coverage-summary.json, thresholds core-mobile/coverage-thresholds.json
• @avalabs/k2-alpine: summary k2-alpine/coverage/coverage-summary.json, thresholds k2-alpine/coverage-thresholds.json

Source run: Mobile PR

[B0Y3R-AVA]

Adversarial review (Claude + Codex, cross-checked against svm-module/bitcoin-module/evm-module @ 3.9.0). The core fix looks sound and, importantly, does cover every method it claims: signingData.account is populated at the module level for all EVM/Solana/Bitcoin signing methods (verified in module source — sign-and-send-transaction.ts requires account: z.string(), bitcoin-send-transaction.ts sets account: from, eth-sign.ts sets account: address, etc.), so the 'account' in signingData guard genuinely reaches them all. Batch EVM is CORE_MOBILE_TOPIC-only, Avalanche/HVM correctly excluded (they force the signer from the active account, not dApp params). Nice, tightly-scoped chokepoint.

One thing I'd change before merge (F1 inline), plus two low-severity follow-ups:

F1 — fail-open on a missing session (inline). The session && short-circuit skips the check entirely when getSession() returns undefined. Suggest failing closed.

F2 — injected in-app browser is fully excluded (informational). !isInAppRequest(request) excludes CORE_MOBILE_TOPIC, which the injected in-app browser also rides. Its safety rests entirely on the injected provider being active-account-only (CP-14382/CP-14385). Out of scope here, but worth a one-line confirmation that a site can't supply a non-active from/account on that path.

F3 — case-insensitive address match (inline, pre-existing). Not exploitable, matches pre-migration behavior; noting for a follow-up.

Tests are genuine (only getSession is mocked; isAddressApproved runs for real). Minor nit: the three "accept" cases don't await requestApproval, relying on navigation happening synchronously in the Promise executor — they'd silently pass if a pre-nav await is ever introduced.

[B0Y3R-AVA]

F1 (suggest fail-closed): when getSession() returns undefined, the whole gate is skipped and the approval opens unchecked. requestApproval runs well after the WC session_request event and after async module work (gas/simulation/XP-address fetch), so a session_delete/expiry landing in that window makes this fresh live lookup return undefined — turning "only granted accounts may sign" into no check. Exploitation is narrow (precise race; the torn-down topic can't receive the response), but it's a fail-open in the exact gate this PR adds.

Since the only non-in-app path here is real WC (where a live session must exist), failing closed is safe:

if ('account' in signingData && !isInAppRequest(request)) {
  const session = WalletConnectService.getSession(request.sessionId)
  if (!session || !isAddressApproved(signingData.account, request.chainId, session.namespaces)) {
    return { error: providerErrors.unauthorized('Requested address is not authorized') }
  }
}

F4 (defense-in-depth, non-blocking): getSession is unguarded here; its client getter throws via assertNotUndefined when WC isn't initialized. Practically unreachable for a real session_request (implies an initialized client), but a try/catch would avoid the CP-14756-style unhandled-rejection class if a future caller reaches this path pre-init.

[bogdandobritoiu]

Fixed in 0efb698 — the gate now fails closed: a missing session rejects with unauthorized, and getSession is wrapped so the uninitialized-client throw takes the same rejection path (F4). The former "skips the check when no session" test now asserts rejection, and there is a new case for the throwing client — both use a granted address so they prove the missing session alone causes the rejection.

On the test nit: await requestApproval on the accept cases would deadlock (the promise only settles via onApprove/onReject), so the accept assertions now flush pending tasks before asserting navigation instead — same effect, they fail loudly if navigation ever moves behind an await.

[B0Y3R-AVA]

F3 (low severity, pre-existing — follow-up): .toLowerCase() on both sides is correct for EVM hex but semantically wrong for case-sensitive base58 (Solana / legacy Bitcoin). Not exploitable — address here is a real wallet-derived signer, so a case-fold collision between two distinct owned keys is cryptographically negligible — and this matches the pre-migration behavior. Worth a follow-up to make matching namespace-aware (exact match for non-eip155, case-insensitive only for hex).

[bogdandobritoiu]

Agreed it is not exploitable and matches pre-migration behavior, so keeping the case-insensitive match in this PR to stay a pure restoration. Namespace-aware matching (exact for non-eip155, case-insensitive for hex) noted as a follow-up.

[bogdandobritoiu]

Update: fixed in this PR after all (056b381) after Copilot re-flagged the same thing — isAddressApproved is now namespace-aware (case-insensitive only for eip155, exact match otherwise) with a Solana case-variant regression test.

[bogdandobritoiu]

Re F2 (injected in-app browser exclusion) — traced the injected path to confirm a site cannot supply a non-active/ungranted signer:

• The injected router runs its own per-request gate before anything reaches ApprovalController: requestedSignerAddresses() extracts the dApp-named signer per method (eth_sendTransaction → params[0].from, personal_sign/typed-data → the address param) and dispatchSigningRequest rejects with unauthorized (no prompt) unless every requested signer is in getGrantedAddresses(origin) — app/hooks/browser/injectedProvider/router.ts:74-94, 318-342. Granted-but-non-active is intentionally allowed (multi-account); ungranted never signs. Covered by router.test.ts:1351-1497 including the exact from-substitution attack and the Permit/off-chain class for personal_sign/eth_signTypedData_v4.
• eth_sendTransactionBatch deliberately gates on the active account instead of from (the batch handler signs with the active index and ignores per-tx from, so gating on from would be bypassable) — router.ts:65-69, tests at router.test.ts:1407, 1430.
• Solana has no injected path at all: approvalMethods.ts only classifies eth_/personal_/avalanche_ as signing methods, so solana_* falls through to the read-only dispatcher and dies with methodNotFound.
• avalanche_* methods are first-party-origin-only and always sign with the active account (router.ts:919-929).

So the CORE_MOBILE_TOPIC exclusion in this PR rests on a gate that exists and is tested, not just on convention.

F1/F4 are fixed in 0efb698 (fail closed on missing session + guarded getSession); F3 stays as a follow-up per the inline thread.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.