[EXT-SEC-AUDIT] Failure to return error in initTotalSupply() method

[pro-wh]

Issue transferred from an external security audit report.

Severity: Informational
Difficulty: Undetermined
Type: Error Reporting
Finding ID: TOB-OL-107
Target: go/consensus/tendermint/apps/staking/genesis.go

Description

initTotalSupply() checks and logs an error if the total supply mismatches between the
expected state value and the supplied method argument, but does not actually return an
error. Instead it goes ahead and sets the total supply of the state to the supplied method
argument. Also, as a separate but co-located code quality issue,
state.SetCommonPool(&st.CommonPool) here appears to be unnecessary.

Figure TOB-OL-106.1 shows https://github.com/oasislabs/oasis-core/blob/f331a877721f44cc2412b4248d66e95f83f4cf9c/go/consensus/tendermint/apps/staking/genesis.go#L112-L122

Exploit Scenario

A typo occurs in the genesis document. totalSupply is instantiated as a zero value and has
the value of CommonPool when passed into initTotalSupply . An error is logged but the
function executes successfully. If the logged error is not noticed this leads to a corrupted
state where totalSupply ’s value is inconsistent with the genesis document.

Recommendation

Return an error corresponding to the total supply mismatch between expected (i.e. state)
and actual values (i.e. method argument) and check it at the call site. Delete
state.SetCommonPool(&st.CommonPool).

[kostko]

Also, as a separate but co-located code quality issue, state.SetCommonPool(&st.CommonPool) here appears to be unnecessary.

Huh? I don't see any other place that initializes the common pool, so it is very necessary. (We could move it to initCommonPool instead though.)

Also note that the total supply mismatch would actually be caught by genesis document sanity checks. But returning an error here is probably the right thing to do.