0xMiden / 0xMiden/protocol

`NoteInclusionProof` never validates `block_note_tree_index` against `note_path.depth()`, so `authenticated_nodes` can panic

Offen
#3,544 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
Vorherrschende Sprache
Rust
Sterne
132
Forks
167
Ø Merge
1 T. 23 Std.
Gemergte PRs (30 T.)
110

Beschreibung

### Summary

`NoteInclusionProof::authenticated_nodes` relies on an invariant that neither `new` nor `read_from` enforces, and its `SAFETY` comment claims the opposite:

https://github.com/0xMiden/protocol/blob/134cc1fd305943ac6d9bda591dc4b87bf8046d25/crates/miden-protocol/src/note/location.rs#L91-L96

`NoteInclusionProof::new` performs exactly one check:

https://github.com/0xMiden/protocol/blob/134cc1fd305943ac6d9bda591dc4b87bf8046d25/crates/miden-protocol/src/note/location.rs#L57-L70

With `BLOCK_NOTE_TREE_DEPTH = 16` and `BATCH_NOTE_TREE_DEPTH = 10`, `HIGHEST_INDEX` works out to `64 * 1024 - 1 = 65535`, which is exactly `u16::MAX`. Since `block_note_tree_index` is a `u16`, that comparison can never be true, so the only check in the constructor is dead code.

Nothing validates `block_note_tree_index` against `note_path.depth()`, and `Deserializable::read_from` does not route through `new` at all:

https://github.com/0xMiden/protocol/blob/134cc1fd305943ac6d9bda591dc4b87bf8046d25/crates/miden-protocol/src/note/location.rs#L125-L132

### Why this leads to a panic

`SparseMerklePath::authenticated_nodes` builds its iterator via `NodeIndex::new(self.depth(), index)`, which returns `Err` when `index >= 2^depth`. `NoteInclusionProof::authenticated_nodes` unwraps that with `.expect("note index is not out of bounds")`.

I verified the underlying `Err` against `miden-crypto` locally (all assertions pass):

- `SparseMerklePath::default().depth() == 0`
- `path.authenticated_nodes(0, _)` is `Ok`, but `path.authenticated_nodes(1, _)` is `Err`, since `1 >= 2^0`
- `SparseMerklePath::from_parts(0, vec![Word::default(); 5])` yields depth 5, and `authenticated_nodes(1000, _)` is `Err` because `1000 >= 2^5` - even though `1000` is a perfectly valid `block_note_tree_index` for a 16-deep block note tree

Since that `Err` is reachable and `authenticated_nodes` unwraps it, any `NoteInclusionProof` whose `note_path` depth does not match its `block_note_tree_index` panics the first time the proof is used, instead of being rejected at construction or deserialization time.

### Reachability

Proofs are constructed directly from RPC responses, with no depth check on the converted path:

https://github.com/0xMiden/rust-sdk/blob/d6a48a45aff40977fab4abb54c4f985f6742b401/crates/rust-client/src/rpc/domain/note.rs#L179-L197

and `authenticated_nodes` is called while assembling transaction advice inputs:

https://github.com/0xMiden/protocol/blob/134cc1fd305943ac6d9bda591dc4b87bf8046d25/crates/miden-protocol/src/transaction/kernel/advice_inputs.rs#L383

So a malformed or malicious `NoteInclusionInBlockProof` reaches a client-side panic rather than a typed error.

### Expected behaviour

`new` should reject a `block_note_tree_index` that is inconsistent with `note_path.depth()` (the path depth should be `BLOCK_NOTE_TREE_DEPTH`), and `read_from` should enforce the same invariant, returning `DeserializationError::InvalidValue`. Once both construction paths guarantee it, the `.expect` in `authenticated_nodes` becomes genuinely safe and its `SAFETY` comment becomes accurate.

Note that the existing `HIGHEST_INDEX` check is dead code as written, so fixing this also removes a misleading bound.

### Notes

Same class as 0xMiden/miden-vm#3277 (merged), and #3494, #3533, #3535, #3538 in this repo: a validating constructor exists, but the invariant is not enforced on every construction path.

A fix with regression tests is open as #3545. I put it up rather than holding it back so the diff is reviewable alongside this report; per CONTRIBUTING I am not asking for it to be merged ahead of this issue being assigned, and I am happy to close it if you would rather handle this internally.

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.