diff --git a/packages/snap/CHANGELOG.md b/packages/snap/CHANGELOG.md index 81625d4f5..772e1d629 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)) 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 c76a3ccff..9f84bb52e 100644 --- a/packages/snap/integration-test/keyring-request.test.ts +++ b/packages/snap/integration-test/keyring-request.test.ts @@ -324,6 +324,7 @@ describe('KeyringRequestHandler', () => { result: { psbt: expect.any(String), // non deterministic txid: expect.any(String), + canBeMalleable: false, }, }); @@ -639,6 +640,7 @@ describe('KeyringRequestHandler', () => { pending: false, result: { txid: expect.any(String), + canBeMalleable: false, }, }); }); @@ -710,6 +712,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..bcf2383ec 100644 --- a/packages/snap/src/entities/account.ts +++ b/packages/snap/src/entities/account.ts @@ -344,3 +344,44 @@ export const networkToCoinType: Record = { signet: Slip44.Testnet, regtest: Slip44.Testnet, }; + +/** + * 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 + * 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 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 { + return ADDRESS_TYPE_TXID_MALLEABILITY[addressType]; +} diff --git a/packages/snap/src/handlers/KeyringRequestHandler.test.ts b/packages/snap/src/handlers/KeyringRequestHandler.test.ts index 2792d4bac..9d6a8af0c 100644 --- a/packages/snap/src/handlers/KeyringRequestHandler.test.ts +++ b/packages/snap/src/handlers/KeyringRequestHandler.test.ts @@ -95,12 +95,13 @@ describe('KeyringRequestHandler', () => { mockConfirmationRepository.insertSignPsbt.mockResolvedValue(undefined); }); - it('executes signPsbt with confirmation', async () => { + it('executes signPsbt with confirmation 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); @@ -125,7 +126,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, + }, }); }); @@ -329,11 +394,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); @@ -348,7 +416,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 }, }); }); @@ -401,11 +486,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); @@ -421,7 +509,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 bf1a3f4d1..13fc720f3 100644 --- a/packages/snap/src/handlers/KeyringRequestHandler.ts +++ b/packages/snap/src/handlers/KeyringRequestHandler.ts @@ -5,6 +5,7 @@ import { assert } from 'superstruct'; import type { ConfirmationRepository } from '../entities'; import { AccountCapability, + AssertionError, InexistentMethodError, NotFoundError, } from '../entities'; @@ -24,6 +25,10 @@ import type { AccountUseCases } from '../use-cases/AccountUseCases'; 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 type ComputeFeeResponse = { @@ -33,6 +38,9 @@ export type ComputeFeeResponse = { 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 type FillPsbtResponse = { @@ -131,17 +139,33 @@ export class KeyringRequestHandler { ); // Creates a fresh PSBT from the original base64 because the original PSBT is mutated by the confirmation repository - const { psbt: signedPsbt, txid } = await this.#accountsUseCases.signPsbt( + const { + psbt: signedPsbt, + txid, + canBeMalleable, + } = await this.#accountsUseCases.signPsbt( id, parsePsbt(psbtBase64), origin, options, feeRate, ); - return this.#toKeyringResponse({ + // 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: signedPsbt.toString(), txid: txid?.toString() ?? null, - } as SignPsbtResponse); + }; + if (canBeMalleable !== undefined) { + response.canBeMalleable = canBeMalleable; + } + return this.#toKeyringResponse(response); } async #fillPsbt( @@ -179,13 +203,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); } @@ -195,7 +220,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, @@ -203,6 +228,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..27cad4a5d 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( + 'signPsbt returned txid without canBeMalleable flag', + ); }); 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..3443c6a2e 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,22 @@ 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) { - throw new AssertionError('Missing transaction ID '); + throw new AssertionError('Missing transaction ID'); + } + if (canBeMalleable === undefined) { + throw new AssertionError( + 'signPsbt returned txid without canBeMalleable flag', + ); } - return { transactionId: txid.toString() }; + return { transactionId: txid.toString(), canBeMalleable }; } async #signAndSend( @@ -168,7 +176,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, @@ -178,10 +186,15 @@ export class RpcHandler { }, ); if (!txid) { - throw new AssertionError('Missing transaction ID '); + throw new AssertionError('Missing transaction ID'); + } + if (canBeMalleable === undefined) { + throw new AssertionError( + 'signPsbt returned txid without canBeMalleable flag', + ); } - 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 210f9f9c9..08b1d151e 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 () => { @@ -1554,6 +1583,7 @@ describe('AccountUseCases', () => { }); const mockAccount = mock({ network: 'bitcoin', + addressType: 'p2wpkh', sign: jest.fn(), capabilities: [AccountCapability.SendTransfer], }); @@ -1624,7 +1654,7 @@ describe('AccountUseCases', () => { mockAccount.getTransaction.mockReturnValue(mockWalletTx); mockTransaction.compute_txid.mockReturnValue(mockTxid); - const txid = await useCases.sendTransfer( + const result = await useCases.sendTransfer( 'account-id', recipients, 'metamask', @@ -1657,7 +1687,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 () => { @@ -1700,6 +1748,7 @@ describe('AccountUseCases', () => { const mockWalletTx = mock(); const mockAccount = mock({ network: 'bitcoin', + addressType: 'p2wpkh', capabilities: [AccountCapability.BroadcastPsbt], }); @@ -1730,8 +1779,8 @@ describe('AccountUseCases', () => { ).rejects.toThrow('Account missing given capability'); }); - it('broadcasts a PSBT and returns txid', async () => { - const txid = await useCases.broadcastPsbt( + it('broadcasts a PSBT and returns txid with canBeMalleable=false for P2WPKH', async () => { + const result = await useCases.broadcastPsbt( 'account-id', mockPsbt, 'metamask', @@ -1744,7 +1793,22 @@ describe('AccountUseCases', () => { mockTransaction, ); expect(mockRepository.update).toHaveBeenCalledWith(mockAccount); - expect(txid).toBe(mockTxid); + expect(result.txid).toBe(mockTxid); + expect(result.canBeMalleable).toBe(false); + }); + + it('returns canBeMalleable=true when broadcasting from 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); }); }); diff --git a/packages/snap/src/use-cases/AccountUseCases.ts b/packages/snap/src/use-cases/AccountUseCases.ts index 345fe472a..b87d32ec7 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, @@ -494,15 +526,15 @@ export class AccountUseCases { 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( @@ -649,7 +681,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()); @@ -677,7 +709,10 @@ export class AccountUseCases { ); } - return txid; + return { + txid, + canBeMalleable: canAccountTxidBeMalleated(account.addressType), + }; } #checkCapability(