diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 6b8303d1..ba4cc763 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -33,6 +33,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix account deletion failing against keyring v2 clients by removing the `AccountDeleted` event emission from the delete flow ([#221](https://github.com/MetaMask/internal-snaps/pull/221)) - v2 clients reject v1 lifecycle events, which aborted the deletion before the account was removed from state. Deletion is client-initiated in v2, so no event is needed. - Ensure certain errors are stringified correctly ([#179](https://github.com/MetaMask/internal-snaps/pull/179)) +- 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. The drain output is exempt from the value check, since it absorbs the remaining balance by design. Only a single appended output is tolerated, and it has to belong to the wallet. Previously only the output count was compared, so a divergent transaction could be signed and broadcast. ## [2.0.1] 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 2a94fa25..639704c9 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -1414,6 +1414,221 @@ 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 appendedChange = identifiableOutput('0014aaaa', 2548n); + const template = mock({ + unsigned_tx: { output: [depositOutput, opReturnOutput] }, + toString: () => 'templateBase64', + }); + const builtPsbt = mock({ + unsigned_tx: { + output: [depositOutput, opReturnOutput, appendedChange], + }, + }); + mockTxBuilder.finish.mockReturnValue(builtPsbt); + mockRepository.get.mockResolvedValueOnce( + accountOwning([appendedChange.script_pubkey]), + ); + + expect(await useCases.fillPsbt('account-id', template)).toBe(builtPsbt); + }); + + it('throws when the built PSBT appends an output that is not ours', async () => { + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const template = mock({ + unsigned_tx: { output: [depositOutput] }, + toString: () => 'templateBase64', + }); + mockTxBuilder.finish.mockReturnValue( + mock({ + unsigned_tx: { + output: [depositOutput, identifiableOutput('5120dddd', 1000n)], + }, + }), + ); + mockRepository.get.mockResolvedValueOnce(accountOwning([])); + + await expect(useCases.fillPsbt('account-id', template)).rejects.toThrow( + 'Built PSBT does not preserve the template outputs', + ); + }); + + it('throws when the built PSBT appends more than one output', async () => { + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const firstAppended = identifiableOutput('0014aaaa', 1000n); + const secondAppended = identifiableOutput('0014eeee', 1000n); + const template = mock({ + unsigned_tx: { output: [depositOutput] }, + toString: () => 'templateBase64', + }); + mockTxBuilder.finish.mockReturnValue( + mock({ + unsigned_tx: { + output: [depositOutput, firstAppended, secondAppended], + }, + }), + ); + mockRepository.get.mockResolvedValueOnce( + accountOwning([ + firstAppended.script_pubkey, + secondAppended.script_pubkey, + ]), + ); + + await expect(useCases.fillPsbt('account-id', template)).rejects.toThrow( + 'Built PSBT does not preserve the template outputs', + ); + }); + + 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, + ); + }); + + it('throws when the rebuild changes the value of the wallet-owned output', async () => { + const depositOutput = identifiableOutput('5120bbbb', 100000n); + const changeOutput = identifiableOutput('0014aaaa', 5000n); + const template = mock({ + unsigned_tx: { output: [depositOutput, changeOutput] }, + toString: () => 'templateBase64', + }); + // first attempt drops the sub-dust drain, so the rebuild adds every + // template output as a fixed recipient and no drain is configured + mockTxBuilder.finish + .mockReturnValueOnce( + mock({ unsigned_tx: { output: [depositOutput] } }), + ) + .mockReturnValueOnce( + mock({ + unsigned_tx: { + output: [depositOutput, identifiableOutput('0014aaaa', 1n)], + }, + }), + ); + mockRepository.get.mockResolvedValueOnce( + accountOwning([changeOutput.script_pubkey]), + ); + + await expect(useCases.fillPsbt('account-id', template)).rejects.toThrow( + 'Built PSBT does not preserve the template outputs', + ); + }); }); 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 e77521f5..c080a74b 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -699,6 +699,16 @@ 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 appended last, so only a trailing output of ours keeps its position. If the template has no output of ours, a change output is added automatically. + const drainOutput = + lastOutput && account.isMine(lastOutput.script_pubkey) + ? lastOutput + : undefined; + + let drainConfigured = drainOutput !== undefined; + let builtPsbt: Psbt; try { let builder = account .buildTx() @@ -706,9 +716,8 @@ 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)) { + for (const txout of templateOutputs) { + if (txout === drainOutput) { builder = builder.drainToByScript(txout.script_pubkey); } else { builder = builder.addRecipientByScript( @@ -717,20 +726,18 @@ export class AccountUseCases { ); } } - let builtPsbt = builder.finish(); + builtPsbt = builder.finish(); - if ( - builtPsbt.unsigned_tx.output.length < - templatePsbt.unsigned_tx.output.length - ) { - // Second attempt: use fixed recipients for all outputs + if (builtPsbt.unsigned_tx.output.length < templateOutputs.length) { + // Second attempt: use fixed recipients for all outputs, so no drain is configured + drainConfigured = false; builder = account .buildTx() .feeRate(feeRateToUse) .unspendable(frozenUTXOs) .untouchedOrdering(); - for (const txout of templatePsbt.unsigned_tx.output) { + for (const txout of templateOutputs) { builder = builder.addRecipientByScript( txout.value, txout.script_pubkey, @@ -738,8 +745,6 @@ export class AccountUseCases { } builtPsbt = builder.finish(); } - - return builtPsbt; } catch (error) { const causeMessage = (error as Error)?.message ?? 'unknown cause'; throw new ValidationError( @@ -752,6 +757,33 @@ export class AccountUseCases { error, ); } + + const builtOutputs = builtPsbt.unsigned_tx.output; + // BDK may append a single change output of ours after the template outputs, and nothing else. + const appended = builtOutputs.slice(templateOutputs.length); + const preserved = + appended.length <= 1 && + appended.every((txout) => account.isMine(txout.script_pubkey)) && + templateOutputs.every( + (txout, index) => + builtOutputs[index]?.script_pubkey.to_hex_string() === + txout.script_pubkey.to_hex_string() && + ((drainConfigured && txout === drainOutput) || + builtOutputs[index]?.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: feeRateToUse, + }, + ); + } + + return builtPsbt; } async #broadcast(