OhShii Labs review, round 2 — 1/6 · 2 findings (#23.1–#23.2): the self-funding loop can re-enter after an ambiguous ledger reject

[rvnt9999]

Second round. Our first round is #4–#11; this one is a commit-point audit — we walked every
await in the first-party tree and asked, at each one, what state was read before the yield and
acted on after it. Method note and the full inventory are in 6/6.

Housekeeping, unchanged: the SECURITY.md advisory link (github.com/dfinity/multidex/security/advisories/new)
still 404s — dfinity/multidex does not resolve — so there is still no private channel and this
is public. Everything here is #play-inert or #play-visible; nothing below requires a
malicious controller.

Corrections to our first round, stated up front rather than left standing. Four of our
round-1 clearances have been corrected by others and we accept all four: #21 (andreij6) retracts
our #10 item 7 certification of scripts/lint-ratchet.sh — we reproduced his finding 65
independently, moc emits : type error [M0096], and the gate's grep -qE ': error' cannot match
it; #17 (andreij6) shows the remedy we proposed in #9 item 1 (PRICE_MIN_SOURCES 2→3) is
insufficient and the correct fix is a high-breakdown estimator; #15 item 2 corrects our #7
closing paragraph on settleNettedPair; #18 Finding 25 shows our #10 item 1 claim that
resetExchange "stops and deletes every archive canister" is too strong — the delete loop sits
inside else if (not IS_PRODUCTION). We are not re-arguing any of these.

---

1. An ambiguous ledger reject keeps the internal debit, arms no interlock, and the 10-minute auto-fuel loop re-enters

Where: src/backend/main.mo:10396 (debit), :10408 (yield), :10425-10436 (the catch),
:10391 and :10519 (the only interlock), :10514-10518 (cooldown), :10500 (tranche)

Severity: #play INFO — the fuel route is unwired on the live play deployment
(:10389, :10511; scripts/deploy.sh:787-788). On #production, where the route points at
the NNS ICP ledger and the CMC, this is the most serious thing we found in this round.

The invariant. However many times a stage-2 transfer ends ambiguously, the treasury must not
spend a second tranche until the first is reconciled.

What holds it. Nothing. _fuelPendingNotify is the only interlock in this saga, and the
catch path deliberately does not set it — correctly, because on an ambiguous failure the
transfer may or may not have landed, and arming the notify slot would assert a block index the
canister does not have. But nothing else is armed either: no journal, no attempt record, and
created_at_time is null on the transfer (:10414), so the ledger's own dedup window is not
engaged.

t1  main.mo:6178 → tickAutoFuel. Headroom is low, so the health check at :10517 does not
    return; the 10-minute cooldown is armed at :10518; `_fuelPendingNotify` is null at
    :10519, so :10523 calls fuelStage2. It debits up to 100 ICP (AUTO_FUEL_ICP_TRANCHE,
    :10500) at :10396 and yields at :10408.

t2  The call is rejected. No attacker is required, and there are two ordinary ways in:
    the NNS subnet is unavailable, or this canister cannot reserve the outgoing call while
    sitting just above its freezing limit — which is exactly the condition :10517 selects
    for. Control lands in the catch at :10425. The debit is KEPT, deliberately and with a
    written rationale at :10426-10432 (re-crediting a transfer that DID land would inflate
    the internal claim past its chain backing — the insolvent direction, and the reasoning
    is right). One log line is written at :10433. `_fuelPendingNotify` is not set. Return.

t3  600 s later the cooldown expires. Headroom is still low — nothing was minted. The
    interlock at :10519 is still null. fuelStage2 debits ANOTHER tranche and yields again.

    Result: the loop re-spends the treasury's internal ICP claim once per window until it is
    empty, and the canister holds no record of how many attempts were made or which ones
    reached the ledger.

What we are not claiming. Each window's transfer carries its own intent, and any ICP that
does reach the CMC subaccount for this canister is genuinely converted to cycles at the CMC's
rate. This is not a double-spend. The defect is the unbounded re-entry, and the absence of
any record that would let an operator reconcile afterwards.

Fix — and this one we compile-tested rather than proposed on faith.

The failure has to be classified instead of collapsed, and the pinned standard library already
ships the exact predicate. .mops/core@2.5.0/src/Error.mo:70-75:

/// Checks if the error is a clean reject.
/// A clean reject means that there must be no state changes on the callee side.
public func isCleanReject(self : Error) : Bool = switch (code(self)) {
  case (#system_fatal or #system_transient or #destination_invalid or #call_error _) true;
  case _ false
};

mops.toml:6 pins core = "2.5.0", and all three canisters already
import Error "mo:core/Error" (main.mo:37, bridge/main.mo:29, arb/main.mo:39).
Error.code is used nowhere in the first-party tree — 18 uses of Error.message, zero of
Error.code — so this capability is present and entirely unused.

} catch (e) {
  if (Error.isCleanReject(e)) {
    // Library guarantee: no state change on the callee side. The transfer provably did
    // NOT happen, so undoing the internal debit is safe.
    Accounts.addBalance(accounts, treasury, "ICP", icpE8s);
    return #err("ledger transfer not sent: " # Error.message(e));
  };
  // Genuinely ambiguous (#system_unknown, #canister_error, …): the transfer MAY have
  // landed. Keep the debit — and ARM AN INTERLOCK so the loop cannot re-enter.
  _fuelAmbiguous := ?{ icpE8s; atNs = Time.now(); msg = Error.message(e) };
  return #err("ledger transport, ambiguous — reconcile on-ledger: " # Error.message(e));
};

fuelStage2 then refuses while _fuelAmbiguous != null, exactly as it already refuses while
_fuelPendingNotify != null at :10391, with an operator lever to clear it after off-chain
reconciliation.

How we tested it. The snippet above was built into a minimal persistent actor and
type-checked with this repository's own toolchain and flags — moc 1.9.0 from
mops toolchain bin moc, sources from mops sources, flags
--default-persistent-actors --implicit-package=core --check — and passes with only the
cosmetic M0217 (redundant persistent under that flag). We validated the harness against a
known positive first: a deliberately ill-typed use of the same symbol fails with
type error [M0096], so the check can see a failure.

Note the direction of the guarantee, because it is easy to invert. isCleanReject == true ⟹
safe to re-credit. isCleanReject == false does not mean the transfer landed — it means you
may not assume it did not. Keeping the debit and blocking re-entry is the conservative branch.

---

2. The auto-fuel trigger extrapolates a 5-minute burn sample ×288 with no clamp and no absolute ICP budget

Where: src/backend/main.mo:10498-10500, :10516-10517

Severity: #play INFO (route unwired), #production MEDIUM.

The trigger fires when Cycles.balance() <= _freezingLimitCycles + max(AUTO_FUEL_HEADROOM_MIN, _burnPerDay),
and _burnPerDay is derived by extrapolating an observed short-window burn across a day. There
is no clamp on the extrapolation and no absolute ceiling on ICP spent per day — only
AUTO_FUEL_ICP_TRANCHE (100 ICP) per action and a 10-minute cooldown.

The consequence is a coupling that is not obvious from either side: any cycle-drain primitive
becomes a treasury-ICP drain. Our #5 item 5 (no global cross-caller rate cap, no cycle floor)
and our #5 item 2 (aiComplete prompt length is caller-controlled, so per-call cycle cost is
caller-controlled) both price their damage in cycles and availability. Through this trigger they
also price it in ICP: a sustained artificial burn raises _burnPerDay, which raises the floor,
which keeps the loop armed.

We are deliberately conservative on severity here. The ICP is converted at the CMC's XDR rate,
so this is a forced conversion on an attacker's schedule, not a loss of value — and on a
healthy venue the burn sample reflects real load. It matters because it is the mechanism that
turns a cycle-cost finding into a treasury finding, and because combined with item 1 above the
same trigger is what keeps re-entering.

Fix direction (not tested, unlike item 1): clamp the extrapolation to a configured multiple
of the trailing median rather than a single window, and add an absolute per-day ICP budget
alongside the per-action tranche, with the trip logged. _fuelCooldownUntil is already stable
and deliberately so (:10505-10508 explains why) — a daily budget belongs in the same place.

---

Round 2 continues in 2/6 (the arbitrageur canister), 3/6 (season reset), 4/6 (the archive
chain), 5/6 (load-shed exits, Bridge admission, oracle timing) and 6/6 (check integrity, latent
items, and the method note).

— Ravenith, OhShii Labs