fix: fail closed on contract allowlist and mark records failed on Horizon rejection (#117)

Closes #117

Addresses the audit gaps on the secured submit endpoint: the per-type
contract check no longer degrades to function-name-only matching when the
contract ID is unset or unextractable, and persisted records are marked
failed when Horizon rejects the transaction instead of lingering as stale
pending rows. Also resolves the committed merge-conflict markers in the
progress tracker.

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>

Author: Marvelg256

context/progress-tracker.md

@@ -6,6 +6,66 @@ pure chore/docs commits). Direct pushes to main must also be logged here.
 
 ---
 
+## 2026-08-27
+
+- Closed the audit gaps on `POST /transactions/submit` (#117):
+  - **Fail-closed contract allowlist** — the contract ID for the declared
+    type must be configured and the XDR must target it. Function-name-only
+    matching was removed: an unset contract ID now rejects with
+    `TRANSACTION_CONTRACT_NOT_CONFIGURED`, and an invocation whose target
+    contract cannot be determined from the XDR rejects with
+    `TRANSACTION_TYPE_MISMATCH` instead of silently skipping the check.
+  - **No stale pending rows on submission failure** — when Horizon rejects
+    the transaction (or submission fails unexpectedly), the persisted record
+    is marked `failed` with the mapped error message and `completed_at`, so
+    the row no longer lingers as `pending` attributable to the submitting
+    wallet. Transient network unavailability (503) leaves the row `pending`
+    for the status checker to reconcile, since the transaction may still be
+    in flight.
+  - Resolved the committed merge-conflict markers in this file (stale
+    StepFi-Contracts content from the wrong repo removed; StepFi-API history
+    retained).
+
+## 2026-08-26
+
+- Fixed registration race conditions in `AuthService.register()` by eliminating application-side pre-checks (`findByWallet`, `checkUsernameExists`) and relying directly on DB-level UNIQUE constraints (`users.wallet_address`, `users.username`).
+- Added idempotent migration `20260826130000_ensure_users_unique_constraints.sql` to ensure unique indexes exist on `users.wallet_address` and `users.username`.
+- Updated `UsersRepository.createProfile()` to catch PostgreSQL unique constraint violation error `23505` and map to structured 409 `ConflictException` (`AUTH_WALLET_EXISTS`, `AUTH_USERNAME_TAKEN`).
+- Added cleanup handlers (`deleteAvatar`, `deleteUserById`) in `AuthService.register()` and `UsersRepository` to ensure failed registrations do not leave orphaned avatar files or partial user records.
+
+## 2026-08-25
+
+- Secured `POST /transactions/submit` (#117):
+  - **Source binding** — the authenticated wallet must be the transaction
+    source account (or the inner source for fee-bump transactions), or must
+    appear as an authorized address in the Soroban invocation auth. Third-party
+    XDR where the wallet is neither source nor authorizer is rejected with
+    `TRANSACTION_SOURCE_MISMATCH`. (Deposit/withdraw/repay/vendor XDRs built
+    by this API use a random source account and authorize via Soroban auth, so
+    the auth check keeps those flows working.)
+  - **Operation allowlist per type** — every operation must be a Soroban
+    `invokeHostFunction` whose function name matches the declared type
+    (`create_loan`, `repay_loan`/`repay_installment`, `deposit`, `withdraw`,
+    `approve_vendor`, `suspend_vendor`) and must target the contract owned by
+    that flow. Rejections use `TRANSACTION_TYPE_MISMATCH` /
+    `TRANSACTION_OPERATION_NOT_ALLOWED`.
+  - **Idempotency** — migration
+    `20260825000001_add_unique_transaction_hash.sql` 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 it
+    (`duplicate: true`) instead of re-submitting, with the unique-constraint
+    violation as the concurrency backstop.
+  - **Rate limits** — `WalletThrottlerGuard` keys `@nestjs/throttler` on the
+    authenticated wallet; the submit route is limited to 10 req / 60 s per
+    wallet AND per IP (global guard), matching the auth-endpoint pattern.
+  - **Persistence-first** — the local record is written (await) before the
+    Horizon submission, so persistence failures surface as
+    `TRANSACTION_PERSISTENCE_FAILED` instead of being silently dropped, and
+    the transaction hash is always known to the status checker / indexer.
+  - Updated `SubmitTransactionResponseDto` (`status` may reflect the recorded
+    status, plus `duplicate` flag), controller Swagger, and unit tests covering
+    every rejection branch plus the happy path.
+
 ## 2026-08-24
 
 - **Session families + refresh-token replay detection** (`sessions.family_id`
@@ -27,13 +87,6 @@ pure chore/docs commits). Direct pushes to main must also be logged here.
   event, blocked-user denial within TTL bound, cache expiry re-query,
   cleanup job deletes-only-expired.
 
-## 2026-08-26
-
-- Fixed registration race conditions in `AuthService.register()` by eliminating application-side pre-checks (`findByWallet`, `checkUsernameExists`) and relying directly on DB-level UNIQUE constraints (`users.wallet_address`, `users.username`).
-- Added idempotent migration `20260826130000_ensure_users_unique_constraints.sql` to ensure unique indexes exist on `users.wallet_address` and `users.username`.
-- Updated `UsersRepository.createProfile()` to catch PostgreSQL unique constraint violation error `23505` and map to structured 409 `ConflictException` (`AUTH_WALLET_EXISTS`, `AUTH_USERNAME_TAKEN`).
-- Added cleanup handlers (`deleteAvatar`, `deleteUserById`) in `AuthService.register()` and `UsersRepository` to ensure failed registrations do not leave orphaned avatar files or partial user records.
-
 ## 2026-07-23
 
 - Added GitHub Actions health check workflow (`health-check.yml`) to ping the Render API every 6 hours to prevent the free tier instance from sleeping. Auto-creates or comments on issues with the `incident` label if the ping fails, preventing silent outages.

src/modules/transactions/dto/submit-transaction-request.dto.ts

@@ -23,12 +23,21 @@ export enum TransactionType {
  * - The declared `type` must match the operations contained in the XDR
  *   (e.g. `deposit` must be a Soroban `deposit` invocation on the liquidity
  *   pool contract). Mismatches are rejected with `TRANSACTION_TYPE_MISMATCH`
- *   or `TRANSACTION_OPERATION_NOT_ALLOWED`.
+ *   or `TRANSACTION_OPERATION_NOT_ALLOWED`. The allowlist is fail-closed on
+ *   the contract ID: when the contract for the flow is not configured on the
+ *   server the submission is rejected with `TRANSACTION_CONTRACT_NOT_CONFIGURED`
+ *   (never falling back to function-name-only matching), and an invocation
+ *   whose target contract cannot be determined from the XDR is rejected with
+ *   `TRANSACTION_TYPE_MISMATCH`.
  * - Submission is idempotent per transaction hash: re-submitting an already
  *   recorded hash returns the original record without a second Horizon
  *   submission (`duplicate: true` on the response).
  * - The local record is persisted before the Horizon submission, so
  *   persistence failures surface as errors rather than being silently dropped.
+ *   When Horizon rejects the transaction (or submission fails unexpectedly),
+ *   the persisted record is marked `failed` with the mapped error message;
+ *   transient network unavailability leaves it `pending` for the status
+ *   checker to reconcile.
  */
 export class SubmitTransactionRequestDto {
   @ApiProperty({

src/modules/transactions/transactions.controller.ts

@@ -57,8 +57,8 @@ export class TransactionsController {
   })
   @ApiResponse({ status: 401, description: 'Unauthorized - missing or invalid JWT' })
   @ApiResponse({ status: 429, description: 'Too many requests - rate limit exceeded (per wallet or per IP)' })
-  @ApiResponse({ status: 500, description: 'Failed to persist the transaction record locally (TRANSACTION_PERSISTENCE_FAILED)' })
-  @ApiResponse({ status: 503, description: 'Stellar network temporarily unavailable' })
+  @ApiResponse({ status: 500, description: 'Failed to persist the transaction record locally (TRANSACTION_PERSISTENCE_FAILED) or an unexpected Stellar submission failure (STELLAR_SUBMISSION_FAILED)' })
+  @ApiResponse({ status: 503, description: 'Stellar network temporarily unavailable, or the contract for the declared type is not configured on the server (TRANSACTION_CONTRACT_NOT_CONFIGURED)' })
   async submitTransaction(
     @CurrentUser() user: { wallet: string },
     @Body() dto: SubmitTransactionRequestDto,

src/modules/transactions/transactions.service.ts

@@ -165,8 +165,9 @@ export class TransactionsService {
     // 4. Persist first so persistence failures surface instead of being
     //    silently dropped. The unique hash indexes backstop the check above
     //    against concurrent duplicate submissions.
+    let lookupColumn: TransactionLookupColumn | null = null;
     try {
-      await this.persistTransactionRecord(wallet, transactionHash, dto.type, dto.xdr);
+      lookupColumn = await this.persistTransactionRecord(wallet, transactionHash, dto.type, dto.xdr);
     } catch (error) {
       if (this.isUniqueViolationError(error)) {
         const existingAfterRace = await this.findTransactionRecord(transactionHash);
@@ -187,9 +188,14 @@ export class TransactionsService {
 
     // 5. Submit to Horizon only after the local record exists, so the
     //    transaction hash is always known to the status checker / indexer.
+    //    When Horizon rejects the transaction (or submission fails
+    //    unexpectedly), the persisted record is marked failed so it does not
+    //    linger as a stale `pending` row attributable to the submitting
+    //    wallet.
     try {
       await this.horizonServer.submitTransaction(transaction);
     } catch (error) {
+      await this.markTransactionFailed(lookupColumn, transactionHash, error);
       this.handleHorizonError(error);
     }
 
@@ -291,8 +297,29 @@ export class TransactionsService {
         });
       }
 
+      // Fail closed on the contract ID: the allowlist never degrades to
+      // function-name-only matching. If the contract for this flow is not
+      // configured, submissions of this type are disabled rather than
+      // accepting invocations of attacker-deployed contracts whose functions
+      // share StepFi names.
       const expectedContractId = this.configService.get<string>(allowlist.contractIdKey);
-      if (expectedContractId && invocation.contractId && invocation.contractId !== expectedContractId.trim()) {
+      if (!expectedContractId) {
+        throw new ServiceUnavailableException({
+          code: 'TRANSACTION_CONTRACT_NOT_CONFIGURED',
+          message: `The contract for transaction type '${type}' is not configured on the server. Submission is disabled until the contract ID is set.`,
+        });
+      }
+
+      // The target contract must be determinable from the XDR — a matching
+      // function name is not enough.
+      if (!invocation.contractId) {
+        throw new BadRequestException({
+          code: 'TRANSACTION_TYPE_MISMATCH',
+          message: `Could not determine the target contract address for transaction type '${type}'.`,
+        });
+      }
+
+      if (invocation.contractId !== expectedContractId.trim()) {
         throw new BadRequestException({
           code: 'TRANSACTION_TYPE_MISMATCH',
           message: `Transaction type '${type}' must invoke contract ${expectedContractId}, but the XDR targets ${invocation.contractId}.`,
@@ -426,7 +453,17 @@ export class TransactionsService {
     return code === '23505' || message.includes('duplicate key value violates unique constraint');
   }
 
-  private handleHorizonError(error: unknown): never {
+  /**
+   * Maps a Horizon submission error to the HTTP status, typed code, and
+   * user-facing message that should be returned to the client. Shared by
+   * `handleHorizonError` (which throws) and `markTransactionFailed` (which
+   * records the same message on the local row).
+   */
+  private describeHorizonError(error: unknown): {
+    httpStatus: number;
+    code: string;
+    message: string;
+  } {
     const err = error as {
       response?: { data?: { extras?: { result_codes?: { transaction?: string; operations?: string[] } } } };
       message?: string;
@@ -441,40 +478,102 @@ export class TransactionsService {
 
       for (const code of allCodes) {
         if (code && HORIZON_ERROR_MAP[code]) {
-          throw new BadRequestException({
+          return {
+            httpStatus: 400,
             code: `STELLAR_${code.toUpperCase()}`,
             message: HORIZON_ERROR_MAP[code],
-          });
+          };
         }
       }
 
-      throw new BadRequestException({
+      return {
+        httpStatus: 400,
         code: 'STELLAR_TRANSACTION_FAILED',
         message: `Transaction rejected by the Stellar network: ${allCodes.join(', ')}`,
-      });
+      };
     }
 
     const message = err?.message ?? 'Unknown error';
     if (message.toLowerCase().includes('timeout') || message.toLowerCase().includes('network')) {
-      throw new ServiceUnavailableException({
+      return {
+        httpStatus: 503,
         code: 'STELLAR_NETWORK_UNAVAILABLE',
         message: 'Stellar network is temporarily unavailable. Please try again later.',
-      });
+      };
     }
 
     this.logger.error(`Horizon submission error: ${message}`);
-    throw new InternalServerErrorException({
+    return {
+      httpStatus: 500,
       code: 'STELLAR_SUBMISSION_FAILED',
       message: 'Failed to submit transaction to the Stellar network. Please try again.',
-    });
+    };
+  }
+
+  private handleHorizonError(error: unknown): never {
+    const details = this.describeHorizonError(error);
+
+    if (details.httpStatus === 503) {
+      throw new ServiceUnavailableException({ code: details.code, message: details.message });
+    }
+    if (details.httpStatus === 500) {
+      throw new InternalServerErrorException({ code: details.code, message: details.message });
+    }
+    throw new BadRequestException({ code: details.code, message: details.message });
+  }
+
+  /**
+   * Best-effort update of the persisted record when Horizon rejected the
+   * transaction or submission failed unexpectedly, so the row does not linger
+   * as a stale `pending` record attributable to the submitting wallet. On
+   * transient network unavailability (503) the outcome is unknown — the
+   * transaction may still be in flight — so the row is left `pending` for the
+   * status checker to reconcile. Never throws: the original Horizon error is
+   * what surfaces to the client.
+   */
+  private async markTransactionFailed(
+    lookupColumn: TransactionLookupColumn | null,
+    hash: string,
+    error: unknown,
+  ): Promise<void> {
+    if (!lookupColumn) {
+      return;
+    }
+
+    const details = this.describeHorizonError(error);
+    if (details.httpStatus === 503) {
+      return;
+    }
+
+    try {
+      const failedAt = new Date().toISOString();
+      const client = this.supabaseService.getServiceRoleClient();
+      const { error: updateError } = await client
+        .from('transactions')
+        .update({
+          status: 'failed',
+          error: details.message,
+          completed_at: failedAt,
+          updated_at: failedAt,
+        })
+        .eq(lookupColumn, hash);
+
+      if (updateError) {
+        this.logger.warn(`Failed to mark transaction ${hash} as failed: ${updateError.message}`);
+      }
+    } catch (persistError) {
+      this.logger.warn(
+        `Failed to mark transaction ${hash} as failed: ${(persistError as Error).message}`,
+      );
+    }
   }
 
   private async persistTransactionRecord(
     wallet: string,
     hash: string,
     type: TransactionType,
     xdr: string,
-  ): Promise<void> {
+  ): Promise<TransactionLookupColumn> {
     const client = this.supabaseService.getServiceRoleClient();
     const submittedAt = new Date().toISOString();
     const transactionHashPayload: Record<string, unknown> = {
@@ -491,7 +590,7 @@ export class TransactionsService {
       .insert(transactionHashPayload);
 
     if (!transactionHashError) {
-      return;
+      return 'transaction_hash';
     }
 
     if (!this.isUnknownColumnError(transactionHashError)) {
@@ -512,6 +611,8 @@ export class TransactionsService {
     if (legacyHashError) {
       throw new Error(legacyHashError.message ?? 'Supabase insert failed');
     }
+
+    return 'hash';
   }
 
   private async findTransactionRecord(hash: string): Promise<TransactionRecord | null> {