search

code-423n4 / 2024-05-canto-findings

0 stars 0 forks source link

Incorrect names provided in `RegisterConcrete` calls break LegacyAmino signing method #2

Open c4-bot-7 opened 3 months ago

c4-bot-7 commented 3 months ago

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:

Module csr:

Module erc20:

Module govshuttle

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

Module inflation

Module onboarding

Module evm

Module feemarket

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 commented 2 months ago

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 commented 2 months ago
c4-judge commented 2 months ago

3docSec marked the issue as satisfactory

c4-judge commented 2 months ago

3docSec marked the issue as selected for report

thebrittfactor commented 2 months ago

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