0xMiden / 0xMiden/protocol

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

Abierto
#3,538 2 comentarios 0 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
Rust
Estrellas
132
Forks
167
Merge medio
1 d 23 h
PR fusionados (30 d)
110

Descripción

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

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.