From b817bf94cd30935a9068b39b50e4027057de12b1 Mon Sep 17 00:00:00 2001 From: Yashvanth B L Date: Mon, 2 Nov 2026 17:39:15 +0530 Subject: [PATCH] fix(sdk-coin-atom): normalize memo recipients in verifyTransaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CosmosCoin.verifyTransaction compared txParams.recipients addresses as raw strings against explained outputs, which report the plain on-chain address plus a tx-level memo. Cosmos destinations represent their memo in more than one way across clients: the enterprise UI and wallet APIs fold it into the address as address?memoId=X, while the retail UI sends a plain address plus a tx-level memo param. The base class failed for the combined form (every withdrawal to a memo-required destination failed with 'Tx outputs does not match with expected txParams recipients', COINS-2285) while silently ignoring memo mismatches in every other form. Normalize both sides in sdk-coin-atom before comparing. A recipient's memo may be declared via the address ?memoId suffix, a recipient.memo field, or the tx-level txParams.memo.value; all declared sources must agree, and empty strings count as absent. A declared memo must match the transaction memo exactly; when no memo is declared anywhere, the transaction must not carry one either — a memo the user never approved is a prebuild-integrity failure. To keep the strict check intact on re-signs, resolveEffectiveTxParams now propagates the persisted intent.memo into effective txParams when the caller supplies none: signing paths that carry no txParams (pendingApproval.approve() -> recreateTxRequest() -> signTxRequest()) otherwise reached the coin verifier with a memo-less txParams while the prebuilt transaction carried the memo approved at send time. Ports the per-coin fix precedent from sdk-coin-tia (COINS-1760) and sdk-coin-zeta (CSHLD-1780) and declares the lodash dependency they use. Ticket: COINS-2285 --- modules/sdk-coin-atom/package.json | 6 +- modules/sdk-coin-atom/src/atom.ts | 76 +++++++- modules/sdk-coin-atom/test/unit/atom.ts | 182 +++++++++++++++++- .../src/bitgo/utils/tss/recipientUtils.ts | 9 + .../unit/bitgo/utils/tss/recipientUtils.ts | 38 ++++ 5 files changed, 307 insertions(+), 4 deletions(-) diff --git a/modules/sdk-coin-atom/package.json b/modules/sdk-coin-atom/package.json index 2b1b97d1517..697c87648e2 100644 --- a/modules/sdk-coin-atom/package.json +++ b/modules/sdk-coin-atom/package.json @@ -47,12 +47,14 @@ "@cosmjs/amino": "^0.29.5", "@cosmjs/encoding": "^0.29.5", "@cosmjs/stargate": "^0.29.5", - "bignumber.js": "^9.1.1" + "bignumber.js": "^9.1.1", + "lodash": "^4.18.0" }, "devDependencies": { "@bitgo/sdk-api": "^3.1.6", "@bitgo/sdk-test": "^9.1.91", - "axios": "1.18.0" + "axios": "1.18.0", + "@types/lodash": "^4.14.183" }, "gitHead": "18e460ddf02de2dbf13c2aa243478188fb539f0c", "files": [ diff --git a/modules/sdk-coin-atom/src/atom.ts b/modules/sdk-coin-atom/src/atom.ts index 29d6d01bac8..00356abcd7f 100644 --- a/modules/sdk-coin-atom/src/atom.ts +++ b/modules/sdk-coin-atom/src/atom.ts @@ -1,5 +1,7 @@ import { CosmosCoin, CosmosKeyPair, GasAmountDetails } from '@bitgo/abstract-cosmos'; -import { BaseCoin, BitGoBase, Environments } from '@bitgo/sdk-core'; +import _ from 'lodash'; +import BigNumber from 'bignumber.js'; +import { BaseCoin, BitGoBase, Environments, TransactionType, VerifyTransactionOptions } from '@bitgo/sdk-core'; import { BaseUnit, BaseCoin as StaticsBaseCoin, coins } from '@bitgo/statics'; import { KeyPair, TransactionBuilderFactory } from './lib'; import { GAS_AMOUNT, GAS_LIMIT } from './lib/constants'; @@ -63,4 +65,76 @@ export class Atom extends CosmosCoin { getAddressFromPublicKey(pubKey: string): string { return new KeyPair({ pub: pubKey }).getAddress(); } + + /** + * @inheritDoc + * + * Overrides CosmosCoin.verifyTransaction because a recipient's memo can be expressed in + * several ways, while explainTransaction() reports the plain on-chain address plus a single + * tx-level `memo` field (stamped on every output): + * 1. folded into the address as `address?memoId=X` (see getAddressDetails) — the form the + * enterprise UI and wallet APIs send for cosmos destinations requiring a memo; + * 2. a `recipient.memo` field (ITransactionRecipient.memo); + * 3. a tx-level `txParams.memo` ({value, type}) — the form the retail UI sends for cosmos + * destinations (plain address plus tx-level memo). + * The base class compares raw address strings, so the combined form fails verification even + * when the transaction was built correctly (reproduced as COINS-2285: every ATOM withdrawal + * to a memo-required destination failed with 'Tx outputs does not match with expected + * txParams recipients'). Normalize both sides to the same {address, amount, memo?} shape + * before comparing. The expected memo is derived solely from txParams (user intent) — never + * from the prebuild. A declared memo must match the transaction memo exactly; empty-string + * memos are treated as absent, and conflicting distinct memo declarations are rejected as + * ambiguous intent. When no memo is declared anywhere in txParams, the transaction must not + * carry one either — a memo the user never approved is a prebuild-integrity failure. + * Intent-based re-signs that would otherwise lose the tx-level memo get it restored into + * txParams.memo by resolveEffectiveTxParams (sdk-core recipientUtils). + */ + async verifyTransaction(params: VerifyTransactionOptions): Promise { + let totalAmount = new BigNumber(0); + const { txPrebuild, txParams } = params; + const rawTx = txPrebuild.txHex; + if (!rawTx) { + throw new Error('missing required tx prebuild property txHex'); + } + const transaction = await this.getBuilder().from(rawTx).build(); + const explainedTx = transaction.explainTransaction(); + if (txParams.recipients && txParams.recipients.length > 0) { + const filteredRecipients = txParams.recipients.map((recipient) => { + const { address, memoId } = this.getAddressDetails(recipient.address); + // A recipient's memo may be declared via the address `?memoId=` suffix, a `recipient.memo` + // field, or the tx-level `txParams.memo.value`. Empty strings mean "no memo" (the UI and + // wallet APIs send memo: '' when the field is unused, and explainTransaction() omits the + // memo entirely for memo-less transactions). Conflicting distinct declarations are an + // ambiguous intent and are rejected rather than silently resolved by precedence. + const declaredMemos = [memoId, recipient.memo, txParams.memo?.value].filter( + (memo): memo is string => memo !== undefined && memo !== '' + ); + if (declaredMemos.some((memo) => memo !== declaredMemos[0])) { + throw new Error('Tx outputs does not match with expected txParams recipients'); + } + const memo = declaredMemos[0]; + return { address, amount: recipient.amount, ...(memo !== undefined ? { memo } : {}) }; + }); + // explainTransaction() stamps the tx-level memo on every output. A declared memo must + // match it exactly; when no memo is declared anywhere in txParams, the tx must not carry + // one either — a memo the user never approved is a prebuild-integrity failure. Intent-based + // re-signs that would otherwise lose the memo get it restored into txParams.memo by + // resolveEffectiveTxParams (see sdk-core recipientUtils). + const filteredOutputs = explainedTx.outputs.map((output) => _.pick(output, ['address', 'amount', 'memo'])); + + if (!_.isEqual(filteredOutputs, filteredRecipients)) { + throw new Error('Tx outputs does not match with expected txParams recipients'); + } + // WithdrawDelegatorRewards and ContractCall transaction don't have amount + if (transaction.type !== TransactionType.StakingWithdraw && transaction.type !== TransactionType.ContractCall) { + for (const recipient of txParams.recipients) { + totalAmount = totalAmount.plus(recipient.amount); + } + if (!totalAmount.isEqualTo(explainedTx.outputAmount)) { + throw new Error('Tx total amount does not match with expected total amount field'); + } + } + } + return true; + } } diff --git a/modules/sdk-coin-atom/test/unit/atom.ts b/modules/sdk-coin-atom/test/unit/atom.ts index ae8a7d5c481..2a9e71d1f96 100644 --- a/modules/sdk-coin-atom/test/unit/atom.ts +++ b/modules/sdk-coin-atom/test/unit/atom.ts @@ -3,7 +3,7 @@ import { BitGoAPI } from '@bitgo/sdk-api'; import { EcdsaRangeProof, EcdsaTypes } from '@bitgo/sdk-lib-mpc'; import { TestBitGo, TestBitGoAPI, mockSerializedChallengeWithProofs } from '@bitgo/sdk-test'; import { coins } from '@bitgo/statics'; -import { ErrorNoInputToRecover } from '@bitgo/sdk-core'; +import { ErrorNoInputToRecover, InvalidAddressError, resolveEffectiveTxParams } from '@bitgo/sdk-core'; import BigNumber from 'bignumber.js'; import sinon from 'sinon'; import { Atom, Tatom } from '../../src'; @@ -196,6 +196,186 @@ describe('ATOM', function () { isTransactionVerified.should.equal(true); }); + it('should succeed to verify transaction with a memo-suffixed recipient address', async function () { + const txPrebuild = { txHex: TEST_TX_WITH_MEMO.signedTxBase64, txInfo: {} }; + const txParams = { + recipients: [ + { + address: `${TEST_TX_WITH_MEMO.to}?memoId=${TEST_TX_WITH_MEMO.memo}`, + amount: TEST_TX_WITH_MEMO.sendAmount, + }, + ], + }; + const isTransactionVerified = await basecoin.verifyTransaction({ txParams, txPrebuild, verification: {} }); + isTransactionVerified.should.equal(true); + }); + + it('should fail to verify transaction when the recipient memoId does not match the tx memo', async function () { + const txPrebuild = { txHex: TEST_TX_WITH_MEMO.signedTxBase64, txInfo: {} }; + const txParams = { + recipients: [{ address: `${TEST_TX_WITH_MEMO.to}?memoId=999999`, amount: TEST_TX_WITH_MEMO.sendAmount }], + }; + await basecoin + .verifyTransaction({ txParams, txPrebuild, verification: {} }) + .should.be.rejectedWith('Tx outputs does not match with expected txParams recipients'); + }); + + it('should fail to verify transaction when no memo is declared but the tx carries one', async function () { + // A memo the user never approved is a prebuild-integrity failure. Intent-based re-signs + // that carry the memo only in the persisted intent get it restored into txParams.memo by + // resolveEffectiveTxParams (sdk-core recipientUtils), so legitimate flows still declare it. + const txPrebuild = { txHex: TEST_TX_WITH_MEMO.signedTxBase64, txInfo: {} }; + const txParams = { + recipients: [{ address: TEST_TX_WITH_MEMO.to, amount: TEST_TX_WITH_MEMO.sendAmount }], + }; + await basecoin + .verifyTransaction({ txParams, txPrebuild, verification: {} }) + .should.be.rejectedWith('Tx outputs does not match with expected txParams recipients'); + }); + + it('should succeed to verify a memo transaction re-signed from a persisted intent (pendingApproval re-sign path)', async function () { + // Composes the two halves of the fix exactly as the TSS signing path does: the retail UI + // sends a plain recipient address plus a tx-level memo, populateIntent persists them as + // intent.recipients (bare address) and intent.memo, and resolveEffectiveTxParams restores + // both into the effective txParams used for verification. + const txPrebuild = { txHex: TEST_TX_WITH_MEMO.signedTxBase64, txInfo: {} }; + const txRequest = { + intent: { + intentType: 'payment', + recipients: [ + { + address: { address: TEST_TX_WITH_MEMO.to }, + amount: { value: TEST_TX_WITH_MEMO.sendAmount, symbol: 'tatom' }, + }, + ], + memo: TEST_TX_WITH_MEMO.memo, + }, + }; + const effectiveTxParams = resolveEffectiveTxParams( + txRequest as Parameters[0], + undefined + ); + const isTransactionVerified = await basecoin.verifyTransaction({ + txParams: effectiveTxParams, + txPrebuild, + verification: {}, + }); + isTransactionVerified.should.equal(true); + }); + + it('should fail to verify a memo transaction re-signed with a mismatched intent memo', async function () { + // The propagated intent memo is user intent, so it is checked strictly, not blindly accepted. + const txPrebuild = { txHex: TEST_TX_WITH_MEMO.signedTxBase64, txInfo: {} }; + const txRequest = { + intent: { + intentType: 'payment', + recipients: [ + { + address: { address: TEST_TX_WITH_MEMO.to }, + amount: { value: TEST_TX_WITH_MEMO.sendAmount, symbol: 'tatom' }, + }, + ], + memo: '999999', + }, + }; + const effectiveTxParams = resolveEffectiveTxParams( + txRequest as Parameters[0], + undefined + ); + await basecoin + .verifyTransaction({ txParams: effectiveTxParams, txPrebuild, verification: {} }) + .should.be.rejectedWith('Tx outputs does not match with expected txParams recipients'); + }); + + it('should fail to verify transaction with a malformed memo suffix in the recipient address', async function () { + // getAddressDetails rejects query strings other than ?memoId= — a malformed destination + // must fail loudly rather than fall through to a bare-address comparison. + const txPrebuild = { txHex: TEST_TX_WITH_MEMO.signedTxBase64, txInfo: {} }; + const txParams = { + recipients: [{ address: `${TEST_TX_WITH_MEMO.to}?foo=bar`, amount: TEST_TX_WITH_MEMO.sendAmount }], + }; + await basecoin + .verifyTransaction({ txParams, txPrebuild, verification: {} }) + .should.be.rejectedWith(InvalidAddressError); + }); + + it('should succeed to verify transaction when the memo is provided as a tx-level param', async function () { + const txPrebuild = { txHex: TEST_TX_WITH_MEMO.signedTxBase64, txInfo: {} }; + const txParams = { + recipients: [{ address: TEST_TX_WITH_MEMO.to, amount: TEST_TX_WITH_MEMO.sendAmount }], + memo: { value: TEST_TX_WITH_MEMO.memo, type: 'text' }, + }; + const isTransactionVerified = await basecoin.verifyTransaction({ txParams, txPrebuild, verification: {} }); + isTransactionVerified.should.equal(true); + }); + + it('should succeed to verify transaction when the memo is provided on the recipient', async function () { + const txPrebuild = { txHex: TEST_TX_WITH_MEMO.signedTxBase64, txInfo: {} }; + const txParams = { + recipients: [ + { + address: TEST_TX_WITH_MEMO.to, + amount: TEST_TX_WITH_MEMO.sendAmount, + memo: TEST_TX_WITH_MEMO.memo, + }, + ], + }; + const isTransactionVerified = await basecoin.verifyTransaction({ txParams, txPrebuild, verification: {} }); + isTransactionVerified.should.equal(true); + }); + + it('should fail to verify transaction when the tx-level memo does not match the tx memo', async function () { + const txPrebuild = { txHex: TEST_TX_WITH_MEMO.signedTxBase64, txInfo: {} }; + const txParams = { + recipients: [{ address: TEST_TX_WITH_MEMO.to, amount: TEST_TX_WITH_MEMO.sendAmount }], + memo: { value: '999999', type: 'text' }, + }; + await basecoin + .verifyTransaction({ txParams, txPrebuild, verification: {} }) + .should.be.rejectedWith('Tx outputs does not match with expected txParams recipients'); + }); + + it('should succeed to verify transaction when the tx-level memo is empty and the tx has no memo', async function () { + const txPrebuild = { txHex: TEST_SEND_TX.signedTxBase64, txInfo: {} }; + const txParams = { + recipients: [{ address: TEST_SEND_TX.recipient, amount: TEST_SEND_TX.sendAmount }], + memo: { value: '', type: 'text' }, + }; + const isTransactionVerified = await basecoin.verifyTransaction({ txParams, txPrebuild, verification: {} }); + isTransactionVerified.should.equal(true); + }); + + it('should succeed to verify transaction when memo sources agree with the tx memo', async function () { + const txPrebuild = { txHex: TEST_TX_WITH_MEMO.signedTxBase64, txInfo: {} }; + const txParams = { + recipients: [ + { + address: `${TEST_TX_WITH_MEMO.to}?memoId=${TEST_TX_WITH_MEMO.memo}`, + amount: TEST_TX_WITH_MEMO.sendAmount, + memo: TEST_TX_WITH_MEMO.memo, + }, + ], + memo: { value: TEST_TX_WITH_MEMO.memo, type: 'text' }, + }; + const isTransactionVerified = await basecoin.verifyTransaction({ txParams, txPrebuild, verification: {} }); + isTransactionVerified.should.equal(true); + }); + + it('should fail to verify transaction when memo sources conflict', async function () { + const txPrebuild = { txHex: TEST_TX_WITH_MEMO.signedTxBase64, txInfo: {} }; + const txParams = { + recipients: [ + { + address: `${TEST_TX_WITH_MEMO.to}?memoId=${TEST_TX_WITH_MEMO.memo}`, + amount: TEST_TX_WITH_MEMO.sendAmount, + }, + ], + memo: { value: '999999', type: 'text' }, + }; + await basecoin + .verifyTransaction({ txParams, txPrebuild, verification: {} }) + .should.be.rejectedWith('Tx outputs does not match with expected txParams recipients'); + }); it('should fail to verify transaction with invalid param', async function () { const txPrebuild = {}; const txParams = { recipients: undefined }; diff --git a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts index f33d44b9b05..b9cde0bfb83 100644 --- a/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/src/bitgo/utils/tss/recipientUtils.ts @@ -189,6 +189,15 @@ export function resolveEffectiveTxParams( effectiveTxParams.wrapParams = { tokenName: intent.tokenName, amount: intent.amount }; } + // Propagate the persisted tx-level memo from the intent. Signing paths that carry no txParams + // (pendingApproval.approve() -> recreateTxRequest() -> signTxRequest()) would otherwise reach + // the coin verifier with a memo-less txParams while the prebuilt transaction carries the memo + // approved at send time — e.g. the retail UI sends a plain cosmos recipient address plus a + // tx-level memo param, and populateIntent persists it as intent.memo (mpcUtils.ts). + if (!effectiveTxParams.memo && typeof intent?.memo === 'string' && intent.memo !== '') { + effectiveTxParams.memo = { value: intent.memo, type: 'text' }; + } + // All staking intents (BSC delegate/undelegate, CELO stake/unstake, etc.) carry // stakingRequestId as a required field on BaseStakeIntent (@bitgo/public-types). // Use its presence as a generic staking signal — no need to enumerate every intentType. 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 bb6ffbf5680..dfae7cfa03c 100644 --- a/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts +++ b/modules/sdk-core/test/unit/bitgo/utils/tss/recipientUtils.ts @@ -169,6 +169,44 @@ describe('recipientUtils', function () { }); }); + describe('tx-level memo propagation', function () { + it('derives memo from the intent when txParams is undefined (pendingApproval re-sign path)', function () { + const txRequest = makeTxRequest({ + intent: { + intentType: 'payment', + recipients: [{ address: { address: 'cosmos1abc' }, amount: { value: '100', symbol: 'atom' } }], + memo: '153', + } as PopulatedIntent, + }); + const result = resolveEffectiveTxParams(txRequest, undefined); + assert.deepStrictEqual(result.memo, { value: '153', type: 'text' }); + }); + it('does not overwrite memo supplied by the caller', function () { + const txRequest = makeTxRequest({ + intent: { + intentType: 'payment', + recipients: [{ address: { address: 'cosmos1abc' }, amount: { value: '100', symbol: 'atom' } }], + memo: '153', + } as PopulatedIntent, + }); + const result = resolveEffectiveTxParams(txRequest, { memo: { value: '999', type: 'id' } }); + assert.deepStrictEqual(result.memo, { value: '999', type: 'id' }); + }); + + it('does not derive memo when the intent has none or an empty one', function () { + const recipient = { address: { address: 'cosmos1abc' }, amount: { value: '100', symbol: 'atom' } }; + const noMemo = makeTxRequest({ + intent: { intentType: 'payment', recipients: [recipient] } as PopulatedIntent, + }); + assert.strictEqual(resolveEffectiveTxParams(noMemo, undefined).memo, undefined); + + const emptyMemo = makeTxRequest({ + intent: { intentType: 'payment', recipients: [recipient], memo: '' } as PopulatedIntent, + }); + assert.strictEqual(resolveEffectiveTxParams(emptyMemo, undefined).memo, undefined); + }); + }); + it('does not throw for exempt types', function () { for (const txType of [ 'acceleration',