From 3f0b3a21e1cbc16beb130b2943aace126e22c648 Mon Sep 17 00:00:00 2001 From: Yashvanth B L Date: Thu, 29 Oct 2026 18:01:30 +0530 Subject: [PATCH] fix(abstract-eth): erc-7984 pa-rebuild approval verifies content-based MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wrapApprove PA-rebuild (the onchain-multisig server-leg approval) failed verification: verifyMultisigWrapApproveTransaction demanded txParams.wrapParams (tokenName + amount) — but the PA-rebuild path carries no caller wrapParams (the PA's buildParams store only the type; the intent's wrapParams live in the journey). The verify's own doc comment contemplates the skip for the pendingApproval re-sign paths; the rebuild's merged txParams accidentally triggered the full check. Restructure verifyMultisigWrapApproveTransaction to be self-sufficient on the PA-rebuild path: - the wrapParams cross-check demand: skipped when txParams.pendingApprovalId is present (the PA-rebuild) - the pair resolved from the tx's own calldata: the approve's spender decoded → the reverse lookup over the registered ERC-7984 pairs on the network (by the wrapper address → the pair's tokenName) - the bounded checks preserved: the inner target = the pair's underlying ERC-20, no native value inside sendMultiSig, the spender = the wrapper, the prebuild-recipients mirror - the wrapParams cross-check (the amount vs the intent) runs only on the direct-sign path where the caller carries the intent Validated in staging: the wrapApprove PA approval — the rebuild 200, the verify passed (isPaRebuild: true, hasWrapParams: false), the tx signed, the approval PUT reached WP and the policy resolved. Ticket: CHALO-2010 --- .../src/abstractEthLikeNewCoins.ts | 94 +++++-- modules/abstract-eth/test/unit/coin.ts | 239 ++++++++++++++++++ 2 files changed, 307 insertions(+), 26 deletions(-) diff --git a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts index 8360491ff65..820369009ff 100644 --- a/modules/abstract-eth/src/abstractEthLikeNewCoins.ts +++ b/modules/abstract-eth/src/abstractEthLikeNewCoins.ts @@ -398,8 +398,12 @@ interface EthTransactionParams extends TransactionParams { wrapParams?: { tokenName: string; amount: string }; /** ERC-7984 unshield intent (`type: 'unwrap'`): wrapper coin name and plaintext amount. */ unwrapParams?: { tokenName: string; amount: string }; + /** Set on server-leg pending-approval rebuilds, which carry no caller wrapParams/unwrapParams. */ + pendingApprovalId?: string; } +const MAX_UINT256 = 2n ** 256n - 1n; + export interface VerifyEthTransactionOptions extends VerifyTransactionOptions { txPrebuild: TransactionPrebuild; txParams: EthTransactionParams; @@ -4021,26 +4025,21 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem if (!txPrebuild?.txHex) { throw new Error('missing txHex in txPrebuild'); } - if (!wrapParams?.tokenName || !wrapParams.amount) { + // CHALO-2010: the PA-rebuild path — the server derived the tx from the pending approval's own + // intent data (the PA carries only the type and the pendingApprovalId; the intent's wrapParams + // live in the journey). Strictness follows the presence of caller wrapParams, never the + // pendingApprovalId alone: when wrapParams carries the intent, tokenName and amount are always + // cross-checked. Only when wrapParams is absent AND a pendingApprovalId is present (the rebuild; + // pendingApprovalId is caller-supplied txParams, so it grants no more than the bounded checks) + // is the pair resolved from the tx's own calldata and the allowance bounded instead. + const requested = + wrapParams?.tokenName && wrapParams.amount + ? { tokenName: wrapParams.tokenName, amount: wrapParams.amount } + : undefined; + if (!requested && (wrapParams?.tokenName || wrapParams?.amount || !txParams.pendingApprovalId)) { return throwRecipientMismatch('wrapApprove transaction requires wrapParams with tokenName and amount', []); } - let wrapper: Erc7984Coin; - let underlyingErc20Address: string; - try { - const staticsCoin = coins.get(wrapParams.tokenName); - if (!(staticsCoin instanceof Erc7984Coin) || !staticsCoin.underlyingErc20Address) { - throw new Error('not an ERC-7984 pair'); - } - wrapper = staticsCoin; - underlyingErc20Address = staticsCoin.underlyingErc20Address.toLowerCase(); - } catch { - return throwRecipientMismatch( - `wrapApprove: '${wrapParams.tokenName}' is not a registered ERC-7984 shield pair`, - [] - ); - } - const txBuilder = this.getTransactionBuilder(); txBuilder.from(txPrebuild.txHex); const txJson = (await txBuilder.build()).toJson(); @@ -4071,13 +4070,6 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem const mismatch = (message: string, address = innerTo, amount = innerAmount): Promise => throwRecipientMismatch(message, [{ address: address || '', amount: amount || '' }]); - if (innerTo.toLowerCase() !== underlyingErc20Address) { - await mismatch('wrapApprove transaction must target the underlying ERC-20 of the wrapper'); - } - if (innerAmount !== '0') { - await mismatch('wrapApprove transaction must not carry native value inside sendMultiSig'); - } - let approve: { spender: string; amount: string }; try { approve = decodeApproveCalldata(innerData || ''); @@ -4086,11 +4078,61 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin implem `wrapApprove transaction must use ERC-20 approve(address,uint256) calldata — ${(e as Error).message}` ); } + + let wrapper: Erc7984Coin; + let underlyingErc20Address: string; + if (!requested) { + // CHALO-2010: no caller wrapParams — resolve the pair by the approve's spender (the reverse + // lookup over the registered ERC-7984 pairs on this network). + const network = this.getNetwork(); + if (!network) { + throw new Error('missing network for ERC-7984 intent verification'); + } + const { byTokenName } = getErc7984WrappersForNetwork(network); + const tokenName = [...byTokenName.entries()].find(([, addr]) => addr === approve.spender.toLowerCase())?.[0]; + const staticsCoin = tokenName ? coins.get(tokenName) : undefined; + if (!(staticsCoin instanceof Erc7984Coin) || !staticsCoin.underlyingErc20Address) { + return await mismatch('wrapApprove transaction spender is not a registered ERC-7984 wrapper', approve.spender); + } + wrapper = staticsCoin; + underlyingErc20Address = staticsCoin.underlyingErc20Address.toLowerCase(); + } else { + try { + const staticsCoin = coins.get(requested.tokenName); + if (!(staticsCoin instanceof Erc7984Coin) || !staticsCoin.underlyingErc20Address) { + throw new Error('not an ERC-7984 pair'); + } + wrapper = staticsCoin; + underlyingErc20Address = staticsCoin.underlyingErc20Address.toLowerCase(); + } catch { + return throwRecipientMismatch( + `wrapApprove: '${requested.tokenName}' is not a registered ERC-7984 shield pair`, + [] + ); + } + } + + if (innerTo.toLowerCase() !== underlyingErc20Address) { + await mismatch('wrapApprove transaction must target the underlying ERC-20 of the wrapper'); + } + if (innerAmount !== '0') { + await mismatch('wrapApprove transaction must not carry native value inside sendMultiSig'); + } + if (approve.spender.toLowerCase() !== wrapper.contractAddress.toLowerCase()) { await mismatch('wrapApprove transaction spender does not match the wrapper contract', approve.spender); } - if (approve.amount !== wrapParams.amount) { - await mismatch('wrapApprove transaction amount does not match the requested amount', innerTo, approve.amount); + if (requested) { + if (approve.amount !== requested.amount) { + await mismatch('wrapApprove transaction amount does not match the requested amount', innerTo, approve.amount); + } + } else if (!/^\d+$/.test(approve.amount) || BigInt(approve.amount) <= 0n || BigInt(approve.amount) >= MAX_UINT256) { + // no caller intent to compare against: still bound the allowance (positive, not unlimited) + await mismatch( + 'wrapApprove transaction amount must be a positive integer and not the unlimited allowance', + innerTo, + approve.amount + ); } await this.assertPrebuildRecipientMirrorsSendMultiSig( diff --git a/modules/abstract-eth/test/unit/coin.ts b/modules/abstract-eth/test/unit/coin.ts index 06b9010246b..b73cb839a19 100644 --- a/modules/abstract-eth/test/unit/coin.ts +++ b/modules/abstract-eth/test/unit/coin.ts @@ -478,6 +478,245 @@ export function runTransactionVerificationTests(coinName: string, bitgo: TestBit .verifyTransaction({ txParams, txPrebuild, wallet, verification }) .should.be.rejectedWith('coin in txPrebuild did not match that in txParams supplied by client'); }); + + // CHALO-2010: the ERC-7984 PA-rebuild path — the approve leg captured from staging + // (the sendMultiSig envelope: approve(spender=0x2debbe04 wrapper, amount=0xf4240) on the + // pair's underlying 0x51a63b56). The PA-rebuild carries no caller wrapParams — the pair is + // resolved from the tx's own calldata. + const erc7984WrapApproveTxHex = + '0xf901ed01843b9aca0083030d40943b58684525564b38a381e46a731703ed03f3212280b901c43912521500000000000000000000000051a63b5621d78de54d2f4d098a23a5a69e76f30b000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c0000000000000000000000000000000000000000000000000000000006adf0240000000000000000000000000000000000000000000000000000000000000000e00000000000000000000000000000000000000000000000000000000000001400000000000000000000000000000000000000000000000000000000000000044095ea7b30000000000000000000000002debbe0487ef921df4457f9e36ed05be2df1ac7500000000000000000000000000000000000000000000000000000000000f4240000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000041000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000831117838080'; + const erc7984Prebuild = { + coin: coinTest, + txHex: erc7984WrapApproveTxHex, + recipients: [ + { + address: '0x51a63b5621d78de54d2f4d098a23a5a69e76f30b', + amount: '0', + data: '0x095ea7b30000000000000000000000002debbe0487ef921df4457f9e36ed05be2df1ac7500000000000000000000000000000000000000000000000000000000000f4240', + }, + ], + }; + + it('verifies the ERC-7984 wrapApprove PA-rebuild without caller wrapParams', async () => { + const wallet = new Wallet(bitgo, basecoin, {}); + + const txParams = { + pendingApprovalId: '6ae3233a7d9bc0b7b3f187a165996e15', + type: 'wrapApprove', + recipients: [], + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const verification = {}; + + const isTransactionVerified = await basecoin.verifyTransaction({ + txParams, + txPrebuild: erc7984Prebuild, + wallet, + verification, + }); + isTransactionVerified.should.equal(true); + }); + + it('fails closed when the PA-rebuild spender is not a registered ERC-7984 wrapper', async () => { + const wallet = new Wallet(bitgo, basecoin, {}); + + const txParams = { + pendingApprovalId: '6ae3233a7d9bc0b7b3f187a165996e15', + type: 'wrapApprove', + recipients: [], + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const tamperedTxHex = erc7984WrapApproveTxHex.replace( + '2debbe0487ef921df4457f9e36ed05be2df1ac75', + 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef' + ); + const txPrebuild = { ...erc7984Prebuild, txHex: tamperedTxHex }; + + const verification = {}; + + await basecoin + .verifyTransaction({ txParams, txPrebuild, wallet, verification }) + .should.be.rejectedWith('wrapApprove transaction spender is not a registered ERC-7984 wrapper'); + }); + + it('still demands wrapParams for the direct-sign wrapApprove path (no PA-rebuild)', async () => { + const wallet = new Wallet(bitgo, basecoin, {}); + + const txParams = { + type: 'wrapApprove', + recipients: [], + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const verification = {}; + + await basecoin + .verifyTransaction({ txParams, txPrebuild: erc7984Prebuild, wallet, verification }) + .should.be.rejectedWith('wrapApprove transaction requires wrapParams with tokenName and amount'); + }); + + it('still cross-checks the approve amount against wrapParams on the direct-sign path', async () => { + const wallet = new Wallet(bitgo, basecoin, {}); + + const txParams = { + type: 'wrapApprove', + wrapParams: { tokenName: 'hteth:cusdt', amount: '999' }, + recipients: [], + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const verification = {}; + + await basecoin + .verifyTransaction({ txParams, txPrebuild: erc7984Prebuild, wallet, verification }) + .should.be.rejectedWith('wrapApprove transaction amount does not match the requested amount'); + }); + + it('cross-checks wrapParams against the tx even when a pendingApprovalId is present', async () => { + const wallet = new Wallet(bitgo, basecoin, {}); + + const txParams = { + pendingApprovalId: '6ae3233a7d9bc0b7b3f187a165996e15', + type: 'wrapApprove', + wrapParams: { tokenName: 'hteth:cusdt', amount: '1000000' }, + recipients: [], + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const verification = {}; + + const isTransactionVerified = await basecoin.verifyTransaction({ + txParams, + txPrebuild: erc7984Prebuild, + wallet, + verification, + }); + isTransactionVerified.should.equal(true); + }); + + it('rejects a mismatched wrapParams tokenName even when a pendingApprovalId is present', async () => { + const wallet = new Wallet(bitgo, basecoin, {}); + + const txParams = { + pendingApprovalId: '6ae3233a7d9bc0b7b3f187a165996e15', + type: 'wrapApprove', + wrapParams: { tokenName: 'hteth:notreal', amount: '1000000' }, + recipients: [], + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const verification = {}; + + await basecoin + .verifyTransaction({ txParams, txPrebuild: erc7984Prebuild, wallet, verification }) + .should.be.rejectedWith("wrapApprove: 'hteth:notreal' is not a registered ERC-7984 shield pair"); + }); + + it('rejects a zero approve amount on the PA-rebuild path', async () => { + const wallet = new Wallet(bitgo, basecoin, {}); + + const txParams = { + pendingApprovalId: '6ae3233a7d9bc0b7b3f187a165996e15', + type: 'wrapApprove', + recipients: [], + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const amountWord = '00000000000000000000000000000000000000000000000000000000000f4240'; + const zeroAmountTxHex = erc7984WrapApproveTxHex.replace(amountWord, '0'.repeat(64)); + const txPrebuild = { ...erc7984Prebuild, txHex: zeroAmountTxHex }; + + const verification = {}; + + await basecoin + .verifyTransaction({ txParams, txPrebuild, wallet, verification }) + .should.be.rejectedWith( + 'wrapApprove transaction amount must be a positive integer and not the unlimited allowance' + ); + }); + + it('rejects the unlimited (MAX_UINT256) approve amount on the PA-rebuild path', async () => { + const wallet = new Wallet(bitgo, basecoin, {}); + + const txParams = { + pendingApprovalId: '6ae3233a7d9bc0b7b3f187a165996e15', + type: 'wrapApprove', + recipients: [], + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const amountWord = '00000000000000000000000000000000000000000000000000000000000f4240'; + const unlimitedAmountTxHex = erc7984WrapApproveTxHex.replace(amountWord, 'f'.repeat(64)); + const txPrebuild = { ...erc7984Prebuild, txHex: unlimitedAmountTxHex }; + + const verification = {}; + + await basecoin + .verifyTransaction({ txParams, txPrebuild, wallet, verification }) + .should.be.rejectedWith( + 'wrapApprove transaction amount must be a positive integer and not the unlimited allowance' + ); + }); + + it('rejects non-zero native value inside sendMultiSig on the PA-rebuild path', async () => { + const wallet = new Wallet(bitgo, basecoin, {}); + + const txParams = { + pendingApprovalId: '6ae3233a7d9bc0b7b3f187a165996e15', + type: 'wrapApprove', + recipients: [], + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const innerToWord = '00000000000000000000000051a63b5621d78de54d2f4d098a23a5a69e76f30b'; + const tamperedTxHex = erc7984WrapApproveTxHex.replace( + innerToWord + '0'.repeat(64), + innerToWord + '0'.repeat(63) + '1' + ); + const txPrebuild = { ...erc7984Prebuild, txHex: tamperedTxHex }; + + const verification = {}; + + await basecoin + .verifyTransaction({ txParams, txPrebuild, wallet, verification }) + .should.be.rejectedWith('wrapApprove transaction must not carry native value inside sendMultiSig'); + }); + + it('rejects a sendMultiSig target that is not the pair underlying on the PA-rebuild path', async () => { + const wallet = new Wallet(bitgo, basecoin, {}); + + const txParams = { + pendingApprovalId: '6ae3233a7d9bc0b7b3f187a165996e15', + type: 'wrapApprove', + recipients: [], + wallet: wallet, + walletPassphrase: 'fakeWalletPassphrase', + }; + + const innerToWord = '00000000000000000000000051a63b5621d78de54d2f4d098a23a5a69e76f30b'; + const tamperedTxHex = erc7984WrapApproveTxHex.replace( + innerToWord, + '000000000000000000000000deadbeefdeadbeefdeadbeefdeadbeefdeadbeef' + ); + const txPrebuild = { ...erc7984Prebuild, txHex: tamperedTxHex }; + + const verification = {}; + + await basecoin + .verifyTransaction({ txParams, txPrebuild, wallet, verification }) + .should.be.rejectedWith('wrapApprove transaction must target the underlying ERC-20 of the wrapper'); + }); }); }