Fix/ltc and doge check paypro - #4225
Open
MichaelAJay wants to merge 21 commits into
Open
MichaelAJay wants to merge 21 commits into
MichaelAJay wants to merge 21 commits into
Conversation
leolambo
reviewed
Aug 25, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Verifier.checkPaypro in bitcore-wallet-client so PayPro verification fails closed for all supported multisig/UTXO chains (BTC, BCH, DOGE, LTC) by validating the full, order-independent output set (destination + atomic amount) against PayPro instructions, using chain-specific address canonicalization.
Changes:
- Canonicalize and compare PayPro instructions vs proposal outputs for BTC/BCH/DOGE/LTC, including output count and totals, rejecting malformed/unsupported inputs without throwing.
- Add detailed warning logs on PayPro verification failures (including TXP id when available).
- Add extensive unit and boundary tests covering matching, substitution attacks, address equivalence forms, malformed inputs, and multi-output ordering/duplication.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/bitcore-wallet-client/src/lib/verifier.ts | Expands PayPro verification to DOGE/LTC and to full output-set matching with chain-specific address parsing and stricter validation. |
| packages/bitcore-wallet-client/test/verifier.test.ts | Adds comprehensive PayPro verification and checkTxProposal boundary regression coverage across chains and edge cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…tc-and-doge-check-paypro
…ed output calldata - prior tests GREEN
…tc-and-doge-check-paypro
leolambo
reviewed
Sep 4, 2026
| } | ||
|
|
||
| if (opts.paypro && !this.checkPaypro(txp, opts.paypro)) return false; | ||
| normalizedEntries.push({ toAddress: entry.toAddress, amount, raw: entry }); |
Contributor
There was a problem hiding this comment.
there's a potential backdoor if a script is provided via packages/bitcore-wallet-client/src/lib/common/utils.ts:389 (if (o.script) → addOutput). we might want to refuse any PayPro output that has a script at all or validate the script path as well.
Contributor
Author
There was a problem hiding this comment.
addressed in the merged implementation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical PayPro verification gaps remain for SVM, XRP flags, chain/network binding, and legacy outputs.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
packages/bitcore-wallet-client/src/lib/verifier.ts:750
0xis valid zero-byte calldata and the EVM builder serializes it the same as absent data. Returning'0x'here makes a signed instruction withdata: '0x'reject a proposal whose output omitsdata(and vice versa), causing otherwise equivalent native transfers to fail verification. Canonicalize zero-length calldata to the samenullrepresentation.
if (typeof data !== 'string' || !/^0x([0-9a-fA-F]{2})*$/.test(data)) {
throw new Error('invalid EVM calldata');
}
return data.toLowerCase();
packages/bitcore-wallet-client/src/lib/verifier.ts:563
- For BTC, DOGE, and LTC the chain-specific
Addressconstructor is called withouttxp.network, so it infers the network from the address string. A mainnet-form address can consequently pass a testnet/regtest PayPro check when both sides use the same wrong-network encoding, unlike the BCH branch which binds the declared network. Pass the proposal network to these constructors and reject network mismatches.
const normalizeAddress = (address: string) => {
if (chain !== 'bch') return new addressLib.Address(address).toString();
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Lite
Comment on lines
+435
to
441
| if (isRippleChain) { | ||
| // Match the XRP builder's legacy `type` fallback exactly. | ||
| const txType = txp.txType === undefined ? txp.type : txp.txType; | ||
| if (txType != null && (typeof txType !== 'string' || txType.toLowerCase() !== 'payment')) { | ||
| return falseWithLogWarn('unsupported XRP transaction type'); | ||
| } | ||
| } |
Comment on lines
+443
to
+447
| // payproOpts chain/network/currency optional - validate if present | ||
| if (payproOpts.chain != null) { | ||
| // If payproOpts.chain present | ||
| // must be a string in agreement with chain derived above | ||
| if (typeof payproOpts.chain !== 'string' || payproOpts.chain.toLowerCase() !== chain) { |
Comment on lines
+489
to
+491
| let rawOutputs = version >= 3 | ||
| ? txp.outputs | ||
| : [{ toAddress: txp.toAddress, amount: txp.amount }]; |
Comment on lines
+605
to
+606
| this.accountEntriesMatch(outputs, payproInstructions, this.normalizeSolAddress) && | ||
| this.solPaymentDetailsMatch(txp, payproInstructions[0]?.raw) |
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
Fixes IS-1413.
Verifier.checkPaypropreviously verified destination addresses only for BTC and BCH. LTC and DOGE PayPro proposals could therefore substitute a different destination while preserving the invoiced amount and still pass co-signer verification.This change makes PayPro verification fail closed for all supported multisig/UTXO chains: BTC, BCH, DOGE, and LTC. It compares the complete, order-independent set of transaction outputs against the PayPro instructions, including both destination and amount.
Addresses are parsed and canonicalized with each chain’s own bitcore library. This preserves equivalent address representations while ensuring malformed addresses cannot pass merely because their raw strings match.
Changelog
3...and modernM...P2SH representations.Testing Notes
Verified from
packages/bitcore-wallet-client, with a local MongoDB instancealready running and reachable at
localhost:27017(Docker containermongodb, imagemongo:7.0, running since 2026-07-13 — not started for thischange, just already part of the local dev environment):
npm run compile— passed, no errors.../../node_modules/.bin/mocha ts_build/test/verifier.test.js --grep 'checkPaypro|checkTxProposal'— 47 passing, 0 failing.npm run lint— passed, no errors.git diff --check— passed, no whitespace errors.npm test(full package suite, including the MongoDB-backed integration tests) — 589 passing, 12 pending, 0 failing, exit code 0. Coverage: Statements 75.29%, Branches 62.03%, Functions 72.58%, Lines 76.99%; all thresholds met, no database-related failures.The focused coverage includes:
3...and modernM...P2SH equivalence.Verifier.checkTxProposal.For manual verification:
SERVER_COMPROMISED.Checklist