Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions modules/sdk-api/src/bitgoAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -271,10 +272,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(
Expand Down
18 changes: 17 additions & 1 deletion modules/sdk-api/src/types.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { EncryptionVersion, EnvironmentName, IRequestTracer, V1Network } from '@bitgo/sdk-core';
import {
EncryptionVersion,
EnvironmentName,
IRequestTracer,
TssVerificationWarningHandler,
V1Network,
} from '@bitgo/sdk-core';
import { ECPairInterface } from '@bitgo/utxo-lib';
import { type Agent } from 'http';

Expand Down Expand Up @@ -70,6 +76,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;
Expand Down
37 changes: 37 additions & 0 deletions modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -315,13 +315,50 @@ 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;
wallet: IWallet;
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;
}

export interface SupplementGenerateWalletOptions {
Expand Down
12 changes: 11 additions & 1 deletion modules/sdk-core/src/bitgo/bitgoBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
}
2 changes: 2 additions & 0 deletions modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 5 additions & 2 deletions modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
CustomMuDeltaShareGeneratingFunction,
CustomPaillierModulusGetterFunction,
CustomSShareGeneratingFunction,
PopulatedIntent,
RequestType,
TSSParams,
TSSParamsForMessage,
Expand Down Expand Up @@ -832,16 +833,18 @@ 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,
});
} 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,
});
}
signablePayload = Buffer.from(unsignedTx.signableHex, 'hex');
Expand Down
7 changes: 5 additions & 2 deletions modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
CustomMPCv2SigningRound1GeneratingFunction,
CustomMPCv2SigningRound2GeneratingFunction,
CustomMPCv2SigningRound3GeneratingFunction,
PopulatedIntent,
RequestType,
SignatureShareRecord,
TSSParams,
Expand Down Expand Up @@ -991,19 +992,21 @@ 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,
});
if (shouldVerifyWithSerializedTxHex(this.baseCoin) && 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,
});
}
txOrMessageToSign = unsignedTx.signableHex;
Expand Down
14 changes: 13 additions & 1 deletion modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,22 @@ export const NO_RECIPIENT_TX_TYPES = new Set([
'disableToken',
'disabletoken',
'customTx',
// DeFi vault operations — recipients/calldata built server-side from defiParams
// DeFi vault operations — recipients/calldata built server-side from defiParams.
// Both spellings registered on purpose (see the wrapNative note below): the stored
// WP intentType is kebab-case while buildParams.type is camelCase. These types are
// NOT skipped by verifyTssTransaction — the DeFi handlers decode and compare the
// calldata against defiParams; this entry only waives the missing-recipients guard.
'defiApprove',
'defi-approve',
'defiDeposit',
'defi-deposit',
'defiWithdraw',
'defi-withdraw',
// defi-redeem has no EVM verifier handler and is flagged as a violation there;
// listed here so the pending-approval path reaches the verifier instead of
// failing earlier on the missing-recipients guard.
'defi-redeem',
'defiRedeem',
Comment on lines 37 to +47

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Security: with only this PR merged, stored DeFi intents with no txParams get signed with no calldata check at all, where the base branch rejected them. The new defi-approve/defi-deposit/defi-withdraw/defi-redeem/defiRedeem entries pass both no-recipient guards. The EVM DeFi checks in abstractEthLikeNewCoins.ts:3659 only match camelCase defiApprove/defiDeposit/defiWithdraw. Fix: keep these spellings out of NO_RECIPIENT_TX_TYPES until the verifier rejects or checks every one of them, or ship that verifier change in this PR. The comments at recipientUtils.ts:35 and :43 describe checks that do not exist on master. [also at: modules/sdk-coin-evm/src/evmCoin.ts:113 - Security: TSS wallets on non-EIP-1559 EVM chains would sign pending-approval DeFi transactions without any calldata check.]

Why this was flagged

The trigger is the pending-approval path: pendingApproval.approve() -> recreateTxRequest() -> signTxRequest() with txParams undefined, on a stored intent whose intentType is 'defi-deposit' (the spelling wallet.ts:5004 and defiVault.ts:144 write). resolveEffectiveTxParams (recipientUtils.ts:170-175) sets txParams.type = 'defi-deposit'. The guard at recipientUtils.ts:216 no longer throws. In verifyTssTransaction, the guard at abstractEthLikeNewCoins.ts:3583 (and evmCoin.ts:113) passes because NO_RECIPIENT_TX_TYPES now has the kebab spelling. The DeFi block at abstractEthLikeNewCoins.ts:3659 only checks ['defiApprove','defiDeposit','defiWithdraw'], so it is skipped, and the key share signs whatever to/value/data the server built. 'defi-redeem' and 'defiRedeem' have no handler at all, so the comment's claim that they are "flagged as a violation there" is false on master. On the base branch, resolveEffectiveTxParams threw InvalidTransactionError before signing. The PR says the verifier lands in a separate part 2 (#9973), but nothing in this change enforces that the two ship together.

Verification: On the base, the guard at recipientUtils.ts:214-222 threw "Recipient details are required..." because the kebab spellings were not in the set. This diff adds them, so it no longer throws. The DeFi calldata block at abstractEthLikeNewCoins.ts:3659 only matches ['defiApprove', 'defiDeposit', 'defiWithdraw']. With this PR they are signed with no check of the server-built calldata.

// 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
Expand Down
10 changes: 10 additions & 0 deletions modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,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;
Expand Down Expand Up @@ -707,6 +708,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 = {
Expand Down Expand Up @@ -781,6 +783,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 = {
Expand Down Expand Up @@ -827,6 +830,13 @@ 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,
Comment on lines +835 to +837

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit (optional): the new assertion adds an avoidable any cast, (txRequest as any).intent, which goes against the CLAUDE.md rule to avoid any. Fix: use txRequest.intent, since txRequest is already typed as TxRequest and has an intent field.

Why this was flagged

There is no runtime impact. At ecdsaMPCv2.ts:811, txRequest is declared as unknown as TxRequest, and TxRequest declares intent. That makes the as any in the new assertion unnecessary, and it goes against the root CLAUDE.md TypeScript rule to avoid the any type.

Verification: Severity: nit. There is no runtime impact. The diff adds (txRequest as any).intent in the new assertion at modules/sdk-core/test/unit/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts:837. TxRequest in modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts:621 declares intent: unknown; at line 630. Since assert.strictEqual takes unknown, writing txRequest.intent type-checks without the cast. It goes against the root CLAUDE.md line 88.

'stored intent must reach verifyTransaction'
);
});
});

Expand Down
22 changes: 21 additions & 1 deletion modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,15 @@ describe('recipientUtils', function () {
'disabletoken',
'customTx',
'defiApprove',
'defi-approve',
'defiDeposit',
'defi-deposit',
'defiWithdraw',
'defi-withdraw',
// defi-redeem has no EVM verifier handler and is flagged there; listed so the
// pending-approval path reaches the verifier instead of failing the guard first
'defiRedeem',
'defi-redeem',
// Native wrap/unwrap — registered in both spellings on purpose
'wrapNative',
'wrap-native',
Expand Down Expand Up @@ -82,10 +89,23 @@ describe('recipientUtils', function () {
});

it('does not contain value-transfer types', function () {
['payment', 'fanout', 'vote', 'defi-deposit', 'defi-redeem'].forEach((t) => {
['payment', 'fanout', 'vote'].forEach((t) => {
assert.ok(!NO_RECIPIENT_TX_TYPES.has(t), `${t} must NOT be in NO_RECIPIENT_TX_TYPES`);
});
});

it('accepts kebab-case stored DeFi intent types without recipients', function () {
// Stored defi-* intents have no recipients; they must reach verifyTssTransaction's DeFi
// calldata checks (no InvalidTransactionError, no silent skip) instead of being rejected
// by the missing-recipients guard.
for (const intentType of ['defi-approve', 'defi-deposit', 'defi-withdraw', 'defi-redeem']) {
const txRequest = makeTxRequest({ intent: { intentType } as PopulatedIntent });
assert.doesNotThrow(
() => resolveEffectiveTxParams(txRequest, undefined),
`${intentType} should not throw without recipients`
);
}
});
});

describe('resolveEffectiveTxParams', function () {
Expand Down
Loading