refactor: use explicit checks in ica controller upgrade handlers

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

@@ -142,17 +142,42 @@ func (Keeper) OnChanCloseConfirm(
 }
 
 // OnChanUpgradeInit performs the upgrade init step of the channel upgrade handshake.
+// The upgrade init 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) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) (string, error) {
-	if strings.TrimSpace(version) == "" {
-		return "", errorsmod.Wrap(icatypes.ErrInvalidVersion, "version cannot be empty")
-	}
-
+	// 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)
 	}
 
-	metadata, err := icatypes.MetadataFromVersion(version)
+	// verify connection hops has not changed
+	connectionID, err := k.GetConnectionID(ctx, portID, channelID)
+	if err != nil {
+		return "", err
+	}
+
+	if len(connectionHops) != 1 || connectionHops[0] != connectionID {
+		return "", errorsmod.Wrapf(channeltypes.ErrInvalidUpgrade, "expected connection hops %s, got %s", []string{connectionID}, connectionHops)
+	}
+
+	// verify proposed version only modifies tx type or encoding
+	if strings.TrimSpace(version) == "" {
+		return "", errorsmod.Wrap(icatypes.ErrInvalidVersion, "version cannot be empty")
+	}
+
+	proposedMetadata, err := icatypes.MetadataFromVersion(version)
 	if err != nil {
 		return "", err
 	}
@@ -162,30 +187,49 @@ func (k Keeper) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, ord
 		return "", err
 	}
 
-	if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil {
-		return "", errorsmod.Wrap(err, "invalid metadata")
+	// ValidateControllerMetadata will ensure the ICS27 protocol version has not changed and that the
+	// tx type and encoding are supported
+	if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, connectionHops, proposedMetadata); err != nil {
+		return "", errorsmod.Wrap(err, "invalid upgrade metadata")
 	}
 
 	// the interchain account address on the host chain
 	// must remain the same after the upgrade.
-	if currentMetadata.Address != metadata.Address {
+	if currentMetadata.Address != proposedMetadata.Address {
 		return "", errorsmod.Wrap(icatypes.ErrInvalidAccountAddress, "interchain account address cannot be changed")
 	}
 
-	if currentMetadata.ControllerConnectionId != connectionHops[0] {
-		return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed connection hop must not change")
+	if currentMetadata.ControllerConnectionId != proposedMetadata.ControllerConnectionId {
+		return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed controller connection ID must not change")
+	}
+
+	if currentMetadata.HostConnectionId != proposedMetadata.HostConnectionId {
+		return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed host connection ID must not change")
 	}
 
 	return version, nil
 }
 
 // OnChanUpgradeAck implements the ack setup of the channel upgrade handshake.
+// The upgrade ack callback must verify the proposed changes to the channel version.
+// Within the channel 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:
+// - controller connectionID
+// - host connectionID
+// - interchain account address
+// - ICS27 protocol version
 func (k Keeper) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
 	if strings.TrimSpace(counterpartyVersion) == "" {
 		return errorsmod.Wrap(channeltypes.ErrInvalidChannelVersion, "counterparty version cannot be empty")
 	}
 
-	metadata, err := icatypes.MetadataFromVersion(counterpartyVersion)
+	proposedMetadata, err := icatypes.MetadataFromVersion(counterpartyVersion)
 	if err != nil {
 		return err
 	}
@@ -195,27 +239,30 @@ func (k Keeper) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpart
 		return err
 	}
 
-	// the interchain account address on the host chain
-	// must remain the same after the upgrade.
-	if currentMetadata.Address != metadata.Address {
-		return errorsmod.Wrap(icatypes.ErrInvalidAccountAddress, "address cannot be changed")
+	channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID)
+	if !found {
+		return errorsmod.Wrapf(channeltypes.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID)
 	}
 
-	if currentMetadata.ControllerConnectionId != metadata.ControllerConnectionId {
-		return errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed controller connection ID must not change")
+	// ValidateControllerMetadata will ensure the ICS27 protocol version has not changed and that the
+	// tx type and encoding are supported. Note, we pass in the current channel connection hops. The upgrade init
+	// step will verify that the proposed connection hops will not change.
+	if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, channel.ConnectionHops, proposedMetadata); err != nil {
+		return errorsmod.Wrap(err, "invalid upgrade metadata")
 	}
 
-	if currentMetadata.HostConnectionId != metadata.HostConnectionId {
-		return errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed host connection ID must not change")
+	// the interchain account address on the host chain
+	// must remain the same after the upgrade.
+	if currentMetadata.Address != proposedMetadata.Address {
+		return errorsmod.Wrap(icatypes.ErrInvalidAccountAddress, "address cannot be changed")
 	}
 
-	channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID)
-	if !found {
-		return errorsmod.Wrapf(channeltypes.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID)
+	if currentMetadata.ControllerConnectionId != proposedMetadata.ControllerConnectionId {
+		return errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed controller connection ID must not change")
 	}
 
-	if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, channel.ConnectionHops, metadata); err != nil {
-		return err
+	if currentMetadata.HostConnectionId != proposedMetadata.HostConnectionId {
+		return errorsmod.Wrap(connectiontypes.ErrInvalidConnection, "proposed host connection ID must not change")
 	}
 
 	return nil

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

@@ -6,6 +6,7 @@ import (
 	connectiontypes "github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"
 	channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
 	host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
+	ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
 	ibctesting "github.com/cosmos/ibc-go/v8/testing"
 )
 
@@ -478,12 +479,26 @@ func (suite *KeeperTestSuite) TestOnChanCloseConfirm() {
 }
 
 func (suite *KeeperTestSuite) TestOnChanUpgradeInit() {
+	const (
+		invalidVersion = "invalid-version"
+	)
+
 	var (
 		path     *ibctesting.Path
 		metadata icatypes.Metadata
+		version  string
 		order    channeltypes.Order
 	)
 
+	// updateMetadata is a helper function which modifies the metadata stored in the channel version
+	// and marshals it into a string to pass to OnChanUpgradeInit 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)
+		version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
+	}
+
 	testCases := []struct {
 		name     string
 		malleate func()
@@ -495,48 +510,104 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeInit() {
 			nil,
 		},
 		{
-			name: "failure: change ICA address",
+			name: "failure: invalid order",
 			malleate: func() {
-				metadata.Address = TestOwnerAddress
+				order = channeltypes.UNORDERED
 			},
-			expError: icatypes.ErrInvalidAccountAddress,
+			expError: channeltypes.ErrInvalidChannelOrdering,
 		},
 		{
-			name: "failure: change controller connection id",
+			name: "failure: connectionID not found",
 			malleate: func() {
-				metadata.ControllerConnectionId = differentConnectionID
+				// channelID is provided via the endpoint channelID
+				path.EndpointA.ChannelID = "invalid channel"
 			},
-			expError: connectiontypes.ErrInvalidConnection,
+			expError: channeltypes.ErrChannelNotFound,
 		},
 		{
-			name: "failure: change host connection id",
+			name: "failure: invalid proposed connectionHops",
 			malleate: func() {
-				metadata.HostConnectionId = differentConnectionID
+				// connection hops is provided via endpoint connectionID
+				path.EndpointA.ConnectionID = differentConnectionID
 			},
-			expError: connectiontypes.ErrInvalidConnection,
+			expError: channeltypes.ErrInvalidUpgrade,
+		},
+		{
+			name: "failure: empty version",
+			malleate: func() {
+				version = ""
+			},
+			expError: icatypes.ErrInvalidVersion,
 		},
 		{
 			name: "failure: cannot decode version string",
 			malleate: func() {
-				channel := path.EndpointA.GetChannel()
-				channel.Version = "invalid-metadata-string"
-				path.EndpointA.SetChannel(channel)
+				version = invalidVersion
 			},
 			expError: icatypes.ErrUnknownDataType,
 		},
 		{
-			name: "failure: invalid connection hops",
+			name: "failure: cannot decode self version string",
 			malleate: func() {
-				path.EndpointA.ConnectionID = differentConnectionID
+				ch := path.EndpointA.GetChannel()
+				ch.Version = invalidVersion
+				path.EndpointA.SetChannel(ch)
 			},
-			expError: connectiontypes.ErrConnectionNotFound,
+			expError: icatypes.ErrUnknownDataType,
 		},
 		{
-			name: "failure: invalid order",
+			name: "failure: failed controller metadata validation, invalid encoding",
 			malleate: func() {
-				order = channeltypes.UNORDERED
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.Encoding = "invalid-encoding"
+				})
 			},
-			expError: channeltypes.ErrInvalidChannelOrdering,
+			expError: icatypes.ErrInvalidCodec,
+		},
+		{
+			name: "failure: failed controller metadata validation, invalid tx type",
+			malleate: func() {
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.TxType = "invalid-tx-type"
+				})
+			},
+			expError: icatypes.ErrUnknownDataType,
+		},
+		{
+			name: "failure: failed controller metadata validation, invalid interchain account version",
+			malleate: func() {
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.Version = "invalid-interchain-account-version"
+				})
+			},
+			expError: icatypes.ErrInvalidVersion,
+		},
+		{
+			name: "failure: interchain account address changed",
+			malleate: func() {
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.Address = TestOwnerAddress // use valid address
+				})
+			},
+			expError: icatypes.ErrInvalidAccountAddress,
+		},
+		{
+			name: "failure: controller connection ID has changed",
+			malleate: func() {
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.ControllerConnectionId = differentConnectionID
+				})
+			},
+			expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the controller connection identifier are unreachable
+		},
+		{
+			name: "failure: host connection ID has changed",
+			malleate: func() {
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.HostConnectionId = differentConnectionID
+				})
+			},
+			expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the host connection identifier are unreachable
 		},
 	}
 
@@ -563,9 +634,12 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeInit() {
 			// this is the actual change to the version.
 			metadata.Encoding = icatypes.EncodingProto3JSON
 
-			tc.malleate() // malleate mutates test data
+			path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
+			path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
 
-			version := string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
+			version = path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version
+
+			tc.malleate() // malleate mutates test data
 
 			upgradeVersion, err := path.EndpointA.Chain.GetSimApp().ICAControllerKeeper.OnChanUpgradeInit(
 				path.EndpointA.Chain.GetContext(),
@@ -642,7 +716,24 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeAck() {
 			expError: icatypes.ErrUnknownDataType,
 		},
 		{
-			name: "failure: invalid tx type",
+			name: "failure: channel not found",
+			malleate: func() {
+				// channelID is provided via the endpoint channelID
+				path.EndpointA.ChannelID = "invalid channel"
+			},
+			expError: ibcerrors.ErrNotFound, // GetChannel error is unreachable
+		},
+		{
+			name: "failure: failed controller metadata validation, invalid encoding",
+			malleate: func() {
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.Encoding = "invalid-encoding"
+				})
+			},
+			expError: icatypes.ErrInvalidCodec,
+		},
+		{
+			name: "failure: failed controller metadata validation, invalid tx type",
 			malleate: func() {
 				updateMetadata(func(metadata *icatypes.Metadata) {
 					metadata.TxType = "invalid-tx-type"
@@ -651,10 +742,19 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeAck() {
 			expError: icatypes.ErrUnknownDataType,
 		},
 		{
-			name: "failure: interchain account address has changed",
+			name: "failure: failed controller metadata validation, invalid interchain account version",
+			malleate: func() {
+				updateMetadata(func(metadata *icatypes.Metadata) {
+					metadata.Version = "invalid-interchain-account-version"
+				})
+			},
+			expError: icatypes.ErrInvalidVersion,
+		},
+		{
+			name: "failure: interchain account address changed",
 			malleate: func() {
 				updateMetadata(func(metadata *icatypes.Metadata) {
-					metadata.Address = "different-address"
+					metadata.Address = TestOwnerAddress // use valid address
 				})
 			},
 			expError: icatypes.ErrInvalidAccountAddress,
@@ -663,19 +763,19 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeAck() {
 			name: "failure: controller connection ID has changed",
 			malleate: func() {
 				updateMetadata(func(metadata *icatypes.Metadata) {
-					metadata.ControllerConnectionId = "different-connection-id"
+					metadata.ControllerConnectionId = differentConnectionID
 				})
 			},
-			expError: connectiontypes.ErrInvalidConnectionIdentifier,
+			expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the controller identifier are unreachable
 		},
 		{
 			name: "failure: host connection ID has changed",
 			malleate: func() {
 				updateMetadata(func(metadata *icatypes.Metadata) {
-					metadata.HostConnectionId = "different-host-id"
+					metadata.HostConnectionId = differentConnectionID
 				})
 			},
-			expError: connectiontypes.ErrInvalidConnectionIdentifier,
+			expError: connectiontypes.ErrInvalidConnection, // the explicit checks on the host identifier are unreachable
 		},
 	}