Merge pull request #248 from hyperledger-labs/fix-mock-client-verific…

…ation Make mock-client verification more strict Signed-off-by: Jun Kimura <[email protected]>
hyperledger-labs · Feb 11, 2024 · 84529dc · 84529dc
2 parents 215f3d5 + fec9c2c
commit 84529dc
Show file tree

Hide file tree

Showing 7 changed files with 102 additions and 106 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
@@ -1,43 +1,43 @@
 IBCTest:testBenchmarkCreateMockClient() (gas: 209319)
-IBCTest:testBenchmarkLCUpdateMockClient() (gas: 39869)
-IBCTest:testBenchmarkRecvPacket() (gas: 135053)
-IBCTest:testBenchmarkSendPacket() (gas: 85109)
-IBCTest:testBenchmarkUpdateMockClient() (gas: 137282)
+IBCTest:testBenchmarkLCUpdateMockClient() (gas: 39935)
+IBCTest:testBenchmarkRecvPacket() (gas: 136859)
+IBCTest:testBenchmarkSendPacket() (gas: 85154)
+IBCTest:testBenchmarkUpdateMockClient() (gas: 137315)
 IBCTest:testToUint128((uint64,uint64)) (runs: 256, μ: 947, ~: 947)
-TestICS02:testCreateClient() (gas: 24183597)
-TestICS02:testInvalidCreateClient() (gas: 24031459)
-TestICS02:testInvalidUpdateClient() (gas: 24033758)
-TestICS02:testRegisterClient() (gas: 23659268)
-TestICS02:testRegisterClientDuplicatedClientType() (gas: 23641318)
-TestICS02:testRegisterClientInvalidClientType() (gas: 23626190)
-TestICS02:testUpdateClient() (gas: 24199983)
-TestICS03Handshake:testConnOpenAck() (gas: 1631230)
-TestICS03Handshake:testConnOpenConfirm() (gas: 1764283)
-TestICS03Handshake:testConnOpenInit() (gas: 1279681)
-TestICS03Handshake:testConnOpenTry() (gas: 2124662)
-TestICS03Handshake:testInvalidConnOpenAck() (gas: 2001420)
-TestICS03Handshake:testInvalidConnOpenConfirm() (gas: 2007127)
-TestICS03Handshake:testInvalidConnOpenInit() (gas: 666499)
-TestICS03Handshake:testInvalidConnOpenTry() (gas: 2110519)
+TestICS02:testCreateClient() (gas: 24329382)
+TestICS02:testInvalidCreateClient() (gas: 24177244)
+TestICS02:testInvalidUpdateClient() (gas: 24179521)
+TestICS02:testRegisterClient() (gas: 23924001)
+TestICS02:testRegisterClientDuplicatedClientType() (gas: 23906051)
+TestICS02:testRegisterClientInvalidClientType() (gas: 23890923)
+TestICS02:testUpdateClient() (gas: 24345834)
+TestICS03Handshake:testConnOpenAck() (gas: 1631428)
+TestICS03Handshake:testConnOpenConfirm() (gas: 1764503)
+TestICS03Handshake:testConnOpenInit() (gas: 1279747)
+TestICS03Handshake:testConnOpenTry() (gas: 2124860)
+TestICS03Handshake:testInvalidConnOpenAck() (gas: 2001640)
+TestICS03Handshake:testInvalidConnOpenConfirm() (gas: 2007413)
+TestICS03Handshake:testInvalidConnOpenInit() (gas: 666565)
+TestICS03Handshake:testInvalidConnOpenTry() (gas: 2110651)
 TestICS03Version:testCopyVersions() (gas: 558658)
 TestICS03Version:testFindSupportedVersion() (gas: 19400)
 TestICS03Version:testIsSupportedVersion() (gas: 7864)
 TestICS03Version:testPickVersion() (gas: 25399)
 TestICS03Version:testVerifyProposedVersion() (gas: 11777)
 TestICS03Version:testVerifySupportedFeature() (gas: 4153)
 TestICS04Handshake:testBindPort() (gas: 40333)
-TestICS04Handshake:testChanOpenAck() (gas: 2881101)
-TestICS04Handshake:testChanOpenConfirm() (gas: 3060863)
-TestICS04Handshake:testChanOpenInit() (gas: 2197742)
-TestICS04Handshake:testChanOpenTry() (gas: 2655131)
-TestICS04Handshake:testInvalidChanOpenAck() (gas: 2058888)
-TestICS04Handshake:testInvalidChanOpenConfirm() (gas: 2116977)
-TestICS04Handshake:testInvalidChanOpenInit() (gas: 1273616)
-TestICS04Handshake:testInvalidChanOpenTry() (gas: 1347198)
-TestICS04Packet:testInvalidSendPacket() (gas: 2204391)
-TestICS04Packet:testSendPacket() (gas: 2263269)
-TestICS20:testAddressToHex(address) (runs: 256, μ: 22673, ~: 22824)
+TestICS04Handshake:testChanOpenAck() (gas: 2881255)
+TestICS04Handshake:testChanOpenConfirm() (gas: 3061061)
+TestICS04Handshake:testChanOpenInit() (gas: 2197808)
+TestICS04Handshake:testChanOpenTry() (gas: 2655241)
+TestICS04Handshake:testInvalidChanOpenAck() (gas: 2059020)
+TestICS04Handshake:testInvalidChanOpenConfirm() (gas: 2117109)
+TestICS04Handshake:testInvalidChanOpenInit() (gas: 1273682)
+TestICS04Handshake:testInvalidChanOpenTry() (gas: 1347330)
+TestICS04Packet:testInvalidSendPacket() (gas: 2207885)
+TestICS04Packet:testSendPacket() (gas: 2264031)
+TestICS20:testAddressToHex(address) (runs: 256, μ: 22687, ~: 22824)
 TestICS20:testHexToAddress(string) (runs: 256, μ: 4776, ~: 4734)
 TestICS20:testIsEscapedString() (gas: 48979)
 TestICS20:testMarshaling() (gas: 148145)
-TestICS20:testParseAmount(uint256) (runs: 256, μ: 26683, ~: 21388)
+TestICS20:testParseAmount(uint256) (runs: 256, μ: 27096, ~: 22172)
diff --git a/contracts/clients/MockClient.sol b/contracts/clients/MockClient.sol
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0
 pragma solidity ^0.8.12;
 
+import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
 import {ILightClient} from "../core/02-client/ILightClient.sol";
 import {IBCHeight} from "../core/02-client/IBCHeight.sol";
 import {IIBCHandler} from "../core/25-handler/IIBCHandler.sol";
@@ -14,7 +15,7 @@ import {GoogleProtobufAny as Any} from "../proto/GoogleProtobufAny.sol";
 
 // MockClient implements https://github.com/datachainlab/ibc-mock-client
 // WARNING: This client is intended to be used for testing purpose. Therefore, it is not generally available in a production, except in a fully trusted environment.
-contract MockClient is ILightClient {
+contract MockClient is Ownable, ILightClient {
     using IBCHeight for Height.Data;
 
     string private constant HEADER_TYPE_URL = "/ibc.lightclients.mock.v1.Header";
@@ -109,6 +110,16 @@ contract MockClient is ILightClient {
         return statuses[clientId];
     }
 
+    /**
+     * @dev setStatus sets the status of the client corresponding to `clientId`.
+     */
+    function setStatus(string calldata clientId, ClientStatus status) external virtual onlyOwner {
+        statuses[clientId] = status;
+    }
+
+    /**
+     * @dev updateClient updates the client state and returns the updated heights.
+     */
     function updateClient(string calldata clientId, Header.Data calldata header)
         public
         returns (Height.Data[] memory heights)
@@ -137,13 +148,14 @@ contract MockClient is ILightClient {
         uint64,
         bytes calldata proof,
         bytes calldata prefix,
-        bytes memory,
+        bytes memory path,
         bytes calldata value
     ) external view virtual override returns (bool) {
         require(consensusStates[clientId][height.toUint128()].timestamp != 0, "consensus state not found");
-        require(proof.length == 32, "invalid proof length");
         require(keccak256(IIBCHandler(ibcHandler).getCommitmentPrefix()) == keccak256(prefix), "invalid prefix");
-        return sha256(value) == bytes32(proof);
+        require(proof.length == 32, "invalid proof length");
+        return
+            sha256(abi.encodePacked(height.toUint128(), sha256(prefix), sha256(path), sha256(value))) == bytes32(proof);
     }
 
     /**

diff --git a/pkg/testing/chains.go b/pkg/testing/chains.go
@@ -14,6 +14,7 @@ import (
 	"testing"
 	"time"
 
+	clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
 	host "github.com/cosmos/ibc-go/v7/modules/core/24-host"
 	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/accounts/abi"
@@ -490,7 +491,7 @@ func (chain *Chain) ConnectionOpenTry(ctx context.Context, counterparty *Chain,
 	if err != nil {
 		return "", err
 	}
-	consensusStateBytes, proofConsensus, err := counterparty.QueryConsensusStateProof(chain, connection.ClientID, latestHeight, proofConnection.Height.ToBN())
+	consensusStateBytes, proofConsensus, err := counterparty.QueryConsensusStateProof(chain, connection.ClientID, counterpartyConnection.ClientID, latestHeight, proofConnection.Height.ToBN())
 	if err != nil {
 		return "", err
 	}
@@ -537,7 +538,7 @@ func (chain *Chain) ConnectionOpenAck(
 	if err != nil {
 		return err
 	}
-	consensusStateBytes, proofConsensus, err := counterparty.QueryConsensusStateProof(chain, connection.ClientID, latestHeight, proofConnection.Height.ToBN())
+	consensusStateBytes, proofConsensus, err := counterparty.QueryConsensusStateProof(chain, connection.ClientID, counterpartyConnection.ClientID, latestHeight, proofConnection.Height.ToBN())
 	if err != nil {
 		return err
 	}
@@ -758,8 +759,7 @@ func (chain *Chain) HandlePacketRecv(
 	}
 	switch chain.ClientType() {
 	case ibcclient.MockClient:
-		h := sha256.Sum256(commitPacket(packet))
-		proof.Data = h[:]
+		proof.Data = chain.generateMockClientProof(proof.Height, host.PacketCommitmentPath(packet.SourcePort, packet.SourceChannel, packet.Sequence), commitPacket(packet))
 	}
 	return chain.WaitIfNoError(ctx, "IBCHandler::RecvPacket")(
 		chain.IBCHandler.RecvPacket(
@@ -786,8 +786,7 @@ func (chain *Chain) HandlePacketAcknowledgement(
 	}
 	switch chain.ClientType() {
 	case ibcclient.MockClient:
-		h := sha256.Sum256(commitAcknowledgement(acknowledgement))
-		proof.Data = h[:]
+		proof.Data = chain.generateMockClientProof(proof.Height, host.PacketAcknowledgementPath(packet.DestinationPort, packet.DestinationChannel, packet.Sequence), commitAcknowledgement(acknowledgement))
 	}
 	return chain.WaitIfNoError(ctx, "IBCHandler::AcknowledgePacket")(
 		chain.IBCHandler.AcknowledgePacket(
@@ -1231,30 +1230,30 @@ func (chain *Chain) QueryClientStateProof(counterparty *Chain, clientID, counter
 	var latestHeight ibcclient.Height
 	switch chain.ClientType() {
 	case ibcclient.MockClient:
-		h := sha256.Sum256(cs)
-		proof.Data = h[:]
+		proof.Data = chain.generateMockClientProof(proof.Height, host.FullClientStatePath(counterpartyClientID), cs)
 		latestHeight = chain.GetMockClientState(clientID).LatestHeight
 	case ibcclient.BesuIBFT2Client:
 		latestHeight = chain.GetIBFT2ClientState(clientID).LatestHeight
 	}
 	return cs, latestHeight, proof, nil
 }
 
-func (chain *Chain) QueryConsensusStateProof(counterparty *Chain, clientID string, consensusHeight ibcclient.Height, height *big.Int) ([]byte, *Proof, error) {
+func (chain *Chain) QueryConsensusStateProof(counterparty *Chain, clientID, counterpartyClientID string, consensusHeight ibcclient.Height, height *big.Int) ([]byte, *Proof, error) {
 	cons, found, err := chain.IBCHandler.GetConsensusState(chain.CallOpts(context.Background(), RelayerKeyIndex), clientID, ibchandler.HeightData(consensusHeight))
 	if err != nil {
 		return nil, nil, err
 	} else if !found {
 		return nil, nil, fmt.Errorf("consensus state not found: %v", consensusHeight)
 	}
-	proof, err := chain.QueryProof(counterparty, clientID, commitment.ConsensusStateCommitmentSlot(clientID, consensusHeight), height)
+	proof, err := chain.QueryProof(counterparty, counterpartyClientID, commitment.ConsensusStateCommitmentSlot(clientID, consensusHeight), height)
 	if err != nil {
 		return nil, nil, err
 	}
 	switch chain.ClientType() {
 	case ibcclient.MockClient:
-		h := sha256.Sum256(cons)
-		proof.Data = h[:]
+		proof.Data = chain.generateMockClientProof(proof.Height, host.FullConsensusStatePath(counterpartyClientID, clienttypes.NewHeight(
+			consensusHeight.RevisionNumber, consensusHeight.RevisionHeight,
+		)), cons)
 	}
 	return cons, proof, nil
 }
@@ -1279,8 +1278,7 @@ func (chain *Chain) QueryConnectionProof(counterparty *Chain, counterpartyClient
 		if err != nil {
 			return nil, err
 		}
-		h := sha256.Sum256(bz)
-		proof.Data = h[:]
+		proof.Data = chain.generateMockClientProof(proof.Height, host.ConnectionPath(connectionID), bz)
 	}
 	return proof, nil
 }
@@ -1305,8 +1303,7 @@ func (chain *Chain) QueryChannelProof(counterparty *Chain, counterpartyClientID
 		if err != nil {
 			return nil, err
 		}
-		h := sha256.Sum256(bz)
-		proof.Data = h[:]
+		proof.Data = chain.generateMockClientProof(proof.Height, host.ChannelPath(channel.PortID, channel.ID), bz)
 	}
 	return proof, nil
 }
@@ -1348,12 +1345,26 @@ func (chain *Chain) QueryNextSequenceRecvProof(counterparty *Chain, counterparty
 		}
 		bz := make([]byte, 8)
 		binary.BigEndian.PutUint64(bz, seq)
-		h := sha256.Sum256(bz)
-		proof.Data = h[:]
+		proof.Data = chain.generateMockClientProof(proof.Height, host.NextSequenceRecvPath(channel.PortID, channel.ID), bz)
 	}
 	return proof, nil
 }
 
+func (chain *Chain) generateMockClientProof(height ibcclient.Height, path string, value []byte) []byte {
+	var heightBz [16]byte
+	binary.BigEndian.PutUint64(heightBz[:8], height.RevisionNumber)
+	binary.BigEndian.PutUint64(heightBz[8:], height.RevisionHeight)
+	hashPrefix := sha256.Sum256([]byte("ibc"))
+	hashPath := sha256.Sum256([]byte(path))
+	hashValue := sha256.Sum256(value)
+
+	hash := append(heightBz[:], hashPrefix[:]...)
+	hash = append(hash, hashPath[:]...)
+	hash = append(hash, hashValue[:]...)
+	h := sha256.Sum256(hash)
+	return h[:]
+}
+
 func (chain *Chain) LastHeader() *gethtypes.Header {
 	return chain.LatestLCInputData.Header()
 }

diff --git a/tests/foundry/src/IBC.t.sol b/tests/foundry/src/IBC.t.sol
@@ -26,7 +26,7 @@ contract IBCTest is Test {
 
     string private constant MOCK_CLIENT_TYPE = "mock-client";
     string private constant MOCK_PORT_ID = "mock";
-    bytes32 private testPacketCommitment;
+    bytes32 private testPacketCommitmentProof;
 
     function setUp() public {
         handler = new TestableIBCHandler(
@@ -82,7 +82,9 @@ contract IBCTest is Test {
         handler.setNextSequenceRecv(MOCK_PORT_ID, "channel-0", 1);
         handler.setNextSequenceAck(MOCK_PORT_ID, "channel-0", 1);
 
-        testPacketCommitment = makePacketCommitment(createPacket(0, 100));
+        testPacketCommitmentProof = makeMockClientPacketCommitmentProof(
+            createPacket(0, 100), Height.Data({revision_number: 0, revision_height: 1})
+        );
     }
 
     function setUpMockApp() internal {
@@ -125,7 +127,7 @@ contract IBCTest is Test {
         handler.recvPacket(
             IIBCChannelRecvPacket.MsgPacketRecv({
                 packet: packet,
-                proof: abi.encodePacked(sha256(abi.encodePacked(testPacketCommitment))),
+                proof: abi.encodePacked(testPacketCommitmentProof),
                 proofHeight: Height.Data({revision_number: 0, revision_height: 1})
             })
         );
@@ -228,14 +230,26 @@ contract IBCTest is Test {
         });
     }
 
-    function makePacketCommitment(Packet.Data memory packet) internal pure returns (bytes32) {
-        return sha256(
+    function makeMockClientPacketCommitmentProof(Packet.Data memory packet, Height.Data memory proofHeight)
+        internal
+        pure
+        returns (bytes32)
+    {
+        bytes32 value = sha256(
             abi.encodePacked(
                 packet.timeout_timestamp,
                 packet.timeout_height.revision_number,
                 packet.timeout_height.revision_height,
                 sha256(packet.data)
             )
         );
+        return sha256(
+            abi.encodePacked(
+                proofHeight.toUint128(),
+                sha256("ibc"),
+                sha256(IBCCommitment.packetCommitmentPath(packet.source_port, packet.source_channel, packet.sequence)),
+                sha256(abi.encodePacked(value))
+            )
+        );
     }
 }
diff --git a/tests/foundry/src/ICS02.t.sol b/tests/foundry/src/ICS02.t.sol
@@ -12,8 +12,8 @@ import "../../../contracts/proto/MockClient.sol";
 import "../../../contracts/proto/Connection.sol";
 import "../../../contracts/proto/Channel.sol";
 import "../../../contracts/apps/mock/IBCMockApp.sol";
+import "../../../contracts/clients/MockClient.sol";
 import "./TestableIBCHandler.t.sol";
-import "./helpers/ModifiedMockClient.sol";
 
 abstract contract TestIBCBase is Test {
     bytes internal constant DEFAULT_COMMITMENT_PREFIX = bytes("ibc");
@@ -38,9 +38,9 @@ abstract contract TestIBCBase is Test {
 abstract contract TestMockClientHelper is TestIBCBase {
     using IBCHeight for Height.Data;
 
-    function ibcHandlerMockClient() internal returns (TestableIBCHandler, ModifiedMockClient) {
+    function ibcHandlerMockClient() internal returns (TestableIBCHandler, MockClient) {
         TestableIBCHandler handler = defaultIBCHandler();
-        ModifiedMockClient mockClient = new ModifiedMockClient(address(handler));
+        MockClient mockClient = new MockClient(address(handler));
         handler.registerClient(MOCK_CLIENT_TYPE, mockClient);
         return (handler, mockClient);
     }

diff --git a/tests/foundry/src/ICS04.t.sol b/tests/foundry/src/ICS04.t.sol
@@ -821,8 +821,8 @@ contract TestICS04Packet is TestIBCBase, TestMockClientHelper, TestICS03Helper,
 
     TestableIBCHandler handler;
     TestableIBCHandler counterpartyHandler;
-    ModifiedMockClient client;
-    ModifiedMockClient counterpartyClient;
+    MockClient client;
+    MockClient counterpartyClient;
     IBCMockApp mockApp;
     IBCMockApp counterpartyMockApp;
 
@@ -832,8 +832,8 @@ contract TestICS04Packet is TestIBCBase, TestMockClientHelper, TestICS03Helper,
     string counterpartyConnectionId;
 
     function setUp() public {
-        (TestableIBCHandler _handler, ModifiedMockClient _client) = ibcHandlerMockClient();
-        (TestableIBCHandler _counterpartyHandler, ModifiedMockClient _counterpartyClient) = ibcHandlerMockClient();
+        (TestableIBCHandler _handler, MockClient _client) = ibcHandlerMockClient();
+        (TestableIBCHandler _counterpartyHandler, MockClient _counterpartyClient) = ibcHandlerMockClient();
         handler = _handler;
         counterpartyHandler = _counterpartyHandler;
         client = _client;