[EXT-SEC-AUDIT] Risk of corrupted state on error

Issue transferred from an external security audit report.

Severity: Low
Type: Denial of Service
Target: go/consensus/tendermint/apps/staking
Difficulty: High
Finding ID: TOB-OL-110

Description

The disburseFees and BeginBlock functions leave the state inconsistent when an error
occurs. For disburseFees it is indicated by the comment and the only call of this function
eventually terminates the program (panic) on such error. Also, any error returned from
BeginBlock will terminate the program as well. While at the moment the issue is mitigated,
the mechanism is fragile to semantic changes in remote places which might go unnoticed
while developing the codebase. Places in both functions where the corrupted state might
arise were highlighted with red color on Figure TOB-OL-110.1 and Figure TOB-OL-110.2.
Functions return error there however the state has been already modified prior to that.

Figure TOB-OL-110.1 shows https://github.com/oasislabs/oasis-core/blob/f331a877721f44cc2412b4248d66e95f83f4cf9c/go/consensus/tendermint/apps/staking/fees.go#L15-L135

Figure TOB-OL-110.2 shows https://github.com/oasislabs/oasis-core/blob/f331a877721f44cc2412b4248d66e95f83f4cf9c/go/consensus/tendermint/apps/staking/staking.go#L55-L99

Exploit Scenario

An Oasis developer mistakenly uses the disburseFees function opening a possibility to
corrupt the chain state.

Recommendation

Modify the functions to mutate state atomically. Collect intermediate results into local
variables and apply them at once to the shared state.

Start with go/consensus/tendermint/apps/staking/fees.go and staking.go, focusing on disburseFees and BeginBlock and the highlighted error paths. Trace how each function changes shared state before returning an error. Done means both functions apply state changes atomically so an error cannot leave corrupted state.