From 4386a267f3ac016a3dfee67f9807fabcf3446c97 Mon Sep 17 00:00:00 2001 From: lyambo Date: Fri, 28 Aug 2026 15:13:26 -0400 Subject: [PATCH 1/3] Bind prePublishRaw fallback to the proposal Verifier.checkTxProposalSignature falls back to txp.prePublishRaw when the locally-rebuilt tx does not match the creator's signature. It only checked that the signature was valid over prePublishRaw, never that prePublishRaw described the proposal being verified. A compromised server could pair a valid (prePublishRaw, proposalSignature) with a tampered destination and the check still passed. Bind prePublishRaw to the proposal. Recover the single field the server mutates at publish -- the Solana recent blockhash or the EVM/XRP nonce -- from prePublishRaw, rebuild the current proposal with that value substituted, and require a byte-for-byte match. Any other difference (destination, amount, from, contract) now fails as SERVER_COMPROMISED. Non-mutable chains (UTXO) reject prePublishRaw outright. The same guard is applied to the server-side publishTx fallback. --- .../bitcore-wallet-client/src/lib/verifier.ts | 74 ++++++++++++++++--- .../test/verifier.test.ts | 58 +++++++++++++++ .../src/lib/chain/btc/index.ts | 5 ++ .../src/lib/chain/eth/index.ts | 4 + .../src/lib/chain/index.ts | 4 + .../src/lib/chain/sol/index.ts | 5 ++ .../src/lib/chain/xrp/index.ts | 4 + .../src/lib/common/utils.ts | 31 +++++++- .../bitcore-wallet-service/src/lib/server.ts | 8 +- .../src/types/chain.d.ts | 1 + .../test/integration/server.test.ts | 72 ++++++++++++++++++ .../src/transactions/eth/index.ts | 9 +++ .../src/transactions/sol/index.ts | 14 ++++ .../src/transactions/xrp/index.ts | 9 +++ .../test/transactions.test.ts | 42 +++++++++++ 15 files changed, 328 insertions(+), 12 deletions(-) diff --git a/packages/bitcore-wallet-client/src/lib/verifier.ts b/packages/bitcore-wallet-client/src/lib/verifier.ts index c81cc8dcc50..45e60871a46 100644 --- a/packages/bitcore-wallet-client/src/lib/verifier.ts +++ b/packages/bitcore-wallet-client/src/lib/verifier.ts @@ -1,7 +1,8 @@ import { BitcoreLib as Bitcore, BitcoreLibCash, - Utils as CWCUtils + Utils as CWCUtils, + Transactions } from '@bitpay-labs/crypto-wallet-core'; import { singleton } from 'preconditions'; import { Constants, Utils } from './common'; @@ -278,14 +279,24 @@ export class Verifier { log.debug(`[TXP ${txp.id}] Regenerating & verifying tx proposal hash -> Hash: ${hash}, Signature: ${txp.proposalSignature}`); const verified = Utils.verifyMessage(hash, txp.proposalSignature, creatorSigningPubKey); - if (!verified && !txp.prePublishRaw) { - log.debug(`[TXP ${txp.id}] Invalid proposal signature, no prePublishRaw to fall back to`); - return false; - } - - if (!verified && txp.prePublishRaw && !Utils.verifyMessage(txp.prePublishRaw, txp.proposalSignature, creatorSigningPubKey)) { - log.debug(`[TXP ${txp.id}] Invalid proposal signature, even with prePublishRaw fallback`); - return false; + if (!verified) { + // Local rebuild != creator's signature. Legit only when BWS mutated a field at publish (SVM recent + // blockhash, or EVM/XRP deferred nonce): the creator signed the pre-publish serialization, stored as + // txp.prePublishRaw. Fall back to it only if the signature is valid over it AND it is bound to this + // proposal -- else a hostile server could pair a valid (prePublishRaw, proposalSignature) with a + // tampered destination. See checkPrePublishRaw. + if (!txp.prePublishRaw) { + log.debug(`[TXP ${txp.id}] Invalid proposal signature, no prePublishRaw to fall back to`); + return false; + } + if (!Utils.verifyMessage(txp.prePublishRaw, txp.proposalSignature, creatorSigningPubKey)) { + log.debug(`[TXP ${txp.id}] Invalid proposal signature, even with prePublishRaw fallback`); + return false; + } + if (!this.checkPrePublishRaw(chain, txp)) { + log.warn(`[TXP ${txp.id}] prePublishRaw is not bound to this proposal; possible server tampering`); + return false; + } } if (Constants.UTXO_CHAINS.includes(chain)) { @@ -305,6 +316,51 @@ export class Verifier { return true; } + /** + * True only if txp.prePublishRaw is the same transaction as the current proposal, differing solely in a + * field BWS mutates at publish (SVM blockhash / EVM-XRP nonce). Binds the creator's fallback signature to + * this proposal: without it a compromised server could pair a valid (prePublishRaw, proposalSignature) + * with a tampered destination/amount. Rejects non-mutable chains and fails closed on any error. + * + * @param {string} chain - lower-cased chain of the proposal + * @param {Object} txp - the transaction proposal (must carry prePublishRaw) + */ + static checkPrePublishRaw(chain, txp) { + // Only chains with a publish-mutable serialized field legitimately carry prePublishRaw: the recent + // blockhash (SVM) or account nonce (EVM/XRP). Anywhere else (e.g. UTXO) its presence is illegitimate. + const canHaveMutableLifetime = [ + ...Constants.SVM_CHAINS, + ...Constants.EVM_CHAINS, + ...Constants.RIPPLE_CHAINS + ].includes(chain); + if (!canHaveMutableLifetime) { + log.warn(`[TXP ${txp.id}] prePublishRaw present on chain ${chain} that cannot mutate at publish; refusing fallback`); + return false; + } + try { + const prePublishRaw = Array.isArray(txp.prePublishRaw) ? txp.prePublishRaw : [txp.prePublishRaw]; + // Recover the mutable field (blockhash / nonce) from the pre-publish serialization the creator signed; + // the stored proposal carries the refreshed value, so it isn't readable off txp directly. + const provider: any = Transactions.get({ chain }); + const mutableFields = provider.getMutableFields(prePublishRaw[0]); + if (!mutableFields || Object.values(mutableFields).every(v => v == null)) { + log.warn(`[TXP ${txp.id}] Could not recover pre-publish mutable fields from prePublishRaw; refusing fallback`); + return false; + } + // Rebuild with the pre-publish mutable field: an untampered proposal reproduces prePublishRaw exactly; + // any changed field (destination, amount, from, contract) serializes differently and fails the compare. + const rebuilt = Utils.buildTx({ ...txp, ...mutableFields }).uncheckedSerialize(); + const rebuiltArr = Array.isArray(rebuilt) ? rebuilt : [rebuilt]; + if (rebuiltArr.length !== prePublishRaw.length) { + return false; + } + return rebuiltArr.every((raw, i) => raw === prePublishRaw[i]); + } catch (err) { + log.warn(`[TXP ${txp.id}] Failed to verify prePublishRaw binding: ${err?.message || err}`); + return false; + } + } + static checkPaypro(txp, payproOpts) { let toAddress, amount; diff --git a/packages/bitcore-wallet-client/test/verifier.test.ts b/packages/bitcore-wallet-client/test/verifier.test.ts index 4dad8ab00e5..f8f1b9e7611 100644 --- a/packages/bitcore-wallet-client/test/verifier.test.ts +++ b/packages/bitcore-wallet-client/test/verifier.test.ts @@ -1,6 +1,7 @@ 'use strict'; import chai from 'chai'; +import { Utils } from '../src/lib/common'; import { Verifier } from '../src/lib/verifier'; import { Key } from '../src/lib/key'; @@ -222,4 +223,61 @@ describe('Verifier', function() { }).should.be.true; }); }); + + describe('checkPrePublishRaw', function() { + const ATTACKER_EVM = '0x1111111111111111111111111111111111111111'; + const ATTACKER_SOL = 'F7FknkRckx4yvA3Gexnx1H3nwPxndMxVt58BwAzEQhcY'; + + const solTxp = (overrides = {}) => ({ + chain: 'sol', + category: 'transfer', + from: '8WyoNvKsmfdG6zrbzNBVN8DETyLra3ond61saU9C52YR', + outputs: [{ toAddress: '3xkNjKm2zvGRvH2z2Nf9Y5hFvHZFtQXbQb1PBxDT56Xy', amount: 3896000000000000 }], + blockHash: 'GtV1Hb3FvP3HURHAsj8mGwEqCumvP3pv3i6CVCzYNj3d', + blockHeight: 531575, + ...overrides + }); + + const evmTxp = (overrides = {}) => ({ + chain: 'eth', + network: 'livenet', + outputs: [{ toAddress: '0x37d7B3bBD88EFdE6a93cF74D2F5b0385D3E3B08A', amount: 1000 }], + from: '0x37d7B3bBD88EFdE6a93cF74D2F5b0385D3E3B08A', + nonce: 0, + gasLimit: 21000, + gasPrice: 20000000000, + data: '0x', + ...overrides + }); + + it('accepts a SOL proposal whose blockhash was refreshed at publish', function() { + const prePublishRaw = Utils.buildTx(solTxp()).uncheckedSerialize(); + const current = solTxp({ blockHash: 'H1oRr1nfr4b6eZjs9Ssn3bxUmcRAjqRxrbTKuwSPZ9mE', prePublishRaw }); + Verifier.checkPrePublishRaw('sol', current).should.be.true; + }); + + it('rejects a SOL proposal whose destination was tampered', function() { + const prePublishRaw = Utils.buildTx(solTxp()).uncheckedSerialize(); + const current = solTxp({ + blockHash: 'H1oRr1nfr4b6eZjs9Ssn3bxUmcRAjqRxrbTKuwSPZ9mE', + outputs: [{ toAddress: ATTACKER_SOL, amount: 3896000000000000 }], + prePublishRaw + }); + Verifier.checkPrePublishRaw('sol', current).should.be.false; + }); + + it('rejects an EVM proposal whose destination was tampered', function() { + const prePublishRaw = Utils.buildTx(evmTxp()).uncheckedSerialize(); + const current = evmTxp({ + nonce: 9, + outputs: [{ toAddress: ATTACKER_EVM, amount: 1000 }], + prePublishRaw + }); + Verifier.checkPrePublishRaw('eth', current).should.be.false; + }); + + it('rejects prePublishRaw on a non-mutable (UTXO) chain', function() { + Verifier.checkPrePublishRaw('btc', { chain: 'btc', prePublishRaw: 'anything' }).should.be.false; + }); + }); }); diff --git a/packages/bitcore-wallet-service/src/lib/chain/btc/index.ts b/packages/bitcore-wallet-service/src/lib/chain/btc/index.ts index a132fad1562..e6cc726a4d0 100644 --- a/packages/bitcore-wallet-service/src/lib/chain/btc/index.ts +++ b/packages/bitcore-wallet-service/src/lib/chain/btc/index.ts @@ -349,6 +349,11 @@ export class BtcChain implements IChain { }); } + isPrePublishRawBound(_txp: TxProposal) { + // UTXO chains never mutate tx data at publish, so they never legitimately carry prePublishRaw. + return false; + } + getBitcoreTx(txp, opts = { signed: true }) { const t = new this.bitcoreLib.Transaction(); diff --git a/packages/bitcore-wallet-service/src/lib/chain/eth/index.ts b/packages/bitcore-wallet-service/src/lib/chain/eth/index.ts index 7296a662e96..46eaa859db8 100644 --- a/packages/bitcore-wallet-service/src/lib/chain/eth/index.ts +++ b/packages/bitcore-wallet-service/src/lib/chain/eth/index.ts @@ -287,6 +287,10 @@ export class EthChain implements IChain { }); } + isPrePublishRawBound(txp: TxProposal) { + return Common.Utils.isPrePublishRawBound(this, txp); + } + getBitcoreTx(txp: TxProposal, opts = { signed: true }) { const { data, diff --git a/packages/bitcore-wallet-service/src/lib/chain/index.ts b/packages/bitcore-wallet-service/src/lib/chain/index.ts index 76995c274ae..b0b6bc8dc1d 100644 --- a/packages/bitcore-wallet-service/src/lib/chain/index.ts +++ b/packages/bitcore-wallet-service/src/lib/chain/index.ts @@ -98,6 +98,10 @@ class ChainProxy { return this.get(txp.chain).getBitcoreTx(txp, { signed: opts.signed }); } + isPrePublishRawBound(txp: TxProposal) { + return this.get(txp.chain).isPrePublishRawBound(txp); + } + convertFeePerKb(chain, p, feePerKb) { return this.get(chain).convertFeePerKb(p, feePerKb); } diff --git a/packages/bitcore-wallet-service/src/lib/chain/sol/index.ts b/packages/bitcore-wallet-service/src/lib/chain/sol/index.ts index cb2e00f94f1..5f05b904d72 100644 --- a/packages/bitcore-wallet-service/src/lib/chain/sol/index.ts +++ b/packages/bitcore-wallet-service/src/lib/chain/sol/index.ts @@ -2,6 +2,7 @@ import { Transactions, Utils, Validation } from '@bitpay-labs/crypto-wallet-core import _ from 'lodash'; import { IChain } from '../../../types/chain'; import { WalletWithOpts } from '../../blockchainexplorers/v8'; +import { Common } from '../../common'; import { Defaults } from '../../common/defaults'; import { Errors } from '../../errors/errordefinitions'; import logger from '../../logger'; @@ -98,6 +99,10 @@ export class SolChain implements IChain { }); } + isPrePublishRawBound(txp) { + return Common.Utils.isPrePublishRawBound(this, txp); + } + getBitcoreTx(txp, opts = { signed: true }) { const { data, diff --git a/packages/bitcore-wallet-service/src/lib/chain/xrp/index.ts b/packages/bitcore-wallet-service/src/lib/chain/xrp/index.ts index b20ab8fe475..0ee767dc07f 100644 --- a/packages/bitcore-wallet-service/src/lib/chain/xrp/index.ts +++ b/packages/bitcore-wallet-service/src/lib/chain/xrp/index.ts @@ -143,6 +143,10 @@ export class XrpChain implements IChain { }); } + isPrePublishRawBound(txp) { + return Common.Utils.isPrePublishRawBound(this, txp); + } + getBitcoreTx(txp, opts = { signed: true }) { const { destinationTag, outputs, outputOrder, multiTx } = txp; const chain = 'XRP'; diff --git a/packages/bitcore-wallet-service/src/lib/common/utils.ts b/packages/bitcore-wallet-service/src/lib/common/utils.ts index c49bd14716f..c5a7f682328 100644 --- a/packages/bitcore-wallet-service/src/lib/common/utils.ts +++ b/packages/bitcore-wallet-service/src/lib/common/utils.ts @@ -3,7 +3,8 @@ import { BitcoreLibCash, BitcoreLibDoge, BitcoreLibLtc, - Constants as CWConstants + Constants as CWConstants, + Transactions } from '@bitpay-labs/crypto-wallet-core'; import _ from 'lodash'; import { singleton } from 'preconditions'; @@ -23,6 +24,34 @@ const Bitcore_ = { export const Utils = { + /** + * Shared implementation of IChain.isPrePublishRawBound for account-based chains (SVM/EVM/XRP). True only if + * txp.prePublishRaw is the same transaction as the current proposal, differing solely in the field BWS + * mutates at publish (blockhash on SVM, nonce on EVM/XRP). Recovers that field from prePublishRaw, rebuilds + * the proposal via the chain's own getBitcoreTx, and requires byte-for-byte equality -- so a tampered + * stored proposal cannot reuse an old, still-valid proposalSignature. Fails closed on any error. + * + * @param chain - the IChain implementation (provides chain + getBitcoreTx) + * @param txp - the transaction proposal (must carry prePublishRaw) + */ + isPrePublishRawBound(chain, txp): boolean { + try { + const prePublishRaw = Array.isArray(txp.prePublishRaw) ? txp.prePublishRaw : [txp.prePublishRaw]; + const provider = Transactions.get({ chain: txp.chain }) as any; + if (typeof provider?.getMutableFields !== 'function') return false; + const mutableFields = provider.getMutableFields(prePublishRaw[0]); + if (!mutableFields || Object.values(mutableFields).every(v => v == null)) return false; + const cloned = Object.assign(Object.create(Object.getPrototypeOf(txp)), txp, mutableFields); + const rebuilt = chain.getBitcoreTx(cloned).uncheckedSerialize(); + const rebuiltArr = Array.isArray(rebuilt) ? rebuilt : [rebuilt]; + if (rebuiltArr.length !== prePublishRaw.length) return false; + return rebuiltArr.every((raw, i) => raw === prePublishRaw[i]); + } catch (err) { + logger.warn('prePublishRaw binding check failed: %o', err); + return false; + } + }, + /** * @deprecated * Moved from ChainService to prevent circular dependency diff --git a/packages/bitcore-wallet-service/src/lib/server.ts b/packages/bitcore-wallet-service/src/lib/server.ts index 264fbd71691..7c66aedeb59 100644 --- a/packages/bitcore-wallet-service/src/lib/server.ts +++ b/packages/bitcore-wallet-service/src/lib/server.ts @@ -3063,8 +3063,12 @@ export class WalletService implements IWalletService { let signingKey = this._getSigningKey(raw, opts.proposalSignature, copayer.requestPubKeys); if (!signingKey) { - // If the txp has been published previously, we will verify the signature against the previously published raw tx - if (txp.hasMutableTxData() && txp.prePublishRaw) { + // The signature didn't match the current (refreshed) raw tx. Fall back to the previously-published + // raw tx -- but only if it is bound to THIS proposal (differs from the current tx solely in the + // field BWS mutates at publish: blockhash/nonce). Without that check a tampered stored proposal + // could reuse an old, still-valid proposalSignature over prePublishRaw. Mirrors the client + // Verifier.checkPrePublishRaw guard. + if (txp.hasMutableTxData() && txp.prePublishRaw && ChainService.isPrePublishRawBound(txp)) { raw = txp.prePublishRaw; signingKey = this._getSigningKey(raw, opts.proposalSignature, copayer.requestPubKeys); } diff --git a/packages/bitcore-wallet-service/src/types/chain.d.ts b/packages/bitcore-wallet-service/src/types/chain.d.ts index 2354e937126..5e54927eea2 100644 --- a/packages/bitcore-wallet-service/src/types/chain.d.ts +++ b/packages/bitcore-wallet-service/src/types/chain.d.ts @@ -33,6 +33,7 @@ export interface IChain { checkScriptOutput(output: { script: string; amount: number }); getFee(server: WalletService, wallet: IWallet, opts: { fee: number; feePerKb: number; signatures?: number } & any); getBitcoreTx(txp: TxProposal, opts: { signed: boolean }); + isPrePublishRawBound(txp: TxProposal): boolean; convertFeePerKb(p: number, feePerKb: number); checkTx(server: WalletService, txp: ITxProposal); checkTxUTXOs(server: WalletService, txp: ITxProposal, opts: { noCashAddr: boolean } & any, cb); diff --git a/packages/bitcore-wallet-service/test/integration/server.test.ts b/packages/bitcore-wallet-service/test/integration/server.test.ts index 7cd974da4c1..c98da1f81f3 100644 --- a/packages/bitcore-wallet-service/test/integration/server.test.ts +++ b/packages/bitcore-wallet-service/test/integration/server.test.ts @@ -8319,6 +8319,64 @@ describe('Wallet service', function() { result.nonce.should.equal(5); // future: result.gasPrice, result.maxFee would also be set here }); + + describe('prePublishRaw binding (isPrePublishRawBound)', function() { + const ATTACKER_ADDR = '0x1111111111111111111111111111111111111111'; + + it('accepts prePublishRaw when only the mutable field (nonce) changed', async function() { + blockchainExplorer.getTransactionCount = sinon.stub().callsArgWith(1, null, '42'); + const created = await helpers.createAndPublishTx(server, { + outputs: [{ toAddress: ETH_ADDR, amount: 8000 }], + feePerKb: 123e2, from: fromAddr, deferNonce: true + }, TestData.copayers[0].privKey_1H_0); + + // Server assigns the JIT nonce -> the stored tx now differs from prePublishRaw only in the nonce. + await util.promisify(server.prepareTx).call(server, { txProposalId: created.id }); + const withNonce = await util.promisify(server.getTx).call(server, { txProposalId: created.id }); + should.exist(withNonce.prePublishRaw); + withNonce.nonce.should.equal(42); + + ChainService.isPrePublishRawBound(withNonce).should.equal(true); + }); + + it('rejects prePublishRaw when the destination was tampered', async function() { + const created = await helpers.createAndPublishTx(server, { + outputs: [{ toAddress: ETH_ADDR, amount: 8000 }], + feePerKb: 123e2, from: fromAddr, deferNonce: true + }, TestData.copayers[0].privKey_1H_0); + + const txp = await util.promisify(server.getTx).call(server, { txProposalId: created.id }); + should.exist(txp.prePublishRaw); + ChainService.isPrePublishRawBound(txp).should.equal(true); // bound before tampering + txp.outputs[0].toAddress = ATTACKER_ADDR; // swap payee, keep prePublishRaw + proposalSignature + ChainService.isPrePublishRawBound(txp).should.equal(false); + }); + + it('rejects the publishTx fallback when the stored proposal was tampered', async function() { + blockchainExplorer.getTransactionCount = sinon.stub().callsArgWith(1, null, '7'); + const created = await util.promisify(server.createTx).call(server, { + outputs: [{ toAddress: ETH_ADDR, amount: 8000 }], + feePerKb: 123e2, from: fromAddr, deferNonce: true + }); + const publishOpts = helpers.getProposalSignatureOpts(created, TestData.copayers[0].privKey_1H_0); + await util.promisify(server.publishTx).call(server, publishOpts); + + // Assign the nonce so a re-publish must fall back to prePublishRaw... + await util.promisify(server.prepareTx).call(server, { txProposalId: created.id }); + // ...then tamper the stored destination and replay the original (still-valid) proposalSignature. + const stored = await util.promisify(server.storage.fetchTx).call(server.storage, wallet.id, created.id); + should.exist(stored.prePublishRaw); + stored.outputs[0].toAddress = ATTACKER_ADDR; + await util.promisify(server.storage.storeTx).call(server.storage, wallet.id, stored); + + let err; + try { + await util.promisify(server.publishTx).call(server, publishOpts); + } catch (e) { err = e; } + should.exist(err); + err.message.should.contain('Invalid proposal signature'); + }); + }); }); describe('#prepareTx XRP (deferred nonce)', function() { @@ -8410,6 +8468,20 @@ describe('Wallet service', function() { }); signed.status.should.equal('accepted'); }); + + it('rejects prePublishRaw binding when an XRP destination was tampered', async function() { + const ATTACKER_XRP = 'rPT1Sjq2YGrBMTttX4GZHjKu9dyfzbpAYe'; + const created = await helpers.createAndPublishTx(server, { + outputs: [{ toAddress: XRP_ADDR, amount: 8000 }], + feePerKb: 123e2, from: fromAddr, deferNonce: true + }, TestData.copayers[0].privKey_1H_0); + + const txp = await util.promisify(server.getTx).call(server, { txProposalId: created.id }); + should.exist(txp.prePublishRaw); + ChainService.isPrePublishRawBound(txp).should.equal(true); // untampered baseline + txp.outputs[0].toAddress = ATTACKER_XRP; + ChainService.isPrePublishRawBound(txp).should.equal(false); + }); }); describe('#signTx nonce override', function() { diff --git a/packages/crypto-wallet-core/src/transactions/eth/index.ts b/packages/crypto-wallet-core/src/transactions/eth/index.ts index c9fa2c49c3e..d7a2c2a28b1 100644 --- a/packages/crypto-wallet-core/src/transactions/eth/index.ts +++ b/packages/crypto-wallet-core/src/transactions/eth/index.ts @@ -127,6 +127,15 @@ export class ETHTxProvider { return ethers.Transaction.from(tx).hash; } + /** + * Reads the fields BWS may mutate between proposal creation and publish out of a raw tx, to bind + * txp.prePublishRaw to the proposal. For EVM that is only the account nonce (deferred-nonce proposals). + */ + getMutableFields(rawTx: string): { nonce?: number } { + const parsed = ethers.Transaction.from(rawTx); + return { nonce: parsed.nonce != null ? Number(parsed.nonce) : undefined }; + } + applySignature(params: { tx: string; signature: any }) { const { tx, signature } = params; const parsedTx = ethers.Transaction.from(tx); diff --git a/packages/crypto-wallet-core/src/transactions/sol/index.ts b/packages/crypto-wallet-core/src/transactions/sol/index.ts index 0f6d78386af..3a894ae2041 100644 --- a/packages/crypto-wallet-core/src/transactions/sol/index.ts +++ b/packages/crypto-wallet-core/src/transactions/sol/index.ts @@ -180,6 +180,20 @@ export class SOLTxProvider { return SolKit.decompileTransactionMessage(compiledTransactionMessage); } + /** + * Reads the fields BWS may mutate between proposal creation and publish out of a raw tx, to bind + * txp.prePublishRaw to the proposal. For Solana that is the recent blockhash (refreshOnPublish), which is + * the compiled message's `lifetimeToken`. We decode only the compiled message (never decompile) so this + * stays safe for txs using Address Lookup Tables -- decompilation would need the on-chain table accounts, + * which a client-side lib cannot fetch. lastValidBlockHeight is not on the wire and does not affect + * serialization, so it is intentionally not recovered. + */ + getMutableFields(rawTx: string): { blockHash?: string } { + const decoded: any = this.decodeRawTransaction({ rawTx, decodeTransactionMessage: false }); + const compiled: any = SolKit.getCompiledTransactionMessageDecoder().decode(decoded.messageBytes); + return { blockHash: compiled?.lifetimeToken }; + } + async sign(params: { tx: string; key: Key }): Promise { const { tx, key } = params; const decodedTx = this.decodeRawTransaction({ rawTx: tx, decodeTransactionMessage: false }); diff --git a/packages/crypto-wallet-core/src/transactions/xrp/index.ts b/packages/crypto-wallet-core/src/transactions/xrp/index.ts index 00f4167cb13..b493c753278 100644 --- a/packages/crypto-wallet-core/src/transactions/xrp/index.ts +++ b/packages/crypto-wallet-core/src/transactions/xrp/index.ts @@ -99,6 +99,15 @@ export class XRPTxProvider { return this.sha512Half(prefix + tx); } + /** + * Reads the fields BWS may mutate between proposal creation and publish out of a raw tx, to bind + * txp.prePublishRaw to the proposal. For XRP that is only the account sequence/nonce (deferred-nonce). + */ + getMutableFields(rawTx: string): { nonce?: number } { + const txJSON = (xrpl.decode(rawTx) as any); + return { nonce: txJSON?.Sequence != null ? Number(txJSON.Sequence) : undefined }; + } + applySignature(params: { tx: string; signature: string; pubKey: string }): string { const { tx, signature, pubKey } = params; const txJSON = (xrpl.decode(tx) as any) as xrpl.Transaction; diff --git a/packages/crypto-wallet-core/test/transactions.test.ts b/packages/crypto-wallet-core/test/transactions.test.ts index 52f12d405d8..5f3335ae570 100644 --- a/packages/crypto-wallet-core/test/transactions.test.ts +++ b/packages/crypto-wallet-core/test/transactions.test.ts @@ -873,6 +873,48 @@ describe('Transaction', function() { }); + describe('getMutableFields', () => { + it('recovers the SOL recent blockhash only (not blockHeight)', () => { + const blockHash = 'GtV1Hb3FvP3HURHAsj8mGwEqCumvP3pv3i6CVCzYNj3d'; + const raw = Transactions.create({ + chain: 'SOL', + category: 'transfer', + from: '8WyoNvKsmfdG6zrbzNBVN8DETyLra3ond61saU9C52YR', + recipients: [{ address: 'F7FknkRckx4yvA3Gexnx1H3nwPxndMxVt58BwAzEQhcY', amount: 3896000000000000 }], + blockHash, + blockHeight: 531575 + }); + const fields = (Transactions.get({ chain: 'SOL' }) as any).getMutableFields(raw); + expect(fields).to.deep.equal({ blockHash }); + }); + + it('recovers the EVM nonce', () => { + const raw = Transactions.create({ + chain: 'ETH', + network: 'livenet', + recipients: [{ address: '0x37d7B3bBD88EFdE6a93cF74D2F5b0385D3E3B08A', amount: 1000 }], + nonce: 7, + gasLimit: 21000, + gasPrice: 20000000000, + data: '0x' + }); + const fields = (Transactions.get({ chain: 'ETH' }) as any).getMutableFields(raw); + expect(fields).to.deep.equal({ nonce: 7 }); + }); + + it('recovers the XRP sequence (nonce)', () => { + const raw = Transactions.create({ + chain: 'XRP', + recipients: [{ address: 'rEqj9WKSH7wEkPvWf6b4gCi26Y3F7HbKUF', amount: '123456' }], + from: 'rEqj9WKSH7wEkPvWf6b4gCi26Y3F7HbKUF', + fee: 12, + nonce: 4 + }); + const fields = (Transactions.get({ chain: 'XRP' }) as any).getMutableFields(raw); + expect(fields).to.deep.equal({ nonce: 4 }); + }); + }); + describe('sign', () => { it('should sign an ETH tx', () => { const signedTx = Transactions.sign({ From 9fa224b7085d54851abb31ac544c5e4936213804 Mon Sep 17 00:00:00 2001 From: lyambo Date: Thu, 3 Sep 2026 17:00:23 -0400 Subject: [PATCH 2/3] Log error string in prePublishRaw binding check The binding check's catch dumped the full error object with %o, which adds noise to production logs. Log the stack or message string instead, matching the pattern used elsewhere in this file. --- packages/bitcore-wallet-service/src/lib/common/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/bitcore-wallet-service/src/lib/common/utils.ts b/packages/bitcore-wallet-service/src/lib/common/utils.ts index c5a7f682328..8d053352e5f 100644 --- a/packages/bitcore-wallet-service/src/lib/common/utils.ts +++ b/packages/bitcore-wallet-service/src/lib/common/utils.ts @@ -47,7 +47,7 @@ export const Utils = { if (rebuiltArr.length !== prePublishRaw.length) return false; return rebuiltArr.every((raw, i) => raw === prePublishRaw[i]); } catch (err) { - logger.warn('prePublishRaw binding check failed: %o', err); + logger.warn('prePublishRaw binding check failed: %o', err.stack || err.message || err); return false; } }, From 1dc68652d0a7afd50f7bf656c7cd5cd414f4c6f0 Mon Sep 17 00:00:00 2001 From: lyambo Date: Tue, 15 Sep 2026 10:44:46 -0400 Subject: [PATCH 3/3] Accept tx proposals only when trust is proven checkTxProposalSignature returned true by falling through past the checks it knew to reject, so any case it did not explicitly handle was treated as trusted. Establish trust affirmatively instead: track a signatureTrusted flag that starts false and is set true only when the signature matches the proposal, directly or via a prePublishRaw bound to it, and end on an explicit address check rather than a fall-through. Also guard checkPrePublishRaw against an empty result set, which would make the byte-for-byte match vacuously true, and add XRP coverage to the checkPrePublishRaw tests. --- .../bitcore-wallet-client/src/lib/verifier.ts | 71 ++++++++++++------- .../test/verifier.test.ts | 26 +++++++ 2 files changed, 71 insertions(+), 26 deletions(-) diff --git a/packages/bitcore-wallet-client/src/lib/verifier.ts b/packages/bitcore-wallet-client/src/lib/verifier.ts index 27981fcd015..1a579abffc9 100644 --- a/packages/bitcore-wallet-client/src/lib/verifier.ts +++ b/packages/bitcore-wallet-client/src/lib/verifier.ts @@ -278,41 +278,59 @@ export class Verifier { log.debug(`[TXP ${txp.id}] Regenerating & verifying tx proposal hash -> Hash: ${hash}, Signature: ${txp.proposalSignature}`); - const verified = Utils.verifyMessage(hash, txp.proposalSignature, creatorSigningPubKey); - if (!verified) { + // Establish signature trust affirmatively: a proposal is trusted only when the creator's signature + // matches the transaction we rebuilt, or (for publish-mutable chains) matches the pre-publish + // serialization that is provably bound to this exact proposal. Every other case leaves it untrusted and + // is rejected -- we never fall through to "trusted" for a situation we didn't explicitly establish. + let signatureTrusted = Utils.verifyMessage(hash, txp.proposalSignature, creatorSigningPubKey); + if (!signatureTrusted) { // Local rebuild != creator's signature. Legit only when BWS mutated a field at publish (SVM recent // blockhash, or EVM/XRP deferred nonce): the creator signed the pre-publish serialization, stored as - // txp.prePublishRaw. Fall back to it only if the signature is valid over it AND it is bound to this - // proposal -- else a hostile server could pair a valid (prePublishRaw, proposalSignature) with a - // tampered destination. See checkPrePublishRaw. + // txp.prePublishRaw. Trust it only if the signature is valid over it AND it is bound to this proposal. if (!txp.prePublishRaw) { log.debug(`[TXP ${txp.id}] Invalid proposal signature, no prePublishRaw to fall back to`); - return false; - } - if (!Utils.verifyMessage(txp.prePublishRaw, txp.proposalSignature, creatorSigningPubKey)) { + } else if (!Utils.verifyMessage(txp.prePublishRaw, txp.proposalSignature, creatorSigningPubKey)) { log.debug(`[TXP ${txp.id}] Invalid proposal signature, even with prePublishRaw fallback`); - return false; - } - if (!this.checkPrePublishRaw(chain, txp)) { + } else if (!this.checkPrePublishRaw(chain, txp)) { log.warn(`[TXP ${txp.id}] prePublishRaw is not bound to this proposal; possible server tampering`); - return false; + } else { + signatureTrusted = true; } } - - if (Constants.UTXO_CHAINS.includes(chain)) { - if (txp.changeAddress && !this.checkAddress(credentials, txp.changeAddress)) { - log.debug(`[TXP ${txp.id}] Invalid change address`); - return false; - } else if (!txp.changeAddress && !txp.sendMax) { - log.warn(`[TXP ${txp.id}] Missing change address for non sendMax transaction proposal`); - return false; - } - if (txp.escrowAddress && !this.checkAddress(credentials, txp.escrowAddress, txp.inputs)) { - log.debug(`[TXP ${txp.id}] Invalid escrow address`); - return false; - } + if (!signatureTrusted) { + return false; } + // Accept only when every required invariant is affirmatively proven: the signature is trusted (above) + // and, for UTXO chains, the change/escrow addresses are ours. The proposal is trusted because these held, + // not because no known-bad condition was hit. + return this.checkUtxoAddresses(credentials, chain, txp); + } + + /** + * For UTXO chains, verifies the change and escrow addresses belong to this wallet. Returns true for + * non-UTXO chains (they carry no such addresses to prove). Split out of checkTxProposalSignature so that + * function terminates on an explicit proven-accept verdict rather than a fall-through. + * + * @param {Object} credentials + * @param {string} chain - lower-cased chain of the proposal + * @param {Object} txp - the transaction proposal + */ + static checkUtxoAddresses(credentials, chain, txp) { + if (!Constants.UTXO_CHAINS.includes(chain)) { + return true; + } + if (txp.changeAddress && !this.checkAddress(credentials, txp.changeAddress)) { + log.debug(`[TXP ${txp.id}] Invalid change address`); + return false; + } else if (!txp.changeAddress && !txp.sendMax) { + log.warn(`[TXP ${txp.id}] Missing change address for non sendMax transaction proposal`); + return false; + } + if (txp.escrowAddress && !this.checkAddress(credentials, txp.escrowAddress, txp.inputs)) { + log.debug(`[TXP ${txp.id}] Invalid escrow address`); + return false; + } return true; } @@ -351,7 +369,8 @@ export class Verifier { // any changed field (destination, amount, from, contract) serializes differently and fails the compare. const rebuilt = Utils.buildTx({ ...txp, ...mutableFields }).uncheckedSerialize(); const rebuiltArr = Array.isArray(rebuilt) ? rebuilt : [rebuilt]; - if (rebuiltArr.length !== prePublishRaw.length) { + // Require a non-empty, positive match: an empty set would make `.every()` vacuously true. + if (rebuiltArr.length === 0 || rebuiltArr.length !== prePublishRaw.length) { return false; } return rebuiltArr.every((raw, i) => raw === prePublishRaw[i]); diff --git a/packages/bitcore-wallet-client/test/verifier.test.ts b/packages/bitcore-wallet-client/test/verifier.test.ts index 1d3344905c6..76eb5d9af13 100644 --- a/packages/bitcore-wallet-client/test/verifier.test.ts +++ b/packages/bitcore-wallet-client/test/verifier.test.ts @@ -415,6 +415,7 @@ describe('Verifier', function() { describe('checkPrePublishRaw', function() { const ATTACKER_EVM = '0x1111111111111111111111111111111111111111'; const ATTACKER_SOL = 'F7FknkRckx4yvA3Gexnx1H3nwPxndMxVt58BwAzEQhcY'; + const ATTACKER_XRP = 'rPT1Sjq2YGrBMTttX4GZHjKu9dyfzbpAYe'; const solTxp = (overrides = {}) => ({ chain: 'sol', @@ -438,6 +439,15 @@ describe('Verifier', function() { ...overrides }); + const xrpTxp = (overrides = {}) => ({ + chain: 'xrp', + outputs: [{ toAddress: 'rDzTZxa7NwD9vmNf5dvTbW4FQDNSRsfPv6', amount: 8000 }], + from: 'rEqj9WKSH7wEkPvWf6b4gCi26Y3F7HbKUF', + fee: 12, + nonce: 1, + ...overrides + }); + it('accepts a SOL proposal whose blockhash was refreshed at publish', function() { const prePublishRaw = Utils.buildTx(solTxp()).uncheckedSerialize(); const current = solTxp({ blockHash: 'H1oRr1nfr4b6eZjs9Ssn3bxUmcRAjqRxrbTKuwSPZ9mE', prePublishRaw }); @@ -464,6 +474,22 @@ describe('Verifier', function() { Verifier.checkPrePublishRaw('eth', current).should.be.false; }); + it('accepts an XRP proposal whose nonce was assigned at publish', function() { + const prePublishRaw = Utils.buildTx(xrpTxp()).uncheckedSerialize(); + const current = xrpTxp({ nonce: 9, prePublishRaw }); + Verifier.checkPrePublishRaw('xrp', current).should.be.true; + }); + + it('rejects an XRP proposal whose destination was tampered', function() { + const prePublishRaw = Utils.buildTx(xrpTxp()).uncheckedSerialize(); + const current = xrpTxp({ + nonce: 9, + outputs: [{ toAddress: ATTACKER_XRP, amount: 8000 }], + prePublishRaw + }); + Verifier.checkPrePublishRaw('xrp', current).should.be.false; + }); + it('rejects prePublishRaw on a non-mutable (UTXO) chain', function() { Verifier.checkPrePublishRaw('btc', { chain: 'btc', prePublishRaw: 'anything' }).should.be.false; });