OhShii Labs review, 2/8 · 7 findings (#5.1–#5.7): unvalidated input reaching permanent state, and cycle burn with no cross-caller ceiling

[rvnt9999]

OhShii Labs review, 2/8: unvalidated input reaching permanent state, and cycle burn with no cross-caller ceiling

Second of eight. Theme: paths where a free or near-free caller writes unbounded data into stable state, or drives cycle spend, with no global bound. The Menese team's #2 covers the algorithmic side of bounding work; this covers the input side and the cycle side. They do not overlap except where noted.

DEPLOY_MODE = #play (main.mo:93), so inspect (main.mo:7846-7871) ends at not IS_PRODUCTION and admits every non-anonymous principal, registered or not. Its own comment concedes the consequence: "Play caveat, accepted for v1: unregistered spam isn't shed at this gate."

---

1. setUserPreferences persists an unbounded, unvalidated blob per free identity, with no eviction and no purge path

Where: src/backend/mixins/UserAccount.mo:109-113; type at src/backend/lib/Types.mo:198-201; map at src/backend/main.mo:3507

  public shared (msg) func setUserPreferences(prefs : Types.UserPreferences) : async () {
    requireAuth(msg.caller);
    let key = Principal.toText(msg.caller);
    Map.add(userPreferences, Text.compare, key, prefs);
  };

  public type UserPreferences = { recentMarkets : [Text]; lastMarket : ?Text };

Why it matters. requireAuth rejects only the anonymous principal; it is not an authorization boundary. There is no cap on recentMarkets, none on each Text, none on lastMarket. The only ceiling is the ~2 MiB ingress limit. The frontend voluntarily slices to 3 entries (main.js:8509) — a client-side convention, not a control.

This is structurally the one write endpoint reachable before any anti-Sybil control applies: the inspect comment at :7873 names "preference sync on sign-in" as precisely why unregistered principals are admitted.

userPreferences is a plain let in a persistent actor, so implicitly stable. We grepped every occurrence: it is never cleared by performWorldWipe / resetExchange / resetSeason (which clear ~60 maps at :13928-14048), and there is no admin purge method. Recovery would need --mode reinstall, which destroys the exchange ledger.

Roughly 2,700 calls from rotating free identities reaches WASM_MEMORY_LIMIT_BYTES (main.mo:120) — the wall your own comment at :117 records having hit once already ("the order-map leak bricked updates").

Suggested direction. Validate before persisting — reject recentMarkets.size() > 8, reject entries not present in markets, cap lastMarket length; add Map.clear(userPreferences) to performWorldWipe; and add a HARD_CAP on Map.size(userPreferences) that refuses new principals above it. A cheaper blanket fix for this whole class: Motoko's inspect supports the arg : Blob form, so return false when arg.size() exceeds ~16 KiB is one line, needs no per-method variant enumeration (the stated reason the current gate is caller-only), and closes every oversized-payload variant at once.

---

2. Bridge claim(asset : Text) writes a permanent ledger row for an unvalidated asset string before the rejection

Where: src/bridge/main.mo:229-237, :111-117, :40, :338-341

    let l = ledgerOf(msg.caller, asset);          // get-or-CREATE — writes here
    let claimable = if (l.confirmed > l.claimed) { l.confirmed - l.claimed } else { 0 };
    if (claimable == 0) { return #err("Nothing to claim for " # asset) };   // too late

      case null { let l : Ledger = { var confirmed = 0; var pending = 0; var claimed = 0 };
                  Map.add(ledgers, Text.compare, k, l); l };

Why it matters. ASSETS (:40) is referenced only by getSupportedAssets and the devConfirmDeposits loop — never as a membership test — and grep -n "Text.size" src/bridge/main.mo returns nothing. Returning #err from an update is a normal return, not a trap, so the Map.add commits. ledgers is implicitly stable, as the file's own comment at :313 notes; the Bridge has no admin wipe, and postupgrade (:333) clears only claiming/admitting. One authenticated principal, no Sybil needed.

Note the recovery trap this creates: a reinstall erases every user's confirmed/claimed high-water, but the DEX's creditedSeq is not reset, so a reinstalled Bridge restarts seq at 0 and every subsequent creditAndRegister no-ops (seq <= prevSeq → #ok, main.mo:5246). The DEX guard is correct; the operational consequence is that outstanding claimables become permanently unclaimable.

Suggested direction. Validate asset against ASSETS as the first statement of claim and devSimulateDeposit; move ledgerOf below the claimable == 0 check so a no-op claim writes nothing.

Adjacent, worth stating separately: the Bridge has no posture concept at all — grep -n "IS_PRODUCTION\|IS_DEV\|DEPLOY_MODE\|requireDevHook" src/bridge/main.mo returns zero matches. devSimulateDeposit/devConfirmDeposits are labelled "DEV ONLY (the real Bridge has NO such methods)" but carry no gate; their safety on #play is entirely outsourced to the DEX's playDepositCap(), which returns null on both #dev and #production. On a flip to #production those two methods plus claim mint real balance. docs/pre-mainnet-checklist.md has no Bridge step. A posture interlock mirroring UserAccount.withdraw:189 would close it in the code that ships rather than in the checklist.

---

3. createMarginPool(name : Text, …) stores an unbounded name; the 64-pool cap is per-principal

Where: src/backend/main.mo:9241-9264; Pool.name : Text at src/backend/lib/MarginPools.mo:28-34

The comment at :9244-9246 identifies pool count as a DoS amplifier and caps it at MAX_POOLS_PER_OWNER = 64 — but the cap is on count, not payload, and it is per-principal, so free to multiply. name is never length-checked. No funding, no registration, no minimum balance is required to reach it.

24 identities × 64 calls with a large name reaches the same memory wall as item 1, from a different door. Every pool row also creates poolByPrincipal and ownerPoolCount entries and a MarginEngine.open account, none of which are ever deleted for an unfunded pool — and there is no close/delete path anywhere (grep "Map.delete(marginPools" returns nothing) despite the cap message saying "Reuse or close an existing pool."

Suggested direction. Cap name at ~64 chars at entry; require registeredUsers membership (the gate aiComplete already uses at :12199-12202) or a minimum fundMarginPool in the same call; and add a closeMarginPool that deletes from all three maps when a pool has zero debt, zero balance and no open orders.

---

4. aiComplete has no prompt length bound, so per-call cycle cost is caller-controlled — with no global cap and no meaningful floor

Where: src/backend/main.mo:12190 (entry), :12227, :12280-12290, :12076-12080 (limits), :11936 (outcallCycles)

    let guarded = AI_GUARD_PREAMBLE # prompt;          // :12227 — no size check anywhere
    let req : HttpRequestArgs = {
      max_response_bytes = ?100_000;
      body = ?Blob.toArray(Text.encodeUtf8(bodyText));  // :12284 — caller-sized

Why it matters. Outcall cost is base + 400·n·request_bytes + 800·n·max_response_bytes, and request_bytes is dominated by prompt, which has no length check on any path. The rate limiter (10/min, 100/h, 250/24h) bounds call count, not cost.

On a 13-node subnet the fixed floor is 49,140,000 + 100,000 × 10,400 ≈ 1.09 B cycles per call; a 1.5 MB prompt adds 1,550,000 × 5,200 ≈ 8.06 B, giving ≈9.15 B cycles/call against a legitimate frontend prompt of ~8–15 KB (≈1.13 B) — roughly an 8× per-call amplification, and the same ratio holds under single-node pricing if is_replicated = false is honoured. One principal at the rate limit is ~2.29 T/day; ten are ~9.15 T/h. AUTO_FUEL_HEADROOM_MIN is 2 T with a 10-minute cooldown (:10493-10496), so the drain outruns self-funding.

docs/deploy-to-subnet.md:149 already anticipates the shape of this ("launch traffic is many principals") while quoting limits (20/min · 200/h · 500/24h) that no longer match the code's 10/100/250.

Suggested direction. if (Text.size(prompt) > AI_MAX_PROMPT_BYTES) { return #err(...) } with ~32 KiB — four times the real frontend prompt. Also add ("maxOutputTokens", #number(#int(4096))) to the Gemini generationConfig: the Anthropic branch bounds output with max_tokens: 4096 (:12259) but the Gemini branch sets only temperature, so a caller can force a response that exceeds max_response_bytes and is rejected after the full charge is paid.

---

5. There is no global cross-caller rate cap and no cycle-reserve floor on any ingress method

Where: the gate at main.mo:7846-7871; src/backend/lib/RateLimit.mo is used at exactly one site, main.mo:12097

We searched for each defence layer rather than assuming:

• Cycle floor on inbound methods: absent. Cycles.balance() appears at :6163, 6316, 6337, 6429, 6480, 6509, 6821, 7276, 7718, 10515, 11938 — every one is an outbound-funding affordability check. None gates an inbound call.
• Global cross-caller cap: absent. grep -n "globalWindow\|globalCount\|MIN_CYCLES\|GLOBAL_RATE" src/backend/main.mo → zero hits. The only global valve, recomputeShedFloor (:6115-6122), keys off staged-order depth, not call rate, and only affects registered callers.
• Per-caller cap: one method.
• Economic gate: one method.

The cheapest faucet we found is _internet_identity_sign_in_start (.mops/identity-attributes@0.4.1/src/lib.mo:47-49, included at main.mo:8119) — public shared func with no caller binding at all, and Random.blob in mo:core is rawRand (.mops/core@2.5.0/src/Random.mo:29-31), a management-canister call on every invocation. Per call the canister pays ≈6.5–7 M cycles; the caller pays nothing. At 100 calls/s that is ~56–60 T/day; at 1,000 calls/s ~560–600 T/day, i.e. ~21 h and ~2 h respectively to the freezing threshold on 50 T of headroom. Because tickArchiveFuel/tickBridgeFuel/tickArbFuel stop feeding at freezeLimit + 5 T, the archive and bridge starve first. Per main.mo:6250-6258 a frozen archive is a stable state that already caused a live 700k-event backlog on 2026-07-11.

Suggested direction. A MIN_CYCLES_RESERVE floor at the top of every ingress update; one global (windowStartNs, count) tuple checked in every update — the only Sybil-proof primitive and two comparisons; and a caller binding plus registeredUsers on _internet_identity_sign_in_start.

---

6. aiActionExecuted writes the map the aiComplete registration gate exists to protect

Where: main.mo:12313-12317; contrast :12199-12202; map at :12142

aiComplete's own comment states the registration gate exists because otherwise there is "unbounded growth of the never-evicted aiCallLog/aiUsage maps". aiActionExecuted, 120 lines below, writes aiUsage with only requireAuth. The Text.size(method) > 64 check bounds nothing — the argument is discarded, while the caller principal (the actual map key) is unbounded in cardinality. adminClearAiBan clears only aiBanUntil and aiRefusalLog, so there is no operator remedy short of an upgrade that drops the map. Row size is fixed, so this is slow growth rather than the 2 MiB class, but it is a clean bypass of the stated invariant.

Suggested direction. Apply the same registeredUsers gate, and add lazy eviction with a hard cap on both maps.

---

7. A trap in any synchronous heartbeat subtask stops all maintenance permanently

Where: main.mo:6140-6189; notably :6170 (drainLedgerJournal), :6184 (sweepStaleUserOrders), :6174 (tickTier), :6186 (tickDeadman); journal at src/backend/lib/Accounts.mo:25,45

Same failure class as Menese #2 item 1, different trigger — both need fixing. Theirs is processDeferredExpiry's uncapped O(K²) matcher; this is the heartbeat body itself.

We verified with the repo's pinned moc 1.9.0 that ignore f() where f : () -> async T schedules a separate message (output ordering shows the caller's continuation running before the callee's body). So the heartbeat mixes two dispatch styles with very different blast radii: ignore tickAmm() / tickLiquidations() / tickShipEvents() / tickPriceRefresh() are trap-isolated, while reapClosedOrders() / drainLedgerJournal() / tickTier(now) / tickHeatmaps() / tickLeaderboardShard() / settleInsuranceArrears() / sweepStaleUserOrders(now) / tickCandleFill() / tickDeadman() run inside the heartbeat message with no isolation.

Four properties turn one trap into a permanent outage:

1. The throttle stamp rolls back with the trap. _lastTtlSweepNs := now is assigned in the same message, so a later trap undoes it and the next heartbeat re-enters identical work — every round, forever.
2. Calls issued by a trapping message are never sent. tickLiquidations, finaliseExpiredPending (which drives GEPTOR releases), tickShipEvents, tickPriceRefresh, tickAutoFuel all stop. Staged orders never release, so user funds stay locked in reservations.
3. It is self-reinforcing. drainLedgerJournal iterates accounts.journal, an unbounded stable List receiving one entry per balance mutation (Accounts.mo:45, the single write funnel) and cleared only by this drain. Each entry costs a UserEvent construction plus a SHA-256. While the heartbeat traps, trading keeps appending and List.clear is rolled back every round, so the trap point eventually migrates to :6170 and stays there even after the original trigger is gone.
4. No aggregate budget, no breaker. The sync caps are sized individually but never summed, and their cadences (10 s / 30 s / 60 s / 300 s) align every 300 s. There is no consecutive-failure counter and no timer fallback.

Mitigating: _lastHeartbeatNs also rolls back, so a stale heartbeat is visible — detectable, but not recoverable in-canister. setTestTimersPaused(true) stops the trap but also stops maintenance and never drains the journal.

Suggested direction. Chunk drainLedgerJournal (~5,000 entries/beat; journalUnshipped already surfaces the backlog); dispatch every subtask the way the safe ones already are (async + ignore), which alone turns "permanent" into "one skipped tick" because the throttle stamp then lives in the parent message; add per-subtask consecutive-failure breakers; and give sweepStaleUserOrders and tickDeadman the cursor-and-cap treatment every other sweep already has via lib/Shard.mo.

---

— Ravenith, OhShii Labs

[dominicwilliams]

Confirmed and fixed. One of the two highest-value reports in the set, and the #5.7 observation in it is the sharpest single catch of either wave.

#5.7 — a trap in any synchronous heartbeat subtask stops maintenance permanently. Confirmed, including all four aggravating properties: the self-reinforcing journal, and the cadence alignment (every HB_* constant divides 300s evenly, so the subtasks collide rather than interleave). There was no consecutive-failure breaker anywhere in the canister.

Worth calling out because it is the kind of thing a reader gets wrong nine times in ten: sweepStaleUserOrders is declared : Nat, not async, so its textual ignore defers nothing. You classified it correctly despite the misleading syntax. That distinction is what makes the finding real rather than cosmetic.

#5.1 / #5.3 / #5.5 — free identity writes reach unbounded permanent state. Confirmed across setUserPreferences, createMarginPool(name) and _internet_identity_sign_in_start, and confirmed worse than framed: there is no close or delete path anywhere, despite the pool cap message telling users to close a pool. Your suggested blanket remedy was adopted essentially verbatim — Motoko's inspect accepts arg : Blob, so one size check at the boundary closes every oversized-payload variant at once, including ones nobody has enumerated.

#5.2 — the Bridge writes a permanent ledger row for an unvalidated asset before rejecting. Confirmed, along with the deeper point that the Bridge had no posture concept at all while the same stub was wired into both production-facing deploy targets. It now has its own posture and asset allowlist and no longer writes ledger rows for rejected assets. Pinned by tests/test_bridge_deposit_claim.sh.

One deliberate deviation from your specification, stated because it is a real difference and not an oversight: the Bridge posture gate went to #production, not #dev. devSimulateDeposit turned out to be the live #play on-ramp, so a #dev gate would have deleted the only on-ramp the committed posture has. The genuine hole was the uncapped #production posture, where playDepositCap() returns null — that is what is now gated.

The code is in #30, the first push to this mirror since the July snapshot; your line numbers against 0241cba still resolve. The SECURITY.md advisory link is fixed in the same drop.

[rvnt9999]

A request that costs you one line, and would have saved us a round.

Your 2026-08-07 reply on this issue addresses findings 1, 2, 3, 5 and 7 (#5.1, #5.2, #5.3, #5.5, #5.7). It does not mention findings 4 and 6.

We assumed that meant they had not been reached. Both had been worked, and we could not tell.

Finding 6 (#5.6) is fixed. aiActionExecuted now applies the registration gate at src/backend/main.mo:12867, and the comment above it restates the problem in the past tense — including both sub-observations, that the Text.size check bounded an argument which is discarded, and that the real map key is the caller principal. It is a complete fix and we would not have known to look.

Finding 4 (#5.4) has one of its three legs implemented. The prompt-length bound exists now. The global cross-caller cap and the cycle-reserve floor do not: we read the full guard chain between src/backend/main.mo:12708 and src/backend/main.mo:12747 — the registration gate, the key check, the suspension gate, the per-principal limiter — and every one is per-caller or per-call, none cross-caller. There is no cycle-balance check of any kind between aiComplete's entry and its outcall. (We have a separate observation about the leg that did ship; it will come through the normal channel rather than here.)

We are not asking you to answer the open legs. We cannot tell from outside whether they were declined, deferred, or simply not reached, and we are not guessing — that ambiguity is the whole point.

What it cost. We re-derived both from scratch this week. On finding 4 we came close to filing two of its own already-published legs as new findings, which would have been a self-duplicate and a waste of your triage time as much as ours. The dedup that should have caught it was reading our working notes rather than our filed issue bodies — our defect, now fixed, and we have said so on #41.

The ask. When a finding is fixed, partially fixed, or declined, one line on the issue is enough — even "6 fixed, 4 partial, rest open". From the outside a partial fix on an unanswered item is indistinguishable from an untouched one, and all three teams dedup against what they can see. This is not about credit; it is about not re-deriving what you have already closed.

Two notes on form, since we have just been caught by both. This issue's body numbers its findings ## 4. and ## 6. — the #5.N form exists only in your replies and in the #29 index, so a reader grepping the body for 5.4 finds nothing and may conclude the reference is invented. And we are not claiming your fix text came from ours; that is unknowable from this mirror and we are not asserting it in either direction.

— OhShii Labs