IBC-11: fix clientType and clientId validations

Signed-off-by: Jun Kimura <[email protected]>
hyperledger-labs · Dec 4, 2024 · 7e97315 · 7e97315
1 parent 4a6f4c9
commit 7e97315
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 12 deletions.
diff --git a/contracts/core/02-client/IBCClient.sol b/contracts/core/02-client/IBCClient.sol
@@ -7,6 +7,7 @@ import {Height} from "../../proto/Client.sol";
 import {IBCHost} from "../24-host/IBCHost.sol";
 import {IBCCommitment} from "../24-host/IBCCommitment.sol";
 import {IIBCClient} from "../02-client/IIBCClient.sol";
+import {IBCClientLib} from "../02-client/IBCClientLib.sol";
 import {IIBCClientErrors} from "../02-client/IIBCClientErrors.sol";
 
 /**
@@ -116,6 +117,9 @@ contract IBCClient is IBCHost, IIBCClient, IIBCClientErrors {
         HostStorage storage hostStorage = getHostStorage();
         string memory identifier =
             string(abi.encodePacked(clientType, "-", Strings.toString(hostStorage.nextClientSequence)));
+        if (!IBCClientLib.validateClientId(bytes(identifier))) {
+            revert IBCClientInvalidClientId(identifier);
+        }
         hostStorage.nextClientSequence++;
         return identifier;
     }

diff --git a/contracts/core/02-client/IBCClientLib.sol b/contracts/core/02-client/IBCClientLib.sol
@@ -4,24 +4,29 @@ pragma solidity ^0.8.20;
 library IBCClientLib {
     /**
      * @dev validateClientType validates the client type
-     *   - clientType must be non-empty
-     *   - clientType must be in the form of `^[a-z][a-z0-9-]*[a-z0-9]$`
+     *      - clientType must be non-empty
+     *      - clientType must be between 7 and 62 characters long
+     *        - This is because the length of the client ID is 9-64 characters long in the ICS-24, and the client ID is composed of the client type and the counter suffix (minimum 2 characters long).
+     *      - clientType must be in the form of `^[a-z][a-z0-9-]*[a-z0-9]$`
      */
     function validateClientType(bytes memory clientTypeBytes) internal pure returns (bool) {
-        uint256 byteLength = clientTypeBytes.length;
-        if (byteLength == 0) {
+        uint256 bytesLength = clientTypeBytes.length;
+        if (bytesLength == 0) {
             return false;
         }
-        for (uint256 i = 0; i < byteLength; i++) {
+        if (bytesLength < 7 || bytesLength > 62) {
+            return false;
+        }
+        for (uint256 i = 0; i < bytesLength; i++) {
             uint256 c = uint256(uint8(clientTypeBytes[i]));
             if (0x61 <= c && c <= 0x7a) {
                 // a-z
                 continue;
             } else if (c == 0x2d) {
                 // hyphen cannot be the first or last character
                 unchecked {
-                    // SAFETY: `byteLength` is greater than 0
-                    if (i == 0 || i == byteLength - 1) {
+                    // SAFETY: `bytesLength` is greater than 0
+                    if (i == 0 || i == bytesLength - 1) {
                         return false;
                     }
                 }
@@ -35,4 +40,17 @@ library IBCClientLib {
         }
         return true;
     }
+
+    /**
+     * @dev validateClientId validates the client ID
+     *      NOTE: The client ID must be composed of the client type is validated by `validateClientType` and the counter suffix.
+     *      - clientId must be between 9 and 64 characters long
+     */
+    function validateClientId(bytes memory clientIdBytes) internal pure returns (bool) {
+        uint256 bytesLength = clientIdBytes.length;
+        if (bytesLength < 9 || bytesLength > 64) {
+            return false;
+        }
+        return true;
+    }
 }
diff --git a/contracts/core/02-client/IIBCClientErrors.sol b/contracts/core/02-client/IIBCClientErrors.sol
@@ -4,6 +4,9 @@ pragma solidity ^0.8.20;
 import {Height} from "../../proto/Client.sol";
 
 interface IIBCClientErrors {
+    /// @param clientId the client identifier
+    error IBCClientInvalidClientId(string clientId);
+
     /// @param clientType the client type
     error IBCClientUnregisteredClientType(string clientType);
 

diff --git a/tests/foundry/src/ICS02.t.sol b/tests/foundry/src/ICS02.t.sol
@@ -18,7 +18,7 @@ contract TestICS02 is Test, MockClientTestHelper {
         TestableIBCHandler handler = defaultIBCHandler();
         MockClient mockClient = new MockClient(address(handler));
         handler.registerClient(MOCK_CLIENT_TYPE, mockClient);
-        handler.registerClient("test", mockClient);
+        handler.registerClient("testtes", mockClient);
     }
 
     function testRegisterClientDuplicatedClientType() public {
@@ -37,14 +37,22 @@ contract TestICS02 is Test, MockClientTestHelper {
         handler.registerClient(MOCK_CLIENT_TYPE, ILightClient(address(0)));
 
         MockClient mockClient = new MockClient(address(handler));
+
+        // empty client type
         vm.expectRevert(abi.encodeWithSelector(IIBCHostErrors.IBCHostInvalidClientType.selector, ""));
         handler.registerClient("", mockClient);
 
-        vm.expectRevert(abi.encodeWithSelector(IIBCHostErrors.IBCHostInvalidClientType.selector, "-mock"));
-        handler.registerClient("-mock", mockClient);
+        // too short client type
+        vm.expectRevert(abi.encodeWithSelector(IIBCHostErrors.IBCHostInvalidClientType.selector, "testte"));
+        handler.registerClient("testte", mockClient);
+
+        // first character is not a letter
+        vm.expectRevert(abi.encodeWithSelector(IIBCHostErrors.IBCHostInvalidClientType.selector, "-mocktest"));
+        handler.registerClient("-mocktest", mockClient);
 
-        vm.expectRevert(abi.encodeWithSelector(IIBCHostErrors.IBCHostInvalidClientType.selector, "mock-"));
-        handler.registerClient("mock-", mockClient);
+        // last character is not a letter or number
+        vm.expectRevert(abi.encodeWithSelector(IIBCHostErrors.IBCHostInvalidClientType.selector, "mocktest-"));
+        handler.registerClient("mocktest-", mockClient);
     }
 
     function testHeightToUint128(Height.Data memory height) public pure {