Skip to content

Commit 7251d82

Browse files
mullapudipruthvikmullapudipruthvik
committed
fix(sdk-core): add validation for unsigned txHex
Ticket: COIN-3222 TICKET: COIN-3222
1 parent da9b4eb commit 7251d82

File tree

8 files changed

+136
-12
lines changed

8 files changed

+136
-12
lines changed

‎modules/abstract-cosmos/src/cosmosCoin.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,6 @@ export class CosmosCoin<CustomMessage = never> extends BaseCoin {
360360
}
361361
const transaction = await this.getBuilder().from(rawTx).build();
362362
const explainedTx = transaction.explainTransaction();
363-
364363
if (txParams.recipients && txParams.recipients.length > 0) {
365364
const filteredRecipients = txParams.recipients?.map((recipient) => _.pick(recipient, ['address', 'amount']));
366365
const filteredOutputs = explainedTx.outputs.map((output) => _.pick(output, ['address', 'amount']));

‎modules/abstract-eth/src/abstractEthLikeNewCoins.ts‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,10 @@ import {
6666
calculateForwarderV1Address,
6767
ERC1155TransferBuilder,
6868
ERC721TransferBuilder,
69+
getBufferedByteCode,
6970
getCommon,
7071
getProxyInitcode,
72+
getRawDecoded,
7173
getToken,
7274
KeyPair as KeyPairLib,
7375
TransactionBuilder,
@@ -2523,13 +2525,13 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
25232525
* @param {Wallet} params.wallet - Wallet object to obtain keys to verify against
25242526
* @returns {boolean}
25252527
*/
2526-
verifyTssTransaction(params: VerifyEthTransactionOptions): boolean {
2528+
async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise<boolean> {
25272529
const { txParams, txPrebuild, wallet } = params;
25282530
if (
25292531
!txParams?.recipients &&
25302532
!(
25312533
txParams.prebuildTx?.consolidateId ||
2532-
(txParams.type && ['acceleration', 'fillNonce', 'transferToken'].includes(txParams.type))
2534+
(txParams.type && ['acceleration', 'fillNonce', 'transferToken', 'transfer'].includes(txParams.type))
25332535
)
25342536
) {
25352537
throw new Error(`missing txParams`);
@@ -2540,6 +2542,39 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
25402542
if (txParams.hop && txParams.recipients && txParams.recipients.length > 1) {
25412543
throw new Error(`tx cannot be both a batch and hop transaction`);
25422544
}
2545+
2546+
if (txParams.type && ['transfer'].includes(txParams.type)) {
2547+
if (txParams.recipients && txParams.recipients.length === 1) {
2548+
const recipients = txParams.recipients;
2549+
const expectedAmount = recipients[0].amount.toString();
2550+
const expectedDestination = recipients[0].address;
2551+
2552+
const txBuilder = this.getTransactionBuilder();
2553+
txBuilder.from(txPrebuild.txHex);
2554+
const tx = await txBuilder.build();
2555+
const txJson = tx.toJson();
2556+
if (txJson.data === '0x') {
2557+
if (expectedAmount !== txJson.value) {
2558+
throw new Error('the transaction amount in txPrebuild does not match the value given by client');
2559+
}
2560+
if (expectedDestination !== txJson.to) {
2561+
throw new Error('destination address does not match with the recipient address');
2562+
}
2563+
} else if (txJson.data.startsWith('0xa9059cbb')) {
2564+
const [recipientAddress, amount] = getRawDecoded(
2565+
['address', 'uint256'],
2566+
getBufferedByteCode('0xa9059cbb', txJson.data)
2567+
);
2568+
if (expectedAmount !== amount.toString()) {
2569+
throw new Error('the transaction amount in txPrebuild does not match the value given by client');
2570+
}
2571+
if (expectedDestination !== addHexPrefix(recipientAddress.toString())) {
2572+
throw new Error('destination address does not match with the recipient address');
2573+
}
2574+
}
2575+
}
2576+
}
2577+
25432578
return true;
25442579
}
25452580

‎modules/bitgo/test/v2/unit/internal/tssUtils/ecdsa.ts‎

Lines changed: 83 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -659,8 +659,11 @@ describe('TSS Ecdsa Utils:', async function () {
659659
transactions: [
660660
{
661661
unsignedTx: {
662-
serializedTxHex: 'TOO MANY SECRETS',
663-
signableHex: 'TOO MANY SECRETS',
662+
// hteth txid: 0xc5a7bfe6b13ceae563da0f9feaa9c4ad1c101a15366a2a488828a5dd27cb9da3
663+
serializedTxHex:
664+
'02f38242688084448b9b8084448b9b908301637894a1cfb9d51c0af191ff21c5f0f01723e056f7dc12865af3107a400080c0808080',
665+
signableHex:
666+
'02f08242688084448b9b8084448b9b908301637894a1cfb9d51c0af191ff21c5f0f01723e056f7dc12865af3107a400080c0',
664667
derivationPath: '', // Needs this when key derivation is supported
665668
},
666669
state: 'pendingSignature',
@@ -669,8 +672,11 @@ describe('TSS Ecdsa Utils:', async function () {
669672
],
670673
unsignedTxs: [
671674
{
672-
serializedTxHex: 'TOO MANY SECRETS',
673-
signableHex: 'TOO MANY SECRETS',
675+
// hteth txid: 0xc5a7bfe6b13ceae563da0f9feaa9c4ad1c101a15366a2a488828a5dd27cb9da3
676+
serializedTxHex:
677+
'02f38242688084448b9b8084448b9b908301637894a1cfb9d51c0af191ff21c5f0f01723e056f7dc12865af3107a400080c0808080',
678+
signableHex:
679+
'02f38242688084448b9b8084448b9b908301637894a1cfb9d51c0af191ff21c5f0f01723e056f7dc12865af3107a400080c0808080',
674680
derivationPath: '', // Needs this when key derivation is supported
675681
},
676682
],
@@ -933,6 +939,68 @@ describe('TSS Ecdsa Utils:', async function () {
933939
userGpgActual.should.startWith('-----BEGIN PGP PUBLIC KEY BLOCK-----');
934940
});
935941

942+
it('signTxRequest should fail with wrong recipient', async function () {
943+
await setupSignTxRequestNocks(true, userSignShare, aShare, dShare, enterpriseData);
944+
await tssUtils
945+
.signTxRequest({
946+
txRequest: txRequestId,
947+
prv: JSON.stringify({
948+
pShare: userKeyShare.pShare,
949+
bitgoNShare: bitgoKeyShare.nShares[1],
950+
backupNShare: backupKeyShare.nShares[1],
951+
}),
952+
reqId,
953+
txParams: { recipients: [{ address: '0x1234', amount: '100000000000000' }], type: 'transfer' },
954+
})
955+
.should.be.rejectedWith('destination address does not match with the recipient address');
956+
});
957+
958+
it('signTxRequest should fail with incorrect value', async function () {
959+
await setupSignTxRequestNocks(true, userSignShare, aShare, dShare, enterpriseData);
960+
await tssUtils
961+
.signTxRequest({
962+
txRequest: txRequestId,
963+
prv: JSON.stringify({
964+
pShare: userKeyShare.pShare,
965+
bitgoNShare: bitgoKeyShare.nShares[1],
966+
backupNShare: backupKeyShare.nShares[1],
967+
}),
968+
reqId,
969+
txParams: {
970+
recipients: [{ address: '0xa1cfb9d51c0af191ff21c5f0f01723e056f7dc12', amount: '1' }],
971+
type: 'transfer',
972+
},
973+
})
974+
.should.be.rejectedWith('the transaction amount in txPrebuild does not match the value given by client');
975+
});
976+
977+
it('signTxRequest should fail with incorrect value for token txn', async function () {
978+
const signableHex =
979+
'02f86d8242681083122c9e83122cae8301e04994ebe8b46a42f05072b723b00013ff822b2af1b5cb80b844a9059cbb0000000000000000000000002b0d6cb2f8c388757f4d7ad857fccab18290dbc900000000000000000000000000000000000000000000000000000000000186a0c0';
980+
const serializedTxHex =
981+
'02f8708242681083122c9e83122cae8301e04994ebe8b46a42f05072b723b00013ff822b2af1b5cb80b844a9059cbb0000000000000000000000002b0d6cb2f8c388757f4d7ad857fccab18290dbc900000000000000000000000000000000000000000000000000000000000186a0c0808080';
982+
await setupSignTxRequestNocks(true, userSignShare, aShare, dShare, enterpriseData, {
983+
signableHex,
984+
serializedTxHex,
985+
apiVersion: 'full',
986+
});
987+
await tssUtils
988+
.signTxRequest({
989+
txRequest: txRequestId,
990+
prv: JSON.stringify({
991+
pShare: userKeyShare.pShare,
992+
bitgoNShare: bitgoKeyShare.nShares[1],
993+
backupNShare: backupKeyShare.nShares[1],
994+
}),
995+
reqId,
996+
txParams: {
997+
recipients: [{ address: '0x2b0d6cb2f8c388757f4d7ad857fccab18290dbc9', amount: '707' }],
998+
type: 'transfer',
999+
},
1000+
})
1001+
.should.be.rejectedWith('the transaction amount in txPrebuild does not match the value given by client');
1002+
});
1003+
9361004
it('getOfflineSignerPaillierModulus should succeed', async function () {
9371005
const paillierModulus = tssUtils.getOfflineSignerPaillierModulus({
9381006
prv: JSON.stringify({
@@ -1101,7 +1169,12 @@ describe('TSS Ecdsa Utils:', async function () {
11011169
userSignShare: ECDSA.SignShareRT,
11021170
aShare: ECDSA.AShare,
11031171
dShare: ECDSA.DShare,
1104-
enterpriseData?: EnterpriseData
1172+
enterpriseData: EnterpriseData,
1173+
{
1174+
signableHex,
1175+
serializedTxHex,
1176+
apiVersion,
1177+
}: { signableHex?: string; serializedTxHex?: string; apiVersion?: 'full' | 'lite' } = {}
11051178
) {
11061179
if (enterpriseData) {
11071180
await nockGetEnterprise({ enterpriseId: enterpriseData.id, response: enterpriseData, times: 1 });
@@ -1116,12 +1189,13 @@ describe('TSS Ecdsa Utils:', async function () {
11161189
{
11171190
...txRequest,
11181191
unsignedTx: {
1119-
signableHex: txRequest.unsignedTxs[0].signableHex,
1120-
serializedTxHex: txRequest.unsignedTxs[0].serializedTxHex,
1192+
signableHex: signableHex ?? txRequest.unsignedTxs[0].signableHex,
1193+
serializedTxHex: serializedTxHex ?? txRequest.unsignedTxs[0].serializedTxHex,
11211194
derivationPath,
11221195
},
11231196
},
11241197
],
1198+
apiVersion: apiVersion,
11251199
},
11261200
],
11271201
};
@@ -1145,6 +1219,7 @@ describe('TSS Ecdsa Utils:', async function () {
11451219
},
11461220
},
11471221
],
1222+
apiVersion: apiVersion,
11481223
},
11491224
],
11501225
};
@@ -1165,6 +1240,7 @@ describe('TSS Ecdsa Utils:', async function () {
11651240
},
11661241
},
11671242
],
1243+
apiVersion: apiVersion,
11681244
},
11691245
],
11701246
};

‎modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ export interface VerificationOptions {
160160
export interface VerifyTransactionOptions {
161161
txPrebuild: TransactionPrebuild;
162162
txParams: TransactionParams;
163-
wallet: Wallet;
163+
wallet: IWallet;
164164
verification?: VerificationOptions;
165165
reqId?: IRequestTracer;
166166
walletType?: 'onchain' | 'tss';

‎modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { Key, SerializedKeyPair } from 'openpgp';
22
import { IRequestTracer } from '../../../api';
3-
import { KeychainsTriplet, ParsedTransaction } from '../../baseCoin';
3+
import { KeychainsTriplet, ParsedTransaction, TransactionParams } from '../../baseCoin';
44
import { ApiKeyShare, Keychain } from '../../keychain';
55
import { ApiVersion, Memo, WalletType } from '../../wallet';
66
import { EDDSA, GShare, Signature, SignShare } from '../../../account-lib/mpc/tss';
@@ -430,6 +430,7 @@ export type TSSParams = {
430430
txRequest: string | TxRequest; // can be either a string or TxRequest
431431
reqId: IRequestTracer;
432432
apiVersion?: ApiVersion;
433+
txParams?: TransactionParams;
433434
};
434435

435436
export type TSSParamsForMessage = TSSParams & {

‎modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -900,6 +900,12 @@ export class EcdsaUtils extends BaseEcdsaUtils {
900900
assert(txRequest.transactions || txRequest.unsignedTxs, 'Unable to find transactions in txRequest');
901901
const unsignedTx =
902902
txRequest.apiVersion === 'full' ? txRequest.transactions![0].unsignedTx : txRequest.unsignedTxs[0];
903+
await this.baseCoin.verifyTransaction({
904+
txPrebuild: { txHex: unsignedTx.signableHex },
905+
txParams: params.txParams || { recipients: [] },
906+
wallet: this.wallet,
907+
walletType: this.wallet.multisigType(),
908+
});
903909
signablePayload = Buffer.from(unsignedTx.signableHex, 'hex');
904910
derivationPath = unsignedTx.derivationPath;
905911
} else if (requestType === RequestType.message) {

‎modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -731,6 +731,12 @@ export class EcdsaMPCv2Utils extends BaseEcdsaUtils {
731731
assert(txRequest.transactions || txRequest.unsignedTxs, 'Unable to find transactions in txRequest');
732732
const unsignedTx =
733733
txRequest.apiVersion === 'full' ? txRequest.transactions![0].unsignedTx : txRequest.unsignedTxs[0];
734+
await this.baseCoin.verifyTransaction({
735+
txPrebuild: { txHex: unsignedTx.signableHex },
736+
txParams: params.txParams || { recipients: [] },
737+
wallet: this.wallet,
738+
walletType: this.wallet.multisigType(),
739+
});
734740
txOrMessageToSign = unsignedTx.signableHex;
735741
derivationPath = unsignedTx.derivationPath;
736742
bufferContent = Buffer.from(txOrMessageToSign, 'hex');

‎modules/sdk-core/src/bitgo/wallet/wallet.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3441,6 +3441,7 @@ export class Wallet implements IWallet {
34413441
try {
34423442
return await this.tssUtils!.signTxRequest({
34433443
txRequest: params.txPrebuild.txRequestId,
3444+
txParams: params.txPrebuild.buildParams,
34443445
prv: params.prv,
34453446
reqId: params.reqId || new RequestTracer(),
34463447
apiVersion: params.apiVersion,

0 commit comments

Comments
 (0)