fix(payment-link): validate Solana OCP tx recipient/amount/asset (BUG-1260)

[davidleomay]

Problem (High — anonymous payment completion)

Solana OCP tx-completion (?tx=<hash> on `payment-link` payments) accepted any real, finalized Solana tx. The switch case routed through `doVerifiedTxIdPayment`, which only called `SolanaClient.isTxComplete` — finality + no error, no recipient/amount/asset binding. Any unrelated finalized Solana tx flipped a real payment obligation to `TX_BLOCKCHAIN` (settlement).

Reported by external hunter (BUG-1260, Compass Security).

Fix

New `doSolanaTxIdPayment` mirrors the EVM/Firo path:

• Fetches the parsed tx via `SolanaService.getTransaction`.
• Iterates the quote's Solana activations and matches via new `TxValidationService.validateSolanaTransaction` — required: destination pays the expected wallet-owner address for the expected asset (mint disambiguates SPL from SOL), amount ≥ activation.amount. Overpayment accepted; wrong owner / wrong mint / underpayment fail.
• Owner-equality works uniformly because `SolanaTransactionDto.destinations[].to` is populated with the wallet owner in every branch of the parser (`create`/`closeAccount`/`transfer`/`transferChecked` via `updateTokenInstruction`) — no ATA derivation is needed.

Also closes a secondary tx-replay class implied by the report but not exercised in the PoC: without a unique index on `PaymentQuote.txId`, a legitimate DFX-bound tx could satisfy an unrelated future quote for the same asset+amount. New `getEarliestQuoteClaimingTx` returns the oldest (ORDER BY id ASC) quote in any tx-active state OR `TX_FAILED` — the latter matters because `txFailed()` does not clear `txId`, so a failed quote still lays claim. `doSolanaTxIdPayment` rejects a submission whose earliest claimant is a different quote (`uniqueId` mismatch); self-match (retry of same quote) passes.

Out of scope (follow-ups)

`SolanaClient.createTransactionDto` mis-classifies native+SPL mixed txs (any system-program instruction routes the whole tx into the native branch and drops SPL) and merges multi-SPL transfers via last-wins in `getTokenInstructions`/`updateTokenInstruction`. Pre-existing DoS risks on legit payments, not payment-side security bypasses. Should be filed as separate parser hardening.

PR completeness

• Migration: none (no entity/column change)
• Environment / Infrastructure: none
• Service / frontend sync: none (public DTO shape unchanged)
• Impact on GS: none

Test plan

• `npx tsc --noEmit` full project: clean
• `npx eslint` on all changed files: clean
• `npx jest --testPathPattern='payment-link|tx-validation|solana'`: 10 suites, 81/81 passing
• Repro the hunter's Solana curl on a running instance with a foreign tx (dev)

Related

• Bug: BUG-1260 (Compass Security)
• Companion PR: fix(payment-link): scope externalPaymentId lookups to the authorized link (BUG-1289) #4436 (BUG-1289 cross-merchant payment-link disclosure)

[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 develop).

Round 1 was a self-consistent but wrong draft — I derived the recipient's ATA and matched it against SolanaTransactionDto.destinations[].to. Both lenses in round 2 flagged that the parser (SolanaClient.getTokenInstructions + updateTokenInstruction) actually populates to with the wallet owner in every branch — the ATA I was deriving would never match. Round 1's fix would have DoS'd every legitimate SPL payment.

Round 2 replaced ATA derivation with owner-equality on both native and SPL paths, removed the now-dead getAssociatedTokenAddress wrappers on SolanaClient/SolanaService, and added an activation.asset.chainId guard. Round-2 security also surfaced a tx-replay class the original report implied but did not exercise: without a unique index on PaymentQuote.txId, a legitimate DFX-bound tx could satisfy an unrelated future quote for the same asset+amount. Added the initial guard using the existing getQuoteByTxId.

Round 3 surfaced two more issues on that guard:

1. DB row-ordering non-determinism. findOne without ORDER BY on SQL Server can return either the earlier owner or the current quote (both match after txReceived saves the current row with the same txId), which lets a replay silently pass. Replaced with a dedicated getEarliestQuoteClaimingTx that adds order: { id: 'ASC' } — first-writer wins deterministically.

2. TX_FAILED quotes retain txId but were outside the status filter. txFailed() on the entity does not clear txId, so a failed quote still lays claim to its tx; excluding TX_FAILED from the guard's status list would let a foreign quote replay the same tx as if it were fresh. Widened the filter to include TX_FAILED alongside PaymentQuoteTxStates.

Round 3 also confirmed the isolation-race bound: with saveTransaction awaited before the per-chain handler runs, each request's own row is committed before its own guard fires, so concurrent submissions of the same tx from different quotes both see BOTH rows and the id-ASC tie-break gives them the same earliest quote — first-writer wins, no double-credit.

Two pre-existing SolanaClient parser issues surfaced by round 2 are out of scope but worth follow-ups:

• createTransactionDto routes any tx containing a system-program instruction into the native branch and drops SPL — a native+SPL mixed tx would DoS a legitimate SPL payment.
• getTokenInstructions/updateTokenInstruction merges multi-SPL transfers via last-wins, so two SPL transfers in one tx produce a non-deterministic single destination.

Both are parser DoS on legit inputs, not payment-side security bypasses; documented for a separate parser-hardening PR.

Coverage gap noted, not blocking: no unit test exists for doSolanaTxIdPayment, validateSolanaTransaction, or the new replay-guard behavior. payment-quote-firo.spec.ts is the natural template for a follow-up spec.

[davidleomay]

Update after a full review pass: one more round was needed. 4 review passes total now (two independent lenses per pass: security/correctness and conformance/regression, each re-run from scratch against develop).

Round 4 caught a CONFIRMED HIGH bypass that survived rounds 1–3 because I had scoped it out of the earlier review as "pre-existing SolanaClient parser DoS" — it isn't just DoS, it's the same bypass class BUG-1260 is meant to close:

Parser instruction-merge bypass. SolanaClient.getTokenInstructions merged fields across a single tx's parsed token instructions into ONE Partial<SolanaTokenInstructionsDto> via a switch: create set {mint, source, destination}, transfer/transferChecked set {authority, amount}. In a single tx bundling both, the returned DTO glued DFX's destination + mint (from a createAssociatedTokenAccountIdempotent(DFX, USDC_mint) — rent ~$0.005) to an unrelated transfer's amount (a transferChecked from attacker to attacker for the required amount). validateSolanaTransaction saw to === DFX_owner, tokenInfo.address === USDC_mint, amount ≥ activation.amount and returned isValid; quote flipped to TX_BLOCKCHAIN; DFX received zero USDC. Exploit cost: ~$0.005 + gas.

Fix rewrote the SPL parser path (getTokenTransactionDestinations) to emit ONE destination per transfer/transferChecked — not one merged destination for the whole tx. Each destination ATA is resolved to its wallet owner via postTokenBalances[b].accountIndex → accountKeys[..].pubkey, and its mint is read from the same balance entry. create / closeAccount are now ignored (no value transfer; a real create+fund tx still works because it also carries the transfer that provides the destination + amount). Removed the dead getTokenInstructions, updateTokenInstruction, getTokenTransactionDestination, plus the unused DTO import.

Round 4 also verified against six scenarios: original exploit (blocked), legit create+fund (works), plain transfer (works), multi-transfer in one tx (each emitted separately, validator matches the DFX one), cross-mint substitution (rejected on mint mismatch), missing postTokenBalances (fail-closed).

Two adjacent items noted, not addressed here (pre-existing, separate follow-ups):

• CPI-hidden transfers are invisible to the outer-instruction loop. Fail-closed today (no destination emitted → validator rejects), but worth documenting.
• fromLamportAmount float collapse for token base units > 2^53 — pre-existing precision concern, harmless to the underpayment check.

Also from round 4, three conformance nits worth addressing in a follow-up rather than this PR: getEarliestQuoteClaimingTx could be private (no external callers), the naming reads awkwardly compared to getQuoteByTxId, and the !isCoin && !activation.asset.chainId guard in doSolanaTxIdPayment duplicates a check in validateSolanaTransaction. Test coverage gap (no unit tests for the new handler / validator / guard) also carries forward — payment-quote-solana.spec.ts mirroring payment-quote-firo.spec.ts would materially strengthen this.

[davidleomay]

Follow-up to the previous comment, after the R4 parser rewrite plus a prettier line-wrap: one more full pass ran (two independent lenses, each re-run from scratch against develop on the current HEAD c533db8ac). 6 review passes in total.

Fresh security probes on the rewritten parser and the still-present replay guard — all fail-closed:

• ATA forgery: an ATA's owner is baked into address derivation, enforced by the SPL Token program at the runtime layer, not by DFX. An attacker cannot mint an ATA claiming DFX as owner for a key they control.
• postTokenBalances injection: validator-computed post-execution, not user-supplied. Immune.
• accountIndex out of range: accountKeys[b.accountIndex]?.pubkey.toBase58() short-circuits to undefined, string equality returns false. No throw path.
• Multi-transfer selection: .find() first-match wins, but the amount + owner + mint checks are still authoritative. Underpayment against a matching DFX destination rejected on match.amount < expectedAmount.
• Token 2022 transferCheckedWithFee: parser type filter fails-closed on it (feature gap, not a vulnerability).
• Combined native+SPL tx: else if selection means the SPL branch is skipped when a SystemProgram transfer is present in the same tx — activation match then falls through to fail. Pre-existing DoS on that specific tx shape, not a security bypass.
• System transfer bleed into SPL loop: System instructions use lamports, not amount; the rawAmount == null filter drops them.
• Cross-blockchain txId collision: txBlockchain: Equal(...) in the replay guard prevents any cross-chain match. Solana signatures are base58 (case-sensitive, Number(...) = NaN), so no numeric aliasing class analogous to the ICP one (fix(payment-link): validate ICP OCP tx recipient/amount/asset in the sender-less fallback (BUG-1260 sibling) #4490).
• Concurrent aliased submissions: base58 has no aliases; ordering-race remains handled by the id-ASC tiebreak from round 3.
• CPI-hidden create+transfer: inner instructions are outside message.instructions scope, parser never sees them; no destination emitted, validator rejects — fail-closed.

Pre-existing weakness noted (not introduced or worsened by this PR, worth a separate follow-up ticket): an attacker can InitializeAccount a non-ATA token account with owner = DFX_wallet, transfer tokens to it, and the parser would still match destinationBalance.owner === DFX_wallet. Impact: laundering / accounting-mismatch vector, not a free-money exploit — DFX's outbound createTokenTransaction only spends from the ATA (getAssociatedTokenAddress), so the non-ATA balance is stranded on the receive side. Same weakness existed in the old parser (updateTokenInstruction used the same postTokenBalances.owner lookup). To close it: verify destinationAta === getAssociatedTokenAddressSync(mint, DFX_wallet) before accepting.

Conformance pass surfaced two minor items and a doc drift, none blocking:

1. SolanaTokenInstructionsDto (src/integration/blockchain/solana/dto/solana.dto.ts:33) is now a dead export after the R4 rewrite removed its only consumers (getTokenInstructions, updateTokenInstruction, getTokenTransactionDestination). Grep-confirmed no other references. Kept for now to avoid API-shape churn; drop in a follow-up cleanup.
2. getEarliestQuoteClaimingTx (payment-quote.service.ts:158) could be private — only in-class callers, matches getActualQuoteByUniqueId's visibility.
3. PR description was last updated before the R4 rewrite; the "no ATA derivation needed" line no longer matches shipped code (the rewrite DOES resolve destination ATA to owner via postTokenBalances). Description accuracy nit, not a code issue.

CI: all 12 checks green after the prettier line-wrap.