From 90aceaa553cbdcffa32dc6570859f89aca49b88b Mon Sep 17 00:00:00 2001 From: Deepak Jangid Date: Wed, 4 Nov 2026 11:54:36 +0530 Subject: [PATCH 1/4] 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 () { From e47c0e47f4976b62a957f4e13bf42ff459e37694 Mon Sep 17 00:00:00 2001 From: Deepak Jangid Date: Wed, 4 Nov 2026 22:08:37 +0530 Subject: [PATCH 2/4] fix(abstract-eth): close EVM TSS verification regressions Verify legacy transactions using serialized bytes and bind them to the signing payload. Reject caller-declared payments without recipients rather than falling back to server-provided intent. Add regression coverage and update package changelogs. Ticket: CECHO-2460 Co-authored-by: Ralph CLI Ralph: 18.13.2-bitgo.6 openrouter/deepseek/deepseek-v4.1-flash-1017@default:high+smol:high+task:high openrouter/anthropic/claude-opus-5.5@slow:high+plan:xhigh openrouter/openai/gpt-6.1-terra@vision:low+advisor:medium openrouter/z-ai/glm-5.3-flash@commit:high+memory:low openrouter/inclusionai/ling-3.1-flash@tiny:low openrouter/moonshotai/kimi-k3.1-pro@designer:high openrouter/google/gemini-3-pro-image@image web/exa@web openrouter/microsoft/mai-transcribe-2@dictation openrouter/typesafe/jev-1.14@judge --- modules/abstract-eth/CHANGELOG.md | 7 + .../src/abstractEthLikeNewCoins.ts | 22 +- modules/abstract-eth/src/tssVerification.ts | 4 +- .../sdk-coin-evm/test/unit/tssSigningPath.ts | 234 +++++++++++------- .../sdk-coin-evm/test/unit/tssVerification.ts | 106 +++++++- modules/sdk-core/CHANGELOG.md | 6 + .../src/bitgo/utils/tss/ecdsa/ecdsa.ts | 7 + modules/statics/CHANGELOG.md | 6 + modules/statics/src/allCoinsAndTokens.ts | 4 + modules/statics/test/unit/coins.ts | 8 - 10 files changed, 303 insertions(+), 101 deletions(-) diff --git a/modules/abstract-eth/CHANGELOG.md b/modules/abstract-eth/CHANGELOG.md index 5ce0ae94ce4..e6369bb483b 100644 --- a/modules/abstract-eth/CHANGELOG.md +++ b/modules/abstract-eth/CHANGELOG.md @@ -3,6 +3,13 @@ All notable changes to this project will be documented in this file. See [Conventional Commits](https://conventionalcommits.org) for commit guidelines. +## Unreleased + +### Bug Fixes + +* Reject caller-declared EVM TSS payments without recipients in both verification modes; never substitute stored recipients for missing caller intent. +* Bind legacy EVM signing payloads using the coin instance's network configuration, avoiding ambiguous chain-id lookups. + # [25.6.0](https://github.com/BitGo/BitGoJS/compare/%40bitgo%2Fabstract-eth%4025.5.4...%40bitgo%2Fabstract-eth%4025.6.0) (2026-11-03) ### Bug Fixes diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index c5dea35e2ed..01345b982ba 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -3397,8 +3397,11 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem * /** @inheritdoc CoinWithSignableConsistency */ assertSignableConsistency(serializedTxHex: string, signableHex: string): void { - const common = AbstractEthLikeNewCoins.getCustomChainCommon(this.getChainId()); - // getCustomChainCommon defaults to the london hardfork, which is only valid for chains + // Build the common from this coin's own network. Resolving it from the chain id through + // getCustomChainCommon is ambiguous: the id -> coin registry is shared with other families + // (kavaevm's 2222 maps to 'kava'), so the lookup can throw or pick the wrong network. + const common = getCommon(this.getNetwork() as EthLikeNetwork); + // getCommon defaults to the london hardfork, which is only valid for chains // that activated EIP-1559. Chains without it (e.g. BSC, XDC) sign under petersburg. if (!this.staticsCoin?.features.includes(CoinFeature.EIP1559)) { common.setHardfork(optionalDeps.EthCommon.Hardfork.Petersburg); @@ -3631,6 +3634,21 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem throw new Error('tx cannot be both a batch and hop transaction'); } + // A caller that names a payment (`transfer` / `payment`) must supply the recipients it is + // paying. With none there is no caller intent to verify the bytes against, and the stored + // intent was dropped (its destination may be attacker-chosen), so the handler would only + // warn. `[]` declares nothing, exactly like omitting the field. This runs before the + // warn / strict policy: neither mode may downgrade an unverifiable payment to a warning. + // Only the caller's own params are read here; the back-filled `txParams` and the stored + // intent must not supply the recipients or the type. + if (canonicalizeEvmTssIntentType(callerParams?.type) === 'transfer' && !callerParams?.recipients?.length) { + await throwRecipientMismatch( + 'caller declared a payment without recipients: pass txParams.recipients so the transaction ' + + 'can be verified against the user intent', + [] + ); + } + // 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 diff --git a/modules/abstract-eth/src/tssVerification.ts b/modules/abstract-eth/src/tssVerification.ts index 72dd6652928..1e25d4f3458 100644 --- a/modules/abstract-eth/src/tssVerification.ts +++ b/modules/abstract-eth/src/tssVerification.ts @@ -326,7 +326,9 @@ export function callerDeclaresIntent(callerParams?: CallerDeclarableParams): boo * * - 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. + * consolidation / DeFi fields. A caller that declares a payment type (`transfer` / `payment`) + * but no recipients is rejected by `verifyTssTransaction` before any handler runs, because + * nothing would be left to verify the bytes against; the stored intent is never used instead. * - The caller declared nothing (signing a stored tx request, e.g. pending approval): the stored * intent, which `resolveEffectiveTxParams` has already folded into `txParams`. * diff --git a/modules/sdk-coin-evm/test/unit/tssSigningPath.ts b/modules/sdk-coin-evm/test/unit/tssSigningPath.ts index 148d16fff3f..c603b4972d0 100644 --- a/modules/sdk-coin-evm/test/unit/tssSigningPath.ts +++ b/modules/sdk-coin-evm/test/unit/tssSigningPath.ts @@ -1,7 +1,7 @@ import 'should'; import { BitGoAPI } from '@bitgo/sdk-api'; import { TestBitGo } from '@bitgo/sdk-test'; -import { coins } from '@bitgo/statics'; +import { coins, EthereumNetwork } from '@bitgo/statics'; import { EcdsaUtils, RequestTracer, @@ -10,6 +10,7 @@ import { TxRequest, Wallet, } from '@bitgo/sdk-core'; +import { getCommon, optionalDeps } from '@bitgo/abstract-eth'; import { EvmCoin } from '../../src/evmCoin'; import { TransactionBuilder } from '../../src/lib'; @@ -18,99 +19,156 @@ 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. + * + * Legacy (non-EIP-1559) chains carry two different server-supplied hexes: signableHex is the + * EIP-155 signing preimage `RLP([nonce, gasPrice, gasLimit, to, value, data, chainId, 0, 0])` and + * serializedTxHex is the broadcast-format unsigned transaction. Only the latter is parseable by + * ethereumjs, which is why these chains need + * `CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX`, and why both hexes are built here as the + * distinct values production sends instead of the same fabricated string. */ -describe('EVM TSS signing path stops tampered bytes before signing', function () { - const chain = 'kavaevm'; +const legacyChains = ['kavaevm', 'tkavaevm', 'prividiumeth', 'tprividiumeth']; + +interface SigningMaterial { + serializedTxHex: string; + signableHex: string; +} + +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: {} }); + +/** Build the two hexes a legacy EVM tx request carries, exactly as the wallet platform shapes them. */ +const buildSigningMaterial = async ( + chain: string, + opts: { to: string; value: string; data?: string } +): Promise => { 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 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); + const broadcast = (await builder.build()).toBroadcastFormat(); + // The wallet platform stores serializedTxHex without the 0x prefix. + const serializedTxHex = broadcast.startsWith('0x') ? broadcast.slice(2) : broadcast; - 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(); - }; + const common = getCommon(staticsCoin.network as EthereumNetwork); + const legacyTx = optionalDeps.EthTx.Transaction.fromSerializedTx(Buffer.from(serializedTxHex, 'hex'), { common }); + // getMessageToSign(false) appends chainId, 0, 0: the EIP-155 preimage the platform sends as + // signableHex. ethereumjs rejects it in fromSerializedTx (v = chainId), so it must never be the + // bytes the verifier decodes. + const signableHex = Buffer.from(optionalDeps.ethUtil.rlp.encode(legacyTx.getMessageToSign(false))).toString('hex'); - /** 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 }; - }; + return { serializedTxHex, signableHex }; +}; - 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/); - } +/** Signs through EcdsaUtils and reports whether the signing round (after verification) started. */ +const sign = async (opts: { + chain: string; + material: SigningMaterial; + txParams?: Record; + strict?: boolean; +}): Promise<{ signingStarted: boolean; error?: Error }> => { + const staticsCoin = coins.get(opts.chain); + 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: { + intentType: 'payment', + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: opts.chain } }], + }, + transactions: [ + { + unsignedTx: { + derivationPath: 'm/0', + signableHex: opts.material.signableHex, + serializedTxHex: opts.material.serializedTxHex, + }, + 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 }; +}; + +const callerPayment = { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }; + +describe('EVM TSS signing path stops tampered bytes before signing (legacy chains)', function () { + for (const chain of legacyChains) { + describe(chain, function () { + it('signs a payment whose serialized bytes match the intent', async function () { + const material = await buildSigningMaterial(chain, { to: recipientAddress, value: '1000' }); + material.signableHex.should.not.equal( + material.serializedTxHex, + 'the preimage and the broadcast bytes must be distinct, as the platform sends them' + ); + for (const txParams of [undefined, callerPayment]) { + const { signingStarted, error } = await sign({ chain, material, 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 material = await buildSigningMaterial(chain, { to: attackerAddress, value: '1000' }); + for (const strict of [false, true]) { + const { signingStarted, error } = await sign({ chain, material, 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 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 a preimage that does not match the verified serialized bytes', async function () { + const chain = 'kavaevm'; + const benign = await buildSigningMaterial(chain, { to: recipientAddress, value: '1000' }); + const attacker = await buildSigningMaterial(chain, { to: attackerAddress, value: '1000' }); + // Verify the benign serialized bytes, then try to sign the attacker's preimage. + const material = { serializedTxHex: benign.serializedTxHex, signableHex: attacker.signableHex }; + const { signingStarted, error } = await sign({ chain, material, txParams: callerPayment, strict: true }); + signingStarted.should.equal(false); + (error as Error).message.should.match(/signableHex is inconsistent with serializedTxHex/); }); 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 }); + const material = await buildSigningMaterial('kavaevm', { + to: tokenContractAddress, + value: '0', + data: approveAttackerMax, + }); + const { signingStarted, error } = await sign({ chain: 'kavaevm', material, 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/); @@ -118,8 +176,12 @@ describe('EVM TSS signing path stops tampered bytes before signing', function () 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 }); + const material = await buildSigningMaterial('kavaevm', { + to: tokenContractAddress, + value: '0', + data: approveAttackerMax, + }); + const { signingStarted } = await sign({ chain: 'kavaevm', material, 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 index 06e89649434..249850516a2 100644 --- a/modules/sdk-coin-evm/test/unit/tssVerification.ts +++ b/modules/sdk-coin-evm/test/unit/tssVerification.ts @@ -269,12 +269,17 @@ async function buildTxHex( await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/not a recognized payment shape/); }); - it('flags missing recipients under a payment intent', async function () { + it('rejects missing recipients under a caller-declared payment intent (fail closed)', async function () { + // An empty recipients array declares nothing to verify the bytes against: signing stops + // whether or not the client opted into strictTssVerification. 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/); + for (const strict of [false, true]) { + await verify(txParams, txHex, { strict }).should.be.rejectedWith( + /caller declared a payment without recipients/ + ); + } + warnings.should.have.length(0); }); }); @@ -581,6 +586,99 @@ async function buildTxHex( /cannot be resolved to a token contract/ ); }); + + describe('caller-declared payment without recipients (fail closed)', function () { + // A caller that names a payment type but supplies no recipients leaves nothing to verify + // the bytes against: the stored intent is dropped (its destination may be attacker + // chosen), so signing must stop instead of continuing behind a warning. An empty array + // declares nothing, exactly like omitting the field, and no mode may downgrade this. + const attackerIntent = { + intentType: 'payment', + recipients: [{ address: { address: attackerAddress }, amount: { value: '9999', symbol: 'hteth' } }], + }; + + it('rejects a caller `transfer` type with no recipients, both modes', async function () { + const txHex = await build({ to: attackerAddress, value: '9999' }); + for (const strict of [false, true]) { + await verifySigningPath({ type: 'transfer' }, attackerIntent, txHex, strict).should.be.rejectedWith( + /caller declared a payment without recipients/ + ); + } + warnings.should.have.length(0); + }); + + it('rejects a caller `transfer` type with an empty recipients array, both modes', async function () { + const caller = { type: 'transfer', recipients: [] }; + const txHex = await build({ to: attackerAddress, value: '9999' }); + for (const strict of [false, true]) { + await verifySigningPath(caller, attackerIntent, txHex, strict).should.be.rejectedWith( + /caller declared a payment without recipients/ + ); + } + warnings.should.have.length(0); + }); + + it('treats the stored spelling `payment` as the same caller declaration', async function () { + const txHex = await build({ to: attackerAddress, value: '9999' }); + for (const strict of [false, true]) { + await verifySigningPath({ type: 'payment' }, attackerIntent, txHex, strict).should.be.rejectedWith( + /caller declared a payment without recipients/ + ); + } + warnings.should.have.length(0); + }); + + it('keeps signing a stored payment intent when the caller declared nothing', async function () { + const intent = { + intentType: 'payment', + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: 'hteth' } }], + }; + const txHex = await build({ to: recipientAddress, value: '1000' }); + for (const strict of [false, true]) { + (await verifySigningPath(undefined, intent, txHex, strict)).should.equal(true); + } + warnings.should.have.length(0); + }); + + it('keeps the stored token identity when the caller supplies only a token name', 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: encodeErc20Transfer(recipientAddress, '1000'), + }); + for (const strict of [false, true]) { + (await verifySigningPath({ tokenName: localTokenName }, intent, txHex, strict)).should.equal(true); + } + warnings.should.have.length(0); + }); + + it('leaves other types without recipients on the warn-first policy', async function () { + // The fail-closed rule is limited to caller-declared payments: a transferToken with no + // recipients keeps warning by default and keeps throwing only in strict mode. + const caller = { type: 'transferToken' }; + const intent = { + intentType: 'transferToken', + tokenName: localTokenName, + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: localTokenName } }], + }; + const txHex = await build({ + to: localTokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '1000'), + }); + + (await verifySigningPath(caller, intent, txHex)).should.equal(true); + warnings.some((w) => /must declare exactly one token recipient/.test(w.reason)).should.equal(true); + await verifySigningPath(caller, intent, txHex, true).should.be.rejectedWith( + /must declare exactly one token recipient/ + ); + }); + }); }); describe('batch payments without a configured batcher contract (statics gap)', function () { diff --git a/modules/sdk-core/CHANGELOG.md b/modules/sdk-core/CHANGELOG.md index fae39f2b2e6..359d9ffde76 100644 --- a/modules/sdk-core/CHANGELOG.md +++ b/modules/sdk-core/CHANGELOG.md @@ -3,6 +3,12 @@ All notable changes to this project will be documented in this file. See [Conventional Commits](https://conventionalcommits.org) for commit guidelines. +## Unreleased + +### Bug Fixes + +* Check signing-payload consistency on the ECDSA serialized-transaction verification path, matching MPCv2, before entering the signing round. + # [40.5.0](https://github.com/BitGo/BitGoJS/compare/%40bitgo%2Fsdk-core%4040.4.0...%40bitgo%2Fsdk-core%4040.5.0) (2026-11-03) ### Bug Fixes 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 b919b44ecbd..362db762a26 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts @@ -53,6 +53,7 @@ import { BaseEcdsaUtils } from './base'; import { EncryptionVersion, IRequestTracer } from '../../../../api'; import { resolveEffectiveTxParams } from '../recipientUtils'; import { shouldVerifyWithSerializedTxHex } from '../serializedTxHexVerify'; +import { isCoinWithSignableConsistency } from '../signableConsistency'; const encryptNShare = ECDSAMethods.encryptNShare; @@ -839,6 +840,12 @@ export class EcdsaUtils extends BaseEcdsaUtils { intent: txRequest.intent as PopulatedIntent, callerTxParams: params.txParams, }); + // The verifier above inspects serializedTxHex only, but the signing round below signs + // signableHex. Both are server-supplied, so bind them the same way the MPCv2 path does: + // otherwise verified bytes could be paired with a different signing preimage. + if (isCoinWithSignableConsistency(this.baseCoin)) { + this.baseCoin.assertSignableConsistency(unsignedTx.serializedTxHex, unsignedTx.signableHex); + } } else { await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.signableHex }, diff --git a/modules/statics/CHANGELOG.md b/modules/statics/CHANGELOG.md index ca530236e44..8de0bb5508c 100644 --- a/modules/statics/CHANGELOG.md +++ b/modules/statics/CHANGELOG.md @@ -3,6 +3,12 @@ All notable changes to this project will be documented in this file. See [Conventional Commits](https://conventionalcommits.org) for commit guidelines. +## Unreleased + +### Bug Fixes + +* Verify Kava EVM and Prividium TSS transactions using serialized bytes rather than legacy EIP-155 signing preimages, on mainnet and testnet. + # [59.28.0](https://github.com/BitGo/BitGoJS/compare/%40bitgo%2Fstatics%4059.27.0...%40bitgo%2Fstatics%4059.28.0) (2026-11-03) ### Bug Fixes diff --git a/modules/statics/src/allCoinsAndTokens.ts b/modules/statics/src/allCoinsAndTokens.ts index 54c495c9f14..f49e9856ba3 100644 --- a/modules/statics/src/allCoinsAndTokens.ts +++ b/modules/statics/src/allCoinsAndTokens.ts @@ -2170,6 +2170,7 @@ export const allCoinsAndTokens = [ CoinFeature.EVM_COMPATIBLE_WP, CoinFeature.EVM_NON_BITGO_RECOVERY, CoinFeature.EVM_UNSIGNED_SWEEP_RECOVERY, + CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX, ] ), account( @@ -2189,6 +2190,7 @@ export const allCoinsAndTokens = [ CoinFeature.EVM_COMPATIBLE_WP, CoinFeature.EVM_NON_BITGO_RECOVERY, CoinFeature.EVM_UNSIGNED_SWEEP_RECOVERY, + CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX, ] ), account( @@ -2878,6 +2880,7 @@ export const allCoinsAndTokens = [ CoinFeature.EVM_COMPATIBLE_UI, CoinFeature.EVM_COMPATIBLE_WP, CoinFeature.SUPPORTS_ERC20, + CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX, ] ), account( @@ -2895,6 +2898,7 @@ export const allCoinsAndTokens = [ CoinFeature.EVM_COMPATIBLE_IMS, CoinFeature.EVM_COMPATIBLE_UI, CoinFeature.EVM_COMPATIBLE_WP, + CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX, ] ), diff --git a/modules/statics/test/unit/coins.ts b/modules/statics/test/unit/coins.ts index 969ff514bb1..17b75429912 100644 --- a/modules/statics/test/unit/coins.ts +++ b/modules/statics/test/unit/coins.ts @@ -1439,14 +1439,6 @@ describe('ERC20 Bulk Transaction Feature', () => { }); describe('TSS Verify Use Serialized Tx Hex Feature', () => { - it('should have TSS_VERIFY_USE_SERIALIZED_TX_HEX for coins whose signableHex is not parseable', () => { - const coinsNeedingSerializedTxHexVerify = ['bsc', 'tbsc', 'xdc', 'txdc', 'icp', 'ticp']; - coinsNeedingSerializedTxHexVerify.forEach((coinName) => { - const coin = coins.get(coinName); - coin.features.includes(CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX).should.eql(true); - }); - }); - it('should have TSS_VERIFY_USE_SERIALIZED_TX_HEX on BSC and XDC tokens', () => { coins.get('bsc:busd').features.includes(CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX).should.eql(true); coins.get('xdc:usdc').features.includes(CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX).should.eql(true); From 6e9bbe9b39f44077447fa8005437272e71f3a745 Mon Sep 17 00:00:00 2001 From: Deepak Jangid Date: Wed, 4 Nov 2026 23:29:29 +0530 Subject: [PATCH 3/4] fix(abstract-eth): close EVM TSS verification review gaps Honor caller token metadata and receive-address nonce fills, and normalize calldata before comparing intent. Reject undecodable payloads, missing token recipients, and stored DeFi intents without a trusted vault binding. Keep warn-by-default rollout and strict opt-in enforcement. Isolate warning callback failures and bound console diagnostics. Ticket: CECHO-2460 Co-authored-by: Ralph CLI Ralph: 18.13.2-bitgo.6 openrouter/deepseek/deepseek-v4.1-flash-1017@default:high+smol:high+task:high openrouter/anthropic/claude-opus-5.5@slow:high+plan:xhigh openrouter/openai/gpt-6.1-terra@vision:low+advisor:medium openrouter/z-ai/glm-5.3-flash@commit:high+memory:low openrouter/inclusionai/ling-3.1-flash@tiny:low openrouter/moonshotai/kimi-k3.1-pro@designer:high openrouter/google/gemini-3-pro-image@image web/exa@web openrouter/microsoft/mai-transcribe-2@dictation openrouter/typesafe/jev-1.14@judge --- modules/abstract-eth/CHANGELOG.md | 3 + modules/abstract-eth/README.md | 37 +- .../src/abstractEthLikeNewCoins.ts | 208 +++++--- modules/abstract-eth/src/tssVerification.ts | 225 +++++--- modules/sdk-coin-eth/test/unit/eth.ts | 99 +++- .../sdk-coin-evm/test/unit/tssVerification.ts | 490 ++++++++++++++++-- modules/sdk-coin-flr/test/unit/flr.ts | 24 +- .../src/bitgo/utils/tss/recipientUtils.ts | 9 +- .../unit/bitgo/utils/tss/recipientUtils.ts | 6 +- 9 files changed, 912 insertions(+), 189 deletions(-) diff --git a/modules/abstract-eth/CHANGELOG.md b/modules/abstract-eth/CHANGELOG.md index e6369bb483b..d2ed3f12496 100644 --- a/modules/abstract-eth/CHANGELOG.md +++ b/modules/abstract-eth/CHANGELOG.md @@ -9,6 +9,9 @@ See [Conventional Commits](https://conventionalcommits.org) for commit guideline * Reject caller-declared EVM TSS payments without recipients in both verification modes; never substitute stored recipients for missing caller intent. * Bind legacy EVM signing payloads using the coin instance's network configuration, avoiding ambiguous chain-id lookups. +* Resolve caller tokenData consistently, reject conflicting token identities, canonicalize calldata, and verify fillNonce against the declared receive address. +* Reject undecodable EVM TSS payloads, recipient-less token transfers, stored DeFi intents without a trusted vault binding, and DeFi legs carrying native value. +* Isolate verification-warning callback failures and redact calldata in console warnings without weakening strict verification. # [25.6.0](https://github.com/BitGo/BitGoJS/compare/%40bitgo%2Fabstract-eth%4025.5.4...%40bitgo%2Fabstract-eth%4025.6.0) (2026-11-03) diff --git a/modules/abstract-eth/README.md b/modules/abstract-eth/README.md index 1a54297486b..c65864ca754 100644 --- a/modules/abstract-eth/README.md +++ b/modules/abstract-eth/README.md @@ -3,21 +3,34 @@ ## 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). +`data` with the declared intent before the user key share signs (TOB-BITGOEDMPC-2 / INFOSEC-671). +The default rollout reports new mismatches without blocking signing; it is not full fail-closed +enforcement. Set `strictTssVerification: true` locally on `BitGoAPI` to reject every finding. - 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. +- Token identity comes from the recipient's `tokenAddress` / `tokenName` and + `tokenData.tokenContractAddress` / `tokenData.tokenName`, the top-level `tokenName`, and the + coin instance. Conflicting or unresolvable declarations produce a finding; no declaration + silently overrides another. Payment shapes include native, ERC-20 / ERC-7984, and single-asset + batch transfers. Calldata comparisons canonicalize the hex prefix, case, and empty values. +- Missing or undecodable transaction bytes and caller-declared `transfer` / `payment` / + `transferToken` without recipients always reject, in either mode. Historic destination, + value, ERC-20 recipient and amount checks, plus `consolidationToBaseAddress`, remain fatal. +- Other findings go to `console.warn` and the optional `onVerificationWarning` callback in + default mode. Console reason text is bounded, and structured calldata fields show only the + selector and original length; this is not a guarantee of secret-free logs. The callback receives + full fields. Callback throws and rejected promises do not abort or delay signing. + Strict mode rejects the first finding, including unsupported operations such as acceleration, + staking, wrap/unwrap, token enablement, and bridging; it never downgrades them to warnings. - 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. +- `fillNonce` must send exactly 1 wei with no calldata to the declared `receiveAddress`, or to + the wallet base address when none is declared. Stored intents supply this address only when + the caller declared no intent of its own. +- `contractCall` and `customTx` always reject on EVM. Stored DeFi approve/deposit/withdraw + intents also reject: there is no locally trusted vault registry to bind the target or spender. + Stored `defi-redeem` remains unsupported. Caller-declared DeFi legs retain selector, amount, + and receiver/owner checks and reject native value; token-wallet approvals also bind the token + contract. These checks do not establish the vault's identity or the approval spender. diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index 01345b982ba..bd2487394a5 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -54,6 +54,7 @@ import { DeriveAddressResult, NO_RECIPIENT_TX_TYPES, CoinWithSignableConsistency, + TssVerificationMismatch, TssVerificationWarning, } from '@bitgo/sdk-core'; import { getDerivationPath } from '@bitgo/sdk-lib-mpc'; @@ -64,7 +65,6 @@ import { DecodedEvmTx, evaluateEvmTssIntent, isUnsupportedEvmTssIntentType, - mapDefiParamsFromIntent, resolveCoinTokenContractAddress, selectDeclaredIntent, TssFinding, @@ -414,6 +414,11 @@ interface EthTransactionParams extends TransactionParams { tokenName?: string; feeToken?: string; defiParams?: DefiIntentParams; + /** + * Address a self-send must pay (fillNonce). Callers that fund a specific address pass it so the + * verifier compares the decoded `to` with that address instead of the wallet's own base address. + */ + receiveAddress?: string; /** ERC-7984 shield intent (`type: 'wrapApprove' | 'wrap'`): wrapper coin name and exact amount. */ wrapParams?: { tokenName: string; amount: string }; /** ERC-7984 unshield intent (`type: 'unwrap'`): wrapper coin name and plaintext amount. */ @@ -424,6 +429,49 @@ interface EthTransactionParams extends TransactionParams { const MAX_UINT256 = 2n ** 256n - 1n; +/** Longest calldata prefix a console warning may echo: the 4-byte selector and nothing more. */ +const MAX_LOGGED_CALLDATA_PREFIX = 10; + +/** + * Bound the calldata a console warning prints. `data` can be arbitrarily large and carries the + * arguments of the call about to be signed, so only the selector and the payload length are + * logged. The `onVerificationWarning` callback still receives the full mismatch. + */ +function redactMismatchForLog(mismatch: TssVerificationMismatch): TssVerificationMismatch { + if (mismatch.data === undefined || mismatch.data.length <= MAX_LOGGED_CALLDATA_PREFIX) { + return mismatch; + } + return { + ...mismatch, + data: `${mismatch.data.slice(0, MAX_LOGGED_CALLDATA_PREFIX)}...(${mismatch.data.length} chars)`, + }; +} + +/** Longest finding reason a console warning may print. */ +const MAX_LOGGED_REASON_CHARS = 256; + +/** + * Bound the reason text a console warning prints. A finding reason can embed a parser error that + * echoes calldata, and a token identity in it can be arbitrarily long; the console is a diagnostic + * log, so it gets a capped string. The `onVerificationWarning` callback still receives the full + * reason. + */ +function redactReasonForLog(reason: string): string { + if (reason.length <= MAX_LOGGED_REASON_CHARS) { + return reason; + } + return `${reason.slice(0, MAX_LOGGED_REASON_CHARS)}...(truncated, ${reason.length} chars)`; +} + +/** True when a decoded transaction value is exactly zero; an unparseable value is not zero. */ +function isZeroNativeValue(value?: string): boolean { + try { + return BigInt(value || '0') === 0n; + } catch { + return false; + } +} + export interface VerifyEthTransactionOptions extends VerifyTransactionOptions { txPrebuild: TransactionPrebuild; txParams: EthTransactionParams; @@ -3585,6 +3633,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem * @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 {Error} if txPrebuild.txHex is missing or cannot be decoded, in either mode * @throws {TxIntentMismatchRecipientError} if transaction recipients don't match user intent */ async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise { @@ -3634,17 +3683,22 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem throw new Error('tx cannot be both a batch and hop transaction'); } - // A caller that names a payment (`transfer` / `payment`) must supply the recipients it is - // paying. With none there is no caller intent to verify the bytes against, and the stored - // intent was dropped (its destination may be attacker-chosen), so the handler would only - // warn. `[]` declares nothing, exactly like omitting the field. This runs before the - // warn / strict policy: neither mode may downgrade an unverifiable payment to a warning. - // Only the caller's own params are read here; the back-filled `txParams` and the stored - // intent must not supply the recipients or the type. - if (canonicalizeEvmTssIntentType(callerParams?.type) === 'transfer' && !callerParams?.recipients?.length) { + // A caller that names a payment (`transfer` / `payment`) or a token payment (`transferToken`) + // must supply the recipients it is paying. With none there is no caller intent to verify the + // bytes against, and the stored intent was dropped (its destination may be attacker-chosen), + // so the handler would only warn. `[]` declares nothing, exactly like omitting the field. + // This runs before the warn / strict policy: neither mode may downgrade an unverifiable + // payment to a warning. Only the caller's own params are read here; the back-filled `txParams` + // and the stored intent must not supply the recipients or the type. + const declaredPaymentType = canonicalizeEvmTssIntentType(callerParams?.type); + if ( + (declaredPaymentType === 'transfer' || declaredPaymentType === 'transferToken') && + !callerParams?.recipients?.length + ) { + const declaredPayment = declaredPaymentType === 'transferToken' ? 'token transfer' : 'payment'; await throwRecipientMismatch( - 'caller declared a payment without recipients: pass txParams.recipients so the transaction ' + - 'can be verified against the user intent', + `caller declared a ${declaredPayment} without recipients: pass txParams.recipients so the ` + + 'transaction can be verified against the user intent', [] ); } @@ -3674,20 +3728,19 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem 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; + // The bytes about to be signed are the only ground truth and every check below reads them. + // Bytes that cannot be decoded cannot be verified against anything, so reject here, before a + // handler or the warn / strict policy runs: no mode may turn missing or undecodable bytes into + // a warning, and no placeholder transaction may stand in for them. + let tx: DecodedEvmTx; try { - decodedTx = await this.decodeTssTransaction(txPrebuild.txHex); + tx = await this.decodeTssTransaction(txPrebuild.txHex); } catch (e) { - decodeError = e as Error; - } - const requireDecoded = (): DecodedEvmTx => { - if (!decodedTx) { - throw decodeError; + if (!txPrebuild.txHex) { + throw new Error('missing txHex in txPrebuild'); } - return decodedTx; - }; + throw new Error(`transaction bytes could not be decoded to verify intent: ${(e as Error).message}`); + } const raise = (finding: TssFinding): Promise => throwRecipientMismatch(finding.reason, [ { address: finding.decoded.to ?? '', amount: finding.decoded.value ?? '' }, @@ -3701,50 +3754,75 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem const coinBaseAddress = wallet.coinSpecific()?.baseAddress; const baseAddress = typeof coinBaseAddress === 'string' ? coinBaseAddress : undefined; + // Locally known identity of this coin instance: set only when the coin itself is a token. + const coinTokenContractAddress = resolveCoinTokenContractAddress(this.getChain()); const ctx = buildEvmTssIntentContext({ intentType: canonicalType, txParams, callerParams, storedIntent, - coinTokenContractAddress: resolveCoinTokenContractAddress(this.getChain()), + coinTokenContractAddress, baseAddress, batcherAddress: ethNetwork?.batcherContractAddress, }); - // 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' }); + // Fatal findings (the checks that threw before fail-closed verification existed) throw now; + // the rest wait for the policy step at the end. + const intentFindings = evaluateEvmTssIntent(ctx, tx); await raiseFatal(intentFindings); - // DeFi vault calldata vs defiParams. Stored defi-* intents reach this branch through the - // intent type mapping, with defiParams reconstructed from the stored intent fields. + // DeFi vault calldata vs defiParams. The leg is built server-side from defiParams, so the + // amount and the in-calldata destination are all the caller declared; the target contract and + // the approve spender are not among them. Statics is the only registry this process can + // consult while signing, and it holds no vault entries, so the vault itself can never be + // anchored locally. It does hold the wallet's own token contract, which is the contract an + // ERC-20 allowance for the vault must be granted on. if (['defiApprove', 'defiDeposit', 'defiWithdraw'].includes(canonicalType)) { - if (!txPrebuild.txHex) { - throw new Error('missing txHex in txPrebuild'); + // A stored intent carries only vaultId and amount, all of it server-supplied. Comparing it + // with server-supplied bytes would only prove the server agrees with itself, so the stored + // flow is rejected instead of reported as verified. + if (storedIntent) { + await throwRecipientMismatch( + `stored ${canonicalType} intent cannot be verified: no locally known vault contract for ` + + `vault '${storedIntent.vaultId ?? ''}' to anchor the transaction target and spender`, + [] + ); } - const defiParams = txParams.defiParams ?? (storedIntent ? mapDefiParamsFromIntent(storedIntent) : undefined); + const defiParams = txParams.defiParams; if (!coinBaseAddress || !defiParams) { await throwRecipientMismatch('DeFi transaction is missing required intent parameters', []); - return false; } const validatedBaseAddress = coinBaseAddress as string; - const validatedDefiParams = defiParams; - const txJson = requireDecoded(); + const validatedDefiParams = defiParams as DefiIntentParams; + const txJson = tx; const mismatch = async (message: string, address = txJson.to, amount = txJson.value): Promise => throwRecipientMismatch(message, [{ address: address || '', amount: amount || '' }]); const data = txJson.data || ''; + // An ERC-20 approve or an ERC-4626 deposit / redeem leg moves no native currency. A nonzero + // value would hand the wallet's native balance to whatever contract `to` names. + if (!isZeroNativeValue(txJson.value)) { + await mismatch(`${canonicalType} transaction must not carry native value`); + } + 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'); } + // The allowance is granted by the token contract. When this coin instance is that token, + // statics names the contract locally, so the leg can be anchored to it. Base-coin wallets + // have no local token identity, and no local registry holds vault addresses, so for them + // neither the target contract nor the spender can be anchored here. + if (coinTokenContractAddress && txJson.to.toLowerCase() !== coinTokenContractAddress.toLowerCase()) { + await mismatch( + 'defiApprove transaction must target the wallet token contract', + txJson.to, + validatedDefiParams.amount + ); + } const [, amount] = getRawDecoded(['address', 'uint256'], getBufferedByteCode(selector, data)); if (amount.toString() !== validatedDefiParams.amount) { await mismatch( @@ -3825,7 +3903,6 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem 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'); } @@ -3845,21 +3922,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem await raiseFatal(consolidationFindings); } - 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: {}, - }, - ]; - - await this.applyTssVerificationPolicy([...consolidationFindings, ...decodeFindings, ...intentFindings], raise); + await this.applyTssVerificationPolicy([...consolidationFindings, ...intentFindings], raise); return true; } @@ -3876,8 +3939,9 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem /** * 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. + * the client sets `strictTssVerification`, and otherwise go to `console.warn` (reason bounded, + * calldata redacted) and the optional `onVerificationWarning` callback, which receives both in + * full and whose own failures are dropped. */ private async applyTssVerificationPolicy( findings: TssFinding[], @@ -3887,12 +3951,40 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem return; } if (this.bitgo?.strictTssVerification) { + // Strict mode fails on the first finding in policy order, including an `unhandled` intent + // type: nothing here filters or reorders `findings`. 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); + console.warn( + `[tss verification] ${redactReasonForLog(reason)}`, + JSON.stringify({ expected: redactMismatchForLog(expected), decoded: redactMismatchForLog(decoded) }) + ); + this.notifyVerificationWarning(warning); + } + } + + /** + * Hand one finding to the client's local warning sink. Warn mode must never fail because of the + * sink: a synchronous throw is dropped, and the promise of an `async` handler is not awaited (it + * cannot hold up signing) and its rejection is swallowed. The sink still receives the full + * warning, exactly once per finding, in policy order. + */ + private notifyVerificationWarning(warning: TssVerificationWarning): void { + const handler = this.bitgo?.onVerificationWarning; + if (!handler) { + return; + } + try { + const result: unknown = handler(warning); + if (result && typeof (result as PromiseLike).then === 'function') { + Promise.resolve(result).catch((e) => + debug('onVerificationWarning rejected: %s', (e as Error)?.message ?? String(e)) + ); + } + } catch (e) { + debug('onVerificationWarning threw: %s', (e as Error)?.message ?? String(e)); } } diff --git a/modules/abstract-eth/src/tssVerification.ts b/modules/abstract-eth/src/tssVerification.ts index 1e25d4f3458..f3e5f1ccda0 100644 --- a/modules/abstract-eth/src/tssVerification.ts +++ b/modules/abstract-eth/src/tssVerification.ts @@ -14,7 +14,6 @@ * `strictTssVerification`. */ import { - DefiIntentParams, ITransactionRecipient, PopulatedIntent, TssVerificationMismatch, @@ -111,6 +110,12 @@ export interface TssFinding extends TssVerificationWarning { severity: TssFindingSeverity; } +/** One token identity a recipient declares, either flat or through the intent `tokenData` shape. */ +export interface EvmTokenIdentityDeclaration { + tokenName?: string; + tokenContractAddress?: string; +} + /** One recipient of the user intent, flattened from txParams or the stored tx request intent. */ export interface EvmTssExpectedRecipient { address: string; @@ -119,6 +124,12 @@ export interface EvmTssExpectedRecipient { data?: string; tokenName?: string; tokenContractAddress?: string; + /** + * Every token identity the source declared for this recipient (`tokenAddress` / `tokenName` and + * `tokenData.tokenName` / `tokenData.tokenContractAddress`). They are all resolved, so two + * declarations of different tokens stay ambiguous instead of one silently overriding the other. + */ + declaredTokenIdentities?: EvmTokenIdentityDeclaration[]; } export interface EvmTssIntentContext { @@ -133,6 +144,11 @@ export interface EvmTssIntentContext { * token of its own. */ coinTokenContractAddress?: string; + /** + * Address whose nonce is filled (`fillNonce`), read from the source that speaks for the user + * intent only: the declared `receiveAddress`, or the stored intent's `receiveAddress`. + */ + receiveAddress?: string; /** Wallet base address (`wallet.coinSpecific().baseAddress`). */ baseAddress?: string; /** Network batcher contract from statics (`network.batcherContractAddress`). */ @@ -180,12 +196,21 @@ function toBigInt(value: unknown): bigint { return BigInt(String(value)); } -function isEmptyCalldata(data?: string): boolean { - return !data || data.toLowerCase() === '0x'; +/** + * Canonical calldata for every comparison: missing data, `''` and `'0x'` are the same empty + * payload, hex is lowercased, and a dropped `0x` prefix is restored. Selector gates and decoders + * all read this form, so a format difference can never route the same bytes past a check. + */ +function normalizeCalldata(data?: string): string { + const lower = (data ?? '').toLowerCase(); + if (!lower || lower === '0x') { + return '0x'; + } + return lower.startsWith('0x') ? lower : `0x${lower}`; } -function normalizeCalldata(data?: string): string { - return (data ?? '0x').toLowerCase(); +function isEmptyCalldata(data?: string): boolean { + return normalizeCalldata(data) === '0x'; } function valuesEqual(a: unknown, b: unknown): boolean { @@ -247,24 +272,35 @@ 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. + * `tokenAddress` / `tokenName` / `tokenData` declarations, 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 } + declared: { + tokenName?: string; + tokenContractAddress?: string; + declaredTokenIdentities?: EvmTokenIdentityDeclaration[]; + } ): 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]) { + // Every identity the recipient declared is resolved, not just the first that happens to be + // set: two declarations of different tokens must stay ambiguous rather than one silently + // overriding the other. + const identities: EvmTokenIdentityDeclaration[] = declared.declaredTokenIdentities?.length + ? declared.declaredTokenIdentities + : [{ tokenName: declared.tokenName, tokenContractAddress: declared.tokenContractAddress }]; + for (const identity of identities) { + if (identity.tokenContractAddress !== undefined) { + sources.push({ + label: `tokenAddress '${identity.tokenContractAddress}'`, + contract: EVM_ADDRESS.test(identity.tokenContractAddress) ? identity.tokenContractAddress : undefined, + }); + } + const name = identity.tokenName; if (!name) { continue; } @@ -282,6 +318,23 @@ function resolveAsset( sources.push({ label: `token '${name}'`, contract: resolveTokenContractAddress(name) }); } } + const declaredTopLevelName = ctx.tokenName; + if (declaredTopLevelName) { + let staticsCoin; + try { + staticsCoin = coins.get(declaredTopLevelName); + } catch { + staticsCoin = undefined; + } + if (staticsCoin && !(staticsCoin instanceof ContractAddressDefinedToken && staticsCoin.isToken)) { + sources.push({ label: `coin '${declaredTopLevelName}'`, contract: undefined, native: true }); + } else { + sources.push({ + label: `token '${declaredTopLevelName}'`, + contract: resolveTokenContractAddress(declaredTopLevelName), + }); + } + } if (ctx.coinTokenContractAddress) { sources.push({ label: 'the coin token', contract: ctx.coinTokenContractAddress }); } @@ -314,7 +367,16 @@ function resolveAsset( return { kind: 'token', contract: first.contract }; } -type CallerDeclarableParams = { type?: string; tokenName?: string; recipients?: ITransactionRecipient[] }; +/** EVM recipients also carry the intent `tokenData` shape that `populateIntent` preserves. */ +type CallerDeclarableRecipient = ITransactionRecipient & { tokenData?: EvmTokenIdentityDeclaration }; + +type CallerDeclarableParams = { + type?: string; + tokenName?: string; + recipients?: CallerDeclarableRecipient[]; + /** `fillNonce`: the address whose nonce is filled. */ + receiveAddress?: string; +}; /** True when the caller named a transaction type or recipients, i.e. declared its own intent. */ export function callerDeclaresIntent(callerParams?: CallerDeclarableParams): boolean { @@ -326,9 +388,10 @@ export function callerDeclaresIntent(callerParams?: CallerDeclarableParams): boo * * - 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. A caller that declares a payment type (`transfer` / `payment`) - * but no recipients is rejected by `verifyTssTransaction` before any handler runs, because - * nothing would be left to verify the bytes against; the stored intent is never used instead. + * consolidation / DeFi fields. A caller that declares a payment type (`transfer` / `payment` / + * `transferToken`) but no recipients is rejected by `verifyTssTransaction` before any handler + * runs, because nothing would be left to verify the bytes against; the stored intent is never + * used instead. * - The caller declared nothing (signing a stored tx request, e.g. pending approval): the stored * intent, which `resolveEffectiveTxParams` has already folded into `txParams`. * @@ -353,7 +416,10 @@ export function selectDeclaredIntent(params: { /** * 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. + * params. Absent local `data` stays absent, a recipient's flat and `tokenData` token identities + * are all kept (so conflicting declarations stay ambiguous), and only a source that declared no + * identity of its own inherits the stored one. `receiveAddress` comes from that same source, so a + * caller's declared receive address is never replaced by the stored intent's. */ export function buildEvmTssIntentContext(params: { intentType: string; @@ -371,14 +437,31 @@ export function buildEvmTssIntentContext(params: { const source = storedIntent ? params.txParams : params.callerParams; const recipients: EvmTssExpectedRecipient[] = (source?.recipients ?? []).map((recipient, i) => { + // Token identity reaches a recipient both flat (`tokenAddress` / `tokenName`) and inside the + // intent `tokenData` shape that `populateIntent` keeps. Both are declarations of the source + // that speaks for the user, so both stay in resolution; the stored intent only fills in when + // the source declared no identity of its own. + const declaredTokenIdentities: EvmTokenIdentityDeclaration[] = []; + if (recipient.tokenAddress !== undefined || recipient.tokenName !== undefined) { + declaredTokenIdentities.push({ tokenContractAddress: recipient.tokenAddress, tokenName: recipient.tokenName }); + } + if (recipient.tokenData?.tokenContractAddress !== undefined || recipient.tokenData?.tokenName !== undefined) { + declaredTokenIdentities.push({ ...recipient.tokenData }); + } + const storedTokenContractAddress = storedIntent?.recipients?.[i]?.tokenData?.tokenContractAddress; + if (storedTokenContractAddress !== undefined) { + declaredTokenIdentities.push({ tokenContractAddress: storedTokenContractAddress }); + } const tokenContractAddress = - recipient.tokenAddress ?? storedIntent?.recipients?.[i]?.tokenData?.tokenContractAddress; + recipient.tokenAddress ?? recipient.tokenData?.tokenContractAddress ?? storedTokenContractAddress; + const tokenName = recipient.tokenName ?? recipient.tokenData?.tokenName; return { address: recipient.address, amount: String(recipient.amount), ...(recipient.data !== undefined && { data: recipient.data }), - ...(recipient.tokenName !== undefined && { tokenName: recipient.tokenName }), + ...(tokenName !== undefined && { tokenName }), ...(tokenContractAddress !== undefined && { tokenContractAddress }), + ...(declaredTokenIdentities.length > 0 && { declaredTokenIdentities }), }; }); @@ -387,39 +470,19 @@ export function buildEvmTssIntentContext(params: { recipients, tokenName: source?.tokenName ?? storedIntent?.tokenName, coinTokenContractAddress: params.coinTokenContractAddress, + receiveAddress: source?.receiveAddress ?? storedIntent?.receiveAddress, 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 expected = { to: recipient.address, value: recipient.amount, data: normalizeCalldata(recipient.data) }; const decoded = { to: tx.to, value: tx.value, data: tx.data }; if (normalizeAddress(tx.to) !== normalizeAddress(recipient.address)) { warnings.push( @@ -450,7 +513,10 @@ function verifyHistoricPayment( ): TssFinding[] { const findings: TssFinding[] = []; const decoded = { to: tx.to, value: tx.value, data: tx.data }; - if (isEmptyCalldata(tx.data)) { + // Compare and decode the canonical form only: `''`, `'0x'`, mixed case and a dropped `0x` + // prefix all describe the same bytes, so none of them may skip the selector gate below. + const calldata = normalizeCalldata(tx.data); + if (calldata === '0x') { const expected = { to: recipient.address, value: recipient.amount, data: '0x' }; if (!valuesEqual(tx.value, recipient.amount)) { findings.push( @@ -470,20 +536,21 @@ function verifyHistoricPayment( } return findings; } - if (!normalizeCalldata(tx.data).startsWith(erc20TransferMethodId)) { + if (!calldata.startsWith(erc20TransferMethodId)) { return findings; } const [innerRecipient, innerAmount] = getRawDecoded( ['address', 'uint256'], - getBufferedByteCode(erc20TransferMethodId, tx.data) + getBufferedByteCode(erc20TransferMethodId, calldata) ); 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 declaredData = recipient.data === undefined ? undefined : normalizeCalldata(recipient.data); + let expected = { to: recipient.address, value: recipient.amount, data: declaredData }; + if (declaredData && declaredData.startsWith(erc20TransferMethodId)) { const [declaredRecipient, declaredAmount] = getRawDecoded( ['address', 'uint256'], - getBufferedByteCode(erc20TransferMethodId, recipient.data) + getBufferedByteCode(erc20TransferMethodId, declaredData) ); expected = { ...expected, to: toAddressString(declaredRecipient), value: toBigInt(declaredAmount).toString() }; } @@ -543,7 +610,7 @@ function verifyTokenTransfer( try { const [innerRecipient, innerAmount] = getRawDecoded( ['address', 'uint256'], - getBufferedByteCode(erc20TransferMethodId, tx.data) + getBufferedByteCode(erc20TransferMethodId, data) ); decodedRecipient = toAddressString(innerRecipient); decodedAmount = toBigInt(innerAmount); @@ -583,7 +650,7 @@ function verifyTokenTransfer( : confidentialTransferWithProofTypes; let decodedRecipient: string; try { - const [innerRecipient] = getRawDecoded(types, getBufferedByteCode(selector, tx.data)); + const [innerRecipient] = getRawDecoded(types, getBufferedByteCode(selector, data)); decodedRecipient = toAddressString(innerRecipient); } catch (e) { warnings.push( @@ -720,7 +787,7 @@ function verifyBatch(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { } let batch: BatchTransferData; try { - batch = decodeBatchTransferData(tx.data); + batch = decodeBatchTransferData(data); } catch (e) { warnings.push(violation(ctx, `batch calldata could not be decoded: ${(e as Error).message}`, expected, decoded)); return warnings; @@ -747,7 +814,7 @@ function verifyBatch(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { } let batch: BatchTransferFromData; try { - batch = decodeBatchTransferFromData(tx.data); + batch = decodeBatchTransferFromData(data); } catch (e) { warnings.push( violation(ctx, `batchTransferFrom calldata could not be decoded: ${(e as Error).message}`, expected, decoded) @@ -925,6 +992,7 @@ function verifyTransferToken(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFin function verifyTokenApproval(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { const warnings: TssFinding[] = []; const decoded = { to: tx.to, value: tx.value, data: tx.data }; + const calldata = normalizeCalldata(tx.data); const asset = resolveAsset(ctx, {}); const tokenContract = asset.kind === 'token' ? asset.contract : undefined; const expected = { to: tokenContract, value: '0', data: undefined }; @@ -952,7 +1020,7 @@ function verifyTokenApproval(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFin let spender: string; let amount: string; try { - const decodedApprove = decodeApproveCalldata(tx.data); + const decodedApprove = decodeApproveCalldata(calldata); spender = normalizeAddress(decodedApprove.spender); amount = decodedApprove.amount; } catch (e) { @@ -999,17 +1067,38 @@ function verifyTokenApproval(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFin return warnings; } -/** `fillNonce` handler — a 1-wei self-send to the wallet base address. */ +/** + * `fillNonce` handler — a 1-wei self-send to the address whose nonce is being filled: the + * declared `receiveAddress` (the wallet's chosen receive address) when the intent names one, + * otherwise 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)); + const expectedTo = ctx.receiveAddress ?? ctx.baseAddress; + const expected = { to: expectedTo, value: '1', data: '0x' }; + if (!expectedTo) { + warnings.push( + violation( + ctx, + 'fillNonce intent has no receive address or 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 (normalizeAddress(tx.to) !== normalizeAddress(expectedTo)) { + warnings.push( + violation( + ctx, + ctx.receiveAddress + ? 'fillNonce transaction must send to the declared receive address' + : '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)); @@ -1039,7 +1128,8 @@ export function verifyConsolidation(tx: DecodedEvmTx, baseAddress: string, sever const ctx = { intentType: 'consolidate' }; const expected = { to: baseAddress }; const decoded = { to: tx.to, value: tx.value }; - if (!normalizeCalldata(tx.data).startsWith(erc20TransferMethodId)) { + const calldata = normalizeCalldata(tx.data); + if (!calldata.startsWith(erc20TransferMethodId)) { return normalizeAddress(tx.to) === normalizeAddress(baseAddress) ? [] : [ @@ -1054,7 +1144,7 @@ export function verifyConsolidation(tx: DecodedEvmTx, baseAddress: string, sever } // 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 [innerRecipient] = getRawDecoded(['address', 'uint256'], getBufferedByteCode(erc20TransferMethodId, calldata)); const decodedRecipient = toAddressString(innerRecipient); if (decodedRecipient !== normalizeAddress(baseAddress)) { findings.push( @@ -1103,7 +1193,8 @@ export function evaluateEvmTssIntent(ctx: EvmTssIntentContext, tx: DecodedEvmTx) case 'defiDeposit': case 'defiWithdraw': case 'wrapApprove': - // Dedicated fail-closed calldata checks in verifyTssTransaction handle these. + // Caller-declared legs carry dedicated fail-closed calldata checks in verifyTssTransaction + // (stored defi-* intents are rejected there before those checks run). return []; default: return [unhandledTypeViolation(ctx, tx)]; diff --git a/modules/sdk-coin-eth/test/unit/eth.ts b/modules/sdk-coin-eth/test/unit/eth.ts index aa9df0ed083..24494b02463 100644 --- a/modules/sdk-coin-eth/test/unit/eth.ts +++ b/modules/sdk-coin-eth/test/unit/eth.ts @@ -7,6 +7,7 @@ import * as secp256k1 from 'secp256k1'; import request from 'superagent'; import { common, + DefiIntentParams, ErrorNoInputToRecover, generateRandomPassword, InvalidAddressError, @@ -15,6 +16,7 @@ import { RecoveryKeyMismatchError, RecoveryProviderError, resolveEffectiveTxParams, + TransactionParams, TransactionType, TxIntentMismatchRecipientError, UnexpectedAddressError, @@ -37,7 +39,7 @@ import { UnsignedSweepTxMPCv2, VerifyEthAddressOptions, } from '../../src'; -import { coins, Erc7984Coin, EthereumNetwork } from '@bitgo/statics'; +import { coins, ContractAddressDefinedToken, Erc7984Coin, EthereumNetwork } from '@bitgo/statics'; import assert from 'assert'; import { getBuilder } from './getBuilder'; import * as testData from '../resources/eth'; @@ -49,6 +51,9 @@ import { ethTssBackupKey } from './fixtures/ethTssBackupKey'; nock.enableNetConnect(); +// DeFi legs carry defiParams, which TransactionParams does not model; the verifier reads it. +type DeFiTxParams = TransactionParams & { defiParams: DefiIntentParams }; + describe('ETH:', function () { let bitgo: TestBitGoAPI; let hopTxBitgoSignature; @@ -67,7 +72,8 @@ describe('ETH:', function () { const buildDefiTxHex = async ( data: string, to: string = address1, - coinName: 'eth' | 'hteth' = 'hteth' + coinName: 'eth' | 'hteth' = 'hteth', + value?: string ): Promise => { const txBuilder = getBuilder(coinName) as TransactionBuilder; txBuilder.type(TransactionType.ContractCall); @@ -75,6 +81,34 @@ describe('ETH:', function () { txBuilder.counter(1); txBuilder.contract(to); txBuilder.data(data); + if (value) { + txBuilder.value(value); + } + return (await txBuilder.build()).toBroadcastFormat(); + }; + + // verifyTransaction's EVM prebuild is the fully typed abstract-eth shape, but the TSS verifier + // only reads txHex; the other required fields are inert defaults so the fixture stays typed. + const defiPrebuild = (txHex: string, coin: string) => ({ + txHex, + buildParams: { recipients: [] }, + recipients: [], + nextContractSequenceId: 0, + gasPrice: 10, + gasLimit: 100000, + isBatch: false, + coin, + }); + + // Real native send bytes. verifyTssTransaction rejects bytes it cannot decode, so flows that + // used to pass a placeholder '0x' now build the transaction they actually verify. + const buildNativeTxHex = async (to: string, value: string, coinName: 'eth' | 'hteth' = 'hteth'): Promise => { + const txBuilder = getBuilder(coinName) as TransactionBuilder; + txBuilder.type(TransactionType.SingleSigSend); + txBuilder.fee({ fee: '10', gasLimit: '21000' }); + txBuilder.counter(1); + txBuilder.contract(to); + txBuilder.value(value); return (await txBuilder.build()).toBroadcastFormat(); }; @@ -724,7 +758,7 @@ describe('ETH:', function () { // txPrebuild with consolidateId (set by server during consolidation build) const txPrebuild = { consolidateId: '68a7d5d0c66e74e216b97173bd558c6d', - txHex: '0x', + txHex: await buildNativeTxHex(baseAddress, '1000000000000000000'), coin: 'hteth', walletId: 'fakeWalletId', }; @@ -757,7 +791,7 @@ describe('ETH:', function () { }; const txPrebuild = { - txHex: '0x', + txHex: await buildNativeTxHex(baseAddress, '1000000000000000000'), coin: 'hteth', walletId: 'fakeWalletId', }; @@ -792,7 +826,7 @@ describe('ETH:', function () { }; const txPrebuild = { - txHex: '0x', + txHex: await buildNativeTxHex(baseAddress, '1000000000000000000'), coin: 'hteth', walletId: 'fakeWalletId', }; @@ -858,7 +892,7 @@ describe('ETH:', function () { }; const txPrebuild = { - txHex: '0x', + txHex: await buildNativeTxHex(baseAddress, '1'), coin: 'hteth', walletId: 'fakeWalletId', }; @@ -890,8 +924,10 @@ describe('ETH:', function () { walletPassphrase: 'fakeWalletPassphrase', }; + // fillNonce is exactly a 1 wei self-send to the base address, so the bytes are built, not + // faked: undecodable bytes are rejected before any handler runs. const txPrebuild = { - txHex: '0x', + txHex: await buildNativeTxHex(baseAddress, '1'), coin: 'hteth', walletId: 'fakeWalletId', }; @@ -1956,6 +1992,55 @@ describe('ETH:', function () { ); }); + it('should reject a DeFi leg that carries native value', async function () { + const coin = bitgo.coin('hteth') as Hteth; + const wallet = new Wallet(bitgo, coin, { coinSpecific: { baseAddress: address1 } }); + const verify = async (type: string, data: string) => { + const txParams: DeFiTxParams = { type, defiParams: { vaultId: 'vault', amount: '10' } }; + return coin.verifyTransaction({ + txParams, + txPrebuild: defiPrebuild(await buildDefiTxHex(data, address1, 'hteth', '1'), 'hteth'), + wallet, + verification: {}, + walletType: 'tss', + }); + }; + + // Native value on a vault calldata call pays whatever contract `to` names. + await expectDefiRejection( + verify('defiApprove', defiCalldata('approve(address,uint256)', [address2, '10'])), + /must not carry native value/ + ); + await expectDefiRejection( + verify('defiDeposit', defiCalldata('deposit(uint256,address)', ['10', address1])), + /must not carry native value/ + ); + await expectDefiRejection( + verify('defiWithdraw', defiCalldata('redeem(uint256,address,address)', ['10', address1, address1])), + /must not carry native value/ + ); + }); + + it('should anchor a defiApprove leg to the wallet token contract when statics knows it', async function () { + const tokenCoin = bitgo.coin('hteth:tusdc') as Erc20Token; + const tokenContract = String((coins.get('hteth:tusdc') as ContractAddressDefinedToken).contractAddress); + const wallet = new Wallet(bitgo, tokenCoin, { coinSpecific: { baseAddress: address1 } }); + // approve(vault, amount) on the token contract is the ERC-4626 approve leg. + const data = defiCalldata('approve(address,uint256)', [address2, '10']); + const txParams: DeFiTxParams = { type: 'defiApprove', defiParams: { vaultId: 'vault', amount: '10' } }; + const verify = async (to: string) => + tokenCoin.verifyTransaction({ + txParams, + txPrebuild: defiPrebuild(await buildDefiTxHex(data, to), 'hteth:tusdc'), + wallet, + verification: {}, + walletType: 'tss', + }); + + (await verify(tokenContract)).should.equal(true); + await expectDefiRejection(verify(address2), /must target the wallet token contract/); + }); + it('should verify TSS transaction with defiWithdraw type', async function () { const coin = bitgo.coin('hteth') as Hteth; const wallet = new Wallet(bitgo, coin, { coinSpecific: { baseAddress: address1 } }); diff --git a/modules/sdk-coin-evm/test/unit/tssVerification.ts b/modules/sdk-coin-evm/test/unit/tssVerification.ts index 249850516a2..567054ca5f0 100644 --- a/modules/sdk-coin-evm/test/unit/tssVerification.ts +++ b/modules/sdk-coin-evm/test/unit/tssVerification.ts @@ -1,8 +1,10 @@ import 'should'; +import { setTimeout as delay } from 'timers/promises'; import { BitGoAPI } from '@bitgo/sdk-api'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; import { BaseCoin as StaticsBaseCoin, coins, ContractAddressDefinedToken, Erc7984Coin } from '@bitgo/statics'; import { + ITransactionRecipient, PopulatedIntent, resolveEffectiveTxParams, TransactionParams, @@ -118,8 +120,16 @@ async function buildTxHex( 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 }; + // Caller params may also carry the EVM-only tokenName / receiveAddress that EthTransactionParams + // adds, and EVM recipients carry the intent tokenData identity next to the flat fields. + type SigningPathRecipient = ITransactionRecipient & { + tokenData?: { tokenType?: string; tokenQuantity?: string; tokenName?: string; tokenContractAddress?: string }; + }; + type SigningPathCallerParams = Omit & { + tokenName?: string; + receiveAddress?: string; + recipients?: SigningPathRecipient[]; + }; const verify = ( txParams: Record, @@ -233,6 +243,52 @@ async function buildTxHex( await verify(txParams, swappedHex, { strict: true }).should.be.rejectedWith(/calldata does not match/); }); + it('treats absent, empty and 0x calldata as the same empty payload', async function () { + // A caller that declares `''` (or omits data) is declaring a native send; the decoded + // bytes are `0x`. Format alone must not turn a matching payment into a mismatch. + const txHex = await build({ to: recipientAddress, value: '1000' }); + for (const data of [undefined, '', '0x']) { + warnings = []; + const recipient = { address: recipientAddress, amount: '1000', ...(data !== undefined ? { data } : {}) }; + (await verify({ type: 'transfer', recipients: [recipient] }, txHex)).should.equal(true); + warnings.should.have.length(0); + } + }); + + it('normalizes the calldata prefix and case before comparing', async function () { + const data = encodeErc20Transfer(recipientAddress, '1000'); + const txHex = await build({ to: tokenContractAddress, value: '0', data }); + for (const declared of [data, data.slice(2), `0x${data.slice(2).toUpperCase()}`, data.toUpperCase()]) { + warnings = []; + ( + await verify( + { type: 'transfer', recipients: [{ address: tokenContractAddress, amount: '0', data: declared }] }, + txHex + ) + ).should.equal(true); + warnings.should.have.length(0); + } + }); + + it('still runs the ERC-20 inner check on prefix-less declared calldata', async function () { + // Same bytes without `0x`: the selector gate must recognize them, so a swapped inner + // recipient stays the fatal check it is for prefixed calldata instead of becoming a + // calldata-format warning. + const declared = encodeErc20Transfer(attackerAddress, '1000').slice(2); + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '1000'), + }); + for (const strict of [false, true]) { + await verify( + { type: 'transfer', recipients: [{ address: tokenContractAddress, amount: '0', data: declared }] }, + txHex, + { strict } + ).should.be.rejectedWith(/destination address 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 }); @@ -371,6 +427,59 @@ async function buildTxHex( ); }); + it('resolves the caller recipient tokenData tokenContractAddress', async function () { + // Callers that use the intent recipient shape declare the token in `tokenData` rather + // than `tokenAddress`. It is the caller's own declaration, so it selects the token and + // the ERC-20 transfer is verified against it. + const caller = { + type: 'transfer', + recipients: [ + { + address: recipientAddress, + amount: '10000000', + tokenData: { + tokenType: 'ERC20', + tokenQuantity: '10000000', + tokenContractAddress: localTokenContractAddress, + }, + }, + ], + }; + const txHex = await build({ + to: localTokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '10000000'), + }); + (await verify(caller, txHex)).should.equal(true); + warnings.should.have.length(0); + + // A transaction paying another token is a token-contract mismatch, not an unverified + // native send: the declared identity now anchors the check. + const otherTokenHex = await build({ + to: serverTokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '10000000'), + }); + warnings = []; + (await verify(caller, otherTokenHex)).should.equal(true); + warnings.some((w) => /not the intent token contract/.test(w.reason)).should.equal(true); + await verify(caller, otherTokenHex, { strict: true }).should.be.rejectedWith(/not the intent token contract/); + }); + + it('resolves the caller recipient tokenData tokenName through statics', async function () { + const caller = { + type: 'transferToken', + recipients: [{ address: recipientAddress, amount: '1000', tokenData: { tokenName: localTokenName } }], + }; + const txHex = await build({ + to: localTokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '1000'), + }); + (await verify(caller, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + it('flags batch calldata carried by a single declared recipient', async function () { const txHex = await build({ to: tokenContractAddress, @@ -426,6 +535,108 @@ async function buildTxHex( await verifySigningPath(caller, intent, txHex, true).should.be.rejectedWith(/not the intent token contract/); }); + it('keeps the caller tokenData identity when the stored intent names another token', async function () { + const caller = { + type: 'transferToken', + recipients: [ + { + address: recipientAddress, + amount: '1000', + tokenData: { tokenContractAddress: localTokenContractAddress }, + }, + ], + }; + const intent = { + intentType: 'transferToken', + tokenName: serverTokenName, + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '1000', symbol: serverTokenName }, + tokenData: { tokenName: serverTokenName, tokenContractAddress: serverTokenContractAddress }, + }, + ], + }; + + // Paying the stored intent's token contradicts the caller's tokenData declaration. + const serverHex = await build({ to: serverTokenContractAddress, value: '0', data: tokenTransferData }); + (await verifySigningPath(caller, intent, serverHex)).should.equal(true); + warnings.some((w) => /not the intent token contract/.test(w.reason)).should.equal(true); + + // Paying the caller's own token verifies with nothing from the stored intent leaking in. + warnings = []; + const localHex = await build({ to: localTokenContractAddress, value: '0', data: tokenTransferData }); + (await verifySigningPath(caller, intent, localHex)).should.equal(true); + warnings.should.have.length(0); + (await verifySigningPath(caller, intent, localHex, true)).should.equal(true); + }); + + it('rejects caller tokenData identity that contradicts a flat tokenAddress', async function () { + // Two declarations of different tokens: the intended asset is unknown, so neither may + // silently win (as a precedence chain would). + const caller = { + type: 'transferToken', + recipients: [ + { + address: recipientAddress, + amount: '1000', + tokenAddress: localTokenContractAddress, + tokenData: { tokenContractAddress: serverTokenContractAddress }, + }, + ], + }; + const txHex = await build({ to: localTokenContractAddress, value: '0', data: tokenTransferData }); + + (await verifySigningPath(caller, {}, txHex)).should.equal(true); + warnings.some((w) => /name different token contracts/.test(w.reason)).should.equal(true); + await verifySigningPath(caller, {}, txHex, true).should.be.rejectedWith(/name different token contracts/); + }); + + it('rejects caller tokenData tokenName that contradicts its tokenContractAddress', async function () { + const caller = { + type: 'transferToken', + recipients: [ + { + address: recipientAddress, + amount: '1000', + tokenData: { tokenName: localTokenName, tokenContractAddress: serverTokenContractAddress }, + }, + ], + }; + const txHex = await build({ to: serverTokenContractAddress, value: '0', data: tokenTransferData }); + + (await verifySigningPath(caller, {}, txHex)).should.equal(true); + warnings.some((w) => /name different token contracts/.test(w.reason)).should.equal(true); + await verifySigningPath(caller, {}, txHex, true).should.be.rejectedWith(/name different token contracts/); + }); + + it('uses the stored tokenData tokenContractAddress when the caller declared nothing', async function () { + // The pending-approval effective params carry only `tokenName` (derived from + // `tokenData.tokenName` / the amount symbol); the stored `tokenData.tokenContractAddress` + // is the only contract anchor, so it must reach asset resolution. + const intent = { + intentType: 'transferToken', + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '1000', symbol: chain }, + tokenData: { tokenContractAddress: localTokenContractAddress }, + }, + ], + }; + const goodHex = await build({ to: localTokenContractAddress, value: '0', data: tokenTransferData }); + (await verifySigningPath(undefined, intent, goodHex)).should.equal(true); + warnings.should.have.length(0); + + warnings = []; + const otherTokenHex = await build({ to: serverTokenContractAddress, value: '0', data: tokenTransferData }); + (await verifySigningPath(undefined, intent, otherTokenHex)).should.equal(true); + warnings.some((w) => /not the intent token contract/.test(w.reason)).should.equal(true); + await verifySigningPath(undefined, intent, otherTokenHex, 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 = { @@ -657,26 +868,40 @@ async function buildTxHex( warnings.should.have.length(0); }); - it('leaves other types without recipients on the warn-first policy', async function () { - // The fail-closed rule is limited to caller-declared payments: a transferToken with no - // recipients keeps warning by default and keeps throwing only in strict mode. - const caller = { type: 'transferToken' }; + it('rejects a caller `transferToken` type with no recipients, both modes', async function () { + // A token payment leaves the same gap as a native one: with no recipients nothing + // anchors the bytes, and the stored intent would only supply the attacker's destination. const intent = { intentType: 'transferToken', tokenName: localTokenName, - recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: localTokenName } }], + recipients: [{ address: { address: attackerAddress }, amount: { value: '1000', symbol: localTokenName } }], }; const txHex = await build({ to: localTokenContractAddress, value: '0', - data: encodeErc20Transfer(recipientAddress, '1000'), + data: encodeErc20Transfer(attackerAddress, '1000'), }); + for (const strict of [false, true]) { + await verifySigningPath({ type: 'transferToken' }, intent, txHex, strict).should.be.rejectedWith( + /caller declared a token transfer without recipients/ + ); + } + warnings.should.have.length(0); + }); - (await verifySigningPath(caller, intent, txHex)).should.equal(true); - warnings.some((w) => /must declare exactly one token recipient/.test(w.reason)).should.equal(true); - await verifySigningPath(caller, intent, txHex, true).should.be.rejectedWith( - /must declare exactly one token recipient/ - ); + it('rejects the kebab-case `transfer-token` spelling with no recipients as well', async function () { + const intent = { + intentType: 'transferToken', + tokenName: localTokenName, + recipients: [{ address: { address: attackerAddress }, amount: { value: '1000', symbol: localTokenName } }], + }; + const txHex = await build({ to: localTokenContractAddress, value: '0' }); + for (const strict of [false, true]) { + await verifySigningPath({ type: 'transfer-token' }, intent, txHex, strict).should.be.rejectedWith( + /caller declared a token transfer without recipients/ + ); + } + warnings.should.have.length(0); }); }); }); @@ -775,7 +1000,7 @@ async function buildTxHex( }); describe('fillNonce handler', function () { - it('verifies a 1-wei self-send to the base address and flags deviations', async function () { + it('falls back to the base address when the intent declares no receive address', async function () { const good = await build({ to: baseAddress, value: '1' }); (await verify({ type: 'fillNonce' }, good)).should.equal(true); warnings.should.have.length(0); @@ -798,6 +1023,46 @@ async function buildTxHex( /must send to the wallet base address/ ); }); + + it('verifies a 1-wei self-send to the declared receive address, not the base address', async function () { + // The nonce that gets filled is the receive address's; the wallet base address is only + // the fallback when the intent names none. + const txParams = { type: 'fillNonce', receiveAddress: recipientAddress }; + const good = await build({ to: recipientAddress, value: '1' }); + (await verify(txParams, good)).should.equal(true); + warnings.should.have.length(0); + + const toBase = await build({ to: baseAddress, value: '1' }); + (await verify(txParams, toBase)).should.equal(true); + const destinationWarning = warnings.find((w) => /must send to the declared receive address/.test(w.reason)); + should.exist(destinationWarning); + destinationWarning?.expected.to?.toLowerCase().should.equal(recipientAddress.toLowerCase()); + await verify(txParams, toBase, { strict: true }).should.be.rejectedWith( + /must send to the declared receive address/ + ); + + warnings = []; + const wrongValue = await build({ to: recipientAddress, value: '2' }); + (await verify(txParams, wrongValue)).should.equal(true); + warnings.some((w) => /must send exactly 1 wei/.test(w.reason)).should.equal(true); + }); + + it('uses the stored receive address when the caller declared nothing (pending approval)', async function () { + // resolveEffectiveTxParams does not copy receiveAddress, so the stored intent is the only + // source of the address whose nonce is filled on this path. + const intent = { intentType: 'fillNonce', receiveAddress: recipientAddress }; + const txRequest = { txRequestId: 'pa-fill', walletId: 'w', intent } as unknown as TxRequest; + const txParams = resolveEffectiveTxParams(txRequest, undefined, chain) as unknown as Record; + + const good = await build({ to: recipientAddress, value: '1' }); + (await verify(txParams, good, { intent: intent as unknown as PopulatedIntent })).should.equal(true); + warnings.should.have.length(0); + + warnings = []; + const toBase = await build({ to: baseAddress, value: '1' }); + (await verify(txParams, toBase, { intent: intent as unknown as PopulatedIntent })).should.equal(true); + warnings.some((w) => /must send to the declared receive address/.test(w.reason)).should.equal(true); + }); }); describe('consolidation to the wallet base address', function () { @@ -868,6 +1133,42 @@ async function buildTxHex( }); }); + describe('undecodable bytes (fail closed before any handler)', function () { + it('rejects a placeholder txHex in both modes instead of warning', async function () { + for (const strict of [false, true]) { + await verify({ type: 'fillNonce' }, '0x', { strict }).should.be.rejectedWith( + /transaction bytes could not be decoded to verify intent/ + ); + } + }); + + it('rejects hex that is not a transaction for a handled and an unhandled type alike', async function () { + const cases: Array> = [ + { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, + { type: 'acceleration' }, + ]; + for (const txParams of cases) { + for (const strict of [false, true]) { + await verify(txParams, '0xdeadbeef', { strict }).should.be.rejectedWith( + /transaction bytes could not be decoded to verify intent/ + ); + } + } + }); + + it('rejects a missing txHex with the historical message', async function () { + await coin + .verifyTransaction({ + txParams: { type: 'fillNonce' }, + txPrebuild: {}, + wallet, + verification: {}, + walletType: 'tss', + }) + .should.be.rejectedWith('missing txHex in txPrebuild'); + }); + }); + describe('pending-approval path (stored intent via resolveEffectiveTxParams)', function () { const verifyPendingApproval = async ( intent: Record, @@ -917,40 +1218,48 @@ async function buildTxHex( 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', - }, + it('keeps verifying a caller-declared DeFi leg that matches defiParams', async function () { + // The stored flow is rejected below, but a caller that declares the leg itself still + // gets the calldata checks: the caller's amount and its intent type are its own. + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeApprove(attackerAddress, '123'), + }); + for (const strict of [false, true]) { + ( + await verify({ type: 'defiApprove', defiParams: { vaultId: 'vault', amount: '123' } }, txHex, { strict }) + ).should.equal(true); + } + }); + + it('rejects stored defi-* intents: no local vault binding can verify their calldata', async function () { + // A stored intent carries only vaultId and amount, and both come from the server. Nothing + // local names the vault contract or the approve spender, so comparing the bytes with the + // stored fields would only prove the server agrees with itself: fail closed instead. + const defiCases: Array<{ intentType: string; data: string; amountField: string }> = [ + { intentType: 'defi-approve', data: encodeApprove(attackerAddress, '123'), 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 = []; + for (const { intentType, data, amountField } of defiCases) { 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/ - ); + for (const strict of [false, true]) { + await verifyPendingApproval(intent, txHex, strict).should.be.rejectedWith( + /stored defi[A-Za-z]+ intent cannot be verified: no locally known vault contract/ + ); + } } + warnings.should.have.length(0); }); it('keeps rejecting stored defi-redeem intents, which have no handler, as master does', async function () { @@ -1006,6 +1315,119 @@ async function buildTxHex( await verify({ type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, txHex); warnings.should.have.length(0); }); + + // The sink is local client code, not trusted input: neither a synchronous throw nor a + // rejection may decide whether the user's key share signs, and warn mode never awaits it. + const verifyWithSink = async (sink: (warning: TssVerificationWarning) => void): Promise => { + const sinkBitgo = TestBitGo.decorate(BitGoAPI, { env: 'test', onVerificationWarning: sink }); + const sinkCoin = EvmCoin.createInstance(sinkBitgo, staticsCoin); + const sinkWallet = new Wallet(sinkBitgo, sinkCoin, { coinSpecific: { baseAddress } }); + // A new (non-fatal) finding, so the sink is exactly what this test exercises. + const txHex = await build({ to: attackerAddress, value: '0', data: '0xdeadbe00' }); + return sinkCoin.verifyTransaction({ + txParams: { + type: 'transfer', + recipients: [{ address: attackerAddress, amount: '0', data: '0xdeadbeef' }], + }, + txPrebuild: { txHex }, + wallet: sinkWallet, + verification: {}, + walletType: 'tss', + }); + }; + + it('keeps warn mode signing when the sink throws synchronously', async function () { + const result = await verifyWithSink(() => { + throw new Error('sink exploded'); + }); + result.should.equal(true); + }); + + it('keeps warn mode signing when the sink rejects, leaving no unhandled rejection', async function () { + const rejections: unknown[] = []; + const onRejection = (reason: unknown): void => { + rejections.push(reason); + }; + process.on('unhandledRejection', onRejection); + try { + const result = await verifyWithSink(async () => { + throw new Error('sink rejected'); + }); + result.should.equal(true); + // Give the event loop a turn: a floating rejection from the sink would surface now. + await delay(0); + rejections.should.have.length(0); + } finally { + process.removeListener('unhandledRejection', onRejection); + } + }); + + it('bounds calldata in the console warning but hands the full mismatch to the sink', async function () { + const longData = `0xdeadbeef${'ab'.repeat(600)}`; + const txHex = await build({ to: attackerAddress, value: '0', data: '0xdeadbe00' }); + const logged: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]): void => { + logged.push(args.map((arg) => String(arg)).join(' ')); + }; + try { + const result = await verify( + { type: 'transfer', recipients: [{ address: attackerAddress, amount: '0', data: longData }] }, + txHex + ); + result.should.equal(true); + } finally { + console.warn = originalWarn; + } + + const warning = warnings.find((w) => /calldata does not match/.test(w.reason)); + warning?.expected.data?.should.equal(longData); + const line = logged.find((entry) => /calldata does not match/.test(entry)); + line!.should.containEql('0xdeadbeef...(1210 chars)'); + line!.should.not.containEql(longData); + }); + + it('bounds the logged reason but gives the sink the full diagnostic', async function () { + // A reason can echo an unbounded value: an unresolvable local token identity is the one a + // valid transaction reaches deterministically (decoder errors can echo calldata too). The + // bytes are a real, matching ERC-20 transfer; only the local token name is unresolvable. + const longTokenName = `hteth:${'not-a-token-'.repeat(40)}`; + const txHex = await build({ + to: localTokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '1000'), + }); + const logged: string[][] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]): void => { + logged.push(args.map((arg) => String(arg))); + }; + let result = false; + try { + result = await verifySigningPath( + { + type: 'transferToken', + tokenName: longTokenName, + recipients: [{ address: recipientAddress, amount: '1000' }], + }, + { intentType: 'transferToken', recipients: [] }, + txHex + ); + } finally { + console.warn = originalWarn; + } + result.should.equal(true); + + const warning = warnings.find((w) => /cannot be resolved to a token contract/.test(w.reason)); + warning!.reason.length.should.be.greaterThan(256); + warning!.reason.should.containEql(longTokenName); + + const [loggedReason] = logged[0]; + loggedReason.should.containEql('...(truncated, '); + loggedReason.should.containEql(`, ${warning!.reason.length} chars)`); + loggedReason.should.not.containEql(longTokenName); + loggedReason.length.should.be.lessThan(warning!.reason.length); + }); }); }); }); diff --git a/modules/sdk-coin-flr/test/unit/flr.ts b/modules/sdk-coin-flr/test/unit/flr.ts index 602fd28d2be..e61330b1f76 100644 --- a/modules/sdk-coin-flr/test/unit/flr.ts +++ b/modules/sdk-coin-flr/test/unit/flr.ts @@ -4,7 +4,7 @@ import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; import { BitGoAPI } from '@bitgo/sdk-api'; import { coins } from '@bitgo/statics'; -import { Flr, Tflr } from '../../src/index'; +import { Flr, Tflr, TransactionBuilder } from '../../src/index'; import { ExplainTransactionOptions } from '../../src/iface'; import { UnsignedSweepTxMPCv2 } from '@bitgo/abstract-eth'; import { mockDataUnsignedSweep, mockDataNonBitGoRecovery, EXPORT_C_TEST_DATA } from '../resources'; @@ -31,6 +31,19 @@ describe('flr', function () { let hopExportTxBitgoSignature; const address2 = '0x7e85bdc27c050e3905ebf4b8e634d9ad6edd0de6'; + const tssBaseAddress = '0x174cfd823af8ce27ed0afee3fcf3c3ba259116be'; + + // verifyTssTransaction rejects bytes it cannot decode, so the TSS flows below build the + // transaction they verify instead of passing a placeholder '0x'. + const buildTflrNativeTxHex = async (to: string, value: string): Promise => { + const txBuilder = new TransactionBuilder(coins.get('tflr')); + txBuilder.type(TransactionType.SingleSigSend); + txBuilder.fee({ fee: '10', gasLimit: '21000' }); + txBuilder.counter(1); + txBuilder.contract(to); + txBuilder.value(value); + return (await txBuilder.build()).toBroadcastFormat(); + }; const hopExportTx = EXPORT_C_TEST_DATA.fullsigntxHex; const hopExportTxId = '0x' + getTxHash(hopExportTx).toString('hex'); @@ -460,7 +473,7 @@ describe('flr', function () { // txPrebuild with consolidateId (set by server during consolidation build) const txPrebuild = { consolidateId: '68a7d5d0c66e74e216b97173bd558c6d', - txHex: '0x', + txHex: await buildTflrNativeTxHex(tssBaseAddress, '1000000000000000000'), coin: 'tflr', walletId: 'fakeWalletId', }; @@ -491,7 +504,7 @@ describe('flr', function () { }; const txPrebuild = { - txHex: '0x', + txHex: await buildTflrNativeTxHex(tssBaseAddress, '1000000000000000000'), coin: 'tflr', walletId: 'fakeWalletId', }; @@ -522,7 +535,7 @@ describe('flr', function () { }; const txPrebuild = { - txHex: '0x', + txHex: await buildTflrNativeTxHex(tssBaseAddress, '1'), coin: 'tflr', walletId: 'fakeWalletId', }; @@ -552,8 +565,9 @@ describe('flr', function () { walletPassphrase: 'fakeWalletPassphrase', }; + // fillNonce is exactly a 1 wei self-send to the base address. const txPrebuild = { - txHex: '0x', + txHex: await buildTflrNativeTxHex(tssBaseAddress, '1'), coin: 'tflr', walletId: 'fakeWalletId', }; diff --git a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts index 9cdc6edf0ce..f1556a43706 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts @@ -31,9 +31,12 @@ export const NO_RECIPIENT_TX_TYPES = new Set([ 'customTx', // 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. + // WP intentType is kebab-case while buildParams.type is camelCase. This entry only + // waives the missing-recipients guard so a caller-declared (camelCase) leg reaches the + // DeFi calldata checks in verifyTssTransaction, which compare the amount and the + // in-calldata destination with defiParams. A stored (kebab-case) defi-* intent is + // rejected there instead: it carries only vaultId and amount from the server, and no + // local vault registry exists to anchor the target contract or the approve spender. 'defiApprove', 'defi-approve', 'defiDeposit', 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 fb615f12b6c..7c3a804ecba 100644 --- a/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts @@ -91,9 +91,9 @@ describe('recipientUtils', function () { }); 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. + // Stored defi-* intents have no recipients; the guard must not reject them here, so they + // reach verifyTssTransaction, which rejects a stored DeFi intent explicitly (no local + // vault binding to anchor the target contract or spender) instead of silently passing it. for (const intentType of ['defi-approve', 'defi-deposit', 'defi-withdraw']) { const txRequest = makeTxRequest({ intent: { intentType } as PopulatedIntent }); assert.doesNotThrow( From 0476c52a97de07ccb6bc93477c9077eb870e8a7c Mon Sep 17 00:00:00 2001 From: Deepak Jangid Date: Thu, 5 Nov 2026 10:04:27 +0530 Subject: [PATCH 4/4] fix: restore FLR export and NFT TSS verification Verify FLR C-to-P atomic exports without routing through the EVM decoder, checking chain direction, recipient and amount bounds. Validate ERC-721 and ERC-1155 transfers against declared token metadata on direct-send and pending-approval paths. Add regression coverage and document verification boundaries. Ticket: CECHO-2460 Co-authored-by: Ralph CLI Ralph: 18.13.2-bitgo.6 openrouter/deepseek/deepseek-v4.1-flash-1017@default:high+smol:high+task:high openrouter/anthropic/claude-opus-5.5@slow:high+plan:xhigh openrouter/openai/gpt-6.1-terra@vision:low+advisor:medium openrouter/z-ai/glm-5.3-flash@commit:high+memory:low openrouter/inclusionai/ling-3.1-flash@tiny:low openrouter/moonshotai/kimi-k3.1-pro@designer:high openrouter/google/gemini-3-pro-image@image web/exa@web openrouter/microsoft/mai-transcribe-2@dictation openrouter/typesafe/jev-1.14@judge --- modules/abstract-eth/README.md | 18 +- modules/abstract-eth/src/lib/walletUtil.ts | 2 + modules/abstract-eth/src/tssVerification.ts | 267 ++++++++++++++- .../sdk-coin-evm/test/unit/tssVerification.ts | 306 ++++++++++++++++++ modules/sdk-coin-flr/src/flr.ts | 224 ++++++++++++- modules/sdk-coin-flr/test/unit/flr.ts | 277 +++++++++++++++- 6 files changed, 1073 insertions(+), 21 deletions(-) diff --git a/modules/abstract-eth/README.md b/modules/abstract-eth/README.md index c65864ca754..31f08e66226 100644 --- a/modules/abstract-eth/README.md +++ b/modules/abstract-eth/README.md @@ -12,9 +12,11 @@ enforcement. Set `strictTssVerification: true` locally on `BitGoAPI` to reject e signing); server data never fills absent caller calldata or replaces the declared token. - Token identity comes from the recipient's `tokenAddress` / `tokenName` and `tokenData.tokenContractAddress` / `tokenData.tokenName`, the top-level `tokenName`, and the - coin instance. Conflicting or unresolvable declarations produce a finding; no declaration - silently overrides another. Payment shapes include native, ERC-20 / ERC-7984, and single-asset - batch transfers. Calldata comparisons canonicalize the hex prefix, case, and empty values. + coin instance. Conflicting or unresolvable declarations produce a finding. For declared NFTs, + a registered generic placeholder name of the same standard is display metadata only when an + explicit collection address is present; real collection names must still agree. Payment shapes + include native, ERC-20 / ERC-7984, and single-asset batch transfers. Calldata comparisons + canonicalize the hex prefix, case, and empty values. - Missing or undecodable transaction bytes and caller-declared `transfer` / `payment` / `transferToken` without recipients always reject, in either mode. Historic destination, value, ERC-20 recipient and amount checks, plus `consolidationToBaseAddress`, remain fatal. @@ -26,6 +28,16 @@ enforcement. Set `strictTssVerification: true` locally on `BitGoAPI` to reject e staking, wrap/unwrap, token enablement, and bridging; it never downgrades them to warnings. - A consolidation must pay the wallet base address, and an ERC-20 consolidation must not carry native value. +- `transferToken` supports ERC-721 and ERC-1155 when `tokenData` declares the standard, + collection, token ID, and ERC-1155 quantity. Verification checks the collection contract, zero + native value, decoded recipient, token ID, and quantity. ERC-1155 batch calldata must contain + exactly the single declared entry. NFT sends declaring raw calldata use exact-call verification. +- FLR MPCv2 C-chain-to-P-chain atomic exports use atomic decoding, not EVM decoding. They require + a declared export intent, decoded C-chain source and P-chain destination, one matching P-chain + recipient address set, and an exported nanoFLR amount equal to the declared wei amount converted + to nanoFLR plus an import fee within the network's configured minimum/maximum bounds. These + checks reject in either verification mode. Atomic imports still reach the parent EVM decoder + and reject; this export-specific path does not restore import signing. - `fillNonce` must send exactly 1 wei with no calldata to the declared `receiveAddress`, or to the wallet base address when none is declared. Stored intents supply this address only when the caller declared no intent of its own. diff --git a/modules/abstract-eth/src/lib/walletUtil.ts b/modules/abstract-eth/src/lib/walletUtil.ts index 23349874561..f4b91ecd235 100644 --- a/modules/abstract-eth/src/lib/walletUtil.ts +++ b/modules/abstract-eth/src/lib/walletUtil.ts @@ -24,6 +24,8 @@ export const flushERC1155ForwarderTokensMethodId = '0xe6bd0aa4'; export const flushERC1155ForwarderTokensMethodIdV4 = '0x8972c17c'; export const ERC721SafeTransferTypeMethodId = '0xb88d4fde'; +// Selector for the 3-argument safeTransferFrom(address,address,uint256) ERC-721 overload. +export const ERC721SafeTransferFromMethodId = '0x42842e0e'; export const ERC721TransferFromMethodId = '0x23b872dd'; export const ERC1155SafeTransferTypeMethodId = '0xf242432a'; export const ERC1155BatchTransferTypeMethodId = '0x2eb2c2d6'; diff --git a/modules/abstract-eth/src/tssVerification.ts b/modules/abstract-eth/src/tssVerification.ts index f3e5f1ccda0..2a159c60ce7 100644 --- a/modules/abstract-eth/src/tssVerification.ts +++ b/modules/abstract-eth/src/tssVerification.ts @@ -34,6 +34,15 @@ import { decodeApproveCalldata, decodeBatchTransferData, decodeBatchTransferFromData, + ERC1155BatchTransferTypeMethodId, + ERC1155BatchTransferTypes, + ERC1155SafeTransferTypeMethodId, + ERC1155SafeTransferTypes, + ERC721SafeTransferFromMethodId, + ERC721SafeTransferTypeMethodId, + ERC721SafeTransferTypes, + ERC721TransferFromMethodId, + ERC721TransferFromTypes, getBufferedByteCode, getRawDecoded, } from './lib'; @@ -116,6 +125,17 @@ export interface EvmTokenIdentityDeclaration { tokenContractAddress?: string; } +/** + * The NFT fields of the intent `tokenData` shape. They say which standard and which token the + * recipient moves; the contract identity itself stays in {@link EvmTokenIdentityDeclaration}, so + * `tokenType` / `tokenId` / `tokenQuantity` are never fed into asset resolution. + */ +interface EvmTokenDataDeclaration extends EvmTokenIdentityDeclaration { + tokenType?: string; + tokenId?: string; + tokenQuantity?: string; +} + /** One recipient of the user intent, flattened from txParams or the stored tx request intent. */ export interface EvmTssExpectedRecipient { address: string; @@ -130,6 +150,12 @@ export interface EvmTssExpectedRecipient { * declarations of different tokens stay ambiguous instead of one silently overriding the other. */ declaredTokenIdentities?: EvmTokenIdentityDeclaration[]; + /** `tokenData.tokenType`: the declared standard, which picks the calldata shape (`transferToken`). */ + tokenType?: string; + /** `tokenData.tokenId`: the declared NFT id (ERC-721 / ERC-1155). */ + tokenId?: string; + /** `tokenData.tokenQuantity`: the declared amount (ERC-1155, and ERC-721 must be 1 when set). */ + tokenQuantity?: string; } export interface EvmTssIntentContext { @@ -270,6 +296,17 @@ type PaymentAsset = { kind: 'native' } | { kind: 'token'; contract: string } | { const EVM_ADDRESS = /^0x[0-9a-fA-F]{40}$/; +/** + * Name prefixes of the statics placeholder entries each NFT standard uses when the collection is + * not itself registered (`erc721:token` / `terc721:token`, and the per-chain placeholders). Only a + * name of the recipient's own standard can be a label for its collection; any other name keeps its + * identity meaning. + */ +const NFT_DISPLAY_NAME_PREFIXES: Record<'ERC721' | 'ERC1155', RegExp> = { + ERC721: /^t?erc721:/i, + ERC1155: /^t?erc1155:/i, +}; + /** * Resolve the asset a payment is declared to move. Identity comes from the recipient's own * `tokenAddress` / `tokenName` / `tokenData` declarations, the top-level `tokenName`, and the @@ -277,6 +314,13 @@ const EVM_ADDRESS = /^0x[0-9a-fA-F]{40}$/; * 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. + * + * `nftTokenType` (set by the `transferToken` handler for a declared ERC-721 / ERC-1155) enables one + * narrow exception: a statics placeholder of that same standard (`tokenData.tokenName` is required + * by the intent schema, and `getTokenName` leaves `erc721:token` in place when the collection is + * unknown). Such a name is display metadata, so it is dropped from identity when the recipient also + * declared a real collection address. Nothing else is: an unregistered name still fails + * resolution, and a name that resolves to a real contract still contradicts the declared one. */ function resolveAsset( ctx: EvmTssIntentContext, @@ -284,9 +328,28 @@ function resolveAsset( tokenName?: string; tokenContractAddress?: string; declaredTokenIdentities?: EvmTokenIdentityDeclaration[]; - } + }, + nftTokenType?: 'ERC721' | 'ERC1155' ): PaymentAsset { const sources: { label: string; contract: string | undefined; native?: boolean }[] = []; + // Only a real collection address can anchor a declared NFT; without one the name is the only + // identity and must still resolve (or be reported unresolvable) exactly as for any token. + const nftContract = + nftTokenType && declared.tokenContractAddress && EVM_ADDRESS.test(declared.tokenContractAddress) + ? declared.tokenContractAddress + : undefined; + const nftDisplayName = nftTokenType ? NFT_DISPLAY_NAME_PREFIXES[nftTokenType] : undefined; + // True only for a statics placeholder of the declared standard whose contract is the + // `0xerc721:token`-style pseudo-address: `getTokenName` leaves `tokenData.tokenName` as a generic + // label when the collection is unknown, and the schema makes that field mandatory. An + // unregistered name here still expresses an unresolvable token, so it keeps failing resolution. + const isDisplayOnlyNftName = (name: string): boolean => { + if (!nftContract || !nftDisplayName?.test(name)) { + return false; + } + const contract = resolveTokenContractAddress(name); + return contract !== undefined && !EVM_ADDRESS.test(contract); + }; // Every identity the recipient declared is resolved, not just the first that happens to be // set: two declarations of different tokens must stay ambiguous rather than one silently // overriding the other. @@ -301,7 +364,7 @@ function resolveAsset( }); } const name = identity.tokenName; - if (!name) { + if (!name || isDisplayOnlyNftName(name)) { continue; } let staticsCoin; @@ -319,7 +382,7 @@ function resolveAsset( } } const declaredTopLevelName = ctx.tokenName; - if (declaredTopLevelName) { + if (declaredTopLevelName && !isDisplayOnlyNftName(declaredTopLevelName)) { let staticsCoin; try { staticsCoin = coins.get(declaredTopLevelName); @@ -368,7 +431,7 @@ function resolveAsset( } /** EVM recipients also carry the intent `tokenData` shape that `populateIntent` preserves. */ -type CallerDeclarableRecipient = ITransactionRecipient & { tokenData?: EvmTokenIdentityDeclaration }; +type CallerDeclarableRecipient = ITransactionRecipient & { tokenData?: EvmTokenDataDeclaration }; type CallerDeclarableParams = { type?: string; @@ -419,7 +482,9 @@ export function selectDeclaredIntent(params: { * params. Absent local `data` stays absent, a recipient's flat and `tokenData` token identities * are all kept (so conflicting declarations stay ambiguous), and only a source that declared no * identity of its own inherits the stored one. `receiveAddress` comes from that same source, so a - * caller's declared receive address is never replaced by the stored intent's. + * caller's declared receive address is never replaced by the stored intent's. The NFT fields of + * `tokenData` (`tokenType` / `tokenId` / `tokenQuantity`) follow the same precedence and are read + * from the stored intent directly when the effective params carry no `tokenData`. */ export function buildEvmTssIntentContext(params: { intentType: string; @@ -437,6 +502,10 @@ export function buildEvmTssIntentContext(params: { const source = storedIntent ? params.txParams : params.callerParams; const recipients: EvmTssExpectedRecipient[] = (source?.recipients ?? []).map((recipient, i) => { + // The effective params carry no `tokenData` on the pending-approval path, so the stored + // intent supplies it there. The NFT fields it carries (`tokenType` / `tokenId` / + // `tokenQuantity`) never take part in asset identity resolution. + const tokenData = recipient.tokenData ?? storedIntent?.recipients?.[i]?.tokenData; // Token identity reaches a recipient both flat (`tokenAddress` / `tokenName`) and inside the // intent `tokenData` shape that `populateIntent` keeps. Both are declarations of the source // that speaks for the user, so both stay in resolution; the stored intent only fills in when @@ -445,22 +514,28 @@ export function buildEvmTssIntentContext(params: { if (recipient.tokenAddress !== undefined || recipient.tokenName !== undefined) { declaredTokenIdentities.push({ tokenContractAddress: recipient.tokenAddress, tokenName: recipient.tokenName }); } - if (recipient.tokenData?.tokenContractAddress !== undefined || recipient.tokenData?.tokenName !== undefined) { - declaredTokenIdentities.push({ ...recipient.tokenData }); + if (tokenData?.tokenContractAddress !== undefined || tokenData?.tokenName !== undefined) { + declaredTokenIdentities.push({ + tokenContractAddress: tokenData.tokenContractAddress, + tokenName: tokenData.tokenName, + }); } const storedTokenContractAddress = storedIntent?.recipients?.[i]?.tokenData?.tokenContractAddress; if (storedTokenContractAddress !== undefined) { declaredTokenIdentities.push({ tokenContractAddress: storedTokenContractAddress }); } const tokenContractAddress = - recipient.tokenAddress ?? recipient.tokenData?.tokenContractAddress ?? storedTokenContractAddress; - const tokenName = recipient.tokenName ?? recipient.tokenData?.tokenName; + recipient.tokenAddress ?? tokenData?.tokenContractAddress ?? storedTokenContractAddress; + const tokenName = recipient.tokenName ?? tokenData?.tokenName; return { address: recipient.address, amount: String(recipient.amount), ...(recipient.data !== undefined && { data: recipient.data }), ...(tokenName !== undefined && { tokenName }), ...(tokenContractAddress !== undefined && { tokenContractAddress }), + ...(tokenData?.tokenType !== undefined && { tokenType: tokenData.tokenType }), + ...(tokenData?.tokenId !== undefined && { tokenId: tokenData.tokenId }), + ...(tokenData?.tokenQuantity !== undefined && { tokenQuantity: tokenData.tokenQuantity }), ...(declaredTokenIdentities.length > 0 && { declaredTokenIdentities }), }; }); @@ -682,6 +757,165 @@ function verifyTokenTransfer( return warnings; } +/** The `tokenData.tokenType` values with a dedicated calldata handler, or undefined for the rest. */ +function normalizeNftTokenType(tokenType?: string): 'ERC721' | 'ERC1155' | undefined { + const normalized = tokenType?.trim().toUpperCase(); + return normalized === 'ERC721' || normalized === 'ERC1155' ? normalized : undefined; +} + +/** One calldata shape an NFT standard accepts, with the ABI types needed to decode it. */ +interface NftTransferShape { + selector: string; + types: string[]; + /** `safeBatchTransferFrom`: the id and quantity are single-element arrays of the declared values. */ + batch?: boolean; +} + +/** The ERC-721 transfer signatures this handler accepts. */ +const ERC721_TRANSFER_SHAPES: NftTransferShape[] = [ + // `safeTransferFrom(address,address,uint256)`: the ERC-721 overload without the data parameter. + { selector: ERC721SafeTransferFromMethodId, types: ERC721TransferFromTypes }, + // `safeTransferFrom(address,address,uint256,bytes)`: the ERC-721 overload with the data parameter. + { selector: ERC721SafeTransferTypeMethodId, types: ERC721SafeTransferTypes }, + // `transferFrom(address,address,uint256)`: Hedera HTS NFTs only accept this form. + { selector: ERC721TransferFromMethodId, types: ERC721TransferFromTypes }, +]; + +/** The ERC-1155 transfer signatures this handler accepts (single entry or one-entry batch). */ +const ERC1155_TRANSFER_SHAPES: NftTransferShape[] = [ + { selector: ERC1155SafeTransferTypeMethodId, types: ERC1155SafeTransferTypes }, + { selector: ERC1155BatchTransferTypeMethodId, types: ERC1155BatchTransferTypes, batch: true }, +]; + +/** + * R3b — one recipient that declares an NFT (`tokenData.tokenType`): `to` must be the collection + * contract, `value` must be zero, and the decoded transfer must move the declared token id (and, + * for ERC-1155, quantity) to the declared destination. Only the signatures listed for the + * declared standard are accepted; a selector of the other standard, or an unknown one, is a + * violation. The sender (`_from`) is not compared: it is the wallet or one of its forwarders, and + * the intent never names it. + */ +function verifyNftTokenTransfer( + ctx: EvmTssIntentContext, + tx: DecodedEvmTx, + recipient: EvmTssExpectedRecipient, + tokenContract: string, + tokenType: 'ERC721' | 'ERC1155' +): TssFinding[] { + const findings: TssFinding[] = []; + const data = normalizeCalldata(tx.data); + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + const expected = { to: tokenContract, value: '0', data: recipient.data }; + // Findings that are about the token id / quantity still name the destination pair, so the + // report says which transfer was wrong without relabelling an id as a native amount. + const expectedTransfer = { ...expected, to: recipient.address }; + const isErc721 = tokenType === 'ERC721'; + + if (!valuesEqual(tx.value, '0')) { + findings.push(violation(ctx, 'token transfer must not carry native value', expected, decoded)); + } + if (normalizeAddress(tx.to) !== normalizeAddress(tokenContract)) { + findings.push(violation(ctx, 'transaction to is not the intent token contract', expected, decoded)); + } + + const shape = (isErc721 ? ERC721_TRANSFER_SHAPES : ERC1155_TRANSFER_SHAPES).find( + (candidate) => candidate.selector === data.slice(0, 10) + ); + if (!shape) { + findings.push( + violation( + ctx, + `${tokenType} token transfer must use ${ + isErc721 + ? 'safeTransferFrom(address,address,uint256[,bytes]) or transferFrom(address,address,uint256)' + : 'safeTransferFrom(address,address,uint256,uint256,bytes) or safeBatchTransferFrom(address,address,uint256[],uint256[],bytes)' + } calldata`, + expected, + decoded + ) + ); + return findings; + } + + const tokenId = recipient.tokenId; + const tokenQuantity = recipient.tokenQuantity; + if (tokenId === undefined) { + findings.push(violation(ctx, `${tokenType} token transfer must declare tokenData.tokenId`, expected, decoded)); + return findings; + } + if (!isErc721 && tokenQuantity === undefined) { + findings.push(violation(ctx, 'ERC1155 token transfer must declare tokenData.tokenQuantity', expected, decoded)); + return findings; + } + if (isErc721 && tokenQuantity !== undefined && !valuesEqual(tokenQuantity, '1')) { + findings.push(violation(ctx, 'ERC721 token transfer must declare tokenData.tokenQuantity of 1', expected, decoded)); + } + + let decodedRecipient: string; + let decodedIds: string[]; + let decodedQuantities: string[] | undefined; + try { + // The selector is already matched at offset 0, so decode exactly the arguments after it. + // `getBufferedByteCode` splits the calldata on the selector string, which is wrong here: an + // argument (a tokenId is 32 bytes) may legitimately contain the selector's bytes. + const raw = getRawDecoded(shape.types, Buffer.from(data.slice(10), 'hex')); + decodedRecipient = toAddressString(raw[1]); + if (shape.batch) { + decodedIds = (raw[2] as unknown[]).map((id) => toBigInt(id).toString()); + decodedQuantities = (raw[3] as unknown[]).map((quantity) => toBigInt(quantity).toString()); + } else { + decodedIds = [toBigInt(raw[2]).toString()]; + decodedQuantities = isErc721 ? undefined : [toBigInt(raw[3]).toString()]; + } + } catch (e) { + findings.push( + violation(ctx, `${tokenType} transfer calldata could not be decoded: ${(e as Error).message}`, expected, decoded) + ); + return findings; + } + + const decodedTransfer = { ...decoded, to: addHexPrefix(decodedRecipient) }; + if (normalizeAddress(decodedRecipient) !== normalizeAddress(recipient.address)) { + findings.push( + violation(ctx, 'destination address does not match with the recipient address', expectedTransfer, decodedTransfer) + ); + } + + if (shape.batch && (decodedIds.length !== 1 || decodedQuantities?.length !== 1)) { + findings.push( + violation( + ctx, + `ERC1155 batch transfer must move exactly the declared tokenId and tokenQuantity (decoded ` + + `${decodedIds.length} ids / ${decodedQuantities?.length ?? 0} quantities)`, + expectedTransfer, + decodedTransfer + ) + ); + return findings; + } + if (!valuesEqual(decodedIds[0], tokenId)) { + findings.push( + violation( + ctx, + `${tokenType} tokenId in calldata does not match tokenData.tokenId (expected ${tokenId}, decoded ${decodedIds[0]})`, + expectedTransfer, + decodedTransfer + ) + ); + } + if (!isErc721 && decodedQuantities !== undefined && !valuesEqual(decodedQuantities[0], tokenQuantity)) { + findings.push( + violation( + ctx, + `ERC1155 tokenQuantity in calldata does not match tokenData.tokenQuantity (expected ${tokenQuantity}, decoded ${decodedQuantities[0]})`, + expectedTransfer, + decodedTransfer + ) + ); + } + return findings; +} + /** * 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 @@ -941,7 +1175,11 @@ function verifyPaymentShape( ]; } -/** `transferToken` handler — same shape set as R3, exactly one recipient, which must move a token. */ +/** + * `transferToken` handler — exactly one recipient, which must move a token. A recipient that + * declares `tokenData.tokenType` ERC-721 / ERC-1155 is verified against that standard's transfer + * calldata (R3b); every other token keeps the R3 ERC-20 / ERC-7984 shapes. + */ function verifyTransferToken(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { const decoded = { to: tx.to, value: tx.value, data: tx.data }; if (ctx.recipients.length !== 1) { @@ -955,7 +1193,11 @@ function verifyTransferToken(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFin ]; } const recipient = ctx.recipients[0]; - const asset = resolveAsset(ctx, recipient); + // A declared NFT standard picks its own calldata set. `tokenData.tokenName` is display metadata + // only when it is that standard's statics placeholder, so the declared collection contract (or a + // name that resolves to a real one) anchors the transfer. + const nftTokenType = normalizeNftTokenType(recipient.tokenType); + const asset = resolveAsset(ctx, recipient, nftTokenType); if (asset.kind === 'unresolved') { return [ violation(ctx, `transferToken ${asset.reason}`, { to: recipient.address, value: recipient.amount }, decoded), @@ -971,6 +1213,9 @@ function verifyTransferToken(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFin ), ]; } + if (nftTokenType) { + return verifyNftTokenTransfer(ctx, tx, recipient, asset.contract, nftTokenType); + } if (isEmptyCalldata(tx.data)) { return [ violation( diff --git a/modules/sdk-coin-evm/test/unit/tssVerification.ts b/modules/sdk-coin-evm/test/unit/tssVerification.ts index 567054ca5f0..68f63f6c636 100644 --- a/modules/sdk-coin-evm/test/unit/tssVerification.ts +++ b/modules/sdk-coin-evm/test/unit/tssVerification.ts @@ -76,6 +76,45 @@ function encodeConfidentialTransferNoProof(to: string): string { return encodeWithAbi('confidentialTransfer', ['address', 'bytes32'], [to, '0x' + '11'.repeat(32)]); } +// ERC-721 / ERC-1155 collection used by the transferToken NFT tests, plus the standard transfer +// calldata the verifier accepts: safeTransferFrom with and without the data parameter and +// transferFrom for ERC-721, safeTransferFrom and safeBatchTransferFrom for ERC-1155. +const nftContractAddress = '0x0000000000000000000000000000000000000abc'; +// A registered ERC-721 whose name resolves to a real, different contract than nftContractAddress. +const otherNftName = 'erc721:witch'; + +function encodeErc721SafeTransferFrom(to: string, tokenId: string): string { + return encodeWithAbi('safeTransferFrom', ['address', 'address', 'uint256'], [baseAddress, to, tokenId]); +} + +function encodeErc721SafeTransferFromWithData(to: string, tokenId: string): string { + return encodeWithAbi( + 'safeTransferFrom', + ['address', 'address', 'uint256', 'bytes'], + [baseAddress, to, tokenId, Buffer.alloc(0)] + ); +} + +function encodeErc721TransferFrom(to: string, tokenId: string): string { + return encodeWithAbi('transferFrom', ['address', 'address', 'uint256'], [baseAddress, to, tokenId]); +} + +function encodeErc1155SafeTransferFrom(to: string, tokenId: string, quantity: string): string { + return encodeWithAbi( + 'safeTransferFrom', + ['address', 'address', 'uint256', 'uint256', 'bytes'], + [baseAddress, to, tokenId, quantity, Buffer.alloc(0)] + ); +} + +function encodeErc1155SafeBatchTransferFrom(to: string, tokenIds: string[], quantities: string[]): string { + return encodeWithAbi( + 'safeBatchTransferFrom', + ['address', 'address', 'uint256[]', 'uint256[]', 'bytes'], + [baseAddress, to, tokenIds, quantities, Buffer.alloc(0)] + ); +} + async function buildTxHex( coin: Readonly, opts: { to: string; value?: string; data?: string } @@ -492,6 +531,273 @@ async function buildTxHex( }); }); + // ERC-721 / ERC-1155 transferToken: the recipient names the destination and the tokenData + // standard / id / quantity / collection, while the bytes go to the collection with the + // standard's transfer calldata. + describe('transferToken handler (ERC-721 / ERC-1155)', function () { + const erc721Recipient = (tokenId: string, tokenQuantity = '1'): Record => ({ + address: recipientAddress, + amount: '0', + tokenData: { + tokenType: 'ERC721', + tokenQuantity, + tokenId, + tokenContractAddress: nftContractAddress, + }, + }); + const erc1155Recipient = (tokenId: string, tokenQuantity: string): Record => ({ + address: recipientAddress, + amount: '0', + tokenData: { + tokenType: 'ERC1155', + tokenQuantity, + tokenId, + tokenContractAddress: nftContractAddress, + }, + }); + const nftTx = (data: string, overrides: { to?: string; value?: string } = {}): Promise => + build({ to: overrides.to ?? nftContractAddress, value: overrides.value ?? '0', data }); + + it('verifies the ERC-721 safeTransferFrom and transferFrom signatures', async function () { + const txParams = { type: 'transferToken', recipients: [erc721Recipient('7')] }; + for (const data of [ + encodeErc721SafeTransferFrom(recipientAddress, '7'), + encodeErc721SafeTransferFromWithData(recipientAddress, '7'), + encodeErc721TransferFrom(recipientAddress, '7'), + ]) { + warnings = []; + const txHex = await nftTx(data); + (await verify(txParams, txHex)).should.equal(true); + warnings.should.have.length(0); + (await verify(txParams, txHex, { strict: true })).should.equal(true); + } + }); + + it('verifies a tokenId whose 32-byte encoding contains the selector bytes', async function () { + // The arguments are decoded from the fixed position after the matched selector, so an id + // that happens to encode the selector's own bytes (0x42842e0e / 0xb88d4fde) is legitimate. + const selectorId = BigInt('0x42842e0e').toString(); + const dataSelectorId = BigInt('0xb88d4fde').toString(); + const cases: Array<{ tokenId: string; data: string }> = [ + { tokenId: selectorId, data: encodeErc721SafeTransferFrom(recipientAddress, selectorId) }, + { tokenId: dataSelectorId, data: encodeErc721SafeTransferFromWithData(recipientAddress, dataSelectorId) }, + ]; + for (const { tokenId, data } of cases) { + warnings = []; + const txParams = { type: 'transferToken', recipients: [erc721Recipient(tokenId)] }; + const txHex = await nftTx(data); + (await verify(txParams, txHex)).should.equal(true); + warnings.should.have.length(0); + (await verify(txParams, txHex, { strict: true })).should.equal(true); + } + }); + + it('verifies ERC-1155 single and one-entry batch transfers', async function () { + const txParams = { type: 'transferToken', recipients: [erc1155Recipient('7', '3')] }; + for (const data of [ + encodeErc1155SafeTransferFrom(recipientAddress, '7', '3'), + encodeErc1155SafeBatchTransferFrom(recipientAddress, ['7'], ['3']), + ]) { + warnings = []; + const txHex = await nftTx(data); + (await verify(txParams, txHex)).should.equal(true); + warnings.should.have.length(0); + (await verify(txParams, txHex, { strict: true })).should.equal(true); + } + }); + + it('flags an NFT sent to a swapped destination (warn, throw in strict mode)', async function () { + const txParams = { type: 'transferToken', recipients: [erc721Recipient('7')] }; + const txHex = await nftTx(encodeErc721SafeTransferFrom(attackerAddress, '7')); + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /destination address does not match/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/destination address does not match/); + }); + + it('flags a swapped ERC-721 tokenId and an inflated ERC-1155 quantity', async function () { + const erc721 = { type: 'transferToken', recipients: [erc721Recipient('7')] }; + const wrongIdHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '8')); + (await verify(erc721, wrongIdHex)).should.equal(true); + warnings.some((w) => /tokenId in calldata does not match/.test(w.reason)).should.equal(true); + await verify(erc721, wrongIdHex, { strict: true }).should.be.rejectedWith(/tokenId in calldata does not match/); + + warnings = []; + const erc1155 = { type: 'transferToken', recipients: [erc1155Recipient('7', '3')] }; + const wrongQuantityHex = await nftTx(encodeErc1155SafeTransferFrom(recipientAddress, '7', '4')); + (await verify(erc1155, wrongQuantityHex)).should.equal(true); + warnings.some((w) => /tokenQuantity in calldata does not match/.test(w.reason)).should.equal(true); + await verify(erc1155, wrongQuantityHex, { strict: true }).should.be.rejectedWith( + /tokenQuantity in calldata does not match/ + ); + }); + + it('flags an ERC-1155 batch that moves more than the single declared entry', async function () { + const erc1155 = { type: 'transferToken', recipients: [erc1155Recipient('7', '3')] }; + const twoEntries = ['7', '8']; + const twoEntryQuantities = ['3', '1']; + const twoEntriesHex = await nftTx( + encodeErc1155SafeBatchTransferFrom(recipientAddress, twoEntries, twoEntryQuantities) + ); + (await verify(erc1155, twoEntriesHex)).should.equal(true); + warnings + .some((w) => /must move exactly the declared tokenId and tokenQuantity/.test(w.reason)) + .should.equal(true); + await verify(erc1155, twoEntriesHex, { strict: true }).should.be.rejectedWith( + /must move exactly the declared tokenId and tokenQuantity/ + ); + }); + + it('flags another standard selector, native value, a foreign contract and a missing tokenId', async function () { + const erc721 = { type: 'transferToken', recipients: [erc721Recipient('7')] }; + // The declared standard decides: ERC-1155 calldata is not an ERC-721 transfer. + const otherStandardHex = await nftTx(encodeErc1155SafeTransferFrom(recipientAddress, '7', '1')); + (await verify(erc721, otherStandardHex)).should.equal(true); + warnings.some((w) => /ERC721 token transfer must use/.test(w.reason)).should.equal(true); + + warnings = []; + const nativeValueHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '7'), { value: '5' }); + (await verify(erc721, nativeValueHex)).should.equal(true); + warnings.some((w) => /must not carry native value/.test(w.reason)).should.equal(true); + + warnings = []; + const otherContractHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '7'), { + to: tokenContractAddress, + }); + (await verify(erc721, otherContractHex)).should.equal(true); + warnings.some((w) => /not the intent token contract/.test(w.reason)).should.equal(true); + + warnings = []; + const noTokenId = { + type: 'transferToken', + recipients: [ + { + address: recipientAddress, + amount: '0', + tokenData: { tokenType: 'ERC721', tokenQuantity: '1', tokenContractAddress: nftContractAddress }, + }, + ], + }; + (await verify(noTokenId, await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '7')))).should.equal(true); + warnings.some((w) => /must declare tokenData.tokenId/.test(w.reason)).should.equal(true); + }); + + it('verifies a stored NFT intent whose tokenData tokenName is a generic display label', async function () { + // The intent schema makes tokenData.tokenName mandatory, and for an unsupported collection + // it is only a label ('erc721:token' -> the '0xerc721:token' statics placeholder). The + // declared contract is the identity, so the legitimate transfer must verify cleanly. + const intent = { + intentType: 'transferToken', + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '0', symbol: 'erc721:token' }, + tokenData: { + tokenType: 'ERC721', + tokenQuantity: '1', + tokenId: '9', + tokenName: 'erc721:token', + tokenContractAddress: nftContractAddress, + }, + }, + ], + }; + const txHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '9')); + (await verifySigningPath(undefined, intent, txHex)).should.equal(true); + warnings.should.have.length(0); + (await verifySigningPath(undefined, intent, txHex, true)).should.equal(true); + }); + + it('flags an unregistered or other-standard NFT tokenName instead of treating it as a label', async function () { + // Only a statics placeholder of the declared standard is a display label. Any other name is + // an identity declaration: unregistered it cannot be resolved, and a placeholder of the + // other standard contradicts the declared collection. + const callerFor = (tokenName: string): Record => ({ + type: 'transferToken', + recipients: [ + { + address: recipientAddress, + amount: '0', + tokenData: { + tokenType: 'ERC721', + tokenQuantity: '1', + tokenId: '7', + tokenName, + tokenContractAddress: nftContractAddress, + }, + }, + ], + }); + const txHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '7')); + + const unknownName = callerFor('hteth:not-a-registered-collection'); + (await verify(unknownName, txHex)).should.equal(true); + warnings.some((w) => /cannot be resolved to a token contract/.test(w.reason)).should.equal(true); + await verify(unknownName, txHex, { strict: true }).should.be.rejectedWith( + /cannot be resolved to a token contract/ + ); + + warnings = []; + const otherStandardName = callerFor('erc1155:token'); + (await verify(otherStandardName, txHex)).should.equal(true); + warnings.some((w) => /name different token contracts/.test(w.reason)).should.equal(true); + await verify(otherStandardName, txHex, { strict: true }).should.be.rejectedWith( + /name different token contracts/ + ); + }); + + it('flags a stored NFT intent whose tokenName names another collection', async function () { + // A name that resolves to a real, different collection is still a conflict: only the + // generic statics placeholder of the declared standard is a display label. + const intent = { + intentType: 'transferToken', + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '0', symbol: otherNftName }, + tokenData: { + tokenType: 'ERC721', + tokenQuantity: '1', + tokenId: '9', + tokenName: otherNftName, + tokenContractAddress: nftContractAddress, + }, + }, + ], + }; + const txHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '9')); + (await verifySigningPath(undefined, intent, txHex)).should.equal(true); + warnings.some((w) => /name different token contracts/.test(w.reason)).should.equal(true); + await verifySigningPath(undefined, intent, txHex, true).should.be.rejectedWith( + /name different token contracts/ + ); + }); + + it('enforces the stored tokenData tokenId on the pending-approval path', async function () { + const intent = { + intentType: 'transferToken', + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '0', symbol: 'erc721:token' }, + tokenData: { + tokenType: 'ERC721', + tokenQuantity: '1', + tokenId: '9', + tokenName: 'erc721:token', + tokenContractAddress: nftContractAddress, + }, + }, + ], + }; + const txHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '10')); + (await verifySigningPath(undefined, intent, txHex)).should.equal(true); + warnings.some((w) => /tokenId in calldata does not match/.test(w.reason)).should.equal(true); + await verifySigningPath(undefined, intent, txHex, true).should.be.rejectedWith( + /tokenId in calldata does not match/ + ); + }); + }); + describe('declared intent isolation and asset binding (CECHO-2460)', function () { const tokenTransferData = encodeErc20Transfer(recipientAddress, '1000'); const approvalData = encodeApprove(attackerAddress, MAX_UINT256); diff --git a/modules/sdk-coin-flr/src/flr.ts b/modules/sdk-coin-flr/src/flr.ts index 908700b2913..737b5dd0e68 100644 --- a/modules/sdk-coin-flr/src/flr.ts +++ b/modules/sdk-coin-flr/src/flr.ts @@ -26,9 +26,10 @@ import { Recipient, SignableTransaction, TransactionExplanation, + TransactionType, Entry, } from '@bitgo/sdk-core'; -import { BaseCoin as StaticsBaseCoin, coins } from '@bitgo/statics'; +import { BaseCoin as StaticsBaseCoin, coins, FlareNetwork } from '@bitgo/statics'; import { AbstractEthLikeNewCoins, optionalDeps, @@ -36,6 +37,7 @@ import { UnsignedSweepTxMPCv2, RecoverOptions, OfflineVaultTxInfo, + VerifyEthTransactionOptions, } from '@bitgo/abstract-eth'; import { TransactionBuilder } from './lib'; import { FlrPLib } from '@bitgo/sdk-coin-flrp'; @@ -57,6 +59,14 @@ import { */ interface AtomicTransactionExplanation extends TransactionExplanation { inputs: Entry[]; + /** Decoded transaction type, set by the atomic builder's explainTransaction. */ + type?: TransactionType; +} + +/** The user intent a TSS transaction must satisfy, taken from the one source that speaks for it. */ +interface FlrDeclaredTssIntent { + type?: string; + recipients: Array<{ address: string; amount: string }>; } export class Flr extends AbstractEthLikeNewCoins { @@ -136,6 +146,16 @@ export class Flr extends AbstractEthLikeNewCoins { return false; } + /** + * Decode atomic transaction bytes with the atomic builder. + * @param {string} txHex - the transaction hex + * @returns {Promise} the built atomic transaction + * @private + */ + private async getAtomicTransaction(txHex: string): Promise { + return this.getAtomicBuilder().from(txHex).build(); + } + /** * Explains an atomic transaction using atomic builder. * @param {string} txHex - the transaction hex @@ -143,8 +163,7 @@ export class Flr extends AbstractEthLikeNewCoins { * @private */ private async explainAtomicTransaction(txHex: string): Promise { - const txBuilder = this.getAtomicBuilder().from(txHex); - const tx = await txBuilder.build(); + const tx = await this.getAtomicTransaction(txHex); return tx.explainTransaction() as AtomicTransactionExplanation; } @@ -155,8 +174,7 @@ export class Flr extends AbstractEthLikeNewCoins { * @private */ private async verifySignatureForAtomicTransaction(txHex: string): Promise { - const txBuilder = this.getAtomicBuilder().from(txHex); - const tx = await txBuilder.build(); + const tx = await this.getAtomicTransaction(txHex); const payload = tx.signablePayload; const signatures = tx.signature.map((s) => Buffer.from(FlrPLib.Utils.removeHexPrefix(s), 'hex')); const recoverPubkey = signatures.map((s) => FlrPLib.Utils.recoverySignature(payload, s)); @@ -312,6 +330,202 @@ export class Flr extends AbstractEthLikeNewCoins { return true; } + /** + * Verify a TSS transaction against the intent the user approved. + * + * A C-chain -> P-chain export is signed as an Avalanche atomic transaction, which the EVM + * transaction decoder cannot parse, so it is verified here before the parent verifier runs. + * Every other FLR TSS transaction keeps the parent's EVM intent verification unchanged. + * + * @param {VerifyEthTransactionOptions} params + * @returns {Promise} + */ + async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise { + const intent = Flr.resolveDeclaredTssIntent(params); + const declaredExport = Flr.isExportIntent(intent); + if (!Flr.isAtomicTransactionHex(params.txPrebuild?.txHex)) { + if (declaredExport) { + // Exporting from the C-chain is only ever an atomic transaction: bytes that are not atomic + // can never satisfy a declared export, and no warn / strict policy may downgrade that. + throw new Error('declared export intent requires an atomic transaction in txPrebuild.txHex'); + } + return super.verifyTssTransaction(params); + } + if (!declaredExport) { + // Other atomic formats (import / importtoc) keep the parent baseline: this verifier owns + // only the C-chain -> P-chain export shape, so their policy is not decided here. + return super.verifyTssTransaction(params); + } + return this.verifyAtomicExportTssTransaction(params, intent); + } + + /** + * Verify the Avalanche atomic export bytes about to be signed against the declared intent. + * + * The amount follows the Export convention already used by {@link validateHopPrebuild}: the + * declared C-chain amount is in wei, the P-chain output is in nanoFLR, and the output must cover + * the declared amount plus the import fee, up to the network's maximum import fee. + * + * @param {VerifyEthTransactionOptions} params + * @param {FlrDeclaredTssIntent} intent - the intent resolved by the caller + * @returns {Promise} + * @private + */ + private async verifyAtomicExportTssTransaction( + params: VerifyEthTransactionOptions, + intent: FlrDeclaredTssIntent + ): Promise { + const { txPrebuild, wallet } = params; + if (!wallet || !txPrebuild?.txHex) { + throw new Error('missing params'); + } + + let tx: BaseTransaction; + let explained: AtomicTransactionExplanation; + try { + tx = await this.getAtomicTransaction(txPrebuild.txHex); + explained = tx.explainTransaction() as AtomicTransactionExplanation; + } catch (e) { + throw new Error(`transaction bytes could not be decoded to verify intent: ${(e as Error).message}`); + } + + if (explained.type !== TransactionType.Export) { + throw new Error(`atomic transaction type ${String(explained.type)} does not match the declared export intent`); + } + // Explanations map every owner onto this coin's P-chain alias, so matching the output address + // does not prove the export targets the P-chain. Read the chains encoded in the bytes. + const { sourceChain, destinationChain } = (tx as FlrPLib.Transaction).toJson(); + if (sourceChain !== 'C' || destinationChain !== 'P') { + throw new Error( + `atomic export must be a C-chain to P-chain export but encodes ${sourceChain} -> ${destinationChain}` + ); + } + const outputs = explained.outputs; + if (outputs.length !== 1) { + throw new Error(`atomic export transaction must have exactly 1 output but ${outputs.length} found`); + } + if (intent.recipients.length !== 1) { + throw new Error(`atomic export requires exactly 1 recipient but ${intent.recipients.length} found`); + } + + const declaredRecipient = intent.recipients[0]; + const declaredDestination = Flr.normalizePChainAddressSet(declaredRecipient.address); + if (!declaredDestination) { + throw new Error(`atomic export recipient ${declaredRecipient.address} is not a valid P-chain address`); + } + const decodedDestination = Flr.normalizePChainAddressSet(outputs[0].address); + if (!decodedDestination) { + throw new Error(`atomic export destination ${outputs[0].address} is not a valid P-chain address`); + } + if (declaredDestination !== decodedDestination) { + throw new Error( + `atomic export destination ${outputs[0].address} does not match the declared recipient ${declaredRecipient.address}` + ); + } + + this.assertAtomicExportAmount(declaredRecipient.amount, explained.outputAmount); + return true; + } + + /** + * Resolve the intent that speaks for the user, with the same precedence as the parent verifier: + * the caller's own params when it declared a type or recipients, otherwise the stored tx request + * intent. `txParams` cannot answer this because the linking path back-fills it from the stored + * intent, so `callerTxParams` (the untouched caller input) decides. + * + * @param {VerifyEthTransactionOptions} params + * @returns {FlrDeclaredTssIntent} + * @private + */ + private static resolveDeclaredTssIntent(params: VerifyEthTransactionOptions): FlrDeclaredTssIntent { + const callerParams = params.callerTxParams ?? (params.intent ? undefined : params.txParams); + const callerDeclaresIntent = !!(callerParams?.type || callerParams?.recipients?.length); + const storedIntent = params.intent && !callerDeclaresIntent ? params.intent : undefined; + + if (storedIntent) { + return { + type: storedIntent.intentType, + recipients: (storedIntent.recipients ?? []).map((recipient) => ({ + address: recipient.address.address, + amount: String(recipient.amount.value), + })), + }; + } + + const declared = callerParams ?? params.txParams; + return { + type: declared?.type, + recipients: (declared?.recipients ?? []).map((recipient) => ({ + address: recipient.address, + amount: String(recipient.amount), + })), + }; + } + + /** True when the declared intent type selects an export, in either the caller or stored spelling. */ + private static isExportIntent(intent: FlrDeclaredTssIntent): boolean { + return (intent.type ?? '').trim().toLowerCase() === 'export'; + } + + /** + * Avalanche atomic transactions are identified by their codec prefix (0x0000), the same marker + * used by {@link explainTransaction} and {@link isSignablePreHashed}. + */ + private static isAtomicTransactionHex(txHex?: string): boolean { + const hex = txHex?.startsWith('0x') ? txHex.slice(2) : txHex; + return !!hex && hex.startsWith('0000'); + } + + /** + * Normalize a P-chain address (a single address or a `~`-separated multisig set) so address + * order and case cannot make two descriptions of the same destination compare unequal. + * + * @param {string} address - the P-chain address or address set + * @returns {string | undefined} the normalized set, undefined when any address is invalid + * @private + */ + private static normalizePChainAddressSet(address?: string): string | undefined { + const addresses = (address ?? '') + .split('~') + .map((part) => part.trim()) + .filter((part) => !!part); + if (!addresses.length || !addresses.every((part) => FlrPLib.Utils.isValidAddress(part))) { + return undefined; + } + return addresses + .map((part) => part.toLowerCase()) + .sort() + .join('~'); + } + + /** + * Check that the atomic export output covers the declared C-chain amount plus an import fee. + * + * The declared amount is in wei and the export output is in nanoFLR (the same wei -> nanoFLR + * conversion {@link validateHopPrebuild} applies). The output may not exceed the declared amount + * plus the network's maximum import fee, which is also the bound Flrp.validateExportTx applies. + * + * @param {string} declaredAmount - the C-chain amount declared by the user, in wei + * @param {string} outputAmount - the amount the atomic export pays on the P-chain, in nanoFLR + * @private + */ + private assertAtomicExportAmount(declaredAmount: string, outputAmount?: string): void { + const flrpNetwork = coins.get(this.getFlrP()).network as FlareNetwork; + const amount = new BigNumber(declaredAmount); + const output = new BigNumber(outputAmount ?? ''); + if (!amount.isFinite() || amount.isNegative() || !output.isFinite() || output.isNegative()) { + throw new Error(`atomic export amount ${declaredAmount} or output amount ${outputAmount} is not a valid amount`); + } + const expectedOutput = amount.div(1e9); + const minOutput = expectedOutput.plus(flrpNetwork.minImportToPFee); + const maxOutput = expectedOutput.plus(flrpNetwork.maxImportFee); + if (output.isLessThan(minOutput) || output.isGreaterThan(maxOutput)) { + throw new Error( + `atomic export output amount ${outputAmount} is outside the expected range [${minOutput.toFixed()}, ${maxOutput.toFixed()}] for the declared amount ${declaredAmount}` + ); + } + } + /** * Check if an address is a FLR C-chain address (0x format) * @param {string} address - the address to check diff --git a/modules/sdk-coin-flr/test/unit/flr.ts b/modules/sdk-coin-flr/test/unit/flr.ts index e61330b1f76..5a34da88d08 100644 --- a/modules/sdk-coin-flr/test/unit/flr.ts +++ b/modules/sdk-coin-flr/test/unit/flr.ts @@ -2,11 +2,12 @@ import * as should from 'should'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; import { BitGoAPI } from '@bitgo/sdk-api'; -import { coins } from '@bitgo/statics'; +import { coins, FlareNetwork } from '@bitgo/statics'; import { Flr, Tflr, TransactionBuilder } from '../../src/index'; -import { ExplainTransactionOptions } from '../../src/iface'; +import { ExplainTransactionOptions, FlrTransactionParams } from '../../src/iface'; import { UnsignedSweepTxMPCv2 } from '@bitgo/abstract-eth'; +import { FlrPLib } from '@bitgo/sdk-coin-flrp'; import { mockDataUnsignedSweep, mockDataNonBitGoRecovery, EXPORT_C_TEST_DATA } from '../resources'; import nock from 'nock'; import { common, TransactionType, Wallet } from '@bitgo/sdk-core'; @@ -647,6 +648,278 @@ describe('flr', function () { }); isTransactionVerified.should.equal(true); }); + + describe('TSS atomic export', function () { + // The export fixture is a real C -> P atomic export whose P-chain output is + // EXPORT_C_TEST_DATA.amount nanoFLR. The legitimate C-chain request that produces it + // exports that output minus the minimum P-chain import fee, in wei. + const minImportToPFee = (coins.get('tflrp').network as FlareNetwork).minImportToPFee; + const declaredAmount = ( + (BigInt(EXPORT_C_TEST_DATA.amount) - BigInt(minImportToPFee)) * + BigInt(1000000000) + ).toString(); + const exportPrebuild = { + txHex: EXPORT_C_TEST_DATA.unsignedTxHex, + coin: 'tflr', + walletId: 'fakeWalletId', + }; + const pAddress = EXPORT_C_TEST_DATA.pAddresses[0]; + const singlePAddress = EXPORT_C_TEST_DATA.pAddresses[1]; + + const verifyTss = ( + txParams: FlrTransactionParams, + txPrebuild: { txHex: string } & Record, + extra: Record = {} + ) => + tflrCoin.verifyTransaction({ + txParams, + txPrebuild, + wallet: new Wallet(bitgo, tflrCoin, { + coinSpecific: { baseAddress: EXPORT_C_TEST_DATA.cHexAddress }, + }), + walletType: 'tss', + ...extra, + }); + + it('should verify a legitimate TSS atomic export', async function () { + const verified = await verifyTss( + { + type: 'Export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + exportPrebuild + ); + verified.should.equal(true); + }); + + it('should verify a legitimate TSS atomic export declared with the multisig set sorted', async function () { + const verified = await verifyTss( + { + type: 'export', + recipients: [ + { + address: EXPORT_C_TEST_DATA.pAddresses.slice().sort().join('~'), + amount: declaredAmount, + }, + ], + }, + exportPrebuild + ); + verified.should.equal(true); + }); + + it('should verify a legitimate TSS atomic export from the stored pending-approval intent', async function () { + // Pending approval: the caller supplies no txParams, resolveEffectiveTxParams has already + // folded the stored intent into txParams, and the stored intent is the source of truth. + const intent = { + intentType: 'export', + recipients: [ + { + address: { address: EXPORT_C_TEST_DATA.pMultisigAddress }, + amount: { value: declaredAmount, symbol: 'tflr' }, + }, + ], + }; + const verified = await verifyTss( + { + type: 'export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + exportPrebuild, + { callerTxParams: undefined, intent } + ); + verified.should.equal(true); + }); + + it('should reject an atomic export whose destination is not the declared recipient', async function () { + await verifyTss( + { type: 'Export', recipients: [{ address: singlePAddress, amount: declaredAmount }] }, + exportPrebuild + ).should.be.rejectedWith(/does not match the declared recipient/); + }); + + it('should reject an atomic export from the stored intent whose destination is not the declared recipient', async function () { + const intent = { + intentType: 'export', + recipients: [{ address: { address: singlePAddress }, amount: { value: declaredAmount, symbol: 'tflr' } }], + }; + await verifyTss( + { + type: 'export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + exportPrebuild, + { callerTxParams: undefined, intent } + ).should.be.rejectedWith(/does not match the declared recipient/); + }); + + it('should let the caller-declared intent win over a stored export intent', async function () { + const intent = { + intentType: 'export', + recipients: [ + { + address: { address: EXPORT_C_TEST_DATA.pMultisigAddress }, + amount: { value: declaredAmount, symbol: 'tflr' }, + }, + ], + }; + await verifyTss( + { + type: 'export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + exportPrebuild, + { + callerTxParams: { type: 'Export', recipients: [{ address: singlePAddress, amount: declaredAmount }] }, + intent, + } + ).should.be.rejectedWith(/does not match the declared recipient/); + }); + + it('should reject an atomic export that exports less than the declared amount', async function () { + await verifyTss( + { + type: 'Export', + recipients: [ + { + address: EXPORT_C_TEST_DATA.pMultisigAddress, + amount: (BigInt(declaredAmount) * BigInt(2)).toString(), + }, + ], + }, + exportPrebuild + ).should.be.rejectedWith(/outside the expected range/); + }); + + it('should reject an atomic export that exports more than the declared amount plus the maximum import fee', async function () { + await verifyTss( + { type: 'Export', recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: '1000000' }] }, + exportPrebuild + ).should.be.rejectedWith(/outside the expected range/); + }); + + it('should reject an atomic export with more than one declared recipient', async function () { + await verifyTss( + { + type: 'Export', + recipients: [ + { address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }, + { address: pAddress, amount: '1' }, + ], + }, + exportPrebuild + ).should.be.rejectedWith(/exactly 1 recipient/); + }); + + it('should reject atomic bytes declared as a payment instead of an export', async function () { + // Wrongly labelled atomic bytes must fail closed before the warn / strict policy. + await verifyTss( + { + type: 'transfer', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + exportPrebuild + ).should.be.rejected(); + }); + + it('should reject a C-chain export tx labelled with an import intent', async function () { + // These are Export bytes declared as an import: the type is wrong, so nothing here may + // verify them. Import / ImportToC policy is not decided by this verifier, so the request + // keeps the parent baseline and fails closed on bytes the EVM decoder cannot read. + await verifyTss( + { + type: 'import', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + exportPrebuild + ).should.be.rejectedWith(/could not be decoded to verify intent/); + }); + + it('should reject an atomic export whose destination chain is not the P-chain', async function () { + // The fixture's destinationChain field holds the 32-byte P-chain ID. Replacing it with + // the C-chain ID keeps the destination address and amount identical, so only the chain + // check can catch it: explanations format every owner with this coin's P-chain alias. + // Layout: 10-byte header, 32-byte C-chain blockchainId, then 32-byte destinationChain. + const destinationChainOffset = 84; + const cChainIdHex = FlrPLib.Utils.cb58Decode( + (coins.get('tflrp').network as FlareNetwork).cChainBlockchainID + ).toString('hex'); + const exportTxHex = FlrPLib.Utils.removeHexPrefix(EXPORT_C_TEST_DATA.unsignedTxHex); + const misdirectedExportHex = + '0x' + + exportTxHex.slice(0, destinationChainOffset) + + cChainIdHex + + exportTxHex.slice(destinationChainOffset + cChainIdHex.length); + + await verifyTss( + { + type: 'Export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + { txHex: misdirectedExportHex, coin: 'tflr', walletId: 'fakeWalletId' } + ).should.be.rejectedWith(/must be a C-chain to P-chain export/); + }); + + it('should reject a declared export backed by non-atomic transaction bytes', async function () { + const evmTxHex = await buildTflrNativeTxHex(tssBaseAddress, '1'); + await verifyTss( + { + type: 'Export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + { txHex: evmTxHex, coin: 'tflr', walletId: 'fakeWalletId' } + ).should.be.rejectedWith(/declared export intent requires an atomic transaction/); + }); + + it('should reject a declared export that only supplies the prehashed signable digest', async function () { + // The MPCv1 signing path passes signableHex (SHA-256 of the atomic body) as txHex because + // v1 has no prehashed signable support. The serialized bytes are required to verify the + // intent, so a digest-only export fails closed instead of being signed unverified. + await verifyTss( + { + type: 'Export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + { txHex: 'a'.repeat(64), coin: 'tflr', walletId: 'fakeWalletId' } + ).should.be.rejectedWith(/declared export intent requires an atomic transaction/); + }); + + it('should reject undecodable atomic bytes', async function () { + await verifyTss( + { + type: 'Export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + { txHex: '0x0000deadbeef', coin: 'tflr', walletId: 'fakeWalletId' } + ).should.be.rejectedWith(/could not be decoded to verify intent/); + }); + + it('should keep ordinary EVM TSS transfers on the parent verifier', async function () { + const verified = await verifyTss( + { + type: 'transfer', + recipients: [{ address: tssBaseAddress, amount: '100000000000000000' }], + }, + { + txHex: await buildTflrNativeTxHex(tssBaseAddress, '100000000000000000'), + coin: 'tflr', + walletId: 'fakeWalletId', + } + ); + verified.should.equal(true); + }); + + it('should keep rejecting EVM TSS transfers whose destination is not the declared recipient', async function () { + await verifyTss( + { type: 'transfer', recipients: [{ address: address2, amount: '100000000000000000' }] }, + { + txHex: await buildTflrNativeTxHex(tssBaseAddress, '100000000000000000'), + coin: 'tflr', + walletId: 'fakeWalletId', + } + ).should.be.rejected(); + }); + }); }); describe('Hop export tx verify', () => {