From 04fa64a9b12519203786f5a34db37801a17bee96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 8 Feb 2022 18:04:15 +0100 Subject: [PATCH] chore: add defensive check to ensure metadata does not change when reopening an active channel (#847) ## Description closes: #795 --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing) - [ ] Updated relevant documentation (`docs/`) or specification (`x//spec/`) - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md` - [ ] Re-reviewed `Files changed` in the Github PR explorer - [ ] Review `Codecov Report` in the comment section below once CI passes (cherry picked from commit 482b7abb097488b3931637e43fa5a958e3af5e07) --- .../controller/keeper/handshake.go | 16 ++- .../controller/keeper/handshake_test.go | 45 +++++++ .../host/keeper/handshake.go | 21 ++- .../host/keeper/handshake_test.go | 32 ++++- .../27-interchain-accounts/types/metadata.go | 15 +++ .../types/metadata_test.go | 121 ++++++++++++++++++ 6 files changed, 243 insertions(+), 7 deletions(-) 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