Skip to content

Commit

Permalink
audit: threshold set as parameter
Browse files Browse the repository at this point in the history
  • Loading branch information
@JordyRo1
JordyRo1 committed Jul 17, 2024
1 parent 484fe5e commit 90cc291
Show file tree
Hide file tree
Showing 10 changed files with 78 additions and 139 deletions.
17 changes: 5 additions & 12 deletions contracts/src/contracts/isms/aggregation/aggregation.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,17 @@ pub mod aggregation {
pub const MODULES_ALREADY_STORED: felt252 = 'Modules already stored';
pub const NO_MODULES_PROVIDED: felt252 = 'No modules provided';
pub const TOO_MANY_MODULES_PROVIDED: felt252 = 'Too many modules provided';
pub const THRESHOLD_TOO_HIGH: felt252 = 'Threshold too high';
}

#[constructor]
fn constructor(ref self: ContractState, _owner: ContractAddress, _modules: Span<felt252>) {
fn constructor(ref self: ContractState, _owner: ContractAddress, _modules: Span<felt252>, _threshold: u8) {
self.ownable.initializer(_owner);
assert(_modules.len() < 256, Errors::TOO_MANY_MODULES_PROVIDED);
assert(_threshold <= 255, Errors::THRESHOLD_TOO_HIGH);
self.threshold.write(_threshold);
self.set_modules(_modules);

}

#[abi(embed_v0)]
Expand Down Expand Up @@ -132,17 +136,6 @@ pub mod aggregation {
fn get_threshold(self: @ContractState) -> u8 {
self.threshold.read()
}

/// Sets 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: u8) {
self.ownable.assert_only_owner();
self.threshold.write(_threshold);
}
}
#[generate_trait]
impl InternalImpl of InternalTrait {
Expand Down
16 changes: 4 additions & 12 deletions contracts/src/contracts/isms/multisig/merkleroot_multisig_ism.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -46,9 +47,11 @@ 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);
self.set_validators(_validators);
assert(_threshold <= 0xffffffff, Errors::THRESHOLD_TOO_HIGH);
self.threshold.write(_threshold);
}

#[abi(embed_v0)]
Expand Down Expand Up @@ -113,17 +116,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
///
Expand Down
16 changes: 4 additions & 12 deletions contracts/src/contracts/isms/multisig/messageid_multisig_ism.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -49,9 +50,11 @@ 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);
self.set_validators(_validators);
assert(_threshold <= 0xffffffff, Errors::THRESHOLD_TOO_HIGH);
self.threshold.write(_threshold);
}

#[abi(embed_v0)]
Expand Down Expand Up @@ -117,17 +120,6 @@ pub mod messageid_multisig_ism {
}


/// 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
///
Expand Down
4 changes: 0 additions & 4 deletions contracts/src/interfaces.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -276,8 +274,6 @@ pub trait IAggregation<TContractState> {
fn get_modules(self: @TContractState) -> Span<ContractAddress>;

fn get_threshold(self: @TContractState) -> u8;

fn set_threshold(ref self: TContractState, _threshold: u8);
}


Expand Down
42 changes: 18 additions & 24 deletions contracts/src/tests/isms/test_aggregation.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,25 +19,18 @@ use starknet::ContractAddress;

#[test]
fn test_aggregation_module_type() {
let aggregation = setup_aggregation(MODULES());
let threshold = 4;
let aggregation = setup_aggregation(MODULES(), threshold);
assert(
aggregation.module_type() == ModuleType::AGGREGATION(aggregation.contract_address),
'Aggregation: Wrong module type'
);
}

#[test]
fn test_aggregation_set_threshold() {
let threshold = 3;
let aggregation = setup_aggregation(MODULES());
let ownable = IOwnableDispatcher { contract_address: aggregation.contract_address };
start_prank(CheatTarget::One(ownable.contract_address), OWNER());
aggregation.set_threshold(threshold);
}

#[test]
#[should_panic]
fn test_aggregation_initialize_with_too_many_modules() {
let threshold = 4;
let mut modules = array![];
let mut cur_idx = 0;
loop {
Expand All @@ -47,32 +40,35 @@ fn test_aggregation_initialize_with_too_many_modules() {
modules.append('module_1'.into());
cur_idx += 1;
};
setup_aggregation(modules.span());
setup_aggregation(modules.span(), threshold);
}


#[test]
#[should_panic(expected: ('Threshold not set',))]
fn test_aggregation_verify_fails_if_treshold_not_set() {
let aggregation = setup_aggregation(MODULES());
aggregation.verify(BytesTrait::new(42, array![]), MessageTrait::default());
}

#[test]
#[should_panic]
fn test_setup_aggregation_with_null_module_address() {
let threshold = 2;
let modules: Span<felt252> = array![0, 'module_1'].span();
setup_aggregation(modules);
setup_aggregation(modules, threshold);
}

#[test]
fn test_get_modules() {
let aggregation = setup_aggregation(MODULES());
let threshold = 2;
let aggregation = setup_aggregation(MODULES(), threshold);
let ownable = IOwnableDispatcher { contract_address: aggregation.contract_address };
start_prank(CheatTarget::One(ownable.contract_address), OWNER());
assert(aggregation.get_modules() == CONTRACT_MODULES(), 'set modules failed');
}

#[test]
fn test_get_treshold() {
let threshold = 2;
let aggregation = setup_aggregation(MODULES(), threshold);
let ownable = IOwnableDispatcher { contract_address: aggregation.contract_address };
start_prank(CheatTarget::One(ownable.contract_address), OWNER());
assert(aggregation.get_threshold() == threshold, 'threshold writing failed');
}

#[test]
fn test_aggregation_verify() {
Expand All @@ -97,7 +93,7 @@ fn test_aggregation_verify() {
};
let (_, validators_address, _) = get_message_and_signature();
let (messageid, messageid_validator_configuration) = setup_messageid_multisig_ism(
validators_address.span()
validators_address.span(), threshold
);
let origin_merkle_tree: u256 = 'origin_merkle_tree_hook'.try_into().unwrap();
let root: u256 = 'root'.try_into().unwrap();
Expand All @@ -107,15 +103,13 @@ fn test_aggregation_verify() {
contract_address: messageid_validator_configuration.contract_address
};
start_prank(CheatTarget::One(ownable.contract_address), OWNER());
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()
array![messageid.contract_address.into(), noop_ism.contract_address.into(),].span(), threshold.try_into().unwrap()
);
let ownable = IOwnableDispatcher { contract_address: aggregation.contract_address };
start_prank(CheatTarget::One(ownable.contract_address), OWNER());
aggregation.set_threshold(threshold);
let mut concat_metadata = BytesTrait::new_empty();
concat_metadata.append_u128(0x00000010000001A0000001A0000001A9);
concat_metadata.concat(@message_id_metadata);
Expand Down
44 changes: 18 additions & 26 deletions contracts/src/tests/isms/test_merkleroot_multisig.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ use snforge_std::{start_prank, CheatTarget};

#[test]
fn test_set_validators() {
let threshold = 4;
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());
let validators_span = validators.get_validators();
Expand All @@ -35,30 +36,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());
validators.set_threshold(new_threshold);
assert(validators.get_threshold() == new_threshold, 'wrong validator threshold');
assert(validators.get_threshold() == 2, 'wrong validator threshold');
}


#[test]
#[should_panic]
fn test_set_validators_fails_if_null_validator() {
let threshold = 3;
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());
validators.set_threshold(new_threshold);
setup_merkleroot_multisig_ism(new_validators, threshold);
}


Expand Down Expand Up @@ -108,7 +99,8 @@ 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),
Expand All @@ -119,6 +111,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,
Expand All @@ -136,7 +129,7 @@ fn test_merkle_root_multisig_verify_with_4_valid_signatures() {
};
let (_, validators_address, _) = get_merkle_message_and_signature();
let (merkleroot_ism, merkleroot_validator_configuration) = setup_merkleroot_multisig_ism(
validators_address.span()
validators_address.span(), threshold
);
let origin_merkle_tree_hook: u256 = 'origin_merkle_tree_hook'.try_into().unwrap();
let message_index: u32 = 1;
Expand All @@ -149,14 +142,14 @@ fn test_merkle_root_multisig_verify_with_4_valid_signatures() {
contract_address: merkleroot_validator_configuration.contract_address
};
start_prank(CheatTarget::One(ownable.contract_address), OWNER());
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,
Expand All @@ -173,8 +166,8 @@ 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;
Expand All @@ -186,14 +179,14 @@ fn test_merkle_root_multisig_verify_with_insufficient_valid_signatures() {
metadata.update_at(1100, 0);
let ownable = IOwnableDispatcher { contract_address: merkleroot_ism.contract_address };
start_prank(CheatTarget::One(ownable.contract_address), OWNER());
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,
Expand All @@ -211,11 +204,10 @@ fn test_merkle_root_multisig_verify_with_empty_metadata() {
};
let (_, validators_address, _) = get_merkle_message_and_signature();
let (merkle_root_ism, merkleroot_validator_config) = setup_merkleroot_multisig_ism(
validators_address.span()
validators_address.span(), threshold
);
let ownable = IOwnableDispatcher { contract_address: merkle_root_ism.contract_address };
start_prank(CheatTarget::One(ownable.contract_address), OWNER());
merkleroot_validator_config.set_threshold(4);
let bytes_metadata = BytesTrait::new_empty();
assert(merkle_root_ism.verify(bytes_metadata, message) == true, 'verification failed');
}
Expand All @@ -224,6 +216,7 @@ 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,
Expand All @@ -241,7 +234,7 @@ fn test_merkle_root_multisig_verify_with_4_valid_signatures_fails_if_duplicate_s
};
let (_, validators_address, _) = get_merkle_message_and_signature();
let (merkleroot_ism, merkleroot_validator_configuration) = setup_merkleroot_multisig_ism(
validators_address.span()
validators_address.span(), threshold
);
let origin_merkle_tree_hook: u256 = 'origin_merkle_tree_hook'.try_into().unwrap();
let message_index: u32 = 1;
Expand All @@ -254,6 +247,5 @@ fn test_merkle_root_multisig_verify_with_4_valid_signatures_fails_if_duplicate_s
contract_address: merkleroot_validator_configuration.contract_address
};
start_prank(CheatTarget::One(ownable.contract_address), OWNER());
merkleroot_validator_configuration.set_threshold(4);
assert(merkleroot_ism.verify(metadata, message) == true, 'verification failed');
}
Loading

0 comments on commit 90cc291

Please sign in to comment.