feat(collateral)!: remove the two-term mirror morph and its mint path - #39
Conversation
`DigCollateralCoin::morph_store_launcher_id_for_mirror` derived a mirror hint from `morph(store_launcher_id + epoch)` under the tag `DIG_STORE_MIRROR_COLLATERAL` — the same tag `dig-mirror-coin` uses for `morph(store + root + owner + epoch)`. These were not two namespaces sharing a tag. Both morphs hash an additive sum, so the extra terms are absorbed rather than separating the two: measured on computed values, a four-term advertisement with a zero root and owner lands byte-identically on the two-term hint, and an attacker choosing only the epoch can solve `e' = store + epoch - store' - root' - owner'` to land a coin bonding their own store and root exactly on a victim's hint. `dig-mirror-coin` closes that by having the coin declare its four terms and checking the declaration as well as recomputing the hint. The two-term form had no equivalent check and could not gain one, since the epoch a coin was really built with is not recoverable from its hint. Rather than ship a weaker rival in a namespace another crate owns, the morph and its `create_mirror` mint path are removed; mirror collateral now has a single producer. Nothing already on chain is stranded: `from_coin_state` reads the morphed id out of memos and never recomputes it, and `spend` does not use the hint. BREAKING CHANGE: `DigCollateralCoin::morph_store_launcher_id_for_mirror` and `DigCollateralCoin::create_mirror` are removed. Use the `dig-mirror-coin` crate. Refs: DIG-Network/dig_ecosystem#3168 Co-Authored-By: Claude <noreply@anthropic.com>
|
IN PROGRESS — not the verdict. Gate review at head 1. The collision is independently REPRODUCED (not taken on trust)I did not read the argument and agree with it. I reimplemented the CLVM tree hash from scratch in
The equality is in fact encoding-independent: both derivations are the same function applied to a 2. The transcription pin is real and it CONSTRAINS
All seven assertions are on computed 32-byte values; none asserts a symbol's presence. The two Still to check: deletion blast radius, on-chain spendability of already-minted coins, the manual |
loop-security — IN PROGRESS, not the verdictAudited head: Posting findings as they resolve so they survive an interruption. 1. Money path: existing mirror coins stay readable AND spendable — CONFIRMEDThe highest-severity question was whether deleting the derivation strands funds already on chain. It does not.
So the CHANGELOG's claim holds for spend and read. One narrowing the PR does not state, and should. Spendability is preserved given a 2. Removal is clean — no dangling references, no build break
3. Blast radius is smaller than the major bump implies: the removed API was Rust-only
4. No secrets introducedThe diff adds no key, seed, credential or test vector — it is a deletion plus a doc comment plus three version bumps. Nothing added logs or prints key material. Still open: the class sweep for other additive morphs, whether |
loop-security — IN PROGRESS (2/2), not the verdictHead 5. The four-term form does NOT defeat the solve — arity was never the defence. CONFIRMEDThe PR's rationale is correct and, importantly, it does not overclaim. What actually stops it is
That last point is what makes check 2 load-bearing, and I tried to break it. The attack would be: place a coin in a victim's bucket, declare the victim's tuple in memos, and write a matching No read path accepts a coin on hint match alone. 6. Question 4 — the surviving
|
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
PASS
Head reviewed: f46ca1dd498bd156cd0fc0dbb3ecd983966f9521 (resolved from the remote, not from the dispatch brief). Fresh context; I verified every claim rather than accepting the lane's account.
I independently reproduced the collision. It is real.
This is the fact the whole PR turns on, so I did not read the argument and agree with it. I reimplemented the CLVM tree hash from scratch in Python — atom(b)=sha256(0x01||b), pair(l,r)=sha256(0x02||l||r), minimal two's-complement big-endian atom encoding — and computed values with no DIG code in the path. With store=[0x11;32] under tag DIG_STORE_MIRROR_COLLATERAL:
| derivation | 32 bytes |
|---|---|
two-term morph(store + 7) |
0819e926…7e32aa05 |
four-term morph(store + 0 + 0 + 7) |
0819e926…7e32aa05 — identical |
attacker four-term, store'=[0xAA;32] root'=[0xBB;32] owner'=[0xCC;32], e' = store+7-store'-root'-owner' |
0819e926…7e32aa05 — identical |
| control: epoch 8 | 68f97104… differs |
control: tag DIG_STORE_COLLATERAL |
d901bebc… differs |
The equality is stronger than a computation: both derivations are the same function of a scalar sum (dig-mirror-coin/src/namespace.rs:82-87 vs the removed morph_store_launcher_id_for_mirror), so equal sums give equal bytes under any atom encoding. The lane's reframing is correct and the ticket's framing was not: these are one namespace, not two schemes sharing a label, and the ticket's "it fails closed today" is false against an adversary who chooses the epoch. A remedy selected from the ticket's framing — a retag, or a four-term rewrite — would have been the wrong remedy.
The tests are load-bearing, and the transcription pin genuinely constrains
dig-collateral-coin/tests/mirror_namespace_separation.rs: all seven assertions compare computed 32-byte values; none asserts a symbol's presence. Two controls discriminate — the_canonical_morph_separates_two_epochs_of_one_advertisement, and the assert_ne! inside the solved-epoch test that fails if the collision arrives for any reason other than the solve.
The transcription risk (a self-consistent fiction) is closed by the_transcribed_retired_morph_reproduces_the_removed_upstream_function, which pins the bytes observed from the real datalayer_driver 3.0.0 function. My independent Python implementation reproduces that pinned constant byte for byte — so the transcription is now checked against two implementations neither of which is itself.
Nothing on chain is stranded — checked in the code, not the doc comment
spend (src/dig_collateral_coin.rs:187) uses only the inner P2ParentCoin, its lineage proof, the synthetic key and the fee coins; the morphed id is absent from it. from_coin_state (:132) parses the morphed id out of memos_vec[0] and never recomputes or compares it. Rediscovery-by-hint also survives: dig_mirror_coin::mirror_hint(store, [0;32], [0;32], epoch) reproduces the retired hint byte-identically — the same collision that justified the removal guarantees no capability is lost. (Inline comment asks for that sentence in the CHANGELOG.)
Deletion over #[deprecated] — the judgement holds
All four crates.io reverse dependents (dig-wallet 2.0.0, dig-store-coin 2.1.0, dig-collateral-coin 0.1.4 at ^3.0.0; digstore-chain 0.29.0 at ^5) are semver-incompatible with 6.0.0, so a deprecation would warn nobody who is not already choosing to upgrade, while leaving a colliding money-path derivation callable. Compile error plus a changelog naming dig_mirror_coin::mirror_hint / ::create is the right volume.
Version and CHANGELOG
5.0.0 → 6.0.0 in Cargo.toml, Cargo.lock and napi/package.json, same value in all three; correct for a public-API removal. The CHANGELOG states the break and names the replacement. Checks: 18 SUCCESS, 2 SKIPPED, 0 failing.
Three non-gating notes (posted inline; I am resolving all three so none bars the merge)
CHANGELOG.md:33— an unchanged item listed under### Removed (BREAKING).src/dig_collateral_coin.rs:37— add the rediscovery sentence.CHANGELOG.md:27— the PR body's "zero code hits" cites an instrument that also returns zero for a symbol known to exist; the conclusion is right but the cited evidence is not what establishes it.
Two conditions the orchestrator needs, neither caused by this PR
mainis NOT branch-protected on this repo —repos/DIG-Network/DataLayer-Driver/branches/main/protectionreturns404 Branch not protected.check-merge-preconditions.shtherefore exits 2 (UNREADABLE), which is not a merge verdict. §3.6b requires protection on every submodule but dig-browser. Pre-existing; worth its own ticket.- Publish is manual — the repo has only
CI.yml, no release workflow and nocliff.toml. Nothing in this PR assumes an automatic release (I checked the body and the diff). Merging does not put 6.0.0 on crates.io; latest published is still 5.0.0, anddig-collateral-coinPR#5 stays blocked until someone publishes by hand.
Blast radius established by grep + direct read + the crates.io API — this repo is in no local gitnexus index. The PR body states that fallback explicitly rather than implying an index was used, which is correct per §2.0.
Verdict: PASS. The premise is reproduced, the remedy follows from it, the money path is safe, and the tests would fail if the code were wrong.
loop-security — IN PROGRESS, not the verdictAudited head: Q2 (asked first) — the surviving
|
loop-security — IN PROGRESS (2/2), not the verdictHead re-confirmed Q3 — CONFIRMED: the arity does not defeat the solve;
|
| path | keyed on | hint-only? |
|---|---|---|
discover (query.rs:351) |
computes hint at :358, fetches unspent_coins_by_hint at :360, then filters through mirror.advertises(...) at :373 before pushing at :375 |
no |
list (query.rs:269) |
mirror.owner_puzzle_hash() == owner_puzzle_hash at :291 — the lineage proof; never touches the hint |
no |
census C8 (census.rs:723) |
coin.advertises(coin.store_launcher_id(), coin.root_hash(), coin.epoch()) — self-consistency: does the coin's own declaration reproduce its own hint |
no |
The one consumer in the ecosystem is dig-node, via dig_mirror_coin::census (dig-node-service/src/collateral_census.rs:42). It does not call discover/list in non-test code and never reads namespace_hint() to make a trust decision. I found no path anywhere that accepts a coin on hint match without the declared-tuple equality.
Q6 — secrets: clean
The diff adds 56 lines across 5 files and adds no test file at all (the brief anticipated one; there is none). Scanning the added lines for key/seed/secret/token/mnemonic/PEM material and for 32+ byte hex or 40+ char base64 runs returns zero hits. The change is a deletion plus doc comments plus three version bumps. Nothing added logs anything, key material or otherwise.
Correction to the dispatch brief — merging can publish, and the stated reason was wrong
The brief says "Publish is manual — only CI.yml, no release workflow, no cliff.toml. Merging does not publish 6.0.0." The conclusion is right; the reason is not, and the difference matters operationally.
CI.yml does contain an automated publish path — two jobs, publish-npm (:269) and publish-crates (:325), both gated if: github.event_name == 'push' && github.ref == 'refs/heads/main'. They are the two SKIPPED checks on this PR. What actually withholds the publish is a second, inner gate: the job body requires the last commit message to match ^[0-9]\+\.[0-9]\+\.[0-9]\+$ — a bare version and nothing else (:339, :357, :312).
This PR's squash subject will be feat(collateral)!: remove the two-term mirror morph and its mint path, which does not match, so the merge itself publishes nothing. But releasing 6.0.0 is not a separate workflow dispatch — it is one push to main with the commit message 6.0.0. Whoever makes the yank/advisory call in Q4 should know the release trigger is that cheap, and that it is a push: main event rather than a reviewable dispatch.
NON-GATING, pre-existing, but on the path this version will take — CI.yml:373 publishes with cargo publish --no-verify --allow-dirty, immediately after a step that runs cargo update --dry-run || true and git commits any Cargo.lock change (:343-351). --no-verify skips building the packaged crate and --allow-dirty publishes uncommitted working-tree state, so the crates.io artifact is not guaranteed to correspond to the merged tree and is never verified to build from the package. That is the same shape recorded for chia-l2-consensus, where an --allow-dirty publish path let a published crate carry a different consensus rule than its source. It is worth a follow-up ticket precisely because the point of 6.0.0 is that a path is absent: a publish that can carry dirty state weakens the guarantee that the released 6.0.0 actually lacks it. Also permissions: contents: write, id-token: write (:6-8) is workflow-scoped rather than job-scoped, so every build job inherits write + OIDC.
Remaining: Q4 (downgrade exposure, reverse dependents) and Q5 (disclosure), then the verdict.
loop-security — VERDICT: PASSAudited head: Detail for each question is in my two interim comments above. This is the verdict. Answering Q2 first: the surviving namespace does not under-fixThe premise did not hold, and the real answer is stronger than the one the question was hunting for. There is no epoch-0 disjointness assertion in this PR. The diff is five files and contains no test at all; the PR body has zero hits for But the property it under-tests is structurally general, so the weak test is a test-quality issue rather than a vacuous guarantee. Two independent reasons:
A third, incidental separator: the left atoms use different encodings ( Conclusion: this PR fixes the class within its own repo, not merely the instance. The one namespace it retains cannot carry the defect by construction. Non-gating, different repo: the comment at Findings, ranked1. NON-GATING (but the most consequential fact here) — the vulnerable path stays live on crates.io, re-exported by a maintained DIG crate
crates.io, verified directly:
This does not gate the PR, and gating on it would be actively wrong — removing the path is strictly an improvement, and blocking the removal would leave it present in 6.0.0 as well. It is a follow-up family: migrate the four dependents, then decide on the registry action. 2. NON-GATING — what actually remains exploitable in the wild (Q4), stated plainlyAn attacker with 3.x/4.x/5.x can call So the residual harm is not attacker capability against the census; it is honest-user money visibility: anyone who mints mirror collateral through 3.x today produces coins that On yank vs advisory — the facts, since the call is the user's. A yank of 3.0.0 would break fresh builds of the three 3. NON-GATING — Q5, disclosure: do not narrow the PR descriptionThe repo is public and the PR describes the attack in detail while vulnerable versions stay published, so the question is fair. My answer is that narrowing buys nothing real:
Redacting a description whose contents are already executable in a sibling public crate would trade a real migration signal for no confidentiality. 4. NON-GATING, pre-existing — the publish path uses
|
| area | verdict |
|---|---|
| Class fix vs instance fix (Q1) | Clear in-repo. DIG_STORE_MIRROR_COLLATERAL appears in no code path at head — only in the doc comment and CHANGELOG. One namespace tag literal remains (:58), one namespace morph among 6 .tree_hash() sites, and it takes no caller input. Residual is outside the repo (finding 1). |
| Surviving namespace disjointness (Q2) | Clear, and general over all epochs — by tag position, not by offset. Not a vacuous epoch-0 guarantee. |
| Four-term form vs the solve (Q3) | Confirmed as suspected: arity does not defeat it (namespace.rs:198 asserts the solve within the four-term form). advertises carries the load, with the owner from the lineage proof. |
| Read paths accepting on hint alone (Q3) | None found. discover (query.rs:358 then :360 then :373) filters through advertises after the hint fetch; list (:291) is owner-keyed off the lineage proof and never touches the hint; census C8 (census.rs:723) checks self-consistency. Sole consumer dig-node uses census only and never reads namespace_hint() for a trust decision. |
| Custody / money paths | Untouched. Independently spot-confirmed: morphed_store_id appears nowhere in spend; from_coin_state (:132-136) parses it from memos_vec[0] and never recomputes. No signing, key handling, or privilege change in the diff. |
| Secrets (Q6) | Clean. 56 added lines, no test file added (the brief anticipated one — there is none), zero hits for key/seed/secret/token/mnemonic/PEM material or 32-byte-hex / 40-char-base64 runs. Nothing added logs anything. |
| Dependencies | No dependency added, removed or loosened. The only Cargo.lock change is this crate's own 5.0.0 to 6.0.0. The num_bigint import is dropped in step with its last use. |
| Versioning | Consistent at 6.0.0 across Cargo.toml, Cargo.lock and napi/package.json. Major is correct — two public methods removed. |
| CI | 18 SUCCESS, 2 SKIPPED (the two publish jobs), 0 failing at f46ca1dd. |
On merge preconditions: main on this repo is not branch-protected (/branches/main/protection returns 404), so check-merge-preconditions.sh exits 2 = UNREADABLE, which is not a merge verdict either way. I therefore asserted the checks by name from the rollup rather than relying on that script, and I am not representing its exit as a green. The unprotected main is itself worth a ticket — it means the required-checks and thread-resolution gates are not mechanically enforced on this repo.
Verdict
PASS. No security defect in this diff. It removes a genuine collision primitive, retains a namespace that cannot carry the same defect, touches no money path, adds no secret and no dependency, and is correctly versioned as a major.
The residual risk is entirely outside this PR — 3.x remains published and re-exported by dig-collateral-coin — and it is reduced, not created, by merging. Findings 1-4 belong on dig_ecosystem#3168 as follow-ups; none of them should hold this merge.
DRAFT — gate round not yet returned. DO NOT MERGE.
Retires the two-term mirror morph. Part of
https://github.com/DIG-Network/dig_ecosystem/issues/3168 (epic
https://github.com/DIG-Network/dig_ecosystem/issues/3166).
What was wrong
DigCollateralCoin::morph_store_launcher_id_for_mirrorderived a mirror hint frommorph(store_launcher_id + epoch)under the tagDIG_STORE_MIRROR_COLLATERAL.dig-mirror-coin0.7.0 — the canonical owner of mirror collateral — derives one from
morph(store + root + owner + epoch)under that identical tag.The ticket framed this as two schemes mislabelling themselves. It is worse: they are one
namespace. Both morphs hash an additive sum, so the extra terms are absorbed rather than
separating them. Measured on computed 32-byte values (
dig-collateral-coin,tests/mirror_namespace_separation.rs, against datalayer-driver 3.0.0 + dig-mirror-coin 0.7.0):hint — differing arity does not separate the schemes;
e' = store + epoch - store' - root' - owner'andlands a coin bonding their own store and root exactly on a victim's two-term hint. Every other
term there is attacker-chosen and distinct from the victim's, so this is not the zero case
restated: a fix that stopped the zero reduction would leave it standing.
Two controls in the same file pass, so the harness is attached rather than returning a constant.
Why removal rather than a retag or a four-term rewrite
MirrorCoin::advertises, the declared-tuple check that is the only thing defeating thesolved-epoch attack. It would look canonical, genuinely collide with real mirror hints, and be
silently weaker than the crate it imitates.
silently changes on-chain coin identity for anyone already calling
create_mirror. A silentbehaviour change on a money path is worse than a compile error.
A
#[deprecated]would reach existing users, which is a genuine argument for it. But this is acollateral money path with a constructible collision and no declaration check, and a compile error
at upgrade — beside a changelog naming the replacement — is the right volume. Pinned consumers are
untouched until they choose to upgrade.
Blast radius
Established by grep + direct read + the crates.io API; this repo is not present in any local
gitnexus index, so §2.0's sanctioned fallback was used and is stated here rather than implied.
create_mirror(removed with it). No napi binding, notest, and no README reference to either symbol —
grep -rn 'create_mirror|morph_store_launcher_id_for_mirror'over
*.rs/*.ts/*.js/*.mdreturns only the two definitions and that one call site.datalayer-driver: four, all DIG-Network's own —dig-wallet2.0.0,
dig-store-coin2.1.0,dig-collateral-coin0.1.4 (all^3.0.0),digstore-chain0.29.0(
^5). No third-party dependent is registered.gh search code --owner DIG-Network morph_store_launcher_id_for_mirror: zero hits outside thisrepo.
Nothing already on chain is stranded.
from_coin_statereads the morphed id out of the coin'smemos and never recomputes it, and
spenddoes not use the hint, so mirror coins minted through theremoved path remain readable and spendable.
Version
5.0.0→6.0.0inCargo.tomlandnapi/package.json(both files, same value). Major: a publicAPI is removed.
cargo check --libgreen;cargo fmt --checkgreen.