fix: add fix for checking for correct messageId in OPL2ToL1Ism and …

…`ArbL2ToL1Ism` via external call (hyperlane-xyz#4437)

### Description

- In agreement with Chainlight team's recommendation, added a second
isVerified() check after portal call to make sure an arbitrary call
which passes the check for metadata length and messageId cannot be
verified without calling is verifyMessageId(messageId) in `OPL2ToL1Ism`
and `ArbL2ToL1`

### Drive-by changes

None

### Related issues

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

### Backward compatibility

No, but the contracts aren't deployed anywhere

### Testing

Unit testing

.changeset/itchy-singers-hang.md

@@ -0,0 +1,5 @@
+---
+'@hyperlane-xyz/core': patch
+---
+
+Patched OPL2ToL1Ism to check for correct messageId for external call in verify

solidity/contracts/isms/hook/ArbL2ToL1Ism.sol

@@ -63,6 +63,7 @@ contract ArbL2ToL1Ism is
     ) external override returns (bool) {
         if (!isVerified(message)) {
             _verifyWithOutboxCall(metadata, message);
+            require(isVerified(message), "ArbL2ToL1Ism: message not verified");
         }
         releaseValueToRecipient(message);
         return true;

solidity/contracts/isms/hook/OPL2ToL1Ism.sol

@@ -66,9 +66,9 @@ contract OPL2ToL1Ism is
         bytes calldata metadata,
         bytes calldata message
     ) external override returns (bool) {
-        bool verified = isVerified(message);
-        if (!verified) {
+        if (!isVerified(message)) {
             _verifyWithPortalCall(metadata, message);
+            require(isVerified(message), "OPL2ToL1Ism: message not verified");
         }
         releaseValueToRecipient(message);
         return true;

solidity/test/isms/ERC5164ISM.t.sol

@@ -150,6 +150,8 @@ contract ERC5164IsmTest is ExternalBridgeTest {
 
     function test_verify_valueAlreadyClaimed(uint256) public override {}
 
+    function test_verify_false_arbitraryCall() public override {}
+
     /* ============ helper functions ============ */
 
     function _externalBridgeDestinationCall(

solidity/test/isms/ExternalBridgeTest.sol

@@ -135,14 +135,14 @@ abstract contract ExternalBridgeTest is Test {
             1 ether,
             messageId
         );
-        ism.verify(externalCalldata, encodedMessage);
+        assertTrue(ism.verify(externalCalldata, encodedMessage));
         assertEq(address(testRecipient).balance, 1 ether);
     }
 
     function test_verify_revertsWhen_invalidIsm() public virtual {
         bytes memory externalCalldata = _encodeExternalDestinationBridgeCall(
             address(hook),
-            address(this),
+            address(hook),
             0,
             messageId
         );
@@ -217,6 +217,19 @@ abstract contract ExternalBridgeTest is Test {
         assertEq(address(testRecipient).balance, _msgValue);
     }
 
+    function test_verify_false_arbitraryCall() public virtual {
+        bytes memory incorrectCalldata = _encodeExternalDestinationBridgeCall(
+            address(hook),
+            address(this),
+            0,
+            messageId
+        );
+
+        vm.expectRevert();
+        ism.verify(incorrectCalldata, encodedMessage);
+        assertFalse(ism.isVerified(encodedMessage));
+    }
+
     /* ============ helper functions ============ */
 
     function _encodeTestMessage() internal view returns (bytes memory) {
@@ -265,4 +278,7 @@ abstract contract ExternalBridgeTest is Test {
     function _setExternalOriginSender(
         address _sender
     ) internal virtual returns (bytes memory) {}
+
+    // meant to mock an arbitrary successful call made by the external bridge
+    function verifyMessageId(bytes32 /*messageId*/) public payable {}
 }

solidity/test/isms/OPStackIsm.t.sol

@@ -133,10 +133,10 @@ contract OPStackIsmTest is ExternalBridgeTest {
     }
 
     function _encodeExternalDestinationBridgeCall(
-        address _from,
-        address _to,
-        uint256 _msgValue,
-        bytes32 _messageId
+        address /*_from*/,
+        address /*_to*/,
+        uint256 /*_msgValue*/,
+        bytes32 /*_messageId*/
     ) internal pure override returns (bytes memory) {
         return new bytes(0);
     }
@@ -148,6 +148,8 @@ contract OPStackIsmTest is ExternalBridgeTest {
 
     function test_verify_revertsWhen_invalidIsm() public override {}
 
+    function test_verify_false_arbitraryCall() public override {}
+
     /* ============ ISM.verifyMessageId ============ */
 
     function test_verify_revertsWhen_notAuthorizedHook() public override {