diff --git a/x/ibc/applications/transfer/module.go b/x/ibc/applications/transfer/module.go index d14637614b02..bf324e2c9b36 100644 --- a/x/ibc/applications/transfer/module.go +++ b/x/ibc/applications/transfer/module.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "math" "math/rand" "github.com/grpc-ecosystem/grpc-gateway/runtime" @@ -182,23 +183,32 @@ func (am AppModule) WeightedOperations(_ module.SimulationState) []simtypes.Weig //____________________________________________________________________________ -// OnChanOpenInit implements the IBCModule interface -func (am AppModule) OnChanOpenInit( +// ValidateTransferChannelParams does validation of a newly created transfer channel. A transfer +// channel must be UNORDERED, use the correct port (by default 'transfer'), and use the current +// supported version. Only 2^32 channels are allowed to be created. +func ValidateTransferChannelParams( ctx sdk.Context, + keeper keeper.Keeper, order channeltypes.Order, - connectionHops []string, portID string, channelID string, - chanCap *capabilitytypes.Capability, - counterparty channeltypes.Counterparty, version string, ) error { + // NOTE: for escrow address security only 2^32 channels are allowed to be created + // Issue: https://github.com/cosmos/cosmos-sdk/issues/7737 + channelSequence, err := channeltypes.ParseChannelSequence(channelID) + if err != nil { + return err + } + if channelSequence > math.MaxUint32 { + return sdkerrors.Wrapf(types.ErrMaxTransferChannels, "channel sequence %d is greater than max allowed transfer channels %d", channelSequence, math.MaxUint32) + } if order != channeltypes.UNORDERED { return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s ", channeltypes.UNORDERED, order) } // Require portID is the portID transfer module is bound to - boundPort := am.keeper.GetPort(ctx) + boundPort := keeper.GetPort(ctx) if boundPort != portID { return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "invalid port: %s, expected %s", portID, boundPort) } @@ -206,6 +216,23 @@ func (am AppModule) OnChanOpenInit( if version != types.Version { return sdkerrors.Wrapf(types.ErrInvalidVersion, "got %s, expected %s", version, types.Version) } + return nil +} + +// OnChanOpenInit implements the IBCModule interface +func (am AppModule) OnChanOpenInit( + ctx sdk.Context, + order channeltypes.Order, + connectionHops []string, + portID string, + channelID string, + chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, + version string, +) error { + if err := ValidateTransferChannelParams(ctx, am.keeper, order, portID, channelID, version); err != nil { + return err + } // Claim channel capability passed back by IBC module if err := am.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { @@ -227,18 +254,8 @@ func (am AppModule) OnChanOpenTry( version, counterpartyVersion string, ) error { - if order != channeltypes.UNORDERED { - return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s ", channeltypes.UNORDERED, order) - } - - // Require portID is the portID transfer module is bound to - boundPort := am.keeper.GetPort(ctx) - if boundPort != portID { - return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "invalid port: %s, expected %s", portID, boundPort) - } - - if version != types.Version { - return sdkerrors.Wrapf(types.ErrInvalidVersion, "got: %s, expected %s", version, types.Version) + if err := ValidateTransferChannelParams(ctx, am.keeper, order, portID, channelID, version); err != nil { + return err } if counterpartyVersion != types.Version { diff --git a/x/ibc/applications/transfer/module_test.go b/x/ibc/applications/transfer/module_test.go index b6ce1077c6a0..d2acfb404311 100644 --- a/x/ibc/applications/transfer/module_test.go +++ b/x/ibc/applications/transfer/module_test.go @@ -1,6 +1,8 @@ package transfer_test import ( + "math" + capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" "github.com/cosmos/cosmos-sdk/x/ibc/applications/transfer/types" channeltypes "github.com/cosmos/cosmos-sdk/x/ibc/core/04-channel/types" @@ -26,6 +28,11 @@ func (suite *TransferTestSuite) TestOnChanOpenInit() { { "success", func() {}, true, }, + { + "max channels reached", func() { + testChannel.ID = channeltypes.FormatChannelIdentifier(math.MaxUint32 + 1) + }, false, + }, { "invalid order - ORDERED", func() { channel.Ordering = channeltypes.ORDERED @@ -109,6 +116,11 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() { { "success", func() {}, true, }, + { + "max channels reached", func() { + testChannel.ID = channeltypes.FormatChannelIdentifier(math.MaxUint32 + 1) + }, false, + }, { "capability already claimed in INIT should pass", func() { err := suite.chainA.App.ScopedTransferKeeper.ClaimCapability(suite.chainA.GetContext(), chanCap, host.ChannelCapabilityPath(testChannel.PortID, testChannel.ID)) diff --git a/x/ibc/applications/transfer/types/errors.go b/x/ibc/applications/transfer/types/errors.go index e3311bed31c9..07cba1949157 100644 --- a/x/ibc/applications/transfer/types/errors.go +++ b/x/ibc/applications/transfer/types/errors.go @@ -13,4 +13,5 @@ var ( ErrTraceNotFound = sdkerrors.Register(ModuleName, 6, "denomination trace not found") ErrSendDisabled = sdkerrors.Register(ModuleName, 7, "fungible token transfers from this chain are disabled") ErrReceiveDisabled = sdkerrors.Register(ModuleName, 8, "fungible token transfers to this chain are disabled") + ErrMaxTransferChannels = sdkerrors.Register(ModuleName, 9, "max transfer channels") ) diff --git a/x/ibc/applications/transfer/types/keys.go b/x/ibc/applications/transfer/types/keys.go index 1b0e45b9c7db..c156af3fd88d 100644 --- a/x/ibc/applications/transfer/types/keys.go +++ b/x/ibc/applications/transfer/types/keys.go @@ -1,10 +1,9 @@ package types import ( + "crypto/sha256" "fmt" - "github.com/tendermint/tendermint/crypto" - sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -39,11 +38,18 @@ var ( DenomTraceKey = []byte{0x02} ) -// GetEscrowAddress returns the escrow address for the specified channel -// -// CONTRACT: this assumes that there's only one bank bridge module that owns the -// port associated with the channel ID so that the address created is actually -// unique. +// GetEscrowAddress returns the escrow address for the specified channel. +// The escrow address follows the format as outlined in ADR 028: +// https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-028-public-key-addresses.md func GetEscrowAddress(portID, channelID string) sdk.AccAddress { - return sdk.AccAddress(crypto.AddressHash([]byte(fmt.Sprintf("%s/%s", portID, channelID)))) + // a slash is used to create domain separation between port and channel identifiers to + // prevent address collisions between escrow addresses created for different channels + contents := fmt.Sprintf("%s/%s", portID, channelID) + + // ADR 028 AddressHash construction + preImage := []byte(Version) + preImage = append(preImage, 0) + preImage = append(preImage, contents...) + hash := sha256.Sum256(preImage) + return hash[:20] }