diff --git a/x/ibc/applications/transfer/module.go b/x/ibc/applications/transfer/module.go index d14637614..bf324e2c9 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,6 +183,42 @@ func (am AppModule) WeightedOperations(_ module.SimulationState) []simtypes.Weig //____________________________________________________________________________ +// 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, + portID string, + channelID string, + 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 := 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) + } + return nil +} + // OnChanOpenInit implements the IBCModule interface func (am AppModule) OnChanOpenInit( ctx sdk.Context, @@ -193,18 +230,8 @@ func (am AppModule) OnChanOpenInit( counterparty channeltypes.Counterparty, version 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 } // Claim channel capability passed back by IBC module @@ -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 b6ce1077c..d2acfb404 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 e3311bed3..07cba1949 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 1b0e45b9c..c156af3fd 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] }