fix(payment-link): validate ICP OCP tx recipient/amount/asset in the sender-less fallback (BUG-1260 sibling)

[davidleomay]

Problem (High — anonymous payment completion, same class as BUG-1260)

`doIcpPayment` falls back to a "no `sender`" branch whenever the client cannot use the ICRC-2 approve/pull flow — the client self-broadcasts the payment and posts back a raw txId. That fallback routed through `doVerifiedTxIdPayment`, which only called `isTxComplete` — recipient, amount, and asset were never validated. Any authenticated OCP caller could submit any real finalized ICP tx (omit `sender`, pass `?tx=`) and settle a foreign payment obligation.

Client-controlled — attacker just omits one field. Same class as the Solana bug in #4453.

Fix

New `doIcpTxIdPayment` mirrors the Solana handler:

• Same replay guard via `getEarliestQuoteClaimingTx` (introduced in fix(payment-link): validate Solana OCP tx recipient/amount/asset (BUG-1260) #4453 — rejects a tx already claimed by a different quote, includes TX_FAILED, ORDER BY id ASC).
• Waits for finality.
• Iterates ICP activations. For ICRC-3, rejects up-front when the txId's canister id (`canisterId:blockIndex`) does not match `activation.asset.chainId` — without this, a legit transfer of N units of a cheap ICRC-3 token to the DFX principal would satisfy a quote for N units of a different (expensive) ICRC-3 token (same recipient, same amount).
• Fetches the transfer via new `InternetComputerClient.getTransferByTxId(txId, asset)`, which handles all three ICP txId formats (Rosetta 64-hex hash, ICRC-3 `canisterId:blockIndex`, native block-index) and normalizes them into `IcpTransfer`. Asserts `blockIndex === requested index` (fail-closed against archive-skip in `getIcrcTransfers`/`getTransfers` that could otherwise substitute a later block into a foreign quote).
• Normalizes the expected recipient to match the format the transfer carries — native = account-identifier hex via `InternetComputerUtil.accountIdentifier`; ICRC-3 = raw principal text. Calls new `TxValidationService.validateIcpTransfer`. Overpayment accepted, wrong recipient / underpayment fails.

Removes the now-dead `doVerifiedTxIdPayment` — the ICP fallback was its only remaining caller after #4453.

Stacked on #4453

This branch is based on `fix/paymentlink-solana-anonymous-completion` (PR #4453) — it depends on `getEarliestQuoteClaimingTx`, `TxValidationService` seams, and `SolanaService` injection scaffolding introduced there. Merge #4453 first, then this rebases cleanly onto `develop` on merge.

PR completeness

• Migration: none
• Environment / Infrastructure: none
• Service / frontend sync: none (public DTO shape unchanged; `sender=undefined` clients still work but must now submit a tx that actually pays DFX)
• Impact on GS: none

Test plan

• `npx tsc --noEmit`: clean
• `npx eslint` on all changed files: clean
• `npx jest --testPathPattern='payment-link|tx-validation|icp|solana'`: 12 suites, 102/102 passing
• Repro the hunter-shaped Solana exploit against ICP on a running instance: submit `?tx=<foreign_finalized_icp_hash>` with no `sender` — expect `Transaction ... does not pay any matching activation`

Related

• Bug: BUG-1260 (Compass Security) — same class, Solana half in fix(payment-link): validate Solana OCP tx recipient/amount/asset (BUG-1260) #4453
• Companion PR: fix(payment-link): scope externalPaymentId lookups to the authorized link (BUG-1289) #4436 (BUG-1289 cross-merchant payment-link disclosure — separate class)

[davidleomay]

3 review passes to reach zero findings (two independent lenses per pass: security/correctness and conformance/regression, each re-run from scratch on the then-current HEAD against the base branch).

Round 1 surfaced one CONFIRMED security bypass and three conformance items:

1. Cross-canister ICRC-3 substitution. getTransferByTxId used the canister id parsed from the txId itself, and validateIcpTransfer never cross-checked activation.asset.chainId. Since the DFX ICP payment address is one principal shared across all tokens, a legit transfer of N units of a cheap ICRC-3 token to that principal would satisfy an unrelated N-unit quote for an expensive ICRC-3 token — same recipient, same amount, different asset. Fixed by rejecting up-front in doIcpTxIdPayment when transferInfo.tx.split(':')[0] !== activation.asset.chainId.
2. Silent-drop concern on getTransferByTxId(asset?). The optional param let a caller pass an ICRC-3 txId without an asset, silently returning undefined. Every real caller has an asset (the activation being matched), so made it required — comment updated to state that ICRC-3 needs the asset's decimals.
3. Method placement. getTransferByTxId was inserted between the isTxComplete/isTxHashComplete public/private pair, splitting them. Moved below isTxHashComplete so the txId-shape helpers stay cohesive.
4. Stale "doVerifiedTxIdPayment" comment reference. The method was removed as part of this fix (it was the ICP fallback's only remaining caller after fix(payment-link): validate Solana OCP tx recipient/amount/asset (BUG-1260) #4453). Reworded the comment in doIcpPayment to describe the insecure-then-fixed history without naming the removed method.

Round 2 surfaced one PLAUSIBLE follow-up:

5. Archive-skip block substitution. getTransferByTxId returned transfers[0] without asserting the returned block matches the requested index. getIcrcTransfers/getTransfers explicitly skip archived ranges when the ledger returns first_block_index > start — so a request for canisterA:42 could hand back block 50's transfer, letting a foreign block satisfy an activation via recipient/amount collision. Whether real ICRC/ICP ledgers actually gap-fill vs. return empty was implementation-dependent (reference impls return empty), so PLAUSIBLE, not CONFIRMED. Fixed by asserting transfers[0]?.blockIndex === index in both the ICRC-3 and native branches — fail-closed. Comment added to explain the invariant.

Round 3 verified the block-index guard: off-by-one is correct (firstIndex + 0 === requested iff the ledger returned the genuine block), Rosetta hash branch untouched (either matches or returns undefined — no skip vector), type-coercion via Number() produces the same value both sides of the comparison, NaN / empty response fail-closed. No new findings.

Coverage gap noted, not blocking: no unit tests exist for getTransferByTxId, validateIcpTransfer, or doIcpTxIdPayment. payment-quote-firo.spec.ts is the natural template for a follow-up payment-quote-icp.spec.ts.

[davidleomay]

Update after a full review pass: two more rounds were needed (rebase pulled the parser-rewrite fix from #4453, and a new adversarial pass caught a replay-guard bypass I had missed). 5 review passes total now (two independent lenses per pass: security/correctness and conformance/regression, each re-run from scratch against the base branch).

Round 4 caught one CONFIRMED HIGH bypass of the replay guard added in round 2:

Number-coerced txId aliases bypass the replay guard. The guard uses TypeORM Equal(txId) — an exact-string SQL match — but both isTxComplete and getTransferByTxId resolve the block via Number(txId). Distinct strings that Number-coerce to the same block ("42", "042", "+42", " 42", "42.0", "4.2e1", "0x2a" — all → 42) each pass the guard independently, then hit the same real block via the ledger call. If that block is a legit DFX-bound native ICP transfer of ≥ activation.amount, the validator accepts — attacker settles their quote against another user's on-chain payment. Same class hits ICRC-3 (canA:42 vs canA:042 vs canA:4.2e1).

Fix canonicalizes the txId before both the guard and the ledger call:

• New InternetComputerUtil.canonicalizeTxId(txId) normalizes each of the three ICP formats to one form per underlying block: Rosetta 64-hex → lowercase; ICRC-3 canisterId:blockIndex → block-index re-serialized through Number(...); native block-index → same. Non-numeric / unknown shapes are returned untouched (downstream lookup fails).
• doIcpTxIdPayment runs the canonicalizer at the top, persists the canonical form to the DB via saveTransaction if it differs from what txReceived stored, and uses the canonical txId for the replay guard, waitForTxConfirmation, getTransferByTxId, and the final quote.txInBlockchain(txId). The catch-block message keeps the raw input for user diagnostics.

Round 5 verified the fix against the full alias table ("42", "042", "+42", " 42", "42.0", "4.2e1", "0x2a" all fold to "42"; "1e2" correctly stays as "100", i.e. a different block), the concurrent-race timing (saveTransaction is PK-scoped so no cross-row overwrite; ORDER BY id ASC tiebreak picks the earliest quote deterministically), and confirmed no other blockchain in PaymentQuoteService has the same vulnerability class (Solana signatures are base58 → Number is NaN; EVM hashes are 0x-prefixed hex → too long to alias).

The branch was also rebased onto the updated Solana PR head (#4453 got its own round-4 parser-rewrite fix), which resolved one conflict in the comment above doSolanaTxIdPayment (kept the updated version). Both PRs still stack cleanly.

Coverage gap noted (unchanged from prior rounds): no unit tests for doIcpTxIdPayment, validateIcpTransfer, getTransferByTxId, or the new canonicalizeTxId helper. Alias-collapse table would be a natural spec.

[davidleomay]

Follow-up to the previous comment, after a rebase onto #4453's R4.1 (prettier line-wrap): one more full pass ran (two independent lenses, each re-run from scratch against develop on the current HEAD ebf7b746a). 6 review passes in total (rebase + adversarial verification).

Both lenses returned zero findings on the current head:

Security probes on canonicalization edge cases — every path is fail-closed:

• Whitespace / unicode Number-coercion: standard whitespace variants (" 42", "42 ", "42
") all fold to 42 via the canonicalizer; fullwidth digits "４２" → NaN → returned as-is → downstream Number("４２") again NaN → Number.isFinite false → getTransferByTxId returns undefined → txFailed.
• 2^53 float-precision collision: theoretically real, but ICP block heights are ~2×10⁷ today vs 2⁵³≈9×10¹⁵ — five-plus orders of magnitude of headroom, not exploitable in practice.
• Negative indices: canonical form preserves "-1"; getTransfers(-1, 1) hits BigInt(-1) sent as IDL.Nat64 (unsigned) → candid serialization throws → outer catch → txFailed.
• Empty index ("canA:"): canonicalizes to "canA:0"; block 0 on real ICRC-3 canisters is Mint, not transfer → mapIcrcTransaction returns undefined → transfers=[] → guard fails → txFailed.
• Empty / whitespace-only txId: outer !transferInfo.tx catches empty; " " canonicalizes to itself, downstream fetch of block 0 is Mint → txFailed.
• Uppercase canister id: preserved verbatim in canonical form, Principal.fromText("CanA") throws (principals lowercase-only) → getIcrcBlockHeight catches, returns false → waitForTxConfirmation throws not confirmed → txFailed.
• Chain-id strict !== at payment-quote.service.ts:834: correct for DFX's lowercase-normalized asset.chainId; case mismatch fails closed.
• Race conditions: both quotes canonicalize to the same form; saveTransaction PK-scoped, no cross-row overwrite; getEarliestQuoteClaimingTx ORDER BY id ASC deterministically picks the earlier claimant; loser blocked with "already assigned".

Downstream state consistency verified: the ICP pay-in strategy at subdomains/supporting/payin/strategies/register/impl/icp.strategy.ts:146 independently emits blockIndex.toString() and ${canisterId}:${blockIndex} — matches the canonical form the OCP path now persists. Reconciliation via getQuoteByTxId(cryptoInput.inTxId) at payment-link-payment.service.ts:346 matches on the same string. Webhook payload uses paymentLink fields, not quote.txId directly.

Conformance pass verified: layering (canonicalize on InternetComputerUtil alongside sibling helpers, getTransferByTxId on the client with getTransfers/getIcrcTransfers/getNativeTransfersForAddress), order of ops in doIcpTxIdPayment (canonicalize → save-if-diff → replay guard → wait → activations), no dead imports, no stale getTokenInstructions/updateTokenInstruction refs after the rebase, doVerifiedTxIdPayment removed with zero remaining callers (the ICP fallback was the last one after #4453).

prettier, eslint, tsc --noEmit clean. 9 test suites / 83/83 pass locally. CI green.

Coverage gap unchanged from prior rounds: no unit tests for canonicalizeTxId, getTransferByTxId, validateIcpTransfer, doIcpTxIdPayment. A fixture-table test of the alias set (~15 lines) would lock the R4 fix against future regressions — worth a follow-up.

Stacking: base still fix/paymentlink-solana-anonymous-completion. Merge #4453 first, this rebases cleanly onto develop on merge.