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

[EXT-SEC-AUDIT] Inconsistent error handling and use of panic #2811

Closed
pro-wh opened this issue Apr 1, 2020 · 1 comment · Fixed by #2674
Closed

[EXT-SEC-AUDIT] Inconsistent error handling and use of panic #2811

pro-wh opened this issue Apr 1, 2020 · 1 comment · Fixed by #2674

Comments

@pro-wh
Copy link
Contributor

pro-wh commented Apr 1, 2020

Issue transferred from an external security audit report.

Severity: Medium
Difficulty: Low
Type: Error Reporting
Finding ID: TOB-OL-109
Target: go/consensus/tendermint/apps/staking/state/state.go, multiple locations

Description

The deserialization failures are handled inconsistently inside the staking state code. Some
functions are returning errors and some are using panic. This mismatch might lead to false
assumptions about which function might fail when developing the codebase. Examples of
both cases can be witnessed on Figures TOB-OL-110.1 and TOB-OL-110.2.

Figure TOB-OL-109.1 shows https://github.com/oasislabs/oasis-core/blob/f331a877721f44cc2412b4248d66e95f83f4cf9c/go/consensus/tendermint/apps/staking/state/state.go#L297-L308

Figure TOB-OL-109.2 shows https://github.com/oasislabs/oasis-core/blob/f331a877721f44cc2412b4248d66e95f83f4cf9c/go/consensus/tendermint/apps/staking/state/state.go#L108-L117

Exploit Scenario

An Oasis developer calls a function which panics with an assumption that it doesn’t fail and
introduces a possibility for a Denial of Service.

Recommendation

Short term, modify the staking state module to return errors.
Long term, avoid using panic unless necessary. Panics are fatal and should only be used
when the error cannot be solved.

@kostko
Copy link
Member

kostko commented Apr 2, 2020

This should be addressed once #2674 is merged as it makes error handling more consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants