From 5f39dbfa100cf058781f9173dd5079caa623d3c9 Mon Sep 17 00:00:00 2001 From: Bogdan Dobritoiu Date: Mon, 20 Jul 2026 19:26:23 +0300 Subject: [PATCH 1/4] restoring WalletConnect session-account authorization so dApps can only sign with granted accounts. --- .../isAccountApproved.test.ts | 92 +++++++- .../isAccountApproved/isAccountApproved.ts | 26 ++- .../ApprovalController.test.ts | 200 ++++++++++++++++++ .../ApprovalController/ApprovalController.ts | 31 +++ 4 files changed, 341 insertions(+), 8 deletions(-) diff --git a/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.test.ts b/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.test.ts index 5f6566e66d..1813815cd7 100644 --- a/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.test.ts +++ b/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.test.ts @@ -1,4 +1,94 @@ -import { isAccountApproved } from './isAccountApproved' +import { isAccountApproved, isAddressApproved } from './isAccountApproved' + +describe('isAddressApproved', () => { + const namespaces = { + eip155: { + accounts: [ + 'eip155:43113:0xC7E5ffBd7843EdB88cCB2ebaECAa07EC55c65318', + 'eip155:43114:0xcA0E993876152ccA6053eeDFC753092c8cE712D0' + ], + methods: ['eth_sendTransaction'], + events: ['chainChanged', 'accountsChanged'] + }, + solana: { + accounts: [ + 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp:9gQmZ7fTTgv5hVScrr9QqT6SpBs7i4cKLDdj4tuae3sW' + ], + methods: ['solana_signTransaction'], + events: [] + } + } + + it('returns true when the address is granted in the namespace', () => { + expect( + isAddressApproved( + '0xcA0E993876152ccA6053eeDFC753092c8cE712D0', + 'eip155:43114', + namespaces + ) + ).toBe(true) + }) + + it('matches addresses granted under another chain of the same namespace', () => { + expect( + isAddressApproved( + '0xC7E5ffBd7843EdB88cCB2ebaECAa07EC55c65318', + 'eip155:43114', + namespaces + ) + ).toBe(true) + }) + + it('matches case-insensitively', () => { + expect( + isAddressApproved( + '0xca0e993876152cca6053eedfc753092c8ce712d0', + 'eip155:43114', + namespaces + ) + ).toBe(true) + }) + + it('returns false when the address is not granted', () => { + expect( + isAddressApproved( + '0x341b0073b66bfc19FCB54308861f604F5Eb8f51b', + 'eip155:43114', + namespaces + ) + ).toBe(false) + }) + + it('returns true for a granted Solana account', () => { + expect( + isAddressApproved( + '9gQmZ7fTTgv5hVScrr9QqT6SpBs7i4cKLDdj4tuae3sW', + 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp', + namespaces + ) + ).toBe(true) + }) + + it('returns false for a Solana account that was never granted', () => { + expect( + isAddressApproved( + '4Nd1mYQZ6X8jY6nWn6iVrDpcLzJq9z2NKV1S1nGiqNz1', + 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp', + namespaces + ) + ).toBe(false) + }) + + it('returns false when the session has no namespace for the chain', () => { + expect( + isAddressApproved( + 'bc1qxy2kgdygjrsqtzq2n0yrf2493p83kkfjhx0wlh', + 'bip122:000000000019d6689c085ae165831e93', + namespaces + ) + ).toBe(false) + }) +}) describe('isAccountApproved', () => { it('should return true if address is approved', () => { diff --git a/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.ts b/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.ts index 242849cbfa..ed22addb60 100644 --- a/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.ts +++ b/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.ts @@ -4,12 +4,13 @@ import { getAddressForChainId } from 'store/rpc/handlers/wc_sessionRequest/utils' -export const isAccountApproved = ( - account: CoreAccountAddresses, +// A granted account under ANY chain of the request's namespace authorizes the +// address — sessions grant per-namespace account access, not per-chain. +export const isAddressApproved = ( + address: string, caip2ChainId: string, namespaces: SessionTypes.Namespaces ): boolean => { - const address = getAddressForChainId(caip2ChainId, account) const namespace = caip2ChainId.split(':')[0] if (!namespace || !namespaces[namespace]) { @@ -17,9 +18,20 @@ export const isAccountApproved = ( } return Boolean( - address && - namespaces[namespace]?.accounts.some( - acc => acc.split(':')[2]?.toLowerCase() === address.toLowerCase() - ) + namespaces[namespace]?.accounts.some( + acc => acc.split(':')[2]?.toLowerCase() === address.toLowerCase() + ) + ) +} + +export const isAccountApproved = ( + account: CoreAccountAddresses, + caip2ChainId: string, + namespaces: SessionTypes.Namespaces +): boolean => { + const address = getAddressForChainId(caip2ChainId, account) + + return Boolean( + address && isAddressApproved(address, caip2ChainId, namespaces) ) } diff --git a/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts b/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts index 6e1867af47..bf611dff20 100644 --- a/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts +++ b/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts @@ -70,6 +70,10 @@ jest.mock('services/ledger/LedgerService', () => ({ __esModule: true, default: { disconnect: jest.fn() } })) +jest.mock('services/walletconnectv2/WalletConnectService', () => ({ + __esModule: true, + default: { getSession: jest.fn() } +})) jest.mock( 'services/walletconnectv2/walletConnectCache/walletConnectCache', () => { @@ -160,6 +164,9 @@ const mockGetPublicKeyFor = jest.requireMock('services/wallet/WalletService') .default.getPublicKeyFor as jest.Mock const mockDisconnect = jest.requireMock('services/ledger/LedgerService').default .disconnect as jest.Mock +const mockGetSession = jest.requireMock( + 'services/walletconnectv2/WalletConnectService' +).default.getSession as jest.Mock global.confetti = { restart: jest.fn() } as unknown as typeof global.confetti @@ -1088,6 +1095,199 @@ describe('ApprovalController', () => { }) }) + // ── session account authorization (CP-14604) ───────────────────────────── + + describe('session account authorization', () => { + const displayData = {} as never + const SOLANA_CHAIN_ID = 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp' + const GRANTED_SVM_ADDRESS = '9gQmZ7fTTgv5hVScrr9QqT6SpBs7i4cKLDdj4tuae3sW' + const UNGRANTED_SVM_ADDRESS = '4Nd1mYQZ6X8jY6nWn6iVrDpcLzJq9z2NKV1S1nGiqNz1' + + const makeSolanaSigningData = (account: string) => + ({ + type: RpcMethod.SOLANA_SIGN_TRANSACTION, + account, + data: 'serialized-tx' + } as never) + + const sessionWithGrantedAccounts = { + namespaces: { + solana: { + accounts: [`${SOLANA_CHAIN_ID}:${GRANTED_SVM_ADDRESS}`], + methods: ['solana_signTransaction'], + events: [] + } + } + } as never + + it('rejects when the signing account was never granted to the WC session', async () => { + mockGetSession.mockReturnValue(sessionWithGrantedAccounts) + + const request = makeDappRequest( + RpcMethod.SOLANA_SIGN_TRANSACTION, + SOLANA_CHAIN_ID + ) + + const result = await approvalController.requestApproval({ + request, + displayData, + signingData: makeSolanaSigningData(UNGRANTED_SVM_ADDRESS) + }) + + expect(mockGetSession).toHaveBeenCalledWith(DAPP_SESSION_ID) + expect('error' in result && result.error).toMatchObject({ + message: 'Requested address is not authorized' + }) + expect(mockRouter.navigate).not.toHaveBeenCalled() + expect(mockWalletConnectCacheSet).not.toHaveBeenCalled() + }) + + it('shows the approval screen when the signing account is granted to the WC session', () => { + mockGetSession.mockReturnValue(sessionWithGrantedAccounts) + + const request = makeDappRequest( + RpcMethod.SOLANA_SIGN_TRANSACTION, + SOLANA_CHAIN_ID + ) + + approvalController.requestApproval({ + request, + displayData, + signingData: makeSolanaSigningData(GRANTED_SVM_ADDRESS) + }) + + expect(mockRouter.navigate).toHaveBeenCalledWith( + expect.objectContaining({ pathname: '/approval' }) + ) + }) + + it('never queries the WC client for in-app / injected-browser requests (WC may be uninitialized)', () => { + const request = makeInjectedDappRequest( + RpcMethod.SOLANA_SIGN_TRANSACTION, + SOLANA_CHAIN_ID + ) + + approvalController.requestApproval({ + request, + displayData, + signingData: makeSolanaSigningData(UNGRANTED_SVM_ADDRESS) + }) + + expect(mockGetSession).not.toHaveBeenCalled() + expect(mockRouter.navigate).toHaveBeenCalledWith( + expect.objectContaining({ pathname: '/approval' }) + ) + }) + + it('skips the check when no WC session exists for the topic', () => { + mockGetSession.mockReturnValue(undefined) + + const request = makeDappRequest( + RpcMethod.SOLANA_SIGN_TRANSACTION, + SOLANA_CHAIN_ID + ) + + approvalController.requestApproval({ + request, + displayData, + signingData: makeSolanaSigningData(UNGRANTED_SVM_ADDRESS) + }) + + expect(mockRouter.navigate).toHaveBeenCalledWith( + expect.objectContaining({ pathname: '/approval' }) + ) + }) + + it('rejects an EVM transaction whose from-address was never granted to the WC session', async () => { + mockGetSession.mockReturnValue({ + namespaces: { + eip155: { + accounts: [`eip155:43114:${EVM_ADDRESS}`], + methods: ['eth_sendTransaction'], + events: [] + } + } + } as never) + + const request = makeDappRequest( + RpcMethod.ETH_SEND_TRANSACTION, + 'eip155:43114' + ) + + const result = await approvalController.requestApproval({ + request, + displayData, + signingData: { + type: RpcMethod.ETH_SEND_TRANSACTION, + account: '0x341b0073b66bfc19FCB54308861f604F5Eb8f51b', + data: {} + } as never + }) + + expect('error' in result && result.error).toMatchObject({ + message: 'Requested address is not authorized' + }) + expect(mockRouter.navigate).not.toHaveBeenCalled() + }) + + it('accepts an EVM address granted under a different chain of the eip155 namespace', () => { + mockGetSession.mockReturnValue({ + namespaces: { + eip155: { + accounts: [`eip155:1:${EVM_ADDRESS}`], + methods: ['eth_sendTransaction'], + events: [] + } + } + } as never) + + const request = makeDappRequest( + RpcMethod.ETH_SEND_TRANSACTION, + 'eip155:43114' + ) + + approvalController.requestApproval({ + request, + displayData, + signingData: { + type: RpcMethod.ETH_SEND_TRANSACTION, + // lowercased vs the checksummed EVM_ADDRESS in the session grant — + // dApp-supplied casing must not defeat authorization + account: EVM_ADDRESS.toLowerCase(), + data: {} + } as never + }) + + expect(mockRouter.navigate).toHaveBeenCalledWith( + expect.objectContaining({ pathname: '/approval' }) + ) + }) + + it('skips the check for signing data that carries no account (avalanche transactions)', () => { + mockGetSession.mockReturnValue(sessionWithGrantedAccounts) + + const request = makeDappRequest( + RpcMethod.AVALANCHE_SEND_TRANSACTION, + AvalancheCaip2ChainId.P + ) + + approvalController.requestApproval({ + request, + displayData, + signingData: { + type: RpcMethod.AVALANCHE_SEND_TRANSACTION, + unsignedTxJson: '{}', + data: {}, + vm: 'PVM' + } as never + }) + + expect(mockRouter.navigate).toHaveBeenCalledWith( + expect.objectContaining({ pathname: '/approval' }) + ) + }) + }) + // ── requestApproval ─────────────────────────────────────────────────────── describe('requestApproval', () => { diff --git a/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts b/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts index c62575b40b..7375c4ff94 100644 --- a/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts +++ b/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts @@ -33,6 +33,9 @@ import WalletService from 'services/wallet/WalletService' import { Curve } from 'utils/publicKeys' import { ledgerParamsStore } from 'features/ledger/store' import { OnApproveParams } from 'services/walletconnectv2/walletConnectCache/types' +import WalletConnectService from 'services/walletconnectv2/WalletConnectService' +import { providerErrors } from '@metamask/rpc-errors' +import { isAddressApproved } from 'store/rpc/utils/isAccountApproved/isAccountApproved' import { WalletType } from 'services/wallet/types' import { promptForAppReviewAfterSuccessfulTransaction } from 'features/appReview/utils/promptForAppReviewAfterSuccessfulTransaction' import { CONFETTI_DURATION_MS } from 'common/consts' @@ -538,6 +541,34 @@ class ApprovalController implements VmModuleApprovalController { const requestId = request.requestId this.userCancelledMap.delete(requestId) + // A dApp may only sign with accounts the user granted to its WalletConnect + // session (session.namespaces) — enforced pre-vm-module via + // isAddressApproved and lost in the migration (CP-14604). signingData + // .account is the module-resolved signer, so this checks exactly the + // address that would sign. In-app and injected-browser requests + // (CORE_MOBILE_TOPIC) are excluded: they keep their own account gating, + // and getSession would throw while the WC client is still initializing. + // Avalanche signingData variants carry no `account` and are knowingly not + // checked: the avax namespace is only ever granted to Core-domain dApps, + // and those methods always sign with the active account the user sees. + 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' + ) + } + } + } + // Quick Swaps bypass — sync find then async run keeps the common // (no-validator) path microtask-free so the modal navigation // below happens on the same tick callers observe. From 0efb698ce6853d1f447697799090c777b79a0e39 Mon Sep 17 00:00:00 2001 From: Bogdan Dobritoiu Date: Wed, 22 Jul 2026 14:24:28 +0300 Subject: [PATCH 2/4] CP-14604: fail closed when the WC session is missing or the client throws MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up (PR #3994 F1/F4): a session_delete/expiry landing between the WC request event and requestApproval made getSession return undefined, which skipped the authorization gate entirely. Reject instead — the only non-in-app path is live WalletConnect, so a missing session means there are no grants to honor and the response is undeliverable anyway. getSession is also guarded against the uninitialized-client throw (same fail-closed path). Accept-path tests now flush pending tasks before asserting navigation so they keep failing loudly if navigation ever moves behind an await. Co-Authored-By: Claude Fable 5 --- .../ApprovalController.test.ts | 53 +++++++++++++++---- .../ApprovalController/ApprovalController.ts | 13 ++++- 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts b/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts index bf611dff20..9dcf99bbd6 100644 --- a/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts +++ b/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts @@ -1099,6 +1099,14 @@ describe('ApprovalController', () => { describe('session account authorization', () => { const displayData = {} as never + + // Accept-path assertions flush pending (micro)tasks first so they keep + // failing loudly if navigation ever moves behind an await inside + // requestApproval (the returned promise itself only settles on + // approve/reject, so it can't be awaited here). Matches the helper in + // the cancel-bridge describe block. + const flushMicrotasks = (): Promise => + new Promise(resolve => setTimeout(resolve, 0)) const SOLANA_CHAIN_ID = 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp' const GRANTED_SVM_ADDRESS = '9gQmZ7fTTgv5hVScrr9QqT6SpBs7i4cKLDdj4tuae3sW' const UNGRANTED_SVM_ADDRESS = '4Nd1mYQZ6X8jY6nWn6iVrDpcLzJq9z2NKV1S1nGiqNz1' @@ -1142,7 +1150,7 @@ describe('ApprovalController', () => { expect(mockWalletConnectCacheSet).not.toHaveBeenCalled() }) - it('shows the approval screen when the signing account is granted to the WC session', () => { + it('shows the approval screen when the signing account is granted to the WC session', async () => { mockGetSession.mockReturnValue(sessionWithGrantedAccounts) const request = makeDappRequest( @@ -1155,13 +1163,14 @@ describe('ApprovalController', () => { displayData, signingData: makeSolanaSigningData(GRANTED_SVM_ADDRESS) }) + await flushMicrotasks() expect(mockRouter.navigate).toHaveBeenCalledWith( expect.objectContaining({ pathname: '/approval' }) ) }) - it('never queries the WC client for in-app / injected-browser requests (WC may be uninitialized)', () => { + it('never queries the WC client for in-app / injected-browser requests (WC may be uninitialized)', async () => { const request = makeInjectedDappRequest( RpcMethod.SOLANA_SIGN_TRANSACTION, SOLANA_CHAIN_ID @@ -1172,6 +1181,7 @@ describe('ApprovalController', () => { displayData, signingData: makeSolanaSigningData(UNGRANTED_SVM_ADDRESS) }) + await flushMicrotasks() expect(mockGetSession).not.toHaveBeenCalled() expect(mockRouter.navigate).toHaveBeenCalledWith( @@ -1179,7 +1189,7 @@ describe('ApprovalController', () => { ) }) - it('skips the check when no WC session exists for the topic', () => { + it('rejects (fails closed) when no WC session exists for the topic', async () => { mockGetSession.mockReturnValue(undefined) const request = makeDappRequest( @@ -1187,15 +1197,38 @@ describe('ApprovalController', () => { SOLANA_CHAIN_ID ) - approvalController.requestApproval({ + const result = await approvalController.requestApproval({ request, displayData, - signingData: makeSolanaSigningData(UNGRANTED_SVM_ADDRESS) + signingData: makeSolanaSigningData(GRANTED_SVM_ADDRESS) }) - expect(mockRouter.navigate).toHaveBeenCalledWith( - expect.objectContaining({ pathname: '/approval' }) + expect('error' in result && result.error).toMatchObject({ + message: 'Requested address is not authorized' + }) + expect(mockRouter.navigate).not.toHaveBeenCalled() + }) + + it('rejects (fails closed) when the WC client is not initialized (getSession throws)', async () => { + mockGetSession.mockImplementation(() => { + throw new Error('WalletConnect client is not initialized') + }) + + const request = makeDappRequest( + RpcMethod.SOLANA_SIGN_TRANSACTION, + SOLANA_CHAIN_ID ) + + const result = await approvalController.requestApproval({ + request, + displayData, + signingData: makeSolanaSigningData(GRANTED_SVM_ADDRESS) + }) + + expect('error' in result && result.error).toMatchObject({ + message: 'Requested address is not authorized' + }) + expect(mockRouter.navigate).not.toHaveBeenCalled() }) it('rejects an EVM transaction whose from-address was never granted to the WC session', async () => { @@ -1230,7 +1263,7 @@ describe('ApprovalController', () => { expect(mockRouter.navigate).not.toHaveBeenCalled() }) - it('accepts an EVM address granted under a different chain of the eip155 namespace', () => { + it('accepts an EVM address granted under a different chain of the eip155 namespace', async () => { mockGetSession.mockReturnValue({ namespaces: { eip155: { @@ -1257,13 +1290,14 @@ describe('ApprovalController', () => { data: {} } as never }) + await flushMicrotasks() expect(mockRouter.navigate).toHaveBeenCalledWith( expect.objectContaining({ pathname: '/approval' }) ) }) - it('skips the check for signing data that carries no account (avalanche transactions)', () => { + it('skips the check for signing data that carries no account (avalanche transactions)', async () => { mockGetSession.mockReturnValue(sessionWithGrantedAccounts) const request = makeDappRequest( @@ -1281,6 +1315,7 @@ describe('ApprovalController', () => { vm: 'PVM' } as never }) + await flushMicrotasks() expect(mockRouter.navigate).toHaveBeenCalledWith( expect.objectContaining({ pathname: '/approval' }) diff --git a/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts b/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts index 7375c4ff94..387c2b62cb 100644 --- a/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts +++ b/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts @@ -551,10 +551,19 @@ class ApprovalController implements VmModuleApprovalController { // Avalanche signingData variants carry no `account` and are knowingly not // checked: the avax namespace is only ever granted to Core-domain dApps, // and those methods always sign with the active account the user sees. + // Fails closed: a request with no live session (deleted/expired mid-flight, + // or WC uninitialized) has no grants to check against, so it's rejected — + // its response is undeliverable anyway. if ('account' in signingData && !isInAppRequest(request)) { - const session = WalletConnectService.getSession(request.sessionId) + const session = (() => { + try { + return WalletConnectService.getSession(request.sessionId) + } catch { + return undefined + } + })() if ( - session && + !session || !isAddressApproved( signingData.account, request.chainId, From 056b381cb97b469a08bdd3eb4556b87885c57ecb Mon Sep 17 00:00:00 2001 From: Bogdan Dobritoiu Date: Wed, 22 Jul 2026 17:33:55 +0300 Subject: [PATCH 3/4] CP-14604: make isAddressApproved namespace-aware (exact match for non-EVM addresses) Case-insensitive comparison is only correct for eip155 hex addresses (EIP-55 checksums vary casing); base58 Solana pubkeys are case-sensitive, so case variants must not be treated as authorized. Co-Authored-By: Claude Fable 5 --- .../isAccountApproved/isAccountApproved.test.ts | 12 +++++++++++- .../utils/isAccountApproved/isAccountApproved.ts | 14 +++++++++----- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.test.ts b/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.test.ts index 1813815cd7..8a6af42c5a 100644 --- a/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.test.ts +++ b/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.test.ts @@ -39,7 +39,7 @@ describe('isAddressApproved', () => { ).toBe(true) }) - it('matches case-insensitively', () => { + it('matches EVM addresses case-insensitively (EIP-55 checksum casing)', () => { expect( isAddressApproved( '0xca0e993876152cca6053eedfc753092c8ce712d0', @@ -79,6 +79,16 @@ describe('isAddressApproved', () => { ).toBe(false) }) + it('returns false for a case variant of a granted Solana account (base58 is case-sensitive)', () => { + expect( + isAddressApproved( + '9gqmz7fttgv5hvscrr9qqt6spbs7i4cklddj4tuae3sw', + 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp', + namespaces + ) + ).toBe(false) + }) + it('returns false when the session has no namespace for the chain', () => { expect( isAddressApproved( diff --git a/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.ts b/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.ts index ed22addb60..8a1cec408e 100644 --- a/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.ts +++ b/packages/core-mobile/app/store/rpc/utils/isAccountApproved/isAccountApproved.ts @@ -17,11 +17,15 @@ export const isAddressApproved = ( return false } - return Boolean( - namespaces[namespace]?.accounts.some( - acc => acc.split(':')[2]?.toLowerCase() === address.toLowerCase() - ) - ) + // Only EVM hex addresses are case-insensitive (EIP-55 checksums vary the + // casing); other namespaces (e.g. base58 Solana) must match exactly. + const matches = + namespace === 'eip155' + ? (acc: string): boolean => + acc.split(':')[2]?.toLowerCase() === address.toLowerCase() + : (acc: string): boolean => acc.split(':')[2] === address + + return Boolean(namespaces[namespace]?.accounts.some(matches)) } export const isAccountApproved = ( From 732958bc4a27aaf5434f8bc4807827a72479ca95 Mon Sep 17 00:00:00 2001 From: James Boyer Date: Thu, 30 Jul 2026 10:41:53 -0400 Subject: [PATCH 4/4] CP-14604: enforce WalletConnect account grant for avalanche_signMessage at dispatch and approval time --- .../request/handleRequestViaVMModule.ts | 37 +++- .../isAvalancheSignMessageAuthorized.test.ts | 166 ++++++++++++++++++ .../isAvalancheSignMessageAuthorized.ts | 78 ++++++++ .../ApprovalController.test.ts | 115 ++++++++++++ .../ApprovalController/ApprovalController.ts | 64 +++++-- 5 files changed, 446 insertions(+), 14 deletions(-) create mode 100644 packages/core-mobile/app/store/rpc/utils/isAvalancheSignMessageAuthorized/isAvalancheSignMessageAuthorized.test.ts create mode 100644 packages/core-mobile/app/store/rpc/utils/isAvalancheSignMessageAuthorized/isAvalancheSignMessageAuthorized.ts diff --git a/packages/core-mobile/app/store/rpc/listeners/request/handleRequestViaVMModule.ts b/packages/core-mobile/app/store/rpc/listeners/request/handleRequestViaVMModule.ts index 032ee88310..d9d98c6c5f 100644 --- a/packages/core-mobile/app/store/rpc/listeners/request/handleRequestViaVMModule.ts +++ b/packages/core-mobile/app/store/rpc/listeners/request/handleRequestViaVMModule.ts @@ -1,4 +1,4 @@ -import { rpcErrors } from '@metamask/rpc-errors' +import { rpcErrors, providerErrors } from '@metamask/rpc-errors' import { Module, RpcMethod as VmModuleRpcMethod @@ -11,8 +11,14 @@ import { mapToVmNetwork } from 'vmModule/utils/mapToVmNetwork' import { getChainIdFromCaip2 } from 'utils/caip2ChainIds' import { Avalanche } from '@avalabs/core-wallets-sdk' import { getAddressByVM } from 'store/account/utils' -import { Account, selectActiveAccount } from 'store/account' +import { + Account, + selectActiveAccount, + selectAccountByIndex +} from 'store/account' import { selectActiveWallet } from 'store/wallet/slice' +import WalletConnectService from 'services/walletconnectv2/WalletConnectService' +import { isAvalancheSignMessageAuthorized } from 'store/rpc/utils/isAvalancheSignMessageAuthorized/isAvalancheSignMessageAuthorized' import { WalletType } from 'services/wallet/types' import { selectIsDeveloperMode } from 'store/settings/advanced' import { @@ -100,6 +106,33 @@ export const handleRequestViaVMModule = async ({ const params = request.data.params.request.params const method = request.method as unknown as VmModuleRpcMethod + // avalanche_signMessage picks its signer by a dApp-supplied account index + // (params [message, accountIndex]), not a signingData.account address, so it + // slips past the ApprovalController grant check. Enforce the WalletConnect + // session grant here — on the same account the approval screen will sign with + // (selectAccountByIndex, or the active account when no index is given). No-op + // for every other method and for in-app / injected-browser requests; fails + // closed on a missing session. CP-14604. + if ( + !isAvalancheSignMessageAuthorized({ + method, + isInAppRequest: request.data.topic === CORE_MOBILE_TOPIC, + params, + caip2ChainId, + activeAccount, + getAccountByIndex: index => + selectAccountByIndex(activeWallet.id, index)(state), + getSession: () => WalletConnectService.getSession(request.data.topic) + }) + ) { + rpcProvider.onError({ + request, + error: providerErrors.unauthorized('Requested address is not authorized'), + listenerApi + }) + return + } + // Merge, don't fallback: a non-empty `request.context` from the caller must // not suppress the per-method auto-injected context (e.g. Avalanche `account` // for AVALANCHE_SEND/SIGN_TRANSACTION). Caller wins on key conflicts. diff --git a/packages/core-mobile/app/store/rpc/utils/isAvalancheSignMessageAuthorized/isAvalancheSignMessageAuthorized.test.ts b/packages/core-mobile/app/store/rpc/utils/isAvalancheSignMessageAuthorized/isAvalancheSignMessageAuthorized.test.ts new file mode 100644 index 0000000000..9a1f1d2ebf --- /dev/null +++ b/packages/core-mobile/app/store/rpc/utils/isAvalancheSignMessageAuthorized/isAvalancheSignMessageAuthorized.test.ts @@ -0,0 +1,166 @@ +import { AvalancheCaip2ChainId } from '@avalabs/core-chains-sdk' +import { RpcMethod } from '@avalabs/vm-module-types' +import { SessionTypes } from '@walletconnect/types' +import { CoreAccountAddresses } from 'store/rpc/handlers/wc_sessionRequest/utils' +import { isAvalancheSignMessageAuthorized } from './isAvalancheSignMessageAuthorized' + +const X = AvalancheCaip2ChainId.X + +const GRANTED_AVM = 'X-fuji1granted00000000000000000000000000000' +const UNGRANTED_AVM = 'X-fuji1ungranted000000000000000000000000000' + +const account = (addressAVM: string): CoreAccountAddresses => ({ + addressC: '0xc0000000000000000000000000000000000000c0', + addressBTC: 'bc1qbtc', + addressCoreEth: '0xcoreeth', + addressAVM, + addressPVM: 'P-fuji1pvm', + addressSVM: 'svmAddr' +}) + +const sessionGranting = (addresses: string[]): SessionTypes.Struct => + ({ + namespaces: { + avax: { + accounts: addresses.map(a => `${X}:${a}`), + methods: ['avalanche_signMessage'], + events: [] + } + } + } as unknown as SessionTypes.Struct) + +// Explicit index 1 → an ungranted account; anything else → a granted account. +const getAccountByIndex = (index: number): CoreAccountAddresses | undefined => + index === 1 ? account(UNGRANTED_AVM) : account(GRANTED_AVM) + +const base = { + method: RpcMethod.AVALANCHE_SIGN_MESSAGE as string, + isInAppRequest: false, + params: ['hello', 1] as unknown, + caip2ChainId: X as string, + activeAccount: account(GRANTED_AVM), + getAccountByIndex, + getSession: (): SessionTypes.Struct | undefined => + sessionGranting([GRANTED_AVM]) +} + +describe('isAvalancheSignMessageAuthorized', () => { + it('is a no-op (authorized) for any method other than avalanche_signMessage', () => { + expect( + isAvalancheSignMessageAuthorized({ + ...base, + method: RpcMethod.ETH_SIGN, + // would fail closed if the guard applied, proving it is skipped + getSession: () => { + throw new Error('should not be consulted') + } + }) + ).toBe(true) + }) + + it('is a no-op (authorized) for in-app / injected-browser requests', () => { + expect( + isAvalancheSignMessageAuthorized({ + ...base, + isInAppRequest: true, + getSession: () => { + throw new Error('should not be consulted') + } + }) + ).toBe(true) + }) + + it('rejects when the dApp-supplied account index resolves to an ungranted account', () => { + // params [message, 1] → getAccountByIndex(1) → UNGRANTED_AVM + expect(isAvalancheSignMessageAuthorized(base)).toBe(false) + }) + + it('allows when the dApp-supplied account index resolves to a granted account', () => { + expect( + isAvalancheSignMessageAuthorized({ ...base, params: ['hello', 0] }) + ).toBe(true) + }) + + it('falls back to the active account when no index is supplied (active granted → allow)', () => { + expect( + isAvalancheSignMessageAuthorized({ ...base, params: ['hello'] }) + ).toBe(true) + }) + + it('falls back to the active account when no index is supplied (active ungranted → reject)', () => { + expect( + isAvalancheSignMessageAuthorized({ + ...base, + params: ['hello'], + activeAccount: account(UNGRANTED_AVM) + }) + ).toBe(false) + }) + + it('fails closed when no WC session exists for the topic', () => { + expect( + isAvalancheSignMessageAuthorized({ + ...base, + params: ['hello', 0], + getSession: () => undefined + }) + ).toBe(false) + }) + + it('fails closed when getSession throws (WC client not initialized)', () => { + expect( + isAvalancheSignMessageAuthorized({ + ...base, + params: ['hello', 0], + getSession: () => { + throw new Error('WalletConnect client is not initialized') + } + }) + ).toBe(false) + }) + + it('rejects a fractional index whose accounts[0] fallback is ungranted (schema allows non-integers)', () => { + // 0.5 passes the module schema (nonnegative, not .int()); selectAccountByIndex + // finds no match and signs with accounts[0]. The guard must check that same + // resolved account, not the (granted) active account — else the bypass reopens. + expect( + isAvalancheSignMessageAuthorized({ + ...base, + params: ['hello', 0.5], + getAccountByIndex: () => account(UNGRANTED_AVM) + }) + ).toBe(false) + }) + + it('allows a fractional index only when its resolved account is granted', () => { + expect( + isAvalancheSignMessageAuthorized({ + ...base, + params: ['hello', 0.5], + getAccountByIndex: () => account(GRANTED_AVM) + }) + ).toBe(true) + }) + + it('fails closed when the account index resolves to no account', () => { + expect( + isAvalancheSignMessageAuthorized({ + ...base, + params: ['hello', 5], + getAccountByIndex: () => undefined + }) + ).toBe(false) + }) + + it('treats an invalid (negative) index as absent and checks the active account', () => { + // module schema rejects negatives; our resolution mirrors the "no index" + // path (active account) rather than trusting a bogus dApp value + expect( + isAvalancheSignMessageAuthorized({ + ...base, + params: ['hello', -1], + activeAccount: account(GRANTED_AVM) + }) + ).toBe(true) + }) +}) diff --git a/packages/core-mobile/app/store/rpc/utils/isAvalancheSignMessageAuthorized/isAvalancheSignMessageAuthorized.ts b/packages/core-mobile/app/store/rpc/utils/isAvalancheSignMessageAuthorized/isAvalancheSignMessageAuthorized.ts new file mode 100644 index 0000000000..e3c1ed488f --- /dev/null +++ b/packages/core-mobile/app/store/rpc/utils/isAvalancheSignMessageAuthorized/isAvalancheSignMessageAuthorized.ts @@ -0,0 +1,78 @@ +import { RpcMethod } from '@avalabs/vm-module-types' +import { SessionTypes } from '@walletconnect/types' +import { CoreAccountAddresses } from 'store/rpc/handlers/wc_sessionRequest/utils' +import { isAccountApproved } from 'store/rpc/utils/isAccountApproved/isAccountApproved' + +/** + * Request-time WalletConnect account authorization for `avalanche_signMessage` + * (CP-14604 follow-up). + * + * The ApprovalController grant check runs on `signingData.account` — the + * module-resolved signer address. `avalanche_signMessage` is the one signing + * method whose signer is chosen by a dApp-supplied account *index* + * (`params: [message, accountIndex]`) instead of an address, so its signingData + * carries `accountIndex`, not `account`, and the ApprovalController check never + * sees it. Without this, a dApp granted account A could request a signature from + * account B of the same wallet. + * + * This resolves the signer the same way the approval screen does + * (`getAccountSelector`): an explicit non-negative index selects that account + * via `selectAccountByIndex`; otherwise the active account signs. It then checks + * the resolved account against the session's granted accounts. + * + * Returns `true` (authorized / no-op) for every other method and for in-app / + * injected-browser requests, so callers can invoke it unconditionally. Fails + * closed: a missing WC session (deleted, expired, or client uninitialized) has + * no grant to check against and is treated as unauthorized. + */ +export const isAvalancheSignMessageAuthorized = ({ + method, + isInAppRequest, + params, + caip2ChainId, + activeAccount, + getAccountByIndex, + getSession +}: { + method: string + isInAppRequest: boolean + params: unknown + caip2ChainId: string + activeAccount: CoreAccountAddresses + getAccountByIndex: (index: number) => CoreAccountAddresses | undefined + getSession: () => SessionTypes.Struct | undefined +}): boolean => { + if (isInAppRequest || method !== RpcMethod.AVALANCHE_SIGN_MESSAGE) { + return true + } + + // Mirror the avalanche-module param shape ([message, accountIndex]). Its + // schema is z.number().nonnegative() — NOT .int() — so a fractional index + // (e.g. 0.5) is accepted and passed through to signingData.accountIndex, and + // both the approval screen and the signer resolve it via selectAccountByIndex, + // which finds no match and falls back to accounts[0]. We must therefore treat + // ANY non-negative finite number as an explicit index and resolve it the SAME + // way (getAccountByIndex), so the account we authorize is byte-for-byte the + // account that will sign. Checking the active account for a fractional index + // instead would reopen the exact bypass this guards against. A negative / NaN + // / non-numeric value is rejected by the module schema before signing, so we + // treat it as "no index" (active account) — inert either way. + const index = Array.isArray(params) ? params[1] : undefined + const hasExplicitIndex = + typeof index === 'number' && Number.isFinite(index) && index >= 0 + + const account = hasExplicitIndex ? getAccountByIndex(index) : activeAccount + + let session: SessionTypes.Struct | undefined + try { + session = getSession() + } catch { + session = undefined + } + + if (!account || !session) { + return false + } + + return isAccountApproved(account, caip2ChainId, session.namespaces) +} diff --git a/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts b/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts index 9dcf99bbd6..02e681d002 100644 --- a/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts +++ b/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.test.ts @@ -1321,6 +1321,121 @@ describe('ApprovalController', () => { expect.objectContaining({ pathname: '/approval' }) ) }) + + // avalanche_signMessage's signer is resolved from LIVE state at approval + // time, so the request-time gate (in handleRequestViaVMModule) can be + // outrun by a mid-flight account/wallet switch. ApprovalController re-checks + // the account that is actually about to sign. CP-14604. + describe('avalanche_signMessage approval-time recheck (TOCTOU)', () => { + const X = AvalancheCaip2ChainId.X + const GRANTED_AVM = 'X-fuji1granted00000000000000000000000000000' + const UNGRANTED_AVM = 'X-fuji1ungranted000000000000000000000000000' + + const avmAccount = (addressAVM: string): never => + ({ + addressC: '0xc0000000000000000000000000000000000000c0', + addressBTC: 'bc1qbtc', + addressCoreEth: '0xcoreeth', + addressAVM, + index: 1 + } as never) + + const avaxSessionGranting = (addressAVM: string): never => + ({ + namespaces: { + avax: { + accounts: [`${X}:${addressAVM}`], + methods: ['avalanche_signMessage'], + events: [] + } + } + } as never) + + const signMessageSigningData = { + type: RpcMethod.AVALANCHE_SIGN_MESSAGE, + data: 'deadbeef', + accountIndex: 1 + } as never + + const captureOnApprove = (): ((params: unknown) => Promise) => + mockWalletConnectCacheSet.mock.calls[ + mockWalletConnectCacheSet.mock.calls.length - 1 + ][0].onApprove + + it('rejects at approval time when the live signer is not granted', async () => { + mockGetSession.mockReturnValue(avaxSessionGranting(GRANTED_AVM)) + // getAddressForChainId is mocked in this suite — resolve the live + // signer to its (ungranted) X-chain address so real grant matching runs + mockGetAddressForChainId.mockReturnValue(UNGRANTED_AVM) + + const resultPromise = approvalController.requestApproval({ + request: makeDappRequest(RpcMethod.AVALANCHE_SIGN_MESSAGE, X), + displayData, + signingData: signMessageSigningData + }) + await flushMicrotasks() + + // user switched to an ungranted account before tapping Approve + await captureOnApprove()({ + walletType: WalletType.MNEMONIC, + walletId: 'w1', + account: avmAccount(UNGRANTED_AVM) + }) + + const result = await resultPromise + expect('error' in result && result.error).toMatchObject({ + message: 'Requested address is not authorized' + }) + expect(mockOnApprove).not.toHaveBeenCalled() + }) + + it('signs when the live signer is granted', async () => { + mockGetSession.mockReturnValue(avaxSessionGranting(GRANTED_AVM)) + mockGetAddressForChainId.mockReturnValue(GRANTED_AVM) + + approvalController.requestApproval({ + request: makeDappRequest(RpcMethod.AVALANCHE_SIGN_MESSAGE, X), + displayData, + signingData: signMessageSigningData + }) + await flushMicrotasks() + + await captureOnApprove()({ + walletType: WalletType.MNEMONIC, + walletId: 'w1', + account: avmAccount(GRANTED_AVM) + }) + + expect(mockOnApprove).toHaveBeenCalledWith( + expect.objectContaining({ signingData: signMessageSigningData }) + ) + }) + + it('fails closed when the session is gone by approval time', async () => { + mockGetSession.mockReturnValue(avaxSessionGranting(GRANTED_AVM)) + mockGetAddressForChainId.mockReturnValue(GRANTED_AVM) + + const resultPromise = approvalController.requestApproval({ + request: makeDappRequest(RpcMethod.AVALANCHE_SIGN_MESSAGE, X), + displayData, + signingData: signMessageSigningData + }) + await flushMicrotasks() + + mockGetSession.mockReturnValue(undefined) // dropped mid-flight + await captureOnApprove()({ + walletType: WalletType.MNEMONIC, + walletId: 'w1', + account: avmAccount(GRANTED_AVM) + }) + + const result = await resultPromise + expect('error' in result && result.error).toMatchObject({ + message: 'Requested address is not authorized' + }) + expect(mockOnApprove).not.toHaveBeenCalled() + }) + }) }) // ── requestApproval ─────────────────────────────────────────────────────── diff --git a/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts b/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts index 387c2b62cb..a4480d80aa 100644 --- a/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts +++ b/packages/core-mobile/app/vmModule/ApprovalController/ApprovalController.ts @@ -7,7 +7,8 @@ import { BatchApprovalParams, BatchApprovalResponse, RpcRequest, - RequestPublicKeyParams + RequestPublicKeyParams, + RpcMethod } from '@avalabs/vm-module-types' import { walletConnectCache } from 'services/walletconnectv2/walletConnectCache/walletConnectCache' import { transactionSnackbar } from 'new/common/utils/toast' @@ -35,7 +36,10 @@ import { ledgerParamsStore } from 'features/ledger/store' import { OnApproveParams } from 'services/walletconnectv2/walletConnectCache/types' import WalletConnectService from 'services/walletconnectv2/WalletConnectService' import { providerErrors } from '@metamask/rpc-errors' -import { isAddressApproved } from 'store/rpc/utils/isAccountApproved/isAccountApproved' +import { + isAddressApproved, + isAccountApproved +} from 'store/rpc/utils/isAccountApproved/isAccountApproved' import { WalletType } from 'services/wallet/types' import { promptForAppReviewAfterSuccessfulTransaction } from 'features/appReview/utils/promptForAppReviewAfterSuccessfulTransaction' import { CONFETTI_DURATION_MS } from 'common/consts' @@ -355,6 +359,19 @@ class ApprovalController implements VmModuleApprovalController { this.activeApprovals.delete(requestId) } + // WalletConnect session for a request's topic, or undefined. getSession throws + // while the WC client is still initializing (and returns undefined for an + // unknown/expired topic); both collapse to undefined so callers fail closed. + private getSessionOrUndefined( + sessionId: string + ): ReturnType { + try { + return WalletConnectService.getSession(sessionId) + } catch { + return undefined + } + } + // Cancel a parked approval whose request was aborted. Runs the REAL reject so // the orphaned promise settles and Ledger BLE + store are torn down; once // on-device Ledger signing has begun (`ledgerSigning`) the cancel is a no-op. @@ -507,6 +524,33 @@ class ApprovalController implements VmModuleApprovalController { // up; a late Approve tap must not sign/broadcast. (CP-14422) if (this.userCancelledMap.get(requestId)) return + // avalanche_signMessage picks its signer from LIVE state at approval time + // (getAccountSelector → active account or dApp-supplied index), so re-verify + // the account that is actually about to sign against the WC session grant. + // The request-time gate in handleRequestViaVMModule checks dispatch-time + // state and can be outrun by a mid-flight account/wallet switch; every other + // method pins its signer as signingData.account (already grant-checked in + // requestApproval). In-app requests keep their own gating; fails closed on a + // missing session. CP-14604. + if ( + signingData.type === RpcMethod.AVALANCHE_SIGN_MESSAGE && + !isInAppRequest(request) + ) { + const session = this.getSessionOrUndefined(request.sessionId) + if ( + !session || + !isAccountApproved(params.account, request.chainId, session.namespaces) + ) { + this.clearCancelBridge(requestId) + resolve({ + error: providerErrors.unauthorized( + 'Requested address is not authorized' + ) + }) + return + } + } + // Cache the actual selected signer so onTransactionConfirmed / // onTransactionReverted emit the real address for dApp txs — including // the injected browser (which the !isInAppRequest gate excluded, @@ -548,20 +592,16 @@ class ApprovalController implements VmModuleApprovalController { // address that would sign. In-app and injected-browser requests // (CORE_MOBILE_TOPIC) are excluded: they keep their own account gating, // and getSession would throw while the WC client is still initializing. - // Avalanche signingData variants carry no `account` and are knowingly not - // checked: the avax namespace is only ever granted to Core-domain dApps, - // and those methods always sign with the active account the user sees. + // Avalanche signingData variants carry no `account`, so they aren't checked + // here: AVALANCHE_SEND/SIGN_TRANSACTION sign with the server-injected active + // account (getContext in handleRequestViaVMModule), and avalanche_signMessage + // — the one method whose signer is a dApp-supplied account index — is grant- + // checked upstream in handleRequestViaVMModule (isAvalancheSignMessageAuthorized). // Fails closed: a request with no live session (deleted/expired mid-flight, // or WC uninitialized) has no grants to check against, so it's rejected — // its response is undeliverable anyway. if ('account' in signingData && !isInAppRequest(request)) { - const session = (() => { - try { - return WalletConnectService.getSession(request.sessionId) - } catch { - return undefined - } - })() + const session = this.getSessionOrUndefined(request.sessionId) if ( !session || !isAddressApproved(