correctly escape and unescape merkle keys

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

@@ -2,6 +2,7 @@ package keeper
 
 import (
 	"fmt"
+	"net/url"
 	"reflect"
 	"strings"
 
@@ -253,7 +254,9 @@ func (k Keeper) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientS
 
 	if tmClient.UpgradePath != "" {
 		// For now, SDK IBC implementation assumes that upgrade path (if defined) is defined by SDK upgrade module
-		expectedUpgradePath := fmt.Sprintf("%s/%s", upgradetypes.StoreKey, upgradetypes.KeyUpgradedClient)
+		// Must escape any merkle key before adding it to upgrade path
+		upgradeKey := url.PathEscape(upgradetypes.KeyUpgradedClient)
+		expectedUpgradePath := fmt.Sprintf("%s/%s", upgradetypes.StoreKey, upgradeKey)
 		if tmClient.UpgradePath != expectedUpgradePath {
 			return sdkerrors.Wrapf(types.ErrInvalidClient, "upgrade path must be the upgrade path defined by upgrade module. expected %s, got %s",
 				expectedUpgradePath, tmClient.UpgradePath)

x/ibc/light-clients/07-tendermint/types/tendermint_test.go

@@ -18,7 +18,6 @@ import (
 	ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/07-tendermint/types"
 	ibctesting "github.com/cosmos/cosmos-sdk/x/ibc/testing"
 	ibctestingmock "github.com/cosmos/cosmos-sdk/x/ibc/testing/mock"
-	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 )
 
 const (
@@ -34,7 +33,7 @@ const (
 var (
 	height          = clienttypes.NewHeight(0, 4)
 	newClientHeight = clienttypes.NewHeight(1, 1)
-	upgradePath     = fmt.Sprintf("%s/%s", "upgrade", upgradetypes.KeyUpgradedClient)
+	upgradePath     = fmt.Sprintf("%s/%s", "upgrade", "upgradedClient")
 )
 
 type TendermintTestSuite struct {

x/ibc/light-clients/07-tendermint/types/upgrade.go

@@ -2,6 +2,7 @@ package types
 
 import (
 	"fmt"
+	"net/url"
 	"reflect"
 	"strings"
 
@@ -29,7 +30,10 @@ func (cs ClientState) VerifyUpgrade(
 	if cs.UpgradePath == "" {
 		return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, no upgrade path set")
 	}
-	upgradePath := constructUpgradeMerklePath(cs.UpgradePath, upgradeHeight)
+	upgradePath, err := constructUpgradeMerklePath(cs.UpgradePath, upgradeHeight)
+	if err != nil {
+		return sdkerrors.Wrapf(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade client, unescaping key with URL format failed: %v", err)
+	}
 
 	// UpgradeHeight must be in same epoch as client state height
 	if cs.GetLatestHeight().GetEpochNumber() != upgradeHeight.GetEpochNumber() {
@@ -101,12 +105,17 @@ func (cs ClientState) VerifyUpgrade(
 }
 
 // construct MerklePath from upgradePath
-func constructUpgradeMerklePath(upgradePath string, upgradeHeight exported.Height) commitmenttypes.MerklePath {
+func constructUpgradeMerklePath(upgradePath string, upgradeHeight exported.Height) (commitmenttypes.MerklePath, error) {
 	// assume that all keys here are separated by `/` and
 	// any `/` within a merkle key is correctly escaped
 	upgradeKeys := strings.Split(upgradePath, "/")
+	// unescape the last key so that we can append `/{height}` to the last key
+	lastKey, err := url.PathUnescape(upgradeKeys[len(upgradeKeys)-1])
+	if err != nil {
+		return commitmenttypes.MerklePath{}, err
+	}
 	// append upgradeHeight to last key in merkle path
 	// this will create the IAVL key that is used to store client in upgrade store
-	upgradeKeys[len(upgradeKeys)-1] = fmt.Sprintf("%s/%d", upgradeKeys[len(upgradeKeys)-1], upgradeHeight.GetEpochHeight())
-	return commitmenttypes.NewMerklePath(upgradeKeys)
+	upgradeKeys[len(upgradeKeys)-1] = fmt.Sprintf("%s/%d", lastKey, upgradeHeight.GetEpochHeight())
+	return commitmenttypes.NewMerklePath(upgradeKeys), nil
 }

x/ibc/light-clients/07-tendermint/types/upgrade_test.go

@@ -184,7 +184,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 			expPass: false,
 		},
 		{
-			name: "unsuccessful upgrade: upgrade path is nil",
+			name: "unsuccessful upgrade: upgrade path is empty",
 			setup: func() {
 
 				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
@@ -206,13 +206,43 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 
 				proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetEpochHeight())), cs.GetLatestHeight().GetEpochHeight())
 
-				// SetClientState with nil upgrade path
+				// SetClientState with empty upgrade path
 				tmClient, _ := cs.(*types.ClientState)
 				tmClient.UpgradePath = ""
 				suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), clientA, tmClient)
 			},
 			expPass: false,
 		},
+		{
+			name: "unsuccessful upgrade: upgrade path is malformed and cannot be correctly unescaped",
+			setup: func() {
+
+				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, newClientHeight, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
+
+				// upgrade Height is at next block
+				upgradeHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1))
+
+				// zero custom fields and store in upgrade store
+				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetEpochHeight()), upgradedClient)
+
+				// commit upgrade store changes and update clients
+
+				suite.coordinator.CommitBlock(suite.chainB)
+				err := suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.Tendermint)
+				suite.Require().NoError(err)
+
+				cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
+				suite.Require().True(found)
+
+				proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetEpochHeight())), cs.GetLatestHeight().GetEpochHeight())
+
+				// SetClientState with nil upgrade path
+				tmClient, _ := cs.(*types.ClientState)
+				tmClient.UpgradePath = "upgraded%Client"
+				suite.chainA.App.IBCKeeper.ClientKeeper.SetClientState(suite.chainA.GetContext(), clientA, tmClient)
+			},
+			expPass: false,
+		},
 		{
 			name: "unsuccessful upgrade: upgraded height is not greater than current height",
 			setup: func() {
@@ -267,9 +297,9 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 			name: "unsuccessful upgrade: client is expired",
 			setup: func() {
 
-				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), &upgradePath, false, false)
+				upgradedClient = types.NewClientState("newChainId", types.DefaultTrustLevel, trustingPeriod, ubdPeriod+trustingPeriod, maxClockDrift, upgradeHeight, commitmenttypes.GetSDKSpecs(), upgradePath, false, false)
 				// zero custom fields and store in upgrade store
-				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), upgradedClient)
+				suite.chainB.App.UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(upgradeHeight.GetEpochHeight()), upgradedClient)
 
 				// commit upgrade store changes and update clients
 
@@ -283,7 +313,7 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() {
 				cs, found := suite.chainA.App.IBCKeeper.ClientKeeper.GetClientState(suite.chainA.GetContext(), clientA)
 				suite.Require().True(found)
 
-				proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(), cs.GetLatestHeight().GetEpochHeight())
+				proofUpgrade, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(upgradeHeight.GetEpochHeight())), cs.GetLatestHeight().GetEpochHeight())
 			},
 			expPass: false,
 		},

x/ibc/testing/chain.go

@@ -37,7 +37,6 @@ import (
 	ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/light-clients/07-tendermint/types"
 	"github.com/cosmos/cosmos-sdk/x/ibc/testing/mock"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
-	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 )
 
 const (
@@ -71,7 +70,7 @@ var (
 	TestHash                              = tmhash.Sum([]byte("TESTING HASH"))
 	TestCoin                              = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))
 
-	UpgradePath = fmt.Sprintf("%s/%s", "upgrade", upgradetypes.KeyUpgradedClient)
+	UpgradePath = fmt.Sprintf("%s/%s", "upgrade", "upgradedClient")
 
 	ConnectionVersion = connectiontypes.GetCompatibleEncodedVersions()[0]