diff --git a/modules/bitgo/test/v2/unit/internal/tssUtils/eddsaMPCv2/signTxRequest.ts b/modules/bitgo/test/v2/unit/internal/tssUtils/eddsaMPCv2/signTxRequest.ts index e5403c3f4d..1e6d03300c 100644 --- a/modules/bitgo/test/v2/unit/internal/tssUtils/eddsaMPCv2/signTxRequest.ts +++ b/modules/bitgo/test/v2/unit/internal/tssUtils/eddsaMPCv2/signTxRequest.ts @@ -1,9 +1,11 @@ +import * as sinon from 'sinon'; import { BaseCoin, BitgoGPGPublicKey, common, ECDSAUtils, EDDSAUtils, + InvalidTransactionError, RequestTracer, RequestType, SignatureShareRecord, @@ -419,6 +421,133 @@ describe('signTxRequest:', function () { nockPromises[3].isDone().should.be.false(); }); + describe('resolveEffectiveTxParams guard (WCI-1111)', function () { + let sandbox: sinon.SinonSandbox; + + beforeEach(function () { + sandbox = sinon.createSandbox(); + }); + + afterEach(function () { + sandbox.restore(); + }); + + it('throws InvalidTransactionError when txParams is absent and intent has no recipients (malicious/empty-recipient path)', async function () { + // Simulate the stakingAuthorize attack vector: intent has no recipients + // and intentType is not on the NO_RECIPIENT_TX_TYPES allowlist. + const maliciousTxRequest: TxRequest = { + ...txRequest, + intent: { intentType: 'stakingAuthorize' } as any, + }; + const userPrvBase64 = Buffer.from(userKeyShare).toString('base64'); + await tssUtils + .signTxRequest({ + txRequest: maliciousTxRequest, + prv: userPrvBase64, + reqId, + // No txParams — the re-sign path that was previously vulnerable + }) + .should.be.rejectedWith(InvalidTransactionError); + }); + + it('throws InvalidTransactionError when txParams has empty recipients and intentType is not allowlisted', async function () { + const maliciousTxRequest: TxRequest = { + ...txRequest, + intent: { intentType: 'payment' } as any, + }; + const userPrvBase64 = Buffer.from(userKeyShare).toString('base64'); + await tssUtils + .signTxRequest({ + txRequest: maliciousTxRequest, + prv: userPrvBase64, + reqId, + txParams: { recipients: [] }, + }) + .should.be.rejectedWith(InvalidTransactionError); + }); + + it('does not throw for allowlisted no-recipient intentType (deactivate)', async function () { + sandbox.stub(baseCoin, 'verifyTransaction').resolves(true); + const nockPromises = await getNockPromisesForEddsaSigning(txRequest); + await Promise.all(nockPromises); + + const noRecipientTxRequest: TxRequest = { + ...txRequest, + intent: { intentType: 'deactivate' } as any, + }; + const userPrvBase64 = Buffer.from(userKeyShare).toString('base64'); + await tssUtils.signTxRequest({ + txRequest: noRecipientTxRequest, + prv: userPrvBase64, + reqId, + // No txParams — legitimate no-recipient flow + }); + }); + + it('does not throw for allowlisted no-recipient intentType (consolidate)', async function () { + sandbox.stub(baseCoin, 'verifyTransaction').resolves(true); + const nockPromises = await getNockPromisesForEddsaSigning(txRequest); + await Promise.all(nockPromises); + + const consolidateTxRequest: TxRequest = { + ...txRequest, + intent: { intentType: 'consolidate' } as any, + }; + const userPrvBase64 = Buffer.from(userKeyShare).toString('base64'); + await tssUtils.signTxRequest({ + txRequest: consolidateTxRequest, + prv: userPrvBase64, + reqId, + }); + }); + + it('uses intent recipients when txParams is absent and intent has recipients', async function () { + sandbox.stub(baseCoin, 'verifyTransaction').resolves(true); + const nockPromises = await getNockPromisesForEddsaSigning(txRequest); + await Promise.all(nockPromises); + + const intentRecipientTxRequest: TxRequest = { + ...txRequest, + intent: { + intentType: 'payment', + recipients: [ + { + address: { address: 'HMEgbR4S2hLKfst2VZUVpHVUu4FioFPyW5iUuJvZdMvs' }, + amount: { value: '999990000', symbol: 'sol' }, + }, + ], + } as any, + }; + const userPrvBase64 = Buffer.from(userKeyShare).toString('base64'); + // Should not throw — intent provides the recipients + await tssUtils.signTxRequest({ + txRequest: intentRecipientTxRequest, + prv: userPrvBase64, + reqId, + }); + }); + + it('does not throw for staking intent with stakingRequestId (generic staking signal)', async function () { + sandbox.stub(baseCoin, 'verifyTransaction').resolves(true); + const nockPromises = await getNockPromisesForEddsaSigning(txRequest); + await Promise.all(nockPromises); + + const stakingTxRequest: TxRequest = { + ...txRequest, + intent: { + intentType: 'delegate', + stakingRequestId: 'staking-req-id-123', + } as any, + }; + const userPrvBase64 = Buffer.from(userKeyShare).toString('base64'); + await tssUtils.signTxRequest({ + txRequest: stakingTxRequest, + prv: userPrvBase64, + reqId, + }); + }); + }); + async function getNockPromisesForEddsaSigning( txRequest: TxRequest, requestType: RequestType = RequestType.tx, diff --git a/modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts b/modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts index 1d830a3ede..d4ee0a9c4f 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts @@ -1,7 +1,7 @@ import { EncryptionVersion, IRequestTracer } from '../../../api'; import * as openpgp from 'openpgp'; import { Key, readKey, SerializedKeyPair } from 'openpgp'; -import { IBaseCoin, KeychainsTriplet, TransactionParams } from '../../baseCoin'; +import { IBaseCoin, KeychainsTriplet } from '../../baseCoin'; import { BitGoBase } from '../../bitgoBase'; import { Keychain, KeyIndices, WebauthnKeyEncryptionInfo } from '../../keychain'; import { getTxRequest } from '../../tss'; @@ -31,7 +31,6 @@ import { IntentOptionsForMessage, IntentOptionsForTypedData, ITssUtils, - PopulatedIntent, PopulatedIntentForMessageSigning, PopulatedIntentForTypedDataSigning, PrebuildTransactionWithIntentOptions, @@ -50,28 +49,6 @@ import { getBitgoGpgPubKey } from '../opengpgUtils'; import assert from 'assert'; import { MessageStandardType } from '../messageTypes'; -/** - * Derives txParams from the persisted intent on a TxRequest for EdDSA MPCv2 signing paths - * where no SDK-local buildParams is available (PA path and UI re-sign path). - * Native coin transfers (where symbol equals the chain name) are excluded from tokenName. - */ -export function txParamsFromIntent(intent: unknown, chainName: string): TransactionParams | undefined { - if (typeof intent !== 'object' || intent === null || !('recipients' in intent)) { - return undefined; - } - const { recipients } = intent as PopulatedIntent; - if (!recipients?.length) { - return undefined; - } - return { - recipients: recipients.map((r) => ({ - address: r.address.address, - amount: String(r.amount.value), - ...(r.amount.symbol && r.amount.symbol !== chainName && { tokenName: r.amount.symbol }), - })), - }; -} - /** * BaseTssUtil class which different signature schemes have to extend */ @@ -604,14 +581,7 @@ export default class BaseTssUtils extends MpcUtils implements ITssUtil await this.deleteSignatureShares(txRequestId, reqId); // after delete signatures shares get the tx without them const txRequest = await this.getTxRequest(txRequestId, reqId); - // EdDSA MPCv2 re-verifies the transaction against txParams.recipients before DSG starts. - // On the PA path there is no SDK-local buildParams, so derive txParams from the persisted - // intent. Other TSS variants either skip recipient verification or already work without txParams. - const txParams = - this.wallet.multisigTypeVersion() === 'MPCv2' && this.baseCoin.getMPCAlgorithm() === 'eddsa' - ? txParamsFromIntent(txRequest.intent, this.baseCoin.getChain()) - : undefined; - return await this.signTxRequest({ txRequest, prv: decryptedPrv, reqId, txParams }); + return await this.signTxRequest({ txRequest, prv: decryptedPrv, reqId }); } /** diff --git a/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts b/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts index 311262515c..9613906817 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts @@ -47,6 +47,7 @@ import { import { EncryptionVersion } from '../../../../api'; import { BitGoBase } from '../../../bitgoBase'; import { BaseEddsaUtils } from './base'; +import { resolveEffectiveTxParams } from '../recipientUtils'; import { EddsaMPCv2KeyGenSendFn, KeyGenSenderForEnterprise } from './eddsaMPCv2KeyGenSender'; import { EddsaMPCv2RecoveryKeyShares } from './types'; @@ -553,7 +554,7 @@ export class EddsaMPCv2Utils extends BaseEddsaUtils { bufferContent = Buffer.from(txOrMessageToSign, 'hex'); await this.baseCoin.verifyTransaction({ txPrebuild: { txHex: unsignedTx.serializedTxHex ?? txOrMessageToSign }, - txParams: params.txParams || { recipients: [] }, + txParams: resolveEffectiveTxParams(txRequest, params.txParams, this.baseCoin.getChain()), wallet: this.wallet, walletType: this.wallet.multisigType(), }); diff --git a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts index 244eedd422..d7116dea0d 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts @@ -69,6 +69,9 @@ export const NO_RECIPIENT_TX_TYPES = new Set([ // SOL: deactivate stake account — no on-chain transfer recipient. 'deactivate', + + // SOL: authorize stake account — no transfer recipient. + 'authorize', ]); /** diff --git a/modules/sdk-core/src/bitgo/wallet/wallet.ts b/modules/sdk-core/src/bitgo/wallet/wallet.ts index 6c5babf9f4..23931e6821 100644 --- a/modules/sdk-core/src/bitgo/wallet/wallet.ts +++ b/modules/sdk-core/src/bitgo/wallet/wallet.ts @@ -54,7 +54,6 @@ import { } from '../utils'; import { decodeWithCodec } from '../utils/codecs'; import { postWithCodec } from '../utils/postWithCodec'; -import { txParamsFromIntent } from '../utils/tss/baseTSSUtils'; import { EcdsaMPCv2Utils, EcdsaUtils } from '../utils/tss/ecdsa'; import EddsaUtils, { EddsaMPCv2Utils } from '../utils/tss/eddsa'; import { getTxRequestApiVersion, validateTxRequestApiVersion } from '../utils/txRequest'; @@ -5056,21 +5055,8 @@ export class Wallet implements IWallet { throw new Error('prv required to sign transactions with TSS'); } - let txRequest: string | TxRequest = params.txPrebuild.txRequestId; - let txParams: TransactionParams | undefined = params.txPrebuild.buildParams; - - // EdDSA MPCv2 re-sign path: buildParams is absent when the UI calls signAndSendTxRequest with - // only txRequestId. Derive txParams from the persisted intent so verifyTransaction receives - // the correct recipients before DSG starts. Other TSS variants are unaffected by the guard. - if (!txParams && this.multisigTypeVersion() === 'MPCv2' && this.baseCoin.getMPCAlgorithm() === 'eddsa') { - txRequest = await getTxRequest( - this.bitgo, - this.id(), - params.txPrebuild.txRequestId, - params.reqId || new RequestTracer() - ); - txParams = txParamsFromIntent(txRequest.intent, this.baseCoin.getChain()); - } + const txRequest: string | TxRequest = params.txPrebuild.txRequestId; + const txParams: TransactionParams | undefined = params.txPrebuild.buildParams; try { return await this.tssUtils!.signTxRequest({ diff --git a/modules/sdk-core/test/unit/bitgo/utils/tss/baseTSSUtils.ts b/modules/sdk-core/test/unit/bitgo/utils/tss/baseTSSUtils.ts index 020af23025..ac33e733fa 100644 --- a/modules/sdk-core/test/unit/bitgo/utils/tss/baseTSSUtils.ts +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/baseTSSUtils.ts @@ -338,7 +338,7 @@ describe('Base TSS Utils', function () { return coin; } - it('derives txParams from intent for EdDSA MPCv2 wallets', async function () { + it('passes undefined txParams to signTxRequest', async function () { const txRequestId = 'tx-req-id-1'; const reqId = new RequestTracer(); const txRequest = buildTxRequest({ @@ -356,69 +356,6 @@ describe('Base TSS Utils', function () { await utils.recreateTxRequest(txRequestId, 'prv', reqId); - // Native SOL: symbol equals the chain name, so tokenName must be omitted - assert.deepStrictEqual(signTxRequestStub.firstCall.args[0].txParams, { - recipients: [{ address: 'solAddr1', amount: '5000000' }], - }); - }); - - it('sets tokenName for SPL tokens (symbol differs from chain name)', async function () { - const txRequestId = 'tx-req-id-spl'; - const reqId = new RequestTracer(); - const txRequest = buildTxRequest({ - txRequestId, - intent: { - intentType: 'payment', - recipients: [{ address: { address: 'splAddr1' }, amount: { value: '1000', symbol: 'tsol:usdc' } }], - }, - }); - - const utils = new TestBaseTssUtils(mockBitgo, makeCoin('eddsa', 'tsol'), makeWallet('MPCv2')); - sinon.stub(utils, 'deleteSignatureShares').resolves(); - sinon.stub(utils, 'getTxRequest').resolves(txRequest); - const signTxRequestStub = sinon.stub(utils, 'signTxRequest').resolves(txRequest); - - await utils.recreateTxRequest(txRequestId, 'prv', reqId); - - // SPL token: symbol differs from chain name, so tokenName must be set - assert.deepStrictEqual(signTxRequestStub.firstCall.args[0].txParams, { - recipients: [{ address: 'splAddr1', amount: '1000', tokenName: 'tsol:usdc' }], - }); - }); - - it('passes undefined txParams for EdDSA MPCv2 when intent has no recipients', async function () { - const txRequestId = 'tx-req-id-2'; - const reqId = new RequestTracer(); - const txRequest = buildTxRequest({ txRequestId, intent: { intentType: 'enableToken' } }); - - const utils = new TestBaseTssUtils(mockBitgo, makeCoin('eddsa'), makeWallet('MPCv2')); - sinon.stub(utils, 'deleteSignatureShares').resolves(); - sinon.stub(utils, 'getTxRequest').resolves(txRequest); - const signTxRequestStub = sinon.stub(utils, 'signTxRequest').resolves(txRequest); - - await utils.recreateTxRequest(txRequestId, 'prv', reqId); - - assert.strictEqual(signTxRequestStub.firstCall.args[0].txParams, undefined); - }); - - it('passes undefined txParams for ECDSA MPCv2 wallets (guard does not apply)', async function () { - const txRequestId = 'tx-req-id-3'; - const reqId = new RequestTracer(); - const txRequest = buildTxRequest({ - txRequestId, - intent: { - intentType: 'payment', - recipients: [{ address: { address: 'ethAddr1' }, amount: { value: '1000000', symbol: 'eth' } }], - }, - }); - - const utils = new TestBaseTssUtils(mockBitgo, makeCoin('ecdsa'), makeWallet('MPCv2')); - sinon.stub(utils, 'deleteSignatureShares').resolves(); - sinon.stub(utils, 'getTxRequest').resolves(txRequest); - const signTxRequestStub = sinon.stub(utils, 'signTxRequest').resolves(txRequest); - - await utils.recreateTxRequest(txRequestId, 'prv', reqId); - assert.strictEqual(signTxRequestStub.firstCall.args[0].txParams, undefined); }); }); 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 dd76899cf4..72c452681f 100644 --- a/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts @@ -55,6 +55,8 @@ describe('recipientUtils', function () { 'importtoc', // SOL: deactivate stake account (solDeactivateIntent) 'deactivate', + // SOL: authorize stake account (solAuthorizeIntent) + 'authorize', ]; expected.forEach((t) => assert.ok(NO_RECIPIENT_TX_TYPES.has(t), `${t} should be in NO_RECIPIENT_TX_TYPES`)); assert.strictEqual(NO_RECIPIENT_TX_TYPES.size, expected.length); @@ -360,6 +362,11 @@ describe('recipientUtils', function () { assert.doesNotThrow(() => resolveEffectiveTxParams(txRequest, {})); }); + it('does not throw for "authorize" (solAuthorizeIntent)', function () { + const txRequest = makeTxRequest({ intent: { intentType: 'authorize' } as any }); + assert.doesNotThrow(() => resolveEffectiveTxParams(txRequest, {})); + }); + it('throws for stakingAuthorize — must be validated at coin layer', function () { const txRequest = makeTxRequest({ intent: { intentType: 'stakingAuthorize' } as any }); assert.throws(() => resolveEffectiveTxParams(txRequest, {}), InvalidTransactionError);