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..18d45202131 --- /dev/null +++ b/modules/sdk-coin-eth/test/unit/tssPaymentVerification.ts @@ -0,0 +1,491 @@ +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 { + 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'; + +/** + * 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(); + }); + }); + + // 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 }; + + 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; 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;