From 7e97315c6ee6530d06af5c637034bfe2b268c513 Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Mon, 25 Nov 2024 15:11:18 +0900 Subject: [PATCH] IBC-11: fix `clientType` and `clientId` validations Signed-off-by: Jun Kimura --- contracts/core/02-client/IBCClient.sol | 4 +++ contracts/core/02-client/IBCClientLib.sol | 32 +++++++++++++++---- contracts/core/02-client/IIBCClientErrors.sol | 3 ++ tests/foundry/src/ICS02.t.sol | 18 ++++++++--- 4 files changed, 45 insertions(+), 12 deletions(-) diff --git a/contracts/core/02-client/IBCClient.sol b/contracts/core/02-client/IBCClient.sol index 9f2100d6..9e3d2d18 100644 --- 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 index 3e3c7c25..d800da10 100644 --- a/contracts/core/02-client/IBCClientLib.sol +++ b/contracts/core/02-client/IBCClientLib.sol @@ -4,15 +4,20 @@ 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 @@ -20,8 +25,8 @@ library IBCClientLib { } 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 index cee43296..16d76f43 100644 --- 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 index 30bad122..2fc297df 100644 --- 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 {