From 90aceaa553cbdcffa32dc6570859f89aca49b88b Mon Sep 17 00:00:00 2001 From: Deepak Jangid Date: Wed, 4 Nov 2026 11:54:36 +0530 Subject: [PATCH] fix(abstract-eth): verify EVM TSS bytes against intent before signing verifyTssTransaction trusted the server-declared txParams.type and only decoded two calldata shapes under transfer. A compromised server could label approve(attacker, MAX) or any contract call as a payment, or redirect a payment on the pending-approval path, and the user key share would sign it (TOB-BITGOEDMPC-2 / INFOSEC-671). The ecdsa and ecdsaMPCv2 signing paths now pass the stored txRequest.intent, the caller's untouched txParams (callerTxParams) and the chain name into verifyTransaction. When the caller declares a type or recipients, only its own params count. The stored intent is used only when the caller declared nothing (pending approval), so server data never fills gaps in a caller's intent. The asset comes from local identity only; unresolvable or conflicting identity is a finding. The verifier decodes the bytes once and runs small checks picked by the intent type, each accepting a closed set of shapes: native send, ERC-20 or ERC-7984 transfer, exact dApp call, batcher batch / batchTransferFrom, tokenApproval to the batcher for MAX, fillNonce, consolidation to the base address, and the DeFi calldata checks. contractCall and customTx throw. A token consolidation must carry no native value, which closes a path where transfer(baseAddress, 0) sent to any contract moved the native balance there. Checks return findings and one policy step decides warn or throw. Fatal findings, the checks that threw before, still throw with the same messages. Every other finding goes to console.warn and the new onVerificationWarning callback, or throws when the client sets the new client-side strictTssVerification BitGoAPI option, which is never read from server data. Stored kebab-case DeFi intents (defi-approve, defi-deposit, defi-withdraw) now pass the missing-recipients guard so their calldata is checked. defi-redeem has no handler and stays rejected. EvmCoin.verifyLegacyTssTransaction, which returned true without decoding, is removed so kavaevm and prividiumeth run the parent verifier. The verifier helpers stay internal to abstract-eth. Tests cover the audit cases on kavaevm, hbarevm and tbsc (batcher), and an end-to-end case runs the real verifier inside EcdsaUtils.signTxRequest to show rejected bytes never reach the signing round. Ticket: CECHO-2460 Co-Authored-By: Claude Opus 5.5 --- modules/abstract-eth/README.md | 22 + .../src/abstractEthLikeNewCoins.ts | 303 +++-- modules/abstract-eth/src/lib/iface.ts | 15 + modules/abstract-eth/src/lib/utils.ts | 65 + modules/abstract-eth/src/lib/walletUtil.ts | 8 + modules/abstract-eth/src/tssVerification.ts | 1109 +++++++++++++++++ modules/sdk-api/src/bitgoAPI.ts | 7 + modules/sdk-api/src/types.ts | 18 +- .../test/unit/tssBatcherVerification.ts | 312 +++++ modules/sdk-coin-eth/test/unit/eth.ts | 60 +- modules/sdk-coin-evm/src/evmCoin.ts | 43 - .../sdk-coin-evm/test/unit/tssSigningPath.ts | 125 ++ .../sdk-coin-evm/test/unit/tssVerification.ts | 913 ++++++++++++++ .../sdk-core/src/bitgo/baseCoin/iBaseCoin.ts | 49 + modules/sdk-core/src/bitgo/bitgoBase.ts | 12 +- .../sdk-core/src/bitgo/utils/tss/baseTypes.ts | 2 + .../src/bitgo/utils/tss/ecdsa/ecdsa.ts | 9 +- .../src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts | 9 +- .../src/bitgo/utils/tss/recipientUtils.ts | 12 +- .../unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts | 92 ++ .../tss/ecdsa/ecdsaVerifyTransactionWiring.ts | 132 ++ .../unit/bitgo/utils/tss/recipientUtils.ts | 26 +- 22 files changed, 3140 insertions(+), 203 deletions(-) create mode 100644 modules/abstract-eth/src/tssVerification.ts create mode 100644 modules/sdk-coin-bsc/test/unit/tssBatcherVerification.ts create mode 100644 modules/sdk-coin-evm/test/unit/tssSigningPath.ts create mode 100644 modules/sdk-coin-evm/test/unit/tssVerification.ts create mode 100644 modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaVerifyTransactionWiring.ts diff --git a/modules/abstract-eth/README.md b/modules/abstract-eth/README.md index 856c0af47ca..1a54297486b 100644 --- a/modules/abstract-eth/README.md +++ b/modules/abstract-eth/README.md @@ -1 +1,23 @@ # BitGo abstract-eth + +## EVM TSS transaction verification + +`verifyTssTransaction` decodes the EVM bytes about to be signed and compares `to` / `value` / +`data` with the user intent before the user key share signs (TOB-BITGOEDMPC-2 / INFOSEC-671). + +- The caller's own `txParams` are authoritative whenever they declare `type` or `recipients`. A + stored tx request intent is consulted only when the caller declared nothing (pending-approval + signing); server data never fills absent caller calldata or replaces the declared token. +- The asset is resolved locally from the recipient's `tokenAddress` / `tokenName`, the top-level + `tokenName` and the coin instance. Unresolvable or contradictory identity fails closed, and the + calldata shape must match the asset: a native send, an ERC-20 / ERC-7984 transfer, or one batch + that moves a single asset with the matching batch method. +- The bytes are decoded once, each check returns findings, and one policy step decides warn or + throw. Fatal findings (native destination and value, ERC-20 recipient and amount, and the + consolidation target when `consolidationToBaseAddress` is set) always throw. Every other + finding is reported through `console.warn` and the optional `onVerificationWarning` callback; + pass `strictTssVerification: true` to `BitGoAPI` to make them hard failures. +- A consolidation must pay the wallet base address, and an ERC-20 consolidation must not carry + native value. +- `contractCall` and `customTx` intents always throw on EVM, and stored `defi-redeem` intents + are rejected because no handler exists for them yet. diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index ff45cad8def..c5dea35e2ed 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -54,8 +54,23 @@ import { DeriveAddressResult, NO_RECIPIENT_TX_TYPES, CoinWithSignableConsistency, + TssVerificationWarning, } from '@bitgo/sdk-core'; import { getDerivationPath } from '@bitgo/sdk-lib-mpc'; +import { + buildEvmTssIntentContext, + callerDeclaresIntent, + canonicalizeEvmTssIntentType, + DecodedEvmTx, + evaluateEvmTssIntent, + isUnsupportedEvmTssIntentType, + mapDefiParamsFromIntent, + resolveCoinTokenContractAddress, + selectDeclaredIntent, + TssFinding, + TssFindingSeverity, + verifyConsolidation, +} from './tssVerification'; import { bip32 } from '@bitgo/secp256k1'; import { BaseCoin as StaticsBaseCoin, @@ -3550,15 +3565,37 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem } /** + * Verify that a TSS transaction prebuild complies with the original intention before the + * user key share signs it (TOB-BITGOEDMPC-2 / INFOSEC-671). + * + * One flow: the intent is normalized once, the bytes are decoded once, and small checks per + * operation return findings. The intent type picks the checks; each accepts a closed set of + * decoded transaction shapes and anything unmatched is a finding. Fatal findings (the checks + * that historically threw) throw at once, including on the pending-approval path. Every other + * finding goes through {@link applyTssVerificationPolicy}: `console.warn` plus the optional + * `onVerificationWarning` callback, or a hard failure when the client opts into `BitGoAPI`'s + * `strictTssVerification`. + * * @param {VerifyEthTransactionOptions} params * @param {TransactionParams} params.txParams - params object passed to send * @param {TransactionPrebuild} params.txPrebuild - prebuild object returned by server * @param {Wallet} params.wallet - Wallet object to obtain keys to verify against + * @param {PopulatedIntent} params.intent - stored tx request intent (pending-approval path) * @returns {boolean} * @throws {TxIntentMismatchRecipientError} if transaction recipients don't match user intent */ async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise { - const { txParams, txPrebuild, wallet } = params; + const ethNetwork = this.getNetwork(); + const { txParams, txPrebuild, wallet, intent } = params; + // Exactly one source speaks for the user intent: the caller's own params when it declared a + // type or recipients, otherwise the stored tx request intent (pending approval). The other + // source is dropped, so server data never selects the handler or fills in what the caller + // left out. See selectDeclaredIntent. + const { callerParams, storedIntent } = selectDeclaredIntent({ + txParams, + callerTxParams: params.callerTxParams, + intent, + }); // Helper to throw TxIntentMismatchRecipientError with recipient details const throwRecipientMismatch = async (message: string, mismatchedRecipients: Recipient[]): Promise => { @@ -3578,6 +3615,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem !( txParams.prebuildTx?.consolidateId || txPrebuild?.consolidateId || + storedIntent?.consolidateId || txParams.stakingRequestId || txParams.prebuildTx?.stakingRequestId || (txParams.type && NO_RECIPIENT_TX_TYPES.has(txParams.type)) @@ -3593,91 +3631,98 @@ 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; - - 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; - } + // Canonical verifier intent type, derived only from the source that speaks for the user. + // The caller's own `type` wins and is never defaulted from the stored intent: on a direct + // send `txParams.type` is the caller's alone, and a caller that declared recipients without a + // type is making a payment. Only when the caller declared nothing does the stored intent + // type apply: resolveEffectiveTxParams has already put the raw stored spelling (payment, + // defi-approve, …) in txParams.type, which is normalized here. + const explicitType = storedIntent + ? canonicalizeEvmTssIntentType(txParams.type) || canonicalizeEvmTssIntentType(storedIntent.intentType) + : canonicalizeEvmTssIntentType(callerParams?.type); + // A consolidation id on the prebuild or the stored intent is server data. It marks the tx as a + // consolidation only when the caller declared nothing of its own; it can never reroute a + // caller-declared payment to the consolidate handler. + const consolidateId = callerDeclaresIntent(callerParams) + ? undefined + : txParams.prebuildTx?.consolidateId || txPrebuild?.consolidateId || storedIntent?.consolidateId || undefined; + const isConsolidation = + !!params.verification?.consolidationToBaseAddress || explicitType === 'consolidate' || !!consolidateId; + const canonicalType = explicitType || (isConsolidation ? 'consolidate' : 'transfer'); + + // contractCall / customTx cannot be created on EVM and carry arbitrary calldata with no + // intent shape to verify — never let them reach signing on this path. + if (isUnsupportedEvmTssIntentType(canonicalType)) { + throw new Error(`unsupported intent type for EVM TSS: ${canonicalType}`); + } + + // Decode the bytes once; every check below reads these same fields. + let decodedTx: DecodedEvmTx | undefined; + let decodeError: Error | undefined; + try { + decodedTx = await this.decodeTssTransaction(txPrebuild.txHex); + } catch (e) { + decodeError = e as Error; + } + const requireDecoded = (): DecodedEvmTx => { + if (!decodedTx) { + throw decodeError; + } + return decodedTx; + }; + const raise = (finding: TssFinding): Promise => + throwRecipientMismatch(finding.reason, [ + { address: finding.decoded.to ?? '', amount: finding.decoded.value ?? '' }, + ]); + const raiseFatal = async (findings: TssFinding[]): Promise => { + const fatal = findings.find((finding) => finding.severity === 'fatal'); + if (fatal) { + await raise(fatal); + } + }; - 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() }] - ); - } + const coinBaseAddress = wallet.coinSpecific()?.baseAddress; + const baseAddress = typeof coinBaseAddress === 'string' ? coinBaseAddress : undefined; + const ctx = buildEvmTssIntentContext({ + intentType: canonicalType, + txParams, + callerParams, + storedIntent, + coinTokenContractAddress: resolveCoinTokenContractAddress(this.getChain()), + baseAddress, + batcherAddress: ethNetwork?.batcherContractAddress, + }); - if (expectedRecipientAddress !== addHexPrefix(recipientAddress.toString()).toLowerCase()) { - await throwRecipientMismatch('destination address does not match with the recipient address', [ - { address: addHexPrefix(recipientAddress.toString()), amount: amount.toString() }, - ]); - } - } - } + // Operation checks. A single-recipient payment's historic checks need the decoded bytes, so + // bytes that cannot be decoded still fail there as they always did. Fatal findings (those + // historic checks) throw now; the rest wait for the policy step at the end. + if (canonicalType === 'transfer' && ctx.recipients.length === 1) { + requireDecoded(); } + const intentFindings = evaluateEvmTssIntent(ctx, decodedTx ?? { to: '', value: '0', data: '0x' }); + await raiseFatal(intentFindings); - if (txParams.type && ['defiApprove', 'defiDeposit', 'defiWithdraw'].includes(txParams.type)) { + // DeFi vault calldata vs defiParams. Stored defi-* intents reach this branch through the + // intent type mapping, with defiParams reconstructed from the stored intent fields. + if (['defiApprove', 'defiDeposit', 'defiWithdraw'].includes(canonicalType)) { if (!txPrebuild.txHex) { throw new Error('missing txHex in txPrebuild'); } - const baseAddress = wallet.coinSpecific()?.baseAddress; - const defiParams = txParams.defiParams; - if (!baseAddress || !defiParams) { + const defiParams = txParams.defiParams ?? (storedIntent ? mapDefiParamsFromIntent(storedIntent) : undefined); + if (!coinBaseAddress || !defiParams) { await throwRecipientMismatch('DeFi transaction is missing required intent parameters', []); return false; } - const validatedBaseAddress = baseAddress as string; + const validatedBaseAddress = coinBaseAddress as string; const validatedDefiParams = defiParams; - const txBuilder = this.getTransactionBuilder(); - txBuilder.from(txPrebuild.txHex); - const txJson = (await txBuilder.build()).toJson(); + const txJson = requireDecoded(); const mismatch = async (message: string, address = txJson.to, amount = txJson.value): Promise => throwRecipientMismatch(message, [{ address: address || '', amount: amount || '' }]); const data = txJson.data || ''; - if (txParams.type === 'defiApprove') { + if (canonicalType === 'defiApprove') { const selector = addHexPrefix(optionalDeps.ethAbi.methodID('approve', ['address', 'uint256']).toString('hex')); if (!data.startsWith(selector)) { await mismatch('defiApprove transaction must use ERC-20 approve(address,uint256) calldata'); @@ -3690,7 +3735,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem amount.toString() ); } - } else if (txParams.type === 'defiDeposit') { + } else if (canonicalType === 'defiDeposit') { const selector = addHexPrefix(optionalDeps.ethAbi.methodID('deposit', ['uint256', 'address']).toString('hex')); if (!data.startsWith(selector)) { await mismatch('defiDeposit transaction must use deposit(uint256,address) calldata'); @@ -3744,7 +3789,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem } } - if (txParams.type === 'wrapApprove') { + if (canonicalType === 'wrapApprove') { await this.verifyWrapApproveTransaction(params, throwRecipientMismatch); } else { // Declared as anything else: only an approve of a registered wrapper is rejected (see the method doc); @@ -3752,55 +3797,85 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem await this.assertWrapperApproveIsDeclared(params, throwRecipientMismatch); } - // Verify consolidation transactions send to base address - if (params.verification?.consolidationToBaseAddress) { - const coinSpecific = wallet.coinSpecific(); - if (!coinSpecific || !coinSpecific.baseAddress) { - throw new Error('Unable to determine base address for consolidation'); - } - const baseAddress = coinSpecific.baseAddress; - - if (!txPrebuild.txHex) { - throw new Error('missing txHex in txPrebuild'); + // Consolidations must pay the wallet base address. The check is fatal when the caller sets + // consolidationToBaseAddress, as it always was; for consolidate intents and consolidateId + // flows it is a new check that follows the warn / strict policy. + const consolidationFindings: TssFinding[] = []; + if (isConsolidation) { + const severity: TssFindingSeverity = params.verification?.consolidationToBaseAddress ? 'fatal' : 'new'; + try { + if (!baseAddress) { + throw new Error('Unable to determine base address for consolidation'); + } + const tx = requireDecoded(); + if (!tx.to) { + throw new Error('Consolidation transaction is missing recipient address'); + } + consolidationFindings.push(...verifyConsolidation(tx, baseAddress, severity)); + } catch (e) { + if (severity === 'fatal') { + throw e; + } + consolidationFindings.push({ + severity, + intentType: 'consolidate', + reason: `consolidation destination could not be verified: ${(e as Error).message}`, + expected: { to: baseAddress ?? '' }, + decoded: {}, + }); } + await raiseFatal(consolidationFindings); + } - const txBuilder = this.getTransactionBuilder(); - txBuilder.from(txPrebuild.txHex); - const tx = await txBuilder.build(); - const txJson = tx.toJson(); - - // Verify the transaction recipient matches the base address - if (!txJson.to) { - throw new Error('Consolidation transaction is missing recipient address'); - } + const decodeFindings: TssFinding[] = decodedTx + ? [] + : [ + { + severity: 'new', + intentType: canonicalType, + reason: txPrebuild.txHex + ? `transaction bytes could not be decoded to verify intent: ${decodeError?.message}` + : 'missing txHex in txPrebuild: transaction bytes cannot be verified against intent', + expected: {}, + decoded: {}, + }, + ]; - const erc20TransferSelector = addHexPrefix( - optionalDeps.ethAbi.methodID('transfer', ['address', 'uint256']).toString('hex') - ); + await this.applyTssVerificationPolicy([...consolidationFindings, ...decodeFindings, ...intentFindings], raise); + return true; + } - if (txJson.data && txJson.data.startsWith(erc20TransferSelector)) { - // For ERC20 token consolidations, txJson.to is the token contract address, - // not the actual transfer recipient. The real recipient is encoded in the - // transfer(address,uint256) calldata. Decode it and verify against baseAddress. - const [recipientAddress] = getRawDecoded( - ['address', 'uint256'], - getBufferedByteCode(erc20TransferSelector, txJson.data) - ); - const decodedRecipient = addHexPrefix(recipientAddress.toString()).toLowerCase(); - if (decodedRecipient !== baseAddress.toLowerCase()) { - await throwRecipientMismatch('Consolidation transaction recipient does not match wallet base address', [ - { address: decodedRecipient, amount: txJson.value }, - ]); - } - } 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 }, - ]); - } + /** Decodes the TSS transaction bytes once, so every verifier check reads the same fields. */ + private async decodeTssTransaction(txHex: string | undefined): Promise { + if (!txHex) { + throw new Error('missing txHex in txPrebuild'); } + const txBuilder = this.getTransactionBuilder(); + txBuilder.from(txHex); + const txJson = (await txBuilder.build()).toJson(); + return { to: txJson.to ?? '', value: txJson.value ?? '0', data: txJson.data ?? '0x' }; + } - return true; + /** + * The one place that decides warn or throw for findings that are not fatal: they throw when + * the client sets `strictTssVerification`, and otherwise go to `console.warn` and the + * optional `onVerificationWarning` callback. + */ + private async applyTssVerificationPolicy( + findings: TssFinding[], + raise: (finding: TssFinding) => Promise + ): Promise { + if (!findings.length) { + return; + } + if (this.bitgo?.strictTssVerification) { + await raise(findings[0]); + } + for (const { intentType, reason, expected, decoded } of findings) { + const warning: TssVerificationWarning = { intentType, reason, expected, decoded }; + console.warn(`[tss verification] ${reason}`, JSON.stringify({ expected, decoded })); + this.bitgo?.onVerificationWarning?.(warning); + } } /** diff --git a/modules/abstract-eth/src/lib/iface.ts b/modules/abstract-eth/src/lib/iface.ts index fd5c6cdc000..77bb8640555 100644 --- a/modules/abstract-eth/src/lib/iface.ts +++ b/modules/abstract-eth/src/lib/iface.ts @@ -149,6 +149,21 @@ export interface NativeTransferData extends TransferData { data: string; } +export interface BatchTransferRecipient { + address: string; + amount: string; +} + +/** Decoded inner recipients of batch(address[],uint256[]) batcher calldata. */ +export interface BatchTransferData { + recipients: BatchTransferRecipient[]; +} + +/** Decoded batchTransferFrom(address,address[],uint256[]) batcher calldata. */ +export interface BatchTransferFromData extends BatchTransferData { + tokenContractAddress: string; +} + export interface WalletInitializationData { salt?: string; owners: string[]; diff --git a/modules/abstract-eth/src/lib/utils.ts b/modules/abstract-eth/src/lib/utils.ts index 95dc0ca69d9..a47efbebe84 100644 --- a/modules/abstract-eth/src/lib/utils.ts +++ b/modules/abstract-eth/src/lib/utils.ts @@ -32,6 +32,8 @@ import { } from '@bitgo/sdk-core'; import { + BatchTransferData, + BatchTransferFromData, ERC1155TransferData, ERC721TransferData, FlushTokensData, @@ -91,6 +93,10 @@ import { confidentialTransferWithProofMethodId, confidentialTransferWithProofTypes, confidentialTransferNoProofMethodId, + batchMethodId, + batchMethodTypes, + batchTransferFromMethodId, + batchTransferFromMethodTypes, } from './walletUtil'; import { EthTransactionData } from './types'; import { @@ -479,6 +485,65 @@ export function decodeTransferData(data: string, isFirstSigner?: boolean): Trans } } +/** + * Decode batch(address[],uint256[]) calldata used for native batch sends to the batcher contract. + * + * @param data Hex string starting with the batch method selector + * @returns Decoded recipients and amounts in the order they appear in the calldata + */ +export function decodeBatchTransferData(data: string): BatchTransferData { + if (!data.toLowerCase().startsWith(batchMethodId)) { + throw new BuildTransactionError(`Invalid batch transfer bytecode: ${data}`); + } + const [addresses, amounts] = getRawDecoded(batchMethodTypes, getBufferedByteCode(batchMethodId, data)); + if (!Array.isArray(addresses) || !Array.isArray(amounts)) { + throw new BuildTransactionError(`Invalid batch transfer bytecode: ${data}`); + } + if (addresses.length !== amounts.length) { + throw new BuildTransactionError( + `Mismatched batch address/amount array lengths: ${addresses.length} vs ${amounts.length}` + ); + } + return { + recipients: addresses.map((addr, i) => ({ + address: addHexPrefix(addr as string), + amount: new BigNumber(bufferToHex(amounts[i] as Buffer)).toFixed(), + })), + }; +} + +/** + * Decode batchTransferFrom(address,address[],uint256[]) calldata used for token batch sends + * to the batcher contract. + * + * @param data Hex string starting with the batchTransferFrom method selector + * @returns Decoded token contract, recipients and amounts in calldata order + */ +export function decodeBatchTransferFromData(data: string): BatchTransferFromData { + if (!data.toLowerCase().startsWith(batchTransferFromMethodId)) { + throw new BuildTransactionError(`Invalid batchTransferFrom bytecode: ${data}`); + } + const [tokenContractAddress, addresses, amounts] = getRawDecoded( + batchTransferFromMethodTypes, + getBufferedByteCode(batchTransferFromMethodId, data) + ); + if (!Array.isArray(addresses) || !Array.isArray(amounts)) { + throw new BuildTransactionError(`Invalid batchTransferFrom bytecode: ${data}`); + } + if (addresses.length !== amounts.length) { + throw new BuildTransactionError( + `Mismatched batchTransferFrom address/amount array lengths: ${addresses.length} vs ${amounts.length}` + ); + } + return { + tokenContractAddress: addHexPrefix(tokenContractAddress as string), + recipients: addresses.map((addr, i) => ({ + address: addHexPrefix(addr as string), + amount: new BigNumber(bufferToHex(amounts[i] as Buffer)).toFixed(), + })), + }; +} + /** * Decode the given ABI-encoded transfer data for the sendMultisigToken function and return parsed fields * diff --git a/modules/abstract-eth/src/lib/walletUtil.ts b/modules/abstract-eth/src/lib/walletUtil.ts index 8d5891d0162..23349874561 100644 --- a/modules/abstract-eth/src/lib/walletUtil.ts +++ b/modules/abstract-eth/src/lib/walletUtil.ts @@ -1,5 +1,13 @@ export const sendMultisigMethodId = '0x39125215'; export const sendMultisigTokenMethodId = '0x0dcd7a6c'; +// Selector for batch(address[],uint256[]) used by batcher contract sends (native batches). +export const batchMethodId = '0xc00c4e9e'; +export const batchMethodName = 'batch'; +export const batchMethodTypes = ['address[]', 'uint256[]']; +// Selector for batchTransferFrom(address,address[],uint256[]) used for token batches. +export const batchTransferFromMethodId = '0x4885b254'; +export const batchTransferFromMethodName = 'batchTransferFrom'; +export const batchTransferFromMethodTypes = ['address', 'address[]', 'uint256[]']; export const v1CreateForwarderMethodId = '0xfb90b320'; export const v4CreateForwarderMethodId = '0x13b2f75c'; export const v1WalletInitializationFirstBytes = '0x60806040'; diff --git a/modules/abstract-eth/src/tssVerification.ts b/modules/abstract-eth/src/tssVerification.ts new file mode 100644 index 00000000000..72dd6652928 --- /dev/null +++ b/modules/abstract-eth/src/tssVerification.ts @@ -0,0 +1,1109 @@ +/** + * Fail-closed EVM TSS intent verification. + * + * `verifyTssTransaction` must not let a user key share sign EVM bytes unless the decoded + * `to` / `value` / `data` match the user intent (TOB-BITGOEDMPC-2 / INFOSEC-671). The intent + * type picks the handler; each handler accepts a closed set of decoded transaction shapes + * and anything unmatched is a violation. Types are never derived from calldata: selectors + * are shared across intents and `payment + data` carries arbitrary calldata. + * + * The checks only return findings; `AbstractEthLikeNewCoins.verifyTssTransaction` applies the + * policy in one place. A `fatal` finding (a check that already threw before this module existed) + * always throws. `new` and `unhandled` findings are reported through `console.warn` plus an + * optional `onVerificationWarning` callback, and throw when the client opts into + * `strictTssVerification`. + */ +import { + DefiIntentParams, + ITransactionRecipient, + PopulatedIntent, + TssVerificationMismatch, + TssVerificationWarning, +} from '@bitgo/sdk-core'; +import { coins, ContractAddressDefinedToken } from '@bitgo/statics'; +import { Buffer } from 'buffer'; +import { addHexPrefix, bufferToHex } from 'ethereumjs-util'; +import { + batchMethodId, + batchTransferFromMethodId, + BatchTransferData, + BatchTransferFromData, + confidentialTransferNoProofMethodId, + confidentialTransferNoProofTypes, + confidentialTransferWithProofMethodId, + confidentialTransferWithProofTypes, + decodeApproveCalldata, + decodeBatchTransferData, + decodeBatchTransferFromData, + getBufferedByteCode, + getRawDecoded, +} from './lib'; + +/** ERC-20 `transfer(address,uint256)` selector. */ +export const erc20TransferMethodId = '0xa9059cbb'; +/** Maximum ERC-20 allowance: uint256 max. */ +export const MAX_UINT256 = ((1n << 256n) - 1n).toString(); + +/** + * Canonical verifier intent types with a fail-closed handler. Anything outside this set + * (staking, `*WithCallData`, `acceleration`, wrap / unwrap-native, `enableToken`, + * `bridgeFunds`, `defiRedeem`, …) has no handler and is flagged instead of silently passing. + */ +export type EvmTssIntentType = + | 'transfer' + | 'transferToken' + | 'tokenApproval' + | 'fillNonce' + | 'consolidate' + | 'defiApprove' + | 'defiDeposit' + | 'defiWithdraw' + | 'wrapApprove' + | 'contractCall' + | 'customTx'; + +/** + * Stored intent types that cannot be created on EVM and must never reach signing there. + * `contractCall` / `customTx` carry arbitrary calldata with no intent shape to verify. + */ +const UNSUPPORTED_EVM_INTENT_TYPES = new Set(['contractCall', 'customTx']); + +/** Stored (kebab-case) intent spellings mapped to canonical verifier types. */ +const INTENT_TYPE_ALIASES: Record = { + payment: 'transfer', + 'transfer-token': 'transferToken', + // `Wallet.prebuildTransactionTxRequests` switches on the lowercase spelling for token sends. + transfertoken: 'transferToken', + 'token-approval': 'tokenApproval', + 'fill-nonce': 'fillNonce', + 'bridge-funds': 'bridgeFunds', + 'enable-token': 'enableToken', + 'custom-tx': 'customTx', + 'contract-call': 'contractCall', + 'defi-approve': 'defiApprove', + 'defi-deposit': 'defiDeposit', + 'defi-withdraw': 'defiWithdraw', + 'defi-redeem': 'defiRedeem', + 'wrap-approve': 'wrapApprove', + 'wrap-native': 'wrapNative', + 'unwrap-native': 'unwrapNative', + 'finalize-unwrap': 'finalizeUnwrap', + 'stake-with-call-data': 'stakeWithCallData', + 'unstake-with-call-data': 'unstakeWithCallData', +}; + +/** Transaction fields decoded from the bytes about to be signed. */ +export interface DecodedEvmTx { + to: string; + value: string; + data: string; +} + +/** + * How a finding is enforced. `fatal` always throws: these are the checks that threw before + * fail-closed verification existed. `new` (a mismatch on a handled intent type) and `unhandled` + * (an intent type with no handler) follow the client's warn / strict policy. + */ +export type TssFindingSeverity = 'fatal' | 'new' | 'unhandled'; + +/** One rule the transaction bytes broke, with the severity that decides warn or throw. */ +export interface TssFinding extends TssVerificationWarning { + severity: TssFindingSeverity; +} + +/** One recipient of the user intent, flattened from txParams or the stored tx request intent. */ +export interface EvmTssExpectedRecipient { + address: string; + amount: string; + /** Raw calldata the intent declares for this recipient (WalletConnect / dApp flows). */ + data?: string; + tokenName?: string; + tokenContractAddress?: string; +} + +export interface EvmTssIntentContext { + /** Canonical verifier intent type (after {@link canonicalizeEvmTssIntentType}). */ + intentType: string; + recipients: EvmTssExpectedRecipient[]; + /** Token name declared by the caller (or, on the pending-approval path, the stored intent). */ + tokenName?: string; + /** + * Contract of the token the coin instance itself represents (e.g. `bitgo.coin('eth:usdc')`), + * undefined for base coins. Locally selected: it is the asset of any payment that declares no + * token of its own. + */ + coinTokenContractAddress?: string; + /** Wallet base address (`wallet.coinSpecific().baseAddress`). */ + baseAddress?: string; + /** Network batcher contract from statics (`network.batcherContractAddress`). */ + batcherAddress?: string; +} + +function kebabToCamel(value: string): string { + return value.replace(/-([a-z0-9])/g, (_, c: string) => c.toUpperCase()); +} + +/** + * Map a declared transaction type onto the canonical verifier type. Caller-supplied + * `txParams.type` and stored `intent.intentType` both flow through here, so kebab-case + * stored spellings (`payment`, `defi-approve`, …) land on their canonical handlers. + */ +export function canonicalizeEvmTssIntentType(rawType?: string): string { + if (!rawType) { + return ''; + } + const trimmed = rawType.trim(); + return INTENT_TYPE_ALIASES[trimmed.toLowerCase()] ?? INTENT_TYPE_ALIASES[trimmed] ?? kebabToCamel(trimmed); +} + +/** True when the canonical type must never be signed on EVM chains. */ +export function isUnsupportedEvmTssIntentType(canonicalType: string): boolean { + return UNSUPPORTED_EVM_INTENT_TYPES.has(canonicalType as EvmTssIntentType); +} + +function normalizeAddress(address?: string): string { + return (address ?? '').toLowerCase(); +} + +function toAddressString(value: unknown): string { + const raw = Buffer.isBuffer(value) ? bufferToHex(value as Buffer) : String(value); + return addHexPrefix(raw).toLowerCase(); +} + +function toBigInt(value: unknown): bigint { + if (typeof value === 'bigint') { + return value; + } + if (Buffer.isBuffer(value)) { + return BigInt(bufferToHex(value as Buffer)); + } + return BigInt(String(value)); +} + +function isEmptyCalldata(data?: string): boolean { + return !data || data.toLowerCase() === '0x'; +} + +function normalizeCalldata(data?: string): string { + return (data ?? '0x').toLowerCase(); +} + +function valuesEqual(a: unknown, b: unknown): boolean { + try { + return toBigInt(a) === toBigInt(b); + } catch { + return String(a) === String(b); + } +} + +function violation( + ctx: Pick, + reason: string, + expected: TssVerificationMismatch, + decoded: TssVerificationMismatch, + severity: TssFindingSeverity = 'new' +): TssFinding { + return { intentType: ctx.intentType, reason, expected, decoded, severity }; +} + +/** + * Resolve the on-chain contract of `tokenName` from statics. Returns undefined for unknown + * names and for coins without a contract address (native coins, known statics gaps). + */ +export function resolveTokenContractAddress(tokenName?: string): string | undefined { + if (!tokenName) { + return undefined; + } + try { + const coin = coins.get(tokenName); + return coin instanceof ContractAddressDefinedToken ? coin.contractAddress.toString() : undefined; + } catch { + return undefined; + } +} + +/** + * Contract of the token that `chain` itself represents (`bitgo.coin('eth:usdc')` is the USDC + * contract). Undefined for base coins and for native coins that merely expose an ERC-20 + * interface (`isToken: false`). The coin instance is chosen locally, so it is a trusted source + * of asset identity for payments that name no token of their own. + */ +export function resolveCoinTokenContractAddress(chain?: string): string | undefined { + if (!chain) { + return undefined; + } + try { + const coin = coins.get(chain); + return coin instanceof ContractAddressDefinedToken && coin.isToken ? coin.contractAddress.toString() : undefined; + } catch { + return undefined; + } +} + +/** The asset a payment moves, resolved only from locally declared identity. */ +type PaymentAsset = { kind: 'native' } | { kind: 'token'; contract: string } | { kind: 'unresolved'; reason: string }; + +const EVM_ADDRESS = /^0x[0-9a-fA-F]{40}$/; + +/** + * Resolve the asset a payment is declared to move. Identity comes from the recipient's own + * `tokenAddress` / `tokenName`, the top-level `tokenName`, and the coin instance, and nothing + * else: the context never carries server-supplied token metadata when the caller declared its + * own intent. No declared identity at all means native. Identity that cannot be resolved to a + * contract, or that names two different contracts, is `unresolved`: the intended asset is + * unknown, so no calldata shape can be accepted for it. + */ +function resolveAsset( + ctx: EvmTssIntentContext, + declared: { tokenName?: string; tokenContractAddress?: string } +): PaymentAsset { + const sources: { label: string; contract: string | undefined; native?: boolean }[] = []; + if (declared.tokenContractAddress !== undefined) { + sources.push({ + label: `tokenAddress '${declared.tokenContractAddress}'`, + contract: EVM_ADDRESS.test(declared.tokenContractAddress) ? declared.tokenContractAddress : undefined, + }); + } + for (const name of [declared.tokenName, ctx.tokenName]) { + if (!name) { + continue; + } + let staticsCoin; + try { + staticsCoin = coins.get(name); + } catch { + staticsCoin = undefined; + } + if (staticsCoin && !(staticsCoin instanceof ContractAddressDefinedToken && staticsCoin.isToken)) { + // The name of a native coin (`hteth`, or a native coin that merely exposes an ERC-20 + // interface such as `celo`): the same identity as declaring no token at all. + sources.push({ label: `coin '${name}'`, contract: undefined, native: true }); + } else { + sources.push({ label: `token '${name}'`, contract: resolveTokenContractAddress(name) }); + } + } + if (ctx.coinTokenContractAddress) { + sources.push({ label: 'the coin token', contract: ctx.coinTokenContractAddress }); + } + const tokenSources = sources.filter((source) => !source.native); + if (tokenSources.length === 0) { + return { kind: 'native' }; + } + const missing = tokenSources.find((source) => source.contract === undefined); + if (missing) { + return { + kind: 'unresolved', + reason: `${missing.label} cannot be resolved to a token contract: the intended asset cannot be verified`, + }; + } + const nativeSource = sources.find((source) => source.native); + if (nativeSource) { + return { + kind: 'unresolved', + reason: `${nativeSource.label} and ${tokenSources[0].label} name different assets: the intended asset is ambiguous`, + }; + } + const [first, ...rest] = tokenSources as { label: string; contract: string }[]; + const conflict = rest.find((source) => normalizeAddress(source.contract) !== normalizeAddress(first.contract)); + if (conflict) { + return { + kind: 'unresolved', + reason: `${first.label} and ${conflict.label} name different token contracts: the intended asset is ambiguous`, + }; + } + return { kind: 'token', contract: first.contract }; +} + +type CallerDeclarableParams = { type?: string; tokenName?: string; recipients?: ITransactionRecipient[] }; + +/** True when the caller named a transaction type or recipients, i.e. declared its own intent. */ +export function callerDeclaresIntent(callerParams?: CallerDeclarableParams): boolean { + return !!(callerParams?.type || callerParams?.recipients?.length); +} + +/** + * Decide which source speaks for the user intent. Exactly one does: + * + * - The caller declared a type or recipients: the caller's own params. The stored intent is + * dropped entirely: it must not select the handler, add calldata, name the asset, or supply + * consolidation / DeFi fields. + * - The caller declared nothing (signing a stored tx request, e.g. pending approval): the stored + * intent, which `resolveEffectiveTxParams` has already folded into `txParams`. + * + * `txParams` alone cannot answer this: with a stored `intent` present, `type` and `recipients` + * have already been back-filled into it from server data. `callerTxParams` is the untouched + * caller input (`undefined` when the caller supplied none). Without a stored intent `txParams` + * is the caller input. + */ +export function selectDeclaredIntent(params: { + txParams: T; + callerTxParams?: T; + intent?: PopulatedIntent; +}): { callerParams?: T; storedIntent?: PopulatedIntent } { + // `callerTxParams` is authoritative whenever the signing path supplies it. Without it, a + // stored `intent` means the caller supplied nothing (pending approval), and without an intent + // `txParams` is the caller input (direct `verifyTransaction` callers). + const callerParams = params.callerTxParams ?? (params.intent ? undefined : params.txParams); + const storedIntent = params.intent && !callerDeclaresIntent(callerParams) ? params.intent : undefined; + return { callerParams, storedIntent }; +} + +/** + * Build the handler context from the source chosen by {@link selectDeclaredIntent}: the stored + * intent (via the effective `txParams`) when `storedIntent` is set, otherwise the caller's own + * params. Absent local `data` stays absent and local token identity is never replaced. + */ +export function buildEvmTssIntentContext(params: { + intentType: string; + /** Effective params: back-filled from the stored intent when `storedIntent` is set. */ + txParams: CallerDeclarableParams; + /** The caller's own declared params. */ + callerParams?: CallerDeclarableParams; + /** Set only when the caller declared nothing and the stored intent speaks for the user. */ + storedIntent?: PopulatedIntent; + coinTokenContractAddress?: string; + baseAddress?: string; + batcherAddress?: string; +}): EvmTssIntentContext { + const { storedIntent } = params; + const source = storedIntent ? params.txParams : params.callerParams; + + const recipients: EvmTssExpectedRecipient[] = (source?.recipients ?? []).map((recipient, i) => { + const tokenContractAddress = + recipient.tokenAddress ?? storedIntent?.recipients?.[i]?.tokenData?.tokenContractAddress; + return { + address: recipient.address, + amount: String(recipient.amount), + ...(recipient.data !== undefined && { data: recipient.data }), + ...(recipient.tokenName !== undefined && { tokenName: recipient.tokenName }), + ...(tokenContractAddress !== undefined && { tokenContractAddress }), + }; + }); + + return { + intentType: params.intentType, + recipients, + tokenName: source?.tokenName ?? storedIntent?.tokenName, + coinTokenContractAddress: params.coinTokenContractAddress, + baseAddress: params.baseAddress, + batcherAddress: params.batcherAddress, + }; +} + +/** + * Map stored DeFi intent fields onto the `defiParams` consumed by the DeFi calldata checks. + * `shareTokenAmount` is the withdraw (share) amount; `amount` covers approve / deposit. + */ +export function mapDefiParamsFromIntent(intent: PopulatedIntent): DefiIntentParams | undefined { + const canonicalType = canonicalizeEvmTssIntentType(intent.intentType); + const rawAmount = + canonicalType === 'defiWithdraw' + ? intent.shareTokenAmount ?? intent.amount + : intent.amount ?? intent.shareTokenAmount; + const amount = typeof rawAmount === 'object' ? rawAmount?.value : rawAmount; + if (amount === undefined || amount === null) { + return undefined; + } + return { + vaultId: intent.vaultId ?? '', + amount: String(amount), + ...(intent.operationId !== undefined && { operationId: intent.operationId }), + }; +} + +/** + * R1 — one recipient that declares raw calldata (WalletConnect / dApp): `to`, `value` and + * `data` must each be exactly equal to the intent. + */ +function verifyExactCall(ctx: EvmTssIntentContext, tx: DecodedEvmTx, recipient: EvmTssExpectedRecipient): TssFinding[] { + const warnings: TssFinding[] = []; + const expected = { to: recipient.address, value: recipient.amount, data: recipient.data }; + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + if (normalizeAddress(tx.to) !== normalizeAddress(recipient.address)) { + warnings.push( + violation(ctx, 'transaction to does not match the recipient address given by the client', expected, decoded) + ); + } + if (!valuesEqual(tx.value, recipient.amount)) { + warnings.push(violation(ctx, 'transaction value does not match the amount given by the client', expected, decoded)); + } + if (normalizeCalldata(tx.data) !== normalizeCalldata(recipient.data)) { + warnings.push( + violation(ctx, 'transaction calldata does not match the calldata given by the client', expected, decoded) + ); + } + return warnings; +} + +/** + * The payment checks that threw before fail-closed verification existed, kept fatal and in their + * original order (amount, then destination). Empty calldata must match `to` and `value`; ERC-20 + * `transfer(address,uint256)` calldata must match the inner recipient and amount. A WalletConnect + * recipient whose own calldata is an ERC-20 transfer supplies that expectation instead. + */ +function verifyHistoricPayment( + ctx: EvmTssIntentContext, + tx: DecodedEvmTx, + recipient: EvmTssExpectedRecipient +): TssFinding[] { + const findings: TssFinding[] = []; + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + if (isEmptyCalldata(tx.data)) { + const expected = { to: recipient.address, value: recipient.amount, data: '0x' }; + if (!valuesEqual(tx.value, recipient.amount)) { + findings.push( + violation( + ctx, + 'the transaction amount in txPrebuild does not match the value given by client', + expected, + decoded, + 'fatal' + ) + ); + } + if (normalizeAddress(tx.to) !== normalizeAddress(recipient.address)) { + findings.push( + violation(ctx, 'destination address does not match with the recipient address', expected, decoded, 'fatal') + ); + } + return findings; + } + if (!normalizeCalldata(tx.data).startsWith(erc20TransferMethodId)) { + return findings; + } + + const [innerRecipient, innerAmount] = getRawDecoded( + ['address', 'uint256'], + getBufferedByteCode(erc20TransferMethodId, tx.data) + ); + const decodedInner = { ...decoded, to: toAddressString(innerRecipient), value: toBigInt(innerAmount).toString() }; + let expected = { to: recipient.address, value: recipient.amount, data: recipient.data }; + if (recipient.data && normalizeCalldata(recipient.data).startsWith(erc20TransferMethodId)) { + const [declaredRecipient, declaredAmount] = getRawDecoded( + ['address', 'uint256'], + getBufferedByteCode(erc20TransferMethodId, recipient.data) + ); + expected = { ...expected, to: toAddressString(declaredRecipient), value: toBigInt(declaredAmount).toString() }; + } + if (!valuesEqual(decodedInner.value, expected.value)) { + findings.push( + violation( + ctx, + 'the transaction amount in txPrebuild does not match the value given by client', + expected, + decodedInner, + 'fatal' + ) + ); + } + if (decodedInner.to !== normalizeAddress(expected.to)) { + findings.push( + violation(ctx, 'destination address does not match with the recipient address', expected, decodedInner, 'fatal') + ); + } + return findings; +} + +/** + * R3 — one ERC-20 recipient: `to` must be the token contract, `value` must be zero and the + * decoded `transfer(address,uint256)` recipient and amount must match the intent. + * ERC-7984 `confidentialTransfer` (`0x5bebed7e` / `0x2fb74e62`) is checked on the recipient + * only, since the amount is encrypted. On a payment the ERC-20 recipient and amount are already + * fatal checks (`verifyHistoricPayment`), so `checkInnerTransfer` is false there. + */ +function verifyTokenTransfer( + ctx: EvmTssIntentContext, + tx: DecodedEvmTx, + recipient: EvmTssExpectedRecipient, + tokenContract: string, + checkInnerTransfer: boolean +): TssFinding[] { + const warnings: TssFinding[] = []; + const data = normalizeCalldata(tx.data); + const selector = data.slice(0, 10); + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + const expected = { to: tokenContract, value: '0', data: recipient.data }; + + if (!valuesEqual(tx.value, '0')) { + warnings.push(violation(ctx, 'token transfer must not carry native value', expected, decoded)); + } + + if (normalizeAddress(tx.to) !== normalizeAddress(tokenContract)) { + warnings.push(violation(ctx, 'transaction to is not the intent token contract', expected, decoded)); + } + + if (selector === erc20TransferMethodId) { + if (!checkInnerTransfer) { + return warnings; + } + let decodedRecipient: string; + let decodedAmount: bigint; + try { + const [innerRecipient, innerAmount] = getRawDecoded( + ['address', 'uint256'], + getBufferedByteCode(erc20TransferMethodId, tx.data) + ); + decodedRecipient = toAddressString(innerRecipient); + decodedAmount = toBigInt(innerAmount); + } catch (e) { + warnings.push( + violation(ctx, `ERC-20 transfer calldata could not be decoded: ${(e as Error).message}`, expected, decoded) + ); + return warnings; + } + if (decodedRecipient !== normalizeAddress(recipient.address)) { + warnings.push( + violation( + ctx, + 'destination address does not match with the recipient address', + { ...expected, to: recipient.address, value: recipient.amount }, + { ...decoded, to: addHexPrefix(decodedRecipient), value: decodedAmount.toString() } + ) + ); + } + if (!valuesEqual(decodedAmount, recipient.amount)) { + warnings.push( + violation( + ctx, + 'the transaction amount in txPrebuild does not match the value given by client', + { ...expected, to: recipient.address, value: recipient.amount }, + { ...decoded, to: addHexPrefix(decodedRecipient), value: decodedAmount.toString() } + ) + ); + } + return warnings; + } + + if (selector === confidentialTransferNoProofMethodId || selector === confidentialTransferWithProofMethodId) { + const types = + selector === confidentialTransferNoProofMethodId + ? confidentialTransferNoProofTypes + : confidentialTransferWithProofTypes; + let decodedRecipient: string; + try { + const [innerRecipient] = getRawDecoded(types, getBufferedByteCode(selector, tx.data)); + decodedRecipient = toAddressString(innerRecipient); + } catch (e) { + warnings.push( + violation(ctx, `confidentialTransfer calldata could not be decoded: ${(e as Error).message}`, expected, decoded) + ); + return warnings; + } + if (decodedRecipient !== normalizeAddress(recipient.address)) { + warnings.push( + violation( + ctx, + 'confidentialTransfer recipient does not match the recipient address given by the client', + { ...expected, to: recipient.address }, + { ...decoded, to: addHexPrefix(decodedRecipient) } + ) + ); + } + return warnings; + } + + warnings.push( + violation( + ctx, + 'token transfer must use ERC-20 transfer(address,uint256) or ERC-7984 confidentialTransfer calldata', + expected, + decoded + ) + ); + return warnings; +} + +/** + * R4 — more than one recipient: `to` must be the network batcher contract and the decoded + * batch arrays must match the intent 1:1. Native `batch(address[],uint256[])` carries + * `value` = the sum of amounts; token `batchTransferFrom(address,address[],uint256[])` + * names the intent token contract and carries `value` = 0. + */ +function verifyBatch(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { + const warnings: TssFinding[] = []; + const data = normalizeCalldata(tx.data); + const selector = data.slice(0, 10); + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + const expectedTotal = ctx.recipients.reduce((sum, r) => sum + toBigInt(r.amount), 0n).toString(); + const expected = { to: ctx.batcherAddress, value: expectedTotal, data: undefined }; + + if (!ctx.batcherAddress) { + warnings.push( + violation( + ctx, + 'no batcher contract address is configured for this network; batch payment cannot be verified', + expected, + decoded + ) + ); + return warnings; + } + if (normalizeAddress(tx.to) !== normalizeAddress(ctx.batcherAddress)) { + warnings.push( + violation(ctx, 'batch transaction to does not match the network batcher contract', expected, decoded) + ); + } + const assets = ctx.recipients.map((recipient) => resolveAsset(ctx, recipient)); + const unresolvedAsset = assets.find((asset) => asset.kind === 'unresolved'); + if (unresolvedAsset?.kind === 'unresolved') { + warnings.push(violation(ctx, `batch payment: ${unresolvedAsset.reason}`, expected, decoded)); + return warnings; + } + const batchAsset = assets[0]; + const mixedAssets = assets.some( + (asset) => + asset.kind !== batchAsset.kind || + (asset.kind === 'token' && + batchAsset.kind === 'token' && + normalizeAddress(asset.contract) !== normalizeAddress(batchAsset.contract)) + ); + if (mixedAssets) { + warnings.push( + violation( + ctx, + 'batch payment recipients declare more than one asset: cannot verify one batch call', + expected, + decoded + ) + ); + return warnings; + } + + const compareRecipients = ( + recipients: { address: string; amount: string }[], + valueExpected: string, + valueReason: string + ): void => { + if (recipients.length !== ctx.recipients.length) { + warnings.push( + violation( + ctx, + `batch calldata recipient count (${recipients.length}) does not match intent recipients (${ctx.recipients.length})`, + expected, + decoded + ) + ); + return; + } + for (let i = 0; i < recipients.length; i++) { + const want = ctx.recipients[i]; + const got = recipients[i]; + if (normalizeAddress(got.address) !== normalizeAddress(want.address) || !valuesEqual(got.amount, want.amount)) { + warnings.push( + violation( + ctx, + `batch calldata entry ${i} does not match intent recipient`, + { ...expected, to: want.address, value: want.amount }, + { ...decoded, to: got.address, value: got.amount } + ) + ); + } + } + if (!valuesEqual(tx.value, valueExpected)) { + warnings.push(violation(ctx, valueReason, { ...expected, value: valueExpected }, decoded)); + } + }; + + if (selector === batchMethodId) { + if (batchAsset.kind !== 'native') { + warnings.push( + violation( + ctx, + 'token batch payment must use batchTransferFrom(address,address[],uint256[]), not native batch(address[],uint256[])', + expected, + decoded + ) + ); + return warnings; + } + let batch: BatchTransferData; + try { + batch = decodeBatchTransferData(tx.data); + } catch (e) { + warnings.push(violation(ctx, `batch calldata could not be decoded: ${(e as Error).message}`, expected, decoded)); + return warnings; + } + compareRecipients( + batch.recipients, + expectedTotal, + 'batch transaction value does not match the sum of intent amounts' + ); + return warnings; + } + + if (selector === batchTransferFromMethodId) { + if (batchAsset.kind !== 'token') { + warnings.push( + violation( + ctx, + 'native batch payment must use batch(address[],uint256[]), not batchTransferFrom(address,address[],uint256[])', + expected, + decoded + ) + ); + return warnings; + } + let batch: BatchTransferFromData; + try { + batch = decodeBatchTransferFromData(tx.data); + } catch (e) { + warnings.push( + violation(ctx, `batchTransferFrom calldata could not be decoded: ${(e as Error).message}`, expected, decoded) + ); + return warnings; + } + if (normalizeAddress(batch.tokenContractAddress) !== normalizeAddress(batchAsset.contract)) { + warnings.push( + violation( + ctx, + 'batchTransferFrom token contract does not match the intent token contract', + { ...expected, to: batchAsset.contract }, + { ...decoded, to: batch.tokenContractAddress } + ) + ); + } + compareRecipients(batch.recipients, '0', 'token batch transaction must not carry native value'); + return warnings; + } + + warnings.push( + violation( + ctx, + 'batch payment must use batch(address[],uint256[]) or batchTransferFrom(address,address[],uint256[]) calldata', + expected, + decoded + ) + ); + return warnings; +} + +/** Payment handler (`transfer` / `payment`) — rules R1–R5. */ +function verifyPayment(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + if (ctx.recipients.length === 0) { + return [ + violation( + ctx, + 'payment intent has no recipients: the transaction bytes cannot be verified against intent', + {}, + decoded + ), + ]; + } + if (ctx.recipients.length > 1) { + return verifyBatch(ctx, tx); + } + const recipient = ctx.recipients[0]; + return [...verifyHistoricPayment(ctx, tx, recipient), ...verifyPaymentShape(ctx, tx, recipient)]; +} + +/** + * The new checks for a single-recipient payment: the calldata shape must match the declared + * asset. The fields `verifyHistoricPayment` already compares fatally are not compared again. + */ +function verifyPaymentShape( + ctx: EvmTssIntentContext, + tx: DecodedEvmTx, + recipient: EvmTssExpectedRecipient +): TssFinding[] { + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + if (recipient.data !== undefined) { + return verifyExactCall(ctx, tx, recipient); + } + const asset = resolveAsset(ctx, recipient); + if (asset.kind === 'unresolved') { + return [violation(ctx, `payment ${asset.reason}`, { to: recipient.address, value: recipient.amount }, decoded)]; + } + const selector = normalizeCalldata(tx.data).slice(0, 10); + const isTokenShape = + selector === erc20TransferMethodId || + selector === confidentialTransferNoProofMethodId || + selector === confidentialTransferWithProofMethodId; + const isBatchShape = selector === batchMethodId || selector === batchTransferFromMethodId; + if (asset.kind === 'native') { + if (isEmptyCalldata(tx.data)) { + // `to` and `value` of a native send are fatal checks in verifyHistoricPayment. + return []; + } + if (isTokenShape || isBatchShape) { + return [ + violation( + ctx, + 'payment intent declares a native asset but the transaction carries token or batch calldata', + { to: recipient.address, value: recipient.amount, data: '0x' }, + decoded + ), + ]; + } + } else { + if (isEmptyCalldata(tx.data)) { + return [ + violation( + ctx, + 'token payment intent but the transaction is a native send: expected ERC-20 transfer calldata', + { to: asset.contract, value: '0', data: undefined }, + decoded + ), + ]; + } + if (isTokenShape) { + return verifyTokenTransfer(ctx, tx, recipient, asset.contract, false); + } + if (isBatchShape) { + return [ + violation( + ctx, + 'batch calldata cannot be verified against a single declared recipient', + { to: recipient.address, value: recipient.amount }, + decoded + ), + ]; + } + } + // R5 — anything else (e.g. `approve(attacker, MAX)` disguised as a payment) is a violation. + return [ + violation( + ctx, + 'payment intent calldata is not a recognized payment shape (native send, ERC-20 transfer, ' + + 'ERC-7984 confidentialTransfer, or batch): refusing to treat it as a payment', + { to: recipient.address, value: recipient.amount, data: '0x' }, + decoded + ), + ]; +} + +/** `transferToken` handler — same shape set as R3, exactly one recipient, which must move a token. */ +function verifyTransferToken(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + if (ctx.recipients.length !== 1) { + return [ + violation( + ctx, + `transferToken intent must declare exactly one token recipient (found ${ctx.recipients.length})`, + {}, + decoded + ), + ]; + } + const recipient = ctx.recipients[0]; + const asset = resolveAsset(ctx, recipient); + if (asset.kind === 'unresolved') { + return [ + violation(ctx, `transferToken ${asset.reason}`, { to: recipient.address, value: recipient.amount }, decoded), + ]; + } + if (asset.kind === 'native') { + return [ + violation( + ctx, + 'transferToken intent names no token: the intended asset cannot be verified', + { to: recipient.address, value: recipient.amount }, + decoded + ), + ]; + } + if (isEmptyCalldata(tx.data)) { + return [ + violation( + ctx, + 'transferToken transaction must use ERC-20 transfer(address,uint256) calldata; expected token calldata, got an empty data payload', + { to: asset.contract, value: '0' }, + decoded + ), + ]; + } + return verifyTokenTransfer(ctx, tx, recipient, asset.contract, true); +} + +/** + * `tokenApproval` handler — `to` must be the token contract and the calldata must be + * `approve(spender = batcher, MAX)`, with `value` = 0. Guards against a substituted + * `approve(attacker, MAX_UINT256)`. + */ +function verifyTokenApproval(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { + const warnings: TssFinding[] = []; + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + const asset = resolveAsset(ctx, {}); + const tokenContract = asset.kind === 'token' ? asset.contract : undefined; + const expected = { to: tokenContract, value: '0', data: undefined }; + + if (!valuesEqual(tx.value, '0')) { + warnings.push(violation(ctx, 'tokenApproval transaction must not carry native value', expected, decoded)); + } + if (!tokenContract) { + warnings.push( + violation( + ctx, + asset.kind === 'unresolved' + ? `tokenApproval ${asset.reason}` + : `tokenApproval intent names no token contract that can be resolved from statics ('${ + ctx.tokenName ?? '' + }'); transaction to cannot be verified`, + expected, + decoded + ) + ); + } else if (normalizeAddress(tx.to) !== normalizeAddress(tokenContract)) { + warnings.push(violation(ctx, 'tokenApproval transaction to is not the intent token contract', expected, decoded)); + } + + let spender: string; + let amount: string; + try { + const decodedApprove = decodeApproveCalldata(tx.data); + spender = normalizeAddress(decodedApprove.spender); + amount = decodedApprove.amount; + } catch (e) { + warnings.push( + violation( + ctx, + `tokenApproval transaction must use ERC-20 approve(address,uint256) calldata: ${(e as Error).message}`, + expected, + decoded + ) + ); + return warnings; + } + + if (!ctx.batcherAddress) { + warnings.push( + violation( + ctx, + 'no batcher contract address is configured for this network; tokenApproval spender cannot be verified', + expected, + { ...decoded, to: spender, value: amount } + ) + ); + } else if (normalizeAddress(spender) !== normalizeAddress(ctx.batcherAddress)) { + warnings.push( + violation( + ctx, + 'tokenApproval spender is not the network batcher contract', + { ...expected, to: ctx.batcherAddress }, + { ...decoded, to: spender, value: amount } + ) + ); + } + if (!valuesEqual(amount, MAX_UINT256)) { + warnings.push( + violation( + ctx, + 'tokenApproval amount is not the maximum ERC-20 allowance', + { ...expected, value: MAX_UINT256 }, + { ...decoded, to: spender, value: amount } + ) + ); + } + return warnings; +} + +/** `fillNonce` handler — a 1-wei self-send to the wallet base address. */ +function verifyFillNonce(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { + const warnings: TssFinding[] = []; + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + const expected = { to: ctx.baseAddress, value: '1', data: '0x' }; + if (!ctx.baseAddress) { + warnings.push(violation(ctx, 'fillNonce intent has no wallet base address to verify against', expected, decoded)); + return warnings; + } + if (normalizeAddress(tx.to) !== normalizeAddress(ctx.baseAddress)) { + warnings.push(violation(ctx, 'fillNonce transaction must send to the wallet base address', expected, decoded)); + } + if (!valuesEqual(tx.value, '1')) { + warnings.push(violation(ctx, 'fillNonce transaction must send exactly 1 wei', expected, decoded)); + } + if (!isEmptyCalldata(tx.data)) { + warnings.push(violation(ctx, 'fillNonce transaction must not carry calldata', expected, decoded)); + } + return warnings; +} + +function unhandledTypeViolation(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding { + return violation( + ctx, + `no fail-closed verifier for intent type '${ctx.intentType}': transaction bytes were not compared against intent`, + {}, + { to: tx.to, value: tx.value, data: tx.data }, + 'unhandled' + ); +} + +/** + * A consolidation must pay the wallet base address: a native send to it, or an ERC-20 transfer + * to it that carries no native value. Without the value check, `transfer(baseAddress, 0)` + * calldata sent to an arbitrary contract would move the native balance there. + */ +export function verifyConsolidation(tx: DecodedEvmTx, baseAddress: string, severity: TssFindingSeverity): TssFinding[] { + const ctx = { intentType: 'consolidate' }; + const expected = { to: baseAddress }; + const decoded = { to: tx.to, value: tx.value }; + if (!normalizeCalldata(tx.data).startsWith(erc20TransferMethodId)) { + return normalizeAddress(tx.to) === normalizeAddress(baseAddress) + ? [] + : [ + violation( + ctx, + 'Consolidation transaction recipient does not match wallet base address', + expected, + decoded, + severity + ), + ]; + } + // For ERC-20 consolidations `to` is the token contract; the real recipient is in the calldata. + const findings: TssFinding[] = []; + const [innerRecipient] = getRawDecoded(['address', 'uint256'], getBufferedByteCode(erc20TransferMethodId, tx.data)); + const decodedRecipient = toAddressString(innerRecipient); + if (decodedRecipient !== normalizeAddress(baseAddress)) { + findings.push( + violation( + ctx, + 'Consolidation transaction recipient does not match wallet base address', + expected, + { ...decoded, to: decodedRecipient }, + severity + ) + ); + } + if (!valuesEqual(tx.value, '0')) { + findings.push( + violation( + ctx, + 'Consolidation token transfer must not carry native value', + { ...expected, value: '0' }, + decoded, + severity + ) + ); + } + return findings; +} + +/** + * Evaluate the decoded transaction against the intent. Returns every violated rule for the + * canonical intent type; an empty array means the bytes match the closed set of accepted + * shapes for that type. + */ +export function evaluateEvmTssIntent(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { + switch (ctx.intentType) { + case 'transfer': + return verifyPayment(ctx, tx); + case 'transferToken': + return verifyTransferToken(ctx, tx); + case 'tokenApproval': + return verifyTokenApproval(ctx, tx); + case 'fillNonce': + return verifyFillNonce(ctx, tx); + case 'consolidate': + // The base-address check in verifyTssTransaction is the handler for consolidations. + return []; + case 'defiApprove': + case 'defiDeposit': + case 'defiWithdraw': + case 'wrapApprove': + // Dedicated fail-closed calldata checks in verifyTssTransaction handle these. + return []; + default: + return [unhandledTypeViolation(ctx, tx)]; + } +} diff --git a/modules/sdk-api/src/bitgoAPI.ts b/modules/sdk-api/src/bitgoAPI.ts index 16bbb40b7d8..5eb419557bc 100644 --- a/modules/sdk-api/src/bitgoAPI.ts +++ b/modules/sdk-api/src/bitgoAPI.ts @@ -24,6 +24,7 @@ import { IRequestTracer, makeRandomKey, sanitizeLegacyPath, + TssVerificationWarningHandler, } from '@bitgo/sdk-core'; import * as sdkHmac from '@bitgo/sdk-hmac'; import { DefaultHmacAuthStrategy, type IHmacAuthStrategy } from '@bitgo/sdk-hmac'; @@ -283,10 +284,16 @@ export class BitGoAPI implements BitGoBase { private _requestIdPrefix?: string; private getAdditionalHeadersCb?: AdditionalHeadersCallback; protected _hmacAuthStrategy: IHmacAuthStrategy; + /** @inheritDoc */ + public readonly strictTssVerification: boolean; + /** @inheritDoc */ + public readonly onVerificationWarning?: TssVerificationWarningHandler; constructor(params: BitGoAPIOptions = {}) { this.getAdditionalHeadersCb = params.getAdditionalHeadersCb; this._requestIdPrefix = params.requestIdPrefix; + this.strictTssVerification = params.strictTssVerification ?? false; + this.onVerificationWarning = params.onVerificationWarning; this.cookiesPropagationEnabled = false; if ( !common.validateParams( diff --git a/modules/sdk-api/src/types.ts b/modules/sdk-api/src/types.ts index 8e51d6d8d1f..d1136b8af9d 100644 --- a/modules/sdk-api/src/types.ts +++ b/modules/sdk-api/src/types.ts @@ -1,4 +1,10 @@ -import { EncryptionVersion, EnvironmentName, IRequestTracer, V1Network } from '@bitgo/sdk-core'; +import { + EncryptionVersion, + EnvironmentName, + IRequestTracer, + TssVerificationWarningHandler, + V1Network, +} from '@bitgo/sdk-core'; import { type BitGoTlsPolicy } from './tlsPolicy'; import { ECPairInterface } from '@bitgo/utxo-lib'; import { type Agent } from 'http'; @@ -76,6 +82,16 @@ export interface BitGoAPIOptions { cookiesPropagationEnabled?: boolean; getAdditionalHeadersCb?: AdditionalHeadersCallback; requestIdPrefix?: string; + /** + * Client-side opt-in: fail-closed EVM TSS verification warnings become hard failures + * instead of `console.warn` + `onVerificationWarning`. Never read from server data. + */ + strictTssVerification?: boolean; + /** + * Sink for fail-closed EVM TSS verification warnings (decoded vs expected `to` / + * `value` / `data` for a transaction about to be signed). + */ + onVerificationWarning?: TssVerificationWarningHandler; evm?: { [key: string]: { baseUrl?: string; diff --git a/modules/sdk-coin-bsc/test/unit/tssBatcherVerification.ts b/modules/sdk-coin-bsc/test/unit/tssBatcherVerification.ts new file mode 100644 index 00000000000..e69e5596489 --- /dev/null +++ b/modules/sdk-coin-bsc/test/unit/tssBatcherVerification.ts @@ -0,0 +1,312 @@ +import 'should'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; +import { coins, ContractAddressDefinedToken, Erc7984Coin } from '@bitgo/statics'; +import { TransactionType, TssVerificationWarning, Wallet } from '@bitgo/sdk-core'; +import { optionalDeps } from '@bitgo/abstract-eth'; + +import { Tbsc } from '../../src'; +import { TransactionBuilder } from '../../src/lib'; + +/** + * Fail-closed EVM TSS verification for batcher-contract flows (CECHO-2460). + * + * Native batches (`batch(address[],uint256[])`) and token batches + * (`batchTransferFrom(address,address[],uint256[])`) must reach the network's batcher + * contract with the intent's recipients 1:1, and `tokenApproval` must be + * `approve(batcher, MAX)` on the intent token contract. tbsc is used because its statics + * network carries a real batcher contract address. + */ + +const recipientAddress = '0x174cfd823af8ce27ed0afee3fcf3c3ba259116be'; +const attackerAddress = '0x7e85bdc27c050e3905ebf4b8e634d9ad6edd0de6'; +const tokenContractAddress = '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48'; +const MAX_UINT256 = ((1n << 256n) - 1n).toString(); + +const erc7984Pair = coins.get('hteth:cusdt') as Erc7984Coin; +const erc7984WrapperAddress = String(erc7984Pair.contractAddress); + +// Two distinct tbsc tokens, so a batch can be shown to move exactly one asset. +const tokenContractOf = (tokenName: string): string => { + const token = coins.get(tokenName); + if (!(token instanceof ContractAddressDefinedToken)) { + throw new Error(`${tokenName} has no contract address in statics`); + } + return String(token.contractAddress); +}; +const busdTokenName = 'tbsc:busd'; +const usd1TokenName = 'tbsc:usd1'; +const busdContractAddress = tokenContractOf(busdTokenName); + +const word = (value: bigint): string => value.toString(16).padStart(64, '0'); +const addrWord = (address: string): string => address.toLowerCase().replace(/^0x/, '').padStart(64, '0'); + +function encodeApprove(spender: string, amount: string): string { + return `0x095ea7b3${addrWord(spender)}${word(BigInt(amount))}`; +} + +function encodeWithAbi(signature: string, types: string[], values: unknown[]): string { + const selector = optionalDeps.ethAbi.methodID(signature, types); + const args = optionalDeps.ethAbi.rawEncode(types, values); + return '0x' + Buffer.concat([selector, args]).toString('hex'); +} + +function encodeBatch(addresses: string[], amounts: string[]): string { + return encodeWithAbi('batch', ['address[]', 'uint256[]'], [addresses, amounts]); +} + +function encodeBatchTransferFrom(token: string, addresses: string[], amounts: string[]): string { + return encodeWithAbi('batchTransferFrom', ['address', 'address[]', 'uint256[]'], [token, addresses, amounts]); +} + +async function buildTxHex(opts: { to: string; value?: string; data?: string }): Promise { + const builder = new TransactionBuilder(coins.get('tbsc')); + builder.type(opts.data && opts.data !== '0x' ? TransactionType.ContractCall : TransactionType.SingleSigSend); + builder.fee({ fee: '10', gasLimit: '21000' }); + builder.counter(1); + builder.contract(opts.to); + if (opts.data) { + builder.data(opts.data); + } + if (opts.value) { + builder.value(opts.value); + } + return (await builder.build()).toBroadcastFormat(); +} + +describe('EVM TSS fail-closed verification (batcher contract flows)', function () { + const batcherAddress = String( + (coins.get('tbsc').network as unknown as { batcherContractAddress: string }).batcherContractAddress + ); + + let bitgo: TestBitGoAPI; + let strictBitgo: TestBitGoAPI; + let warnings: TssVerificationWarning[]; + let coin: Tbsc; + let wallet: Wallet; + + beforeEach(function () { + warnings = []; + bitgo = TestBitGo.decorate(BitGoAPI, { + env: 'test', + onVerificationWarning: (warning) => warnings.push(warning), + }); + strictBitgo = TestBitGo.decorate(BitGoAPI, { + env: 'test', + strictTssVerification: true, + }); + bitgo.safeRegister('tbsc', Tbsc.createInstance); + bitgo.initializeTestVars(); + strictBitgo.safeRegister('tbsc', Tbsc.createInstance); + strictBitgo.initializeTestVars(); + coin = bitgo.coin('tbsc') as Tbsc; + wallet = new Wallet( + bitgo, + coin as any, + { + coinSpecific: { baseAddress: recipientAddress }, + } as any + ); + }); + + const verify = (txParams: Record, txHex: string, strict = false): Promise => { + const target = strict ? (strictBitgo.coin('tbsc') as Tbsc) : coin; + const w = strict + ? new Wallet(strictBitgo, target as any, { coinSpecific: { baseAddress: recipientAddress } } as any) + : wallet; + return (target as any).verifyTransaction({ + txParams, + txPrebuild: { txHex, coin: 'tbsc', walletId: 'w' }, + wallet: w, + verification: {}, + walletType: 'tss', + }); + }; + + it('pins the hardcoded batch selectors to their runtime-computed values', function () { + encodeBatch([recipientAddress], ['1']).slice(0, 10).should.equal('0xc00c4e9e'); + encodeBatchTransferFrom(tokenContractAddress, [recipientAddress], ['1']).slice(0, 10).should.equal('0x4885b254'); + }); + + describe('native batch', function () { + const recipients = [ + { address: recipientAddress, amount: '10' }, + { address: attackerAddress, amount: '20' }, + ]; + + it('verifies batch calldata against the batcher contract and the intent arrays', async function () { + const txHex = await buildTxHex({ + to: batcherAddress, + value: '30', + data: encodeBatch([recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients }, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('flags swapped inner recipients and inflated batch value', async function () { + const swappedHex = await buildTxHex({ + to: batcherAddress, + value: '30', + data: encodeBatch([attackerAddress, recipientAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients }, swappedHex)).should.equal(true); + warnings.some((w) => /entry 0 does not match/.test(w.reason)).should.equal(true); + await verify({ type: 'transfer', recipients }, swappedHex, true).should.be.rejectedWith(/entry 0 does not match/); + + warnings = []; + const inflatedHex = await buildTxHex({ + to: batcherAddress, + value: '999', + data: encodeBatch([recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients }, inflatedHex)).should.equal(true); + warnings.some((w) => /value does not match the sum/.test(w.reason)).should.equal(true); + }); + + it('flags a batch sent somewhere other than the network batcher contract', async function () { + const txHex = await buildTxHex({ + to: attackerAddress, + value: '30', + data: encodeBatch([recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients }, txHex)).should.equal(true); + warnings.some((w) => /does not match the network batcher contract/.test(w.reason)).should.equal(true); + }); + + it('flags batch-shaped sends whose calldata is not batch or batchTransferFrom', async function () { + const txHex = await buildTxHex({ + to: batcherAddress, + value: '30', + data: encodeWithAbi('transfer', ['address', 'uint256'], [recipientAddress, '30']), + }); + (await verify({ type: 'transfer', recipients }, txHex)).should.equal(true); + warnings.some((w) => /must use batch\(address\[\],uint256\[\]\)/.test(w.reason)).should.equal(true); + }); + }); + + describe('token batch', function () { + const recipients = [ + { address: recipientAddress, amount: '10', tokenName: 'hteth:cusdt' }, + { address: attackerAddress, amount: '20', tokenName: 'hteth:cusdt' }, + ]; + + it('verifies batchTransferFrom against the intent token contract with zero value', async function () { + const txHex = await buildTxHex({ + to: batcherAddress, + value: '0', + data: encodeBatchTransferFrom(erc7984WrapperAddress, [recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients }, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('flags a token batch whose token contract is not the intent token', async function () { + const txHex = await buildTxHex({ + to: batcherAddress, + value: '0', + data: encodeBatchTransferFrom(tokenContractAddress, [recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients }, txHex)).should.equal(true); + warnings.some((w) => /token contract does not match/.test(w.reason)).should.equal(true); + await verify({ type: 'transfer', recipients }, txHex, true).should.be.rejectedWith( + /token contract does not match/ + ); + }); + }); + + describe('batch asset binding (CECHO-2460)', function () { + const nativeRecipients = [ + { address: recipientAddress, amount: '10' }, + { address: attackerAddress, amount: '20' }, + ]; + const tokenRecipients = [ + { address: recipientAddress, amount: '10', tokenName: busdTokenName }, + { address: attackerAddress, amount: '20', tokenName: busdTokenName }, + ]; + + it('rejects native batch calldata for a token batch intent', async function () { + const txHex = await buildTxHex({ + to: batcherAddress, + value: '30', + data: encodeBatch([recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients: tokenRecipients }, txHex)).should.equal(true); + warnings.some((w) => /token batch payment must use batchTransferFrom/.test(w.reason)).should.equal(true); + await verify({ type: 'transfer', recipients: tokenRecipients }, txHex, true).should.be.rejectedWith( + /token batch payment must use batchTransferFrom/ + ); + }); + + it('rejects batchTransferFrom calldata for a native batch intent', async function () { + const txHex = await buildTxHex({ + to: batcherAddress, + value: '0', + data: encodeBatchTransferFrom(busdContractAddress, [recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients: nativeRecipients }, txHex)).should.equal(true); + warnings + .some((w) => /native batch payment must use batch\(address\[\],uint256\[\]\)/.test(w.reason)) + .should.equal(true); + await verify({ type: 'transfer', recipients: nativeRecipients }, txHex, true).should.be.rejectedWith( + /native batch payment must use batch\(address\[\],uint256\[\]\)/ + ); + }); + + it('rejects a batch whose recipients declare two different tokens', async function () { + const mixedRecipients = [ + { address: recipientAddress, amount: '10', tokenName: busdTokenName }, + { address: attackerAddress, amount: '20', tokenName: usd1TokenName }, + ]; + const txHex = await buildTxHex({ + to: batcherAddress, + value: '0', + data: encodeBatchTransferFrom(busdContractAddress, [recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients: mixedRecipients }, txHex)).should.equal(true); + warnings.some((w) => /more than one asset/.test(w.reason)).should.equal(true); + await verify({ type: 'transfer', recipients: mixedRecipients }, txHex, true).should.be.rejectedWith( + /more than one asset/ + ); + }); + + it('rejects a batch with an unresolvable token identity', async function () { + const unresolvedRecipients = [ + { address: recipientAddress, amount: '10', tokenName: 'tbsc:not-a-registered-token' }, + { address: attackerAddress, amount: '20', tokenName: 'tbsc:not-a-registered-token' }, + ]; + const txHex = await buildTxHex({ + to: batcherAddress, + value: '0', + data: encodeBatchTransferFrom(busdContractAddress, [recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients: unresolvedRecipients }, txHex)).should.equal(true); + warnings.some((w) => /cannot be resolved to a token contract/.test(w.reason)).should.equal(true); + }); + }); + + describe('tokenApproval', function () { + const approvalParams = { type: 'tokenApproval', tokenName: 'hteth:cusdt' }; + + it('verifies approve(batcher, MAX) on the intent token contract', async function () { + const txHex = await buildTxHex({ + to: erc7984WrapperAddress, + value: '0', + data: encodeApprove(batcherAddress, MAX_UINT256), + }); + (await verify(approvalParams, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('flags approve(attacker, MAX) under a tokenApproval intent (TOB-BITGOEDMPC-2)', async function () { + const txHex = await buildTxHex({ + to: erc7984WrapperAddress, + value: '0', + data: encodeApprove(attackerAddress, MAX_UINT256), + }); + (await verify(approvalParams, txHex)).should.equal(true); + warnings.some((w) => /spender is not the network batcher contract/.test(w.reason)).should.equal(true); + await verify(approvalParams, txHex, true).should.be.rejectedWith(/spender is not the network batcher contract/); + }); + }); +}); diff --git a/modules/sdk-coin-eth/test/unit/eth.ts b/modules/sdk-coin-eth/test/unit/eth.ts index 9eeb9106d37..aa9df0ed083 100644 --- a/modules/sdk-coin-eth/test/unit/eth.ts +++ b/modules/sdk-coin-eth/test/unit/eth.ts @@ -1626,7 +1626,7 @@ describe('ETH:', function () { }); }; - ['tokenApproval', 'customTx', 'defiApprove'].forEach((type) => { + ['tokenApproval', 'acceleration', 'defiApprove'].forEach((type) => { it(`should reject approve(wrapper, amount) on the underlying relabelled as ${type}`, async function () { await expectDefiRejection( verifyAs(type, defiCalldata('approve(address,uint256)', [wrapper, amount])), @@ -1635,62 +1635,44 @@ describe('ETH:', function () { }); }); - it('should reject a mutated-amount wrapper approve relabelled as customTx', async function () { + it('should reject customTx and contractCall outright as unsupported EVM TSS intent types', async function () { + for (const type of ['customTx', 'contractCall']) { + await verifyAs(type, defiCalldata('approve(address,uint256)', [wrapper, amount])).should.be.rejectedWith( + /unsupported intent type for EVM TSS/ + ); + } + }); + + it('should reject a mutated-amount wrapper approve relabelled as another type', async function () { await expectDefiRejection( - verifyAs('customTx', defiCalldata('approve(address,uint256)', [wrapper, '999999999999'])), + verifyAs('acceleration', defiCalldata('approve(address,uint256)', [wrapper, '999999999999'])), /must be declared as a wrapApprove intent/ ); }); it('should reject a wrapper approve with trailing bytes (an ERC-20 ignores them)', async function () { await expectDefiRejection( - verifyAs('customTx', defiCalldata('approve(address,uint256)', [wrapper, amount]) + '00'), + verifyAs('acceleration', defiCalldata('approve(address,uint256)', [wrapper, amount]) + '00'), /must be declared as a wrapApprove intent/ ); }); it('should not affect approvals of the underlying to other spenders', async function () { - (await verifyAs('customTx', defiCalldata('approve(address,uint256)', [address2, amount]))).should.equal(true); + (await verifyAs('acceleration', defiCalldata('approve(address,uint256)', [address2, amount]))).should.equal( + true + ); (await verifyAs('tokenApproval', defiCalldata('approve(address,uint256)', [address2, amount]))).should.equal( true ); }); - it('should not parse the tx at all when the calldata carries no approve selector', async function () { - const coin = bitgo.coin('hteth') as Hteth; - const wallet = new Wallet(bitgo, coin, { coinSpecific: { baseAddress: address1 } }); - const verifyWith = async (data: string) => { - const txHex = await buildDefiTxHex(data, underlying); - // getTransactionBuilder is not part of the coin's public type, so spy through a minimal structural view. - const builderSpy = sinon.spy( - coin as unknown as { getTransactionBuilder: () => unknown }, - 'getTransactionBuilder' - ); - try { - await coin.verifyTransaction({ - txParams: { type: 'customTx', wallet } as any, - txPrebuild: { txHex, coin: 'hteth', walletId: 'w' } as any, - wallet, - verification: {}, - walletType: 'tss', - }); - return builderSpy.callCount; - } finally { - builderSpy.restore(); - } - }; - - (await verifyWith(defiCalldata('transfer(address,uint256)', [wrapper, amount]))).should.equal(0); - (await verifyWith(defiCalldata('approve(address,uint256)', [address2, amount]))).should.equal(1); - }); - it('should leave other declared types untouched for txs that are not a wrapper approve', async function () { // Every type below skips the wrapApprove check and reaches the guard; none of these txs is an approve of a // wrapper, so all must verify exactly as they did before the guard existed. const benign: Array<[string, string, string]> = [ - ['customTx', defiCalldata('transfer(address,uint256)', [address2, amount]), underlying], - ['customTx', defiCalldata('approve(address,uint256)', [address2, amount]), underlying], - ['customTx', defiCalldata('approve(address,uint256)', [wrapper, amount]), address2], + ['bridgeFunds', defiCalldata('transfer(address,uint256)', [address2, amount]), underlying], + ['bridgeFunds', defiCalldata('approve(address,uint256)', [address2, amount]), underlying], + ['bridgeFunds', defiCalldata('approve(address,uint256)', [wrapper, amount]), address2], ['tokenApproval', defiCalldata('approve(address,uint256)', [address2, amount]), underlying], ['tokenApproval', defiCalldata('approve(address,uint256)', [wrapper, amount]), address2], ['acceleration', defiCalldata('transfer(address,uint256)', [address2, amount]), address2], @@ -1703,9 +1685,11 @@ describe('ETH:', function () { }); it('should not affect transfers to the underlying or approves of the wrapper on other contracts', async function () { - (await verifyAs('customTx', defiCalldata('transfer(address,uint256)', [wrapper, amount]))).should.equal(true); + (await verifyAs('acceleration', defiCalldata('transfer(address,uint256)', [wrapper, amount]))).should.equal( + true + ); ( - await verifyAs('customTx', defiCalldata('approve(address,uint256)', [wrapper, amount]), address2) + await verifyAs('acceleration', defiCalldata('approve(address,uint256)', [wrapper, amount]), address2) ).should.equal(true); }); diff --git a/modules/sdk-coin-evm/src/evmCoin.ts b/modules/sdk-coin-evm/src/evmCoin.ts index 4da310e95cb..1013b8b4732 100644 --- a/modules/sdk-coin-evm/src/evmCoin.ts +++ b/modules/sdk-coin-evm/src/evmCoin.ts @@ -9,7 +9,6 @@ import { MPCAlgorithm, MultisigType, multisigTypes, - NO_RECIPIENT_TX_TYPES, } from '@bitgo/sdk-core'; import { BaseCoin as StaticsBaseCoin, CoinFeature, coins, CoinFamily } from '@bitgo/statics'; import { @@ -17,7 +16,6 @@ import { ERC721TransferBuilder, recoveryBlockchainExplorerQuery, TransactionBuilder as EthLikeTransactionBuilder, - VerifyEthTransactionOptions, } from '@bitgo/abstract-eth'; import { TransactionBuilder } from './lib'; import { isHtsEvmAddress, recovery_HBAREVM_BlockchainExplorerQuery, validateHederaAccountId } from './lib/utils'; @@ -87,47 +85,6 @@ export class EvmCoin extends AbstractEthLikeNewCoins { } } - /** @inheritDoc */ - async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise { - const supportsEIP1559 = this.staticsCoin?.features?.includes(CoinFeature.EIP1559); - if (supportsEIP1559) { - return await super.verifyTssTransaction(params); - } else { - return await this.verifyLegacyTssTransaction(params); - } - } - - /** - * Verifies legacy (non-EIP-1559) TSS transactions with basic validation. - */ - private async verifyLegacyTssTransaction(params: VerifyEthTransactionOptions): Promise { - const { txParams, txPrebuild, wallet } = params; - - // Basic validation for legacy transactions only - if ( - !txParams?.recipients && - !( - txParams.prebuildTx?.consolidateId || - txParams.stakingRequestId || - txParams.prebuildTx?.stakingRequestId || - (txParams.type && NO_RECIPIENT_TX_TYPES.has(txParams.type)) - ) - ) { - throw new Error(`missing txParams`); - } - - if (!wallet || !txPrebuild) { - throw new Error(`missing params`); - } - - if (txParams.hop && txParams.recipients && txParams.recipients.length > 1) { - throw new Error(`tx cannot be both a batch and hop transaction`); - } - - // If validation passes, consider it verified - return true; - } - /** @inheritDoc */ isValidAddress(address: string, isAlternateAddress?: boolean): boolean { if (isAlternateAddress && this.getFamily() === CoinFamily.HBAREVM) { diff --git a/modules/sdk-coin-evm/test/unit/tssSigningPath.ts b/modules/sdk-coin-evm/test/unit/tssSigningPath.ts new file mode 100644 index 00000000000..148d16fff3f --- /dev/null +++ b/modules/sdk-coin-evm/test/unit/tssSigningPath.ts @@ -0,0 +1,125 @@ +import 'should'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { TestBitGo } from '@bitgo/sdk-test'; +import { coins } from '@bitgo/statics'; +import { + EcdsaUtils, + RequestTracer, + TransactionType, + TxIntentMismatchRecipientError, + TxRequest, + Wallet, +} from '@bitgo/sdk-core'; + +import { EvmCoin } from '../../src/evmCoin'; +import { TransactionBuilder } from '../../src/lib'; + +/** + * End to end on the ECDSA signing path (CECHO-2460). tssVerification.ts calls verifyTransaction + * directly; this suite runs the real EVM verifier inside EcdsaUtils.signTxRequest and proves that + * bytes it rejects never reach the signing round. + */ +describe('EVM TSS signing path stops tampered bytes before signing', function () { + const chain = 'kavaevm'; + const staticsCoin = coins.get(chain); + const walletId = '62fe536a6b4cf70007acb48c0e7bb0b0'; + const baseAddress = '0x1111111111111111111111111111111111111111'; + const recipientAddress = '0x174cfd823af8ce27ed0afee3fcf3c3ba259116be'; + const attackerAddress = '0x7e85bdc27c050e3905ebf4b8e634d9ad6edd0de6'; + const tokenContractAddress = '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48'; + const approveAttackerMax = `0x095ea7b3${attackerAddress.slice(2).padStart(64, '0')}${'f'.repeat(64)}`; + // Minimal signing material: signRequestBase only needs pShare.i === 1, a backupNShare and the + // paillier modulus before it hands the unsigned bytes to verifyTransaction. + const prv = JSON.stringify({ pShare: { i: 1, n: 'paillier-modulus' }, backupNShare: {} }); + const paymentIntent = { + intentType: 'payment', + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: chain } }], + }; + const callerPayment = { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }; + + const buildTxHex = async (opts: { to: string; value: string; data?: string }): Promise => { + const builder = new TransactionBuilder(staticsCoin); + builder.type(opts.data ? TransactionType.ContractCall : TransactionType.SingleSigSend); + builder.fee({ fee: '10', gasLimit: '21000' }); + builder.counter(1); + builder.contract(opts.to); + if (opts.data) { + builder.data(opts.data); + } + builder.value(opts.value); + return (await builder.build()).toBroadcastFormat(); + }; + + /** Signs through EcdsaUtils and reports whether the signing round (after verification) started. */ + const sign = async (opts: { + txHex: string; + txParams?: Record; + strict?: boolean; + }): Promise<{ signingStarted: boolean; error?: Error }> => { + const bitgo = TestBitGo.decorate(BitGoAPI, { + env: 'test', + strictTssVerification: opts.strict ?? false, + onVerificationWarning: () => undefined, + }); + const coin = new (EvmCoin as any)(bitgo, staticsCoin); + const wallet = new Wallet(bitgo, coin, { id: walletId, coinSpecific: { baseAddress }, multisigType: 'tss' } as any); + const utils = new EcdsaUtils(bitgo, coin, wallet); + let signingStarted = false; + (utils as any).getEcdsaSigningChallenges = async () => { + signingStarted = true; + throw new Error('stop: signing round started'); + }; + const txRequest = { + txRequestId: 'signing-path', + apiVersion: 'full', + walletId, + intent: paymentIntent, + transactions: [ + { + unsignedTx: { derivationPath: 'm/0', signableHex: opts.txHex, serializedTxHex: opts.txHex }, + signatureShares: [], + }, + ], + } as unknown as TxRequest; + try { + await utils.signTxRequest({ txRequest, txParams: opts.txParams as any, prv, reqId: new RequestTracer() }); + } catch (e) { + return { signingStarted, error: e as Error }; + } + return { signingStarted }; + }; + + it('reaches the signing round for a payment whose bytes match the intent', async function () { + const txHex = await buildTxHex({ to: recipientAddress, value: '1000' }); + for (const txParams of [undefined, callerPayment]) { + const { signingStarted, error } = await sign({ txHex, txParams, strict: true }); + signingStarted.should.equal(true); + (error as Error).message.should.match(/signing round started/); + } + }); + + it('never starts signing a redirected payment on the pending-approval path, in either mode', async function () { + const txHex = await buildTxHex({ to: attackerAddress, value: '1000' }); + for (const strict of [false, true]) { + const { signingStarted, error } = await sign({ txHex, strict }); + signingStarted.should.equal(false); + (error as Error).should.be.instanceOf(TxIntentMismatchRecipientError); + (error as Error).message.should.match(/destination address does not match/); + } + }); + + it('never starts signing approve(attacker, MAX) labelled as a payment under strictTssVerification', async function () { + const txHex = await buildTxHex({ to: tokenContractAddress, value: '0', data: approveAttackerMax }); + const { signingStarted, error } = await sign({ txHex, txParams: callerPayment, strict: true }); + signingStarted.should.equal(false); + (error as Error).should.be.instanceOf(TxIntentMismatchRecipientError); + (error as Error).message.should.match(/not a recognized payment shape/); + }); + + it('lets the same bytes reach signing in default mode, as the warn-first rollout agreed', async function () { + // Pins today's policy so a change to it is deliberate: this flips once strict becomes the default. + const txHex = await buildTxHex({ to: tokenContractAddress, value: '0', data: approveAttackerMax }); + const { signingStarted } = await sign({ txHex, txParams: callerPayment }); + signingStarted.should.equal(true); + }); +}); diff --git a/modules/sdk-coin-evm/test/unit/tssVerification.ts b/modules/sdk-coin-evm/test/unit/tssVerification.ts new file mode 100644 index 00000000000..06e89649434 --- /dev/null +++ b/modules/sdk-coin-evm/test/unit/tssVerification.ts @@ -0,0 +1,913 @@ +import 'should'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; +import { BaseCoin as StaticsBaseCoin, coins, ContractAddressDefinedToken, Erc7984Coin } from '@bitgo/statics'; +import { + PopulatedIntent, + resolveEffectiveTxParams, + TransactionParams, + TransactionType, + TssVerificationWarning, + TxRequest, + Wallet, +} from '@bitgo/sdk-core'; +import { optionalDeps } from '@bitgo/abstract-eth'; + +import { EvmCoin } from '../../src/evmCoin'; +import { TransactionBuilder } from '../../src/lib'; + +/** + * Fail-closed EVM TSS verification (TOB-BITGOEDMPC-2 / INFOSEC-671 / CECHO-2460). + * + * The user's key share must not sign EVM TSS bytes unless the decoded to / value / data match + * the intent. Checks that threw before this change keep throwing (including on the + * pending-approval path); every new check flags the violation (warn by default, throw under + * `strictTssVerification`). + * + * Runs on kavaevm (legacy, non-EIP-1559 — the chains that used to bypass the verifier through + * `EvmCoin.verifyLegacyTssTransaction`) and hbarevm (EIP-1559). Batcher-contract cases + * (native/token batch sends, tokenApproval spender) need a chain with a statics batcher + * address and live in @bitgo/sdk-coin-bsc's tssBatcherVerification tests. + */ + +const recipientAddress = '0x174cfd823af8ce27ed0afee3fcf3c3ba259116be'; +const attackerAddress = '0x7e85bdc27c050e3905ebf4b8e634d9ad6edd0de6'; +const baseAddress = '0x1111111111111111111111111111111111111111'; +const tokenContractAddress = '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48'; +const MAX_UINT256 = ((1n << 256n) - 1n).toString(); + +const erc7984Pair = coins.get('hteth:cusdt') as Erc7984Coin; +const erc7984WrapperAddress = String(erc7984Pair.contractAddress); + +// Two distinct registered tokens whose contracts resolve locally from statics. The caller +// and the stored intent name different tokens to prove the server cannot replace local identity. +const tokenContractOf = (tokenName: string): string => { + const token = coins.get(tokenName); + if (!(token instanceof ContractAddressDefinedToken)) { + throw new Error(`${tokenName} has no contract address in statics`); + } + return String(token.contractAddress); +}; +const localTokenName = 'hteth:tusdc'; +const serverTokenName = 'hteth:tusdt'; +const localTokenContractAddress = tokenContractOf(localTokenName); +const serverTokenContractAddress = tokenContractOf(serverTokenName); + +const word = (value: bigint): string => value.toString(16).padStart(64, '0'); +const addrWord = (address: string): string => address.toLowerCase().replace(/^0x/, '').padStart(64, '0'); + +function encodeErc20Transfer(to: string, amount: string): string { + return `0xa9059cbb${addrWord(to)}${word(BigInt(amount))}`; +} + +function encodeApprove(spender: string, amount: string): string { + return `0x095ea7b3${addrWord(spender)}${word(BigInt(amount))}`; +} + +function encodeWithAbi(signature: string, types: string[], values: unknown[]): string { + const selector = optionalDeps.ethAbi.methodID(signature, types); + const args = optionalDeps.ethAbi.rawEncode(types, values); + return '0x' + Buffer.concat([selector, args]).toString('hex'); +} + +function encodeConfidentialTransferNoProof(to: string): string { + return encodeWithAbi('confidentialTransfer', ['address', 'bytes32'], [to, '0x' + '11'.repeat(32)]); +} + +async function buildTxHex( + coin: Readonly, + opts: { to: string; value?: string; data?: string } +): Promise { + const builder = new TransactionBuilder(coin); + builder.type(opts.data && opts.data !== '0x' ? TransactionType.ContractCall : TransactionType.SingleSigSend); + builder.fee({ fee: '10', gasLimit: '21000' }); + builder.counter(1); + builder.contract(opts.to); + if (opts.data) { + builder.data(opts.data); + } + if (opts.value) { + builder.value(opts.value); + } + return (await builder.build()).toBroadcastFormat(); +} + +// kavaevm is legacy (non-EIP-1559) and hbarevm is EIP-1559; neither has a statics batcher +// contract, so the "statics gap" paths are what both exercise for batcher-dependent rules. +['kavaevm', 'hbarevm'].forEach((chain) => { + describe(`EVM TSS fail-closed verification (${chain})`, function () { + const staticsCoin = coins.get(chain); + + let bitgo: TestBitGoAPI; + let strictBitgo: TestBitGoAPI; + let warnings: TssVerificationWarning[]; + let coin: EvmCoin; + let wallet: Wallet; + + beforeEach(function () { + warnings = []; + bitgo = TestBitGo.decorate(BitGoAPI, { + env: 'test', + onVerificationWarning: (warning) => warnings.push(warning), + }); + strictBitgo = TestBitGo.decorate(BitGoAPI, { + env: 'test', + strictTssVerification: true, + }); + coin = new (EvmCoin as any)(bitgo, staticsCoin); + wallet = new Wallet(bitgo, coin as any, { coinSpecific: { baseAddress } } as any); + }); + + // Caller params may also carry the EVM-only tokenName that EthTransactionParams adds. + type SigningPathCallerParams = TransactionParams & { tokenName?: string }; + + const verify = ( + txParams: Record, + txHex: string, + opts: { + strict?: boolean; + intent?: PopulatedIntent; + callerTxParams?: SigningPathCallerParams; + verification?: Record; + } = {} + ): Promise => { + const target = opts.strict ? new (EvmCoin as any)(strictBitgo, staticsCoin) : coin; + const w = opts.strict ? new Wallet(strictBitgo, target as any, { coinSpecific: { baseAddress } } as any) : wallet; + return (target as any).verifyTransaction({ + txParams, + txPrebuild: { txHex, coin: chain, walletId: 'w' }, + wallet: w, + verification: opts.verification ?? {}, + walletType: 'tss', + ...(opts.intent ? { intent: opts.intent } : {}), + ...(opts.callerTxParams ? { callerTxParams: opts.callerTxParams } : {}), + }); + }; + + // Mirrors the ECDSA / MPCv2 signing paths: resolveEffectiveTxParams back-fills the stored + // intent into the effective txParams, while callerTxParams stays the untouched caller + // input. This is the wiring that decides which source speaks for the user intent. + const verifySigningPath = ( + callerTxParams: SigningPathCallerParams | undefined, + intent: Record, + txHex: string, + strict = false + ): Promise => { + const txRequest = { txRequestId: 'signing-path', walletId: 'w', intent } as unknown as TxRequest; + const effective = resolveEffectiveTxParams(txRequest, callerTxParams, chain); + return verify(effective as unknown as Record, txHex, { + strict, + intent: intent as unknown as PopulatedIntent, + ...(callerTxParams ? { callerTxParams } : {}), + }); + }; + + const build = (opts: { to: string; value?: string; data?: string }): Promise => + buildTxHex(staticsCoin, opts); + + describe('payment handler (transfer)', function () { + it('verifies a native send that matches the intent, with no warnings', async function () { + const txHex = await build({ to: recipientAddress, value: '1000' }); + const result = await verify( + { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, + txHex + ); + result.should.equal(true); + warnings.should.have.length(0); + }); + + it('throws on a redirected destination (existing check, both modes)', async function () { + const txHex = await build({ to: attackerAddress, value: '1000' }); + for (const strict of [false, true]) { + await verify({ type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, txHex, { + strict, + }).should.be.rejectedWith(/destination address does not match/); + } + }); + + it('throws on an inflated value (existing check, both modes)', async function () { + const txHex = await build({ to: recipientAddress, value: '9999' }); + for (const strict of [false, true]) { + await verify({ type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, txHex, { + strict, + }).should.be.rejectedWith(/amount in txPrebuild does not match/); + } + }); + + it('verifies an exact dApp call (to, value and data equal to the intent)', async function () { + const data = '0xdeadbeef'; + const txHex = await build({ to: tokenContractAddress, value: '0', data }); + const result = await verify( + { type: 'transfer', recipients: [{ address: tokenContractAddress, amount: '0', data }] }, + txHex + ); + result.should.equal(true); + warnings.should.have.length(0); + }); + + it('flags swapped calldata under a payment intent (warn, throw in strict mode)', async function () { + // ERC-20 calldata with a swapped inner recipient: the existing inner check throws. + const intentData = encodeErc20Transfer(recipientAddress, '1000'); + const tamperedData = encodeErc20Transfer(attackerAddress, '1000'); + const tamperedHex = await build({ to: tokenContractAddress, value: '0', data: tamperedData }); + await verify( + { + type: 'transfer', + recipients: [{ address: tokenContractAddress, amount: '0', data: intentData }], + }, + tamperedHex + ).should.be.rejectedWith(/destination address does not match/); + + // Non-ERC-20 dApp calldata mismatch is a new check: warn, or throw in strict mode. + const swappedHex = await build({ to: tokenContractAddress, value: '0', data: '0xdeadbeef' }); + const txParams = { + type: 'transfer', + recipients: [{ address: tokenContractAddress, amount: '0', data: '0xdeadbe00' }], + }; + const result = await verify(txParams, swappedHex); + result.should.equal(true); + warnings.should.have.length(1); + warnings[0].reason.should.match(/calldata does not match/); + warnings[0].decoded.data.should.equal('0xdeadbeef'); + warnings[0].expected.data.should.equal('0xdeadbe00'); + await verify(txParams, swappedHex, { strict: true }).should.be.rejectedWith(/calldata does not match/); + }); + + it('flags a mismatched destination under an exact-call (dApp) intent', async function () { + const data = '0xdeadbeef'; + const txHex = await build({ to: attackerAddress, value: '0', data }); + const txParams = { + type: 'transfer', + recipients: [{ address: tokenContractAddress, amount: '0', data }], + }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /to does not match/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/to does not match/); + }); + + it('flags a mismatched value under an exact-call (dApp) intent', async function () { + const data = '0xdeadbeef'; + const txHex = await build({ to: tokenContractAddress, value: '7', data }); + const txParams = { + type: 'transfer', + recipients: [{ address: tokenContractAddress, amount: '0', data }], + }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /value does not match/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/value does not match/); + }); + + it('flags approve(attacker, MAX) disguised as a transfer (TOB-BITGOEDMPC-2)', async function () { + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeApprove(attackerAddress, MAX_UINT256), + }); + const txParams = { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /not a recognized payment shape/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/not a recognized payment shape/); + }); + + it('flags missing recipients under a payment intent', async function () { + const txHex = await build({ to: recipientAddress, value: '1000' }); + const txParams = { type: 'transfer', recipients: [] }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /no recipients/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/no recipients/); + }); + }); + + describe('payment handler (token transfers)', function () { + const tokenRecipient = { + address: recipientAddress, + amount: '10000000', + tokenAddress: tokenContractAddress, + }; + + it('verifies an ERC-20 transfer against the intent token contract, value zero', async function () { + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '10000000'), + }); + const result = await verify({ type: 'transfer', recipients: [tokenRecipient] }, txHex); + result.should.equal(true); + warnings.should.have.length(0); + }); + + it('flags an ERC-20 transfer whose to is not the intent token contract', async function () { + const txHex = await build({ + to: attackerAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '10000000'), + }); + const txParams = { type: 'transfer', recipients: [tokenRecipient] }; + (await verify(txParams, txHex)).should.equal(true); + const flag = warnings.find((w) => /not the intent token contract/.test(w.reason)); + should.exist(flag); + flag?.expected.to?.toLowerCase().should.equal(tokenContractAddress.toLowerCase()); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/not the intent token contract/); + }); + + it('flags an ERC-20 transfer that carries native value', async function () { + const txHex = await build({ + to: tokenContractAddress, + value: '5', + data: encodeErc20Transfer(recipientAddress, '10000000'), + }); + const txParams = { type: 'transfer', recipients: [tokenRecipient] }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /must not carry native value/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/must not carry native value/); + }); + + it('throws on a tampered ERC-20 recipient or amount (existing checks)', async function () { + const toAttacker = await build({ + to: tokenContractAddress, + value: '0', + data: encodeErc20Transfer(attackerAddress, '10000000'), + }); + await verify({ type: 'transfer', recipients: [tokenRecipient] }, toAttacker).should.be.rejectedWith( + /destination address does not match/ + ); + const inflated = await build({ + to: tokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '99999999'), + }); + await verify({ type: 'transfer', recipients: [tokenRecipient] }, inflated).should.be.rejectedWith( + /amount in txPrebuild does not match/ + ); + }); + + it('checks the ERC-7984 confidentialTransfer recipient only', async function () { + const intentParams = { + type: 'transfer', + recipients: [{ address: recipientAddress, amount: '0', tokenName: 'hteth:cusdt' }], + }; + const goodHex = await build({ + to: erc7984WrapperAddress, + value: '0', + data: encodeConfidentialTransferNoProof(recipientAddress), + }); + (await verify(intentParams, goodHex)).should.equal(true); + warnings.should.have.length(0); + + const tamperedHex = await build({ + to: erc7984WrapperAddress, + value: '0', + data: encodeConfidentialTransferNoProof(attackerAddress), + }); + (await verify(intentParams, tamperedHex)).should.equal(true); + warnings.some((w) => /confidentialTransfer recipient does not match/.test(w.reason)).should.equal(true); + await verify(intentParams, tamperedHex, { strict: true }).should.be.rejectedWith( + /confidentialTransfer recipient does not match/ + ); + }); + + it('flags batch calldata carried by a single declared recipient', async function () { + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeWithAbi('batch', ['address[]', 'uint256[]'], [[recipientAddress], ['1']]), + }); + const txParams = { type: 'transfer', recipients: [tokenRecipient] }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /single declared recipient/.test(w.reason)).should.equal(true); + }); + }); + + describe('declared intent isolation and asset binding (CECHO-2460)', function () { + const tokenTransferData = encodeErc20Transfer(recipientAddress, '1000'); + const approvalData = encodeApprove(attackerAddress, MAX_UINT256); + + it('does not let stored calldata fill a caller recipient that declared none', async function () { + // The caller declares a native payment and supplies no calldata. The stored intent adds + // approve(attacker, MAX): it must not turn the caller's payment into an exact dApp call. + const caller = { type: 'transfer', recipients: [{ address: tokenContractAddress, amount: '0' }] }; + const intent = { + intentType: 'payment', + recipients: [ + { + address: { address: tokenContractAddress }, + amount: { value: '0', symbol: 'hteth' }, + data: approvalData, + }, + ], + }; + const txHex = await build({ to: tokenContractAddress, value: '0', data: approvalData }); + + (await verifySigningPath(caller, intent, txHex)).should.equal(true); + warnings.some((w) => /not a recognized payment shape/.test(w.reason)).should.equal(true); + await verifySigningPath(caller, intent, txHex, true).should.be.rejectedWith(/not a recognized payment shape/); + }); + + it('keeps the caller token identity when the stored intent names another token', async function () { + const caller = { + type: 'transferToken', + tokenName: localTokenName, + recipients: [{ address: recipientAddress, amount: '1000' }], + }; + const intent = { + intentType: 'transferToken', + tokenName: serverTokenName, + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: serverTokenName } }], + }; + const txHex = await build({ to: serverTokenContractAddress, value: '0', data: tokenTransferData }); + + (await verifySigningPath(caller, intent, txHex)).should.equal(true); + warnings.some((w) => /not the intent token contract/.test(w.reason)).should.equal(true); + await verifySigningPath(caller, intent, txHex, true).should.be.rejectedWith(/not the intent token contract/); + }); + + it('rejects token calldata for a caller-declared native payment', async function () { + const caller = { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }; + const intent = { + intentType: 'payment', + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '1000', symbol: 'hteth' }, + tokenData: { tokenName: localTokenName }, + }, + ], + }; + const txHex = await build({ to: localTokenContractAddress, value: '0', data: tokenTransferData }); + + (await verifySigningPath(caller, intent, txHex)).should.equal(true); + warnings + .some((w) => /native asset but the transaction carries token or batch calldata/.test(w.reason)) + .should.equal(true); + await verifySigningPath(caller, intent, txHex, true).should.be.rejectedWith( + /native asset but the transaction carries token or batch calldata/ + ); + }); + + it('rejects a native send for a caller-declared token payment', async function () { + const caller = { + type: 'transfer', + recipients: [{ address: recipientAddress, amount: '1000', tokenAddress: localTokenContractAddress }], + }; + const intent = { + intentType: 'payment', + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: localTokenName } }], + }; + const txHex = await build({ to: recipientAddress, value: '1000' }); + + (await verifySigningPath(caller, intent, txHex)).should.equal(true); + warnings + .some((w) => /token payment intent but the transaction is a native send/.test(w.reason)) + .should.equal(true); + await verifySigningPath(caller, intent, txHex, true).should.be.rejectedWith( + /token payment intent but the transaction is a native send/ + ); + }); + + it('flags a tokenAddress that does not match the transaction token', async function () { + const caller = { + type: 'transfer', + recipients: [{ address: recipientAddress, amount: '1000', tokenAddress: localTokenContractAddress }], + }; + const txHex = await build({ to: serverTokenContractAddress, value: '0', data: tokenTransferData }); + + (await verifySigningPath(caller, { intentType: 'payment', recipients: [] }, txHex)).should.equal(true); + warnings.some((w) => /not the intent token contract/.test(w.reason)).should.equal(true); + await verifySigningPath(caller, { intentType: 'payment', recipients: [] }, txHex, true).should.be.rejectedWith( + /not the intent token contract/ + ); + }); + + it('flags an unresolvable token name instead of silently skipping the token check', async function () { + const caller = { + type: 'transferToken', + tokenName: 'hteth:not-a-registered-token', + recipients: [{ address: recipientAddress, amount: '1000' }], + }; + const txHex = await build({ to: localTokenContractAddress, value: '0', data: tokenTransferData }); + + (await verifySigningPath(caller, { intentType: 'transferToken', recipients: [] }, txHex)).should.equal(true); + warnings.some((w) => /cannot be resolved to a token contract/.test(w.reason)).should.equal(true); + await verifySigningPath( + caller, + { intentType: 'transferToken', recipients: [] }, + txHex, + true + ).should.be.rejectedWith(/cannot be resolved to a token contract/); + }); + + it('flags conflicting local token identity (tokenAddress and tokenName name two contracts)', async function () { + const caller = { + type: 'transferToken', + tokenName: serverTokenName, + recipients: [{ address: recipientAddress, amount: '1000', tokenAddress: localTokenContractAddress }], + }; + const txHex = await build({ to: localTokenContractAddress, value: '0', data: tokenTransferData }); + + (await verifySigningPath(caller, { intentType: 'transferToken', recipients: [] }, txHex)).should.equal(true); + warnings.some((w) => /name different token contracts/.test(w.reason)).should.equal(true); + await verifySigningPath( + caller, + { intentType: 'transferToken', recipients: [] }, + txHex, + true + ).should.be.rejectedWith(/name different token contracts/); + }); + + it('verifies a legitimate caller payment even when the stored intent contradicts it', async function () { + const caller = { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }; + const intent = { + intentType: 'payment', + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '1000', symbol: 'hteth' }, + data: approvalData, + }, + ], + }; + const txHex = await build({ to: recipientAddress, value: '1000' }); + + (await verifySigningPath(caller, intent, txHex)).should.equal(true); + warnings.should.have.length(0); + (await verifySigningPath(caller, intent, txHex, true)).should.equal(true); + }); + + it('verifies a legitimate caller token payment even when the stored intent names another token', async function () { + const caller = { + type: 'transferToken', + tokenName: localTokenName, + recipients: [{ address: recipientAddress, amount: '1000' }], + }; + const intent = { + intentType: 'transferToken', + tokenName: serverTokenName, + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: serverTokenName } }], + }; + const txHex = await build({ to: localTokenContractAddress, value: '0', data: tokenTransferData }); + + (await verifySigningPath(caller, intent, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('uses the stored token identity when the caller declared nothing (pending approval)', async function () { + const intent = { + intentType: 'transferToken', + tokenName: localTokenName, + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: localTokenName } }], + }; + const txHex = await build({ to: localTokenContractAddress, value: '0', data: tokenTransferData }); + + (await verifySigningPath(undefined, intent, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('fails closed on an unresolvable stored token identity (pending approval)', async function () { + const intent = { + intentType: 'transferToken', + tokenName: 'hteth:not-a-registered-token', + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '1000', symbol: 'hteth:not-a-registered-token' }, + }, + ], + }; + const txHex = await build({ to: localTokenContractAddress, value: '0', data: tokenTransferData }); + + (await verifySigningPath(undefined, intent, txHex)).should.equal(true); + warnings.some((w) => /cannot be resolved to a token contract/.test(w.reason)).should.equal(true); + await verifySigningPath(undefined, intent, txHex, true).should.be.rejectedWith( + /cannot be resolved to a token contract/ + ); + }); + }); + + describe('batch payments without a configured batcher contract (statics gap)', function () { + const recipients = [ + { address: recipientAddress, amount: '10' }, + { address: attackerAddress, amount: '20' }, + ]; + + it('flags native batch calldata instead of silently passing it', async function () { + const txHex = await build({ + to: attackerAddress, + value: '30', + data: encodeWithAbi( + 'batch', + ['address[]', 'uint256[]'], + [ + [recipientAddress, attackerAddress], + ['10', '20'], + ] + ), + }); + const txParams = { type: 'transfer', recipients }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /no batcher contract address is configured/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith( + /no batcher contract address is configured/ + ); + }); + + it('flags token batch calldata instead of silently passing it', async function () { + const txHex = await build({ + to: attackerAddress, + value: '0', + data: encodeWithAbi( + 'batchTransferFrom', + ['address', 'address[]', 'uint256[]'], + [tokenContractAddress, [recipientAddress, attackerAddress], ['10', '20']] + ), + }); + (await verify({ type: 'transfer', recipients }, txHex)).should.equal(true); + warnings.some((w) => /no batcher contract address is configured/.test(w.reason)).should.equal(true); + }); + }); + + describe('tokenApproval handler', function () { + const approvalParams = { type: 'tokenApproval', tokenName: 'hteth:cusdt' }; + + it('flags a tokenApproval when the network has no configured batcher contract (statics gap)', async function () { + const txHex = await build({ + to: erc7984WrapperAddress, + value: '0', + data: encodeApprove(attackerAddress, MAX_UINT256), + }); + (await verify(approvalParams, txHex)).should.equal(true); + warnings.some((w) => /no batcher contract address is configured/.test(w.reason)).should.equal(true); + }); + + it('flags a non-maximum allowance, non-approve calldata, native value and a wrong to', async function () { + const smallAllowance = await build({ + to: erc7984WrapperAddress, + value: '0', + data: encodeApprove(attackerAddress, '5'), + }); + (await verify(approvalParams, smallAllowance)).should.equal(true); + warnings.some((w) => /not the maximum ERC-20 allowance/.test(w.reason)).should.equal(true); + + warnings = []; + const notApprove = await build({ + to: erc7984WrapperAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '5'), + }); + (await verify(approvalParams, notApprove)).should.equal(true); + warnings.some((w) => /must use ERC-20 approve/.test(w.reason)).should.equal(true); + + warnings = []; + const carriesValue = await build({ + to: erc7984WrapperAddress, + value: '1', + data: encodeApprove(attackerAddress, MAX_UINT256), + }); + (await verify(approvalParams, carriesValue)).should.equal(true); + warnings.some((w) => /must not carry native value/.test(w.reason)).should.equal(true); + + warnings = []; + const wrongTo = await build({ + to: attackerAddress, + value: '0', + data: encodeApprove(attackerAddress, MAX_UINT256), + }); + (await verify(approvalParams, wrongTo)).should.equal(true); + warnings.some((w) => /to is not the intent token contract/.test(w.reason)).should.equal(true); + }); + }); + + describe('fillNonce handler', function () { + it('verifies a 1-wei self-send to the base address and flags deviations', async function () { + const good = await build({ to: baseAddress, value: '1' }); + (await verify({ type: 'fillNonce' }, good)).should.equal(true); + warnings.should.have.length(0); + + const wrongTo = await build({ to: recipientAddress, value: '1' }); + (await verify({ type: 'fillNonce' }, wrongTo)).should.equal(true); + warnings.some((w) => /must send to the wallet base address/.test(w.reason)).should.equal(true); + + warnings = []; + const wrongValue = await build({ to: baseAddress, value: '2' }); + (await verify({ type: 'fillNonce' }, wrongValue)).should.equal(true); + warnings.some((w) => /must send exactly 1 wei/.test(w.reason)).should.equal(true); + + warnings = []; + const withCalldata = await build({ to: baseAddress, value: '1', data: '0xdeadbeef' }); + (await verify({ type: 'fillNonce' }, withCalldata)).should.equal(true); + warnings.some((w) => /must not carry calldata/.test(w.reason)).should.equal(true); + + await verify({ type: 'fillNonce' }, wrongTo, { strict: true }).should.be.rejectedWith( + /must send to the wallet base address/ + ); + }); + }); + + describe('consolidation to the wallet base address', function () { + const consolidate = { type: 'consolidate', recipients: [] }; + const toBase = { consolidationToBaseAddress: true }; + + it('verifies a token consolidation that pays the base address and carries no native value', async function () { + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeErc20Transfer(baseAddress, '5'), + }); + (await verify(consolidate, txHex, { verification: toBase })).should.equal(true); + (await verify(consolidate, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('rejects native value riding along token-shaped calldata when the caller asked for the check', async function () { + // transfer(baseAddress, 0) passes the recipient check, so without the value check the + // native balance would go to whatever contract `to` names. + const txHex = await build({ + to: attackerAddress, + value: '5000', + data: encodeErc20Transfer(baseAddress, '0'), + }); + for (const strict of [false, true]) { + await verify(consolidate, txHex, { strict, verification: toBase }).should.be.rejectedWith( + /must not carry native value/ + ); + } + }); + + it('flags the same transaction on a consolidate intent (warn, throw in strict mode)', async function () { + const txHex = await build({ + to: attackerAddress, + value: '5000', + data: encodeErc20Transfer(baseAddress, '0'), + }); + (await verify(consolidate, txHex)).should.equal(true); + warnings.some((w) => /must not carry native value/.test(w.reason)).should.equal(true); + await verify(consolidate, txHex, { strict: true }).should.be.rejectedWith(/must not carry native value/); + }); + }); + + describe('unsupported and unhandled intent types', function () { + it('rejects contractCall and customTx outright on EVM (INFOSEC-671)', async function () { + const txHex = await build({ to: attackerAddress, value: '0', data: '0xdeadbeef' }); + for (const type of ['contractCall', 'customTx']) { + for (const strict of [false, true]) { + await verify({ type, recipients: [{ address: attackerAddress, amount: '0' }] }, txHex, { + strict, + }).should.be.rejectedWith(/unsupported intent type for EVM TSS/); + } + } + }); + + it('flags every intent type without a handler instead of silently passing', async function () { + const txHex = await build({ to: attackerAddress, value: '0', data: '0xdeadbeef' }); + for (const type of ['acceleration', 'bridgeFunds', 'enableToken', 'defiRedeem', 'stakeWithCallData']) { + warnings = []; + const result = await verify({ type, recipients: [] }, txHex); + result.should.equal(true, `${type} must not throw`); + warnings.some((w) => /no fail-closed verifier/.test(w.reason)).should.equal(true, `${type} must be flagged`); + } + await verify({ type: 'bridgeFunds', recipients: [] }, txHex, { strict: true }).should.be.rejectedWith( + /no fail-closed verifier/ + ); + }); + }); + + describe('pending-approval path (stored intent via resolveEffectiveTxParams)', function () { + const verifyPendingApproval = async ( + intent: Record, + txHex: string, + strict = false + ): Promise => { + const txRequest = { txRequestId: 'pa', walletId: 'w', intent } as unknown as TxRequest; + const txParams = resolveEffectiveTxParams(txRequest, undefined, chain); + return verify(txParams as unknown as Record, txHex, { + strict, + intent: txRequest.intent as PopulatedIntent, + }); + }; + + it('keeps the existing native checks throwing on this path (redirect and inflated value)', async function () { + const intent = { + intentType: 'payment', + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: 'eth' } }], + }; + const redirectHex = await build({ to: attackerAddress, value: '1000' }); + await verifyPendingApproval(intent, redirectHex).should.be.rejectedWith(/destination address does not match/); + + const inflatedHex = await build({ to: recipientAddress, value: '9999' }); + await verifyPendingApproval(intent, inflatedHex).should.be.rejectedWith(/amount in txPrebuild does not match/); + }); + + it('flags swapped calldata on this path (warn, throw in strict mode)', async function () { + const data = '0xdeadbeef'; + const intent = { + intentType: 'payment', + recipients: [{ address: { address: tokenContractAddress }, amount: { value: '0', symbol: 'eth' }, data }], + }; + const txHex = await build({ to: tokenContractAddress, value: '0', data: '0xdeadbe00' }); + (await verifyPendingApproval(intent, txHex)).should.equal(true); + warnings.some((w) => /calldata does not match/.test(w.reason)).should.equal(true); + await verifyPendingApproval(intent, txHex, true).should.be.rejectedWith(/calldata does not match/); + }); + + it('verifies a matching payment intent on this path with no warnings', async function () { + const data = '0xdeadbeef'; + const intent = { + intentType: 'payment', + recipients: [{ address: { address: tokenContractAddress }, amount: { value: '0', symbol: 'eth' }, data }], + }; + const txHex = await build({ to: tokenContractAddress, value: '0', data }); + (await verifyPendingApproval(intent, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('verifies stored defi-* intents without InvalidTransactionError and without a silent skip', async function () { + const defiCases: Array<{ intentType: string; data: string; tampered: string; amountField: string }> = [ + { + intentType: 'defi-approve', + data: encodeApprove(attackerAddress, '123'), + tampered: encodeApprove(attackerAddress, '999'), + amountField: 'amount', + }, + { + intentType: 'defi-deposit', + data: encodeWithAbi('deposit', ['uint256', 'address'], ['123', baseAddress]), + tampered: encodeWithAbi('deposit', ['uint256', 'address'], ['999', baseAddress]), + amountField: 'amount', + }, + { + intentType: 'defi-withdraw', + data: encodeWithAbi('redeem', ['uint256', 'address', 'address'], ['123', baseAddress, baseAddress]), + tampered: encodeWithAbi('redeem', ['uint256', 'address', 'address'], ['999', baseAddress, baseAddress]), + amountField: 'shareTokenAmount', + }, + ]; + for (const { intentType, data, tampered, amountField } of defiCases) { + warnings = []; + const intent: Record = { intentType, vaultId: 'vault', [amountField]: '123' }; + const txHex = await build({ to: tokenContractAddress, value: '0', data }); + const result = await verifyPendingApproval(intent, txHex); + result.should.equal(true, `${intentType} must verify`); + + // Not a silent skip: tampered amounts are rejected by the DeFi calldata checks. + const tamperedHex = await build({ to: tokenContractAddress, value: '0', data: tampered }); + await verifyPendingApproval(intent, tamperedHex).should.be.rejectedWith( + /does not match the requested amount|do not match the requested amount/ + ); + } + }); + + it('keeps rejecting stored defi-redeem intents, which have no handler, as master does', async function () { + const intent = { intentType: 'defi-redeem', vaultId: 'v', amount: '1' }; + const txHex = await build({ to: tokenContractAddress, value: '0', data: '0xdeadbeef' }); + for (const strict of [false, true]) { + await verifyPendingApproval(intent, txHex, strict).should.be.rejectedWith(/Recipient details are required/); + } + warnings.should.have.length(0); + }); + }); + + describe('legacy (non-EIP-1559) chains run the parent verifier', function () { + it('rejects contractCall outright instead of returning true', async function () { + const txHex = await build({ to: attackerAddress, value: '0', data: '0xdeadbeef' }); + await verify( + { type: 'contractCall', recipients: [{ address: attackerAddress, amount: '0' }] }, + txHex + ).should.be.rejectedWith(/unsupported intent type for EVM TSS/); + }); + + it('flags a payment with unrecognized calldata instead of returning true', async function () { + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeApprove(attackerAddress, MAX_UINT256), + }); + const txParams = { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /not a recognized payment shape/.test(w.reason)).should.equal(true); + }); + }); + + describe('warning infrastructure', function () { + it('reports decoded vs expected to / value / data through onVerificationWarning', async function () { + const data = '0xdeadbeef'; + const txHex = await build({ to: attackerAddress, value: '7', data: '0xdeadbe00' }); + await verify({ type: 'transfer', recipients: [{ address: tokenContractAddress, amount: '0', data }] }, txHex); + warnings.length.should.be.greaterThanOrEqual(3); + const destinationWarning = warnings.find((w) => /to does not match/.test(w.reason)); + destinationWarning?.expected.to?.toLowerCase().should.equal(tokenContractAddress.toLowerCase()); + destinationWarning?.decoded.to?.toLowerCase().should.equal(attackerAddress.toLowerCase()); + const valueWarning = warnings.find((w) => /value does not match/.test(w.reason)); + valueWarning?.expected.value?.should.equal('0'); + valueWarning?.decoded.value?.should.equal('7'); + const dataWarning = warnings.find((w) => /calldata does not match/.test(w.reason)); + dataWarning?.expected.data?.should.equal('0xdeadbeef'); + dataWarning?.decoded.data?.should.equal('0xdeadbe00'); + }); + + it('does not fire the callback when the transaction matches the intent', async function () { + const txHex = await build({ to: recipientAddress, value: '1000' }); + await verify({ type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, txHex); + warnings.should.have.length(0); + }); + }); + }); +}); diff --git a/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts b/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts index cb4ec7f61c7..a3423db3f1d 100644 --- a/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts +++ b/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts @@ -315,6 +315,35 @@ export interface VerificationOptions { consolidationToBaseAddress?: boolean; } +/** + * Decoded vs expected transaction fields reported by fail-closed TSS verification. + * Undefined fields were not applicable to the violated rule. + */ +export interface TssVerificationMismatch { + to?: string; + value?: string; + data?: string; +} + +/** + * A non-fatal TSS verification finding: the transaction bytes about to be signed do not + * match the user intent. Reported through `console.warn` and an optional + * `onVerificationWarning` callback; becomes a hard failure when the client opts into + * `strictTssVerification`. + */ +export interface TssVerificationWarning { + /** Canonical verifier intent type the handler ran under (e.g. 'transfer'). */ + intentType: string; + /** Human-readable description of the violated rule. */ + reason: string; + /** Values derived from the user intent (txParams or the stored tx request intent). */ + expected: TssVerificationMismatch; + /** Values decoded from the transaction bytes that are about to be signed. */ + decoded: TssVerificationMismatch; +} + +export type TssVerificationWarningHandler = (warning: TssVerificationWarning) => void; + export interface VerifyTransactionOptions { txPrebuild: TransactionPrebuild; txParams: TransactionParams; @@ -322,6 +351,26 @@ export interface VerifyTransactionOptions { verification?: VerificationOptions; reqId?: IRequestTracer; walletType?: 'onchain' | 'tss'; + /** + * The persisted TxRequest intent (`PopulatedIntent`), set by the TSS signing path when + * verifying a pending-approval transaction. Verifiers use it as the source of truth for + * the intended `to` / `value` / `data` and for intent fields with no txParams equivalent + * (`tokenName`, `nonce`, `txid`, `consolidateId`, DeFi amounts). Never server-controlled + * beyond the tx request itself, which is the object the user approved. + */ + intent?: PopulatedIntent; + /** + * The `txParams` the caller of the signing API supplied, before `resolveEffectiveTxParams` + * back-filled `type` and `recipients` from the stored tx request intent. `undefined` when the + * caller supplied none (pending-approval signing of a stored tx request). + * + * `txParams` cannot answer "did the caller declare this?": the stored intent has already been + * merged into it. Verifiers that accept a stored `intent` use this to keep the caller's + * declared intent and the stored intent apart: a caller that declared `type` or `recipients` + * is the only source of truth, and the stored intent must not select the handler, add + * calldata, or name the asset. + */ + callerTxParams?: TransactionParams; } export interface SupplementGenerateWalletOptions { diff --git a/modules/sdk-core/src/bitgo/bitgoBase.ts b/modules/sdk-core/src/bitgo/bitgoBase.ts index 698e4846fd4..5cca2f1fb0a 100644 --- a/modules/sdk-core/src/bitgo/bitgoBase.ts +++ b/modules/sdk-core/src/bitgo/bitgoBase.ts @@ -8,7 +8,7 @@ import { IEncryptionSession, IRequestTracer, } from '../api'; -import { IBaseCoin } from './baseCoin'; +import { IBaseCoin, TssVerificationWarningHandler } from './baseCoin'; import { CoinConstructor } from './coinFactory'; import { EnvironmentName } from './environments'; import { EcdhDerivedKeypair, GetSigningKeyApi } from './keychain'; @@ -61,4 +61,14 @@ export interface BitGoBase { setRequestTracer(reqTracer: IRequestTracer): void; url(path: string, version?: number): string; register(name: string, coin: CoinConstructor): void; + /** + * Client-side opt-in: fail-closed TSS verification warnings become hard failures. + * Set only from local `BitGoAPI` options — never read from server data. + */ + readonly strictTssVerification?: boolean; + /** + * Optional sink for fail-closed TSS verification warnings (decoded vs expected + * transaction fields). Set only from local `BitGoAPI` options. + */ + readonly onVerificationWarning?: TssVerificationWarningHandler; } diff --git a/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts b/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts index 0473a7ee397..d0649c6f23e 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts @@ -503,6 +503,8 @@ export interface PopulatedIntent extends PopulatedIntentBase, DefiIntentFields { feeToken?: string; hopParams?: HopParams; txid?: string; + /** Consolidation batch id; presence marks the tx as a consolidation to the wallet base address. */ + consolidateId?: string; receiveAddress?: string; custodianTransactionId?: string; custodianMessageId?: string; diff --git a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts index fece8b88d0c..b919b44ecbd 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts @@ -25,6 +25,7 @@ import { CustomMuDeltaShareGeneratingFunction, CustomPaillierModulusGetterFunction, CustomSShareGeneratingFunction, + PopulatedIntent, RequestType, TSSParams, TSSParamsForMessage, @@ -832,16 +833,20 @@ export class EcdsaUtils extends BaseEcdsaUtils { if (shouldVerifyWithSerializedTxHex(this.baseCoin)) { 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(), + intent: txRequest.intent as PopulatedIntent, + callerTxParams: params.txParams, }); } 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(), + intent: txRequest.intent as PopulatedIntent, + callerTxParams: params.txParams, }); } signablePayload = Buffer.from(unsignedTx.signableHex, 'hex'); 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..1064182bd6f 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts @@ -39,6 +39,7 @@ import { CustomMPCv2SigningRound1GeneratingFunction, CustomMPCv2SigningRound2GeneratingFunction, CustomMPCv2SigningRound3GeneratingFunction, + PopulatedIntent, RequestType, SignatureShareRecord, TSSParams, @@ -991,9 +992,11 @@ 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(), + intent: txRequest.intent as PopulatedIntent, + callerTxParams: params.txParams, }); if (shouldVerifyWithSerializedTxHex(this.baseCoin) && isCoinWithSignableConsistency(this.baseCoin)) { this.baseCoin.assertSignableConsistency(unsignedTx.serializedTxHex, unsignedTx.signableHex); @@ -1001,9 +1004,11 @@ 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(), + intent: txRequest.intent as PopulatedIntent, + callerTxParams: params.txParams, }); } txOrMessageToSign = unsignedTx.signableHex; diff --git a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts index f33d44b9b05..9cdc6edf0ce 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts @@ -29,10 +29,20 @@ export const NO_RECIPIENT_TX_TYPES = new Set([ 'disableToken', 'disabletoken', 'customTx', - // DeFi vault operations — recipients/calldata built server-side from defiParams + // DeFi vault operations — recipients/calldata built server-side from defiParams. + // Both spellings registered on purpose (see the wrapNative note below): the stored + // WP intentType is kebab-case while buildParams.type is camelCase. These types are + // NOT skipped by verifyTssTransaction — the DeFi handlers decode and compare the + // calldata against defiParams; this entry only waives the missing-recipients guard. 'defiApprove', + 'defi-approve', 'defiDeposit', + 'defi-deposit', 'defiWithdraw', + 'defi-withdraw', + // defi-redeem is deliberately absent: it has no EVM verifier handler, so a stored + // defi-redeem intent keeps failing the missing-recipients guard instead of reaching + // signing with only a warning. // Native wrap/unwrap (WETH9 deposit()/withdraw()) — calldata and the WETH9 // address are resolved server-side from the vault binding, so no recipients. // Registered in BOTH spellings on purpose: this set is matched against 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..b262f970e06 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 @@ -19,6 +19,7 @@ import { EcdsaMPCv2Utils, EcdsaMPCv2KeyGenCallbacks, IBaseCoin, + IRequestTracer, IWallet, RequestTracer, SignatureShareRecord, @@ -26,6 +27,7 @@ import { TxRequest, } from '../../../../../../src'; import { bitgoGpgKey } from './gpgKeys'; +import { readKey } from 'openpgp'; describe('ECDSA MPC v2', async () => { let userShare: Buffer; @@ -636,6 +638,7 @@ describe('ECDSA MPC v2', async () => { verifyTransaction: sinon.stub().resolves(true), getMPCAlgorithm: sinon.stub().returns('ecdsa'), getConfig: sinon.stub().returns({ family: 'flrp' }), + getChain: sinon.stub().returns('flrp'), isSignablePreHashed: (unsignedTx: { serializedTxHex?: string }) => unsignedTx.serializedTxHex?.startsWith('0000') ?? false, } as unknown as IBaseCoin; @@ -707,6 +710,7 @@ describe('ECDSA MPC v2', async () => { verifyTransaction: sinon.stub().resolves(true), getMPCAlgorithm: sinon.stub().returns('ecdsa'), getConfig: sinon.stub().returns({ family: 'flr' }), + getChain: sinon.stub().returns('flr'), } as unknown as IBaseCoin; const mockWallet = { @@ -781,6 +785,7 @@ describe('ECDSA MPC v2', async () => { verifyTransaction: verifyTransactionSpy, getMPCAlgorithm: sinon.stub().returns('ecdsa'), getConfig: sinon.stub().returns({ family: 'hteth' }), + getChain: sinon.stub().returns('hteth'), } as unknown as IBaseCoin; const mockWallet = { @@ -827,7 +832,94 @@ describe('ECDSA MPC v2', async () => { const verifyCallArgs = verifyTransactionSpy.firstCall.args[0]; assert.strictEqual(verifyCallArgs.txParams.type, intentType); assert.strictEqual(verifyCallArgs.txParams.recipients, undefined); + // The stored intent must reach the verifier so fail-closed EVM checks can compare + // decoded to / value / data against it on the pending-approval path. + assert.strictEqual( + verifyCallArgs.intent, + (txRequest as any).intent, + 'stored intent must reach verifyTransaction' + ); + assert.strictEqual( + verifyCallArgs.callerTxParams, + undefined, + 'pending approval supplies no caller txParams; the stored intent is the only source' + ); + }); + }); + + it('signRequestBase should pass the caller txParams and the stored intent to verifyTransaction', async function () { + const serializedTxHex = 'f86c808504a817c80082520894' + '00'.repeat(20) + '80808080'; + const callerTxParams = { + type: 'transfer', + recipients: [{ address: '0x' + '11'.repeat(20), amount: '1000' }], + }; + const intent = { + intentType: 'payment', + recipients: [{ address: { address: '0x' + '11'.repeat(20) }, amount: { value: '1000', symbol: 'hteth' } }], + }; + + const mockBgWithPost = {} as BitGoBase; + mockBgWithPost.getEnv = sinon.stub().returns('test'); + mockBgWithPost.setRequestTracer = sinon.stub(); + mockBgWithPost.encrypt = sinon.stub().resolves('encrypted'); + mockBgWithPost.decrypt = sinon.stub().resolves('decrypted'); + mockBgWithPost.post = sinon.stub().returns({ + send: sinon.stub().returnsThis(), + set: sinon.stub().returnsThis(), + result: sinon.stub().rejects(new Error('mock: HTTP not available')), }); + + const verifyTransactionSpy = sinon.stub().resolves(true); + const mockCoinForDirectSend = { + getHashFunction: sinon.stub().callsFake(() => createKeccakHash('keccak256') as Hash), + verifyTransaction: verifyTransactionSpy, + getMPCAlgorithm: sinon.stub().returns('ecdsa'), + getConfig: sinon.stub().returns({ family: 'hteth' }), + getChain: sinon.stub().returns('hteth'), + } as unknown as IBaseCoin; + + const mockWallet = { + id: sinon.stub().returns(walletID), + multisigType: sinon.stub().returns('tss'), + multisigTypeVersion: sinon.stub().returns('MPCv2'), + }; + + const directUtils = new EcdsaMPCv2Utils(mockBgWithPost, mockCoinForDirectSend, mockWallet as unknown as IWallet); + const bitgoPubKey = await readKey({ armoredKey: bitgoGpgKey.public }); + sinon.stub(directUtils, 'pickBitgoPubGpgKeyForSigning').resolves(bitgoPubKey); + + const txRequest = { + txRequestId: 'direct-send-test', + apiVersion: 'full', + walletId: walletID, + intent, + transactions: [ + { + unsignedTx: { derivationPath: 'm/0', signableHex: serializedTxHex, serializedTxHex }, + signatureShares: [], + }, + ], + } as unknown as TxRequest; + + try { + await directUtils.signTxRequest({ + txRequest, + txParams: callerTxParams, + prv: userShare.toString('base64'), + reqId: { inc: sinon.stub(), toString: sinon.stub().returns('test-req') } as unknown as IRequestTracer, + }); + } catch (e) {} + + assert.strictEqual(verifyTransactionSpy.callCount, 1, 'verifyTransaction must be reached exactly once'); + const verifyCallArgs = verifyTransactionSpy.firstCall.args[0]; + assert.strictEqual(verifyCallArgs.intent, intent, 'stored intent must reach verifyTransaction'); + assert.deepStrictEqual( + verifyCallArgs.callerTxParams, + callerTxParams, + 'the untouched caller input must reach the verifier' + ); + assert.strictEqual(verifyCallArgs.txParams.type, 'transfer'); + assert.deepStrictEqual(verifyCallArgs.txParams.recipients, callerTxParams.recipients); }); it('should still apply keccak256 for regular FLR EVM transactions', async () => { diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaVerifyTransactionWiring.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaVerifyTransactionWiring.ts new file mode 100644 index 00000000000..2156e3e808c --- /dev/null +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaVerifyTransactionWiring.ts @@ -0,0 +1,132 @@ +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import { + BitGoBase, + EcdsaUtils, + IBaseCoin, + IRequestTracer, + IWallet, + TssSignTxRequestParamsWithPrv, + TxRequest, +} from '../../../../../../src'; + +/** + * CECHO-2460: the EVM TSS verifier must receive both the caller's own txParams and the stored + * tx request intent. `txParams` is the post-`resolveEffectiveTxParams` object (the stored intent + * is back-filled into it), so it cannot tell the verifier whether the caller declared anything; + * `callerTxParams` is the untouched caller input that decides which source is authoritative. + * + * This pins the ECDSA (MPCv1) wiring. EcdsaMPCv2Utils has the equivalent test in ecdsaMPCv2.ts. + */ +describe('EcdsaUtils signing verifyTransaction wiring', function () { + const walletID = '62fe536a6b4cf70007acb48c0e7bb0b0'; + const derivationPath = 'm/0'; + const signableHex = 'f86c808504a817c80082520894' + '00'.repeat(20) + '80808080'; + + const makeTxRequest = (intent: Record): TxRequest => + ({ + txRequestId: 'ecdsa-wiring', + apiVersion: 'full', + walletId: walletID, + intent, + transactions: [ + { + unsignedTx: { derivationPath, signableHex, serializedTxHex: signableHex }, + signatureShares: [], + }, + ], + } as unknown as TxRequest); + + // Minimal signing material: signRequestBase only needs pShare.i === 1, a backupNShare and + // the paillier modulus before it hands the unsigned bytes to verifyTransaction. + const prv = JSON.stringify({ pShare: { i: 1, n: 'paillier-modulus' }, backupNShare: {} }); + + let verifyTransactionStub: sinon.SinonStub; + + const signWith = async (txParams: TssSignTxRequestParamsWithPrv['txParams'], intent: Record) => { + const mockBitgo = { + getEnv: sinon.stub().returns('test'), + setRequestTracer: sinon.stub(), + } as unknown as BitGoBase; + const mockCoin = { + verifyTransaction: verifyTransactionStub, + getConfig: sinon.stub().returns({}), + getChain: sinon.stub().returns('hteth'), + } as unknown as IBaseCoin; + const mockWallet = { + id: sinon.stub().returns(walletID), + multisigType: sinon.stub().returns('tss'), + coinSpecific: sinon.stub().returns(undefined), + }; + + const utils = new EcdsaUtils(mockBitgo, mockCoin, mockWallet as unknown as IWallet); + // Stop right after the verifier: the TSS networking that follows is covered elsewhere. + sinon.stub(utils, 'getEcdsaSigningChallenges').rejects(new Error('stop-after-verify')); + + try { + await utils.signTxRequest({ + txRequest: makeTxRequest(intent), + txParams, + prv, + reqId: { inc: sinon.stub(), toString: sinon.stub().returns('test-req') } as unknown as IRequestTracer, + }); + } catch (e) { + assert.match((e as Error).message, /stop-after-verify/); + } + + assert.strictEqual(verifyTransactionStub.callCount, 1, 'verifyTransaction must be reached exactly once'); + return verifyTransactionStub.firstCall.args[0] as { + txParams: { type?: string; recipients?: unknown[] }; + intent?: Record; + callerTxParams?: unknown; + }; + }; + + beforeEach(function () { + verifyTransactionStub = sinon.stub().resolves(true); + }); + + afterEach(function () { + sinon.restore(); + }); + + it('passes the caller txParams and the stored intent to the verifier on a direct send', async function () { + const callerTxParams = { + type: 'transfer', + recipients: [{ address: '0x' + '11'.repeat(20), amount: '1000' }], + }; + const intent = { + intentType: 'payment', + recipients: [{ address: { address: '0x' + '11'.repeat(20) }, amount: { value: '1000', symbol: 'hteth' } }], + }; + + const verifyArgs = await signWith(callerTxParams, intent); + + assert.strictEqual(verifyArgs.intent, intent, 'stored intent must reach verifyTransaction'); + assert.deepStrictEqual( + verifyArgs.callerTxParams, + callerTxParams, + 'the untouched caller input must reach the verifier' + ); + assert.strictEqual(verifyArgs.txParams.type, 'transfer'); + assert.deepStrictEqual(verifyArgs.txParams.recipients, callerTxParams.recipients); + }); + + it('passes callerTxParams as undefined on the pending-approval path', async function () { + const intent = { + intentType: 'payment', + recipients: [{ address: { address: '0x' + '22'.repeat(20) }, amount: { value: '1000', symbol: 'hteth' } }], + }; + + const verifyArgs = await signWith(undefined, intent); + + assert.strictEqual(verifyArgs.intent, intent, 'stored intent must reach verifyTransaction'); + assert.strictEqual( + verifyArgs.callerTxParams, + undefined, + 'the pending-approval path supplies no caller params; the stored intent speaks' + ); + assert.strictEqual(verifyArgs.txParams.type, 'payment'); + assert.strictEqual(verifyArgs.txParams.recipients?.length, 1, 'stored recipients are back-filled'); + }); +}); diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts index bb6ffbf5680..fb615f12b6c 100644 --- a/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts @@ -29,8 +29,11 @@ describe('recipientUtils', function () { 'disabletoken', 'customTx', 'defiApprove', + 'defi-approve', 'defiDeposit', + 'defi-deposit', 'defiWithdraw', + 'defi-withdraw', // Native wrap/unwrap — registered in both spellings on purpose 'wrapNative', 'wrap-native', @@ -82,10 +85,31 @@ describe('recipientUtils', function () { }); it('does not contain value-transfer types', function () { - ['payment', 'fanout', 'vote', 'defi-deposit', 'defi-redeem'].forEach((t) => { + ['payment', 'fanout', 'vote'].forEach((t) => { assert.ok(!NO_RECIPIENT_TX_TYPES.has(t), `${t} must NOT be in NO_RECIPIENT_TX_TYPES`); }); }); + + it('accepts kebab-case stored DeFi intent types without recipients', function () { + // Stored defi-* intents have no recipients; they must reach verifyTssTransaction's DeFi + // calldata checks (no InvalidTransactionError, no silent skip) instead of being rejected + // by the missing-recipients guard. + for (const intentType of ['defi-approve', 'defi-deposit', 'defi-withdraw']) { + const txRequest = makeTxRequest({ intent: { intentType } as PopulatedIntent }); + assert.doesNotThrow( + () => resolveEffectiveTxParams(txRequest, undefined), + `${intentType} should not throw without recipients` + ); + } + }); + + it('keeps rejecting stored defi-redeem intents, which have no EVM verifier handler', function () { + for (const intentType of ['defi-redeem', 'defiRedeem']) { + assert.ok(!NO_RECIPIENT_TX_TYPES.has(intentType), `${intentType} must NOT be in NO_RECIPIENT_TX_TYPES`); + const txRequest = makeTxRequest({ intent: { intentType } as PopulatedIntent }); + assert.throws(() => resolveEffectiveTxParams(txRequest, undefined), InvalidTransactionError); + } + }); }); describe('resolveEffectiveTxParams', function () {