Skip to content

fix: secure POST /transactions/submit against third-party XDR (#117) - #125

Open
Marvelg256 wants to merge 1 commit into
StepFi-app:mainfrom
Marvelg256:security/117-secure-xdr-submission
Open

fix: secure POST /transactions/submit against third-party XDR (#117)#125
Marvelg256 wants to merge 1 commit into
StepFi-app:mainfrom
Marvelg256:security/117-secure-xdr-submission

Conversation

@Marvelg256

Copy link
Copy Markdown

🔗 Related Issue

Closes #117

🔖 Title

Secure POST /transactions/submit: wallet binding, operation allowlist, idempotency, and rate limits

📝 Description

TransactionsService.submitTransaction() previously accepted any well-formed XDR, immediately submitted it to Horizon, and recorded it against the authenticated wallet — without ever verifying that the wallet was actually a party to the transaction. This PR closes that open-relay hole:

  • Source-account binding — the transaction source account (or the inner source of a fee-bump transaction) must equal the authenticated wallet, or the wallet must appear as an authorized address in the Soroban invocation auth. Third-party-sourced XDR is rejected with a typed TRANSACTION_SOURCE_MISMATCH error before any Horizon call or DB write.
  • Operation allowlist per declared type — every operation must be a Soroban invokeHostFunction whose function name matches the declared type (loan_create, loan_repay, deposit, withdraw, vendor_approve, vendor_suspend) and, when configured, must target the contract owned by that flow. Mismatches are rejected with TRANSACTION_TYPE_MISMATCH / TRANSACTION_OPERATION_NOT_ALLOWED.
  • Idempotency per transaction hash — a new migration adds partial unique indexes on transaction_hash and hash (with pre-existing-row dedupe); the service checks for an existing record before submitting and returns the original record (duplicate: true) instead of re-submitting, with the unique-constraint violation as the concurrency backstop. Duplicate hashes can no longer create duplicate rows or double Horizon submissions.
  • Per-wallet + per-IP rate limits — a new WalletThrottlerGuard keys @nestjs/throttler on the authenticated wallet (the repo's established middleware), and the route is limited to 10 req / 60 s per wallet AND per IP via the global guard.
  • Persistence-first, no silent drops — the local record is written (awaited) before the Horizon submission, so persistence failures surface as TRANSACTION_PERSISTENCE_FAILED instead of the old fire-and-forget behavior, and the transaction hash is always known to the status checker / indexer reconciliation paths.

🔄 Changes Made

  • src/modules/transactions/transactions.service.ts — allowlist + source/auth validation, hash idempotency, persist-before-submit flow in submitTransaction
  • src/modules/transactions/wallet-throttler.guard.ts — new per-wallet throttler guard
  • src/modules/transactions/transactions.controller.ts — per-wallet + per-IP rate limits, Swagger responses updated (400 codes, 429, 500)
  • src/modules/transactions/dto/submit-transaction-request.dto.ts — documents the enforced guarantees
  • src/modules/transactions/dto/submit-transaction-response.dto.tsstatus reflects the recorded status; new duplicate flag
  • supabase/migrations/20260825000001_add_unique_transaction_hash.sql — unique indexes + dedupe
  • test/unit/modules/transactions/transactions.service.spec.ts — happy path + every rejection branch (source mismatch, fee-bump inner source, allowlist/function/contract mismatch, classic ops, duplicate pre-check, unique-violation race, persistence failure, Horizon error mapping)
  • test/unit/modules/transactions/wallet-throttler.guard.spec.ts — new guard tracker tests
  • test/unit/modules/transactions/transactions.controller.spec.ts — compiles with ThrottlerModule
  • context/progress-tracker.md — documented

🗒️ Additional Notes

  • Source binding intentionally also accepts the wallet as a Soroban authorizer (not only as the transaction source) because the deposit/withdraw/repay/vendor XDR builders currently set a random source account — a strict source-equality check would reject StepFi's own two-step flows. loan_create uses the wallet as source and passes the strict path.
  • transactions.repository.ts was reviewed but not modified: backfill's upsert(onConflict: 'transaction_hash') remains compatible with the new partial unique index, and findForReconciliation is unaffected.
  • The migration lives in supabase/migrations/ (outside src//test/) as required by the issue and context/code-standards.md.

✅ Verification

  • npm run build — zero TypeScript errors
  • npm test — 30 suites / 358 tests passing (test count increased from baseline, per the mandatory checklist)
  • ESLint clean on all changed files (remaining repo lint errors are pre-existing in untouched files)

…-app#117)

Bind submissions to the authenticated wallet (source account, fee-bump inner
source, or Soroban invocation auth), enforce a per-type operation allowlist,
make submission idempotent per transaction hash via a unique-index migration,
rate limit per wallet and IP, and persist records before Horizon submission
so persistence failures surface instead of being silently dropped.

Generated with Codebuff 🤖
Co-Authored-By: Codebuff <noreply@codebuff.com>
@Marvelg256
Marvelg256 requested a review from EmeditWeb as a code owner August 25, 2026 16:14

@EmeditWeb EmeditWeb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Automated Audit: solves

The PR addresses every root cause enumerated in issue #117: source-account binding (source or fee-bump inner source, with a cryptographically sound relaxation allowing the wallet as a Soroban address-credential authorizer, whose signature Horizon enforces), a per-type operation allowlist keyed to declared contract IDs, hash idempotency backed by partial unique indexes plus a pre-insert check and unique-violation race backstop, per-wallet and per-IP throttling, and persist-before-submit so failures surface as TRANSACTION_PERSISTENCE_FAILED instead of fire-and-forget. Comprehensive unit tests were added covering each rejection branch and the happy path, satisfying the testing standard. Confidence is capped below 0.9 because key validation logic (assertWalletAuthorizes, extractInvocationAttributes) falls in the truncated portion of the diff and cannot be fully verified.

Gaps identified:

  • Contract-ID enforcement is conditional: if a contract-id env var is unset, the allowlist degrades to function-name-only matching, permitting invocations of attacker-deployed contracts whose functions share StepFi names
  • If invocation.contractId cannot be extracted but the function name matches, the contract check is silently skipped rather than failing closed
  • Persisted rows are not rolled back or marked failed when the subsequent Horizon submission fails, leaving stale 'pending' rows attributable to the submitting wallet
  • Full assertWalletAuthorizes implementation not visible in the provided diff (only test coverage attests to its behavior)

Audited by stepfi-audit-bot 🤖

@EmeditWeb

EmeditWeb commented Aug 26, 2026

Copy link
Copy Markdown
Member

@Marvelg256 fix Merge Conflicts and gaps identified in your codes

@EmeditWeb EmeditWeb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Automated Audit: does_not_solve

@Marvelg256 Please look into the issue again and address the gaps below.

The code changes substantively address all five root causes described in issue #117: source-account binding via assertWalletAuthorizes, operation allowlist per declared type, idempotency via hash uniqueness + pre-check, per-wallet rate limiting via WalletThrottlerGuard, and persistence-first flow. Tests cover every rejection branch. However, the independent verification reports merge conflicts with the base branch, and per the rules this means the PR cannot be approved. Additionally, the PR title is flagged as vague (shares no keywords with the linked issue), which is an author-fixable gap.

Gaps identified:

  • Merge conflicts with base branch must be resolved before merge
  • PR title should reference specific fix keywords from the issue (e.g. 'source-account binding', 'third-party XDR')

CI checks: ✅ PASSED: build-test
Merge conflicts: ⚠️ YES — this PR has conflicts with the base branch and cannot be merged.

Audited by stepfi-audit-bot 🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

critical: POST /transactions submits arbitrary third-party XDR with no source-account binding, idempotency, or rate limits

2 participants