OhShii Labs review, round 2 — 5/6 · 4 findings (#27.1–#27.4): load shedding refuses exits, the Bridge counts admissions behind the commit, and two oracle clocks measure the wrong thing

[rvnt9999]

Round 2, part 5 of 6. Four items that share nothing except being the last four.

---

1. The load-shed inspect is caller-scoped, so it refuses shed users' exits — cancel, close, unstake, withdraw

Where: main.mo:7846-7871, :7860-7861, :767-769 (levelRank), :6115-6122 and :6188 (recomputeShedFloor), :729-732 (thresholds)

Severity: #play LOW (play money caps the loss, not the lockout), #production HIGH.

system func inspect({ caller : Principal }) destructures only caller. There is no msg
variant match anywhere in it, and this is deliberate — the comment at :7829-7832 states the
policy is "CALLER-ONLY (we don't pattern-match msg, so this never needs editing when a method
is added — Motoko would otherwise force every one of the ~100 methods to be listed here)". The
reasoning is sound as an argument about maintenance cost. The consequence was not followed
through.

The verdict at :7860-7861 is one uniform answer for the caller, applied identically to every
ingress update:

if (_shedFloor == 0) { return true };
return levelRank(levelOfKey(key)) >= _shedFloor;

levelRank maps L0 → 0 (:767-769), so at _shedFloor == 1 every rank-0 registered caller is
refused pre-consensus, on all updates. The exit set is in that group, and none of these is
reachable by query or by inter-canister call from the user:

endpoint	line
cancelMyOrder	:11420
cancelAllMyOrders	:8827
closePosition	:9602
withdrawMarginPool	:9288
unstakeInsurance	:10654
withdrawLp	:13666
withdraw	mixins/UserAccount.mo:176

The floor is not a dev-only pin. recomputeShedFloor (:6115-6122) runs unconditionally every
heartbeat (:6188) off Map.size(deferredExecs) against SHED_SOFT_STAGED = 2_000 /
SHED_HARD_STAGED = 5_000 (:729-732).

Why this is the sharpest form of the class. inspect is pre-consensus and is not a security
boundary — we are not claiming it is. It is an availability boundary, and it is the one place
where being stricter than the method body has no repair path: the user cannot retry into
consensus, cannot route around it, and receives no error the UI can classify. The congestion that
raised the floor is exactly what makes closing a position urgent. Our own #7 item 2 established
how a single account reaches 2,080 staged slots; this is what that escalation does to everybody
else, and Menese's #22 item 1 measurement (the batch traps at N = 2,400 loaned users with
S = 5,000 staged) shows the staged count is cheap to inflate.

Fix direction, and the trade-off is real. Exempting the exit set requires giving up the
"never edit inspect again" property for those methods only — a small msg match listing the
seven exits, accepted as a maintained list. The alternative that preserves the property is to
move the shed verdict into the method bodies of the entry-side methods and leave inspect
rejecting nothing but anonymous callers; that costs the pre-consensus saving on exactly the
methods where it was worth having. We think the first is the better trade and that the list
should carry a comment saying why it exists.

---

2. The Bridge advances its admission counter behind the DEX commit, so a lost reply double-charges the lifetime allowance

Where: src/bridge/main.mo:277-278, :281 (yield), :289-290, :295; src/backend/main.mo:5683, :5699-5701, :5254

Severity: #play MEDIUM, class HIGH with real funds.

t1  bridge:277-278  devSimulateDeposit("BTC", 1000). admitted = 0; admitting[k] := true.
                    Yields at :281 with seq = 0 + 1000 = 1000.
t2  main.mo:5683    1000 > playAdmitSeq(0) → not a replay.
    main.mo:5699    playDebit(bucket, markValue(1000))
    main.mo:5700    playReservedUnits[sk] += 1000
    main.mo:5701    playAdmitSeq[sk] := 1000        → replies #ok
t3  The Bridge's continuation is LOST — an upgrade lands, which is exactly the scenario the
    file documents at :310-332. Lines :289, :290 and :295 never run: admittedUnits[k] stays
    0 and l.pending stays 0. postupgrade (:335) clears `admitting`, so the user can retry.
t4  bridge:277      the user retries at a DIFFERENT size, say 1500 — nothing pins the retry
                    amount. admitted is still 0, so seq = 1500.
    main.mo:5683    1500 > playAdmitSeq(1000) → the replay gate does NOT fire
    main.mo:5699-5700  a SECOND markValue(1500) is debited and another 1500 reserved.

    Result: 2,500 units of value debited from the allowance against 1,500 admitted, and the
    first deposit's pending credit was never written either.

devSimulateDeposit is not dev-gated: bridge:271-272 checks only requireAuth and
amount != 0, and main.mo:5652 confirms it is the live on-ramp. The trigger is a Bridge
upgrade landing inside a deposit's await window — a routine deploy, and precisely the event the
file's own note at :310-318 calls "the routine act of deploying".

The root cause is not only the write-behind ordering, and this matters for the fix. The two
sequence numbers have opposite semantics despite the Bridge comment claiming "the same
discipline":

• Admit side (main.mo:5700): reserves the raw amount; the seq (playAdmitSeq) is a
  pure monotone replay token, used only at :5683 as if (seq <= existing) { return #ok }.
• Claim side (main.mo:5254): credits seq − prevSeq, a slice derived from the DEX's own
  high-water — deliberately, with a written rationale at :5248-5253 explaining that crediting
  the raw amount there would double-mint.

Fix direction — and one obvious version of it is wrong. Make the admission counter
write-ahead: move Map.add(admittedUnits, …) up beside the guard at bridge:278, and roll it
back only on the explicit #err at :282. This is safe on the admit side specifically
because the reserve is amount-based: a skipped seq range costs nothing, while a repeated
one double-reserves. It also makes the retry reproduce the same seq, which is what lets the
DEX's replay gate at :5683 do its job — today the retry computes a different seq, which is why
the gate misses.

Do not mirror this onto claim. There the credit is seq − prevSeq, so advancing the
Bridge's counter before the DEX confirms would shrink the next slice and lose user funds.

We are stating this as a fix direction, not a patch: unlike the fix in 1/6 we have not
compile-tested or end-to-end tested it, and one caveat remains open — write-ahead fixes the
allowance double-charge but the first deposit's l.pending credit (:295) is still lost when
the continuation dies. Writing both ahead and rolling both back on the explicit #err appears to
close that too, and is safe against the ambiguous-reject case because retrying with the same
seq is a no-op at :5683 — but we would want that reasoning checked by someone who owns the
Bridge before it ships.

---

3. The jump breaker's 30-second independence gate compares message-arrival clocks, not sample times

Where: main.mo:12941, :12960, :13015, :13019, :13031

The circuit breaker requires two independent observations at least 30 s apart before confirming a
move greater than 2.5%. The per-sample time at which the venue actually priced the asset is
recorded and then never read; the gate compares the times at which the two responses arrived
at this canister.

Because the oracle fires its outcalls in parallel and the responses return in an order the
canister does not control, an observation drawn from an older sample can arrive later and be
treated as the independent confirmation of a newer jump. The gate measures the transport, not the
market.

This extends our #9 item 2 and #9 item 3, which established that concurrent refreshes can stamp a
stale price as fresh; this is the same clock confusion inside the breaker that is supposed to
catch the result.

Fix direction. Judge independence on the sample timestamps that are already stored, not on
arrival; and discard any reading whose sample time predates the last accepted one.

---

4. The XRC fallback anchor is aged from arrival, never from the minute XRC priced, and it re-stamps refPrice as now

Where: main.mo:13099-13104, :13117-13118, :13170-13175, :13186-13191, :13229-13237, :5204; AMM.mo:320-324

The XrcAnchor record stores both clocks and says so in its own comments:

xrcTimestampSecs : Nat;   // the minute XRC priced (already 30-90s behind wall clock)
receivedNs       : Int;   // when WE stored it — the freshness clock for use

Repo-wide, xrcTimestampSecs appears at :13101 (declaration), :13172 (write), :5204 (the
dev injector setTestXrcRate, which hardcodes 0) and once in docs/. There is no reader.
Freshness is judged only on receivedNs, captured in the continuation of the outcall at :13159.

Two consequences compound:

1. XRC_ANCHOR_MAX_AGE_NS = 180 s (:13118) against a refresh cadence of 120 s (:13117) bounds
   how long we have held the anchor, not how old the price is. XRC's own timestamp is already
   30–90 s behind wall clock by the record's own comment, and that lag is invisible to the gate.
2. The fallback writer at :13229-13237 calls AMM.withRefPrice(p, a.rateE8, now), and
   AMM.mo:320-324 sets refPriceUpdatedNs = now. So an anchor of arbitrary admissible age is
   re-stamped as instantaneous, and the 180 s anchor window then composes with the
   downstream staleness walls (MARGIN_MAX_REFPRICE_AGE_NS = 300 s, :586) instead of bounding
   them.

This extends our #9 item 3 (which noted the XRC anchor is advisory) and is the same shape as the
xrcSources-never-read item already recorded in our audit: a field written for a safety purpose
with no consumer.

Fix direction. Age the anchor from xrcTimestampSecs (converting to ns) or from
min(receivedNs, xrcTimestampSecs), and pass the anchor's own effective timestamp into
AMM.withRefPrice rather than now, so downstream staleness gates see the true age.

— Ravenith, OhShii Labs

[dominicwilliams]

Replying on #27.3, which is partially fixed — and I want to be exact about which half, because the half that landed is not the half your finding is really about. The other three are confirmed and open.

#27.3 — the jump breaker's 30s gate compares arrival clocks. Partially fixed.

What landed: applyFreshAggregate now rejects any aggregate whose timestamp is at or before the current mark's — "stale aggregate: sampled at or before the current mark". That covers the second clause of your fix direction, discarding a reading whose sample time predates the last accepted one, and it closes the stale-overwrite case.

What did not land: the independence gate itself is unchanged. acceptOrPendPrice still compares now - pending.firstSeenNs against the confirmation gap, where now is the caller's continuation clock. So a confirmation is still judged independent by when we happened to process it, not by when the venues actually priced.

Two things I should state rather than let the fix imply more than it does:

• PriceFeed.Reading.fetchedAtNs — the genuine per-sample field — is written at four sites and read nowhere in the backend. The data your fix needs is already being collected and thrown away.
• The new guard's comment claims monotonicity in sample time, but agg.timestamp is itself stamped Time.now() in the refresh continuation. So it is really monotonic in aggregate-completion time. It still fixes stale overwrite; it does not order two refreshes by when the sources actually observed the market. The comment overstates what the code does, and that will be corrected.

#27.1, #27.2, #27.4 — confirmed, not fixed.

• OhShii Labs review, round 2 — 5/6 · 4 findings (#27.1–#27.4): load shedding refuses exits, the Bridge counts admissions behind the commit, and two oracle clocks measure the wrong thing #27.1 — the load-shed inspect still destructures only caller and returns a uniform verdict, so a shed user cannot reach an exit method. Refusing exits under load is the failure mode that turns congestion into forced losses, and it is the one I would fix first of the four.
• OhShii Labs review, round 2 — 5/6 · 4 findings (#27.1–#27.4): load shedding refuses exits, the Bridge counts admissions behind the commit, and two oracle clocks measure the wrong thing #27.2 — the Bridge still advances admittedUnits after the DEX commit, so a lost continuation leaves the counter at its old value, the retry computes a different seq, and the DEX replay gate misses. Write-ahead plus rollback-on-#err is the right shape.
• OhShii Labs review, round 2 — 5/6 · 4 findings (#27.1–#27.4): load shedding refuses exits, the Bridge counts admissions behind the commit, and two oracle clocks measure the wrong thing #27.4 — the XRC anchor still ages from arrival, and xrcTimestampSecs is written (including a hardcoded 0 on the dev injector) and has no reader. The fallback re-stamps the anchor as instantaneous.

Round 2 (#23–#28) has not been triaged as a wave; the first wave took priority and these are queued. Saying so is more useful to you than silence.

The code is in #30, the first push to this mirror since the July snapshot; your line numbers against 0241cba still resolve.