Skip to content

Commit

Permalink
Audit: setthresholdas constructor param (#86)
Browse files Browse the repository at this point in the history 
* feat: sender and receiver ContractAddress -> u256

* fix: format code

* fix: convert owner to ContractAddress in tests

* fix: format code

* fix: recipient/sender as u256 (integration tests)

* feat: threshold set as constructor for aggregation.cairo

* feat: threshold as constructor

* fix(test): owner type

* fix(format): format code

* fix(test): sender/recipient as u256

* fix(tests): remove unused variable
  • Loading branch information
@JordyRo1
JordyRo1 authored Jul 25, 2024
1 parent bca3875 commit 647fe8b
Show file tree
Hide file tree
Showing 9 changed files with 65 additions and 151 deletions.
21 changes: 9 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,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);
}

Expand Down Expand Up @@ -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
///
Expand Down
22 changes: 9 additions & 13 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,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);
}

Expand Down Expand Up @@ -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
///
Expand Down
2 changes: 0 additions & 2 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
12 changes: 2 additions & 10 deletions contracts/src/tests/isms/test_aggregation.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ fn test_aggregation_module_type() {
);
}


#[test]
#[should_panic]
fn test_aggregation_initialize_with_too_many_modules() {
Expand Down Expand Up @@ -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());
Expand Down
65 changes: 20 additions & 45 deletions contracts/src/tests/isms/test_merkleroot_multisig.cairo
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);
}


Expand Down Expand Up @@ -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),
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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;
Expand All @@ -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,
Expand All @@ -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');
}
Expand All @@ -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,
Expand All @@ -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');
}
Loading

0 comments on commit 647fe8b

Please sign in to comment.