diff --git a/docs/docs-developers/docs/aztec-nr/framework-description/state_variables.md b/docs/docs-developers/docs/aztec-nr/framework-description/state_variables.md index a4f96f8bdb64..fc9c522767b9 100644 --- a/docs/docs-developers/docs/aztec-nr/framework-description/state_variables.md +++ b/docs/docs-developers/docs/aztec-nr/framework-description/state_variables.md @@ -174,10 +174,16 @@ The existence of minimum delays means that a private function that reads a publi #### Declaration -Unlike other state variables, `DelayedPublicMutable` receives not only a type parameter for the underlying datatype, but also a `DELAY` type parameter with the value change delay as a number of seconds. +Unlike other state variables, `DelayedPublicMutable` receives not only a type parameter for the underlying datatype, but also a `DELAY` type parameter with the value change delay as a number of seconds. Delays must be greater than zero, both here and when scheduling a delay change: with no delay the value could change at any moment, leaving no window in which a private read is valid. #include_code delayed_public_mutable_storage /noir-projects/labs/noir-contracts/contracts/app/auth_contract/src/main.nr rust +#### Choosing delays + +The zero check is the only bound that is enforced; delays above zero are accepted but not necessarily safe. A transaction that privately reads the value expires `DELAY` seconds after its anchor block timestamp, so a delay of a few seconds is just as unusable in practice: no transaction can be proven, broadcast and included that quickly. Short delays also lower the transaction's expiration timestamp, shrinking its privacy set. There is no universal minimum that is safe for every use case, but a delay of at least a couple of hours is recommended. + +Delay selection is also a power in itself. An account that can schedule delay changes can drive the delay low enough that private reads become unusable, blocking user actions that depend on them, which is a subtle escalation of privileges: the delay is precisely what protects users from whoever controls the value. If your contract lets an admin change the delay, enforce a minimum appropriate to your use case in the function that schedules the change. + #### `schedule_value_change` This is the means by which a `DelayedPublicMutable` variable mutates its contents. It schedules a value change for the variable at a future timestamp after the `DELAY` has elapsed. diff --git a/noir-projects/fnd/noir-contracts/contracts/protocol/aztec_sublib/src/state_vars/delayed_public_mutable.nr b/noir-projects/fnd/noir-contracts/contracts/protocol/aztec_sublib/src/state_vars/delayed_public_mutable.nr index a72ccc13fefb..1c35efdbd703 100644 --- a/noir-projects/fnd/noir-contracts/contracts/protocol/aztec_sublib/src/state_vars/delayed_public_mutable.nr +++ b/noir-projects/fnd/noir-contracts/contracts/protocol/aztec_sublib/src/state_vars/delayed_public_mutable.nr @@ -73,7 +73,20 @@ use crate::{context::{PrivateContext, PublicContext, UtilityContext}, state_vars /// /// Additionally, a lower `expiration_timestamp` obviously causes transactions to expire earlier, resulting in /// multiple issues. Among others, this can make large transactions that take long to prove be unfeasible, restrict -/// users with slow proving devices, and force large transaction fees to guarantee fast inclusion. +/// users with slow proving devices, and force large transaction fees to guarantee fast inclusion. At the limit, a zero +/// delay leaves no window at all in which a read is known to hold, making private reads impossible. Zero delays are +/// therefore rejected outright, both when declaring the state variable and when scheduling a delay change. +/// +/// The zero check is the only bound that is enforced: it rejects a configuration that can never work, not one that +/// is merely unwise. A delay of a few seconds is accepted by the compiler despite being just as unusable in practice, +/// since no transaction can be proven, broadcast and included that quickly. There is no universal minimum that is +/// safe for every use case: proving times, user hardware and the nature of the guarded value all factor in. +/// +/// Delay selection is also a power in itself. An account that can schedule delay changes can drive the delay low +/// enough that private reads become unusable, blocking user actions that depend on them: a subtle escalation of +/// privileges, since the delay is precisely what protects users from whoever controls the value. Contracts that let a +/// privileged account call [`DelayedPublicMutable::schedule_delay_change`] should enforce a minimum delay appropriate +/// to their use case in that function. /// /// In practice, a delay of at least a couple hours is recommended. From a privacy point of view the optimal delay is /// [`crate::protocol::constants::MAX_TX_LIFETIME`], which puts contracts in the same privacy set as those that do not @@ -129,6 +142,7 @@ where DelayedPublicMutableValues: Packable, { fn new(context: Context, storage_slot: Field) -> Self { + std::static_assert(InitialDelay > 0, "InitialDelay must be greater than zero"); assert(storage_slot != 0, "Storage slot 0 not allowed. Storage slots must start from 1."); Self { context, storage_slot } } @@ -211,6 +225,9 @@ where /// [`get_current_delay`](DelayedPublicMutable::get_current_delay) automatically begins to return `new_delay`, and /// [`schedule_value_change`](DelayedPublicMutable::schedule_value_change) begins using it. /// + /// `new_delay` must be greater than zero. This is the only enforced bound; values above zero are not necessarily + /// safe: see the Choosing Delays section on [`DelayedPublicMutable`] for guidance. + /// /// ## Multiple Scheduled Changes /// /// Only a **single** delay can be scheduled to become the new delay at a given point in time. Any prior scheduled diff --git a/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/delayed_public_mutable_values.nr b/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/delayed_public_mutable_values.nr index f77e674e51f3..394f610d8e0c 100644 --- a/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/delayed_public_mutable_values.nr +++ b/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/delayed_public_mutable_values.nr @@ -56,6 +56,8 @@ where pub fn unpack_delay_change( packed: Field, ) -> ScheduledDelayChange { + std::static_assert(INITIAL_DELAY > 0, "INITIAL_DELAY must be greater than zero"); + // This function expects to be called with just the first field of the packed representation, which contains sdc // and svc timestamp_of_change. We'll discard the svc component. let svc_timestamp_of_change = packed as u32; diff --git a/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/delayed_public_mutable_values/test.nr b/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/delayed_public_mutable_values/test.nr index 123710fc12ac..cb264c80ebdd 100644 --- a/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/delayed_public_mutable_values/test.nr +++ b/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/delayed_public_mutable_values/test.nr @@ -1,7 +1,8 @@ use crate::{constants::{MAX_FIELD_VALUE, MAX_U32_VALUE}, traits::Packable}; use crate::delayed_public_mutable::{ - delayed_public_mutable_values::DelayedPublicMutableValues, - scheduled_delay_change::ScheduledDelayChange, scheduled_value_change::ScheduledValueChange, + delayed_public_mutable_values::{DelayedPublicMutableValues, unpack_delay_change}, + scheduled_delay_change::ScheduledDelayChange, + scheduled_value_change::ScheduledValueChange, }; global TEST_INITIAL_DELAY: u64 = 13; @@ -150,6 +151,12 @@ unconstrained fn schedule_change_accepts_delay_at_u32_max() { assert_eq(sdc.post.unwrap(), max_u32); } +// Every read of a stored delay goes through `unpack_delay_change` +#[test(should_fail_with = "INITIAL_DELAY must be greater than zero")] +unconstrained fn unpacking_a_zero_initial_delay_fails() { + let _ = unpack_delay_change::<0_u64>(0); +} + #[test] unconstrained fn packed_delayed_public_mutable_values_match_typescript() { let pre_value = MockStruct { a: 1, b: 2 }; diff --git a/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/scheduled_delay_change.nr b/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/scheduled_delay_change.nr index 197196802bbb..8ca9b310b298 100644 --- a/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/scheduled_delay_change.nr +++ b/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/scheduled_delay_change.nr @@ -9,6 +9,9 @@ mod test; // is performed via `schedule_change` in order to satisfy ScheduleValueChange constraints: if e.g. we allowed for the // delay to be decreased immediately then it'd be possible for the state variable to schedule a value change with a // reduced delay, invalidating prior private reads. +// INITIAL_DELAY must be nonzero, since a zero delay leaves no window during which a private read is known to remain +// valid. DelayedPublicMutable rejects it when the state variable is declared, and `unpack_delay_change` guards every +// read of a stored delay, which also covers readers that do not go through a state variable. pub struct ScheduledDelayChange { // Both pre and post are stored in public storage, so by default they are zeroed. By wrapping them in an Option, // they default to Option::none(), which we detect and replace with INITIAL_DELAY. The end result is that a @@ -52,7 +55,12 @@ impl ScheduledDelayChange { /// - when reducing the delay, the change will take effect after a delay equal to the difference between old and /// new delay. For example, if reducing from 3 days to 1 day, the reduction will be scheduled to happen after 2 /// days. + /// + /// The new delay must be strictly positive: a zero delay would let the value change at any moment, so there'd be no + /// window during which a private read is known to remain valid. pub fn schedule_change(&mut self, new: u64, current_timestamp: u64) { + assert(new > 0, "Delay must be greater than zero"); + let current = self.get_current(current_timestamp); // When changing the delay value we must ensure that it is not possible to produce a value change with a delay diff --git a/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/scheduled_delay_change/test.nr b/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/scheduled_delay_change/test.nr index 438c057194e8..c8922750900d 100644 --- a/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/scheduled_delay_change/test.nr +++ b/noir-projects/fnd/noir-protocol-circuits/crates/types/src/delayed_public_mutable/scheduled_delay_change/test.nr @@ -161,6 +161,13 @@ unconstrained fn test_schedule_change_to_longer_delay_from_initial() { assert_eq(delay_change.get_current(current_timestamp), new); } +#[test(should_fail_with = "Delay must be greater than zero")] +unconstrained fn test_schedule_change_to_zero_delay_fails() { + let mut delay_change = get_initial_delay_change(); + + delay_change.schedule_change(0, 50); +} + unconstrained fn assert_effective_minimum_delay_invariants( delay_change: &mut ScheduledDelayChange, anchor_block_timestamp: u64, @@ -191,11 +198,11 @@ unconstrained fn assert_effective_minimum_delay_invariants: Packable, { fn new(context: Context, storage_slot: Field) -> Self { + std::static_assert(InitialDelay > 0, "InitialDelay must be greater than zero"); assert(storage_slot != 0, "Storage slot 0 not allowed. Storage slots must start from 1."); Self { context, storage_slot } } @@ -213,6 +227,9 @@ where /// [`get_current_delay`](DelayedPublicMutable::get_current_delay) automatically begins to return `new_delay`, and /// [`schedule_value_change`](DelayedPublicMutable::schedule_value_change) begins using it. /// + /// `new_delay` must be greater than zero. This is the only enforced bound; values above zero are not necessarily + /// safe: see the Choosing Delays section on [`DelayedPublicMutable`] for guidance. + /// /// ## Multiple Scheduled Changes /// /// Only a **single** delay can be scheduled to become the new delay at a given point in time. Any prior scheduled diff --git a/noir-projects/labs/aztec-nr/aztec/src/state_vars/delayed_public_mutable/test.nr b/noir-projects/labs/aztec-nr/aztec/src/state_vars/delayed_public_mutable/test.nr index 8a190319d3fc..ba97268aa0b7 100644 --- a/noir-projects/labs/aztec-nr/aztec/src/state_vars/delayed_public_mutable/test.nr +++ b/noir-projects/labs/aztec-nr/aztec/src/state_vars/delayed_public_mutable/test.nr @@ -33,6 +33,12 @@ unconstrained fn in_utility( DelayedPublicMutable::new(context, storage_slot) } +// The `static_assert` failure surfaces as a test failure because each test is compiled on demand. +#[test(should_fail_with = "InitialDelay must be greater than zero")] +unconstrained fn declaring_with_zero_initial_delay_fails() { + let _: DelayedPublicMutable = DelayedPublicMutable::new(zeroed(), storage_slot); +} + #[test] unconstrained fn get_current_value_in_public_initial() { let env = TestEnvironment::new();