add explicit checks in OnChanUpgradeTry callback of ICA host (#5519)

* add explicit checks in on chan upgrade try callback of ica host

* pr review

* Update modules/apps/27-interchain-accounts/host/keeper/handshake.go

---------

Co-authored-by: Charly <charly@interchain.io>
Co-authored-by: Charly <charly@interchain.berlin>
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>

modules/apps/27-interchain-accounts/host/keeper/handshake.go

@@ -128,21 +128,45 @@ func (Keeper) OnChanCloseConfirm(
 }
 
 // OnChanUpgradeTry performs the upgrade try step of the channel upgrade handshake.
+// The upgrade try callback must verify the proposed changes to the order, connectionHops, and version.
+// Within the version we have the tx type, encoding, interchain account address, host/controller connectionID's
+// and the ICS27 protocol version.
+//
+// The following may be changed:
+// - tx type (must be supported)
+// - encoding (must be supported)
+//
+// The following may not be changed:
+// - order
+// - connectionHops (and subsequently host/controller connectionIDs)
+// - interchain account address
+// - ICS27 protocol version
 func (k Keeper) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, counterpartyVersion string) (string, error) {
+	// verify order has not changed
+	// support for unordered ICA channels is not implemented yet
+	if order != channeltypes.ORDERED {
+		return "", errorsmod.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order)
+	}
+
 	if portID != icatypes.HostPortID {
 		return "", errorsmod.Wrapf(porttypes.ErrInvalidPort, "expected %s, got %s", icatypes.HostPortID, portID)
 	}
 
-	if strings.TrimSpace(counterpartyVersion) == "" {
-		return "", errorsmod.Wrap(channeltypes.ErrInvalidChannelVersion, "counterparty version cannot be empty")
+	// verify connection hops has not changed
+	connectionID, err := k.getConnectionID(ctx, portID, channelID)
+	if err != nil {
+		return "", err
 	}
 
-	// support for unordered ICA channels is not implemented yet
-	if order != channeltypes.ORDERED {
-		return "", errorsmod.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order)
+	if len(connectionHops) != 1 || connectionHops[0] != connectionID {
+		return "", errorsmod.Wrapf(channeltypes.ErrInvalidUpgrade, "expected connection hops %s, got %s", []string{connectionID}, connectionHops)
 	}
 
-	metadata, err := icatypes.MetadataFromVersion(counterpartyVersion)
+	if strings.TrimSpace(counterpartyVersion) == "" {
+		return "", errorsmod.Wrap(channeltypes.ErrInvalidChannelVersion, "counterparty version cannot be empty")
+	}
+
+	proposedCounterpartyMetadata, err := icatypes.MetadataFromVersion(counterpartyVersion)
 	if err != nil {
 		return "", err
 	}
@@ -152,16 +176,24 @@ func (k Keeper) OnChanUpgradeTry(ctx sdk.Context, portID, channelID string, orde
 		return "", err
 	}
 
-	if err := icatypes.ValidateHostMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil {
+	// ValidateHostMetadata will ensure the ICS27 protocol version has not changed and that the
+	// tx type and encoding are supported. It also validates the connection params against the counterparty metadata.
+	if err := icatypes.ValidateHostMetadata(ctx, k.channelKeeper, connectionHops, proposedCounterpartyMetadata); err != nil {
 		return "", errorsmod.Wrap(err, "invalid metadata")
 	}
 
 	// the interchain account address on the host chain
 	// must remain the same after the upgrade.
-	if currentMetadata.Address != metadata.Address {
+	if currentMetadata.Address != proposedCounterpartyMetadata.Address {
 		return "", errorsmod.Wrap(icatypes.ErrInvalidAccountAddress, "interchain account address cannot be changed")
 	}
 
+	// these explicit checks on the controller connection identifier should be unreachable
+	if currentMetadata.ControllerConnectionId != proposedCounterpartyMetadata.ControllerConnectionId {
+		return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed controller connection ID must not change")
+	}
+
+	// these explicit checks on the host connection identifier should be unreachable
 	if currentMetadata.HostConnectionId != connectionHops[0] {
 		return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed connection hop must not change")
 	}

modules/apps/27-interchain-accounts/host/keeper/handshake_test.go

@@ -15,6 +15,10 @@ import (
 	ibctesting "github.com/cosmos/ibc-go/v8/testing"
 )
 
+const (
+	differentConnectionID = "connection-100"
+)
+
 // open and close channel is a helper function for TestOnChanOpenTry for reopening accounts
 func (suite *KeeperTestSuite) openAndCloseChannel(path *ibctesting.Path) {
 	err := path.EndpointB.ChanOpenTry()
@@ -165,6 +169,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
 				path.EndpointB.SetChannel(*channel)
 			}, false,
 		},
+
 		{
 			"invalid port ID",
 			func() {
@@ -426,6 +431,15 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeTry() {
 		counterpartyVersion string
 	)
 
+	// updateMetadata is a helper function which modifies the metadata stored in the channel version
+	// and marshals it into a string to pass to OnChanUpgradeTry as the counterpartyVersion string.
+	updateMetadata := func(modificationFn func(*icatypes.Metadata)) {
+		metadata, err := icatypes.MetadataFromVersion(path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version)
+		suite.Require().NoError(err)
+		modificationFn(&metadata)
+		counterpartyVersion = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
+	}
+
 	testCases := []struct {
 		name     string
 		malleate func()
@@ -443,6 +457,21 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeTry() {
 			},
 			expError: porttypes.ErrInvalidPort,
 		},
+		{
+			name: "failure: invalid order",
+			malleate: func() {
+				order = channeltypes.UNORDERED
+			},
+			expError: channeltypes.ErrInvalidChannelOrdering,
+		},
+		{
+			name: "failure: invalid proposed connectionHops",
+			malleate: func() {
+				// connection hops is provided via endpoint connectionID
+				path.EndpointB.ConnectionID = differentConnectionID
+			},
+			expError: channeltypes.ErrInvalidUpgrade,
+		},
 		{
 			name: "failure: empty counterparty version",
 			malleate: func() {
@@ -469,37 +498,54 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeTry() {
 		{
 			name: "failure: metadata encoding not supported",
 			malleate: func() {
-				metadata.Encoding = "invalid-encoding-format"
-				counterpartyVersion = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.Encoding = "invalid-encoding-format"
+				})
 			},
 			expError: icatypes.ErrInvalidCodec,
 		},
+		{
+			name: "failure: metadata tx type not supported",
+			malleate: func() {
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.TxType = "invalid-tx-type"
+				})
+			},
+			expError: icatypes.ErrUnknownDataType,
+		},
 		{
 			name: "failure: interchain account address has changed",
 			malleate: func() {
-				channel := path.EndpointB.GetChannel()
-				metadata.Address = "invalid address"
-				channel.Version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
-				path.EndpointB.SetChannel(channel)
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.Address = TestOwnerAddress // use valid address
+				})
 			},
 			expError: icatypes.ErrInvalidAccountAddress,
 		},
 		{
-			name: "failure: invalid connection identifier",
+			name: "failure: controller connection ID has changed",
 			malleate: func() {
-				channel := path.EndpointB.GetChannel()
-				metadata.HostConnectionId = "invalid-connection-id"
-				channel.Version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
-				path.EndpointB.SetChannel(channel)
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.ControllerConnectionId = differentConnectionID
+				})
 			},
-			expError: connectiontypes.ErrInvalidConnectionIdentifier,
+			expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the controller connection identifier are unreachable
 		},
 		{
-			name: "failure: invalid order",
+			name: "failure: host connection ID has changed",
 			malleate: func() {
-				order = channeltypes.UNORDERED
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.HostConnectionId = differentConnectionID
+				})
 			},
-			expError: channeltypes.ErrInvalidChannelOrdering,
+			expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the host connection identifier are unreachable
+		},
+		{
+			name: "failure: channel not found",
+			malleate: func() {
+				path.EndpointB.ChannelID = "invalid-channel-id"
+			},
+			expError: channeltypes.ErrChannelNotFound,
 		},
 	}
 

modules/apps/27-interchain-accounts/host/keeper/keeper.go

@@ -16,6 +16,7 @@ import (
 	genesistypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/genesis/types"
 	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types"
 	icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types"
+	channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
 	porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types"
 	host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
 	ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
@@ -84,6 +85,15 @@ func (Keeper) Logger(ctx sdk.Context) log.Logger {
 	return ctx.Logger().With("module", fmt.Sprintf("x/%s-%s", exported.ModuleName, icatypes.ModuleName))
 }
 
+// getConnectionID returns the connection id for the given port and channelIDs.
+func (k Keeper) getConnectionID(ctx sdk.Context, portID, channelID string) (string, error) {
+	channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID)
+	if !found {
+		return "", errorsmod.Wrapf(channeltypes.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
+	}
+	return channel.ConnectionHops[0], nil
+}
+
 // setPort sets the provided portID in state.
 func (k Keeper) setPort(ctx sdk.Context, portID string) {
 	store := ctx.KVStore(k.storeKey)

modules/apps/27-interchain-accounts/types/metadata.go

@@ -78,7 +78,8 @@ func IsPreviousMetadataEqual(previousVersion string, metadata Metadata) bool {
 		previousMetadata.TxType == metadata.TxType)
 }
 
-// ValidateControllerMetadata performs validation of the provided ICS27 controller metadata parameters
+// ValidateControllerMetadata performs validation of the provided ICS27 controller metadata parameters as well
+// as the connection params against the provided metadata
 func ValidateControllerMetadata(ctx sdk.Context, channelKeeper ChannelKeeper, connectionHops []string, metadata Metadata) error {
 	if !isSupportedEncoding(metadata.Encoding) {
 		return errorsmod.Wrapf(ErrInvalidCodec, "unsupported encoding format %s", metadata.Encoding)