nick-doc/09 - Audits/Logic Audit - 2026-05-24.md

title, tags, created, status

title	tags	created	status
Logic & Correctness Audit — 2026-05-24	audit logic correctness findings	2026-05-24	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:

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:

// 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.)

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

'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

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:

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:

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:

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

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

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:

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:

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