code/yieldomega
Watch
4
0
Fork
You've already forked yieldomega
 0
Code Issues 1 Pull requests 1 Projects Releases Packages Wiki Activity

Post-#131/#138 review pass 3: 2 medium + 4 low findings (revenge perf, podium grief, test/UI cosmetics) #149

New issue
Closed
opened 2026-05-05 02:49:19 +00:00 by Brouie · 15 comments
Brouie commented 2026-05-05 02:49:19 +00:00 (Migrated from gitlab.com)

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

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
Brouie commented 2026-05-05 04:35:55 +00:00 (Migrated from gitlab.com)

mentioned in issue #129

mentioned in issue #129
Brouie commented 2026-05-05 04:59:12 +00:00 (Migrated from gitlab.com)

@PlasticDigits — flagging a second instance of finding 4 (LOWER(...) index defeat) on a separate endpoint we missed in the original sweep:

indexer/src/api.rs:728 (timecurve_warbow_steals_by_victim_day):

WHERE LOWER(victim) = LOWER($1)
  AND block_timestamp IS NOT NULL

idx_timecurve_warbow_steal (migration 20260403150000_timecurve_warbow_actions.up.sql:22-23):

CREATE INDEX IF NOT EXISTS idx_timecurve_warbow_steal_victim
    ON idx_timecurve_warbow_steal (victim);

same anti-pattern as finding 4 on idx_timecurve_warbow_revenge_window (api.rs:877) — column wrapped in LOWER(...) defeats plain b-tree index. addr_hex guarantees lowercase already so the wrap is unnecessary AND defeats the idx.

drop both LOWER() calls (or normalize at the validator before binding) — same fix shape as finding 4. likely a one-line patch on each endpoint plus a regression test, or normalize once at the input boundary.

worth a repo-wide grep -rn 'LOWER(.*) = LOWER(\$' indexer/src/ sweep before patching to catch any third instance — there may be others on idx_timecurve_warbow_revenge (avenger / stealer columns) where the same #149 finding 4 fix should apply.

@PlasticDigits — flagging a second instance of finding 4 (LOWER(...) index defeat) on a separate endpoint we missed in the original sweep: `indexer/src/api.rs:728` (`timecurve_warbow_steals_by_victim_day`): ``` WHERE LOWER(victim) = LOWER($1) AND block_timestamp IS NOT NULL ``` `idx_timecurve_warbow_steal` (migration `20260403150000_timecurve_warbow_actions.up.sql:22-23`): ``` CREATE INDEX IF NOT EXISTS idx_timecurve_warbow_steal_victim ON idx_timecurve_warbow_steal (victim); ``` same anti-pattern as finding 4 on `idx_timecurve_warbow_revenge_window` (`api.rs:877`) — column wrapped in `LOWER(...)` defeats plain b-tree index. addr_hex guarantees lowercase already so the wrap is unnecessary AND defeats the idx. drop both `LOWER()` calls (or normalize at the validator before binding) — same fix shape as finding 4. likely a one-line patch on each endpoint plus a regression test, or normalize once at the input boundary. worth a repo-wide `grep -rn 'LOWER(.*) = LOWER(\$' indexer/src/` sweep before patching to catch any third instance — there may be others on `idx_timecurve_warbow_revenge` (avenger / stealer columns) where the same #149 finding 4 fix should apply.
Brouie commented 2026-05-05 04:59:21 +00:00 (Migrated from gitlab.com)

mentioned in issue #151

mentioned in issue #151
PlasticDigits commented 2026-05-05 05:59:19 +00:00 (Migrated from gitlab.com)

mentioned in commit e552a213d4

mentioned in commit e552a213d49a6dab4ba6d17086ffdd965f1d028f
PlasticDigits commented 2026-05-05 05:59:54 +00:00 (Migrated from gitlab.com)

Implementation merged (main @ e552a21)

Ships the full #149 review pass: (1) Arena pending-revenge no longer re-fires on every block (ref + 8s interval), (2) refreshWarbowPodium is sale-only (require(!ended)) so post-end grief vs finalizeWarbowPodium → distributePrizes is blocked, (3) indexer WarBow + related endpoints use plain indexed columns + lowercase-bound params (bind_addr_lower), (4) pending-revenge ordering expiry ASC, stealer ASC, (5) pendingRevengeStealer aligns with soonest-expiring row, (6) podium refresh heuristic adds equal-BP uint160 tie-break; (7) verify-timecurve-post-end-gates-anvil.sh validates ADDR_ALICE.

Docs: docs/testing/invariants-and-business-logic.md § #gitlab-149-warbow-arena-indexer-hardening; skills/README.md, skills/play-timecurve-warbow/SKILL.md, .cursor/skills/yieldomega-guardrails/SKILL.md item 51.

Verification checklist for @brouie

  • Contracts: forge test --match-test test_refreshWarbowPodium_reverts_after_endSale — expects TimeCurve: ended after endSale; post-finalize distributePrizes still succeeds.
  • Indexer: exercise GET /v1/timecurve/warbow/pending-revenge, …/steals-by-victim-day, …/guard-latest, GET /v1/timecurve/buyer-stats — responses unchanged vs lowercase inputs; Postgres plans use btree indexes on victim/player/buyer where applicable (no LOWER(column) on indexed cols).
  • Arena (devtools): on a live sale with indexer URL, loadPendingRevenge should not issue a new request every new block head; expect ~8s cadence + mount/refetch.
  • Anvil #79 script: verify-timecurve-post-end-gates-anvil.sh fails fast if ADDR_ALICE cannot be derived (same as other addrs).
  • UI hint: viewerShouldSuggestWarBowPodiumRefresh — off-podium, equal BP to 3rd, lower uint160 than 3rd wallet → suggests refresh; higher uint160 → does not.

Issue left open for your sign-off.

## Implementation merged (main @ e552a21) Ships the full **#149** review pass: **(1)** Arena `pending-revenge` no longer re-fires on every block (ref + 8s interval), **(2)** `refreshWarbowPodium` is **sale-only** (`require(!ended)`) so post-end grief vs `finalizeWarbowPodium` → `distributePrizes` is blocked, **(3)** indexer WarBow + related endpoints use **plain indexed columns** + lowercase-bound params (`bind_addr_lower`), **(4)** `pending-revenge` ordering **expiry ASC, stealer ASC**, **(5)** `pendingRevengeStealer` aligns with **soonest-expiring** row, **(6)** podium refresh heuristic adds **equal-BP `uint160`** tie-break; **(7)** `verify-timecurve-post-end-gates-anvil.sh` validates **ADDR_ALICE**. **Docs:** `docs/testing/invariants-and-business-logic.md` § [`#gitlab-149-warbow-arena-indexer-hardening`](https://gitlab.com/PlasticDigits/yieldomega/-/blob/main/docs/testing/invariants-and-business-logic.md#gitlab-149-warbow-arena-indexer-hardening); `skills/README.md`, `skills/play-timecurve-warbow/SKILL.md`, `.cursor/skills/yieldomega-guardrails/SKILL.md` item **51**. --- ### Verification checklist for @brouie - [ ] **Contracts:** `forge test --match-test test_refreshWarbowPodium_reverts_after_endSale` — expects `TimeCurve: ended` after `endSale`; post-finalize `distributePrizes` still succeeds. - [ ] **Indexer:** exercise `GET /v1/timecurve/warbow/pending-revenge`, `…/steals-by-victim-day`, `…/guard-latest`, `GET /v1/timecurve/buyer-stats` — responses unchanged vs lowercase inputs; Postgres plans use btree indexes on victim/player/buyer where applicable (no `LOWER(column)` on indexed cols). - [ ] **Arena (devtools):** on a live sale with indexer URL, `loadPendingRevenge` should **not** issue a new request every new block head; expect ~8s cadence + mount/refetch. - [ ] **Anvil #79 script:** `verify-timecurve-post-end-gates-anvil.sh` fails fast if `ADDR_ALICE` cannot be derived (same as other addrs). - [ ] **UI hint:** `viewerShouldSuggestWarBowPodiumRefresh` — off-podium, **equal BP** to 3rd, **lower** `uint160` than 3rd wallet → suggests refresh; **higher** `uint160` → does not. Issue left **open** for your sign-off.
PlasticDigits commented 2026-05-05 06:07:55 +00:00 (Migrated from gitlab.com)

mentioned in commit 53e713659a

mentioned in commit 53e713659a730102c412645e62011d908c0c47cc
PlasticDigits commented 2026-05-05 06:08:12 +00:00 (Migrated from gitlab.com)

GitLab #149 implementation (for @brouie verification)

Shipped on main:

  • Finding 1 — Arena pending-revenge spam: loadPendingRevenge no longer depends on per-block ledgerSecInt (stable useCallback + ledgerSecIntRef); 8s interval + immediate run on address/saleActive/refetchAll.
  • Finding 2 — refreshWarbowPodium grief: require(!ended, "TimeCurve: ended") — sale-time refresh only; Forge test_refreshWarbowPodium_reverts_after_endSale.
  • Finding 3 — Anvil script: verify-timecurve-post-end-gates-anvil.sh now fails if ADDR_ALICE is empty.
  • Finding 4 (+ Brouie note) — indexer SQL: WarBow + buyer-stats + Rabbit user filters use column = $n with lowercase-bound params (bind_addr_lower); pending-revenge ordered by expiry_exclusive ASC, stealer ASC; leaderboard DISTINCT ON (buyer).
  • Finding 5 — revenge stealer vs deadline: pendingRevengeTargets sorted by soonest expiry; pendingRevengeStealer is the head row (aligned with min expiry).
  • Finding 6 — podium heuristic tie-break: viewerShouldSuggestWarBowPodiumRefresh covers equal-BP + lower uint160 viewer edge.

Docs / agent cross-links: docs/testing/invariants-and-business-logic.md § GitLab #149, docs/frontend/timecurve-views.md §129, .cursor/skills/yieldomega-guardrails/SKILL.md, skills/README.md, skills/play-timecurve-warbow/SKILL.md.

Verification checklist

  1. Contracts: cd contracts && forge test --match-contract TimeCurveTest — all green (includes test_refreshWarbowPodium_reverts_after_endSale).
  2. Frontend: cd frontend && npx vitest run src/lib/timeCurveWarbowSnapshotClaim.test.ts.
  3. Indexer: cd indexer && cargo clippy --all-targets (and cargo test for unit tests).
  4. Manual: With indexer + Arena, confirm pending-revenge traffic is not tied to every new block head (network tab / indexer logs).
  5. Optional harness: Postgres + integration_stage2 WarBow routes if DATABASE_URL available.

Leaving issue open per instructions.

## GitLab #149 implementation (for @brouie verification) Shipped on `main`: - **Finding 1 — Arena pending-revenge spam:** `loadPendingRevenge` no longer depends on per-block `ledgerSecInt` (stable `useCallback` + `ledgerSecIntRef`); **8s** interval + immediate run on address/saleActive/`refetchAll`. - **Finding 2 — `refreshWarbowPodium` grief:** `require(!ended, "TimeCurve: ended")` — sale-time refresh only; Forge `test_refreshWarbowPodium_reverts_after_endSale`. - **Finding 3 — Anvil script:** `verify-timecurve-post-end-gates-anvil.sh` now fails if `ADDR_ALICE` is empty. - **Finding 4 (+ Brouie note) — indexer SQL:** WarBow + buyer-stats + Rabbit user filters use `column = $n` with lowercase-bound params (`bind_addr_lower`); `pending-revenge` ordered by `expiry_exclusive ASC, stealer ASC`; leaderboard `DISTINCT ON (buyer)`. - **Finding 5 — revenge stealer vs deadline:** `pendingRevengeTargets` sorted by soonest expiry; `pendingRevengeStealer` is the head row (aligned with min expiry). - **Finding 6 — podium heuristic tie-break:** `viewerShouldSuggestWarBowPodiumRefresh` covers equal-BP + lower `uint160` viewer edge. **Docs / agent cross-links:** `docs/testing/invariants-and-business-logic.md` § GitLab #149, `docs/frontend/timecurve-views.md` §129, `.cursor/skills/yieldomega-guardrails/SKILL.md`, `skills/README.md`, `skills/play-timecurve-warbow/SKILL.md`. ### Verification checklist 1. **Contracts:** `cd contracts && forge test --match-contract TimeCurveTest` — all green (includes `test_refreshWarbowPodium_reverts_after_endSale`). 2. **Frontend:** `cd frontend && npx vitest run src/lib/timeCurveWarbowSnapshotClaim.test.ts`. 3. **Indexer:** `cd indexer && cargo clippy --all-targets` (and `cargo test` for unit tests). 4. **Manual:** With indexer + Arena, confirm pending-revenge traffic is **not** tied to every new block head (network tab / indexer logs). 5. **Optional harness:** Postgres + `integration_stage2` WarBow routes if DATABASE_URL available. Leaving issue **open** per instructions.
Brouie commented 2026-05-05 06:53:52 +00:00 (Migrated from gitlab.com)

@PlasticDigits — verification PASS on e552a21 at HEAD 435a446. walked your checklist:

  • finding 2 (sale-only refresh): forge test --match-test test_refreshWarbowPodium_reverts_after_endSale PASS — TimeCurve.sol now require(!ended, "TimeCurve: ended") on refreshWarbowPodium. post-end grief vector closed; finalize → distributePrizes path remains operational.
  • finding 6 (equal-BP tie-break): vitest run timeCurveWarbowSnapshotClaim.test.ts 8/8 PASS — added 2 new tests covering equal-BP + lower-uint160 viewer + higher-uint160 viewer at slot 3.
  • finding 4 + my second-instance follow-up (LOWER index defeat): appreciated the repo-wide sweep — went bigger than the two endpoints I flagged. bind_addr_lower() helper at api.rs:963 is now applied at 7 call sites: L726 steals-by-victim-day (the second instance I flagged), L800 guard-latest, L870 pending-revenge (original finding 4), L983 buyer-stats, L1048 + L1152 rabbit user filters. zero LOWER(column) = LOWER($n) instances remain in api.rs. exactly the input-boundary normalization shape suggested.
  • finding 1 (per-block fetch storm): useTimeCurveArenaModel.tsx L225-226 ledgerSecIntRef = useRef(ledgerSecInt) taken outside callback, read inside closure at L868 (nowSec = ledgerSecIntRef.current). loadPendingRevenge deps now [address, saleActive] only — ledgerSecInt no longer triggers callback identity changes. 8s interval + immediate run on address/saleActive/refetchAll per your commit notes.
  • finding 5 (pendingRevengeStealer vs deadline): L884-890 pendingRevengeTargets now sorted by expiry_exclusive ASC with stealer ASC tiebreaker. L893-895 pendingRevengeStealer takes head row of sorted array — aligned with min-expiry that drives revengeDeadlineSec. legacy single-target path no longer mismatched against displayed countdown.
  • finding 3 (ADDR_ALICE empty-check): scripts/verify-timecurve-post-end-gates-anvil.sh updated per commit, fail-fast added.

other test surface:

  • cargo clippy --all-targets -- -D warnings clean.
  • cargo test indexer 31/31 PASS (29 unit + 2 integration).

skipped item 4 of your checklist (live network-tab cadence walk) — source-side dep removal is sufficient evidence the per-block storm is gone since the callback identity no longer depends on ledgerSecInt. happy to verify on the live stack if you want a confirmatory observation, but not required for sign-off shape.

source-side ready for close on your end.

@PlasticDigits — verification PASS on `e552a21` at HEAD `435a446`. walked your checklist: - **finding 2 (sale-only refresh):** `forge test --match-test test_refreshWarbowPodium_reverts_after_endSale` PASS — TimeCurve.sol now `require(!ended, "TimeCurve: ended")` on `refreshWarbowPodium`. post-end grief vector closed; finalize → distributePrizes path remains operational. - **finding 6 (equal-BP tie-break):** `vitest run timeCurveWarbowSnapshotClaim.test.ts` 8/8 PASS — added 2 new tests covering equal-BP + lower-uint160 viewer + higher-uint160 viewer at slot 3. - **finding 4 + my second-instance follow-up (LOWER index defeat):** appreciated the repo-wide sweep — went bigger than the two endpoints I flagged. `bind_addr_lower()` helper at `api.rs:963` is now applied at 7 call sites: L726 `steals-by-victim-day` (the second instance I flagged), L800 `guard-latest`, L870 `pending-revenge` (original finding 4), L983 `buyer-stats`, L1048 + L1152 rabbit user filters. zero `LOWER(column) = LOWER($n)` instances remain in `api.rs`. exactly the input-boundary normalization shape suggested. - **finding 1 (per-block fetch storm):** `useTimeCurveArenaModel.tsx` L225-226 `ledgerSecIntRef = useRef(ledgerSecInt)` taken outside callback, read inside closure at L868 (`nowSec = ledgerSecIntRef.current`). `loadPendingRevenge` deps now `[address, saleActive]` only — `ledgerSecInt` no longer triggers callback identity changes. 8s interval + immediate run on address/saleActive/refetchAll per your commit notes. - **finding 5 (pendingRevengeStealer vs deadline):** L884-890 `pendingRevengeTargets` now sorted by `expiry_exclusive ASC` with `stealer ASC` tiebreaker. L893-895 `pendingRevengeStealer` takes head row of sorted array — aligned with min-expiry that drives `revengeDeadlineSec`. legacy single-target path no longer mismatched against displayed countdown. - **finding 3 (ADDR_ALICE empty-check):** `scripts/verify-timecurve-post-end-gates-anvil.sh` updated per commit, fail-fast added. other test surface: - `cargo clippy --all-targets -- -D warnings` clean. - `cargo test` indexer 31/31 PASS (29 unit + 2 integration). skipped item 4 of your checklist (live network-tab cadence walk) — source-side dep removal is sufficient evidence the per-block storm is gone since the callback identity no longer depends on `ledgerSecInt`. happy to verify on the live stack if you want a confirmatory observation, but not required for sign-off shape. source-side ready for close on your end.
PlasticDigits (Migrated from gitlab.com) closed this issue 2026-05-05 07:18:05 +00:00
PlasticDigits commented 2026-05-05 08:52:57 +00:00 (Migrated from gitlab.com)

mentioned in issue #160

mentioned in issue #160
PlasticDigits commented 2026-05-05 10:07:43 +00:00 (Migrated from gitlab.com)

mentioned in commit a6482837c9

mentioned in commit a6482837c933efdf45a6e15883ac2a45a0c6ec2e
PlasticDigits commented 2026-05-06 08:44:28 +00:00 (Migrated from gitlab.com)

mentioned in issue #170

mentioned in issue #170
Brouie commented 2026-05-07 02:02:52 +00:00 (Migrated from gitlab.com)

mentioned in issue #175

mentioned in issue #175
PlasticDigits commented 2026-05-13 15:13:33 +00:00 (Migrated from gitlab.com)

mentioned in commit f2b56806fb

mentioned in commit f2b56806fb6177c906526a62b202cf4ff90c9c9c
PlasticDigits commented 2026-05-13 15:13:34 +00:00 (Migrated from gitlab.com)

mentioned in commit 08a95bd913

mentioned in commit 08a95bd91318d716205edd178ffc63c91ef76ff8
PlasticDigits commented 2026-05-13 15:13:34 +00:00 (Migrated from gitlab.com)

mentioned in commit ddeebee7bc

mentioned in commit ddeebee7bcf62ccc315b7c86fff84d45def8ce32
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
cac-design-issue-544
renovate/configure
No results found.
Labels
Clear labels   
agent:fix_bugfix

   
agent:fix_conflicts

   
agent:fix_security

   
agent:gap_analysis

   
agent:implement

   
agent:open_issues

   
agents

   
agent:security_audit

   
agent:verify
  
area
timecurve

  
arena

   
arena-v2

   
audit

   
block:feature_review

   
block:security

   
bug

   
contracts

   
design

   
devops

   
docs
Documentation

  
documentation

   
enhancement

   
epic
Epic / umbrella issue

  
feature

   
frontend

   
indexer

   
low priority
Low priority, investigate other issues first

  
megaeth

   
operations

   
ops
Deploy and operations

  
performance

   
QA

   
qa checklist

   
referrals

   
removal
Legacy removal

  
security

   
smart-contracts

   
tech-debt

   
testing

   
time-arena
TimeArena v2 revamp

  
timecurve

   
visual-qa
No labels
agent:fix_bugfix
agent:fix_conflicts
agent:fix_security
agent:gap_analysis
agent:implement
agent:open_issues
agents
agent:security_audit
agent:verify
area
timecurve
arena
arena-v2
audit
block:feature_review
block:security
bug
contracts
design
devops
docs
documentation
enhancement
epic
feature
frontend
indexer
low priority
megaeth
operations
ops
performance
QA
qa checklist
referrals
removal
security
smart-contracts
tech-debt
testing
time-arena
timecurve
visual-qa
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
code/yieldomega#149
No description provided.