check if a module supports expected interface

Signed-off-by: Jun Kimura <[email protected]>

.gas-snapshot

@@ -8,13 +8,13 @@ IBCTest:testBenchmarkRecvPacket() (gas: 158888)
 IBCTest:testBenchmarkSendPacket() (gas: 128432)
 IBCTest:testBenchmarkUpdateMockClient() (gas: 160229)
 IBCTest:testToUint128((uint64,uint64)) (runs: 256, μ: 947, ~: 947)
-TestICS02:testCreateClient() (gas: 36596358)
-TestICS02:testInvalidCreateClient() (gas: 36493526)
-TestICS02:testInvalidUpdateClient() (gas: 36492485)
-TestICS02:testRegisterClient() (gas: 36148151)
-TestICS02:testRegisterClientDuplicatedClientType() (gas: 36133460)
-TestICS02:testRegisterClientInvalidClientType() (gas: 36162921)
-TestICS02:testUpdateClient() (gas: 36660685)
+TestICS02:testCreateClient() (gas: 36656370)
+TestICS02:testInvalidCreateClient() (gas: 36553538)
+TestICS02:testInvalidUpdateClient() (gas: 36552497)
+TestICS02:testRegisterClient() (gas: 36208163)
+TestICS02:testRegisterClientDuplicatedClientType() (gas: 36193472)
+TestICS02:testRegisterClientInvalidClientType() (gas: 36222933)
+TestICS02:testUpdateClient() (gas: 36720697)
 TestICS03Handshake:testConnOpenAck() (gas: 1858230)
 TestICS03Handshake:testConnOpenConfirm() (gas: 2054143)
 TestICS03Handshake:testConnOpenInit() (gas: 1429838)
@@ -46,23 +46,23 @@ TestICS04Packet:testRecvPacketTimeoutHeight() (gas: 3259769)
 TestICS04Packet:testRecvPacketTimeoutTimestamp() (gas: 3279145)
 TestICS04Packet:testSendPacket() (gas: 6411842)
 TestICS04Packet:testTimeoutOnClose() (gas: 3553375)
-TestICS04Upgrade:testUpgradeAuthorityCancel() (gas: 46736852)
-TestICS04Upgrade:testUpgradeCannotCancelWithOldErrorReceipt() (gas: 3456618)
-TestICS04Upgrade:testUpgradeCannotRecvNextUpgradePacket() (gas: 5264735)
-TestICS04Upgrade:testUpgradeCounterpartyAdvanceNextSequenceBeforeOpen() (gas: 5234977)
-TestICS04Upgrade:testUpgradeCrossingHelloIncompatibleProposals() (gas: 4406999)
-TestICS04Upgrade:testUpgradeFull() (gas: 57771943)
-TestICS04Upgrade:testUpgradeInit() (gas: 3069201)
-TestICS04Upgrade:testUpgradeNoChanges() (gas: 2471837)
-TestICS04Upgrade:testUpgradeOutOfSync() (gas: 3903101)
-TestICS04Upgrade:testUpgradeRelaySuccessAtCounterpartyFlushComplete() (gas: 5236500)
-TestICS04Upgrade:testUpgradeRelaySuccessAtFlushing() (gas: 5609709)
-TestICS04Upgrade:testUpgradeSendPacketFailAtFlushingOrFlushComplete() (gas: 4070583)
-TestICS04Upgrade:testUpgradeTimeoutAbortAck() (gas: 17691838)
-TestICS04Upgrade:testUpgradeTimeoutAbortConfirm() (gas: 21328699)
-TestICS04Upgrade:testUpgradeTimeoutUpgrade() (gas: 71035138)
-TestICS04Upgrade:testUpgradeToOrdered() (gas: 56500423)
-TestICS04Upgrade:testUpgradeToUnordered() (gas: 45106369)
+TestICS04Upgrade:testUpgradeAuthorityCancel() (gas: 46744907)
+TestICS04Upgrade:testUpgradeCannotCancelWithOldErrorReceipt() (gas: 3458294)
+TestICS04Upgrade:testUpgradeCannotRecvNextUpgradePacket() (gas: 5265242)
+TestICS04Upgrade:testUpgradeCounterpartyAdvanceNextSequenceBeforeOpen() (gas: 5235969)
+TestICS04Upgrade:testUpgradeCrossingHelloIncompatibleProposals() (gas: 4433366)
+TestICS04Upgrade:testUpgradeFull() (gas: 57832187)
+TestICS04Upgrade:testUpgradeInit() (gas: 3071188)
+TestICS04Upgrade:testUpgradeNoChanges() (gas: 2472864)
+TestICS04Upgrade:testUpgradeOutOfSync() (gas: 3904556)
+TestICS04Upgrade:testUpgradeRelaySuccessAtCounterpartyFlushComplete() (gas: 5237007)
+TestICS04Upgrade:testUpgradeRelaySuccessAtFlushing() (gas: 5610216)
+TestICS04Upgrade:testUpgradeSendPacketFailAtFlushingOrFlushComplete() (gas: 4071077)
+TestICS04Upgrade:testUpgradeTimeoutAbortAck() (gas: 17704880)
+TestICS04Upgrade:testUpgradeTimeoutAbortConfirm() (gas: 21335923)
+TestICS04Upgrade:testUpgradeTimeoutUpgrade() (gas: 71044674)
+TestICS04Upgrade:testUpgradeToOrdered() (gas: 56505414)
+TestICS04Upgrade:testUpgradeToUnordered() (gas: 45110367)
 TestICS04UpgradeApp:testUpgradeAuthorizationChanneNotFound() (gas: 61690)
 TestICS04UpgradeApp:testUpgradeAuthorizationRePropose() (gas: 2565379)
 TestICS04UpgradeApp:testUpgradeAuthorizationRemove() (gas: 2473938)

contracts/core/04-channel/IBCChannelUpgrade.sol

@@ -14,6 +14,7 @@ import {
 } from "../04-channel/IIBCChannelUpgrade.sol";
 import {IBCCommitment} from "../24-host/IBCCommitment.sol";
 import {IBCModuleManager} from "../26-router/IBCModuleManager.sol";
+import {IIBCModuleUpgrade} from "../26-router/IIBCModuleUpgrade.sol";
 import {IBCChannelLib} from "./IBCChannelLib.sol";
 import {IIBCChannelUpgradeErrors} from "./IIBCChannelUpgradeErrors.sol";
 
@@ -84,12 +85,13 @@ abstract contract IBCChannelUpgradeBase is
      * it will store the nextSequenceSend and upgrade timeout in the upgrade state.
      */
     function startFlushUpgradeHandshake(
+        IIBCModuleUpgrade module,
         Channel.Data storage channel,
         Upgrade.Data storage upgrade,
         string calldata portId,
         string calldata channelId
     ) internal {
-        Timeout.Data memory upgradeTimeout = getUpgradeTimeout(portId, channelId);
+        Timeout.Data memory upgradeTimeout = module.getUpgradeTimeout(portId, channelId);
         if (
             !(
                 upgradeTimeout.height.revision_number > 0 || upgradeTimeout.height.revision_height > 0
@@ -200,22 +202,6 @@ abstract contract IBCChannelUpgradeBase is
             == keccak256(bytes(proposedConnection.counterparty.connection_id));
     }
 
-    function isAuthorizedUpgrader(string calldata portId, string calldata channelId, address msgSender)
-        internal
-        view
-        returns (bool)
-    {
-        return lookupUpgradableModuleByPort(portId).isAuthorizedUpgrader(portId, channelId, msgSender);
-    }
-
-    function getUpgradeTimeout(string calldata portId, string calldata channelId)
-        internal
-        view
-        returns (Timeout.Data memory)
-    {
-        return lookupUpgradableModuleByPort(portId).getUpgradeTimeout(portId, channelId);
-    }
-
     // --------------------- Private Functions --------------------- //
 
     function writeErrorReceipt(
@@ -358,7 +344,8 @@ contract IBCChannelUpgradeInitTryAck is IBCChannelUpgradeBase, IIBCChannelUpgrad
      * @dev See {IIBCChannelUpgrade-channelUpgradeInit}
      */
     function channelUpgradeInit(MsgChannelUpgradeInit calldata msg_) public override returns (uint64) {
-        if (!isAuthorizedUpgrader(msg_.portId, msg_.channelId, _msgSender())) {
+        IIBCModuleUpgrade module = lookupUpgradableModuleByPort(msg_.portId);
+        if (!module.isAuthorizedUpgrader(msg_.portId, msg_.channelId, _msgSender())) {
             revert IBCChannelUpgradeUnauthorizedChannelUpgrader(_msgSender());
         }
 
@@ -378,9 +365,8 @@ contract IBCChannelUpgradeInitTryAck is IBCChannelUpgradeBase, IIBCChannelUpgrad
         upgrade.fields.ordering = msg_.proposedUpgradeFields.ordering;
         upgrade.fields.connection_hops = new string[](1);
         upgrade.fields.connection_hops[0] = msg_.proposedUpgradeFields.connection_hops[0];
-        upgrade.fields.version = lookupUpgradableModuleByPort(msg_.portId).onChanUpgradeInit(
-            msg_.portId, msg_.channelId, channel.upgrade_sequence, msg_.proposedUpgradeFields
-        );
+        upgrade.fields.version =
+            module.onChanUpgradeInit(msg_.portId, msg_.channelId, channel.upgrade_sequence, msg_.proposedUpgradeFields);
         updateUpgradeCommitment(msg_.portId, msg_.channelId, upgrade);
 
         emit ChannelUpgradeInit(msg_.portId, msg_.channelId, channel.upgrade_sequence, msg_.proposedUpgradeFields);
@@ -392,6 +378,7 @@ contract IBCChannelUpgradeInitTryAck is IBCChannelUpgradeBase, IIBCChannelUpgrad
      * @dev See {IIBCChannelUpgrade-channelUpgradeTry}
      */
     function channelUpgradeTry(MsgChannelUpgradeTry calldata msg_) public override returns (bool, uint64) {
+        IIBCModuleUpgrade module = lookupUpgradableModuleByPort(msg_.portId);
         // current channel must be OPEN (i.e. not in FLUSHING)
         Channel.Data storage channel = channels[msg_.portId][msg_.channelId];
         if (channel.state != Channel.State.STATE_OPEN) {
@@ -478,12 +465,11 @@ contract IBCChannelUpgradeInitTryAck is IBCChannelUpgradeBase, IIBCChannelUpgrad
 
         // call startFlushUpgradeHandshake to move channel to FLUSHING, which will block
         // upgrade from progressing to OPEN until flush completes on both ends
-        startFlushUpgradeHandshake(channel, existingUpgrade, msg_.portId, msg_.channelId);
+        startFlushUpgradeHandshake(module, channel, existingUpgrade, msg_.portId, msg_.channelId);
         updateChannelCommitment(msg_.portId, msg_.channelId, channel);
 
-        existingUpgrade.fields.version = lookupUpgradableModuleByPort(msg_.portId).onChanUpgradeTry(
-            msg_.portId, msg_.channelId, channel.upgrade_sequence, upgradeFields
-        );
+        existingUpgrade.fields.version =
+            module.onChanUpgradeTry(msg_.portId, msg_.channelId, channel.upgrade_sequence, upgradeFields);
         updateUpgradeCommitment(msg_.portId, msg_.channelId, existingUpgrade);
 
         emit ChannelUpgradeTry(
@@ -534,11 +520,13 @@ contract IBCChannelUpgradeInitTryAck is IBCChannelUpgradeBase, IIBCChannelUpgrad
             return false;
         }
 
+        IIBCModuleUpgrade module = unsafeLookupUpgradableModuleByPort(msg_.portId);
+
         if (channel.state == Channel.State.STATE_OPEN) {
             // prove counterparty and move our own state to flushing
             // if we are already at flushing, then no state changes occur
             // upgrade is blocked on this channelEnd from progressing until flush completes on its end
-            startFlushUpgradeHandshake(channel, existingUpgrade, msg_.portId, msg_.channelId);
+            startFlushUpgradeHandshake(module, channel, existingUpgrade, msg_.portId, msg_.channelId);
             // NOTE: `upgrade` and `channel` are updated only when channel.state is OPEN
             updateChannelCommitment(msg_.portId, msg_.channelId, channel);
             updateUpgradeCommitment(msg_.portId, msg_.channelId, existingUpgrade);
@@ -566,7 +554,7 @@ contract IBCChannelUpgradeInitTryAck is IBCChannelUpgradeBase, IIBCChannelUpgrad
         // module can error on counterparty version
         // ACK should not change state to the new parameters yet
         // as that will happen on the onChanUpgradeOpen callback
-        try lookupUpgradableModuleByPort(msg_.portId).onChanUpgradeAck(
+        try module.onChanUpgradeAck(
             msg_.portId, msg_.channelId, channel.upgrade_sequence, msg_.counterpartyUpgrade.fields.version
         ) {} catch {
             restoreChannel(msg_.portId, msg_.channelId, UpgradeHandshakeError.AckCallbackFailed);
@@ -641,7 +629,7 @@ contract IBCChannelUpgradeConfirmTimeoutCancel is IBCChannelUpgradeBase, IIBCCha
                 && msg_.counterpartyChannelState == Channel.State.STATE_FLUSHCOMPLETE
         ) {
             openUpgradeHandshake(msg_.portId, msg_.channelId);
-            lookupUpgradableModuleByPort(msg_.portId).onChanUpgradeOpen(
+            unsafeLookupUpgradableModuleByPort(msg_.portId).onChanUpgradeOpen(
                 msg_.portId, msg_.channelId, channel.upgrade_sequence
             );
         }
@@ -710,7 +698,7 @@ contract IBCChannelUpgradeConfirmTimeoutCancel is IBCChannelUpgradeBase, IIBCCha
 
         // open callback must not return error since counterparty successfully upgraded
         // make application state changes based on new channel parameters
-        lookupUpgradableModuleByPort(msg_.portId).onChanUpgradeOpen(
+        unsafeLookupUpgradableModuleByPort(msg_.portId).onChanUpgradeOpen(
             msg_.portId, msg_.channelId, channel.upgrade_sequence
         );
     }
@@ -731,8 +719,9 @@ contract IBCChannelUpgradeConfirmTimeoutCancel is IBCChannelUpgradeBase, IIBCCha
         // otherwise, we can only cancel if the counterparty wrote an
         // error receipt during the upgrade handshake
         if (
-            isAuthorizedUpgrader(msg_.portId, msg_.channelId, _msgSender())
-                && channel.state != Channel.State.STATE_FLUSHCOMPLETE
+            unsafeLookupUpgradableModuleByPort(msg_.portId).isAuthorizedUpgrader(
+                msg_.portId, msg_.channelId, _msgSender()
+            ) && channel.state != Channel.State.STATE_FLUSHCOMPLETE
         ) {
             return restoreChannel(msg_.portId, msg_.channelId, UpgradeHandshakeError.Cancel);
         }

contracts/core/24-host/IBCHostConfigurator.sol

@@ -35,7 +35,7 @@ abstract contract IBCHostConfigurator is IIBCHostConfigurator, IBCModuleManager
             revert IBCHostInvalidModuleAddress(address(moduleAddress));
         }
         if (!moduleAddress.supportsInterface(type(IIBCModule).interfaceId)) {
-            revert IBCHostModuleDoesNotImplementIIBCModule(type(IIBCModule).interfaceId);
+            revert IBCHostModuleDoesNotSupportIIBCModule(type(IIBCModule).interfaceId);
         }
         claimPortCapability(portId, address(moduleAddress));
     }

contracts/core/24-host/IIBCHostErrors.sol

@@ -28,7 +28,10 @@ interface IIBCHostErrors {
     error IBCHostModuleChannelNotFound(string portId, string channelId);
 
     /// @param interfaceId expected interface identifier
-    error IBCHostModuleDoesNotImplementIIBCModule(bytes4 interfaceId);
+    error IBCHostModuleDoesNotSupportIIBCModule(bytes4 interfaceId);
+
+    /// @param module module contract address
+    error IBCHostModuleDoesNotSupportIIBCModuleUpgrade(address module);
 
     /// @param portId port identifier
     error IBCHostPortCapabilityAlreadyClaimed(string portId);

contracts/core/26-router/IBCModuleManager.sol

@@ -72,13 +72,27 @@ contract IBCModuleManager is IBCHost, Context {
     }
 
     /**
-     * @dev lookupUpgradableModuleByPort will return the IBCModule along with the capability associated with a given portID
-     * If the module is not found, it will revert
+     * @dev unsafeLookupUpgradableModuleByPort will return the IBCModule corresponding to the portID
+     * It will revert if the module is not found
+     *
+     * Since the function does not check if the module supports the `IIBCModuleUpgrade` interface via ERC-165, it is unsafe but cheaper in gas cost than `lookupUpgradableModuleByPort`
      */
-    function lookupUpgradableModuleByPort(string calldata portId) internal view returns (IIBCModuleUpgrade) {
+    function unsafeLookupUpgradableModuleByPort(string calldata portId) internal view returns (IIBCModuleUpgrade) {
         return IIBCModuleUpgrade(address(lookupModuleByPort(portId)));
     }
 
+    /**
+     * @dev lookupUpgradableModuleByPort will return the IBCModule corresponding to the portID
+     * It will revert if the module does not support the `IIBCModuleUpgrade` interface or the module is not found
+     */
+    function lookupUpgradableModuleByPort(string calldata portId) internal view returns (IIBCModuleUpgrade) {
+        IIBCModule module = lookupModuleByPort(portId);
+        if (!module.supportsInterface(type(IIBCModuleUpgrade).interfaceId)) {
+            revert IBCHostModuleDoesNotSupportIIBCModuleUpgrade(address(module));
+        }
+        return IIBCModuleUpgrade(address(module));
+    }
+
     /**
      * @dev canTransitionToFlushComplete checks if the module can transition to flush complete at the given upgrade sequence
      * If the module is not found, it will revert
@@ -94,7 +108,7 @@ contract IBCModuleManager is IBCHost, Context {
                 return true;
             }
         }
-        return lookupUpgradableModuleByPort(portId).canTransitionToFlushComplete(
+        return unsafeLookupUpgradableModuleByPort(portId).canTransitionToFlushComplete(
             portId, channelId, upgradeSequence, _msgSender()
         );
     }