Skip to content

fix(core/bitcoin): reject SPV proofs with unconsumed leaf-index bits#431

Open
dsmfa10 wants to merge 1 commit into
mainfrom
fix/core-spv-index-bound
Open

fix(core/bitcoin): reject SPV proofs with unconsumed leaf-index bits#431
dsmfa10 wants to merge 1 commit into
mainfrom
fix/core-spv-index-bound

Conversation

@dsmfa10
Copy link
Copy Markdown
Collaborator

@dsmfa10 dsmfa10 commented Jun 3, 2026

Problem

verify_spv_proof walks the Merkle siblings, consuming one bit of the leaf
index per level via idx >>= 1, but never checks that all index bits were
consumed:

// src/bitcoin/spv.rs (before)
let mut idx = proof.index;
for sibling in &proof.siblings {
    if idx & 1 == 0 { current = merkle_parent(&current, sibling); }
    else            { current = merkle_parent(sibling, &current); }
    idx >>= 1;
}
current == *merkle_root          // <-- leftover high bits of idx ignored

After folding siblings.len() levels, any high bits remaining in idx are
silently discarded. So leaf index i and i + (1 << siblings.len()) produce the
same fold and verify against the same root — proof malleability (multiple
distinct index values accepted for one proof). This contradicts the
canonical-encoding hardening already applied to SpvProof::from_bytes (issue
#181 Finding 3), which rejects trailing garbage for exactly this reason.

Fix

Require idx == 0 after the fold, so the index must address exactly the tree
depth the proof provides.

// After folding siblings.len() levels, every index bit must be consumed.
current == *merkle_root && idx == 0

A valid Merkle proof's leaf index is < 2^depth where depth == siblings.len(),
so idx shifts down to 0 — legitimate proofs pass. The single-tx (empty
siblings) case correctly requires index == 0.

Verification

cargo test -p dsm --lib bitcoin::spv — all 5 tests pass (single-tx, two-tx,
roundtrip, trailing-garbage rejection, header extraction).

Notes

A related, separate observation (not fixed here): verify_spv_proof has no
guard against an interior 64-byte node being passed as a "txid"; in practice the
consumer (limbo_vault) independently deserializes the raw tx, so it's covered
there. Left as-is to avoid changing accept behavior.

CI gates & coverage

Full verification for this PR runs in CI — Rust (cargo fmt --check,
clippy -D warnings, workspace tests), Frontend, Android Unit Tests,
Coverage, SPDX headers, CodeQL (see the PR's Checks tab). The local
check noted above is a subset; the broader mandated gates (full workspace test
suite, codegen/scan, Android, Frontend) run in CI, not locally — none is
silently skipped.

`verify_spv_proof` shifted `idx` right once per sibling but never checked
that all index bits were consumed. Any high bits beyond `siblings.len()`
levels were silently dropped, so leaf index `i` and `i + (1 << siblings.len())`
verified against the same root — proof malleability.

Require `idx == 0` after the fold so the index must address exactly the tree
depth the proof provides. Existing single/two-tx proof tests still pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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.

2 participants