Fixes the hash collision reported by Sergio @ Argent (#38)

src/call_trait.cairo

@@ -1,21 +1,21 @@
 use core::array::{ArrayTrait, SpanTrait};
-use core::hash::{LegacyHash, HashStateTrait, HashStateExTrait, Hash};
+use core::hash::{HashStateTrait, HashStateExTrait, Hash};
 use core::result::{ResultTrait};
+use core::serde::{Serde};
 use core::traits::{Into};
 use starknet::account::{Call};
 use starknet::{ContractAddress};
 use starknet::{SyscallResult, syscalls::call_contract_syscall};
 
-pub impl HashCall<S, +HashStateTrait<S>, +Drop<S>, +Copy<S>> of Hash<@Call, S> {
-    fn update_state(state: S, value: @Call) -> S {
-        let mut s = state.update_with((*value.to)).update_with(*value.selector);
-
-        let mut data_span: Span<felt252> = *value.calldata;
-        while let Option::Some(word) = data_span.pop_front() {
-            s = s.update(*word);
+pub impl HashSerializable<T, S, +Serde<T>, +HashStateTrait<S>, +Drop<S>> of Hash<@T, S> {
+    fn update_state(mut state: S, value: @T) -> S {
+        let mut arr = array![];
+        Serde::serialize(value, ref arr);
+        state = state.update(arr.len().into());
+        while let Option::Some(word) = arr.pop_front() {
+            state = state.update(word)
         };
-
-        s
+        state
     }
 }
 

src/call_trait_test.cairo

@@ -1,7 +1,8 @@
 use core::array::{Array, ArrayTrait};
-use core::hash::{LegacyHash};
+use core::hash::{LegacyHash, HashStateExTrait, HashStateTrait};
+use core::poseidon::{PoseidonTrait};
 use core::serde::{Serde};
-use governance::call_trait::{CallTrait, HashCall};
+use governance::call_trait::{CallTrait, HashSerializable};
 use governance::test::test_token::{deploy as deploy_token};
 use starknet::{contract_address_const, get_contract_address, account::{Call}};
 
@@ -10,7 +11,7 @@ fn test_hash_empty() {
     let call = Call { to: contract_address_const::<0>(), selector: 0, calldata: array![].span() };
     assert_eq!(
         LegacyHash::hash(0, @call),
-        0x6bf1b215edde951b1b50c19e77f7b362d23c6cb4232ae8b95bc112ff94d3956
+        592531356294457842089938121745653035784273932434733687203842865999223838417
     );
 }
 
@@ -19,7 +20,7 @@ fn test_hash_empty_different_state() {
     let call = Call { to: contract_address_const::<0>(), selector: 0, calldata: array![].span() };
     assert_eq!(
         LegacyHash::hash(1, @call),
-        1832368551659298682277041292338758811780503233378895654121359846824467233868
+        641779498390055840747899186344080567584946769797748441152535133488389427639
     );
 }
 
@@ -28,15 +29,16 @@ fn test_hash_address_one() {
     let call = Call { to: contract_address_const::<1>(), selector: 0, calldata: array![].span() };
     assert_eq!(
         LegacyHash::hash(0, @call),
-        0x5f6208726bc717f95f23a8e3632dd5a30f4b61d11db5ea4f4fab24bf931a053
+        822101510419032526850572827036529302322534847455039029719271666012578939011
     );
 }
 
 #[test]
 fn test_hash_address_entry_point_one() {
     let call = Call { to: contract_address_const::<0>(), selector: 1, calldata: array![].span() };
     assert_eq!(
-        LegacyHash::hash(0, @call), 0x137c95c76862129847d0f5e3618c7a4c3822ee344f4aa80bcb897cb97d3e16
+        LegacyHash::hash(0, @call),
+        2649728997388989667623494598440207295058382579827039351281187174838687580826
     );
 }
 
@@ -46,7 +48,7 @@ fn test_hash_address_data_one() {
 
     assert_eq!(
         LegacyHash::hash(0, @call),
-        0x200a54d7737c13f1013835f88c566515921c2b9c7c7a50cc44ff6f176cf06b2
+        941644435636445739851438544160869526074009333114430642156303468449419287369
     );
 }
 
@@ -58,7 +60,7 @@ fn test_hash_address_data_one_two() {
 
     assert_eq!(
         LegacyHash::hash(0, @call),
-        0x6f615c05fa309e4041f96f83d47a23acec3d725b47f8c1005f388aa3d26c187
+        2486526694913415670406871967728275622122901127926391718078378231682443645806
     );
 }
 
@@ -113,3 +115,24 @@ fn test_execute_valid_call_data() {
     call.execute();
 }
 
+#[test]
+fn test_hash_no_collision_span_length() {
+    let call_a1 = Call {
+        to: contract_address_const::<1>(), selector: 2, calldata: array![3, 4].span()
+    };
+    let call_a2 = Call {
+        to: contract_address_const::<5>(), selector: 6, calldata: array![].span()
+    };
+    let hash_a = PoseidonTrait::new().update_with(@call_a1).update_with(@call_a2).finalize();
+
+    let call_b1 = Call {
+        to: contract_address_const::<1>(), selector: 2, calldata: array![].span()
+    };
+    let call_b2 = Call {
+        to: contract_address_const::<3>(), selector: 4, calldata: array![5, 6].span()
+    };
+    let hash_b = PoseidonTrait::new().update_with(@call_b1).update_with(@call_b2).finalize();
+
+    assert_ne!(hash_a, hash_b);
+}
+

src/factory.cairo

@@ -69,7 +69,7 @@ pub mod Factory {
         fn get_timelock_class_hash(self: @ContractState) -> ClassHash {
             self.timelock_class_hash.read()
         }
-        
+
         fn deploy(
             ref self: ContractState, token: ContractAddress, params: DeploymentParameters
         ) -> DeploymentResult {

src/governor.cairo

@@ -68,9 +68,10 @@ pub trait IGovernor<TContractState> {
 
 #[starknet::contract]
 pub mod Governor {
-    use core::hash::{LegacyHash};
+    use core::hash::{HashStateTrait, HashStateExTrait};
     use core::num::traits::zero::{Zero};
-    use governance::call_trait::{HashCall, CallTrait};
+    use core::poseidon::{PoseidonTrait};
+    use governance::call_trait::{HashSerializable, CallTrait};
     use governance::staker::{IStakerDispatcherTrait};
     use starknet::{get_block_timestamp, get_caller_address, contract_address_const};
     use super::{
@@ -137,7 +138,10 @@ pub mod Governor {
     }
 
     pub fn to_call_id(call: @Call) -> felt252 {
-        LegacyHash::hash(selector!("ekubo::governance::governor::Governor::to_call_id"), call)
+        PoseidonTrait::new()
+            .update(selector!("governance::governor::Governor::to_call_id"))
+            .update_with(call)
+            .finalize()
     }
 
     #[abi(embed_v0)]

src/governor_test.cairo

@@ -117,7 +117,7 @@ fn test_to_call_id() {
                 calldata: array![1, 2, 3].span()
             }
         ),
-        3468069799942858391288170742121635082941840484768382693792476025465085752161
+        1066148822741379800704886069792510413495941578585564382784326423848733446336
     );
 }
 

src/timelock.cairo

@@ -63,10 +63,11 @@ pub struct ExecutionWindow {
 
 #[starknet::contract]
 pub mod Timelock {
-    use core::hash::LegacyHash;
+    use core::hash::{HashStateExTrait, HashStateTrait};
     use core::num::traits::zero::{Zero};
-    use core::result::ResultTrait;
-    use governance::call_trait::{CallTrait, HashCall};
+    use core::poseidon::{PoseidonTrait, HashState as PoseidonHashState};
+    use core::result::{ResultTrait};
+    use governance::call_trait::{CallTrait, HashSerializable};
     use starknet::{
         get_caller_address, get_contract_address, SyscallResult,
         syscalls::{call_contract_syscall, replace_class_syscall}, get_block_timestamp
@@ -117,12 +118,11 @@ pub mod Timelock {
     // Take a list of calls and convert it to a unique identifier for the execution
     // Two lists of calls will always have the same ID if they are equivalent
     // A list of calls can only be queued and executed once. To make 2 different calls, add an empty call.
-    pub(crate) fn to_id(mut calls: Span<Call>) -> felt252 {
-        let mut state = selector!("ekubo::governance::Timelock::to_id");
-        while let Option::Some(call) = calls.pop_front() {
-            state = LegacyHash::hash(state, call);
-        };
-        state
+    pub fn to_calls_id(mut calls: Span<Call>) -> felt252 {
+        PoseidonTrait::new()
+            .update(selector!("governance::timelock::Timelock::to_calls_id"))
+            .update_with(@calls)
+            .finalize()
     }
 
     #[generate_trait]
@@ -141,7 +141,7 @@ pub mod Timelock {
         fn queue(ref self: ContractState, calls: Span<Call>) -> felt252 {
             self.check_owner();
 
-            let id = to_id(calls);
+            let id = to_calls_id(calls);
             let execution_state = self.execution_state.read(id);
 
             assert(execution_state.canceled.is_zero(), 'HAS_BEEN_CANCELED');
@@ -180,7 +180,7 @@ pub mod Timelock {
         }
 
         fn execute(ref self: ContractState, mut calls: Span<Call>) -> Array<Span<felt252>> {
-            let id = to_id(calls);
+            let id = to_calls_id(calls);
 
             let execution_state = self.execution_state.read(id);
 

src/timelock_test.cairo

@@ -2,7 +2,9 @@ use core::array::{Array, ArrayTrait, SpanTrait};
 use governance::execution_state::{ExecutionState};
 use governance::interfaces::erc20::{IERC20Dispatcher, IERC20DispatcherTrait};
 use governance::test::test_token::{deploy as deploy_token};
-use governance::timelock::{ITimelockDispatcher, ITimelockDispatcherTrait, Timelock, Config};
+use governance::timelock::{
+    ITimelockDispatcher, ITimelockDispatcherTrait, Timelock, Config, Timelock::{to_calls_id}
+};
 use starknet::account::{Call};
 use starknet::{
     get_contract_address, syscalls::{deploy_syscall}, ClassHash, contract_address_const,
@@ -144,3 +146,26 @@ fn test_queue_executed_too_late() {
     set_block_timestamp(execution_window.latest);
     timelock.execute(single_call(transfer_call(token, recipient, 500_u256)));
 }
+
+
+#[test]
+fn test_no_collision_to_calls_id() {
+    let call_a1 = Call {
+        to: contract_address_const::<1>(), selector: 2, calldata: array![3, 4].span()
+    };
+    let call_a2 = Call {
+        to: contract_address_const::<5>(), selector: 6, calldata: array![].span()
+    };
+
+    let call_b1 = Call {
+        to: contract_address_const::<1>(), selector: 2, calldata: array![].span()
+    };
+    let call_b2 = Call {
+        to: contract_address_const::<3>(), selector: 4, calldata: array![5, 6].span()
+    };
+
+    let calls_a: Span<Call> = array![call_a1, call_a2].span();
+    let calls_b: Span<Call> = array![call_b1, call_b2].span();
+
+    assert_ne!(to_calls_id(calls_a), to_calls_id(calls_b));
+}