From 4a021a3c90fc9eb53e978597c7a9ee618d7def8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 4 Jan 2024 21:14:09 +0100 Subject: [PATCH] refactor: use explicit checks in ica controller upgrade handlers (#5472) * refactor: explicit checks in upgrade init and upgrade ack for ica controller Updated UpgradeInit logic Updated UpgradeAck logic Refactored UpgradeInit tests * test: complete upgrade ack tests * test: add code cov * lint appease * nit: use correct error type * nit comment change * fix typo --------- Co-authored-by: Cian Hatton Co-authored-by: Carlos Rodriguez --- .../controller/keeper/handshake.go | 95 ++++++++--- .../controller/keeper/handshake_test.go | 154 +++++++++++++++--- 2 files changed, 198 insertions(+), 51 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake.go index 4888ebc3f87..17234cc62a9 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go +++ b/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 diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go index e34d91481a2..b2fd351cc06 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go +++ b/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 version 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 }, }