Skip to content

Commit 2ca0baf

Browse files
committed
fix(sdk-coin-stx): validate sbtcWithdrawParams in verifyTransaction
verifyTransaction only checked txParams.recipients against explainTransaction outputs, sbtcWithdrawParams (amount, btcAddress, maxFee) was never actually verified. Add a type guard + dedicated verifySbtcWithdrawTransaction path that parses the raw tx via SbtcWithdrawBuilder and checks amount/maxFee/btcAddress against the expected params. TICKET: CSHLD-1451
1 parent 91c613d commit 2ca0baf

4 files changed

Lines changed: 169 additions & 1 deletion

File tree

modules/sdk-coin-stx/src/lib/index.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,6 @@ export * from './keyPair';
33
export * from './transaction';
44
export * from './transactionBuilderFactory';
55
export * from './sbtcWithdrawBuilder';
6+
export * from './btcAddressUtils';
7+
export * from './iface';
68
export * as Utils from './utils';

modules/sdk-coin-stx/src/lib/sbtcWithdrawBuilder.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,35 @@ export class SbtcWithdrawBuilder extends AbstractContractBuilder {
105105
this._isDeserialized = true;
106106
}
107107

108+
/**
109+
* Get the withdrawal params decoded from the deserialized/built transaction, including the
110+
* raw sBTC recipient version and hash bytes (as opposed to a btcAddress string, which cannot
111+
* be recovered from the on-chain args alone).
112+
*/
113+
getWithdrawParams():
114+
| { amount: string; maxFee: string; recipientVersion: number; recipientHashBytes: Buffer }
115+
| undefined {
116+
if (!this._withdrawParams) {
117+
return undefined;
118+
}
119+
const payload = this.transaction.stxTransaction.payload as ContractCallPayload;
120+
const recipientTuple = payload.functionArgs[1];
121+
if (recipientTuple?.type !== ClarityType.Tuple) {
122+
return undefined;
123+
}
124+
const versionBuf = recipientTuple.data['version'];
125+
const hashbytesBuf = recipientTuple.data['hashbytes'];
126+
if (versionBuf?.type !== ClarityType.Buffer || hashbytesBuf?.type !== ClarityType.Buffer) {
127+
return undefined;
128+
}
129+
return {
130+
amount: this._withdrawParams.amount,
131+
maxFee: this._withdrawParams.maxFee,
132+
recipientVersion: versionBuf.buffer[0],
133+
recipientHashBytes: Buffer.from(hashbytesBuf.buffer),
134+
};
135+
}
136+
108137
/** @inheritdoc */
109138
protected async buildImplementation(): Promise<Transaction> {
110139
if (!this._withdrawParams) {

modules/sdk-coin-stx/src/stx.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
MultisigType,
1212
multisigTypes,
1313
SignedTransaction,
14+
TransactionParams,
1415
TransactionRecipient,
1516
TransactionType,
1617
VerifyAddressOptions,
@@ -114,6 +115,11 @@ export class Stx extends BaseCoin {
114115
if (!rawTx) {
115116
throw new Error('missing required tx prebuild property txHex');
116117
}
118+
119+
if (this.hasSbtcWithdrawParams(txParams)) {
120+
return this.verifySbtcWithdrawTransaction(rawTx, txParams.sbtcWithdrawParams);
121+
}
122+
117123
const explainedTx = await this.explainTransaction({ txHex: rawTx, feeInfo: { fee: '' } });
118124
const recipient = txParams.recipients?.[0];
119125
if (recipient !== undefined && explainedTx) {
@@ -149,6 +155,54 @@ export class Stx extends BaseCoin {
149155
return true;
150156
}
151157

158+
private hasSbtcWithdrawParams(
159+
txParams: TransactionParams
160+
): txParams is TransactionParams & { sbtcWithdrawParams: StxLib.SbtcWithdrawParams } {
161+
return 'sbtcWithdrawParams' in txParams && txParams.sbtcWithdrawParams !== undefined;
162+
}
163+
164+
/**
165+
* Verify an sBTC withdrawal (burn) transaction matches the expected withdrawal params.
166+
*
167+
* @param rawTx - the raw (built) transaction hex from txPrebuild
168+
* @param expected - the sbtcWithdrawParams supplied by the caller in txParams
169+
*/
170+
private async verifySbtcWithdrawTransaction(rawTx: string, expected: StxLib.SbtcWithdrawParams): Promise<boolean> {
171+
const factory = new StxLib.TransactionBuilderFactory(coins.get(this.getChain()));
172+
const builder = factory.from(rawTx);
173+
if (!(builder instanceof StxLib.SbtcWithdrawBuilder)) {
174+
throw new Error('Tx is not a valid sBTC withdrawal transaction');
175+
}
176+
177+
const actual = builder.getWithdrawParams();
178+
if (!actual) {
179+
throw new Error('Unable to parse sBTC withdrawal params from tx');
180+
}
181+
182+
if (BigInt(actual.amount) !== BigInt(expected.amount)) {
183+
throw new Error(
184+
`Tx sBTC withdrawal amount does not match expected amount: expected ${expected.amount} but got ${actual.amount}`
185+
);
186+
}
187+
if (BigInt(actual.maxFee) !== BigInt(expected.maxFee)) {
188+
throw new Error(
189+
`Tx sBTC withdrawal maxFee does not match expected maxFee: expected ${expected.maxFee} but got ${actual.maxFee}`
190+
);
191+
}
192+
193+
const decodedExpected = StxLib.decodeBtcAddress(expected.btcAddress);
194+
if (
195+
decodedExpected.version !== actual.recipientVersion ||
196+
!decodedExpected.hashBytes.equals(actual.recipientHashBytes)
197+
) {
198+
throw new Error(
199+
`Tx sBTC withdrawal btcAddress does not match expected btcAddress: expected ${expected.btcAddress}`
200+
);
201+
}
202+
203+
return true;
204+
}
205+
152206
/**
153207
* Check if address is valid, then make sure it matches the base address.
154208
*

modules/sdk-coin-stx/test/unit/stx.ts

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,10 @@ import { BitGoAPI } from '@bitgo/sdk-api';
55
import { Wallet } from '@bitgo/sdk-core';
66
import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test';
77
import { coins } from '@bitgo/statics';
8-
import { cvToString } from '@stacks/transactions';
8+
import { cvToString, pubKeyfromPrivKey, publicKeyToString } from '@stacks/transactions';
99

1010
import * as testData from '../fixtures';
11+
import * as resources from './resources';
1112
import { Stx, StxLib, Tstx } from '../../src';
1213
import { RecoveryInfo, RecoveryOptions, RecoveryTransaction } from '../../src/lib/iface';
1314

@@ -431,6 +432,88 @@ describe('STX:', function () {
431432
});
432433
});
433434

435+
describe('Verify sBTC Withdraw Transaction', function () {
436+
const factory = new StxLib.TransactionBuilderFactory(coins.get('tstx'));
437+
const prvKeysString = resources.prvKeysString.slice(0, 2);
438+
439+
const buildWithdrawTx = async (withdrawParams: { amount: string; btcAddress: string; maxFee: string }) => {
440+
const builder = factory.getSbtcWithdrawBuilder();
441+
builder.fee({ fee: '1000' });
442+
builder.nonce(1);
443+
const pubKeys = prvKeysString.map((prv) => publicKeyToString(pubKeyfromPrivKey(prv)));
444+
builder.fromPubKey(pubKeys);
445+
builder.numberSignatures(2);
446+
builder.withdraw(withdrawParams);
447+
builder.sign({ key: prvKeysString[0] });
448+
builder.sign({ key: prvKeysString[1] });
449+
const tx = await builder.build();
450+
return tx.toBroadcastFormat();
451+
};
452+
453+
it('should succeed to verify a matching sBTC withdrawal', async function () {
454+
const withdrawParams = {
455+
amount: '1000',
456+
btcAddress: 'bc1prxl88w47srqh703pxv567q47e7epzume4nlz8cgewfhtuenn8ngqgwm80w',
457+
maxFee: '10000',
458+
};
459+
const txHex = await buildWithdrawTx(withdrawParams);
460+
const txParams = {
461+
sbtcWithdrawParams: withdrawParams,
462+
recipients: [{ address: 'SM1K9VF5GN48Q0AC2C7SB8WM5N5NR6DYC9VM3QEJE', amount: '10' }],
463+
};
464+
const result = await basecoin.verifyTransaction({ txPrebuild: { txHex }, txParams });
465+
result.should.equal(true);
466+
});
467+
468+
it('should fail to verify with wrong amount', async function () {
469+
const withdrawParams = {
470+
amount: '1000',
471+
btcAddress: 'bc1prxl88w47srqh703pxv567q47e7epzume4nlz8cgewfhtuenn8ngqgwm80w',
472+
maxFee: '10000',
473+
};
474+
const txHex = await buildWithdrawTx(withdrawParams);
475+
const txParams = {
476+
sbtcWithdrawParams: { ...withdrawParams, amount: '9999' },
477+
recipients: [{ address: 'SM1K9VF5GN48Q0AC2C7SB8WM5N5NR6DYC9VM3QEJE', amount: '10' }],
478+
};
479+
await basecoin
480+
.verifyTransaction({ txPrebuild: { txHex }, txParams })
481+
.should.be.rejectedWith(/sBTC withdrawal amount does not match/);
482+
});
483+
484+
it('should fail to verify with wrong maxFee', async function () {
485+
const withdrawParams = {
486+
amount: '1000',
487+
btcAddress: 'bc1prxl88w47srqh703pxv567q47e7epzume4nlz8cgewfhtuenn8ngqgwm80w',
488+
maxFee: '10000',
489+
};
490+
const txHex = await buildWithdrawTx(withdrawParams);
491+
const txParams = {
492+
sbtcWithdrawParams: { ...withdrawParams, maxFee: '1' },
493+
recipients: [{ address: 'SM1K9VF5GN48Q0AC2C7SB8WM5N5NR6DYC9VM3QEJE', amount: '10' }],
494+
};
495+
await basecoin
496+
.verifyTransaction({ txPrebuild: { txHex }, txParams })
497+
.should.be.rejectedWith(/sBTC withdrawal maxFee does not match/);
498+
});
499+
500+
it('should fail to verify with wrong btcAddress', async function () {
501+
const withdrawParams = {
502+
amount: '1000',
503+
btcAddress: 'bc1prxl88w47srqh703pxv567q47e7epzume4nlz8cgewfhtuenn8ngqgwm80w',
504+
maxFee: '10000',
505+
};
506+
const txHex = await buildWithdrawTx(withdrawParams);
507+
const txParams = {
508+
sbtcWithdrawParams: { ...withdrawParams, btcAddress: '1BvBMSEYstWetqTFn5Au4m4GFg7xJaNVN2' },
509+
recipients: [{ address: 'SM1K9VF5GN48Q0AC2C7SB8WM5N5NR6DYC9VM3QEJE', amount: '10' }],
510+
};
511+
await basecoin
512+
.verifyTransaction({ txPrebuild: { txHex }, txParams })
513+
.should.be.rejectedWith(/sBTC withdrawal btcAddress does not match/);
514+
});
515+
});
516+
434517
describe('Recover Transaction STX', function () {
435518
before(function () {
436519
nock.enableNetConnect();

0 commit comments

Comments
 (0)