Remediate Trevor's comments (#2835)

Co-authored-by: Yorke Rhodes <yorke@hyperlane.xyz>

solidity/contracts/hooks/MerkleTreeHook.sol

@@ -17,6 +17,7 @@ import {MerkleLib} from "../libs/Merkle.sol";
 import {Message} from "../libs/Message.sol";
 import {MailboxClient} from "../client/MailboxClient.sol";
 import {Indexed} from "../libs/Indexed.sol";
+import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";
 import {AbstractPostDispatchHook} from "./libs/AbstractPostDispatchHook.sol";
 import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol";
 
@@ -49,6 +50,13 @@ contract MerkleTreeHook is AbstractPostDispatchHook, MailboxClient, Indexed {
         return (root(), count() - 1);
     }
 
+    // ============ External Functions ============
+
+    /// @inheritdoc IPostDispatchHook
+    function hookType() external pure override returns (uint8) {
+        return uint8(IPostDispatchHook.Types.MERKLE_TREE);
+    }
+
     // ============ Internal Functions ============
 
     /// @inheritdoc AbstractPostDispatchHook

solidity/contracts/hooks/OPStackHook.sol

@@ -48,7 +48,7 @@ contract OPStackHook is AbstractMessageIdAuthHook {
     constructor(
         address _mailbox,
         uint32 _destinationDomain,
-        address _ism,
+        bytes32 _ism,
         address _l1Messenger
     ) AbstractMessageIdAuthHook(_mailbox, _destinationDomain, _ism) {
         require(
@@ -58,8 +58,7 @@ contract OPStackHook is AbstractMessageIdAuthHook {
         l1Messenger = ICrossDomainMessenger(_l1Messenger);
     }
 
-    // ============ External functions ============
-
+    // ============ Internal functions ============
     function _quoteDispatch(bytes calldata, bytes calldata)
         internal
         pure
@@ -69,8 +68,6 @@ contract OPStackHook is AbstractMessageIdAuthHook {
         return 0; // gas subsidized by the L2
     }
 
-    // ============ Internal functions ============
-
     /// @inheritdoc AbstractMessageIdAuthHook
     function _sendMessageId(bytes calldata metadata, bytes memory payload)
         internal
@@ -81,7 +78,7 @@ contract OPStackHook is AbstractMessageIdAuthHook {
             "OPStackHook: msgValue must be less than 2 ** 255"
         );
         l1Messenger.sendMessage{value: metadata.msgValue(0)}(
-            ism,
+            TypeCasts.bytes32ToAddress(ism),
             payload,
             GAS_LIMIT
         );

solidity/contracts/hooks/PausableHook.sol

@@ -14,6 +14,7 @@ pragma solidity >=0.8.0;
 @@@@@@@@@       @@@@@@@@*/
 
 import {StandardHookMetadata} from "../hooks/libs/StandardHookMetadata.sol";
+import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";
 import {AbstractPostDispatchHook} from "./libs/AbstractPostDispatchHook.sol";
 
 import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
@@ -32,6 +33,13 @@ contract PausableHook is AbstractPostDispatchHook, Ownable, Pausable {
         _unpause();
     }
 
+    // ============ External Functions ============
+
+    /// @inheritdoc IPostDispatchHook
+    function hookType() external pure override returns (uint8) {
+        return uint8(IPostDispatchHook.Types.PAUSABLE);
+    }
+
     // ============ Internal functions ============
 
     /// @inheritdoc AbstractPostDispatchHook

solidity/contracts/hooks/StaticProtocolFee.sol

@@ -17,6 +17,7 @@ pragma solidity >=0.8.0;
 import {Message} from "../libs/Message.sol";
 import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol";
 import {AbstractPostDispatchHook} from "./libs/AbstractPostDispatchHook.sol";
+import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";
 
 // ============ External Imports ============
 import {Address} from "@openzeppelin/contracts/utils/Address.sol";
@@ -59,6 +60,11 @@ contract StaticProtocolFee is AbstractPostDispatchHook, Ownable {
 
     // ============ External Functions ============
 
+    /// @inheritdoc IPostDispatchHook
+    function hookType() external pure override returns (uint8) {
+        return uint8(IPostDispatchHook.Types.PROTOCOL_FEE);
+    }
+
     /**
      * @notice Sets the protocol fee.
      * @param _protocolFee The new protocol fee.

solidity/contracts/hooks/aggregation/ERC5164Hook.sol

@@ -33,7 +33,7 @@ contract ERC5164Hook is AbstractMessageIdAuthHook {
     constructor(
         address _mailbox,
         uint32 _destinationDomain,
-        address _ism,
+        bytes32 _ism,
         address _dispatcher
     ) AbstractMessageIdAuthHook(_mailbox, _destinationDomain, _ism) {
         require(
@@ -43,13 +43,15 @@ contract ERC5164Hook is AbstractMessageIdAuthHook {
         dispatcher = IMessageDispatcher(_dispatcher);
     }
 
+    // ============ Internal Functions ============
+
     function _quoteDispatch(bytes calldata, bytes calldata)
         internal
         pure
         override
         returns (uint256)
     {
-        revert("not implemented");
+        return 0; // EIP-5164 doesn't enforce a gas abstraction
     }
 
     function _sendMessageId(
@@ -58,6 +60,10 @@ contract ERC5164Hook is AbstractMessageIdAuthHook {
         bytes memory payload
     ) internal override {
         require(msg.value == 0, "ERC5164Hook: no value allowed");
-        dispatcher.dispatchMessage(destinationDomain, ism, payload);
+        dispatcher.dispatchMessage(
+            destinationDomain,
+            TypeCasts.bytes32ToAddress(ism),
+            payload
+        );
     }
 }

solidity/contracts/hooks/aggregation/StaticAggregationHook.sol

@@ -23,6 +23,11 @@ contract StaticAggregationHook is AbstractPostDispatchHook {
 
     // ============ External functions ============
 
+    /// @inheritdoc IPostDispatchHook
+    function hookType() external pure override returns (uint8) {
+        return uint8(IPostDispatchHook.Types.AGGREGATION);
+    }
+
     /// @inheritdoc AbstractPostDispatchHook
     function _postDispatch(bytes calldata metadata, bytes calldata message)
         internal

solidity/contracts/hooks/igp/InterchainGasPaymaster.sol

@@ -92,6 +92,11 @@ contract InterchainGasPaymaster is
 
     // ============ External Functions ============
 
+    /// @inheritdoc IPostDispatchHook
+    function hookType() external pure override returns (uint8) {
+        return uint8(IPostDispatchHook.Types.INTERCHAIN_GAS_PAYMASTER);
+    }
+
     /**
      * @param _owner The owner of the contract.
      * @param _beneficiary The beneficiary.

solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol

@@ -14,6 +14,7 @@ pragma solidity >=0.8.0;
 @@@@@@@@@       @@@@@@@@*/
 
 // ============ Internal Imports ============
+import {IPostDispatchHook} from "../../interfaces/hooks/IPostDispatchHook.sol";
 import {AbstractPostDispatchHook} from "./AbstractPostDispatchHook.sol";
 import {AbstractMessageIdAuthorizedIsm} from "../../isms/hook/AbstractMessageIdAuthorizedIsm.sol";
 import {TypeCasts} from "../../libs/TypeCasts.sol";
@@ -35,8 +36,8 @@ abstract contract AbstractMessageIdAuthHook is
 
     // ============ Constants ============
 
-    // address for ISM to verify messages
-    address public immutable ism;
+    // left-padded address for ISM to verify messages
+    bytes32 public immutable ism;
     // Domain of chain on which the ISM is deployed
     uint32 public immutable destinationDomain;
 
@@ -45,9 +46,9 @@ abstract contract AbstractMessageIdAuthHook is
     constructor(
         address _mailbox,
         uint32 _destinationDomain,
-        address _ism
+        bytes32 _ism
     ) MailboxClient(_mailbox) {
-        require(_ism != address(0), "AbstractMessageIdAuthHook: invalid ISM");
+        require(_ism != bytes32(0), "AbstractMessageIdAuthHook: invalid ISM");
         require(
             _destinationDomain != 0,
             "AbstractMessageIdAuthHook: invalid destination domain"
@@ -56,6 +57,11 @@ abstract contract AbstractMessageIdAuthHook is
         destinationDomain = _destinationDomain;
     }
 
+    /// @inheritdoc IPostDispatchHook
+    function hookType() external pure returns (uint8) {
+        return uint8(IPostDispatchHook.Types.ID_AUTH_ISM);
+    }
+
     // ============ Internal functions ============
 
     /// @inheritdoc AbstractPostDispatchHook

solidity/contracts/hooks/routing/DomainRoutingHook.sol

@@ -42,8 +42,15 @@ contract DomainRoutingHook is AbstractPostDispatchHook, MailboxClient {
         _transferOwnership(_owner);
     }
 
-    function setHook(uint32 destination, address hook) public onlyOwner {
-        hooks[destination] = IPostDispatchHook(hook);
+    // ============ External Functions ============
+
+    /// @inheritdoc IPostDispatchHook
+    function hookType() external pure virtual override returns (uint8) {
+        return uint8(IPostDispatchHook.Types.ROUTING);
+    }
+
+    function setHook(uint32 _destination, address _hook) public onlyOwner {
+        hooks[_destination] = IPostDispatchHook(_hook);
     }
 
     function setHooks(HookConfig[] calldata configs) external onlyOwner {

solidity/contracts/hooks/routing/FallbackDomainRoutingHook.sol

@@ -36,17 +36,25 @@ contract FallbackDomainRoutingHook is DomainRoutingHook {
         fallbackHook = IPostDispatchHook(_fallback);
     }
 
+    // ============ External Functions ============
+
+    /// @inheritdoc IPostDispatchHook
+    function hookType() external pure override returns (uint8) {
+        return uint8(IPostDispatchHook.Types.FALLBACK_ROUTING);
+    }
+
     // ============ Internal Functions ============
 
     function _getConfiguredHook(bytes calldata message)
         internal
         view
         override
-        returns (IPostDispatchHook hook)
+        returns (IPostDispatchHook)
     {
-        hook = hooks[message.destination()];
-        if (address(hook) == address(0)) {
-            hook = fallbackHook;
+        IPostDispatchHook _hook = hooks[message.destination()];
+        if (address(_hook) == address(0)) {
+            _hook = fallbackHook;
         }
+        return _hook;
     }
 }

solidity/contracts/interfaces/hooks/IPostDispatchHook.sol

@@ -14,6 +14,23 @@ pragma solidity >=0.8.0;
 @@@@@@@@@       @@@@@@@@*/
 
 interface IPostDispatchHook {
+    enum Types {
+        UNUSED,
+        ROUTING,
+        AGGREGATION,
+        MERKLE_TREE,
+        INTERCHAIN_GAS_PAYMASTER,
+        FALLBACK_ROUTING,
+        ID_AUTH_ISM,
+        PAUSABLE,
+        PROTOCOL_FEE
+    }
+
+    /**
+     * @notice Returns an enum that represents the type of hook
+     */
+    function hookType() external view returns (uint8);
+
     /**
      * @notice Returns whether the hook supports metadata
      * @param metadata metadata

solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol

@@ -18,7 +18,6 @@ pragma solidity >=0.8.0;
 import {IInterchainSecurityModule} from "../../interfaces/IInterchainSecurityModule.sol";
 import {LibBit} from "../../libs/LibBit.sol";
 import {Message} from "../../libs/Message.sol";
-import {TypeCasts} from "../../libs/TypeCasts.sol";
 
 // ============ External Imports ============
 
@@ -46,9 +45,9 @@ abstract contract AbstractMessageIdAuthorizedIsm is
     /// @dev the first bit is reserved for verification and the rest 255 bits are for the msg.value
     mapping(bytes32 => uint256) public verifiedMessages;
     /// @notice Index of verification bit in verifiedMessages
-    uint256 public constant MASK_INDEX = 255;
-    /// @notice Address for the authorized hook
-    address public authorizedHook;
+    uint256 public constant VERIFIED_MASK_INDEX = 255;
+    /// @notice address for the authorized hook
+    bytes32 public authorizedHook;
 
     // ============ Events ============
 
@@ -57,9 +56,9 @@ abstract contract AbstractMessageIdAuthorizedIsm is
 
     // ============ Initializer ============
 
-    function setAuthorizedHook(address _hook) external initializer {
+    function setAuthorizedHook(bytes32 _hook) external initializer {
         require(
-            _hook != address(0),
+            _hook != bytes32(0),
             "AbstractMessageIdAuthorizedIsm: invalid authorized hook"
         );
         authorizedHook = _hook;
@@ -79,12 +78,18 @@ abstract contract AbstractMessageIdAuthorizedIsm is
         bytes32 messageId = message.id();
 
         // check for the first bit (used for verification)
-        bool verified = verifiedMessages[messageId].isBitSet(MASK_INDEX);
+        bool verified = verifiedMessages[messageId].isBitSet(
+            VERIFIED_MASK_INDEX
+        );
         // rest 255 bits contains the msg.value passed from the hook
         if (verified) {
-            payable(message.recipientAddress()).sendValue(
-                verifiedMessages[messageId].clearBit(MASK_INDEX)
+            uint256 _msgValue = verifiedMessages[messageId].clearBit(
+                VERIFIED_MASK_INDEX
             );
+            if (_msgValue > 0) {
+                verifiedMessages[messageId] -= _msgValue;
+                payable(message.recipientAddress()).sendValue(_msgValue);
+            }
         }
         return verified;
     }
@@ -99,8 +104,12 @@ abstract contract AbstractMessageIdAuthorizedIsm is
             _isAuthorized(),
             "AbstractMessageIdAuthorizedIsm: sender is not the hook"
         );
+        require(
+            msg.value < 2**VERIFIED_MASK_INDEX,
+            "AbstractMessageIdAuthorizedIsm: msg.value must be less than 2^255"
+        );
 
-        verifiedMessages[messageId] = msg.value.setBit(MASK_INDEX);
+        verifiedMessages[messageId] = msg.value.setBit(VERIFIED_MASK_INDEX);
         emit ReceivedMessage(messageId);
     }
 

solidity/contracts/isms/hook/OPStackIsm.sol

@@ -52,6 +52,7 @@ contract OPStackIsm is
      * @notice Check if sender is authorized to message `verifyMessageId`.
      */
     function _isAuthorized() internal view override returns (bool) {
-        return _crossChainSender() == authorizedHook;
+        return
+            _crossChainSender() == TypeCasts.bytes32ToAddress(authorizedHook);
     }
 }

solidity/contracts/test/TestPostDispatchHook.sol

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity >=0.8.0;
 
+import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";
 import {AbstractPostDispatchHook} from "../hooks/libs/AbstractPostDispatchHook.sol";
 
 contract TestPostDispatchHook is AbstractPostDispatchHook {
@@ -9,6 +10,13 @@ contract TestPostDispatchHook is AbstractPostDispatchHook {
     // test fees for quoteDispatch
     uint256 public fee = 0;
 
+    // ============ External Functions ============
+
+    /// @inheritdoc IPostDispatchHook
+    function hookType() external pure override returns (uint8) {
+        return uint8(IPostDispatchHook.Types.UNUSED);
+    }
+
     function supportsMetadata(bytes calldata)
         public
         pure

solidity/test/MerkleTreeHook.t.sol

@@ -7,6 +7,7 @@ import {Message} from "../contracts/libs/Message.sol";
 import {TypeCasts} from "../contracts/libs/TypeCasts.sol";
 import {TestMerkleTreeHook} from "../contracts/test/TestMerkleTreeHook.sol";
 import {TestMailbox} from "../contracts/test/TestMailbox.sol";
+import {IPostDispatchHook} from "../contracts/interfaces/hooks/IPostDispatchHook.sol";
 
 contract MerkleTreeHookTest is Test {
     using Message for bytes;
@@ -68,4 +69,8 @@ contract MerkleTreeHookTest is Test {
             assertEq(hook.quoteDispatch("", currMessage), 0);
         }
     }
+
+    function testHookType() public {
+        assertEq(hook.hookType(), uint8(IPostDispatchHook.Types.MERKLE_TREE));
+    }
 }