diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake.go index 5b1fc619f9e..26a90878abf 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/handshake.go @@ -1,6 +1,7 @@ package keeper import ( + "fmt" "strings" sdk "github.com/cosmos/cosmos-sdk/types" @@ -49,9 +50,20 @@ func (k Keeper) OnChanOpenInit( return err } - activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionHops[0], portID) + activeChannelID, found := k.GetActiveChannelID(ctx, connectionHops[0], portID) if found { - return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID) + channel, found := k.channelKeeper.GetChannel(ctx, portID, activeChannelID) + if !found { + panic(fmt.Sprintf("active channel mapping set for %s but channel does not exist in channel store", activeChannelID)) + } + + if channel.State == channeltypes.OPEN { + return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s is already OPEN", activeChannelID, portID) + } + + if !icatypes.IsPreviousMetadataEqual(channel.Version, metadata) { + return sdkerrors.Wrap(icatypes.ErrInvalidVersion, "previous active channel metadata does not match provided version") + } } 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 0f1e1af41a0..61c8a092a81 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go @@ -30,6 +30,51 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() { }, true, }, + { + "success - previous active channel closed", + func() { + suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + channel := channeltypes.Channel{ + State: channeltypes.CLOSED, + Ordering: channeltypes.ORDERED, + Counterparty: counterparty, + ConnectionHops: []string{path.EndpointA.ConnectionID}, + Version: TestVersion, + } + + path.EndpointA.SetChannel(channel) + }, + true, + }, + { + "invalid metadata - previous metadata is different", + func() { + // set active channel to closed + suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + closedChannel := channeltypes.Channel{ + State: channeltypes.CLOSED, + Ordering: channeltypes.ORDERED, + Counterparty: counterparty, + ConnectionHops: []string{path.EndpointA.ConnectionID}, + Version: TestVersion, + } + + path.EndpointA.SetChannel(closedChannel) + + // modify metadata + metadata.Version = "ics27-2" + + versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata) + suite.Require().NoError(err) + + channel.Version = string(versionBytes) + }, + false, + }, { "invalid order - UNORDERED", func() { diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake.go b/modules/apps/27-interchain-accounts/host/keeper/handshake.go index 48b3570dd67..11a7c7a378e 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake.go @@ -1,6 +1,7 @@ package keeper import ( + "fmt" "strings" sdk "github.com/cosmos/cosmos-sdk/types" @@ -47,8 +48,20 @@ func (k Keeper) OnChanOpenTry( return "", err } - if activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionHops[0], counterparty.PortId); found { - return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID) + activeChannelID, found := k.GetActiveChannelID(ctx, connectionHops[0], counterparty.PortId) + if found { + channel, found := k.channelKeeper.GetChannel(ctx, portID, activeChannelID) + if !found { + panic(fmt.Sprintf("active channel mapping set for %s but channel does not exist in channel store", activeChannelID)) + } + + if channel.State == channeltypes.OPEN { + return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s is already OPEN", activeChannelID, portID) + } + + if !icatypes.IsPreviousMetadataEqual(channel.Version, metadata) { + return "", sdkerrors.Wrap(icatypes.ErrInvalidVersion, "previous active channel metadata does not match provided version") + } } // On the host chain the capability may only be claimed during the OnChanOpenTry @@ -83,9 +96,9 @@ func (k Keeper) OnChanOpenConfirm( } // It is assumed the controller chain will not allow multiple active channels to be created for the same connectionID/portID - // If the controller chain does allow multiple active channels to be created for the same connectionID/portID, + // If the controller chain does allow multiple active channels to be created for the same connectionID/portID, // disallowing overwriting the current active channel guarantees the channel can no longer be used as the controller - // and host will disagree on what the currently active channel is + // and host will disagree on what the currently active channel is k.SetActiveChannelID(ctx, channel.ConnectionHops[0], channel.Counterparty.PortId, channelID) return nil 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 e920a55f0df..0cac2912ccb 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go @@ -30,6 +30,36 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { }, true, }, + { + "success - reopening closed active channel", + func() { + // create a new channel and set it in state + ch := channeltypes.NewChannel(channeltypes.CLOSED, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointA.ConnectionID}, TestVersion) + suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, ch) + + // set the active channelID in state + suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID) + }, true, + }, + { + "invalid metadata - previous metadata is different", + func() { + // create a new channel and set it in state + ch := channeltypes.NewChannel(channeltypes.CLOSED, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointA.ConnectionID}, TestVersion) + suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, ch) + + // set the active channelID in state + suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID) + + // modify metadata + metadata.Version = "ics27-2" + + versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata) + suite.Require().NoError(err) + + path.EndpointA.ChannelConfig.Version = string(versionBytes) + }, false, + }, { "invalid order - UNORDERED", func() { @@ -140,7 +170,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { func() { // create a new channel and set it in state ch := channeltypes.NewChannel(channeltypes.OPEN, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointA.ConnectionID}, ibctesting.DefaultChannelVersion) - suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID, ch) + suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, ch) // set the active channelID in state suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID) diff --git a/modules/apps/27-interchain-accounts/types/metadata.go b/modules/apps/27-interchain-accounts/types/metadata.go index 67588e2d809..3a7eae51cdf 100644 --- a/modules/apps/27-interchain-accounts/types/metadata.go +++ b/modules/apps/27-interchain-accounts/types/metadata.go @@ -27,6 +27,21 @@ func NewMetadata(version, controllerConnectionID, hostConnectionID, accAddress, } } +// IsPreviousMetadataEqual compares a metadata to a previous version string set in a channel struct. +// It ensures all fields are equal except the Address string +func IsPreviousMetadataEqual(previousVersion string, metadata Metadata) bool { + var previousMetadata Metadata + if err := ModuleCdc.UnmarshalJSON([]byte(previousVersion), &previousMetadata); err != nil { + return false + } + + return (previousMetadata.Version == metadata.Version && + previousMetadata.ControllerConnectionId == metadata.ControllerConnectionId && + previousMetadata.HostConnectionId == metadata.HostConnectionId && + previousMetadata.Encoding == metadata.Encoding && + previousMetadata.TxType == metadata.TxType) +} + // ValidateControllerMetadata performs validation of the provided ICS27 controller metadata parameters func ValidateControllerMetadata(ctx sdk.Context, channelKeeper ChannelKeeper, connectionHops []string, metadata Metadata) error { if !isSupportedEncoding(metadata.Encoding) { diff --git a/modules/apps/27-interchain-accounts/types/metadata_test.go b/modules/apps/27-interchain-accounts/types/metadata_test.go index 1c53b8f7126..05a1b457c38 100644 --- a/modules/apps/27-interchain-accounts/types/metadata_test.go +++ b/modules/apps/27-interchain-accounts/types/metadata_test.go @@ -5,6 +5,127 @@ import ( ibctesting "github.com/cosmos/ibc-go/v3/testing" ) +// use TestVersion as metadata being compared against +func (suite *TypesTestSuite) TestIsPreviousMetadataEqual() { + + var ( + metadata types.Metadata + previousVersion string + ) + + testCases := []struct { + name string + malleate func() + expEqual bool + }{ + { + "success", + func() { + versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata) + suite.Require().NoError(err) + previousVersion = string(versionBytes) + }, + true, + }, + { + "success with empty account address", + func() { + metadata.Address = "" + + versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata) + suite.Require().NoError(err) + previousVersion = string(versionBytes) + }, + true, + }, + { + "cannot decode previous version", + func() { + previousVersion = "invalid previous version" + }, + false, + }, + { + "unequal encoding format", + func() { + metadata.Encoding = "invalid-encoding-format" + + versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata) + suite.Require().NoError(err) + previousVersion = string(versionBytes) + }, + false, + }, + { + "unequal transaction type", + func() { + metadata.TxType = "invalid-tx-type" + + versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata) + suite.Require().NoError(err) + previousVersion = string(versionBytes) + }, + false, + }, + { + "unequal controller connection", + func() { + metadata.ControllerConnectionId = "connection-10" + + versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata) + suite.Require().NoError(err) + previousVersion = string(versionBytes) + }, + false, + }, + { + "unequal host connection", + func() { + metadata.HostConnectionId = "connection-10" + + versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata) + suite.Require().NoError(err) + previousVersion = string(versionBytes) + }, + false, + }, + { + "unequal version", + func() { + metadata.Version = "invalid version" + + versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata) + suite.Require().NoError(err) + previousVersion = string(versionBytes) + }, + false, + }, + } + + for _, tc := range testCases { + tc := tc + suite.Run(tc.name, func() { + suite.SetupTest() // reset + + path := ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.SetupConnections(path) + + expectedMetadata := types.NewMetadata(types.Version, ibctesting.FirstConnectionID, ibctesting.FirstConnectionID, TestOwnerAddress, types.EncodingProtobuf, types.TxTypeSDKMultiMsg) + metadata = expectedMetadata // default success case + + tc.malleate() // malleate mutates test data + + equal := types.IsPreviousMetadataEqual(previousVersion, expectedMetadata) + + if tc.expEqual { + suite.Require().True(equal) + } else { + suite.Require().False(equal) + } + }) + } +} + func (suite *TypesTestSuite) TestValidateControllerMetadata() { var metadata types.Metadata