From d52cc25654e72fc8fddefd46b26764488a7db69a Mon Sep 17 00:00:00 2001 From: Deepak Jangid Date: Thu, 5 Nov 2026 17:27:58 +0530 Subject: [PATCH 1/2] fix(abstract-eth): harden EVM TSS payment and consolidation checks verifyTssTransaction compared only the destination and amount of a single native or ERC-20 transfer declared as `transfer`. It skipped stored `payment` intents, batches, the token contract, native value on token transfers, and the calldata shape, so mismatched bytes could reach signing. Payments: stored `payment` is treated as `transfer`. Explicit calldata must match to/value/data exactly; a native send carries no calldata; an ERC-20 transfer must be canonical, carry no native value and call the declared token contract. Multi-recipient payments must use the network batcher with matching batch/batchTransferFrom arrays, the summed value for native and zero for tokens. Consolidation keeps its base-address checks and adds zero native value, canonical calldata and the declared token contract for ERC-20 sweeps, and no calldata for native sweeps. Sweep amounts are not compared. Unresolved token or batcher metadata and intent types without a verifier log a warning and never suppress other checks. Signing routing and legacy verification are unchanged. Ticket: CECHO-2460 Co-Authored-By: Claude Opus 5.5 --- .../src/abstractEthLikeNewCoins.ts | 369 +++++++++++++++--- modules/sdk-coin-bsc/test/unit/bsc.ts | 2 +- .../test/unit/tssPaymentVerification.ts | 361 +++++++++++++++++ modules/sdk-coin-xdc/test/unit/xdcToken.ts | 3 +- 4 files changed, 674 insertions(+), 61 deletions(-) create mode 100644 modules/sdk-coin-eth/test/unit/tssPaymentVerification.ts diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index ff45cad8def..b07629af0f4 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -20,6 +20,7 @@ import { InvalidAddressError, InvalidAddressVerificationObjectPropertyError, InvalidTransactionError, + ITransactionRecipient, IWallet, KeyPair, MPCSweepRecoveryOptions, @@ -588,6 +589,58 @@ export const optionalDeps = { }, }; +/** Intent types other than payments that verifyTssTransaction already verifies. */ +const TSS_DEDICATED_INTENT_TYPES = new Set(['defiApprove', 'defiDeposit', 'defiWithdraw', 'wrapApprove']); + +type TssDecodedArg = ReturnType[number]; + +const TSS_ERC20_TRANSFER_TYPES = ['address', 'uint256']; +const TSS_NATIVE_BATCH_TYPES = ['address[]', 'uint256[]']; +const TSS_TOKEN_BATCH_TYPES = ['address', 'address[]', 'uint256[]']; + +function tssMethodId(name: string, types: string[]): string { + return addHexPrefix(optionalDeps.ethAbi.methodID(name, types).toString('hex')).toLowerCase(); +} + +function isEmptyTssCalldata(data?: string): boolean { + return !data || data === '0x'; +} + +function tssValuesEqual(a: unknown, b: unknown): boolean { + try { + return BigInt(String(a)) === BigInt(String(b)); + } catch { + return false; + } +} + +/** ABI-decode the arguments after the 4-byte selector, without splitting on the selector bytes. */ +function decodeTssCalldataArgs(data: string, types: string[]): TssDecodedArg[] { + return getRawDecoded(types, Buffer.from(stripHexPrefix(data).slice(8), 'hex')); +} + +/** Prefix decoded addresses so the values can be passed back to rawEncode. */ +function toTssEncodable(value: TssDecodedArg): unknown { + if (Array.isArray(value)) { + return value.map(toTssEncodable); + } + return typeof value === 'string' ? addHexPrefix(value) : value; +} + +/** + * True when the arguments after the selector are exactly the canonical ABI encoding of `types`: + * no trailing bytes, no dirty address padding, no non-standard dynamic offsets. + */ +function isCanonicalTssCalldata(data: string, types: string[]): boolean { + try { + const args = stripHexPrefix(data).slice(8).toLowerCase(); + const decoded = decodeTssCalldataArgs(data, types); + return optionalDeps.ethAbi.rawEncode(types, decoded.map(toTssEncodable)).toString('hex') === args; + } catch { + return false; + } +} + export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implements CoinWithSignableConsistency { static hopTransactionSalt = 'bitgoHopAddressRequestSalt'; protected readonly sendMethodName: 'sendMultiSig' | 'sendMultiSigToken'; @@ -3549,6 +3602,219 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem } } + /** + * Report a check verifyTssTransaction could not complete. Only the intent type and the missing + * check are logged: addresses, amounts and calldata are not. + */ + protected warnTssVerificationIncomplete(intentType: string, reason: string): void { + console.warn(`[tss verification] ${intentType}: ${reason}`); + } + + /** + * Resolve a declared token to its contract from local statics. With no token name, a token coin + * declares itself and a base coin declares no asset (`isToken: false`). + */ + private resolveTssToken(tokenName?: string): { + isToken: boolean; + contract?: string; + confidential?: boolean; + reason?: string; + } { + // Token coins (e.g. `txdc:usdc`) resolve through their own chain name. + const self = coins.getOrUndefined(this.getChain()); + const network = self?.network ?? this.staticsCoin?.network; + const declared = tokenName ?? (self?.isToken ? self.name : undefined); + if (!declared) { + return { isToken: false }; + } + const token = coins.getOrUndefined(declared); + if (token && !token.isToken && token.network === network) { + return { isToken: false }; // the chain's own native coin + } + const contract = (token as { contractAddress?: string } | undefined)?.contractAddress; + if (!token || token.network !== network || !contract || !/^0x[0-9a-fA-F]{40}$/.test(contract)) { + return { isToken: true, reason: `token '${declared}' cannot be resolved to a contract on this network` }; + } + if (token instanceof Erc7984Coin) { + return { isToken: true, confidential: true, reason: `token '${declared}' is an ERC-7984 token` }; + } + return { isToken: true, contract: contract.toLowerCase() }; + } + + /** + * Verify a single-recipient payment: explicit calldata must match exactly; otherwise the bytes + * must be a plain native send or a canonical ERC-20 transfer of the declared token. + */ + private async verifyTssSinglePayment( + txParams: EthTransactionParams, + recipient: ITransactionRecipient, + txHex: string | undefined, + throwRecipientMismatch: (message: string, mismatchedRecipients: Recipient[]) => Promise + ): Promise { + const txBuilder = this.getTransactionBuilder(); + txBuilder.from(txHex); + const txJson = (await txBuilder.build()).toJson(); + const to = (txJson.to ?? '').toLowerCase(); + const value = txJson.value ?? '0'; + const data = (txJson.data || '0x').toLowerCase(); + const mismatch = (message: string, address = to, amount = value): Promise => + throwRecipientMismatch(message, [{ address, amount }]); + + // Explicit dApp / WalletConnect calldata: the caller authorised exactly these bytes, including + // any native value sent with them. + if (!isEmptyTssCalldata(recipient.data)) { + if (to !== recipient.address.toLowerCase()) { + await mismatch('destination address does not match with the recipient address'); + } + if (!tssValuesEqual(value, recipient.amount)) { + await mismatch('the transaction amount in txPrebuild does not match the value given by client'); + } + if (data !== addHexPrefix(recipient.data as string).toLowerCase()) { + await mismatch('transaction calldata does not match the calldata given by client'); + } + return; + } + + const token = this.resolveTssToken(recipient.tokenName ?? txParams.tokenName); + + if (isEmptyTssCalldata(data)) { + if (token.isToken) { + await mismatch('token payment carries no ERC-20 transfer calldata'); + } + if (!tssValuesEqual(value, recipient.amount)) { + await mismatch('the transaction amount in txPrebuild does not match the value given by client'); + } + if (to !== recipient.address.toLowerCase()) { + await mismatch('destination address does not match with the recipient address'); + } + return; + } + + const transferMethodId = tssMethodId('transfer', TSS_ERC20_TRANSFER_TYPES); + if (!data.startsWith(transferMethodId)) { + if (token.confidential) { + this.warnTssVerificationIncomplete('transfer', `${token.reason}: the payment calldata was not verified`); + return; + } + await mismatch('payment calldata is not a native send or an ERC-20 transfer(address,uint256)'); + } + if (!isCanonicalTssCalldata(data, TSS_ERC20_TRANSFER_TYPES)) { + await mismatch('ERC-20 transfer calldata is not canonical'); + } + const [innerRecipient, innerAmount] = decodeTssCalldataArgs(data, TSS_ERC20_TRANSFER_TYPES); + const decodedRecipient = addHexPrefix(innerRecipient as string).toLowerCase(); + const decodedAmount = (innerAmount as BN).toString(10); + if (!tssValuesEqual(decodedAmount, recipient.amount)) { + await mismatch( + 'the transaction amount in txPrebuild does not match the value given by client', + decodedRecipient, + decodedAmount + ); + } + if (decodedRecipient !== recipient.address.toLowerCase()) { + await mismatch('destination address does not match with the recipient address', decodedRecipient, decodedAmount); + } + if (!tssValuesEqual(value, '0')) { + await mismatch('token transfer must not carry native value'); + } + if (token.contract) { + if (to !== token.contract) { + await mismatch('transaction is not sent to the declared token contract'); + } + } else { + // No declared token (historic behaviour: recipient and amount are still enforced above) or a + // token statics cannot resolve: the contract being called is unverified, not mismatched. + this.warnTssVerificationIncomplete( + 'transfer', + `${token.reason ?? 'payment declares no token'}: the token contract was not verified` + ); + } + } + + /** + * Verify a multi-recipient payment sent through the network batcher: `batch` for the native + * asset (value = sum of amounts), `batchTransferFrom` for a token (value = 0). + */ + private async verifyTssBatchPayment( + txParams: EthTransactionParams, + recipients: ITransactionRecipient[], + txHex: string | undefined, + throwRecipientMismatch: (message: string, mismatchedRecipients: Recipient[]) => Promise + ): Promise { + const txBuilder = this.getTransactionBuilder(); + txBuilder.from(txHex); + const txJson = (await txBuilder.build()).toJson(); + const to = (txJson.to ?? '').toLowerCase(); + const value = txJson.value ?? '0'; + const data = (txJson.data || '0x').toLowerCase(); + const mismatch = (message: string, address = to, amount = value): Promise => + throwRecipientMismatch(message, [{ address, amount }]); + + // A missing batcher is incomplete verification; it must not hide the comparisons below. + const batcher = this.getNetwork()?.batcherContractAddress; + if (!batcher) { + this.warnTssVerificationIncomplete( + 'transfer', + 'no batcher contract is configured: the batch destination was not verified' + ); + } else if (to !== batcher.toLowerCase()) { + await mismatch('batch transaction is not sent to the network batcher contract'); + } + + const nativeMethodId = tssMethodId('batch', TSS_NATIVE_BATCH_TYPES); + const tokenMethodId = tssMethodId('batchTransferFrom', TSS_TOKEN_BATCH_TYPES); + const isNativeBatch = data.startsWith(nativeMethodId); + if (!isNativeBatch && !data.startsWith(tokenMethodId)) { + await mismatch( + 'batch payment calldata is not batch(address[],uint256[]) or batchTransferFrom(address,address[],uint256[])' + ); + } + const types = isNativeBatch ? TSS_NATIVE_BATCH_TYPES : TSS_TOKEN_BATCH_TYPES; + if (!isCanonicalTssCalldata(data, types)) { + await mismatch('batch calldata is not canonical'); + } + const decoded = decodeTssCalldataArgs(data, types); + const [addresses, amounts] = (isNativeBatch ? decoded : decoded.slice(1)) as [string[], BN[]]; + if (addresses.length !== recipients.length) { + await mismatch(`batch calldata has ${addresses.length} recipients but the intent has ${recipients.length}`); + } + for (let i = 0; i < recipients.length; i++) { + const address = addHexPrefix(addresses[i]).toLowerCase(); + const amount = amounts[i].toString(10); + if (address !== recipients[i].address.toLowerCase() || !tssValuesEqual(amount, recipients[i].amount)) { + await mismatch(`batch calldata entry ${i} does not match the intent recipient`, address, amount); + } + } + + // Each recipient declares its own asset; a batch moves exactly one. + const tokens = recipients.map((recipient) => this.resolveTssToken(recipient.tokenName ?? txParams.tokenName)); + if (isNativeBatch) { + if (tokens.some((token) => token.isToken)) { + await mismatch('token batch payment must use batchTransferFrom calldata'); + } + const total = recipients.reduce((sum, recipient) => sum + BigInt(String(recipient.amount)), 0n); + if (!tssValuesEqual(value, total.toString())) { + await mismatch('batch transaction value does not match the sum of intent amounts'); + } + return; + } + + if (!tssValuesEqual(value, '0')) { + await mismatch('token batch transaction must not carry native value'); + } + const tokenContract = addHexPrefix(decoded[0] as string).toLowerCase(); + for (const token of tokens) { + if (!token.contract) { + this.warnTssVerificationIncomplete( + 'transfer', + `${token.reason ?? 'batch recipient declares no token'}: the batch token contract was not verified` + ); + } else if (token.contract !== tokenContract) { + await mismatch('batchTransferFrom token contract does not match the declared token', tokenContract); + } + } + } + /** * @param {VerifyEthTransactionOptions} params * @param {TransactionParams} params.txParams - params object passed to send @@ -3593,67 +3859,23 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem throw new Error('tx cannot be both a batch and hop transaction'); } - if (txParams.type && ['transfer'].includes(txParams.type)) { - if (txParams.recipients && txParams.recipients.length === 1) { - const recipients = txParams.recipients; - const expectedAmount = recipients[0].amount.toString(); - const expectedDestination = recipients[0].address; - - const txBuilder = this.getTransactionBuilder(); - txBuilder.from(txPrebuild.txHex); - const tx = await txBuilder.build(); - const txJson = tx.toJson(); - if (txJson.data === '0x') { - if (expectedAmount !== txJson.value) { - await throwRecipientMismatch( - 'the transaction amount in txPrebuild does not match the value given by client', - [{ address: txJson.to, amount: txJson.value }] - ); - } - if (expectedDestination.toLowerCase() !== txJson.to.toLowerCase()) { - await throwRecipientMismatch('destination address does not match with the recipient address', [ - { address: txJson.to, amount: txJson.value }, - ]); - } - } else if (txJson.data.startsWith('0xa9059cbb')) { - const [recipientAddress, amount] = getRawDecoded( - ['address', 'uint256'], - getBufferedByteCode('0xa9059cbb', txJson.data) - ); - - // Check if recipients[0].data exists (WalletConnect flow) - let expectedRecipientAddress: string; - let expectedTokenAmount: string; - const recipientData = (recipients[0] as any).data; + // Stored tx requests persist a payment as `payment`; callers declare it as `transfer`. + const txType = txParams.type === 'payment' ? 'transfer' : txParams.type; - if (recipientData && recipientData.startsWith('0xa9059cbb')) { - // WalletConnect: decode expected recipient and amount from recipients[0].data - const [expectedRecipient, expectedAmount] = getRawDecoded( - ['address', 'uint256'], - getBufferedByteCode('0xa9059cbb', recipientData) - ); - expectedRecipientAddress = addHexPrefix(expectedRecipient.toString()).toLowerCase(); - expectedTokenAmount = expectedAmount.toString(); - } else { - // Normal flow: use recipients[0].address and recipients[0].amount - expectedRecipientAddress = expectedDestination.toLowerCase(); - expectedTokenAmount = expectedAmount; - } - - if (expectedTokenAmount !== amount.toString()) { - await throwRecipientMismatch( - 'the transaction amount in txPrebuild does not match the value given by client', - [{ address: addHexPrefix(recipientAddress.toString()), amount: amount.toString() }] - ); - } - - if (expectedRecipientAddress !== addHexPrefix(recipientAddress.toString()).toLowerCase()) { - await throwRecipientMismatch('destination address does not match with the recipient address', [ - { address: addHexPrefix(recipientAddress.toString()), amount: amount.toString() }, - ]); - } - } + if (txType === 'transfer') { + const recipients = txParams.recipients ?? []; + if (recipients.length === 0) { + this.warnTssVerificationIncomplete(txType, 'no recipients were given: the transaction was not verified'); + } else if (recipients.length === 1) { + await this.verifyTssSinglePayment(txParams, recipients[0], txPrebuild.txHex, throwRecipientMismatch); + } else { + await this.verifyTssBatchPayment(txParams, recipients, txPrebuild.txHex, throwRecipientMismatch); } + } else if (txType && !TSS_DEDICATED_INTENT_TYPES.has(txType) && !params.verification?.consolidationToBaseAddress) { + this.warnTssVerificationIncomplete( + txType, + 'no verifier is implemented for this intent type: the transaction was not compared against the intent' + ); } if (txParams.type && ['defiApprove', 'defiDeposit', 'defiWithdraw'].includes(txParams.type)) { @@ -3792,11 +4014,40 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem { address: decodedRecipient, amount: txJson.value }, ]); } + if (!isCanonicalTssCalldata(txJson.data, TSS_ERC20_TRANSFER_TYPES)) { + await throwRecipientMismatch('Consolidation transaction calldata is not a canonical ERC-20 transfer', [ + { address: decodedRecipient, amount: txJson.value }, + ]); + } + if (!tssValuesEqual(txJson.value, '0')) { + await throwRecipientMismatch('Consolidation token transfer must not carry native value', [ + { address: txJson.to, amount: txJson.value }, + ]); + } + // Sweep amounts are not compared: a consolidation moves whatever the address holds. + const token = this.resolveTssToken(txParams.tokenName); + if (token.contract) { + if (txJson.to.toLowerCase() !== token.contract) { + await throwRecipientMismatch('Consolidation transaction does not call the declared token contract', [ + { address: txJson.to, amount: txJson.value }, + ]); + } + } else { + this.warnTssVerificationIncomplete( + 'consolidate', + `${token.reason}: the consolidation token contract was not verified` + ); + } } else if (txJson.to.toLowerCase() !== baseAddress.toLowerCase()) { // Native coin consolidation: txJson.to is the actual recipient await throwRecipientMismatch('Consolidation transaction recipient does not match wallet base address', [ { address: txJson.to, amount: txJson.value }, ]); + } else if (!isEmptyTssCalldata(txJson.data)) { + // A native consolidation is a plain send to the base address. + await throwRecipientMismatch('Native consolidation transaction must not carry calldata', [ + { address: txJson.to, amount: txJson.value }, + ]); } } diff --git a/modules/sdk-coin-bsc/test/unit/bsc.ts b/modules/sdk-coin-bsc/test/unit/bsc.ts index 6badbedd83b..3ac45d927a9 100644 --- a/modules/sdk-coin-bsc/test/unit/bsc.ts +++ b/modules/sdk-coin-bsc/test/unit/bsc.ts @@ -224,7 +224,7 @@ describe('Native BNB', function () { txPrebuild: { txHex, coin: 'tbsc', walletId: 'fakeWalletId' } as any, wallet, }) - .should.be.rejectedWith('destination address does not match with the recipient address'); + .should.be.rejectedWith('transaction calldata does not match the calldata given by client'); }); describe('assertSignableConsistency (Gap 2 / WCI-1398)', function () { diff --git a/modules/sdk-coin-eth/test/unit/tssPaymentVerification.ts b/modules/sdk-coin-eth/test/unit/tssPaymentVerification.ts new file mode 100644 index 00000000000..c5238b5d16e --- /dev/null +++ b/modules/sdk-coin-eth/test/unit/tssPaymentVerification.ts @@ -0,0 +1,361 @@ +import 'should'; +import sinon from 'sinon'; +import EthereumAbi from 'ethereumjs-abi'; +import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { TransactionType, TxIntentMismatchRecipientError, Wallet } from '@bitgo/sdk-core'; +import { coins, EthereumNetwork } from '@bitgo/statics'; +import { Hteth, TransactionBuilder } from '../../src'; +import { getBuilder } from './getBuilder'; + +/** + * verifyTssTransaction payment / consolidation checks (CECHO-2460). Each case builds real hteth + * transaction bytes and checks them against the declared intent. + */ +describe('TSS payment verification', function () { + const baseAddress = '0x174cfd823af8ce27ed0afee3fcf3c3ba259116be'; + const recipient = '0x7e85bdc27c050e3905ebf4b8e634d9ad6edd0de6'; + const otherRecipient = '0x9f8d0b5e1b3c3f0e2f4b7a6c1d2e3f4a5b6c7d8e'; + const tokenName = 'hteth:stgusd1'; + const tokenContract = '0xe4cf91a5bf7cc3d75ac85639e591e51d34948954'; + const otherTokenContract = '0x5ce9c87a0b7208491f9fe52bcc8e919eb43c58b2'; + const batcher = (coins.get('hteth').network as EthereumNetwork).batcherContractAddress as string; + + let bitgo: TestBitGoAPI; + let coin: Hteth; + let wallet: Wallet; + let warn: sinon.SinonStub; + + before(function () { + bitgo = TestBitGo.decorate(BitGoAPI, { env: 'test' }); + bitgo.safeRegister('hteth', Hteth.createInstance); + bitgo.initializeTestVars(); + coin = bitgo.coin('hteth') as Hteth; + wallet = new Wallet(bitgo, coin, { coinSpecific: { baseAddress } }); + }); + + beforeEach(function () { + warn = sinon.stub(console, 'warn'); + }); + + afterEach(function () { + sinon.restore(); + }); + + const calldata = (name: string, types: string[], values: unknown[]): string => + '0x' + Buffer.concat([EthereumAbi.methodID(name, types), EthereumAbi.rawEncode(types, values)]).toString('hex'); + const erc20Transfer = (to: string, amount: string) => calldata('transfer', ['address', 'uint256'], [to, amount]); + const nativeBatch = (addresses: string[], amounts: string[]) => + calldata('batch', ['address[]', 'uint256[]'], [addresses, amounts]); + const tokenBatch = (token: string, addresses: string[], amounts: string[]) => + calldata('batchTransferFrom', ['address', 'address[]', 'uint256[]'], [token, addresses, amounts]); + + async function buildTx({ to, value = '0', data }: { to: string; value?: string; data?: string }): Promise { + const txBuilder = getBuilder('hteth') as TransactionBuilder; + txBuilder.type(data ? TransactionType.ContractCall : TransactionType.SingleSigSend); + txBuilder.fee({ fee: '10', gasLimit: '100000' }); + txBuilder.counter(1); + txBuilder.contract(to); + txBuilder.value(value); + if (data) { + txBuilder.data(data); + } + return (await txBuilder.build()).toBroadcastFormat(); + } + + function verify(txParams: Record, txHex: string, verification = {}) { + return coin.verifyTransaction({ + txParams: { wallet, ...txParams } as never, + txPrebuild: { txHex, coin: 'hteth', walletId: 'fakeWalletId' } as never, + wallet, + verification, + walletType: 'tss', + }); + } + + async function expectMismatch(p: Promise, pattern: RegExp): Promise { + try { + await p; + } catch (e) { + e.should.be.instanceOf(TxIntentMismatchRecipientError); + (e as Error).message.should.match(pattern); + return; + } + throw new Error('expected verifyTransaction to reject'); + } + + const warned = (pattern: RegExp): boolean => warn.getCalls().some((call) => pattern.test(String(call.args[0]))); + + describe('single native payment', function () { + it('verifies a matching native send', async function () { + const txHex = await buildTx({ to: recipient, value: '1000' }); + ( + await verify({ type: 'transfer', recipients: [{ address: recipient, amount: '1000' }] }, txHex) + ).should.be.true(); + }); + + it('treats a stored payment intent as a transfer', async function () { + const txHex = await buildTx({ to: otherRecipient, value: '1000' }); + await expectMismatch( + verify({ type: 'payment', recipients: [{ address: recipient, amount: '1000' }] }, txHex), + /destination address does not match/ + ); + }); + + it('rejects a wrong amount', async function () { + const txHex = await buildTx({ to: recipient, value: '999' }); + await expectMismatch( + verify({ type: 'transfer', recipients: [{ address: recipient, amount: '1000' }] }, txHex), + /amount in txPrebuild does not match/ + ); + }); + + it('rejects calldata on a native payment', async function () { + const data = calldata('approve', ['address', 'uint256'], [otherRecipient, '1000']); + const txHex = await buildTx({ to: recipient, value: '1000', data }); + await expectMismatch( + verify({ type: 'transfer', recipients: [{ address: recipient, amount: '1000' }] }, txHex), + /not a native send or an ERC-20 transfer/ + ); + }); + + it('warns for ERC-20 calldata with no declared token but still enforces the transfer', async function () { + const intent = { type: 'transfer', recipients: [{ address: recipient, amount: '1000' }] }; + const good = await buildTx({ to: tokenContract, data: erc20Transfer(recipient, '1000') }); + (await verify(intent, good)).should.be.true(); + warned(/payment declares no token: the token contract was not verified/).should.be.true(); + + const redirected = await buildTx({ to: tokenContract, data: erc20Transfer(otherRecipient, '1000') }); + await expectMismatch(verify(intent, redirected), /destination address does not match/); + const withValue = await buildTx({ to: tokenContract, value: '1', data: erc20Transfer(recipient, '1000') }); + await expectMismatch(verify(intent, withValue), /must not carry native value/); + }); + }); + + describe('single ERC-20 payment', function () { + const intent = { type: 'transfer', recipients: [{ address: recipient, amount: '1000', tokenName }] }; + + it('verifies a matching transfer on the declared token contract', async function () { + const txHex = await buildTx({ to: tokenContract, data: erc20Transfer(recipient, '1000') }); + (await verify(intent, txHex)).should.be.true(); + warn.called.should.be.false(); + }); + + it('accepts the token declared at the top level', async function () { + const txHex = await buildTx({ to: tokenContract, data: erc20Transfer(recipient, '1000') }); + const txParams = { type: 'transfer', tokenName, recipients: [{ address: recipient, amount: '1000' }] }; + (await verify(txParams, txHex)).should.be.true(); + }); + + it('rejects a different token contract', async function () { + const txHex = await buildTx({ to: otherTokenContract, data: erc20Transfer(recipient, '1000') }); + await expectMismatch(verify(intent, txHex), /not sent to the declared token contract/); + }); + + it('rejects native value', async function () { + const txHex = await buildTx({ to: tokenContract, value: '1', data: erc20Transfer(recipient, '1000') }); + await expectMismatch(verify(intent, txHex), /must not carry native value/); + }); + + it('rejects a wrong function on the token contract', async function () { + const data = calldata('approve', ['address', 'uint256'], [recipient, '1000']); + const txHex = await buildTx({ to: tokenContract, data }); + await expectMismatch(verify(intent, txHex), /not a native send or an ERC-20 transfer/); + }); + + it('rejects trailing bytes after the transfer arguments', async function () { + const txHex = await buildTx({ to: tokenContract, data: erc20Transfer(recipient, '1000') + '00'.repeat(32) }); + await expectMismatch(verify(intent, txHex), /not canonical/); + }); + + it('rejects a wrong token amount', async function () { + const txHex = await buildTx({ to: tokenContract, data: erc20Transfer(recipient, '999') }); + await expectMismatch(verify(intent, txHex), /amount in txPrebuild does not match/); + }); + + it('rejects a token payment with no calldata', async function () { + const txHex = await buildTx({ to: recipient, value: '1000' }); + await expectMismatch(verify(intent, txHex), /carries no ERC-20 transfer calldata/); + }); + + it('warns for an unresolved token but still rejects a wrong amount', async function () { + const unresolved = { + type: 'transfer', + recipients: [{ address: recipient, amount: '1000', tokenName: 'hteth:notatoken' }], + }; + const good = await buildTx({ to: tokenContract, data: erc20Transfer(recipient, '1000') }); + (await verify(unresolved, good)).should.be.true(); + warned(/cannot be resolved/).should.be.true(); + + const bad = await buildTx({ to: tokenContract, data: erc20Transfer(recipient, '999') }); + await expectMismatch(verify(unresolved, bad), /amount in txPrebuild does not match/); + }); + }); + + describe('explicit calldata', function () { + const data = calldata('deposit', ['uint256', 'address'], ['5', baseAddress]); + + it('verifies an exact to / value / data match, including nonzero native value', async function () { + const txHex = await buildTx({ to: otherRecipient, value: '77', data }); + const txParams = { type: 'transfer', recipients: [{ address: otherRecipient, amount: '77', data }] }; + (await verify(txParams, txHex)).should.be.true(); + }); + + it('rejects calldata that differs from the declared calldata', async function () { + const txHex = await buildTx({ to: otherRecipient, value: '77', data }); + const declared = calldata('deposit', ['uint256', 'address'], ['6', baseAddress]); + const txParams = { type: 'transfer', recipients: [{ address: otherRecipient, amount: '77', data: declared }] }; + await expectMismatch(verify(txParams, txHex), /calldata does not match/); + }); + + it('rejects a different native value', async function () { + const txHex = await buildTx({ to: otherRecipient, value: '78', data }); + const txParams = { type: 'transfer', recipients: [{ address: otherRecipient, amount: '77', data }] }; + await expectMismatch(verify(txParams, txHex), /amount in txPrebuild does not match/); + }); + }); + + describe('batch payments', function () { + const nativeIntent = { + type: 'transfer', + recipients: [ + { address: recipient, amount: '100' }, + { address: otherRecipient, amount: '200' }, + ], + }; + const tokenIntent = { + type: 'transfer', + recipients: [ + { address: recipient, amount: '100', tokenName }, + { address: otherRecipient, amount: '200', tokenName }, + ], + }; + + it('verifies a native batch to the network batcher', async function () { + const txHex = await buildTx({ + to: batcher, + value: '300', + data: nativeBatch([recipient, otherRecipient], ['100', '200']), + }); + (await verify(nativeIntent, txHex)).should.be.true(); + }); + + it('rejects a native batch value that is not the sum', async function () { + const txHex = await buildTx({ + to: batcher, + value: '301', + data: nativeBatch([recipient, otherRecipient], ['100', '200']), + }); + await expectMismatch(verify(nativeIntent, txHex), /value does not match the sum/); + }); + + it('rejects a native batch that is not sent to the batcher', async function () { + const txHex = await buildTx({ + to: otherRecipient, + value: '300', + data: nativeBatch([recipient, otherRecipient], ['100', '200']), + }); + await expectMismatch(verify(nativeIntent, txHex), /not sent to the network batcher/); + }); + + it('rejects a batch entry that does not match the intent', async function () { + const txHex = await buildTx({ + to: batcher, + value: '300', + data: nativeBatch([recipient, otherRecipient], ['200', '100']), + }); + await expectMismatch(verify(nativeIntent, txHex), /entry 0 does not match/); + }); + + it('still rejects wrong batch amounts when no batcher is configured', async function () { + sinon + .stub(coin, 'getNetwork') + .returns({ ...(coin.getNetwork() as EthereumNetwork), batcherContractAddress: undefined }); + const txHex = await buildTx({ + to: otherRecipient, + value: '300', + data: nativeBatch([recipient, otherRecipient], ['100', '199']), + }); + await expectMismatch(verify(nativeIntent, txHex), /entry 1 does not match/); + warned(/no batcher contract is configured/).should.be.true(); + }); + + it('verifies a token batch with no top-level tokenName', async function () { + const txHex = await buildTx({ + to: batcher, + data: tokenBatch(tokenContract, [recipient, otherRecipient], ['100', '200']), + }); + (await verify(tokenIntent, txHex)).should.be.true(); + }); + + it('rejects a token batch that moves a different token', async function () { + const txHex = await buildTx({ + to: batcher, + data: tokenBatch(otherTokenContract, [recipient, otherRecipient], ['100', '200']), + }); + await expectMismatch(verify(tokenIntent, txHex), /token contract does not match/); + }); + + it('rejects a token batch carrying native value', async function () { + const txHex = await buildTx({ + to: batcher, + value: '1', + data: tokenBatch(tokenContract, [recipient, otherRecipient], ['100', '200']), + }); + await expectMismatch(verify(tokenIntent, txHex), /must not carry native value/); + }); + + it('rejects a token batch sent through the native batch function', async function () { + const txHex = await buildTx({ + to: batcher, + data: nativeBatch([recipient, otherRecipient], ['100', '200']), + }); + await expectMismatch(verify(tokenIntent, txHex), /must use batchTransferFrom/); + }); + }); + + describe('other intents', function () { + it('warns for contractCall instead of claiming it was verified', async function () { + const txHex = await buildTx({ to: recipient, value: '1' }); + (await verify({ type: 'contractCall', recipients: [] }, txHex)).should.be.true(); + warned(/contractCall: no verifier is implemented/).should.be.true(); + }); + + it('warns for an intent type with no verifier', async function () { + const txHex = await buildTx({ to: recipient, value: '1' }); + (await verify({ type: 'acceleration' }, txHex)).should.be.true(); + warned(/acceleration: no verifier is implemented/).should.be.true(); + }); + }); + + describe('consolidation to base address', function () { + const consolidation = { consolidationToBaseAddress: true }; + + it('rejects calldata on a native consolidation', async function () { + const data = calldata('approve', ['address', 'uint256'], [recipient, '1']); + const txHex = await buildTx({ to: baseAddress, value: '1000', data }); + await expectMismatch(verify({ type: 'consolidate' }, txHex, consolidation), /must not carry calldata/); + }); + + it('verifies an ERC-20 sweep on the declared token without comparing the amount', async function () { + const txHex = await buildTx({ to: tokenContract, data: erc20Transfer(baseAddress, '123456789') }); + (await verify({ type: 'consolidate', tokenName }, txHex, consolidation)).should.be.true(); + }); + + it('rejects an ERC-20 sweep carrying native value', async function () { + const txHex = await buildTx({ to: tokenContract, value: '1', data: erc20Transfer(baseAddress, '1') }); + await expectMismatch( + verify({ type: 'consolidate', tokenName }, txHex, consolidation), + /must not carry native value/ + ); + }); + + it('rejects an ERC-20 sweep on a different token contract', async function () { + const txHex = await buildTx({ to: otherTokenContract, data: erc20Transfer(baseAddress, '1') }); + await expectMismatch( + verify({ type: 'consolidate', tokenName }, txHex, consolidation), + /does not call the declared token contract/ + ); + }); + }); +}); diff --git a/modules/sdk-coin-xdc/test/unit/xdcToken.ts b/modules/sdk-coin-xdc/test/unit/xdcToken.ts index 337b371094b..a6582290d2d 100644 --- a/modules/sdk-coin-xdc/test/unit/xdcToken.ts +++ b/modules/sdk-coin-xdc/test/unit/xdcToken.ts @@ -131,7 +131,8 @@ describe('XDC Token:', function () { describe('verifyTssTransaction', function () { const recipientAddress = '0x174cfd823af8ce27ed0afee3fcf3c3ba259116be'; const wrongAddress = '0x7e85bdc27c050e3905ebf4b8e634d9ad6edd0de6'; - const tokenContractAddress = '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48'; + // txdc:tmt's contract in statics: verifyTssTransaction checks the token contract being called. + const tokenContractAddress = '0xb283ec8dad644effc5c4c50bb7bb21442ac3c2db'; it('should accept an ERC-20 transfer where calldata matches declared recipient', async function () { const token = bitgo.coin('txdc:tmt') as XdcToken; From d4235dbbba0bf1556446f21dd97e308d3bef22ec Mon Sep 17 00:00:00 2001 From: Deepak Jangid Date: Fri, 6 Nov 2026 00:19:21 +0530 Subject: [PATCH 2/2] fix(sdk-core): preserve token identity in MPCv2 approvals Stored payment approvals reconstruct recipients without caller txParams. Pass the signing coin's chain name in both ECDSA MPCv2 verification branches so amount.symbol restores tokenName for confidential payments. Cover both signing-byte routes with the real EVM verifier and preserve existing caller-recipient precedence. Ticket: CECHO-2460 Co-authored-by: Ralph CLI Ralph: 18.13.2-bitgo.6 openrouter/deepseek/deepseek-v4.1-flash-1017@default:high+smol:high+task:high openrouter/anthropic/claude-opus-5.5@slow:high+plan:xhigh openrouter/openai/gpt-6.1-terra@vision:low+advisor:medium openrouter/z-ai/glm-5.3-flash@commit:high+memory:low openrouter/inclusionai/ling-3.1-flash@tiny:low openrouter/moonshotai/kimi-k3.1-pro@designer:high openrouter/google/gemini-3-pro-image@image web/exa@web openrouter/microsoft/mai-transcribe-2@dictation openrouter/typesafe/jev-1.14@judge --- .../test/unit/tssPaymentVerification.ts | 134 +++++++++++++++++- .../src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts | 4 +- .../src/bitgo/utils/tss/recipientUtils.ts | 3 + .../unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts | 3 + 4 files changed, 140 insertions(+), 4 deletions(-) diff --git a/modules/sdk-coin-eth/test/unit/tssPaymentVerification.ts b/modules/sdk-coin-eth/test/unit/tssPaymentVerification.ts index c5238b5d16e..18d45202131 100644 --- a/modules/sdk-coin-eth/test/unit/tssPaymentVerification.ts +++ b/modules/sdk-coin-eth/test/unit/tssPaymentVerification.ts @@ -1,10 +1,20 @@ import 'should'; import sinon from 'sinon'; +import { Hash } from 'crypto'; import EthereumAbi from 'ethereumjs-abi'; +import { FeeMarketEIP1559Transaction } from '@ethereumjs/tx'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; import { BitGoAPI } from '@bitgo/sdk-api'; -import { TransactionType, TxIntentMismatchRecipientError, Wallet } from '@bitgo/sdk-core'; -import { coins, EthereumNetwork } from '@bitgo/statics'; +import { + EcdsaMPCv2Utils, + RequestTracer, + TransactionType, + TxIntentMismatchRecipientError, + TxRequest, + Wallet, +} from '@bitgo/sdk-core'; +import { AbstractEthLikeNewCoins, TransferBuilderERC7984 } from '@bitgo/abstract-eth'; +import { BaseCoin as StaticsBaseCoin, CoinFeature, coins, EthereumNetwork } from '@bitgo/statics'; import { Hteth, TransactionBuilder } from '../../src'; import { getBuilder } from './getBuilder'; @@ -328,6 +338,126 @@ describe('TSS payment verification', function () { }); }); + // Approval recreation carries no caller txParams; the token survives only in amount.symbol. + // Exercise both verification routes with the real coin verifier, stopping before signing. + describe('MPCv2 signing of a stored confidential payment without caller txParams', function () { + const confidentialToken = 'hteth:ctest1'; + const confidentialRecipient = '0x19645032c7f1533395d44a629462e751084d3e4c'; + const confidentialSendAmount = '1000000'; + const encryptedHandle = '0x' + 'ab'.repeat(32); + const inputProof = '0x' + 'cd'.repeat(50); + const gpgKeyStub = 'stubbed-gpg-key'; + const signedBoundaryMessage = 'signing boundary reached after recipient verification'; + + function tokenContractAddress(tokenName: string): string { + const token = coins.get(tokenName); + if (!('contractAddress' in token) || typeof token.contractAddress !== 'string') { + throw new Error(`${tokenName} has no contract address in statics`); + } + return token.contractAddress; + } + + function buildConfidentialTransferBytes(): { signableHex: string; serializedTxHex: string } { + const tokenContract = tokenContractAddress(confidentialToken); + const common = AbstractEthLikeNewCoins.getCustomChainCommon(coin.getChainId()); + const confidentialTransfer = new TransferBuilderERC7984() + .to(confidentialRecipient) + .tokenContractAddress(tokenContract) + .encryptedHandle(encryptedHandle) + .inputProof(inputProof) + .build(); + const tx = FeeMarketEIP1559Transaction.fromTxData( + { + nonce: 1, + maxPriorityFeePerGas: 10, + maxFeePerGas: 100, + gasLimit: 200000, + to: tokenContract, + value: 0, + data: confidentialTransfer, + chainId: coin.getChainId(), + }, + { common } + ); + return { + signableHex: tx.getMessageToSign(false).toString('hex'), + serializedTxHex: tx.serialize().toString('hex'), + }; + } + + async function expectSigningBoundary(useSerializedTxHex: boolean): Promise { + const { signableHex, serializedTxHex } = buildConfidentialTransferBytes(); + const signingWallet = new Wallet(bitgo, coin, { + id: 'confidentialSigningWallet', + multisigType: 'tss', + multisigTypeVersion: 'MPCv2', + coinSpecific: { baseAddress }, + }); + const utils = new EcdsaMPCv2Utils(bitgo, coin, signingWallet); + // Avoid the remote GPG lookup; verification does not consume this key. + const stubbableUtils = utils as unknown as Record Promise>; + sinon.stub(stubbableUtils, 'pickBitgoPubGpgKeyForSigning').resolves(gpgKeyStub); + + const realFeatures = coin + .getConfig() + .features.filter((feature) => feature !== CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX); + sinon.stub(coin, 'getConfig').returns({ + ...coin.getConfig(), + features: useSerializedTxHex ? [...realFeatures, CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX] : realFeatures, + } as unknown as Readonly); + + // The MPCv2 flow hashes the verified bytes before it exchanges any signature share. Throwing + // here proves verification succeeded: a recipient mismatch rejects before this boundary. + const boundary = new Error(signedBoundaryMessage); + sinon.stub(coin, 'getHashFunction').returns({ + update: () => { + throw boundary; + }, + } as unknown as Hash); + + const txRequest = { + txRequestId: 'confidential-payment-signing', + apiVersion: 'full', + walletId: 'confidentialSigningWallet', + intent: { + intentType: 'payment', + recipients: [ + { + address: { address: confidentialRecipient }, + amount: { value: confidentialSendAmount, symbol: confidentialToken }, + }, + ], + }, + transactions: [ + { + unsignedTx: { derivationPath: 'm/0', signableHex, serializedTxHex }, + signatureShares: [], + }, + ], + } as unknown as TxRequest; + + try { + await utils.signTxRequest({ + txRequest, + prv: '', + reqId: new RequestTracer(), + }); + } catch (e) { + e.should.be.exactly(boundary); + return; + } + throw new Error('expected signTxRequest to stop at the signing boundary'); + } + + it('verifies the stored payment from signableHex', async function () { + await expectSigningBoundary(false); + }); + + it('verifies the stored payment from serializedTxHex', async function () { + await expectSigningBoundary(true); + }); + }); + describe('consolidation to base address', function () { const consolidation = { consolidationToBaseAddress: true }; diff --git a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts index b942656b947..d7bfc2b6ef5 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts @@ -991,7 +991,7 @@ export class EcdsaMPCv2Utils extends BaseEcdsaUtils { if (shouldVerifyWithSerializedTxHex(this.baseCoin) || shouldUsePreHashedSignable(this.baseCoin, unsignedTx)) { await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.serializedTxHex, txInfo: unsignedTx.signableHex }, - txParams: resolveEffectiveTxParams(txRequest, params.txParams), + txParams: resolveEffectiveTxParams(txRequest, params.txParams, this.baseCoin.getChain()), wallet: this.wallet, walletType: this.wallet.multisigType(), }); @@ -1001,7 +1001,7 @@ export class EcdsaMPCv2Utils extends BaseEcdsaUtils { } else { await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.signableHex }, - txParams: resolveEffectiveTxParams(txRequest, params.txParams), + txParams: resolveEffectiveTxParams(txRequest, params.txParams, this.baseCoin.getChain()), wallet: this.wallet, walletType: this.wallet.multisigType(), }); diff --git a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts index b9cde0bfb83..5618eeb85b5 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts @@ -114,6 +114,9 @@ export const NO_RECIPIENT_TX_TYPES = new Set([ * * tokenName is derived from tokenData.tokenName when present, otherwise from * amount.symbol when chainName is provided and symbol differs from it. + * MPCv2 callers supply the signing coin's chain name to retain token identity on + * stored-request approvals. This fallback only affects reconstructed recipients; + * a nonempty caller-supplied recipient list is preserved unchanged. * * Staking intents (BSC delegate/undelegate, CELO stake/unstake, etc.) are * identified generically by the presence of `stakingRequestId` on the intent — diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts index a72732aeeb3..3360946ce85 100644 --- a/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts @@ -635,6 +635,7 @@ describe('ECDSA MPC v2', async () => { getHashFunction: hashFunctionSpy, verifyTransaction: sinon.stub().resolves(true), getMPCAlgorithm: sinon.stub().returns('ecdsa'), + getChain: sinon.stub().returns('flrp'), getConfig: sinon.stub().returns({ family: 'flrp' }), isSignablePreHashed: (unsignedTx: { serializedTxHex?: string }) => unsignedTx.serializedTxHex?.startsWith('0000') ?? false, @@ -706,6 +707,7 @@ describe('ECDSA MPC v2', async () => { getHashFunction: hashFunctionSpy, verifyTransaction: sinon.stub().resolves(true), getMPCAlgorithm: sinon.stub().returns('ecdsa'), + getChain: sinon.stub().returns('flr'), getConfig: sinon.stub().returns({ family: 'flr' }), } as unknown as IBaseCoin; @@ -780,6 +782,7 @@ describe('ECDSA MPC v2', async () => { getHashFunction: sinon.stub().callsFake(() => createKeccakHash('keccak256') as Hash), verifyTransaction: verifyTransactionSpy, getMPCAlgorithm: sinon.stub().returns('ecdsa'), + getChain: sinon.stub().returns('hteth'), getConfig: sinon.stub().returns({ family: 'hteth' }), } as unknown as IBaseCoin;