Skip to content

fix(shielded-pool): bound the zero-hash ladder and the tree depth shift - #124

Merged
nol4lej merged 1 commit into
mainfrom
security/merkle-recursion-and-shift-bounds
Aug 7, 2026
Merged

fix(shielded-pool): bound the zero-hash ladder and the tree depth shift#124
nol4lej merged 1 commit into
mainfrom
security/merkle-recursion-and-shift-bounds

Conversation

@nol4lej

@nol4lej nol4lej commented Aug 7, 2026

Copy link
Copy Markdown
Member

Verified by instantiating at 32 and confirming the error, then removing the probe. The assertion has to be referenced from capacity() — an associated const is not evaluated where nothing uses it.

On reachability

This is hardening of latent code, not a live hole. Every level at every call site comes from a 0..depth loop bound by DEFAULT_TREE_DEPTH, so no external input selects one. What changes is where the bound lives: attached to the danger, rather than in a runtime config that a downstream caller of a pub generic type never sees.

Verification

Unit tests 377 (5 new)
Clippy, CI feature set, -D warnings clean
Dev-node: ladder unchanged 10/10
Dev-node: adversarial 30/30

The equivalence test is the one that matters. It compares the loop against a local recursive reference across levels 0–24, including past the cache boundary where the fall-through lives. Mutation-checked: breaking the loop by a single iteration (0..level.saturating_sub(1)) makes it fail.

The ladder E2E reads the digests back out of real Merkle paths served by the Wasm runtime — not computed natively — and matches all 19 levels against pinned values. It requires an empty tree and aborts with instructions otherwise, since on a populated tree those siblings are real subtrees and the comparison reports a divergence that is not one.

The adversarial E2E tries to break the node instead of confirming it: u32::MAX leaf indices, the sign boundary, 64 KiB and non-hex commitments, a 50-request burst, and storage queries at levels 21 through 255 — checking block height after every batch. Everything is refused without the node hanging or stalling.

That last one proves less than it looks. Tracing the callers showed the vector is not reachable from outside at all, so it confirms the surfaces reject garbage rather than demonstrating the fix works. The 1 MiB stack measurement above is what actually demonstrates it.

Corrections made while verifying

An earlier version of the test comment claimed 5,000 frames already overflowed 1 MB. That was never measured and is wrong — the threshold sits between 10,000 and 20,000. Corrected in both the test and the changelog with the measured figures.

Mid-measurement, a single zero_hash_at_level(1) overflowed a 1 MB stack, which looked like something worse than this PR: one Poseidon call exhausting the runtime stack. It is a debug artifact — unoptimised arkworks leaves large structures on the stack, and release builds pass comfortably. Production runs release. Checked before reporting it as a finding.

Scope

Six files: the two primitives, their tests, and the two dev-node runs. The spec_version bump ships separately.

@nol4lej
nol4lej merged commit 23120e8 into main Aug 7, 2026
6 checks passed
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.

1 participant