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

FeegrantKeeper is not checked if defined #12

Closed
c4-bot-6 opened this issue Jun 11, 2024 · 2 comments
Closed

FeegrantKeeper is not checked if defined #12

c4-bot-6 opened this issue Jun 11, 2024 · 2 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 insufficient quality report This report is not of sufficient quality 🤖_primary AI based primary recommendation

Comments

@c4-bot-6
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/app/ante/handler_options.go#L39-L57

Vulnerability details

Impact

The Validate function will not revert when FeegrantKeeper is nil

Proof of Concept

Here's the Validate function:

// Validate checks if the keepers are defined func (options HandlerOptions) Validate() error { if options.AccountKeeper == nil { return errorsmod.Wrap(sdkerrors.ErrLogic, "account keeper is required for AnteHandler") } if options.BankKeeper == nil { return errorsmod.Wrap(sdkerrors.ErrLogic, "bank keeper is required for AnteHandler") } if options.SignModeHandler == nil { return errorsmod.Wrap(sdkerrors.ErrLogic, "sign mode handler is required for ante builder") } if options.FeeMarketKeeper == nil { return errorsmod.Wrap(sdkerrors.ErrLogic, "fee market keeper is required for AnteHandler") } if options.EvmKeeper == nil { return errorsmod.Wrap(sdkerrors.ErrLogic, "evm keeper is required for AnteHandler") } return nil }

https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/app/ante/handler_options.go#L39-L57

The function does not check if FeegrantKeeper is not nil. Therefore the function will not return an error when FeegrantKeeper is nil.

Tools Used

Manual review

Recommended Mitigation Steps

It's recommended to add a check for the FeeGrantKeeper in the Validate function, similar to the checks for other keepers.

if options.FeegrantKeeper == nil {
return errorsmod.Wrap(sdkerrors.ErrLogic, "fee grant keeper is required for AnteHandler")
}

Assessed type

Context

@c4-bot-6 c4-bot-6 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 11, 2024
c4-bot-4 added a commit that referenced this issue Jun 11, 2024
@c4-bot-12 c4-bot-12 added the 🤖_primary AI based primary recommendation label Jun 20, 2024
@howlbot-integration howlbot-integration bot added the insufficient quality report This report is not of sufficient quality label Jun 21, 2024
@ololade97
Copy link

I believe this issue is wrongly marked by the bot for insufficient report.

The issue is FeeGrantKeeper is not checked if it is nil like the others in the Validate function. So it will not return an error when nil.

@3docSec
Copy link

3docSec commented Jul 3, 2024

Insufficient quality is OK here, there is no explanation for how options.FeegrantKeeper can be nil. It is recommended that you put more effort in your findings; remember that the burden of proof is on you, the warden

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 insufficient quality report This report is not of sufficient quality 🤖_primary AI based primary recommendation
Projects
None yet
Development

No branches or pull requests

4 participants