0xMiden / 0xMiden/protocol

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

オープン
#3,544 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る
主要言語
Rust
スター
132
フォーク
167
平均マージ
1日 23時間
マージ済み PR(30日)
110

説明

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

コントリビューションガイド

コントリビューションガイドを開く

評価

この issue はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。