Fix Governor timelock salt (#1305)

* fix: xor issue

* feat: add invariant comment

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.

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()
 }
 
 //

packages/test_common/src/mocks/governor.cairo

@@ -399,10 +399,20 @@ 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)
+        }
     }
 }
 
 #[starknet::interface]
 pub trait CancelOperations<TContractState> {
     fn cancel_operations(ref self: TContractState, proposal_id: felt252, description_hash: felt252);
 }
+
+#[starknet::interface]
+pub trait TimelockSalt<TContractState> {
+    fn timelock_salt(ref self: TContractState, description_hash: felt252) -> felt252;
+}

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",
 ]