Only allow valid denoms in bank's denom Metadata

[amaury1093]

Summary of Bug

ref: #10701 (comment)

Only allow valid denoms in bank's denom Metadata.

The denom regex we use is

cosmos-sdk/types/coin.go

Line 856 in 23baecf

reDnmString = `[a-zA-Z][a-zA-Z0-9/:._-]{2,127}`

But it seems to me we don't do a regex check when writing the metadata to state.

Version

v0.46.0-rc1

Steps to Reproduce

Currently, the denom metadata is set in InitGenesis, by reading a genesis JSON file. This json file can contain denom strings that don't adhere to the regex above. But they are still written to state:

cosmos-sdk/x/bank/keeper/keeper.go

Lines 305 to 312 in 5505428

	// SetDenomMetaData sets the denominations metadata
	func (k BaseKeeper) SetDenomMetaData(ctx sdk.Context, denomMetaData types.Metadata) {
	store := ctx.KVStore(k.storeKey)
	denomMetaDataStore := prefix.NewStore(store, types.DenomMetadataPrefix)
	m := k.cdc.MustMarshal(&denomMetaData)
	denomMetaDataStore.Set([]byte(denomMetaData.Base), m)
	}

Proposed fix

A validate basic function on Metadata to verify that all denoms match the regex.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[aaronc]

That makes sense but can we please get rid of the global regex when we fix this

[amaury1093]

The regex should definitely go into x/bank

[alexanderbez]

We should do this along with ensuring globally unique aliases.

[amaury1093]

@alexanderbez I would like to add this to the current sprint, because of #12729 (comment). Are you still planning to tackle this? If not, I can take it.

[amaury1093]

One thing to discuss is what to do if the app has existing coins metadata already in their state, whose denoms don't match the regex.

I think we can add a bank migration that don't do any state migrations, but simply checks existing metadata, and errors/panics if there are invalid denoms with a useful message.

[alexanderbez]

@AmauryM I am not working on it and we should definitely add it to the sprint!

I think we can add a bank migration that don't do any state migrations, but simply checks existing metadata, and errors/panics if there are invalid denoms with a useful message.

That sounds reasonable to me.

[aaronc]

Two related issues - one the denom regex is global and mutable (seeing I had already pointed this out above). We should either make it configurable (maybe a bank keeper param) or totally fixed. For the needs of sign mode textual it seems that we need ASCII. I know people want emojis and stuff too. These seem like two contrary aims and I might lean towards the needs of textual here over emojis. Either way would be good to address.

[alexanderbez]

Even if the regex is a global, the app dev can modify it which would be used by all denom verification, no?

[aaronc]

Even if the regex is a global, the app dev can modify it which would be used by all denom verification, no?

We can make it unexported if we want to prevent modification.

[amaury1093]

What I understood is that it's:

• either global but non-mutable
• or a bank keeper param

Note that if we go with a keeper param, we need to remove stateless validation on Coins, e.g.

cosmos-sdk/types/coin.go

Line 42 in 0943a70

func (coin Coin) Validate() error {

[aaronc]

Is there a way textual could handle non-ASCII denoms? What does textual do with raw strings with non-ASCII chars?

[amaury1093]

Yes Textual needs its own way to deal with UTF8 in any case. Latest development from yesterday goes towards actually accepting UTF8 in the spec itself, and add spec on how the signing device escapes utf8 chars (if it wants), e.g. with its display code.

I think this makes the emoji issue in denoms orthogonal to Textual.

[alexanderbez]

I'm a bit weary of making it a bank keeper param. I think methods on the coin type might prove to be challening as you've pointed out.

Can we keep a private immutable global (for coins and metadata) while using a different regex for sign modes?

[aaronc]

Can we keep a private immutable global (for coins and metadata) while using a different regex for sign modes?

Then there's no way of guaranteeing whether sign mode textual will actually work if they're different. Do we have no use cases for custom regexes?

If sign mode textual can deal with UTF-8 somehow then the simplest solution might be to choose the most permissive regex which still is parseable.

[alexanderbez]

Then there's no way of guaranteeing whether sign mode textual will actually work if they're different. Do we have no use cases for custom regexes?

Maybe we do, but at that point, we're just gonna have to pass around a Regex everywhere, which I guess is fine.

[amaury1093]

then the simplest solution might be to choose the most permissive regex which still is parseable.

Let's go with that. Any ideas how the regex might look like? We might still want to disallow dangerous denoms which start with numeral-looking unicodes, e.g. ⒈ (\u2488)

[amaury1093]

I have a PR here: #13247