Wrong address prefix for ethermint bech32 account leads to inability to authorize users

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/ethermint-main/app/app.go#L373

Vulnerability details

According to breaking changes:

#9759 NewAccountKeeeper in x/auth now takes an additional bech32Prefix argument that represents sdk.Bech32MainPrefix.

cosmos/cosmos-sdk#9759

While this is added, Canto uses hardcoded default one from Cosmos SDK:

ethermint-main/app/app.go

    // use custom Ethermint account for contracts
    app.AccountKeeper = authkeeper.NewAccountKeeper(
        appCodec,
        runtime.NewKVStoreService(keys[authtypes.StoreKey]),
        ethermint.ProtoAccount,
        maccPerms,
        authcodec.NewBech32Codec(sdk.GetConfig().GetBech32AccountAddrPrefix()),
        sdk.Bech32MainPrefix, // @audit using hardcoded Cosmos default prefix
        authtypes.NewModuleAddress(govtypes.ModuleName).String(),
    )

github.com/cosmos/[email protected]/types/address.go

    // Bech32MainPrefix defines the main SDK Bech32 prefix of an account's address
    Bech32MainPrefix = "cosmos"

It is "cosmos", while ethermint overrides it to custom ethm in ethermint-main/cmd/config/config.go:

const (
	// Bech32Prefix defines the Bech32 prefix used for EthAccounts
	Bech32Prefix = "ethm"

	// Bech32PrefixAccAddr defines the Bech32 prefix of an account's address
	Bech32PrefixAccAddr = Bech32Prefix
	// Bech32PrefixAccPub defines the Bech32 prefix of an account's public key
	Bech32PrefixAccPub = Bech32Prefix + sdk.PrefixPublic

//[...]

// SetBech32Prefixes sets the global prefixes to be used when serializing addresses and public keys to Bech32 strings.
func SetBech32Prefixes(config *sdk.Config) {
	config.SetBech32PrefixForAccount(Bech32PrefixAccAddr, Bech32PrefixAccPub)
	config.SetBech32PrefixForValidator(Bech32PrefixValAddr, Bech32PrefixValPub)
	config.SetBech32PrefixForConsensusNode(Bech32PrefixConsAddr, Bech32PrefixConsPub)
}

Which is then used in ethermintd on startup:

func main() {
	setupConfig()
//[...]
}

func setupConfig() {
	// set the address prefixes
	config := sdk.GetConfig()
	cmdcfg.SetBech32Prefixes(config)
//[...]
}

As a sidenote, tendermint docs mention that accounts have eth prefix. Similarly, Evmos, while successor to Tendermint, uses evmos prefix according to the docs.

This is problematic in case of messages, that translate Bech32 addresses to EVM compatible addresses, like usage of msg.Sender.

Impact

Failing account validation during bech32 to EVM address conversion.

Proof of Concept

When converting the address from Bech32 to EVM, the following is called:

func AccAddressFromBech32(address string) (addr AccAddress, err error) {
    if len(strings.TrimSpace(address)) == 0 {
        return AccAddress{}, errors.New("empty address string is not allowed")
    }

    bech32PrefixAccAddr := GetConfig().GetBech32AccountAddrPrefix()

    bz, err := GetFromBech32(address, bech32PrefixAccAddr)
    if err != nil {
        return nil, err
    }

And inside, GetBech32AccountAddrPrefix() takes the prefix from address:

func (config *Config) GetBech32AccountAddrPrefix() string {
    return config.bech32AddressPrefix["account_addr"]
}

Finally, GetFromBech32() decodes the address and verifies that the prefix passed is the same as config.bech32AddressPrefix:

func GetFromBech32(bech32str, prefix string) ([]byte, error) {
	if len(bech32str) == 0 {
		return nil, errBech32EmptyAddress
	}

	hrp, bz, err := bech32.DecodeAndConvert(bech32str)
	if err != nil {
		return nil, err
	}

	if hrp != prefix { // @audit if prefix doesn't match config.bech32AddressPrefix, it treats it as invalid
		return nil, fmt.Errorf("invalid Bech32 prefix; expected %s, got %s", prefix, hrp)
	}

	return bz, nil
}

So, while the bech32 prefix is hardcoded to cosmos in ethermint-main/app/app.go, here it's taken from the config and it has value of ethm.

Because of this all functions requiring authority may stop working. E.g. in ethermint-main/x/evm/keeper/msg_server.go:

func (k *Keeper) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) {
    if _, err := sdk.AccAddressFromBech32(req.Authority); err != nil {
        return nil, errorsmod.Wrap(err, "invalid authority address")
    }

    if k.authority.String() != req.Authority {
        return nil, errorsmod.Wrapf(govtypes.ErrInvalidSigner, "invalid authority, expected %s, got %s", k.authority.String(), req.Authority)
    }

The same occurs with verifying msg.sender:

canto-main/x/erc20/keeper/msg_server.go:

    _, err := sdk.AccAddressFromBech32(msg.Sender)
    if err != nil {
        return nil, errorsmod.Wrap(err, "invalid sender address")
    }

Tools Used

Manual Review

Recommended Mitigation Steps

The best option seems to be using cosmos/[email protected]/types/config.go#GetBech32AccountAddrPrefix() and make sure that account_addr config property is set. This way, sdk.AccAddressFromBech32() will not error out, because there won't be an address mismatch.

Exemplary fix diff:

+   cfg := sdk.GetConfig()
+
    // use custom Ethermint account for contracts
    app.AccountKeeper = authkeeper.NewAccountKeeper(
        appCodec,
        runtime.NewKVStoreService(keys[authtypes.StoreKey]),
        ethermint.ProtoAccount,
        maccPerms,
        authcodec.NewBech32Codec(sdk.GetConfig().GetBech32AccountAddrPrefix()),
-       sdk.Bech32MainPrefix,
+       cfg.GetBech32AccountAddrPrefix(),
        authtypes.NewModuleAddress(govtypes.ModuleName).String(),
    )

Assessed type

Invalid Validation

[poorphd]

• Label

  • sponsor confirmed

• Reasoning

  $ cantod q auth bech32-prefix
  bech32_prefix: cosmos
  

  As shown above, the output of the cantod q auth bech32-prefix query is cosmos, which is the default value of the cosmos-sdk.

• Severity

  • High → Low
  • The query output is incorrect as explained in the reasoning above. However, the bech32Prefix set in NewAccountKeeper is only used in getBech32Prefix(), and this function is only used in the bech32-prefix query.
  • The AddressCodec used to encode/decode the address has the bech32 prefix of canto set correctly.

• Patch

  • We will patch this before the v0.50 production release

    // use custom Ethermint account for contracts
    app.AccountKeeper = authkeeper.NewAccountKeeper(
        appCodec,
        runtime.NewKVStoreService(keys[authtypes.StoreKey]),
        ethermint.ProtoAccount,
        maccPerms,
        authcodec.NewBech32Codec(sdk.GetConfig().GetBech32AccountAddrPrefix()),
    -   sdk.Bech32MainPrefix,
    +   sdk.GetConfig().GetBech32AccountAddrPrefix()),
        authtypes.NewModuleAddress(govtypes.ModuleName).String(),
    )

[3docSec]

I agree with the sponsor, the faulty one is only used by the GRPC query server, while the application logic is consistently using the proper configuration.

While the issue is valid and somewhat annoying for users who can't rely on the RPC query output, I don't see grounds to justify an H or M severity

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b