0xMiden / 0xMiden/protocol

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

Đang mở
#3,538 2 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Rust
Star
132
Fork
167
Merge trung bình
1 ngày 23 giờ
Pull request đã merge (30 ngày)
110

Mô tả

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

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.