fix(contracts): InterchainAccountRouter minor audit remediation (#4581)

### Description

- disabled the ICARouter's ability to change hook given that the user
doesn't expect the hook to change after they deploy their ICA account.
Hook is not part of the derivation like ism on the destination chain and
hence, cannot be configured custom by the user.

### Drive-by changes

- MailboxClient events for hook and ism setting
- ProtocolFee events for setting beneficiary and owner

### Related issues

- partly fixes
https://github.com/chainlight-io/2024-08-hyperlane/issues/14

### Backward compatibility

No

### Testing

Unit tests

.changeset/silent-berries-attend.md

@@ -0,0 +1,5 @@
+---
+'@hyperlane-xyz/core': minor
+---
+
+disabled the ICARouter's ability to change hook given that the user doesn't expect the hook to change after they deploy their ICA account. Hook is not part of the derivation like ism on the destination chain and hence, cannot be configured custom by the user.

solidity/contracts/client/MailboxClient.sol

@@ -1,6 +1,18 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity >=0.6.11;
 
+/*@@@@@@@       @@@@@@@@@
+ @@@@@@@@@       @@@@@@@@@
+  @@@@@@@@@       @@@@@@@@@
+   @@@@@@@@@       @@@@@@@@@
+    @@@@@@@@@@@@@@@@@@@@@@@@@
+     @@@@@  HYPERLANE  @@@@@@@
+    @@@@@@@@@@@@@@@@@@@@@@@@@
+   @@@@@@@@@       @@@@@@@@@
+  @@@@@@@@@       @@@@@@@@@
+ @@@@@@@@@       @@@@@@@@@
+@@@@@@@@@       @@@@@@@@*/
+
 // ============ Internal Imports ============
 import {IMailbox} from "../interfaces/IMailbox.sol";
 import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";
@@ -15,6 +27,9 @@ import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/Own
 abstract contract MailboxClient is OwnableUpgradeable, PackageVersioned {
     using Message for bytes;
 
+    event HookSet(address _hook);
+    event IsmSet(address _ism);
+
     IMailbox public immutable mailbox;
 
     uint32 public immutable localDomain;
@@ -63,8 +78,11 @@ abstract contract MailboxClient is OwnableUpgradeable, PackageVersioned {
      * @notice Sets the address of the application's custom hook.
      * @param _hook The address of the hook contract.
      */
-    function setHook(address _hook) public onlyContractOrNull(_hook) onlyOwner {
+    function setHook(
+        address _hook
+    ) public virtual onlyContractOrNull(_hook) onlyOwner {
         hook = IPostDispatchHook(_hook);
+        emit HookSet(_hook);
     }
 
     /**
@@ -75,6 +93,7 @@ abstract contract MailboxClient is OwnableUpgradeable, PackageVersioned {
         address _module
     ) public onlyContractOrNull(_module) onlyOwner {
         interchainSecurityModule = IInterchainSecurityModule(_module);
+        emit IsmSet(_module);
     }
 
     // ======== Initializer =========

solidity/contracts/hooks/ProtocolFee.sol

@@ -32,6 +32,9 @@ contract ProtocolFee is AbstractPostDispatchHook, Ownable {
     using Address for address payable;
     using Message for bytes;
 
+    event ProtocolFeeSet(uint256 protocolFee);
+    event BeneficiarySet(address beneficiary);
+
     // ============ Constants ============
 
     /// @notice The maximum protocol fee that can be set.
@@ -126,6 +129,7 @@ contract ProtocolFee is AbstractPostDispatchHook, Ownable {
             "ProtocolFee: exceeds max protocol fee"
         );
         protocolFee = _protocolFee;
+        emit ProtocolFeeSet(_protocolFee);
     }
 
     /**
@@ -135,5 +139,6 @@ contract ProtocolFee is AbstractPostDispatchHook, Ownable {
     function _setBeneficiary(address _beneficiary) internal {
         require(_beneficiary != address(0), "ProtocolFee: invalid beneficiary");
         beneficiary = _beneficiary;
+        emit BeneficiarySet(_beneficiary);
     }
 }

solidity/contracts/middleware/InterchainAccountRouter.sol

@@ -22,6 +22,7 @@ import {TypeCasts} from "../libs/TypeCasts.sol";
 import {StandardHookMetadata} from "../hooks/libs/StandardHookMetadata.sol";
 import {EnumerableMapExtended} from "../libs/EnumerableMapExtended.sol";
 import {Router} from "../client/Router.sol";
+import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";
 
 // ============ External Imports ============
 import {Create2} from "@openzeppelin/contracts/utils/Create2.sol";
@@ -160,6 +161,12 @@ contract InterchainAccountRouter is Router {
         }
     }
 
+    function setHook(
+        address _hook
+    ) public override onlyContractOrNull(_hook) onlyOwner {
+        hook = IPostDispatchHook(_hook);
+    }
+
     // ============ External Functions ============
     /**
      * @notice Dispatches a single remote call to be made by an owner's
@@ -600,7 +607,14 @@ contract InterchainAccountRouter is Router {
     ) private returns (bytes32) {
         require(_router != bytes32(0), "no router specified for destination");
         emit RemoteCallDispatched(_destination, msg.sender, _router, _ism);
-        return mailbox.dispatch{value: msg.value}(_destination, _router, _body);
+        return
+            mailbox.dispatch{value: msg.value}(
+                _destination,
+                _router,
+                _body,
+                new bytes(0),
+                hook
+            );
     }
 
     /**
@@ -625,7 +639,8 @@ contract InterchainAccountRouter is Router {
                 _destination,
                 _router,
                 _body,
-                _hookMetadata
+                _hookMetadata,
+                hook
             );
     }
 
@@ -665,7 +680,13 @@ contract InterchainAccountRouter is Router {
     function quoteGasPayment(
         uint32 _destination
     ) external view returns (uint256 _gasPayment) {
-        return _quoteDispatch(_destination, "");
+        return
+            _Router_quoteDispatch(
+                _destination,
+                new bytes(0),
+                new bytes(0),
+                address(hook)
+            );
     }
 
     /**
@@ -679,13 +700,12 @@ contract InterchainAccountRouter is Router {
         bytes calldata _messageBody,
         uint256 gasLimit
     ) external view returns (uint256 _gasPayment) {
-        bytes32 _router = _mustHaveRemoteRouter(_destination);
         return
-            mailbox.quoteDispatch(
+            _Router_quoteDispatch(
                 _destination,
-                _router,
                 _messageBody,
-                StandardHookMetadata.overrideGasLimit(gasLimit)
+                StandardHookMetadata.overrideGasLimit(gasLimit),
+                address(hook)
             );
     }
 }

solidity/contracts/test/TestPostDispatchHook.sol

@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity >=0.8.0;
+
 import {Message} from "../libs/Message.sol";
 
 import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol";
@@ -35,15 +36,15 @@ contract TestPostDispatchHook is AbstractPostDispatchHook {
 
     // ============ Internal functions ============
     function _postDispatch(
-        bytes calldata /*metadata*/,
-        bytes calldata message
+        bytes calldata,
+        /*metadata*/ bytes calldata message
     ) internal override {
         messageDispatched[message.id()] = true;
     }
 
     function _quoteDispatch(
-        bytes calldata /*metadata*/,
-        bytes calldata /*message*/
+        bytes calldata,
+        /*metadata*/ bytes calldata /*message*/
     ) internal view override returns (uint256) {
         return fee;
     }

solidity/test/InterchainAccountRouter.t.sol

@@ -13,6 +13,8 @@ import {TestInterchainGasPaymaster} from "../contracts/test/TestInterchainGasPay
 import {IPostDispatchHook} from "../contracts/interfaces/hooks/IPostDispatchHook.sol";
 import {CallLib, OwnableMulticall, InterchainAccountRouter} from "../contracts/middleware/InterchainAccountRouter.sol";
 import {InterchainAccountIsm} from "../contracts/isms/routing/InterchainAccountIsm.sol";
+import {AbstractPostDispatchHook} from "../contracts/hooks/libs/AbstractPostDispatchHook.sol";
+import {TestPostDispatchHook} from "../contracts/test/TestPostDispatchHook.sol";
 
 contract Callable {
     mapping(address => bytes32) public data;
@@ -108,10 +110,11 @@ contract InterchainAccountRouterTestBase is Test {
         address owner = address(this);
         originIcaRouter = deployProxiedIcaRouter(
             environment.mailboxes(origin),
-            environment.igps(destination),
+            environment.igps(origin),
             icaIsm,
             owner
         );
+
         destinationIcaRouter = deployProxiedIcaRouter(
             environment.mailboxes(destination),
             environment.igps(destination),
@@ -392,6 +395,25 @@ contract InterchainAccountRouterTest is InterchainAccountRouterTestBase {
         );
     }
 
+    function test_quoteDispatch_differentHook() public {
+        // arrange
+        TestPostDispatchHook testHook = new TestPostDispatchHook();
+        originIcaRouter = deployProxiedIcaRouter(
+            environment.mailboxes(origin),
+            testHook,
+            icaIsm,
+            address(this)
+        );
+        originIcaRouter.enrollRemoteRouterAndIsm(
+            destination,
+            routerOverride,
+            ismOverride
+        );
+
+        // assert
+        assertEq(originIcaRouter.quoteGasPayment(destination), 0);
+    }
+
     function testFuzz_singleCallRemoteWithDefault(
         bytes32 data,
         uint256 value
@@ -444,6 +466,35 @@ contract InterchainAccountRouterTest is InterchainAccountRouterTestBase {
         assertIgpPayment(balanceBefore, balanceAfter, igp.getDefaultGasUsage());
     }
 
+    function testFuzz_callRemoteWithDefault_differentHook(
+        bytes32 data,
+        uint256 value
+    ) public {
+        // arrange
+        TestPostDispatchHook testHook = new TestPostDispatchHook();
+        originIcaRouter = deployProxiedIcaRouter(
+            environment.mailboxes(origin),
+            testHook,
+            icaIsm,
+            address(this)
+        );
+        originIcaRouter.enrollRemoteRouterAndIsm(
+            destination,
+            routerOverride,
+            ismOverride
+        );
+
+        // assert
+        vm.expectCall(
+            address(testHook),
+            0,
+            abi.encodePacked(AbstractPostDispatchHook.postDispatch.selector)
+        );
+
+        // act
+        originIcaRouter.callRemote(destination, getCalls(data, value));
+    }
+
     function testFuzz_overrideAndCallRemote(
         bytes32 data,
         uint256 value
@@ -558,6 +609,7 @@ contract InterchainAccountRouterTest is InterchainAccountRouterTestBase {
         uint256 balanceAfter = address(this).balance;
         assertRemoteCallReceived(data, value);
         assertIgpPayment(balanceBefore, balanceAfter, igp.getDefaultGasUsage());
+        assertEq(address(originIcaRouter.hook()), address(0));
     }
 
     function testFuzz_callRemoteWithOverrides_metadata(
@@ -591,6 +643,38 @@ contract InterchainAccountRouterTest is InterchainAccountRouterTestBase {
         assertIgpPayment(balanceBefore, balanceAfter, gasLimit);
     }
 
+    function testFuzz_callRemoteWithOverrides_withHook(
+        bytes32 data,
+        uint256 value
+    ) public {
+        TestPostDispatchHook testHook = new TestPostDispatchHook();
+
+        originIcaRouter = deployProxiedIcaRouter(
+            environment.mailboxes(origin),
+            testHook,
+            icaIsm,
+            address(this)
+        );
+        originIcaRouter.enrollRemoteRouterAndIsm(
+            destination,
+            routerOverride,
+            ismOverride
+        );
+
+        vm.expectCall(
+            address(testHook),
+            0,
+            abi.encodePacked(AbstractPostDispatchHook.postDispatch.selector)
+        );
+        originIcaRouter.callRemoteWithOverrides(
+            destination,
+            routerOverride,
+            ismOverride,
+            getCalls(data, value),
+            new bytes(0)
+        );
+    }
+
     function testFuzz_callRemoteWithFailingIsmOverride(
         bytes32 data,
         uint256 value

solidity/test/hooks/ProtocolFee.t.sol

@@ -42,6 +42,9 @@ contract ProtocolFeeTest is Test {
 
     function testSetProtocolFee(uint256 fee) public {
         fee = bound(fee, 0, fees.MAX_PROTOCOL_FEE());
+
+        vm.expectEmit(true, true, true, true);
+        emit ProtocolFee.ProtocolFeeSet(fee);
         fees.setProtocolFee(fee);
         assertEq(fees.protocolFee(), fee);
     }
@@ -69,6 +72,13 @@ contract ProtocolFeeTest is Test {
         assertEq(fees.protocolFee(), FEE);
     }
 
+    function testSetBeneficiary(address beneficiary) public {
+        vm.expectEmit(true, true, true, true);
+        emit ProtocolFee.BeneficiarySet(beneficiary);
+        fees.setBeneficiary(beneficiary);
+        assertEq(fees.beneficiary(), beneficiary);
+    }
+
     function testSetBeneficiary_revertWhen_notOwner() public {
         vm.prank(charlie);