Fix identifier validations

.gas-snapshot

@@ -1,8 +1,8 @@
-IBCTest:testBenchmarkCreateMockClient() (gas: 214230)
-IBCTest:testBenchmarkRecvPacket() (gas: 157491)
+IBCTest:testBenchmarkCreateMockClient() (gas: 214273)
+IBCTest:testBenchmarkRecvPacket() (gas: 156079)
 IBCTest:testBenchmarkSendPacket() (gas: 86050)
-IBCTest:testBenchmarkUpdateMockClient() (gas: 129918)
-IBCTest:testConnectionOpenInit() (gas: 496526)
+IBCTest:testBenchmarkUpdateMockClient() (gas: 129961)
+IBCTest:testConnectionOpenInit() (gas: 496569)
 IBCTest:testSendPacketInvalidTimestamp() (gas: 39277)
 IBCTest:testToUint128((uint64,uint64)) (runs: 256, μ: 1047, ~: 1047)
 TestICS20:testAddressToHex(address) (runs: 256, μ: 23720, ~: 23889)

contracts/core/02-client/IBCClient.sol

@@ -17,6 +17,7 @@ contract IBCClient is IBCStore, IIBCClient {
      * @dev registerClient registers a new client type into the client registry
      */
     function registerClient(string calldata clientType, ILightClient client) external override {
+        require(validateClientType(bytes(clientType)), "invalid clientType");
         require(address(clientRegistry[clientType]) == address(0), "clientType already exists");
         require(address(client) != address(this) && Address.isContract(address(client)), "invalid client address");
         clientRegistry[clientType] = address(client);
@@ -36,7 +37,7 @@ contract IBCClient is IBCStore, IIBCClient {
         require(ok, "failed to create client");
 
         // update commitments
-        commitments[keccak256(IBCCommitment.clientStatePath(clientId))] = clientStateCommitment;
+        commitments[IBCCommitment.clientStateCommitmentKey(clientId)] = clientStateCommitment;
         commitments[IBCCommitment.consensusStateCommitmentKey(
             clientId, update.height.revision_number, update.height.revision_height
         )] = update.consensusStateCommitment;
@@ -55,7 +56,7 @@ contract IBCClient is IBCStore, IIBCClient {
 
         // update commitments
         if (clientStateCommitment != 0) {
-            commitments[keccak256(IBCCommitment.clientStatePath(msg_.clientId))] = clientStateCommitment;
+            commitments[IBCCommitment.clientStateCommitmentKey(msg_.clientId)] = clientStateCommitment;
         }
         for (uint256 i = 0; i < updates.length; i++) {
             commitments[IBCCommitment.consensusStateCommitmentKey(
@@ -64,9 +65,44 @@ contract IBCClient is IBCStore, IIBCClient {
         }
     }
 
+    /**
+     * @dev generateClientIdentifier generates a new client identifier for a given client type
+     */
     function generateClientIdentifier(string calldata clientType) private returns (string memory) {
         string memory identifier = string(abi.encodePacked(clientType, "-", Strings.toString(nextClientSequence)));
         nextClientSequence++;
         return identifier;
     }
+
+    /**
+     * @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]$`
+     */
+    function validateClientType(bytes memory clientTypeBytes) internal pure returns (bool) {
+        if (clientTypeBytes.length == 0) {
+            return false;
+        }
+        unchecked {
+            for (uint256 i = 0; i < clientTypeBytes.length; 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
+                    if (i == 0 || i == clientTypeBytes.length - 1) {
+                        return false;
+                    }
+                    continue;
+                } else if (0x30 <= c && c <= 0x39) {
+                    // 0-9
+                    continue;
+                } else {
+                    return false;
+                }
+            }
+        }
+        return true;
+    }
 }

contracts/core/05-port/ModuleManager.sol

@@ -12,7 +12,8 @@ abstract contract ModuleManager {
      * @dev bindPort binds to an unallocated port, failing if the port has already been allocated.
      */
     function bindPort(string calldata portId, address moduleAddress) public virtual {
-        require(moduleAddress != address(this) && Address.isContract(moduleAddress), "invalid module address");
+        require(validatePortIdentifier(bytes(portId)), "invalid portId");
+        require(moduleAddress != address(this) && Address.isContract(moduleAddress), "invalid moduleAddress");
         claimCapability(portCapabilityPath(portId), moduleAddress);
     }
 
@@ -69,4 +70,37 @@ abstract contract ModuleManager {
      * Currently, the function returns only one module.
      */
     function lookupModules(bytes memory name) internal view virtual returns (address[] storage, bool);
+
+    /**
+     * @dev validatePortIdentifier validates a port identifier string
+     *     check if the string consist of characters in one of the following categories only:
+     *     - Alphanumeric
+     *     - `.`, `_`, `+`, `-`, `#`
+     *     - `[`, `]`, `<`, `>`
+     */
+    function validatePortIdentifier(bytes memory portId) internal pure returns (bool) {
+        if (portId.length < 2 || portId.length > 128) {
+            return false;
+        }
+        unchecked {
+            for (uint256 i = 0; i < portId.length; i++) {
+                uint256 c = uint256(uint8(portId[i]));
+                if (
+                    // a-z
+                    (c >= 0x61 && c <= 0x7A)
+                    // 0-9
+                    || (c >= 0x30 && c <= 0x39)
+                    // A-Z
+                    || (c >= 0x41 && c <= 0x5A)
+                    // ".", "_", "+", "-"
+                    || (c == 0x2E || c == 0x5F || c == 0x2B || c == 0x2D)
+                    // "#", "[", "]", "<", ">"
+                    || (c == 0x23 || c == 0x5B || c == 0x5D || c == 0x3C || c == 0x3E)
+                ) {
+                    continue;
+                }
+            }
+        }
+        return true;
+    }
 }

tests/foundry/src/IBC.t.sol

@@ -12,16 +12,16 @@ import "../../../contracts/clients/MockClient.sol";
 import "../../../contracts/proto/MockClient.sol";
 import "../../../contracts/proto/Connection.sol";
 import "../../../contracts/proto/Channel.sol";
+import "../../../contracts/apps/mock/IBCMockApp.sol";
 import "./TestableIBCHandler.t.sol";
-import "./MockApp.t.sol";
 
 // TODO split setup code into other contracts
 contract IBCTest is Test {
     using IBCHeight for Height.Data;
 
     TestableIBCHandler handler;
     MockClient mockClient;
-    MockApp mockApp;
+    IBCMockApp mockApp;
 
     string private constant MOCK_CLIENT_TYPE = "mock-client";
     string private constant MOCK_PORT_ID = "mock";
@@ -83,7 +83,7 @@ contract IBCTest is Test {
     }
 
     function setUpMockApp() internal {
-        mockApp = new MockApp(address(handler));
+        mockApp = new IBCMockApp(handler);
         handler.bindPort(MOCK_PORT_ID, address(mockApp));
         handler.claimCapabilityDirectly(handler.channelCapabilityPath(MOCK_PORT_ID, "channel-0"), address(mockApp));
         handler.claimCapabilityDirectly(handler.channelCapabilityPath(MOCK_PORT_ID, "channel-0"), address(this));
@@ -136,12 +136,8 @@ contract IBCTest is Test {
         );
     }
 
-    event MockRecv(bool ok);
-
     function testBenchmarkRecvPacket() public {
         Packet.Data memory packet = createPacket(0, 100);
-        vm.expectEmit(false, false, false, true);
-        emit MockRecv(true);
         handler.recvPacket(
             IBCMsgs.MsgPacketRecv({
                 packet: packet,