From ad3413e8278158d3af61ab4e9fb6985d9a95f47c Mon Sep 17 00:00:00 2001 From: Jeremy <261848901+jeremy-consensys@users.noreply.github.com> Date: Mon, 27 Apr 2026 18:05:07 +0700 Subject: [PATCH 1/4] feat: add canBeMalleable flag to broadcast/send responses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surface a `canBeMalleable: boolean` on every snap response that returns a post-broadcast txid, so consumers can detect when a transaction's id may be rewritten by a third party before confirmation. Computed from the source account's address type: only legacy P2PKH (BIP44) keeps signatures in scriptSig and is therefore malleable. The codebase's `p2sh` is BIP49 wrapped SegWit (sh(wpkh(...))) — signatures live in the witness, so it is not malleable. Today every account is P2WPKH, so the flag is always false; the implementation is in place for when legacy support is added. Wired through every consumer-facing txid path: - AccountUseCases#broadcast (now the chokepoint, returns BroadcastResult) - signPsbt / broadcastPsbt / sendTransfer (use-case layer) - KeyringRequestHandler signPsbt / broadcastPsbt / sendTransfer - RpcHandler executeSendFlow / signAndSend / confirmSend - mapPsbtToTransaction (KeyringTransaction shape used by confirmSend) KeyringRequestHandler asserts that signPsbt cannot return a txid without the flag, mirroring the existing RpcHandler assertion. The helper's exhaustive switch throws AssertionError on unknown AddressType so a future variant cannot leak a non-boolean to consumers. OpenRPC schemas updated for both sendTransactionResponse shapes and confirmSend's KeyringTransaction return. Limitation: the flag is computed from the snap account's address type, which is correct for transactions the snap builds itself. For externally-provided PSBTs broadcast via broadcastPsbt or signPsbt({broadcast:true,fill:false}), foreign legacy inputs from collaborative transactions could still produce a malleable txid; per- input analysis is a follow-up. --- .../integration-test/client-request.test.ts | 2 + .../integration-test/keyring-request.test.ts | 3 + packages/snap/openrpc.json | 19 ++- packages/snap/src/entities/account.test.ts | 15 +++ packages/snap/src/entities/account.ts | 41 ++++++ .../handlers/KeyringRequestHandler.test.ts | 121 ++++++++++++++++-- .../src/handlers/KeyringRequestHandler.ts | 45 +++++-- packages/snap/src/handlers/RpcHandler.test.ts | 64 ++++++++- packages/snap/src/handlers/RpcHandler.ts | 15 ++- packages/snap/src/handlers/mappings.test.ts | 39 ++++++ packages/snap/src/handlers/mappings.ts | 22 +++- .../src/use-cases/AccountUseCases.test.ts | 111 +++++++++++++++- .../snap/src/use-cases/AccountUseCases.ts | 61 +++++++-- 13 files changed, 506 insertions(+), 52 deletions(-) create mode 100644 packages/snap/src/entities/account.test.ts diff --git a/packages/snap/integration-test/client-request.test.ts b/packages/snap/integration-test/client-request.test.ts index 89111e22e..2cb56a670 100644 --- a/packages/snap/integration-test/client-request.test.ts +++ b/packages/snap/integration-test/client-request.test.ts @@ -84,6 +84,7 @@ describe('OnClientRequestHandler', () => { expect(response).toRespondWith({ transactionId: expect.any(String), + canBeMalleable: false, }); const { transactionId } = ( response.response as { result: { transactionId: string } } @@ -331,6 +332,7 @@ describe('OnClientRequestHandler', () => { }, }, ], + canBeMalleable: false, }); }); diff --git a/packages/snap/integration-test/keyring-request.test.ts b/packages/snap/integration-test/keyring-request.test.ts index fde6972ea..f3c1d6d9a 100644 --- a/packages/snap/integration-test/keyring-request.test.ts +++ b/packages/snap/integration-test/keyring-request.test.ts @@ -301,6 +301,7 @@ describe('KeyringRequestHandler', () => { result: { psbt: expect.any(String), // non deterministic txid: expect.any(String), + canBeMalleable: false, }, }); }); @@ -543,6 +544,7 @@ describe('KeyringRequestHandler', () => { pending: false, result: { txid: expect.any(String), + canBeMalleable: false, }, }); }); @@ -610,6 +612,7 @@ describe('KeyringRequestHandler', () => { pending: false, result: { txid: expect.any(String), + canBeMalleable: false, }, }); }); diff --git a/packages/snap/openrpc.json b/packages/snap/openrpc.json index 19ab92e02..6dc009186 100644 --- a/packages/snap/openrpc.json +++ b/packages/snap/openrpc.json @@ -51,11 +51,15 @@ { "type": "null" }, { "type": "object", - "required": ["transactionId"], + "required": ["transactionId", "canBeMalleable"], "properties": { "transactionId": { "type": "string", "description": "The transaction ID." + }, + "canBeMalleable": { + "type": "boolean", + "description": "Whether the transaction ID can be changed by a third party (transaction malleability) before confirmation. True only for legacy P2PKH accounts; false for all currently supported address types." } } } @@ -108,11 +112,15 @@ { "type": "null" }, { "type": "object", - "required": ["transactionId"], + "required": ["transactionId", "canBeMalleable"], "properties": { "transactionId": { "type": "string", "description": "The transaction ID." + }, + "canBeMalleable": { + "type": "boolean", + "description": "Whether the transaction ID can be changed by a third party (transaction malleability) before confirmation. True only for legacy P2PKH accounts; false for all currently supported address types." } } } @@ -368,13 +376,18 @@ "events", "to", "from", - "fees" + "fees", + "canBeMalleable" ], "properties": { "id": { "type": "string", "description": "The transaction ID." }, + "canBeMalleable": { + "type": "boolean", + "description": "Whether the transaction ID can be changed by a third party (transaction malleability) before confirmation. True only for legacy P2PKH accounts; false for all currently supported address types." + }, "account": { "type": "string", "description": "The account ID that created this transaction." diff --git a/packages/snap/src/entities/account.test.ts b/packages/snap/src/entities/account.test.ts new file mode 100644 index 000000000..7dc620170 --- /dev/null +++ b/packages/snap/src/entities/account.test.ts @@ -0,0 +1,15 @@ +import type { AddressType } from '@metamask/bitcoindevkit'; + +import { canAccountTxidBeMalleated } from './account'; + +describe('canAccountTxidBeMalleated', () => { + it.each<[AddressType, boolean]>([ + ['p2pkh', true], + ['p2sh', false], + ['p2wpkh', false], + ['p2wsh', false], + ['p2tr', false], + ])('returns %s -> %s', (addressType, expected) => { + expect(canAccountTxidBeMalleated(addressType)).toBe(expected); + }); +}); diff --git a/packages/snap/src/entities/account.ts b/packages/snap/src/entities/account.ts index f7f30af65..8c41f2847 100644 --- a/packages/snap/src/entities/account.ts +++ b/packages/snap/src/entities/account.ts @@ -16,6 +16,7 @@ import type { Address, } from '@metamask/bitcoindevkit'; +import { AssertionError } from './error'; import type { Inscription } from './meta-protocols'; import type { TransactionBuilder } from './transaction'; @@ -344,3 +345,43 @@ export const networkToCoinType: Record = { signet: Slip44.Testnet, regtest: Slip44.Testnet, }; + +/** + * Whether transactions broadcast from an account of the given address type + * can have their txid changed by a third party (transaction malleability) + * before confirmation. + * + * Only legacy P2PKH (BIP44) carries signatures in scriptSig and is therefore + * malleable. Every other supported AddressType puts signatures in witness + * data, which is excluded from the txid hash. Note that `p2sh` here means + * BIP49 wrapped SegWit (sh(wpkh(...))), not generic legacy P2SH; its + * scriptSig is a fixed canonical push of the witness program and signatures + * live in the witness. If support for arbitrary legacy P2SH descriptors + * (bare multisig, custom redeem scripts with signatures in scriptSig) is + * added later, this helper must be revisited — do not blindly reuse the + * 'p2sh' branch. + * + * @param addressType - The account's address type. + * @returns `true` if a third party can rewrite the txid of a transaction + * broadcast from this account before confirmation; `false` otherwise. + */ +export function canAccountTxidBeMalleated(addressType: AddressType): boolean { + switch (addressType) { + case 'p2pkh': + return true; + case 'p2sh': + case 'p2wpkh': + case 'p2wsh': + case 'p2tr': + return false; + default: { + // Compile-time exhaustiveness guard. Throws at runtime if a new + // AddressType variant is ever added without a deliberate decision + // here, so consumers never receive a non-boolean value for this flag. + const _exhaustive: never = addressType; + throw new AssertionError('Unhandled AddressType for malleability check', { + addressType: _exhaustive, + }); + } + } +} diff --git a/packages/snap/src/handlers/KeyringRequestHandler.test.ts b/packages/snap/src/handlers/KeyringRequestHandler.test.ts index 5394e6889..d76f02ad7 100644 --- a/packages/snap/src/handlers/KeyringRequestHandler.test.ts +++ b/packages/snap/src/handlers/KeyringRequestHandler.test.ts @@ -73,12 +73,13 @@ describe('KeyringRequestHandler', () => { account: 'account-id', }); - it('executes signPsbt', async () => { + it('executes signPsbt and propagates canBeMalleable when broadcasting', async () => { mockAccountsUseCases.signPsbt.mockResolvedValue({ psbt: 'psbtBase64', txid: mock({ toString: jest.fn().mockReturnValue('txid'), }), + canBeMalleable: false, }); const result = await handler.route(mockRequest); @@ -96,7 +97,71 @@ describe('KeyringRequestHandler', () => { ); expect(result).toStrictEqual({ pending: false, - result: { psbt: 'psbtBase64', txid: 'txid' }, + result: { + psbt: 'psbtBase64', + txid: 'txid', + canBeMalleable: false, + }, + }); + }); + + it('omits canBeMalleable when broadcast=false (no txid returned)', async () => { + const noBroadcastRequest = mock({ + origin, + request: { + method: AccountCapability.SignPsbt, + params: { + psbt: 'psbtBase64', + feeRate: 3, + options: { fill: false, broadcast: false }, + }, + }, + account: 'account-id', + }); + mockAccountsUseCases.signPsbt.mockResolvedValue({ + psbt: 'psbtBase64', + }); + + const result = await handler.route(noBroadcastRequest); + + expect(result).toStrictEqual({ + pending: false, + result: { psbt: 'psbtBase64', txid: null }, + }); + }); + + it('throws AssertionError if usecase returns txid without canBeMalleable', async () => { + mockAccountsUseCases.signPsbt.mockResolvedValue({ + psbt: 'psbtBase64', + txid: mock({ + toString: jest.fn().mockReturnValue('txid'), + }), + // canBeMalleable intentionally omitted — protocol violation + }); + + await expect(handler.route(mockRequest)).rejects.toThrow( + 'signPsbt returned txid without canBeMalleable flag', + ); + }); + + it('passes canBeMalleable=true through for legacy P2PKH accounts', async () => { + mockAccountsUseCases.signPsbt.mockResolvedValue({ + psbt: 'psbtBase64', + txid: mock({ + toString: jest.fn().mockReturnValue('txid'), + }), + canBeMalleable: true, + }); + + const result = await handler.route(mockRequest); + + expect(result).toStrictEqual({ + pending: false, + result: { + psbt: 'psbtBase64', + txid: 'txid', + canBeMalleable: true, + }, }); }); @@ -263,11 +328,14 @@ describe('KeyringRequestHandler', () => { account: 'account-id', }); - it('executes broadcastPsbt', async () => { + it('executes broadcastPsbt and propagates canBeMalleable', async () => { const mockTxid = mock({ toString: jest.fn().mockReturnValue('txid'), }); - mockAccountsUseCases.broadcastPsbt.mockResolvedValue(mockTxid); + mockAccountsUseCases.broadcastPsbt.mockResolvedValue({ + txid: mockTxid, + canBeMalleable: false, + }); const result = await handler.route(mockRequest); @@ -282,7 +350,24 @@ describe('KeyringRequestHandler', () => { ); expect(result).toStrictEqual({ pending: false, - result: { txid: 'txid' }, + result: { txid: 'txid', canBeMalleable: false }, + }); + }); + + it('passes canBeMalleable=true through for legacy P2PKH accounts', async () => { + const mockTxid = mock({ + toString: jest.fn().mockReturnValue('txid'), + }); + mockAccountsUseCases.broadcastPsbt.mockResolvedValue({ + txid: mockTxid, + canBeMalleable: true, + }); + + const result = await handler.route(mockRequest); + + expect(result).toStrictEqual({ + pending: false, + result: { txid: 'txid', canBeMalleable: true }, }); }); @@ -331,11 +416,14 @@ describe('KeyringRequestHandler', () => { account: 'account-id', }); - it('executes sendTransferq', async () => { + it('executes sendTransfer and propagates canBeMalleable', async () => { const mockTxid = mock({ toString: jest.fn().mockReturnValue('txid'), }); - mockAccountsUseCases.sendTransfer.mockResolvedValue(mockTxid); + mockAccountsUseCases.sendTransfer.mockResolvedValue({ + txid: mockTxid, + canBeMalleable: false, + }); const result = await handler.route(mockRequest); @@ -351,7 +439,24 @@ describe('KeyringRequestHandler', () => { ); expect(result).toStrictEqual({ pending: false, - result: { txid: 'txid' }, + result: { txid: 'txid', canBeMalleable: false }, + }); + }); + + it('passes canBeMalleable=true through for legacy P2PKH accounts', async () => { + const mockTxid = mock({ + toString: jest.fn().mockReturnValue('txid'), + }); + mockAccountsUseCases.sendTransfer.mockResolvedValue({ + txid: mockTxid, + canBeMalleable: true, + }); + + const result = await handler.route(mockRequest); + + expect(result).toStrictEqual({ + pending: false, + result: { txid: 'txid', canBeMalleable: true }, }); }); diff --git a/packages/snap/src/handlers/KeyringRequestHandler.ts b/packages/snap/src/handlers/KeyringRequestHandler.ts index fb84f7a8b..be92528c2 100644 --- a/packages/snap/src/handlers/KeyringRequestHandler.ts +++ b/packages/snap/src/handlers/KeyringRequestHandler.ts @@ -12,6 +12,7 @@ import { import { AccountCapability, + AssertionError, InexistentMethodError, NotFoundError, } from '../entities'; @@ -31,6 +32,10 @@ export const SignPsbtRequest = object({ export type SignPsbtResponse = { psbt: string; txid: string | null; + // Present only when broadcast happened. True if the source account's + // address type allows third-party txid malleation before confirmation + // (currently only legacy P2PKH). + canBeMalleable?: boolean; }; export const ComputeFeeRequest = object({ @@ -49,6 +54,9 @@ export const BroadcastPsbtRequest = object({ export type BroadcastPsbtResponse = { txid: string; + // True if the source account's address type allows third-party txid + // malleation before confirmation (currently only legacy P2PKH). + canBeMalleable: boolean; }; export const FillPsbtRequest = object({ @@ -153,17 +161,30 @@ export class KeyringRequestHandler { options: { fill: boolean; broadcast: boolean }, feeRate?: number, ): Promise { - const { psbt, txid } = await this.#accountsUseCases.signPsbt( - id, - parsePsbt(psbtBase64), - origin, - options, - feeRate, - ); - return this.#toKeyringResponse({ + const { psbt, txid, canBeMalleable } = + await this.#accountsUseCases.signPsbt( + id, + parsePsbt(psbtBase64), + origin, + options, + feeRate, + ); + // Invariant: signPsbt sets txid and canBeMalleable together (when broadcast + // happened) or neither (when it didn't). A txid without the flag would + // leak a possibly-malleable txid to the consumer. + if (txid !== undefined && canBeMalleable === undefined) { + throw new AssertionError( + 'signPsbt returned txid without canBeMalleable flag', + ); + } + const response: SignPsbtResponse = { psbt: psbt.toString(), txid: txid?.toString() ?? null, - } as SignPsbtResponse); + }; + if (canBeMalleable !== undefined) { + response.canBeMalleable = canBeMalleable; + } + return this.#toKeyringResponse(response); } async #fillPsbt( @@ -201,13 +222,14 @@ export class KeyringRequestHandler { psbtBase64: string, origin: string, ): Promise { - const txid = await this.#accountsUseCases.broadcastPsbt( + const { txid, canBeMalleable } = await this.#accountsUseCases.broadcastPsbt( id, parsePsbt(psbtBase64), origin, ); return this.#toKeyringResponse({ txid: txid.toString(), + canBeMalleable, } as BroadcastPsbtResponse); } @@ -217,7 +239,7 @@ export class KeyringRequestHandler { origin: string, feeRate?: number, ): Promise { - const txid = await this.#accountsUseCases.sendTransfer( + const { txid, canBeMalleable } = await this.#accountsUseCases.sendTransfer( id, recipients, origin, @@ -225,6 +247,7 @@ export class KeyringRequestHandler { ); return this.#toKeyringResponse({ txid: txid.toString(), + canBeMalleable, } as BroadcastPsbtResponse); } diff --git a/packages/snap/src/handlers/RpcHandler.test.ts b/packages/snap/src/handlers/RpcHandler.test.ts index 0360bc5af..01e7d9a1e 100644 --- a/packages/snap/src/handlers/RpcHandler.test.ts +++ b/packages/snap/src/handlers/RpcHandler.test.ts @@ -327,6 +327,7 @@ describe('RpcHandler', () => { txid: mock({ toString: jest.fn().mockReturnValue('txId'), }), + canBeMalleable: false, }); const result = await handler.route(origin, request); @@ -338,7 +339,43 @@ describe('RpcHandler', () => { 'metamask', { broadcast: true, fill: false }, ); - expect(result).toStrictEqual({ transactionId: 'txId' }); + expect(result).toStrictEqual({ + transactionId: 'txId', + canBeMalleable: false, + }); + }); + + it('propagates canBeMalleable=true from legacy P2PKH accounts', async () => { + mockSendFlowUseCases.display.mockResolvedValue(mockPsbt); + mockAccountsUseCases.signPsbt.mockResolvedValue({ + psbt: 'psbtBase64', + txid: mock({ + toString: jest.fn().mockReturnValue('txId'), + }), + canBeMalleable: true, + }); + + const result = await handler.route(origin, request); + + expect(result).toStrictEqual({ + transactionId: 'txId', + canBeMalleable: true, + }); + }); + + it('throws when canBeMalleable is missing (signPsbt did not broadcast)', async () => { + mockSendFlowUseCases.display.mockResolvedValue(mockPsbt); + mockAccountsUseCases.signPsbt.mockResolvedValue({ + psbt: 'psbtBase64', + txid: mock({ + toString: jest.fn().mockReturnValue('txId'), + }), + // canBeMalleable intentionally omitted + }); + + await expect(handler.route(origin, request)).rejects.toThrow( + 'Missing transaction ID', + ); }); it('propagates errors from display', async () => { @@ -382,6 +419,7 @@ describe('RpcHandler', () => { txid: mock({ toString: jest.fn().mockReturnValue('txId'), }), + canBeMalleable: false, }); const result = await handler.route(origin, request); @@ -392,7 +430,10 @@ describe('RpcHandler', () => { 'metamask', { broadcast: true, fill: true }, ); - expect(result).toStrictEqual({ transactionId: 'txId' }); + expect(result).toStrictEqual({ + transactionId: 'txId', + canBeMalleable: false, + }); }); it('propagates errors from signAndSendTransaction', async () => { @@ -718,9 +759,11 @@ describe('RpcHandler', () => { // we mock the mapping function since we don't care about the result structure here // it is tested in mappings.test.ts - jest - .mocked(mapPsbtToTransaction) - .mockReturnValue({} as KeyringTransaction); + jest.mocked(mapPsbtToTransaction).mockReturnValue({ + canBeMalleable: false, + } as KeyringTransaction & { + canBeMalleable: boolean; + }); }); it('creates and signs a transaction successfully', async () => { @@ -740,6 +783,17 @@ describe('RpcHandler', () => { expect(result).toBeDefined(); }); + it('passes the canBeMalleable flag through from mapPsbtToTransaction', async () => { + jest.mocked(mapPsbtToTransaction).mockReturnValueOnce({ + id: 'tx-id', + canBeMalleable: true, + } as unknown as KeyringTransaction & { canBeMalleable: boolean }); + + const result = await handler.route(origin, validRequest); + + expect((result as any).canBeMalleable).toBe(true); + }); + it('handles different amounts and addresses', async () => { const customRequest: JsonRpcRequest = { id: 1, diff --git a/packages/snap/src/handlers/RpcHandler.ts b/packages/snap/src/handlers/RpcHandler.ts index bbc4747a1..f814caee7 100644 --- a/packages/snap/src/handlers/RpcHandler.ts +++ b/packages/snap/src/handlers/RpcHandler.ts @@ -57,6 +57,9 @@ export const ComputeFeeRequest = object({ export type SendTransactionResponse = { transactionId: string; + // True if the source account's address type allows third-party txid + // malleation before confirmation (currently only legacy P2PKH). + canBeMalleable: boolean; }; export const VerifyMessageRequest = object({ @@ -148,17 +151,17 @@ export class RpcHandler { if (!psbt) { return null; } - const { txid } = await this.#accountUseCases.signPsbt( + const { txid, canBeMalleable } = await this.#accountUseCases.signPsbt( account, psbt, origin, { fill: false, broadcast: true }, ); - if (!txid) { + if (!txid || canBeMalleable === undefined) { throw new AssertionError('Missing transaction ID '); } - return { transactionId: txid.toString() }; + return { transactionId: txid.toString(), canBeMalleable }; } async #signAndSend( @@ -168,7 +171,7 @@ export class RpcHandler { ): Promise { const psbt = parsePsbt(transaction); - const { txid } = await this.#accountUseCases.signPsbt( + const { txid, canBeMalleable } = await this.#accountUseCases.signPsbt( accountId, psbt, origin, @@ -177,11 +180,11 @@ export class RpcHandler { broadcast: true, }, ); - if (!txid) { + if (!txid || canBeMalleable === undefined) { throw new AssertionError('Missing transaction ID '); } - return { transactionId: txid.toString() }; + return { transactionId: txid.toString(), canBeMalleable }; } async #computeFee( diff --git a/packages/snap/src/handlers/mappings.test.ts b/packages/snap/src/handlers/mappings.test.ts index c2dd3b8be..4394759e9 100644 --- a/packages/snap/src/handlers/mappings.test.ts +++ b/packages/snap/src/handlers/mappings.test.ts @@ -75,11 +75,13 @@ describe('mapPsbtToTransaction', () => { * * @param network - The network type ('bitcoin' or 'testnet'). * @param feeSatoshis - The fee amount in satoshis. + * @param addressType - The account's address type (script type). * @returns A mocked BitcoinAccount object. */ function createMockAccount( network: 'bitcoin' | 'testnet' = 'bitcoin', feeSatoshis = 500, + addressType: 'p2pkh' | 'p2sh' | 'p2wpkh' | 'p2wsh' | 'p2tr' = 'p2wpkh', ) { const mockFeeAmount = mock(); jest @@ -89,6 +91,7 @@ describe('mapPsbtToTransaction', () => { const account = mock(); account.id = ACCOUNT_ID; account.network = network; + account.addressType = addressType; jest.spyOn(account, 'calculateFee').mockReturnValue(mockFeeAmount); jest.spyOn(account, 'isMine').mockReturnValue(false); @@ -146,9 +149,45 @@ describe('mapPsbtToTransaction', () => { }, }, ], + canBeMalleable: false, }); }); + it('sets canBeMalleable=true for legacy P2PKH accounts', () => { + const account = createMockAccount('bitcoin', 500, 'p2pkh'); + const output = createMockOutput(10000); + const transaction = createMockTransaction('p2pkhtx', [output]); + + jest.mocked(Address.from_script).mockImplementationOnce( + () => + ({ + toString: () => '1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa', + }) as any, + ); + + const result = mapPsbtToTransaction(account, transaction); + + expect(result.canBeMalleable).toBe(true); + }); + + it('sets canBeMalleable=false for BIP49 wrapped-segwit (p2sh) accounts', () => { + // BIP49 sh(wpkh(...)) keeps signatures in witness, not scriptSig. + const account = createMockAccount('bitcoin', 500, 'p2sh'); + const output = createMockOutput(10000); + const transaction = createMockTransaction('p2shtx', [output]); + + jest.mocked(Address.from_script).mockImplementationOnce( + () => + ({ + toString: () => '3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy', + }) as any, + ); + + const result = mapPsbtToTransaction(account, transaction); + + expect(result.canBeMalleable).toBe(false); + }); + it('filters out change outputs owned by the account', () => { const account = createMockAccount(); const changeOutput = createMockOutput(5000); diff --git a/packages/snap/src/handlers/mappings.ts b/packages/snap/src/handlers/mappings.ts index 8eabe7a30..fc260c67c 100644 --- a/packages/snap/src/handlers/mappings.ts +++ b/packages/snap/src/handlers/mappings.ts @@ -19,7 +19,11 @@ import { TransactionStatus, } from '@metamask/keyring-api'; -import { type BitcoinAccount, networkToCurrencyUnit } from '../entities'; +import { + type BitcoinAccount, + canAccountTxidBeMalleated, + networkToCurrencyUnit, +} from '../entities'; import type { Caip19Asset } from './caip'; import { addressTypeToCaip, networkToCaip19, networkToScope } from './caip'; @@ -216,7 +220,18 @@ export function mapToTransaction( } /** - * Maps a PSBT to a Keyring Transaction. + * KeyringTransaction augmented with a malleability flag. The base + * `KeyringTransaction` shape from `@metamask/keyring-api` does not include + * `canBeMalleable`; consumers (extension, dApp) that ignore unknown fields + * will see the standard shape, while consumers that opt in can read the flag + * to decide whether to trust the txid before block confirmation. + */ +export type KeyringTransactionWithMalleability = KeyringTransaction & { + canBeMalleable: boolean; +}; + +/** + * Maps a PSBT to a Keyring Transaction with a malleability flag. * * @param account - The Bitcoin account. * @param tx - The extracted transaction from the PSBT. @@ -225,7 +240,7 @@ export function mapToTransaction( export function mapPsbtToTransaction( account: BitcoinAccount, tx: Transaction, -): KeyringTransaction { +): KeyringTransactionWithMalleability { const txid = tx.compute_txid(); const currentTime = Date.now(); @@ -255,6 +270,7 @@ export function mapPsbtToTransaction( to: getRecipients(tx.output), from: [], fees: [mapToTransactionFees(account.calculateFee(tx), account.network)], + canBeMalleable: canAccountTxidBeMalleated(account.addressType), }; } diff --git a/packages/snap/src/use-cases/AccountUseCases.test.ts b/packages/snap/src/use-cases/AccountUseCases.test.ts index bea64eeae..f99f94e71 100644 --- a/packages/snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/snap/src/use-cases/AccountUseCases.test.ts @@ -865,6 +865,7 @@ describe('AccountUseCases', () => { }); const mockAccount = mock({ network: 'bitcoin', + addressType: 'p2wpkh', sign: jest.fn(), capabilities: [AccountCapability.SignPsbt], }); @@ -938,7 +939,7 @@ describe('AccountUseCases', () => { mockAccount.getTransaction.mockReturnValue(mockWalletTx); mockTransaction.compute_txid.mockReturnValue(mockTxid); - const { txid, psbt } = await useCases.signPsbt( + const { txid, psbt, canBeMalleable } = await useCases.signPsbt( 'account-id', mockPsbt, 'metamask', @@ -970,6 +971,34 @@ describe('AccountUseCases', () => { ); expect(txid).toBe(mockTxid); expect(psbt).toBe('mockSignedPsbt'); + expect(canBeMalleable).toBe(false); + }); + + it('omits canBeMalleable when broadcast is false', async () => { + const { canBeMalleable } = await useCases.signPsbt( + 'account-id', + mockPsbt, + 'metamask', + { fill: false, broadcast: false }, + ); + + expect(canBeMalleable).toBeUndefined(); + }); + + it('sets canBeMalleable=true when broadcasting from a legacy P2PKH account', async () => { + mockAccount.getTransaction.mockReturnValue(mockWalletTx); + mockTransaction.compute_txid.mockReturnValue(mockTxid); + const legacyAccount = { ...mockAccount, addressType: 'p2pkh' as const }; + mockRepository.getWithSigner.mockResolvedValueOnce(legacyAccount); + + const { canBeMalleable } = await useCases.signPsbt( + 'account-id', + mockPsbt, + 'metamask', + { fill: false, broadcast: true }, + ); + + expect(canBeMalleable).toBe(true); }); it('fills, signs and broadcasts a PSBT', async () => { @@ -1525,6 +1554,63 @@ describe('AccountUseCases', () => { }); }); + describe('broadcastPsbt', () => { + const mockTxid = mock(); + const mockPsbt = mock(); + const mockTransaction = mock({ + compute_txid: jest.fn(), + clone: jest.fn(), + }); + const mockWalletTx = mock(); + const mockAccount = mock({ + network: 'bitcoin', + addressType: 'p2wpkh', + capabilities: [AccountCapability.BroadcastPsbt], + }); + + beforeEach(() => { + mockRepository.get.mockResolvedValue(mockAccount); + mockTransaction.compute_txid.mockReturnValue(mockTxid); + mockTransaction.clone.mockReturnThis(); + mockAccount.extractTransaction.mockReturnValue(mockTransaction); + mockAccount.getTransaction.mockReturnValue(mockWalletTx); + }); + + it('throws if account is not found', async () => { + mockRepository.get.mockResolvedValue(null); + + await expect( + useCases.broadcastPsbt('non-existent-id', mockPsbt, 'metamask'), + ).rejects.toThrow('Account not found'); + }); + + it('returns canBeMalleable=false for a P2WPKH account', async () => { + const result = await useCases.broadcastPsbt( + 'account-id', + mockPsbt, + 'metamask', + ); + + expect(mockChain.broadcast).toHaveBeenCalled(); + expect(result.txid).toBe(mockTxid); + expect(result.canBeMalleable).toBe(false); + }); + + it('returns canBeMalleable=true for a legacy P2PKH account', async () => { + const legacyAccount = { ...mockAccount, addressType: 'p2pkh' as const }; + mockRepository.get.mockResolvedValueOnce(legacyAccount); + + const result = await useCases.broadcastPsbt( + 'account-id', + mockPsbt, + 'metamask', + ); + + expect(result.txid).toBe(mockTxid); + expect(result.canBeMalleable).toBe(true); + }); + }); + describe('sendTransfer', () => { const recipients = [ { @@ -1558,6 +1644,7 @@ describe('AccountUseCases', () => { }); const mockAccount = mock({ network: 'bitcoin', + addressType: 'p2wpkh', sign: jest.fn(), capabilities: [AccountCapability.SendTransfer], }); @@ -1611,7 +1698,7 @@ describe('AccountUseCases', () => { mockTransaction.compute_txid.mockReturnValue(mockTxid); mockTxBuilder.finish.mockReturnValueOnce(mockPsbt); - const txid = await useCases.sendTransfer( + const result = await useCases.sendTransfer( 'account-id', recipients, 'metamask', @@ -1648,7 +1735,25 @@ describe('AccountUseCases', () => { mockWalletTx, 'metamask', ); - expect(txid).toBe(mockTxid); + expect(result.txid).toBe(mockTxid); + expect(result.canBeMalleable).toBe(false); + }); + + it('sets canBeMalleable=true on legacy P2PKH accounts', async () => { + mockAccount.getTransaction.mockReturnValue(mockWalletTx); + mockTransaction.compute_txid.mockReturnValue(mockTxid); + mockTxBuilder.finish.mockReturnValueOnce(mockPsbt); + + const legacyAccount = { ...mockAccount, addressType: 'p2pkh' as const }; + mockRepository.getWithSigner.mockResolvedValueOnce(legacyAccount); + + const result = await useCases.sendTransfer( + 'account-id', + recipients, + 'metamask', + ); + + expect(result.canBeMalleable).toBe(true); }); it('propagates an error if getWithSigner fails', async () => { diff --git a/packages/snap/src/use-cases/AccountUseCases.ts b/packages/snap/src/use-cases/AccountUseCases.ts index 2cb2e1314..0bde6b428 100644 --- a/packages/snap/src/use-cases/AccountUseCases.ts +++ b/packages/snap/src/use-cases/AccountUseCases.ts @@ -25,6 +25,7 @@ import { AccountCapability, addressTypeToPurpose, AssertionError, + canAccountTxidBeMalleated, networkToCoinType, NotFoundError, PermissionError, @@ -48,6 +49,29 @@ export type CreateAccountParams = DiscoverAccountParams & { accountName?: string; }; +/** + * Result of broadcasting a Bitcoin transaction. + * + * `canBeMalleable` is `true` when the source account's address type allows a + * third party to rewrite the txid before confirmation (currently only legacy + * P2PKH). Consumers that persist or surface the txid to the dApp should treat + * a `true` flag as a signal that the txid may change, and avoid trusting it + * for finality decisions before block confirmation. + * + * Limitation: the flag is computed from the snap account's address type, which + * is correct for transactions the snap builds itself (`sendTransfer`, + * `executeSendFlow`, `confirmSend`). For externally-provided PSBTs broadcast + * via `broadcastPsbt` or `signPsbt({ broadcast: true, fill: false })`, the + * transaction may include foreign inputs of any address type; if any foreign + * input is legacy non-witness, the txid is malleable even when the snap's + * account is P2WPKH. Callers building collaborative transactions must compute + * malleability per-input. + */ +export type BroadcastResult = { + txid: Txid; + canBeMalleable: boolean; +}; + export class AccountUseCases { readonly #logger: Logger; @@ -372,7 +396,7 @@ export class AccountUseCases { origin: string, options: { fill: boolean; broadcast: boolean }, feeRate?: number, - ): Promise<{ psbt: string; txid?: Txid }> { + ): Promise<{ psbt: string; txid?: Txid; canBeMalleable?: boolean }> { this.#logger.debug('Signing PSBT: %s', id, options); const account = await this.#repository.getWithSigner(id); @@ -389,7 +413,11 @@ export class AccountUseCases { if (options.broadcast) { const psbtString = signedPsbt.toString(); const tx = account.extractTransaction(signedPsbt); - const txid = await this.#broadcast(account, tx, origin); + const { txid, canBeMalleable } = await this.#broadcast( + account, + tx, + origin, + ); this.#logger.info( 'Transaction sent successfully: %s. Account: %s, Network: %s, Options: %o', @@ -398,7 +426,7 @@ export class AccountUseCases { account.network, options, ); - return { psbt: psbtString, txid }; + return { psbt: psbtString, txid, canBeMalleable }; } this.#logger.info( @@ -427,7 +455,11 @@ export class AccountUseCases { return psbt.fee(); } - async broadcastPsbt(id: string, psbt: Psbt, origin: string): Promise { + async broadcastPsbt( + id: string, + psbt: Psbt, + origin: string, + ): Promise { this.#logger.debug('Sending transaction: %s', id); const account = await this.#repository.get(id); @@ -437,16 +469,16 @@ export class AccountUseCases { this.#checkCapability(account, AccountCapability.BroadcastPsbt); const tx = account.extractTransaction(psbt); - const txid = await this.#broadcast(account, tx, origin); + const result = await this.#broadcast(account, tx, origin); this.#logger.info( 'Transaction sent successfully: %s. Account: %s, Network: %s', - txid, + result.txid, account.id, account.network, ); - return txid; + return result; } async sendTransfer( @@ -454,7 +486,7 @@ export class AccountUseCases { recipients: { address: string; amount: string }[], origin: string, feeRate?: number, - ): Promise { + ): Promise { this.#logger.debug( 'Transferring funds: %s. Recipients: %o', id, @@ -478,15 +510,15 @@ export class AccountUseCases { const psbt = await this.#fillPsbt(account, templatePsbt, feeRate); const signedPsbt = account.sign(psbt); const tx = account.extractTransaction(signedPsbt); - const txid = await this.#broadcast(account, tx, origin); + const result = await this.#broadcast(account, tx, origin); this.#logger.info( 'Funds transferred successfully: %s. Account: %s, Network: %s', - txid.toString(), + result.txid.toString(), account.id, account.network, ); - return txid; + return result; } async signMessage( @@ -633,7 +665,7 @@ export class AccountUseCases { account: BitcoinAccount, tx: Transaction, origin: string, - ): Promise { + ): Promise { const txid = tx.compute_txid(); await this.#chain.broadcast(account.network, tx.clone()); account.applyUnconfirmedTx(tx, getCurrentUnixTimestamp()); @@ -661,7 +693,10 @@ export class AccountUseCases { ); } - return txid; + return { + txid, + canBeMalleable: canAccountTxidBeMalleated(account.addressType), + }; } #checkCapability( From 541fd6f98cb4059b5a3f8a96156711d09267c390 Mon Sep 17 00:00:00 2001 From: Jeremy <261848901+jeremy-consensys@users.noreply.github.com> Date: Thu, 30 Apr 2026 13:38:41 +0700 Subject: [PATCH 2/4] fix: split RpcHandler assertion messages by failure mode Cursor review noted the combined `!txid || canBeMalleable === undefined` guard threw 'Missing transaction ID' regardless of which condition fired, masking missing-flag protocol violations. Split the guard so each branch reports its own cause, matching the message used in KeyringRequestHandler. --- packages/snap/src/handlers/RpcHandler.test.ts | 2 +- packages/snap/src/handlers/RpcHandler.ts | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/snap/src/handlers/RpcHandler.test.ts b/packages/snap/src/handlers/RpcHandler.test.ts index 01e7d9a1e..27cad4a5d 100644 --- a/packages/snap/src/handlers/RpcHandler.test.ts +++ b/packages/snap/src/handlers/RpcHandler.test.ts @@ -374,7 +374,7 @@ describe('RpcHandler', () => { }); await expect(handler.route(origin, request)).rejects.toThrow( - 'Missing transaction ID', + 'signPsbt returned txid without canBeMalleable flag', ); }); diff --git a/packages/snap/src/handlers/RpcHandler.ts b/packages/snap/src/handlers/RpcHandler.ts index f814caee7..3443c6a2e 100644 --- a/packages/snap/src/handlers/RpcHandler.ts +++ b/packages/snap/src/handlers/RpcHandler.ts @@ -157,8 +157,13 @@ export class RpcHandler { origin, { fill: false, broadcast: true }, ); - if (!txid || canBeMalleable === undefined) { - throw new AssertionError('Missing transaction ID '); + if (!txid) { + throw new AssertionError('Missing transaction ID'); + } + if (canBeMalleable === undefined) { + throw new AssertionError( + 'signPsbt returned txid without canBeMalleable flag', + ); } return { transactionId: txid.toString(), canBeMalleable }; @@ -180,8 +185,13 @@ export class RpcHandler { broadcast: true, }, ); - if (!txid || canBeMalleable === undefined) { - throw new AssertionError('Missing transaction ID '); + if (!txid) { + throw new AssertionError('Missing transaction ID'); + } + if (canBeMalleable === undefined) { + throw new AssertionError( + 'signPsbt returned txid without canBeMalleable flag', + ); } return { transactionId: txid.toString(), canBeMalleable }; From b781669711c4c1d639186bedea8abc64bc3802cc Mon Sep 17 00:00:00 2001 From: Jeremy <261848901+jeremy-consensys@users.noreply.github.com> Date: Fri, 8 May 2026 00:57:18 +0800 Subject: [PATCH 3/4] docs: add changelog entry for canBeMalleable flag --- packages/snap/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/snap/CHANGELOG.md b/packages/snap/CHANGELOG.md index bf02a33ae..ffe0ca194 100644 --- a/packages/snap/CHANGELOG.md +++ b/packages/snap/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `canBeMalleable` flag to broadcast/send response shapes so consumers can detect when a transaction id may be rewritten by a third party before confirmation (relevant only for legacy P2PKH accounts, currently always `false`) ([#599](https://github.com/MetaMask/snap-bitcoin-wallet/pull/599)) + ### Changed - Show a confirmation dialog before signing a PSBT from KeyringHandler and sending a transfer ([#591](https://github.com/MetaMask/snap-bitcoin-wallet/pull/591)) From 1afce537b26539d9dcdc4bfa16bf0560c99cc5ce Mon Sep 17 00:00:00 2001 From: Jeremy <261848901+jeremy-consensys@users.noreply.github.com> Date: Mon, 18 May 2026 14:18:22 +0700 Subject: [PATCH 4/4] refactor: use Record for malleability table Replaces the switch with a typed lookup object guarded by `satisfies Record` for compile-time exhaustiveness. A new AddressType variant added upstream now surfaces as a TypeScript error on the literal, rather than a runtime AssertionError no one will see until production. Drops the now-unused AssertionError import. --- packages/snap/src/entities/account.ts | 46 +++++++++++++-------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/packages/snap/src/entities/account.ts b/packages/snap/src/entities/account.ts index 8c41f2847..bcf2383ec 100644 --- a/packages/snap/src/entities/account.ts +++ b/packages/snap/src/entities/account.ts @@ -16,7 +16,6 @@ import type { Address, } from '@metamask/bitcoindevkit'; -import { AssertionError } from './error'; import type { Inscription } from './meta-protocols'; import type { TransactionBuilder } from './transaction'; @@ -347,8 +346,8 @@ export const networkToCoinType: Record = { }; /** - * Whether transactions broadcast from an account of the given address type - * can have their txid changed by a third party (transaction malleability) + * Whether transactions broadcast from an account of each AddressType can + * have their txid changed by a third party (transaction malleability) * before confirmation. * * Only legacy P2PKH (BIP44) carries signatures in scriptSig and is therefore @@ -358,30 +357,31 @@ export const networkToCoinType: Record = { * scriptSig is a fixed canonical push of the witness program and signatures * live in the witness. If support for arbitrary legacy P2SH descriptors * (bare multisig, custom redeem scripts with signatures in scriptSig) is - * added later, this helper must be revisited — do not blindly reuse the - * 'p2sh' branch. + * added later, this table must be revisited — do not blindly keep the + * `p2sh` entry as `false`. + * + * Compile-time exhaustiveness: `satisfies Record` + * forces every AddressType variant to appear as a key. If a new variant is + * ever added upstream, this object literal becomes a TypeScript error + * until a deliberate malleability decision is recorded here. + */ +const ADDRESS_TYPE_TXID_MALLEABILITY = { + p2pkh: true, + p2sh: false, + p2wpkh: false, + p2wsh: false, + p2tr: false, +} as const satisfies Record; + +/** + * Whether transactions broadcast from an account of the given address type + * can have their txid changed by a third party (transaction malleability) + * before confirmation. * * @param addressType - The account's address type. * @returns `true` if a third party can rewrite the txid of a transaction * broadcast from this account before confirmation; `false` otherwise. */ export function canAccountTxidBeMalleated(addressType: AddressType): boolean { - switch (addressType) { - case 'p2pkh': - return true; - case 'p2sh': - case 'p2wpkh': - case 'p2wsh': - case 'p2tr': - return false; - default: { - // Compile-time exhaustiveness guard. Throws at runtime if a new - // AddressType variant is ever added without a deliberate decision - // here, so consumers never receive a non-boolean value for this flag. - const _exhaustive: never = addressType; - throw new AssertionError('Unhandled AddressType for malleability check', { - addressType: _exhaustive, - }); - } - } + return ADDRESS_TYPE_TXID_MALLEABILITY[addressType]; }