---
title: Logic & Correctness Audit — 2026-05-24
tags: [audit, logic, correctness, findings]
created: 2026-05-24
status: open
---

# Logic & Correctness Audit — 2026-05-24

Full-codebase review of business logic, state machines, race conditions, and data integrity. Triggered by completion of Request Network integration, Telegram first-class auth, and the internal funds ledger. Every finding is derived from actual source code — no hypothetical issues are included.

> [!danger] Action required
> 4 CRITICAL findings: two are exploitable in the current production code path (double-spend via concurrent webhooks, simulated-payment bypass); two cause data corruption (orphan User documents, non-atomic create/delete).

---

## CRITICAL

### CRIT-1 — Concurrent Webhooks Can Double-Process the Same Payment
**File:** `src/services/payment/shkeeper/shkeeperWebhook.ts:195–241` and `src/services/payment/paymentCoordinator.ts:25–60`
**Status:** Open

The duplicate-detection guard uses an in-memory 10-second window. There is no database-level atomic lock. Two simultaneous webhook deliveries for the same `external_id` will both read `payment` before either writes, both pass the in-memory deduplication check, both enter `PaymentCoordinator.coordinatePaymentUpdate`, and both proceed — because `coordinationState` is an in-memory `Map` whose `get/set` pair is not atomic under async concurrency (any `await` between them allows the event loop to interleave).

The `status === 'completed'` DB guard at `paymentCoordinator.ts:54` only protects re-processing of already-completed payments. The first webhook has not yet written `completed` when the second arrives, so both pass.

Side effects of double-processing:
- Duplicate chats created between buyer and seller (`chatService.createChat` has no uniqueness guard).
- Duplicate socket events sent to the buyer.
- Depending on timing, duplicate state transitions on `PurchaseRequest`.

**Remediation:** Replace the coordinator's check with an atomic MongoDB conditional update:
```ts
const result = await Payment.findOneAndUpdate(
  { _id: paymentId, status: { $ne: 'completed' } },
  { $set: { status: 'completed', ... } },
  { new: true }
);
if (!result) return; // already processed
```
For chat creation, add a unique compound index on `{ 'relatedTo.type': 1, 'relatedTo.id': 1 }` and handle the duplicate key error gracefully.

---

### CRIT-2 — Telegram Auto-Provisioning Race Creates Orphan User Documents
**File:** `src/services/auth/authController.ts:377–434`
**Status:** Open

The flow is:
1. `TelegramLink.findOne({ telegramUserId })` → null (new user)
2. `new User({...}).save()` — creates User document
3. `TelegramLink.findOneAndUpdate({ telegramUserId }, ..., { upsert: true })`

Two parallel Telegram auth requests for the same `telegramUserId` both find no link, both create a new `User`, and both try to upsert `TelegramLink`. Because `TelegramLink.telegramUserId` has `unique: true`, only one upsert wins. The loser's `User.save()` has already committed, producing a fully-active `User` document with no `TelegramLink` — it can never be authenticated via Telegram and will not be cleaned up.

**Remediation:** Use optimistic locking or a MongoDB transaction. Practical approach without transactions:
```ts
// Step 1: try to upsert the link first
const link = await TelegramLink.findOneAndUpdate(
  { telegramUserId },
  { $setOnInsert: { telegramUserId, source, ... } },
  { upsert: true, new: true, rawResult: true }
);
const isNew = link.lastErrorObject?.upserted != null;
// Step 2: create User only if this process won the upsert
if (isNew) { user = await User.create({...}); await link.value.updateOne({ userId: user._id }); }
else { user = await User.findById(link.value.userId); }
```

---

### CRIT-3 — Simulated Transaction Bypass Active in Production
**File:** `src/services/payment/paymentRoutes.ts:379–396`
**Status:** Open

*(Also cited in Security Audit C-3.)*

```ts
if (paymentHash.startsWith('SIM_') || ...) {
  isVerified = true;
}
```

No environment guard. A client sending `paymentHash: "SIM_anything"` gets a completed `Payment` and the `PurchaseRequest` advances without any real funds.

**Remediation:** Wrap in `if (process.env.NODE_ENV !== 'production')`.

---

### CRIT-4 — Hardcoded Credential in Production Payment Code
**File:** `src/services/payment/shkeeper/shkeeperPayoutService.ts:224–228`
**Status:** Open

*(Also cited in Security Audit C-5.)*

```ts
'Authorization': `Basic ${Buffer.from(`admin:!NMI4WdGkVQ#dQ`).toString('base64')}`
```

**Remediation:** `process.env.SHKEEPER_ADMIN_PASSWORD` — rotate immediately.

---

## HIGH

### HIGH-1 — `updatePurchaseRequest` Has a Read-Modify-Write Race Condition
**File:** `src/services/marketplace/PurchaseRequestService.ts:405–425`
**Status:** Open

```ts
const currentRequest = await PurchaseRequest.findById(requestId); // read
// ... validate status ...
await PurchaseRequest.findByIdAndUpdate(requestId, updateData, { new: true }); // write
```

Two concurrent calls both read the same `currentRequest.status`, both pass `isValidStatusProgression`, and both write — potentially into conflicting terminal states or setting the same field twice.

Furthermore, `isValidStatusProgression` allows any status → any terminal state transition (e.g., `pending` → `completed` in one step), because the guard only blocks backward progression among non-terminal states.

**Remediation:** Use atomic optimistic locking:
```ts
const updated = await PurchaseRequest.findOneAndUpdate(
  { _id: requestId, status: currentStatus }, // guard: status unchanged
  { $set: { status: newStatus } },
  { new: true }
);
if (!updated) throw new ConflictError('State changed concurrently — retry');
```
Additionally, tighten `isValidStatusProgression` to use an explicit allowed-transitions table rather than a blanket terminal-from-anywhere rule.

---

### HIGH-2 — Seller Offer Acceptance Has No Concurrency Guard
**File:** `src/services/marketplace/SellerOfferService.ts:355–426`
**Status:** Open

`acceptOffer` reads the offer, then updates it in a separate write. Two simultaneous accept calls for different offers on the same purchase request will both succeed: both set their target offer to `accepted`, and both run `updateMany` to reject all others — each query excluding its own `offerId`. The result is two `accepted` offers on the same purchase request.

**Remediation:** Use `findOneAndUpdate` with a status guard:
```ts
const updated = await SellerOffer.findOneAndUpdate(
  { _id: offerId, status: 'pending' },
  { $set: { status: 'accepted' } },
  { new: true }
);
if (!updated) throw new ConflictError('Offer already processed');
await SellerOffer.updateMany(
  { purchaseRequestId, _id: { $ne: offerId } },
  { $set: { status: 'rejected' } }
);
```
Consider wrapping in a MongoDB transaction to keep the accept + reject-others atomic.

---

### HIGH-3 — `refreshTokens[]` Array Grows Unboundedly
**File:** `src/services/auth/authController.ts:62–63` and login handler
**Status:** Open

Every `login` pushes a new refresh token onto `user.refreshTokens[]` without any cap or expiry pruning. The `User` model has no `maxlength` on this array. A user logging in from many devices, or an attacker rapidly requesting tokens, causes the array to grow without limit — increasing document size and slowing any query that reads the user document.

The `refreshToken` rotation endpoint prunes the old token on rotation, but `login` does not prune expired tokens before pushing.

**Remediation:** Before pushing, prune expired tokens:
```ts
const now = Date.now() / 1000;
user.refreshTokens = user.refreshTokens.filter(t => {
  try { return jwt.decode(t)?.exp > now; } catch { return false; }
});
```
Enforce a hard cap (e.g. `slice(-10)`) after pruning.

---

### HIGH-4 — Blocked Telegram User Can Bypass Block via Deleted TelegramLink
**File:** `src/services/auth/authController.ts:355–363`
**Status:** Open

The block check queries `TelegramLink` where `status = 'blocked'` AND `blockedReason != 'unlinked_by_user'`. If the blocked user's `TelegramLink` is **deleted** entirely (not just status-changed), the check finds nothing, `activeLink` finds nothing, and the code auto-provisions a brand-new `User` with a fresh `TelegramLink` — bypassing the block entirely.

**Remediation:** Store the blocked Telegram user ID on the `User` document (a `blockedTelegramIds` set at the app level, or a `status: 'suspended'` flag tied to the Telegram ID) so the block persists even without a `TelegramLink` record.

---

### HIGH-5 — `verifyEmailWithCode`: Non-Atomic User Create + TempVerification Delete
**File:** `src/services/auth/authController.ts:620–633`
**Status:** Open

```ts
await user.save();                                         // line 630
await TempVerification.findByIdAndDelete(tempVerification._id); // line 633
```

If `user.save()` succeeds but the delete fails (network error, timeout), the `TempVerification` document remains. The next call with the same `email+code` will find it valid, attempt `User.create` again, and hit the unique email index — returning a 500 error instead of a clean "already verified" message. This persists until the MongoDB TTL scan runs (up to 15 min + scan interval).

**Remediation:** Delete the TempVerification **before** saving the user, or check for duplicate-key errors after `user.save()` and delete the TempVerification in the catch handler.

---

## MEDIUM

### MED-1 — Funds Ledger Availability Check Is Not Atomic with Append
**File:** `src/services/payment/orchestration/releaseRefundService.ts:37–55,99–114`
**Status:** Open

`validateReleaseAvailability` reads a balance via aggregation, checks `availableBalance >= amount`, then `appendFundsLedgerEntry` inserts the entry. Two concurrent release/refund calls can both read the same balance snapshot, both pass the availability check, and both insert entries — resulting in a double-spend from the ledger.

The `idempotencyKey` on `FundsLedgerEntry` prevents exact duplicates when the same `txHash` is used, but does not prevent two different `txHash` values from both passing the availability check concurrently.

**Remediation:** Wrap the check + insert in a MongoDB transaction, or add a single-release constraint: before inserting a `release` entry, verify no prior `release` entry exists for the same `paymentId`.

---

### MED-2 — `updatePurchaseRequestStatus` Bypasses the State Machine Validator
**File:** `src/services/marketplace/PurchaseRequestService.ts:551–589`
**Status:** Open

`updatePurchaseRequestStatus` calls `PurchaseRequest.findByIdAndUpdate` directly, without calling `isValidStatusProgression`. The webhook path (`paymentCoordinator.ts:208`) uses this method, meaning payment-triggered state transitions skip the state machine guard entirely.

**Remediation:** Add `isValidStatusProgression(currentStatus, newStatus)` check inside `updatePurchaseRequestStatus` before writing.

---

### MED-3 — `getUserPayments` Queries Non-Existent Field `userId`
**File:** `src/services/payment/paymentService.ts:342–346`
**Status:** Open

```ts
query.userId = userId;
```

The `Payment` schema has no top-level `userId` field. The buyer is stored as `buyerId` and the seller as `sellerId`. This query will always return zero results for current payments; only legacy payments with `metadata.legacyUserId` are returned (none for new users). The endpoint appears non-functional for all users.

**Remediation:**
```ts
query.$or = [{ buyerId: userId }, { sellerId: userId }];
```

---

### MED-4 — Payout Created Without Verifying an Inbound Completed Payment Exists
**File:** `src/services/payment/shkeeper/shkeeperPayoutService.ts:42–80`
**Status:** Open

`createPayoutTask` takes `buyerId`, `sellerId`, `purchaseRequestId`, and `amount` from the request body. The service checks for duplicate payouts (`existingPayout` idempotency) but does not verify that a corresponding inbound `Payment` with `status: 'completed'` and `direction: 'in'` exists. An admin could accidentally initiate a payout for a purchase request that has never been paid.

**Remediation:**
```ts
const inboundPayment = await Payment.findOne({
  purchaseRequestId, direction: 'in', status: 'completed'
});
if (!inboundPayment) throw new AppError(400, 'No completed inbound payment for this request');
```

---

### MED-5 — Unauthenticated `/payment/callback` Endpoint Can Mutate Payment Status
**File:** `src/services/payment/paymentControllerRoutes.ts:20`
**Status:** Open

`POST /callback` is mounted with no authentication middleware. The handler accepts `{ transactionId, status, amount }` and updates a payment's status to any value without verification. Unlike the SHKeeper webhook (which has optional HMAC), this endpoint has zero authentication.

**Remediation:** Either remove the endpoint (the SHKeeper webhook path supersedes it), restrict to internal network only (Nginx `allow` rules), or add HMAC verification.

---

### MED-6 — In-Memory Payment Coordinator State Not Shared Across Replicas
**File:** `src/services/payment/paymentCoordinator.ts:24–25`
**Status:** Open

The `coordinationState` Map is process-local. In a multi-process deployment (PM2 cluster, k8s with >1 replica), the 10-second debounce window provides no protection — each process has its own empty map at startup, and webhooks routed to different replicas will both process. The existing DB `status === 'completed'` guard is the only durable protection.

*(The code comment at line 24 acknowledges this. Fix is the same as CRIT-1 — atomic DB conditional update makes this moot.)*

---

### MED-7 — Seller Offer Notification Uses Undefined `offer.title` Field
**File:** `src/services/marketplace/SellerOfferService.ts:403,415`
**Status:** Open

`SellerOffer` does not have a `title` field directly — the title belongs to the associated `PurchaseRequest`. `offer.title` resolves to `undefined` in the notification message body, resulting in broken notification text for buyers and sellers.

**Remediation:** Either populate the `purchaseRequestId` and use `purchaseRequest.title`, or populate the field name from the offer's own descriptive field.

---

## LOW

### LOW-1 — Duplicate Callback Route Definitions
**File:** `src/services/payment/paymentRoutes.ts:29,256`
**Status:** Open

`POST /callback` is defined at line 29 (mapped to `handlePaymentCallback`) and again as `POST /payments/callback` at line 256. These resolve to different URL paths given the router mount, but suggest dead code. No security impact; creates maintenance confusion.

**Remediation:** Remove the dead route definition.

---

### LOW-2 — `paymentCoordinator.ts` Map Grows Without Bound Until Cleanup Fires
**File:** `src/services/payment/paymentCoordinator.ts:35`
**Status:** Open

The cleanup function fires after each coordination cycle but only on active payments. Under a webhook flood before cleanup, the Map accumulates entries. Not a crash risk at current scale; becomes a concern at high webhook volume.

**Remediation:** The atomic DB fix in CRIT-1 eliminates the need for the Map entirely. Until then, add a size cap and a periodic cleanup interval independent of payment processing.

---

## Confirmed PASS

| Check | Result | Source |
|-------|--------|--------|
| TempVerification TTL index | PASS | `TempVerification.ts:59` — `expireAfterSeconds: 0` on expiry field |
| Legacy email token cleared on first use | PASS | `authController.ts:667` — token set to undefined before save |
| SHKeeper HMAC timing-safe comparison | PASS | `shkeeperWebhook.ts:84` — `crypto.timingSafeEqual` |
| Socket event DB as source of truth | PASS | DB write committed before emit; emit failure is non-fatal |
| `FundsLedgerEntry` idempotency key (unique) | PASS | `FundsLedgerEntry.ts:86` — sparse unique index |
| `TelegramSession` TTL index | PASS | `TelegramSession.ts:66` |
| Password change clears refresh tokens | PASS | `authController.ts:887` |

---

## Summary

| Severity | Count |
|----------|-------|
| CRITICAL | 4 |
| HIGH | 5 |
| MEDIUM | 7 |
| LOW | 2 |

**Top priority:** CRIT-1 (concurrent webhook double-processing) and CRIT-2 (Telegram race condition) require atomic DB operations. CRIT-3 (simulation bypass) and CRIT-4 (hardcoded credential) are one-line fixes.

---

## Related

- [[Security Audit - 2026-05-24]]
- [[Performance Audit - 2026-05-24]]
- [[Funds Ledger and Escrow State Machine Specification]]
- [[Payment Provider Adapter Spec]]
- [[Authentication Flow]]
- [[Platform Logical Audit - 2026-05-24]]