diff --git a/.gas-snapshot b/.gas-snapshot index 7e400c3c..0c68f54a 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -1,5 +1,5 @@ IBCMockAppTest:testHandshake() (gas: 4420576) -IBCMockAppTest:testHandshakeBetweenDifferentPorts() (gas: 3339589) +IBCMockAppTest:testHandshakeBetweenDifferentPorts() (gas: 3339603) IBCMockAppTest:testPacketRelay() (gas: 13935831) IBCMockAppTest:testPacketTimeout() (gas: 4284259) IBCTest:testBenchmarkCreateMockClient() (gas: 233366) @@ -8,13 +8,14 @@ IBCTest:testBenchmarkRecvPacket() (gas: 158888) IBCTest:testBenchmarkSendPacket() (gas: 128432) IBCTest:testBenchmarkUpdateMockClient() (gas: 160229) IBCTest:testToUint128((uint64,uint64)) (runs: 256, μ: 947, ~: 947) -TestICS02:testCreateClient() (gas: 36633805) -TestICS02:testInvalidCreateClient() (gas: 36530922) -TestICS02:testInvalidUpdateClient() (gas: 36529932) -TestICS02:testRegisterClient() (gas: 36185598) -TestICS02:testRegisterClientDuplicatedClientType() (gas: 36170907) -TestICS02:testRegisterClientInvalidClientType() (gas: 36200368) -TestICS02:testUpdateClient() (gas: 36698132) +ICS24HostTest:testValidatePortIdentifier() (gas: 37060) +TestICS02:testCreateClient() (gas: 36639835) +TestICS02:testInvalidCreateClient() (gas: 36536946) +TestICS02:testInvalidUpdateClient() (gas: 36535962) +TestICS02:testRegisterClient() (gas: 36191628) +TestICS02:testRegisterClientDuplicatedClientType() (gas: 36176937) +TestICS02:testRegisterClientInvalidClientType() (gas: 36206398) +TestICS02:testUpdateClient() (gas: 36704162) TestICS03Handshake:testConnOpenAck() (gas: 1858230) TestICS03Handshake:testConnOpenConfirm() (gas: 2054143) TestICS03Handshake:testConnOpenInit() (gas: 1429838) @@ -29,7 +30,7 @@ TestICS03Version:testIsSupportedVersion() (gas: 7864) TestICS03Version:testPickVersion() (gas: 25327) TestICS03Version:testVerifyProposedVersion() (gas: 11777) TestICS03Version:testVerifySupportedFeature() (gas: 4153) -TestICS04Handshake:testBindPort() (gas: 458549) +TestICS04Handshake:testBindPort() (gas: 458623) TestICS04Handshake:testChanClose() (gas: 12973942) TestICS04Handshake:testChanOpenAck() (gas: 3459492) TestICS04Handshake:testChanOpenConfirm() (gas: 3770761) @@ -54,7 +55,7 @@ TestICS04Upgrade:testUpgradeCrossingHelloIncompatibleProposals() (gas: 5068281) TestICS04Upgrade:testUpgradeFull() (gas: 56559261) TestICS04Upgrade:testUpgradeInit() (gas: 3074224) TestICS04Upgrade:testUpgradeNoChanges() (gas: 2473090) -TestICS04Upgrade:testUpgradeNotUpgradableModule() (gas: 3648610) +TestICS04Upgrade:testUpgradeNotUpgradableModule() (gas: 3648638) TestICS04Upgrade:testUpgradeOutOfSync() (gas: 3905908) TestICS04Upgrade:testUpgradeRelaySuccessAtCounterpartyFlushComplete() (gas: 5230956) TestICS04Upgrade:testUpgradeRelaySuccessAtFlushing() (gas: 5604165) diff --git a/contracts/core/24-host/IBCHostConfigurator.sol b/contracts/core/24-host/IBCHostConfigurator.sol index ab98a133..2984e491 100644 --- a/contracts/core/24-host/IBCHostConfigurator.sol +++ b/contracts/core/24-host/IBCHostConfigurator.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.20; import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; import {IBCClientLib} from "../02-client/IBCClientLib.sol"; import {ILightClient} from "../02-client/ILightClient.sol"; +import {IBCHostLib} from "./IBCHostLib.sol"; import {IBCModuleManager} from "../26-router/IBCModuleManager.sol"; import {IIBCModule} from "../26-router/IIBCModule.sol"; import {IIBCHostConfigurator} from "./IIBCHostConfigurator.sol"; @@ -30,7 +31,7 @@ abstract contract IBCHostConfigurator is IIBCHostConfigurator, IBCModuleManager function _bindPort(string calldata portId, IIBCModule module) internal virtual { address moduleAddress = address(module); - if (!validatePortIdentifier(bytes(portId))) { + if (!IBCHostLib.validatePortIdentifier(bytes(portId))) { revert IBCHostInvalidPortIdentifier(portId); } if (moduleAddress == address(0) || moduleAddress == address(this)) { diff --git a/contracts/core/24-host/IBCHostLib.sol b/contracts/core/24-host/IBCHostLib.sol new file mode 100644 index 00000000..42dfe66d --- /dev/null +++ b/contracts/core/24-host/IBCHostLib.sol @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.20; + +library IBCHostLib { + /** + * @dev validatePortIdentifier validates a port identifier string + * check if the string consist of characters in one of the following categories only: + * - Alphanumeric + * - `.`, `_`, `+`, `-`, `#` + * - `[`, `]`, `<`, `>` + * + * The validation is based on the ibc-go implementation: + * https://github.com/cosmos/ibc-go/blob/b0ed0b412ea75e66091cc02746c95f9e6cf4445d/modules/core/24-host/validate.go#L86 + */ + function validatePortIdentifier(bytes memory portId) internal pure returns (bool) { + uint256 portIdLength = portId.length; + if (portIdLength < 2 || portIdLength > 128) { + return false; + } + unchecked { + for (uint256 i = 0; i < portIdLength; 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) + ) { + return false; + } + } + } + return true; + } +} diff --git a/contracts/core/26-router/IBCModuleManager.sol b/contracts/core/26-router/IBCModuleManager.sol index e3ff2ea2..9de4ead2 100644 --- a/contracts/core/26-router/IBCModuleManager.sol +++ b/contracts/core/26-router/IBCModuleManager.sol @@ -112,37 +112,4 @@ contract IBCModuleManager is IBCHost, Context { portId, channelId, upgradeSequence, _msgSender() ); } - - /** - * @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; - } } diff --git a/tests/foundry/src/ICS24Host.t.sol b/tests/foundry/src/ICS24Host.t.sol new file mode 100644 index 00000000..9ad30646 --- /dev/null +++ b/tests/foundry/src/ICS24Host.t.sol @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity ^0.8.20; + +import "./helpers/IBCTestHelper.t.sol"; +import {IBCHostLib} from "../../../contracts/core/24-host/IBCHostLib.sol"; + +contract ICS24HostTest is IBCTestHelper { + struct ValidatePortIdentifierTestCase { + string m; + string id; + bool expPass; + } + + function testValidatePortIdentifier() public { + // The following test cases are based on the test cases of ibc-go: + // https://github.com/cosmos/ibc-go/blob/e443a88e0f2c84c131c5a1de47945a5733ff9c91/modules/core/24-host/validate_test.go#L57 + ValidatePortIdentifierTestCase[] memory testCases = new ValidatePortIdentifierTestCase[](12); + testCases[0] = ValidatePortIdentifierTestCase({ + m: "valid lowercase", + id: "transfer", + expPass: true + }); + testCases[1] = ValidatePortIdentifierTestCase({ + m: "valid id special chars", + id: "._+-#[]<>._+-#[]<>", + expPass: true + }); + testCases[2] = ValidatePortIdentifierTestCase({ + m: "valid id lower and special chars", + id: "lower._+-#[]<>", + expPass: true + }); + testCases[3] = ValidatePortIdentifierTestCase({ + m: "numeric id", + id: "1234567890", + expPass: true + }); + testCases[4] = ValidatePortIdentifierTestCase({ + m: "uppercase id", + id: "NOTLOWERCASE", + expPass: true + }); + testCases[5] = ValidatePortIdentifierTestCase({ + m: "numeric id", + id: "1234567890", + expPass: true + }); + testCases[6] = ValidatePortIdentifierTestCase({ + m: "blank id", + id: " ", + expPass: false + }); + testCases[7] = ValidatePortIdentifierTestCase({ + m: "id length out of range", + id: "1", + expPass: false + }); + testCases[8] = ValidatePortIdentifierTestCase({ + m: "id is too long", + id: "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Duis eros neque, ultricies vel ligula ac, convallis porttitor elit. Maecenas tincidunt turpis elit, vel faucibus nisl pellentesque sodales", + expPass: false + }); + testCases[9] = ValidatePortIdentifierTestCase({ + m: "path-like id", + id: "lower/case/id", + expPass: false + }); + testCases[10] = ValidatePortIdentifierTestCase({ + m: "invalid id", + id: "(clientid)", + expPass: false + }); + testCases[11] = ValidatePortIdentifierTestCase({ + m: "empty string", + id: "", + expPass: false + }); + + for (uint i = 0; i < testCases.length; i++) { + ValidatePortIdentifierTestCase memory tc = testCases[i]; + bool res = IBCHostLib.validatePortIdentifier(bytes(tc.id)); + if (tc.expPass) { + require(res, tc.m); + } else { + require(!res, tc.m); + } + } + } +}