Incorrect names provided in RegisterConcrete calls break LegacyAmino signing method

[c4-bot-7]

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

[thebrittfactor]

Warden will be acting as the judge for this audit and therefore, has agreed to forfeit their submissions and will not be eligible for awards for this audit.

[dudong2]

• Label
  • sponsor confirmed
• Reasoning
  • Through your test code, we checked that several LegacyAmino has wrong type name. And it can cause AminoJson signing failing.
• Severity
  • Mid
• Patch
  • We will patch this before the v0.50 production release.

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as selected for report

[thebrittfactor]

For transparency, staff have added the appropriate sponsor label, per discord confirmation and this comment from the sponsor team.