Skip to content

Commit

Permalink
Merge pull request #288 from hyperledger-labs/fix-port-identifier-val…
Browse files Browse the repository at this point in the history
…idation

Fix port identifier validation and add tests

Signed-off-by: Jun Kimura <[email protected]>
  • Loading branch information
bluele authored Jul 23, 2024
2 parents 49d88ae + 18c7639 commit 138814b
Show file tree
Hide file tree
Showing 5 changed files with 143 additions and 44 deletions.
21 changes: 11 additions & 10 deletions .gas-snapshot
Original file line number Diff line number Diff line change
@@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion contracts/core/24-host/IBCHostConfigurator.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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)) {
Expand Down
41 changes: 41 additions & 0 deletions contracts/core/24-host/IBCHostLib.sol
Original file line number Diff line number Diff line change
@@ -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;
}
}
33 changes: 0 additions & 33 deletions contracts/core/26-router/IBCModuleManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
89 changes: 89 additions & 0 deletions tests/foundry/src/ICS24Host.t.sol
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
}

0 comments on commit 138814b

Please sign in to comment.