Make recipientIsm more robust (#2767)

Fixes hyperlane-xyz/issues#629
hyperlane-xyz · Oct 2, 2023 · bf3d3c4 · bf3d3c4
1 parent c91f589
commit bf3d3c4
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 14 deletions.
diff --git a/solidity/contracts/Mailbox.sol b/solidity/contracts/Mailbox.sol
@@ -392,20 +392,23 @@ contract Mailbox is IMailbox, Indexed, Versioned, OwnableUpgradeable {
         view
         returns (IInterchainSecurityModule)
     {
-        // Use a default interchainSecurityModule if one is not specified by the
-        // recipient.
-        // This is useful for backwards compatibility and for convenience as
-        // recipients are not mandated to specify an ISM.
-        try
-            ISpecifiesInterchainSecurityModule(_recipient)
-                .interchainSecurityModule()
-        returns (IInterchainSecurityModule _val) {
-            // If the recipient specifies a zero address, use the default ISM.
-            if (address(_val) != address(0)) {
-                return _val;
+        // use low-level staticcall in case of revert or empty return data
+        (bool success, bytes memory returnData) = _recipient.staticcall(
+            abi.encodeCall(
+                ISpecifiesInterchainSecurityModule.interchainSecurityModule,
+                ()
+            )
+        );
+        // check if call was successful and returned data
+        if (success && returnData.length != 0) {
+            // check if returnData is a valid address
+            address ism = abi.decode(returnData, (address));
+            // check if the ISM is a contract
+            if (ism != address(0)) {
+                return IInterchainSecurityModule(ism);
             }
-            // solhint-disable-next-line no-empty-blocks
-        } catch {}
+        }
+        // Use the default if a valid one is not specified by the recipient.
         return defaultIsm;
     }
 

diff --git a/solidity/test/Mailbox.t.sol b/solidity/test/Mailbox.t.sol
@@ -13,6 +13,12 @@ import "../contracts/hooks/MerkleTreeHook.sol";
 import {StandardHookMetadata} from "../contracts/hooks/libs/StandardHookMetadata.sol";
 import {TypeCasts} from "../contracts/libs/TypeCasts.sol";
 
+contract Empty {}
+
+contract EmptyFallback {
+    fallback() external {}
+}
+
 contract MailboxTest is Test, Versioned {
     using StandardHookMetadata for bytes;
     using TypeCasts for address;
@@ -78,7 +84,24 @@ contract MailboxTest is Test, Versioned {
         IInterchainSecurityModule ism = mailbox.recipientIsm(
             address(recipient)
         );
-        assertEq(address(mailbox.defaultIsm()), address(ism));
+        assertEq(address(defaultIsm), address(ism));
+
+        // check no ism function returns default
+        Empty empty = new Empty();
+        ism = mailbox.recipientIsm(address(empty));
+        assertEq(address(defaultIsm), address(ism));
+
+        // check empty fallback returns default
+        EmptyFallback emptyFallback = new EmptyFallback();
+        ism = mailbox.recipientIsm(address(emptyFallback));
+        assertEq(address(defaultIsm), address(ism));
+
+        // check zero address returns default
+        recipient.setInterchainSecurityModule(address(0));
+        ism = mailbox.recipientIsm(address(recipient));
+        assertEq(address(defaultIsm), address(ism));
+
+        // check recipient override is used
         TestIsm newIsm = new TestIsm();
         recipient.setInterchainSecurityModule(address(newIsm));
         ism = mailbox.recipientIsm(address(recipient));