Add an IBC prefix to transfer escrow addresses (#7967)

* add IBC prefix to escrow addresses * use ADR 028 AddressHash construction * remove extra space * apply @AdityaSripal suggestion; prefix from ibc -> ibc-transfer-escrow-prefix * Use alternative 1 proposed by @andrey-kuprianov * anticpate necessary build fix * add escrow address issue link into module.go * Update x/ibc/applications/transfer/types/keys.go Co-authored-by: Andrey Kuprianov <[email protected]> * Update x/ibc/applications/transfer/types/keys.go Co-authored-by: Andrey Kuprianov <[email protected]> * apply @andrey-kuprianov review suggestions Deduplicate code into a helper function as suggested. Add unit tests for max transfer channels test. Co-authored-by: Andrey Kuprianov <[email protected]>
cosmos · Nov 26, 2020 · ed9fc05 · ed9fc05
1 parent 512b533
commit ed9fc05
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 26 deletions.
diff --git 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,30 +183,56 @@ 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)
 	}
 
 	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
@@ -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
@@ -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
@@ -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]
 }