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/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-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/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; 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'); + }); +});