From 54d49c5a0c526a20f8357770cb05f6c827e9e045 Mon Sep 17 00:00:00 2001 From: jeremytsng Date: Mon, 17 Aug 2026 23:54:16 +0700 Subject: [PATCH] fix(bitcoin-wallet-snap): preserve template output order when filling a PSBT Only treat a wallet-owned template output as the drain output when it is the last output. BDK appends the drain output, so a wallet-owned output placed anywhere earlier was silently moved to the end of the transaction, reordering templates that put change before another output. Verify the built transaction against the template before returning it: every template output must appear at its original index with its original script and value, except the drain output, which takes the excess. The previous check compared only the number of outputs, so a transaction whose outputs diverged from the template could still be signed and broadcast. --- packages/bitcoin-wallet-snap/CHANGELOG.md | 6 + .../bitcoin-wallet-snap/snap.manifest.json | 2 +- .../src/use-cases/AccountUseCases.test.ts | 140 ++++++++++++++++++ .../src/use-cases/AccountUseCases.ts | 86 ++++++++--- 4 files changed, 214 insertions(+), 20 deletions(-) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index b4baf62a3..b4b2f0aa5 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Keep the template output order when filling a PSBT ([#157](https://github.com/MetaMask/internal-snaps/pull/157)) + - A template output belonging to the wallet is now only used as the drain output when it is the last output. Previously any such output was moved to the end of the transaction, silently reordering templates that place change before another output. + - Filling a PSBT now fails with a `ValidationError` when the built transaction does not reproduce every template output, at its original index, with its original value. Previously only the output count was compared, so a divergent transaction could be signed and broadcast. + ## [2.0.1] ### Fixed diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index f594f203d..17ce781de 100644 --- a/packages/bitcoin-wallet-snap/snap.manifest.json +++ b/packages/bitcoin-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "jV649WZbbfbj3FpOMD5U/xDPuRD0t4F+pxCoy08a/O0=", + "shasum": "B9w8DmGsWcU8IBkEJemc1WCP46/jbdO4uBwcmDgtvg4=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts index 694d55691..830349acf 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -1595,6 +1595,146 @@ describe('AccountUseCases', () => { // Result should be the rebuilt PSBT with all outputs preserved expect(result).toBe(rebuiltPsbt); }); + + const identifiableOutput = (scriptHex: string, sats: bigint): TxOut => { + const scriptPubkey = mock(); + scriptPubkey.to_hex_string.mockReturnValue(scriptHex); + const value = mock(); + value.to_sat.mockReturnValue(sats); + + return mock({ script_pubkey: scriptPubkey, value }); + }; + + const accountOwning = (owned: ScriptBuf[]): BitcoinAccount => { + const account = mock({ + id: 'account-id', + network: 'bitcoin', + isMine: (script: ScriptBuf) => owned.includes(script), + capabilities: [AccountCapability.FillPsbt], + }); + account.buildTx.mockReturnValue(mockTxBuilder); + return account; + }; + + it('adds every template output as a fixed recipient when the wallet-owned output is not last', async () => { + const changeOutput = identifiableOutput('0014aaaa', 2548n); + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const template = mock({ + unsigned_tx: { output: [changeOutput, depositOutput] }, + toString: () => 'templateBase64', + }); + mockTxBuilder.finish.mockReturnValue( + mock({ + unsigned_tx: { output: [changeOutput, depositOutput] }, + }), + ); + mockRepository.get.mockResolvedValueOnce( + accountOwning([changeOutput.script_pubkey]), + ); + + await useCases.fillPsbt('account-id', template); + + expect(mockTxBuilder.drainToByScript).not.toHaveBeenCalled(); + expect(mockTxBuilder.addRecipientByScript).toHaveBeenCalledTimes(2); + expect(mockTxBuilder.addRecipientByScript).toHaveBeenNthCalledWith( + 1, + changeOutput.value, + changeOutput.script_pubkey, + ); + expect(mockTxBuilder.addRecipientByScript).toHaveBeenNthCalledWith( + 2, + depositOutput.value, + depositOutput.script_pubkey, + ); + }); + + it('throws when the built outputs are reordered against the template', async () => { + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const opReturnOutput = identifiableOutput('6a3ecccc', 0n); + const template = mock({ + unsigned_tx: { output: [depositOutput, opReturnOutput] }, + toString: () => 'templateBase64', + }); + mockTxBuilder.finish.mockReturnValue( + mock({ + unsigned_tx: { + output: [ + opReturnOutput, + identifiableOutput('0014aaaa', 2548n), + depositOutput, + ], + }, + }), + ); + mockRepository.get.mockResolvedValueOnce(accountOwning([])); + + await expect(useCases.fillPsbt('account-id', template)).rejects.toThrow( + 'Built PSBT does not preserve the template outputs', + ); + }); + + it('throws when a built output value diverges from the template', async () => { + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const template = mock({ + unsigned_tx: { output: [depositOutput] }, + toString: () => 'templateBase64', + }); + mockTxBuilder.finish.mockReturnValue( + mock({ + unsigned_tx: { output: [identifiableOutput('5120bbbb', 1n)] }, + }), + ); + mockRepository.get.mockResolvedValueOnce(accountOwning([])); + + await expect(useCases.fillPsbt('account-id', template)).rejects.toThrow( + 'Built PSBT does not preserve the template outputs', + ); + }); + + it('accepts a built PSBT that appends a change output after the template outputs', async () => { + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const opReturnOutput = identifiableOutput('6a3ecccc', 0n); + const template = mock({ + unsigned_tx: { output: [depositOutput, opReturnOutput] }, + toString: () => 'templateBase64', + }); + const builtPsbt = mock({ + unsigned_tx: { + output: [ + depositOutput, + opReturnOutput, + identifiableOutput('0014aaaa', 2548n), + ], + }, + }); + mockTxBuilder.finish.mockReturnValue(builtPsbt); + mockRepository.get.mockResolvedValueOnce(accountOwning([])); + + expect(await useCases.fillPsbt('account-id', template)).toBe(builtPsbt); + }); + + it('accepts the drained output taking a value the template did not specify', async () => { + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const changeOutput = identifiableOutput('0014aaaa', 1000n); + const template = mock({ + unsigned_tx: { output: [depositOutput, changeOutput] }, + toString: () => 'templateBase64', + }); + const builtPsbt = mock({ + unsigned_tx: { + output: [depositOutput, identifiableOutput('0014aaaa', 2548n)], + }, + }); + mockTxBuilder.finish.mockReturnValue(builtPsbt); + mockRepository.get.mockResolvedValueOnce( + accountOwning([changeOutput.script_pubkey]), + ); + + expect(await useCases.fillPsbt('account-id', template)).toBe(builtPsbt); + expect(mockTxBuilder.drainToByScript).toHaveBeenCalledWith( + changeOutput.script_pubkey, + ); + }); }); describe('computeFee', () => { diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index 85c8b34d9..2e089cffc 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -779,6 +779,15 @@ export class AccountUseCases { const frozenUTXOs = await this.#repository.getFrozenUTXOs(account.id); const feeRateToUse = feeRate ?? (await this.getFallbackFeeRate(account)); + const templateOutputs = templatePsbt.unsigned_tx.output; + const lastOutput = templateOutputs[templateOutputs.length - 1]; + // The drain output is always appended last, so only a trailing output of ours can keep its position. Any other output of ours stays a fixed recipient. + const drainIndex = + lastOutput && account.isMine(lastOutput.script_pubkey) + ? templateOutputs.length - 1 + : -1; + + let builtPsbt: Psbt; try { let builder = account .buildTx() @@ -786,23 +795,16 @@ export class AccountUseCases { .unspendable(frozenUTXOs) .untouchedOrdering(); // we need to strictly adhere to the template output order. Many protocols use the order (e.g: 1: deposit, 2: OP_RETURN, 3: change) - for (const txout of templatePsbt.unsigned_tx.output) { - // if the PSBT contains an output that is sending to ourselves, we change its value. If the PSBT contains no change outputs, one will automatically be added. - if (account.isMine(txout.script_pubkey)) { - builder = builder.drainToByScript(txout.script_pubkey); - } else { - builder = builder.addRecipientByScript( - txout.value, - txout.script_pubkey, - ); - } - } - let builtPsbt = builder.finish(); + templateOutputs.forEach((txout, index) => { + // the drain output takes the excess, so its template value is replaced. If the template has no output of ours, one will automatically be added. + builder = + index === drainIndex + ? builder.drainToByScript(txout.script_pubkey) + : builder.addRecipientByScript(txout.value, txout.script_pubkey); + }); + builtPsbt = builder.finish(); - if ( - builtPsbt.unsigned_tx.output.length < - templatePsbt.unsigned_tx.output.length - ) { + if (builtPsbt.unsigned_tx.output.length < templateOutputs.length) { // Second attempt: use fixed recipients for all outputs builder = account .buildTx() @@ -810,7 +812,7 @@ export class AccountUseCases { .unspendable(frozenUTXOs) .untouchedOrdering(); - for (const txout of templatePsbt.unsigned_tx.output) { + for (const txout of templateOutputs) { builder = builder.addRecipientByScript( txout.value, txout.script_pubkey, @@ -818,8 +820,6 @@ export class AccountUseCases { } builtPsbt = builder.finish(); } - - return builtPsbt; } catch (error) { const causeMessage = (error as Error)?.message ?? 'unknown cause'; throw new ValidationError( @@ -832,6 +832,54 @@ export class AccountUseCases { error, ); } + + this.#assertTemplateOutputsPreserved( + account, + templatePsbt, + builtPsbt, + drainIndex, + feeRateToUse, + ); + + return builtPsbt; + } + + #assertTemplateOutputsPreserved( + account: BitcoinAccount, + templatePsbt: Psbt, + builtPsbt: Psbt, + drainIndex: number, + feeRate: number, + ): void { + const templateOutputs = templatePsbt.unsigned_tx.output; + const builtOutputs = builtPsbt.unsigned_tx.output; + + const preserved = + builtOutputs.length >= templateOutputs.length && + templateOutputs.every((txout, index) => { + const builtOutput = builtOutputs[index]; + if (!builtOutput) { + return false; + } + return ( + builtOutput.script_pubkey.to_hex_string() === + txout.script_pubkey.to_hex_string() && + (index === drainIndex || + builtOutput.value.to_sat() === txout.value.to_sat()) + ); + }); + + if (!preserved) { + throw new ValidationError( + 'Built PSBT does not preserve the template outputs', + { + id: account.id, + templatePsbt: templatePsbt.toString(), + builtPsbt: builtPsbt.toString(), + feeRate, + }, + ); + } } async #broadcast(