0xMiden / 0xMiden/protocol

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

Open
#3,538 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
132
Forks
167
Avg merge
1d 23h
Merged PRs (30d)
110

Description

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

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.