diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake.go b/modules/apps/27-interchain-accounts/host/keeper/handshake.go index 819abd89426..4798563f7c1 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake.go @@ -132,21 +132,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 } @@ -156,16 +180,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") } diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go index c14ac85a576..bb69322d40a 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go +++ b/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 order - UNORDERED", func() { @@ -433,6 +438,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() @@ -450,6 +464,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() { @@ -476,37 +505,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, }, } diff --git a/modules/apps/27-interchain-accounts/host/keeper/keeper.go b/modules/apps/27-interchain-accounts/host/keeper/keeper.go index be0cfc8a0b5..92055909a00 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/keeper.go +++ b/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) diff --git a/modules/apps/27-interchain-accounts/types/metadata.go b/modules/apps/27-interchain-accounts/types/metadata.go index 61c077a46f0..a63e4a93083 100644 --- a/modules/apps/27-interchain-accounts/types/metadata.go +++ b/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)