Unhandled type assertion failure in AutoCliOpts function

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

AutoCliOpts function continues to execute when type assertion fails

Proof of Concept

The AutoCliOpts function does not handle the case where the type assertion fails.

if moduleWithName, ok := m.(module.HasName); ok { moduleName := moduleWithName.Name() if appModule, ok := moduleWithName.(appmodule.AppModule); ok { modules[moduleName] = appModule }

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

Note that the ok value from the type assertions is not handled when it is false. If the type assertion fails (i.e., ok is false), the code silently continues execution without handling the error. Both moduleName and modules[moduleName] will not be set.

Tools Used

Manual review

Recommended Mitigation Steps

To address this issue, the code should handle the case where the type assertion fails (ok is false).

Assessed type

Context

[poorphd]

// AutoCliOpts returns the autocli options for the app.
func (app *Canto) AutoCliOpts() autocli.AppOptions {
    modules := make(map[string]appmodule.AppModule, 0)
    for _, m := range app.ModuleManager.Modules {
        if moduleWithName, ok := m.(module.HasName); ok {
            moduleName := moduleWithName.Name()
            if appModule, ok := moduleWithName.(appmodule.AppModule); ok {
                modules[moduleName] = appModule
            }
        }
    }

    return autocli.AppOptions{
        Modules:               modules,
        ModuleOptions:         runtimeservices.ExtractAutoCLIOptions(app.ModuleManager.Modules),
        AddressCodec:          authcodec.NewBech32Codec(sdk.GetConfig().GetBech32AccountAddrPrefix()),
        ValidatorAddressCodec: authcodec.NewBech32Codec(sdk.GetConfig().GetBech32ValidatorAddrPrefix()),
        ConsensusAddressCodec: authcodec.NewBech32Codec(sdk.GetConfig().GetBech32ConsensusAddrPrefix()),
    }
}

• Labels
  • sponsor disputed
• Reasoning
  • In the AutoCliOpts() function, a module.HasName type assertion is made against the modules in app.ModuleManager.Modules. The app.ModuleManager is defined in app.go. Looking at the NewManager() function, it calls module.Name() on its target modules. This means all modules registered in app.ModuleManager satisfy the type assertion to module.HasName.
  • HasName exists for legacy purposes and isn't required in app_v2. This means modules added later may not satisfy HasName.
  • Therefore, error handling for the type assertion in AutoCliOpts() isn't required.
• Severity
  • N/A

[3docSec]

The finding does not provide sufficient:

• proof that the issue can happen (especially in light of the sponsor's reply)
• elaboration of the impact of what could happen if the issue happens

[c4-judge]

3docSec marked the issue as unsatisfactory: Insufficient proof

[ololade97]

Reasoning

• In the AutoCliOpts() function, a module.HasName type assertion is made against the modules in app.ModuleManager.Modules. The app.ModuleManager is defined in app.go. Looking at the NewManager() function, it calls module.Name() on its target modules. This means all modules registered in app.ModuleManager satisfy the type assertion to module.HasName.

First, just because a module has a Name() method doesn't mean it implements HasName.

1. What NewManager() does:

• It uses module.Name() to get the name of each module.
• This only proves that each module has a Name() method.

2. What AutoCliOpts() does:

• It checks if each module is a HasName type.
• This is a stricter check than just having a Name() method.

3. The misunderstanding:

• The reasoning assumes that having a Name() method automatically makes something a HasName type.

Second, the bug report mentions two type assertions failure not handled, but the sponsor reasoning only addresses the first one (module.HasName). The second type assertion to appmodule.AppModule is not discussed, which is a significant oversight.

Third,

• HasName exists for legacy purposes and isn't required in app_v2. This means modules added later may not satisfy HasName.

HasName may exists for legacy purposes truly and may not be required in app_v2. However, what's most important is that it's in the present code and the future is uncertain. It's best to dot the i's and cross the t's.

Unhadled type assertions failure ultimately allows the code to silently execute without reverting.

[3docSec]

The reasons for invalidating mentioned in https://github.com/code-423n4/2024-05-canto-findings/issues/31#issuecomment-2191570963 still hold.

The finding nor the comment above provide proof that there is an issue at all, and while I am all in for dotting the i's without forgetting the t's uncrossed, futureproofing suggestions qualify well for QA reports but not for H/M findings, for which the C4 categorization is quite strict