third-pass review on the broader stack at HEAD `a421ba1`, after the #131 children (#132-#137) and #138 children (#139-#146) shipped clean. dedup'd against everything filed/verified earlier in the week and against today's hygiene ticket #148. ordered by severity descending. findings #1 and #2 stack with each other under MegaETH sub-second block load — pre-mainnet attention recommended even though severity reads low on a strict-funds-loss reading. --- ## Finding 1 — Frontend: pending-revenge query fires on every chain block (Medium) `frontend/src/pages/timeCurveArena/useTimeCurveArenaModel.tsx`: ``` const loadPendingRevenge = useCallback(() => { if (!address || !indexerBaseUrl() || !saleActive) { setPendingRevengeRows([]); return; } void fetchWarbowPendingRevenge(address, ledgerSecInt).then(/* ... */); }, [address, ledgerSecInt, saleActive]); // L876 — note ledgerSecInt in deps // L898-900 useEffect(() => { loadPendingRevenge(); }, [loadPendingRevenge]); ``` `ledgerSecInt = Math.floor(blockChainSec)` advances on every new head block reported by `useBlock({watch: true})` (line 224). Because `ledgerSecInt` is in the callback's dependency array, the callback identity changes every block, which makes the effect re-fire every block and re-issue `GET /v1/timecurve/warbow/pending-revenge?victim=…&now_sec=…`. **concrete consequence on MegaETH**: at sub-second block times, every connected arena viewer issues ~100 indexer requests/second per user. revenge windows are 24h, not seconds — the freshness budget for this query is wildly over-spent. drains indexer rate budget and per-user RPC quota for zero correctness benefit. **stacks with finding #4 below** (the indexer-side query is also seq-scan-per-call due to a separate index defeat). compounded effect: every connected viewer triggers a seq scan over `idx_timecurve_warbow_revenge_window` once per block. **mitigation**: pin the callback to `[address, saleActive]` and capture `ledgerSecInt` via a ref read inside the closure; or replace the effect with a bounded `setInterval` (5-10s) plus an immediate refresh on address/saleActive change and on user actions via the existing `refetchAll` path. revenge windows are 24h — no UX benefit to sub-second freshness. --- ## Finding 2 — Contract: `refreshWarbowPodium` is permissionless and clears finalize latch unconditionally — griefs `distributePrizes` (Medium, may be reframed Low) `contracts/src/TimeCurve.sol:1070-1078`: ``` function refreshWarbowPodium(address[] calldata candidates) external nonReentrant { warbowPodiumFinalized = false; // L1071 — first statement, unconditional for (uint256 i; i < candidates.length; ++i) { address c = candidates[i]; if (c == address(0)) continue; _updateWarbowPodium(c, battlePoints[c]); } emit WarbowPodiumRefreshed(msg.sender, candidates.length); } ``` no `onlyOwner`, no `require(!ended)`, no `require(saleStart > 0)`, no minimum candidates length. **anyone** can call this **after `endSale`** with an empty array (~25k gas — single SSTORE clearing the bool + one event emit) and force `warbowPodiumFinalized = false`. **attack path on a public mempool**: 1. owner calls `finalizeWarbowPodium([…])` post-endSale — sets `warbowPodiumFinalized = true` 2. owner queues `distributePrizes()` tx 3. attacker frontruns with `refreshWarbowPodium(new address[](0))` — legal, ~25k gas 4. `warbowPodiumFinalized = false` again 5. `distributePrizes` reverts at L854 with `TimeCurve: warbow podium not finalized` 6. attacker repeats indefinitely owner can recover via multicall / atomic bundle (or a future `finalizeAndDistribute` bundling function), but on a public mempool this forces ops into bundling rather than two simple txs. **not a fund-loss** in the strict sense — owner can still complete via a contract that bundles finalize + distribute. that's why a strict reading is Low. but it forces a deploy/ops change to a public-mempool grief vector that didn't exist before, on a contract going to mainnet. dev's pattern on #131 was to bump severity on adversarial findings so flagging this as Medium-leaning rather than Low. **mitigation options**: - **(A) gate refreshWarbowPodium with `require(!ended)`** — simplest, cleanest. refresh is a sale-time UX nicety; once ended, only owner-controlled `finalizeWarbowPodium` should mutate the snapshot. matches the function's NatSpec intent. - **(B) require `candidates.length > 0`** — still permissionless post-end but at least requires one address. weaker — attacker just picks a known BP holder. - **(C) add `onlyOwner finalizeAndDistribute(address[] candidates)`** — bundles both ops behind a single `nonReentrant` call. solves the racing window without changing existing function semantics. (A) is what I'd lean. (C) is a useful add either way for ops ergonomics. --- ## Finding 3 — Test script: `verify-timecurve-post-end-gates-anvil.sh` empty-check silently drops ADDR_ALICE (Low) `scripts/verify-timecurve-post-end-gates-anvil.sh:30-33`: ``` PK_ALICE="${PK_ALICE:-0x59c699...}" ADDR_ALICE="${ADDR_ALICE:-$(cast wallet address --private-key "$PK_ALICE" 2>/dev/null || true)}" PK_BOB="${PK_BOB:-0x5de411...}" PK_CAROL="${PK_CAROL:-0x7c8521...}" ADDR_BOB="${ADDR_BOB:-$(cast wallet address --private-key "$PK_BOB" 2>/dev/null || true)}" ADDR_CAROL="${ADDR_CAROL:-$(cast wallet address --private-key "$PK_CAROL" 2>/dev/null || true)}" ADDR_DEPLOYER="${ADDR_DEPLOYER:-$(cast wallet address --private-key "$PK_DEPLOYER" 2>/dev/null || true)}" ``` then **L41-43** validates ADDR_BOB / ADDR_CAROL / ADDR_DEPLOYER are non-empty: ``` if [[ -z "$ADDR_BOB" || -z "$ADDR_CAROL" || -z "$ADDR_DEPLOYER" ]]; then echo "verify-timecurve-post-end-gates-anvil: could not derive ADDR_BOB / ADDR_CAROL / ADDR_DEPLOYER." >&2 exit 1 fi ``` **ADDR_ALICE is missing from the empty-check** even though it's interpolated downstream at L161: ``` FIN_WB="[$ADDR_ALICE,$ADDR_BOB,$ADDR_CAROL,$ADDR_DEPLOYER]" cast send "$TC" "finalizeWarbowPodium(address[])" "$FIN_WB" ... ``` **failure modes**: - if `cast wallet address --private-key "$PK_ALICE"` fails (cast missing in subshell PATH, malformed PK_ALICE env override, etc.) → `ADDR_ALICE=""` and `FIN_WB="[,0x...,0x...,0x...]"` - depending on cast version: either rejects with parse error (script exits with no diagnostic for the root cause), or normalizes the empty slot to `address(0)` which the contract silently skips at TimeCurve.sol:1075 — alice never participates in finalize, the script appears to PASS, and the test's coverage is silently smaller than advertised **mitigation**: re-add ADDR_ALICE to the empty-check at L41: ``` if [[ -z "$ADDR_ALICE" || -z "$ADDR_BOB" || -z "$ADDR_CAROL" || -z "$ADDR_DEPLOYER" ]]; then ``` or fail-fast immediately after each `cast wallet address` derive if the result is empty. --- ## Finding 4 — Indexer: pending-revenge query wraps both victim columns in `LOWER(...)` and defeats the b-tree index (Low — stacks with #1 to Medium under load) `indexer/src/api.rs:877` and `889-891`: ``` WHERE LOWER(victim) = LOWER($1) -- ... AND NOT EXISTS ( SELECT 1 FROM idx_timecurve_warbow_revenge r WHERE LOWER(r.avenger) = LOWER($1) AND LOWER(r.stealer) = LOWER(l.stealer) -- ... ) ``` migration `20260504200000_warbow_revenge_window_gl135.up.sql:19-20`: ``` CREATE INDEX IF NOT EXISTS idx_timecurve_warbow_revenge_window_victim ON idx_timecurve_warbow_revenge_window (victim); ``` every address insertion goes through `addr_hex(a) = format!("{:#x}", a)` (`indexer/src/persist.rs:17-18`). Rust's `{:#x}` format for the alloy `Address` type produces lowercase hex with `0x` prefix. **all addresses in the table are guaranteed lowercase by construction**. wrapping the indexed column in `LOWER(...)` defeats the b-tree index — postgres can only use the index if the query expression matches the index expression. `LOWER(victim) = ...` doesn't match an index on plain `victim`, so the planner falls back to seq scan. **concrete consequence**: every `/v1/timecurve/warbow/pending-revenge` request triggers a seq scan over `idx_timecurve_warbow_revenge_window`. at low traffic this is fine. **stacked with finding #1** (~100 req/sec/user during arena viewing on MegaETH), every connected viewer triggers a seq scan per block over a table that grows linearly with WarBow activity. cliff under sustained PvP. **mitigation**: drop the column-side LOWER (rely on the lowercase invariant established by `addr_hex`): ``` WHERE victim = LOWER($1) -- ... AND LOWER(r.avenger) = LOWER($1) -- ok, idx_timecurve_warbow_revenge has no avenger index per migration check AND r.stealer = LOWER(l.stealer) ``` or add a functional index on `LOWER(victim)` if the codebase wants to keep the case-insensitive guarantee defensively. dropping is cheaper. --- ## Finding 5 — Frontend: `pendingRevengeStealer` (alphabetical first) and `revengeDeadlineSec` (min expiry) can refer to different stealers (Low) `frontend/src/pages/timeCurveArena/useTimeCurveArenaModel.tsx:884-887`: ``` const pendingRevengeStealer = pendingRevengeTargets[0]?.stealer !== undefined ? (pendingRevengeTargets[0].stealer as `0x${string}`) : undefined; ``` L888-899: ``` const revengeDeadlineSec = useMemo(() => { if (pendingRevengeTargets.length === 0) return 0n; return pendingRevengeTargets.reduce( (min, r) => { const e = BigInt(r.expiry_exclusive); return min === 0n || e < min ? e : min; }, 0n, ); }, [pendingRevengeTargets]); ``` `pendingRevengeStealer` picks the **first item** in `pendingRevengeTargets` (whose order is whatever the API returned). per `indexer/src/api.rs:896` the API `ORDER BY l.stealer` (alphabetical hex). `revengeDeadlineSec` reduces all targets to the **minimum expiry**. with three open windows `{0xA: T+12h, 0xB: T+6h, 0xC: T+24h}`: - `pendingRevengeStealer = 0xA` (alphabetical first) - `revengeDeadlineSec = T+6h` (0xB's expiry) **downstream consumers**: - `runWarBowRevenge` simulation hook at L2151-2163 fires `args: [pendingRevengeStealer]` — simulates the wrong stealer - action handler at L2574: `const stealer = stealerArg ?? pendingRevengeStealer` — falls back to the legacy field if no arg passed, fires the tx against the wrong stealer - `dotMega.ts:87` already marks the field `@deprecated single-target; prefer pendingRevengeStealers for multi-window (#135)` — dev knows it's stale, hasn't fully removed active per-stealer buttons in `WarbowHeroActions.tsx` pass an explicit stealer arg, so the visible UI path is safe. but the deprecated singular field is still wired into hot paths and could be invoked by mistake (headless calls, future consumers, anywhere the optional `stealerArg` is omitted). **mitigation**: drop the legacy single-target field outright — every consumer already has access to `pendingRevengeTargets` via the model export. or define both fields as the soonest-expiring entry (sort `pendingRevengeTargets` by `expiry_exclusive` ASC once and re-export the head as both fields). --- ## Finding 6 — Frontend: `viewerShouldSuggestWarBowPodiumRefresh` heuristic misses the contract's equal-BP tie-break (Low — cosmetic) `frontend/src/lib/timeCurveWarbowSnapshotClaim.ts:42`: ``` return viewerBp > v3; ``` contract's `_shouldSwapPodium` (`contracts/src/TimeCurve.sol:1037-1042`): ``` function _shouldSwapPodium(Podium storage p, uint8 a, uint8 b) internal view returns (bool) { if (p.values[a] < p.values[b]) return true; if (p.values[a] > p.values[b]) return false; if (p.winners[a] == p.winners[b]) return false; return uint160(p.winners[a]) > uint160(p.winners[b]); // lower-numerical-address wins ties } ``` contract treats equal BP as a tie broken by **lower-numerical-address wins**. heuristic only suggests refresh on strict-greater BP and misses the equal-BP-with-lower-address case where the viewer would actually displace slot 3 after a refresh. **concrete case**: `viewerBp == podiumValues[2]` and `uint160(viewerAddress) < uint160(podiumWallets[2])` — viewer would displace slot 2 after refresh, but heuristic returns `false` and UI doesn't surface the suggestion. **not blocking** — viewer can still click refresh manually. the heuristic's promise of "suggest when refresh would help" is just incomplete on the tie-break edge. **mitigation**: when not on the podium, suggest if `viewerBp > podiumValues[2] || (viewerBp == podiumValues[2] && BigInt(viewer) < BigInt(podiumWallets[2]))` — JS equivalent of the contract's tie-break. --- ## Pre-deploy relevance - **deploy-tonight scope**: findings 1, 2, 4 (load + grief on a permissionless mempool — pre-mainnet attention recommended). - **post-deploy hardening**: findings 3, 5, 6. cc @PlasticDigits