Skip to content

OhShii Labs review, round 2 — 2/6 · 5 findings (#24.1–#24.5): the arbitrageur canister trades against the venue with no shared invariants #24

Description

@rvnt9999

Round 2, part 2 of 6 (part 1: the self-funding loop). src/arb/main.mo is 262 lines and holds
10 of the 66 await sites in the first-party tree — the densest multi-await financial state
machine in the repository. It is also the only trading participant that lives in a different
canister from the book it trades on, and every constant it uses to size an order is its own.

Read in full against src/backend/main.mo's extMarketSwap (:4919-4981) and the AMM path.

#play severities are real here: the arb holds venue inventory and its failure modes are
availability and price-quality, neither of which play money caps. Nothing below requires a
malicious controller; tickOnce is controller-gated (arb:173, checked before the first await
at :176 — we verified that and it is correct).


1. The arb's per-tick clip is 2.05× the DEX's per-call cap, so its own inventory becomes unflattenable

Where: src/arb/main.mo:53, :203-205, :215, :222-224; src/backend/main.mo:4826, :4937-4938

TRADE_CAP_USD = 1_025_000_000_000 = $10,250 at Fixed.SCALE = 1e8 (arb:53).
ARB_MAX_SWAP_USD = 500_000_000_000 = $5,000 (main.mo:4826). The two live in different
canisters with no shared constant, no assertion and no fallback.

t1  arb:203/:215  capBase = mulDiv(TRADE_CAP_USD, SCALE, mark, false)   — $10,250 of notional
t2  arb:204       q = Nat.min(availBase, capBase)
                  whenever availBase·mark > $5,000 this yields a leg worth more than $5,000
t3  arb:205       await dex.extMarketSwap(token, #exportBase, q)
                  → main.mo:4937 grossUsd = Fixed.mul(q, refPrice, true) > 500_000_000_000
                  → main.mo:4938 return #err("Exceeds per-call cap")
t4  arb:207       note(...) and continue. availBase is unchanged, so t2–t4 repeat every
                  TICK_NS = 5 s (arb:46).

The flatten step is the arb's only way to close a position, and it is the step that is
refused. No adversary is needed to get there: the cheap branch buys through the ordinary
limit-order path (arb:244 placeLimitOrderExp), which is not subject to ARB_MAX_SWAP_USD
it is capped only at arb:242, i.e. up to the full $10,250. One fully-filled cheap-side buy
leaves the arb holding more inventory than its own exit accepts.

To be precise about the word "permanent": the stuck predicate is evaluated against the current
mark, so a sufficient mark decline drops the notional under $5,000 and frees it. There is no
mechanism that makes that happen; it is weather.

Fix direction. Make the DEX the single source of truth: getArbStats() already exists
(main.mo:5010) — publish perCallCapUsd through it and clamp every leg to
Nat.min(capBase, mulDiv(perCallCapUsd, SCALE, mark, false)). Failing that, hard-set
TRADE_CAP_USD ≤ ARB_MAX_SWAP_USD. Separately, add a slice loop so a position larger than the
per-call cap is exported in cap-sized chunks across ticks rather than not at all.


2. The rich branch commits the import before the hedge exists, and the justifying liquidity can be withdrawn inside the await

Where: src/arb/main.mo:212 (depth read), :217, :222, :224 (import commits), :226 (hedge), :228 (no compensation)

t0  Attacker rests a bid at mark×1.0051, sized just under $5,000 so the import is accepted.
    That clears richFloor = bpsUp(mark, BAND_BPS=50) at arb:213/:217.
t1  arb:212  the arb reads the book; q = Nat.min(depth.bids[0].quantity, capBase) at :222.
t2  arb:224  the arb yields on extMarketSwap(#importBase, q). This COMMITS on the DEX:
             main.mo:4948 subtracts cost, :4949 credits base, :4957-4958 write two permanent
             rows, :4959 charges _arbHourUsd.
t3  the attacker cancels the bid — a separate ingress message. Timing is free: arb:150
    publishes `lastTickNs` on an unauthenticated query, so the next tick is at +5 s.
t4  arb:226  the hedge is placed into a book with no rich bid. It rests, expires at
    ORDER_TTL_SEC, and the base is unwound at a loss on a later tick's flatten leg.

_inFlight (arb:86) serialises the arb against itself and says nothing about the book, which
every other principal mutates freely. The #err arm at :228 calls note(...) and falls
through — there is no compensating action.

The precondition is that the attacker's bid rests rather than crosses, i.e. the AMM's best
ask is above mark×1.005 — which holds when volRegime is elevated, exactly when the arbitrageur
matters. placeLimitOrderPO (main.mo:8665) makes attempts free: a post-only bid is killed at
release if it would cross, so a failed attempt costs nothing.

We are filing this as a design flaw, not as an await race. The interleaving window at :224
is real, but the defect would exist even if the two calls were atomic: an unconditional external
haircut is paid before a venue hedge that may be unfillable, with no unwind.

Fix direction. Invert the legs — place the venue sell first as post-only/IOC and import only
the quantity that actually filled. If the import must lead, re-read the depth immediately after
:224 and export straight back in the same tick rather than resting an 8-second order.


3. extMarketSwap has no price bound, and the arb prices its hedge off a mark up to three round-trips stale

Where: src/arb/main.mo:117 (the Candid signature), :189, :192, :201/:205/:212, :224, :226, :239; src/backend/main.mo:4919, :4937, :4946

The interface is extMarketSwap : (Text, {#importBase; #exportBase}, Nat) -> async {#ok : Nat; #err : Text}.
There is no maxCost, minProceeds or expectedMark parameter, so the arb cannot express an
acceptable price at all. The DEX's own marketable collar is 5% — 25× wider than the arb's
EDGE_BPS = 20 edge.

mark is captured once at arb:192 from the pools read at :189, then the arb yields three more
times (:201, :205, :212) before using it. The DEX's own price pipeline
(main.mo:13412 tickPriceRefresh, :13323 fetchAndSetRefPrice) can write a new refPrice in
that window. The import at :224 is then priced by the DEX at its current mark
(main.mo:4946), while the hedge at :226 is priced at bpsUp(mark, EDGE_BPS) off the stale
one.

Aggravating, and worth fixing on its own: now is captured once at arb:187, before the
first await, and reused for every market's freshness test at :193-194. By the last market in
the loop that check is being evaluated against a now several seconds old, so it
systematically under-states the mark's age — the staleness guard is loosest exactly where the
data is oldest.

Fix direction. Add a price bound to the external-market interface and have the DEX refuse
when its current mark would breach it. This is the standard slippage guard, and under an
unbounded await (Motoko has no caller-side timeout) it is the only thing that makes the
cross-canister leg safe. On the arb side, re-read the pool immediately before pricing at :226/
:239, and move the now capture inside the loop.


4. The arb prices both legs of one arbitrage off two different marks

Where: src/arb/main.mo:187, :189, :192-194

The narrower, adversary-free form of item 3: the DEX charges the import live and releases the
limit order against a newer mark, while both were decided against the opening mark. The arb can
therefore take the AMM's side of its own staleness. No interleaving and no attacker required —
only the loop's own duration.


5. ARB_HOURLY_CAP_USD is charged gross on both legs against a tumbling window, so an hour's budget dies in about 65 seconds

Where: src/backend/main.mo:4830, :4836-4837, :4939-4940, :4959, :4974; src/arb/main.mo:46, :205, :224

_arbHourUsd is charged the gross notional on the import (:4959) and on the export
(:4974), so one round trip of the same dollar costs the budget twice. The window reset at
:4939 is now − _arbHourStartNs > 3_600_000_000_000tumbling, not rolling.

Four base markets × 2 legs × $5,000 = $40,000 chargeable per tick, at TICK_NS = 5 s = 720 ticks
per hour, against a cap of $512,500 (:4830). The cap trips after roughly 13 ticks ≈ 65
seconds
, and the arb is then dark for the remaining ~59 minutes — including its flatten leg,
so inventory opened before the trip is stranded and unhedged for the rest of the window. The trip
returns before any logEventF, so the event log records nothing.

Item 2 above gives an attacker direct control of q (they set depth.bids[0].quantity), so the
gross-both-ways accounting hands them 2× leverage on the budget for free. But no adversary is
required — a genuinely volatile hour across four markets reaches the same state.

The prior round approved this cap's stability properties and we still think that reasoning was
right; what we missed was that the same cap doubles as a one-minute kill switch on the
price-pinning mechanism.

Fix direction. Charge the cap on net external exposure rather than gross turnover — an
import followed by an export of the same base is exposure-neutral — or exempt the #exportBase
direction entirely, since closing a position can only reduce risk. Convert the window to a
genuinely rolling one (a small ring of per-minute buckets). Size TICK_NS, TRADE_CAP_USD and
ARB_HOURLY_CAP_USD against each other in one place with an assertion, and log the trip.


Delineation. None of the above overlaps #22 (Menese, matcher work multipliers): we verified
in source that extMarketSwap (main.mo:4919) is a synthetic AMM-priced swap that never enters
MatchingEngine.matchLoop. Nor #13 (andreij6, executeSwapCross) or #16 (LP deposit path).

— 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