upgradetypes.ModuleName has been mistakenly added to SetOrderBeginBlockers

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Description

According to the upgrading guide of cosmos sdk for version 0.50.6, when using (legacy) application wiring, the following must be added to app.go:

+app.ModuleManager.SetOrderPreBlockers(
+   upgradetypes.ModuleName,
+)

app.ModuleManager.SetOrderBeginBlockers(
-   upgradetypes.ModuleName,
)

However, in the app.go of Canto and Ethermint, the upgradetypes.ModuleName under SetOrderBeginBlockers has not been removed.

Canto App
File: app.go
823:        upgradetypes.ModuleName, //@audit should be removed? 

Ethermint App
File: app.go
668:        upgradetypes.ModuleName,//@audit should be removed

Impact

Running the same module twice in a single block cycle can introduce unnecessary computational overhead. This redundancy can slow down block processing without adding any benefit.

Proof of Concept

Consider this scenario, in every block processing, upgradetypes.ModuleName will be executed twice, one in preblock phase and one in beginblock phase. This will delay the overall performance of Canto blockchain as well as unnecessary consumption of resources.

Tools Used

Manual Review

Recommended Mitigation Steps

Remove the upgradetypes.ModuleName under SetOrderBeginBlockers as per upgrade guide.

Assessed type

Other

[poorphd]

• Labels
  • sponsor disputed
• Reasoning
  • As demonstrated in the upgrades/v8/upgrades_test, the Upgrade handler is called only once. This is because, even though the upgrade module is set in both the PreBlocker and the BeginBlocker order (as seen in canto and ethermint), the actual upgrade module logic exists only in the PreBlocker, as shown in the sdk code. This means the upgrade does not execute twice in the BeginBlocker because the BeginBlock checks for HasBeginBlocker, ensuring the Upgrade module is not triggered in BeginBlock, which can be verified in the code. Thus, this is not a vulnerability.
  • For better code aesthetics and to avoid potential misunderstandings, it would be advisable to remove the upgrade module from the BeginBlocker ordering. However, such a change does not impact the security of the app and is not necessary to implement at the audit stage.
• Severity
  • Mid → Not valid
• Patch
  • N/A
• Appendix
  • Upgrade module is set on PreBlockers as well [canto, ethermint]
  • Upgrade module has only PreBlocker [sdk code]
  • BeginBlock checks for HasBeginBlocker, so Upgrade module not triggered on BeginBlock [code]
  • Related PRs: https://github.com/cosmos/cosmos-sdk/pull/17421, https://github.com/cosmos/cosmos-sdk/pull/17713
  • ADR 068 Preblock, upgrade guide

[3docSec]

I agree with the sponsor. The finding is inconsequential because for a hook to be called, the module needs to implement the right interface. And because x/upgrade doesn't, the SetOrderBeginBlockers call has no effect

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid