0xMiden / 0xMiden/protocol

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

Abierto
#3,544 0 comentarios 0 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
Rust
Estrellas
132
Forks
167
Merge medio
1 d 23 h
PR fusionados (30 d)
110

Descripción

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

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.