0xMiden / 0xMiden/protocol

AccountDelta::read_from skips the full-state non-Create-op check that AccountDelta::new enforces

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

説明

`AccountDelta::new` enforces two invariants; `AccountDelta::read_from` only enforces one of them.

Same class as #3494 / #3533 / #3535: a validating constructor exists, but the `Deserializable` impl doesn't fully route through it. This one is a partial miss rather than a total one, which is what made it easy to overlook.

`new` (`crates/miden-protocol/src/account/delta/mod.rs:86`) checks:
1. `nonce_delta` is non-zero if storage or vault were updated (via `validate_nonce`)
2. if `code` is `Some` (a "full state" delta), the storage patch contains only `Create` ops, no `Update`/`Remove` (`storage.contains_non_create_ops()`)

`read_from` (same file, ~line 444) calls `validate_nonce`, so check 1 is enforced, but never checks `contains_non_create_ops`, so check 2 is silently skipped.

This matters because of the documented invariant in `impl TryFrom<&AccountDelta> for Account`: "A full state delta consists of `Create` slot patches, so applying it to empty storage reconstructs the account's full storage" (line ~370). A deserialized `AccountDelta` with `code = Some(..)` and a non-Create op in its storage patch violates that assumption.

I wrote a test confirming all of this end to end: `new()` rejects a full-state delta with an `Update` op (`FullStateDeltaContainsNonCreateOp`), the same delta constructed directly and round-tripped through `to_bytes()`/`read_from_bytes()` deserializes successfully, and then feeding that into `Account::try_from` does correctly fail, because `AccountStorage::apply_patch` looks up the target slot by name before updating/removing it, and on empty storage it's never there (`StorageSlotNameNotFound`).

Severity: lower than it looks, and I'd rather say so than overstate it. In this specific call site the type's own guarantee happens to be backstopped by a downstream check. But `is_full_state()` is a cheap boolean based only on `code.is_some()`, documented elsewhere as implying "storage patch is Create-only." Any current or future caller that relies on that documented implication without going through `apply_patch` (which happens to catch it) wouldn't be protected. Having `read_from` enforce the same invariant `new()` does would make the type's own guarantee hold unconditionally, rather than depending on this one downstream check happening to catch it.

Happy to send a PR for this if it can be assigned to me — I already have a working regression test for it.

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

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

評価

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

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

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