Skip to content

BatchPaymentService::execute_batch never validates signed_xdr contents match the claimed legs array #22

Description

@abayomicornelius

BatchPaymentService::execute_batch (src/services/batch.rs) validates the shape of req.legs (non-empty, ≤100, each leg's amount parses as a positive f64) and validates that req.signed_xdr is non-empty — but it never validates that the content of signed_xdr (the actual on-chain operations the client already signed) matches the legs array the server is about to record as ground truth in the transactions table.

// Pre-create one pending transaction row per leg, all sharing batch_id.
for (index, leg) in req.legs.iter().enumerate() {
    ...
    sqlx::query(/* INSERT INTO transactions ... */)
        .bind(&asset).bind(&leg.amount).bind(&req.source_account).bind(&leg.destination)
        ...
}
// Submit the single, already-signed batch transaction once.
let submission = stellar.submit_transaction(&req.signed_xdr).await;

The legs array and signed_xdr are two independently client-supplied inputs that are trusted to agree with each other. Nothing decodes signed_xdr (via stellar_xdr, already a dependency used extensively in src/services/soroban.rs) to confirm its Payment/PathPaymentStrictSend operations actually have the same destinations, amounts, and asset codes as the legs array claims.

Why it matters

  • A client (malicious or simply buggy) could submit a signed_xdr transaction paying Alice: 1 XLM while the legs array claims Bob: 1000 XLM. stellar.submit_transaction will happily broadcast whatever the signed envelope actually contains (Horizon doesn't know or care about StellarSend's legs metadata), and on success the code marks all batch_id rows Completed with the same tx_hash — regardless of what actually happened on-chain per leg.
  • This produces a transactions table that is authoritative-looking (has a real, verifiable stellar_tx_hash) but factually wrong about who got paid what — a serious integrity gap for a payments product whose entire value proposition is an accurate transaction history/audit trail.
  • This is a variant of a "trust the client's metadata about its own signed transaction" bug — the non-custodial design (client signs, server relays) is sound in principle (documented in escrow.rs's comments about caller.require_auth()), but relaying and separately recording claimed details without cross-checking them undermines the whole point of keeping server-side records.

Reproduction / detection

  1. Build a signed_xdr transaction (any valid testnet payment) paying account X amount 1, but submit it to POST /api/payments/batch with a legs array describing a payment to account Y for amount 999.
  2. Observe that execute_batch still succeeds, submits the real (mismatched) transaction, and records Y: 999 in transactions as Completed with the real tx_hash — the DB record does not reflect on-chain reality.

Proposed fix

  • Decode req.signed_xdr via stellar_xdr::curr::TransactionEnvelope::from_xdr_base64 (same pattern already used in SorobanService's tests, e.g. envelope_round_trips_through_xdr) and extract its Payment/PathPaymentStrictSend operations.
  • Before submission, assert a 1:1 correspondence (same order or matched by destination+asset) between decoded operations and req.legs: same destination, same asset_code/asset_issuer, and amount within acceptable rounding tolerance of what's encoded on-chain (Stellar amounts are i64 stroops; legs[i].amount is a decimal string that needs the same precision-safe parsing recommended in issue No numeric validation on payment amount fields (QuoteRequest.amount, SendPaymentRequest.send_amount) #17).
  • Reject the whole batch with AppError::Validation if any leg doesn't match a corresponding operation, before pre-creating any transactions rows or calling submit_transaction.

Edge cases

  • The transaction might legitimately contain other operations besides Payments (e.g. a SetOptions sequence-bump op some wallets add) — the matcher needs to specifically look for payment-shaped operations and ignore/allow known-benign extras rather than requiring an exact 1:1 op-count match.
  • Multi-asset batches: verify asset_issuer (or its absence for native XLM) matches exactly, not just asset_code, since two different issuers can use the same code (e.g. two different "USDC" issuers).
  • Order independence: clients may build operations in a different order than the legs array — match by content, not positionally, unless the API contract explicitly requires positional correspondence (worth documenting either way).

Testing strategy

  • Unit test decoding a hand-built TransactionEnvelope with known operations and asserting the matcher accepts a correct legs array and rejects a mismatched amount, mismatched destination, and mismatched asset issuer individually.
  • Integration test against testnet: submit a real signed batch transaction with intentionally mismatched legs metadata and assert the API rejects it with 4xx before ever calling stellar.submit_transaction (verify via a spy that the Horizon submit endpoint was never hit).

Metadata

Metadata

Assignees

No one assigned

    Labels

    GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial Campaign | FWC26Campaign: Official Campaign | FWC26backendBackend service logicbugSomething isn't workingsecuritySecurity concernvery hardVery difficult / senior-level bounty issue

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions