`NoteInclusionProof` never validates `block_note_tree_index` against `note_path.depth()`, so `authenticated_nodes` can panic
- 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
Evaluación
Este issue todavía no se ha evaluado.