From 734c375baf298d0b165bb8a30f75d235241ced8a Mon Sep 17 00:00:00 2001 From: Venkatesh V Date: Thu, 27 Aug 2026 15:10:52 +0000 Subject: [PATCH] fix(abstract-eth): validate defi calldata Decode DeFi calldata in TSS verification and compare embedded values against wallet intent; add regression coverage for redirected recipients and excess approvals. Ticket: DEFI-614 Session-Id: d90e4bb7-8f74-4cb7-aedb-aa9947b9c0ae Task-Id: 98400f52-e042-4e3a-88f1-71b74a0737bd --- .../src/abstractEthLikeNewCoins.ts | 90 +++++++++ modules/sdk-coin-eth/test/unit/eth.ts | 177 +++++++++++++++++- 2 files changed, 265 insertions(+), 2 deletions(-) diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index 16223616db..75110f12d4 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -6,6 +6,7 @@ import { BitGoBase, BuildNftTransferDataOptions, common, + DefiIntentParams, Ecdsa, ECDSAMethodTypes, ECDSAUtils, @@ -380,6 +381,7 @@ interface EthTransactionParams extends TransactionParams { prebuildTx?: PrebuildTransactionResult; tokenName?: string; feeToken?: string; + defiParams?: DefiIntentParams; } export interface VerifyEthTransactionOptions extends VerifyTransactionOptions { @@ -3294,6 +3296,94 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem } } + if (txParams.type && ['defiApprove', 'defiDeposit', 'defiWithdraw'].includes(txParams.type)) { + if (!txPrebuild.txHex) { + throw new Error('missing txHex in txPrebuild'); + } + + const baseAddress = wallet.coinSpecific()?.baseAddress; + const defiParams = txParams.defiParams; + if (!baseAddress || !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 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') { + 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'); + } + const [, amount] = getRawDecoded(['address', 'uint256'], getBufferedByteCode(selector, data)); + if (amount.toString() !== validatedDefiParams.amount) { + await mismatch( + 'defiApprove transaction amount does not match the requested amount', + txJson.to, + amount.toString() + ); + } + } else if (txParams.type === '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'); + } + const [amount, receiver] = getRawDecoded(['uint256', 'address'], getBufferedByteCode(selector, data)); + const decodedReceiver = addHexPrefix(receiver.toString()).toLowerCase(); + if (decodedReceiver !== validatedBaseAddress.toLowerCase()) { + await mismatch( + 'defiDeposit transaction receiver does not match wallet base address', + decodedReceiver, + amount.toString() + ); + } + if (amount.toString() !== validatedDefiParams.amount) { + await mismatch( + 'defiDeposit transaction amount does not match the requested amount', + decodedReceiver, + amount.toString() + ); + } + } else { + const selector = addHexPrefix( + optionalDeps.ethAbi.methodID('redeem', ['uint256', 'address', 'address']).toString('hex') + ); + if (!data.startsWith(selector)) { + await mismatch('defiWithdraw transaction must use redeem(uint256,address,address) calldata'); + } + const [shares, receiver, owner] = getRawDecoded( + ['uint256', 'address', 'address'], + getBufferedByteCode(selector, data) + ); + const decodedReceiver = addHexPrefix(receiver.toString()).toLowerCase(); + const decodedOwner = addHexPrefix(owner.toString()).toLowerCase(); + if ( + decodedReceiver !== validatedBaseAddress.toLowerCase() || + decodedOwner !== validatedBaseAddress.toLowerCase() + ) { + await mismatch( + 'defiWithdraw transaction receiver and owner must match wallet base address', + decodedReceiver, + shares.toString() + ); + } + if (shares.toString() !== validatedDefiParams.amount) { + await mismatch( + 'defiWithdraw transaction shares do not match the requested amount', + decodedReceiver, + shares.toString() + ); + } + } + } + // Verify consolidation transactions send to base address if (params.verification?.consolidationToBaseAddress) { const coinSpecific = wallet.coinSpecific(); diff --git a/modules/sdk-coin-eth/test/unit/eth.ts b/modules/sdk-coin-eth/test/unit/eth.ts index 03d88373c8..fcb5274ac0 100644 --- a/modules/sdk-coin-eth/test/unit/eth.ts +++ b/modules/sdk-coin-eth/test/unit/eth.ts @@ -12,6 +12,7 @@ import { InvalidAddressVerificationObjectPropertyError, MPCSweepTxs, TransactionType, + TxIntentMismatchRecipientError, UnexpectedAddressError, Wallet, } from '@bitgo/sdk-core'; @@ -55,6 +56,22 @@ describe('ETH:', function () { const userReqSig = '0x404db307f6147f0d8cd338c34c13906ef46a6faa7e0e119d5194ef05aec16e6f3d710f9b7901460f97e924066b62efd74443bd34402c6d40b49c203a559ff2c8'; + const buildDefiTxHex = async (data: string): Promise => { + const txBuilder = getBuilder('hteth') as TransactionBuilder; + txBuilder.type(TransactionType.ContractCall); + txBuilder.fee({ fee: '10', gasLimit: '100000' }); + txBuilder.counter(1); + txBuilder.contract(address1); + txBuilder.data(data); + return (await txBuilder.build()).toBroadcastFormat(); + }; + + const defiCalldata = (signature: string, values: string[]): string => { + const types = signature.slice(signature.indexOf('(') + 1, -1).split(','); + const methodId = EthereumAbi.methodID(signature.split('(')[0], types); + return '0x' + Buffer.concat([methodId, EthereumAbi.rawEncode(types, values)]).toString('hex'); + }; + before(function () { const bitgoKeyXprv = 'xprv9s21ZrQH143K3tpWBHWe31sLoXNRQ9AvRYJgitkKxQ4ATFQMwvr7hHNqYRUnS7PsjzB7aK1VxqHLuNQjj1sckJ2Jwo2qxmsvejwECSpFMfC'; @@ -665,6 +682,17 @@ describe('ETH:', function () { }); describe('TSS Transaction Verification', function () { + const expectDefiRejection = async (p: Promise, messagePattern: RegExp) => { + try { + await p; + } catch (err) { + err.should.be.instanceOf(TxIntentMismatchRecipientError); + (err as Error).message.should.match(messagePattern); + return; + } + throw new Error('expected verifyTransaction to be rejected, but it resolved'); + }; + it('should verify TSS consolidation transaction when txPrebuild has consolidateId', async function () { const coin = bitgo.coin('hteth') as Hteth; const baseAddress = '0x174cfd823af8ce27ed0afee3fcf3c3ba259116be'; @@ -886,7 +914,7 @@ describe('ETH:', function () { }; const txPrebuild = { - txHex: '0x', + txHex: await buildDefiTxHex(defiCalldata('approve(address,uint256)', [address1, '10'])), coin: 'hteth', walletId: 'fakeWalletId', }; @@ -952,7 +980,7 @@ describe('ETH:', function () { }; const txPrebuild = { - txHex: '0x', + txHex: await buildDefiTxHex(defiCalldata('deposit(uint256,address)', ['10', address1])), coin: 'hteth', walletId: 'fakeWalletId', }; @@ -969,6 +997,151 @@ describe('ETH:', function () { isTransactionVerified.should.equal(true); }); + it('should reject DeFi calldata that redirects funds or grants excess approval', async function () { + const coin = bitgo.coin('hteth') as Hteth; + const wallet = new Wallet(bitgo, coin, { coinSpecific: { baseAddress: address1 } }); + const verify = async (type: string, defiParams: Record | undefined, data: string) => + coin.verifyTransaction({ + txParams: { type, defiParams, wallet } as any, + txPrebuild: { txHex: await buildDefiTxHex(data), coin: 'hteth', walletId: 'fakeWalletId' } as any, + wallet, + verification: {}, + walletType: 'tss', + }); + + // defiApprove: attacker approve(attacker, MAX_UINT256) disguised as defiApprove + await expectDefiRejection( + verify( + 'defiApprove', + { vaultId: 'vault', amount: '10' }, + defiCalldata('approve(address,uint256)', [address2, '1000000000000000000']) + ), + /amount does not match/ + ); + // defiDeposit: funds redirected to attacker receiver + await expectDefiRejection( + verify( + 'defiDeposit', + { vaultId: 'vault', amount: '10' }, + defiCalldata('deposit(uint256,address)', ['10', address2]) + ), + /receiver does not match/ + ); + // defiWithdraw: receiver and owner both attacker + await expectDefiRejection( + verify( + 'defiWithdraw', + { vaultId: 'vault', amount: '10' }, + defiCalldata('redeem(uint256,address,address)', ['5', address2, address2]) + ), + /receiver and owner/ + ); + }); + + it('should reject DeFi calldata with wrong selector, wrong owner alone, or wrong deposit amount', async function () { + const coin = bitgo.coin('hteth') as Hteth; + const wallet = new Wallet(bitgo, coin, { coinSpecific: { baseAddress: address1 } }); + const verify = async (type: string, defiParams: Record, data: string) => + coin.verifyTransaction({ + txParams: { type, defiParams, wallet } as any, + txPrebuild: { txHex: await buildDefiTxHex(data), coin: 'hteth', walletId: 'fakeWalletId' } as any, + wallet, + verification: {}, + walletType: 'tss', + }); + const defiParams = { vaultId: 'vault', amount: '10' }; + + // defiApprove with a non-approve selector (transfer disguised as approve) + await expectDefiRejection( + verify('defiApprove', defiParams, defiCalldata('transfer(address,uint256)', [address2, '10'])), + /approve\(address,uint256\)/ + ); + // defiDeposit with a non-deposit selector + await expectDefiRejection( + verify('defiDeposit', defiParams, defiCalldata('approve(address,uint256)', [address1, '10'])), + /deposit\(uint256,address\)/ + ); + // defiWithdraw with a non-redeem selector + await expectDefiRejection( + verify('defiWithdraw', defiParams, defiCalldata('approve(address,uint256)', [address1, '10'])), + /redeem\(uint256,address,address\)/ + ); + // defiWithdraw: receiver ok but owner is attacker + await expectDefiRejection( + verify('defiWithdraw', defiParams, defiCalldata('redeem(uint256,address,address)', ['10', address1, address2])), + /receiver and owner/ + ); + // defiWithdraw: receiver and owner ok but shares do not match defiParams.amount + await expectDefiRejection( + verify( + 'defiWithdraw', + defiParams, + defiCalldata('redeem(uint256,address,address)', ['999', address1, address1]) + ), + /shares do not match/ + ); + // defiDeposit: receiver ok but amount mismatch + await expectDefiRejection( + verify('defiDeposit', defiParams, defiCalldata('deposit(uint256,address)', ['999', address1])), + /amount does not match/ + ); + }); + + it('should fail closed when defi intent parameters are missing or calldata is short', async function () { + const coin = bitgo.coin('hteth') as Hteth; + const wallet = new Wallet(bitgo, coin, { coinSpecific: { baseAddress: address1 } }); + const verify = async (type: string, defiParams: Record | undefined, data: string) => + coin.verifyTransaction({ + txParams: { type, defiParams, wallet } as any, + txPrebuild: { txHex: await buildDefiTxHex(data), coin: 'hteth', walletId: 'fakeWalletId' } as any, + wallet, + verification: {}, + walletType: 'tss', + }); + + // Missing defiParams for each defi type -> fail closed + await expectDefiRejection( + verify('defiApprove', undefined, defiCalldata('approve(address,uint256)', [address1, '10'])), + /missing required intent parameters/ + ); + await expectDefiRejection( + verify('defiDeposit', undefined, defiCalldata('deposit(uint256,address)', ['10', address1])), + /missing required intent parameters/ + ); + await expectDefiRejection( + verify('defiWithdraw', undefined, defiCalldata('redeem(uint256,address,address)', ['5', address1, address1])), + /missing required intent parameters/ + ); + // Short/empty calldata with no valid selector prefix -> selector mismatch (fail closed) + await expectDefiRejection( + verify('defiApprove', { vaultId: 'vault', amount: '10' }, '0x'), + /approve\(address,uint256\)/ + ); + }); + + 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 } }); + const txParams = { + type: 'defiWithdraw', + defiParams: { vaultId: 'hteth-usdc-test', amount: '10' }, + wallet, + }; + const txPrebuild = { + txHex: await buildDefiTxHex(defiCalldata('redeem(uint256,address,address)', ['10', address1, address1])), + coin: 'hteth', + walletId: 'fakeWalletId', + }; + const isTransactionVerified = await coin.verifyTransaction({ + txParams: txParams as any, + txPrebuild: txPrebuild as any, + wallet, + verification: {}, + walletType: 'tss', + }); + isTransactionVerified.should.equal(true); + }); + describe('consolidationToBaseAddress verification', function () { it('should verify consolidation when recipient matches base address', async function () { const coin = bitgo.coin('hteth') as Hteth;