From bd70a1f42d18ebaade6d893d2c4794b4adce3bea Mon Sep 17 00:00:00 2001 From: Remo Date: Thu, 16 Apr 2026 19:35:45 +0300 Subject: [PATCH 1/4] replace_to post upgrade upgradability validation --- .../utils/src/components/replaceability.cairo | 3 + .../components/replaceability/interface.cairo | 8 + .../components/replaceability/mock_v2.cairo | 58 +++++ .../replaceability/replaceability.cairo | 112 ++++++++- .../src/components/replaceability/test.cairo | 229 +++++++++++++++++- .../replaceability/test_utils.cairo | 14 +- 6 files changed, 407 insertions(+), 17 deletions(-) create mode 100644 packages/utils/src/components/replaceability/mock_v2.cairo diff --git a/packages/utils/src/components/replaceability.cairo b/packages/utils/src/components/replaceability.cairo index 93b81904..159834d1 100644 --- a/packages/utils/src/components/replaceability.cairo +++ b/packages/utils/src/components/replaceability.cairo @@ -13,6 +13,9 @@ mod eic_test_contract; #[cfg(test)] pub(crate) mod mock; +#[cfg(test)] +pub(crate) mod mock_v2; + #[cfg(test)] mod test; diff --git a/packages/utils/src/components/replaceability/interface.cairo b/packages/utils/src/components/replaceability/interface.cairo index 58c30e2b..c3a70f06 100644 --- a/packages/utils/src/components/replaceability/interface.cairo +++ b/packages/utils/src/components/replaceability/interface.cairo @@ -1,5 +1,9 @@ use starknet::class_hash::ClassHash; +/// Sentinel panic value used by `validate_upgradeability` to signal a successful dry-run. +/// Any other panic from a dry-run is treated as a real failure. +pub(crate) const UPGRADEABILITY_VALIDATION_SUCCESS: felt252 = 'UPGRADEABILITY_OK'; + /// Holds EIC data. /// * eic_hash is the EIC class hash. /// * eic_init_data is a span of the EIC init args. @@ -39,8 +43,12 @@ pub trait IReplaceable { self: @TContractState, implementation_data: ImplementationData, ) -> u64; fn add_new_implementation(ref self: TContractState, implementation_data: ImplementationData); + fn add_new_implementation_unsafe( + ref self: TContractState, implementation_data: ImplementationData, + ); fn remove_implementation(ref self: TContractState, implementation_data: ImplementationData); fn replace_to(ref self: TContractState, implementation_data: ImplementationData); + fn validate_upgradeability(ref self: TContractState, impl_hash: ClassHash); } #[derive(Copy, Drop, PartialEq, starknet::Event)] diff --git a/packages/utils/src/components/replaceability/mock_v2.cairo b/packages/utils/src/components/replaceability/mock_v2.cairo new file mode 100644 index 00000000..1d7303d1 --- /dev/null +++ b/packages/utils/src/components/replaceability/mock_v2.cairo @@ -0,0 +1,58 @@ +// Functionally identical to `ReplaceabilityMock` but compiles to a different class hash +// (via the unused `_v2_marker` field). Used as a valid upgrade target in tests where the +// target must include the replaceability component yet differ from the deployed contract. +// +// IMPORTANT: keep this in lockstep with `mock.cairo`. The validation tests assume the two +// classes share the same upgrade machinery; any change to the components, storage layout, or +// constructor of `ReplaceabilityMock` must be mirrored here. +#[starknet::contract] +pub(crate) mod ReplaceabilityMockV2 { + use CommonRolesComponent::InternalTrait as CommonRolesInternalTrait; + use openzeppelin::access::accesscontrol::AccessControlComponent; + use openzeppelin::introspection::src5::SRC5Component; + use starknet::ContractAddress; + use starkware_utils::components::common_roles::CommonRolesComponent; + use starkware_utils::components::replaceability::ReplaceabilityComponent; + use starkware_utils::components::replaceability::ReplaceabilityComponent::InternalReplaceabilityTrait; + + component!(path: ReplaceabilityComponent, storage: replaceability, event: ReplaceabilityEvent); + component!(path: CommonRolesComponent, storage: common_roles, event: CommonRolesEvent); + component!(path: AccessControlComponent, storage: accesscontrol, event: AccessControlEvent); + component!(path: SRC5Component, storage: src5, event: SRC5Event); + + #[storage] + struct Storage { + #[substorage(v0)] + replaceability: ReplaceabilityComponent::Storage, + #[substorage(v0)] + common_roles: CommonRolesComponent::Storage, + #[substorage(v0)] + accesscontrol: AccessControlComponent::Storage, + #[substorage(v0)] + src5: SRC5Component::Storage, + // Extra field to produce a different class hash from ReplaceabilityMock. + _v2_marker: bool, + } + + #[event] + #[derive(Drop, starknet::Event)] + pub(crate) enum Event { + ReplaceabilityEvent: ReplaceabilityComponent::Event, + CommonRolesEvent: CommonRolesComponent::Event, + AccessControlEvent: AccessControlComponent::Event, + SRC5Event: SRC5Component::Event, + } + + #[constructor] + fn constructor(ref self: ContractState, upgrade_delay: u64, governance_admin: ContractAddress) { + self.common_roles.initialize(:governance_admin); + self.replaceability.initialize(:upgrade_delay); + } + + #[abi(embed_v0)] + impl ReplaceabilityImpl = + ReplaceabilityComponent::ReplaceabilityImpl; + + #[abi(embed_v0)] + impl CommonRolesImpl = CommonRolesComponent::CommonRolesImpl; +} diff --git a/packages/utils/src/components/replaceability/replaceability.cairo b/packages/utils/src/components/replaceability/replaceability.cairo index 93f10c76..9b0a8e93 100644 --- a/packages/utils/src/components/replaceability/replaceability.cairo +++ b/packages/utils/src/components/replaceability/replaceability.cairo @@ -4,18 +4,23 @@ pub(crate) mod ReplaceabilityComponent { use core::poseidon; use openzeppelin::access::accesscontrol::AccessControlComponent; use openzeppelin::introspection::src5::SRC5Component; - use starknet::get_block_timestamp; + use starknet::class_hash::ClassHash; use starknet::storage::{ Map, StorageMapReadAccess, StorageMapWriteAccess, StoragePointerReadAccess, StoragePointerWriteAccess, }; - use starknet::syscalls::{library_call_syscall, replace_class_syscall}; + use starknet::syscalls::{ + get_class_hash_at_syscall, library_call_syscall, replace_class_syscall, + }; + use starknet::{SyscallResultTrait, get_block_timestamp, get_contract_address}; use starkware_utils::components::common_roles::CommonRolesComponent; use starkware_utils::components::common_roles::CommonRolesComponent::InternalTrait; use starkware_utils::components::replaceability::errors::ReplaceErrors; use starkware_utils::components::replaceability::interface::{ - EIC_INITIALIZE_SELECTOR, IMPLEMENTATION_EXPIRATION, IReplaceable, ImplementationAdded, + EIC_INITIALIZE_SELECTOR, IMPLEMENTATION_EXPIRATION, IReplaceable, + IReplaceableDispatcherTrait, IReplaceableLibraryDispatcher, ImplementationAdded, ImplementationData, ImplementationFinalized, ImplementationRemoved, ImplementationReplaced, + UPGRADEABILITY_VALIDATION_SUCCESS, }; @@ -69,17 +74,39 @@ pub(crate) mod ReplaceabilityComponent { self.impl_activation_time.read(impl_key) } + // Schedules a new implementation and validates that it is upgradeable. If the new + // implementation cannot itself perform a full upgrade cycle (add + replace), the entire + // transaction reverts. Finalized implementations (`final = true`) skip the validation. fn add_new_implementation( ref self: ComponentState, implementation_data: ImplementationData, ) { - // The call is restricted to the upgrade governor. + // The auth check exists in two places. Here it is fail-fast: bail before paying the + // library_call cost of validation. The check inside `_unsafe` below is the one that + // actually enforces the role on direct callers and exercises the target's role + // wiring during the validation dispatch. + let common_roles = get_dep_component!(@self, CommonRoles); + common_roles.only_upgrade_governor(); + + if (!implementation_data.final) { + self.invoke_upgradeability_validation(implementation_data.impl_hash); + } + self.add_new_implementation_unsafe(implementation_data); + } + + // Schedules a new implementation without running upgradeability validation. Bypassing + // this check can permanently brick the contract if the new code lacks a working upgrade + // path — only use when the target has been validated through some other means. + fn add_new_implementation_unsafe( + ref self: ComponentState, implementation_data: ImplementationData, + ) { + // Authoritative auth check for direct callers. Also load-bearing during validation: + // when `validate_upgradeability` dispatches this on the target class, this check is + // what proves the target's upgrade_governor role is wired correctly. let common_roles = get_dep_component!(@self, CommonRoles); common_roles.only_upgrade_governor(); let activation_time = get_block_timestamp() + self.get_upgrade_delay(); let expiration_time = activation_time + IMPLEMENTATION_EXPIRATION; - // TODO(Yaniv, 01/08/2024) - add an assertion that the `implementation_data.impl_hash` - // is declared. self.set_impl_activation_time(:implementation_data, :activation_time); self.set_impl_expiration_time(:implementation_data, :expiration_time); self.emit(ImplementationAdded { implementation_data }); @@ -102,8 +129,8 @@ pub(crate) mod ReplaceabilityComponent { } } - // Replaces the non-finalized current implementation to one that was previously added and - // whose activation time had passed. + // Replaces the class hash to a previously-added implementation whose activation time + // has passed. fn replace_to( ref self: ComponentState, implementation_data: ImplementationData, ) { @@ -159,6 +186,40 @@ pub(crate) mod ReplaceabilityComponent { self.set_impl_activation_time(:implementation_data, activation_time: 0); self.set_impl_expiration_time(:implementation_data, expiration_time: 0); } + + // Dry-run validation that the target class can perform a full upgrade cycle (add + + // replace). Always panics — `UPGRADEABILITY_VALIDATION_SUCCESS` on success, or the + // underlying failure otherwise. Callers wrap this in a library_call so the panic + // reverts all side effects. + // + // Both stages are dispatched to the target class explicitly so this function tests the + // target's own `add_new_implementation_unsafe` and `replace_to` — `_unsafe` is used + // for the add stage to avoid re-invoking validation (recursion). + // + // Coverage is partial: validation only exercises the (add + replace) path with + // `eic_data: None` and `final: false`. Targets that require EIC initialization, or + // whose `remove_implementation` is broken, are not covered. + // + // Threat model: this guards against an upgrade_governor accidentally scheduling a + // non-upgradeable class. It is not a defense against a malicious governor. + fn validate_upgradeability(ref self: ComponentState, impl_hash: ClassHash) { + let implementation_data = ImplementationData { + impl_hash, eic_data: Option::None, final: false, + }; + let dispatcher = IReplaceableLibraryDispatcher { class_hash: impl_hash }; + + // Zero the delay so the dispatched add yields `activation_time = block_timestamp`, + // which then satisfies `replace_to`'s `activation_time <= now` check. Relies on the + // production invariant that `block_timestamp > 0`; tests cheat the timestamp in + // `deploy_replaceability_mock` to bridge snforge's zero default. + self.upgrade_delay.write(0); + dispatcher.add_new_implementation_unsafe(:implementation_data); + dispatcher.replace_to(:implementation_data); + + // Load-bearing: the panic is what reverts `upgrade_delay.write(0)` above and the + // dispatchers' side effects. A normal return here would silently zero the delay. + core::panic_with_felt252(UPGRADEABILITY_VALIDATION_SUCCESS); + } } #[generate_trait] pub impl InternalReplaceabilityImpl< @@ -175,6 +236,41 @@ pub(crate) mod ReplaceabilityComponent { impl PrivateReplaceabilityImpl< TContractState, +HasComponent, +Drop, > of PrivateReplaceabilityTrait { + // Runs `validate_upgradeability` against `new_class_hash` via library_call. Must be + // called while the contract's active class hash is still trusted (i.e., before the + // class replacement happens) — the validation logic itself comes from this active + // class, not the untrusted target. + // + // Returns silently on `UPGRADABILITY_VALIDATION_SUCCESS` (the library call's side + // effects are reverted by the runtime). Any other panic is propagated, reverting the + // caller's transaction. + fn invoke_upgradeability_validation( + ref self: ComponentState, new_class_hash: ClassHash, + ) { + let current_class_hash = get_class_hash_at_syscall(get_contract_address()) + .unwrap_syscall(); + + let mut calldata = array![]; + new_class_hash.serialize(ref calldata); + let result = library_call_syscall( + class_hash: current_class_hash, + function_selector: selector!("validate_upgradeability"), + calldata: calldata.span(), + ); + + match result { + Result::Ok(_) => core::panic_with_felt252('VALIDATION_DID_NOT_PANIC'), + Result::Err(panic_data) => { + // The runtime appends 'ENTRYPOINT_FAILED' to panic data from a failed entry + // point, so match on element 0 rather than the whole array. + if panic_data.is_empty() + || *panic_data.at(0) != UPGRADEABILITY_VALIDATION_SUCCESS { + core::panics::panic(panic_data); + } + }, + } + } + fn is_finalized(self: @ComponentState) -> bool { self.finalized.read() } diff --git a/packages/utils/src/components/replaceability/test.cairo b/packages/utils/src/components/replaceability/test.cairo index c5f5018a..b348ce2c 100644 --- a/packages/utils/src/components/replaceability/test.cairo +++ b/packages/utils/src/components/replaceability/test.cairo @@ -17,7 +17,8 @@ mod ReplaceabilityTests { assert_implementation_replaced_event_emitted, deploy_dummy_contract, deploy_replaceability_mock, dummy_final_implementation_data_with_class_hash, dummy_nonfinal_eic_implementation_data_with_class_hash, - dummy_nonfinal_implementation_data_with_class_hash, get_upgrade_governor_account, + dummy_nonfinal_implementation_data_with_class_hash, get_replaceability_mock_v2_class_hash, + get_upgrade_governor_account, }; use snforge_std::{ CheatSpan, EventSpyAssertionsTrait, EventSpyTrait, EventsFilterTrait, cheat_block_timestamp, @@ -65,7 +66,10 @@ mod ReplaceabilityTests { fn test_add_new_implementation() { let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; - let implementation_data = DUMMY_NONFINAL_IMPLEMENTATION_DATA(); + // Use a real upgradeable class hash so validation at add time can dispatch into it. + let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( + class_hash: get_replaceability_mock_v2_class_hash(), + ); // Check implementation time pre addition. assert!(replaceable_dispatcher.get_impl_activation_time(:implementation_data).is_zero()); @@ -75,9 +79,11 @@ mod ReplaceabilityTests { ); let mut spy = spy_events(); replaceable_dispatcher.add_new_implementation(:implementation_data); + // Test setup pins block_timestamp to 1, so activation_time = 1 + DEFAULT_UPGRADE_DELAY. assert!( replaceable_dispatcher - .get_impl_activation_time(:implementation_data) == DEFAULT_UPGRADE_DELAY, + .get_impl_activation_time(:implementation_data) == DEFAULT_UPGRADE_DELAY + + 1, ); // Validate event emission. @@ -101,6 +107,9 @@ mod ReplaceabilityTests { fn test_add_new_implementation_not_upgrade_governor() { let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; + // class_hash = 0 is intentional: the auth check fires before validation, so the + // placeholder class is never dispatched into. If the order ever flips, this would + // surface as "class not declared" instead of ONLY_UPGRADE_GOVERNOR. let implementation_data = DUMMY_NONFINAL_IMPLEMENTATION_DATA(); // Invoke not as an Upgrade Governor. @@ -112,7 +121,10 @@ mod ReplaceabilityTests { fn test_remove_implementation() { let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; - let implementation_data = DUMMY_NONFINAL_IMPLEMENTATION_DATA(); + // Use a real upgradeable class hash so validation at add time can dispatch into it. + let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( + class_hash: get_replaceability_mock_v2_class_hash(), + ); cheat_caller_address( :contract_address, @@ -217,7 +229,8 @@ mod ReplaceabilityTests { let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; - let new_class_hash = get_class_hash(contract_address: deploy_dummy_contract()); + // Deploy a V2 mock to get a different class hash that still includes replaceability. + let new_class_hash = get_replaceability_mock_v2_class_hash(); assert_ne!(get_class_hash(:contract_address), new_class_hash); let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( @@ -319,7 +332,10 @@ mod ReplaceabilityTests { fn test_replace_to_not_upgrade_governor() { let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; - let implementation_data = DUMMY_NONFINAL_IMPLEMENTATION_DATA(); + // Use a real declared class hash so the validation library_call can resolve it. + let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( + class_hash: get_replaceability_mock_v2_class_hash(), + ); // Invoke not as an Upgrade Governor. cheat_caller_address_once(:contract_address, caller_address: NOT_UPGRADE_GOVERNOR_ACCOUNT); @@ -336,7 +352,10 @@ mod ReplaceabilityTests { cheat_caller_address_once( :contract_address, caller_address: get_upgrade_governor_account(:contract_address), ); - let implementation_data = DUMMY_NONFINAL_IMPLEMENTATION_DATA(); + // Use a real declared class hash so the validation library_call can resolve it. + let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( + class_hash: get_replaceability_mock_v2_class_hash(), + ); // Calling replace_to without previously adding the implementation. replaceable_dispatcher.replace_to(:implementation_data); @@ -503,6 +522,200 @@ mod ReplaceabilityTests { // Verify the granted account can call add_new_implementation. cheat_caller_address_once(:contract_address, caller_address: account); replaceable_dispatcher - .add_new_implementation(implementation_data: DUMMY_NONFINAL_IMPLEMENTATION_DATA()); + .add_new_implementation( + implementation_data: dummy_nonfinal_implementation_data_with_class_hash( + class_hash: get_replaceability_mock_v2_class_hash(), + ), + ); + } + + #[test] + #[feature("safe_dispatcher")] + fn test_add_new_implementation_blocks_non_upgradeable() { + // Adding a non-final implementation that points to a contract without the replaceability + // component should fail at add time — validation cannot complete the dry-run upgrade + // cycle on a target that lacks `add_new_implementation_unsafe` or `replace_to`. + let replaceable_dispatcher = deploy_replaceability_mock(); + let contract_address = replaceable_dispatcher.contract_address; + let safe_dispatcher = IReplaceableSafeDispatcher { contract_address }; + + let new_class_hash = get_class_hash(contract_address: deploy_dummy_contract()); + let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( + class_hash: new_class_hash, + ); + + cheat_caller_address_once( + :contract_address, caller_address: get_upgrade_governor_account(:contract_address), + ); + match safe_dispatcher.add_new_implementation(:implementation_data) { + Result::Ok(_) => panic!("Should have failed: target has no replaceability component"), + Result::Err(_) => (), + } + } + + #[test] + fn test_add_new_implementation_final_skips_validation() { + // Adding a final implementation skips validation — even if the target lacks a working + // upgrade path. Final implementations intentionally surrender upgradeability. + let replaceable_dispatcher = deploy_replaceability_mock(); + let contract_address = replaceable_dispatcher.contract_address; + + let new_class_hash = get_class_hash(contract_address: deploy_dummy_contract()); + let implementation_data = dummy_final_implementation_data_with_class_hash( + class_hash: new_class_hash, + ); + + cheat_caller_address( + :contract_address, + caller_address: get_upgrade_governor_account(:contract_address), + span: CheatSpan::TargetCalls(2), + ); + // Add succeeds despite non-upgradeable target — final=true skips validation. + replaceable_dispatcher.add_new_implementation(:implementation_data); + cheat_block_timestamp(contract_address, DEFAULT_UPGRADE_DELAY + 1, CheatSpan::Indefinite); + + replaceable_dispatcher.replace_to(:implementation_data); + assert_eq!(get_class_hash(:contract_address), new_class_hash); + assert_finalized_status(expected: true, :contract_address); + } + + #[test] + fn test_upgradeability_validation_no_side_effects() { + // After a successful add_new_implementation, verify that the validation dry-run did + // not corrupt storage (upgrade_delay should be unchanged — validation writes 0 in the + // dry-run state, which is reverted by the library_call panic). + let replaceable_dispatcher = deploy_replaceability_mock(); + let contract_address = replaceable_dispatcher.contract_address; + + let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( + class_hash: get_replaceability_mock_v2_class_hash(), + ); + + cheat_caller_address_once( + :contract_address, caller_address: get_upgrade_governor_account(:contract_address), + ); + replaceable_dispatcher.add_new_implementation(:implementation_data); + + assert_eq!(replaceable_dispatcher.get_upgrade_delay(), DEFAULT_UPGRADE_DELAY); + } + + #[test] + fn test_add_new_implementation_unsafe_succeeds_for_invalid_target() { + // add_new_implementation_unsafe bypasses validation — an impl pointing to a contract + // without the replaceability component is accepted. Verify the activation/expiration + // entries are written. + let replaceable_dispatcher = deploy_replaceability_mock(); + let contract_address = replaceable_dispatcher.contract_address; + + let dummy_class_hash = get_class_hash(contract_address: deploy_dummy_contract()); + let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( + class_hash: dummy_class_hash, + ); + + cheat_caller_address_once( + :contract_address, caller_address: get_upgrade_governor_account(:contract_address), + ); + replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data); + + // Test setup pins block_timestamp to 1, so activation_time = 1 + DEFAULT_UPGRADE_DELAY. + assert!( + replaceable_dispatcher + .get_impl_activation_time(:implementation_data) == DEFAULT_UPGRADE_DELAY + + 1, + ); + } + + #[test] + #[should_panic(expected: "ONLY_UPGRADE_GOVERNOR")] + fn test_add_new_implementation_unsafe_not_upgrade_governor() { + let replaceable_dispatcher = deploy_replaceability_mock(); + let contract_address = replaceable_dispatcher.contract_address; + // class_hash = 0 is intentional: auth fires before any storage write, so the + // placeholder class is never used. + let implementation_data = DUMMY_NONFINAL_IMPLEMENTATION_DATA(); + + cheat_caller_address_once(:contract_address, caller_address: NOT_UPGRADE_GOVERNOR_ACCOUNT); + replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data); + } + + #[test] + #[feature("safe_dispatcher")] + fn test_add_new_implementation_blocked_when_finalized() { + // After finalization, add_new_implementation rejects — validation dispatches replace_to + // on the target class, which (since it shares this contract's storage) reads the + // `finalized` flag and panics FINALIZED. add_new_implementation_unsafe still succeeds + // because it bypasses validation entirely. + let replaceable_dispatcher = deploy_replaceability_mock(); + let contract_address = replaceable_dispatcher.contract_address; + let safe_dispatcher = IReplaceableSafeDispatcher { contract_address }; + + // Finalize: schedule a final impl pointing to self, then apply it. + let final_data = dummy_final_implementation_data_with_class_hash( + class_hash: get_class_hash(:contract_address), + ); + cheat_caller_address( + :contract_address, + caller_address: get_upgrade_governor_account(:contract_address), + span: CheatSpan::Indefinite, + ); + replaceable_dispatcher.add_new_implementation(implementation_data: final_data); + cheat_block_timestamp(contract_address, DEFAULT_UPGRADE_DELAY + 2, CheatSpan::Indefinite); + replaceable_dispatcher.replace_to(implementation_data: final_data); + assert_finalized_status(expected: true, :contract_address); + + // add_new_implementation must now fail — the dispatched replace_to inside validation + // hits the finalized flag. + let nonfinal_data = dummy_nonfinal_implementation_data_with_class_hash( + class_hash: get_replaceability_mock_v2_class_hash(), + ); + match safe_dispatcher.add_new_implementation(implementation_data: nonfinal_data) { + Result::Ok(_) => panic!("Should fail: contract is finalized"), + Result::Err(_) => (), + } + + // add_new_implementation_unsafe still works — it bypasses validation. + replaceable_dispatcher.add_new_implementation_unsafe(implementation_data: nonfinal_data); + assert!( + replaceable_dispatcher + .get_impl_activation_time(implementation_data: nonfinal_data) + .is_non_zero(), + ); + } + + #[test] + #[feature("safe_dispatcher")] + fn test_intentional_brick_via_unsafe_add() { + // add_new_implementation_unsafe lets an upgrade_governor schedule an impl that would + // otherwise be blocked by validation. Once scheduled, replace_to applies it — bricking + // the contract because the new code has no upgrade machinery. This test confirms both + // halves: the unsafe path succeeds where the safe path would have blocked, AND the + // resulting contract cannot be upgraded further. + let replaceable_dispatcher = deploy_replaceability_mock(); + let contract_address = replaceable_dispatcher.contract_address; + let safe_dispatcher = IReplaceableSafeDispatcher { contract_address }; + + let dummy_class_hash = get_class_hash(contract_address: deploy_dummy_contract()); + let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( + class_hash: dummy_class_hash, + ); + + cheat_caller_address( + :contract_address, + caller_address: get_upgrade_governor_account(:contract_address), + span: CheatSpan::TargetCalls(2), + ); + replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data); + cheat_block_timestamp(contract_address, DEFAULT_UPGRADE_DELAY + 1, CheatSpan::Indefinite); + + // Apply the bad impl via the normal replace_to — succeeds because validation already + // ran (or rather, was bypassed) at add time. + replaceable_dispatcher.replace_to(:implementation_data); + assert_eq!(get_class_hash(:contract_address), dummy_class_hash); + + // Contract is now bricked: any upgrade-related call hits a non-existent entry point. + match safe_dispatcher.add_new_implementation(:implementation_data) { + Result::Ok(_) => panic!("Bricked contract should not accept add_new_implementation"), + Result::Err(_) => (), + } } } diff --git a/packages/utils/src/components/replaceability/test_utils.cairo b/packages/utils/src/components/replaceability/test_utils.cairo index bb244586..ab020790 100644 --- a/packages/utils/src/components/replaceability/test_utils.cairo +++ b/packages/utils/src/components/replaceability/test_utils.cairo @@ -1,5 +1,7 @@ use snforge_std::cheatcodes::events::{Event, Events}; -use snforge_std::{ContractClassTrait, DeclareResultTrait, declare, load}; +use snforge_std::{ + CheatSpan, ContractClassTrait, DeclareResultTrait, cheat_block_timestamp, declare, load, +}; use starknet::ContractAddress; use starknet::class_hash::ClassHash; use starkware_utils::components::replaceability::ReplaceabilityComponent; @@ -48,9 +50,19 @@ pub(crate) fn deploy_replaceability_mock() -> IReplaceableDispatcher { @array![Constants::DEFAULT_UPGRADE_DELAY.into(), Constants::GOVERNANCE_ADMIN.into()], ) .unwrap(); + // Pin block_timestamp to a non-zero baseline so the upgradeability validation dry-run + // (which sets upgrade_delay to 0 internally) yields a non-zero activation_time. Tests + // can override by calling `cheat_block_timestamp` again. + cheat_block_timestamp(:contract_address, block_timestamp: 1, span: CheatSpan::Indefinite); return IReplaceableDispatcher { contract_address: contract_address }; } +// Returns the class hash of `ReplaceabilityMockV2` — a valid upgrade target distinct from +// `ReplaceabilityMock`'s class hash. +pub(crate) fn get_replaceability_mock_v2_class_hash() -> ClassHash { + *declare("ReplaceabilityMockV2").unwrap().contract_class().class_hash +} + #[starknet::contract] mod DummyContract { #[storage] From 500fdceae8658a013ed0ef73c32fbb03d0877dd6 Mon Sep 17 00:00:00 2001 From: Remo Date: Sun, 3 May 2026 11:58:31 +0300 Subject: [PATCH 2/4] review issues --- .../components/replaceability/errors.cairo | 4 + .../components/replaceability/interface.cairo | 6 +- .../replaceability/replaceability.cairo | 143 +++++++++++------- .../src/components/replaceability/test.cairo | 86 +++++++---- .../replaceability/test_utils.cairo | 10 ++ 5 files changed, 164 insertions(+), 85 deletions(-) diff --git a/packages/utils/src/components/replaceability/errors.cairo b/packages/utils/src/components/replaceability/errors.cairo index 6c3e6989..f8c7abad 100644 --- a/packages/utils/src/components/replaceability/errors.cairo +++ b/packages/utils/src/components/replaceability/errors.cairo @@ -4,11 +4,13 @@ use starkware_utils::errors::{Describable, ErrorDisplay}; pub(crate) enum ReplaceErrors { ALREADY_INITIALIZED, FINALIZED, + FINALIZE_IS_UNSAFE, UNKNOWN_IMPLEMENTATION, NOT_ENABLED_YET, IMPLEMENTATION_EXPIRED, EIC_LIB_CALL_FAILED, REPLACE_CLASS_HASH_FAILED, + FAILED_REPLACE_CLASS_HASH_B2A, } impl DescribableError of Describable { @@ -16,11 +18,13 @@ impl DescribableError of Describable { match self { ReplaceErrors::ALREADY_INITIALIZED => "ALREADY_INITIALIZED", ReplaceErrors::FINALIZED => "FINALIZED", + ReplaceErrors::FINALIZE_IS_UNSAFE => "FINALIZE_IS_UNSAFE", ReplaceErrors::UNKNOWN_IMPLEMENTATION => "UNKNOWN_IMPLEMENTATION", ReplaceErrors::NOT_ENABLED_YET => "NOT_ENABLED_YET", ReplaceErrors::IMPLEMENTATION_EXPIRED => "IMPLEMENTATION_EXPIRED", ReplaceErrors::EIC_LIB_CALL_FAILED => "EIC_LIB_CALL_FAILED", ReplaceErrors::REPLACE_CLASS_HASH_FAILED => "REPLACE_CLASS_HASH_FAILED", + ReplaceErrors::FAILED_REPLACE_CLASS_HASH_B2A => "FAILED_REPLACE_CLASS_HASH_B2A", } } } diff --git a/packages/utils/src/components/replaceability/interface.cairo b/packages/utils/src/components/replaceability/interface.cairo index c3a70f06..5442e628 100644 --- a/packages/utils/src/components/replaceability/interface.cairo +++ b/packages/utils/src/components/replaceability/interface.cairo @@ -48,7 +48,11 @@ pub trait IReplaceable { ); fn remove_implementation(ref self: TContractState, implementation_data: ImplementationData); fn replace_to(ref self: TContractState, implementation_data: ImplementationData); - fn validate_upgradeability(ref self: TContractState, impl_hash: ClassHash); + // Always panics by design — Should be invoked only via library_call from + // `add_new_implementation`, never directly. + // It must live in `IReplaceable` so every contract embedding + // `ReplaceabilityImpl` exports the selector; otherwise the library_call dispatch fails. + fn validate_upgradeability(ref self: TContractState, implementation_data: ImplementationData); } #[derive(Copy, Drop, PartialEq, starknet::Event)] diff --git a/packages/utils/src/components/replaceability/replaceability.cairo b/packages/utils/src/components/replaceability/replaceability.cairo index 9b0a8e93..a15cb3ee 100644 --- a/packages/utils/src/components/replaceability/replaceability.cairo +++ b/packages/utils/src/components/replaceability/replaceability.cairo @@ -4,7 +4,6 @@ pub(crate) mod ReplaceabilityComponent { use core::poseidon; use openzeppelin::access::accesscontrol::AccessControlComponent; use openzeppelin::introspection::src5::SRC5Component; - use starknet::class_hash::ClassHash; use starknet::storage::{ Map, StorageMapReadAccess, StorageMapWriteAccess, StoragePointerReadAccess, StoragePointerWriteAccess, @@ -74,22 +73,16 @@ pub(crate) mod ReplaceabilityComponent { self.impl_activation_time.read(impl_key) } - // Schedules a new implementation and validates that it is upgradeable. If the new - // implementation cannot itself perform a full upgrade cycle (add + replace), the entire - // transaction reverts. Finalized implementations (`final = true`) skip the validation. + // Schedules a new implementation and validates that it is upgradeable. If the target + // cannot itself perform a full upgrade cycle (add + replace, in both directions), the + // entire transaction reverts. Final adds (`implementation_data.final = true`) are + // rejected with `FINALIZE_IS_UNSAFE` — to finalize, use + // `add_new_implementation_unsafe` instead. fn add_new_implementation( ref self: ComponentState, implementation_data: ImplementationData, ) { - // The auth check exists in two places. Here it is fail-fast: bail before paying the - // library_call cost of validation. The check inside `_unsafe` below is the one that - // actually enforces the role on direct callers and exercises the target's role - // wiring during the validation dispatch. - let common_roles = get_dep_component!(@self, CommonRoles); - common_roles.only_upgrade_governor(); - - if (!implementation_data.final) { - self.invoke_upgradeability_validation(implementation_data.impl_hash); - } + assert!(!implementation_data.final, "{}", ReplaceErrors::FINALIZE_IS_UNSAFE); + self.invoke_upgradeability_validation(implementation_data); self.add_new_implementation_unsafe(implementation_data); } @@ -99,9 +92,10 @@ pub(crate) mod ReplaceabilityComponent { fn add_new_implementation_unsafe( ref self: ComponentState, implementation_data: ImplementationData, ) { - // Authoritative auth check for direct callers. Also load-bearing during validation: - // when `validate_upgradeability` dispatches this on the target class, this check is - // what proves the target's upgrade_governor role is wired correctly. + // Authoritative auth check for direct callers. Also load-bearing during + // validation: step 2's `impl_b_dispatcher.add_new_implementation_unsafe` runs + // this check on the target class, so a target with broken `upgrade_governor` + // role wiring fails the dry-run cycle here. let common_roles = get_dep_component!(@self, CommonRoles); common_roles.only_upgrade_governor(); @@ -187,37 +181,71 @@ pub(crate) mod ReplaceabilityComponent { self.set_impl_expiration_time(:implementation_data, expiration_time: 0); } - // Dry-run validation that the target class can perform a full upgrade cycle (add + - // replace). Always panics — `UPGRADEABILITY_VALIDATION_SUCCESS` on success, or the - // underlying failure otherwise. Callers wrap this in a library_call so the panic - // reverts all side effects. + // Dry-run validation of a full upgrade cycle for the user's `implementation_data`. + // Always panics — `UPGRADEABILITY_VALIDATION_SUCCESS` on success, or the underlying + // failure otherwise. Callers wrap this in a library_call so the panic reverts every + // side effect: storage writes, emitted events, and the `replace_class_syscall`s in + // both `replace_to`s below. // - // Both stages are dispatched to the target class explicitly so this function tests the - // target's own `add_new_implementation_unsafe` and `replace_to` — `_unsafe` is used - // for the add stage to avoid re-invoking validation (recursion). + // Step 1 (A→B): runs the user's add + replace on `self`, exercising the user's EIC + // and `final` flag through the actual production code path. After step 1 the on-chain + // class hash is briefly impl_b — the outer panic reverts it. // - // Coverage is partial: validation only exercises the (add + replace) path with - // `eic_data: None` and `final: false`. Targets that require EIC initialization, or - // whose `remove_implementation` is broken, are not covered. + // Step 2 (B→A): library-dispatches an add + replace back to impl_a on impl_b. This + // proves the new class can itself perform an upgrade cycle and catches a target that + // lacks `add_new_implementation_unsafe` / `replace_to` or has a broken role wiring. + // Step 2 carries `eic_data: None` and `final: false` — the round trip's purpose is + // to exercise the upgrade machinery, not run another EIC or burn finalization. // - // Threat model: this guards against an upgrade_governor accidentally scheduling a - // non-upgradeable class. It is not a defense against a malicious governor. - fn validate_upgradeability(ref self: ComponentState, impl_hash: ClassHash) { - let implementation_data = ImplementationData { - impl_hash, eic_data: Option::None, final: false, - }; - let dispatcher = IReplaceableLibraryDispatcher { class_hash: impl_hash }; + // Threat model: guards against an upgrade_governor accidentally scheduling a + // non-upgradeable class. A malicious governor can still brick via + // `add_new_implementation_unsafe`. A malicious target cannot spoof the success + // sentinel — the runtime appends `'ENTRYPOINT_FAILED'` per dispatch frame, so a + // spoofed panic from a step-2 dispatched function fails the exact-array match in + // `invoke_upgradeability_validation`. + fn validate_upgradeability( + ref self: ComponentState, implementation_data: ImplementationData, + ) { + // impl_a_hash: current contract class hash. + let impl_a_hash = get_class_hash_at_syscall(get_contract_address()).unwrap_syscall(); - // Zero the delay so the dispatched add yields `activation_time = block_timestamp`, - // which then satisfies `replace_to`'s `activation_time <= now` check. Relies on the - // production invariant that `block_timestamp > 0`; tests cheat the timestamp in + // impl_b_hash: class hash of user-planned upgrade. + let impl_b_hash = implementation_data.impl_hash; + + // Step 1 (A→B): complete the user-planned upgrade on `self`. Zero the delay so + // the add yields `activation_time = block_timestamp`, which satisfies + // `replace_to`'s `activation_time <= now` check. Relies on the production + // invariant that `block_timestamp > 0`; tests cheat the timestamp in // `deploy_replaceability_mock` to bridge snforge's zero default. self.upgrade_delay.write(0); - dispatcher.add_new_implementation_unsafe(:implementation_data); - dispatcher.replace_to(:implementation_data); + self.add_new_implementation_unsafe(implementation_data); + self.replace_to(implementation_data); + + // Step 2 (B→A): library-dispatch an upgrade back to impl_a using impl_b's own + // machinery. Re-zero `upgrade_delay` because step 1's EIC is the only legitimate + // way to modify it, and step 2's timing check requires activation_time <= now. + self.upgrade_delay.write(0); + let back2a_impl_data = ImplementationData { + impl_hash: impl_a_hash, eic_data: Option::None, final: false, + }; + let impl_b_dispatcher = IReplaceableLibraryDispatcher { class_hash: impl_b_hash }; + + impl_b_dispatcher.add_new_implementation_unsafe(implementation_data: back2a_impl_data); + impl_b_dispatcher.replace_to(implementation_data: back2a_impl_data); + // Defends against a target whose `replace_to` returns Ok without actually + // calling `replace_class_syscall` (e.g. a hostile no-op re-implementation). The + // step-1 equivalent is unnecessary because step 1 runs `self`'s own `replace_to`, + // which already asserts the syscall succeeded. + assert!( + get_class_hash_at_syscall(get_contract_address()).unwrap_syscall() == impl_a_hash, + "{}", + ReplaceErrors::FAILED_REPLACE_CLASS_HASH_B2A, + ); - // Load-bearing: the panic is what reverts `upgrade_delay.write(0)` above and the - // dispatchers' side effects. A normal return here would silently zero the delay. + // Load-bearing: the panic is what reverts every side effect above — the + // `upgrade_delay` writes, the storage entries from the two adds, the emitted + // events, and (critically) the `replace_class_syscall`s in both `replace_to`s. + // A normal return here would leave the contract in step 2's state. core::panic_with_felt252(UPGRADEABILITY_VALIDATION_SUCCESS); } } @@ -236,38 +264,37 @@ pub(crate) mod ReplaceabilityComponent { impl PrivateReplaceabilityImpl< TContractState, +HasComponent, +Drop, > of PrivateReplaceabilityTrait { - // Runs `validate_upgradeability` against `new_class_hash` via library_call. Must be - // called while the contract's active class hash is still trusted (i.e., before the - // class replacement happens) — the validation logic itself comes from this active - // class, not the untrusted target. + // Runs `validate_upgradeability(impl_data)` via library_call against the contract's + // current (trusted) class hash — the validation logic comes from this class, not from + // the untrusted target referenced in `impl_data`. // - // Returns silently on `UPGRADABILITY_VALIDATION_SUCCESS` (the library call's side + // Returns silently on `UPGRADEABILITY_VALIDATION_SUCCESS` (the library call's side // effects are reverted by the runtime). Any other panic is propagated, reverting the // caller's transaction. fn invoke_upgradeability_validation( - ref self: ComponentState, new_class_hash: ClassHash, + ref self: ComponentState, impl_data: ImplementationData, ) { let current_class_hash = get_class_hash_at_syscall(get_contract_address()) .unwrap_syscall(); let mut calldata = array![]; - new_class_hash.serialize(ref calldata); + impl_data.serialize(ref calldata); let result = library_call_syscall( class_hash: current_class_hash, function_selector: selector!("validate_upgradeability"), calldata: calldata.span(), ); - match result { - Result::Ok(_) => core::panic_with_felt252('VALIDATION_DID_NOT_PANIC'), - Result::Err(panic_data) => { - // The runtime appends 'ENTRYPOINT_FAILED' to panic data from a failed entry - // point, so match on element 0 rather than the whole array. - if panic_data.is_empty() - || *panic_data.at(0) != UPGRADEABILITY_VALIDATION_SUCCESS { - core::panics::panic(panic_data); - } - }, + // Unreachable in practice — validate_upgradeability always panics. + let panic_data = result.expect_err('VALIDATION_DID_NOT_PANIC'); + // On success, validate_upgradeability panics with the + // UPGRADEABILITY_VALIDATION_SUCCESS sentinel; the Starknet runtime then appends + // 'ENTRYPOINT_FAILED' (the runtime-dictated suffix for any failed entry point) + // to the panic data. Match the exact 2-element pattern: a target that spoofs + // the sentinel from its own dispatched function gets an extra runtime suffix + // and fails this comparison. + if panic_data != array![UPGRADEABILITY_VALIDATION_SUCCESS, 'ENTRYPOINT_FAILED'] { + core::panics::panic(panic_data); } } diff --git a/packages/utils/src/components/replaceability/test.cairo b/packages/utils/src/components/replaceability/test.cairo index b348ce2c..30a05e64 100644 --- a/packages/utils/src/components/replaceability/test.cairo +++ b/packages/utils/src/components/replaceability/test.cairo @@ -16,6 +16,7 @@ mod ReplaceabilityTests { assert_finalized_status, assert_implementation_finalized_event_emitted, assert_implementation_replaced_event_emitted, deploy_dummy_contract, deploy_replaceability_mock, dummy_final_implementation_data_with_class_hash, + dummy_nonfinal_broken_eic_implementation_data_with_class_hash, dummy_nonfinal_eic_implementation_data_with_class_hash, dummy_nonfinal_implementation_data_with_class_hash, get_replaceability_mock_v2_class_hash, get_upgrade_governor_account, @@ -107,8 +108,8 @@ mod ReplaceabilityTests { fn test_add_new_implementation_not_upgrade_governor() { let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; - // class_hash = 0 is intentional: the auth check fires before validation, so the - // placeholder class is never dispatched into. If the order ever flips, this would + // class_hash = 0 is intentional: the auth check inside validation's first call fires + // before anything dereferences the class hash. If that order ever flips, this would // surface as "class not declared" instead of ONLY_UPGRADE_GOVERNOR. let implementation_data = DUMMY_NONFINAL_IMPLEMENTATION_DATA(); @@ -388,10 +389,11 @@ mod ReplaceabilityTests { span: CheatSpan::TargetCalls(8), ); - // Add implementations. + // Add implementations. The "other" impl is final and points to a placeholder + // class_hash, so it must go through `_unsafe` to bypass the FINALIZE_IS_UNSAFE assert. replaceable_dispatcher.add_new_implementation(:implementation_data); replaceable_dispatcher - .add_new_implementation(implementation_data: other_implementation_data); + .add_new_implementation_unsafe(implementation_data: other_implementation_data); assert!( replaceable_dispatcher.get_impl_activation_time(:implementation_data).is_non_zero(), ); @@ -444,7 +446,9 @@ mod ReplaceabilityTests { span: CheatSpan::TargetCalls(2), ); let mut spy = spy_events(); - replaceable_dispatcher.add_new_implementation(:implementation_data); + // Final adds must go through `_unsafe` — the safe path rejects with + // FINALIZE_IS_UNSAFE. + replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data); // Advance time to enable implementation. cheat_block_timestamp( @@ -490,7 +494,9 @@ mod ReplaceabilityTests { caller_address: get_upgrade_governor_account(:contract_address), span: CheatSpan::TargetCalls(3), ); - replaceable_dispatcher.add_new_implementation(:implementation_data); + // Final adds must go through `_unsafe` — the safe path rejects with + // FINALIZE_IS_UNSAFE. + replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data); // Advance time to enable implementation. cheat_block_timestamp(contract_address, DEFAULT_UPGRADE_DELAY + 1, CheatSpan::Indefinite); @@ -554,29 +560,34 @@ mod ReplaceabilityTests { } #[test] - fn test_add_new_implementation_final_skips_validation() { - // Adding a final implementation skips validation — even if the target lacks a working - // upgrade path. Final implementations intentionally surrender upgradeability. + #[feature("safe_dispatcher")] + fn test_add_new_implementation_rejects_final() { + // Final adds via the safe path are rejected with FINALIZE_IS_UNSAFE — bricking the + // upgrade path is intentional and must go through `_unsafe` to prove intent. The + // class hash never matters here; the assert fires before validation runs. let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; + let safe_dispatcher = IReplaceableSafeDispatcher { contract_address }; - let new_class_hash = get_class_hash(contract_address: deploy_dummy_contract()); let implementation_data = dummy_final_implementation_data_with_class_hash( - class_hash: new_class_hash, + class_hash: get_replaceability_mock_v2_class_hash(), ); cheat_caller_address( :contract_address, caller_address: get_upgrade_governor_account(:contract_address), - span: CheatSpan::TargetCalls(2), + span: CheatSpan::Indefinite, ); - // Add succeeds despite non-upgradeable target — final=true skips validation. - replaceable_dispatcher.add_new_implementation(:implementation_data); - cheat_block_timestamp(contract_address, DEFAULT_UPGRADE_DELAY + 1, CheatSpan::Indefinite); + match safe_dispatcher.add_new_implementation(:implementation_data) { + Result::Ok(_) => panic!("Should fail: final adds must use `_unsafe`"), + Result::Err(_) => (), + } - replaceable_dispatcher.replace_to(:implementation_data); - assert_eq!(get_class_hash(:contract_address), new_class_hash); - assert_finalized_status(expected: true, :contract_address); + // Migration path: same data via `_unsafe` succeeds. + replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data); + assert!( + replaceable_dispatcher.get_impl_activation_time(:implementation_data).is_non_zero(), + ); } #[test] @@ -599,6 +610,29 @@ mod ReplaceabilityTests { assert_eq!(replaceable_dispatcher.get_upgrade_delay(), DEFAULT_UPGRADE_DELAY); } + #[test] + #[feature("safe_dispatcher")] + fn test_add_new_implementation_blocks_broken_eic() { + // Validation runs the user's EIC during step 1's `replace_to`. An EIC that panics + // on init must surface as a rejected add. The broken EIC here is the test EIC + // called with empty init_data, which trips its own length-check assert. + let replaceable_dispatcher = deploy_replaceability_mock(); + let contract_address = replaceable_dispatcher.contract_address; + let safe_dispatcher = IReplaceableSafeDispatcher { contract_address }; + + let implementation_data = dummy_nonfinal_broken_eic_implementation_data_with_class_hash( + class_hash: get_replaceability_mock_v2_class_hash(), + ); + + cheat_caller_address_once( + :contract_address, caller_address: get_upgrade_governor_account(:contract_address), + ); + match safe_dispatcher.add_new_implementation(:implementation_data) { + Result::Ok(_) => panic!("Should fail: EIC init panics"), + Result::Err(_) => (), + } + } + #[test] fn test_add_new_implementation_unsafe_succeeds_for_invalid_target() { // add_new_implementation_unsafe bypasses validation — an impl pointing to a contract @@ -641,15 +675,15 @@ mod ReplaceabilityTests { #[test] #[feature("safe_dispatcher")] fn test_add_new_implementation_blocked_when_finalized() { - // After finalization, add_new_implementation rejects — validation dispatches replace_to - // on the target class, which (since it shares this contract's storage) reads the - // `finalized` flag and panics FINALIZED. add_new_implementation_unsafe still succeeds - // because it bypasses validation entirely. + // After finalization, add_new_implementation rejects — validation step 1's + // self.replace_to reads the finalized flag and panics. add_new_implementation_unsafe + // still succeeds because it bypasses validation entirely. let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; let safe_dispatcher = IReplaceableSafeDispatcher { contract_address }; - // Finalize: schedule a final impl pointing to self, then apply it. + // Finalize: schedule a final impl pointing to self (via `_unsafe` since the safe path + // rejects final adds), then apply it. let final_data = dummy_final_implementation_data_with_class_hash( class_hash: get_class_hash(:contract_address), ); @@ -658,13 +692,13 @@ mod ReplaceabilityTests { caller_address: get_upgrade_governor_account(:contract_address), span: CheatSpan::Indefinite, ); - replaceable_dispatcher.add_new_implementation(implementation_data: final_data); + replaceable_dispatcher.add_new_implementation_unsafe(implementation_data: final_data); cheat_block_timestamp(contract_address, DEFAULT_UPGRADE_DELAY + 2, CheatSpan::Indefinite); replaceable_dispatcher.replace_to(implementation_data: final_data); assert_finalized_status(expected: true, :contract_address); - // add_new_implementation must now fail — the dispatched replace_to inside validation - // hits the finalized flag. + // add_new_implementation must now fail — validation step 1's self.replace_to hits the + // finalized flag. let nonfinal_data = dummy_nonfinal_implementation_data_with_class_hash( class_hash: get_replaceability_mock_v2_class_hash(), ); diff --git a/packages/utils/src/components/replaceability/test_utils.cairo b/packages/utils/src/components/replaceability/test_utils.cairo index ab020790..35bb4492 100644 --- a/packages/utils/src/components/replaceability/test_utils.cairo +++ b/packages/utils/src/components/replaceability/test_utils.cairo @@ -111,6 +111,16 @@ pub(crate) fn dummy_nonfinal_eic_implementation_data_with_class_hash( ImplementationData { impl_hash: class_hash, eic_data: Option::Some(eic_data), final: false } } +// Returns implementation_data wired to `EICTestContract` with empty init_data — the EIC +// asserts `eic_init_data.len() == 1` and panics with EIC_INIT_DATA_LEN_MISMATCH on init. +pub(crate) fn dummy_nonfinal_broken_eic_implementation_data_with_class_hash( + class_hash: ClassHash, +) -> ImplementationData { + let eic_contract = declare("EICTestContract").unwrap().contract_class(); + let eic_data = EICData { eic_hash: *eic_contract.class_hash, eic_init_data: array![].span() }; + ImplementationData { impl_hash: class_hash, eic_data: Option::Some(eic_data), final: false } +} + pub(crate) fn assert_implementation_replaced_event_emitted( mut spied_event: @(ContractAddress, Event), implementation_data: ImplementationData, ) { From b7cd2f7105dd3d4faf690d2603e283af0a6d291f Mon Sep 17 00:00:00 2001 From: Remo Date: Tue, 5 May 2026 19:17:47 +0300 Subject: [PATCH 3/4] fix comments --- .../components/replaceability/errors.cairo | 2 + .../components/replaceability/interface.cairo | 4 - .../components/replaceability/mock_v2.cairo | 8 +- .../replaceability/replaceability.cairo | 96 +++++-------------- .../src/components/replaceability/test.cairo | 67 +++---------- .../replaceability/test_utils.cairo | 9 +- 6 files changed, 42 insertions(+), 144 deletions(-) diff --git a/packages/utils/src/components/replaceability/errors.cairo b/packages/utils/src/components/replaceability/errors.cairo index f8c7abad..8695030c 100644 --- a/packages/utils/src/components/replaceability/errors.cairo +++ b/packages/utils/src/components/replaceability/errors.cairo @@ -10,6 +10,7 @@ pub(crate) enum ReplaceErrors { IMPLEMENTATION_EXPIRED, EIC_LIB_CALL_FAILED, REPLACE_CLASS_HASH_FAILED, + FAILED_REPLACE_CLASS_HASH_A2B, FAILED_REPLACE_CLASS_HASH_B2A, } @@ -24,6 +25,7 @@ impl DescribableError of Describable { ReplaceErrors::IMPLEMENTATION_EXPIRED => "IMPLEMENTATION_EXPIRED", ReplaceErrors::EIC_LIB_CALL_FAILED => "EIC_LIB_CALL_FAILED", ReplaceErrors::REPLACE_CLASS_HASH_FAILED => "REPLACE_CLASS_HASH_FAILED", + ReplaceErrors::FAILED_REPLACE_CLASS_HASH_A2B => "FAILED_REPLACE_CLASS_HASH_A2B", ReplaceErrors::FAILED_REPLACE_CLASS_HASH_B2A => "FAILED_REPLACE_CLASS_HASH_B2A", } } diff --git a/packages/utils/src/components/replaceability/interface.cairo b/packages/utils/src/components/replaceability/interface.cairo index 5442e628..bfd439a8 100644 --- a/packages/utils/src/components/replaceability/interface.cairo +++ b/packages/utils/src/components/replaceability/interface.cairo @@ -48,10 +48,6 @@ pub trait IReplaceable { ); fn remove_implementation(ref self: TContractState, implementation_data: ImplementationData); fn replace_to(ref self: TContractState, implementation_data: ImplementationData); - // Always panics by design — Should be invoked only via library_call from - // `add_new_implementation`, never directly. - // It must live in `IReplaceable` so every contract embedding - // `ReplaceabilityImpl` exports the selector; otherwise the library_call dispatch fails. fn validate_upgradeability(ref self: TContractState, implementation_data: ImplementationData); } diff --git a/packages/utils/src/components/replaceability/mock_v2.cairo b/packages/utils/src/components/replaceability/mock_v2.cairo index 1d7303d1..11630ab6 100644 --- a/packages/utils/src/components/replaceability/mock_v2.cairo +++ b/packages/utils/src/components/replaceability/mock_v2.cairo @@ -1,10 +1,4 @@ -// Functionally identical to `ReplaceabilityMock` but compiles to a different class hash -// (via the unused `_v2_marker` field). Used as a valid upgrade target in tests where the -// target must include the replaceability component yet differ from the deployed contract. -// -// IMPORTANT: keep this in lockstep with `mock.cairo`. The validation tests assume the two -// classes share the same upgrade machinery; any change to the components, storage layout, or -// constructor of `ReplaceabilityMock` must be mirrored here. +// Variant of `ReplaceabilityMock` with a class hash distinctly different than `mock.cairo`. #[starknet::contract] pub(crate) mod ReplaceabilityMockV2 { use CommonRolesComponent::InternalTrait as CommonRolesInternalTrait; diff --git a/packages/utils/src/components/replaceability/replaceability.cairo b/packages/utils/src/components/replaceability/replaceability.cairo index a15cb3ee..8bf1318a 100644 --- a/packages/utils/src/components/replaceability/replaceability.cairo +++ b/packages/utils/src/components/replaceability/replaceability.cairo @@ -73,11 +73,7 @@ pub(crate) mod ReplaceabilityComponent { self.impl_activation_time.read(impl_key) } - // Schedules a new implementation and validates that it is upgradeable. If the target - // cannot itself perform a full upgrade cycle (add + replace, in both directions), the - // entire transaction reverts. Final adds (`implementation_data.final = true`) are - // rejected with `FINALIZE_IS_UNSAFE` — to finalize, use - // `add_new_implementation_unsafe` instead. + // Schedule a new implementation upgrade and validates the implementation upgradeability. fn add_new_implementation( ref self: ComponentState, implementation_data: ImplementationData, ) { @@ -86,16 +82,10 @@ pub(crate) mod ReplaceabilityComponent { self.add_new_implementation_unsafe(implementation_data); } - // Schedules a new implementation without running upgradeability validation. Bypassing - // this check can permanently brick the contract if the new code lacks a working upgrade - // path — only use when the target has been validated through some other means. + // Schedule a new implementation upgrade without validation. fn add_new_implementation_unsafe( ref self: ComponentState, implementation_data: ImplementationData, ) { - // Authoritative auth check for direct callers. Also load-bearing during - // validation: step 2's `impl_b_dispatcher.add_new_implementation_unsafe` runs - // this check on the target class, so a target with broken `upgrade_governor` - // role wiring fails the dry-run cycle here. let common_roles = get_dep_component!(@self, CommonRoles); common_roles.only_upgrade_governor(); @@ -109,7 +99,6 @@ pub(crate) mod ReplaceabilityComponent { fn remove_implementation( ref self: ComponentState, implementation_data: ImplementationData, ) { - // The call is restricted to the upgrade governor. let common_roles = get_dep_component!(@self, CommonRoles); common_roles.only_upgrade_governor(); @@ -147,10 +136,8 @@ pub(crate) mod ReplaceabilityComponent { assert!(impl_activation_time <= now, "{}", ReplaceErrors::NOT_ENABLED_YET); assert!(now <= impl_expiration_time, "{}", ReplaceErrors::IMPLEMENTATION_EXPIRED); - // We emit now so that finalize emits last (if it does). self.emit(ImplementationReplaced { implementation_data }); - // Finalize implementation, if needed. if (implementation_data.final) { self.finalize(); self.emit(ImplementationFinalized { impl_hash: implementation_data.impl_hash }); @@ -181,49 +168,31 @@ pub(crate) mod ReplaceabilityComponent { self.set_impl_expiration_time(:implementation_data, expiration_time: 0); } - // Dry-run validation of a full upgrade cycle for the user's `implementation_data`. - // Always panics — `UPGRADEABILITY_VALIDATION_SUCCESS` on success, or the underlying - // failure otherwise. Callers wrap this in a library_call so the panic reverts every - // side effect: storage writes, emitted events, and the `replace_class_syscall`s in - // both `replace_to`s below. - // - // Step 1 (A→B): runs the user's add + replace on `self`, exercising the user's EIC - // and `final` flag through the actual production code path. After step 1 the on-chain - // class hash is briefly impl_b — the outer panic reverts it. - // - // Step 2 (B→A): library-dispatches an add + replace back to impl_a on impl_b. This - // proves the new class can itself perform an upgrade cycle and catches a target that - // lacks `add_new_implementation_unsafe` / `replace_to` or has a broken role wiring. - // Step 2 carries `eic_data: None` and `final: false` — the round trip's purpose is - // to exercise the upgrade machinery, not run another EIC or burn finalization. - // - // Threat model: guards against an upgrade_governor accidentally scheduling a - // non-upgradeable class. A malicious governor can still brick via - // `add_new_implementation_unsafe`. A malicious target cannot spoof the success - // sentinel — the runtime appends `'ENTRYPOINT_FAILED'` per dispatch frame, so a - // spoofed panic from a step-2 dispatched function fails the exact-array match in - // `invoke_upgradeability_validation`. + // Dry-run a full upgrade cycle: + // 1. User planned upgrade (A->B). + // 2. From target implementation (B->A). + // Always panics to revert side-effects: + // `UPGRADEABILITY_VALIDATION_SUCCESS` on success, or the underlying error otherwise. fn validate_upgradeability( ref self: ComponentState, implementation_data: ImplementationData, ) { - // impl_a_hash: current contract class hash. + // impl_a_hash: current class-hash. impl_b_hash: target class-hash. let impl_a_hash = get_class_hash_at_syscall(get_contract_address()).unwrap_syscall(); - - // impl_b_hash: class hash of user-planned upgrade. let impl_b_hash = implementation_data.impl_hash; - // Step 1 (A→B): complete the user-planned upgrade on `self`. Zero the delay so - // the add yields `activation_time = block_timestamp`, which satisfies - // `replace_to`'s `activation_time <= now` check. Relies on the production - // invariant that `block_timestamp > 0`; tests cheat the timestamp in - // `deploy_replaceability_mock` to bridge snforge's zero default. + // Step 1 (A->B): User planned upgrade. + // Zero the upgrade delay to allow instant add & replace. self.upgrade_delay.write(0); self.add_new_implementation_unsafe(implementation_data); self.replace_to(implementation_data); + assert!( + get_class_hash_at_syscall(get_contract_address()).unwrap_syscall() == impl_b_hash, + "{}", + ReplaceErrors::FAILED_REPLACE_CLASS_HASH_A2B, + ); - // Step 2 (B→A): library-dispatch an upgrade back to impl_a using impl_b's own - // machinery. Re-zero `upgrade_delay` because step 1's EIC is the only legitimate - // way to modify it, and step 2's timing check requires activation_time <= now. + // Step 2 (B->A): Upgrade back to impl_a using target class code. + // Re-zero upgrade delay in case EIC altered it. self.upgrade_delay.write(0); let back2a_impl_data = ImplementationData { impl_hash: impl_a_hash, eic_data: Option::None, final: false, @@ -232,23 +201,16 @@ pub(crate) mod ReplaceabilityComponent { impl_b_dispatcher.add_new_implementation_unsafe(implementation_data: back2a_impl_data); impl_b_dispatcher.replace_to(implementation_data: back2a_impl_data); - // Defends against a target whose `replace_to` returns Ok without actually - // calling `replace_class_syscall` (e.g. a hostile no-op re-implementation). The - // step-1 equivalent is unnecessary because step 1 runs `self`'s own `replace_to`, - // which already asserts the syscall succeeded. assert!( get_class_hash_at_syscall(get_contract_address()).unwrap_syscall() == impl_a_hash, "{}", ReplaceErrors::FAILED_REPLACE_CLASS_HASH_B2A, ); - // Load-bearing: the panic is what reverts every side effect above — the - // `upgrade_delay` writes, the storage entries from the two adds, the emitted - // events, and (critically) the `replace_class_syscall`s in both `replace_to`s. - // A normal return here would leave the contract in step 2's state. core::panic_with_felt252(UPGRADEABILITY_VALIDATION_SUCCESS); } } + #[generate_trait] pub impl InternalReplaceabilityImpl< TContractState, +HasComponent, +Drop, @@ -264,35 +226,25 @@ pub(crate) mod ReplaceabilityComponent { impl PrivateReplaceabilityImpl< TContractState, +HasComponent, +Drop, > of PrivateReplaceabilityTrait { - // Runs `validate_upgradeability(impl_data)` via library_call against the contract's - // current (trusted) class hash — the validation logic comes from this class, not from - // the untrusted target referenced in `impl_data`. - // - // Returns silently on `UPGRADEABILITY_VALIDATION_SUCCESS` (the library call's side - // effects are reverted by the runtime). Any other panic is propagated, reverting the - // caller's transaction. + // Invoke `validate_upgradeability` via library_call on the current class. + // Returns on the success sentinel; propagates any other panic. fn invoke_upgradeability_validation( - ref self: ComponentState, impl_data: ImplementationData, + ref self: ComponentState, implementation_data: ImplementationData, ) { let current_class_hash = get_class_hash_at_syscall(get_contract_address()) .unwrap_syscall(); let mut calldata = array![]; - impl_data.serialize(ref calldata); + implementation_data.serialize(ref calldata); let result = library_call_syscall( class_hash: current_class_hash, function_selector: selector!("validate_upgradeability"), calldata: calldata.span(), ); - // Unreachable in practice — validate_upgradeability always panics. + // validate_upgradeability always panics. let panic_data = result.expect_err('VALIDATION_DID_NOT_PANIC'); - // On success, validate_upgradeability panics with the - // UPGRADEABILITY_VALIDATION_SUCCESS sentinel; the Starknet runtime then appends - // 'ENTRYPOINT_FAILED' (the runtime-dictated suffix for any failed entry point) - // to the panic data. Match the exact 2-element pattern: a target that spoofs - // the sentinel from its own dispatched function gets an extra runtime suffix - // and fails this comparison. + // Catch the success sentinel panic, re-throw any other panic. if panic_data != array![UPGRADEABILITY_VALIDATION_SUCCESS, 'ENTRYPOINT_FAILED'] { core::panics::panic(panic_data); } diff --git a/packages/utils/src/components/replaceability/test.cairo b/packages/utils/src/components/replaceability/test.cairo index 30a05e64..36c6b394 100644 --- a/packages/utils/src/components/replaceability/test.cairo +++ b/packages/utils/src/components/replaceability/test.cairo @@ -9,8 +9,8 @@ mod ReplaceabilityTests { }; use replaceability::mock::ReplaceabilityMock; use replaceability::test_utils::Constants::{ - DEFAULT_UPGRADE_DELAY, DUMMY_FINAL_IMPLEMENTATION_DATA, DUMMY_NONFINAL_IMPLEMENTATION_DATA, - EIC_UPGRADE_DELAY_ADDITION, GOVERNANCE_ADMIN, NOT_UPGRADE_GOVERNOR_ACCOUNT, + DEFAULT_UPGRADE_DELAY, DUMMY_NONFINAL_IMPLEMENTATION_DATA, EIC_UPGRADE_DELAY_ADDITION, + GOVERNANCE_ADMIN, NOT_UPGRADE_GOVERNOR_ACCOUNT, }; use replaceability::test_utils::{ assert_finalized_status, assert_implementation_finalized_event_emitted, @@ -67,7 +67,6 @@ mod ReplaceabilityTests { fn test_add_new_implementation() { let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; - // Use a real upgradeable class hash so validation at add time can dispatch into it. let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( class_hash: get_replaceability_mock_v2_class_hash(), ); @@ -80,7 +79,7 @@ mod ReplaceabilityTests { ); let mut spy = spy_events(); replaceable_dispatcher.add_new_implementation(:implementation_data); - // Test setup pins block_timestamp to 1, so activation_time = 1 + DEFAULT_UPGRADE_DELAY. + // block_timestamp pinned to 1 in deploy. assert!( replaceable_dispatcher .get_impl_activation_time(:implementation_data) == DEFAULT_UPGRADE_DELAY @@ -108,9 +107,6 @@ mod ReplaceabilityTests { fn test_add_new_implementation_not_upgrade_governor() { let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; - // class_hash = 0 is intentional: the auth check inside validation's first call fires - // before anything dereferences the class hash. If that order ever flips, this would - // surface as "class not declared" instead of ONLY_UPGRADE_GOVERNOR. let implementation_data = DUMMY_NONFINAL_IMPLEMENTATION_DATA(); // Invoke not as an Upgrade Governor. @@ -122,7 +118,6 @@ mod ReplaceabilityTests { fn test_remove_implementation() { let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; - // Use a real upgradeable class hash so validation at add time can dispatch into it. let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( class_hash: get_replaceability_mock_v2_class_hash(), ); @@ -230,7 +225,7 @@ mod ReplaceabilityTests { let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; - // Deploy a V2 mock to get a different class hash that still includes replaceability. + // new_class_hash is explicitly different than the current class hash. let new_class_hash = get_replaceability_mock_v2_class_hash(); assert_ne!(get_class_hash(:contract_address), new_class_hash); @@ -333,7 +328,6 @@ mod ReplaceabilityTests { fn test_replace_to_not_upgrade_governor() { let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; - // Use a real declared class hash so the validation library_call can resolve it. let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( class_hash: get_replaceability_mock_v2_class_hash(), ); @@ -353,7 +347,6 @@ mod ReplaceabilityTests { cheat_caller_address_once( :contract_address, caller_address: get_upgrade_governor_account(:contract_address), ); - // Use a real declared class hash so the validation library_call can resolve it. let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( class_hash: get_replaceability_mock_v2_class_hash(), ); @@ -380,7 +373,9 @@ mod ReplaceabilityTests { let implementation_data = dummy_nonfinal_implementation_data_with_class_hash( class_hash: get_class_hash(contract_address), ); - let other_implementation_data = DUMMY_FINAL_IMPLEMENTATION_DATA(); + let other_implementation_data = dummy_nonfinal_implementation_data_with_class_hash( + class_hash: get_replaceability_mock_v2_class_hash(), + ); // Invoke as an Upgrade Governor. cheat_caller_address( @@ -389,11 +384,9 @@ mod ReplaceabilityTests { span: CheatSpan::TargetCalls(8), ); - // Add implementations. The "other" impl is final and points to a placeholder - // class_hash, so it must go through `_unsafe` to bypass the FINALIZE_IS_UNSAFE assert. replaceable_dispatcher.add_new_implementation(:implementation_data); replaceable_dispatcher - .add_new_implementation_unsafe(implementation_data: other_implementation_data); + .add_new_implementation(implementation_data: other_implementation_data); assert!( replaceable_dispatcher.get_impl_activation_time(:implementation_data).is_non_zero(), ); @@ -446,8 +439,6 @@ mod ReplaceabilityTests { span: CheatSpan::TargetCalls(2), ); let mut spy = spy_events(); - // Final adds must go through `_unsafe` — the safe path rejects with - // FINALIZE_IS_UNSAFE. replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data); // Advance time to enable implementation. @@ -494,8 +485,6 @@ mod ReplaceabilityTests { caller_address: get_upgrade_governor_account(:contract_address), span: CheatSpan::TargetCalls(3), ); - // Final adds must go through `_unsafe` — the safe path rejects with - // FINALIZE_IS_UNSAFE. replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data); // Advance time to enable implementation. @@ -538,9 +527,6 @@ mod ReplaceabilityTests { #[test] #[feature("safe_dispatcher")] fn test_add_new_implementation_blocks_non_upgradeable() { - // Adding a non-final implementation that points to a contract without the replaceability - // component should fail at add time — validation cannot complete the dry-run upgrade - // cycle on a target that lacks `add_new_implementation_unsafe` or `replace_to`. let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; let safe_dispatcher = IReplaceableSafeDispatcher { contract_address }; @@ -562,9 +548,7 @@ mod ReplaceabilityTests { #[test] #[feature("safe_dispatcher")] fn test_add_new_implementation_rejects_final() { - // Final adds via the safe path are rejected with FINALIZE_IS_UNSAFE — bricking the - // upgrade path is intentional and must go through `_unsafe` to prove intent. The - // class hash never matters here; the assert fires before validation runs. + // add_new_implementation intentional reverts a final impl with FINALIZE_IS_UNSAFE. let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; let safe_dispatcher = IReplaceableSafeDispatcher { contract_address }; @@ -583,7 +567,6 @@ mod ReplaceabilityTests { Result::Err(_) => (), } - // Migration path: same data via `_unsafe` succeeds. replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data); assert!( replaceable_dispatcher.get_impl_activation_time(:implementation_data).is_non_zero(), @@ -592,9 +575,6 @@ mod ReplaceabilityTests { #[test] fn test_upgradeability_validation_no_side_effects() { - // After a successful add_new_implementation, verify that the validation dry-run did - // not corrupt storage (upgrade_delay should be unchanged — validation writes 0 in the - // dry-run state, which is reverted by the library_call panic). let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; @@ -613,9 +593,6 @@ mod ReplaceabilityTests { #[test] #[feature("safe_dispatcher")] fn test_add_new_implementation_blocks_broken_eic() { - // Validation runs the user's EIC during step 1's `replace_to`. An EIC that panics - // on init must surface as a rejected add. The broken EIC here is the test EIC - // called with empty init_data, which trips its own length-check assert. let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; let safe_dispatcher = IReplaceableSafeDispatcher { contract_address }; @@ -635,9 +612,6 @@ mod ReplaceabilityTests { #[test] fn test_add_new_implementation_unsafe_succeeds_for_invalid_target() { - // add_new_implementation_unsafe bypasses validation — an impl pointing to a contract - // without the replaceability component is accepted. Verify the activation/expiration - // entries are written. let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; @@ -651,7 +625,7 @@ mod ReplaceabilityTests { ); replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data); - // Test setup pins block_timestamp to 1, so activation_time = 1 + DEFAULT_UPGRADE_DELAY. + // block_timestamp pinned to 1 in deploy. assert!( replaceable_dispatcher .get_impl_activation_time(:implementation_data) == DEFAULT_UPGRADE_DELAY @@ -664,8 +638,6 @@ mod ReplaceabilityTests { fn test_add_new_implementation_unsafe_not_upgrade_governor() { let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; - // class_hash = 0 is intentional: auth fires before any storage write, so the - // placeholder class is never used. let implementation_data = DUMMY_NONFINAL_IMPLEMENTATION_DATA(); cheat_caller_address_once(:contract_address, caller_address: NOT_UPGRADE_GOVERNOR_ACCOUNT); @@ -675,15 +647,11 @@ mod ReplaceabilityTests { #[test] #[feature("safe_dispatcher")] fn test_add_new_implementation_blocked_when_finalized() { - // After finalization, add_new_implementation rejects — validation step 1's - // self.replace_to reads the finalized flag and panics. add_new_implementation_unsafe - // still succeeds because it bypasses validation entirely. let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; let safe_dispatcher = IReplaceableSafeDispatcher { contract_address }; - // Finalize: schedule a final impl pointing to self (via `_unsafe` since the safe path - // rejects final adds), then apply it. + // Finalize the contract. let final_data = dummy_final_implementation_data_with_class_hash( class_hash: get_class_hash(:contract_address), ); @@ -697,8 +665,6 @@ mod ReplaceabilityTests { replaceable_dispatcher.replace_to(implementation_data: final_data); assert_finalized_status(expected: true, :contract_address); - // add_new_implementation must now fail — validation step 1's self.replace_to hits the - // finalized flag. let nonfinal_data = dummy_nonfinal_implementation_data_with_class_hash( class_hash: get_replaceability_mock_v2_class_hash(), ); @@ -707,7 +673,7 @@ mod ReplaceabilityTests { Result::Err(_) => (), } - // add_new_implementation_unsafe still works — it bypasses validation. + // _unsafe bypasses validation. replaceable_dispatcher.add_new_implementation_unsafe(implementation_data: nonfinal_data); assert!( replaceable_dispatcher @@ -719,11 +685,6 @@ mod ReplaceabilityTests { #[test] #[feature("safe_dispatcher")] fn test_intentional_brick_via_unsafe_add() { - // add_new_implementation_unsafe lets an upgrade_governor schedule an impl that would - // otherwise be blocked by validation. Once scheduled, replace_to applies it — bricking - // the contract because the new code has no upgrade machinery. This test confirms both - // halves: the unsafe path succeeds where the safe path would have blocked, AND the - // resulting contract cannot be upgraded further. let replaceable_dispatcher = deploy_replaceability_mock(); let contract_address = replaceable_dispatcher.contract_address; let safe_dispatcher = IReplaceableSafeDispatcher { contract_address }; @@ -741,12 +702,10 @@ mod ReplaceabilityTests { replaceable_dispatcher.add_new_implementation_unsafe(:implementation_data); cheat_block_timestamp(contract_address, DEFAULT_UPGRADE_DELAY + 1, CheatSpan::Indefinite); - // Apply the bad impl via the normal replace_to — succeeds because validation already - // ran (or rather, was bypassed) at add time. replaceable_dispatcher.replace_to(:implementation_data); assert_eq!(get_class_hash(:contract_address), dummy_class_hash); - // Contract is now bricked: any upgrade-related call hits a non-existent entry point. + // Contract is now bricked. match safe_dispatcher.add_new_implementation(:implementation_data) { Result::Ok(_) => panic!("Bricked contract should not accept add_new_implementation"), Result::Err(_) => (), diff --git a/packages/utils/src/components/replaceability/test_utils.cairo b/packages/utils/src/components/replaceability/test_utils.cairo index 35bb4492..0a0f45b6 100644 --- a/packages/utils/src/components/replaceability/test_utils.cairo +++ b/packages/utils/src/components/replaceability/test_utils.cairo @@ -50,15 +50,11 @@ pub(crate) fn deploy_replaceability_mock() -> IReplaceableDispatcher { @array![Constants::DEFAULT_UPGRADE_DELAY.into(), Constants::GOVERNANCE_ADMIN.into()], ) .unwrap(); - // Pin block_timestamp to a non-zero baseline so the upgradeability validation dry-run - // (which sets upgrade_delay to 0 internally) yields a non-zero activation_time. Tests - // can override by calling `cheat_block_timestamp` again. + // Pin block_timestamp > 0 so the validation dry-run yields a non-zero activation_time. cheat_block_timestamp(:contract_address, block_timestamp: 1, span: CheatSpan::Indefinite); return IReplaceableDispatcher { contract_address: contract_address }; } -// Returns the class hash of `ReplaceabilityMockV2` — a valid upgrade target distinct from -// `ReplaceabilityMock`'s class hash. pub(crate) fn get_replaceability_mock_v2_class_hash() -> ClassHash { *declare("ReplaceabilityMockV2").unwrap().contract_class().class_hash } @@ -111,8 +107,7 @@ pub(crate) fn dummy_nonfinal_eic_implementation_data_with_class_hash( ImplementationData { impl_hash: class_hash, eic_data: Option::Some(eic_data), final: false } } -// Returns implementation_data wired to `EICTestContract` with empty init_data — the EIC -// asserts `eic_init_data.len() == 1` and panics with EIC_INIT_DATA_LEN_MISMATCH on init. +// Empty init_data trips the EIC's length-check assert. pub(crate) fn dummy_nonfinal_broken_eic_implementation_data_with_class_hash( class_hash: ClassHash, ) -> ImplementationData { From bd0f64b8839b622e8487c3b7a8dddffae6a1cd94 Mon Sep 17 00:00:00 2001 From: Remo Date: Mon, 4 May 2026 20:02:14 +0300 Subject: [PATCH 4/4] Bump scarb -> 2.18.0 & foundry -> 0.60.0 --- .github/workflows/on-pull-request.yaml | 4 ++-- .github/workflows/publish-testing.yml | 2 +- .github/workflows/publish.yml | 2 +- .tool-versions | 4 ++-- Scarb.lock | 8 ++++---- Scarb.toml | 8 ++++---- packages/testing/Scarb.toml | 4 +++- packages/utils/Scarb.toml | 4 +++- 8 files changed, 20 insertions(+), 16 deletions(-) diff --git a/.github/workflows/on-pull-request.yaml b/.github/workflows/on-pull-request.yaml index 41808e1a..10935495 100644 --- a/.github/workflows/on-pull-request.yaml +++ b/.github/workflows/on-pull-request.yaml @@ -13,10 +13,10 @@ jobs: - uses: actions/checkout@v4 - uses: foundry-rs/setup-snfoundry@v3 with: - starknet-foundry-version: "0.55.0" + starknet-foundry-version: "0.60.0" - uses: software-mansion/setup-scarb@v1 with: - scarb-version: "2.15.1" + scarb-version: "2.18.0" - name: Install cairo-coverage run: | diff --git a/.github/workflows/publish-testing.yml b/.github/workflows/publish-testing.yml index a997073b..fc89e641 100644 --- a/.github/workflows/publish-testing.yml +++ b/.github/workflows/publish-testing.yml @@ -22,7 +22,7 @@ jobs: - uses: software-mansion/setup-scarb@v1 with: - scarb-version: "2.11.4" + scarb-version: "2.18.0" - name: Extract current versions working-directory: ./packages/testing diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index ef05bcfb..5261f7ab 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -22,7 +22,7 @@ jobs: - uses: software-mansion/setup-scarb@v1 with: - scarb-version: "2.11.4" + scarb-version: "2.18.0" - name: Extract publish version run: | diff --git a/.tool-versions b/.tool-versions index 05e7d65d..0cb6bbaf 100644 --- a/.tool-versions +++ b/.tool-versions @@ -1,2 +1,2 @@ -scarb 2.15.1 -starknet-foundry 0.55.0 +scarb 2.18.0 +starknet-foundry 0.60.0 diff --git a/Scarb.lock b/Scarb.lock index 2f5c226d..20da88f5 100644 --- a/Scarb.lock +++ b/Scarb.lock @@ -141,15 +141,15 @@ dependencies = [ [[package]] name = "snforge_scarb_plugin" -version = "0.55.0" +version = "0.60.0" source = "registry+https://scarbs.xyz/" -checksum = "sha256:638535780a23d1491c2438e64045c479d16de6a69e41ad17ac065272c485873b" +checksum = "sha256:924358bf316e502923f6733b50e239ea37585a05dc24c5fc8dd9e45f88cf7339" [[package]] name = "snforge_std" -version = "0.55.0" +version = "0.60.0" source = "registry+https://scarbs.xyz/" -checksum = "sha256:a04b0bf731f02307506dad368713099e701565edd9b98b044ca54b932c29ef74" +checksum = "sha256:32e6baabec4f9af21089bc7ca685ffea5e4164497340ecbdb99314e568029195" dependencies = [ "snforge_scarb_plugin", ] diff --git a/Scarb.toml b/Scarb.toml index f0b27564..38a1d217 100644 --- a/Scarb.toml +++ b/Scarb.toml @@ -17,16 +17,16 @@ repository = "https://github.com/starkware-libs/starkware-starknet-utils" version = "1.0.0" [workspace.dependencies] -assert_macros = "2.15.1" +assert_macros = "2.18.0" openzeppelin = "3.0.0" -snforge_std = "0.55.0" -starknet = "2.15.1" +snforge_std = "0.60.0" +starknet = "2.18.0" [workspace.tool.fmt] sort-module-level-items = true [workspace.tool.scarb] -allow-prebuilt-plugins = ["snforge_std"] +allow-prebuilt-plugins = ["snforge_std", "snforge_scarb_plugin"] [profile.dev.cairo] inlining-strategy = "avoid" diff --git a/packages/testing/Scarb.toml b/packages/testing/Scarb.toml index e007d11e..25ec994b 100644 --- a/packages/testing/Scarb.toml +++ b/packages/testing/Scarb.toml @@ -29,7 +29,9 @@ test = "SNFORGE_BACKTRACE=1 snforge test" [tool] fmt.workspace = true -scarb.workspace = true + +[tool.scarb] +allow-prebuilt-plugins = ["snforge_std", "snforge_scarb_plugin"] [profile.dev.cairo] unstable-add-statements-functions-debug-info = true diff --git a/packages/utils/Scarb.toml b/packages/utils/Scarb.toml index af82eb10..944b8df7 100644 --- a/packages/utils/Scarb.toml +++ b/packages/utils/Scarb.toml @@ -27,7 +27,9 @@ name = "starkware_utils_unittest" [tool] fmt.workspace = true -scarb.workspace = true + +[tool.scarb] +allow-prebuilt-plugins = ["snforge_std", "snforge_scarb_plugin"] [profile.dev.cairo] unstable-add-statements-functions-debug-info = true