Merge pull request #306 from hyperledger-labs/audit-202409

Audit-202409

Signed-off-by: Jun Kimura <[email protected]>

Makefile

@@ -1,7 +1,7 @@
 FOUNDRY_PROFILE=default
 FORGE=FOUNDRY_PROFILE=$(FOUNDRY_PROFILE) forge
 SOLC_VERSION=0.8.24
-EVM_VERSION=paris
+EVM_VERSION=cancun
 DOCKER=docker
 ABIGEN="$(DOCKER) run -v .:/workspace -w /workspace -it ethereum/client-go:alltools-v1.11.6 abigen"
 SOLHINT=npx solhint
@@ -16,7 +16,7 @@ TEST_UPGRADEABLE=false
 
 .PHONY: build
 build:
-	$(FORGE) build --sizes --skip test --use solc:$(SOLC_VERSION)
+	$(FORGE) build --sizes --skip test --use solc:$(SOLC_VERSION) --evm-version $(EVM_VERSION)
 
 .PHONY: clean
 clean:
@@ -33,7 +33,7 @@ lint:
 
 .PHONY: test
 test:
-	TEST_UPGRADEABLE=$(TEST_UPGRADEABLE) $(FORGE) test -vvvv --gas-report --isolate --use solc:$(SOLC_VERSION)
+	TEST_UPGRADEABLE=$(TEST_UPGRADEABLE) $(FORGE) test -vvvv --gas-report --isolate --use solc:$(SOLC_VERSION) --evm-version $(EVM_VERSION)
 
 .PHONY: snapshot
 snapshot:

chains/ibft2/chain0/ibftConfigFile.json

@@ -2,7 +2,8 @@
     "genesis": {
       "config": {
          "chainId": 2018,
-         "muirglacierblock": 0,
+         "cancunTime": 0,
+         "zeroBaseFee": true,
          "ibft2": {
            "blockperiodseconds": 1,
            "epochlength": 30000,

chains/ibft2/chain1/ibftConfigFile.json

@@ -2,7 +2,8 @@
     "genesis": {
       "config": {
          "chainId": 3018,
-         "muirglacierblock": 0,
+         "cancunTime": 0,
+         "zeroBaseFee": true,
          "ibft2": {
            "blockperiodseconds": 1,
            "epochlength": 30000,

chains/qbft/chain0/Dockerfile

@@ -1,4 +1,4 @@
-FROM hyperledger/besu:24.3.0
+FROM hyperledger/besu:24.10.0
 
 USER root
 

chains/qbft/chain0/qbftConfigFile.json

@@ -2,7 +2,8 @@
   "genesis": {
     "config": {
       "chainId": 2018,
-      "muirglacierblock": 0,
+      "cancunTime": 0,
+      "zeroBaseFee": true,
       "qbft": {
         "blockperiodseconds": 1,
         "epochlength": 30000,

chains/qbft/chain1/Dockerfile

@@ -1,4 +1,4 @@
-FROM hyperledger/besu:24.3.0
+FROM hyperledger/besu:24.10.0
 
 USER root
 

chains/qbft/chain1/qbftConfigFile.json

@@ -2,7 +2,8 @@
   "genesis": {
     "config": {
       "chainId": 3018,
-      "muirglacierblock": 0,
+      "cancunTime": 0,
+      "zeroBaseFee": true,
       "qbft": {
         "blockperiodseconds": 1,
         "epochlength": 30000,

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";
 
 /**
@@ -41,6 +42,8 @@ contract IBCClient is IBCHost, IIBCClient, IIBCClientErrors {
      */
     function updateClient(MsgUpdateClient calldata msg_) external override {
         (address lc, bytes4 selector, bytes memory args) = routeUpdateClient(msg_);
+        // NOTE: We assume that the client contract was correctly validated by the authority at registration via `registerClient` function.
+        //       For details, see the `registerClient` function in the IBCHostConfigurator.
         (bool success, bytes memory returndata) = lc.call(abi.encodePacked(selector, args));
         if (!success) {
             if (returndata.length > 0) {
@@ -61,6 +64,9 @@ contract IBCClient is IBCHost, IIBCClient, IIBCClientErrors {
     /**
      * @dev routeUpdateClient returns the LC contract address and the calldata to the receiving function of the client message.
      *      Light client contract may encode a client message as other encoding scheme(e.g. ethereum ABI)
+     *      WARNING: If the caller is an EOA like a relayer, the caller must validate the return values with the allow list of the contract functions before calling the LC contract with the data.
+     *               This validation is always required because even if the caller trusts the IBC contract, a malicious RPC provider can return arbitrary data to the caller.
+     *      Check ADR-001 for details.
      */
     function routeUpdateClient(MsgUpdateClient calldata msg_)
         public
@@ -69,6 +75,7 @@ contract IBCClient is IBCHost, IIBCClient, IIBCClientErrors {
         returns (address, bytes4, bytes memory)
     {
         ILightClient lc = checkAndGetClient(msg_.clientId);
+        // NOTE: The `lc.routeUpdateClient` function must be validated by the authority at registration via `registerClient` function.
         (bytes4 functionId, bytes memory args) = lc.routeUpdateClient(msg_.clientId, msg_.protoClientMessage);
         return (address(lc), functionId, args);
     }
@@ -95,10 +102,17 @@ contract IBCClient is IBCHost, IIBCClient, IIBCClientErrors {
             bytes32 key = IBCCommitment.consensusStateCommitmentKey(
                 clientId, heights[i].revision_number, heights[i].revision_height
             );
-            if (commitments[key] != bytes32(0)) {
-                continue;
+            bytes32 commitment = keccak256(consensusState);
+            bytes32 prev = commitments[key];
+            if (prev != bytes32(0) && commitment != prev) {
+                // Revert if the new commitment is inconsistent with the previous one.
+                // This case may indicate misbehavior of either the LightClient or the target chain.
+                // Since the definition and specification of misbehavior are defined for each LightClient,
+                // if a relayer detects this error, it is recommended to submit an evidence of misbehaviour to the LightClient accordingly.
+                // (e.g., via the updateClient function).
+                revert IBCClientInconsistentConsensusStateCommitment(key, commitment, prev);
             }
-            commitments[key] = keccak256(consensusState);
+            commitments[key] = commitment;
         }
     }
 
@@ -109,6 +123,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;
     }

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;
+    }
 }

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);
 
@@ -20,4 +23,9 @@ interface IIBCClientErrors {
     /// @param selector the function selector
     /// @param args the calldata
     error IBCClientFailedUpdateClient(bytes4 selector, bytes args);
+
+    /// @param commitmentKey the commitment key
+    /// @param commitment the commitment
+    /// @param prev the previous commitment
+    error IBCClientInconsistentConsensusStateCommitment(bytes32 commitmentKey, bytes32 commitment, bytes32 prev);
 }

contracts/core/03-connection/IBCConnection.sol

@@ -43,6 +43,10 @@ abstract contract IBCConnection is IBCHost, IBCSelfStateValidator, IIBCConnectio
             }
             connection.versions.push(msg_.version);
         } else {
+            // if the version features are empty, the version identifier should also be empty
+            if (bytes(msg_.version.identifier).length > 0) {
+                revert IBCConnectionVersionIdentifierNotEmpty();
+            }
             IBCConnectionLib.setSupportedVersions(getCompatibleVersions(), connection.versions);
         }
 

contracts/core/03-connection/IIBCConnection.sol

@@ -7,40 +7,80 @@ import {Version, Counterparty} from "../../proto/Connection.sol";
 interface IIBCConnection {
     // --------------------- Data Structure --------------------- //
 
+    /**
+     * @dev MsgConnectionOpenInit defines the msg sent by an account on Chain A to initialize a connection with Chain B.
+     * @param clientId of Chain B
+     * @param counterparty is composed of clientID of Chain A on Chain B and the prefix of Chain B
+     * @param version chosen by an account on Chain A
+     * @param delayPeriod is set to the connection delay time period
+     */
     struct MsgConnectionOpenInit {
         string clientId;
         Counterparty.Data counterparty;
         Version.Data version;
         uint64 delayPeriod;
     }
 
+    /**
+     * @dev MsgConnectionOpenTry defines a msg sent by an account on Chain B to try to open a connection on Chain B.
+     * @param counterparty is composed of clientID of Chain A on Chain B and the prefix of Chain B
+     * @param delayPeriod chosen by Chain A in `connectionOpenInit`
+     * @param clientId of Chain A
+     * @param clientStateBytes clientState that Chain A has for Chain B
+     * @param counterpartyVersions supported versions of Chain A
+     * @param proofInit proof that Chain A stored connectionEnd in state (on connectionOpenInit)
+     * @param proofClient proof that Chain A stored a light client of Chain B
+     * @param proofConsensus proof that Chain A stored Chain B's consensus state at consensus height
+     * @param proofHeight height at which relayer constructs proof of A storing connectionEnd in state
+     * @param consensusHeight latest height of Chain B which Chain A has stored in its Chain B client
+     * @param hostConsensusStateProof proof data for the consensus state of Chain B
+     */
     struct MsgConnectionOpenTry {
-        Counterparty.Data counterparty; // counterpartyConnectionIdentifier, counterpartyPrefix and counterpartyClientIdentifier
+        Counterparty.Data counterparty;
         uint64 delayPeriod;
-        string clientId; // clientID of chainA
-        bytes clientStateBytes; // clientState that chainA has for chainB
-        Version.Data[] counterpartyVersions; // supported versions of chain A
-        bytes proofInit; // proof that chainA stored connectionEnd in state (on ConnOpenInit)
-        bytes proofClient; // proof that chainA stored a light client of chainB
-        bytes proofConsensus; // proof that chainA stored chainB's consensus state at consensus height
-        Height.Data proofHeight; // height at which relayer constructs proof of A storing connectionEnd in state
-        Height.Data consensusHeight; // latest height of chain B which chain A has stored in its chain B client
-        bytes hostConsensusStateProof; // optional proof data for host state machines that are unable to introspect their own consensus state
+        string clientId;
+        bytes clientStateBytes;
+        Version.Data[] counterpartyVersions;
+        bytes proofInit;
+        bytes proofClient;
+        bytes proofConsensus;
+        Height.Data proofHeight;
+        Height.Data consensusHeight;
+        bytes hostConsensusStateProof;
     }
 
+    /**
+     * @dev MsgConnectionOpenAck defines a msg sent by an account on Chain A to acknowledge the change of connection state to TRYOPEN on Chain B.
+     * @param connectionId identifier of the connection generated at `connectionOpenInit`
+     * @param clientStateBytes clientState that Chain B has for Chain A
+     * @param version chosen by Chain B in `connectionOpenTry`
+     * @param counterpartyConnectionId identifier of the connection on Chain B
+     * @param proofTry proof that Chain B stored connectionEnd in state
+     * @param proofClient proof that Chain B stored a light client of Chain A
+     * @param proofConsensus proof that Chain B stored Chain A's consensus state at consensus height
+     * @param proofHeight height at which relayer constructed proof of B storing connectionEnd in state
+     * @param consensusHeight latest height of Chain A which Chain B has stored in its Chain A client
+     * @param hostConsensusStateProof proof data for the consensus state of Chain A
+     */
     struct MsgConnectionOpenAck {
         string connectionId;
-        bytes clientStateBytes; // client state for chainA on chainB
-        Version.Data version; // version that ChainB chose in ConnOpenTry
+        bytes clientStateBytes;
+        Version.Data version;
         string counterpartyConnectionId;
-        bytes proofTry; // proof that connectionEnd was added to ChainB state in ConnOpenTry
-        bytes proofClient; // proof of client state on chainB for chainA
-        bytes proofConsensus; // proof that chainB has stored ConsensusState of chainA on its client
-        Height.Data proofHeight; // height that relayer constructed proofTry
-        Height.Data consensusHeight; // latest height of chainA that chainB has stored on its chainA client
-        bytes hostConsensusStateProof; // optional proof data for host state machines that are unable to introspect their own consensus state
+        bytes proofTry;
+        bytes proofClient;
+        bytes proofConsensus;
+        Height.Data proofHeight;
+        Height.Data consensusHeight;
+        bytes hostConsensusStateProof;
     }
 
+    /**
+     * @dev MsgConnectionOpenConfirm defines a msg sent by an account on Chain B to acknowledge the change of connection state to OPEN on Chain A.
+     * @param connectionId identifier of the connection generated at `connectionOpenTry`
+     * @param proofAck proof that Chain A stored connectionEnd in state
+     * @param proofHeight height at which relayer constructed proof of A storing connectionEnd in state
+     */
     struct MsgConnectionOpenConfirm {
         string connectionId;
         bytes proofAck;
@@ -54,26 +94,26 @@ interface IIBCConnection {
     event GeneratedConnectionIdentifier(string connectionId);
 
     /**
-     * @dev connectionOpenInit initialises a connection attempt on chain A. The generated connection identifier
+     * @dev connectionOpenInit initialises a connection attempt on Chain A. The generated connection identifier
      * is returned.
      */
     function connectionOpenInit(MsgConnectionOpenInit calldata msg_) external returns (string memory connectionId);
 
     /**
-     * @dev connectionOpenTry relays notice of a connection attempt on chain A to chain B (this
-     * code is executed on chain B).
+     * @dev connectionOpenTry relays notice of a connection attempt on Chain A to Chain B (this
+     * code is executed on Chain B).
      */
     function connectionOpenTry(MsgConnectionOpenTry calldata msg_) external returns (string memory);
 
     /**
-     * @dev connectionOpenAck relays acceptance of a connection open attempt from chain B back
-     * to chain A (this code is executed on chain A).
+     * @dev connectionOpenAck relays acceptance of a connection open attempt from Chain B back
+     * to Chain A (this code is executed on Chain A).
      */
     function connectionOpenAck(MsgConnectionOpenAck calldata msg_) external;
 
     /**
-     * @dev connectionOpenConfirm confirms opening of a connection on chain A to chain B, after
-     * which the connection is open on both chains (this code is executed on chain B).
+     * @dev connectionOpenConfirm confirms opening of a connection on Chain A to Chain B, after
+     * which the connection is open on both chains (this code is executed on Chain B).
      */
     function connectionOpenConfirm(MsgConnectionOpenConfirm calldata msg_) external;
 }

contracts/core/03-connection/IIBCConnectionErrors.sol

@@ -18,6 +18,8 @@ interface IIBCConnectionErrors {
 
     error IBCConnectionIBCVersionNotSupported();
 
+    error IBCConnectionVersionIdentifierNotEmpty();
+
     error IBCConnectionInvalidSelfClientState();
 
     error IBCConnectionInvalidHostConsensusStateProof();