0xMiden / 0xMiden/protocol

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

Open
#3,544 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
132
Forks
167
Avg merge
1d 23h
Merged PRs (30d)
110

Description

### 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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.