From 25fb89dce05e658631497889a3485fda8f83644c Mon Sep 17 00:00:00 2001 From: Sean King Date: Mon, 31 Jan 2022 16:01:48 +0100 Subject: [PATCH] Defensive checks for active channel (#785) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: Adding check onChanOpenAck to check for active, open channel * Add defensive check of active channel to OnChanOpenTry * fix: use counterparty.PortId in active channel check * update: change base error for active channel check * comment: add comment explaining overwrite of active channel * fix: change err type * fix: updating to use counterparty PortID * fix: tests * Update modules/apps/27-interchain-accounts/host/keeper/handshake.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> --- .../controller/keeper/handshake.go | 7 +++++-- .../controller/keeper/handshake_test.go | 11 +++++++++++ .../host/keeper/genesis_test.go | 2 +- .../host/keeper/handshake.go | 10 +++++++++- .../host/keeper/handshake_test.go | 11 +++++++++++ .../host/keeper/keeper.go | 17 +++++++++++++++++ .../host/keeper/keeper_test.go | 4 ++-- .../apps/27-interchain-accounts/types/errors.go | 15 ++++++++------- 8 files changed, 64 insertions(+), 13 deletions(-) diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake.go index 533bb8d89da..5b1fc619f9e 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/handshake.go @@ -9,7 +9,6 @@ import ( icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" - porttypes "github.com/cosmos/ibc-go/v3/modules/core/05-port/types" ) // OnChanOpenInit performs basic validation of channel initialization. @@ -52,7 +51,7 @@ func (k Keeper) OnChanOpenInit( activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionHops[0], portID) if found { - return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "existing active channel %s for portID %s", activeChannelID, portID) + return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID) } return nil @@ -79,6 +78,10 @@ func (k Keeper) OnChanOpenAck( return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata") } + if activeChannelID, found := k.GetOpenActiveChannel(ctx, metadata.ControllerConnectionId, portID); found { + return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID) + } + channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID) if !found { return sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID) 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 3a810a8790a..b08a6913aaf 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go @@ -247,6 +247,17 @@ func (suite *KeeperTestSuite) TestOnChanOpenAck() { }, false, }, + { + "active channel already set", + 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.EndpointB.ConnectionID}, ibctesting.DefaultChannelVersion) + suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, ch) + + // set the active channelID in state + suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + }, false, + }, } for _, tc := range testCases { diff --git a/modules/apps/27-interchain-accounts/host/keeper/genesis_test.go b/modules/apps/27-interchain-accounts/host/keeper/genesis_test.go index 78e447878a7..138d713cf67 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/genesis_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/genesis_test.go @@ -55,7 +55,7 @@ func (suite *KeeperTestSuite) TestExportGenesis() { genesisState := keeper.ExportGenesis(suite.chainB.GetContext(), suite.chainB.GetSimApp().ICAHostKeeper) suite.Require().Equal(path.EndpointB.ChannelID, genesisState.ActiveChannels[0].ChannelId) - suite.Require().Equal(path.EndpointB.ChannelConfig.PortID, genesisState.ActiveChannels[0].PortId) + suite.Require().Equal(path.EndpointA.ChannelConfig.PortID, genesisState.ActiveChannels[0].PortId) suite.Require().Equal(TestAccAddress.String(), genesisState.InterchainAccounts[0].AccountAddress) suite.Require().Equal(path.EndpointA.ChannelConfig.PortID, genesisState.InterchainAccounts[0].PortId) diff --git a/modules/apps/27-interchain-accounts/host/keeper/handshake.go b/modules/apps/27-interchain-accounts/host/keeper/handshake.go index ac8c863cc19..48b3570dd67 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake.go @@ -47,6 +47,10 @@ 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) + } + // On the host chain the capability may only be claimed during the OnChanOpenTry // The capability being claimed in OpenInit is for a controller chain (the port is different) if err := k.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { @@ -78,7 +82,11 @@ func (k Keeper) OnChanOpenConfirm( return sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID) } - k.SetActiveChannelID(ctx, channel.ConnectionHops[0], portID, channelID) + // 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, + // 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 + 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 beadf23b674..6f3dc0d25e1 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/handshake_test.go @@ -111,6 +111,17 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { }, false, }, + { + "active channel already set", + 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) + + // set the active channelID in state + suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID) + }, false, + }, } for _, tc := range testCases { diff --git a/modules/apps/27-interchain-accounts/host/keeper/keeper.go b/modules/apps/27-interchain-accounts/host/keeper/keeper.go index da3c23062c7..598c68789fc 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/keeper.go +++ b/modules/apps/27-interchain-accounts/host/keeper/keeper.go @@ -14,6 +14,7 @@ import ( "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/host/types" icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types" + channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" host "github.com/cosmos/ibc-go/v3/modules/core/24-host" ) @@ -102,6 +103,22 @@ func (k Keeper) GetActiveChannelID(ctx sdk.Context, connectionID, portID string) return string(store.Get(key)), true } +// GetOpenActiveChannel retrieves the active channelID from the store, keyed by the provided connectionID and portID & checks if the channel in question is in state OPEN +func (k Keeper) GetOpenActiveChannel(ctx sdk.Context, connectionID, portID string) (string, bool) { + channelID, found := k.GetActiveChannelID(ctx, connectionID, portID) + if !found { + return "", false + } + + channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID) + + if found && channel.State == channeltypes.OPEN { + return channelID, true + } + + return "", false +} + // GetAllActiveChannels returns a list of all active interchain accounts host channels and their associated connection and port identifiers func (k Keeper) GetAllActiveChannels(ctx sdk.Context) []icatypes.ActiveChannel { store := ctx.KVStore(k.storeKey) diff --git a/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go index c1a1487335d..2b38f6b5e84 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/keeper_test.go @@ -165,7 +165,7 @@ func (suite *KeeperTestSuite) TestGetAllActiveChannels() { expectedChannels := []icatypes.ActiveChannel{ { ConnectionId: ibctesting.FirstConnectionID, - PortId: path.EndpointB.ChannelConfig.PortID, + PortId: path.EndpointA.ChannelConfig.PortID, ChannelId: path.EndpointB.ChannelID, }, { @@ -223,7 +223,7 @@ func (suite *KeeperTestSuite) TestIsActiveChannel() { err := SetupICAPath(path, TestOwnerAddress) suite.Require().NoError(err) - isActive := suite.chainB.GetSimApp().ICAHostKeeper.IsActiveChannel(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointB.ChannelConfig.PortID) + isActive := suite.chainB.GetSimApp().ICAHostKeeper.IsActiveChannel(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID) suite.Require().True(isActive) } diff --git a/modules/apps/27-interchain-accounts/types/errors.go b/modules/apps/27-interchain-accounts/types/errors.go index e0a5c141de9..6ce9f2a9dea 100644 --- a/modules/apps/27-interchain-accounts/types/errors.go +++ b/modules/apps/27-interchain-accounts/types/errors.go @@ -14,11 +14,12 @@ var ( ErrInterchainAccountNotFound = sdkerrors.Register(ModuleName, 8, "interchain account not found") ErrInterchainAccountAlreadySet = sdkerrors.Register(ModuleName, 9, "interchain account is already set") ErrActiveChannelNotFound = sdkerrors.Register(ModuleName, 10, "no active channel for this owner") - ErrInvalidVersion = sdkerrors.Register(ModuleName, 11, "invalid interchain accounts version") - ErrInvalidAccountAddress = sdkerrors.Register(ModuleName, 12, "invalid account address") - ErrUnsupported = sdkerrors.Register(ModuleName, 13, "interchain account does not support this action") - ErrInvalidControllerPort = sdkerrors.Register(ModuleName, 14, "invalid controller port") - ErrInvalidHostPort = sdkerrors.Register(ModuleName, 15, "invalid host port") - ErrInvalidTimeoutTimestamp = sdkerrors.Register(ModuleName, 16, "timeout timestamp must be in the future") - ErrInvalidCodec = sdkerrors.Register(ModuleName, 17, "codec is not supported") + ErrActiveChannelAlreadySet = sdkerrors.Register(ModuleName, 11, "active channel already set for this owner") + ErrInvalidVersion = sdkerrors.Register(ModuleName, 12, "invalid interchain accounts version") + ErrInvalidAccountAddress = sdkerrors.Register(ModuleName, 13, "invalid account address") + ErrUnsupported = sdkerrors.Register(ModuleName, 14, "interchain account does not support this action") + ErrInvalidControllerPort = sdkerrors.Register(ModuleName, 15, "invalid controller port") + ErrInvalidHostPort = sdkerrors.Register(ModuleName, 16, "invalid host port") + ErrInvalidTimeoutTimestamp = sdkerrors.Register(ModuleName, 17, "timeout timestamp must be in the future") + ErrInvalidCodec = sdkerrors.Register(ModuleName, 18, "codec is not supported") )