Audit: setthresholdas constructor param

contracts/src/contracts/isms/multisig/merkleroot_multisig_ism.cairo

@@ -36,6 +36,7 @@ pub mod merkleroot_multisig_ism {
         pub const EMPTY_METADATA: felt252 = 'Empty metadata';
         pub const VALIDATOR_ADDRESS_CANNOT_BE_NULL: felt252 = 'Validator address cannot be 0';
         pub const NO_VALIDATORS_PROVIDED: felt252 = 'No validators provided';
+        pub const THRESHOLD_TOO_HIGH: felt252 = 'Threshold too high';
     }
 
     #[event]
@@ -46,8 +47,15 @@ pub mod merkleroot_multisig_ism {
     }
 
     #[constructor]
-    fn constructor(ref self: ContractState, _owner: ContractAddress, _validators: Span<felt252>) {
+    fn constructor(
+        ref self: ContractState,
+        _owner: ContractAddress,
+        _validators: Span<felt252>,
+        _threshold: u32
+    ) {
         self.ownable.initializer(_owner);
+        assert(_threshold <= 0xffffffff, Errors::THRESHOLD_TOO_HIGH);
+        self.threshold.write(_threshold);
         self.set_validators(_validators);
     }
 
@@ -113,17 +121,6 @@ pub mod merkleroot_multisig_ism {
             self.threshold.read()
         }
 
-        /// Sets the threshold, the number of validator signatures needed to verify a message 
-        /// Dev: callable only by the admin
-        /// 
-        /// # Arguments 
-        ///
-        /// * - `_threshold` - the number of validator signature needed
-        fn set_threshold(ref self: ContractState, _threshold: u32) {
-            self.ownable.assert_only_owner();
-            self.threshold.write(_threshold);
-        }
-
         /// Returns the set of validators responsible for verifying _message and the number of signatures required
         /// Dev: Can change based on the content of _message
         /// 

contracts/src/contracts/isms/multisig/messageid_multisig_ism.cairo

@@ -36,6 +36,7 @@ pub mod messageid_multisig_ism {
         pub const EMPTY_METADATA: felt252 = 'Empty metadata';
         pub const VALIDATOR_ADDRESS_CANNOT_BE_NULL: felt252 = 'Validator address cannot be 0';
         pub const NO_VALIDATORS_PROVIDED: felt252 = 'No validators provided';
+        pub const THRESHOLD_TOO_HIGH: felt252 = 'Threshold too high';
     }
 
     #[event]
@@ -49,8 +50,15 @@ pub mod messageid_multisig_ism {
 
 
     #[constructor]
-    fn constructor(ref self: ContractState, _owner: ContractAddress, _validators: Span<felt252>) {
+    fn constructor(
+        ref self: ContractState,
+        _owner: ContractAddress,
+        _validators: Span<felt252>,
+        _threshold: u32
+    ) {
         self.ownable.initializer(_owner);
+        assert(_threshold <= 0xffffffff, Errors::THRESHOLD_TOO_HIGH);
+        self.threshold.write(_threshold);
         self.set_validators(_validators);
     }
 
@@ -116,18 +124,6 @@ pub mod messageid_multisig_ism {
             self.threshold.read()
         }
 
-
-        /// Set the threshold for validation
-        /// Dev: callable only by the owner
-        /// 
-        /// # Arguments
-        /// 
-        /// * - `_threshold` - The number of validator signatures needed
-        fn set_threshold(ref self: ContractState, _threshold: u32) {
-            self.ownable.assert_only_owner();
-            self.threshold.write(_threshold);
-        }
-
         /// Returns the set of validators responsible for verifying _message and the number of signatures required
         /// Dev: Can change based on the content of _message
         /// 

contracts/src/interfaces.cairo

@@ -105,8 +105,6 @@ pub trait IValidatorConfiguration<TContractState> {
     fn get_validators(self: @TContractState) -> Span<EthAddress>;
 
     fn get_threshold(self: @TContractState) -> u32;
-
-    fn set_threshold(ref self: TContractState, _threshold: u32);
 }
 
 #[starknet::interface]

contracts/src/tests/isms/test_aggregation.cairo

@@ -28,7 +28,6 @@ fn test_aggregation_module_type() {
     );
 }
 
-
 #[test]
 #[should_panic]
 fn test_aggregation_initialize_with_too_many_modules() {
@@ -86,23 +85,16 @@ fn test_aggregation_verify() {
         body: message_body.clone()
     };
     let (_, validators_address, _) = get_message_and_signature();
-    let (messageid, messageid_validator_configuration) = setup_messageid_multisig_ism(
-        validators_address.span()
-    );
+    let (messageid, _) = setup_messageid_multisig_ism(validators_address.span(), threshold);
     let origin_merkle_tree: u256 = 'origin_merkle_tree_hook'.try_into().unwrap();
     let root: u256 = 'root'.try_into().unwrap();
     let index = 1;
     let message_id_metadata = build_messageid_metadata(origin_merkle_tree, root, index);
-    let ownable = IOwnableDispatcher {
-        contract_address: messageid_validator_configuration.contract_address
-    };
-    start_prank(CheatTarget::One(ownable.contract_address), OWNER().try_into().unwrap());
-    messageid_validator_configuration.set_threshold(5);
     // Noop ism
     let noop_ism = setup_noop_ism();
     let aggregation = setup_aggregation(
         array![messageid.contract_address.into(), noop_ism.contract_address.into(),].span(),
-        threshold
+        threshold.try_into().unwrap()
     );
     let ownable = IOwnableDispatcher { contract_address: aggregation.contract_address };
     start_prank(CheatTarget::One(ownable.contract_address), OWNER().try_into().unwrap());

contracts/src/tests/isms/test_merkleroot_multisig.cairo

@@ -24,10 +24,11 @@ use snforge_std::{start_prank, CheatTarget};
 
 #[test]
 fn test_set_validators() {
+    let threshold = 2;
     let new_validators: Array<felt252> = array![
         VALIDATOR_ADDRESS_1().into(), VALIDATOR_ADDRESS_2().into()
     ];
-    let (_, validators) = setup_merkleroot_multisig_ism(new_validators.span());
+    let (_, validators) = setup_merkleroot_multisig_ism(new_validators.span(), threshold);
     let ownable = IOwnableDispatcher { contract_address: validators.contract_address };
     start_prank(CheatTarget::One(ownable.contract_address), OWNER().try_into().unwrap());
     let validators_span = validators.get_validators();
@@ -38,30 +39,20 @@ fn test_set_validators() {
 
 #[test]
 fn test_set_threshold() {
-    let new_threshold = 3;
-    let (_, validators) = setup_merkleroot_multisig_ism(array!['validator_1'].span());
+    let threshold = 3;
+    let (_, validators) = setup_merkleroot_multisig_ism(array!['validator_1'].span(), threshold);
     let ownable = IOwnableDispatcher { contract_address: validators.contract_address };
     start_prank(CheatTarget::One(ownable.contract_address), OWNER().try_into().unwrap());
-    validators.set_threshold(new_threshold);
-    assert(validators.get_threshold() == new_threshold, 'wrong validator threshold');
+    assert(validators.get_threshold() == threshold, 'wrong validator threshold');
 }
 
 
 #[test]
 #[should_panic]
 fn test_set_validators_fails_if_null_validator() {
+    let threshold = 2;
     let new_validators = array![VALIDATOR_ADDRESS_1().into(), 0].span();
-    setup_merkleroot_multisig_ism(new_validators);
-}
-
-#[test]
-#[should_panic(expected: ('Caller is not the owner',))]
-fn test_set_threshold_fails_if_caller_not_owner() {
-    let new_threshold = 3;
-    let (_, validators) = setup_merkleroot_multisig_ism(array!['validator_1'].span());
-    let ownable = IOwnableDispatcher { contract_address: validators.contract_address };
-    start_prank(CheatTarget::One(ownable.contract_address), NEW_OWNER().try_into().unwrap());
-    validators.set_threshold(new_threshold);
+    setup_merkleroot_multisig_ism(new_validators, threshold);
 }
 
 
@@ -111,7 +102,10 @@ fn test_merkleroot_ism_metadata() {
 
 #[test]
 fn test_merkle_root_multisig_module_type() {
-    let (merkleroot_ism, _) = setup_merkleroot_multisig_ism(array!['validator_1'].span());
+    let threshold = 2;
+    let (merkleroot_ism, _) = setup_merkleroot_multisig_ism(
+        array!['validator_1'].span(), threshold
+    );
     assert(
         merkleroot_ism
             .module_type() == ModuleType::MERKLE_ROOT_MULTISIG(merkleroot_ism.contract_address),
@@ -122,6 +116,7 @@ fn test_merkle_root_multisig_module_type() {
 
 #[test]
 fn test_merkle_root_multisig_verify_with_4_valid_signatures() {
+    let threshold = 4;
     let array = array![
         0x01020304050607080910111213141516,
         0x01020304050607080910111213141516,
@@ -138,28 +133,22 @@ fn test_merkle_root_multisig_verify_with_4_valid_signatures() {
         body: message_body.clone()
     };
     let (_, validators_address, _) = get_merkle_message_and_signature();
-    let (merkleroot_ism, merkleroot_validator_configuration) = setup_merkleroot_multisig_ism(
-        validators_address.span()
-    );
+    let (merkleroot_ism, _) = setup_merkleroot_multisig_ism(validators_address.span(), threshold);
     let origin_merkle_tree_hook: u256 = 'origin_merkle_tree_hook'.try_into().unwrap();
     let message_index: u32 = 1;
     let signed_index: u32 = 2;
     let signed_message_id: u256 = 'signed_message_id'.try_into().unwrap();
     let metadata = build_merkle_metadata(
         origin_merkle_tree_hook, message_index, signed_index, signed_message_id
     );
-    let ownable = IOwnableDispatcher {
-        contract_address: merkleroot_validator_configuration.contract_address
-    };
-    start_prank(CheatTarget::One(ownable.contract_address), OWNER().try_into().unwrap());
-    merkleroot_validator_configuration.set_threshold(4);
     assert(merkleroot_ism.verify(metadata, message) == true, 'verification failed');
 }
 
 
 #[test]
 #[should_panic(expected: ('No match for given signature',))]
 fn test_merkle_root_multisig_verify_with_insufficient_valid_signatures() {
+    let threshold = 4;
     let array = array![
         0x01020304050607080910111213141516,
         0x01020304050607080910111213141516,
@@ -176,9 +165,7 @@ fn test_merkle_root_multisig_verify_with_insufficient_valid_signatures() {
         body: message_body.clone()
     };
     let (_, validators_address, _) = get_merkle_message_and_signature();
-    let (merkleroot_ism, merkleroot_validator_config) = setup_merkleroot_multisig_ism(
-        validators_address.span()
-    );
+    let (merkleroot_ism, _) = setup_merkleroot_multisig_ism(validators_address.span(), threshold);
     let origin_merkle_tree_hook: u256 = 'origin_merkle_tree_hook'.try_into().unwrap();
     let message_index: u32 = 1;
     let signed_index: u32 = 2;
@@ -187,16 +174,14 @@ fn test_merkle_root_multisig_verify_with_insufficient_valid_signatures() {
         origin_merkle_tree_hook, message_index, signed_index, signed_message_id
     );
     metadata.update_at(1100, 0);
-    let ownable = IOwnableDispatcher { contract_address: merkleroot_ism.contract_address };
-    start_prank(CheatTarget::One(ownable.contract_address), OWNER().try_into().unwrap());
-    merkleroot_validator_config.set_threshold(4);
     assert(merkleroot_ism.verify(metadata, message) == true, 'verification failed');
 }
 
 
 #[test]
 #[should_panic(expected: ('Empty metadata',))]
 fn test_merkle_root_multisig_verify_with_empty_metadata() {
+    let threshold = 4;
     let array = array![
         0x01020304050607080910111213141516,
         0x01020304050607080910111213141516,
@@ -213,12 +198,7 @@ fn test_merkle_root_multisig_verify_with_empty_metadata() {
         body: message_body.clone()
     };
     let (_, validators_address, _) = get_merkle_message_and_signature();
-    let (merkle_root_ism, merkleroot_validator_config) = setup_merkleroot_multisig_ism(
-        validators_address.span()
-    );
-    let ownable = IOwnableDispatcher { contract_address: merkle_root_ism.contract_address };
-    start_prank(CheatTarget::One(ownable.contract_address), OWNER().try_into().unwrap());
-    merkleroot_validator_config.set_threshold(4);
+    let (merkle_root_ism, _) = setup_merkleroot_multisig_ism(validators_address.span(), threshold);
     let bytes_metadata = BytesTrait::new_empty();
     assert(merkle_root_ism.verify(bytes_metadata, message) == true, 'verification failed');
 }
@@ -227,6 +207,8 @@ fn test_merkle_root_multisig_verify_with_empty_metadata() {
 #[test]
 #[should_panic(expected: ('No match for given signature',))]
 fn test_merkle_root_multisig_verify_with_4_valid_signatures_fails_if_duplicate_signatures() {
+    let threshold = 4;
+
     let array = array![
         0x01020304050607080910111213141516,
         0x01020304050607080910111213141516,
@@ -243,20 +225,13 @@ fn test_merkle_root_multisig_verify_with_4_valid_signatures_fails_if_duplicate_s
         body: message_body.clone()
     };
     let (_, validators_address, _) = get_merkle_message_and_signature();
-    let (merkleroot_ism, merkleroot_validator_configuration) = setup_merkleroot_multisig_ism(
-        validators_address.span()
-    );
+    let (merkleroot_ism, _) = setup_merkleroot_multisig_ism(validators_address.span(), threshold);
     let origin_merkle_tree_hook: u256 = 'origin_merkle_tree_hook'.try_into().unwrap();
     let message_index: u32 = 1;
     let signed_index: u32 = 2;
     let signed_message_id: u256 = 'signed_message_id'.try_into().unwrap();
     let metadata = build_fake_merkle_metadata(
         origin_merkle_tree_hook, message_index, signed_index, signed_message_id
     );
-    let ownable = IOwnableDispatcher {
-        contract_address: merkleroot_validator_configuration.contract_address
-    };
-    start_prank(CheatTarget::One(ownable.contract_address), OWNER().try_into().unwrap());
-    merkleroot_validator_configuration.set_threshold(4);
     assert(merkleroot_ism.verify(metadata, message) == true, 'verification failed');
 }