From 9195773f65c8b90e7be154fa2622834b48de0296 Mon Sep 17 00:00:00 2001 From: kilodesodiq-arch Date: Sat, 26 Sep 2026 11:14:15 +0100 Subject: [PATCH] fix(proxy): bound the upgrade delay and announce every change `set_upgrade_delay_blocks` checked nothing but the admin. The delay is the entire safety model of the two-step upgrade, so an unbounded setter means a compromised admin can remove the timelock outright. `0` is the sharp end: `set_implementation` computes `upgrade_effective_at = current_block + delay`, and `confirm_implementation` only requires `current_block >= upgrade_effective_at`. With a delay of 0 the two fit in one block, so stage-then-confirm becomes a single transaction and no observer gets a window in which to notice. The delay is now constrained to [MIN_UPGRADE_DELAY_BLOCKS, MAX_UPGRADE_DELAY_BLOCKS]. The cap is not only about "unusably long" delays. A delay anywhere near `u64::MAX` overflows that same addition, and the wrapped result lands in the past -- so a huge delay was never a safe way to freeze upgrades; it silently removed the timelock just as effectively as 0. `set_implementation` now uses `saturating_add` as well, so a saturated sum leaves the upgrade unconfirmable rather than instantly confirmable, which is the safe direction to fail in. Every accepted change emits `UpgradeDelayChanged { old_delay, new_delay, by }`. Previously a delay change left no trace: the only way to observe one was to diff storage around the transaction, so shortening the delay ahead of an upgrade was invisible until the upgrade landed. `InvalidUpgradeDelay` is appended to `ProxyError` so no existing discriminant moves, and the authorisation check still runs first so a non-admin cannot probe the bounds. A pending upgrade's `effective_at` is computed at stage time and is deliberately left alone, so shortening the delay cannot pull an already-staged upgrade forward. The three constants live in this contract rather than in `propchain-traits`, contrary to the issue's suggestion: the crate deliberately has no `propchain-traits` dependency, and a proxy is the last place that should take on one. Closes #1170 Closes #1167 Closes #1168 Closes #1169 --- contracts/proxy/src/lib.rs | 342 ++++++++++++++++++++++++++++++++++++- 1 file changed, 336 insertions(+), 6 deletions(-) diff --git a/contracts/proxy/src/lib.rs b/contracts/proxy/src/lib.rs index 667fc85d4..f8dface02 100644 --- a/contracts/proxy/src/lib.rs +++ b/contracts/proxy/src/lib.rs @@ -20,6 +20,15 @@ //! 3. The implementation address is updated; all subsequent fallback calls //! route to the new implementation. //! +//! ## Upgrade delay bounds +//! +//! The delay is the whole safety model of the two-step upgrade, so it is +//! constrained to `[MIN_UPGRADE_DELAY_BLOCKS, MAX_UPGRADE_DELAY_BLOCKS]` and +//! every change emits `UpgradeDelayChanged`. A delay of `0` would make an +//! upgrade confirmable in the block it was staged in, removing the timelock; +//! an unbounded delay could overflow the `effective_at` computation and, by +//! wrapping into the past, remove it just as effectively. +//! //! ## Call forwarding //! //! The `call_implementation` message forwards a selector + encoded input to the @@ -36,6 +45,37 @@ pub mod propchain_proxy { use ink::env::call::build_call; use ink::prelude::vec::Vec; + /// Default upgrade delay in blocks, applied by the constructor. + /// + /// ~10 minutes at a 6-second block time. Kept identical to the value the + /// constructor hard-coded before the bounds were introduced, so a freshly + /// deployed proxy behaves exactly as it did. + pub const DEFAULT_UPGRADE_DELAY_BLOCKS: u64 = 100; + + /// Smallest upgrade delay an admin may configure. + /// + /// A delay of `0` makes a staged upgrade confirmable in the very block it + /// was staged in, which removes the timelock entirely: a compromised or + /// rogue admin could stage and confirm within one transaction and no + /// observer would get a chance to react. `10` blocks (~1 minute at + /// 6-second blocks) is the floor below which the delay stops being a + /// meaningful review window. + pub const MIN_UPGRADE_DELAY_BLOCKS: u64 = 10; + + /// Largest upgrade delay an admin may configure. + /// + /// 30 days at a 6-second block time, matching `LOCK_PERIOD_30_DAYS` in + /// `propchain-traits`. + /// + /// The cap does more than rule out an "effectively forever" delay. With an + /// unbounded `u64` delay, `set_implementation` evaluates + /// `block_number() + delay`; a delay anywhere near `u64::MAX` overflows + /// that addition, and the wrapped result lands in the past — so the + /// "delayed" upgrade becomes immediately confirmable. A large delay was + /// therefore not even a safe way to disable upgrades; it silently removed + /// the timelock. Bounding the delay removes that path. + pub const MAX_UPGRADE_DELAY_BLOCKS: u64 = 432_000; + /// Errors that the proxy itself can return. #[derive(Debug, Clone, PartialEq, Eq, scale::Encode, scale::Decode)] #[cfg_attr(feature = "std", derive(scale_info::TypeInfo))] @@ -50,6 +90,11 @@ pub mod propchain_proxy { InvalidImplementation, /// The implementation call reverted. ImplementationCallFailed, + /// The requested upgrade delay is outside + /// `[MIN_UPGRADE_DELAY_BLOCKS, MAX_UPGRADE_DELAY_BLOCKS]`. + /// + /// Appended last so no existing discriminant moves. + InvalidUpgradeDelay, } impl From for ProxyError { @@ -90,11 +135,30 @@ pub mod propchain_proxy { new_implementation: AccountId, } + /// Emitted when the admin changes the upgrade delay. + /// + /// Without this, a delay change is invisible to off-chain monitoring: the + /// only way to observe one was to read storage before and after the + /// transaction, so a delay being shortened ahead of an upgrade was + /// invisible until the upgrade landed. + #[ink(event)] + pub struct UpgradeDelayChanged { + /// The delay in effect before this change. + #[ink(topic)] + old_delay: u64, + /// The delay now in effect. + #[ink(topic)] + new_delay: u64, + /// Admin that authorised the change. + by: AccountId, + } + impl TransparentProxy { /// Deploy the proxy with an initial implementation. /// - /// The deployer becomes the admin. The upgrade delay defaults to 100 - /// blocks (~10 minutes on a 6-second chain). + /// The deployer becomes the admin. The upgrade delay starts at + /// [`DEFAULT_UPGRADE_DELAY_BLOCKS`] and can be changed by the admin + /// within `[MIN_UPGRADE_DELAY_BLOCKS, MAX_UPGRADE_DELAY_BLOCKS]`. #[ink(constructor)] pub fn new(implementation: AccountId, admin: AccountId) -> Self { Self { @@ -102,7 +166,7 @@ pub mod propchain_proxy { pending_implementation: AccountId::from([0u8; 32]), upgrade_effective_at: 0, admin, - upgrade_delay_blocks: 100, + upgrade_delay_blocks: DEFAULT_UPGRADE_DELAY_BLOCKS, } } @@ -124,7 +188,13 @@ pub mod propchain_proxy { } let current_block = self.env().block_number() as u64; self.pending_implementation = new_implementation; - self.upgrade_effective_at = current_block + self.upgrade_delay_blocks; + // `saturating_add` rather than `+`: the configured delay is now + // bounded, but a wrapped sum would land `upgrade_effective_at` in + // the past and make this "delayed" upgrade confirmable immediately, + // which is the exact failure the timelock exists to prevent. If the + // sum would ever saturate, the upgrade stays unconfirmable, which + // is the safe direction to fail in. + self.upgrade_effective_at = current_block.saturating_add(self.upgrade_delay_blocks); self.env().emit_event(UpgradeStaged { new_implementation, @@ -161,15 +231,35 @@ pub mod propchain_proxy { /// Update the upgrade delay (admin only). /// - /// Only affects future `set_implementation` calls; a pending upgrade's - /// `effective_at` is not retroactively changed. + /// The new delay must be within + /// `[MIN_UPGRADE_DELAY_BLOCKS, MAX_UPGRADE_DELAY_BLOCKS]`. Only + /// affects future `set_implementation` calls; a pending upgrade's + /// `effective_at` is not retroactively changed, so shortening the delay + /// cannot pull an already-staged upgrade forward. #[ink(message)] pub fn set_upgrade_delay_blocks(&mut self, new_delay: u64) -> Result<(), ProxyError> { self.ensure_admin()?; + if !(MIN_UPGRADE_DELAY_BLOCKS..=MAX_UPGRADE_DELAY_BLOCKS).contains(&new_delay) { + return Err(ProxyError::InvalidUpgradeDelay); + } + let old_delay = self.upgrade_delay_blocks; self.upgrade_delay_blocks = new_delay; + + self.env().emit_event(UpgradeDelayChanged { + old_delay, + new_delay, + by: self.env().caller(), + }); Ok(()) } + /// Read-only accessor for the delay bounds, so off-chain tooling and + /// UIs can validate a proposed value without hard-coding the numbers. + #[ink(message)] + pub fn upgrade_delay_bounds(&self) -> (u64, u64) { + (MIN_UPGRADE_DELAY_BLOCKS, MAX_UPGRADE_DELAY_BLOCKS) + } + // ── Public read-only messages ───────────────────────────────────── /// Return the current implementation address. @@ -246,12 +336,29 @@ pub mod propchain_proxy { #[cfg(test)] mod tests { + use scale::{Decode, Encode}; + use super::*; fn accounts() -> ink::env::test::DefaultAccounts { ink::env::test::default_accounts::() } + /// A proxy owned by `alice`, with the caller set to `alice`. + fn proxy_owned_by_alice() -> ( + ink::env::test::DefaultAccounts, + TransparentProxy, + ) { + let accounts = accounts(); + ink::env::test::set_caller::(accounts.alice); + let proxy = TransparentProxy::new(accounts.bob, accounts.alice); + (accounts, proxy) + } + + fn block_number() -> u64 { + ink::env::block_number::() as u64 + } + #[ink::test] fn constructor_sets_fields() { let accounts = accounts(); @@ -338,6 +445,229 @@ pub mod propchain_proxy { assert_eq!(proxy.upgrade_delay_blocks(), 50); } + // ── #1170: upgrade delay bounds ────────────────────────────────── + + #[ink::test] + fn constructor_default_delay_is_within_bounds() { + let (_accounts, proxy) = proxy_owned_by_alice(); + let (min, max) = proxy.upgrade_delay_bounds(); + assert_eq!( + proxy.upgrade_delay_blocks(), + DEFAULT_UPGRADE_DELAY_BLOCKS + ); + assert!((min..=max).contains(&proxy.upgrade_delay_blocks())); + } + + #[ink::test] + fn delay_bounds_are_ordered_and_exclude_zero() { + let (_accounts, proxy) = proxy_owned_by_alice(); + let (min, max) = proxy.upgrade_delay_bounds(); + assert!(min > 0, "a zero delay would remove the timelock"); + assert!(min <= DEFAULT_UPGRADE_DELAY_BLOCKS); + assert!(DEFAULT_UPGRADE_DELAY_BLOCKS <= max); + } + + #[ink::test] + fn zero_delay_is_rejected() { + let (_accounts, mut proxy) = proxy_owned_by_alice(); + assert_eq!( + proxy.set_upgrade_delay_blocks(0), + Err(ProxyError::InvalidUpgradeDelay) + ); + assert_eq!(proxy.upgrade_delay_blocks(), DEFAULT_UPGRADE_DELAY_BLOCKS); + } + + #[ink::test] + fn delay_below_the_minimum_is_rejected() { + let (_accounts, mut proxy) = proxy_owned_by_alice(); + for delay in 1..MIN_UPGRADE_DELAY_BLOCKS { + assert_eq!( + proxy.set_upgrade_delay_blocks(delay), + Err(ProxyError::InvalidUpgradeDelay), + "delay {delay} is below the minimum and must be rejected" + ); + } + assert_eq!(proxy.upgrade_delay_blocks(), DEFAULT_UPGRADE_DELAY_BLOCKS); + } + + #[ink::test] + fn delay_at_the_minimum_is_accepted() { + let (_accounts, mut proxy) = proxy_owned_by_alice(); + assert!(proxy + .set_upgrade_delay_blocks(MIN_UPGRADE_DELAY_BLOCKS) + .is_ok()); + assert_eq!(proxy.upgrade_delay_blocks(), MIN_UPGRADE_DELAY_BLOCKS); + } + + #[ink::test] + fn delay_at_the_maximum_is_accepted() { + let (_accounts, mut proxy) = proxy_owned_by_alice(); + assert!(proxy + .set_upgrade_delay_blocks(MAX_UPGRADE_DELAY_BLOCKS) + .is_ok()); + assert_eq!(proxy.upgrade_delay_blocks(), MAX_UPGRADE_DELAY_BLOCKS); + } + + #[ink::test] + fn delay_above_the_maximum_is_rejected() { + let (_accounts, mut proxy) = proxy_owned_by_alice(); + for delay in [MAX_UPGRADE_DELAY_BLOCKS + 1, 1_000_000, u64::MAX] { + assert_eq!( + proxy.set_upgrade_delay_blocks(delay), + Err(ProxyError::InvalidUpgradeDelay), + "delay {delay} is above the maximum and must be rejected" + ); + } + assert_eq!(proxy.upgrade_delay_blocks(), DEFAULT_UPGRADE_DELAY_BLOCKS); + } + + /// The reason the maximum exists: an unbounded delay made + /// `current_block + delay` overflow, wrapping `upgrade_effective_at` + /// into the past and making the "delayed" upgrade confirmable + /// immediately. Rejecting the value is what closes that path. + #[ink::test] + fn absurd_delay_cannot_wrap_the_effective_block() { + let (accounts, mut proxy) = proxy_owned_by_alice(); + assert!(proxy.set_upgrade_delay_blocks(u64::MAX).is_err()); + + let current = block_number(); + proxy.set_implementation(accounts.charlie).unwrap(); + assert!( + proxy.upgrade_effective_at() > current, + "effective_at must stay in the future, never wrap into the past" + ); + } + + #[ink::test] + fn staging_with_the_maximum_delay_does_not_wrap() { + let (accounts, mut proxy) = proxy_owned_by_alice(); + proxy + .set_upgrade_delay_blocks(MAX_UPGRADE_DELAY_BLOCKS) + .unwrap(); + + let current = block_number(); + proxy.set_implementation(accounts.charlie).unwrap(); + assert_eq!( + proxy.upgrade_effective_at(), + current + MAX_UPGRADE_DELAY_BLOCKS + ); + assert!(proxy.upgrade_effective_at() > current); + } + + #[ink::test] + fn rejected_delay_leaves_the_stored_value_untouched() { + let (_accounts, mut proxy) = proxy_owned_by_alice(); + proxy.set_upgrade_delay_blocks(200).unwrap(); + for bad in [0, MIN_UPGRADE_DELAY_BLOCKS - 1, MAX_UPGRADE_DELAY_BLOCKS + 1] { + assert!(proxy.set_upgrade_delay_blocks(bad).is_err()); + assert_eq!(proxy.upgrade_delay_blocks(), 200); + } + } + + #[ink::test] + fn non_admin_cannot_change_the_delay() { + let accounts = accounts(); + let mut proxy = TransparentProxy::new(accounts.bob, accounts.alice); + + ink::env::test::set_caller::(accounts.bob); + assert_eq!( + proxy.set_upgrade_delay_blocks(50), + Err(ProxyError::Unauthorized) + ); + assert_eq!(proxy.upgrade_delay_blocks(), DEFAULT_UPGRADE_DELAY_BLOCKS); + } + + /// Authorisation is checked before the bounds, so a non-admin cannot + /// distinguish "out of range" from "out of range but only for admins". + #[ink::test] + fn authorisation_is_checked_before_the_bounds() { + let accounts = accounts(); + let mut proxy = TransparentProxy::new(accounts.bob, accounts.alice); + + ink::env::test::set_caller::(accounts.bob); + assert_eq!( + proxy.set_upgrade_delay_blocks(0), + Err(ProxyError::Unauthorized) + ); + assert_eq!( + proxy.set_upgrade_delay_blocks(u64::MAX), + Err(ProxyError::Unauthorized) + ); + } + + #[ink::test] + fn changing_the_delay_emits_an_event() { + let (accounts, mut proxy) = proxy_owned_by_alice(); + proxy.set_upgrade_delay_blocks(50).unwrap(); + + let events = ink::env::test::recorded_events::(); + assert_eq!(events.len(), 1, "exactly one event per delay change"); + let event = &events[0]; + // `old_delay` and `new_delay` are indexed topics. ink! encodes a + // numeric topic as the plain SCALE encoding of the value, so + // compare against `Encode` of the expected number rather than + // hand-assembling bytes. `contains` is used because topics[0] is + // the event selector, and the field order within the remaining + // topics is not what this test is asserting. + assert!(event + .topics + .contains(&DEFAULT_UPGRADE_DELAY_BLOCKS.encode())); + assert!(event.topics.contains(&50u64.encode())); + // `by` is the only non-topic field, so the payload is the admin. + let by = AccountId::decode(&mut &event.data[..]).unwrap(); + assert_eq!(by, accounts.alice); + } + + #[ink::test] + fn a_rejected_delay_emits_no_event() { + let (_accounts, mut proxy) = proxy_owned_by_alice(); + assert!(proxy.set_upgrade_delay_blocks(0).is_err()); + assert!(proxy + .set_upgrade_delay_blocks(MAX_UPGRADE_DELAY_BLOCKS + 1) + .is_err()); + assert_eq!( + ink::env::test::recorded_events::().len(), + 0, + "a rejected change must not announce a new delay" + ); + } + + #[ink::test] + fn a_delay_change_records_both_the_old_and_the_new_value() { + let (_accounts, mut proxy) = proxy_owned_by_alice(); + proxy.set_upgrade_delay_blocks(MIN_UPGRADE_DELAY_BLOCKS).unwrap(); + proxy.set_upgrade_delay_blocks(500).unwrap(); + + let events = ink::env::test::recorded_events::(); + assert_eq!(events.len(), 2); + // First change: default -> minimum. + assert!(events[0] + .topics + .contains(&DEFAULT_UPGRADE_DELAY_BLOCKS.encode())); + assert!(events[0] + .topics + .contains(&MIN_UPGRADE_DELAY_BLOCKS.encode())); + // Second change: minimum -> 500. + assert!(events[1] + .topics + .contains(&MIN_UPGRADE_DELAY_BLOCKS.encode())); + assert!(events[1].topics.contains(&500u64.encode())); + } + + /// The delay a change is allowed to affect is only the *next* staged + /// upgrade: an already-staged `effective_at` is computed at stage time + /// and must not move, so shortening the delay cannot pull a pending + /// upgrade forward. + #[ink::test] + fn changing_the_delay_does_not_retroactively_move_a_pending_upgrade() { + let (accounts, mut proxy) = proxy_owned_by_alice(); + proxy.set_implementation(accounts.charlie).unwrap(); + let staged_at = proxy.upgrade_effective_at(); + + proxy.set_upgrade_delay_blocks(MIN_UPGRADE_DELAY_BLOCKS).unwrap(); + assert_eq!(proxy.upgrade_effective_at(), staged_at); + } + #[ink::test] fn zero_address_rejected() { let accounts = accounts();