feat(ui): Ghibli/Miyazaki reskin + Obsidian docs vault + project audit
UI: warm daylight design system (Tailwind v4 @theme palette, gh-* component classes, watercolor grain, Zen Maru Gothic + Klee One fonts), animated SSR-safe GhibliBackground (drifting clouds, meadow hills, soot sprites), and a full reskin of navbar, connect button, dapp page, loan cards, settings modal, and readme. Fixes the bg-white-on-dark loan-card inconsistency. Web3/business logic untouched. Docs: converted docs/ into an Obsidian vault (frontmatter, [[wikilinks]], callouts, Home MOC, folders Architecture/Operations/Audits) and added a full-project audit note (Project Audit 2026-06). Redacted a real leaked Schedy key value from the security audit example (rotate it at Schedy). Also commits the previously-untracked server layer: app/api (cron + tasks routes) and lib (redis, ssrf-guard, task-store). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Siavash Sameni
675
docs/Audits/Performance Audit.md
Normal file
675
docs/Audits/Performance Audit.md
Normal file
@@ -0,0 +1,675 @@
|
||||
---
|
||||
title: Performance Audit
|
||||
tags: [mortgagefi, audit, performance]
|
||||
type: audit
|
||||
status: reference
|
||||
updated: 2026-06-14
|
||||
---
|
||||
|
||||
# Performance Audit
|
||||
|
||||
**Audit Date:** 2026-05-01
|
||||
**Scope:** Next.js frontend, nftcache (Go), embedded scheduler API, Docker builds, blockchain interaction patterns
|
||||
**Overall Rating:** 🔴 **HIGH IMPACT** — Multiple performance bottlenecks affect user experience, RPC costs, and scalability.
|
||||
|
||||
> [!warning] Overall Rating: HIGH IMPACT
|
||||
> Multiple performance bottlenecks affect user experience, RPC costs, and scalability.
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
| Component | Score | Biggest Issue |
|
||||
|-----------|-------|---------------|
|
||||
| Frontend (DApp) | 🔴 Poor | 1,400-line monolithic component with 15 `useEffect`, 0 `useCallback`, and unbatched RPC reads |
|
||||
| nftcache | 🟡 Fair | Serial HTTP scanning (1 round-trip per token); no connection pooling |
|
||||
| Scheduler API | 🟡 Fair | N+1 Redis queries; sequential task execution |
|
||||
| Docker / Build | 🟡 Fair | 69MB `.next/` output; stale build args referencing removed service |
|
||||
| Blockchain Reads | 🔴 Poor | 14+ separate `useReadContract` hooks that should be batched |
|
||||
|
||||
> [!info] Estimated impact
|
||||
> - **Frontend:** ~3-5 second time-to-interactive on mobile; janky re-renders on every input keystroke
|
||||
> - **nftcache:** 10-60 seconds to scan a 1,000-token contract; RPC rate limit exhaustion under load
|
||||
> - **Scheduler:** O(n) Redis round-trips for n due tasks; cron can take >60s with retries
|
||||
|
||||
---
|
||||
|
||||
## 1. Frontend Performance (Critical)
|
||||
|
||||
### F1 — Monolithic 1,400-Line Component with Zero Callback Memoization 🔴
|
||||
|
||||
**Location:** `app/dapp/page.tsx` (1,405 lines)
|
||||
|
||||
> [!warning] Impact
|
||||
> Every keystroke, every state change, and every prop drift triggers a full re-render of the entire DApp UI.
|
||||
|
||||
**Metrics:**
|
||||
- `useState`: 18 instances
|
||||
- `useEffect`: 15 instances
|
||||
- `useMemo`: 13 instances
|
||||
- `useCallback`: **0 instances**
|
||||
|
||||
**Problem:** Without `useCallback`, every inline function (event handlers, `onToggle`, `setPaymentAmount`, `handlePay`, etc.) is recreated on every render. Any child component or memoized selector that receives these functions as props will see them as "changed" and re-render unnecessarily.
|
||||
|
||||
**Example:**
|
||||
```typescript
|
||||
// This function is recreated on EVERY render
|
||||
const setPaymentAmount = (tokenId: bigint, value: string) => {
|
||||
setPayInputs((prev) => ({ ...prev, [tokenId.toString()]: value }));
|
||||
};
|
||||
```
|
||||
|
||||
> [!tip] Remediation
|
||||
> 1. **Extract sub-components** — Split into `PositionCard`, `PaymentControls`, `ScanPanel`, `AlertToggle`
|
||||
> 2. **Memoize callbacks:**
|
||||
> ```typescript
|
||||
> const setPaymentAmount = useCallback((tokenId: bigint, value: string) => {
|
||||
> setPayInputs((prev) => ({ ...prev, [tokenId.toString()]: value }));
|
||||
> }, []);
|
||||
>
|
||||
> const handlePay = useCallback(async (tokenId: bigint) => {
|
||||
> // ...
|
||||
> }, [debtAddress, stableDecimals, payInputs, payMaxByTokenId, writeContractAsync, selectedChainId]);
|
||||
> ```
|
||||
> 3. **Memoize expensive derived data** with stable dependencies
|
||||
|
||||
---
|
||||
|
||||
### F2 — Unstable `useReadContracts` Dependencies Cause Excessive Refetching 🔴
|
||||
|
||||
**Location:** `app/dapp/page.tsx:519-588`
|
||||
|
||||
> [!warning] Impact
|
||||
> wagmi re-fetches debt data on every render even when token IDs haven't meaningfully changed.
|
||||
|
||||
**Problem:**
|
||||
```typescript
|
||||
const debtReads = useMemo(() => {
|
||||
if (!tokenIds.length) return [] as any[];
|
||||
return tokenIds.flatMap((id) => [
|
||||
{ abi: debtAbi as Abi, address: debtAddress as `0x${string}`, functionName: 'openDebt', args: [id], chainId: selectedChainId },
|
||||
// ... 6 more reads per token
|
||||
];
|
||||
}, [tokenIds, debtAddress, selectedChainId]);
|
||||
|
||||
const { data: debtResults, refetch } = useReadContracts({
|
||||
contracts: debtReads as any,
|
||||
query: { enabled: debtReads.length > 0 },
|
||||
});
|
||||
```
|
||||
|
||||
`debtReads` is a new array of new objects on every render. wagmi's `useReadContracts` deep-compares the `contracts` array, but if the objects aren't stable, it can trigger unnecessary cache invalidation and re-fetching.
|
||||
|
||||
> [!tip] Remediation
|
||||
> ```typescript
|
||||
> // Use stringify for stable comparison, or better: keep tokenIds stable
|
||||
> const debtReads = useMemo(() => {
|
||||
> // ... same logic
|
||||
> }, [tokenIds.join(','), debtAddress, selectedChainId]);
|
||||
> ```
|
||||
>
|
||||
> Even better: **batch all static reads into a single `useReadContracts` call** instead of mixing `useReadContract` + `useReadContracts`.
|
||||
|
||||
---
|
||||
|
||||
### F3 — 14 Separate `useReadContract` Hooks Instead of One Batch 🔴
|
||||
|
||||
**Location:** `app/dapp/page.tsx:458-702`
|
||||
|
||||
> [!warning] Impact
|
||||
> 14 individual RPC round-trips on every mount/chain switch instead of 1 batched multicall.
|
||||
|
||||
**Current hooks:**
|
||||
1. `supportsInterface`
|
||||
2. `balanceOf`
|
||||
3. `calculateAPR`
|
||||
4. `stablecoin`
|
||||
5. `contractCoin`
|
||||
6. `decimals` (stable)
|
||||
7. `symbol` (stable)
|
||||
8. `decimals` (collateral)
|
||||
9. `symbol` (collateral)
|
||||
10. `decimals` (debt token)
|
||||
11. `symbol` (debt token)
|
||||
12. `balanceOf` (debt token)
|
||||
13. `balanceOf` (collateral)
|
||||
14. `balanceOf` (stable)
|
||||
15. `allowance`
|
||||
|
||||
**Problem:** Each `useReadContract` is a separate wagmi query. Even with HTTP batching, this creates 15 separate cache entries, 15 separate error boundaries, and 15 separate re-render triggers.
|
||||
|
||||
> [!tip] Remediation
|
||||
> Collapse all static/global reads into a single `useReadContracts`:
|
||||
> ```typescript
|
||||
> const { data: globalReads } = useReadContracts({
|
||||
> contracts: [
|
||||
> { abi: erc721Abi, address: nftAddress, functionName: 'supportsInterface', args: ['0x780e9d63'] },
|
||||
> { abi: erc721Abi, address: nftAddress, functionName: 'balanceOf', args: [effectiveWallet] },
|
||||
> { abi: debtAbi, address: debtAddress, functionName: 'calculateAPR' },
|
||||
> { abi: debtAbi, address: debtAddress, functionName: 'stablecoin' },
|
||||
> { abi: debtAbi, address: debtAddress, functionName: 'contractCoin' },
|
||||
> // ... all 15 reads in one array
|
||||
> ],
|
||||
> query: { enabled: !!nftAddress && !!debtAddress },
|
||||
> });
|
||||
> ```
|
||||
>
|
||||
> This reduces re-render noise from 15 separate state changes to 1.
|
||||
|
||||
---
|
||||
|
||||
### F4 — Auto-Reschedule Effect Fires on Every `parsed` Mutation 🔴
|
||||
|
||||
**Location:** `app/dapp/page.tsx:948-974`
|
||||
|
||||
> [!warning] Impact
|
||||
> Every time debt data refreshes, the effect loops over all positions and cancels/re-creates schedules.
|
||||
|
||||
**Problem:**
|
||||
```typescript
|
||||
useEffect(() => {
|
||||
(async () => {
|
||||
for (const row of parsed) {
|
||||
// ... checks drift, cancels old jobs, schedules new ones
|
||||
}
|
||||
})();
|
||||
}, [parsed.map(r => String(r.tokenId)+':'+String(r.secondsTillLiq)).join(','), notifSettings.daysBefore, positionsStoreKey]);
|
||||
```
|
||||
|
||||
`parsed.map(...).join(',')` creates a new string on every render. Even if `secondsTillLiq` hasn't changed, string coercion of `bigint` can be expensive, and the async IIFE fires unnecessarily.
|
||||
|
||||
> [!tip] Remediation
|
||||
> 1. Use a stable hash or deep-equality check instead of string serialization
|
||||
> 2. Add a mutex/ref to prevent concurrent scheduling:
|
||||
> ```typescript
|
||||
> const isScheduling = useRef(false);
|
||||
> useEffect(() => {
|
||||
> if (isScheduling.current) return;
|
||||
> isScheduling.current = true;
|
||||
> (async () => { ... })().finally(() => { isScheduling.current = false; });
|
||||
> }, [stableDependency]);
|
||||
> ```
|
||||
|
||||
---
|
||||
|
||||
### F5 — No Code Splitting or Lazy Loading 🔴
|
||||
|
||||
**Location:** `app/dapp/page.tsx`, `app/layout.tsx`
|
||||
|
||||
> [!warning] Impact
|
||||
> The entire DApp (~700KB+ of JS) downloads before the user sees anything.
|
||||
|
||||
**Evidence:**
|
||||
- No `next/dynamic` imports anywhere
|
||||
- No `React.lazy` or `Suspense`
|
||||
- `SettingsModal` (~350 lines) is bundled into the initial chunk despite being rarely used
|
||||
- `framer-motion` is imported but may not be used on first paint
|
||||
|
||||
> [!tip] Remediation
|
||||
> ```typescript
|
||||
> import dynamic from 'next/dynamic';
|
||||
>
|
||||
> const SettingsModal = dynamic(() => import('@/components/SettingsModal'), {
|
||||
> loading: () => <div className="animate-pulse">Loading...</div>,
|
||||
> });
|
||||
> ```
|
||||
>
|
||||
> Also lazy-load heavy Web3 libraries if possible:
|
||||
> ```typescript
|
||||
> const Web3Provider = dynamic(() => import('@/providers/Web3Provider'), { ssr: false });
|
||||
> ```
|
||||
|
||||
---
|
||||
|
||||
### F6 — localStorage Access in Render Path and Effects 🟡
|
||||
|
||||
**Location:** `app/dapp/page.tsx:85-96`, `components/SettingsModal.tsx:49-73`
|
||||
|
||||
> [!warning] Impact
|
||||
> Synchronous localStorage reads block the main thread; JSON parsing on every mount.
|
||||
|
||||
**Problem:**
|
||||
```typescript
|
||||
useEffect(() => {
|
||||
if (typeof window !== 'undefined' && window.localStorage) {
|
||||
const ls = window.localStorage;
|
||||
setNftcacheEnabled(ls.getItem('nftcache:enabled') === '1');
|
||||
// ... more reads and JSON parsing
|
||||
}
|
||||
}, []);
|
||||
```
|
||||
|
||||
> [!tip] Remediation
|
||||
> Use a single initialization effect with `try/catch`, and persist via a custom hook that debounces writes:
|
||||
> ```typescript
|
||||
> function useLocalStorage<T>(key: string, initial: T) {
|
||||
> const [value, setValue] = useState<T>(() => {
|
||||
> try {
|
||||
> const raw = localStorage.getItem(key);
|
||||
> return raw ? (JSON.parse(raw) as T) : initial;
|
||||
> } catch { return initial; }
|
||||
> });
|
||||
> useEffect(() => {
|
||||
> localStorage.setItem(key, JSON.stringify(value));
|
||||
> }, [key, value]);
|
||||
> return [value, setValue] as const;
|
||||
> }
|
||||
> ```
|
||||
|
||||
---
|
||||
|
||||
### F7 — Large JavaScript Bundle 🟡
|
||||
|
||||
**Location:** `.next/static/chunks/`
|
||||
|
||||
> [!warning] Impact
|
||||
> 690KB+ of uncompressed JS across main chunks; ~293KB largest chunk.
|
||||
|
||||
**Breakdown:**
|
||||
| Chunk | Size | Likely Contents |
|
||||
|-------|------|-----------------|
|
||||
| `63d1a7d9…js` | 293 KB | Main app chunk (wagmi + viem + Web3Modal) |
|
||||
| `612aa671…js` | 202 KB | Framework runtime (Next.js + React) |
|
||||
| `a6dad97d…js` | 110 KB | Component/framework code |
|
||||
| `a5e16e63…js` | 86 KB | Vendor libraries |
|
||||
|
||||
**Total `.next/`: 69 MB** (includes standalone server, static assets, cache)
|
||||
|
||||
> [!tip] Remediation
|
||||
> 1. Enable tree-shaking for `viem` by importing only needed modules:
|
||||
> ```typescript
|
||||
> import { parseUnits } from 'viem'; // ✅ good
|
||||
> import { Abi } from 'viem'; // ✅ good
|
||||
> ```
|
||||
> 2. Replace `@web3modal/wagmi` with the newer `@reown/appkit` (smaller bundle)
|
||||
> 3. Remove unused dependencies (`@web3-react/core`, `@web3-react/injected-connector` if WalletConnect covers all wallets)
|
||||
> 4. Use `next/dynamic` for non-critical components
|
||||
|
||||
---
|
||||
|
||||
## 2. nftcache Performance (High)
|
||||
|
||||
### N1 — Serial HTTP Scanning (1 Round-Trip Per Token) 🔴
|
||||
|
||||
**Location:** `nftcache/internal/fetcher/rpc.go:55-113`
|
||||
|
||||
> [!warning] Impact
|
||||
> Scanning 1,000 tokens takes **1,000 sequential HTTP requests** = 10-60 seconds depending on latency.
|
||||
|
||||
**Problem:**
|
||||
```go
|
||||
for i := 0; i < maxTokenId; i++ {
|
||||
tokenOwner, err := c.getOwnerOf(rpcURL, contract, i, debug)
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
Each `getOwnerOf` makes a full HTTP POST, waits for response, then proceeds to the next token. No batching, no pipelining.
|
||||
|
||||
**Modern RPCs support `eth_call` batching:**
|
||||
```json
|
||||
[
|
||||
{"jsonrpc":"2.0","id":1,"method":"eth_call","params":[{"to":"0x...","data":"0x6352211e..."},"latest"]},
|
||||
{"jsonrpc":"2.0","id":2,"method":"eth_call","params":[{"to":"0x...","data":"0x6352211e..."},"latest"]},
|
||||
// ... up to 100 per batch
|
||||
]
|
||||
```
|
||||
|
||||
> [!tip] Remediation
|
||||
> Implement batch RPC calls:
|
||||
> ```go
|
||||
> const batchSize = 100
|
||||
> for batchStart := 0; batchStart < maxTokenId; batchStart += batchSize {
|
||||
> end := min(batchStart+batchSize, maxTokenId)
|
||||
> owners, err := c.getOwnerOfBatch(rpcURL, contract, batchStart, end)
|
||||
> // ...
|
||||
> }
|
||||
> ```
|
||||
>
|
||||
> This reduces 1,000 HTTP round-trips to **10**, cutting scan time from ~60s to ~2-5s.
|
||||
|
||||
---
|
||||
|
||||
### N2 — No HTTP Connection Pooling 🟡
|
||||
|
||||
**Location:** `nftcache/internal/fetcher/rpc.go:21-23`
|
||||
|
||||
> [!warning] Impact
|
||||
> TCP handshake overhead on every request.
|
||||
|
||||
**Problem:**
|
||||
```go
|
||||
http: &http.Client{}, // default — no connection reuse tuning
|
||||
```
|
||||
|
||||
> [!tip] Remediation
|
||||
> ```go
|
||||
> http: &http.Client{
|
||||
> Transport: &http.Transport{
|
||||
> MaxIdleConns: 100,
|
||||
> MaxIdleConnsPerHost: 100,
|
||||
> IdleConnTimeout: 90 * time.Second,
|
||||
> },
|
||||
> Timeout: 30 * time.Second,
|
||||
> },
|
||||
> ```
|
||||
|
||||
---
|
||||
|
||||
### N3 — Rate Limiter Bottleneck 🟡
|
||||
|
||||
**Location:** `nftcache/internal/fetcher/rpc.go:20-45`
|
||||
|
||||
> [!warning] Impact
|
||||
> Hard-coded 5 TPS limits throughput even for batched requests.
|
||||
|
||||
**Problem:**
|
||||
```go
|
||||
rateLimiter: make(chan struct{}, 5), // 5 TPS
|
||||
```
|
||||
|
||||
With batching, you could safely increase this to 20-50 TPS because each RPC call carries 100 `eth_call`s. The limit should be on RPC **calls**, not HTTP **requests**.
|
||||
|
||||
---
|
||||
|
||||
### N4 — Unbounded Background Refresh Goroutines 🟡
|
||||
|
||||
**Location:** `nftcache/cmd/nftcache/main.go:133-151`
|
||||
|
||||
> [!warning] Impact
|
||||
> Under load, stale cache hits spawn unlimited goroutines, exhausting memory and RPC quota.
|
||||
|
||||
**Problem:**
|
||||
```go
|
||||
if age >= a.ttl {
|
||||
go func(net, contract string, maxTokenId int, key string) {
|
||||
// ... full rescan
|
||||
}(net, cContract, maxTokenId, contractCacheKey)
|
||||
}
|
||||
```
|
||||
|
||||
> [!tip] Remediation
|
||||
> Use `sync.SingleFlight` to deduplicate:
|
||||
> ```go
|
||||
> import "golang.org/x/sync/singleflight"
|
||||
>
|
||||
> type app struct {
|
||||
> // ...
|
||||
> refreshGroup singleflight.Group
|
||||
> }
|
||||
>
|
||||
> // In getNFTs:
|
||||
> if age >= a.ttl {
|
||||
> go a.refreshGroup.Do(contractCacheKey, func() (interface{}, error) {
|
||||
> // ... refresh logic
|
||||
> return nil, nil
|
||||
> })
|
||||
> }
|
||||
> ```
|
||||
|
||||
---
|
||||
|
||||
## 3. Scheduler API Performance (Medium)
|
||||
|
||||
### S1 — N+1 Redis Queries in `listDueTasks` 🟡
|
||||
|
||||
**Location:** `lib/task-store.ts:45-56`
|
||||
|
||||
> [!warning] Impact
|
||||
> If 50 tasks are due, this makes **51 Redis round-trips** (1 zrange + 50 gets).
|
||||
|
||||
**Problem:**
|
||||
```typescript
|
||||
export async function listDueTasks(before: number): Promise<Task[]> {
|
||||
const ids = await r.zrange<string[]>(TASK_ZSET, 0, before, { byScore: true });
|
||||
const tasks: Task[] = [];
|
||||
for (const id of ids) {
|
||||
const t = await getTask(id); // N individual GETs
|
||||
if (t) tasks.push(t);
|
||||
}
|
||||
return tasks;
|
||||
}
|
||||
```
|
||||
|
||||
> [!tip] Remediation
|
||||
> Use `mget` for batch retrieval:
|
||||
> ```typescript
|
||||
> export async function listDueTasks(before: number): Promise<Task[]> {
|
||||
> const ids = await r.zrange<string[]>(TASK_ZSET, 0, before, { byScore: true });
|
||||
> if (!ids || ids.length === 0) return [];
|
||||
>
|
||||
> const keys = ids.map(id => `${TASK_PREFIX}${id}`);
|
||||
> const datas = await r.mget<string[]>(keys);
|
||||
>
|
||||
> return datas
|
||||
> .map((data, i) => ({ data, id: ids[i] }))
|
||||
> .filter(({ data }) => data !== null)
|
||||
> .map(({ data, id }) => {
|
||||
> try {
|
||||
> return JSON.parse(data!) as Task;
|
||||
> } catch {
|
||||
> return null;
|
||||
> }
|
||||
> })
|
||||
> .filter((t): t is Task => t !== null);
|
||||
> }
|
||||
> ```
|
||||
>
|
||||
> This reduces 51 round-trips to **2**.
|
||||
|
||||
---
|
||||
|
||||
### S2 — Sequential Task Execution in Cron 🟡
|
||||
|
||||
**Location:** `app/api/cron/route.ts:25-65`
|
||||
|
||||
> [!warning] Impact
|
||||
> If 10 tasks are due with 3 retries each at 5s intervals, cron takes `10 × 3 × 5s = 150s` — exceeding the 60s Vercel function limit.
|
||||
|
||||
**Problem:** Tasks execute in a `for...of` loop with `await sleep()`.
|
||||
|
||||
> [!tip] Remediation
|
||||
> Execute independent tasks in parallel with `Promise.all` and a concurrency limit:
|
||||
> ```typescript
|
||||
> import pLimit from 'p-limit';
|
||||
>
|
||||
> const limit = pLimit(5); // max 5 concurrent webhook calls
|
||||
>
|
||||
> await Promise.all(
|
||||
> tasks.map(task => limit(() => executeTask(task)))
|
||||
> );
|
||||
> ```
|
||||
|
||||
---
|
||||
|
||||
### S3 — No Task Cleanup on Failure 🟡
|
||||
|
||||
**Location:** `app/api/cron/route.ts:56-62`
|
||||
|
||||
> [!warning] Impact
|
||||
> Failed tasks accumulate in Redis forever, bloating storage and slowing down zrange scans.
|
||||
|
||||
**Problem:** Only successful tasks are deleted. Failed tasks remain with their original score.
|
||||
|
||||
> [!tip] Remediation
|
||||
> On final failure, delete the task or move it to a dead-letter set:
|
||||
> ```typescript
|
||||
> if (!ok) {
|
||||
> await deleteTask(task.id);
|
||||
> await redis.zadd('mortgagefi:tasks:dead', { score: nowSec, member: task.id });
|
||||
> }
|
||||
> ```
|
||||
|
||||
---
|
||||
|
||||
## 4. Docker & Build Performance (Medium)
|
||||
|
||||
### D1 — Stale Build Arguments in Dockerfile 🟡
|
||||
|
||||
**Location:** `mortgagefi-frontend/Dockerfile`
|
||||
|
||||
> [!warning] Impact
|
||||
> Build still references removed `NEXT_PUBLIC_SCHEDY_URL`.
|
||||
|
||||
```dockerfile
|
||||
ARG NEXT_PUBLIC_SCHEDY_URL
|
||||
ENV NEXT_PUBLIC_SCHEDY_URL=${NEXT_PUBLIC_SCHEDY_URL:-/schedy}
|
||||
```
|
||||
|
||||
> [!tip] Remediation
|
||||
> Remove the SCHEDY argument and add Redis/CRON secrets if needed for standalone server:
|
||||
> ```dockerfile
|
||||
> ARG CRON_SECRET
|
||||
> ARG UPSTASH_REDIS_REST_URL
|
||||
> ARG UPSTASH_REDIS_REST_TOKEN
|
||||
> ENV CRON_SECRET=${CRON_SECRET}
|
||||
> ENV UPSTASH_REDIS_REST_URL=${UPSTASH_REDIS_REST_URL}
|
||||
> ENV UPSTASH_REDIS_REST_TOKEN=${UPSTASH_REDIS_REST_TOKEN}
|
||||
> ```
|
||||
|
||||
---
|
||||
|
||||
### D2 — Large `.next/` Output (69MB) 🟡
|
||||
|
||||
**Location:** `mortgagefi-frontend/.next/`
|
||||
|
||||
> [!warning] Impact
|
||||
> Slow Docker builds, large image layers.
|
||||
|
||||
**Breakdown:**
|
||||
- Standalone server: ~40MB
|
||||
- Static chunks: ~1.2MB
|
||||
- Cache, trace files, source maps: ~25MB
|
||||
|
||||
> [!tip] Remediation
|
||||
> 1. Add `.dockerignore` to exclude `.next/cache`, `node_modules`, `.git`:
|
||||
> ```
|
||||
> .next/cache
|
||||
> node_modules
|
||||
> .git
|
||||
> .env*
|
||||
> ```
|
||||
> 2. Disable source maps in production:
|
||||
> ```typescript
|
||||
> // next.config.ts
|
||||
> productionBrowserSourceMaps: false,
|
||||
> ```
|
||||
|
||||
---
|
||||
|
||||
## 5. Blockchain Read Optimization (High)
|
||||
|
||||
### B1 — ERC721Enumerable Scanning Is Unused 🔴
|
||||
|
||||
**Location:** `app/dapp/page.tsx:492-513`
|
||||
|
||||
> [!warning] Impact
|
||||
> Code detects `ERC721Enumerable` support but never actually uses `tokenOfOwnerByIndex` to fetch token IDs.
|
||||
|
||||
**Problem:**
|
||||
```typescript
|
||||
useEffect(() => {
|
||||
const fetchTokens = async () => {
|
||||
if (!effectiveWallet || !balance || !canEnumerate) return;
|
||||
const count = Number(balance);
|
||||
const max = Math.min(count, 10);
|
||||
const reads = Array.from({ length: max }, (_, i) => ({
|
||||
abi: erc721Abi,
|
||||
address: nftAddress,
|
||||
functionName: 'tokenOfOwnerByIndex',
|
||||
args: [effectiveWallet, BigInt(i)],
|
||||
}));
|
||||
// ... reads are built but NEVER EXECUTED
|
||||
};
|
||||
fetchTokens();
|
||||
}, [effectiveWallet, balance, canEnumerate, nftAddress, selectedChainId]);
|
||||
```
|
||||
|
||||
The `reads` array is built but never passed to `useReadContracts` or `publicClient.multicall`. This is dead code that runs useless logic on every balance change.
|
||||
|
||||
> [!tip] Remediation
|
||||
> Either implement the multicall or remove this effect entirely.
|
||||
|
||||
---
|
||||
|
||||
### B2 — Manual `ownerOf` Scanning Is Inefficient 🔴
|
||||
|
||||
**Location:** `app/dapp/page.tsx:318-414`
|
||||
|
||||
> [!warning] Impact
|
||||
> Browser scans 12 tokens at a time via individual RPC calls. For a wallet with 50 NFTs, this is 50+ sequential RPC round-trips from the browser.
|
||||
|
||||
**Problem:** The frontend calls `publicClient.readContract` in a loop with `await sleep(50)` between each.
|
||||
|
||||
> [!tip] Remediation
|
||||
> 1. **Prefer nftcache API** — it's already implemented and batch-scans server-side
|
||||
> 2. If client-side scanning is needed, use `publicClient.multicall`:
|
||||
> ```typescript
|
||||
> const results = await publicClient.multicall({
|
||||
> contracts: indices.map(i => ({
|
||||
> abi: erc721Abi,
|
||||
> address: nftAddress,
|
||||
> functionName: 'ownerOf',
|
||||
> args: [BigInt(i)],
|
||||
> })),
|
||||
> });
|
||||
> ```
|
||||
>
|
||||
> This sends all 12 `ownerOf` calls in a single HTTP request.
|
||||
|
||||
---
|
||||
|
||||
## Remediation Priority Matrix
|
||||
|
||||
| Priority | Issue | Effort | Impact |
|
||||
|----------|-------|--------|--------|
|
||||
| **P0** | F3 — Batch 14 `useReadContract` into 1 | 4h | Massive reduction in RPC calls and re-renders |
|
||||
| **P0** | N1 — Batch RPC scanning in nftcache | 4h | 10-50x faster contract scanning |
|
||||
| **P1** | F1 — Split dapp page into components | 1d | Eliminates full-page re-renders |
|
||||
| **P1** | F5 — Add `next/dynamic` for SettingsModal | 30m | Faster initial page load |
|
||||
| **P1** | S1 — Use `mget` in `listDueTasks` | 1h | 25x fewer Redis round-trips |
|
||||
| **P1** | S2 — Parallelize cron task execution | 1h | Prevents Vercel timeout |
|
||||
| **P2** | F2 — Stabilize `useReadContracts` deps | 2h | Reduces unnecessary refetching |
|
||||
| **P2** | F4 — Fix auto-reschedule effect | 1h | Prevents job thrashing |
|
||||
| **P2** | N4 — Deduplicate background refreshes | 2h | Prevents memory/RPC exhaustion |
|
||||
| **P2** | B2 — Use `multicall` for client scan | 2h | 12x faster NFT discovery |
|
||||
| **P3** | F7 — Optimize bundle size | 4h | Faster time-to-interactive |
|
||||
| **P3** | D1/D2 — Fix Dockerfile + .dockerignore | 1h | Faster builds, smaller images |
|
||||
|
||||
---
|
||||
|
||||
## Quick Wins (Apply Today)
|
||||
|
||||
### 1. Enable RPC Batching in wagmi
|
||||
```typescript
|
||||
// config/web3.ts
|
||||
http(baseRpc, { batch: true, retryCount: 2 })
|
||||
```
|
||||
Already set — verify your RPC provider supports `eth_call` batching.
|
||||
|
||||
### 2. Remove Dead Code
|
||||
```typescript
|
||||
// app/dapp/page.tsx ~ lines 492-513
|
||||
// Delete the unused ERC721Enumerable fetchTokens effect
|
||||
```
|
||||
|
||||
### 3. Add `.dockerignore`
|
||||
```
|
||||
.next/cache
|
||||
node_modules
|
||||
.git
|
||||
.env*
|
||||
*.md
|
||||
```
|
||||
|
||||
### 4. Use nftcache by Default
|
||||
Instead of client-side `ownerOf` scanning, default to the nftcache API which does the heavy lifting server-side.
|
||||
|
||||
---
|
||||
|
||||
## Related
|
||||
|
||||
- [[Home]]
|
||||
- [[Architecture]]
|
||||
- [[Security Audit]]
|
||||
- [[Project Audit 2026-06]]
|
||||
217
docs/Audits/Project Audit 2026-06.md
Normal file
217
docs/Audits/Project Audit 2026-06.md
Normal file
@@ -0,0 +1,217 @@
|
||||
---
|
||||
title: Project Audit 2026-06
|
||||
tags: [mortgagefi, audit, security, performance, dependencies]
|
||||
type: audit
|
||||
status: current
|
||||
updated: 2026-06-14
|
||||
---
|
||||
|
||||
# Project Audit — June 2026
|
||||
|
||||
A fresh, whole-project review of MortgageFi after a period of inactivity, covering the frontend web3 logic, the server/API layer, the Go `nftcache` service, infrastructure/ops, and dependencies. It complements the deeper, topic-specific [[Security Audit]] and [[Performance Audit]] — where those go further on a topic, they are linked inline.
|
||||
|
||||
> [!danger] Three issues need attention before anything else
|
||||
> 1. **A live secret is shipped to every browser** — `NEXT_PUBLIC_SCHEDY_API_KEY` is set in `.env`, so it is inlined into the client bundle. **Rotate the Schedy key now** and remove the `NEXT_PUBLIC_` variant.
|
||||
> 2. **The scheduler's SSRF guard is bypassable** — it validates the hostname string at *creation* time but the cron worker fires the request later with no re-validation, no redirect control, and no DNS-rebinding protection.
|
||||
> 3. **`GET /api/tasks` and `DELETE /api/tasks/[id]` have no authentication** — anyone can enumerate every scheduled task (and the secrets stored in their headers) or delete them.
|
||||
|
||||
## Executive summary
|
||||
|
||||
The codebase is, structurally, in good shape: a modern and internally-consistent core stack (Next.js 16, React 19, wagmi v3 + viem v2, TypeScript strict), a clean scheduling migration to Next.js API routes + Upstash Redis, non-root Docker images that aren't port-published, and genuine security thought (RPC allowlist, known-preset compound guard, SSRF allowlist scaffolding, a real CSP).
|
||||
|
||||
The problems are concentrated in **secret handling**, the **outbound-request (SSRF) trust model**, **endpoint authentication**, and the **NFT-scanning design** (both the frontend `ownerOf` walk and the backend serial scan are correctness-fragile and DoS-prone). There is also notable **dead weight**: two unused web3 stacks (`@web3-react`, `@web3modal/wagmi`) and the orphaned `schedy` submodule that Docker still builds and runs.
|
||||
|
||||
### Findings by severity
|
||||
|
||||
| Domain | 🔴 Critical | 🟠 High | 🟡 Medium | 🟢 Low | ⚪ Info |
|
||||
|---|:--:|:--:|:--:|:--:|:--:|
|
||||
| [Frontend web3](#1-frontend-web3--ui-logic) | 0 | 3 | 7 | 4 | 1 |
|
||||
| [API & server libs](#2-api--server-libs) | 2 | 3 | 3 | 2 | 1 |
|
||||
| [nftcache (Go)](#3-nftcache-go-service) | 2 | 4 | 4 | 3 | 2 |
|
||||
| [Infra & ops](#4-infrastructure--ops) | 2 | 3 | 3 | 2 | 2 |
|
||||
| [Dependencies & build](#5-dependencies--build) | 0 | 2 | 2 | 2 | 1 |
|
||||
| **Total** | **6** | **15** | **19** | **13** | **7** |
|
||||
|
||||
> [!note] Severity is engineering judgement, not a formal CVSS score. The two "Critical" deps/web3modal-style items were normalised down because the affected code is unused; see each finding.
|
||||
|
||||
## Priority remediation roadmap
|
||||
|
||||
> [!todo] Do first (Critical)
|
||||
> - [ ] **Rotate the Schedy API key** and delete `NEXT_PUBLIC_SCHEDY_API_KEY`; proxy Schedy through a server-side route that reads the non-public `SCHEDY_API_KEY`. Remove the stray `manwe-secret` line.
|
||||
> - [ ] **Rewrite `lib/ssrf-guard.ts` to validate at fire-time**: resolve all A/AAAA records, reject any private/reserved/loopback/link-local IP, pin the connection to the validated IP (defeat rebinding), set `redirect: 'manual'` and re-validate each hop, and parse decimal/octal/hex/IPv4-mapped-IPv6 forms with a real IP library.
|
||||
> - [ ] **Authenticate `GET /api/tasks` and `DELETE /api/tasks/[id]`**, scope tasks to an owner, and never return raw `headers`/`payload`.
|
||||
> - [ ] **nftcache: move the API-key check to the top of every handler** (before any param parsing, cache access, or RPC), and **restrict `nft_contract`/`network` to the YAML allowlist** so an arbitrary address can't trigger a 10k-call scan.
|
||||
> - [ ] **Audit published images for baked-in secrets** (`COPY . .` + missing frontend `.dockerignore`); add a `.dockerignore` excluding `.env*`, then rotate anything that may have shipped.
|
||||
|
||||
> [!todo] Do next (High)
|
||||
> - [ ] Make `CRON_SECRET` (frontend) and `X-API-Key` (nftcache) comparisons constant-time (`crypto.timingSafeEqual` / `crypto/subtle`).
|
||||
> - [ ] Fail the stack closed when `REDIS_REST_TOKEN` is unset; stop shipping `local-redis-token-change-me` as a working default.
|
||||
> - [ ] Make the rate limiter atomic + fail-closed, and stop trusting client `X-Forwarded-For` for the key.
|
||||
> - [ ] Replace the frontend's sequential `ownerOf` gap-scan with `Transfer`-log / enumerable / multicall detection; only count a "gap" on a true `nonexistent token` revert, and never permanently mark a wallet "complete" after transient errors.
|
||||
> - [ ] Give nftcache HTTP-server timeouts + graceful shutdown (`cache.Close()`), thread `context` through RPC calls, and add single-flight to the stale-refresh path.
|
||||
> - [ ] Add nginx `limit_req` / `client_max_body_size`; confirm ntfy isn't open (`NTFY_AUTH_DEFAULT_ACCESS=deny-all`).
|
||||
> - [ ] Gate write actions (`pay`/`approve`/`compound`) on `chainId === selectedChainId`; scope the compound preset check to the selected chain.
|
||||
> - [ ] Remove `@web3-react/*` and `@web3modal/wagmi`; remove the orphaned `schedy` service from `docker-compose.yml`/`Dockerfile`.
|
||||
|
||||
---
|
||||
|
||||
## 1. Frontend web3 & UI logic
|
||||
|
||||
> Files: `app/dapp/page.tsx`, `config/web3.ts`, `providers/Web3Provider.tsx`, `utils/scheduler.ts`, `utils/useLocalStorage.ts`, `components/SettingsModal.tsx`
|
||||
|
||||
The web3 read plumbing is mostly sound (on-chain `decimals`/`symbol`, `parseUnits`/BigInt money math, payment capped to `currentPaymentPending`). The weak spots are the NFT-discovery heuristics and several correctness traps around chain switching.
|
||||
|
||||
### 🟠 High
|
||||
- **Gap-limit scan conflates RPC errors with non-existent tokens** — `app/dapp/page.tsx:339-392`. Any non-429 error is treated as a "definitive gap"; 5 in a row write `complete: true`, after which `scanMore` early-returns forever and the wallet looks empty — a safety-relevant false negative that can hide a position nearing liquidation. *Fix:* only count a gap on an actual `nonexistent token` revert; persist a "stopped due to errors" state.
|
||||
- **Sequential scan assumes contiguous tokenIds** — `app/dapp/page.tsx:325-371`. Walks `0,1,2,…` and stops after 5 misses; burns/non-sequential IDs are undetectable except by manual entry. *Fix:* prefer `Transfer`-log scan or ERC721Enumerable (`tokenOfOwnerByIndex`) — see the dead-code finding below.
|
||||
- **Chain-switch race** — `app/dapp/page.tsx:429-437`. `selectedChainId` drives every read while the wallet may be on another chain; the auto-`switchChain` result is ignored and reads/writes proceed regardless. *Fix:* gate writes on `chainId === selectedChainId`, await `switchChainAsync`, prompt the user instead of silent auto-switch.
|
||||
|
||||
### 🟡 Medium
|
||||
- **APR display is a magnitude heuristic** — `:991-1003`. Infers scale at runtime (`>1e9 ⇒ 1e18 scale, else basis points`); any other denominator is mis-rendered, potentially off by orders of magnitude for a financial figure. *Fix:* read/hardcode the real per-preset denominator; unit-test against on-chain value.
|
||||
- **Loan-term/expiration heuristic** — `:1226-1237`. `expiration <= 200 ? "N Years" : date`. Conflates two meanings of one field by magnitude. *Fix:* format deterministically from the ABI's actual semantics.
|
||||
- **Provider tokens & API keys in `localStorage`, logged to console** — `:116-118`, `:1404`; `SettingsModal.tsx:330-334`; `useLocalStorage.ts`. Gotify token, ntfy topic, emails, NFTCache API key persisted and the whole settings object repeatedly `console.log`'d. *Fix:* strip the logging; treat the tokens as secrets (don't persist, or schedule server-side).
|
||||
- **`nftcacheApiKey` saved but no UI to view/clear it** — `SettingsModal.tsx:36,51-56`. Half-wired secret that can be persisted but never edited or removed via the UI. *Fix:* add the field or remove the state.
|
||||
- **Custom-RPC trust model is partial & silent** — `config/web3.ts:11-49`. The allowlist only governs read transports (not the wallet's signing RPC), the Alchemy regex accepts any key-shaped suffix, and a rejected RPC silently falls back to a public default with only a `console.warn`. *Fix:* surface rejection in the UI; document the scope.
|
||||
- **Monolithic component re-renders + un-debounced auto-reschedule** — whole file; effects at `:250-304`, `:929-955`. ~25 hooks in one component; every keystroke re-renders the tree, and the auto-reschedule effect fires `scheduleJob`/cancel on >60s drift with `exhaustive-deps` disabled — risking duplicate/cancelled jobs from stale closures. *Fix:* extract child components/hooks, debounce, add an in-flight guard. See [[Performance Audit]] for the broader re-render analysis.
|
||||
- **Compound guard checks address but not chain** — `:1017-1039`. Validates `debtAddress` across *all* chains' presets, then writes with `selectedChainId`. *Fix:* scope to `PRESETS[selectedChainId]` and require chain match.
|
||||
|
||||
### 🟢 Low
|
||||
- **Dead enumerable code path** — `:481-502`. `tokenOfOwnerByIndex` reads are built then discarded; the more-correct enumeration is never used. *Fix:* implement via `multicall` or delete.
|
||||
- **`parseUnits` failures swallowed in pay/approve handlers** — `:697-736`. Bad input (`"1.2.3"`, `"1e5"`) throws and is only `console.warn`'d; the money action silently no-ops. *Fix:* surface parse errors; reuse the render's `amount === null` guard.
|
||||
- **Scheduler fabricates a job id on ambiguous responses** — `utils/scheduler.ts:45-65`. A non-JSON/`id`-less response yields `task_<random>`, so a failed schedule looks successful (false confidence an alert exists). *Fix:* treat missing `id` as failure.
|
||||
- **`useLocalStorage` trusts persisted JSON with no schema validation** — `utils/useLocalStorage.ts:18-39`. Tampered/corrupt storage is accepted as typed settings that drive network requests; double-effect can cause a redundant write. *Fix:* validate shape (zod) before accepting.
|
||||
|
||||
### ⚪ Info
|
||||
- **Deep-link payload from `localStorage` drives chain/preset** — `:170-195`. Same-origin, age-bounded, and IDs are later guarded — acceptable; consider range-validating `preset`/`tokenId`.
|
||||
|
||||
---
|
||||
|
||||
## 2. API & server libs
|
||||
|
||||
> Files: `app/api/cron/route.ts`, `app/api/tasks/route.ts`, `app/api/tasks/[id]/route.ts`, `lib/redis.ts`, `lib/ssrf-guard.ts`, `lib/task-store.ts`, `utils/cronhost.ts`
|
||||
|
||||
The skeleton is sound (CRON_SECRET gate, an SSRF allowlist, a rate limiter, header sanitisation) but the SSRF guard doesn't actually protect the outbound request, and two endpoints are unauthenticated.
|
||||
|
||||
### 🔴 Critical
|
||||
- **SSRF guard bypassable via DNS rebinding, redirects, and IP encodings** — `lib/ssrf-guard.ts:30-84`; enforcement gap at `app/api/cron/route.ts:36`. Validation runs on the literal hostname at creation time only. Bypasses: (1) DNS rebinding — `attacker.com` passes, then resolves to `169.254.169.254`/`127.0.0.1` at fire time; (2) no `redirect: 'manual'`, so a whitelisted host can 302 to metadata; (3) decimal/octal/hex IPv4 (`http://2130706433/`, `0177.0.0.1`, `127.1`) skip the dotted-quad regex; (4) IPv4-mapped IPv6 (`[::ffff:169.254.169.254]`) and long-form loopback bypass the prefix checks; (5) GCP `metadata.google.internal` and the decimal form of the IMDS address aren't blocked. *Fix:* see the roadmap — resolve-and-pin at fire time. The `BLOCKED_HOSTS` Docker-service-name list confirms an internal network is reachable. (Also tracked in [[Security Audit]].)
|
||||
- **No auth on `GET /api/tasks` & `DELETE /api/tasks/[id]`** — `app/api/tasks/route.ts:65-72`; `app/api/tasks/[id]/route.ts:4-19`. GET returns *every* task including each `url`, full `headers` (which legitimately carry `Authorization`/API keys for the target) and `payload`; DELETE removes any task whose id `startsWith('task_')`. Anonymous credential disclosure + sabotage/DoS, with no per-owner scoping anywhere. *Fix:* authenticate + scope + redact.
|
||||
|
||||
### 🟠 High
|
||||
- **Stored task headers carry secrets and are returned/forwarded unredacted** — `app/api/tasks/route.ts:50,59`; `lib/task-store.ts:21`. `sanitizeHeaders` strips only hop-by-hop headers; `Authorization`/`Cookie`/`X-Api-Key` survive, are stored in plaintext, echoed in the POST 201, exposed by the unauthenticated GET, and replayed on every retry. *Fix:* encrypt at rest, never echo, redact on cross-origin redirects.
|
||||
- **Rate limiter fails open + check-then-act race + spoofable key** — `lib/task-store.ts:76-91`; `app/api/tasks/route.ts:6-12`. Returns *allow* when Redis is unconfigured; non-atomic count under concurrency; keyed off client-controlled `x-forwarded-for`; collapses missing-IP callers to one `unknown` bucket. *Fix:* atomic sliding window (Lua/`INCR`+`EXPIRE`), fail closed, trusted-proxy IP only.
|
||||
- **`CRON_SECRET` comparison not timing-safe** — `app/api/cron/route.ts:9-13`. Plain `!==` on the one credential protecting the highest-privilege endpoint. *Fix:* `crypto.timingSafeEqual` over equal-length buffers.
|
||||
|
||||
### 🟡 Medium
|
||||
- **`listAllTasks` uses `KEYS` + N+1 `GET`** — `lib/task-store.ts:58-73` (and `listDueTasks` N+1 at `:51-54`). `KEYS` is O(N)/blocking and Upstash-discouraged; an attacker can trigger it repeatedly via the open GET. *Fix:* drive listing off the existing `TASK_ZSET` with `ZRANGE` + `MGET`.
|
||||
- **Cron forces POST + overrides stored content-type inconsistently** — `app/api/cron/route.ts:37,40`. Tasks store no method; the worker always POSTs and hardcodes content-type while letting other arbitrary headers through. *Fix:* make method/content-type explicit and validated; consistent precedence.
|
||||
- **Weak input validation (URL length, payload size, header values)** — `app/api/tasks/route.ts:21-26,50`. No max URL length, no payload-size cap (stored & sent verbatim), header values not type/CRLF-checked. *Fix:* enforce limits; reject control/CRLF chars.
|
||||
|
||||
### 🟢 Low
|
||||
- **Internal error detail leaked to clients** — `cron/route.ts:20`, `tasks/route.ts:34,61,70`. `Blocked URL: ${reason}` even discloses which SSRF rule fired. *Fix:* generic client errors, detailed server logs.
|
||||
- **Contradictory localhost policy in the guard** — `lib/ssrf-guard.ts:39-48`. Dev carve-out for `http://localhost` is immediately undone by `BLOCKED_HOSTS`; signals the guard wasn't tested end-to-end. *Fix:* resolve the contradiction; key protocol policy off an explicit flag, not `NODE_ENV`.
|
||||
|
||||
### ⚪ Info
|
||||
- **`utils/cronhost.ts` is dead/legacy stub code** — confirmed no imports anywhere. *Fix:* delete it.
|
||||
|
||||
---
|
||||
|
||||
## 3. nftcache (Go service)
|
||||
|
||||
> Files: `cmd/nftcache/main.go`, `internal/config/config.go`, `internal/fetcher/rpc.go`, `internal/fetcher/alchemy.go`, `internal/store/store.go`, `go.mod`
|
||||
|
||||
A BadgerDB-backed stale-while-revalidate cache fronting per-token `ownerOf` scans. Auth and address validation exist but the authorization ordering is fragile and the scan design is a DoS amplifier.
|
||||
|
||||
### 🔴 Critical
|
||||
- **API-key check ordering is fragile** — `cmd/nftcache/main.go:82-124,239-245`. The key is checked after param/config resolution; gating currently works but any future edit touching RPC before line 122 would expose an unauthenticated amplifier. *Fix:* check the key first thing in every handler (middleware).
|
||||
- **Serial RPC scan is a DoS amplifier** — `internal/fetcher/rpc.go:55-113`, called from `main.go:172,245`. One request → up to `maxTokenId` (cap 10000) serial `eth_call`s against the upstream, throttled by a *global* 5 TPS limiter, for any caller-supplied `network`+`nft_contract`. 1 HTTP request → up to 10k upstream calls, and one abusive scan starves all legitimate traffic. *Fix:* reject contracts not in the YAML allowlist; bound per-request concurrency; per-call deadlines.
|
||||
|
||||
### 🟠 High
|
||||
- **Non-constant-time API-key compare** — `main.go:123,239,262`. *Fix:* `crypto/subtle.ConstantTimeCompare`.
|
||||
- **No HTTP-server timeouts & no graceful shutdown** — `main.go:303`, `main()`. Default server (no Read/Write/Idle timeouts → slowloris), no SIGTERM handler, `cache.Close()` never called (Badger not flushed). *Fix:* explicit `http.Server` timeouts + `server.Shutdown` + `defer cache.Close()`.
|
||||
- **No context/timeout on RPC calls; scans uncancellable** — `rpc.go:23,227`; `FetchAllTokenOwners` takes no `context`. Hung upstream blocks a scan goroutine indefinitely; client disconnect doesn't abort a 10k-call scan; background refreshes are fire-and-forget. *Fix:* `http.Client{Timeout}`, thread `context`, derive refresh ctx from server lifetime.
|
||||
- **Stale-refresh thundering herd (no single-flight)** — `main.go:143-162`. N concurrent stale requests launch N full scans of the same contract. *Fix:* `singleflight` per contract key.
|
||||
|
||||
### 🟡 Medium
|
||||
- **Serial scanning instead of batch/multicall** — `rpc.go:65-109`. 1000 tokens ≥200s, 10000 ≥33min. Multicall3 (same address on eth/arb/base) or JSON-RPC batch, ERC721Enumerable, or the already-implemented-but-unused Alchemy `getNFTsForOwner` would collapse this. *Fix:* adopt multicall/Alchemy path. (See [[Performance Audit]].)
|
||||
- **Upstream/internal error detail leaked to clients** — `main.go:182,246,267`. *Fix:* generic messages, server-side logs.
|
||||
- **Rate-limit handling deletes cache → feedback loop** — `main.go:151-154,177-180`. On 429 exhaustion it deletes the entry, so the next request rescans into the already-limited upstream; detection is brittle substring matching. *Fix:* keep stale data + back off; typed/sentinel errors.
|
||||
- **No `network` allowlist; CORS supports `*`** — `main.go:83,38-52`. Bad networks fail deep in the scan; `CORS_ALLOW_ORIGIN=*` is allowed (default empty is safe). *Fix:* validate `network` up front; document `*` must not pair with credentialed flows.
|
||||
|
||||
### 🟢 Low
|
||||
- **Leftover "Token 103" debug + unconditional `fmt.Printf`** — `rpc.go:106-111`, `main.go:197-199`. *Fix:* remove special-cases; gate behind a debug flag; use `log` consistently.
|
||||
- **`canonAddr` accepts any 40-hex string; duplicated in two files** — `main.go:64-80`, `rpc.go:141-157`. *Fix:* combine with the allowlist; dedupe the helper.
|
||||
- **`json.Marshal` errors ignored in store writes** — `internal/store/store.go:50,58`. *Fix:* check & propagate.
|
||||
|
||||
### ⚪ Info
|
||||
- **No Badger-layer TTL / value-log GC** — `store/store.go:49-66`. Entries never expire on disk (app-side SWR only) → unbounded growth. *Fix:* optional Badger TTL + periodic `RunValueLogGC`.
|
||||
- **Rate limiter is approximate (burst >5 TPS) + refill goroutine leaks on exit** — `rpc.go:24-43`. Acceptable for a soft limiter; use `x/time/rate` if strictness is needed.
|
||||
|
||||
### Stale dependencies
|
||||
`go 1.22.0` (current 1.24+), `badger/v4 v4.2.0`, and several stale indirect deps with advisories — notably `golang.org/x/net v0.7.0` (HTTP/2 rapid-reset family), `google.golang.org/protobuf v1.28.1` (< 1.33.0 JSON unmarshal loop), `golang.org/x/sys v0.5.0`. *Fix:* `go get -u ./...`, bump the Go directive, run `govulncheck`.
|
||||
|
||||
---
|
||||
|
||||
## 4. Infrastructure & ops
|
||||
|
||||
> Files: `docker-compose.yml`, `nginx/nginx.conf`, `.env.example`, `.env` / `.env.local`, `config/contracts.yaml`, both `Dockerfile`s, `next.config.ts`
|
||||
|
||||
Self-hosted Compose stack. Architecture is reasonable (internal services not port-published, non-root images, a real CSP) but secret handling and the nginx edge need work.
|
||||
|
||||
### 🔴 Critical
|
||||
- **Secret shipped to the client via `NEXT_PUBLIC_`** — `.env` / `.env.local`. A real Schedy API key is set as both `SCHEDY_API_KEY` *and* `NEXT_PUBLIC_SCHEDY_API_KEY`; the latter is inlined into the JS every browser downloads. A stray `manwe-secret` duplicates the WalletConnect id. *Fix:* delete the `NEXT_PUBLIC_` variant, proxy Schedy server-side, **rotate the key**, remove the stray line.
|
||||
- **Live third-party secrets on disk** — `.env`, `mortgagefi-frontend/.env.local`. Contains real `ALCHEMY_API_KEY`, `NFTCACHE_API_KEY`, `SCHEDY_API_KEY`, `CRONHOST_API`, and a Gmail SMTP app password. Root `.gitignore` covers them and this isn't a git repo *today*, but one `git init`/sub-repo commit away from exposure. *Fix:* secret manager / Docker secrets; verify the frontend subdir's own `.gitignore` covers `.env.local`; ensure Docker build context doesn't bake them in (next finding).
|
||||
|
||||
### 🟠 High
|
||||
- **Weak hardcoded default Redis token** — `docker-compose.yml:16,76`; `.env.example:23`. `SRH_TOKEN`/`UPSTASH_REDIS_REST_TOKEN` default to `local-redis-token-change-me`, shipped as a working value in `.env.example`; the stack is likely running with it. Anyone on the Docker network can then read/write all scheduling state. *Fix:* `${REDIS_REST_TOKEN:?set me}` (fail closed); set a strong random token.
|
||||
- **`COPY . .` may bake `.env` into the frontend image** — `mortgagefi-frontend/Dockerfile:17`. No confirmed `.dockerignore`; the `.env.local` target lives inside the frontend dir, and `NEXT_PUBLIC_*` values inline into the bundle in the published image `…/mortgagefi-frontend:alert`. *Fix:* add `.dockerignore` (`.env*`, `node_modules`, `.next`, `data/`); inspect the published image; rotate anything baked in.
|
||||
- **No rate limiting / body-size limit / edge auth on proxied paths** — `nginx/nginx.conf`. Port 80 forwards `/ntfy/` (full UI + publish API), `/nftcache/`, and the app with no `limit_req`, `limit_conn`, or `client_max_body_size`; security rests entirely on each service's own auth (ntfy default is open). *Fix:* add `client_max_body_size` + `limit_req` zones; `auth_basic`/IP-allowlist `/ntfy/`; set `NTFY_AUTH_DEFAULT_ACCESS=deny-all`.
|
||||
|
||||
### 🟡 Medium
|
||||
- **Cron loop swallows all errors + plaintext Bearer + spurious deps** — `docker-compose.yml:84-102`. `curlimages/curl:latest` loop with `|| true` hides every failure (no alerting/dead-man's-switch); depends on `redis` it never uses. *Fix:* log non-2xx (drop `|| true`), pin the image, drop the redis dep, confirm `/api/cron` 401s on bad secret and isn't reachable via nginx `/`.
|
||||
- **Mutable / `:latest` image tags** — `docker-compose.yml:11,23,67,85,105`. `serverless-redis-http:latest`, untagged `ntfy`, mutable `mortgagefi-frontend:alert`, `curl:latest`. *Fix:* pin to digests/immutable tags.
|
||||
- **No healthchecks on any service** — `docker-compose.yml`. `depends_on` only waits for start, not readiness → startup 502s, no auto-restart on hung process. *Fix:* add `healthcheck` + `condition: service_healthy`.
|
||||
|
||||
### 🟢 Low
|
||||
- **Duplicate YAML key in nftcache config** — `config/contracts.yaml:10-18`. `cbbtc` defined twice (placeholder vs real); YAML silently keeps the last. *Fix:* remove the placeholder.
|
||||
- **Security-header drift between nginx and Next.js** — `nginx/nginx.conf:6-9,74-79` vs `next.config.ts`. nginx sets no CSP; Next.js does; overlap can diverge on error pages. *Fix:* centralise headers in one layer (prefer the Next.js CSP).
|
||||
|
||||
### ⚪ Info
|
||||
- **`proxy_buffering off` is global** — `nginx/nginx.conf:12`. Needed for ntfy SSE but applied everywhere → higher upstream hold time. *Fix:* scope it to `/ntfy/`.
|
||||
- **ntfy uses a personal Gmail app password** — `docker-compose.yml:31-34`. *Fix:* prefer a scoped transactional-mail credential; rotate if ever shared.
|
||||
|
||||
---
|
||||
|
||||
## 5. Dependencies & build
|
||||
|
||||
> Files: `mortgagefi-frontend/package.json`, `tsconfig.json`, `next.config.ts`, `eslint.config.mjs`, `nftcache/go.mod`, `submodules/schedy`
|
||||
|
||||
Core stack is modern and coherent (Next 16, React 19, wagmi v3 + viem v2, TS strict). The issues are dead weight and a few hardening gaps.
|
||||
|
||||
### 🟠 High
|
||||
- **Deprecated, unused, version-mismatched `@web3modal/wagmi@^5`** — `package.json`. v5 targets wagmi v2 but the project runs wagmi v3; it's also superseded by Reown AppKit — and it isn't imported anywhere. *Fix:* remove it; adopt `@reown/appkit` + `@reown/appkit-adapter-wagmi` only if a connect modal is later needed. *(Listed High, not Critical: unused = not exploitable, but it's deprecated + mismatched.)*
|
||||
- **Unused redundant `@web3-react/*` stack** — `package.json`. `@web3-react/core@^8`, `@web3-react/injected-connector@^6` are unmaintained and fully unused; the app is bare wagmi v3. *Fix:* remove both; use `wagmi/connectors` if needed.
|
||||
- **Orphaned `schedy` submodule still built & run by Docker** — `submodules/schedy/`, `docker-compose.yml`, `Dockerfile:22`. Scheduling moved to Next.js API routes + Upstash; all in-source `schedy` references are now legacy-prune code or an SSRF-allowlist token, yet Docker still builds the Go scheduler and sets `NEXT_PUBLIC_SCHEDY_URL`. *Fix:* confirm nothing deploys it, then delete the submodule, the `schedy` service/`depends_on`, and `NEXT_PUBLIC_SCHEDY_URL`; clean residual comments/keys.
|
||||
|
||||
### 🟡 Medium
|
||||
- **nftcache Go module on an old toolchain with stale transitive deps** — `nftcache/go.mod`. (Same set as §3.) *Fix:* bump Go + `go get -u ./...` + `govulncheck`.
|
||||
- **CSP allows `'unsafe-eval'` and `'unsafe-inline'` for scripts** — `next.config.ts`. Meaningfully weakens XSS protection for a wallet-signing dApp. *Fix:* move toward nonce/hash-based `script-src`, drop `'unsafe-eval'` if wagmi/walletconnect tolerate it; drop the deprecated `X-XSS-Protection`; consider adding HSTS.
|
||||
|
||||
### 🟢 Low
|
||||
- **Packages a minor behind (some security-relevant)** — `npm outdated`: `next` 16.0.10 → 16.2.9, `wagmi` 3.1.0 → 3.6.16, `viem` 2.42.1 → 2.52.2, `@tanstack/react-query`, `tailwindcss`, `framer-motion`. Treat Next.js patch bumps as security-relevant. *Fix:* run in-range minors; evaluate eslint 10 / TS 6 majors separately.
|
||||
- **Minimal ESLint config** — `eslint.config.mjs`. Only Next defaults; no `no-floating-promises`/`no-misused-promises` despite fire-and-forget `await fetch` patterns. *Fix:* add `@typescript-eslint` recommended-type-checked rules.
|
||||
|
||||
### ⚪ Info
|
||||
- **TS `target: ES2017` is dated** — `tsconfig.json` (strict is otherwise good). *Fix:* optionally raise to ES2022, consider `noUncheckedIndexedAccess`.
|
||||
|
||||
---
|
||||
|
||||
## What's done well
|
||||
|
||||
- On-chain `decimals`/`symbol` and BigInt/`parseUnits` math throughout — no float drift in money paths; payments capped to the reset-timer amount.
|
||||
- Genuine security scaffolding: RPC allowlist, known-preset compound guard, SSRF allowlist + header sanitisation, `CRON_SECRET`-gated cron that fails closed, future-dating bounds on `execute_at`, unguessable `crypto.randomUUID()` task ids.
|
||||
- nftcache: API key required at startup (fails fast if unset), CORS closed by default, correct Badger transaction usage, stale-while-revalidate for latency, 429 backoff.
|
||||
- Infra: internal services not port-published, non-root + distroless images, `CGO_ENABLED=0`, read-only `config` mount, consistent `platform` pinning, a real CSP + security-header baseline, correct `.gitignore`, `output: 'standalone'`.
|
||||
- Clean, self-contained scheduling migration to Next.js API routes + Upstash Redis.
|
||||
|
||||
## Related
|
||||
[[Home]] · [[Architecture]] · [[Security Audit]] · [[Performance Audit]] · [[Deployment]] · [[Development]]
|
||||
740
docs/Audits/Security Audit.md
Normal file
740
docs/Audits/Security Audit.md
Normal file
@@ -0,0 +1,740 @@
|
||||
---
|
||||
title: Security Audit
|
||||
tags: [mortgagefi, audit, security]
|
||||
type: audit
|
||||
status: reference
|
||||
updated: 2026-06-14
|
||||
---
|
||||
|
||||
# Security Audit — MortgageFi
|
||||
|
||||
**Audit Date:** 2026-05-01
|
||||
**Remediation Date:** 2026-05-01
|
||||
**Scope:** Full stack — Next.js frontend, nftcache (Go), embedded scheduler API, ntfy, nginx, Docker Compose, configuration
|
||||
**Risk Rating:** 🟡 **MEDIUM** — All critical and high-severity issues have been addressed. Remaining risk is low-to-medium and manageable for production.
|
||||
|
||||
> [!warning] Overall Risk Rating: MEDIUM
|
||||
> All critical and high-severity issues have been addressed. Remaining risk is low-to-medium and manageable for production.
|
||||
|
||||
---
|
||||
|
||||
## Remediation Summary
|
||||
|
||||
All **7 critical** and **11 high** severity issues identified in the initial audit have been resolved. Key changes:
|
||||
|
||||
1. ✅ **Schedy removed** — Replaced with Vercel-native Next.js API routes (`/api/tasks`, `/api/cron`) + Redis
|
||||
2. ✅ **SSRF eliminated** — New scheduler validates URLs against private IP / internal hostname blocklist
|
||||
3. ✅ **Secrets removed from client bundle** — No more `NEXT_PUBLIC_` API keys; backend uses server-side env only
|
||||
4. ✅ **Sensitive credentials removed from localStorage** — AWS SNS provider removed; Schedy fields removed; security warnings added
|
||||
5. ✅ **nftcache hardened** — API key now required at startup; address validation rejects malformed/truncated addresses; maxTokenId capped at 10,000
|
||||
6. ✅ **RPC injection prevented** — Frontend validates RPC URLs against an allowlist
|
||||
7. ✅ **Network exposure reduced** — Direct container ports removed; only nginx exposed
|
||||
8. ✅ **Security headers added** — CSP, HSTS, X-Frame-Options, X-Content-Type-Options in nginx and Next.js
|
||||
9. ✅ `.env.example` created and `.gitignore` updated
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary (Original)
|
||||
|
||||
The original codebase contained **7 critical**, **11 high**, and **15 medium/low severity** issues. The most severe were:
|
||||
|
||||
1. **Server-Side Request Forgery (SSRF)** in Schedy allowing internal network probing
|
||||
2. **Secrets committed to version control** including SMTP passwords, API keys, and WalletConnect credentials
|
||||
3. **API keys bundled into client-side JavaScript** via `NEXT_PUBLIC_` variables
|
||||
4. **Sensitive credentials stored in browser localStorage** without encryption (AWS keys, schedy API key, notification tokens)
|
||||
5. **Denial-of-Service via unauthenticated RPC scanning** in nftcache
|
||||
6. **Address canonicalization bug** enabling ownership spoofing
|
||||
|
||||
---
|
||||
|
||||
## Critical Severity
|
||||
|
||||
### C1 — SSRF in Schedy Task Executor 🔴 [FIXED]
|
||||
**Location:** `mortgagefi-frontend/submodules/schedy/internal/executor/executor.go` (removed)
|
||||
**CVSS:** ~9.1 (Critical)
|
||||
|
||||
> [!danger] Critical (CVSS ~9.1) — SSRF in Schedy Task Executor [FIXED]
|
||||
> Schedy executed HTTP webhooks to **arbitrary URLs** with zero URL validation.
|
||||
|
||||
**Issue:** Schedy executed HTTP webhooks to **arbitrary URLs** with zero URL validation.
|
||||
|
||||
**Remediation:** Schedy removed entirely. New scheduler in `app/api/cron/route.ts` uses `lib/ssrf-guard.ts` which blocks private IPs, loopback, link-local, multicast, and internal hostnames. Only HTTPS URLs are allowed in production.
|
||||
|
||||
```go
|
||||
// executor.go:37 — task.URL is used directly
|
||||
req, err := http.NewRequest(http.MethodPost, task.URL, bytes.NewBuffer(bodyBytes))
|
||||
```
|
||||
|
||||
**Impact:** An attacker with a valid Schedy API key (or if auth is disabled) can schedule tasks targeting:
|
||||
- `http://169.254.169.254/latest/meta-data/` — AWS/GCP/Cloud metadata endpoints
|
||||
- `http://localhost:8090/nfts/invalidate` — nftcache internal APIs
|
||||
- `http://localhost:80/ntfy` — ntfy admin endpoints
|
||||
- Internal Docker network services (`http://frontend:3000`, `http://nftcache:8090`)
|
||||
|
||||
**Proof of Concept:**
|
||||
```bash
|
||||
curl -X POST http://localhost:8080/tasks \
|
||||
-H "X-API-Key: $SCHEDY_API_KEY" \
|
||||
-d '{
|
||||
"url": "http://169.254.169.254/latest/meta-data/iam/security-credentials/",
|
||||
"execute_at": "2026-05-01T12:00:00Z",
|
||||
"payload": "x"
|
||||
}'
|
||||
```
|
||||
|
||||
**Remediation:**
|
||||
```go
|
||||
// Add URL validation in CreateTask handler
|
||||
func isAllowedURL(u string) bool {
|
||||
parsed, err := url.Parse(u)
|
||||
if err != nil { return false }
|
||||
if parsed.Scheme != "https" { return false } // enforce HTTPS
|
||||
host := parsed.Hostname()
|
||||
ip := net.ParseIP(host)
|
||||
if ip != nil {
|
||||
// Block private/reserved IPs
|
||||
if ip.IsPrivate() || ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsMulticast() {
|
||||
return false
|
||||
}
|
||||
}
|
||||
// Block internal hostnames
|
||||
blocked := []string{"localhost", "nftcache", "schedy", "ntfy", "frontend", "web"}
|
||||
for _, b := range blocked {
|
||||
if strings.EqualFold(host, b) { return false }
|
||||
}
|
||||
return true
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### C2 — Secrets Committed to Version Control 🔴 [FIXED]
|
||||
**Location:** `.env`, `.env.local`
|
||||
**CVSS:** ~9.0 (Critical)
|
||||
|
||||
> [!danger] Critical (CVSS ~9.0) — Secrets Committed to Version Control [FIXED]
|
||||
> The repository contained **real, active secrets** in committed files.
|
||||
|
||||
**Issue:** The repository contained **real, active secrets** in committed files.
|
||||
|
||||
| Secret | Location | Risk |
|
||||
|--------|----------|------|
|
||||
| `NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID` | `.env`, `.env.local` | Project ID exposed |
|
||||
| `SCHEDY_API_KEY` | `.env`, `.env.local` | Full scheduler control |
|
||||
| `NEXT_PUBLIC_SCHEDY_API_KEY` | `.env`, `.env.local` | Same key, also in browser bundle |
|
||||
| `NTFY_SMTP_SENDER_PASS` | `.env`, `.env.local` | SMTP password (Gmail/App Password) |
|
||||
| `CRONHOST_API` | `.env`, `.env.local` | Legacy cronhost API key |
|
||||
| `manwe-secret` | `.env`, `.env.local` | Unknown but looks sensitive |
|
||||
|
||||
**Impact:** Anyone with repository access (or if repo becomes public) has full credentials. The SMTP password grants access to the `mortgagefi@amn.gg` mailbox.
|
||||
|
||||
**Remediation:** ✅ Applied
|
||||
1. ✅ Created `.env.example` with dummy values only
|
||||
2. ✅ Added `.gitignore` at project root:
|
||||
```
|
||||
.env
|
||||
.env.local
|
||||
.env.*.local
|
||||
```
|
||||
3. **Action required by user:** Rotate all secrets and purge from Git history:
|
||||
```bash
|
||||
git rm --cached .env .env.local
|
||||
git filter-repo --path .env --path .env.local --invert-paths
|
||||
# Or use BFG Repo-Cleaner
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### C3 — API Keys Bundled in Client-Side JavaScript 🔴 [FIXED]
|
||||
**Location:** `.env.local`, `mortgagefi-frontend/utils/scheduler.ts`
|
||||
**CVSS:** ~8.5 (Critical)
|
||||
|
||||
> [!danger] Critical (CVSS ~8.5) — API Keys Bundled in Client-Side JavaScript [FIXED]
|
||||
> `NEXT_PUBLIC_SCHEDY_API_KEY` was compiled into the client bundle. Any visitor could extract it.
|
||||
|
||||
**Issue:** `NEXT_PUBLIC_SCHEDY_API_KEY` was compiled into the client bundle. Any visitor could extract it.
|
||||
|
||||
**Remediation:** ✅ `NEXT_PUBLIC_SCHEDY_API_KEY` and `NEXT_PUBLIC_SCHEDY_URL` removed entirely. The scheduler is now embedded as same-origin API routes (`/api/tasks`), so no external API key is needed in the browser. `NEXT_PUBLIC_WALLETCONNECT_PROJECT_ID` remains (unavoidable for WalletConnect), but should be monitored for abuse at the WalletConnect dashboard.
|
||||
|
||||
```javascript
|
||||
// In compiled JS:
|
||||
const ENV_SCHEDY_API_KEY = "<REDACTED — a real 64-hex key was hardcoded here; rotate it at Schedy>";
|
||||
```
|
||||
|
||||
**Impact:**
|
||||
- Attacker can call Schedy API directly to create/delete tasks
|
||||
- Attacker can abuse WalletConnect project ID for rate limit exhaustion
|
||||
- If nftcache key is set via `NEXT_PUBLIC_NFTCACHE_API_KEY`, same issue
|
||||
|
||||
**Remediation:**
|
||||
- **Never** prefix backend-only secrets with `NEXT_PUBLIC_`
|
||||
- For Schedy: proxy through a Next.js API route (`/api/schedule`) that holds the server-side secret
|
||||
- For WalletConnect: this one is unavoidable for client-side connection, but monitor abuse and restrict origins at WalletConnect dashboard
|
||||
|
||||
---
|
||||
|
||||
### C4 — Sensitive Credentials Stored in localStorage 🔴 [FIXED]
|
||||
**Location:** `mortgagefi-frontend/app/dapp/page.tsx`, `mortgagefi-frontend/components/SettingsModal.tsx`
|
||||
**CVSS:** ~8.2 (Critical)
|
||||
|
||||
> [!danger] Critical (CVSS ~8.2) — Sensitive Credentials Stored in localStorage [FIXED]
|
||||
> Sensitive data (Schedy API key, AWS credentials, notification tokens, RPC endpoints) was stored in **unencrypted browser localStorage**.
|
||||
|
||||
**Issue:** The following sensitive data was stored in **unencrypted browser localStorage**:
|
||||
|
||||
- `notif:settings` — contains Schedy API key, ntfy topic, Gotify token, AWS Access Key ID + Secret Access Key
|
||||
- `notif:positions:v1:*` — position metadata linked to wallet
|
||||
- `rpc:base`, `rpc:arbitrum`, `rpc:mainnet` — custom RPC endpoints
|
||||
- `nftcache:apiKey` — API key for nftcache
|
||||
|
||||
**Impact:** Any XSS vulnerability (or malicious browser extension) can steal:
|
||||
- AWS credentials with SNS publish permissions
|
||||
- Schedy API key granting full task control
|
||||
- Gotify/ntfy tokens enabling notification spam
|
||||
- Custom RPC URLs enabling man-in-the-middle attacks
|
||||
|
||||
**Remediation:** ✅ Applied
|
||||
- ✅ **AWS SNS provider removed entirely** from frontend (no more AWS keys in browser)
|
||||
- ✅ **Schedy URL/API key fields removed** from settings UI
|
||||
- ✅ **Security warning added** to SettingsModal: "Settings are stored locally in your browser. Do not use this on shared computers."
|
||||
- ✅ **CSP implemented** via Next.js headers (see M4)
|
||||
- Gotify token and ntfy topic remain in localStorage (lower sensitivity). Future enhancement: encrypt with user password or use sessionStorage.
|
||||
|
||||
---
|
||||
|
||||
### C5 — Address Canonicalization Bug Enables Ownership Spoofing 🔴 [FIXED]
|
||||
**Location:** `nftcache/cmd/nftcache/main.go:68-76`, `nftcache/internal/fetcher/rpc.go:134-140`
|
||||
**CVSS:** ~7.5 (High)
|
||||
|
||||
> [!danger] High (CVSS ~7.5) — Address Canonicalization Bug Enables Ownership Spoofing [FIXED]
|
||||
> `canonAddr()` silently **truncated** addresses longer than 40 hex chars instead of rejecting them, enabling ownership spoofing.
|
||||
|
||||
**Issue:** `canonAddr()` silently **truncated** addresses longer than 40 hex chars instead of rejecting them:
|
||||
|
||||
```go
|
||||
func canonAddr(s string) string {
|
||||
x := strings.ToLower(strings.TrimSpace(s))
|
||||
if strings.HasPrefix(x, "0x") { x = x[2:] }
|
||||
if len(x) > 40 { x = x[len(x)-40:] } // BUG: truncates!
|
||||
if len(x) < 40 { x = strings.Repeat("0", 40-len(x)) + x }
|
||||
return "0x" + x
|
||||
}
|
||||
```
|
||||
|
||||
**Impact:**
|
||||
```
|
||||
Input: 0x0000000000000000000000000000000000000000deadbeef1234567890abcdef12345678
|
||||
Output: 0xdeadbeef1234567890abcdef1234567890abcdef (completely different address!)
|
||||
```
|
||||
|
||||
An attacker could claim tokens belonging to address `0xdeadbeef...` by providing a maliciously padded `user_wallet` parameter. The cache key and owner comparison would match the truncated version.
|
||||
|
||||
**Remediation:** ✅ Applied. `canonAddr()` now returns `(string, error)` and rejects any address that is not exactly `0x` + 40 lowercase hex characters. All call sites updated to handle the error.
|
||||
|
||||
---
|
||||
|
||||
### C6 — Denial of Service via Unauthenticated RPC Scanning 🔴 [FIXED]
|
||||
**Location:** `nftcache/cmd/nftcache/main.go:78-196`
|
||||
**CVSS:** ~7.5 (High)
|
||||
|
||||
> [!danger] High (CVSS ~7.5) — Denial of Service via Unauthenticated RPC Scanning [FIXED]
|
||||
> The `/nfts` endpoint performed expensive on-chain scanning. When `NFTCACHE_API_KEY` was empty, the endpoint was **completely unauthenticated**.
|
||||
|
||||
**Issue:** The `/nfts` endpoint performed expensive on-chain scanning. When `NFTCACHE_API_KEY` was empty, the endpoint was **completely unauthenticated**.
|
||||
|
||||
**Impact:**
|
||||
- Attacker can spam `GET /nfts?network=base&nft_contract=cbbtc&user_wallet=0x...` to exhaust RPC rate limits
|
||||
- Each request triggers up to 1000 `eth_call` RPC requests (or more if config has higher `max_token_id`)
|
||||
- Background refresh goroutines multiply the effect (stale cache triggers background scan)
|
||||
- Can incur significant RPC provider costs and cause service degradation
|
||||
|
||||
**Proof of Concept:**
|
||||
```bash
|
||||
while true; do
|
||||
curl "http://localhost:8090/nfts?network=base&nft_contract=cbbtc&user_wallet=0x$(openssl rand -hex 20)"
|
||||
done
|
||||
```
|
||||
|
||||
**Remediation:** ✅ Applied
|
||||
1. ✅ **API key required at startup** — `nftcache` now calls `log.Fatal` if `NFTCACHE_API_KEY` is empty
|
||||
2. ✅ **maxTokenId capped at 10,000** — prevents unbounded RPC scanning
|
||||
3. ✅ Per-IP rate limiting implemented in new scheduler API (not yet in nftcache — acceptable since auth is now required)
|
||||
4. Background refresh goroutines still present; future enhancement: deduplicate with `sync.SingleFlight`
|
||||
|
||||
---
|
||||
|
||||
### C7 — RPC URL Injection via localStorage 🔴 [FIXED]
|
||||
**Location:** `mortgagefi-frontend/config/web3.ts:17-31`, `mortgagefi-frontend/components/SettingsModal.tsx:57-65`
|
||||
**CVSS:** ~7.8 (High)
|
||||
|
||||
> [!danger] High (CVSS ~7.8) — RPC URL Injection via localStorage [FIXED]
|
||||
> The frontend read RPC URLs from **localStorage** with zero validation, enabling MITM via attacker-controlled RPC endpoints.
|
||||
|
||||
**Issue:** The frontend read RPC URLs from **localStorage** with zero validation:
|
||||
|
||||
```typescript
|
||||
function runtimeRpc(key: string): string | null {
|
||||
try {
|
||||
if (typeof window !== 'undefined' && window.localStorage) {
|
||||
const v = window.localStorage.getItem(key);
|
||||
return (v && v.trim()) ? v.trim() : null;
|
||||
}
|
||||
} catch {}
|
||||
return null;
|
||||
}
|
||||
```
|
||||
|
||||
**Impact:**
|
||||
- A malicious website with XSS, a malicious browser extension, or phishing attack can set `rpc:base` to an attacker-controlled RPC
|
||||
- All subsequent blockchain reads (balances, allowances, debt data) go through the malicious RPC
|
||||
- Attacker can return fake data (e.g., show zero debt, hide liquidation warnings) or phish transactions
|
||||
- The Settings UI even provides a friendly form for users to enter arbitrary RPC URLs
|
||||
|
||||
**Remediation:** ✅ Applied. `config/web3.ts` now validates RPC URLs against an allowlist of known providers (LlamaRPC, Infura, Alchemy, MeowRPC). Untrusted RPCs from localStorage are rejected with a console warning.
|
||||
|
||||
---
|
||||
|
||||
## High Severity
|
||||
|
||||
### H1 — No HTTPS Enforcement Anywhere [FIXED]
|
||||
**Location:** `docker-compose.yml`, all Go services, nginx config
|
||||
**Impact:** All internal and external communication was plaintext HTTP.
|
||||
|
||||
> [!danger] High — No HTTPS Enforcement Anywhere [FIXED]
|
||||
> All internal and external communication was plaintext HTTP.
|
||||
|
||||
**Remediation:** ✅ nginx config and Next.js headers updated. In production, terminate TLS at nginx with a real certificate (Let's Encrypt) and add HSTS. Internal Docker network communication is isolated.
|
||||
|
||||
**Remediation:**
|
||||
- Terminate TLS at nginx with a real certificate (Let's Encrypt)
|
||||
- Use `https://` for all external service URLs
|
||||
- Add HSTS header: `Strict-Transport-Security: max-age=31536000; includeSubDomains`
|
||||
|
||||
---
|
||||
|
||||
### H2 — Missing Security Headers in Nginx [FIXED]
|
||||
**Location:** `nginx/nginx.conf`
|
||||
**Impact:** No CSP, X-Frame-Options, X-Content-Type-Options, Referrer-Policy.
|
||||
|
||||
> [!danger] High — Missing Security Headers in Nginx [FIXED]
|
||||
> No CSP, X-Frame-Options, X-Content-Type-Options, Referrer-Policy.
|
||||
|
||||
**Remediation:** ✅ Added to `nginx/nginx.conf`: `X-Frame-Options: DENY`, `X-Content-Type-Options: nosniff`, `Referrer-Policy: strict-origin-when-cross-origin`, `X-XSS-Protection: 1; mode=block`. Also added CSP and same headers via `next.config.ts` for Vercel deployments.
|
||||
|
||||
**Remediation:** Add to nginx:
|
||||
```nginx
|
||||
add_header X-Frame-Options "DENY" always;
|
||||
add_header X-Content-Type-Options "nosniff" always;
|
||||
add_header Referrer-Policy "strict-origin-when-cross-origin" always;
|
||||
add_header Content-Security-Policy "default-src 'self'; script-src 'self'; connect-src 'self' https://*.walletconnect.com https://*.llamarpc.com; img-src 'self' data:; style-src 'self' 'unsafe-inline';" always;
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### H3 — Direct Container Ports Exposed to Host [FIXED]
|
||||
**Location:** `docker-compose.yml`
|
||||
**Impact:** ntfy, schedy, and nftcache were exposed directly on the Docker host, bypassing nginx.
|
||||
|
||||
> [!danger] High — Direct Container Ports Exposed to Host [FIXED]
|
||||
> ntfy, schedy, and nftcache were exposed directly on the Docker host, bypassing nginx.
|
||||
|
||||
**Remediation:** ✅ All `ports:` blocks removed from backend services. Only nginx exposes port 80. Internal services communicate via Docker network.
|
||||
|
||||
**Remediation:** Remove `ports:` from all services except nginx:
|
||||
```yaml
|
||||
# ntfy, schedy, nftcache: remove these blocks:
|
||||
# ports:
|
||||
# - "8081:80"
|
||||
```
|
||||
Only nginx should be reachable from outside.
|
||||
|
||||
---
|
||||
|
||||
### H4 — Schedy Task Payload Can Be Used for Header Injection [FIXED]
|
||||
**Location:** `mortgagefi-frontend/submodules/schedy/internal/executor/executor.go` (removed)
|
||||
**Impact:** Custom headers from task payloads were set without validation.
|
||||
|
||||
> [!danger] High — Schedy Task Payload Can Be Used for Header Injection [FIXED]
|
||||
> Custom headers from task payloads were set without validation.
|
||||
|
||||
**Remediation:** ✅ Schedy removed. New executor in `app/api/cron/route.ts` uses `sanitizeHeaders()` which blocks `Host`, `Content-Length`, `Transfer-Encoding`, `Connection`, `Upgrade`, and `Proxy-Authorization`.
|
||||
|
||||
**Remediation:** Blocklist dangerous headers:
|
||||
```go
|
||||
blockedHeaders := map[string]bool{
|
||||
"host": true, "content-length": true, "transfer-encoding": true,
|
||||
"connection": true, "upgrade": true, "proxy-authorization": true,
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### H5 — No Request Body Size Limits [PARTIALLY FIXED]
|
||||
**Location:** All Go HTTP handlers
|
||||
**Impact:** Schedy and nftcache accepted arbitrarily large JSON bodies.
|
||||
|
||||
> [!danger] High — No Request Body Size Limits [PARTIALLY FIXED]
|
||||
> Schedy and nftcache accepted arbitrarily large JSON bodies.
|
||||
|
||||
**Remediation:** ✅ Schedy removed (no longer applicable). nftcache still lacks body size limits; future enhancement: add `http.MaxBytesReader(w, r.Body, 1<<20)` to nftcache handlers.
|
||||
|
||||
**Remediation:**
|
||||
```go
|
||||
r.Body = http.MaxBytesReader(w, r.Body, 1<<20) // 1MB limit
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### H6 — Docker Images from External Registry Without Verification [PARTIALLY FIXED]
|
||||
**Location:** `docker-compose.yml`
|
||||
**Impact:** `latest` tag was mutable; no image digest pinning.
|
||||
|
||||
> [!danger] High — Docker Images from External Registry Without Verification [PARTIALLY FIXED]
|
||||
> `latest` tag was mutable; no image digest pinning.
|
||||
|
||||
**Remediation:** ✅ Schedy image removed. `mortgagefi-frontend` image still uses `:alert` tag. **Action required:** Pin to SHA256 digest:
|
||||
```yaml
|
||||
image: git.manko.yoga/manawenuz/mortgagefi-frontend@sha256:abc123...
|
||||
```
|
||||
|
||||
**Remediation:** Pin to digest:
|
||||
```yaml
|
||||
image: git.manko.yoga/manawenuz/schedy@sha256:abc123...
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### H7 — Unbounded Concurrent Background Refreshes in nftcache [OPEN]
|
||||
**Location:** `nftcache/cmd/nftcache/main.go:133-151`
|
||||
**Impact:** Each stale cache hit spawns a goroutine for background refresh. Under load, this creates an unbounded number of goroutines.
|
||||
|
||||
> [!danger] High — Unbounded Concurrent Background Refreshes in nftcache [OPEN]
|
||||
> Each stale cache hit spawns a goroutine for background refresh. Under load, this creates an unbounded number of goroutines.
|
||||
> **Status:** Not yet fixed.
|
||||
|
||||
**Status:** Not yet fixed. Future enhancement: use `sync.SingleFlight` or a worker pool to deduplicate refreshes for the same contract key.
|
||||
|
||||
**Remediation:** Use a `sync.SingleFlight` or worker pool to deduplicate refreshes for the same contract key.
|
||||
|
||||
---
|
||||
|
||||
### H8 — AWS Credentials Stored in Browser (SNS Provider) [FIXED]
|
||||
**Location:** `mortgagefi-frontend/components/SettingsModal.tsx`
|
||||
**Impact:** The UI included a form for AWS credentials stored in localStorage.
|
||||
|
||||
> [!danger] High — AWS Credentials Stored in Browser (SNS Provider) [FIXED]
|
||||
> The UI included a form for AWS credentials stored in localStorage.
|
||||
|
||||
**Remediation:** ✅ SNS provider removed entirely from frontend and types. Users should use ntfy or Gotify for notifications instead.
|
||||
|
||||
**Remediation:** Remove SNS provider from frontend entirely, or proxy through a server-side endpoint.
|
||||
|
||||
---
|
||||
|
||||
### H9 — `secondsTillLiq` Trusts On-Chain Value Without Bounds Check [FIXED]
|
||||
**Location:** `mortgagefi-frontend/app/dapp/page.tsx`
|
||||
**Impact:** The auto-rescheduler computed `runAt1 = liqAt - offset` without validating bounds.
|
||||
|
||||
> [!danger] High — `secondsTillLiq` Trusts On-Chain Value Without Bounds Check [FIXED]
|
||||
> The auto-rescheduler computed `runAt1 = liqAt - offset` without validating bounds.
|
||||
|
||||
**Remediation:** ✅ Added `MAX_SECONDS = 86400 * 365 * 5` (5 years). Values outside `0 < seconds <= MAX_SECONDS` are rejected.
|
||||
|
||||
**Remediation:**
|
||||
```typescript
|
||||
const MAX_SECONDS = 86400 * 365 * 5; // 5 years max
|
||||
if (!Number.isFinite(seconds) || seconds <= 0 || seconds > MAX_SECONDS) {
|
||||
throw new Error('Liquidation timer out of bounds');
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### H10 — Compound Action Uses Hardcoded Function Selector Without Validation [FIXED]
|
||||
**Location:** `mortgagefi-frontend/app/dapp/page.tsx`
|
||||
**Impact:** `sendTransactionAsync` was used with a hardcoded selector, bypassing ABI validation.
|
||||
|
||||
> [!danger] High — Compound Action Uses Hardcoded Function Selector Without Validation [FIXED]
|
||||
> `sendTransactionAsync` was used with a hardcoded selector, bypassing ABI validation.
|
||||
|
||||
**Remediation:** ✅ Replaced `sendTransactionAsync` with `writeContractAsync` using the debt ABI. Added validation that `debtAddress` matches a known preset before allowing the transaction. `useSendTransaction` import removed.
|
||||
|
||||
**Remediation:** Use `writeContractAsync` with the ABI:
|
||||
```typescript
|
||||
await writeContractAsync({
|
||||
abi: debtAbi,
|
||||
address: debtAddress,
|
||||
functionName: 'compound',
|
||||
chainId: selectedChainId,
|
||||
});
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### H11 — `useSendTransaction` Available but Potentially Dangerous [FIXED]
|
||||
**Location:** `mortgagefi-frontend/app/dapp/page.tsx`
|
||||
**Impact:** `useSendTransaction` was imported and available.
|
||||
|
||||
> [!danger] High — `useSendTransaction` Available but Potentially Dangerous [FIXED]
|
||||
> `useSendTransaction` was imported and available.
|
||||
|
||||
**Remediation:** ✅ `useSendTransaction` import removed from page.tsx. All transactions now go through `writeContractAsync` with ABI validation.
|
||||
|
||||
**Remediation:** Remove the import if not strictly needed, or wrap with strict validation.
|
||||
|
||||
---
|
||||
|
||||
## Medium Severity
|
||||
|
||||
### M1 — Contract Address Not Validated Before RPC Call
|
||||
**Location:** `nftcache/internal/fetcher/rpc.go:186-210`
|
||||
**Impact:** `makeRPCCall` inserts the `contract` parameter directly into JSON RPC payload without validation. A malformed address could cause RPC errors or, in the worst case, exploit a vulnerable JSON-RPC parser.
|
||||
|
||||
> [!warning] Medium — Contract Address Not Validated Before RPC Call
|
||||
> `makeRPCCall` inserts the `contract` parameter directly into JSON RPC payload without validation. A malformed address could cause RPC errors or, in the worst case, exploit a vulnerable JSON-RPC parser.
|
||||
|
||||
**Remediation:** Validate contract address with regex `^0x[a-f0-9]{40}$` before use.
|
||||
|
||||
---
|
||||
|
||||
### M2 — YAML Config Has Duplicate Key
|
||||
**Location:** `config/contracts.yaml`
|
||||
**Impact:**
|
||||
```yaml
|
||||
contracts:
|
||||
cbbtc:
|
||||
network: base
|
||||
address: "0x0987654321098765432109876543210987654321"
|
||||
cbbtc: # DUPLICATE!
|
||||
network: base
|
||||
address: "0xab825f45e9e5d2459fb7a1527a8d0ca082c582f4"
|
||||
```
|
||||
YAML parsers may silently ignore the first or second entry. This could cause the wrong contract to be used.
|
||||
|
||||
> [!warning] Medium — YAML Config Has Duplicate Key
|
||||
> A duplicate `cbbtc` key in `config/contracts.yaml`. YAML parsers may silently ignore one entry, causing the wrong contract to be used.
|
||||
|
||||
**Remediation:** Remove duplicate; use unique slugs (e.g., `cbbtc-v1`, `cbbtc-v2`).
|
||||
|
||||
---
|
||||
|
||||
### M3 — CORS Origin Reflection Subdomain Bypass
|
||||
**Location:** `nftcache/cmd/nftcache/main.go:32-55`, `schedy/cmd/schedy/main.go:28-59`
|
||||
**Impact:** The CORS middleware does exact string match on Origin. An attacker with a subdomain like `evil.mortgagefi.app` could not bypass it, but the code uses `strings.EqualFold` which is correct. However, if `CORS_ALLOW_ORIGIN` is accidentally set to `*` or left empty, CORS is completely disabled.
|
||||
|
||||
> [!warning] Medium — CORS Origin Reflection Subdomain Bypass
|
||||
> The CORS middleware uses correct exact-match logic, but if `CORS_ALLOW_ORIGIN` is accidentally set to `*` or left empty, CORS is completely disabled.
|
||||
|
||||
**Remediation:** Fail startup if `CORS_ALLOW_ORIGIN` is empty or `*` in production.
|
||||
|
||||
---
|
||||
|
||||
### M4 — No Content Security Policy (CSP)
|
||||
**Location:** Frontend (global)
|
||||
**Impact:** Without CSP, XSS attacks have full capability to execute scripts, connect to arbitrary domains, and exfiltrate data.
|
||||
|
||||
> [!warning] Medium — No Content Security Policy (CSP)
|
||||
> Without CSP, XSS attacks have full capability to execute scripts, connect to arbitrary domains, and exfiltrate data.
|
||||
|
||||
**Remediation:** Add CSP meta tag or header. Start with report-only:
|
||||
```html
|
||||
<meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'; connect-src 'self' https://*.walletconnect.com wss://*.walletconnect.org https://*.llamarpc.com; style-src 'self' 'unsafe-inline'; img-src 'self' data:;">
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### M5 — Test/Debug Endpoints and Logging in Production
|
||||
**Location:** `nftcache/internal/fetcher/rpc.go:95-102`
|
||||
**Impact:**
|
||||
```go
|
||||
// Always log token 103 regardless of debug mode
|
||||
if i == 103 {
|
||||
fmt.Printf("ALWAYS: Token 103 owner: %s (canonical: %s)\n", tokenOwner, canonicalOwner)
|
||||
}
|
||||
```
|
||||
Hardcoded debug logging leaks potentially sensitive ownership data in production logs.
|
||||
|
||||
> [!warning] Medium — Test/Debug Endpoints and Logging in Production
|
||||
> Hardcoded debug logging (token 103) leaks potentially sensitive ownership data in production logs.
|
||||
|
||||
**Remediation:** Remove hardcoded debug statements; use structured logging with level control.
|
||||
|
||||
---
|
||||
|
||||
### M6 — Auto-Reschedule Race Condition
|
||||
**Location:** `mortgagefi-frontend/app/dapp/page.tsx:948-974`
|
||||
**Impact:** The auto-reschedule `useEffect` fires an async IIFE that loops over all positions and cancels/re-creates schedules. With rapid re-renders (e.g., user rapidly switching chains), multiple overlapping executions can occur, leading to:
|
||||
- Duplicate scheduled jobs
|
||||
- Jobs created and immediately canceled
|
||||
- Rate limit exhaustion against Schedy API
|
||||
|
||||
> [!warning] Medium — Auto-Reschedule Race Condition
|
||||
> The auto-reschedule `useEffect` can run overlapping async executions on rapid re-renders, causing duplicate jobs, jobs created and immediately canceled, and rate limit exhaustion.
|
||||
|
||||
**Remediation:** Add an `isScheduling` ref/mutex to prevent concurrent executions:
|
||||
```typescript
|
||||
const isScheduling = useRef(false);
|
||||
useEffect(() => {
|
||||
if (isScheduling.current) return;
|
||||
isScheduling.current = true;
|
||||
(async () => { ... })().finally(() => { isScheduling.current = false; });
|
||||
}, [...]);
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### M7 — Missing Input Sanitization in Notification Message Builder
|
||||
**Location:** `mortgagefi-frontend/app/dapp/page.tsx:838`
|
||||
**Impact:** The notification message includes `positionUrl` and `collateralStr` derived from on-chain data. While unlikely, if a token symbol or contract returned malicious Unicode (e.g., RTL override characters), the message could be confusing or deceptive.
|
||||
|
||||
> [!warning] Medium — Missing Input Sanitization in Notification Message Builder
|
||||
> Notification messages include on-chain-derived strings; malicious Unicode (e.g., RTL override characters) could make messages confusing or deceptive.
|
||||
|
||||
**Remediation:** Sanitize all user-visible strings before building messages.
|
||||
|
||||
---
|
||||
|
||||
### M8 — No Backup/Recovery for BadgerDB
|
||||
**Location:** `data/nftcache`, `data/schedy`, `data/ntfy`
|
||||
**Impact:** If the host disk fails, all scheduled tasks and NFT cache data is lost. No backup strategy is documented.
|
||||
|
||||
> [!warning] Medium — No Backup/Recovery for BadgerDB
|
||||
> If the host disk fails, all scheduled tasks and NFT cache data is lost. No backup strategy is documented.
|
||||
|
||||
**Remediation:** Mount volumes from a persistent block store; schedule periodic backups.
|
||||
|
||||
---
|
||||
|
||||
### M9 — ntfy Uses Default/Weak User ID
|
||||
**Location:** `docker-compose.yml:27`
|
||||
**Impact:** `user: "4242:4242"` is a fixed UID/GID. If the host has a user with this ID, container files could be accessible outside the container.
|
||||
|
||||
> [!warning] Medium — ntfy Uses Default/Weak User ID
|
||||
> `user: "4242:4242"` is a fixed UID/GID. If the host has a user with this ID, container files could be accessible outside the container.
|
||||
|
||||
**Remediation:** Use a randomly generated high UID (e.g., `65534:nogroup`).
|
||||
|
||||
---
|
||||
|
||||
### M10 — Frontend Lacks Integrity Checks for Submodules
|
||||
**Location:** `.gitmodules`
|
||||
**Impact:** The `schedy` submodule is not pinned to a specific commit hash in documentation. A compromised submodule repository could inject malicious code.
|
||||
|
||||
> [!warning] Medium — Frontend Lacks Integrity Checks for Submodules
|
||||
> The `schedy` submodule is not pinned to a specific commit hash in documentation. A compromised submodule repository could inject malicious code.
|
||||
|
||||
**Remediation:** Pin submodule to a verified commit hash and verify in CI.
|
||||
|
||||
---
|
||||
|
||||
### M11 — `NFTCACHE_TTL` Default is 10 Minutes in `.env.local`
|
||||
**Location:** `.env.local`
|
||||
**Impact:** The default TTL is `10m`, causing very frequent background refreshes and unnecessary RPC load.
|
||||
|
||||
> [!warning] Medium — `NFTCACHE_TTL` Default is 10 Minutes in `.env.local`
|
||||
> The default TTL is `10m`, causing very frequent background refreshes and unnecessary RPC load.
|
||||
|
||||
**Remediation:** Set to `24h` or longer; make it configurable per contract.
|
||||
|
||||
---
|
||||
|
||||
### M12 — Schedy DeleteTask Performance Issue
|
||||
**Location:** `mortgagefi-frontend/submodules/schedy/internal/api/handler.go:110-142`
|
||||
**Impact:** Deleting a task requires a **full table scan** (`ListTasks()`) to find the timestamp. With thousands of tasks, this is O(n) and blocks other operations.
|
||||
|
||||
> [!warning] Medium — Schedy DeleteTask Performance Issue
|
||||
> Deleting a task requires a **full table scan** (`ListTasks()`) to find the timestamp. With thousands of tasks, this is O(n) and blocks other operations.
|
||||
|
||||
**Remediation:** Add a secondary index or use a key-only lookup.
|
||||
|
||||
---
|
||||
|
||||
## Low Severity / Informational
|
||||
|
||||
### L1 — `handleApprove` Approves Exact User Input, Not Max
|
||||
**Location:** `mortgagefi-frontend/app/dapp/page.tsx:708-725`
|
||||
|
||||
> [!info] Low — `handleApprove` Approves Exact User Input, Not Max
|
||||
> The approve function approves exactly the amount the user typed. This is UX-safe (no unlimited approvals) but requires two transactions for each new amount.
|
||||
|
||||
**Note:** The approve function approves exactly the amount the user typed. This is UX-safe (no unlimited approvals) but requires two transactions for each new amount.
|
||||
|
||||
### L2 — `enableMainnet` Feature Flag Could Be Accidentally Enabled
|
||||
**Location:** `mortgagefi-frontend/app/dapp/page.tsx:24`
|
||||
|
||||
> [!info] Low — `enableMainnet` Feature Flag Could Be Accidentally Enabled
|
||||
> Mainnet support is gated by an env var. If accidentally set to `true`, dummy placeholder addresses (`0x000...0001`) would be used, which is safer than real ones.
|
||||
|
||||
**Note:** Mainnet support is gated by an env var. If accidentally set to `true`, dummy placeholder addresses (`0x000...0001`) would be used, which is safer than real ones.
|
||||
|
||||
### L3 — `NFTCACHE_CONFIG` Path Not Validated
|
||||
**Location:** `nftcache/cmd/nftcache/main.go:263-264`
|
||||
|
||||
> [!info] Low — `NFTCACHE_CONFIG` Path Not Validated
|
||||
> If `NFTCACHE_CONFIG` is set to an arbitrary path, the service reads it. With `ro` mount this is mitigated, but direct file path injection is theoretically possible.
|
||||
|
||||
**Note:** If `NFTCACHE_CONFIG` is set to an arbitrary path, the service reads it. With `ro` mount this is mitigated, but direct file path injection is theoretically possible.
|
||||
|
||||
### L4 — No Health Check Endpoints
|
||||
**Location:** All services
|
||||
|
||||
> [!info] Low — No Health Check Endpoints
|
||||
> No `/health` or `/ready` endpoints for Docker/Kubernetes health checks.
|
||||
|
||||
**Note:** No `/health` or `/ready` endpoints for Docker/Kubernetes health checks.
|
||||
|
||||
### L5 — Verbose Console Logging in Production
|
||||
**Location:** `mortgagefi-frontend/app/dapp/page.tsx` (throughout)
|
||||
|
||||
> [!info] Low — Verbose Console Logging in Production
|
||||
> Extensive `console.log` statements leak internal state (cache keys, wallet addresses, token IDs) to browser DevTools.
|
||||
|
||||
**Note:** Extensive `console.log` statements leak internal state (cache keys, wallet addresses, token IDs) to browser DevTools.
|
||||
|
||||
---
|
||||
|
||||
## Remediation Priority Matrix
|
||||
|
||||
| Priority | Issue | Effort | Impact |
|
||||
|----------|-------|--------|--------|
|
||||
| **P0** | C2 — Rotate secrets & purge from Git | 1h | Critical |
|
||||
| **P0** | C3 — Remove NEXT_PUBLIC_ from secrets | 2h | Critical |
|
||||
| **P0** | C1 — Add SSRF protection to Schedy | 4h | Critical |
|
||||
| **P1** | C4 — Stop storing credentials in localStorage | 1d | High |
|
||||
| **P1** | C5 — Fix address canonicalization | 2h | High |
|
||||
| **P1** | C6 — Require API auth on nftcache | 2h | High |
|
||||
| **P1** | C7 — Validate RPC URLs | 2h | High |
|
||||
| **P1** | H3 — Close direct container ports | 30m | High |
|
||||
| **P2** | H1 — Add HTTPS/TLS | 4h | High |
|
||||
| **P2** | H2 — Add security headers | 1h | Medium |
|
||||
| **P2** | H10 — Fix compound transaction | 1h | Medium |
|
||||
| **P3** | M4 — Implement CSP | 4h | Medium |
|
||||
| **P3** | H6 — Pin Docker image digests | 1h | Medium |
|
||||
| **P3** | M6 — Fix auto-reschedule race | 2h | Medium |
|
||||
|
||||
---
|
||||
|
||||
## Verification Checklist (Post-Remediation)
|
||||
|
||||
- [ ] `.env` and `.env.local` are in `.gitignore` and purged from Git history
|
||||
- [ ] No `NEXT_PUBLIC_` variables contain secrets
|
||||
- [ ] Schedy rejects URLs with private IPs, loopback, and internal hostnames
|
||||
- [ ] nftcache requires `X-API-Key` and rejects requests without it
|
||||
- [ ] `canonAddr` rejects invalid/too-long/too-short addresses
|
||||
- [ ] Only nginx port (80/443) is exposed in Docker Compose
|
||||
- [ ] HTTPS is enforced with valid certificates
|
||||
- [ ] Security headers (CSP, HSTS, X-Frame-Options) are present
|
||||
- [ ] AWS credentials removed from frontend entirely
|
||||
- [ ] RPC URLs from localStorage are validated against an allowlist
|
||||
- [ ] All hardcoded secrets rotated and replaced with environment injection
|
||||
- [ ] Docker images pinned to SHA256 digests
|
||||
- [ ] Rate limiting implemented on all public endpoints
|
||||
|
||||
---
|
||||
|
||||
## Related
|
||||
|
||||
- [[Home]]
|
||||
- [[Architecture]]
|
||||
- [[Performance Audit]]
|
||||
- [[Project Audit 2026-06]]
|
||||
Reference in New Issue
Block a user