diff --git a/modules/abstract-eth/CHANGELOG.md b/modules/abstract-eth/CHANGELOG.md index 5ce0ae94ce4..d2ed3f12496 100644 --- a/modules/abstract-eth/CHANGELOG.md +++ b/modules/abstract-eth/CHANGELOG.md @@ -3,6 +3,16 @@ All notable changes to this project will be documented in this file. See [Conventional Commits](https://conventionalcommits.org) for commit guidelines. +## Unreleased + +### Bug Fixes + +* Reject caller-declared EVM TSS payments without recipients in both verification modes; never substitute stored recipients for missing caller intent. +* Bind legacy EVM signing payloads using the coin instance's network configuration, avoiding ambiguous chain-id lookups. +* Resolve caller tokenData consistently, reject conflicting token identities, canonicalize calldata, and verify fillNonce against the declared receive address. +* Reject undecodable EVM TSS payloads, recipient-less token transfers, stored DeFi intents without a trusted vault binding, and DeFi legs carrying native value. +* Isolate verification-warning callback failures and redact calldata in console warnings without weakening strict verification. + # [25.6.0](https://github.com/BitGo/BitGoJS/compare/%40bitgo%2Fabstract-eth%4025.5.4...%40bitgo%2Fabstract-eth%4025.6.0) (2026-11-03) ### Bug Fixes diff --git a/modules/abstract-eth/README.md b/modules/abstract-eth/README.md index 856c0af47ca..31f08e66226 100644 --- a/modules/abstract-eth/README.md +++ b/modules/abstract-eth/README.md @@ -1 +1,48 @@ # BitGo abstract-eth + +## EVM TSS transaction verification + +`verifyTssTransaction` decodes the EVM bytes about to be signed and compares `to` / `value` / +`data` with the declared intent before the user key share signs (TOB-BITGOEDMPC-2 / INFOSEC-671). +The default rollout reports new mismatches without blocking signing; it is not full fail-closed +enforcement. Set `strictTssVerification: true` locally on `BitGoAPI` to reject every finding. + +- The caller's own `txParams` are authoritative whenever they declare `type` or `recipients`. A + stored tx request intent is consulted only when the caller declared nothing (pending-approval + signing); server data never fills absent caller calldata or replaces the declared token. +- Token identity comes from the recipient's `tokenAddress` / `tokenName` and + `tokenData.tokenContractAddress` / `tokenData.tokenName`, the top-level `tokenName`, and the + coin instance. Conflicting or unresolvable declarations produce a finding. For declared NFTs, + a registered generic placeholder name of the same standard is display metadata only when an + explicit collection address is present; real collection names must still agree. Payment shapes + include native, ERC-20 / ERC-7984, and single-asset batch transfers. Calldata comparisons + canonicalize the hex prefix, case, and empty values. +- Missing or undecodable transaction bytes and caller-declared `transfer` / `payment` / + `transferToken` without recipients always reject, in either mode. Historic destination, + value, ERC-20 recipient and amount checks, plus `consolidationToBaseAddress`, remain fatal. +- Other findings go to `console.warn` and the optional `onVerificationWarning` callback in + default mode. Console reason text is bounded, and structured calldata fields show only the + selector and original length; this is not a guarantee of secret-free logs. The callback receives + full fields. Callback throws and rejected promises do not abort or delay signing. + Strict mode rejects the first finding, including unsupported operations such as acceleration, + staking, wrap/unwrap, token enablement, and bridging; it never downgrades them to warnings. +- A consolidation must pay the wallet base address, and an ERC-20 consolidation must not carry + native value. +- `transferToken` supports ERC-721 and ERC-1155 when `tokenData` declares the standard, + collection, token ID, and ERC-1155 quantity. Verification checks the collection contract, zero + native value, decoded recipient, token ID, and quantity. ERC-1155 batch calldata must contain + exactly the single declared entry. NFT sends declaring raw calldata use exact-call verification. +- FLR MPCv2 C-chain-to-P-chain atomic exports use atomic decoding, not EVM decoding. They require + a declared export intent, decoded C-chain source and P-chain destination, one matching P-chain + recipient address set, and an exported nanoFLR amount equal to the declared wei amount converted + to nanoFLR plus an import fee within the network's configured minimum/maximum bounds. These + checks reject in either verification mode. Atomic imports still reach the parent EVM decoder + and reject; this export-specific path does not restore import signing. +- `fillNonce` must send exactly 1 wei with no calldata to the declared `receiveAddress`, or to + the wallet base address when none is declared. Stored intents supply this address only when + the caller declared no intent of its own. +- `contractCall` and `customTx` always reject on EVM. Stored DeFi approve/deposit/withdraw + intents also reject: there is no locally trusted vault registry to bind the target or spender. + Stored `defi-redeem` remains unsupported. Caller-declared DeFi legs retain selector, amount, + and receiver/owner checks and reject native value; token-wallet approvals also bind the token + contract. These checks do not establish the vault's identity or the approval spender. diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index ff45cad8def..bd2487394a5 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -54,8 +54,23 @@ import { DeriveAddressResult, NO_RECIPIENT_TX_TYPES, CoinWithSignableConsistency, + TssVerificationMismatch, + TssVerificationWarning, } from '@bitgo/sdk-core'; import { getDerivationPath } from '@bitgo/sdk-lib-mpc'; +import { + buildEvmTssIntentContext, + callerDeclaresIntent, + canonicalizeEvmTssIntentType, + DecodedEvmTx, + evaluateEvmTssIntent, + isUnsupportedEvmTssIntentType, + resolveCoinTokenContractAddress, + selectDeclaredIntent, + TssFinding, + TssFindingSeverity, + verifyConsolidation, +} from './tssVerification'; import { bip32 } from '@bitgo/secp256k1'; import { BaseCoin as StaticsBaseCoin, @@ -399,6 +414,11 @@ interface EthTransactionParams extends TransactionParams { tokenName?: string; feeToken?: string; defiParams?: DefiIntentParams; + /** + * Address a self-send must pay (fillNonce). Callers that fund a specific address pass it so the + * verifier compares the decoded `to` with that address instead of the wallet's own base address. + */ + receiveAddress?: string; /** ERC-7984 shield intent (`type: 'wrapApprove' | 'wrap'`): wrapper coin name and exact amount. */ wrapParams?: { tokenName: string; amount: string }; /** ERC-7984 unshield intent (`type: 'unwrap'`): wrapper coin name and plaintext amount. */ @@ -409,6 +429,49 @@ interface EthTransactionParams extends TransactionParams { const MAX_UINT256 = 2n ** 256n - 1n; +/** Longest calldata prefix a console warning may echo: the 4-byte selector and nothing more. */ +const MAX_LOGGED_CALLDATA_PREFIX = 10; + +/** + * Bound the calldata a console warning prints. `data` can be arbitrarily large and carries the + * arguments of the call about to be signed, so only the selector and the payload length are + * logged. The `onVerificationWarning` callback still receives the full mismatch. + */ +function redactMismatchForLog(mismatch: TssVerificationMismatch): TssVerificationMismatch { + if (mismatch.data === undefined || mismatch.data.length <= MAX_LOGGED_CALLDATA_PREFIX) { + return mismatch; + } + return { + ...mismatch, + data: `${mismatch.data.slice(0, MAX_LOGGED_CALLDATA_PREFIX)}...(${mismatch.data.length} chars)`, + }; +} + +/** Longest finding reason a console warning may print. */ +const MAX_LOGGED_REASON_CHARS = 256; + +/** + * Bound the reason text a console warning prints. A finding reason can embed a parser error that + * echoes calldata, and a token identity in it can be arbitrarily long; the console is a diagnostic + * log, so it gets a capped string. The `onVerificationWarning` callback still receives the full + * reason. + */ +function redactReasonForLog(reason: string): string { + if (reason.length <= MAX_LOGGED_REASON_CHARS) { + return reason; + } + return `${reason.slice(0, MAX_LOGGED_REASON_CHARS)}...(truncated, ${reason.length} chars)`; +} + +/** True when a decoded transaction value is exactly zero; an unparseable value is not zero. */ +function isZeroNativeValue(value?: string): boolean { + try { + return BigInt(value || '0') === 0n; + } catch { + return false; + } +} + export interface VerifyEthTransactionOptions extends VerifyTransactionOptions { txPrebuild: TransactionPrebuild; txParams: EthTransactionParams; @@ -3382,8 +3445,11 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem * /** @inheritdoc CoinWithSignableConsistency */ assertSignableConsistency(serializedTxHex: string, signableHex: string): void { - const common = AbstractEthLikeNewCoins.getCustomChainCommon(this.getChainId()); - // getCustomChainCommon defaults to the london hardfork, which is only valid for chains + // Build the common from this coin's own network. Resolving it from the chain id through + // getCustomChainCommon is ambiguous: the id -> coin registry is shared with other families + // (kavaevm's 2222 maps to 'kava'), so the lookup can throw or pick the wrong network. + const common = getCommon(this.getNetwork() as EthLikeNetwork); + // getCommon defaults to the london hardfork, which is only valid for chains // that activated EIP-1559. Chains without it (e.g. BSC, XDC) sign under petersburg. if (!this.staticsCoin?.features.includes(CoinFeature.EIP1559)) { common.setHardfork(optionalDeps.EthCommon.Hardfork.Petersburg); @@ -3550,15 +3616,38 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem } /** + * Verify that a TSS transaction prebuild complies with the original intention before the + * user key share signs it (TOB-BITGOEDMPC-2 / INFOSEC-671). + * + * One flow: the intent is normalized once, the bytes are decoded once, and small checks per + * operation return findings. The intent type picks the checks; each accepts a closed set of + * decoded transaction shapes and anything unmatched is a finding. Fatal findings (the checks + * that historically threw) throw at once, including on the pending-approval path. Every other + * finding goes through {@link applyTssVerificationPolicy}: `console.warn` plus the optional + * `onVerificationWarning` callback, or a hard failure when the client opts into `BitGoAPI`'s + * `strictTssVerification`. + * * @param {VerifyEthTransactionOptions} params * @param {TransactionParams} params.txParams - params object passed to send * @param {TransactionPrebuild} params.txPrebuild - prebuild object returned by server * @param {Wallet} params.wallet - Wallet object to obtain keys to verify against + * @param {PopulatedIntent} params.intent - stored tx request intent (pending-approval path) * @returns {boolean} + * @throws {Error} if txPrebuild.txHex is missing or cannot be decoded, in either mode * @throws {TxIntentMismatchRecipientError} if transaction recipients don't match user intent */ async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise { - const { txParams, txPrebuild, wallet } = params; + const ethNetwork = this.getNetwork(); + const { txParams, txPrebuild, wallet, intent } = params; + // Exactly one source speaks for the user intent: the caller's own params when it declared a + // type or recipients, otherwise the stored tx request intent (pending approval). The other + // source is dropped, so server data never selects the handler or fills in what the caller + // left out. See selectDeclaredIntent. + const { callerParams, storedIntent } = selectDeclaredIntent({ + txParams, + callerTxParams: params.callerTxParams, + intent, + }); // Helper to throw TxIntentMismatchRecipientError with recipient details const throwRecipientMismatch = async (message: string, mismatchedRecipients: Recipient[]): Promise => { @@ -3578,6 +3667,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem !( txParams.prebuildTx?.consolidateId || txPrebuild?.consolidateId || + storedIntent?.consolidateId || txParams.stakingRequestId || txParams.prebuildTx?.stakingRequestId || (txParams.type && NO_RECIPIENT_TX_TYPES.has(txParams.type)) @@ -3593,95 +3683,146 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem throw new Error('tx cannot be both a batch and hop transaction'); } - if (txParams.type && ['transfer'].includes(txParams.type)) { - if (txParams.recipients && txParams.recipients.length === 1) { - const recipients = txParams.recipients; - const expectedAmount = recipients[0].amount.toString(); - const expectedDestination = recipients[0].address; - - const txBuilder = this.getTransactionBuilder(); - txBuilder.from(txPrebuild.txHex); - const tx = await txBuilder.build(); - const txJson = tx.toJson(); - if (txJson.data === '0x') { - if (expectedAmount !== txJson.value) { - await throwRecipientMismatch( - 'the transaction amount in txPrebuild does not match the value given by client', - [{ address: txJson.to, amount: txJson.value }] - ); - } - if (expectedDestination.toLowerCase() !== txJson.to.toLowerCase()) { - await throwRecipientMismatch('destination address does not match with the recipient address', [ - { address: txJson.to, amount: txJson.value }, - ]); - } - } else if (txJson.data.startsWith('0xa9059cbb')) { - const [recipientAddress, amount] = getRawDecoded( - ['address', 'uint256'], - getBufferedByteCode('0xa9059cbb', txJson.data) - ); - - // Check if recipients[0].data exists (WalletConnect flow) - let expectedRecipientAddress: string; - let expectedTokenAmount: string; - const recipientData = (recipients[0] as any).data; - - if (recipientData && recipientData.startsWith('0xa9059cbb')) { - // WalletConnect: decode expected recipient and amount from recipients[0].data - const [expectedRecipient, expectedAmount] = getRawDecoded( - ['address', 'uint256'], - getBufferedByteCode('0xa9059cbb', recipientData) - ); - expectedRecipientAddress = addHexPrefix(expectedRecipient.toString()).toLowerCase(); - expectedTokenAmount = expectedAmount.toString(); - } else { - // Normal flow: use recipients[0].address and recipients[0].amount - expectedRecipientAddress = expectedDestination.toLowerCase(); - expectedTokenAmount = expectedAmount; - } - - if (expectedTokenAmount !== amount.toString()) { - await throwRecipientMismatch( - 'the transaction amount in txPrebuild does not match the value given by client', - [{ address: addHexPrefix(recipientAddress.toString()), amount: amount.toString() }] - ); - } - - if (expectedRecipientAddress !== addHexPrefix(recipientAddress.toString()).toLowerCase()) { - await throwRecipientMismatch('destination address does not match with the recipient address', [ - { address: addHexPrefix(recipientAddress.toString()), amount: amount.toString() }, - ]); - } - } - } + // A caller that names a payment (`transfer` / `payment`) or a token payment (`transferToken`) + // must supply the recipients it is paying. With none there is no caller intent to verify the + // bytes against, and the stored intent was dropped (its destination may be attacker-chosen), + // so the handler would only warn. `[]` declares nothing, exactly like omitting the field. + // This runs before the warn / strict policy: neither mode may downgrade an unverifiable + // payment to a warning. Only the caller's own params are read here; the back-filled `txParams` + // and the stored intent must not supply the recipients or the type. + const declaredPaymentType = canonicalizeEvmTssIntentType(callerParams?.type); + if ( + (declaredPaymentType === 'transfer' || declaredPaymentType === 'transferToken') && + !callerParams?.recipients?.length + ) { + const declaredPayment = declaredPaymentType === 'transferToken' ? 'token transfer' : 'payment'; + await throwRecipientMismatch( + `caller declared a ${declaredPayment} without recipients: pass txParams.recipients so the ` + + 'transaction can be verified against the user intent', + [] + ); } - if (txParams.type && ['defiApprove', 'defiDeposit', 'defiWithdraw'].includes(txParams.type)) { + // Canonical verifier intent type, derived only from the source that speaks for the user. + // The caller's own `type` wins and is never defaulted from the stored intent: on a direct + // send `txParams.type` is the caller's alone, and a caller that declared recipients without a + // type is making a payment. Only when the caller declared nothing does the stored intent + // type apply: resolveEffectiveTxParams has already put the raw stored spelling (payment, + // defi-approve, …) in txParams.type, which is normalized here. + const explicitType = storedIntent + ? canonicalizeEvmTssIntentType(txParams.type) || canonicalizeEvmTssIntentType(storedIntent.intentType) + : canonicalizeEvmTssIntentType(callerParams?.type); + // A consolidation id on the prebuild or the stored intent is server data. It marks the tx as a + // consolidation only when the caller declared nothing of its own; it can never reroute a + // caller-declared payment to the consolidate handler. + const consolidateId = callerDeclaresIntent(callerParams) + ? undefined + : txParams.prebuildTx?.consolidateId || txPrebuild?.consolidateId || storedIntent?.consolidateId || undefined; + const isConsolidation = + !!params.verification?.consolidationToBaseAddress || explicitType === 'consolidate' || !!consolidateId; + const canonicalType = explicitType || (isConsolidation ? 'consolidate' : 'transfer'); + + // contractCall / customTx cannot be created on EVM and carry arbitrary calldata with no + // intent shape to verify — never let them reach signing on this path. + if (isUnsupportedEvmTssIntentType(canonicalType)) { + throw new Error(`unsupported intent type for EVM TSS: ${canonicalType}`); + } + + // The bytes about to be signed are the only ground truth and every check below reads them. + // Bytes that cannot be decoded cannot be verified against anything, so reject here, before a + // handler or the warn / strict policy runs: no mode may turn missing or undecodable bytes into + // a warning, and no placeholder transaction may stand in for them. + let tx: DecodedEvmTx; + try { + tx = await this.decodeTssTransaction(txPrebuild.txHex); + } catch (e) { if (!txPrebuild.txHex) { throw new Error('missing txHex in txPrebuild'); } + throw new Error(`transaction bytes could not be decoded to verify intent: ${(e as Error).message}`); + } + const raise = (finding: TssFinding): Promise => + throwRecipientMismatch(finding.reason, [ + { address: finding.decoded.to ?? '', amount: finding.decoded.value ?? '' }, + ]); + const raiseFatal = async (findings: TssFinding[]): Promise => { + const fatal = findings.find((finding) => finding.severity === 'fatal'); + if (fatal) { + await raise(fatal); + } + }; + + const coinBaseAddress = wallet.coinSpecific()?.baseAddress; + const baseAddress = typeof coinBaseAddress === 'string' ? coinBaseAddress : undefined; + // Locally known identity of this coin instance: set only when the coin itself is a token. + const coinTokenContractAddress = resolveCoinTokenContractAddress(this.getChain()); + const ctx = buildEvmTssIntentContext({ + intentType: canonicalType, + txParams, + callerParams, + storedIntent, + coinTokenContractAddress, + baseAddress, + batcherAddress: ethNetwork?.batcherContractAddress, + }); + + // Fatal findings (the checks that threw before fail-closed verification existed) throw now; + // the rest wait for the policy step at the end. + const intentFindings = evaluateEvmTssIntent(ctx, tx); + await raiseFatal(intentFindings); + + // DeFi vault calldata vs defiParams. The leg is built server-side from defiParams, so the + // amount and the in-calldata destination are all the caller declared; the target contract and + // the approve spender are not among them. Statics is the only registry this process can + // consult while signing, and it holds no vault entries, so the vault itself can never be + // anchored locally. It does hold the wallet's own token contract, which is the contract an + // ERC-20 allowance for the vault must be granted on. + if (['defiApprove', 'defiDeposit', 'defiWithdraw'].includes(canonicalType)) { + // A stored intent carries only vaultId and amount, all of it server-supplied. Comparing it + // with server-supplied bytes would only prove the server agrees with itself, so the stored + // flow is rejected instead of reported as verified. + if (storedIntent) { + await throwRecipientMismatch( + `stored ${canonicalType} intent cannot be verified: no locally known vault contract for ` + + `vault '${storedIntent.vaultId ?? ''}' to anchor the transaction target and spender`, + [] + ); + } - const baseAddress = wallet.coinSpecific()?.baseAddress; const defiParams = txParams.defiParams; - if (!baseAddress || !defiParams) { + if (!coinBaseAddress || !defiParams) { await throwRecipientMismatch('DeFi transaction is missing required intent parameters', []); - return false; } - const validatedBaseAddress = baseAddress as string; - const validatedDefiParams = defiParams; - const txBuilder = this.getTransactionBuilder(); - txBuilder.from(txPrebuild.txHex); - const txJson = (await txBuilder.build()).toJson(); + const validatedBaseAddress = coinBaseAddress as string; + const validatedDefiParams = defiParams as DefiIntentParams; + const txJson = tx; const mismatch = async (message: string, address = txJson.to, amount = txJson.value): Promise => throwRecipientMismatch(message, [{ address: address || '', amount: amount || '' }]); const data = txJson.data || ''; - if (txParams.type === 'defiApprove') { + // An ERC-20 approve or an ERC-4626 deposit / redeem leg moves no native currency. A nonzero + // value would hand the wallet's native balance to whatever contract `to` names. + if (!isZeroNativeValue(txJson.value)) { + await mismatch(`${canonicalType} transaction must not carry native value`); + } + + if (canonicalType === 'defiApprove') { const selector = addHexPrefix(optionalDeps.ethAbi.methodID('approve', ['address', 'uint256']).toString('hex')); if (!data.startsWith(selector)) { await mismatch('defiApprove transaction must use ERC-20 approve(address,uint256) calldata'); } + // The allowance is granted by the token contract. When this coin instance is that token, + // statics names the contract locally, so the leg can be anchored to it. Base-coin wallets + // have no local token identity, and no local registry holds vault addresses, so for them + // neither the target contract nor the spender can be anchored here. + if (coinTokenContractAddress && txJson.to.toLowerCase() !== coinTokenContractAddress.toLowerCase()) { + await mismatch( + 'defiApprove transaction must target the wallet token contract', + txJson.to, + validatedDefiParams.amount + ); + } const [, amount] = getRawDecoded(['address', 'uint256'], getBufferedByteCode(selector, data)); if (amount.toString() !== validatedDefiParams.amount) { await mismatch( @@ -3690,7 +3831,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem amount.toString() ); } - } else if (txParams.type === 'defiDeposit') { + } else if (canonicalType === 'defiDeposit') { const selector = addHexPrefix(optionalDeps.ethAbi.methodID('deposit', ['uint256', 'address']).toString('hex')); if (!data.startsWith(selector)) { await mismatch('defiDeposit transaction must use deposit(uint256,address) calldata'); @@ -3744,7 +3885,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem } } - if (txParams.type === 'wrapApprove') { + if (canonicalType === 'wrapApprove') { await this.verifyWrapApproveTransaction(params, throwRecipientMismatch); } else { // Declared as anything else: only an approve of a registered wrapper is rejected (see the method doc); @@ -3752,55 +3893,99 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem await this.assertWrapperApproveIsDeclared(params, throwRecipientMismatch); } - // Verify consolidation transactions send to base address - if (params.verification?.consolidationToBaseAddress) { - const coinSpecific = wallet.coinSpecific(); - if (!coinSpecific || !coinSpecific.baseAddress) { - throw new Error('Unable to determine base address for consolidation'); - } - const baseAddress = coinSpecific.baseAddress; - - if (!txPrebuild.txHex) { - throw new Error('missing txHex in txPrebuild'); + // Consolidations must pay the wallet base address. The check is fatal when the caller sets + // consolidationToBaseAddress, as it always was; for consolidate intents and consolidateId + // flows it is a new check that follows the warn / strict policy. + const consolidationFindings: TssFinding[] = []; + if (isConsolidation) { + const severity: TssFindingSeverity = params.verification?.consolidationToBaseAddress ? 'fatal' : 'new'; + try { + if (!baseAddress) { + throw new Error('Unable to determine base address for consolidation'); + } + if (!tx.to) { + throw new Error('Consolidation transaction is missing recipient address'); + } + consolidationFindings.push(...verifyConsolidation(tx, baseAddress, severity)); + } catch (e) { + if (severity === 'fatal') { + throw e; + } + consolidationFindings.push({ + severity, + intentType: 'consolidate', + reason: `consolidation destination could not be verified: ${(e as Error).message}`, + expected: { to: baseAddress ?? '' }, + decoded: {}, + }); } + await raiseFatal(consolidationFindings); + } - const txBuilder = this.getTransactionBuilder(); - txBuilder.from(txPrebuild.txHex); - const tx = await txBuilder.build(); - const txJson = tx.toJson(); + await this.applyTssVerificationPolicy([...consolidationFindings, ...intentFindings], raise); + return true; + } - // Verify the transaction recipient matches the base address - if (!txJson.to) { - throw new Error('Consolidation transaction is missing recipient address'); - } + /** Decodes the TSS transaction bytes once, so every verifier check reads the same fields. */ + private async decodeTssTransaction(txHex: string | undefined): Promise { + if (!txHex) { + throw new Error('missing txHex in txPrebuild'); + } + const txBuilder = this.getTransactionBuilder(); + txBuilder.from(txHex); + const txJson = (await txBuilder.build()).toJson(); + return { to: txJson.to ?? '', value: txJson.value ?? '0', data: txJson.data ?? '0x' }; + } - const erc20TransferSelector = addHexPrefix( - optionalDeps.ethAbi.methodID('transfer', ['address', 'uint256']).toString('hex') + /** + * The one place that decides warn or throw for findings that are not fatal: they throw when + * the client sets `strictTssVerification`, and otherwise go to `console.warn` (reason bounded, + * calldata redacted) and the optional `onVerificationWarning` callback, which receives both in + * full and whose own failures are dropped. + */ + private async applyTssVerificationPolicy( + findings: TssFinding[], + raise: (finding: TssFinding) => Promise + ): Promise { + if (!findings.length) { + return; + } + if (this.bitgo?.strictTssVerification) { + // Strict mode fails on the first finding in policy order, including an `unhandled` intent + // type: nothing here filters or reorders `findings`. + await raise(findings[0]); + } + for (const { intentType, reason, expected, decoded } of findings) { + const warning: TssVerificationWarning = { intentType, reason, expected, decoded }; + console.warn( + `[tss verification] ${redactReasonForLog(reason)}`, + JSON.stringify({ expected: redactMismatchForLog(expected), decoded: redactMismatchForLog(decoded) }) ); + this.notifyVerificationWarning(warning); + } + } - if (txJson.data && txJson.data.startsWith(erc20TransferSelector)) { - // For ERC20 token consolidations, txJson.to is the token contract address, - // not the actual transfer recipient. The real recipient is encoded in the - // transfer(address,uint256) calldata. Decode it and verify against baseAddress. - const [recipientAddress] = getRawDecoded( - ['address', 'uint256'], - getBufferedByteCode(erc20TransferSelector, txJson.data) + /** + * Hand one finding to the client's local warning sink. Warn mode must never fail because of the + * sink: a synchronous throw is dropped, and the promise of an `async` handler is not awaited (it + * cannot hold up signing) and its rejection is swallowed. The sink still receives the full + * warning, exactly once per finding, in policy order. + */ + private notifyVerificationWarning(warning: TssVerificationWarning): void { + const handler = this.bitgo?.onVerificationWarning; + if (!handler) { + return; + } + try { + const result: unknown = handler(warning); + if (result && typeof (result as PromiseLike).then === 'function') { + Promise.resolve(result).catch((e) => + debug('onVerificationWarning rejected: %s', (e as Error)?.message ?? String(e)) ); - const decodedRecipient = addHexPrefix(recipientAddress.toString()).toLowerCase(); - if (decodedRecipient !== baseAddress.toLowerCase()) { - await throwRecipientMismatch('Consolidation transaction recipient does not match wallet base address', [ - { address: decodedRecipient, amount: txJson.value }, - ]); - } - } else if (txJson.to.toLowerCase() !== baseAddress.toLowerCase()) { - // Native coin consolidation: txJson.to is the actual recipient - await throwRecipientMismatch('Consolidation transaction recipient does not match wallet base address', [ - { address: txJson.to, amount: txJson.value }, - ]); } + } catch (e) { + debug('onVerificationWarning threw: %s', (e as Error)?.message ?? String(e)); } - - return true; } /** diff --git a/modules/abstract-eth/src/lib/iface.ts b/modules/abstract-eth/src/lib/iface.ts index fd5c6cdc000..77bb8640555 100644 --- a/modules/abstract-eth/src/lib/iface.ts +++ b/modules/abstract-eth/src/lib/iface.ts @@ -149,6 +149,21 @@ export interface NativeTransferData extends TransferData { data: string; } +export interface BatchTransferRecipient { + address: string; + amount: string; +} + +/** Decoded inner recipients of batch(address[],uint256[]) batcher calldata. */ +export interface BatchTransferData { + recipients: BatchTransferRecipient[]; +} + +/** Decoded batchTransferFrom(address,address[],uint256[]) batcher calldata. */ +export interface BatchTransferFromData extends BatchTransferData { + tokenContractAddress: string; +} + export interface WalletInitializationData { salt?: string; owners: string[]; diff --git a/modules/abstract-eth/src/lib/utils.ts b/modules/abstract-eth/src/lib/utils.ts index 95dc0ca69d9..a47efbebe84 100644 --- a/modules/abstract-eth/src/lib/utils.ts +++ b/modules/abstract-eth/src/lib/utils.ts @@ -32,6 +32,8 @@ import { } from '@bitgo/sdk-core'; import { + BatchTransferData, + BatchTransferFromData, ERC1155TransferData, ERC721TransferData, FlushTokensData, @@ -91,6 +93,10 @@ import { confidentialTransferWithProofMethodId, confidentialTransferWithProofTypes, confidentialTransferNoProofMethodId, + batchMethodId, + batchMethodTypes, + batchTransferFromMethodId, + batchTransferFromMethodTypes, } from './walletUtil'; import { EthTransactionData } from './types'; import { @@ -479,6 +485,65 @@ export function decodeTransferData(data: string, isFirstSigner?: boolean): Trans } } +/** + * Decode batch(address[],uint256[]) calldata used for native batch sends to the batcher contract. + * + * @param data Hex string starting with the batch method selector + * @returns Decoded recipients and amounts in the order they appear in the calldata + */ +export function decodeBatchTransferData(data: string): BatchTransferData { + if (!data.toLowerCase().startsWith(batchMethodId)) { + throw new BuildTransactionError(`Invalid batch transfer bytecode: ${data}`); + } + const [addresses, amounts] = getRawDecoded(batchMethodTypes, getBufferedByteCode(batchMethodId, data)); + if (!Array.isArray(addresses) || !Array.isArray(amounts)) { + throw new BuildTransactionError(`Invalid batch transfer bytecode: ${data}`); + } + if (addresses.length !== amounts.length) { + throw new BuildTransactionError( + `Mismatched batch address/amount array lengths: ${addresses.length} vs ${amounts.length}` + ); + } + return { + recipients: addresses.map((addr, i) => ({ + address: addHexPrefix(addr as string), + amount: new BigNumber(bufferToHex(amounts[i] as Buffer)).toFixed(), + })), + }; +} + +/** + * Decode batchTransferFrom(address,address[],uint256[]) calldata used for token batch sends + * to the batcher contract. + * + * @param data Hex string starting with the batchTransferFrom method selector + * @returns Decoded token contract, recipients and amounts in calldata order + */ +export function decodeBatchTransferFromData(data: string): BatchTransferFromData { + if (!data.toLowerCase().startsWith(batchTransferFromMethodId)) { + throw new BuildTransactionError(`Invalid batchTransferFrom bytecode: ${data}`); + } + const [tokenContractAddress, addresses, amounts] = getRawDecoded( + batchTransferFromMethodTypes, + getBufferedByteCode(batchTransferFromMethodId, data) + ); + if (!Array.isArray(addresses) || !Array.isArray(amounts)) { + throw new BuildTransactionError(`Invalid batchTransferFrom bytecode: ${data}`); + } + if (addresses.length !== amounts.length) { + throw new BuildTransactionError( + `Mismatched batchTransferFrom address/amount array lengths: ${addresses.length} vs ${amounts.length}` + ); + } + return { + tokenContractAddress: addHexPrefix(tokenContractAddress as string), + recipients: addresses.map((addr, i) => ({ + address: addHexPrefix(addr as string), + amount: new BigNumber(bufferToHex(amounts[i] as Buffer)).toFixed(), + })), + }; +} + /** * Decode the given ABI-encoded transfer data for the sendMultisigToken function and return parsed fields * diff --git a/modules/abstract-eth/src/lib/walletUtil.ts b/modules/abstract-eth/src/lib/walletUtil.ts index 8d5891d0162..f4b91ecd235 100644 --- a/modules/abstract-eth/src/lib/walletUtil.ts +++ b/modules/abstract-eth/src/lib/walletUtil.ts @@ -1,5 +1,13 @@ export const sendMultisigMethodId = '0x39125215'; export const sendMultisigTokenMethodId = '0x0dcd7a6c'; +// Selector for batch(address[],uint256[]) used by batcher contract sends (native batches). +export const batchMethodId = '0xc00c4e9e'; +export const batchMethodName = 'batch'; +export const batchMethodTypes = ['address[]', 'uint256[]']; +// Selector for batchTransferFrom(address,address[],uint256[]) used for token batches. +export const batchTransferFromMethodId = '0x4885b254'; +export const batchTransferFromMethodName = 'batchTransferFrom'; +export const batchTransferFromMethodTypes = ['address', 'address[]', 'uint256[]']; export const v1CreateForwarderMethodId = '0xfb90b320'; export const v4CreateForwarderMethodId = '0x13b2f75c'; export const v1WalletInitializationFirstBytes = '0x60806040'; @@ -16,6 +24,8 @@ export const flushERC1155ForwarderTokensMethodId = '0xe6bd0aa4'; export const flushERC1155ForwarderTokensMethodIdV4 = '0x8972c17c'; export const ERC721SafeTransferTypeMethodId = '0xb88d4fde'; +// Selector for the 3-argument safeTransferFrom(address,address,uint256) ERC-721 overload. +export const ERC721SafeTransferFromMethodId = '0x42842e0e'; export const ERC721TransferFromMethodId = '0x23b872dd'; export const ERC1155SafeTransferTypeMethodId = '0xf242432a'; export const ERC1155BatchTransferTypeMethodId = '0x2eb2c2d6'; diff --git a/modules/abstract-eth/src/tssVerification.ts b/modules/abstract-eth/src/tssVerification.ts new file mode 100644 index 00000000000..2a159c60ce7 --- /dev/null +++ b/modules/abstract-eth/src/tssVerification.ts @@ -0,0 +1,1447 @@ +/** + * Fail-closed EVM TSS intent verification. + * + * `verifyTssTransaction` must not let a user key share sign EVM bytes unless the decoded + * `to` / `value` / `data` match the user intent (TOB-BITGOEDMPC-2 / INFOSEC-671). The intent + * type picks the handler; each handler accepts a closed set of decoded transaction shapes + * and anything unmatched is a violation. Types are never derived from calldata: selectors + * are shared across intents and `payment + data` carries arbitrary calldata. + * + * The checks only return findings; `AbstractEthLikeNewCoins.verifyTssTransaction` applies the + * policy in one place. A `fatal` finding (a check that already threw before this module existed) + * always throws. `new` and `unhandled` findings are reported through `console.warn` plus an + * optional `onVerificationWarning` callback, and throw when the client opts into + * `strictTssVerification`. + */ +import { + ITransactionRecipient, + PopulatedIntent, + TssVerificationMismatch, + TssVerificationWarning, +} from '@bitgo/sdk-core'; +import { coins, ContractAddressDefinedToken } from '@bitgo/statics'; +import { Buffer } from 'buffer'; +import { addHexPrefix, bufferToHex } from 'ethereumjs-util'; +import { + batchMethodId, + batchTransferFromMethodId, + BatchTransferData, + BatchTransferFromData, + confidentialTransferNoProofMethodId, + confidentialTransferNoProofTypes, + confidentialTransferWithProofMethodId, + confidentialTransferWithProofTypes, + decodeApproveCalldata, + decodeBatchTransferData, + decodeBatchTransferFromData, + ERC1155BatchTransferTypeMethodId, + ERC1155BatchTransferTypes, + ERC1155SafeTransferTypeMethodId, + ERC1155SafeTransferTypes, + ERC721SafeTransferFromMethodId, + ERC721SafeTransferTypeMethodId, + ERC721SafeTransferTypes, + ERC721TransferFromMethodId, + ERC721TransferFromTypes, + getBufferedByteCode, + getRawDecoded, +} from './lib'; + +/** ERC-20 `transfer(address,uint256)` selector. */ +export const erc20TransferMethodId = '0xa9059cbb'; +/** Maximum ERC-20 allowance: uint256 max. */ +export const MAX_UINT256 = ((1n << 256n) - 1n).toString(); + +/** + * Canonical verifier intent types with a fail-closed handler. Anything outside this set + * (staking, `*WithCallData`, `acceleration`, wrap / unwrap-native, `enableToken`, + * `bridgeFunds`, `defiRedeem`, …) has no handler and is flagged instead of silently passing. + */ +export type EvmTssIntentType = + | 'transfer' + | 'transferToken' + | 'tokenApproval' + | 'fillNonce' + | 'consolidate' + | 'defiApprove' + | 'defiDeposit' + | 'defiWithdraw' + | 'wrapApprove' + | 'contractCall' + | 'customTx'; + +/** + * Stored intent types that cannot be created on EVM and must never reach signing there. + * `contractCall` / `customTx` carry arbitrary calldata with no intent shape to verify. + */ +const UNSUPPORTED_EVM_INTENT_TYPES = new Set(['contractCall', 'customTx']); + +/** Stored (kebab-case) intent spellings mapped to canonical verifier types. */ +const INTENT_TYPE_ALIASES: Record = { + payment: 'transfer', + 'transfer-token': 'transferToken', + // `Wallet.prebuildTransactionTxRequests` switches on the lowercase spelling for token sends. + transfertoken: 'transferToken', + 'token-approval': 'tokenApproval', + 'fill-nonce': 'fillNonce', + 'bridge-funds': 'bridgeFunds', + 'enable-token': 'enableToken', + 'custom-tx': 'customTx', + 'contract-call': 'contractCall', + 'defi-approve': 'defiApprove', + 'defi-deposit': 'defiDeposit', + 'defi-withdraw': 'defiWithdraw', + 'defi-redeem': 'defiRedeem', + 'wrap-approve': 'wrapApprove', + 'wrap-native': 'wrapNative', + 'unwrap-native': 'unwrapNative', + 'finalize-unwrap': 'finalizeUnwrap', + 'stake-with-call-data': 'stakeWithCallData', + 'unstake-with-call-data': 'unstakeWithCallData', +}; + +/** Transaction fields decoded from the bytes about to be signed. */ +export interface DecodedEvmTx { + to: string; + value: string; + data: string; +} + +/** + * How a finding is enforced. `fatal` always throws: these are the checks that threw before + * fail-closed verification existed. `new` (a mismatch on a handled intent type) and `unhandled` + * (an intent type with no handler) follow the client's warn / strict policy. + */ +export type TssFindingSeverity = 'fatal' | 'new' | 'unhandled'; + +/** One rule the transaction bytes broke, with the severity that decides warn or throw. */ +export interface TssFinding extends TssVerificationWarning { + severity: TssFindingSeverity; +} + +/** One token identity a recipient declares, either flat or through the intent `tokenData` shape. */ +export interface EvmTokenIdentityDeclaration { + tokenName?: string; + tokenContractAddress?: string; +} + +/** + * The NFT fields of the intent `tokenData` shape. They say which standard and which token the + * recipient moves; the contract identity itself stays in {@link EvmTokenIdentityDeclaration}, so + * `tokenType` / `tokenId` / `tokenQuantity` are never fed into asset resolution. + */ +interface EvmTokenDataDeclaration extends EvmTokenIdentityDeclaration { + tokenType?: string; + tokenId?: string; + tokenQuantity?: string; +} + +/** One recipient of the user intent, flattened from txParams or the stored tx request intent. */ +export interface EvmTssExpectedRecipient { + address: string; + amount: string; + /** Raw calldata the intent declares for this recipient (WalletConnect / dApp flows). */ + data?: string; + tokenName?: string; + tokenContractAddress?: string; + /** + * Every token identity the source declared for this recipient (`tokenAddress` / `tokenName` and + * `tokenData.tokenName` / `tokenData.tokenContractAddress`). They are all resolved, so two + * declarations of different tokens stay ambiguous instead of one silently overriding the other. + */ + declaredTokenIdentities?: EvmTokenIdentityDeclaration[]; + /** `tokenData.tokenType`: the declared standard, which picks the calldata shape (`transferToken`). */ + tokenType?: string; + /** `tokenData.tokenId`: the declared NFT id (ERC-721 / ERC-1155). */ + tokenId?: string; + /** `tokenData.tokenQuantity`: the declared amount (ERC-1155, and ERC-721 must be 1 when set). */ + tokenQuantity?: string; +} + +export interface EvmTssIntentContext { + /** Canonical verifier intent type (after {@link canonicalizeEvmTssIntentType}). */ + intentType: string; + recipients: EvmTssExpectedRecipient[]; + /** Token name declared by the caller (or, on the pending-approval path, the stored intent). */ + tokenName?: string; + /** + * Contract of the token the coin instance itself represents (e.g. `bitgo.coin('eth:usdc')`), + * undefined for base coins. Locally selected: it is the asset of any payment that declares no + * token of its own. + */ + coinTokenContractAddress?: string; + /** + * Address whose nonce is filled (`fillNonce`), read from the source that speaks for the user + * intent only: the declared `receiveAddress`, or the stored intent's `receiveAddress`. + */ + receiveAddress?: string; + /** Wallet base address (`wallet.coinSpecific().baseAddress`). */ + baseAddress?: string; + /** Network batcher contract from statics (`network.batcherContractAddress`). */ + batcherAddress?: string; +} + +function kebabToCamel(value: string): string { + return value.replace(/-([a-z0-9])/g, (_, c: string) => c.toUpperCase()); +} + +/** + * Map a declared transaction type onto the canonical verifier type. Caller-supplied + * `txParams.type` and stored `intent.intentType` both flow through here, so kebab-case + * stored spellings (`payment`, `defi-approve`, …) land on their canonical handlers. + */ +export function canonicalizeEvmTssIntentType(rawType?: string): string { + if (!rawType) { + return ''; + } + const trimmed = rawType.trim(); + return INTENT_TYPE_ALIASES[trimmed.toLowerCase()] ?? INTENT_TYPE_ALIASES[trimmed] ?? kebabToCamel(trimmed); +} + +/** True when the canonical type must never be signed on EVM chains. */ +export function isUnsupportedEvmTssIntentType(canonicalType: string): boolean { + return UNSUPPORTED_EVM_INTENT_TYPES.has(canonicalType as EvmTssIntentType); +} + +function normalizeAddress(address?: string): string { + return (address ?? '').toLowerCase(); +} + +function toAddressString(value: unknown): string { + const raw = Buffer.isBuffer(value) ? bufferToHex(value as Buffer) : String(value); + return addHexPrefix(raw).toLowerCase(); +} + +function toBigInt(value: unknown): bigint { + if (typeof value === 'bigint') { + return value; + } + if (Buffer.isBuffer(value)) { + return BigInt(bufferToHex(value as Buffer)); + } + return BigInt(String(value)); +} + +/** + * Canonical calldata for every comparison: missing data, `''` and `'0x'` are the same empty + * payload, hex is lowercased, and a dropped `0x` prefix is restored. Selector gates and decoders + * all read this form, so a format difference can never route the same bytes past a check. + */ +function normalizeCalldata(data?: string): string { + const lower = (data ?? '').toLowerCase(); + if (!lower || lower === '0x') { + return '0x'; + } + return lower.startsWith('0x') ? lower : `0x${lower}`; +} + +function isEmptyCalldata(data?: string): boolean { + return normalizeCalldata(data) === '0x'; +} + +function valuesEqual(a: unknown, b: unknown): boolean { + try { + return toBigInt(a) === toBigInt(b); + } catch { + return String(a) === String(b); + } +} + +function violation( + ctx: Pick, + reason: string, + expected: TssVerificationMismatch, + decoded: TssVerificationMismatch, + severity: TssFindingSeverity = 'new' +): TssFinding { + return { intentType: ctx.intentType, reason, expected, decoded, severity }; +} + +/** + * Resolve the on-chain contract of `tokenName` from statics. Returns undefined for unknown + * names and for coins without a contract address (native coins, known statics gaps). + */ +export function resolveTokenContractAddress(tokenName?: string): string | undefined { + if (!tokenName) { + return undefined; + } + try { + const coin = coins.get(tokenName); + return coin instanceof ContractAddressDefinedToken ? coin.contractAddress.toString() : undefined; + } catch { + return undefined; + } +} + +/** + * Contract of the token that `chain` itself represents (`bitgo.coin('eth:usdc')` is the USDC + * contract). Undefined for base coins and for native coins that merely expose an ERC-20 + * interface (`isToken: false`). The coin instance is chosen locally, so it is a trusted source + * of asset identity for payments that name no token of their own. + */ +export function resolveCoinTokenContractAddress(chain?: string): string | undefined { + if (!chain) { + return undefined; + } + try { + const coin = coins.get(chain); + return coin instanceof ContractAddressDefinedToken && coin.isToken ? coin.contractAddress.toString() : undefined; + } catch { + return undefined; + } +} + +/** The asset a payment moves, resolved only from locally declared identity. */ +type PaymentAsset = { kind: 'native' } | { kind: 'token'; contract: string } | { kind: 'unresolved'; reason: string }; + +const EVM_ADDRESS = /^0x[0-9a-fA-F]{40}$/; + +/** + * Name prefixes of the statics placeholder entries each NFT standard uses when the collection is + * not itself registered (`erc721:token` / `terc721:token`, and the per-chain placeholders). Only a + * name of the recipient's own standard can be a label for its collection; any other name keeps its + * identity meaning. + */ +const NFT_DISPLAY_NAME_PREFIXES: Record<'ERC721' | 'ERC1155', RegExp> = { + ERC721: /^t?erc721:/i, + ERC1155: /^t?erc1155:/i, +}; + +/** + * Resolve the asset a payment is declared to move. Identity comes from the recipient's own + * `tokenAddress` / `tokenName` / `tokenData` declarations, the top-level `tokenName`, and the + * 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. + * + * `nftTokenType` (set by the `transferToken` handler for a declared ERC-721 / ERC-1155) enables one + * narrow exception: a statics placeholder of that same standard (`tokenData.tokenName` is required + * by the intent schema, and `getTokenName` leaves `erc721:token` in place when the collection is + * unknown). Such a name is display metadata, so it is dropped from identity when the recipient also + * declared a real collection address. Nothing else is: an unregistered name still fails + * resolution, and a name that resolves to a real contract still contradicts the declared one. + */ +function resolveAsset( + ctx: EvmTssIntentContext, + declared: { + tokenName?: string; + tokenContractAddress?: string; + declaredTokenIdentities?: EvmTokenIdentityDeclaration[]; + }, + nftTokenType?: 'ERC721' | 'ERC1155' +): PaymentAsset { + const sources: { label: string; contract: string | undefined; native?: boolean }[] = []; + // Only a real collection address can anchor a declared NFT; without one the name is the only + // identity and must still resolve (or be reported unresolvable) exactly as for any token. + const nftContract = + nftTokenType && declared.tokenContractAddress && EVM_ADDRESS.test(declared.tokenContractAddress) + ? declared.tokenContractAddress + : undefined; + const nftDisplayName = nftTokenType ? NFT_DISPLAY_NAME_PREFIXES[nftTokenType] : undefined; + // True only for a statics placeholder of the declared standard whose contract is the + // `0xerc721:token`-style pseudo-address: `getTokenName` leaves `tokenData.tokenName` as a generic + // label when the collection is unknown, and the schema makes that field mandatory. An + // unregistered name here still expresses an unresolvable token, so it keeps failing resolution. + const isDisplayOnlyNftName = (name: string): boolean => { + if (!nftContract || !nftDisplayName?.test(name)) { + return false; + } + const contract = resolveTokenContractAddress(name); + return contract !== undefined && !EVM_ADDRESS.test(contract); + }; + // Every identity the recipient declared is resolved, not just the first that happens to be + // set: two declarations of different tokens must stay ambiguous rather than one silently + // overriding the other. + const identities: EvmTokenIdentityDeclaration[] = declared.declaredTokenIdentities?.length + ? declared.declaredTokenIdentities + : [{ tokenName: declared.tokenName, tokenContractAddress: declared.tokenContractAddress }]; + for (const identity of identities) { + if (identity.tokenContractAddress !== undefined) { + sources.push({ + label: `tokenAddress '${identity.tokenContractAddress}'`, + contract: EVM_ADDRESS.test(identity.tokenContractAddress) ? identity.tokenContractAddress : undefined, + }); + } + const name = identity.tokenName; + if (!name || isDisplayOnlyNftName(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) }); + } + } + const declaredTopLevelName = ctx.tokenName; + if (declaredTopLevelName && !isDisplayOnlyNftName(declaredTopLevelName)) { + let staticsCoin; + try { + staticsCoin = coins.get(declaredTopLevelName); + } catch { + staticsCoin = undefined; + } + if (staticsCoin && !(staticsCoin instanceof ContractAddressDefinedToken && staticsCoin.isToken)) { + sources.push({ label: `coin '${declaredTopLevelName}'`, contract: undefined, native: true }); + } else { + sources.push({ + label: `token '${declaredTopLevelName}'`, + contract: resolveTokenContractAddress(declaredTopLevelName), + }); + } + } + if (ctx.coinTokenContractAddress) { + sources.push({ label: 'the coin token', contract: ctx.coinTokenContractAddress }); + } + 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 }; +} + +/** EVM recipients also carry the intent `tokenData` shape that `populateIntent` preserves. */ +type CallerDeclarableRecipient = ITransactionRecipient & { tokenData?: EvmTokenDataDeclaration }; + +type CallerDeclarableParams = { + type?: string; + tokenName?: string; + recipients?: CallerDeclarableRecipient[]; + /** `fillNonce`: the address whose nonce is filled. */ + receiveAddress?: string; +}; + +/** True when the caller named a transaction type or recipients, i.e. declared its own intent. */ +export function callerDeclaresIntent(callerParams?: CallerDeclarableParams): boolean { + 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. A caller that declares a payment type (`transfer` / `payment` / + * `transferToken`) but no recipients is rejected by `verifyTssTransaction` before any handler + * runs, because nothing would be left to verify the bytes against; the stored intent is never + * used instead. + * - The caller declared nothing (signing a stored tx request, e.g. pending approval): the stored + * intent, which `resolveEffectiveTxParams` has already folded into `txParams`. + * + * `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, a recipient's flat and `tokenData` token identities + * are all kept (so conflicting declarations stay ambiguous), and only a source that declared no + * identity of its own inherits the stored one. `receiveAddress` comes from that same source, so a + * caller's declared receive address is never replaced by the stored intent's. The NFT fields of + * `tokenData` (`tokenType` / `tokenId` / `tokenQuantity`) follow the same precedence and are read + * from the stored intent directly when the effective params carry no `tokenData`. + */ +export function buildEvmTssIntentContext(params: { + intentType: string; + /** Effective params: back-filled from the stored intent when `storedIntent` is set. */ + txParams: CallerDeclarableParams; + /** The caller's own declared params. */ + callerParams?: CallerDeclarableParams; + /** Set only when the caller declared nothing and the stored intent speaks for the user. */ + storedIntent?: PopulatedIntent; + coinTokenContractAddress?: string; + baseAddress?: string; + batcherAddress?: string; +}): EvmTssIntentContext { + const { storedIntent } = params; + const source = storedIntent ? params.txParams : params.callerParams; + + const recipients: EvmTssExpectedRecipient[] = (source?.recipients ?? []).map((recipient, i) => { + // The effective params carry no `tokenData` on the pending-approval path, so the stored + // intent supplies it there. The NFT fields it carries (`tokenType` / `tokenId` / + // `tokenQuantity`) never take part in asset identity resolution. + const tokenData = recipient.tokenData ?? storedIntent?.recipients?.[i]?.tokenData; + // Token identity reaches a recipient both flat (`tokenAddress` / `tokenName`) and inside the + // intent `tokenData` shape that `populateIntent` keeps. Both are declarations of the source + // that speaks for the user, so both stay in resolution; the stored intent only fills in when + // the source declared no identity of its own. + const declaredTokenIdentities: EvmTokenIdentityDeclaration[] = []; + if (recipient.tokenAddress !== undefined || recipient.tokenName !== undefined) { + declaredTokenIdentities.push({ tokenContractAddress: recipient.tokenAddress, tokenName: recipient.tokenName }); + } + if (tokenData?.tokenContractAddress !== undefined || tokenData?.tokenName !== undefined) { + declaredTokenIdentities.push({ + tokenContractAddress: tokenData.tokenContractAddress, + tokenName: tokenData.tokenName, + }); + } + const storedTokenContractAddress = storedIntent?.recipients?.[i]?.tokenData?.tokenContractAddress; + if (storedTokenContractAddress !== undefined) { + declaredTokenIdentities.push({ tokenContractAddress: storedTokenContractAddress }); + } + const tokenContractAddress = + recipient.tokenAddress ?? tokenData?.tokenContractAddress ?? storedTokenContractAddress; + const tokenName = recipient.tokenName ?? tokenData?.tokenName; + return { + address: recipient.address, + amount: String(recipient.amount), + ...(recipient.data !== undefined && { data: recipient.data }), + ...(tokenName !== undefined && { tokenName }), + ...(tokenContractAddress !== undefined && { tokenContractAddress }), + ...(tokenData?.tokenType !== undefined && { tokenType: tokenData.tokenType }), + ...(tokenData?.tokenId !== undefined && { tokenId: tokenData.tokenId }), + ...(tokenData?.tokenQuantity !== undefined && { tokenQuantity: tokenData.tokenQuantity }), + ...(declaredTokenIdentities.length > 0 && { declaredTokenIdentities }), + }; + }); + + return { + intentType: params.intentType, + recipients, + tokenName: source?.tokenName ?? storedIntent?.tokenName, + coinTokenContractAddress: params.coinTokenContractAddress, + receiveAddress: source?.receiveAddress ?? storedIntent?.receiveAddress, + baseAddress: params.baseAddress, + batcherAddress: params.batcherAddress, + }; +} + +/** + * R1 — one recipient that declares raw calldata (WalletConnect / dApp): `to`, `value` and + * `data` must each be exactly equal to the intent. + */ +function verifyExactCall(ctx: EvmTssIntentContext, tx: DecodedEvmTx, recipient: EvmTssExpectedRecipient): TssFinding[] { + const warnings: TssFinding[] = []; + const expected = { to: recipient.address, value: recipient.amount, data: normalizeCalldata(recipient.data) }; + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + if (normalizeAddress(tx.to) !== normalizeAddress(recipient.address)) { + warnings.push( + violation(ctx, 'transaction to does not match the recipient address given by the client', expected, decoded) + ); + } + if (!valuesEqual(tx.value, recipient.amount)) { + warnings.push(violation(ctx, 'transaction value does not match the amount given by the client', expected, decoded)); + } + if (normalizeCalldata(tx.data) !== normalizeCalldata(recipient.data)) { + warnings.push( + violation(ctx, 'transaction calldata does not match the calldata given by the client', expected, decoded) + ); + } + return warnings; +} + +/** + * The payment checks that threw before fail-closed verification existed, kept fatal and in their + * original order (amount, then destination). Empty calldata must match `to` and `value`; ERC-20 + * `transfer(address,uint256)` calldata must match the inner recipient and amount. A WalletConnect + * recipient whose own calldata is an ERC-20 transfer supplies that expectation instead. + */ +function verifyHistoricPayment( + ctx: EvmTssIntentContext, + tx: DecodedEvmTx, + recipient: EvmTssExpectedRecipient +): TssFinding[] { + const findings: TssFinding[] = []; + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + // Compare and decode the canonical form only: `''`, `'0x'`, mixed case and a dropped `0x` + // prefix all describe the same bytes, so none of them may skip the selector gate below. + const calldata = normalizeCalldata(tx.data); + if (calldata === '0x') { + const expected = { to: recipient.address, value: recipient.amount, data: '0x' }; + if (!valuesEqual(tx.value, recipient.amount)) { + findings.push( + violation( + ctx, + 'the transaction amount in txPrebuild does not match the value given by client', + expected, + decoded, + 'fatal' + ) + ); + } + if (normalizeAddress(tx.to) !== normalizeAddress(recipient.address)) { + findings.push( + violation(ctx, 'destination address does not match with the recipient address', expected, decoded, 'fatal') + ); + } + return findings; + } + if (!calldata.startsWith(erc20TransferMethodId)) { + return findings; + } + + const [innerRecipient, innerAmount] = getRawDecoded( + ['address', 'uint256'], + getBufferedByteCode(erc20TransferMethodId, calldata) + ); + const decodedInner = { ...decoded, to: toAddressString(innerRecipient), value: toBigInt(innerAmount).toString() }; + const declaredData = recipient.data === undefined ? undefined : normalizeCalldata(recipient.data); + let expected = { to: recipient.address, value: recipient.amount, data: declaredData }; + if (declaredData && declaredData.startsWith(erc20TransferMethodId)) { + const [declaredRecipient, declaredAmount] = getRawDecoded( + ['address', 'uint256'], + getBufferedByteCode(erc20TransferMethodId, declaredData) + ); + expected = { ...expected, to: toAddressString(declaredRecipient), value: toBigInt(declaredAmount).toString() }; + } + if (!valuesEqual(decodedInner.value, expected.value)) { + findings.push( + violation( + ctx, + 'the transaction amount in txPrebuild does not match the value given by client', + expected, + decodedInner, + 'fatal' + ) + ); + } + if (decodedInner.to !== normalizeAddress(expected.to)) { + findings.push( + violation(ctx, 'destination address does not match with the recipient address', expected, decodedInner, 'fatal') + ); + } + return findings; +} + +/** + * R3 — one ERC-20 recipient: `to` must be the token contract, `value` must be zero and the + * decoded `transfer(address,uint256)` recipient and amount must match the intent. + * ERC-7984 `confidentialTransfer` (`0x5bebed7e` / `0x2fb74e62`) is checked on the recipient + * only, since the amount is encrypted. On a payment the ERC-20 recipient and amount are already + * fatal checks (`verifyHistoricPayment`), so `checkInnerTransfer` is false there. + */ +function verifyTokenTransfer( + ctx: EvmTssIntentContext, + tx: DecodedEvmTx, + recipient: EvmTssExpectedRecipient, + tokenContract: string, + checkInnerTransfer: boolean +): TssFinding[] { + const warnings: TssFinding[] = []; + const data = normalizeCalldata(tx.data); + const selector = data.slice(0, 10); + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + const expected = { to: tokenContract, value: '0', data: recipient.data }; + + if (!valuesEqual(tx.value, '0')) { + warnings.push(violation(ctx, 'token transfer must not carry native value', expected, decoded)); + } + + if (normalizeAddress(tx.to) !== normalizeAddress(tokenContract)) { + warnings.push(violation(ctx, 'transaction to is not the intent token contract', expected, decoded)); + } + + if (selector === erc20TransferMethodId) { + if (!checkInnerTransfer) { + return warnings; + } + let decodedRecipient: string; + let decodedAmount: bigint; + try { + const [innerRecipient, innerAmount] = getRawDecoded( + ['address', 'uint256'], + getBufferedByteCode(erc20TransferMethodId, data) + ); + decodedRecipient = toAddressString(innerRecipient); + decodedAmount = toBigInt(innerAmount); + } catch (e) { + warnings.push( + violation(ctx, `ERC-20 transfer calldata could not be decoded: ${(e as Error).message}`, expected, decoded) + ); + return warnings; + } + if (decodedRecipient !== normalizeAddress(recipient.address)) { + warnings.push( + violation( + ctx, + 'destination address does not match with the recipient address', + { ...expected, to: recipient.address, value: recipient.amount }, + { ...decoded, to: addHexPrefix(decodedRecipient), value: decodedAmount.toString() } + ) + ); + } + if (!valuesEqual(decodedAmount, recipient.amount)) { + warnings.push( + violation( + ctx, + 'the transaction amount in txPrebuild does not match the value given by client', + { ...expected, to: recipient.address, value: recipient.amount }, + { ...decoded, to: addHexPrefix(decodedRecipient), value: decodedAmount.toString() } + ) + ); + } + return warnings; + } + + if (selector === confidentialTransferNoProofMethodId || selector === confidentialTransferWithProofMethodId) { + const types = + selector === confidentialTransferNoProofMethodId + ? confidentialTransferNoProofTypes + : confidentialTransferWithProofTypes; + let decodedRecipient: string; + try { + const [innerRecipient] = getRawDecoded(types, getBufferedByteCode(selector, data)); + decodedRecipient = toAddressString(innerRecipient); + } catch (e) { + warnings.push( + violation(ctx, `confidentialTransfer calldata could not be decoded: ${(e as Error).message}`, expected, decoded) + ); + return warnings; + } + if (decodedRecipient !== normalizeAddress(recipient.address)) { + warnings.push( + violation( + ctx, + 'confidentialTransfer recipient does not match the recipient address given by the client', + { ...expected, to: recipient.address }, + { ...decoded, to: addHexPrefix(decodedRecipient) } + ) + ); + } + return warnings; + } + + warnings.push( + violation( + ctx, + 'token transfer must use ERC-20 transfer(address,uint256) or ERC-7984 confidentialTransfer calldata', + expected, + decoded + ) + ); + return warnings; +} + +/** The `tokenData.tokenType` values with a dedicated calldata handler, or undefined for the rest. */ +function normalizeNftTokenType(tokenType?: string): 'ERC721' | 'ERC1155' | undefined { + const normalized = tokenType?.trim().toUpperCase(); + return normalized === 'ERC721' || normalized === 'ERC1155' ? normalized : undefined; +} + +/** One calldata shape an NFT standard accepts, with the ABI types needed to decode it. */ +interface NftTransferShape { + selector: string; + types: string[]; + /** `safeBatchTransferFrom`: the id and quantity are single-element arrays of the declared values. */ + batch?: boolean; +} + +/** The ERC-721 transfer signatures this handler accepts. */ +const ERC721_TRANSFER_SHAPES: NftTransferShape[] = [ + // `safeTransferFrom(address,address,uint256)`: the ERC-721 overload without the data parameter. + { selector: ERC721SafeTransferFromMethodId, types: ERC721TransferFromTypes }, + // `safeTransferFrom(address,address,uint256,bytes)`: the ERC-721 overload with the data parameter. + { selector: ERC721SafeTransferTypeMethodId, types: ERC721SafeTransferTypes }, + // `transferFrom(address,address,uint256)`: Hedera HTS NFTs only accept this form. + { selector: ERC721TransferFromMethodId, types: ERC721TransferFromTypes }, +]; + +/** The ERC-1155 transfer signatures this handler accepts (single entry or one-entry batch). */ +const ERC1155_TRANSFER_SHAPES: NftTransferShape[] = [ + { selector: ERC1155SafeTransferTypeMethodId, types: ERC1155SafeTransferTypes }, + { selector: ERC1155BatchTransferTypeMethodId, types: ERC1155BatchTransferTypes, batch: true }, +]; + +/** + * R3b — one recipient that declares an NFT (`tokenData.tokenType`): `to` must be the collection + * contract, `value` must be zero, and the decoded transfer must move the declared token id (and, + * for ERC-1155, quantity) to the declared destination. Only the signatures listed for the + * declared standard are accepted; a selector of the other standard, or an unknown one, is a + * violation. The sender (`_from`) is not compared: it is the wallet or one of its forwarders, and + * the intent never names it. + */ +function verifyNftTokenTransfer( + ctx: EvmTssIntentContext, + tx: DecodedEvmTx, + recipient: EvmTssExpectedRecipient, + tokenContract: string, + tokenType: 'ERC721' | 'ERC1155' +): TssFinding[] { + const findings: TssFinding[] = []; + const data = normalizeCalldata(tx.data); + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + const expected = { to: tokenContract, value: '0', data: recipient.data }; + // Findings that are about the token id / quantity still name the destination pair, so the + // report says which transfer was wrong without relabelling an id as a native amount. + const expectedTransfer = { ...expected, to: recipient.address }; + const isErc721 = tokenType === 'ERC721'; + + if (!valuesEqual(tx.value, '0')) { + findings.push(violation(ctx, 'token transfer must not carry native value', expected, decoded)); + } + if (normalizeAddress(tx.to) !== normalizeAddress(tokenContract)) { + findings.push(violation(ctx, 'transaction to is not the intent token contract', expected, decoded)); + } + + const shape = (isErc721 ? ERC721_TRANSFER_SHAPES : ERC1155_TRANSFER_SHAPES).find( + (candidate) => candidate.selector === data.slice(0, 10) + ); + if (!shape) { + findings.push( + violation( + ctx, + `${tokenType} token transfer must use ${ + isErc721 + ? 'safeTransferFrom(address,address,uint256[,bytes]) or transferFrom(address,address,uint256)' + : 'safeTransferFrom(address,address,uint256,uint256,bytes) or safeBatchTransferFrom(address,address,uint256[],uint256[],bytes)' + } calldata`, + expected, + decoded + ) + ); + return findings; + } + + const tokenId = recipient.tokenId; + const tokenQuantity = recipient.tokenQuantity; + if (tokenId === undefined) { + findings.push(violation(ctx, `${tokenType} token transfer must declare tokenData.tokenId`, expected, decoded)); + return findings; + } + if (!isErc721 && tokenQuantity === undefined) { + findings.push(violation(ctx, 'ERC1155 token transfer must declare tokenData.tokenQuantity', expected, decoded)); + return findings; + } + if (isErc721 && tokenQuantity !== undefined && !valuesEqual(tokenQuantity, '1')) { + findings.push(violation(ctx, 'ERC721 token transfer must declare tokenData.tokenQuantity of 1', expected, decoded)); + } + + let decodedRecipient: string; + let decodedIds: string[]; + let decodedQuantities: string[] | undefined; + try { + // The selector is already matched at offset 0, so decode exactly the arguments after it. + // `getBufferedByteCode` splits the calldata on the selector string, which is wrong here: an + // argument (a tokenId is 32 bytes) may legitimately contain the selector's bytes. + const raw = getRawDecoded(shape.types, Buffer.from(data.slice(10), 'hex')); + decodedRecipient = toAddressString(raw[1]); + if (shape.batch) { + decodedIds = (raw[2] as unknown[]).map((id) => toBigInt(id).toString()); + decodedQuantities = (raw[3] as unknown[]).map((quantity) => toBigInt(quantity).toString()); + } else { + decodedIds = [toBigInt(raw[2]).toString()]; + decodedQuantities = isErc721 ? undefined : [toBigInt(raw[3]).toString()]; + } + } catch (e) { + findings.push( + violation(ctx, `${tokenType} transfer calldata could not be decoded: ${(e as Error).message}`, expected, decoded) + ); + return findings; + } + + const decodedTransfer = { ...decoded, to: addHexPrefix(decodedRecipient) }; + if (normalizeAddress(decodedRecipient) !== normalizeAddress(recipient.address)) { + findings.push( + violation(ctx, 'destination address does not match with the recipient address', expectedTransfer, decodedTransfer) + ); + } + + if (shape.batch && (decodedIds.length !== 1 || decodedQuantities?.length !== 1)) { + findings.push( + violation( + ctx, + `ERC1155 batch transfer must move exactly the declared tokenId and tokenQuantity (decoded ` + + `${decodedIds.length} ids / ${decodedQuantities?.length ?? 0} quantities)`, + expectedTransfer, + decodedTransfer + ) + ); + return findings; + } + if (!valuesEqual(decodedIds[0], tokenId)) { + findings.push( + violation( + ctx, + `${tokenType} tokenId in calldata does not match tokenData.tokenId (expected ${tokenId}, decoded ${decodedIds[0]})`, + expectedTransfer, + decodedTransfer + ) + ); + } + if (!isErc721 && decodedQuantities !== undefined && !valuesEqual(decodedQuantities[0], tokenQuantity)) { + findings.push( + violation( + ctx, + `ERC1155 tokenQuantity in calldata does not match tokenData.tokenQuantity (expected ${tokenQuantity}, decoded ${decodedQuantities[0]})`, + expectedTransfer, + decodedTransfer + ) + ); + } + return findings; +} + +/** + * R4 — more than one recipient: `to` must be the network batcher contract and the decoded + * batch arrays must match the intent 1:1. Native `batch(address[],uint256[])` carries + * `value` = the sum of amounts; token `batchTransferFrom(address,address[],uint256[])` + * names the intent token contract and carries `value` = 0. + */ +function verifyBatch(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { + const warnings: TssFinding[] = []; + const data = normalizeCalldata(tx.data); + const selector = data.slice(0, 10); + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + const expectedTotal = ctx.recipients.reduce((sum, r) => sum + toBigInt(r.amount), 0n).toString(); + const expected = { to: ctx.batcherAddress, value: expectedTotal, data: undefined }; + + if (!ctx.batcherAddress) { + warnings.push( + violation( + ctx, + 'no batcher contract address is configured for this network; batch payment cannot be verified', + expected, + decoded + ) + ); + return warnings; + } + if (normalizeAddress(tx.to) !== normalizeAddress(ctx.batcherAddress)) { + warnings.push( + violation(ctx, 'batch transaction to does not match the network batcher contract', expected, decoded) + ); + } + const assets = ctx.recipients.map((recipient) => resolveAsset(ctx, recipient)); + const unresolvedAsset = assets.find((asset) => asset.kind === 'unresolved'); + if (unresolvedAsset?.kind === 'unresolved') { + warnings.push(violation(ctx, `batch payment: ${unresolvedAsset.reason}`, expected, decoded)); + return warnings; + } + const batchAsset = assets[0]; + const mixedAssets = assets.some( + (asset) => + asset.kind !== batchAsset.kind || + (asset.kind === 'token' && + batchAsset.kind === 'token' && + normalizeAddress(asset.contract) !== normalizeAddress(batchAsset.contract)) + ); + if (mixedAssets) { + warnings.push( + violation( + ctx, + 'batch payment recipients declare more than one asset: cannot verify one batch call', + expected, + decoded + ) + ); + return warnings; + } + + const compareRecipients = ( + recipients: { address: string; amount: string }[], + valueExpected: string, + valueReason: string + ): void => { + if (recipients.length !== ctx.recipients.length) { + warnings.push( + violation( + ctx, + `batch calldata recipient count (${recipients.length}) does not match intent recipients (${ctx.recipients.length})`, + expected, + decoded + ) + ); + return; + } + for (let i = 0; i < recipients.length; i++) { + const want = ctx.recipients[i]; + const got = recipients[i]; + if (normalizeAddress(got.address) !== normalizeAddress(want.address) || !valuesEqual(got.amount, want.amount)) { + warnings.push( + violation( + ctx, + `batch calldata entry ${i} does not match intent recipient`, + { ...expected, to: want.address, value: want.amount }, + { ...decoded, to: got.address, value: got.amount } + ) + ); + } + } + if (!valuesEqual(tx.value, valueExpected)) { + warnings.push(violation(ctx, valueReason, { ...expected, value: valueExpected }, decoded)); + } + }; + + if (selector === batchMethodId) { + if (batchAsset.kind !== 'native') { + warnings.push( + violation( + ctx, + 'token batch payment must use batchTransferFrom(address,address[],uint256[]), not native batch(address[],uint256[])', + expected, + decoded + ) + ); + return warnings; + } + let batch: BatchTransferData; + try { + batch = decodeBatchTransferData(data); + } catch (e) { + warnings.push(violation(ctx, `batch calldata could not be decoded: ${(e as Error).message}`, expected, decoded)); + return warnings; + } + compareRecipients( + batch.recipients, + expectedTotal, + 'batch transaction value does not match the sum of intent amounts' + ); + return warnings; + } + + if (selector === batchTransferFromMethodId) { + if (batchAsset.kind !== 'token') { + warnings.push( + violation( + ctx, + 'native batch payment must use batch(address[],uint256[]), not batchTransferFrom(address,address[],uint256[])', + expected, + decoded + ) + ); + return warnings; + } + let batch: BatchTransferFromData; + try { + batch = decodeBatchTransferFromData(data); + } catch (e) { + warnings.push( + violation(ctx, `batchTransferFrom calldata could not be decoded: ${(e as Error).message}`, expected, decoded) + ); + return warnings; + } + if (normalizeAddress(batch.tokenContractAddress) !== normalizeAddress(batchAsset.contract)) { + warnings.push( + violation( + ctx, + 'batchTransferFrom token contract does not match the intent token contract', + { ...expected, to: batchAsset.contract }, + { ...decoded, to: batch.tokenContractAddress } + ) + ); + } + compareRecipients(batch.recipients, '0', 'token batch transaction must not carry native value'); + return warnings; + } + + warnings.push( + violation( + ctx, + 'batch payment must use batch(address[],uint256[]) or batchTransferFrom(address,address[],uint256[]) calldata', + expected, + decoded + ) + ); + return warnings; +} + +/** Payment handler (`transfer` / `payment`) — rules R1–R5. */ +function verifyPayment(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + if (ctx.recipients.length === 0) { + return [ + violation( + ctx, + 'payment intent has no recipients: the transaction bytes cannot be verified against intent', + {}, + decoded + ), + ]; + } + if (ctx.recipients.length > 1) { + return verifyBatch(ctx, tx); + } + const recipient = ctx.recipients[0]; + return [...verifyHistoricPayment(ctx, tx, recipient), ...verifyPaymentShape(ctx, tx, recipient)]; +} + +/** + * The new checks for a single-recipient payment: the calldata shape must match the declared + * asset. The fields `verifyHistoricPayment` already compares fatally are not compared again. + */ +function verifyPaymentShape( + ctx: EvmTssIntentContext, + tx: DecodedEvmTx, + recipient: EvmTssExpectedRecipient +): TssFinding[] { + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + if (recipient.data !== undefined) { + return verifyExactCall(ctx, tx, recipient); + } + const asset = resolveAsset(ctx, recipient); + if (asset.kind === 'unresolved') { + return [violation(ctx, `payment ${asset.reason}`, { to: recipient.address, value: recipient.amount }, decoded)]; + } + const selector = normalizeCalldata(tx.data).slice(0, 10); + const isTokenShape = + selector === erc20TransferMethodId || + selector === confidentialTransferNoProofMethodId || + selector === confidentialTransferWithProofMethodId; + const isBatchShape = selector === batchMethodId || selector === batchTransferFromMethodId; + if (asset.kind === 'native') { + if (isEmptyCalldata(tx.data)) { + // `to` and `value` of a native send are fatal checks in verifyHistoricPayment. + return []; + } + if (isTokenShape || isBatchShape) { + return [ + violation( + ctx, + 'payment intent declares a native asset but the transaction carries token or batch calldata', + { to: recipient.address, value: recipient.amount, data: '0x' }, + decoded + ), + ]; + } + } else { + if (isEmptyCalldata(tx.data)) { + return [ + violation( + ctx, + 'token payment intent but the transaction is a native send: expected ERC-20 transfer calldata', + { to: asset.contract, value: '0', data: undefined }, + decoded + ), + ]; + } + if (isTokenShape) { + return verifyTokenTransfer(ctx, tx, recipient, asset.contract, false); + } + if (isBatchShape) { + return [ + violation( + ctx, + 'batch calldata cannot be verified against a single declared recipient', + { to: recipient.address, value: recipient.amount }, + decoded + ), + ]; + } + } + // R5 — anything else (e.g. `approve(attacker, MAX)` disguised as a payment) is a violation. + return [ + violation( + ctx, + 'payment intent calldata is not a recognized payment shape (native send, ERC-20 transfer, ' + + 'ERC-7984 confidentialTransfer, or batch): refusing to treat it as a payment', + { to: recipient.address, value: recipient.amount, data: '0x' }, + decoded + ), + ]; +} + +/** + * `transferToken` handler — exactly one recipient, which must move a token. A recipient that + * declares `tokenData.tokenType` ERC-721 / ERC-1155 is verified against that standard's transfer + * calldata (R3b); every other token keeps the R3 ERC-20 / ERC-7984 shapes. + */ +function verifyTransferToken(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + if (ctx.recipients.length !== 1) { + return [ + violation( + ctx, + `transferToken intent must declare exactly one token recipient (found ${ctx.recipients.length})`, + {}, + decoded + ), + ]; + } + const recipient = ctx.recipients[0]; + // A declared NFT standard picks its own calldata set. `tokenData.tokenName` is display metadata + // only when it is that standard's statics placeholder, so the declared collection contract (or a + // name that resolves to a real one) anchors the transfer. + const nftTokenType = normalizeNftTokenType(recipient.tokenType); + const asset = resolveAsset(ctx, recipient, nftTokenType); + if (asset.kind === 'unresolved') { + return [ + violation(ctx, `transferToken ${asset.reason}`, { to: recipient.address, value: recipient.amount }, decoded), + ]; + } + 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 (nftTokenType) { + return verifyNftTokenTransfer(ctx, tx, recipient, asset.contract, nftTokenType); + } + if (isEmptyCalldata(tx.data)) { + return [ + violation( + ctx, + 'transferToken transaction must use ERC-20 transfer(address,uint256) calldata; expected token calldata, got an empty data payload', + { to: asset.contract, value: '0' }, + decoded + ), + ]; + } + return verifyTokenTransfer(ctx, tx, recipient, asset.contract, true); +} + +/** + * `tokenApproval` handler — `to` must be the token contract and the calldata must be + * `approve(spender = batcher, MAX)`, with `value` = 0. Guards against a substituted + * `approve(attacker, MAX_UINT256)`. + */ +function verifyTokenApproval(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { + const warnings: TssFinding[] = []; + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + const calldata = normalizeCalldata(tx.data); + const asset = resolveAsset(ctx, {}); + const tokenContract = asset.kind === 'token' ? asset.contract : undefined; + const expected = { to: tokenContract, value: '0', data: undefined }; + + if (!valuesEqual(tx.value, '0')) { + warnings.push(violation(ctx, 'tokenApproval transaction must not carry native value', expected, decoded)); + } + if (!tokenContract) { + warnings.push( + violation( + ctx, + asset.kind === 'unresolved' + ? `tokenApproval ${asset.reason}` + : `tokenApproval intent names no token contract that can be resolved from statics ('${ + ctx.tokenName ?? '' + }'); transaction to cannot be verified`, + expected, + decoded + ) + ); + } else if (normalizeAddress(tx.to) !== normalizeAddress(tokenContract)) { + warnings.push(violation(ctx, 'tokenApproval transaction to is not the intent token contract', expected, decoded)); + } + + let spender: string; + let amount: string; + try { + const decodedApprove = decodeApproveCalldata(calldata); + spender = normalizeAddress(decodedApprove.spender); + amount = decodedApprove.amount; + } catch (e) { + warnings.push( + violation( + ctx, + `tokenApproval transaction must use ERC-20 approve(address,uint256) calldata: ${(e as Error).message}`, + expected, + decoded + ) + ); + return warnings; + } + + if (!ctx.batcherAddress) { + warnings.push( + violation( + ctx, + 'no batcher contract address is configured for this network; tokenApproval spender cannot be verified', + expected, + { ...decoded, to: spender, value: amount } + ) + ); + } else if (normalizeAddress(spender) !== normalizeAddress(ctx.batcherAddress)) { + warnings.push( + violation( + ctx, + 'tokenApproval spender is not the network batcher contract', + { ...expected, to: ctx.batcherAddress }, + { ...decoded, to: spender, value: amount } + ) + ); + } + if (!valuesEqual(amount, MAX_UINT256)) { + warnings.push( + violation( + ctx, + 'tokenApproval amount is not the maximum ERC-20 allowance', + { ...expected, value: MAX_UINT256 }, + { ...decoded, to: spender, value: amount } + ) + ); + } + return warnings; +} + +/** + * `fillNonce` handler — a 1-wei self-send to the address whose nonce is being filled: the + * declared `receiveAddress` (the wallet's chosen receive address) when the intent names one, + * otherwise the wallet base address. + */ +function verifyFillNonce(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { + const warnings: TssFinding[] = []; + const decoded = { to: tx.to, value: tx.value, data: tx.data }; + const expectedTo = ctx.receiveAddress ?? ctx.baseAddress; + const expected = { to: expectedTo, value: '1', data: '0x' }; + if (!expectedTo) { + warnings.push( + violation( + ctx, + 'fillNonce intent has no receive address or wallet base address to verify against', + expected, + decoded + ) + ); + return warnings; + } + if (normalizeAddress(tx.to) !== normalizeAddress(expectedTo)) { + warnings.push( + violation( + ctx, + ctx.receiveAddress + ? 'fillNonce transaction must send to the declared receive address' + : 'fillNonce transaction must send to the wallet base address', + expected, + decoded + ) + ); + } + if (!valuesEqual(tx.value, '1')) { + warnings.push(violation(ctx, 'fillNonce transaction must send exactly 1 wei', expected, decoded)); + } + if (!isEmptyCalldata(tx.data)) { + warnings.push(violation(ctx, 'fillNonce transaction must not carry calldata', expected, decoded)); + } + return warnings; +} + +function unhandledTypeViolation(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding { + return violation( + ctx, + `no fail-closed verifier for intent type '${ctx.intentType}': transaction bytes were not compared against intent`, + {}, + { to: tx.to, value: tx.value, data: tx.data }, + 'unhandled' + ); +} + +/** + * A consolidation must pay the wallet base address: a native send to it, or an ERC-20 transfer + * to it that carries no native value. Without the value check, `transfer(baseAddress, 0)` + * calldata sent to an arbitrary contract would move the native balance there. + */ +export function verifyConsolidation(tx: DecodedEvmTx, baseAddress: string, severity: TssFindingSeverity): TssFinding[] { + const ctx = { intentType: 'consolidate' }; + const expected = { to: baseAddress }; + const decoded = { to: tx.to, value: tx.value }; + const calldata = normalizeCalldata(tx.data); + if (!calldata.startsWith(erc20TransferMethodId)) { + return normalizeAddress(tx.to) === normalizeAddress(baseAddress) + ? [] + : [ + violation( + ctx, + 'Consolidation transaction recipient does not match wallet base address', + expected, + decoded, + severity + ), + ]; + } + // For ERC-20 consolidations `to` is the token contract; the real recipient is in the calldata. + const findings: TssFinding[] = []; + const [innerRecipient] = getRawDecoded(['address', 'uint256'], getBufferedByteCode(erc20TransferMethodId, calldata)); + const decodedRecipient = toAddressString(innerRecipient); + if (decodedRecipient !== normalizeAddress(baseAddress)) { + findings.push( + violation( + ctx, + 'Consolidation transaction recipient does not match wallet base address', + expected, + { ...decoded, to: decodedRecipient }, + severity + ) + ); + } + if (!valuesEqual(tx.value, '0')) { + findings.push( + violation( + ctx, + 'Consolidation token transfer must not carry native value', + { ...expected, value: '0' }, + decoded, + severity + ) + ); + } + return findings; +} + +/** + * Evaluate the decoded transaction against the intent. Returns every violated rule for the + * canonical intent type; an empty array means the bytes match the closed set of accepted + * shapes for that type. + */ +export function evaluateEvmTssIntent(ctx: EvmTssIntentContext, tx: DecodedEvmTx): TssFinding[] { + switch (ctx.intentType) { + case 'transfer': + return verifyPayment(ctx, tx); + case 'transferToken': + return verifyTransferToken(ctx, tx); + case 'tokenApproval': + return verifyTokenApproval(ctx, tx); + case 'fillNonce': + return verifyFillNonce(ctx, tx); + case 'consolidate': + // The base-address check in verifyTssTransaction is the handler for consolidations. + return []; + case 'defiApprove': + case 'defiDeposit': + case 'defiWithdraw': + case 'wrapApprove': + // Caller-declared legs carry dedicated fail-closed calldata checks in verifyTssTransaction + // (stored defi-* intents are rejected there before those checks run). + return []; + default: + return [unhandledTypeViolation(ctx, tx)]; + } +} diff --git a/modules/sdk-api/src/bitgoAPI.ts b/modules/sdk-api/src/bitgoAPI.ts index 16bbb40b7d8..5eb419557bc 100644 --- a/modules/sdk-api/src/bitgoAPI.ts +++ b/modules/sdk-api/src/bitgoAPI.ts @@ -24,6 +24,7 @@ import { IRequestTracer, makeRandomKey, sanitizeLegacyPath, + TssVerificationWarningHandler, } from '@bitgo/sdk-core'; import * as sdkHmac from '@bitgo/sdk-hmac'; import { DefaultHmacAuthStrategy, type IHmacAuthStrategy } from '@bitgo/sdk-hmac'; @@ -283,10 +284,16 @@ export class BitGoAPI implements BitGoBase { private _requestIdPrefix?: string; private getAdditionalHeadersCb?: AdditionalHeadersCallback; protected _hmacAuthStrategy: IHmacAuthStrategy; + /** @inheritDoc */ + public readonly strictTssVerification: boolean; + /** @inheritDoc */ + public readonly onVerificationWarning?: TssVerificationWarningHandler; constructor(params: BitGoAPIOptions = {}) { this.getAdditionalHeadersCb = params.getAdditionalHeadersCb; this._requestIdPrefix = params.requestIdPrefix; + this.strictTssVerification = params.strictTssVerification ?? false; + this.onVerificationWarning = params.onVerificationWarning; this.cookiesPropagationEnabled = false; if ( !common.validateParams( diff --git a/modules/sdk-api/src/types.ts b/modules/sdk-api/src/types.ts index 8e51d6d8d1f..d1136b8af9d 100644 --- a/modules/sdk-api/src/types.ts +++ b/modules/sdk-api/src/types.ts @@ -1,4 +1,10 @@ -import { EncryptionVersion, EnvironmentName, IRequestTracer, V1Network } from '@bitgo/sdk-core'; +import { + EncryptionVersion, + EnvironmentName, + IRequestTracer, + TssVerificationWarningHandler, + V1Network, +} from '@bitgo/sdk-core'; import { type BitGoTlsPolicy } from './tlsPolicy'; import { ECPairInterface } from '@bitgo/utxo-lib'; import { type Agent } from 'http'; @@ -76,6 +82,16 @@ export interface BitGoAPIOptions { cookiesPropagationEnabled?: boolean; getAdditionalHeadersCb?: AdditionalHeadersCallback; requestIdPrefix?: string; + /** + * Client-side opt-in: fail-closed EVM TSS verification warnings become hard failures + * instead of `console.warn` + `onVerificationWarning`. Never read from server data. + */ + strictTssVerification?: boolean; + /** + * Sink for fail-closed EVM TSS verification warnings (decoded vs expected `to` / + * `value` / `data` for a transaction about to be signed). + */ + onVerificationWarning?: TssVerificationWarningHandler; evm?: { [key: string]: { baseUrl?: string; diff --git a/modules/sdk-coin-bsc/test/unit/tssBatcherVerification.ts b/modules/sdk-coin-bsc/test/unit/tssBatcherVerification.ts new file mode 100644 index 00000000000..e69e5596489 --- /dev/null +++ b/modules/sdk-coin-bsc/test/unit/tssBatcherVerification.ts @@ -0,0 +1,312 @@ +import 'should'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; +import { coins, ContractAddressDefinedToken, Erc7984Coin } from '@bitgo/statics'; +import { TransactionType, TssVerificationWarning, Wallet } from '@bitgo/sdk-core'; +import { optionalDeps } from '@bitgo/abstract-eth'; + +import { Tbsc } from '../../src'; +import { TransactionBuilder } from '../../src/lib'; + +/** + * Fail-closed EVM TSS verification for batcher-contract flows (CECHO-2460). + * + * Native batches (`batch(address[],uint256[])`) and token batches + * (`batchTransferFrom(address,address[],uint256[])`) must reach the network's batcher + * contract with the intent's recipients 1:1, and `tokenApproval` must be + * `approve(batcher, MAX)` on the intent token contract. tbsc is used because its statics + * network carries a real batcher contract address. + */ + +const recipientAddress = '0x174cfd823af8ce27ed0afee3fcf3c3ba259116be'; +const attackerAddress = '0x7e85bdc27c050e3905ebf4b8e634d9ad6edd0de6'; +const tokenContractAddress = '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48'; +const MAX_UINT256 = ((1n << 256n) - 1n).toString(); + +const erc7984Pair = coins.get('hteth:cusdt') as Erc7984Coin; +const erc7984WrapperAddress = String(erc7984Pair.contractAddress); + +// Two distinct tbsc tokens, so a batch can be shown to move exactly one asset. +const tokenContractOf = (tokenName: string): string => { + const token = coins.get(tokenName); + if (!(token instanceof ContractAddressDefinedToken)) { + throw new Error(`${tokenName} has no contract address in statics`); + } + return String(token.contractAddress); +}; +const busdTokenName = 'tbsc:busd'; +const usd1TokenName = 'tbsc:usd1'; +const busdContractAddress = tokenContractOf(busdTokenName); + +const word = (value: bigint): string => value.toString(16).padStart(64, '0'); +const addrWord = (address: string): string => address.toLowerCase().replace(/^0x/, '').padStart(64, '0'); + +function encodeApprove(spender: string, amount: string): string { + return `0x095ea7b3${addrWord(spender)}${word(BigInt(amount))}`; +} + +function encodeWithAbi(signature: string, types: string[], values: unknown[]): string { + const selector = optionalDeps.ethAbi.methodID(signature, types); + const args = optionalDeps.ethAbi.rawEncode(types, values); + return '0x' + Buffer.concat([selector, args]).toString('hex'); +} + +function encodeBatch(addresses: string[], amounts: string[]): string { + return encodeWithAbi('batch', ['address[]', 'uint256[]'], [addresses, amounts]); +} + +function encodeBatchTransferFrom(token: string, addresses: string[], amounts: string[]): string { + return encodeWithAbi('batchTransferFrom', ['address', 'address[]', 'uint256[]'], [token, addresses, amounts]); +} + +async function buildTxHex(opts: { to: string; value?: string; data?: string }): Promise { + const builder = new TransactionBuilder(coins.get('tbsc')); + builder.type(opts.data && opts.data !== '0x' ? TransactionType.ContractCall : TransactionType.SingleSigSend); + builder.fee({ fee: '10', gasLimit: '21000' }); + builder.counter(1); + builder.contract(opts.to); + if (opts.data) { + builder.data(opts.data); + } + if (opts.value) { + builder.value(opts.value); + } + return (await builder.build()).toBroadcastFormat(); +} + +describe('EVM TSS fail-closed verification (batcher contract flows)', function () { + const batcherAddress = String( + (coins.get('tbsc').network as unknown as { batcherContractAddress: string }).batcherContractAddress + ); + + let bitgo: TestBitGoAPI; + let strictBitgo: TestBitGoAPI; + let warnings: TssVerificationWarning[]; + let coin: Tbsc; + let wallet: Wallet; + + beforeEach(function () { + warnings = []; + bitgo = TestBitGo.decorate(BitGoAPI, { + env: 'test', + onVerificationWarning: (warning) => warnings.push(warning), + }); + strictBitgo = TestBitGo.decorate(BitGoAPI, { + env: 'test', + strictTssVerification: true, + }); + bitgo.safeRegister('tbsc', Tbsc.createInstance); + bitgo.initializeTestVars(); + strictBitgo.safeRegister('tbsc', Tbsc.createInstance); + strictBitgo.initializeTestVars(); + coin = bitgo.coin('tbsc') as Tbsc; + wallet = new Wallet( + bitgo, + coin as any, + { + coinSpecific: { baseAddress: recipientAddress }, + } as any + ); + }); + + const verify = (txParams: Record, txHex: string, strict = false): Promise => { + const target = strict ? (strictBitgo.coin('tbsc') as Tbsc) : coin; + const w = strict + ? new Wallet(strictBitgo, target as any, { coinSpecific: { baseAddress: recipientAddress } } as any) + : wallet; + return (target as any).verifyTransaction({ + txParams, + txPrebuild: { txHex, coin: 'tbsc', walletId: 'w' }, + wallet: w, + verification: {}, + walletType: 'tss', + }); + }; + + it('pins the hardcoded batch selectors to their runtime-computed values', function () { + encodeBatch([recipientAddress], ['1']).slice(0, 10).should.equal('0xc00c4e9e'); + encodeBatchTransferFrom(tokenContractAddress, [recipientAddress], ['1']).slice(0, 10).should.equal('0x4885b254'); + }); + + describe('native batch', function () { + const recipients = [ + { address: recipientAddress, amount: '10' }, + { address: attackerAddress, amount: '20' }, + ]; + + it('verifies batch calldata against the batcher contract and the intent arrays', async function () { + const txHex = await buildTxHex({ + to: batcherAddress, + value: '30', + data: encodeBatch([recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients }, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('flags swapped inner recipients and inflated batch value', async function () { + const swappedHex = await buildTxHex({ + to: batcherAddress, + value: '30', + data: encodeBatch([attackerAddress, recipientAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients }, swappedHex)).should.equal(true); + warnings.some((w) => /entry 0 does not match/.test(w.reason)).should.equal(true); + await verify({ type: 'transfer', recipients }, swappedHex, true).should.be.rejectedWith(/entry 0 does not match/); + + warnings = []; + const inflatedHex = await buildTxHex({ + to: batcherAddress, + value: '999', + data: encodeBatch([recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients }, inflatedHex)).should.equal(true); + warnings.some((w) => /value does not match the sum/.test(w.reason)).should.equal(true); + }); + + it('flags a batch sent somewhere other than the network batcher contract', async function () { + const txHex = await buildTxHex({ + to: attackerAddress, + value: '30', + data: encodeBatch([recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients }, txHex)).should.equal(true); + warnings.some((w) => /does not match the network batcher contract/.test(w.reason)).should.equal(true); + }); + + it('flags batch-shaped sends whose calldata is not batch or batchTransferFrom', async function () { + const txHex = await buildTxHex({ + to: batcherAddress, + value: '30', + data: encodeWithAbi('transfer', ['address', 'uint256'], [recipientAddress, '30']), + }); + (await verify({ type: 'transfer', recipients }, txHex)).should.equal(true); + warnings.some((w) => /must use batch\(address\[\],uint256\[\]\)/.test(w.reason)).should.equal(true); + }); + }); + + describe('token batch', function () { + const recipients = [ + { address: recipientAddress, amount: '10', tokenName: 'hteth:cusdt' }, + { address: attackerAddress, amount: '20', tokenName: 'hteth:cusdt' }, + ]; + + it('verifies batchTransferFrom against the intent token contract with zero value', async function () { + const txHex = await buildTxHex({ + to: batcherAddress, + value: '0', + data: encodeBatchTransferFrom(erc7984WrapperAddress, [recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients }, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('flags a token batch whose token contract is not the intent token', async function () { + const txHex = await buildTxHex({ + to: batcherAddress, + value: '0', + data: encodeBatchTransferFrom(tokenContractAddress, [recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients }, txHex)).should.equal(true); + warnings.some((w) => /token contract does not match/.test(w.reason)).should.equal(true); + await verify({ type: 'transfer', recipients }, txHex, true).should.be.rejectedWith( + /token contract does not match/ + ); + }); + }); + + describe('batch asset binding (CECHO-2460)', function () { + const nativeRecipients = [ + { address: recipientAddress, amount: '10' }, + { address: attackerAddress, amount: '20' }, + ]; + const tokenRecipients = [ + { address: recipientAddress, amount: '10', tokenName: busdTokenName }, + { address: attackerAddress, amount: '20', tokenName: busdTokenName }, + ]; + + it('rejects native batch calldata for a token batch intent', async function () { + const txHex = await buildTxHex({ + to: batcherAddress, + value: '30', + data: encodeBatch([recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients: tokenRecipients }, txHex)).should.equal(true); + warnings.some((w) => /token batch payment must use batchTransferFrom/.test(w.reason)).should.equal(true); + await verify({ type: 'transfer', recipients: tokenRecipients }, txHex, true).should.be.rejectedWith( + /token batch payment must use batchTransferFrom/ + ); + }); + + it('rejects batchTransferFrom calldata for a native batch intent', async function () { + const txHex = await buildTxHex({ + to: batcherAddress, + value: '0', + data: encodeBatchTransferFrom(busdContractAddress, [recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients: nativeRecipients }, txHex)).should.equal(true); + warnings + .some((w) => /native batch payment must use batch\(address\[\],uint256\[\]\)/.test(w.reason)) + .should.equal(true); + await verify({ type: 'transfer', recipients: nativeRecipients }, txHex, true).should.be.rejectedWith( + /native batch payment must use batch\(address\[\],uint256\[\]\)/ + ); + }); + + it('rejects a batch whose recipients declare two different tokens', async function () { + const mixedRecipients = [ + { address: recipientAddress, amount: '10', tokenName: busdTokenName }, + { address: attackerAddress, amount: '20', tokenName: usd1TokenName }, + ]; + const txHex = await buildTxHex({ + to: batcherAddress, + value: '0', + data: encodeBatchTransferFrom(busdContractAddress, [recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients: mixedRecipients }, txHex)).should.equal(true); + warnings.some((w) => /more than one asset/.test(w.reason)).should.equal(true); + await verify({ type: 'transfer', recipients: mixedRecipients }, txHex, true).should.be.rejectedWith( + /more than one asset/ + ); + }); + + it('rejects a batch with an unresolvable token identity', async function () { + const unresolvedRecipients = [ + { address: recipientAddress, amount: '10', tokenName: 'tbsc:not-a-registered-token' }, + { address: attackerAddress, amount: '20', tokenName: 'tbsc:not-a-registered-token' }, + ]; + const txHex = await buildTxHex({ + to: batcherAddress, + value: '0', + data: encodeBatchTransferFrom(busdContractAddress, [recipientAddress, attackerAddress], ['10', '20']), + }); + (await verify({ type: 'transfer', recipients: unresolvedRecipients }, txHex)).should.equal(true); + warnings.some((w) => /cannot be resolved to a token contract/.test(w.reason)).should.equal(true); + }); + }); + + describe('tokenApproval', function () { + const approvalParams = { type: 'tokenApproval', tokenName: 'hteth:cusdt' }; + + it('verifies approve(batcher, MAX) on the intent token contract', async function () { + const txHex = await buildTxHex({ + to: erc7984WrapperAddress, + value: '0', + data: encodeApprove(batcherAddress, MAX_UINT256), + }); + (await verify(approvalParams, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('flags approve(attacker, MAX) under a tokenApproval intent (TOB-BITGOEDMPC-2)', async function () { + const txHex = await buildTxHex({ + to: erc7984WrapperAddress, + value: '0', + data: encodeApprove(attackerAddress, MAX_UINT256), + }); + (await verify(approvalParams, txHex)).should.equal(true); + warnings.some((w) => /spender is not the network batcher contract/.test(w.reason)).should.equal(true); + await verify(approvalParams, txHex, true).should.be.rejectedWith(/spender is not the network batcher contract/); + }); + }); +}); diff --git a/modules/sdk-coin-eth/test/unit/eth.ts b/modules/sdk-coin-eth/test/unit/eth.ts index 9eeb9106d37..24494b02463 100644 --- a/modules/sdk-coin-eth/test/unit/eth.ts +++ b/modules/sdk-coin-eth/test/unit/eth.ts @@ -7,6 +7,7 @@ import * as secp256k1 from 'secp256k1'; import request from 'superagent'; import { common, + DefiIntentParams, ErrorNoInputToRecover, generateRandomPassword, InvalidAddressError, @@ -15,6 +16,7 @@ import { RecoveryKeyMismatchError, RecoveryProviderError, resolveEffectiveTxParams, + TransactionParams, TransactionType, TxIntentMismatchRecipientError, UnexpectedAddressError, @@ -37,7 +39,7 @@ import { UnsignedSweepTxMPCv2, VerifyEthAddressOptions, } from '../../src'; -import { coins, Erc7984Coin, EthereumNetwork } from '@bitgo/statics'; +import { coins, ContractAddressDefinedToken, Erc7984Coin, EthereumNetwork } from '@bitgo/statics'; import assert from 'assert'; import { getBuilder } from './getBuilder'; import * as testData from '../resources/eth'; @@ -49,6 +51,9 @@ import { ethTssBackupKey } from './fixtures/ethTssBackupKey'; nock.enableNetConnect(); +// DeFi legs carry defiParams, which TransactionParams does not model; the verifier reads it. +type DeFiTxParams = TransactionParams & { defiParams: DefiIntentParams }; + describe('ETH:', function () { let bitgo: TestBitGoAPI; let hopTxBitgoSignature; @@ -67,7 +72,8 @@ describe('ETH:', function () { const buildDefiTxHex = async ( data: string, to: string = address1, - coinName: 'eth' | 'hteth' = 'hteth' + coinName: 'eth' | 'hteth' = 'hteth', + value?: string ): Promise => { const txBuilder = getBuilder(coinName) as TransactionBuilder; txBuilder.type(TransactionType.ContractCall); @@ -75,6 +81,34 @@ describe('ETH:', function () { txBuilder.counter(1); txBuilder.contract(to); txBuilder.data(data); + if (value) { + txBuilder.value(value); + } + return (await txBuilder.build()).toBroadcastFormat(); + }; + + // verifyTransaction's EVM prebuild is the fully typed abstract-eth shape, but the TSS verifier + // only reads txHex; the other required fields are inert defaults so the fixture stays typed. + const defiPrebuild = (txHex: string, coin: string) => ({ + txHex, + buildParams: { recipients: [] }, + recipients: [], + nextContractSequenceId: 0, + gasPrice: 10, + gasLimit: 100000, + isBatch: false, + coin, + }); + + // Real native send bytes. verifyTssTransaction rejects bytes it cannot decode, so flows that + // used to pass a placeholder '0x' now build the transaction they actually verify. + const buildNativeTxHex = async (to: string, value: string, coinName: 'eth' | 'hteth' = 'hteth'): Promise => { + const txBuilder = getBuilder(coinName) as TransactionBuilder; + txBuilder.type(TransactionType.SingleSigSend); + txBuilder.fee({ fee: '10', gasLimit: '21000' }); + txBuilder.counter(1); + txBuilder.contract(to); + txBuilder.value(value); return (await txBuilder.build()).toBroadcastFormat(); }; @@ -724,7 +758,7 @@ describe('ETH:', function () { // txPrebuild with consolidateId (set by server during consolidation build) const txPrebuild = { consolidateId: '68a7d5d0c66e74e216b97173bd558c6d', - txHex: '0x', + txHex: await buildNativeTxHex(baseAddress, '1000000000000000000'), coin: 'hteth', walletId: 'fakeWalletId', }; @@ -757,7 +791,7 @@ describe('ETH:', function () { }; const txPrebuild = { - txHex: '0x', + txHex: await buildNativeTxHex(baseAddress, '1000000000000000000'), coin: 'hteth', walletId: 'fakeWalletId', }; @@ -792,7 +826,7 @@ describe('ETH:', function () { }; const txPrebuild = { - txHex: '0x', + txHex: await buildNativeTxHex(baseAddress, '1000000000000000000'), coin: 'hteth', walletId: 'fakeWalletId', }; @@ -858,7 +892,7 @@ describe('ETH:', function () { }; const txPrebuild = { - txHex: '0x', + txHex: await buildNativeTxHex(baseAddress, '1'), coin: 'hteth', walletId: 'fakeWalletId', }; @@ -890,8 +924,10 @@ describe('ETH:', function () { walletPassphrase: 'fakeWalletPassphrase', }; + // fillNonce is exactly a 1 wei self-send to the base address, so the bytes are built, not + // faked: undecodable bytes are rejected before any handler runs. const txPrebuild = { - txHex: '0x', + txHex: await buildNativeTxHex(baseAddress, '1'), coin: 'hteth', walletId: 'fakeWalletId', }; @@ -1626,7 +1662,7 @@ describe('ETH:', function () { }); }; - ['tokenApproval', 'customTx', 'defiApprove'].forEach((type) => { + ['tokenApproval', 'acceleration', 'defiApprove'].forEach((type) => { it(`should reject approve(wrapper, amount) on the underlying relabelled as ${type}`, async function () { await expectDefiRejection( verifyAs(type, defiCalldata('approve(address,uint256)', [wrapper, amount])), @@ -1635,62 +1671,44 @@ describe('ETH:', function () { }); }); - it('should reject a mutated-amount wrapper approve relabelled as customTx', async function () { + it('should reject customTx and contractCall outright as unsupported EVM TSS intent types', async function () { + for (const type of ['customTx', 'contractCall']) { + await verifyAs(type, defiCalldata('approve(address,uint256)', [wrapper, amount])).should.be.rejectedWith( + /unsupported intent type for EVM TSS/ + ); + } + }); + + it('should reject a mutated-amount wrapper approve relabelled as another type', async function () { await expectDefiRejection( - verifyAs('customTx', defiCalldata('approve(address,uint256)', [wrapper, '999999999999'])), + verifyAs('acceleration', defiCalldata('approve(address,uint256)', [wrapper, '999999999999'])), /must be declared as a wrapApprove intent/ ); }); it('should reject a wrapper approve with trailing bytes (an ERC-20 ignores them)', async function () { await expectDefiRejection( - verifyAs('customTx', defiCalldata('approve(address,uint256)', [wrapper, amount]) + '00'), + verifyAs('acceleration', defiCalldata('approve(address,uint256)', [wrapper, amount]) + '00'), /must be declared as a wrapApprove intent/ ); }); it('should not affect approvals of the underlying to other spenders', async function () { - (await verifyAs('customTx', defiCalldata('approve(address,uint256)', [address2, amount]))).should.equal(true); + (await verifyAs('acceleration', defiCalldata('approve(address,uint256)', [address2, amount]))).should.equal( + true + ); (await verifyAs('tokenApproval', defiCalldata('approve(address,uint256)', [address2, amount]))).should.equal( true ); }); - it('should not parse the tx at all when the calldata carries no approve selector', async function () { - const coin = bitgo.coin('hteth') as Hteth; - const wallet = new Wallet(bitgo, coin, { coinSpecific: { baseAddress: address1 } }); - const verifyWith = async (data: string) => { - const txHex = await buildDefiTxHex(data, underlying); - // getTransactionBuilder is not part of the coin's public type, so spy through a minimal structural view. - const builderSpy = sinon.spy( - coin as unknown as { getTransactionBuilder: () => unknown }, - 'getTransactionBuilder' - ); - try { - await coin.verifyTransaction({ - txParams: { type: 'customTx', wallet } as any, - txPrebuild: { txHex, coin: 'hteth', walletId: 'w' } as any, - wallet, - verification: {}, - walletType: 'tss', - }); - return builderSpy.callCount; - } finally { - builderSpy.restore(); - } - }; - - (await verifyWith(defiCalldata('transfer(address,uint256)', [wrapper, amount]))).should.equal(0); - (await verifyWith(defiCalldata('approve(address,uint256)', [address2, amount]))).should.equal(1); - }); - it('should leave other declared types untouched for txs that are not a wrapper approve', async function () { // Every type below skips the wrapApprove check and reaches the guard; none of these txs is an approve of a // wrapper, so all must verify exactly as they did before the guard existed. const benign: Array<[string, string, string]> = [ - ['customTx', defiCalldata('transfer(address,uint256)', [address2, amount]), underlying], - ['customTx', defiCalldata('approve(address,uint256)', [address2, amount]), underlying], - ['customTx', defiCalldata('approve(address,uint256)', [wrapper, amount]), address2], + ['bridgeFunds', defiCalldata('transfer(address,uint256)', [address2, amount]), underlying], + ['bridgeFunds', defiCalldata('approve(address,uint256)', [address2, amount]), underlying], + ['bridgeFunds', defiCalldata('approve(address,uint256)', [wrapper, amount]), address2], ['tokenApproval', defiCalldata('approve(address,uint256)', [address2, amount]), underlying], ['tokenApproval', defiCalldata('approve(address,uint256)', [wrapper, amount]), address2], ['acceleration', defiCalldata('transfer(address,uint256)', [address2, amount]), address2], @@ -1703,9 +1721,11 @@ describe('ETH:', function () { }); it('should not affect transfers to the underlying or approves of the wrapper on other contracts', async function () { - (await verifyAs('customTx', defiCalldata('transfer(address,uint256)', [wrapper, amount]))).should.equal(true); + (await verifyAs('acceleration', defiCalldata('transfer(address,uint256)', [wrapper, amount]))).should.equal( + true + ); ( - await verifyAs('customTx', defiCalldata('approve(address,uint256)', [wrapper, amount]), address2) + await verifyAs('acceleration', defiCalldata('approve(address,uint256)', [wrapper, amount]), address2) ).should.equal(true); }); @@ -1972,6 +1992,55 @@ describe('ETH:', function () { ); }); + it('should reject a DeFi leg that carries native value', async function () { + const coin = bitgo.coin('hteth') as Hteth; + const wallet = new Wallet(bitgo, coin, { coinSpecific: { baseAddress: address1 } }); + const verify = async (type: string, data: string) => { + const txParams: DeFiTxParams = { type, defiParams: { vaultId: 'vault', amount: '10' } }; + return coin.verifyTransaction({ + txParams, + txPrebuild: defiPrebuild(await buildDefiTxHex(data, address1, 'hteth', '1'), 'hteth'), + wallet, + verification: {}, + walletType: 'tss', + }); + }; + + // Native value on a vault calldata call pays whatever contract `to` names. + await expectDefiRejection( + verify('defiApprove', defiCalldata('approve(address,uint256)', [address2, '10'])), + /must not carry native value/ + ); + await expectDefiRejection( + verify('defiDeposit', defiCalldata('deposit(uint256,address)', ['10', address1])), + /must not carry native value/ + ); + await expectDefiRejection( + verify('defiWithdraw', defiCalldata('redeem(uint256,address,address)', ['10', address1, address1])), + /must not carry native value/ + ); + }); + + it('should anchor a defiApprove leg to the wallet token contract when statics knows it', async function () { + const tokenCoin = bitgo.coin('hteth:tusdc') as Erc20Token; + const tokenContract = String((coins.get('hteth:tusdc') as ContractAddressDefinedToken).contractAddress); + const wallet = new Wallet(bitgo, tokenCoin, { coinSpecific: { baseAddress: address1 } }); + // approve(vault, amount) on the token contract is the ERC-4626 approve leg. + const data = defiCalldata('approve(address,uint256)', [address2, '10']); + const txParams: DeFiTxParams = { type: 'defiApprove', defiParams: { vaultId: 'vault', amount: '10' } }; + const verify = async (to: string) => + tokenCoin.verifyTransaction({ + txParams, + txPrebuild: defiPrebuild(await buildDefiTxHex(data, to), 'hteth:tusdc'), + wallet, + verification: {}, + walletType: 'tss', + }); + + (await verify(tokenContract)).should.equal(true); + await expectDefiRejection(verify(address2), /must target the wallet token contract/); + }); + it('should verify TSS transaction with defiWithdraw type', async function () { const coin = bitgo.coin('hteth') as Hteth; const wallet = new Wallet(bitgo, coin, { coinSpecific: { baseAddress: address1 } }); diff --git a/modules/sdk-coin-evm/src/evmCoin.ts b/modules/sdk-coin-evm/src/evmCoin.ts index 4da310e95cb..1013b8b4732 100644 --- a/modules/sdk-coin-evm/src/evmCoin.ts +++ b/modules/sdk-coin-evm/src/evmCoin.ts @@ -9,7 +9,6 @@ import { MPCAlgorithm, MultisigType, multisigTypes, - NO_RECIPIENT_TX_TYPES, } from '@bitgo/sdk-core'; import { BaseCoin as StaticsBaseCoin, CoinFeature, coins, CoinFamily } from '@bitgo/statics'; import { @@ -17,7 +16,6 @@ import { ERC721TransferBuilder, recoveryBlockchainExplorerQuery, TransactionBuilder as EthLikeTransactionBuilder, - VerifyEthTransactionOptions, } from '@bitgo/abstract-eth'; import { TransactionBuilder } from './lib'; import { isHtsEvmAddress, recovery_HBAREVM_BlockchainExplorerQuery, validateHederaAccountId } from './lib/utils'; @@ -87,47 +85,6 @@ export class EvmCoin extends AbstractEthLikeNewCoins { } } - /** @inheritDoc */ - async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise { - const supportsEIP1559 = this.staticsCoin?.features?.includes(CoinFeature.EIP1559); - if (supportsEIP1559) { - return await super.verifyTssTransaction(params); - } else { - return await this.verifyLegacyTssTransaction(params); - } - } - - /** - * Verifies legacy (non-EIP-1559) TSS transactions with basic validation. - */ - private async verifyLegacyTssTransaction(params: VerifyEthTransactionOptions): Promise { - const { txParams, txPrebuild, wallet } = params; - - // Basic validation for legacy transactions only - if ( - !txParams?.recipients && - !( - txParams.prebuildTx?.consolidateId || - txParams.stakingRequestId || - txParams.prebuildTx?.stakingRequestId || - (txParams.type && NO_RECIPIENT_TX_TYPES.has(txParams.type)) - ) - ) { - throw new Error(`missing txParams`); - } - - if (!wallet || !txPrebuild) { - throw new Error(`missing params`); - } - - if (txParams.hop && txParams.recipients && txParams.recipients.length > 1) { - throw new Error(`tx cannot be both a batch and hop transaction`); - } - - // If validation passes, consider it verified - return true; - } - /** @inheritDoc */ isValidAddress(address: string, isAlternateAddress?: boolean): boolean { if (isAlternateAddress && this.getFamily() === CoinFamily.HBAREVM) { diff --git a/modules/sdk-coin-evm/test/unit/tssSigningPath.ts b/modules/sdk-coin-evm/test/unit/tssSigningPath.ts new file mode 100644 index 00000000000..c603b4972d0 --- /dev/null +++ b/modules/sdk-coin-evm/test/unit/tssSigningPath.ts @@ -0,0 +1,187 @@ +import 'should'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { TestBitGo } from '@bitgo/sdk-test'; +import { coins, EthereumNetwork } from '@bitgo/statics'; +import { + EcdsaUtils, + RequestTracer, + TransactionType, + TxIntentMismatchRecipientError, + TxRequest, + Wallet, +} from '@bitgo/sdk-core'; +import { getCommon, optionalDeps } from '@bitgo/abstract-eth'; + +import { EvmCoin } from '../../src/evmCoin'; +import { TransactionBuilder } from '../../src/lib'; + +/** + * End to end on the ECDSA signing path (CECHO-2460). tssVerification.ts calls verifyTransaction + * directly; this suite runs the real EVM verifier inside EcdsaUtils.signTxRequest and proves that + * bytes it rejects never reach the signing round. + * + * Legacy (non-EIP-1559) chains carry two different server-supplied hexes: signableHex is the + * EIP-155 signing preimage `RLP([nonce, gasPrice, gasLimit, to, value, data, chainId, 0, 0])` and + * serializedTxHex is the broadcast-format unsigned transaction. Only the latter is parseable by + * ethereumjs, which is why these chains need + * `CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX`, and why both hexes are built here as the + * distinct values production sends instead of the same fabricated string. + */ +const legacyChains = ['kavaevm', 'tkavaevm', 'prividiumeth', 'tprividiumeth']; + +interface SigningMaterial { + serializedTxHex: string; + signableHex: string; +} + +const walletId = '62fe536a6b4cf70007acb48c0e7bb0b0'; +const baseAddress = '0x1111111111111111111111111111111111111111'; +const recipientAddress = '0x174cfd823af8ce27ed0afee3fcf3c3ba259116be'; +const attackerAddress = '0x7e85bdc27c050e3905ebf4b8e634d9ad6edd0de6'; +const tokenContractAddress = '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48'; +const approveAttackerMax = `0x095ea7b3${attackerAddress.slice(2).padStart(64, '0')}${'f'.repeat(64)}`; +// Minimal signing material: signRequestBase only needs pShare.i === 1, a backupNShare and the +// paillier modulus before it hands the unsigned bytes to verifyTransaction. +const prv = JSON.stringify({ pShare: { i: 1, n: 'paillier-modulus' }, backupNShare: {} }); + +/** Build the two hexes a legacy EVM tx request carries, exactly as the wallet platform shapes them. */ +const buildSigningMaterial = async ( + chain: string, + opts: { to: string; value: string; data?: string } +): Promise => { + const staticsCoin = coins.get(chain); + const builder = new TransactionBuilder(staticsCoin); + builder.type(opts.data ? TransactionType.ContractCall : TransactionType.SingleSigSend); + builder.fee({ fee: '10', gasLimit: '21000' }); + builder.counter(1); + builder.contract(opts.to); + if (opts.data) { + builder.data(opts.data); + } + builder.value(opts.value); + const broadcast = (await builder.build()).toBroadcastFormat(); + // The wallet platform stores serializedTxHex without the 0x prefix. + const serializedTxHex = broadcast.startsWith('0x') ? broadcast.slice(2) : broadcast; + + const common = getCommon(staticsCoin.network as EthereumNetwork); + const legacyTx = optionalDeps.EthTx.Transaction.fromSerializedTx(Buffer.from(serializedTxHex, 'hex'), { common }); + // getMessageToSign(false) appends chainId, 0, 0: the EIP-155 preimage the platform sends as + // signableHex. ethereumjs rejects it in fromSerializedTx (v = chainId), so it must never be the + // bytes the verifier decodes. + const signableHex = Buffer.from(optionalDeps.ethUtil.rlp.encode(legacyTx.getMessageToSign(false))).toString('hex'); + + return { serializedTxHex, signableHex }; +}; + +/** Signs through EcdsaUtils and reports whether the signing round (after verification) started. */ +const sign = async (opts: { + chain: string; + material: SigningMaterial; + txParams?: Record; + strict?: boolean; +}): Promise<{ signingStarted: boolean; error?: Error }> => { + const staticsCoin = coins.get(opts.chain); + const bitgo = TestBitGo.decorate(BitGoAPI, { + env: 'test', + strictTssVerification: opts.strict ?? false, + onVerificationWarning: () => undefined, + }); + const coin = new (EvmCoin as any)(bitgo, staticsCoin); + const wallet = new Wallet(bitgo, coin, { id: walletId, coinSpecific: { baseAddress }, multisigType: 'tss' } as any); + const utils = new EcdsaUtils(bitgo, coin, wallet); + let signingStarted = false; + (utils as any).getEcdsaSigningChallenges = async () => { + signingStarted = true; + throw new Error('stop: signing round started'); + }; + const txRequest = { + txRequestId: 'signing-path', + apiVersion: 'full', + walletId, + intent: { + intentType: 'payment', + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: opts.chain } }], + }, + transactions: [ + { + unsignedTx: { + derivationPath: 'm/0', + signableHex: opts.material.signableHex, + serializedTxHex: opts.material.serializedTxHex, + }, + signatureShares: [], + }, + ], + } as unknown as TxRequest; + try { + await utils.signTxRequest({ txRequest, txParams: opts.txParams as any, prv, reqId: new RequestTracer() }); + } catch (e) { + return { signingStarted, error: e as Error }; + } + return { signingStarted }; +}; + +const callerPayment = { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }; + +describe('EVM TSS signing path stops tampered bytes before signing (legacy chains)', function () { + for (const chain of legacyChains) { + describe(chain, function () { + it('signs a payment whose serialized bytes match the intent', async function () { + const material = await buildSigningMaterial(chain, { to: recipientAddress, value: '1000' }); + material.signableHex.should.not.equal( + material.serializedTxHex, + 'the preimage and the broadcast bytes must be distinct, as the platform sends them' + ); + for (const txParams of [undefined, callerPayment]) { + const { signingStarted, error } = await sign({ chain, material, txParams, strict: true }); + signingStarted.should.equal(true); + (error as Error).message.should.match(/signing round started/); + } + }); + + it('never starts signing a redirected payment on the pending-approval path, in either mode', async function () { + const material = await buildSigningMaterial(chain, { to: attackerAddress, value: '1000' }); + for (const strict of [false, true]) { + const { signingStarted, error } = await sign({ chain, material, strict }); + signingStarted.should.equal(false); + (error as Error).should.be.instanceOf(TxIntentMismatchRecipientError); + (error as Error).message.should.match(/destination address does not match/); + } + }); + }); + } + + it('never starts signing a preimage that does not match the verified serialized bytes', async function () { + const chain = 'kavaevm'; + const benign = await buildSigningMaterial(chain, { to: recipientAddress, value: '1000' }); + const attacker = await buildSigningMaterial(chain, { to: attackerAddress, value: '1000' }); + // Verify the benign serialized bytes, then try to sign the attacker's preimage. + const material = { serializedTxHex: benign.serializedTxHex, signableHex: attacker.signableHex }; + const { signingStarted, error } = await sign({ chain, material, txParams: callerPayment, strict: true }); + signingStarted.should.equal(false); + (error as Error).message.should.match(/signableHex is inconsistent with serializedTxHex/); + }); + + it('never starts signing approve(attacker, MAX) labelled as a payment under strictTssVerification', async function () { + const material = await buildSigningMaterial('kavaevm', { + to: tokenContractAddress, + value: '0', + data: approveAttackerMax, + }); + const { signingStarted, error } = await sign({ chain: 'kavaevm', material, txParams: callerPayment, strict: true }); + signingStarted.should.equal(false); + (error as Error).should.be.instanceOf(TxIntentMismatchRecipientError); + (error as Error).message.should.match(/not a recognized payment shape/); + }); + + it('lets the same bytes reach signing in default mode, as the warn-first rollout agreed', async function () { + // Pins today's policy so a change to it is deliberate: this flips once strict becomes the default. + const material = await buildSigningMaterial('kavaevm', { + to: tokenContractAddress, + value: '0', + data: approveAttackerMax, + }); + const { signingStarted } = await sign({ chain: 'kavaevm', material, txParams: callerPayment }); + signingStarted.should.equal(true); + }); +}); diff --git a/modules/sdk-coin-evm/test/unit/tssVerification.ts b/modules/sdk-coin-evm/test/unit/tssVerification.ts new file mode 100644 index 00000000000..68f63f6c636 --- /dev/null +++ b/modules/sdk-coin-evm/test/unit/tssVerification.ts @@ -0,0 +1,1739 @@ +import 'should'; +import { setTimeout as delay } from 'timers/promises'; +import { BitGoAPI } from '@bitgo/sdk-api'; +import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; +import { BaseCoin as StaticsBaseCoin, coins, ContractAddressDefinedToken, Erc7984Coin } from '@bitgo/statics'; +import { + ITransactionRecipient, + PopulatedIntent, + resolveEffectiveTxParams, + TransactionParams, + TransactionType, + TssVerificationWarning, + TxRequest, + Wallet, +} from '@bitgo/sdk-core'; +import { optionalDeps } from '@bitgo/abstract-eth'; + +import { EvmCoin } from '../../src/evmCoin'; +import { TransactionBuilder } from '../../src/lib'; + +/** + * Fail-closed EVM TSS verification (TOB-BITGOEDMPC-2 / INFOSEC-671 / CECHO-2460). + * + * The user's key share must not sign EVM TSS bytes unless the decoded to / value / data match + * the intent. Checks that threw before this change keep throwing (including on the + * pending-approval path); every new check flags the violation (warn by default, throw under + * `strictTssVerification`). + * + * Runs on kavaevm (legacy, non-EIP-1559 — the chains that used to bypass the verifier through + * `EvmCoin.verifyLegacyTssTransaction`) and hbarevm (EIP-1559). Batcher-contract cases + * (native/token batch sends, tokenApproval spender) need a chain with a statics batcher + * address and live in @bitgo/sdk-coin-bsc's tssBatcherVerification tests. + */ + +const recipientAddress = '0x174cfd823af8ce27ed0afee3fcf3c3ba259116be'; +const attackerAddress = '0x7e85bdc27c050e3905ebf4b8e634d9ad6edd0de6'; +const baseAddress = '0x1111111111111111111111111111111111111111'; +const tokenContractAddress = '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48'; +const MAX_UINT256 = ((1n << 256n) - 1n).toString(); + +const erc7984Pair = coins.get('hteth:cusdt') as Erc7984Coin; +const erc7984WrapperAddress = String(erc7984Pair.contractAddress); + +// Two distinct registered tokens whose contracts resolve locally from statics. The caller +// and the stored intent name different tokens to prove the server cannot replace local identity. +const tokenContractOf = (tokenName: string): string => { + const token = coins.get(tokenName); + if (!(token instanceof ContractAddressDefinedToken)) { + throw new Error(`${tokenName} has no contract address in statics`); + } + return String(token.contractAddress); +}; +const localTokenName = 'hteth:tusdc'; +const serverTokenName = 'hteth:tusdt'; +const localTokenContractAddress = tokenContractOf(localTokenName); +const serverTokenContractAddress = tokenContractOf(serverTokenName); + +const word = (value: bigint): string => value.toString(16).padStart(64, '0'); +const addrWord = (address: string): string => address.toLowerCase().replace(/^0x/, '').padStart(64, '0'); + +function encodeErc20Transfer(to: string, amount: string): string { + return `0xa9059cbb${addrWord(to)}${word(BigInt(amount))}`; +} + +function encodeApprove(spender: string, amount: string): string { + return `0x095ea7b3${addrWord(spender)}${word(BigInt(amount))}`; +} + +function encodeWithAbi(signature: string, types: string[], values: unknown[]): string { + const selector = optionalDeps.ethAbi.methodID(signature, types); + const args = optionalDeps.ethAbi.rawEncode(types, values); + return '0x' + Buffer.concat([selector, args]).toString('hex'); +} + +function encodeConfidentialTransferNoProof(to: string): string { + return encodeWithAbi('confidentialTransfer', ['address', 'bytes32'], [to, '0x' + '11'.repeat(32)]); +} + +// ERC-721 / ERC-1155 collection used by the transferToken NFT tests, plus the standard transfer +// calldata the verifier accepts: safeTransferFrom with and without the data parameter and +// transferFrom for ERC-721, safeTransferFrom and safeBatchTransferFrom for ERC-1155. +const nftContractAddress = '0x0000000000000000000000000000000000000abc'; +// A registered ERC-721 whose name resolves to a real, different contract than nftContractAddress. +const otherNftName = 'erc721:witch'; + +function encodeErc721SafeTransferFrom(to: string, tokenId: string): string { + return encodeWithAbi('safeTransferFrom', ['address', 'address', 'uint256'], [baseAddress, to, tokenId]); +} + +function encodeErc721SafeTransferFromWithData(to: string, tokenId: string): string { + return encodeWithAbi( + 'safeTransferFrom', + ['address', 'address', 'uint256', 'bytes'], + [baseAddress, to, tokenId, Buffer.alloc(0)] + ); +} + +function encodeErc721TransferFrom(to: string, tokenId: string): string { + return encodeWithAbi('transferFrom', ['address', 'address', 'uint256'], [baseAddress, to, tokenId]); +} + +function encodeErc1155SafeTransferFrom(to: string, tokenId: string, quantity: string): string { + return encodeWithAbi( + 'safeTransferFrom', + ['address', 'address', 'uint256', 'uint256', 'bytes'], + [baseAddress, to, tokenId, quantity, Buffer.alloc(0)] + ); +} + +function encodeErc1155SafeBatchTransferFrom(to: string, tokenIds: string[], quantities: string[]): string { + return encodeWithAbi( + 'safeBatchTransferFrom', + ['address', 'address', 'uint256[]', 'uint256[]', 'bytes'], + [baseAddress, to, tokenIds, quantities, Buffer.alloc(0)] + ); +} + +async function buildTxHex( + coin: Readonly, + opts: { to: string; value?: string; data?: string } +): Promise { + const builder = new TransactionBuilder(coin); + builder.type(opts.data && opts.data !== '0x' ? TransactionType.ContractCall : TransactionType.SingleSigSend); + builder.fee({ fee: '10', gasLimit: '21000' }); + builder.counter(1); + builder.contract(opts.to); + if (opts.data) { + builder.data(opts.data); + } + if (opts.value) { + builder.value(opts.value); + } + return (await builder.build()).toBroadcastFormat(); +} + +// kavaevm is legacy (non-EIP-1559) and hbarevm is EIP-1559; neither has a statics batcher +// contract, so the "statics gap" paths are what both exercise for batcher-dependent rules. +['kavaevm', 'hbarevm'].forEach((chain) => { + describe(`EVM TSS fail-closed verification (${chain})`, function () { + const staticsCoin = coins.get(chain); + + let bitgo: TestBitGoAPI; + let strictBitgo: TestBitGoAPI; + let warnings: TssVerificationWarning[]; + let coin: EvmCoin; + let wallet: Wallet; + + beforeEach(function () { + warnings = []; + bitgo = TestBitGo.decorate(BitGoAPI, { + env: 'test', + onVerificationWarning: (warning) => warnings.push(warning), + }); + strictBitgo = TestBitGo.decorate(BitGoAPI, { + env: 'test', + strictTssVerification: true, + }); + coin = new (EvmCoin as any)(bitgo, staticsCoin); + wallet = new Wallet(bitgo, coin as any, { coinSpecific: { baseAddress } } as any); + }); + + // Caller params may also carry the EVM-only tokenName / receiveAddress that EthTransactionParams + // adds, and EVM recipients carry the intent tokenData identity next to the flat fields. + type SigningPathRecipient = ITransactionRecipient & { + tokenData?: { tokenType?: string; tokenQuantity?: string; tokenName?: string; tokenContractAddress?: string }; + }; + type SigningPathCallerParams = Omit & { + tokenName?: string; + receiveAddress?: string; + recipients?: SigningPathRecipient[]; + }; + + const verify = ( + txParams: Record, + txHex: string, + opts: { + strict?: boolean; + intent?: PopulatedIntent; + callerTxParams?: SigningPathCallerParams; + verification?: Record; + } = {} + ): Promise => { + const target = opts.strict ? new (EvmCoin as any)(strictBitgo, staticsCoin) : coin; + const w = opts.strict ? new Wallet(strictBitgo, target as any, { coinSpecific: { baseAddress } } as any) : wallet; + return (target as any).verifyTransaction({ + txParams, + txPrebuild: { txHex, coin: chain, walletId: 'w' }, + wallet: w, + verification: opts.verification ?? {}, + walletType: 'tss', + ...(opts.intent ? { intent: opts.intent } : {}), + ...(opts.callerTxParams ? { callerTxParams: opts.callerTxParams } : {}), + }); + }; + + // Mirrors the ECDSA / MPCv2 signing paths: resolveEffectiveTxParams back-fills the stored + // intent into the effective txParams, while callerTxParams stays the untouched caller + // input. This is the wiring that decides which source speaks for the user intent. + const verifySigningPath = ( + callerTxParams: SigningPathCallerParams | undefined, + intent: Record, + txHex: string, + strict = false + ): Promise => { + const txRequest = { txRequestId: 'signing-path', walletId: 'w', intent } as unknown as TxRequest; + const effective = resolveEffectiveTxParams(txRequest, callerTxParams, chain); + return verify(effective as unknown as Record, txHex, { + strict, + intent: intent as unknown as PopulatedIntent, + ...(callerTxParams ? { callerTxParams } : {}), + }); + }; + + const build = (opts: { to: string; value?: string; data?: string }): Promise => + buildTxHex(staticsCoin, opts); + + describe('payment handler (transfer)', function () { + it('verifies a native send that matches the intent, with no warnings', async function () { + const txHex = await build({ to: recipientAddress, value: '1000' }); + const result = await verify( + { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, + txHex + ); + result.should.equal(true); + warnings.should.have.length(0); + }); + + it('throws on a redirected destination (existing check, both modes)', async function () { + const txHex = await build({ to: attackerAddress, value: '1000' }); + for (const strict of [false, true]) { + await verify({ type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, txHex, { + strict, + }).should.be.rejectedWith(/destination address does not match/); + } + }); + + it('throws on an inflated value (existing check, both modes)', async function () { + const txHex = await build({ to: recipientAddress, value: '9999' }); + for (const strict of [false, true]) { + await verify({ type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, txHex, { + strict, + }).should.be.rejectedWith(/amount in txPrebuild does not match/); + } + }); + + it('verifies an exact dApp call (to, value and data equal to the intent)', async function () { + const data = '0xdeadbeef'; + const txHex = await build({ to: tokenContractAddress, value: '0', data }); + const result = await verify( + { type: 'transfer', recipients: [{ address: tokenContractAddress, amount: '0', data }] }, + txHex + ); + result.should.equal(true); + warnings.should.have.length(0); + }); + + it('flags swapped calldata under a payment intent (warn, throw in strict mode)', async function () { + // ERC-20 calldata with a swapped inner recipient: the existing inner check throws. + const intentData = encodeErc20Transfer(recipientAddress, '1000'); + const tamperedData = encodeErc20Transfer(attackerAddress, '1000'); + const tamperedHex = await build({ to: tokenContractAddress, value: '0', data: tamperedData }); + await verify( + { + type: 'transfer', + recipients: [{ address: tokenContractAddress, amount: '0', data: intentData }], + }, + tamperedHex + ).should.be.rejectedWith(/destination address does not match/); + + // Non-ERC-20 dApp calldata mismatch is a new check: warn, or throw in strict mode. + const swappedHex = await build({ to: tokenContractAddress, value: '0', data: '0xdeadbeef' }); + const txParams = { + type: 'transfer', + recipients: [{ address: tokenContractAddress, amount: '0', data: '0xdeadbe00' }], + }; + const result = await verify(txParams, swappedHex); + result.should.equal(true); + warnings.should.have.length(1); + warnings[0].reason.should.match(/calldata does not match/); + warnings[0].decoded.data.should.equal('0xdeadbeef'); + warnings[0].expected.data.should.equal('0xdeadbe00'); + await verify(txParams, swappedHex, { strict: true }).should.be.rejectedWith(/calldata does not match/); + }); + + it('treats absent, empty and 0x calldata as the same empty payload', async function () { + // A caller that declares `''` (or omits data) is declaring a native send; the decoded + // bytes are `0x`. Format alone must not turn a matching payment into a mismatch. + const txHex = await build({ to: recipientAddress, value: '1000' }); + for (const data of [undefined, '', '0x']) { + warnings = []; + const recipient = { address: recipientAddress, amount: '1000', ...(data !== undefined ? { data } : {}) }; + (await verify({ type: 'transfer', recipients: [recipient] }, txHex)).should.equal(true); + warnings.should.have.length(0); + } + }); + + it('normalizes the calldata prefix and case before comparing', async function () { + const data = encodeErc20Transfer(recipientAddress, '1000'); + const txHex = await build({ to: tokenContractAddress, value: '0', data }); + for (const declared of [data, data.slice(2), `0x${data.slice(2).toUpperCase()}`, data.toUpperCase()]) { + warnings = []; + ( + await verify( + { type: 'transfer', recipients: [{ address: tokenContractAddress, amount: '0', data: declared }] }, + txHex + ) + ).should.equal(true); + warnings.should.have.length(0); + } + }); + + it('still runs the ERC-20 inner check on prefix-less declared calldata', async function () { + // Same bytes without `0x`: the selector gate must recognize them, so a swapped inner + // recipient stays the fatal check it is for prefixed calldata instead of becoming a + // calldata-format warning. + const declared = encodeErc20Transfer(attackerAddress, '1000').slice(2); + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '1000'), + }); + for (const strict of [false, true]) { + await verify( + { type: 'transfer', recipients: [{ address: tokenContractAddress, amount: '0', data: declared }] }, + txHex, + { strict } + ).should.be.rejectedWith(/destination address does not match/); + } + }); + + it('flags a mismatched destination under an exact-call (dApp) intent', async function () { + const data = '0xdeadbeef'; + const txHex = await build({ to: attackerAddress, value: '0', data }); + const txParams = { + type: 'transfer', + recipients: [{ address: tokenContractAddress, amount: '0', data }], + }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /to does not match/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/to does not match/); + }); + + it('flags a mismatched value under an exact-call (dApp) intent', async function () { + const data = '0xdeadbeef'; + const txHex = await build({ to: tokenContractAddress, value: '7', data }); + const txParams = { + type: 'transfer', + recipients: [{ address: tokenContractAddress, amount: '0', data }], + }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /value does not match/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/value does not match/); + }); + + it('flags approve(attacker, MAX) disguised as a transfer (TOB-BITGOEDMPC-2)', async function () { + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeApprove(attackerAddress, MAX_UINT256), + }); + const txParams = { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /not a recognized payment shape/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/not a recognized payment shape/); + }); + + it('rejects missing recipients under a caller-declared payment intent (fail closed)', async function () { + // An empty recipients array declares nothing to verify the bytes against: signing stops + // whether or not the client opted into strictTssVerification. + const txHex = await build({ to: recipientAddress, value: '1000' }); + const txParams = { type: 'transfer', recipients: [] }; + for (const strict of [false, true]) { + await verify(txParams, txHex, { strict }).should.be.rejectedWith( + /caller declared a payment without recipients/ + ); + } + warnings.should.have.length(0); + }); + }); + + describe('payment handler (token transfers)', function () { + const tokenRecipient = { + address: recipientAddress, + amount: '10000000', + tokenAddress: tokenContractAddress, + }; + + it('verifies an ERC-20 transfer against the intent token contract, value zero', async function () { + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '10000000'), + }); + const result = await verify({ type: 'transfer', recipients: [tokenRecipient] }, txHex); + result.should.equal(true); + warnings.should.have.length(0); + }); + + it('flags an ERC-20 transfer whose to is not the intent token contract', async function () { + const txHex = await build({ + to: attackerAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '10000000'), + }); + const txParams = { type: 'transfer', recipients: [tokenRecipient] }; + (await verify(txParams, txHex)).should.equal(true); + const flag = warnings.find((w) => /not the intent token contract/.test(w.reason)); + should.exist(flag); + flag?.expected.to?.toLowerCase().should.equal(tokenContractAddress.toLowerCase()); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/not the intent token contract/); + }); + + it('flags an ERC-20 transfer that carries native value', async function () { + const txHex = await build({ + to: tokenContractAddress, + value: '5', + data: encodeErc20Transfer(recipientAddress, '10000000'), + }); + const txParams = { type: 'transfer', recipients: [tokenRecipient] }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /must not carry native value/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/must not carry native value/); + }); + + it('throws on a tampered ERC-20 recipient or amount (existing checks)', async function () { + const toAttacker = await build({ + to: tokenContractAddress, + value: '0', + data: encodeErc20Transfer(attackerAddress, '10000000'), + }); + await verify({ type: 'transfer', recipients: [tokenRecipient] }, toAttacker).should.be.rejectedWith( + /destination address does not match/ + ); + const inflated = await build({ + to: tokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '99999999'), + }); + await verify({ type: 'transfer', recipients: [tokenRecipient] }, inflated).should.be.rejectedWith( + /amount in txPrebuild does not match/ + ); + }); + + it('checks the ERC-7984 confidentialTransfer recipient only', async function () { + const intentParams = { + type: 'transfer', + recipients: [{ address: recipientAddress, amount: '0', tokenName: 'hteth:cusdt' }], + }; + const goodHex = await build({ + to: erc7984WrapperAddress, + value: '0', + data: encodeConfidentialTransferNoProof(recipientAddress), + }); + (await verify(intentParams, goodHex)).should.equal(true); + warnings.should.have.length(0); + + const tamperedHex = await build({ + to: erc7984WrapperAddress, + value: '0', + data: encodeConfidentialTransferNoProof(attackerAddress), + }); + (await verify(intentParams, tamperedHex)).should.equal(true); + warnings.some((w) => /confidentialTransfer recipient does not match/.test(w.reason)).should.equal(true); + await verify(intentParams, tamperedHex, { strict: true }).should.be.rejectedWith( + /confidentialTransfer recipient does not match/ + ); + }); + + it('resolves the caller recipient tokenData tokenContractAddress', async function () { + // Callers that use the intent recipient shape declare the token in `tokenData` rather + // than `tokenAddress`. It is the caller's own declaration, so it selects the token and + // the ERC-20 transfer is verified against it. + const caller = { + type: 'transfer', + recipients: [ + { + address: recipientAddress, + amount: '10000000', + tokenData: { + tokenType: 'ERC20', + tokenQuantity: '10000000', + tokenContractAddress: localTokenContractAddress, + }, + }, + ], + }; + const txHex = await build({ + to: localTokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '10000000'), + }); + (await verify(caller, txHex)).should.equal(true); + warnings.should.have.length(0); + + // A transaction paying another token is a token-contract mismatch, not an unverified + // native send: the declared identity now anchors the check. + const otherTokenHex = await build({ + to: serverTokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '10000000'), + }); + warnings = []; + (await verify(caller, otherTokenHex)).should.equal(true); + warnings.some((w) => /not the intent token contract/.test(w.reason)).should.equal(true); + await verify(caller, otherTokenHex, { strict: true }).should.be.rejectedWith(/not the intent token contract/); + }); + + it('resolves the caller recipient tokenData tokenName through statics', async function () { + const caller = { + type: 'transferToken', + recipients: [{ address: recipientAddress, amount: '1000', tokenData: { tokenName: localTokenName } }], + }; + const txHex = await build({ + to: localTokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '1000'), + }); + (await verify(caller, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('flags batch calldata carried by a single declared recipient', async function () { + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeWithAbi('batch', ['address[]', 'uint256[]'], [[recipientAddress], ['1']]), + }); + const txParams = { type: 'transfer', recipients: [tokenRecipient] }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /single declared recipient/.test(w.reason)).should.equal(true); + }); + }); + + // ERC-721 / ERC-1155 transferToken: the recipient names the destination and the tokenData + // standard / id / quantity / collection, while the bytes go to the collection with the + // standard's transfer calldata. + describe('transferToken handler (ERC-721 / ERC-1155)', function () { + const erc721Recipient = (tokenId: string, tokenQuantity = '1'): Record => ({ + address: recipientAddress, + amount: '0', + tokenData: { + tokenType: 'ERC721', + tokenQuantity, + tokenId, + tokenContractAddress: nftContractAddress, + }, + }); + const erc1155Recipient = (tokenId: string, tokenQuantity: string): Record => ({ + address: recipientAddress, + amount: '0', + tokenData: { + tokenType: 'ERC1155', + tokenQuantity, + tokenId, + tokenContractAddress: nftContractAddress, + }, + }); + const nftTx = (data: string, overrides: { to?: string; value?: string } = {}): Promise => + build({ to: overrides.to ?? nftContractAddress, value: overrides.value ?? '0', data }); + + it('verifies the ERC-721 safeTransferFrom and transferFrom signatures', async function () { + const txParams = { type: 'transferToken', recipients: [erc721Recipient('7')] }; + for (const data of [ + encodeErc721SafeTransferFrom(recipientAddress, '7'), + encodeErc721SafeTransferFromWithData(recipientAddress, '7'), + encodeErc721TransferFrom(recipientAddress, '7'), + ]) { + warnings = []; + const txHex = await nftTx(data); + (await verify(txParams, txHex)).should.equal(true); + warnings.should.have.length(0); + (await verify(txParams, txHex, { strict: true })).should.equal(true); + } + }); + + it('verifies a tokenId whose 32-byte encoding contains the selector bytes', async function () { + // The arguments are decoded from the fixed position after the matched selector, so an id + // that happens to encode the selector's own bytes (0x42842e0e / 0xb88d4fde) is legitimate. + const selectorId = BigInt('0x42842e0e').toString(); + const dataSelectorId = BigInt('0xb88d4fde').toString(); + const cases: Array<{ tokenId: string; data: string }> = [ + { tokenId: selectorId, data: encodeErc721SafeTransferFrom(recipientAddress, selectorId) }, + { tokenId: dataSelectorId, data: encodeErc721SafeTransferFromWithData(recipientAddress, dataSelectorId) }, + ]; + for (const { tokenId, data } of cases) { + warnings = []; + const txParams = { type: 'transferToken', recipients: [erc721Recipient(tokenId)] }; + const txHex = await nftTx(data); + (await verify(txParams, txHex)).should.equal(true); + warnings.should.have.length(0); + (await verify(txParams, txHex, { strict: true })).should.equal(true); + } + }); + + it('verifies ERC-1155 single and one-entry batch transfers', async function () { + const txParams = { type: 'transferToken', recipients: [erc1155Recipient('7', '3')] }; + for (const data of [ + encodeErc1155SafeTransferFrom(recipientAddress, '7', '3'), + encodeErc1155SafeBatchTransferFrom(recipientAddress, ['7'], ['3']), + ]) { + warnings = []; + const txHex = await nftTx(data); + (await verify(txParams, txHex)).should.equal(true); + warnings.should.have.length(0); + (await verify(txParams, txHex, { strict: true })).should.equal(true); + } + }); + + it('flags an NFT sent to a swapped destination (warn, throw in strict mode)', async function () { + const txParams = { type: 'transferToken', recipients: [erc721Recipient('7')] }; + const txHex = await nftTx(encodeErc721SafeTransferFrom(attackerAddress, '7')); + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /destination address does not match/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith(/destination address does not match/); + }); + + it('flags a swapped ERC-721 tokenId and an inflated ERC-1155 quantity', async function () { + const erc721 = { type: 'transferToken', recipients: [erc721Recipient('7')] }; + const wrongIdHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '8')); + (await verify(erc721, wrongIdHex)).should.equal(true); + warnings.some((w) => /tokenId in calldata does not match/.test(w.reason)).should.equal(true); + await verify(erc721, wrongIdHex, { strict: true }).should.be.rejectedWith(/tokenId in calldata does not match/); + + warnings = []; + const erc1155 = { type: 'transferToken', recipients: [erc1155Recipient('7', '3')] }; + const wrongQuantityHex = await nftTx(encodeErc1155SafeTransferFrom(recipientAddress, '7', '4')); + (await verify(erc1155, wrongQuantityHex)).should.equal(true); + warnings.some((w) => /tokenQuantity in calldata does not match/.test(w.reason)).should.equal(true); + await verify(erc1155, wrongQuantityHex, { strict: true }).should.be.rejectedWith( + /tokenQuantity in calldata does not match/ + ); + }); + + it('flags an ERC-1155 batch that moves more than the single declared entry', async function () { + const erc1155 = { type: 'transferToken', recipients: [erc1155Recipient('7', '3')] }; + const twoEntries = ['7', '8']; + const twoEntryQuantities = ['3', '1']; + const twoEntriesHex = await nftTx( + encodeErc1155SafeBatchTransferFrom(recipientAddress, twoEntries, twoEntryQuantities) + ); + (await verify(erc1155, twoEntriesHex)).should.equal(true); + warnings + .some((w) => /must move exactly the declared tokenId and tokenQuantity/.test(w.reason)) + .should.equal(true); + await verify(erc1155, twoEntriesHex, { strict: true }).should.be.rejectedWith( + /must move exactly the declared tokenId and tokenQuantity/ + ); + }); + + it('flags another standard selector, native value, a foreign contract and a missing tokenId', async function () { + const erc721 = { type: 'transferToken', recipients: [erc721Recipient('7')] }; + // The declared standard decides: ERC-1155 calldata is not an ERC-721 transfer. + const otherStandardHex = await nftTx(encodeErc1155SafeTransferFrom(recipientAddress, '7', '1')); + (await verify(erc721, otherStandardHex)).should.equal(true); + warnings.some((w) => /ERC721 token transfer must use/.test(w.reason)).should.equal(true); + + warnings = []; + const nativeValueHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '7'), { value: '5' }); + (await verify(erc721, nativeValueHex)).should.equal(true); + warnings.some((w) => /must not carry native value/.test(w.reason)).should.equal(true); + + warnings = []; + const otherContractHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '7'), { + to: tokenContractAddress, + }); + (await verify(erc721, otherContractHex)).should.equal(true); + warnings.some((w) => /not the intent token contract/.test(w.reason)).should.equal(true); + + warnings = []; + const noTokenId = { + type: 'transferToken', + recipients: [ + { + address: recipientAddress, + amount: '0', + tokenData: { tokenType: 'ERC721', tokenQuantity: '1', tokenContractAddress: nftContractAddress }, + }, + ], + }; + (await verify(noTokenId, await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '7')))).should.equal(true); + warnings.some((w) => /must declare tokenData.tokenId/.test(w.reason)).should.equal(true); + }); + + it('verifies a stored NFT intent whose tokenData tokenName is a generic display label', async function () { + // The intent schema makes tokenData.tokenName mandatory, and for an unsupported collection + // it is only a label ('erc721:token' -> the '0xerc721:token' statics placeholder). The + // declared contract is the identity, so the legitimate transfer must verify cleanly. + const intent = { + intentType: 'transferToken', + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '0', symbol: 'erc721:token' }, + tokenData: { + tokenType: 'ERC721', + tokenQuantity: '1', + tokenId: '9', + tokenName: 'erc721:token', + tokenContractAddress: nftContractAddress, + }, + }, + ], + }; + const txHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '9')); + (await verifySigningPath(undefined, intent, txHex)).should.equal(true); + warnings.should.have.length(0); + (await verifySigningPath(undefined, intent, txHex, true)).should.equal(true); + }); + + it('flags an unregistered or other-standard NFT tokenName instead of treating it as a label', async function () { + // Only a statics placeholder of the declared standard is a display label. Any other name is + // an identity declaration: unregistered it cannot be resolved, and a placeholder of the + // other standard contradicts the declared collection. + const callerFor = (tokenName: string): Record => ({ + type: 'transferToken', + recipients: [ + { + address: recipientAddress, + amount: '0', + tokenData: { + tokenType: 'ERC721', + tokenQuantity: '1', + tokenId: '7', + tokenName, + tokenContractAddress: nftContractAddress, + }, + }, + ], + }); + const txHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '7')); + + const unknownName = callerFor('hteth:not-a-registered-collection'); + (await verify(unknownName, txHex)).should.equal(true); + warnings.some((w) => /cannot be resolved to a token contract/.test(w.reason)).should.equal(true); + await verify(unknownName, txHex, { strict: true }).should.be.rejectedWith( + /cannot be resolved to a token contract/ + ); + + warnings = []; + const otherStandardName = callerFor('erc1155:token'); + (await verify(otherStandardName, txHex)).should.equal(true); + warnings.some((w) => /name different token contracts/.test(w.reason)).should.equal(true); + await verify(otherStandardName, txHex, { strict: true }).should.be.rejectedWith( + /name different token contracts/ + ); + }); + + it('flags a stored NFT intent whose tokenName names another collection', async function () { + // A name that resolves to a real, different collection is still a conflict: only the + // generic statics placeholder of the declared standard is a display label. + const intent = { + intentType: 'transferToken', + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '0', symbol: otherNftName }, + tokenData: { + tokenType: 'ERC721', + tokenQuantity: '1', + tokenId: '9', + tokenName: otherNftName, + tokenContractAddress: nftContractAddress, + }, + }, + ], + }; + const txHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '9')); + (await verifySigningPath(undefined, intent, txHex)).should.equal(true); + warnings.some((w) => /name different token contracts/.test(w.reason)).should.equal(true); + await verifySigningPath(undefined, intent, txHex, true).should.be.rejectedWith( + /name different token contracts/ + ); + }); + + it('enforces the stored tokenData tokenId on the pending-approval path', async function () { + const intent = { + intentType: 'transferToken', + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '0', symbol: 'erc721:token' }, + tokenData: { + tokenType: 'ERC721', + tokenQuantity: '1', + tokenId: '9', + tokenName: 'erc721:token', + tokenContractAddress: nftContractAddress, + }, + }, + ], + }; + const txHex = await nftTx(encodeErc721SafeTransferFrom(recipientAddress, '10')); + (await verifySigningPath(undefined, intent, txHex)).should.equal(true); + warnings.some((w) => /tokenId in calldata does not match/.test(w.reason)).should.equal(true); + await verifySigningPath(undefined, intent, txHex, true).should.be.rejectedWith( + /tokenId in calldata does not match/ + ); + }); + }); + + describe('declared intent isolation and asset binding (CECHO-2460)', function () { + const tokenTransferData = encodeErc20Transfer(recipientAddress, '1000'); + const approvalData = encodeApprove(attackerAddress, MAX_UINT256); + + 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('keeps the caller tokenData identity when the stored intent names another token', async function () { + const caller = { + type: 'transferToken', + recipients: [ + { + address: recipientAddress, + amount: '1000', + tokenData: { tokenContractAddress: localTokenContractAddress }, + }, + ], + }; + const intent = { + intentType: 'transferToken', + tokenName: serverTokenName, + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '1000', symbol: serverTokenName }, + tokenData: { tokenName: serverTokenName, tokenContractAddress: serverTokenContractAddress }, + }, + ], + }; + + // Paying the stored intent's token contradicts the caller's tokenData declaration. + const serverHex = await build({ to: serverTokenContractAddress, value: '0', data: tokenTransferData }); + (await verifySigningPath(caller, intent, serverHex)).should.equal(true); + warnings.some((w) => /not the intent token contract/.test(w.reason)).should.equal(true); + + // Paying the caller's own token verifies with nothing from the stored intent leaking in. + warnings = []; + const localHex = await build({ to: localTokenContractAddress, value: '0', data: tokenTransferData }); + (await verifySigningPath(caller, intent, localHex)).should.equal(true); + warnings.should.have.length(0); + (await verifySigningPath(caller, intent, localHex, true)).should.equal(true); + }); + + it('rejects caller tokenData identity that contradicts a flat tokenAddress', async function () { + // Two declarations of different tokens: the intended asset is unknown, so neither may + // silently win (as a precedence chain would). + const caller = { + type: 'transferToken', + recipients: [ + { + address: recipientAddress, + amount: '1000', + tokenAddress: localTokenContractAddress, + tokenData: { tokenContractAddress: serverTokenContractAddress }, + }, + ], + }; + const txHex = await build({ to: localTokenContractAddress, value: '0', data: tokenTransferData }); + + (await verifySigningPath(caller, {}, txHex)).should.equal(true); + warnings.some((w) => /name different token contracts/.test(w.reason)).should.equal(true); + await verifySigningPath(caller, {}, txHex, true).should.be.rejectedWith(/name different token contracts/); + }); + + it('rejects caller tokenData tokenName that contradicts its tokenContractAddress', async function () { + const caller = { + type: 'transferToken', + recipients: [ + { + address: recipientAddress, + amount: '1000', + tokenData: { tokenName: localTokenName, tokenContractAddress: serverTokenContractAddress }, + }, + ], + }; + const txHex = await build({ to: serverTokenContractAddress, value: '0', data: tokenTransferData }); + + (await verifySigningPath(caller, {}, txHex)).should.equal(true); + warnings.some((w) => /name different token contracts/.test(w.reason)).should.equal(true); + await verifySigningPath(caller, {}, txHex, true).should.be.rejectedWith(/name different token contracts/); + }); + + it('uses the stored tokenData tokenContractAddress when the caller declared nothing', async function () { + // The pending-approval effective params carry only `tokenName` (derived from + // `tokenData.tokenName` / the amount symbol); the stored `tokenData.tokenContractAddress` + // is the only contract anchor, so it must reach asset resolution. + const intent = { + intentType: 'transferToken', + recipients: [ + { + address: { address: recipientAddress }, + amount: { value: '1000', symbol: chain }, + tokenData: { tokenContractAddress: localTokenContractAddress }, + }, + ], + }; + const goodHex = await build({ to: localTokenContractAddress, value: '0', data: tokenTransferData }); + (await verifySigningPath(undefined, intent, goodHex)).should.equal(true); + warnings.should.have.length(0); + + warnings = []; + const otherTokenHex = await build({ to: serverTokenContractAddress, value: '0', data: tokenTransferData }); + (await verifySigningPath(undefined, intent, otherTokenHex)).should.equal(true); + warnings.some((w) => /not the intent token contract/.test(w.reason)).should.equal(true); + await verifySigningPath(undefined, intent, otherTokenHex, true).should.be.rejectedWith( + /not the intent token contract/ + ); + }); + + it('rejects token calldata for a caller-declared native payment', async function () { + const caller = { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }; + const intent = { + 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('caller-declared payment without recipients (fail closed)', function () { + // A caller that names a payment type but supplies no recipients leaves nothing to verify + // the bytes against: the stored intent is dropped (its destination may be attacker + // chosen), so signing must stop instead of continuing behind a warning. An empty array + // declares nothing, exactly like omitting the field, and no mode may downgrade this. + const attackerIntent = { + intentType: 'payment', + recipients: [{ address: { address: attackerAddress }, amount: { value: '9999', symbol: 'hteth' } }], + }; + + it('rejects a caller `transfer` type with no recipients, both modes', async function () { + const txHex = await build({ to: attackerAddress, value: '9999' }); + for (const strict of [false, true]) { + await verifySigningPath({ type: 'transfer' }, attackerIntent, txHex, strict).should.be.rejectedWith( + /caller declared a payment without recipients/ + ); + } + warnings.should.have.length(0); + }); + + it('rejects a caller `transfer` type with an empty recipients array, both modes', async function () { + const caller = { type: 'transfer', recipients: [] }; + const txHex = await build({ to: attackerAddress, value: '9999' }); + for (const strict of [false, true]) { + await verifySigningPath(caller, attackerIntent, txHex, strict).should.be.rejectedWith( + /caller declared a payment without recipients/ + ); + } + warnings.should.have.length(0); + }); + + it('treats the stored spelling `payment` as the same caller declaration', async function () { + const txHex = await build({ to: attackerAddress, value: '9999' }); + for (const strict of [false, true]) { + await verifySigningPath({ type: 'payment' }, attackerIntent, txHex, strict).should.be.rejectedWith( + /caller declared a payment without recipients/ + ); + } + warnings.should.have.length(0); + }); + + it('keeps signing a stored payment intent when the caller declared nothing', async function () { + const intent = { + intentType: 'payment', + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: 'hteth' } }], + }; + const txHex = await build({ to: recipientAddress, value: '1000' }); + for (const strict of [false, true]) { + (await verifySigningPath(undefined, intent, txHex, strict)).should.equal(true); + } + warnings.should.have.length(0); + }); + + it('keeps the stored token identity when the caller supplies only a token name', async function () { + const intent = { + intentType: 'transferToken', + tokenName: localTokenName, + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: localTokenName } }], + }; + const txHex = await build({ + to: localTokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '1000'), + }); + for (const strict of [false, true]) { + (await verifySigningPath({ tokenName: localTokenName }, intent, txHex, strict)).should.equal(true); + } + warnings.should.have.length(0); + }); + + it('rejects a caller `transferToken` type with no recipients, both modes', async function () { + // A token payment leaves the same gap as a native one: with no recipients nothing + // anchors the bytes, and the stored intent would only supply the attacker's destination. + const intent = { + intentType: 'transferToken', + tokenName: localTokenName, + recipients: [{ address: { address: attackerAddress }, amount: { value: '1000', symbol: localTokenName } }], + }; + const txHex = await build({ + to: localTokenContractAddress, + value: '0', + data: encodeErc20Transfer(attackerAddress, '1000'), + }); + for (const strict of [false, true]) { + await verifySigningPath({ type: 'transferToken' }, intent, txHex, strict).should.be.rejectedWith( + /caller declared a token transfer without recipients/ + ); + } + warnings.should.have.length(0); + }); + + it('rejects the kebab-case `transfer-token` spelling with no recipients as well', async function () { + const intent = { + intentType: 'transferToken', + tokenName: localTokenName, + recipients: [{ address: { address: attackerAddress }, amount: { value: '1000', symbol: localTokenName } }], + }; + const txHex = await build({ to: localTokenContractAddress, value: '0' }); + for (const strict of [false, true]) { + await verifySigningPath({ type: 'transfer-token' }, intent, txHex, strict).should.be.rejectedWith( + /caller declared a token transfer without recipients/ + ); + } + warnings.should.have.length(0); + }); + }); + }); + + describe('batch payments without a configured batcher contract (statics gap)', function () { + const recipients = [ + { address: recipientAddress, amount: '10' }, + { address: attackerAddress, amount: '20' }, + ]; + + it('flags native batch calldata instead of silently passing it', async function () { + const txHex = await build({ + to: attackerAddress, + value: '30', + data: encodeWithAbi( + 'batch', + ['address[]', 'uint256[]'], + [ + [recipientAddress, attackerAddress], + ['10', '20'], + ] + ), + }); + const txParams = { type: 'transfer', recipients }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /no batcher contract address is configured/.test(w.reason)).should.equal(true); + await verify(txParams, txHex, { strict: true }).should.be.rejectedWith( + /no batcher contract address is configured/ + ); + }); + + it('flags token batch calldata instead of silently passing it', async function () { + const txHex = await build({ + to: attackerAddress, + value: '0', + data: encodeWithAbi( + 'batchTransferFrom', + ['address', 'address[]', 'uint256[]'], + [tokenContractAddress, [recipientAddress, attackerAddress], ['10', '20']] + ), + }); + (await verify({ type: 'transfer', recipients }, txHex)).should.equal(true); + warnings.some((w) => /no batcher contract address is configured/.test(w.reason)).should.equal(true); + }); + }); + + describe('tokenApproval handler', function () { + const approvalParams = { type: 'tokenApproval', tokenName: 'hteth:cusdt' }; + + it('flags a tokenApproval when the network has no configured batcher contract (statics gap)', async function () { + const txHex = await build({ + to: erc7984WrapperAddress, + value: '0', + data: encodeApprove(attackerAddress, MAX_UINT256), + }); + (await verify(approvalParams, txHex)).should.equal(true); + warnings.some((w) => /no batcher contract address is configured/.test(w.reason)).should.equal(true); + }); + + it('flags a non-maximum allowance, non-approve calldata, native value and a wrong to', async function () { + const smallAllowance = await build({ + to: erc7984WrapperAddress, + value: '0', + data: encodeApprove(attackerAddress, '5'), + }); + (await verify(approvalParams, smallAllowance)).should.equal(true); + warnings.some((w) => /not the maximum ERC-20 allowance/.test(w.reason)).should.equal(true); + + warnings = []; + const notApprove = await build({ + to: erc7984WrapperAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '5'), + }); + (await verify(approvalParams, notApprove)).should.equal(true); + warnings.some((w) => /must use ERC-20 approve/.test(w.reason)).should.equal(true); + + warnings = []; + const carriesValue = await build({ + to: erc7984WrapperAddress, + value: '1', + data: encodeApprove(attackerAddress, MAX_UINT256), + }); + (await verify(approvalParams, carriesValue)).should.equal(true); + warnings.some((w) => /must not carry native value/.test(w.reason)).should.equal(true); + + warnings = []; + const wrongTo = await build({ + to: attackerAddress, + value: '0', + data: encodeApprove(attackerAddress, MAX_UINT256), + }); + (await verify(approvalParams, wrongTo)).should.equal(true); + warnings.some((w) => /to is not the intent token contract/.test(w.reason)).should.equal(true); + }); + }); + + describe('fillNonce handler', function () { + it('falls back to the base address when the intent declares no receive address', async function () { + const good = await build({ to: baseAddress, value: '1' }); + (await verify({ type: 'fillNonce' }, good)).should.equal(true); + warnings.should.have.length(0); + + const wrongTo = await build({ to: recipientAddress, value: '1' }); + (await verify({ type: 'fillNonce' }, wrongTo)).should.equal(true); + warnings.some((w) => /must send to the wallet base address/.test(w.reason)).should.equal(true); + + warnings = []; + const wrongValue = await build({ to: baseAddress, value: '2' }); + (await verify({ type: 'fillNonce' }, wrongValue)).should.equal(true); + warnings.some((w) => /must send exactly 1 wei/.test(w.reason)).should.equal(true); + + warnings = []; + const withCalldata = await build({ to: baseAddress, value: '1', data: '0xdeadbeef' }); + (await verify({ type: 'fillNonce' }, withCalldata)).should.equal(true); + warnings.some((w) => /must not carry calldata/.test(w.reason)).should.equal(true); + + await verify({ type: 'fillNonce' }, wrongTo, { strict: true }).should.be.rejectedWith( + /must send to the wallet base address/ + ); + }); + + it('verifies a 1-wei self-send to the declared receive address, not the base address', async function () { + // The nonce that gets filled is the receive address's; the wallet base address is only + // the fallback when the intent names none. + const txParams = { type: 'fillNonce', receiveAddress: recipientAddress }; + const good = await build({ to: recipientAddress, value: '1' }); + (await verify(txParams, good)).should.equal(true); + warnings.should.have.length(0); + + const toBase = await build({ to: baseAddress, value: '1' }); + (await verify(txParams, toBase)).should.equal(true); + const destinationWarning = warnings.find((w) => /must send to the declared receive address/.test(w.reason)); + should.exist(destinationWarning); + destinationWarning?.expected.to?.toLowerCase().should.equal(recipientAddress.toLowerCase()); + await verify(txParams, toBase, { strict: true }).should.be.rejectedWith( + /must send to the declared receive address/ + ); + + warnings = []; + const wrongValue = await build({ to: recipientAddress, value: '2' }); + (await verify(txParams, wrongValue)).should.equal(true); + warnings.some((w) => /must send exactly 1 wei/.test(w.reason)).should.equal(true); + }); + + it('uses the stored receive address when the caller declared nothing (pending approval)', async function () { + // resolveEffectiveTxParams does not copy receiveAddress, so the stored intent is the only + // source of the address whose nonce is filled on this path. + const intent = { intentType: 'fillNonce', receiveAddress: recipientAddress }; + const txRequest = { txRequestId: 'pa-fill', walletId: 'w', intent } as unknown as TxRequest; + const txParams = resolveEffectiveTxParams(txRequest, undefined, chain) as unknown as Record; + + const good = await build({ to: recipientAddress, value: '1' }); + (await verify(txParams, good, { intent: intent as unknown as PopulatedIntent })).should.equal(true); + warnings.should.have.length(0); + + warnings = []; + const toBase = await build({ to: baseAddress, value: '1' }); + (await verify(txParams, toBase, { intent: intent as unknown as PopulatedIntent })).should.equal(true); + warnings.some((w) => /must send to the declared receive address/.test(w.reason)).should.equal(true); + }); + }); + + describe('consolidation to the wallet base address', function () { + const consolidate = { type: 'consolidate', recipients: [] }; + const toBase = { consolidationToBaseAddress: true }; + + it('verifies a token consolidation that pays the base address and carries no native value', async function () { + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeErc20Transfer(baseAddress, '5'), + }); + (await verify(consolidate, txHex, { verification: toBase })).should.equal(true); + (await verify(consolidate, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('rejects native value riding along token-shaped calldata when the caller asked for the check', async function () { + // transfer(baseAddress, 0) passes the recipient check, so without the value check the + // native balance would go to whatever contract `to` names. + const txHex = await build({ + to: attackerAddress, + value: '5000', + data: encodeErc20Transfer(baseAddress, '0'), + }); + for (const strict of [false, true]) { + await verify(consolidate, txHex, { strict, verification: toBase }).should.be.rejectedWith( + /must not carry native value/ + ); + } + }); + + it('flags the same transaction on a consolidate intent (warn, throw in strict mode)', async function () { + const txHex = await build({ + to: attackerAddress, + value: '5000', + data: encodeErc20Transfer(baseAddress, '0'), + }); + (await verify(consolidate, txHex)).should.equal(true); + warnings.some((w) => /must not carry native value/.test(w.reason)).should.equal(true); + await verify(consolidate, txHex, { strict: true }).should.be.rejectedWith(/must not carry native value/); + }); + }); + + describe('unsupported and unhandled intent types', function () { + it('rejects contractCall and customTx outright on EVM (INFOSEC-671)', async function () { + const txHex = await build({ to: attackerAddress, value: '0', data: '0xdeadbeef' }); + for (const type of ['contractCall', 'customTx']) { + for (const strict of [false, true]) { + await verify({ type, recipients: [{ address: attackerAddress, amount: '0' }] }, txHex, { + strict, + }).should.be.rejectedWith(/unsupported intent type for EVM TSS/); + } + } + }); + + it('flags every intent type without a handler instead of silently passing', async function () { + const txHex = await build({ to: attackerAddress, value: '0', data: '0xdeadbeef' }); + for (const type of ['acceleration', 'bridgeFunds', 'enableToken', 'defiRedeem', 'stakeWithCallData']) { + warnings = []; + const result = await verify({ type, recipients: [] }, txHex); + result.should.equal(true, `${type} must not throw`); + warnings.some((w) => /no fail-closed verifier/.test(w.reason)).should.equal(true, `${type} must be flagged`); + } + await verify({ type: 'bridgeFunds', recipients: [] }, txHex, { strict: true }).should.be.rejectedWith( + /no fail-closed verifier/ + ); + }); + }); + + describe('undecodable bytes (fail closed before any handler)', function () { + it('rejects a placeholder txHex in both modes instead of warning', async function () { + for (const strict of [false, true]) { + await verify({ type: 'fillNonce' }, '0x', { strict }).should.be.rejectedWith( + /transaction bytes could not be decoded to verify intent/ + ); + } + }); + + it('rejects hex that is not a transaction for a handled and an unhandled type alike', async function () { + const cases: Array> = [ + { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, + { type: 'acceleration' }, + ]; + for (const txParams of cases) { + for (const strict of [false, true]) { + await verify(txParams, '0xdeadbeef', { strict }).should.be.rejectedWith( + /transaction bytes could not be decoded to verify intent/ + ); + } + } + }); + + it('rejects a missing txHex with the historical message', async function () { + await coin + .verifyTransaction({ + txParams: { type: 'fillNonce' }, + txPrebuild: {}, + wallet, + verification: {}, + walletType: 'tss', + }) + .should.be.rejectedWith('missing txHex in txPrebuild'); + }); + }); + + describe('pending-approval path (stored intent via resolveEffectiveTxParams)', function () { + const verifyPendingApproval = async ( + intent: Record, + txHex: string, + strict = false + ): Promise => { + const txRequest = { txRequestId: 'pa', walletId: 'w', intent } as unknown as TxRequest; + const txParams = resolveEffectiveTxParams(txRequest, undefined, chain); + return verify(txParams as unknown as Record, txHex, { + strict, + intent: txRequest.intent as PopulatedIntent, + }); + }; + + it('keeps the existing native checks throwing on this path (redirect and inflated value)', async function () { + const intent = { + intentType: 'payment', + recipients: [{ address: { address: recipientAddress }, amount: { value: '1000', symbol: 'eth' } }], + }; + const redirectHex = await build({ to: attackerAddress, value: '1000' }); + await verifyPendingApproval(intent, redirectHex).should.be.rejectedWith(/destination address does not match/); + + const inflatedHex = await build({ to: recipientAddress, value: '9999' }); + await verifyPendingApproval(intent, inflatedHex).should.be.rejectedWith(/amount in txPrebuild does not match/); + }); + + it('flags swapped calldata on this path (warn, throw in strict mode)', async function () { + const data = '0xdeadbeef'; + const intent = { + intentType: 'payment', + recipients: [{ address: { address: tokenContractAddress }, amount: { value: '0', symbol: 'eth' }, data }], + }; + const txHex = await build({ to: tokenContractAddress, value: '0', data: '0xdeadbe00' }); + (await verifyPendingApproval(intent, txHex)).should.equal(true); + warnings.some((w) => /calldata does not match/.test(w.reason)).should.equal(true); + await verifyPendingApproval(intent, txHex, true).should.be.rejectedWith(/calldata does not match/); + }); + + it('verifies a matching payment intent on this path with no warnings', async function () { + const data = '0xdeadbeef'; + const intent = { + intentType: 'payment', + recipients: [{ address: { address: tokenContractAddress }, amount: { value: '0', symbol: 'eth' }, data }], + }; + const txHex = await build({ to: tokenContractAddress, value: '0', data }); + (await verifyPendingApproval(intent, txHex)).should.equal(true); + warnings.should.have.length(0); + }); + + it('keeps verifying a caller-declared DeFi leg that matches defiParams', async function () { + // The stored flow is rejected below, but a caller that declares the leg itself still + // gets the calldata checks: the caller's amount and its intent type are its own. + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeApprove(attackerAddress, '123'), + }); + for (const strict of [false, true]) { + ( + await verify({ type: 'defiApprove', defiParams: { vaultId: 'vault', amount: '123' } }, txHex, { strict }) + ).should.equal(true); + } + }); + + it('rejects stored defi-* intents: no local vault binding can verify their calldata', async function () { + // A stored intent carries only vaultId and amount, and both come from the server. Nothing + // local names the vault contract or the approve spender, so comparing the bytes with the + // stored fields would only prove the server agrees with itself: fail closed instead. + const defiCases: Array<{ intentType: string; data: string; amountField: string }> = [ + { intentType: 'defi-approve', data: encodeApprove(attackerAddress, '123'), amountField: 'amount' }, + { + intentType: 'defi-deposit', + data: encodeWithAbi('deposit', ['uint256', 'address'], ['123', baseAddress]), + amountField: 'amount', + }, + { + intentType: 'defi-withdraw', + data: encodeWithAbi('redeem', ['uint256', 'address', 'address'], ['123', baseAddress, baseAddress]), + amountField: 'shareTokenAmount', + }, + ]; + for (const { intentType, data, amountField } of defiCases) { + const intent: Record = { intentType, vaultId: 'vault', [amountField]: '123' }; + const txHex = await build({ to: tokenContractAddress, value: '0', data }); + for (const strict of [false, true]) { + await verifyPendingApproval(intent, txHex, strict).should.be.rejectedWith( + /stored defi[A-Za-z]+ intent cannot be verified: no locally known vault contract/ + ); + } + } + warnings.should.have.length(0); + }); + + it('keeps rejecting stored defi-redeem intents, which have no handler, as master does', async function () { + const intent = { intentType: 'defi-redeem', vaultId: 'v', amount: '1' }; + const txHex = await build({ to: tokenContractAddress, value: '0', data: '0xdeadbeef' }); + for (const strict of [false, true]) { + await verifyPendingApproval(intent, txHex, strict).should.be.rejectedWith(/Recipient details are required/); + } + warnings.should.have.length(0); + }); + }); + + describe('legacy (non-EIP-1559) chains run the parent verifier', function () { + it('rejects contractCall outright instead of returning true', async function () { + const txHex = await build({ to: attackerAddress, value: '0', data: '0xdeadbeef' }); + await verify( + { type: 'contractCall', recipients: [{ address: attackerAddress, amount: '0' }] }, + txHex + ).should.be.rejectedWith(/unsupported intent type for EVM TSS/); + }); + + it('flags a payment with unrecognized calldata instead of returning true', async function () { + const txHex = await build({ + to: tokenContractAddress, + value: '0', + data: encodeApprove(attackerAddress, MAX_UINT256), + }); + const txParams = { type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }; + (await verify(txParams, txHex)).should.equal(true); + warnings.some((w) => /not a recognized payment shape/.test(w.reason)).should.equal(true); + }); + }); + + describe('warning infrastructure', function () { + it('reports decoded vs expected to / value / data through onVerificationWarning', async function () { + const data = '0xdeadbeef'; + const txHex = await build({ to: attackerAddress, value: '7', data: '0xdeadbe00' }); + await verify({ type: 'transfer', recipients: [{ address: tokenContractAddress, amount: '0', data }] }, txHex); + warnings.length.should.be.greaterThanOrEqual(3); + const destinationWarning = warnings.find((w) => /to does not match/.test(w.reason)); + destinationWarning?.expected.to?.toLowerCase().should.equal(tokenContractAddress.toLowerCase()); + destinationWarning?.decoded.to?.toLowerCase().should.equal(attackerAddress.toLowerCase()); + const valueWarning = warnings.find((w) => /value does not match/.test(w.reason)); + valueWarning?.expected.value?.should.equal('0'); + valueWarning?.decoded.value?.should.equal('7'); + const dataWarning = warnings.find((w) => /calldata does not match/.test(w.reason)); + dataWarning?.expected.data?.should.equal('0xdeadbeef'); + dataWarning?.decoded.data?.should.equal('0xdeadbe00'); + }); + + it('does not fire the callback when the transaction matches the intent', async function () { + const txHex = await build({ to: recipientAddress, value: '1000' }); + await verify({ type: 'transfer', recipients: [{ address: recipientAddress, amount: '1000' }] }, txHex); + warnings.should.have.length(0); + }); + + // The sink is local client code, not trusted input: neither a synchronous throw nor a + // rejection may decide whether the user's key share signs, and warn mode never awaits it. + const verifyWithSink = async (sink: (warning: TssVerificationWarning) => void): Promise => { + const sinkBitgo = TestBitGo.decorate(BitGoAPI, { env: 'test', onVerificationWarning: sink }); + const sinkCoin = EvmCoin.createInstance(sinkBitgo, staticsCoin); + const sinkWallet = new Wallet(sinkBitgo, sinkCoin, { coinSpecific: { baseAddress } }); + // A new (non-fatal) finding, so the sink is exactly what this test exercises. + const txHex = await build({ to: attackerAddress, value: '0', data: '0xdeadbe00' }); + return sinkCoin.verifyTransaction({ + txParams: { + type: 'transfer', + recipients: [{ address: attackerAddress, amount: '0', data: '0xdeadbeef' }], + }, + txPrebuild: { txHex }, + wallet: sinkWallet, + verification: {}, + walletType: 'tss', + }); + }; + + it('keeps warn mode signing when the sink throws synchronously', async function () { + const result = await verifyWithSink(() => { + throw new Error('sink exploded'); + }); + result.should.equal(true); + }); + + it('keeps warn mode signing when the sink rejects, leaving no unhandled rejection', async function () { + const rejections: unknown[] = []; + const onRejection = (reason: unknown): void => { + rejections.push(reason); + }; + process.on('unhandledRejection', onRejection); + try { + const result = await verifyWithSink(async () => { + throw new Error('sink rejected'); + }); + result.should.equal(true); + // Give the event loop a turn: a floating rejection from the sink would surface now. + await delay(0); + rejections.should.have.length(0); + } finally { + process.removeListener('unhandledRejection', onRejection); + } + }); + + it('bounds calldata in the console warning but hands the full mismatch to the sink', async function () { + const longData = `0xdeadbeef${'ab'.repeat(600)}`; + const txHex = await build({ to: attackerAddress, value: '0', data: '0xdeadbe00' }); + const logged: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]): void => { + logged.push(args.map((arg) => String(arg)).join(' ')); + }; + try { + const result = await verify( + { type: 'transfer', recipients: [{ address: attackerAddress, amount: '0', data: longData }] }, + txHex + ); + result.should.equal(true); + } finally { + console.warn = originalWarn; + } + + const warning = warnings.find((w) => /calldata does not match/.test(w.reason)); + warning?.expected.data?.should.equal(longData); + const line = logged.find((entry) => /calldata does not match/.test(entry)); + line!.should.containEql('0xdeadbeef...(1210 chars)'); + line!.should.not.containEql(longData); + }); + + it('bounds the logged reason but gives the sink the full diagnostic', async function () { + // A reason can echo an unbounded value: an unresolvable local token identity is the one a + // valid transaction reaches deterministically (decoder errors can echo calldata too). The + // bytes are a real, matching ERC-20 transfer; only the local token name is unresolvable. + const longTokenName = `hteth:${'not-a-token-'.repeat(40)}`; + const txHex = await build({ + to: localTokenContractAddress, + value: '0', + data: encodeErc20Transfer(recipientAddress, '1000'), + }); + const logged: string[][] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]): void => { + logged.push(args.map((arg) => String(arg))); + }; + let result = false; + try { + result = await verifySigningPath( + { + type: 'transferToken', + tokenName: longTokenName, + recipients: [{ address: recipientAddress, amount: '1000' }], + }, + { intentType: 'transferToken', recipients: [] }, + txHex + ); + } finally { + console.warn = originalWarn; + } + result.should.equal(true); + + const warning = warnings.find((w) => /cannot be resolved to a token contract/.test(w.reason)); + warning!.reason.length.should.be.greaterThan(256); + warning!.reason.should.containEql(longTokenName); + + const [loggedReason] = logged[0]; + loggedReason.should.containEql('...(truncated, '); + loggedReason.should.containEql(`, ${warning!.reason.length} chars)`); + loggedReason.should.not.containEql(longTokenName); + loggedReason.length.should.be.lessThan(warning!.reason.length); + }); + }); + }); +}); diff --git a/modules/sdk-coin-flr/src/flr.ts b/modules/sdk-coin-flr/src/flr.ts index 908700b2913..737b5dd0e68 100644 --- a/modules/sdk-coin-flr/src/flr.ts +++ b/modules/sdk-coin-flr/src/flr.ts @@ -26,9 +26,10 @@ import { Recipient, SignableTransaction, TransactionExplanation, + TransactionType, Entry, } from '@bitgo/sdk-core'; -import { BaseCoin as StaticsBaseCoin, coins } from '@bitgo/statics'; +import { BaseCoin as StaticsBaseCoin, coins, FlareNetwork } from '@bitgo/statics'; import { AbstractEthLikeNewCoins, optionalDeps, @@ -36,6 +37,7 @@ import { UnsignedSweepTxMPCv2, RecoverOptions, OfflineVaultTxInfo, + VerifyEthTransactionOptions, } from '@bitgo/abstract-eth'; import { TransactionBuilder } from './lib'; import { FlrPLib } from '@bitgo/sdk-coin-flrp'; @@ -57,6 +59,14 @@ import { */ interface AtomicTransactionExplanation extends TransactionExplanation { inputs: Entry[]; + /** Decoded transaction type, set by the atomic builder's explainTransaction. */ + type?: TransactionType; +} + +/** The user intent a TSS transaction must satisfy, taken from the one source that speaks for it. */ +interface FlrDeclaredTssIntent { + type?: string; + recipients: Array<{ address: string; amount: string }>; } export class Flr extends AbstractEthLikeNewCoins { @@ -136,6 +146,16 @@ export class Flr extends AbstractEthLikeNewCoins { return false; } + /** + * Decode atomic transaction bytes with the atomic builder. + * @param {string} txHex - the transaction hex + * @returns {Promise} the built atomic transaction + * @private + */ + private async getAtomicTransaction(txHex: string): Promise { + return this.getAtomicBuilder().from(txHex).build(); + } + /** * Explains an atomic transaction using atomic builder. * @param {string} txHex - the transaction hex @@ -143,8 +163,7 @@ export class Flr extends AbstractEthLikeNewCoins { * @private */ private async explainAtomicTransaction(txHex: string): Promise { - const txBuilder = this.getAtomicBuilder().from(txHex); - const tx = await txBuilder.build(); + const tx = await this.getAtomicTransaction(txHex); return tx.explainTransaction() as AtomicTransactionExplanation; } @@ -155,8 +174,7 @@ export class Flr extends AbstractEthLikeNewCoins { * @private */ private async verifySignatureForAtomicTransaction(txHex: string): Promise { - const txBuilder = this.getAtomicBuilder().from(txHex); - const tx = await txBuilder.build(); + const tx = await this.getAtomicTransaction(txHex); const payload = tx.signablePayload; const signatures = tx.signature.map((s) => Buffer.from(FlrPLib.Utils.removeHexPrefix(s), 'hex')); const recoverPubkey = signatures.map((s) => FlrPLib.Utils.recoverySignature(payload, s)); @@ -312,6 +330,202 @@ export class Flr extends AbstractEthLikeNewCoins { return true; } + /** + * Verify a TSS transaction against the intent the user approved. + * + * A C-chain -> P-chain export is signed as an Avalanche atomic transaction, which the EVM + * transaction decoder cannot parse, so it is verified here before the parent verifier runs. + * Every other FLR TSS transaction keeps the parent's EVM intent verification unchanged. + * + * @param {VerifyEthTransactionOptions} params + * @returns {Promise} + */ + async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise { + const intent = Flr.resolveDeclaredTssIntent(params); + const declaredExport = Flr.isExportIntent(intent); + if (!Flr.isAtomicTransactionHex(params.txPrebuild?.txHex)) { + if (declaredExport) { + // Exporting from the C-chain is only ever an atomic transaction: bytes that are not atomic + // can never satisfy a declared export, and no warn / strict policy may downgrade that. + throw new Error('declared export intent requires an atomic transaction in txPrebuild.txHex'); + } + return super.verifyTssTransaction(params); + } + if (!declaredExport) { + // Other atomic formats (import / importtoc) keep the parent baseline: this verifier owns + // only the C-chain -> P-chain export shape, so their policy is not decided here. + return super.verifyTssTransaction(params); + } + return this.verifyAtomicExportTssTransaction(params, intent); + } + + /** + * Verify the Avalanche atomic export bytes about to be signed against the declared intent. + * + * The amount follows the Export convention already used by {@link validateHopPrebuild}: the + * declared C-chain amount is in wei, the P-chain output is in nanoFLR, and the output must cover + * the declared amount plus the import fee, up to the network's maximum import fee. + * + * @param {VerifyEthTransactionOptions} params + * @param {FlrDeclaredTssIntent} intent - the intent resolved by the caller + * @returns {Promise} + * @private + */ + private async verifyAtomicExportTssTransaction( + params: VerifyEthTransactionOptions, + intent: FlrDeclaredTssIntent + ): Promise { + const { txPrebuild, wallet } = params; + if (!wallet || !txPrebuild?.txHex) { + throw new Error('missing params'); + } + + let tx: BaseTransaction; + let explained: AtomicTransactionExplanation; + try { + tx = await this.getAtomicTransaction(txPrebuild.txHex); + explained = tx.explainTransaction() as AtomicTransactionExplanation; + } catch (e) { + throw new Error(`transaction bytes could not be decoded to verify intent: ${(e as Error).message}`); + } + + if (explained.type !== TransactionType.Export) { + throw new Error(`atomic transaction type ${String(explained.type)} does not match the declared export intent`); + } + // Explanations map every owner onto this coin's P-chain alias, so matching the output address + // does not prove the export targets the P-chain. Read the chains encoded in the bytes. + const { sourceChain, destinationChain } = (tx as FlrPLib.Transaction).toJson(); + if (sourceChain !== 'C' || destinationChain !== 'P') { + throw new Error( + `atomic export must be a C-chain to P-chain export but encodes ${sourceChain} -> ${destinationChain}` + ); + } + const outputs = explained.outputs; + if (outputs.length !== 1) { + throw new Error(`atomic export transaction must have exactly 1 output but ${outputs.length} found`); + } + if (intent.recipients.length !== 1) { + throw new Error(`atomic export requires exactly 1 recipient but ${intent.recipients.length} found`); + } + + const declaredRecipient = intent.recipients[0]; + const declaredDestination = Flr.normalizePChainAddressSet(declaredRecipient.address); + if (!declaredDestination) { + throw new Error(`atomic export recipient ${declaredRecipient.address} is not a valid P-chain address`); + } + const decodedDestination = Flr.normalizePChainAddressSet(outputs[0].address); + if (!decodedDestination) { + throw new Error(`atomic export destination ${outputs[0].address} is not a valid P-chain address`); + } + if (declaredDestination !== decodedDestination) { + throw new Error( + `atomic export destination ${outputs[0].address} does not match the declared recipient ${declaredRecipient.address}` + ); + } + + this.assertAtomicExportAmount(declaredRecipient.amount, explained.outputAmount); + return true; + } + + /** + * Resolve the intent that speaks for the user, with the same precedence as the parent verifier: + * the caller's own params when it declared a type or recipients, otherwise the stored tx request + * intent. `txParams` cannot answer this because the linking path back-fills it from the stored + * intent, so `callerTxParams` (the untouched caller input) decides. + * + * @param {VerifyEthTransactionOptions} params + * @returns {FlrDeclaredTssIntent} + * @private + */ + private static resolveDeclaredTssIntent(params: VerifyEthTransactionOptions): FlrDeclaredTssIntent { + const callerParams = params.callerTxParams ?? (params.intent ? undefined : params.txParams); + const callerDeclaresIntent = !!(callerParams?.type || callerParams?.recipients?.length); + const storedIntent = params.intent && !callerDeclaresIntent ? params.intent : undefined; + + if (storedIntent) { + return { + type: storedIntent.intentType, + recipients: (storedIntent.recipients ?? []).map((recipient) => ({ + address: recipient.address.address, + amount: String(recipient.amount.value), + })), + }; + } + + const declared = callerParams ?? params.txParams; + return { + type: declared?.type, + recipients: (declared?.recipients ?? []).map((recipient) => ({ + address: recipient.address, + amount: String(recipient.amount), + })), + }; + } + + /** True when the declared intent type selects an export, in either the caller or stored spelling. */ + private static isExportIntent(intent: FlrDeclaredTssIntent): boolean { + return (intent.type ?? '').trim().toLowerCase() === 'export'; + } + + /** + * Avalanche atomic transactions are identified by their codec prefix (0x0000), the same marker + * used by {@link explainTransaction} and {@link isSignablePreHashed}. + */ + private static isAtomicTransactionHex(txHex?: string): boolean { + const hex = txHex?.startsWith('0x') ? txHex.slice(2) : txHex; + return !!hex && hex.startsWith('0000'); + } + + /** + * Normalize a P-chain address (a single address or a `~`-separated multisig set) so address + * order and case cannot make two descriptions of the same destination compare unequal. + * + * @param {string} address - the P-chain address or address set + * @returns {string | undefined} the normalized set, undefined when any address is invalid + * @private + */ + private static normalizePChainAddressSet(address?: string): string | undefined { + const addresses = (address ?? '') + .split('~') + .map((part) => part.trim()) + .filter((part) => !!part); + if (!addresses.length || !addresses.every((part) => FlrPLib.Utils.isValidAddress(part))) { + return undefined; + } + return addresses + .map((part) => part.toLowerCase()) + .sort() + .join('~'); + } + + /** + * Check that the atomic export output covers the declared C-chain amount plus an import fee. + * + * The declared amount is in wei and the export output is in nanoFLR (the same wei -> nanoFLR + * conversion {@link validateHopPrebuild} applies). The output may not exceed the declared amount + * plus the network's maximum import fee, which is also the bound Flrp.validateExportTx applies. + * + * @param {string} declaredAmount - the C-chain amount declared by the user, in wei + * @param {string} outputAmount - the amount the atomic export pays on the P-chain, in nanoFLR + * @private + */ + private assertAtomicExportAmount(declaredAmount: string, outputAmount?: string): void { + const flrpNetwork = coins.get(this.getFlrP()).network as FlareNetwork; + const amount = new BigNumber(declaredAmount); + const output = new BigNumber(outputAmount ?? ''); + if (!amount.isFinite() || amount.isNegative() || !output.isFinite() || output.isNegative()) { + throw new Error(`atomic export amount ${declaredAmount} or output amount ${outputAmount} is not a valid amount`); + } + const expectedOutput = amount.div(1e9); + const minOutput = expectedOutput.plus(flrpNetwork.minImportToPFee); + const maxOutput = expectedOutput.plus(flrpNetwork.maxImportFee); + if (output.isLessThan(minOutput) || output.isGreaterThan(maxOutput)) { + throw new Error( + `atomic export output amount ${outputAmount} is outside the expected range [${minOutput.toFixed()}, ${maxOutput.toFixed()}] for the declared amount ${declaredAmount}` + ); + } + } + /** * Check if an address is a FLR C-chain address (0x format) * @param {string} address - the address to check diff --git a/modules/sdk-coin-flr/test/unit/flr.ts b/modules/sdk-coin-flr/test/unit/flr.ts index 602fd28d2be..5a34da88d08 100644 --- a/modules/sdk-coin-flr/test/unit/flr.ts +++ b/modules/sdk-coin-flr/test/unit/flr.ts @@ -2,11 +2,12 @@ import * as should from 'should'; import { TestBitGo, TestBitGoAPI } from '@bitgo/sdk-test'; import { BitGoAPI } from '@bitgo/sdk-api'; -import { coins } from '@bitgo/statics'; +import { coins, FlareNetwork } from '@bitgo/statics'; -import { Flr, Tflr } from '../../src/index'; -import { ExplainTransactionOptions } from '../../src/iface'; +import { Flr, Tflr, TransactionBuilder } from '../../src/index'; +import { ExplainTransactionOptions, FlrTransactionParams } from '../../src/iface'; import { UnsignedSweepTxMPCv2 } from '@bitgo/abstract-eth'; +import { FlrPLib } from '@bitgo/sdk-coin-flrp'; import { mockDataUnsignedSweep, mockDataNonBitGoRecovery, EXPORT_C_TEST_DATA } from '../resources'; import nock from 'nock'; import { common, TransactionType, Wallet } from '@bitgo/sdk-core'; @@ -31,6 +32,19 @@ describe('flr', function () { let hopExportTxBitgoSignature; const address2 = '0x7e85bdc27c050e3905ebf4b8e634d9ad6edd0de6'; + const tssBaseAddress = '0x174cfd823af8ce27ed0afee3fcf3c3ba259116be'; + + // verifyTssTransaction rejects bytes it cannot decode, so the TSS flows below build the + // transaction they verify instead of passing a placeholder '0x'. + const buildTflrNativeTxHex = async (to: string, value: string): Promise => { + const txBuilder = new TransactionBuilder(coins.get('tflr')); + txBuilder.type(TransactionType.SingleSigSend); + txBuilder.fee({ fee: '10', gasLimit: '21000' }); + txBuilder.counter(1); + txBuilder.contract(to); + txBuilder.value(value); + return (await txBuilder.build()).toBroadcastFormat(); + }; const hopExportTx = EXPORT_C_TEST_DATA.fullsigntxHex; const hopExportTxId = '0x' + getTxHash(hopExportTx).toString('hex'); @@ -460,7 +474,7 @@ describe('flr', function () { // txPrebuild with consolidateId (set by server during consolidation build) const txPrebuild = { consolidateId: '68a7d5d0c66e74e216b97173bd558c6d', - txHex: '0x', + txHex: await buildTflrNativeTxHex(tssBaseAddress, '1000000000000000000'), coin: 'tflr', walletId: 'fakeWalletId', }; @@ -491,7 +505,7 @@ describe('flr', function () { }; const txPrebuild = { - txHex: '0x', + txHex: await buildTflrNativeTxHex(tssBaseAddress, '1000000000000000000'), coin: 'tflr', walletId: 'fakeWalletId', }; @@ -522,7 +536,7 @@ describe('flr', function () { }; const txPrebuild = { - txHex: '0x', + txHex: await buildTflrNativeTxHex(tssBaseAddress, '1'), coin: 'tflr', walletId: 'fakeWalletId', }; @@ -552,8 +566,9 @@ describe('flr', function () { walletPassphrase: 'fakeWalletPassphrase', }; + // fillNonce is exactly a 1 wei self-send to the base address. const txPrebuild = { - txHex: '0x', + txHex: await buildTflrNativeTxHex(tssBaseAddress, '1'), coin: 'tflr', walletId: 'fakeWalletId', }; @@ -633,6 +648,278 @@ describe('flr', function () { }); isTransactionVerified.should.equal(true); }); + + describe('TSS atomic export', function () { + // The export fixture is a real C -> P atomic export whose P-chain output is + // EXPORT_C_TEST_DATA.amount nanoFLR. The legitimate C-chain request that produces it + // exports that output minus the minimum P-chain import fee, in wei. + const minImportToPFee = (coins.get('tflrp').network as FlareNetwork).minImportToPFee; + const declaredAmount = ( + (BigInt(EXPORT_C_TEST_DATA.amount) - BigInt(minImportToPFee)) * + BigInt(1000000000) + ).toString(); + const exportPrebuild = { + txHex: EXPORT_C_TEST_DATA.unsignedTxHex, + coin: 'tflr', + walletId: 'fakeWalletId', + }; + const pAddress = EXPORT_C_TEST_DATA.pAddresses[0]; + const singlePAddress = EXPORT_C_TEST_DATA.pAddresses[1]; + + const verifyTss = ( + txParams: FlrTransactionParams, + txPrebuild: { txHex: string } & Record, + extra: Record = {} + ) => + tflrCoin.verifyTransaction({ + txParams, + txPrebuild, + wallet: new Wallet(bitgo, tflrCoin, { + coinSpecific: { baseAddress: EXPORT_C_TEST_DATA.cHexAddress }, + }), + walletType: 'tss', + ...extra, + }); + + it('should verify a legitimate TSS atomic export', async function () { + const verified = await verifyTss( + { + type: 'Export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + exportPrebuild + ); + verified.should.equal(true); + }); + + it('should verify a legitimate TSS atomic export declared with the multisig set sorted', async function () { + const verified = await verifyTss( + { + type: 'export', + recipients: [ + { + address: EXPORT_C_TEST_DATA.pAddresses.slice().sort().join('~'), + amount: declaredAmount, + }, + ], + }, + exportPrebuild + ); + verified.should.equal(true); + }); + + it('should verify a legitimate TSS atomic export from the stored pending-approval intent', async function () { + // Pending approval: the caller supplies no txParams, resolveEffectiveTxParams has already + // folded the stored intent into txParams, and the stored intent is the source of truth. + const intent = { + intentType: 'export', + recipients: [ + { + address: { address: EXPORT_C_TEST_DATA.pMultisigAddress }, + amount: { value: declaredAmount, symbol: 'tflr' }, + }, + ], + }; + const verified = await verifyTss( + { + type: 'export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + exportPrebuild, + { callerTxParams: undefined, intent } + ); + verified.should.equal(true); + }); + + it('should reject an atomic export whose destination is not the declared recipient', async function () { + await verifyTss( + { type: 'Export', recipients: [{ address: singlePAddress, amount: declaredAmount }] }, + exportPrebuild + ).should.be.rejectedWith(/does not match the declared recipient/); + }); + + it('should reject an atomic export from the stored intent whose destination is not the declared recipient', async function () { + const intent = { + intentType: 'export', + recipients: [{ address: { address: singlePAddress }, amount: { value: declaredAmount, symbol: 'tflr' } }], + }; + await verifyTss( + { + type: 'export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + exportPrebuild, + { callerTxParams: undefined, intent } + ).should.be.rejectedWith(/does not match the declared recipient/); + }); + + it('should let the caller-declared intent win over a stored export intent', async function () { + const intent = { + intentType: 'export', + recipients: [ + { + address: { address: EXPORT_C_TEST_DATA.pMultisigAddress }, + amount: { value: declaredAmount, symbol: 'tflr' }, + }, + ], + }; + await verifyTss( + { + type: 'export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + exportPrebuild, + { + callerTxParams: { type: 'Export', recipients: [{ address: singlePAddress, amount: declaredAmount }] }, + intent, + } + ).should.be.rejectedWith(/does not match the declared recipient/); + }); + + it('should reject an atomic export that exports less than the declared amount', async function () { + await verifyTss( + { + type: 'Export', + recipients: [ + { + address: EXPORT_C_TEST_DATA.pMultisigAddress, + amount: (BigInt(declaredAmount) * BigInt(2)).toString(), + }, + ], + }, + exportPrebuild + ).should.be.rejectedWith(/outside the expected range/); + }); + + it('should reject an atomic export that exports more than the declared amount plus the maximum import fee', async function () { + await verifyTss( + { type: 'Export', recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: '1000000' }] }, + exportPrebuild + ).should.be.rejectedWith(/outside the expected range/); + }); + + it('should reject an atomic export with more than one declared recipient', async function () { + await verifyTss( + { + type: 'Export', + recipients: [ + { address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }, + { address: pAddress, amount: '1' }, + ], + }, + exportPrebuild + ).should.be.rejectedWith(/exactly 1 recipient/); + }); + + it('should reject atomic bytes declared as a payment instead of an export', async function () { + // Wrongly labelled atomic bytes must fail closed before the warn / strict policy. + await verifyTss( + { + type: 'transfer', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + exportPrebuild + ).should.be.rejected(); + }); + + it('should reject a C-chain export tx labelled with an import intent', async function () { + // These are Export bytes declared as an import: the type is wrong, so nothing here may + // verify them. Import / ImportToC policy is not decided by this verifier, so the request + // keeps the parent baseline and fails closed on bytes the EVM decoder cannot read. + await verifyTss( + { + type: 'import', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + exportPrebuild + ).should.be.rejectedWith(/could not be decoded to verify intent/); + }); + + it('should reject an atomic export whose destination chain is not the P-chain', async function () { + // The fixture's destinationChain field holds the 32-byte P-chain ID. Replacing it with + // the C-chain ID keeps the destination address and amount identical, so only the chain + // check can catch it: explanations format every owner with this coin's P-chain alias. + // Layout: 10-byte header, 32-byte C-chain blockchainId, then 32-byte destinationChain. + const destinationChainOffset = 84; + const cChainIdHex = FlrPLib.Utils.cb58Decode( + (coins.get('tflrp').network as FlareNetwork).cChainBlockchainID + ).toString('hex'); + const exportTxHex = FlrPLib.Utils.removeHexPrefix(EXPORT_C_TEST_DATA.unsignedTxHex); + const misdirectedExportHex = + '0x' + + exportTxHex.slice(0, destinationChainOffset) + + cChainIdHex + + exportTxHex.slice(destinationChainOffset + cChainIdHex.length); + + await verifyTss( + { + type: 'Export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + { txHex: misdirectedExportHex, coin: 'tflr', walletId: 'fakeWalletId' } + ).should.be.rejectedWith(/must be a C-chain to P-chain export/); + }); + + it('should reject a declared export backed by non-atomic transaction bytes', async function () { + const evmTxHex = await buildTflrNativeTxHex(tssBaseAddress, '1'); + await verifyTss( + { + type: 'Export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + { txHex: evmTxHex, coin: 'tflr', walletId: 'fakeWalletId' } + ).should.be.rejectedWith(/declared export intent requires an atomic transaction/); + }); + + it('should reject a declared export that only supplies the prehashed signable digest', async function () { + // The MPCv1 signing path passes signableHex (SHA-256 of the atomic body) as txHex because + // v1 has no prehashed signable support. The serialized bytes are required to verify the + // intent, so a digest-only export fails closed instead of being signed unverified. + await verifyTss( + { + type: 'Export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + { txHex: 'a'.repeat(64), coin: 'tflr', walletId: 'fakeWalletId' } + ).should.be.rejectedWith(/declared export intent requires an atomic transaction/); + }); + + it('should reject undecodable atomic bytes', async function () { + await verifyTss( + { + type: 'Export', + recipients: [{ address: EXPORT_C_TEST_DATA.pMultisigAddress, amount: declaredAmount }], + }, + { txHex: '0x0000deadbeef', coin: 'tflr', walletId: 'fakeWalletId' } + ).should.be.rejectedWith(/could not be decoded to verify intent/); + }); + + it('should keep ordinary EVM TSS transfers on the parent verifier', async function () { + const verified = await verifyTss( + { + type: 'transfer', + recipients: [{ address: tssBaseAddress, amount: '100000000000000000' }], + }, + { + txHex: await buildTflrNativeTxHex(tssBaseAddress, '100000000000000000'), + coin: 'tflr', + walletId: 'fakeWalletId', + } + ); + verified.should.equal(true); + }); + + it('should keep rejecting EVM TSS transfers whose destination is not the declared recipient', async function () { + await verifyTss( + { type: 'transfer', recipients: [{ address: address2, amount: '100000000000000000' }] }, + { + txHex: await buildTflrNativeTxHex(tssBaseAddress, '100000000000000000'), + coin: 'tflr', + walletId: 'fakeWalletId', + } + ).should.be.rejected(); + }); + }); }); describe('Hop export tx verify', () => { diff --git a/modules/sdk-core/CHANGELOG.md b/modules/sdk-core/CHANGELOG.md index fae39f2b2e6..359d9ffde76 100644 --- a/modules/sdk-core/CHANGELOG.md +++ b/modules/sdk-core/CHANGELOG.md @@ -3,6 +3,12 @@ All notable changes to this project will be documented in this file. See [Conventional Commits](https://conventionalcommits.org) for commit guidelines. +## Unreleased + +### Bug Fixes + +* Check signing-payload consistency on the ECDSA serialized-transaction verification path, matching MPCv2, before entering the signing round. + # [40.5.0](https://github.com/BitGo/BitGoJS/compare/%40bitgo%2Fsdk-core%4040.4.0...%40bitgo%2Fsdk-core%4040.5.0) (2026-11-03) ### Bug Fixes diff --git a/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts b/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts index cb4ec7f61c7..a3423db3f1d 100644 --- a/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts +++ b/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts @@ -315,6 +315,35 @@ export interface VerificationOptions { consolidationToBaseAddress?: boolean; } +/** + * Decoded vs expected transaction fields reported by fail-closed TSS verification. + * Undefined fields were not applicable to the violated rule. + */ +export interface TssVerificationMismatch { + to?: string; + value?: string; + data?: string; +} + +/** + * A non-fatal TSS verification finding: the transaction bytes about to be signed do not + * match the user intent. Reported through `console.warn` and an optional + * `onVerificationWarning` callback; becomes a hard failure when the client opts into + * `strictTssVerification`. + */ +export interface TssVerificationWarning { + /** Canonical verifier intent type the handler ran under (e.g. 'transfer'). */ + intentType: string; + /** Human-readable description of the violated rule. */ + reason: string; + /** Values derived from the user intent (txParams or the stored tx request intent). */ + expected: TssVerificationMismatch; + /** Values decoded from the transaction bytes that are about to be signed. */ + decoded: TssVerificationMismatch; +} + +export type TssVerificationWarningHandler = (warning: TssVerificationWarning) => void; + export interface VerifyTransactionOptions { txPrebuild: TransactionPrebuild; txParams: TransactionParams; @@ -322,6 +351,26 @@ export interface VerifyTransactionOptions { verification?: VerificationOptions; reqId?: IRequestTracer; walletType?: 'onchain' | 'tss'; + /** + * The persisted TxRequest intent (`PopulatedIntent`), set by the TSS signing path when + * verifying a pending-approval transaction. Verifiers use it as the source of truth for + * the intended `to` / `value` / `data` and for intent fields with no txParams equivalent + * (`tokenName`, `nonce`, `txid`, `consolidateId`, DeFi amounts). Never server-controlled + * beyond the tx request itself, which is the object the user approved. + */ + intent?: PopulatedIntent; + /** + * The `txParams` the caller of the signing API supplied, before `resolveEffectiveTxParams` + * back-filled `type` and `recipients` from the stored tx request intent. `undefined` when the + * caller supplied none (pending-approval signing of a stored tx request). + * + * `txParams` cannot answer "did the caller declare this?": the stored intent has already been + * merged into it. Verifiers that accept a stored `intent` use this to keep the caller's + * declared intent and the stored intent apart: a caller that declared `type` or `recipients` + * is the only source of truth, and the stored intent must not select the handler, add + * calldata, or name the asset. + */ + callerTxParams?: TransactionParams; } export interface SupplementGenerateWalletOptions { diff --git a/modules/sdk-core/src/bitgo/bitgoBase.ts b/modules/sdk-core/src/bitgo/bitgoBase.ts index 698e4846fd4..5cca2f1fb0a 100644 --- a/modules/sdk-core/src/bitgo/bitgoBase.ts +++ b/modules/sdk-core/src/bitgo/bitgoBase.ts @@ -8,7 +8,7 @@ import { IEncryptionSession, IRequestTracer, } from '../api'; -import { IBaseCoin } from './baseCoin'; +import { IBaseCoin, TssVerificationWarningHandler } from './baseCoin'; import { CoinConstructor } from './coinFactory'; import { EnvironmentName } from './environments'; import { EcdhDerivedKeypair, GetSigningKeyApi } from './keychain'; @@ -61,4 +61,14 @@ export interface BitGoBase { setRequestTracer(reqTracer: IRequestTracer): void; url(path: string, version?: number): string; register(name: string, coin: CoinConstructor): void; + /** + * Client-side opt-in: fail-closed TSS verification warnings become hard failures. + * Set only from local `BitGoAPI` options — never read from server data. + */ + readonly strictTssVerification?: boolean; + /** + * Optional sink for fail-closed TSS verification warnings (decoded vs expected + * transaction fields). Set only from local `BitGoAPI` options. + */ + readonly onVerificationWarning?: TssVerificationWarningHandler; } diff --git a/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts b/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts index 0473a7ee397..d0649c6f23e 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts @@ -503,6 +503,8 @@ export interface PopulatedIntent extends PopulatedIntentBase, DefiIntentFields { feeToken?: string; hopParams?: HopParams; txid?: string; + /** Consolidation batch id; presence marks the tx as a consolidation to the wallet base address. */ + consolidateId?: string; receiveAddress?: string; custodianTransactionId?: string; custodianMessageId?: string; diff --git a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts index fece8b88d0c..362db762a26 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts @@ -25,6 +25,7 @@ import { CustomMuDeltaShareGeneratingFunction, CustomPaillierModulusGetterFunction, CustomSShareGeneratingFunction, + PopulatedIntent, RequestType, TSSParams, TSSParamsForMessage, @@ -52,6 +53,7 @@ import { BaseEcdsaUtils } from './base'; import { EncryptionVersion, IRequestTracer } from '../../../../api'; import { resolveEffectiveTxParams } from '../recipientUtils'; import { shouldVerifyWithSerializedTxHex } from '../serializedTxHexVerify'; +import { isCoinWithSignableConsistency } from '../signableConsistency'; const encryptNShare = ECDSAMethods.encryptNShare; @@ -832,16 +834,26 @@ export class EcdsaUtils extends BaseEcdsaUtils { if (shouldVerifyWithSerializedTxHex(this.baseCoin)) { await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.serializedTxHex, txInfo: unsignedTx.signableHex }, - txParams: resolveEffectiveTxParams(txRequest, params.txParams), + txParams: resolveEffectiveTxParams(txRequest, params.txParams, this.baseCoin.getChain()), wallet: this.wallet, walletType: this.wallet.multisigType(), + intent: txRequest.intent as PopulatedIntent, + callerTxParams: params.txParams, }); + // The verifier above inspects serializedTxHex only, but the signing round below signs + // signableHex. Both are server-supplied, so bind them the same way the MPCv2 path does: + // otherwise verified bytes could be paired with a different signing preimage. + if (isCoinWithSignableConsistency(this.baseCoin)) { + this.baseCoin.assertSignableConsistency(unsignedTx.serializedTxHex, unsignedTx.signableHex); + } } else { await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.signableHex }, - txParams: resolveEffectiveTxParams(txRequest, params.txParams), + txParams: resolveEffectiveTxParams(txRequest, params.txParams, this.baseCoin.getChain()), wallet: this.wallet, walletType: this.wallet.multisigType(), + intent: txRequest.intent as PopulatedIntent, + callerTxParams: params.txParams, }); } signablePayload = Buffer.from(unsignedTx.signableHex, 'hex'); diff --git a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts index b942656b947..1064182bd6f 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts @@ -39,6 +39,7 @@ import { CustomMPCv2SigningRound1GeneratingFunction, CustomMPCv2SigningRound2GeneratingFunction, CustomMPCv2SigningRound3GeneratingFunction, + PopulatedIntent, RequestType, SignatureShareRecord, TSSParams, @@ -991,9 +992,11 @@ export class EcdsaMPCv2Utils extends BaseEcdsaUtils { if (shouldVerifyWithSerializedTxHex(this.baseCoin) || shouldUsePreHashedSignable(this.baseCoin, unsignedTx)) { await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.serializedTxHex, txInfo: unsignedTx.signableHex }, - txParams: resolveEffectiveTxParams(txRequest, params.txParams), + txParams: resolveEffectiveTxParams(txRequest, params.txParams, this.baseCoin.getChain()), wallet: this.wallet, walletType: this.wallet.multisigType(), + intent: txRequest.intent as PopulatedIntent, + callerTxParams: params.txParams, }); if (shouldVerifyWithSerializedTxHex(this.baseCoin) && isCoinWithSignableConsistency(this.baseCoin)) { this.baseCoin.assertSignableConsistency(unsignedTx.serializedTxHex, unsignedTx.signableHex); @@ -1001,9 +1004,11 @@ export class EcdsaMPCv2Utils extends BaseEcdsaUtils { } else { await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.signableHex }, - txParams: resolveEffectiveTxParams(txRequest, params.txParams), + txParams: resolveEffectiveTxParams(txRequest, params.txParams, this.baseCoin.getChain()), wallet: this.wallet, walletType: this.wallet.multisigType(), + intent: txRequest.intent as PopulatedIntent, + callerTxParams: params.txParams, }); } txOrMessageToSign = unsignedTx.signableHex; diff --git a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts index f33d44b9b05..f1556a43706 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts @@ -29,10 +29,23 @@ export const NO_RECIPIENT_TX_TYPES = new Set([ 'disableToken', 'disabletoken', 'customTx', - // DeFi vault operations — recipients/calldata built server-side from defiParams + // DeFi vault operations — recipients/calldata built server-side from defiParams. + // Both spellings registered on purpose (see the wrapNative note below): the stored + // WP intentType is kebab-case while buildParams.type is camelCase. This entry only + // waives the missing-recipients guard so a caller-declared (camelCase) leg reaches the + // DeFi calldata checks in verifyTssTransaction, which compare the amount and the + // in-calldata destination with defiParams. A stored (kebab-case) defi-* intent is + // rejected there instead: it carries only vaultId and amount from the server, and no + // local vault registry exists to anchor the target contract or the approve spender. 'defiApprove', + 'defi-approve', 'defiDeposit', + 'defi-deposit', 'defiWithdraw', + 'defi-withdraw', + // defi-redeem is deliberately absent: it has no EVM verifier handler, so a stored + // defi-redeem intent keeps failing the missing-recipients guard instead of reaching + // signing with only a warning. // Native wrap/unwrap (WETH9 deposit()/withdraw()) — calldata and the WETH9 // address are resolved server-side from the vault binding, so no recipients. // Registered in BOTH spellings on purpose: this set is matched against diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts index a72732aeeb3..b262f970e06 100644 --- a/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts @@ -19,6 +19,7 @@ import { EcdsaMPCv2Utils, EcdsaMPCv2KeyGenCallbacks, IBaseCoin, + IRequestTracer, IWallet, RequestTracer, SignatureShareRecord, @@ -26,6 +27,7 @@ import { TxRequest, } from '../../../../../../src'; import { bitgoGpgKey } from './gpgKeys'; +import { readKey } from 'openpgp'; describe('ECDSA MPC v2', async () => { let userShare: Buffer; @@ -636,6 +638,7 @@ describe('ECDSA MPC v2', async () => { verifyTransaction: sinon.stub().resolves(true), getMPCAlgorithm: sinon.stub().returns('ecdsa'), getConfig: sinon.stub().returns({ family: 'flrp' }), + getChain: sinon.stub().returns('flrp'), isSignablePreHashed: (unsignedTx: { serializedTxHex?: string }) => unsignedTx.serializedTxHex?.startsWith('0000') ?? false, } as unknown as IBaseCoin; @@ -707,6 +710,7 @@ describe('ECDSA MPC v2', async () => { verifyTransaction: sinon.stub().resolves(true), getMPCAlgorithm: sinon.stub().returns('ecdsa'), getConfig: sinon.stub().returns({ family: 'flr' }), + getChain: sinon.stub().returns('flr'), } as unknown as IBaseCoin; const mockWallet = { @@ -781,6 +785,7 @@ describe('ECDSA MPC v2', async () => { verifyTransaction: verifyTransactionSpy, getMPCAlgorithm: sinon.stub().returns('ecdsa'), getConfig: sinon.stub().returns({ family: 'hteth' }), + getChain: sinon.stub().returns('hteth'), } as unknown as IBaseCoin; const mockWallet = { @@ -827,7 +832,94 @@ describe('ECDSA MPC v2', async () => { const verifyCallArgs = verifyTransactionSpy.firstCall.args[0]; assert.strictEqual(verifyCallArgs.txParams.type, intentType); assert.strictEqual(verifyCallArgs.txParams.recipients, undefined); + // The stored intent must reach the verifier so fail-closed EVM checks can compare + // decoded to / value / data against it on the pending-approval path. + assert.strictEqual( + verifyCallArgs.intent, + (txRequest as any).intent, + 'stored intent must reach verifyTransaction' + ); + assert.strictEqual( + verifyCallArgs.callerTxParams, + undefined, + 'pending approval supplies no caller txParams; the stored intent is the only source' + ); + }); + }); + + it('signRequestBase should pass the caller txParams and the stored intent to verifyTransaction', async function () { + const serializedTxHex = 'f86c808504a817c80082520894' + '00'.repeat(20) + '80808080'; + const callerTxParams = { + type: 'transfer', + recipients: [{ address: '0x' + '11'.repeat(20), amount: '1000' }], + }; + const intent = { + intentType: 'payment', + recipients: [{ address: { address: '0x' + '11'.repeat(20) }, amount: { value: '1000', symbol: 'hteth' } }], + }; + + const mockBgWithPost = {} as BitGoBase; + mockBgWithPost.getEnv = sinon.stub().returns('test'); + mockBgWithPost.setRequestTracer = sinon.stub(); + mockBgWithPost.encrypt = sinon.stub().resolves('encrypted'); + mockBgWithPost.decrypt = sinon.stub().resolves('decrypted'); + mockBgWithPost.post = sinon.stub().returns({ + send: sinon.stub().returnsThis(), + set: sinon.stub().returnsThis(), + result: sinon.stub().rejects(new Error('mock: HTTP not available')), }); + + const verifyTransactionSpy = sinon.stub().resolves(true); + const mockCoinForDirectSend = { + getHashFunction: sinon.stub().callsFake(() => createKeccakHash('keccak256') as Hash), + verifyTransaction: verifyTransactionSpy, + getMPCAlgorithm: sinon.stub().returns('ecdsa'), + getConfig: sinon.stub().returns({ family: 'hteth' }), + getChain: sinon.stub().returns('hteth'), + } as unknown as IBaseCoin; + + const mockWallet = { + id: sinon.stub().returns(walletID), + multisigType: sinon.stub().returns('tss'), + multisigTypeVersion: sinon.stub().returns('MPCv2'), + }; + + const directUtils = new EcdsaMPCv2Utils(mockBgWithPost, mockCoinForDirectSend, mockWallet as unknown as IWallet); + const bitgoPubKey = await readKey({ armoredKey: bitgoGpgKey.public }); + sinon.stub(directUtils, 'pickBitgoPubGpgKeyForSigning').resolves(bitgoPubKey); + + const txRequest = { + txRequestId: 'direct-send-test', + apiVersion: 'full', + walletId: walletID, + intent, + transactions: [ + { + unsignedTx: { derivationPath: 'm/0', signableHex: serializedTxHex, serializedTxHex }, + signatureShares: [], + }, + ], + } as unknown as TxRequest; + + try { + await directUtils.signTxRequest({ + txRequest, + txParams: callerTxParams, + prv: userShare.toString('base64'), + reqId: { inc: sinon.stub(), toString: sinon.stub().returns('test-req') } as unknown as IRequestTracer, + }); + } catch (e) {} + + assert.strictEqual(verifyTransactionSpy.callCount, 1, 'verifyTransaction must be reached exactly once'); + const verifyCallArgs = verifyTransactionSpy.firstCall.args[0]; + assert.strictEqual(verifyCallArgs.intent, intent, 'stored intent must reach verifyTransaction'); + assert.deepStrictEqual( + verifyCallArgs.callerTxParams, + callerTxParams, + 'the untouched caller input must reach the verifier' + ); + assert.strictEqual(verifyCallArgs.txParams.type, 'transfer'); + assert.deepStrictEqual(verifyCallArgs.txParams.recipients, callerTxParams.recipients); }); it('should still apply keccak256 for regular FLR EVM transactions', async () => { diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaVerifyTransactionWiring.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaVerifyTransactionWiring.ts new file mode 100644 index 00000000000..2156e3e808c --- /dev/null +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaVerifyTransactionWiring.ts @@ -0,0 +1,132 @@ +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import { + BitGoBase, + EcdsaUtils, + IBaseCoin, + IRequestTracer, + IWallet, + TssSignTxRequestParamsWithPrv, + TxRequest, +} from '../../../../../../src'; + +/** + * CECHO-2460: the EVM TSS verifier must receive both the caller's own txParams and the stored + * tx request intent. `txParams` is the post-`resolveEffectiveTxParams` object (the stored intent + * is back-filled into it), so it cannot tell the verifier whether the caller declared anything; + * `callerTxParams` is the untouched caller input that decides which source is authoritative. + * + * This pins the ECDSA (MPCv1) wiring. EcdsaMPCv2Utils has the equivalent test in ecdsaMPCv2.ts. + */ +describe('EcdsaUtils signing verifyTransaction wiring', function () { + const walletID = '62fe536a6b4cf70007acb48c0e7bb0b0'; + const derivationPath = 'm/0'; + const signableHex = 'f86c808504a817c80082520894' + '00'.repeat(20) + '80808080'; + + const makeTxRequest = (intent: Record): TxRequest => + ({ + txRequestId: 'ecdsa-wiring', + apiVersion: 'full', + walletId: walletID, + intent, + transactions: [ + { + unsignedTx: { derivationPath, signableHex, serializedTxHex: signableHex }, + signatureShares: [], + }, + ], + } as unknown as TxRequest); + + // Minimal signing material: signRequestBase only needs pShare.i === 1, a backupNShare and + // the paillier modulus before it hands the unsigned bytes to verifyTransaction. + const prv = JSON.stringify({ pShare: { i: 1, n: 'paillier-modulus' }, backupNShare: {} }); + + let verifyTransactionStub: sinon.SinonStub; + + const signWith = async (txParams: TssSignTxRequestParamsWithPrv['txParams'], intent: Record) => { + const mockBitgo = { + getEnv: sinon.stub().returns('test'), + setRequestTracer: sinon.stub(), + } as unknown as BitGoBase; + const mockCoin = { + verifyTransaction: verifyTransactionStub, + getConfig: sinon.stub().returns({}), + getChain: sinon.stub().returns('hteth'), + } as unknown as IBaseCoin; + const mockWallet = { + id: sinon.stub().returns(walletID), + multisigType: sinon.stub().returns('tss'), + coinSpecific: sinon.stub().returns(undefined), + }; + + const utils = new EcdsaUtils(mockBitgo, mockCoin, mockWallet as unknown as IWallet); + // Stop right after the verifier: the TSS networking that follows is covered elsewhere. + sinon.stub(utils, 'getEcdsaSigningChallenges').rejects(new Error('stop-after-verify')); + + try { + await utils.signTxRequest({ + txRequest: makeTxRequest(intent), + txParams, + prv, + reqId: { inc: sinon.stub(), toString: sinon.stub().returns('test-req') } as unknown as IRequestTracer, + }); + } catch (e) { + assert.match((e as Error).message, /stop-after-verify/); + } + + assert.strictEqual(verifyTransactionStub.callCount, 1, 'verifyTransaction must be reached exactly once'); + return verifyTransactionStub.firstCall.args[0] as { + txParams: { type?: string; recipients?: unknown[] }; + intent?: Record; + callerTxParams?: unknown; + }; + }; + + beforeEach(function () { + verifyTransactionStub = sinon.stub().resolves(true); + }); + + afterEach(function () { + sinon.restore(); + }); + + it('passes the caller txParams and the stored intent to the verifier on a direct send', async function () { + const callerTxParams = { + type: 'transfer', + recipients: [{ address: '0x' + '11'.repeat(20), amount: '1000' }], + }; + const intent = { + intentType: 'payment', + recipients: [{ address: { address: '0x' + '11'.repeat(20) }, amount: { value: '1000', symbol: 'hteth' } }], + }; + + const verifyArgs = await signWith(callerTxParams, intent); + + assert.strictEqual(verifyArgs.intent, intent, 'stored intent must reach verifyTransaction'); + assert.deepStrictEqual( + verifyArgs.callerTxParams, + callerTxParams, + 'the untouched caller input must reach the verifier' + ); + assert.strictEqual(verifyArgs.txParams.type, 'transfer'); + assert.deepStrictEqual(verifyArgs.txParams.recipients, callerTxParams.recipients); + }); + + it('passes callerTxParams as undefined on the pending-approval path', async function () { + const intent = { + intentType: 'payment', + recipients: [{ address: { address: '0x' + '22'.repeat(20) }, amount: { value: '1000', symbol: 'hteth' } }], + }; + + const verifyArgs = await signWith(undefined, intent); + + assert.strictEqual(verifyArgs.intent, intent, 'stored intent must reach verifyTransaction'); + assert.strictEqual( + verifyArgs.callerTxParams, + undefined, + 'the pending-approval path supplies no caller params; the stored intent speaks' + ); + assert.strictEqual(verifyArgs.txParams.type, 'payment'); + assert.strictEqual(verifyArgs.txParams.recipients?.length, 1, 'stored recipients are back-filled'); + }); +}); diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts index bb6ffbf5680..7c3a804ecba 100644 --- a/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts @@ -29,8 +29,11 @@ describe('recipientUtils', function () { 'disabletoken', 'customTx', 'defiApprove', + 'defi-approve', 'defiDeposit', + 'defi-deposit', 'defiWithdraw', + 'defi-withdraw', // Native wrap/unwrap — registered in both spellings on purpose 'wrapNative', 'wrap-native', @@ -82,10 +85,31 @@ describe('recipientUtils', function () { }); it('does not contain value-transfer types', function () { - ['payment', 'fanout', 'vote', 'defi-deposit', 'defi-redeem'].forEach((t) => { + ['payment', 'fanout', 'vote'].forEach((t) => { assert.ok(!NO_RECIPIENT_TX_TYPES.has(t), `${t} must NOT be in NO_RECIPIENT_TX_TYPES`); }); }); + + it('accepts kebab-case stored DeFi intent types without recipients', function () { + // Stored defi-* intents have no recipients; the guard must not reject them here, so they + // reach verifyTssTransaction, which rejects a stored DeFi intent explicitly (no local + // vault binding to anchor the target contract or spender) instead of silently passing it. + for (const intentType of ['defi-approve', 'defi-deposit', 'defi-withdraw']) { + const txRequest = makeTxRequest({ intent: { intentType } as PopulatedIntent }); + assert.doesNotThrow( + () => resolveEffectiveTxParams(txRequest, undefined), + `${intentType} should not throw without recipients` + ); + } + }); + + it('keeps rejecting stored defi-redeem intents, which have no EVM verifier handler', function () { + for (const intentType of ['defi-redeem', 'defiRedeem']) { + assert.ok(!NO_RECIPIENT_TX_TYPES.has(intentType), `${intentType} must NOT be in NO_RECIPIENT_TX_TYPES`); + const txRequest = makeTxRequest({ intent: { intentType } as PopulatedIntent }); + assert.throws(() => resolveEffectiveTxParams(txRequest, undefined), InvalidTransactionError); + } + }); }); describe('resolveEffectiveTxParams', function () { diff --git a/modules/statics/CHANGELOG.md b/modules/statics/CHANGELOG.md index ca530236e44..8de0bb5508c 100644 --- a/modules/statics/CHANGELOG.md +++ b/modules/statics/CHANGELOG.md @@ -3,6 +3,12 @@ All notable changes to this project will be documented in this file. See [Conventional Commits](https://conventionalcommits.org) for commit guidelines. +## Unreleased + +### Bug Fixes + +* Verify Kava EVM and Prividium TSS transactions using serialized bytes rather than legacy EIP-155 signing preimages, on mainnet and testnet. + # [59.28.0](https://github.com/BitGo/BitGoJS/compare/%40bitgo%2Fstatics%4059.27.0...%40bitgo%2Fstatics%4059.28.0) (2026-11-03) ### Bug Fixes diff --git a/modules/statics/src/allCoinsAndTokens.ts b/modules/statics/src/allCoinsAndTokens.ts index 54c495c9f14..f49e9856ba3 100644 --- a/modules/statics/src/allCoinsAndTokens.ts +++ b/modules/statics/src/allCoinsAndTokens.ts @@ -2170,6 +2170,7 @@ export const allCoinsAndTokens = [ CoinFeature.EVM_COMPATIBLE_WP, CoinFeature.EVM_NON_BITGO_RECOVERY, CoinFeature.EVM_UNSIGNED_SWEEP_RECOVERY, + CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX, ] ), account( @@ -2189,6 +2190,7 @@ export const allCoinsAndTokens = [ CoinFeature.EVM_COMPATIBLE_WP, CoinFeature.EVM_NON_BITGO_RECOVERY, CoinFeature.EVM_UNSIGNED_SWEEP_RECOVERY, + CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX, ] ), account( @@ -2878,6 +2880,7 @@ export const allCoinsAndTokens = [ CoinFeature.EVM_COMPATIBLE_UI, CoinFeature.EVM_COMPATIBLE_WP, CoinFeature.SUPPORTS_ERC20, + CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX, ] ), account( @@ -2895,6 +2898,7 @@ export const allCoinsAndTokens = [ CoinFeature.EVM_COMPATIBLE_IMS, CoinFeature.EVM_COMPATIBLE_UI, CoinFeature.EVM_COMPATIBLE_WP, + CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX, ] ), diff --git a/modules/statics/test/unit/coins.ts b/modules/statics/test/unit/coins.ts index 969ff514bb1..17b75429912 100644 --- a/modules/statics/test/unit/coins.ts +++ b/modules/statics/test/unit/coins.ts @@ -1439,14 +1439,6 @@ describe('ERC20 Bulk Transaction Feature', () => { }); describe('TSS Verify Use Serialized Tx Hex Feature', () => { - it('should have TSS_VERIFY_USE_SERIALIZED_TX_HEX for coins whose signableHex is not parseable', () => { - const coinsNeedingSerializedTxHexVerify = ['bsc', 'tbsc', 'xdc', 'txdc', 'icp', 'ticp']; - coinsNeedingSerializedTxHexVerify.forEach((coinName) => { - const coin = coins.get(coinName); - coin.features.includes(CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX).should.eql(true); - }); - }); - it('should have TSS_VERIFY_USE_SERIALIZED_TX_HEX on BSC and XDC tokens', () => { coins.get('bsc:busd').features.includes(CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX).should.eql(true); coins.get('xdc:usdc').features.includes(CoinFeature.TSS_VERIFY_USE_SERIALIZED_TX_HEX).should.eql(true);