Incorrect names provided in RegisterConcrete calls break LegacyAmino signing method

mediumCode4rena

Lines of code

https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/proto/canto/coinswap/v1/tx.proto#L40 https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/proto/canto/coinswap/v1/tx.proto#L68 https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/proto/canto/coinswap/v1/tx.proto#L98 https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/proto/canto/coinswap/v1/tx.proto#L116 https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/proto/canto/csr/v1/tx.proto#L20 https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/proto/canto/erc20/v1/tx.proto#L133 https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/proto/canto/erc20/v1/tx.proto#L112 https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/proto/canto/inflation/v1/tx.proto#L26 https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/proto/canto/onboarding/v1/tx.proto#L26 https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/ethermint-main/proto/ethermint/evm/v1/tx.proto#L173 https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/ethermint-main/x/feemarket/types/codec.go#L41

Vulnerability details

One of the breaking changes introduced with the Cosmos SDK v0.50.x upgrade is a change in the codec used for Amino JSON (de)serialization. To ensure the new codec behaves as the abandoned one did, the team added amino.name tags to the message types defined in the Canto modules' ".proto" files.

There are however many instances where these tags are inconsistent with the RegisterConcrete calls made by the in-scope modules' func (AppModuleBasic) RegisterInterfaces functions, all summarized below:

Module coinswap:

• MsgAddLiquidity's tag in protobuf (canto/MsgAddLiquidity) does not match the name registered in code (coinswap/coinswap/MsgSwapOrder)
• MsgRemoveLiquidity's tag in protobuf (canto/MsgRemoveLiquidity) does not match the name registered in code (coinswap/coinswap/MsgAddLiquidity)
• MsgSwapOrder's tag in protobuf (canto/MsgSwapOrder) does not match the name registered in code (coinswap/coinswap/MsgRemoveLiquidity)
• MsgUpdateParams's tag in protobuf (canto/MsgUpdateParams) does not match the name registered in code (coinswap/coinswap/MsgUpdateParams)
• Param's tag in protobuf (not set) does not match the name registered in code (coinswap/coinswap/Params)

Module csr:

• MsgUpdateParams's tag in protobuf ("canto/MsgUpdateParams") does not match the name registered in code (canto/x/csr/MsgUpdateParams)
• Param's tag in protobuf (not set) does not match the name registered in code (canto/x/csr/Params)

Module erc20:

• MsgRegisterCoin's tag in protobuf (canto/MsgRegisterCoin) does not match the name registered in code ("canto/RegisterCoinProposal")
• MsgRegisterERC20's tag in protobuf (canto/MsgRegisterERC20) does not match the name registered in code ("canto/RegisterERC20Proposal")
• Params's tag in protobuf (not set) does not match the name registered in code ("canto/Params")

Module govshuttle

Module govshuttle has no discrepancy thanks to the fact that the RegisterConcrete call was not made with the Msg types

Module inflation

• MsgUpdateParams's tag in protobuf (canto/MsgUpdateParams) does not match the name registered in code (canto/x/inflation/MsgUpdateParams)
• Params's tag in protobuf (not set) does not match the name registered in code (canto/x/inflation/Params)

Module onboarding

• MsgUpdateParams's tag in protobuf (canto/MsgUpdateParams) does not match the name registered in code (canto/x/onboarding/MsgUpdateParams)
• Params's tag in protobuf (not set) does not match the name registered in code (canto/x/onboarding/Params)

Module evm

• MsgUpdateParams's tag in protobuf (not set) does not match the name registered in code (ethermint/MsgUpdateParams)

Module feemarket

• MsgUpdateParams's tag in protobuf (not set) does not match the name registered in code (ethermint/feemarket/MsgUpdateParams)

Impact

All the messages with inconsistent settings listed above, when signed with the LegacyAmino method on a v7 or compatible client, will not be recognized (and consequently rejected) by the Canto app v8 message routing

Proof of Concept

This finding can be proved by adapting this generative test (that is the verification tool mentioned in the Cosmos SDK v0.50.x upgrade guide) to check the messages defined in the Canto modules instead of those standard to the Cosmos SDK it was originally written for.

Adapting this test requires a bit of workarounds because the test itself uses internal packages of the Canto SDK that can't be imported directly, so to make a runnable PoC, I've created a Bash script that builds up the test environment, and runs the failing test (note that Git and Go installations are a prerequisite for this script).

This Bash script can be found here and its output (limited to the first of 14 failing tests) is:

Expected :{"type":"coinswap/coinswap/MsgAddLiquidity","value":{"max_token":{"amount":"0"},"exact_standard_amt":"0","min_liquidity":"0"}}
Actual   :{"type":"canto/MsgAddLiquidity","value":{"max_token":{"amount":"0"},"exact_standard_amt":"0","min_liquidity":"0"}}

Tools Used

Code review

Recommended Mitigation Steps

Consider fixing the RegisterConcrete calls to match the amino.name flags of all the messages enumerated above, which fail the test provided as PoC.

Assessed type

en/de-code