Skip to content

Commit

Permalink
Defensive checks for active channel (#785)
Browse files Browse the repository at this point in the history
* 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>
  • Loading branch information
seantking and colin-axner authored Jan 31, 2022
1 parent d8c74f4 commit 25fb89d
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 9 additions & 1 deletion modules/apps/27-interchain-accounts/host/keeper/handshake.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down
11 changes: 11 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/handshake_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
17 changes: 17 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
{
Expand Down Expand Up @@ -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)
}

Expand Down
15 changes: 8 additions & 7 deletions modules/apps/27-interchain-accounts/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)

0 comments on commit 25fb89d

Please sign in to comment.