[EXT-SEC-AUDIT] Improper types force unnecessary error handling

Issue transferred from an external security audit report.

Severity: Informational
Type: Data Validation
Target: go/consensus/tendermint/apps/staking
Difficulty: Undetermined
Finding ID: TOB-OL-104

Description

Some functions in the codebase accept overly permissive argument types. Those types
allow values for which a function is not defined. There exists a carried risk with such types
which include unnecessary error handling, hidden crashes and additional mental overhead
for developers and auditors. An occurrence of such function can be seen on Figure
TOB-OL-104.1. In this example, the attenuationNumerator and attenuationDenominator
should be unsigned integers. The burden of providing values of those types will be pushed
to the place where the values originate. The relevant checks will be performed just once
and the validated precondition will be embedded in the type.

Figure TOB-OL-101.1 shows https://github.com/oasislabs/oasis-core/blob/f331a877721f44cc2412b4248d66e95f83f4cf9c/go/consensus/tendermint/apps/staking/state/state.go#L699-L722

Exploit Scenario

An Oasis developer calls a function which panics when given a value for which it is not
defined, introducing a possibility of node failure.
Recommendation

Short term, update the function to use unsigned integer for the mentioned arguments and
remove excessive error handling.

Long term, always choose types which describe data’s semantics as close as possible.

Start in go/consensus/tendermint/apps/staking/state/state.go at lines 699-722, focusing on attenuationNumerator and attenuationDenominator and their callers. Update the function arguments to unsigned integers, remove the unnecessary error handling described in the audit, and verify that the staking code still builds and tests pass.