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

Case sensitivity Gravity Bridge Token Denom #371

Open
thomas-nguy opened this issue Mar 22, 2022 · 2 comments
Open

Case sensitivity Gravity Bridge Token Denom #371

thomas-nguy opened this issue Mar 22, 2022 · 2 comments
Assignees

Comments

@thomas-nguy
Copy link
Contributor

thomas-nguy commented Mar 22, 2022

Upon a "SendToCosmos" event, gravity tokens will be minted by the gravity module.

The tokens denom will be defined by strings.Join([]string{GravityDenomPrefix, e.Contract}, GravityDenomSeparator)}

(https://github.com/PeggyJV/gravity-bridge/blob/main/module/x/gravity/types/ethereum.go#L66)

The e.Contract represents the erc20 token contract address which is specified in the event https://github.com/PeggyJV/gravity-bridge/blob/main/module/x/gravity/types/msgs.pb.go#L856

While addresses in Ethereum should be not case sensitive, I think there is a possibility depending on the orchestrator implementation to use different address representation (case sensitive) and it would endup mapping to a different denom.

Maybe as a safe guard it would be better to always convert to lowercase?

@EricBolten
Copy link
Contributor

Hey @thomas-nguy, just to give you an update, I've been looking into this and there are a number of places affected by this casing issue, since in some cases we are using the raw input string from messages and in others using hex conversion from an Address type. Unfortunately the fix isn't as simple as just changing it in the GravityCoin serialization...for example, we are also storing mismatched cases in this instance:

func (k Keeper) setCosmosOriginatedDenomToERC20(ctx sdk.Context, denom string, tokenContract string) {
store := ctx.KVStore(k.storeKey)
store.Set(types.MakeDenomToERC20Key(denom), common.HexToAddress(tokenContract).Bytes())
store.Set(types.MakeERC20ToDenomKey(tokenContract), []byte(denom))
}

I suspect a fix for this will require a data migration to a v2 of the module.

@EricBolten EricBolten self-assigned this Mar 30, 2022
@thomas-nguy
Copy link
Contributor Author

I see,

fortunately we have only a single orchestrator implementation and it seems it uses a web3 library that use the lower case format (not the checkum one), so data migration might not be that critical if we assume its all lower case til now.

One idea would also to sanitise the all the address string in the event handler to convert to lower case but it may not be the cleanest solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants