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.