[BWS][BWC][CWC] Bind prePublishRaw fallback to the proposal - #4232
Open
leolambo wants to merge 2 commits into
Open
[BWS][BWC][CWC] Bind prePublishRaw fallback to the proposal#4232leolambo wants to merge 2 commits into
leolambo wants to merge 2 commits into
Conversation
leolambo
changed the title
leolambo
marked this pull request as ready for review
August 28, 2026 19:23
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.
leolambo
force-pushed
the
fix/verifier-prepublishraw-binding
branch
from
August 31, 2026 12:36
16fc803 to
4386a26
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The changes are security-critical and span core tx parsing plus client/server signature verification paths, so a final human review is recommended before approval.
Pull request overview
This PR hardens the prePublishRaw signature fallback path by cryptographically binding prePublishRaw to the current proposal for chains where BWS mutates a publish-time field (SOL blockhash, EVM/XRP nonce), preventing a compromised server from replaying a valid signature while swapping destination/amount.
Changes:
- Adds
getMutableFields(rawTx)to SOL/EVM/XRP transaction providers to recover the publish-mutable field from a raw tx. - Updates BWC verifier and BWS publish flow to only accept
prePublishRawfallback when the rebuilt proposal matchesprePublishRawbyte-for-byte after restoring the mutable field. - Adds unit/integration tests across CWC/BWC/BWS, and rejects
prePublishRawon UTXO chains.
File summaries
| File | Description |
|---|---|
| packages/crypto-wallet-core/test/transactions.test.ts | Adds coverage for getMutableFields recovery on SOL/EVM/XRP. |
| packages/crypto-wallet-core/src/transactions/xrp/index.ts | Implements getMutableFields for XRP (Sequence/nonce). |
| packages/crypto-wallet-core/src/transactions/sol/index.ts | Implements getMutableFields for SOL (recent blockhash from compiled message). |
| packages/crypto-wallet-core/src/transactions/eth/index.ts | Implements getMutableFields for EVM (nonce). |
| packages/bitcore-wallet-service/test/integration/server.test.ts | Adds BWS integration tests for binding checks and publish fallback tamper rejection. |
| packages/bitcore-wallet-service/src/types/chain.d.ts | Extends IChain interface with isPrePublishRawBound. |
| packages/bitcore-wallet-service/src/lib/server.ts | Guards publish-time signature fallback to prePublishRaw with binding check. |
| packages/bitcore-wallet-service/src/lib/common/utils.ts | Adds shared isPrePublishRawBound implementation for account-based chains. |
| packages/bitcore-wallet-service/src/lib/chain/xrp/index.ts | Wires chain-level isPrePublishRawBound for XRP. |
| packages/bitcore-wallet-service/src/lib/chain/sol/index.ts | Wires chain-level isPrePublishRawBound for SOL. |
| packages/bitcore-wallet-service/src/lib/chain/index.ts | Exposes ChainService.isPrePublishRawBound via proxy. |
| packages/bitcore-wallet-service/src/lib/chain/eth/index.ts | Wires chain-level isPrePublishRawBound for EVM chains. |
| packages/bitcore-wallet-service/src/lib/chain/btc/index.ts | Explicitly rejects prePublishRaw binding on UTXO chains. |
| packages/bitcore-wallet-client/test/verifier.test.ts | Adds BWC tests for refreshed mutable field acceptance and tampering rejection. |
| packages/bitcore-wallet-client/src/lib/verifier.ts | Adds checkPrePublishRaw binding guard and tightens fallback signature verification. |
Review details
- Files reviewed: 14/15 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
MichaelAJay
reviewed
Sep 7, 2026
| @@ -305,6 +316,51 @@ export class Verifier { | |||
| return true; | |||
Contributor
There was a problem hiding this comment.
I think the problem is larger than playing whack-a-mole with known false cases. Consider how to make this return true only for explicit trust situations, instead of for all situations we can't explicitly think of.
MichaelAJay
reviewed
Sep 7, 2026
| }).should.be.true; | ||
| }); | ||
| }); | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
IS-1422 · Bug Bounty 2643811
The transaction verifier falls back to
txp.prePublishRawwhen it can't match the creator's signature against the locally rebuilt tx. It only checked that the signature was valid overprePublishRaw, not thatprePublishRawwas the same proposal, so a compromised wallet service could keep a valid signature, swap the destination, and still pass the check. The fallback now recovers the one field the server changes at publish (the Solana blockhash or the EVM/XRP nonce) fromprePublishRaw, puts it back on the current proposal, and requires a byte-for-byte match, so any other change fails as SERVER_COMPROMISED.Changelog
prePublishRawand require the rebuilt proposal to match it exactly, so a tampered destination or amount fails as SERVER_COMPROMISED.prePublishRawon UTXO chains, which never mutate a field at publish.publishTxfallback the same way, so a tampered stored proposal can't reuse an old signature.getMutableFieldsto the SOL, EVM, and XRP providers to pull that field back out of a raw tx.Testing Notes
New tests at each layer:
getMutableFieldsrecovery for SOL, EVM, and XRPcheckPrePublishRawaccepts a refreshed field and rejects a tampered destinationisPrePublishRawBoundplus an end-to-endpublishTxthat rejects a tampered stored proposalRun
npm testin each of those three packages.Checklist
BWCif modifying the bitcore-wallet-client package,CLIif modifying the bitcore-cli package, etc.)