Defensive checks for active channel (cosmos#785)

* 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 <[email protected]> Co-authored-by: colin axnér <[email protected]>
notional-labs · Jan 31, 2022 · 25fb89d · 25fb89d
1 parent d8c74f4
commit 25fb89d
Show file tree

Hide file tree

Showing 8 changed files with 64 additions and 13 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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")
 )