From ddc2c6e55fda72abdb67cf5afcd9ce1ad4389f9e Mon Sep 17 00:00:00 2001 From: Deepak Jangid Date: Mon, 2 Nov 2026 17:53:29 +0530 Subject: [PATCH 1/2] fix(abstract-eth): verify EVM TSS payments against declared asset Preserve caller-declared intent and reject unresolved or contradictory asset and calldata combinations. Ticket: CECHO-2460 --- .../src/abstractEthLikeNewCoins.ts | 42 +- modules/abstract-eth/src/tssVerification.ts | 374 +++++++++++++----- .../sdk-core/src/bitgo/baseCoin/iBaseCoin.ts | 12 + .../src/bitgo/utils/tss/ecdsa/ecdsa.ts | 2 + .../src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts | 2 + 5 files changed, 332 insertions(+), 100 deletions(-) diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index 8de9fe4c693..0b20a509082 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -59,11 +59,14 @@ import { import { getDerivationPath } from '@bitgo/sdk-lib-mpc'; import { buildEvmTssIntentContext, + callerDeclaresIntent, canonicalizeEvmTssIntentType, DecodedEvmTx, evaluateEvmTssIntent, isUnsupportedEvmTssIntentType, mapDefiParamsFromIntent, + resolveCoinTokenContractAddress, + selectDeclaredIntent, } from './tssVerification'; import { bip32 } from '@bitgo/secp256k1'; import { @@ -3580,6 +3583,15 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise { 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 => { @@ -3599,7 +3611,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem !( txParams.prebuildTx?.consolidateId || txPrebuild?.consolidateId || - intent?.consolidateId || + storedIntent?.consolidateId || txParams.stakingRequestId || txParams.prebuildTx?.stakingRequestId || (txParams.type && NO_RECIPIENT_TX_TYPES.has(txParams.type)) @@ -3615,13 +3627,21 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem throw new Error('tx cannot be both a batch and hop transaction'); } - // Canonical verifier intent type. Caller-supplied txParams.type wins; on the - // pending-approval path resolveEffectiveTxParams stores the raw intent type in - // txParams.type, so the stored spelling (payment, defi-approve, …) is normalized here. - const explicitType = - canonicalizeEvmTssIntentType(txParams.type) || canonicalizeEvmTssIntentType(intent?.intentType); - const consolidateId = - txParams.prebuildTx?.consolidateId || txPrebuild?.consolidateId || intent?.consolidateId || undefined; + // 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'); @@ -3706,7 +3726,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem } const baseAddress = wallet.coinSpecific()?.baseAddress; - const defiParams = txParams.defiParams ?? (intent ? mapDefiParamsFromIntent(intent) : undefined); + const defiParams = txParams.defiParams ?? (storedIntent ? mapDefiParamsFromIntent(storedIntent) : undefined); if (!baseAddress || !defiParams) { await throwRecipientMismatch('DeFi transaction is missing required intent parameters', []); return false; @@ -3846,7 +3866,9 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem const ctx = buildEvmTssIntentContext({ intentType: canonicalType, txParams, - intent, + callerParams, + storedIntent, + coinTokenContractAddress: resolveCoinTokenContractAddress(this.getChain()), baseAddress: typeof baseAddress === 'string' ? baseAddress : undefined, batcherAddress: ethNetwork?.batcherContractAddress, }); diff --git a/modules/abstract-eth/src/tssVerification.ts b/modules/abstract-eth/src/tssVerification.ts index e48bd954e3e..8151ac40ffc 100644 --- a/modules/abstract-eth/src/tssVerification.ts +++ b/modules/abstract-eth/src/tssVerification.ts @@ -71,6 +71,8 @@ const UNSUPPORTED_EVM_INTENT_TYPES = new Set(['contractCall', 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', @@ -110,8 +112,14 @@ export interface EvmTssIntentContext { /** Canonical verifier intent type (after {@link canonicalizeEvmTssIntentType}). */ intentType: string; recipients: EvmTssExpectedRecipient[]; - /** Token name from the stored intent or txParams (token sends / tokenApproval). */ + /** 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`). */ @@ -201,55 +209,168 @@ export function resolveTokenContractAddress(tokenName?: string): string | undefi } /** - * Build the handler context from the caller txParams and the optional stored tx request - * intent. Recipient precedence mirrors `resolveEffectiveTxParams`: explicit txParams - * recipients win, intent recipients fill in, and intent-only token fields are merged. + * 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; - txParams: { tokenName?: string; recipients?: ITransactionRecipient[] }; - intent?: PopulatedIntent; + /** 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 { intent, txParams } = params; - const intentRecipients: EvmTssExpectedRecipient[] | undefined = intent?.recipients?.map((recipient) => ({ - address: recipient.address.address, - amount: String(recipient.amount.value), - ...(recipient.data !== undefined && { data: recipient.data }), - ...(recipient.tokenData?.tokenName !== undefined && { tokenName: recipient.tokenData.tokenName }), - ...(recipient.tokenData?.tokenContractAddress !== undefined && { - tokenContractAddress: recipient.tokenData.tokenContractAddress, - }), - })); - const paramsRecipients: EvmTssExpectedRecipient[] | undefined = txParams.recipients?.map((recipient) => ({ - address: recipient.address, - amount: String(recipient.amount), - ...(recipient.data !== undefined && { data: recipient.data }), - ...(recipient.tokenName !== undefined && { tokenName: recipient.tokenName }), - ...(recipient.tokenAddress !== undefined && { tokenContractAddress: recipient.tokenAddress }), - })); - - const primary = paramsRecipients?.length ? paramsRecipients : intentRecipients ?? []; - const secondary = paramsRecipients?.length ? intentRecipients ?? [] : []; - const recipients = primary.map((recipient, i) => { - const extra = secondary[i]; - if (!extra) { - return recipient; - } + 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 { - ...recipient, - ...(recipient.data === undefined && extra.data !== undefined && { data: extra.data }), - ...(recipient.tokenName === undefined && extra.tokenName !== undefined && { tokenName: extra.tokenName }), - ...(recipient.tokenContractAddress === undefined && - extra.tokenContractAddress !== undefined && { tokenContractAddress: extra.tokenContractAddress }), + 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: intent?.tokenName ?? txParams.tokenName, + tokenName: source?.tokenName ?? storedIntent?.tokenName, + coinTokenContractAddress: params.coinTokenContractAddress, baseAddress: params.baseAddress, batcherAddress: params.batcherAddress, }; @@ -276,12 +397,6 @@ export function mapDefiParamsFromIntent(intent: PopulatedIntent): DefiIntentPara }; } -function tokenContractForRecipient(ctx: EvmTssIntentContext, recipient: EvmTssExpectedRecipient): string | undefined { - return ( - recipient.tokenContractAddress ?? resolveTokenContractAddress(recipient.tokenName ?? ctx.tokenName) ?? undefined - ); -} - /** * R1 — one recipient that declares raw calldata (WalletConnect / dApp): `to`, `value` and * `data` must each be exactly equal to the intent. @@ -322,17 +437,6 @@ function verifyNativeSend( const warnings: TssVerificationWarning[] = []; const expected = { to: recipient.address, value: recipient.amount, data: '0x' }; const decoded = { to: tx.to, value: tx.value, data: tx.data }; - if (recipient.tokenName || recipient.tokenContractAddress) { - warnings.push( - violation( - ctx, - 'token recipient cannot be verified against a native transfer: intent declares a token', - expected, - decoded - ) - ); - return warnings; - } if (normalizeAddress(tx.to) !== normalizeAddress(recipient.address)) { warnings.push(violation(ctx, 'destination address does not match with the recipient address', expected, decoded)); } @@ -353,23 +457,21 @@ function verifyNativeSend( function verifyTokenTransfer( ctx: EvmTssIntentContext, tx: DecodedEvmTx, - recipient: EvmTssExpectedRecipient + recipient: EvmTssExpectedRecipient, + tokenContract: string ): TssVerificationWarning[] { const warnings: TssVerificationWarning[] = []; 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: recipient.tokenContractAddress, value: '0', data: recipient.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)); } - const tokenContract = tokenContractForRecipient(ctx, recipient); - if (tokenContract && normalizeAddress(tx.to) !== normalizeAddress(tokenContract)) { - warnings.push( - violation(ctx, 'transaction to is not the intent token contract', { ...expected, to: tokenContract }, decoded) - ); + if (normalizeAddress(tx.to) !== normalizeAddress(tokenContract)) { + warnings.push(violation(ctx, 'transaction to is not the intent token contract', expected, decoded)); } if (selector === erc20TransferMethodId) { @@ -480,6 +582,31 @@ function verifyBatch(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssVerificatio 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 }[], @@ -517,6 +644,17 @@ function verifyBatch(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssVerificatio }; 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); @@ -533,6 +671,17 @@ function verifyBatch(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssVerificatio } 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); @@ -542,15 +691,12 @@ function verifyBatch(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssVerificatio ); return warnings; } - const tokenContract = resolveTokenContractAddress( - ctx.recipients.find((r) => r.tokenName)?.tokenName ?? ctx.tokenName - ); - if (tokenContract && normalizeAddress(batch.tokenContractAddress) !== normalizeAddress(tokenContract)) { + if (normalizeAddress(batch.tokenContractAddress) !== normalizeAddress(batchAsset.contract)) { warnings.push( violation( ctx, 'batchTransferFrom token contract does not match the intent token contract', - { ...expected, to: tokenContract }, + { ...expected, to: batchAsset.contract }, { ...decoded, to: batch.tokenContractAddress } ) ); @@ -590,25 +736,54 @@ function verifyPayment(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssVerificat if (recipient.data !== undefined) { return verifyExactCall(ctx, tx, recipient); } - if (isEmptyCalldata(tx.data)) { - return verifyNativeSend(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); - if (selector === erc20TransferMethodId) { - return verifyTokenTransfer(ctx, tx, recipient); - } - if (selector === confidentialTransferNoProofMethodId || selector === confidentialTransferWithProofMethodId) { - return verifyTokenTransfer(ctx, tx, recipient); - } - if (selector === batchMethodId || selector === batchTransferFromMethodId) { - return [ - violation( - ctx, - 'batch calldata cannot be verified against a single declared recipient', - { to: recipient.address, value: recipient.amount }, - decoded - ), - ]; + const isTokenShape = + selector === erc20TransferMethodId || + selector === confidentialTransferNoProofMethodId || + selector === confidentialTransferWithProofMethodId; + const isBatchShape = selector === batchMethodId || selector === batchTransferFromMethodId; + if (asset.kind === 'native') { + if (isEmptyCalldata(tx.data)) { + return verifyNativeSend(ctx, tx, recipient); + } + 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); + } + 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 [ @@ -622,7 +797,7 @@ function verifyPayment(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssVerificat ]; } -/** `transferToken` handler — same shape set as R3, exactly one token recipient. */ +/** `transferToken` handler — same shape set as R3, exactly one recipient, which must move a token. */ function verifyTransferToken(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssVerificationWarning[] { const decoded = { to: tx.to, value: tx.value, data: tx.data }; if (ctx.recipients.length !== 1) { @@ -636,17 +811,33 @@ function verifyTransferToken(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssVer ]; } 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: recipient.address, value: recipient.amount }, + { to: asset.contract, value: '0' }, decoded ), ]; } - return verifyTokenTransfer(ctx, tx, recipient); + return verifyTokenTransfer(ctx, tx, recipient, asset.contract); } /** @@ -657,7 +848,8 @@ function verifyTransferToken(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssVer function verifyTokenApproval(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssVerificationWarning[] { const warnings: TssVerificationWarning[] = []; const decoded = { to: tx.to, value: tx.value, data: tx.data }; - const tokenContract = resolveTokenContractAddress(ctx.tokenName); + 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')) { @@ -667,9 +859,11 @@ function verifyTokenApproval(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssVer warnings.push( violation( ctx, - `tokenApproval intent names no token contract that can be resolved from statics ('${ - ctx.tokenName ?? '' - }'); transaction to cannot be verified`, + 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 ) diff --git a/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts b/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts index ccaa6582193..a3423db3f1d 100644 --- a/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts +++ b/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts @@ -359,6 +359,18 @@ export interface VerifyTransactionOptions { * 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/utils/tss/ecdsa/ecdsa.ts b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts index a308b5db8bb..b919b44ecbd 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts @@ -837,6 +837,7 @@ export class EcdsaUtils extends BaseEcdsaUtils { wallet: this.wallet, walletType: this.wallet.multisigType(), intent: txRequest.intent as PopulatedIntent, + callerTxParams: params.txParams, }); } else { await this.baseCoin.verifyTransaction({ @@ -845,6 +846,7 @@ export class EcdsaUtils extends BaseEcdsaUtils { 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 d558fdf3dc8..1064182bd6f 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts @@ -996,6 +996,7 @@ export class EcdsaMPCv2Utils extends BaseEcdsaUtils { 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); @@ -1007,6 +1008,7 @@ export class EcdsaMPCv2Utils extends BaseEcdsaUtils { wallet: this.wallet, walletType: this.wallet.multisigType(), intent: txRequest.intent as PopulatedIntent, + callerTxParams: params.txParams, }); } txOrMessageToSign = unsignedTx.signableHex; From ab76aae6ef6d8872c0f0ff002f101c93e79fe734 Mon Sep 17 00:00:00 2001 From: Deepak Jangid Date: Tue, 3 Nov 2026 12:06:37 +0530 Subject: [PATCH 2/2] test(abstract-eth): cover declared asset binding and intent isolation The previous commit made the caller's txParams authoritative over the stored tx request intent and bound EVM TSS payments to a locally resolved asset. These tests pin that: stored calldata cannot fill a caller recipient, a server-named token cannot replace the caller's token, native and token shapes cannot be swapped, batches must move one asset with the matching batch method, and unresolvable or conflicting token identity fails closed. ECDSA and MPCv2 wiring tests check that callerTxParams and the stored intent both reach verifyTransaction, and that pending approval passes no caller params. The README documents the verifier's contract and the strictTssVerification opt-in. Ticket: CECHO-2460 --- modules/abstract-eth/README.md | 17 ++ .../test/unit/tssBatcherVerification.ts | 84 +++++- .../sdk-coin-evm/test/unit/tssVerification.ts | 246 +++++++++++++++++- .../unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts | 82 ++++++ .../tss/ecdsa/ecdsaVerifyTransactionWiring.ts | 132 ++++++++++ 5 files changed, 558 insertions(+), 3 deletions(-) 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..36c8b2b3f61 100644 --- a/modules/abstract-eth/README.md +++ b/modules/abstract-eth/README.md @@ -1 +1,18 @@ # 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. +- Mismatches that already threw (native destination and value, ERC-20 recipient and amount, + consolidation target) keep throwing. New findings are reported through `console.warn` and the + optional `onVerificationWarning` callback; pass `strictTssVerification: true` to `BitGoAPI` to + make them hard failures. diff --git a/modules/sdk-coin-bsc/test/unit/tssBatcherVerification.ts b/modules/sdk-coin-bsc/test/unit/tssBatcherVerification.ts index 4b6a1405c1b..e69e5596489 100644 --- a/modules/sdk-coin-bsc/test/unit/tssBatcherVerification.ts +++ b/modules/sdk-coin-bsc/test/unit/tssBatcherVerification.ts @@ -1,7 +1,7 @@ import 'should'; import { BitGoAPI } from '@bitgo/sdk-api'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; -import { coins, Erc7984Coin } from '@bitgo/statics'; +import { coins, ContractAddressDefinedToken, Erc7984Coin } from '@bitgo/statics'; import { TransactionType, TssVerificationWarning, Wallet } from '@bitgo/sdk-core'; import { optionalDeps } from '@bitgo/abstract-eth'; @@ -26,6 +26,18 @@ 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'); @@ -203,6 +215,76 @@ describe('EVM TSS fail-closed verification (batcher contract flows)', function ( }); }); + 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' }; diff --git a/modules/sdk-coin-evm/test/unit/tssVerification.ts b/modules/sdk-coin-evm/test/unit/tssVerification.ts index 59dbdc9dbac..16ecb476039 100644 --- a/modules/sdk-coin-evm/test/unit/tssVerification.ts +++ b/modules/sdk-coin-evm/test/unit/tssVerification.ts @@ -1,10 +1,11 @@ import 'should'; import { BitGoAPI } from '@bitgo/sdk-api'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; -import { BaseCoin as StaticsBaseCoin, coins, Erc7984Coin } from '@bitgo/statics'; +import { BaseCoin as StaticsBaseCoin, coins, ContractAddressDefinedToken, Erc7984Coin } from '@bitgo/statics'; import { PopulatedIntent, resolveEffectiveTxParams, + TransactionParams, TransactionType, TssVerificationWarning, TxRequest, @@ -38,6 +39,20 @@ 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'); @@ -103,10 +118,13 @@ 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 }; + const verify = ( txParams: Record, txHex: string, - opts: { strict?: boolean; intent?: PopulatedIntent } = {} + opts: { strict?: boolean; intent?: PopulatedIntent; callerTxParams?: SigningPathCallerParams } = {} ): 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; @@ -117,6 +135,25 @@ async function buildTxHex( 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 } : {}), }); }; @@ -336,6 +373,211 @@ async function buildTxHex( }); }); + 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' }, 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 7942a0293e1..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; @@ -837,9 +839,89 @@ describe('ECDSA MPC v2', async () => { (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 () => { // Regular EVM transaction on FLR (e.g. token transfer, not cross-chain). // serializedTxHex starts with 'f8' (RLP prefix), NOT '0000'. 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'); + }); +});