Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an IBC prefix to transfer escrow addresses #7967

Merged
merged 22 commits into from
Nov 26, 2020
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
bb7498f
add IBC prefix to escrow addresses
colin-axner Nov 18, 2020
b7227b4
fix merge conflict
colin-axner Nov 18, 2020
6e4d099
use ADR 028 AddressHash construction
colin-axner Nov 19, 2020
b090898
Merge branch 'master' into colin/7737-prefix-escrow-addr
colin-axner Nov 19, 2020
af76261
remove extra space
colin-axner Nov 19, 2020
8cb1fee
Merge branch 'colin/7737-prefix-escrow-addr' of github.com:cosmos/cos…
colin-axner Nov 19, 2020
5fce47d
apply @AdityaSripal suggestion; prefix from ibc -> ibc-transfer-escro…
colin-axner Nov 19, 2020
7bffc31
Merge branch 'master' into colin/7737-prefix-escrow-addr
colin-axner Nov 19, 2020
91b68dd
Use alternative 1 proposed by @andrey-kuprianov
colin-axner Nov 24, 2020
ab19a4a
anticpate necessary build fix
colin-axner Nov 24, 2020
c57493b
Merge branch 'colin/7737-prefix-escrow-addr' of github.com:cosmos/cos…
colin-axner Nov 24, 2020
6731918
Merge branch 'master' into colin/7737-prefix-escrow-addr
colin-axner Nov 24, 2020
e366e9c
Merge branch 'master' into colin/7737-prefix-escrow-addr
colin-axner Nov 24, 2020
111ed79
Merge branch 'master' into colin/7737-prefix-escrow-addr
colin-axner Nov 24, 2020
d730ae2
add escrow address issue link into module.go
colin-axner Nov 24, 2020
3559e72
Merge branch 'colin/7737-prefix-escrow-addr' of github.com:cosmos/cos…
colin-axner Nov 24, 2020
c697887
Merge branch 'master' into colin/7737-prefix-escrow-addr
colin-axner Nov 25, 2020
c5ef4f9
Merge branch 'master' into colin/7737-prefix-escrow-addr
colin-axner Nov 25, 2020
c9e3a18
Update x/ibc/applications/transfer/types/keys.go
colin-axner Nov 26, 2020
272dd2a
Update x/ibc/applications/transfer/types/keys.go
colin-axner Nov 26, 2020
8a99065
Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/7737…
colin-axner Nov 26, 2020
0677e09
apply @andrey-kuprianov review suggestions
colin-axner Nov 26, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions x/ibc/applications/transfer/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"fmt"
"math"
"math/rand"

"github.com/grpc-ecosystem/grpc-gateway/runtime"
Expand Down Expand Up @@ -193,6 +194,14 @@ func (am AppModule) OnChanOpenInit(
counterparty channeltypes.Counterparty,
version string,
) error {
// NOTE: for escrow address security only 2^32 channels are allowed to be created
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
andrey-kuprianov marked this conversation as resolved.
Show resolved Hide resolved
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)
}
Expand Down Expand Up @@ -227,6 +236,14 @@ func (am AppModule) OnChanOpenTry(
version,
counterpartyVersion string,
) error {
// NOTE: for escrow address security only 2^32 channels are allowed to be created
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)
}
Expand Down
1 change: 1 addition & 0 deletions x/ibc/applications/transfer/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)
26 changes: 18 additions & 8 deletions x/ibc/applications/transfer/types/keys.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
package types

import (
"crypto/sha256"
"fmt"

"github.com/tendermint/tendermint/crypto"

sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand All @@ -30,6 +29,10 @@ const (

// DenomPrefix is the prefix used for internal SDK coin representation.
DenomPrefix = "ibc"

// EscrowPrefix is the prefix used for IBC escrow addresses to avoid address collisions
// with Public Key addresses.
EscrowPrefix = "ibc-v1"
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
)

var (
Expand All @@ -39,11 +42,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(EscrowPrefix)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm what is the precise difference between inserting a prefix here and having a long fixed port name (security-wise)? Is there any?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a UI thing, if we make the port a long fixed name, then denominations would be long, though it seems apparent this information generally won't be user facing. There is also potential for design misuse. Port identifiers are allowed to be as small as 2 characters.

Should I make the escrow prefix longer?

Copy link
Contributor Author

@colin-axner colin-axner Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the length of the prefix increase the difficulty? If not, then we can just use the port as the prefix, the channel as the content and the 0 as the separator (ie still using ADR 028 format)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the length of the prefix does increase the difficulty (of-brute forcing an ECDSA key) the same way a longer port name does. Does ADR 28 analyse this concern in detail? As far as I can tell, it does not, but it should - consider the example of having a 1-character prefix, then every 1/255 (or so) ECDSA keys will have that prefix (although it needs to also have a / somewhere, but that's just then 1/255^2 which is still probably feasible).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any discussion of this either.

Any recommended prefix length to use?

I think using a prefix separate from the port identifier is a fine solution. It doesn't have any negatives from what I can see and reduces the likelihood of a chain accidentally opening themselves up to an attack

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe even with the current construction on master, using a fixed, long portID makes this attack difficult already. From my understanding, the reason we wanted this feature is to remove any dependence between choice of port-id and security of the escrow address, since this would be non-obvious to a ibc application developer.

Which I agree with. Let's try to reduce the number of quirks developers have to keep in mind. Using a standard length prefix separate from portID makes sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the conclusion. I think this may be a point to clarify in ADR 028.

colin-axner marked this conversation as resolved.
Show resolved Hide resolved
preImage = append(preImage, 0)
preImage = append(preImage, contents...)
hash := sha256.Sum256(preImage)
return hash[:20]
}