Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Govshuttle module does not register its transaction MsgServer #5

Open
c4-bot-5 opened this issue Jun 15, 2024 · 5 comments
Open

Govshuttle module does not register its transaction MsgServer #5

c4-bot-5 opened this issue Jun 15, 2024 · 5 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working edited-by-warden ineligible for award M-03 🤖_05_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@c4-bot-5
Copy link
Contributor

c4-bot-5 commented Jun 15, 2024

Lines of code

https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/x/govshuttle/module.go#L127

Vulnerability details

The x/govshuttle module in canto-main defines and handles two messages that can be emitted by a governance proposal:

However, because the module only registers the QueryServer (and not its MsgServer) in its RegisterServices function,
causing no message to be routed to its message server:

func (am AppModule) RegisterServices(cfg module.Configurator) {
	types.RegisterQueryServer(cfg.QueryServer(), am.keeper)
}

If we compare this with another module that can handle messages, for example CSR, we see that this is the place for
registering the MsgServer where transactional messages are routed to:

func (am AppModule) RegisterServices(cfg module.Configurator) {
	types.RegisterMsgServer(cfg.MsgServer(), am.keeper)
	types.RegisterQueryServer(cfg.QueryServer(), am.keeper)
}

Impact

Successful governance actions that include a LendingMarketProposal or TreasuryProposal will fail to execute because
no handler is provided for them.

Proof of Concept

To reproduce the issue it is sufficient to create and approve a proposal among the affected ones.

Tools Used

Code review

Recommended Mitigation Steps

Consider adding a RegisterMsgServer call in the x/govshuttle RegisterService callback.

Assessed type

Other

@c4-bot-5 c4-bot-5 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jun 15, 2024
c4-bot-9 added a commit that referenced this issue Jun 15, 2024
@c4-bot-4 c4-bot-4 changed the title Govshuttle module's "MsgLendingMarketProposal" and "MsgTreasuryProposal" proposal messages can't be executed Govshuttle module does not register its transaction MsgServer Jun 16, 2024
@c4-bot-13 c4-bot-13 added the 🤖_05_group AI based duplicate group recommendation label Jun 20, 2024
@thebrittfactor
Copy link

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
Copy link

dudong2 commented Jul 1, 2024

  • Label
    • sponsor confirmed
  • Reasoning
    • As your description, the MsgServer isn't not registered about govshuttle module.
      Even if gov proposal that include a LendingMarketProposal or TreasuryProposal is passed, the msgs are not executed because there is no handler registered.
  • Severity
    • Mid
  • Patch
    • We will patch this before the v0.50 production release.

@c4-judge c4-judge reopened this Jul 1, 2024
@c4-judge
Copy link

c4-judge commented Jul 1, 2024

3docSec marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jul 1, 2024
@c4-judge
Copy link

c4-judge commented Jul 1, 2024

3docSec marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Jul 1, 2024
@thebrittfactor thebrittfactor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Jul 1, 2024
@thebrittfactor
Copy link

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

@C4-Staff C4-Staff added the M-03 label Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working edited-by-warden ineligible for award M-03 🤖_05_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

7 participants