search

cosmos / interchain-security

Interchain Security is an open sourced IBC application which allows cosmos blockchains to lease their proof-of-stake security to one another.
https://cosmos.github.io/interchain-security/
Other
156 stars 126 forks source link

Decide strategy for handling `error`s #174

Closed danwt closed 2 years ago

danwt commented 2 years ago

I just ran gosec and it returned, among other problems, several cases of ignored error returns in our code. See pastebin, here are some examples

[interchain-security/x/ccv/provider/keeper/keeper.go:497] - G104 (CWE-703): Errors unhandled. (Confidence: HIGH, Severity: LOW)
    496:    // Set unbondingOp
  > 497:    h.k.SetUnbondingOp(ctx, unbondingOp)
    498: 

[interchain-security/x/ccv/provider/keeper/keeper.go:272] - G104 (CWE-703): Errors unhandled. (Confidence: HIGH, Severity: LOW)
    271:    if prevChannel, ok := k.GetChannelToChain(ctx, chainID); ok {
  > 272:        k.chanCloseInit(ctx, channelID)
    273:        return sdkerrors.Wrapf(ccv.ErrDuplicateChannel, "CCV channel with ID: %s already created for consumer chain %s", prevChannel, chainID)

...

I will check the cases to see what absolutely must be fixed.

The question is: should we define a rule or strategy about our error handling? E.g. all errors cases should be handled.

danwt commented 2 years ago

I assigned @mpoke and @sainoe additionally as this is a discussion issue. What do you guys think? Should we get a 4th opinion?

danwt commented 2 years ago

I guess we did this. FYI, make sure all errors are handled and panic if they are related to uncontrollable or unrecoverables. If they are on provider side and due to consumer failure then make sure to recover gracefully.