Skip to content

Fix/wcn 196 eddsa recipient guard #8630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
mrdanish26 wants to merge 1 commit into
base: master
Choose a base branch
from
+165 −14
Draft

Fix/wcn 196 eddsa recipient guard #8630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 50 additions & 6 deletions modules/bitgo/test/v2/unit/internal/tssUtils/eddsa.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,9 +612,11 @@ describe('TSS Utils:', async function () {
});

it('signTxRequest should succeed with txRequest object as input', async function () {
sandbox.stub(baseCoin, 'verifyTransaction').resolves();
const signedTxRequest = await tssUtils.signTxRequest({
txRequest,
prv: JSON.stringify(validUserSigningMaterial),
txParams: { recipients: [{ address: '5f8f5a1d7f', amount: '10000' }] },
reqId,
});
signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs);
Expand All @@ -623,19 +625,57 @@ describe('TSS Utils:', async function () {
});

it('signTxRequest should succeed with txRequest id as input', async function () {
const getTxRequest = sandbox.stub(tssUtils, 'getTxRequest');
getTxRequest.resolves(txRequest);
getTxRequest.calledWith(txRequestId);
sandbox.stub(baseCoin, 'verifyTransaction').resolves();
const getTxRequestStub = sandbox.stub(tssUtils, 'getTxRequest');
getTxRequestStub.resolves(txRequest);
getTxRequestStub.calledWith(txRequestId);

const signedTxRequest = await tssUtils.signTxRequest({
txRequest: txRequestId,
prv: JSON.stringify(validUserSigningMaterial),
txParams: { recipients: [{ address: '5f8f5a1d7f', amount: '10000' }] },
reqId,
});
signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs);

sandbox.verifyAndRestore();
});

it('signTxRequest should reject when txParams.recipients is missing for payment', async function () {
await tssUtils
.signTxRequest({
txRequest,
prv: JSON.stringify(validUserSigningMaterial),
reqId,
})
.should.be.rejectedWith(
'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.'
);
});

it('signTxRequest should succeed for stakingActivate without recipients', async function () {
const stakingTxRequest = { ...txRequest, intent: { intentType: 'stakingActivate' } };
sandbox.stub(baseCoin, 'verifyTransaction').resolves();
const signedTxRequest = await tssUtils.signTxRequest({
txRequest: stakingTxRequest,
prv: JSON.stringify(validUserSigningMaterial),
reqId,
});
signedTxRequest.unsignedTxs.should.deepEqual(stakingTxRequest.unsignedTxs);
sandbox.verifyAndRestore();
});

it('signTxRequest should succeed for walletInitialization without recipients', async function () {
const walletInitTxRequest = { ...txRequest, intent: { intentType: 'walletInitialization' } };
sandbox.stub(baseCoin, 'verifyTransaction').resolves();
const signedTxRequest = await tssUtils.signTxRequest({
txRequest: walletInitTxRequest,
prv: JSON.stringify(validUserSigningMaterial),
reqId,
});
signedTxRequest.unsignedTxs.should.deepEqual(walletInitTxRequest.unsignedTxs);
sandbox.verifyAndRestore();
});
});

describe('signTxRequest With Commitment:', function () {
Expand Down Expand Up @@ -700,9 +740,11 @@ describe('TSS Utils:', async function () {
});

it('signTxRequest should succeed with txRequest object as input', async function () {
sandbox.stub(baseCoin, 'verifyTransaction').resolves();
const signedTxRequest = await tssUtils.signTxRequest({
txRequest,
prv: JSON.stringify(validUserSigningMaterial),
txParams: { recipients: [{ address: '5f8f5a1d7f', amount: '10000' }] },
reqId,
});
signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs);
Expand All @@ -711,13 +753,15 @@ describe('TSS Utils:', async function () {
});

it('signTxRequest should succeed with txRequest id as input', async function () {
const getTxRequest = sandbox.stub(tssUtils, 'getTxRequest');
getTxRequest.resolves(txRequest);
getTxRequest.calledWith(txRequestId);
sandbox.stub(baseCoin, 'verifyTransaction').resolves();
const getTxRequestStub = sandbox.stub(tssUtils, 'getTxRequest');
getTxRequestStub.resolves(txRequest);
getTxRequestStub.calledWith(txRequestId);

const signedTxRequest = await tssUtils.signTxRequest({
txRequest: txRequestId,
prv: JSON.stringify(validUserSigningMaterial),
txParams: { recipients: [{ address: '5f8f5a1d7f', amount: '10000' }] },
reqId,
});
signedTxRequest.unsignedTxs.should.deepEqual(txRequest.unsignedTxs);
Expand Down
9 changes: 9 additions & 0 deletions modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsa.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import { IRequestTracer } from '../../../../api';
import { getBitgoMpcGpgPubKey } from '../../../tss/bitgoPubKeys';
import { EnvironmentName } from '../../../environments';
import { readKey } from 'openpgp';
import { resolveEffectiveTxParams } from '../recipientUtils';

/**
* Utility functions for TSS work flows.
Expand Down Expand Up @@ -610,6 +611,14 @@ export class EddsaUtils extends baseTSSUtils<KeyShare> {
);
unsignedTx =
apiVersion === 'full' ? txRequestResolved.transactions![0].unsignedTx : txRequestResolved.unsignedTxs[0];

const effectiveTxParams = resolveEffectiveTxParams(txRequestResolved, params.txParams);
await this.baseCoin.verifyTransaction({
txPrebuild: { txHex: unsignedTx.signableHex },
txParams: effectiveTxParams,
wallet: this.wallet,
walletType: this.wallet.multisigType(),
});
} else if (requestType === RequestType.message) {
assert(txRequestResolved.messages?.length, 'Unable to find messages in txRequest for message signing');
const message = txRequestResolved.messages[0];
Expand Down
63 changes: 61 additions & 2 deletions modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,71 @@ import { PopulatedIntent, TxRequest } from './baseTypes';
* Transaction types that legitimately carry no explicit recipients.
* verifyTransaction handles no-recipient validation for these internally.
* Mirrors the bypass list in abstractEthLikeNewCoins.ts verifyTssTransaction.
*
* ECDSA types: acceleration, fillNonce, transferToken, tokenApproval, consolidate,
* bridgeFunds, enableToken, customTx
* EdDSA types: staking operations, wallet/account init, token ops, CANTON 2-step flows
*/
export const NO_RECIPIENT_TX_TYPES = new Set([
// ECDSA types
'acceleration',
'fillNonce',
'transferToken',
'tokenApproval',
'consolidate',
'bridgeFunds',
'enableToken',
'customTx', // DeFi/WalletConnect smart contract interactions have no traditional recipients
'customTx',

// EdDSA staking (SOL, ADA, NEAR, DOT, CSPR, SUI, APT)
'stakingActivate',
'stakingDeactivate',
'stakingWithdraw',
'stakingClaim',
'stakingDelegate',
'stakingUnlock',
'stakingUnvote',
'stakingPledge',
'stakingAuthorize',
'stakingAuthorizeRaw',
'stakingLock', // CSPR
'stakingAdd', // SUI
'addStake', // SUI
'withdrawStake', // SUI

// EdDSA account / wallet initialization
'walletInitialization',
'addressInitialization', // DOT
'createAccount', // SOL + CANTON
'accountUpdate', // HBAR

// EdDSA token operations
'trustline', // XLM
'closeAssociatedTokenAccount', // SOL, HBAR
'associatedTokenAccountInitialization', // SOL, HBAR
'tokenTransfer', // SUI
'sendToken', // TON
'sendNFT', // APT

// NEAR
'storageDeposit',

// TON
'singleNominatorWithdraw',
'tonWhalesDeposit',
'tonWhalesWithdrawal',
'tonWhalesVestingDeposit',
'tonWhalesVestingWithdrawal',

// ADA
'voteDelegation',

// CANTON 2-step transfer flows
'transferAccept', // already in populateIntent() exempt list
'transferReject', // already in populateIntent() exempt list
'transferAcknowledge',
'transferOfferWithdrawn', // already in populateIntent() exempt list
'oneStepPreApproval', // CANTON enableToken
]);

/**
Expand Down Expand Up @@ -43,7 +98,11 @@ export function resolveEffectiveTxParams(
recipients: txParams?.recipients?.length ? txParams.recipients : intentRecipients,
};

if (!effectiveTxParams.recipients?.length && !NO_RECIPIENT_TX_TYPES.has(effectiveTxParams.type ?? '')) {
// Fall back to intent.intentType when txParams.type is not explicitly set.
// This covers EdDSA coins where the wallet SDK populates intent but not txParams.type.
const txType = effectiveTxParams.type ?? (txRequest.intent as PopulatedIntent)?.intentType ?? '';

if (!effectiveTxParams.recipients?.length && !NO_RECIPIENT_TX_TYPES.has(txType)) {
throw new InvalidTransactionError(
'Recipient details are required to verify this transaction before signing. Pass txParams with at least one recipient.'
);
Expand Down
51 changes: 45 additions & 6 deletions modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ function makeTxRequest(

describe('recipientUtils', function () {
describe('NO_RECIPIENT_TX_TYPES', function () {
it('contains exactly the 8 expected exempted types', function () {
it('contains all ECDSA exempted types', function () {
const { NO_RECIPIENT_TX_TYPES } = getModule();
const expected = [
const ecdsaTypes = [
'acceleration',
'fillNonce',
'transferToken',
Expand All @@ -35,8 +35,32 @@ describe('recipientUtils', function () {
'enableToken',
'customTx',
];
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);
ecdsaTypes.forEach((t) => assert.ok(NO_RECIPIENT_TX_TYPES.has(t), `${t} should be in NO_RECIPIENT_TX_TYPES`));
});

it('contains EdDSA staking types', function () {
const { NO_RECIPIENT_TX_TYPES } = getModule();
const stakingTypes = [
'stakingActivate',
'stakingDeactivate',
'stakingWithdraw',
'stakingClaim',
'stakingDelegate',
'walletInitialization',
];
stakingTypes.forEach((t) => assert.ok(NO_RECIPIENT_TX_TYPES.has(t), `${t} should be in NO_RECIPIENT_TX_TYPES`));
});

it('contains CANTON transfer flow types', function () {
const { NO_RECIPIENT_TX_TYPES } = getModule();
const cantonTypes = [
'transferAccept',
'transferReject',
'transferAcknowledge',
'transferOfferWithdrawn',
'oneStepPreApproval',
];
cantonTypes.forEach((t) => assert.ok(NO_RECIPIENT_TX_TYPES.has(t), `${t} should be in NO_RECIPIENT_TX_TYPES`));
});
});

Expand Down Expand Up @@ -98,7 +122,22 @@ describe('recipientUtils', function () {
);
});

const NO_RECIPIENT_TYPES = [
it('allows empty recipients when txParams.type is a no-recipient type', function () {
const { resolveEffectiveTxParams } = getModule();
const txRequest = makeTxRequest();
const result = resolveEffectiveTxParams(txRequest, { type: 'stakingActivate' });
result.type.should.equal('stakingActivate');
});

it('allows empty recipients when intent.intentType is a no-recipient type (EdDSA fallback)', function () {
const { resolveEffectiveTxParams } = getModule();
const txRequest = { ...makeTxRequest(), intent: { intentType: 'walletInitialization' } };
// No txParams.type — guard must fall back to intent.intentType
const result = resolveEffectiveTxParams(txRequest, undefined);
assert.ok(!result.recipients?.length, 'No recipients expected');
});

const ECDSA_NO_RECIPIENT_TYPES = [
'acceleration',
'fillNonce',
'transferToken',
Expand All @@ -109,7 +148,7 @@ describe('recipientUtils', function () {
'customTx', // DeFi/WalletConnect smart contract interactions have no traditional recipients
];

NO_RECIPIENT_TYPES.forEach((type) => {
ECDSA_NO_RECIPIENT_TYPES.forEach((type) => {
it(`allows empty recipients for no-recipient tx type: ${type}`, function () {
const { resolveEffectiveTxParams } = getModule();
const txRequest = makeTxRequest();
Expand Down
Loading