dfinity / dfinity/public-multidex
OhShii Labs review, round 18 · 9 findings (#54.1–#54.9): the season seal races its own gates, and what the 1.60 drop fixed for us
Nobody has claimed this yet.
- Dominant language
- Motoko
- Stars
- 14
- Forks
- 6
- PR merge metrics
- No merged PRs in 30d
Description
Scope. The 1.60 hardening drop (f233d18 … 1fff1d7), 229 files, +23,069/−2,358. main.mo
goes from 15,686 to 18,220 lines. This round read three bands of main.mo the task register does
not cite by line (9000-9999, 16000-16999, 17000-17999), the four value-path modules the drop
modified (MatchingEngine.mo, LiquidityManager.mo, MarginPools.mo, ArchiveCanister.mo), and
the delegation page after its fix. Every line number below is at 1fff1d7.
Filed as a public issue per the revised SECURITY.md (aac2da9). Two further items went to the
private channel instead, in one private report that references GHSA-qgvc-r8wq-hjq2: a residue
of the sign-in/delegation flow, which the policy reserves for that channel, and a note on that
page's disclosure timeline. Neither concerns this venue's balances, and neither is repeated here.
What was run. #54.1 is reproduced on PocketIC against the backend wasm compiled from 1fff1d7,
unmodified, with the steps below; the archive tape rules in What we verified were measured the
same way; everything else is a source read with the numbers derived from the code's own arithmetic,
and each finding says which it is.
#54.1 — resetSeason evaluates its three seal gates before the Bridge round-trip and seals after it, so an event landing in the window is written into the permanent season record and then deleted unshipped [play: MEDIUM / prod: N/A]
prod is N/A: resetSeason returns #err on #production at :17040-17042.
The invariant, in the code's words
The gates at :17045-17067 exist so that the record seals exactly the shipped tape: shippedSeq == nextEventSeq and List.size(accounts.journal) == 0 — your W4-01 text at :17058-17063 says
"sealing with rows still queued would leave the season tape short of its final #delta rows while
the gate above reads fully shipped". At the seal, rec.finalEventSeq must equal shippedSeq and
rec.chainHead must equal the sealed archive's certified head.
What the code does
// :17045-17067 three gates, all read here
if (shippedSeq > nextEventSeq) { return #err(...) };
if (nextEventSeq != shippedSeq) { return #err("unshipped history: ...") };
if (List.size(accounts.journal) > 0) { return #err("unshipped ledger journal: ...") };
// :17085 the W4-12 two-phase call — the message yields here
switch (await br.adminSeasonWipe()) { ... };
// :17112-17120 the record is built from state read AFTER the yield
let rec : SeasonRecord = { ... finalEventSeq = nextEventSeq; chainHead = _chainHead; ... };
await* performWorldWipe(true); // :17121 → :16908-16916 clears userEvents, nextEventSeq, shippedSeq, the journal; :16931 chainHead := null
emitEventRaw (:8519-8527) appends unconditionally — there is no boundary latch; nextEventSeq
has exactly two writers (:8526, :16909). Every user update that emits an event (stakeInsurance :12835, depositLp/withdrawLp, fills, deposits) can therefore land between :17067 and :17112.
The only brake in the tree, _timersPaused (:1650), gates heartbeat subtasks, not ingress; and
the exit set is never shed at any floor (:9655-9673, W1-05), so a shed venue still admits exactly
the messages that emit.
Reproduced — PocketIC, backend and bridge wasm built from 1fff1d7, unmodified
Fixture per season: fund one identity through setTestBalance, let the heartbeat drain the ledger
journal and ship the tape (getCanisterInfo reads ledgerJournalPending 0, journalUnshipped 0,
archivedEvents N), then setTestTimersPaused(true) — the runbook's own posture across the
boundary. The three gates pass. Predictions were written before the first run.
| run | topology | what was submitted | rec.finalEventSeq vs N |
archive stats().nextSeq |
heads |
|---|---|---|---|---|---|
| control | Bridge wired, reset alone | resetSeason |
N (2 = 2) |
N |
record head == certified head |
| control | Bridge NOT wired (no await) |
resetSeason, then stakeInsurance in the same round |
N in three runs, whichever message ran first |
— | equal |
| race | Bridge on a second application subnet, stakeInsurance submitted one round after the reset |
N + 1 (2 vs 1), deterministic across three runs |
N — event N never shipped |
record 624f05… ≠ certified 51e43f… |
|
| race | same subnet, both messages inducted in the same round | N + 1 (22 vs 21) on the 5th of 5 attempts (induction order is not the submitter's to choose; the first four ran the stake first and the gate refused the reset, correctly) |
N |
differ |
After the race the DEX reports journalUnshipped 0: the event that the record claims exists
nowhere. The stakeInsurance call itself returned #ok.
A scheduling fact worth stating for the fix: on one PocketIC subnet the DEX→Bridge→DEX round trip
completes inside a single round, so the window is intra-round and is hit only by an ingress inducted
in that round behind the reset; with the Bridge on another subnet the reply cannot arrive before the
next round and any ingress of that round lands in it. We do not know which topology the live
deployment has.
What bounds it
Controller-only and #play/#dev only (:17040-17043); the window is one round trip; the runbook
(:17032-17038) stops the bot fleet, not humans. The season wipe zeroes every wallet anyway, so
the economic content of the lost event is nil for season N+1 — the loss is to season N's sealed
record: finalEventSeq one past the archive, and a chainHead (:17022, "tamper-evidence") that
matches no archive's certified_data. That is the uncapped class.
Nearest filed items, and the discriminator
Both nearest items are ours and both are fixed: #25.3 / W4-01 added the journal gate; #25.2 /
W4-12 added the Bridge call. The fix for #25.2 introduced the yield that defeats the fix for
#25.3 — neither task file, docs/bridge-and-cks-design.md:188-201, nor any of our filings says
the gates run before a yield. #43 (:17046, @andreij6) is the cursor-divergence guard on the
same function and is untouched.
Remedy — shape and clause
Shape: re-evaluate the three gates after :17091 and before :17112. Clause, and it is the
half that can ship separately: a failed re-check cannot abort — the Bridge has already wiped
its half at bridge/main.mo:443-455, and aborting recreates the #25.2 divergence with the roles
reversed. So the re-check can only detect; prevention needs the entry doors closed across the
await — a transient sealing latch set before :17085 that the emitting entry paths refuse under,
exits untouched per the exit-set doctrine, cleared in the wipe — or an honest seal that records
finalEventSeq = shippedSeq and takes chainHead from the archive's certified head, logging the
dropped count as a #gap-class entry. Rejected alternative: calling the Bridge before the
gates — then every gate refusal leaves the pair half-wiped. The property to pin is
rec.finalEventSeq == shippedSeq at :17122 under a two-message race, not a grep.
#54.2 — adminReplayReset does not take the single-flight guard that adminReplayStep holds, so a reset landing inside a step's await produces the W4-09 false reserve alarm through the other writer [play: MEDIUM / prod: MEDIUM]
adminReplayReset (:9196-9204) clears the four fold maps and zeroes _replayCursor without
reading _replayInFlight. adminReplayStep sets the flag at :9217 before its first await
(:9241) and clears it in finally (:9288) — the W4-09 fix, which serialises step against step.
But the step re-derives its cursor from the page it folds (_replayCursor := e.seq + 1, :9274),
so a reset that lands while the second page is in flight empties the maps after page one was
folded; the continuation folds page two into the empty maps and finishes with done = true. Every
account with non-zero net activity in the first page then mismatches in adminReplayReport
(:9306-9318) — "a FALSE reserve alarm on a healthy venue — the worst failure mode for an
integrity check", in W4-09's own words (:9177-9182).
Input. A tape of more than 200 events; adminReplayStep(20_000); adminReplayReset from a
second shell during the step's second await; adminReplayReport. Not reproduced, and here is
why rather than an implication: on one PocketIC subnet the archive page round-trip completes inside
the round, and an ingress inducted behind the step runs after its first yield — before any fold —
so the second window is not reachable by induction in the deployment's own shape. The defect is the
missing key on the second writer, established by reading :9196-9290 whole.
Bounds. Both endpoints are controller-only; the state is transient; a clean
reset→step→report afterwards is correct; nothing in the suite drives reset-vs-step
(tests/test_w4_batch2.sh §1 fires two steps). Same actor, harm and instrument as W4-09, which
you rated MEDIUM/MEDIUM; LOW is defensible because it needs two different endpoints rather than a
double-click.
Remedy — shape and clause. Shape: if (_replayInFlight) { … } at the top of
adminReplayReset. Clause: the refusal must be loud — the method is : async () with no
error channel, and the file's own rule at :9518-9527 forbids a bare return on exactly that
signature — so trap, or widen the signature to { #ok; #err : Text }. Equivalent alternative: an
epoch counter bumped by the reset and checked after every await, the _captureEpoch idiom at
:8961/:9060, which also covers a reset landing between two pages.
Nearest: #26.5 / W4-09 (ours, fixed) — its closing sentence, "two concurrent
adminReplayStep calls cannot both fold the same page", is true and does not cover the other
writer. Our #44-era notes on what the replay means are a different claim.
#54.3 — tests/test_vault_lp.sh §5, the only pin on GHSA-3j44 / #48.3, cannot fail at 1fff1d7: the reset it calls now zeroes the arrears it was built to test against — an instrument finding; class it can no longer see [play: MEDIUM / prod: HIGH], live defect: none
§5 banks arrears (tests/test_vault_lp.sh:150-158), reads W5 = pyf at :159-162, then calls
resetExchange at :169 to "rebuild a CLEAN vault on top of the preserved arrears"
(:165-166: "resetExchange wipes the LP/pool/margin state but NOT insuranceOwedUsd") — every
bare line in this paragraph is in that test file. At 1fff1d7 that is no longer true:
performWorldWipe zeroes insuranceOwedUsd at :16887 — a line this drop added (#49.4, ours,
and correct: the comment at :16882-16886 says why), in the unconditional part of the wipe. Bob's
round trip at :191-193 therefore runs at W = 0, where the pre-fix and post-fix withdrawLp
are identical to the unit, and the assertion at :199-201 passes on the unfixed arithmetic too. The
green line prints the pre-reset $5.1k arrears that is not in force; the hygiene check at :210
(pyf == 0) is true for the same reason.
Numbers, from the code's arithmetic (not run — the suite is a replica test): fixture as
written, W = $5,100: the pre-fix formula nets +$557.65 (your own comment at :187-188 says
"≈ +$558" — the replica of the old formula agrees to $0.35) and the fixed one −$163.50 — the pin
discriminates; fixture as it actually runs, W = 0: both net −$160.02.
Remedy — shape and clause. Shape: bank the arrears again after the last resetExchange at
:169, then drain cash, then assert pyf > 0 immediately before bob's deposit. Clause: the
W > 0 read must be taken after the last reset and before withdrawLp, and the green
message must print that read — a control that restates its subject from an earlier scope is not
a control. We concede first that #49.4 is right; two of our own fixes shipped in one squashed drop
and one emptied the other's fixture.
#54.4 — SeasonRecord.archives is snapshotted from the chain-wide enumeration, so from season 2 onward it carries prior seasons' segments; under #54.1 the same record's chainHead is a third stale field [play: LOW / prod: N/A]
Two helpers sit adjacent and differ by one loop: allArchivePrincipals (:7697-7708, "Every
archive we're responsible for: prior seasons' sealed chains, the current sealed chain, the active
tip, and the pre-spawned successor") and currentArchivePrincipals (:7709-7716, "THIS season's
chain only (the SeasonRecord's provenance snapshot)"). currentArchivePrincipals has zero call
sites; the consumer its comment names calls the other one at :17117. _seasonArchives is
populated by performWorldWipe(true) at :16956-16963, after rec is built, so at :17117
it holds every prior season and none of this one: season 1 is correct, season N carries seasons
1…N. The operator log at :17123-17124 inherits the count.
allArchivePrincipals has five call sites — :7744, :8206, :9140, :16978, :17117 — and
only the last wants this season's set. Nothing internal reads the field (our own #25.5; your
reply there: "the record exists and is unreachable — which is arguably worse than not recording
it, since it looks like provenance is retained"). We are adding the half that could not be known
then: the provenance that looks retained is also wrong, and — under #54.1 — its chainHead can be a
head no archive certifies. @andreij6's Finding 25 is the mirror image on the same enumeration
(too narrow, where this is too wide); credit to him for establishing that its scope is load-bearing.
Remedy — shape and clause. Shape: allArchivePrincipals() → currentArchivePrincipals() at
:17117. Clause: the swap is correct only because _seasonArchives is populated after rec is
built; if the wipe ever moves above the record construction, the same defect returns wearing the
other helper's name. The property to pin is the ordering: a test that closes two seasons and asserts
records[1].archives ∩ records[0].archives == ∅.
#54.5 — archiveExecute reports a budget-truncated walk as complete: degraded empty is documented as "the read was complete", and the hop budget from the GHSA-5rcg fix breaks the walk with degraded = [] [play: LOW / prod: LOW]
:17462-17463: "degraded empty ⇒ the read was complete; non-empty ⇒ rows are real but may be
missing that segment's range, and the UI should say so". :17574: if (hops >= ARCHIVE_MAX_HOPS) { break chain } — segments past the sixth are never visited and take no marker; the row caps at
:17533 and :17552 break the same way. explorer.js:741-746 shows "history may be incomplete"
only on degraded; hasMore means the OQL window, not "more history exists". A caller whose
history spans more than six segments gets a silently truncated result rendered as complete.
Two of our own fixes interacting — the hop budget (GHSA-5rcg) and the degraded marker (W1-03,
#26.26, which asked for "a degraded or complete : Bool field") — so we concede that first.
Read-only; deep history stays reachable per archive. Shape: mark budget-skipped segments in
degraded (or add complete : Bool). Clause: the marker must fire on the budget exits, not
only on the catch at :17563, or the promise at :17462 stays false for exactly the state the
budget creates.
#54.6 — withdrawLp completes a burn that pays an all-zero basket: LP destroyed, #ok, a permanent #lpWithdraw row — the exit-side mirror of the zero-mint guard the deposit side carries [play: LOW / prod: LOW–MEDIUM]
withdrawLp never checks that the basket it is about to pay is non-zero. When every netLeg
(:16448-16451) floors to 0 it still burns lpAmount (:16476-16479), rescales the cost basis
(:16483), emits #lpWithdraw { lpBurned = lpAmount; basket = all 0 } (:16509-16513) and returns
#ok. Three inputs reach it: (a) dust — held × lpAmount < vaultLPSupply on every leg (one LP
unit against a 240k-LP vault); (b) holdingsUsd == 0 with tokens held (:16449, new in this drop)
— practically unreachable now that createAmmPool refuses to overwrite a live pool
(:14017-14027) and setAmmRefPrice rejects 0; (c) arrearsNum == 0, i.e. insuranceOwedUsd ≥ floor(0.996 · holdingsUsd) (:16444-16445, new): with H = $1,000 held, W = $2,000, L = $100,000 lent, an exiter holding 10% burns 10,000 LP worth f·NAV = $9,900 for $0 and #ok,
while the Earn card values the position at f·NAV (:16634).
performLpDeposit refuses the mirror image — "this would mint 0 LP shares, so you would receive
nothing for it" (:14195-14204, W3-10) — and our GHSA-458x closed the (0,0) deposit that
"moved nothing, minted nothing, returned #ok(0) — and still appended a permanent chained row".
The clamp's value is documented as deliberate (:16442-16443) and we concede that; the
subject is that the burn completes. Bounds: (a) needs an LP position (≥ $10, ≥ $1k if first);
(c) needs arrears ≥ 99.6% of physical holdings, the cascade state the fund exists for. Forfeited
value goes to the stayers, not to an attacker — user-harm and tape integrity, not theft.
Shape: after the basket is computed (:16452-16458) and before subVaultLp (:16476),
refuse when all five legs are 0, with the deposit side's own wording. Clause: the refusal fires
only on an exactly zero basket, never on a small one — it traps no value (a zero payout pays
nothing; the user keeps shares that still carry NAV), so the exit-set doctrine is preserved; a
minimum notional on exits is the rejected alternative.
#54.7 — adminUpgradeArchives discards the upgrade error and labels every failure "skipped (blackholed)"; its archive0 == null gate refuses in exactly the states where sealed or season segments exist without an active one [play: LOW / prod: LOW]
:9144-9157: try { … await (system Archive.Archive)(#upgrade full)(…) … } catch (_) { skipped += 1 } — Error.message(e) is dropped; :9161 returns "… skipped (blackholed)" for any cause. With
_blackholeAtSeal defaulting to false (:8314), every non-zero skipped on a dev/play
deployment is a misreport by construction, and the docstring at :9130-9133 itself names a
second cause ("incompatibility") the label cannot express. The runbook step that reads this string
(docs/deploy-to-subnet.md:121-126) then proceeds on a lever that said it succeeded; a skipped
archive0 surfaces later as three ship failures and an L1 roll.
:9137: if (archive0 == null) { return #err("no archive sidecar spawned yet") } gates a loop over
allArchivePrincipals(), which includes season and sealed segments — so right after a season
reset (:16966 nulls archive0) or an L1/L2 seal with spawn blocked (:8710, :8920-8926) the
whole-chain upgrade refuses with a message naming the wrong cause, while N segments are upgradeable.
W1-03 cites this try/catch as the correct per-hop isolation pattern, which is not disputed;
W4-16(b) and #47.3 replaced the bare catch (_) {} swallows on the other archive lifecycle paths
with a logged principal — this is the one they did not reach. Shape: log Error.message(e)
with the principal (copy :9065); gate on allArchivePrincipals() != []. Clause: the label
must stop naming one cause — count blackholed skips separately, or "skipped (see log)" —
otherwise tests/test_archive_chain.sh:175-177, which asserts only a skip count, keeps
certifying a failed upgrade as a design skip.
#54.8 — Every consumer of the debt-aware liquidation price still infers its direction from the size sign, which #15.4 made wrong: a net-long, short-like pool is shown "at liquidation" while healthy and "9.9% away" while liquidatable [play: LOW / prod: LOW]
MarginPools.liqPrice (:145-147) picks the direction from sign(heldBase·ltv − M·debtBase),
"not the sign of the net size: ltv < M, so a pool can be net long in base yet still liquidate on a
RISING mark" — the #15.4 fix (@andreij6, with our terms 2/3 on #15), correct as written.
Its consumers did not move with it: distBps (main.mo:4975-4976) computes signed = if (net > 0) { mark − lp } else { lp − mark }; PositionView (:11083-11095) carries no direction;
pctToLiq is unsigned; main.js:4767 renders the backend's ?0 — "liquidatable at EVERY price"
(MarginPools.mo:154) — as —, the glyph the same file defines as "no practical liquidation".
Input, your own slope fixture (tests/MarginPools.test.mo:181-190): 10 SOL held, 9 SOL owed,
otherColl = $100, ltv 0.85, M 1.15; net LONG 1 SOL; A < 0 → short-like, P = $54.054
(reached as the mark rises). At mark $50 the pool is healthy (health 1.1667, true distance +811
bps) and distBps = 0 → the Markets bar reads "Long Liq −0.0%" — proximity overstated by eight
bands against :5021-5022's "at most one band, never understate it". At $60 the pool is
liquidatable (health 1.1296 < 1.15) and distBps = 990 → "−9.9%" — understated. The position
row shows a "Long" pill with pctToLiq 8.1% and 9.9% respectively, and no surface says "past".
Display only: the liquidation decision is getHealth.isLiquidatable and is unaffected; the heat-map
data (:4692-4700) is signed and correct, only its interpretive text (:4710-4714,
docs/margin-heatmap-design.md:89) is stale. Uncapped class (a display that misleads), small
magnitude: Low. Shape: return the direction with the price (#down iff A > 0) and use it in
distBps and PositionView; render "liquidatable" instead of a distance when past. Clause:
the direction must come from sign(heldLtv − maintDebtBase) — the same A the price derives from
— never from poolNetSize; and "past" must be isLiquidatable, not mark vs liqPrice, because
against getHealth's rounding order the displayed price can sit on the wrong side of the true
crossing by dust (17 of 492 non-dust grid points, worst 6,483 ticks = $0.000065 on $2,665).
#54.9 — The frontend falls back to a fabricated exchange on any initialization exception, and nothing on screen says so [play: MEDIUM / prod: MEDIUM]
main.js:1140-1141 catches every throw in the init path — a block that includes showUI(), the
auto-refresh, resolveDeployModeAndBanner(), the AuthClient construction and the service-worker
registration — and answers with setupDemoMode() (:1800), which replaces the actor with
createMockActor() (:1806). The only signal is a console.warn. index.html contains no occurrence of
the string demo; there is no banner, badge or state flag rendered in the DOM.
The mock serves a populated exchange: balances, orders, deposits, a sign-in that succeeds for
demo-principal, and a getCanisterInfo record (:1915-1929) reporting burnPerDay: 4.2T,
cycles: 500T, fuelRouteWired: true — against the ~186 T/day and false the live subnet
canister reports. A user whose SDK load fails for any reason — a blocked IndexedDB, an extension
interfering with AuthClient, a transient CDN error — sees a working venue with fabricated numbers
and no way to tell.
What we established, and its limit. The literals demo-principal, Brave-Hawk-17 and
4200000000000 survive minification in a bundle built from this tree (positive control:
getCanisterInfo → 5 in the same bundle). We built that bundle locally; we did not fetch anything
from multidex.ai and make no claim about what the deployment serves.
Bounded, stated so the rating is not overclaimed. Nothing moves: every value is client-side and
no real call is placed. The precondition is an exception in init, which a healthy load never
raises. That is a narrow precondition and a bounded blast radius, hence MEDIUM — but it is not
compressed by #play, because what it misleads is a user, not the protocol.
@andreij6 reached the adjacent observation first, in his pending-yield finding: the mock emits
human Numbers where the canister emits base units, so a defect can be invisible in demo mode.
Ours is the reverse face of his — that the mode itself is invisible to the person looking at it.
Remedy — shape and clause. Shape: render a persistent, unmistakable marker whenever
appState.actor is the mock, and disable the sign-in affordance in that state. Clause: the marker
must be driven by the actor identity, not by the catch site — a second fallback path added later
must light it without being told to. The rejected alternative is a toast at the catch, which
disappears and which a later fallback would not share.
Informational — comments the drop left behind, one line each
MatchingEngine.mo:212-214still says "if a protected maker is fully locked by pending matches,
matching stops rather than skipping to a worse-priced maker"; the body at
MatchingEngine.mo:334-347(and:803-806) skips, and says in its own comment that the stop
"doesn't apply". Same class as our
#45.2, whose two comments this drop fixed; this header was not among them.main.mo:9373-9374: "orders are currently never deleted, so this counts every order ever
placed" — the heartbeat reaper (:7554 → :8058 → :8099) has deleted closed orders since before
60f75f6;ordersRetainedcounts retained orders.BorrowEngine.mo:4-5says interest is "rounded UP (owed by the user)";:51-66rounds DOWN
with carry-forward, deliberately. Pre-existing; the header is the pre-change text.- Latent, unreachable on today's markets: the affordability shrink (
MatchingEngine.mo:384-403)
followed bytradeCost = Fixed.mul(fillQty, tradePrice, false)(:414) settles slices for zero
quote whenever the spend cap is below one unit's cost and the price is below 1.0 quote —
256 zero-cost trades per call against the iteration cap. All four markets (:6979-6984) quote
above 1.0 ICPUSD today; it becomes live if a base ever trades below $1. Two sites
(MatchingEngine.mo:414and:845); the guard isif (tradeCost == 0) { break matchLoop }
after the shrink and before settlement —break, notcontinue.
What we verified and found correct, so it is on the record
- The 1.60 arithmetic in the four modified modules holds. The
maxQuoteSpendbudget never
overspends or under-fills (the shrinkfloor(cap·10000/(10000+eff))/pricekeeps cost + fee
within the cap, and settlement re-checks it at:341/:349);quoteSpentequals the sum of
committed slices to the unit; conservation including the treasury leg; FOK all-or-nothing with the
simulation's ceiling above the live floor; price/time/id priority; the#42fix; the W4-22 fee
role and id join; the level-cap boundary;liqPrice's algebra re-derived independently; the
archive'sappendBatchreplay/gap/chain-break rules and the W2-01 outbound anchor measured on
PocketIC with two deliberate breakages (a removedprevHashcheck is caught byverifyChain
reportingok=false, brokenAt; a removed anchor dropslinksCheckedfrom N to N−1 exactly as
the docstring predicts). AndEventChain.moandledger.jsagree: the certified head equals the
hash the browser-side mirror computes. - Your
#48.3fix (GHSA-3j44) is right: across 144 combinations of arrears, loans and deposit
sizes, no input makespayout > f·NAVor a deposit→withdraw round trip positive; the pre-fix
formula reproduces your own "+$558" to $0.35. That is also why #54.3 matters — the pin that would
say so cannot. - Our round-16 follow-up on
#52landed on all three sites:levelCapCheckat
:11281/:11478/:11593, an owner-keyedopenOrderCapCheckthat rejects rather than evicts at
:11285/:11482/:11597, and the min-notional at:11290-11293. The in-code attribution names
GHSA-97xp(eight sites inmain.mo—:1811,:6160,:10702,:10988,:11262,:11473,:11582,:13378— andtests/LiquidityManager.test.mo:67),
the shipped test names both (tests/test_order_caps_margin.sh:2: "GHSA-97xp / #52.1 / #51"),
and your reply on#52credits "#52.1 + follow-up". Our follow-up is dated 2026-08-18; the
earliest dated reference toGHSA-97xpin the tree is 2026-08-20 (:10702). We cannot read
GHSA-97xp, so we do not claim precedence: if it predates 2026-08-18 the credit is its
reporter's and ours is independent confirmation. We would only ask that the code comments cite
#52.1beside it, as the test already does. - The zero-supply LP mint (
GHSA-h888-fcww-xq25) is reproduced on this drop's unmodified
build, on PocketIC, with the numbers predicted in advance — in its advisory thread, not here.
Rejects from this round, with the line that closes each
- An LP deposit landing between the wipe's
awaits (:16986-16987) —Map.clear(pools)at
:16671precedes them andperformLpDeposit :14084refuses; nothing after the loop writes state. adminResetPlayAllowances(:17136-17150) clearingplayReservedUnitswithout the Bridge — a
controller repair tool with its purpose stated at:17130-17135; the#25.2double debit does
not reproduce through it.- The concentration fee and cap denominated on NAV rather than holdings (
:15282,:15298) — a
design question adjacent to W4-11 (#16.3), not decided in the mirror; held, not filed. verifyChain(limit = 0)returningok = true— vacuous by design andlinksChecked = 0says so
(ArchiveCanister.mo:290-301).- The
#fullfirst-store cosmetic (ArchiveCanister.mo:248), the decode-hole paging note, the
exactlywording intests/MarginPools.test.mo:66-81(fixture-dependent, not a code defect) —
noted, not filed. docs/spec-app-connect.md, cited on line 2 of the App Connect bridge page, still does not exist
in the tree — already ours (#46), not re-filed.
Coverage — what this round did not read
main.mo outside the three bands and the call sites named above; the OQL executor
(src/backend/oql/*); OrderBook.mo and Accounts.mo beyond their signatures; the 14 margin
shell suites and tests/test_archive_chain*.sh; docs/margin-pools-design.md and
docs/archive-design.md. The Bridge's live subnet placement is not known to us, and #54.1's
window length on mainnet follows from it. Everything not listed as reproduced above is a source
read, and we say so rather than imply a run.
— Ravenith, OhShii Labs
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with main.mo at resetSeason (:17040-17122) and the replay endpoints (:9196-9290), then inspect bridge/main.mo:443-455 and tests/test_vault_lp.sh §5. Reproduce the two-message race or review the stated test-gap fixture, and use the listed invariants and assertions to verify that the relevant seal, replay, or regression behavior is preserved after remediation.
Written by the indexing model from the issue text.
Assessment
- Domain
- backend-api-design, security, testing-qa
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100