Aggregation ISM cannot skip ISMs
Description
Aggregation ISM in the Hyperlane protocol is an ISM that returns true when m-of-n ISMs return true, where the threshold and the list of ISMs are predetermined. It is implemented in aggregation.cairo:
fn verify(self: @ContractState, _metadata: Bytes, _message: Message,) -> bool {
let (isms, mut threshold) = self.modules_and_threshold(_message.clone());
assert(threshold != 0, Errors::THRESHOLD_NOT_SET);
let modules = self.build_modules_span();
let mut cur_idx: u8 = 0;
loop {
if (threshold == 0) {
break ();
}
if (cur_idx.into() == isms.len()) {
break ();
}
if (!AggregationIsmMetadata::has_metadata(_metadata.clone(), cur_idx)) {
cur_idx += 1;
continue;
}
let ism = IInterchainSecurityModuleDispatcher {
contract_address: *modules.at(cur_idx.into())
};
let metadata = AggregationIsmMetadata::metadata_at(_metadata.clone(), cur_idx);
assert(ism.verify(metadata, _message.clone()), Errors::VERIFICATION_FAILED);
threshold -= 1;
cur_idx += 1;
};
assert(threshold == 0, Errors::THRESHOLD_NOT_REACHED);
true
}Aggregation ISM is expected to skip ISMs that will return false or revert. To implement this behavior, it is checked if the given metadata includes the metadata for the ISM, and ISMs that do not have their metadatas are skipped.
However, the function AggregationIsmMetadata::has_metadata actually does not check the existence of the metadata:
fn has_metadata(_metadata: Bytes, _index: u8) -> bool {
match metadata_range(_metadata, _index) {
Result::Ok((_, _)) => true,
Result::Err(_) => false
}
}
// ...
fn metadata_range(_metadata: Bytes, _index: u8) -> Result<(u32, u32), u8> {
let start = _index.into() * RANGE_SIZE * 2;
let mid = start + RANGE_SIZE;
let (_, mid_metadata) = _metadata.read_u32(mid.into());
let (_, start_metadata) = _metadata.read_u32(start.into());
Result::Ok((start_metadata, mid_metadata))
}The function has_metadata returns false if the function metadata_range returns an error. However, the function metadata_range never returns an error; it should always return the normal result or revert (if mid or start overflows the size of _metadata). Either way, the ISM verification will fail at this point, although there are more ISMs that can be checked.
Impact
M-of-n Aggregation ISM will revert if one of the first m ISMs does not return true.
Recommendations
Consider modifying the function has_metadata in order to allow a relayer to specify ISMs to be skipped.
Remediation
This issue has been acknowledged by Pragma, and a fix was implemented in commit 939b0435↗.