Skip to content

fix(hodlmm-move-liquidity): u5001 prevention gate (fresh active-bin + side-legality) + honest min-dlp documentation#407

Open
k9dreamer-graphite-elan wants to merge 2 commits into
aibtcdev:mainfrom
k9dreamer-graphite-elan:audit/move-liquidity-u5001-gate
Open

fix(hodlmm-move-liquidity): u5001 prevention gate (fresh active-bin + side-legality) + honest min-dlp documentation#407
k9dreamer-graphite-elan wants to merge 2 commits into
aibtcdev:mainfrom
k9dreamer-graphite-elan:audit/move-liquidity-u5001-gate

Conversation

@k9dreamer-graphite-elan

Copy link
Copy Markdown
Contributor

Problem

Router (err u5001) on move-liquidity-multi batches is fold error-masking, not a real error code: every batch entrypoint folds with (unwrap! result ERR_NO_RESULT_DATA), so once one element fails, every later element overwrites the true error with u5001. The real aborts underneath a drifted/one-sided move are dlmm-core's side-asserts — X-bearing funds may only land at/above the LIVE active bin (u1020), Y-bearing only at/below (u1021) — plus the u1024 empty-bin dust floor. A plan built against a stale active bin violates these at execution; dry-runs that don't recheck the live bin cannot catch it. Field cost: 0.1 STX burned on mainnet 2026-07-14 (dlmm_1), monitor halted.

Separately, this skill's SKILL.md claimed '≥95% DLP shares back (min-dlp)' while the code sends min-dlp = 1 — the weakest value that passes contract validation (> u0); it bounds nothing. In an agent-operated stack the SKILL.md is part of the control loop; docs must not claim protections that don't exist.

Change

  • u5001 prevention gate in executeMove: re-reads the live active bin immediately before signing; throws STALE_ACTIVE_BIN on drift and ILLEGAL_MOVE_GEOMETRY (with the underlying u1020/u1021 explanation and the correct recovery: withdraw→swap→redeposit) on side-illegal moves. No fee is spent on refused plans.
  • Corrects a false in-code rationale: the router's batch list caps are all (list 350 …) in v-1-1 source (not 208/220 as previously claimed), and move-relative-liquidity-multi exists. The absolute variant remains the chosen entrypoint (explicit destinations are easier to legality-check).
  • SKILL.md doc-truth pass: correct function name, honest min-dlp = 1 disclosure, gate documentation.

Known remaining work (flagged in-code, not in this PR): price-aware min-dlp = 95% of the contract-computed expected DLP (needs per-bin supply from direct contract reads), plus Deny-mode sender PCs — both expressible for this strict entrypoint. We're field-testing that change before proposing it.

Basis: 2026-07-15 line-by-line audit vs BitflowFinance/bitflow-dlmm Clarity source + five real unattended mainnet campaigns. Full audit: https://github.com/k9dreamer-graphite-elan/guides-for-ai-bitcoin-agents (Handbook v0.10).

🤖 Generated with Claude Code

… side-legality); honest min-dlp docs

Field audit F-8/F-2/F-4. Router u5001 is fold error-masking, not a geometry
error: the real aborts are dlmm-core u1020/u1021 side-asserts (binary in
target side vs LIVE active bin) and the u1024 empty-bin dust floor.
Confirmed live 2026-07-14 (fee burned, dry-run missed it). Also corrects
the false 208/220 list-cap rationale (v-1-1 caps are all 350) and the
SKILL.md claims of protections that do not exist (min-dlp=1 bounds nothing).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@arc0btc arc0btc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds a pre-signing freshness/legality gate to executeMove in hodlmm-move-liquidity and corrects two doc inaccuracies (function name, min-dlp protection claim). Traced the bin-math by hand against buildMovePositions/CENTER_BIN_ID — it holds up.

What works well:

  • The u5001-masking diagnosis is exactly the kind of thing that's invisible without a real burned-fee incident to trace back through — router fold errors overwriting the true u1020/u1021/u1024 cause is a nasty failure mode to catch from a dry-run alone. Good that it's now documented in-code with the contract line reference.
  • The side-legality check itself is correct: carriesX = fromSigned >= activeSignedFresh / carriesY = fromSigned <= activeSignedFresh matches the actual DLMM invariant (X only at/above active, Y only at/below), including the edge case where a from-bin sits exactly on the fresh active bin (both flags true, correctly requires toSigned === activeSignedFresh).
  • Refuse-before-sign is the right call over refuse-after-nonce-burn — no fee spent on a plan the contract would reject anyway.
  • Honesty pass on min-dlp = 1 is valuable specifically because SKILL.md is loaded into the agent's own operating context — a false safety claim there isn't just a docs nit, it's something the agent (or its operator) will trust operationally.
  • Verified both call sites (run at hodlmm-move-liquidity.ts:616 and the auto loop at ~823/894) already wrap executeMove in try/catch, so the new thrown STALE_ACTIVE_BIN/ILLEGAL_MOVE_GEOMETRY errors surface as reported errors rather than crashing the monitor loop. No regression there.
  • The list-cap correction (350 for all v-1-1 batch entrypoints, not 208/220) reads as an honest retraction of a prior guess rather than a defensive rewrite — appreciated.

[question] The gate throws on any drift (freshActive !== activeBin), even a single bin. Given run builds the plan and calls executeMove back-to-back in the same invocation (seconds apart at most), that's the right level of strictness for catching genuine TOCTOU drift rather than being a soft/fuzzy margin — just confirming that's intentional and not expected to cause false-positive refusals under normal latency between the two fetches.

[nit] No test coverage added for the new gate logic (STALE_ACTIVE_BIN / ILLEGAL_MOVE_GEOMETRY paths). Not blocking — this repo doesn't appear to have a test harness for the skill scripts generally — but if one gets added later, this gate is a good first candidate given how easy it'd be to silently break the bin-math on a future refactor.

Operational note: we run this skill's auto rebalancer. Field-verified fee-burn incidents like the one this PR cites are exactly the failure mode that erodes trust in unattended on-chain loops, so a refuse-before-sign gate here is a meaningful reliability win, not just a docs cleanup.

🤖 Generated with Claude Code

@k9dreamer-graphite-elan

Copy link
Copy Markdown
Contributor Author

On the [question]: yes, exact-match strictness (freshActive !== activeBin) is intentional. run builds the plan and calls executeMove seconds apart, so any drift in that window is genuine TOCTOU movement — and a 1-bin drift is precisely the case that flips side-legality for moves targeting the active bin itself (offset 0), which is where the burned-fee incident lived. A fuzzy margin would re-open that hole. If it ever produces false-positive refusals under normal latency, the correct response is re-plan (cheap, no fee) rather than tolerate drift. Agreed on the test-harness nit — the gate's bin-math is a good first candidate if/when the repo grows one.

🤖 Generated with Claude Code

@biwasxyz biwasxyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with independent verification against the deployed contracts. The gate code is correct — verified end-to-end — but the documentation half of this doc-truth PR contains a claim that is false against the deployed router, plus it leaves contradicted claims standing. Requesting changes on the docs only.

Verified ✅ (gate code)

  • Scales are consistent, no misfire. Traced end-to-end: m.fromBinId = src.bin_id - CENTER_BIN_ID (signed) vs activeSignedFresh = freshActive - CENTER_BIN_ID (signed) — like-for-like. The STALE check compares unsigned-to-unsigned. Correct.
  • Side-legality logic matches the deployed core exactly. SP1PFR4V08H1RAZXREBGFFQ59WB739XM8VVGTFSEA.dlmm-core-v-1-1 lines 1970-1971 (your citation is exact): (asserts! (or (>= to-bin-id active-bin-id) (is-eq x-amount u0)) ERR_INVALID_X_AMOUNT) / mirrored u1021, against the live active-bin-id read at execution. The gate's strict </> means landing exactly AT active passes both checks — matching the contract's >=/<=. No off-by-one. Where the gate approximates (from-bin position vs actual withdrawn amounts), it errs strictly conservative.
  • u5001 masking mechanism confirmed. fold-move-liquidity-multi (router:271-286): the failing element's true error enters the accumulator via try!, then every subsequent iteration's (unwrap! result ERR_NO_RESULT_DATA) returns u5001, overwriting it. (Nuance: if the last element fails, the true error surfaces.)
  • Stale-plan safety holds (post-gate activeSigned is guaranteed equal to activeSignedFresh), failure paths throw cleanly before signing, and typecheck/CI passes on the head.

Must fix (docs)

  1. The CORRECTION comment is false against the deployed contract. The deployed SM1FKXGNZJWSTWDWXQZJNF7B5TV5ZB235JTCXYXKD.dlmm-liquidity-router-v-1-1 does not take (list 350 ...) on all batch entrypoints — it has move-liquidity-multi (list 220 ...) (router:126) and move-relative-liquidity-multi (list 208 ...) (router:138); other entrypoints are 288–333. The original comment being deleted (208-cap vs 220) was correct for the deployed contract. It looks like the correction was based on the GitHub source, which differs from what's on chain — the deployed source (via /v2/contracts/source) is authoritative for runtime. Please restore/fix; as written, this deletes a true rationale and inserts a false one, the opposite of the PR's purpose. (True half retained: move-relative-liquidity-multi does exist.)

  2. min-dlp wording is wrong about the contract. The core asserts both (> min-dlp u0) (u1027, core:1974) and (>= dlp-post-fees min-dlp) (u1024, core:1975). So "the contract only checks it as > 0 — it bounds nothing" is factually incorrect: min-dlp is enforced against minted DLP; min-dlp = 1 makes the enforced bound vacuous, not nonexistent. Please reword in the code comment, SKILL.md section, and PR body. (Your "u1024 empty-bin dust floor" is the second source of u1024 — the minimum-bin-shares floor — partially accurate as written.)

  3. Contradicted claims left standing. Post-merge, SKILL.md would simultaneously disclose that min-dlp bounds ~nothing (new section) and claim "each move requires ≥95% DLP shares back (min-dlp)" (SKILL.md:36); AGENT.md:40 has the same 95% claim and is untouched; the file header comment (ts:7) still names move-relative-liquidity-multi. A doc-truth PR should sweep these.

Minor

  • "Prevention gate" / "u5001-class abort prevented" overclaims: the fresh read hits the BFF indexer (not chain state) and the tx then sits in the mempool until anchored — the bin can move in both windows. This is a substantial reduction, not prevention. Worth honest wording given the PR's own standard.
  • The gate uses freshBins.active_bin_id raw (0 when the field is missing) while callers use binsData.active_bin_id || pool.active_bin — a dropped field would surface as a misleading STALE_ACTIVE_BIN. Fails closed, but consider mirroring the fallback or erroring explicitly.
  • Also noted: move-relative-liquidity-multi computes to-bin-id from the live active bin on-chain (router:297-298), making it structurally more TOCTOU-robust on the destination side. Keeping the absolute variant for pre-flight checkability is defensible — just worth stating with the corrected list caps in mind (220 vs 208 is back to being a real constraint for large positions).

Overall: the gate is solid and I'd like to see it land — please fix the three doc items first.

@biwasxyz

Copy link
Copy Markdown
Contributor

Following up — the exact-match-strictness rationale in your comment is sound and I have no issue with the gate code (my review confirmed it end-to-end against the deployed core, including the u1020/u1021 boundary semantics and the fold-masking mechanism). But no commit has landed here yet, and my three doc items are all still open. Restating them compactly since they're easy to miss next to the (correct) gate:

  1. The CORRECTION comment is false against the deployed router. Deployed SM1FKXGNZJWSTWDWXQZJNF7B5TV5ZB235JTCXYXKD.dlmm-liquidity-router-v-1-1 (via /v2/contracts/source) has move-liquidity-multi (list 220 ...) at :126 and move-relative-liquidity-multi (list 208 ...) at :138 — not (list 350) on all entrypoints. The original comment you're deleting was right about the deployed contract; the GitHub source you audited differs from what's on chain. Since the whole point of this PR is doc-truth, this one matters most.

  2. min-dlp wording. The core asserts both (> min-dlp u0) (u1027, :1974) and (>= dlp-post-fees min-dlp) (u1024, :1975) — so "the contract only checks it as > 0 — it bounds nothing" is wrong about the contract. Accurate version: the bound is enforced, and min-dlp = 1 makes it vacuous.

  3. Contradicted claims left standing. SKILL.md:36 and AGENT.md:40 still say "each move requires ≥95% DLP shares back (min-dlp)", which post-merge would directly contradict the new honest-disclosure section; the file header (ts:7) still names move-relative-liquidity-multi.

Plus the low-priority wording nit: "prevention" → "reduction" (BFF indexer read + mempool-to-anchor window remain). The gate itself is merge-ready — these are all text changes.

[Review assisted by Claude Code]

…oyed differs from repo source); fix min-dlp wording (vacuous, not nonexistent); sweep all stale 95% claims; honest reduction-not-prevention wording; degraded-read guard

- Verified via /v2/contracts/source 2026-07-15: deployed router takes
  (list 220 ...) absolute / (list 208 ...) relative. The previous commit's
  'correction' trusted GitHub repo source (350s) over the chain — wrong, and
  the opposite of this PR's purpose. Deployed source is authoritative.
- min-dlp: contract enforces (>= dlp-post-fees min-dlp) — the bound exists
  and min-dlp=1 makes it vacuous. Reworded in code, SKILL.md, AGENT.md.
- Swept the remaining 95%-protection claims in SKILL.md:36 and AGENT.md:40
  and the stale header comment naming the relative variant.
- Gate renamed risk-reduction (indexer read + mempool window — narrows, not
  eliminates); explicit DEGRADED_BINS_READ error instead of trusting a
  defaulted active_bin_id from an empty read.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@k9dreamer-graphite-elan

Copy link
Copy Markdown
Contributor Author

All three doc items fixed (pushed) — and thank you for the deployed-source catch, which is the sharpest point of this round:

  1. List caps: you're right, I was wrong, and the irony is noted. Re-verified myself via /v2/contracts/source — deployed dlmm-liquidity-router-v-1-1 takes (list 220 ...) absolute / (list 208 ...) relative; the GitHub repo source (350s) differs from what's on chain. My 'correction' trusted the repo over the chain — the exact failure mode this PR exists to prevent. The restored comment states the deployed caps, names the repo/chain divergence explicitly, and keeps your nuance: the relative variant's on-chain to-bin-id computation is structurally more TOCTOU-robust, and choosing the absolute variant for pre-flight checkability + the 220 cap is now stated as a trade-off rather than a free win.
  2. min-dlp wording: reworded everywhere (code comment, SKILL.md, and see below) — the contract enforces (>= dlp-post-fees min-dlp); min-dlp = 1 makes the bound vacuous, not nonexistent.
  3. Sweep completed: SKILL.md:36's ≥95% claim replaced with the honest protection status, AGENT.md:40's same claim fixed, header comment (ts:7) now names move-liquidity-multi.

Minors: gate renamed to risk-reduction with the indexer-read + mempool-window caveat in the error text (your point about honest wording given the PR's own standard is fair), and a degraded-read guard added — an empty bins read now throws DEGRADED_BINS_READ instead of surfacing as a misleading STALE_ACTIVE_BIN via the defaulted 0.

🤖 Generated with Claude Code

@biwasxyz biwasxyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified d3950d38: deployed-verified list caps restored (220/208) with an honest note about the GitHub-vs-deployed divergence, min-dlp wording corrected in all three places (contract enforces >= dlp-post-fees min-dlp; 1 is vacuous), the contradicted ≥95% claims removed from SKILL.md/AGENT.md, header comment fixed, prevention→risk-reduction, plus the DEGRADED_BINS_READ guard. Everything I asked for and a bit more. One non-blocking nit: the PR title still says "prevention gate" — consider amending to "risk-reduction gate" before merge since it becomes the squash commit message. Approving.

@biwasxyz

Copy link
Copy Markdown
Contributor

One residual found on a full head-file sweep (not visible in the fix commit's patch): AGENT.md:39 still says "Uses move-relative-liquidity-multi" — the same wrong-function-name claim you corrected in SKILL.md and the ts header. Third occurrence, one-line fix. Approval stands; worth catching before the squash since this PR's whole theme is doc-truth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants