Move check for upgraded client state height to 02-client layer. (cosm…

…os#4368)

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

modules/core/02-client/keeper/client.go

@@ -9,6 +9,7 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
+	ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
 	"github.com/cosmos/ibc-go/v8/modules/core/exported"
 )
 
@@ -128,6 +129,14 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e
 		return errorsmod.Wrapf(types.ErrClientNotActive, "cannot upgrade client (%s) with status %s", clientID, status)
 	}
 
+	// last height of current counterparty chain must be client's latest height
+	lastHeight := clientState.GetLatestHeight()
+
+	if !upgradedClient.GetLatestHeight().GT(lastHeight) {
+		return errorsmod.Wrapf(ibcerrors.ErrInvalidHeight, "upgraded client height %s must be at greater than current client height %s",
+			upgradedClient.GetLatestHeight(), lastHeight)
+	}
+
 	if err := clientState.VerifyUpgradeAndUpdateState(ctx, k.cdc, clientStore,
 		upgradedClient, upgradedConsState, proofUpgradeClient, proofUpgradeConsState,
 	); err != nil {

modules/core/02-client/keeper/client_test.go

@@ -437,6 +437,35 @@ func (suite *KeeperTestSuite) TestUpgradeClient() {
 			},
 			expPass: false,
 		},
+		{
+			name: "unsuccessful upgrade: upgraded height is not greater than current height",
+			setup: func() {
+				// last Height is at next block
+				lastHeight = clienttypes.NewHeight(1, uint64(suite.chainB.GetContext().BlockHeight()+1))
+
+				// zero custom fields and store in upgrade store
+				err := suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(lastHeight.GetRevisionHeight()), upgradedClientBz)
+				suite.Require().NoError(err)
+				err = suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(lastHeight.GetRevisionHeight()), upgradedConsStateBz)
+				suite.Require().NoError(err)
+
+				// change upgradedClient height to be lower than current client state height
+				tmClient := upgradedClient.(*ibctm.ClientState)
+				tmClient.LatestHeight = clienttypes.NewHeight(0, 1)
+				upgradedClient = tmClient
+
+				suite.coordinator.CommitBlock(suite.chainB)
+				err = path.EndpointA.UpdateClient()
+				suite.Require().NoError(err)
+
+				cs, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID)
+				suite.Require().True(found)
+
+				proofUpgradedClient, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(lastHeight.GetRevisionHeight())), cs.GetLatestHeight().GetRevisionHeight())
+				proofUpgradedConsState, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(lastHeight.GetRevisionHeight())), cs.GetLatestHeight().GetRevisionHeight())
+			},
+			expPass: false,
+		},
 	}
 
 	for _, tc := range testCases {

modules/light-clients/07-tendermint/upgrade.go

@@ -12,7 +12,6 @@ import (
 
 	clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
 	commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types"
-	ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
 	"github.com/cosmos/ibc-go/v8/modules/core/exported"
 )
 
@@ -36,14 +35,6 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
 		return errorsmod.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, no upgrade path set")
 	}
 
-	// last height of current counterparty chain must be client's latest height
-	lastHeight := cs.GetLatestHeight()
-
-	if !upgradedClient.GetLatestHeight().GT(lastHeight) {
-		return errorsmod.Wrapf(ibcerrors.ErrInvalidHeight, "upgraded client height %s must be at greater than current client height %s",
-			upgradedClient.GetLatestHeight(), lastHeight)
-	}
-
 	// upgraded client state and consensus state must be IBC tendermint client state and consensus state
 	// this may be modified in the future to upgrade to a new IBC tendermint type
 	// counterparty must also commit to the upgraded consensus state at a sub-path under the upgrade path specified
@@ -67,6 +58,9 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
 		return errorsmod.Wrapf(commitmenttypes.ErrInvalidProof, "could not unmarshal consensus state merkle proof: %v", err)
 	}
 
+	// last height of current counterparty chain must be client's latest height
+	lastHeight := cs.GetLatestHeight()
+
 	// Must prove against latest consensus state to ensure we are verifying against latest upgrade plan
 	// This verifies that upgrade is intended for the provided revision, since committed client must exist
 	// at this consensus state

modules/light-clients/08-wasm/types/upgrade.go

@@ -8,7 +8,6 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
-	ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
 	"github.com/cosmos/ibc-go/v8/modules/core/exported"
 )
 
@@ -35,14 +34,6 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
 			&ConsensusState{}, wasmUpgradeConsState)
 	}
 
-	// last height of current counterparty chain must be client's latest height
-	lastHeight := cs.GetLatestHeight()
-
-	if !upgradedClient.GetLatestHeight().GT(lastHeight) {
-		return errorsmod.Wrapf(ibcerrors.ErrInvalidHeight, "upgraded client height %s must be greater than current client height %s",
-			upgradedClient.GetLatestHeight(), lastHeight)
-	}
-
 	payload := SudoMsg{
 		VerifyUpgradeAndUpdateState: &VerifyUpgradeAndUpdateStateMsg{
 			UpgradeClientState:         *wasmUpgradeClientState,