Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/snap/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
2 changes: 2 additions & 0 deletions packages/snap/integration-test/client-request.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ describe('OnClientRequestHandler', () => {

expect(response).toRespondWith({
transactionId: expect.any(String),
canBeMalleable: false,
});
const { transactionId } = (
response.response as { result: { transactionId: string } }
Expand Down Expand Up @@ -331,6 +332,7 @@ describe('OnClientRequestHandler', () => {
},
},
],
canBeMalleable: false,
});
});

Expand Down
3 changes: 3 additions & 0 deletions packages/snap/integration-test/keyring-request.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,7 @@ describe('KeyringRequestHandler', () => {
result: {
psbt: expect.any(String), // non deterministic
txid: expect.any(String),
canBeMalleable: false,
},
});

Expand Down Expand Up @@ -639,6 +640,7 @@ describe('KeyringRequestHandler', () => {
pending: false,
result: {
txid: expect.any(String),
canBeMalleable: false,
},
});
});
Expand Down Expand Up @@ -710,6 +712,7 @@ describe('KeyringRequestHandler', () => {
pending: false,
result: {
txid: expect.any(String),
canBeMalleable: false,
},
});
});
Expand Down
19 changes: 16 additions & 3 deletions packages/snap/openrpc.json
Original file line number Diff line number Diff line change
Expand Up @@ -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."
}
}
}
Expand Down Expand Up @@ -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."
}
}
}
Expand Down Expand Up @@ -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."
Expand Down
15 changes: 15 additions & 0 deletions packages/snap/src/entities/account.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
41 changes: 41 additions & 0 deletions packages/snap/src/entities/account.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,3 +344,44 @@ export const networkToCoinType: Record<Network, Slip44> = {
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<AddressType, boolean>`
* 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<AddressType, boolean>;

/**
* 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];
}
Comment thread
jeremytsng marked this conversation as resolved.
121 changes: 113 additions & 8 deletions packages/snap/src/handlers/KeyringRequestHandler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<Txid>({
toString: jest.fn().mockReturnValue('txid'),
}),
canBeMalleable: false,
});

const result = await handler.route(mockRequest);
Expand All @@ -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<KeyringRequest>({
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<Txid>({
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<Txid>({
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,
},
});
});

Expand Down Expand Up @@ -329,11 +394,14 @@ describe('KeyringRequestHandler', () => {
account: 'account-id',
});

it('executes broadcastPsbt', async () => {
it('executes broadcastPsbt and propagates canBeMalleable', async () => {
const mockTxid = mock<Txid>({
toString: jest.fn().mockReturnValue('txid'),
});
mockAccountsUseCases.broadcastPsbt.mockResolvedValue(mockTxid);
mockAccountsUseCases.broadcastPsbt.mockResolvedValue({
txid: mockTxid,
canBeMalleable: false,
});

const result = await handler.route(mockRequest);

Expand All @@ -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<Txid>({
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 },
});
});

Expand Down Expand Up @@ -401,11 +486,14 @@ describe('KeyringRequestHandler', () => {
account: 'account-id',
});

it('executes sendTransferq', async () => {
it('executes sendTransfer and propagates canBeMalleable', async () => {
const mockTxid = mock<Txid>({
toString: jest.fn().mockReturnValue('txid'),
});
mockAccountsUseCases.sendTransfer.mockResolvedValue(mockTxid);
mockAccountsUseCases.sendTransfer.mockResolvedValue({
txid: mockTxid,
canBeMalleable: false,
});

const result = await handler.route(mockRequest);

Expand All @@ -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<Txid>({
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 },
});
});

Expand Down
Loading
Loading