IBC-4: fix to ensure the commitment of consensus state corresponding …

…to the height matches the previous one in `updateClientCommitments()`

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

contracts/core/02-client/IBCClient.sol

@@ -95,10 +95,17 @@ contract IBCClient is IBCHost, IIBCClient, IIBCClientErrors {
             bytes32 key = IBCCommitment.consensusStateCommitmentKey(
                 clientId, heights[i].revision_number, heights[i].revision_height
             );
-            if (commitments[key] != bytes32(0)) {
-                continue;
+            bytes32 commitment = keccak256(consensusState);
+            bytes32 prev = commitments[key];
+            if (prev != bytes32(0) && commitment != prev) {
+                // Revert if the new commitment is inconsistent with the previous one.
+                // This case may indicate misbehavior of either the LightClient or the target chain.
+                // Since the definition and specification of misbehavior are defined for each LightClient,
+                // if a relayer detects this error, it is recommended to submit an evidence of misbehaviour to the LightClient accordingly.
+                // (e.g., via the updateClient function).
+                revert IBCClientInconsistentConsensusStateCommitment(key, commitment, prev);
             }
-            commitments[key] = keccak256(consensusState);
+            commitments[key] = commitment;
         }
     }
 

contracts/core/02-client/IIBCClientErrors.sol

@@ -20,4 +20,9 @@ interface IIBCClientErrors {
     /// @param selector the function selector
     /// @param args the calldata
     error IBCClientFailedUpdateClient(bytes4 selector, bytes args);
+
+    /// @param commitmentKey the commitment key
+    /// @param commitment the commitment
+    /// @param prev the previous commitment
+    error IBCClientInconsistentConsensusStateCommitment(bytes32 commitmentKey, bytes32 commitment, bytes32 prev);
 }

docs/adr/adr-001.md

@@ -58,7 +58,16 @@ function updateClientCommitments(string calldata clientId, Height.Data[] memory
         bytes32 key = IBCCommitment.consensusStateCommitmentKey(
             clientId, heights[i].revision_number, heights[i].revision_height
         );
-        require(commitments[key] == bytes32(0), "consensus state already exists");
+        bytes32 commitment = keccak256(consensusState);
+        bytes32 prev = commitments[key];
+        if (prev != bytes32(0) && commitment != prev) {
+            // Revert if the new commitment is inconsistent with the previous one.
+            // This case may indicate misbehavior of either the LightClient or the target chain.
+            // Since the definition and specification of misbehavior are defined for each LightClient,
+            // if a relayer detects this error, it is recommended to submit an evidence of misbehaviour to the LightClient accordingly.
+            // (e.g., via the updateClient function).
+            revert IBCClientInconsistentConsensusStateCommitment(key, commitment, prev);
+        }
         commitments[key] = keccak256(consensusState);
     }
 }

tests/foundry/src/ICS02.t.sol

@@ -126,6 +126,17 @@ contract TestICS02 is Test, MockClientTestHelper {
             );
             prevClientStateCommitment = commitment;
         }
+        {
+            // update with the same height should not revert
+            handler.updateClient(msgUpdateMockClient(clientId, 3));
+            // update with the same height and different consensus state should revert
+            uint256 prev = vm.getBlockTimestamp();
+            vm.warp(prev + 1);
+            IIBCClient.MsgUpdateClient memory msg_ = msgUpdateMockClient(clientId, 3);
+            vm.expectRevert();
+            handler.updateClient(msg_);
+            vm.warp(prev);
+        }
     }
 
     function testInvalidUpdateClient() public {