Verify Client on Connection Handshake (cosmos#7057)

* verify client state

* add client state to msgs and retrieve in handler

* fix connection msgs

* fixed handshake tests

* fix tests

* fix sim tests

* revert pb edit

* add ValidateClient tests

* Apply suggestions from code review

Co-authored-by: Federico Kunze <[email protected]>

* fix tests

* fixed msgs test

* use ibctesting for client state consts

* Apply suggestions from code review

Co-authored-by: colin axnér <[email protected]>
Co-authored-by: Federico Kunze <[email protected]>

* address rest of comments

* rename to ValidateSelfClient and update spec

* lint

* Update x/ibc/02-client/keeper/keeper_test.go

* Update x/ibc/02-client/keeper/keeper_test.go

* complete rest of review

* improve cov

Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: colin axnér <[email protected]>

proto/ibc/connection/connection.proto

@@ -4,6 +4,7 @@ package ibc.connection;
 option go_package = "github.com/cosmos/cosmos-sdk/x/ibc/03-connection/types";
 
 import "gogoproto/gogo.proto";
+import "google/protobuf/any.proto";
 import "ibc/commitment/commitment.proto";
 
 // MsgConnectionOpenInit defines the msg sent by an account on Chain A to
@@ -29,18 +30,23 @@ message MsgConnectionOpenTry {
   string connection_id = 2 [
     (gogoproto.moretags) = "yaml:\"connection_id\""
   ];
-  Counterparty counterparty = 3 [(gogoproto.nullable) = false];
-  repeated string counterparty_versions = 4
+  google.protobuf.Any client_state = 3 [
+    (gogoproto.moretags) = "yaml:\"client_state\""
+  ];
+  Counterparty counterparty = 4 [(gogoproto.nullable) = false];
+  repeated string counterparty_versions = 5
       [(gogoproto.moretags) = "yaml:\"counterparty_versions\""];
+  uint64 proof_height = 6 [(gogoproto.moretags) = "yaml:\"proof_height\""];
   // proof of the initialization the connection on Chain A: `UNITIALIZED ->
   // INIT`
-  bytes proof_init = 5 [(gogoproto.moretags) = "yaml:\"proof_init\""];
-  uint64 proof_height = 6;
+  bytes proof_init = 7 [(gogoproto.moretags) = "yaml:\"proof_init\""];
+  // proof of client state included in message
+  bytes proof_client = 8 [(gogoproto.moretags) = "yaml:\"proof_client\""];
   // proof of client consensus state
-  bytes proof_consensus = 7 [(gogoproto.moretags) = "yaml:\"proof_consensus\""];
-  uint64 consensus_height = 8
+  bytes proof_consensus = 9 [(gogoproto.moretags) = "yaml:\"proof_consensus\""];
+  uint64 consensus_height = 10
       [(gogoproto.moretags) = "yaml:\"consensus_height\""];
-  bytes signer = 9
+  bytes signer = 11
       [(gogoproto.casttype) = "github.com/cosmos/cosmos-sdk/types.AccAddress"];
 }
 
@@ -51,15 +57,20 @@ message MsgConnectionOpenAck {
     (gogoproto.moretags) = "yaml:\"connection_id\""
   ];
   string version = 2;
+  google.protobuf.Any client_state = 3 [
+    (gogoproto.moretags) = "yaml:\"client_state\""
+  ];
+  uint64 proof_height = 4 [(gogoproto.moretags) = "yaml:\"proof_height\""];
   // proof of the initialization the connection on Chain B: `UNITIALIZED ->
   // TRYOPEN`
-  bytes proof_try = 3 [(gogoproto.moretags) = "yaml:\"proof_try\""];
-  uint64 proof_height = 4 [(gogoproto.moretags) = "yaml:\"proof_height\""];
+  bytes proof_try = 5 [(gogoproto.moretags) = "yaml:\"proof_try\""];
+  // proof of client state included in message
+  bytes proof_client = 6 [(gogoproto.moretags) = "yaml:\"proof_client\""];
   // proof of client consensus state
-  bytes proof_consensus = 5 [(gogoproto.moretags) = "yaml:\"proof_consensus\""];
-  uint64 consensus_height = 6
+  bytes proof_consensus = 7 [(gogoproto.moretags) = "yaml:\"proof_consensus\""];
+  uint64 consensus_height = 8
       [(gogoproto.moretags) = "yaml:\"consensus_height\""];
-  bytes signer = 7
+  bytes signer = 9
       [(gogoproto.casttype) = "github.com/cosmos/cosmos-sdk/types.AccAddress"];
 }
 

simapp/test_helpers.go

@@ -319,15 +319,15 @@ func CheckBalance(t *testing.T, app *SimApp, addr sdk.AccAddress, balances sdk.C
 // returned.
 func SignCheckDeliver(
 	t *testing.T, txGen client.TxConfig, app *bam.BaseApp, header tmproto.Header, msgs []sdk.Msg,
-	accNums, seq []uint64, expSimPass, expPass bool, priv ...crypto.PrivKey,
+	chainID string, accNums, seq []uint64, expSimPass, expPass bool, priv ...crypto.PrivKey,
 ) (sdk.GasInfo, *sdk.Result, error) {
 
 	tx, err := helpers.GenTx(
 		txGen,
 		msgs,
 		sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 0)},
 		helpers.DefaultGenTxGas,
-		"",
+		chainID,
 		accNums,
 		seq,
 		priv...,

x/bank/app_test.go

@@ -109,7 +109,7 @@ func TestSendNotEnoughBalance(t *testing.T) {
 	sendMsg := types.NewMsgSend(addr1, addr2, sdk.Coins{sdk.NewInt64Coin("foocoin", 100)})
 	header := tmproto.Header{Height: app.LastBlockHeight() + 1}
 	txGen := simapp.MakeEncodingConfig().TxConfig
-	_, _, err = simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, []sdk.Msg{sendMsg}, []uint64{origAccNum}, []uint64{origSeq}, false, false, priv1)
+	_, _, err = simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, []sdk.Msg{sendMsg}, "", []uint64{origAccNum}, []uint64{origSeq}, false, false, priv1)
 	require.Error(t, err)
 
 	simapp.CheckBalance(t, app, addr1, sdk.Coins{sdk.NewInt64Coin("foocoin", 67)})
@@ -177,7 +177,7 @@ func TestSendToModuleAcc(t *testing.T) {
 
 			header := tmproto.Header{Height: app.LastBlockHeight() + 1}
 			txGen := simapp.MakeEncodingConfig().TxConfig
-			_, _, err = simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, []sdk.Msg{test.msg}, []uint64{origAccNum}, []uint64{origSeq}, test.expSimPass, test.expPass, priv1)
+			_, _, err = simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, []sdk.Msg{test.msg}, "", []uint64{origAccNum}, []uint64{origSeq}, test.expSimPass, test.expPass, priv1)
 			if test.expPass {
 				require.NoError(t, err)
 			} else {
@@ -248,7 +248,7 @@ func TestMsgMultiSendWithAccounts(t *testing.T) {
 	for _, tc := range testCases {
 		header := tmproto.Header{Height: app.LastBlockHeight() + 1}
 		txGen := simapp.MakeEncodingConfig().TxConfig
-		_, _, err := simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, tc.msgs, tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
+		_, _, err := simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, tc.msgs, "", tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
 		if tc.expPass {
 			require.NoError(t, err)
 		} else {
@@ -300,7 +300,7 @@ func TestMsgMultiSendMultipleOut(t *testing.T) {
 	for _, tc := range testCases {
 		header := tmproto.Header{Height: app.LastBlockHeight() + 1}
 		txGen := simapp.MakeEncodingConfig().TxConfig
-		_, _, err := simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, tc.msgs, tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
+		_, _, err := simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, tc.msgs, "", tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
 		require.NoError(t, err)
 
 		for _, eb := range tc.expectedBalances {
@@ -355,7 +355,7 @@ func TestMsgMultiSendMultipleInOut(t *testing.T) {
 	for _, tc := range testCases {
 		header := tmproto.Header{Height: app.LastBlockHeight() + 1}
 		txGen := simapp.MakeEncodingConfig().TxConfig
-		_, _, err := simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, tc.msgs, tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
+		_, _, err := simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, tc.msgs, "", tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
 		require.NoError(t, err)
 
 		for _, eb := range tc.expectedBalances {
@@ -408,7 +408,7 @@ func TestMsgMultiSendDependent(t *testing.T) {
 	for _, tc := range testCases {
 		header := tmproto.Header{Height: app.LastBlockHeight() + 1}
 		txGen := simapp.MakeEncodingConfig().TxConfig
-		_, _, err := simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, tc.msgs, tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
+		_, _, err := simapp.SignCheckDeliver(t, txGen, app.BaseApp, header, tc.msgs, "", tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
 		require.NoError(t, err)
 
 		for _, eb := range tc.expectedBalances {

x/ibc/02-client/exported/exported.go

@@ -32,6 +32,16 @@ type ClientState interface {
 
 	// State verification functions
 
+	VerifyClientState(
+		store sdk.KVStore,
+		cdc codec.BinaryMarshaler,
+		root commitmentexported.Root,
+		height uint64,
+		prefix commitmentexported.Prefix,
+		counterpartyClientIdentifier string,
+		proof []byte,
+		clientState ClientState,
+	) error
 	VerifyClientConsensusState(
 		store sdk.KVStore,
 		cdc codec.BinaryMarshaler,

x/ibc/02-client/keeper/keeper.go

@@ -2,14 +2,17 @@ package keeper
 
 import (
 	"fmt"
+	"reflect"
 	"strings"
 
 	"github.com/tendermint/tendermint/libs/log"
+	"github.com/tendermint/tendermint/light"
 
 	"github.com/cosmos/cosmos-sdk/codec"
 	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
 	"github.com/cosmos/cosmos-sdk/store/prefix"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	"github.com/cosmos/cosmos-sdk/x/ibc/02-client/exported"
 	"github.com/cosmos/cosmos-sdk/x/ibc/02-client/types"
 	ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint/types"
@@ -197,6 +200,52 @@ func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height uint64) (exported.
 	return consensusState, true
 }
 
+// ValidateSelfClient validates the client parameters for a client of the running chain
+// This function is only used to validate the client state the counterparty stores for this chain
+func (k Keeper) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error {
+	tmClient, ok := clientState.(*ibctmtypes.ClientState)
+	if !ok {
+		return sdkerrors.Wrapf(types.ErrInvalidClient, "client must be a Tendermint client, expected: %T, got: %T",
+			&ibctmtypes.ClientState{}, tmClient)
+	}
+
+	if clientState.IsFrozen() {
+		return types.ErrClientFrozen
+	}
+
+	if ctx.ChainID() != tmClient.ChainId {
+		return sdkerrors.Wrapf(types.ErrInvalidClient, "invalid chain-id. expected: %s, got: %s",
+			ctx.ChainID(), tmClient.ChainId)
+	}
+
+	if tmClient.LatestHeight > uint64(ctx.BlockHeight()) {
+		return sdkerrors.Wrapf(types.ErrInvalidClient, "client has LatestHeight %d greater than chain height %d",
+			tmClient.LatestHeight, ctx.BlockHeight())
+	}
+
+	expectedProofSpecs := commitmenttypes.GetSDKSpecs()
+	if !reflect.DeepEqual(expectedProofSpecs, tmClient.ProofSpecs) {
+		return sdkerrors.Wrapf(types.ErrInvalidClient, "client has invalid proof specs. expected: %v got: %v",
+			expectedProofSpecs, tmClient.ProofSpecs)
+	}
+
+	if err := light.ValidateTrustLevel(tmClient.TrustLevel.ToTendermint()); err != nil {
+		return sdkerrors.Wrapf(types.ErrInvalidClient, "trust-level invalid: %v", err)
+	}
+
+	expectedUbdPeriod := k.stakingKeeper.UnbondingTime(ctx)
+	if expectedUbdPeriod != tmClient.UnbondingPeriod {
+		return sdkerrors.Wrapf(types.ErrInvalidClient, "invalid unbonding period. expected: %s, got: %s",
+			expectedUbdPeriod, tmClient.UnbondingPeriod)
+	}
+
+	if tmClient.UnbondingPeriod < tmClient.TrustingPeriod {
+		return sdkerrors.Wrapf(types.ErrInvalidClient, "unbonding period must be greater than trusting period. unbonding period (%d) < trusting period (%d)",
+			tmClient.UnbondingPeriod, tmClient.TrustingPeriod)
+	}
+	return nil
+}
+
 // IterateClients provides an iterator over all stored light client State
 // objects. For each State object, cb will be called. If the cb returns true,
 // the iterator will close and stop.

x/ibc/02-client/keeper/keeper_test.go

@@ -15,9 +15,11 @@ import (
 	"github.com/cosmos/cosmos-sdk/simapp"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/ibc/02-client/exported"
+	clientexported "github.com/cosmos/cosmos-sdk/x/ibc/02-client/exported"
 	"github.com/cosmos/cosmos-sdk/x/ibc/02-client/keeper"
 	"github.com/cosmos/cosmos-sdk/x/ibc/02-client/types"
 	ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint/types"
+	localhosttypes "github.com/cosmos/cosmos-sdk/x/ibc/09-localhost/types"
 	commitmenttypes "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
 )
@@ -124,6 +126,72 @@ func (suite *KeeperTestSuite) TestSetClientConsensusState() {
 	suite.Require().Equal(suite.consensusState, tmConsState, "ConsensusState not stored correctly")
 }
 
+func (suite *KeeperTestSuite) TestValidateSelfClient() {
+	testCases := []struct {
+		name        string
+		clientState clientexported.ClientState
+		expPass     bool
+	}{
+		{
+			"success",
+			ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, uint64(testClientHeight), commitmenttypes.GetSDKSpecs()),
+			true,
+		},
+		{
+			"invalid client type",
+			localhosttypes.NewClientState(testChainID, testClientHeight),
+			false,
+		},
+		{
+			"frozen client",
+			ibctmtypes.ClientState{testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, uint64(testClientHeight), uint64(testClientHeight), commitmenttypes.GetSDKSpecs()},
+			false,
+		},
+		{
+			"incorrect chainID",
+			ibctmtypes.NewClientState("gaiatestnet", ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, uint64(testClientHeight), commitmenttypes.GetSDKSpecs()),
+			false,
+		},
+		{
+			"invalid client height",
+			ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, uint64(testClientHeight)+10, commitmenttypes.GetSDKSpecs()),
+			false,
+		},
+		{
+			"invalid proof specs",
+			ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, uint64(testClientHeight), nil),
+			false,
+		},
+		{
+			"invalid trust level",
+			ibctmtypes.NewClientState(testChainID, ibctmtypes.Fraction{0, 1}, trustingPeriod, ubdPeriod, maxClockDrift, uint64(testClientHeight), commitmenttypes.GetSDKSpecs()),
+			false,
+		},
+		{
+			"invalid unbonding period",
+			ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, trustingPeriod, ubdPeriod+10, maxClockDrift, uint64(testClientHeight), commitmenttypes.GetSDKSpecs()),
+			false,
+		},
+		{
+			"invalid trusting period",
+			ibctmtypes.NewClientState(testChainID, ibctmtypes.DefaultTrustLevel, ubdPeriod+10, ubdPeriod, maxClockDrift, uint64(testClientHeight), commitmenttypes.GetSDKSpecs()),
+			false,
+		},
+	}
+
+	ctx := suite.ctx.WithChainID(testChainID)
+	ctx = ctx.WithBlockHeight(testClientHeight)
+
+	for _, tc := range testCases {
+		err := suite.keeper.ValidateSelfClient(ctx, tc.clientState)
+		if tc.expPass {
+			suite.Require().NoError(err, "expected valid client for case: %s", tc.name)
+		} else {
+			suite.Require().Error(err, "expected invalid client for case: %s", tc.name)
+		}
+	}
+}
+
 func (suite KeeperTestSuite) TestGetAllClients() {
 	clientIDs := []string{
 		testClientID2, testClientID3, testClientID,

x/ibc/02-client/types/codec.go

@@ -70,6 +70,16 @@ func PackClientState(clientState exported.ClientState) (*codectypes.Any, error)
 	return anyClientState, nil
 }
 
+// MustPackClientState calls PackClientState and panics on error.
+func MustPackClientState(clientState exported.ClientState) *codectypes.Any {
+	anyClientState, err := PackClientState(clientState)
+	if err != nil {
+		panic(err)
+	}
+
+	return anyClientState
+}
+
 // UnpackClientState unpacks an Any into a ClientState. It returns an error if the
 // client state can't be unpacked into a ClientState.
 func UnpackClientState(any *codectypes.Any) (exported.ClientState, error) {