Bridge `createDepositAddress` writes a permanent stable `onboarded` row for every free keypair, and no wipe, purge or upgrade path ever removes it

Đang mở
#89 0 bình luận 0 reaction 0 người được giao Xem trên GitHub

Chưa có ai nhận issue này.

Đánh giá

Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức phù hợp với người mới
30/100
Loại issue
Lỗi
Độ rõ ràng
Khá rõ ràng
Mức độ hoạt động
Sôi nổi
Công nghệ
javascript
Lĩnh vực
backend, frontend

Hướng nghiên cứu

Start with createDepositAddress in src/bridge/main.mo (around lines 331-335), where the onboarded map is written, and with its only reader getMyDepositAddresses (lines 337-340). The frontend consumer is in src/frontend/src/main.js around lines 1851-1857. The repro script in the issue can be run with moc to confirm the row count is unchanged after the wipe, purge and postupgrade paths. Done means deciding whether the write is removed or bounded, and the frontend and reclamation behaviour match that decision; the issue leaves that choice open.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

Severity (proposed): Low. A robustness defect that breaks no value, solvency or authorization guarantee. It is the same class the project graded LOW as F33 (H-36) and F40 (H-26) and fixed on the sibling maps. Identical on #play and #production: the method has no posture gate.

Category: Availability: unbounded, unreclaimable state growth that any non-anonymous principal can drive (SECURITY.md, "cheap ways to exhaust ... memory").

Summary

createDepositAddress on the Bridge checks only that the caller is not anonymous, then adds Principal.toText(caller) -> true to onboarded, a plain let map in a persistent actor and therefore implicitly stable. Nothing in the Bridge ever deletes from it: adminSeasonWipe clears only ledgers and admittedUnits, adminPurgeZeroRows scans only those two maps, and postupgrade clears only the claiming and admitting latches.

The flag carries no information the Bridge needs. Deposit addresses are a pure function of the principal, and the only reader is a query the frontend uses to show or hide the "Create deposit address" button. So every throwaway Ed25519 keypair can leave one permanent row (roughly 110-175 bytes) that survives every season reset and every upgrade. H-26 made the Bridge's other per-keypair maps free on refusal and purgeable, and H-36 capped the DEX's pre-registration store; createDepositAddress is the free-identity write site neither fix covered. It has been unchanged since the first public commit (0241cba).

Affected surface

Canister Bridge (src/bridge/main.mo, posture #play), commit 7bcdde6.

Mechanism

  // "Create deposit address" — really register/reveal (addresses are deterministic).
  public shared (msg) func createDepositAddress() : async [ChainAddress] {
    requireAuth(msg.caller);
    Map.add(onboarded, Text.compare, Principal.toText(msg.caller), true);
    addressesFor(msg.caller);
  };

  public query (msg) func getMyDepositAddresses() : async ?[ChainAddress] {
    if (Map.get(onboarded, Text.compare, Principal.toText(msg.caller)) == null) { return null };
    ?addressesFor(msg.caller);
  };

requireAuth (:188-190) traps only on the anonymous principal, and inspect admits any other caller. There is no requireNotProduction (that gate is on claim and devSimulateDeposit only), no wipePending fence, no DEX-registration or binding check, and no cap.

grep -n onboarded src/bridge/main.mo returns only :108 (declaration), :333 (write), :338 (read) and :749/:755 (stats). There is no Map.delete or Map.clear on it anywhere.

The sibling entry points were deliberately hardened so that a no-op call writes nothing. claim validates the asset first ("Without this, claim("") was a permanent write", :380-381) and then uses a read-only lookup instead of the get-or-create ledgerOf so a no-op claim "just costs no state" (:391-399). devSimulateDeposit prunes zero rows on refusal via pruneZeroRows (:214-223). createDepositAddress is the remaining free-identity write.

Map.add in core 2.5.0 is ignore swap(...) (.mops/[email protected]/src/Map.mo:422-424), so a repeat call from the same principal overwrites. Growth is exactly one row per distinct principal; fresh self-authenticating principals cost nothing to generate.

Reproduction

Traced on the live code path (not run against a replica or the live Bridge):

  1. Generate N Ed25519 keypairs offline. None needs a DEX registration, a binding or a deposit.
  2. From each keypair, send one ingress update bridge.createDepositAddress(). inspect (:795-798) and requireAuth pass; :333 commits a row keyed on the 63-character principal text.
  3. getBridgeStats().onboarded (a query, callable anonymously) grows by N.
  4. resetSeason -> adminSeasonWipe, a controller's adminPurgeZeroRows, and a routine --mode upgrade (postupgrade) each leave onboarded unchanged.

Executed in the interpreter. The script below copies the Bridge's state declarations and the bodies of createDepositAddress, the adminSeasonWipe clear block, adminPurgeZeroRows and postupgrade from src/bridge/main.mo, and drives 5,000 distinct principals plus one repeat. An independent variant of the same script with 20,000 keypairs gave the same result (onboarded=20000 after each reclamation path).

repro.mo (moc 1.9.0, commit 7bcdde6)
// Bridge `onboarded` grows one permanent row per distinct non-anonymous
// principal and no Bridge path ever removes it. State + bodies copied from
// src/bridge/main.mo: :106-119 (maps), :331-335 (createDepositAddress),
// :701-702 (adminSeasonWipe clear block), :728-743 (adminPurgeZeroRows),
// :790-793 (postupgrade).
import Map "mo:core/Map";
import Text "mo:core/Text";
import Principal "mo:core/Principal";
import Blob "mo:core/Blob";
import Nat8 "mo:core/Nat8";
import List "mo:core/List";
import Debug "mo:core/Debug";
import Runtime "mo:core/Runtime";

type Ledger = { var confirmed : Nat; var pending : Nat; var claimed : Nat };
let ledgers = Map.empty<Text, Ledger>();
let onboarded = Map.empty<Text, Bool>();
let claiming  = Map.empty<Text, Bool>();
let admittedUnits = Map.empty<Text, Nat>();
let admitting     = Map.empty<Text, Bool>();

func requireAuth(caller : Principal) {
  if (Principal.isAnonymous(caller)) { Runtime.trap("Authentication required") };
};
// :331-335 (addressesFor return dropped: it is pure and writes nothing)
func createDepositAddress(caller : Principal) {
  requireAuth(caller);
  Map.add(onboarded, Text.compare, Principal.toText(caller), true);
};
// :701-702
func adminSeasonWipe_clears() { Map.clear(ledgers); Map.clear(admittedUnits) };
// :728-743
func adminPurgeZeroRows() {
  let deadCounters = List.empty<Text>();
  for ((k, v) in Map.entries(admittedUnits)) {
    if (v == 0 and Map.get(admitting, Text.compare, k) == null) { List.add(deadCounters, k) };
  };
  let deadLedgers = List.empty<Text>();
  for ((k, l) in Map.entries(ledgers)) {
    if (l.confirmed == 0 and l.pending == 0 and l.claimed == 0
        and Map.get(admitting, Text.compare, k) == null
        and Map.get(claiming, Text.compare, k) == null) { List.add(deadLedgers, k) };
  };
  for (k in List.values(deadCounters)) { ignore Map.delete(admittedUnits, Text.compare, k) };
  for (k in List.values(deadLedgers))  { ignore Map.delete(ledgers, Text.compare, k) };
};
// :790-793
func postupgrade() { Map.clear(claiming); Map.clear(admitting) };

// Fresh Ed25519 self-authenticating principal shape: 28-byte hash + 0x02 tag.
func freshPrincipal(i : Nat) : Principal {
  let b = Blob.fromArray(Array_tabulate(29, func(j : Nat) : Nat8 {
    if (j == 28) { 0x02 } else if (j < 4) { Nat8.fromNat((i / (256 ** j)) % 256) } else { Nat8.fromNat((j * 37 + 11) % 256) }
  }));
  Principal.fromBlob(b);
};
func Array_tabulate(n : Nat, f : Nat -> Nat8) : [Nat8] {
  let l = List.empty<Nat8>(); var j = 0; while (j < n) { List.add(l, f(j)); j += 1 }; List.toArray(l);
};

let N = 5000;
var i = 0;
while (i < N) { createDepositAddress(freshPrincipal(i)); i += 1 };
// same principal again: idempotent (growth needs DISTINCT keypairs)
createDepositAddress(freshPrincipal(0));
let p0 = Principal.toText(freshPrincipal(0));
Debug.print("principal text length = " # debug_show(Text.size(p0)) # "  e.g. " # p0);
Debug.print("after " # debug_show(N) # " distinct callers (+1 repeat): onboarded = " # debug_show(Map.size(onboarded)));
adminSeasonWipe_clears();
Debug.print("after adminSeasonWipe clears:   onboarded = " # debug_show(Map.size(onboarded)));
adminPurgeZeroRows();
Debug.print("after adminPurgeZeroRows:       onboarded = " # debug_show(Map.size(onboarded)));
postupgrade();
Debug.print("after postupgrade:              onboarded = " # debug_show(Map.size(onboarded)));
// anonymous is the only refused caller
Debug.print("anonymous isAnonymous = " # debug_show(Principal.isAnonymous(Principal.fromText("2vxsx-fae"))));
principal text length = 63  e.g. 45fbf-3yaaa-aabh6-e5ehd-gwd5u-ld6ye-jwloa-klsxp-cq4v5-a5izx-zae
after 5_000 distinct callers (+1 repeat): onboarded = 5_000
after adminSeasonWipe clears:   onboarded = 5_000
after adminPurgeZeroRows:       onboarded = 5_000
after postupgrade:              onboarded = 5_000
anonymous isAnonymous = true

Run from the repo root: moc $(mops sources) --package mdex src/backend --implicit-package=core -r repro.mo

Heap cost per row (rough estimate). A second script inserted 200,000 rows of the same shape into a Map<Text, Bool> under enhanced orthogonal persistence (moc -wasi-system-api --enhanced-orthogonal-persistence, run under Node's WASI) and subtracted a non-retaining control loop: (delta - control)/N = +175 bytes/row. rts_heap_size includes uncollected garbage on both sides, so this is a noisy upper-end figure; an analytic count (about 88 B for the 63-char text, plus the tuple and B-tree slot) gives about 110-130 B live per row.

Impact

Confirmed (code + interpreter):

  • Any non-anonymous principal, on any posture, can add one permanent row to the Bridge's stable heap per fresh keypair.
  • The row survives season resets (adminSeasonWipe), the only purge tool (adminPurgeZeroRows) and every upgrade (postupgrade), so it accumulates across separate low-rate campaigns, and no operator tool in the tree can reclaim it. scripts/deploy.sh refuses a remote Bridge reinstall (:811-813, :885-890), so on a live venue only a code change can reclaim it.
  • Clearing it loses nothing: addresses are recomputed from the principal (addressesFor, :320-328).

Bounds (estimates, not measured on a replica):

  • No amplification. One small, fixed-size row per distinct principal; a repeat overwrites. Filling the default 3 GiB wasm memory limit (no limit is set for the Bridge: apply_memory_settings in scripts/deploy.sh:1112-1128 touches backend only) would take on the order of 18-28 million distinct signed calls.
  • Cycles run out first. Each admitted update costs the Bridge roughly 6-7 M cycles in ingress and execution fees (IC fee schedule estimate), whether or not it writes; tens of millions of calls is on the order of 100 T cycles. The DEX's tickBridgeFuel (src/backend/main.mo:10192-10216) refills the Bridge from below 1 T to 2 T and stops when its own headroom drops under freeze limit + 5 T, so a fast flood freezes the Bridge on cycles long before the heap is full. That cycle drain is the known OhShii #5.5 class and is not caused by this write. A full heap (every Bridge update trapping, including devSimulateDeposit, the #play on-ramp) is a tail outcome, not shown to be practically reachable.
  • No value impact. No solvency, conservation, authorization or user-targeting impact. The Bridge is the in-tree stub for a future custody canister (docs/bridge-and-cks-design.md), so the defect is in what is deployed today.

The defect specific to this report is permanence: free identities leave unpurgeable residue on a canister whose other per-keypair maps the project has already made purgeable.

Relationship to existing reports

A search for onboarded, createDepositAddress and getMyDepositAddresses across every issue and comment, the advisories, PRs #57/#58, docs/issue-triage-2026-08.md, docs/hardening-plan-2026-09.md, docs/security-review.md, docs/pre-mainnet-checklist.md and docs/tasks/done/ returns no hits. This extends a known class to a site its fixes did not reach:

  • #5.1 / #5.3 / #5.5 / #5.6 (OhShii Labs, triage §1.8): free identities writing unbounded permanent state on the DEX. #5.6 (aiActionExecuted writing a fixed-size row keyed on the caller) is the closest shape. All are DEX-side sites; this is the Bridge.
  • #5.2 (OhShii Labs, triage 2.15): claim() writing a ledgers row for an unvalidated asset. Fixed at bridge :380-399. Different method, different map; this report reuses that fix's pattern.
  • #47 (OhShii Labs), "What this round cut": declares the #5.1/#5.3 class "closed on the reachable surface", but that sweep checked Map.add sites keyed on caller-supplied strings among text-taking update methods. createDepositAddress takes no argument and is keyed on the caller, so it was outside the sweep.
  • H-26 / F40 (LOW): devSimulateDeposit refusals leaving zero rows. Its fix criteria, pruneZeroRows and adminPurgeZeroRows cover only ledgers and admittedUnits; onboarded is not mentioned, though the same release added it to getBridgeStats.
  • H-36 / F33 (LOW): unbounded pre-registration profile/preference records on the DEX, fixed with PREREG_CAP and LRU eviction (lib/PreReg.mo). This is the Bridge-side sibling.
  • docs/bridge-and-cks-design.md:164 ("Address creation is free, but does nothing") is a design statement about polling cost; it does not accept permanent per-principal state, and the stub no longer matches "does nothing".

Suggested fix

The flag is pure UX, so the smallest fix is to stop persisting it, the same state-free treatment claim got:

  1. createDepositAddress: requireAuth(msg.caller); addressesFor(msg.caller) with no write. getMyDepositAddresses: return ?addressesFor(msg.caller) unconditionally. If the frontend still wants a "not yet revealed" state (main.js:1851-1857), keep it client-side (e.g. localStorage).
  2. Keep the let onboarded declaration so --stable-compatible does not hit M0169 (the reason given in the comment above postupgrade, :783), and add Map.clear(onboarded) to postupgrade so existing residue is reclaimed on the next routine upgrade.

If a server-side flag must stay, the minimum is Map.clear(onboarded) next to the clears in adminSeasonWipe (:701-702). That bounds it per season but not within one; a within-season bound would need a cap plus eviction as H-36 did.

Regression pin: in tests/test_bridge_deposit_claim.sh next to the existing getBridgeStats assertions (§17, :387-425), call createDepositAddress from a fresh identity and assert getBridgeStats().onboarded is unchanged.


Part 22 of 28 from an independent review of MULTI/DEX 1.80 (7bcdde6), carried out with Claude Opus 5.5. Each finding was checked against the existing issues, advisories and fix briefs before filing; the severity is a proposal under CONTRIBUTORS.md. Where a step says "traced", it was read from source and not executed.

Ngôn ngữ chính
Shell
Star
19
Fork
7
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Chuẩn bị môi trường

Dự án này không cung cấp dev container, Dockerfile hay hướng dẫn đóng góp, nên bạn cần tự thiết lập môi trường: hãy bắt đầu từ README và xem hướng dẫn đóng góp lần đầu của chúng tôi để biết các bước chung.

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của dfinity/public-multidex

Tất cả issue của dfinity/public-multidex

Issue tương tự

Thêm issue về Shell/Bash

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.