Files
nick-doc/09 - Audits/Platform Logical Audit - 2026-05-24.md
Siavash Sameni 5b93b2d23e docs: add comprehensive logical audit report
Adds a full cross-document audit covering:
- Data Models (broken refs, ghost states, missing constraints)
- API Reference (unauthenticated endpoints, field mismatches, missing pagination)
- Architecture (fictitious deps, statelessness claims vs reality)
- Flows (race conditions, missing failure paths, auth bypasses)
- Security (passkey stubs, JWT storage, webhook verification)

32 findings organized by severity with recommended fixes.
2026-05-24 08:03:20 +04:00

436 lines
20 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Platform Logical Audit
**Date:** 2026-05-24
**Scope:** Data Models, API Reference, Architecture, Flows, Security
**Method:** Cross-document consistency check, state-machine validation, authorization gap analysis, dependency verification
---
## Executive Summary
This audit identifies critical logical contradictions, security holes, and cross-document inconsistencies across the Amn platform documentation. The most severe findings are:
1. **Dispute/escrow race condition** allowing fund release while a dispute is active.
2. **Three mutually incompatible `Dispute` status/action enums** across the Data Model, API Reference, and Flow documents — the documented API cannot be implemented against the documented schema.
3. **Passkey (WebAuthn) implementation is cryptographically broken** and unusable in production.
4. **Multiple financial endpoints require no authentication**, allowing unauthenticated payment record injection and private data exfiltration.
5. **Web3 payment verification only checks `receipt.status`**, not recipient address or amount, making payment fraud trivial.
---
## 🔴 Critical Issues
### 1. Dispute Does Not Auto-Pause Escrow (Funds-at-Risk)
**Files:** `04 - Flows/Dispute Flow.md`, `03 - API Reference/Dispute API.md`
**Finding:** The Dispute Flow explicitly states: *"Today, opening a dispute does **not** flip `Payment.escrowState` away from `funded`. An admin could theoretically still release the escrow before resolving the dispute."*
At the same time, the Dispute API claims dispute creation *"Pauses any in-flight payout (sets a hold flag)."*
**Impact:** A buyer opens a dispute; an admin (or malicious/compromised admin) releases the escrow before resolution. The seller receives funds while the dispute is still open.
**Required Fix:** Introduce a `disputed` escrow state (or `disputeHold` flag) that blocks all release/refund operations until the dispute is resolved. The hold must be enforced in `PaymentCoordinator`, not just in controller logic.
---
### 2. Passkey Authentication Is Completely Broken
**Files:** `04 - Flows/Passkey (WebAuthn) Flow.md`, `02 - Data Models/User.md`
**Findings:**
- **Attestation is stubbed:** The `publicKey` field is stored as the literal string `'simulated-public-key'`. A malicious client can register any credential ID under any user account.
- **Refresh tokens are not persisted:** Passkey-issued refresh tokens are never appended to `user.refreshTokens[]`. Standard `/api/auth/refresh-token` will reject them, breaking session continuity.
- **In-memory challenge store:** `storedChallenges` is a `Map` in process memory. In a horizontally scaled deployment, the challenge created on instance A can only be verified on instance A. Load-balancer round-robin breaks authentication entirely.
**Impact:** Passkey auth is trivially bypassable and non-functional at scale.
**Required Fix:** Replace stub with `@simplewebauthn/server` (or equivalent), persist real public keys, store refresh tokens in the user record, and move challenges to Redis with TTL.
---
### 3. Unauthenticated State-Mutation Endpoints
**Files:** `03 - API Reference/Payment API.md`, `03 - API Reference/AI API.md`, `03 - API Reference/Notification API.md`
**Findings:**
| Endpoint | Risk |
|----------|------|
| `POST /api/payment/decentralized/save` | Anyone can persist a Web3 payment record |
| `POST /api/payment/decentralized/update` | Anyone can update decentralized payment status/confirmations |
| `GET /api/payment/decentralized/history/:userId` | Anyone can read any user's payment history (privacy breach) |
| `POST /api/payment/shkeeper/create-test-payment` | Anyone can inject test payment records into production data |
| `POST /api/payment/decentralized/verify/:paymentId` | Anyone can trigger chain re-verification |
| `POST /api/payment/decentralized/verify-all-pending` | Anyone can trigger a global batch verification job |
| All AI endpoints (`/generate`, `/analyze`, `/translate`, `/assist`) | No caller identity; unlimited OpenAI cost abuse |
| Legacy notification router (`notification/routes.ts`) | Mounted without auth; accepts `?userId=` query parameter allowing notification read/modify for any user |
**Impact:** Data poisoning, privacy violations, and unbounded cost exposure.
**Required Fix:** Add Bearer JWT middleware to all endpoints above. Enforce ownership or admin-role checks on `:userId` parameterized endpoints.
---
### 4. Web3 Payment Verification Trusts the Wrong Data
**Files:** `04 - Flows/Payment Flow - DePay & Web3.md`
**Finding:** The verifier checks only `receipt.status === '0x1'`. It **does not** decode the `Transfer` event to verify:
- `to` address == `ESCROW_WALLET_ADDRESS`
- `value` >= expectedAmount (accounting for decimals)
**Impact:** A malicious actor can submit the hash of any successful transaction (e.g., a 0.01 USDT transfer to their own wallet) and the system will mark the payment as completed.
**Required Fix:** Decode the event logs; verify recipient, token contract address, and amount match the intent.
---
### 5. Three Mutually Incompatible Dispute Enum Sets
**Files:** `02 - Data Models/Dispute.md`, `03 - API Reference/Dispute API.md`, `04 - Flows/Dispute Flow.md`
**Finding:** The three documents describe three different `Dispute` systems:
| Source | Status Enum | Resolution Action Enum |
|--------|-------------|------------------------|
| **Data Model** | `pending`, `in_progress`, `waiting_response`, `resolved`, `rejected`, `closed` | `refund`, `replacement`, `compensation`, `warning_seller`, `ban_seller`, `no_action` |
| **API Reference** | `open`, `under_review`, `resolved_buyer`, `resolved_seller`, `closed` | `buyer`, `seller`, `split` |
| **Flow** | `pending`, `in_progress`, `resolved`, `closed` | `refund`, `partial`, `release`, `reject` |
**Impact:** The API cannot be implemented as documented against the data model. The request/response schemas for `POST /api/disputes/:id/resolve` are completely different between API (uses `decision`) and Flow (uses `action`).
**Required Fix:** Create a single source-of-truth enum set. Align all three documents (or generate API docs from the model). `ban_seller` also has no corresponding `User.status` value (`User.status` is `active | suspended | deleted`).
---
## 🟠 High Severity Issues
### 6. Payment Model Uses `Mixed` for Core Foreign Keys
**File:** `02 - Data Models/Payment.md`
`purchaseRequestId`, `sellerOfferId`, and `sellerId` are `Schema.Types.Mixed` (ObjectId *or* String) to support the template flow. This sacrifices all type safety, referential integrity, and indexing efficiency for the normal (non-template) payment path, which is the vast majority of transactions.
**Fix:** Use strict `ObjectId` refs for the standard path; store template linkage in a separate `metadata` field.
---
### 7. Broken Foreign Key References
**Files:** `02 - Data Models/PointTransaction.md`, `02 - Data Models/Chat.md`
- `PointTransaction.order` references an **`Order` model that does not exist** in any documented schema.
- `Chat.relatedTo.type` includes `Transaction`, but no `Transaction` model exists.
**Fix:** Remove or rename orphaned references. If `Order` is an internal alias, document it.
---
### 8. Delivery Confirmation Code Is Brute-Forceable
**Files:** `04 - Flows/Delivery Confirmation Flow.md`
The 6-digit code (`Math.floor(100000 + Math.random()*900000)`) has only 900,000 combinations. There is **no rate limiting** documented on verification attempts.
**Fix:** Add Redis-backed rate limiting (e.g., 5 attempts per 15 minutes per request). Consider increasing entropy or adding time-based invalidation.
---
### 9. Purchase Request Status Machine Is Inconsistent
**Files:** `02 - Data Models/PurchaseRequest.md`, `03 - API Reference/Marketplace API.md`, `04 - Flows/Purchase Request Flow.md`
Three different status enums:
| Source | Unique Values |
|--------|---------------|
| **Data Model** | `pending_payment`, `active`, `seller_paid` |
| **API** | `draft` |
| **Flow** | `finalized`, `archived` |
The flow also claims a backward transition `in_negotiation → received_offers` is valid, but states elsewhere that "progression is forward-only except into terminal status."
**Fix:** One canonical enum. Remove ghost states (`draft`, `finalized`, `archived`, `pending_payment`, `active`) or implement them consistently.
---
### 10. JWTs in `localStorage` with 7-Day Expiry
**Files:** `01 - Architecture/Security Architecture.md`
Access tokens are stored in `localStorage` (XSS theft vector) and expire in **7 days**. For a financial escrow platform, this is far too long. The docs admit the risk but only list secondary mitigations (CSP, audits).
**Fix:** Move tokens to `httpOnly` cookies with 1560 minute expiry and implement a secure refresh-token rotation mechanism.
---
### 11. SHKeeper Webhook Swallows All Errors
**File:** `04 - Flows/Payment Flow - SHKeeper.md`
The webhook returns `202 Accepted` even on signature failures, DB errors, and unknown payments. Failures are silently swallowed with no dead-letter queue, retry mechanism, or alerting.
**Fix:** Differentiate `400` (client error, do not retry) from `500` (server error, retry). Log structured webhook failures to a DLQ or alerting channel.
---
### 12. Socket.IO Room Joining Is Client-Controlled
**Files:** `01 - Architecture/Real-time Layer.md`
Clients explicitly emit `join-user-room {userId}` with their own ID. The docs include a warning that `userId` arguments "are NOT trusted blindly" and add "(cite: needs verification in `socketService.ts`)." This is an explicit admission the authorization check may not be implemented. If missing, any authenticated user can subscribe to another user's private notifications.
**Fix:** Remove client-driven `join-user-room` events. The server should automatically join the socket to `user-{decoded.id}` immediately after handshake auth.
---
### 13. Rate Limiting Is Effectively Disabled
**Files:** `01 - Architecture/Backend Architecture.md`, `03 - API Reference/API Overview.md`
`express-rate-limit` is disabled by default. Protected paths are limited to auth OTP/resend and AI endpoints. Marketplace, payment, chat, file upload, and notification endpoints have **no documented rate limiting**.
**Fix:** Enable global rate limiting with tiered limits (stricter for auth/financial, moderate for chat/marketplace).
---
## 🟡 Medium Severity Issues
### 14. Architecture Claims Stateless, But Isn't
**Files:** `01 - Architecture/System Architecture.md`, `01 - Architecture/Backend Architecture.md`, `01 - Architecture/Real-time Layer.md`
- Claims backend is **stateless** (JWT-only, no sessions).
- Admits Redis stores **login-attempt lockout counters** (session state).
- Socket.IO requires **sticky sessions** for multi-node, but current infra is a single Docker host. Running `N=2` backend replicas would break real-time events.
**Fix:** Remove the "stateless" claim until auth state and Socket.IO are node-agnostic (Redis adapter + cookie-based sticky sessions).
---
### 15. Fictitious Dependencies
**Files:** `01 - Architecture/Frontend Architecture.md`, `01 - Architecture/Infrastructure.md`
- **Next.js 16** does not exist (latest stable is 15). Using an unverified framework version for a financial platform is high-risk.
- **Redis 8** (`redis:8-alpine`) does not exist (latest stable is 7.x).
**Fix:** Update docs to reflect actual tested versions.
---
### 16. Blog Post `viewsCount` Incremented on GET
**File:** `03 - API Reference/Blog API.md`
`GET /api/blog/posts/:slug` atomically increments `viewsCount`. GET requests should be idempotent and safe.
**Fix:** Move view counting to a `POST /api/blog/posts/:slug/view` beacon endpoint, or use an async analytics pipeline.
---
### 17. API Request/Response Fields Do Not Match Data Models
**File:** `03 - API Reference/*`
| API | Uses | Model Has |
|-----|------|-----------|
| Address (`POST /api/addresses`) | `fullName`, `street`, `postalCode`, `isPrimary` | `name`, `fullAddress`, `zipCode`, `primary` |
| Blog (`POST /api/blog/posts`) | `excerpt`, `isFeatured`, `videoUrl`, `metadata` | `description`, `featured`, `videos[]`, no `metadata` |
| Chat (`POST /api/chat`) | `title` | `name` |
| Chat message (`POST /api/chat/:id/messages`) | `type`, `replyToMessageId` | `messageType`, `replyTo` |
| Payment (multiple) | flat `amount: number` | nested `amount: { amount, currency }` |
| Notification (`POST /api/notifications`) | `body`, `type` (domain) | `message`, `type` (severity), `category` (domain) |
| User profile (`PUT /api/user/profile`) | `name` (alias for `profile.name`) | No `profile.name` field exists |
| Points (`POST /api/points/admin/add`) | `reason` | `description` (required) |
**Fix:** Align API specs to the data models, or vice versa, with a single source of truth.
---
### 18. Seller Can Update Price After Offer Acceptance
**File:** `04 - Flows/Negotiation Flow.md`
The flow admits: *"`updateOffer` does not enforce status... Current code allows the price change, which is dangerous post-payment."*
**Fix:** Reject `updateOffer` if `status !== 'pending'`.
---
### 19. Buyer Can Confirm Delivery Before Seller Ships
**File:** `04 - Flows/Delivery Confirmation Flow.md`
Preconditions list `payment`, `processing`, or `delivery`. The "manual fast-track" lets the buyer skip the code and confirm receipt at any time — including immediately after payment, before the seller acknowledges.
**Fix:** Restrict fast-track confirmation to status `delivery` only, or require admin override.
---
### 20. Payment `confirmed` Status Is a Ghost State
**Files:** `02 - Data Models/Payment.md`, `04 - Flows/Payment Flow - SHKeeper.md`
The model includes `confirmed` in `status`. The SHKeeper webhook maps `PAID` directly to `completed`, skipping `confirmed`. No automated flow appears to set it.
**Fix:** Remove `confirmed` from the enum, or document the transition that sets it.
---
### 21. `partial` Escrow State Does Not Exist in Model
**Files:** `04 - Flows/Escrow Flow.md`, `04 - Flows/Payment Flow - SHKeeper.md`, `02 - Data Models/Payment.md`
Both flows use `escrowState: "partial"` for overpayments. The `Payment` model enum is `funded | releasable | released | refunded | releasing | failed``partial` is absent.
**Fix:** Add `partial` to the model enum, or map overpayments to `funded` with a `overpaidAmount` metadata field.
---
### 22. `BlogPost.comments` Is Just a Number
**File:** `02 - Data Models/BlogPost.md`
There is no `Comment` model. The system counts comments but cannot store or retrieve actual comment content.
**Fix:** Implement a `Comment` collection, or remove the `comments` counter.
---
### 23. No Pagination on List-Oriented Endpoints
**File:** `03 - API Reference/*`
- `GET /api/marketplace/sellers`
- `GET /api/marketplace/purchase-requests/my`
- `GET /api/marketplace/purchase-requests/:id/offers`
- `GET /api/blog/posts/featured`
- `GET /api/points/leaderboard`
- `GET /api/users/contacts`
**Fix:** Add cursor- or offset-based pagination to all unbounded list endpoints.
---
### 24. Missing Array Defaults
**Files:** `02 - Data Models/*`
Across 5+ models, array fields (`offers[]`, `tags[]`, `attachments[]`, `evidence[]`, `timeline[]`, `participants[]`, etc.) have no default value. In Mongoose they default to `undefined`, causing runtime errors on `.push()` or `.length`.
**Fix:** Add `default: []` to all array fields.
---
### 25. Inconsistent Soft-Delete Patterns
**Files:** `02 - Data Models/*`
| Model | Pattern |
|-------|---------|
| User | `status: active / suspended / deleted` |
| BlogPost | `status: draft / published / archived` |
| RequestTemplate, Category, LevelConfig | `isActive` boolean |
| Dispute | `status` enum (no deleted state) |
| SellerOffer | `status` enum |
**Fix:** Standardize on one pattern (e.g., `status` enum with `deleted` or `archived`, plus `deletedAt` timestamp).
---
### 26. Frontend Docker Image Cannot Be Promoted Across Environments
**File:** `01 - Architecture/Frontend Architecture.md`
The frontend uses `process.env.NEXT_PUBLIC_API_URL`, which is baked at **build time**. The same Docker image cannot be promoted from dev → staging → prod without rebuilding.
**Fix:** Inject the API URL at runtime via a server-side config endpoint or an env-replacement script in the container entrypoint.
---
### 27. Watchtower Auto-Deploys with Zero Staging Gate
**File:** `01 - Architecture/Infrastructure.md`
Production auto-updates every 5 minutes on `latest` tag change with no health-check gate, blue/green rollout, or smoke test.
**Fix:** Implement a staging promotion pipeline: build → deploy to staging → smoke test → tag promote to `stable` → Watchtower watches `stable`, not `latest`.
---
## 🟢 Lower Severity / Polish Issues
### 28. Naming Inconsistencies
**Files:** `02 - Data Models/*`
- `PointTransaction.user` vs `*Id` suffix convention used everywhere else
- `User.referredBy` vs `referredById`
- `User.profile.phone` vs `phoneNumber` used in `Address` and `PurchaseRequest`
- `Chat.messages[].senderType` defaults to `User` (PascalCase) while 95% of enums use lowercase
- `Address.addressType` uses `Home` / `Office` / `Other` (PascalCase)
- `Chat.metadata.createdBy` lacks `Id` suffix
- `Dispute.resolution.resolvedBy` lacks `Id` suffix
### 29. Redundant / Derived Fields
**Files:** `02 - Data Models/PurchaseRequest.md`, `02 - Data Models/User.md`
- `PurchaseRequest.deliveryConfirmed` boolean + `deliveryConfirmedAt` — boolean is derivable
- `PurchaseRequest.deliveryInfo.deliveryCodeUsed` boolean — derivable from `deliveryCodeUsedAt`
- `User.points.total` — derivable from `available + used`; risks arithmetic drift
- `PurchaseRequest.rating` + `feedback` — duplicate the dedicated `Review` model (two sources of truth)
### 30. ER Diagram Errors
**File:** `02 - Data Models/Data Model Overview.md`
- `PURCHASE_REQUEST ||--o| REVIEW` implies max one review; the model allows many
- `CHAT ||--o{ DISPUTE` direction is reversed; a chat belongs to at most one dispute
### 31. Missing Audit Timestamps
**Files:** `02 - Data Models/*`
- `User` has `status: deleted` but no `deletedAt`
- `PurchaseRequest` has terminal states (`completed`, `seller_paid`, `cancelled`) but no `completedAt` / `cancelledAt`
- `Review` has `pending → published → rejected` workflow but no transition timestamps
- `Dispute` has `adminId` assignment but no `assignedAt`
### 32. Missing Constraints
**Files:** `02 - Data Models/*`
- `Dispute.purchaseRequestId` has no unique constraint (allows duplicate active disputes per request)
- `PurchaseRequest.deliveryInfo.deliveryCode` is not unique or sparse-unique across requests
- `Category.parentId` has no circular-reference guard
- `User.referredBy` can self-reference with no prevention
- `LevelConfig.discountPercent` has no upper bound (`max: 100`)
- `RequestTemplate.expiresAt` can be created in the past
---
## Cross-Cutting Themes
1. **Documentation Drift:** The four primary document layers (Models, API, Flows, Architecture) describe different versions of the same system. The most egregious example is the `Dispute` entity, which has three incompatible definitions.
2. **Security Theater:** Several sections describe security measures (CSP, audits, future ClamAV) while fundamental flaws (localStorage tokens, unauthenticated endpoints, stubbed crypto) remain unaddressed.
3. **Ghost States:** Multiple status enums contain values that no automated flow sets (`confirmed`, `partial`, `finalized`, `archived`, `active` for SellerOffer).
4. **Operational Fragility:** The production topology is a single Docker host with automatic `latest` deployment. There is no HA, no staging gate, and no upload storage that can survive horizontal scaling (uploads are host bind-mounts).
---
## Recommendations (Priority Order)
1. **Fix the Dispute ↔ Escrow race condition** immediately. A `disputed` state must block all releases.
2. **Align all status enums** across Models, API, and Flows. Create a single source-of-truth document.
3. **Harden Passkey authentication** before any production use.
4. **Add authentication and ownership checks** to all financial endpoints.
5. **Fix Web3 verification** to decode and validate `Transfer` event parameters.
6. **Enable rate limiting globally**, with stricter tiers for auth and financial paths.
7. **Implement idempotency keys** for dispute creation, payment verify, and payout creation.
8. **Protect `/uploads`** with signed URLs or session-based auth for sensitive attachments.
9. **Fix SHKeeper webhook error handling** — return proper status codes and log to a DLQ.
10. **Sanitize logs** to remove plaintext verification/reset codes.