diff --git a/packages/governance/src/governor/extensions/governor_timelock_execution.cairo b/packages/governance/src/governor/extensions/governor_timelock_execution.cairo index a3c141dd2..8e6c49412 100644 --- a/packages/governance/src/governor/extensions/governor_timelock_execution.cairo +++ b/packages/governance/src/governor/extensions/governor_timelock_execution.cairo @@ -39,6 +39,9 @@ pub mod GovernorTimelockExecutionComponent { type ProposalId = felt252; type TimelockProposalId = felt252; + /// This is P - 1 = 2**251 + 17 * 2 ** 192 + const MAX_FELT: felt252 = 0 - 1; + #[storage] pub struct Storage { pub Governor_timelock_controller: ContractAddress, @@ -277,7 +280,8 @@ pub mod GovernorTimelockExecutionComponent { } /// Computes the `TimelockController` operation salt as the XOR of - /// the governor address and `description_hash`. + /// the governor address and `description_hash`. In the case of overflow, it is + /// reduced modulo P. /// /// It is computed with the governor address itself to avoid collisions across /// governor instances using the same timelock. @@ -286,9 +290,16 @@ pub mod GovernorTimelockExecutionComponent { ) -> felt252 { let description_hash: u256 = description_hash.into(); let this: felt252 = starknet::get_contract_address().into(); + let max_felt: u256 = MAX_FELT.into(); - // Unwrap is safe since the u256 value came from a felt252. - (this.into() ^ description_hash).try_into().unwrap() + let mut value = this.into() ^ description_hash; + if value > max_felt { + // Get the value modulo P. + // Invariant: 2 * max_felt > 2 ** 252 - 1 >= value + value = value - max_felt - 1; + } + // Unwrap is safe since value is less or equal than MAX_FELT. + value.try_into().unwrap() } /// Returns the timelock contract address wrapped in a ITimelockDispatcher. diff --git a/packages/governance/src/tests/governor/test_governor_timelock_execution.cairo b/packages/governance/src/tests/governor/test_governor_timelock_execution.cairo index 5a64bc3cf..75ae40d1f 100644 --- a/packages/governance/src/tests/governor/test_governor_timelock_execution.cairo +++ b/packages/governance/src/tests/governor/test_governor_timelock_execution.cairo @@ -16,6 +16,9 @@ use openzeppelin_test_common::mocks::governor::GovernorMock::SNIP12MetadataImpl; use openzeppelin_test_common::mocks::governor::{ CancelOperationsDispatcher, CancelOperationsDispatcherTrait, GovernorTimelockedMock, }; +use openzeppelin_test_common::mocks::governor::{ + TimelockSaltDispatcher, TimelockSaltDispatcherTrait, +}; use openzeppelin_test_common::mocks::timelock::{ IMockContractDispatcher, IMockContractDispatcherTrait, }; @@ -26,9 +29,9 @@ use openzeppelin_utils::bytearray::ByteArrayExtTrait; use openzeppelin_utils::serde::SerializedAppend; use snforge_std::{EventSpy, spy_events, start_mock_call, store}; use snforge_std::{start_cheat_block_timestamp_global, start_cheat_caller_address}; -use starknet::ContractAddress; use starknet::account::Call; use starknet::storage::{StorageMapWriteAccess, StoragePathEntry, StoragePointerWriteAccess}; +use starknet::{ContractAddress, contract_address_const}; const MIN_DELAY: u64 = 100; @@ -45,6 +48,17 @@ fn deploy_governor(timelock: ContractAddress) -> IGovernorDispatcher { IGovernorDispatcher { contract_address: address } } +fn deploy_governor_at( + target_address: ContractAddress, timelock: ContractAddress, +) -> IGovernorDispatcher { + let mut calldata = array![]; + calldata.append_serde(VOTES_TOKEN()); + calldata.append_serde(timelock); + + utils::declare_and_deploy_at("GovernorTimelockedMock", target_address, calldata); + IGovernorDispatcher { contract_address: target_address } +} + fn deploy_timelock(admin: ContractAddress) -> ITimelockDispatcher { let proposers = array![admin].span(); let executors = array![admin].span(); @@ -81,6 +95,42 @@ fn setup_dispatchers() -> (IGovernorDispatcher, ITimelockDispatcher, IMockContra (governor, timelock, target) } +// +// Timelock salt +// + +#[test] +fn test_timelock_salt() { + let governor = deploy_governor(TIMELOCK()); + + let dispatcher = TimelockSaltDispatcher { contract_address: governor.contract_address }; + + let description = "proposal description"; + let description_hash = (@description).hash(); + let salt = dispatcher.timelock_salt(description_hash); + let expected = timelock_salt(governor.contract_address, description_hash); + + assert_eq!(salt, expected); +} + +#[test] +fn test_timelock_salt_overflow() { + // 2^250 + // 2^251 + let address = contract_address_const::< + 0x400000000000000000000000000000000000000000000000000000000000000, + >(); + let description_hash = 0x800000000000000000000000000000000000000000000000000000000000000; + + let governor = deploy_governor_at(address, TIMELOCK()); + let dispatcher = TimelockSaltDispatcher { contract_address: governor.contract_address }; + + let salt = dispatcher.timelock_salt(description_hash); + let expected = timelock_salt(governor.contract_address, description_hash); + + assert_eq!(salt, expected); +} + // // state // @@ -583,7 +633,13 @@ fn timelock_salt(contract_address: ContractAddress, description_hash: felt252) - let description_hash: u256 = description_hash.into(); let contract_address: felt252 = contract_address.into(); - (contract_address.into() ^ description_hash).try_into().unwrap() + let mut value = contract_address.into() ^ description_hash; + let max_felt: u256 = (0 - 1).into(); + if value > max_felt { + // Get the value modulo P. + value = value - max_felt - 1; + } + value.try_into().unwrap() } // diff --git a/packages/test_common/src/mocks/governor.cairo b/packages/test_common/src/mocks/governor.cairo index 9286f4d9b..ea41d1ec0 100644 --- a/packages/test_common/src/mocks/governor.cairo +++ b/packages/test_common/src/mocks/governor.cairo @@ -399,6 +399,11 @@ pub mod GovernorTimelockedMock { ) { self.governor.cancel_operations(proposal_id, description_hash); } + + #[external(v0)] + fn timelock_salt(ref self: ContractState, description_hash: felt252) -> felt252 { + self.governor_timelock_execution.timelock_salt(description_hash) + } } } @@ -406,3 +411,8 @@ pub mod GovernorTimelockedMock { pub trait CancelOperations { fn cancel_operations(ref self: TContractState, proposal_id: felt252, description_hash: felt252); } + +#[starknet::interface] +pub trait TimelockSalt { + fn timelock_salt(ref self: TContractState, description_hash: felt252) -> felt252; +} diff --git a/sncast_scripts/Scarb.lock b/sncast_scripts/Scarb.lock index fe21e097a..d70439ead 100644 --- a/sncast_scripts/Scarb.lock +++ b/sncast_scripts/Scarb.lock @@ -6,7 +6,6 @@ name = "openzeppelin_access" version = "0.20.0" dependencies = [ "openzeppelin_introspection", - "openzeppelin_utils", ] [[package]] @@ -84,15 +83,15 @@ checksum = "sha256:cfd7c73a6f9984880249babfa8664b69c5f7209c737b1081156a284061ccd [[package]] name = "snforge_scarb_plugin" -version = "0.32.0" +version = "0.34.0" source = "registry+https://scarbs.xyz/" -checksum = "sha256:e5a0e80294b1f5f00955c614ee3fc94c843ff0d27935693c3598d0ac8d79250a" +checksum = "sha256:56f2b06ff2f0d8bbdfb7fb6c211fba7e4da6e5334ea70ba849af329a739faf11" [[package]] name = "snforge_std" -version = "0.32.0" +version = "0.34.0" source = "registry+https://scarbs.xyz/" -checksum = "sha256:0e3cb45c6276334fd142a77212f0592d55744f1c022b7a63f20bcd79d0ce3927" +checksum = "sha256:bd20964bde07e6fd0f7adb50d41216f05d66abd422ed82241030369333385876" dependencies = [ "snforge_scarb_plugin", ]