From bf3d3c4639264489c1840804ffe706a690fbde1c Mon Sep 17 00:00:00 2001
From: Yorke Rhodes <yorke@hyperlane.xyz>
Date: Mon, 2 Oct 2023 12:11:25 -0400
Subject: [PATCH] Make recipientIsm more robust (#2767)

Fixes https://github.com/hyperlane-xyz/issues/issues/629
---
 solidity/contracts/Mailbox.sol | 29 ++++++++++++++++-------------
 solidity/test/Mailbox.t.sol    | 25 ++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/solidity/contracts/Mailbox.sol b/solidity/contracts/Mailbox.sol
index 6cfec22af..59b0d63b1 100644
--- 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
index e408b4b01..fe50aa279 100644
--- 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));