Skip to content

OhShii Labs review, round 2 — 4/6 · 5 findings (#26.1–#26.5): the archive chain — one bad segment fails every read, and the segments nobody can fix are the ones nobody observes #26

Description

@rvnt9999

Round 2, part 4 of 6. The archive sidecar chain is the venue's permanent ledger and the backing
for the History surface, the OQL deep-history path and the proof-of-reserves replay. Five items,
all in the read/observability/lifecycle half rather than the write half.

Delineation, stated up front because three issues are adjacent here.


1. The season detach drops sealed archives out of every funding path, so the "permanent" ledger drains until the IC deletes it

Where: main.mo:14161, :14167-14169 (the detach), :6274-6280 (allArchivePrincipals), :6297-6344 (tickArchiveFuel), :6282-6290 (fundArchive), :6348-6353 (adminFundArchive)

allArchivePrincipals() is built from exactly three sources: _archivesSealed (:6276),
archive0 (:6277), _archiveNext (:6278). performWorldWipe(true) empties all three in one
message.

tickArchiveFuel is the only automatic funder and iterates exactly that list at :6298;
both of its fundArchive calls (:6325 watermark, :6339 blind-fund) are inside the loop.
fundArchive has exactly three callers — :6325, :6339, and adminFundArchive (:6348),
which funds archive0 only and returns #err("no archive sidecar spawned yet") when it is null.
Repo-wide, deposit_cycles in first-party source appears at :6284 (archives), :6439/:6453
(Bridge) and :6490 (arb) — the last three take effectiveBridge/effectiveArb, never a
caller-supplied principal. There is no endpoint that deposits cycles into an arbitrary
canister id
, and List.add(_archivesSealed, …) occurs only at :7092 and :7402, both inside
the ship path — there is no re-attach.

The prior season's archives therefore hold their data, are unreachable from the funding loop, and
drain until the IC deletes them. #18 Finding 25's correction is what makes this reachable: the
forSeason branch detaches without deleting, so the canisters genuinely survive the reset —
they are simply no longer anyone's responsibility.

Fix direction. Keep a separate append-only registry of sealed principals that survives the
detach and that tickArchiveFuel iterates, or add an adminFundCanister(principal, amount)
gated to controllers so a detached segment can be rescued at all.


2. One unreachable segment fails every History and OQL read, for every caller

Where: main.mo:14575 (chain loop), :14581 and :14595 (the two federated awaits), :14550 (no auth check); contrast :7459-7472

Both federated awaits sit bare inside label chain for (cid in List.values(visit)). There is no
try/catch, no partial result and no skip for a segment already observed frozen. One rejected
hop fails the entire composite query.

The same file already does this correctly: adminUpgradeArchives wraps each per-archive hop
in try { … } catch (_) { … } at :7459-7472. This is a sibling fix applied to one of two
places.

Triggers, none of which requires a fault injected by an attacker:

  • adminUpgradeArchives (:7450) walks allArchivePrincipals() upgrading each segment in
    place; any archiveExecute landing inside a segment's upgrade window hits a reject;
  • a segment frozen for cycles (item 1 above, or Menese DeFi Team evaluation, Part 1/2: bounding work and state under load #2 item 3's memory wall);
  • a blackholed segment whose status can no longer be read (item 3 below).

archiveExecute (:14550) has no auth check at all, so an anonymous caller reaches the loop
— which means the blast radius is every user of the History page, not only the caller who
triggered it.

Fix direction. Wrap each hop and return what was gathered:
catch (_) { List.add(degraded, cid); continue chain }, plus a degraded : [Principal] or
complete : Bool field on the result so the UI can say "segment X unavailable" instead of
failing the page. Mirror :7459-7472. Additionally skip any cid whose _archiveObs entry
reports frozen = true.


3. Blackholed segments are never re-observed, so the chain table shows a stale ok forever

Where: main.mo:6297-6344, :6300, :6306-6309, :6326-6341, :6377-6390, :7413-7426, :6745; ArchiveCanister.mo:346-370

_archiveObs is written only inside the try, after ic00.canister_status (:6300)
succeeds — and canister_status requires controllership. Seal-time blackholing
(update_settings { controllers = ?[] } at :7413-7426, gated by _blackholeAtSeal, default
false at :6745) permanently removes that controllership. From then on the status call
rejects, control falls to the catch at :6326, and _archiveObs for that principal is never
refreshed again. getArchiveChain's mk (:6378-6390) serves whatever is in _archiveObs, and
the frontend renders it.

So the segments that are immutable by design and therefore unfixable are exactly the segments
whose health is never observed again, and the dashboard reports the last-known ok indefinitely.

The fix already exists 100 lines away and is not applied. tickBridgeFuel (:6422-6445)
reads the callee's own cyclesBalance() query at :6425-6426 precisely because
canister_status would need controllership — the comment at :6411-6413 says so.
ArchiveCanister.stats() (ArchiveCanister.mo:346-370) is a public query returning cycles,
and the frontend already calls it directly. The affordance exists, is already consumed elsewhere,
and is the one thing that restores visibility on the segments that cannot be status-read.

Fix direction. Fall back to stats()/cyclesBalance() when canister_status rejects, and
mark the observation as degraded rather than leaving the previous one in place. Also log the
silent skip at :6337 — today a blackholed segment that fails the blind-fund headroom test is
skipped with no event at all.


4. An archive spawn is unrecorded across its own await, and the epoch-race cleanup swallows the principal

Where: main.mo:7281-7301, :7293-7297, :7373-7390, :7377-7380, :7134-7144

Two related lifecycle leaks at the same three spawn sites.

(a) Unrecorded spawn. archive0 := ?fresh / _archiveNext := ?fresh happen only in the
continuation of await Archive.Archive(...). The canister exists the moment the call returns;
the record of it exists only if the continuation runs. An upgrade landing in that window leaks a
3T-cycle canister that main controls but can no longer fund (allArchivePrincipals never learns
of it), list, or delete.

(b) Swallowed cleanup. When a reset races a spawn, the epoch guard destroys the orphan with
try { await ic00.stop_canister(...); await ic00.delete_canister(...) } catch (_) {} at
:7293-7297 and :7377-7380. A bare swallow: if either call fails, the principal — which exists
only in that continuation — is lost with no trace on-chain and no log line.

Note this is not #2 item 3: their finding is about the settings the child is created with
(memory limit, threshold, lowmemory()); ours is about the spawn not being recorded. Applying
memory settings at creation still leaks the canister; persisting the principal before the await
still ships a 3 GiB/threshold-0 archive. Both fixes are wanted.

Fix direction. Write the intended principal to a _pendingSpawn field before the await —
under enhanced orthogonal persistence a plain var is already stable, so this is a one-line
change — and have a heartbeat subtask reconcile it. Replace the bare catch (_) {} with a log
line carrying the principal.


5. adminReplayStep has no single-flight guard and advances its cursor only after the await

Where: main.mo:7517-7520, :7529, :7543, :7545-7578, :7576

adminReplayStep is the executable proof-of-reserves audit: it folds the archived tape forward
and compares the result against live balances. _replayCursor is read before the await at
:7543 and advanced only in the continuation at :7576, and there is no in-flight guard.

Two overlapping controller calls — a double-click on an ops page, or a retry after a slow reply —
both read the same cursor, both fold the same page, and the tape is double-counted. The output is
a false reserve alarm on a healthy venue, which is the worst possible failure mode for an
integrity check: it burns operator trust in the one mechanism meant to detect real divergence.

Fix direction. The pattern is already in the file: guard with a flag set before the first
await and cleared in a finally, exactly as tickShipEvents does at :7245-7246 and :7440
(which we verified is correct on every axis — it is the repository's only finally and it earns
it). Advance the cursor optimistically before the await and roll it back on the error arm, or
carry the expected cursor into the response so a stale fold is detectable.

— Ravenith, OhShii Labs

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions