Some questions about AggregationISM

[letonchanh]

Hi,

I am looking at the implementation of AggregationISM and I would like to clarify some assumptions that I may or may not understand correctly:

Q1. The function modulesAndThreshold returns an array _isms of ISM addresses and a _threshold, is it always _isms.length >= _threshold and only <= _threshold ISMs have metadata?

Q2. I think the current implementation of verify in AbstractAggregationIsm at

hyperlane-monorepo/solidity/contracts/isms/aggregation/AbstractAggregationIsm.sol

Line 46 in 2093fe4

function verify(

is only safely handling the case of _isms.length <= _threshold or only <= _threshold ISMs have metadata.
In the case of _isms.length > _threshold and there are more than _threshold ISMs having metadata, there are two possible problems:

• The function verify reverts when an ISM fails to verify the message, although enough ISMs can successfully verify the message to reach the threshold.
• Underflow happens when the number of ISMs that can verify the message successfully is greater than _threshold.

Thank you.

[nbayindirli]

Q1. Your understanding is correct– the threshold requirement of successfully verified ISMs is always <= modules.length.

Q2.

• I. Validators pre-verify that ISM metadata is present, so there is no real risk in pre-maturely reverting before we verify there are enough ISMs to meet the threshold (skips at

  hyperlane-monorepo/solidity/contracts/isms/aggregation/AbstractAggregationIsm.sol

  Line 55 in 2093fe4

  if (!AggregationIsmMetadata.hasMetadata(_metadata, i)) continue;

  ). This is of course not explicitly defined behavior, so if you would like to submit a bug, check out our Immunefi, mentioned below.
• II. Nice find– you are correct in that this would lead to an underflow, but this will not result in unsafe behavior, aside from a revert. Indeed one solution is to require threshold <= ([verified] modules).length, exiting if this condition is met early, and keeping track of any failed ism.verify()'s, finally returning a boolean array of those that succeeded and failed verification (allowing the calling contract to hard or quietly revert as necessary on failed verifications). If you'd like to submit a bug/fix, please visit the Hyperlane Immunefi: https://immunefi.com/bounty/hyperlane/