diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 23dac0b03..907374f44 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -48,3 +48,4 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi - [blockchain/v2] [\#4761](https://github.com/tendermint/tendermint/pull/4761) Fix excessive CPU usage caused by spinning on closed channels (@erikgrinaker) - [blockchain/v2] Respect `fast_sync` option (@erikgrinaker) - [light] [\#4741](https://github.com/tendermint/tendermint/pull/4741) Correctly return `ErrSignedHeaderNotFound` and `ErrValidatorSetNotFound` on corresponding RPC errors (@erikgrinaker) +- [types] [\#4764](https://github.com/tendermint/tendermint/pull/4764) Return an error if voting power overflows in `VerifyCommitTrusting` (@melekes) diff --git a/types/validator_set.go b/types/validator_set.go index f3e4627db..2ce466250 100644 --- a/types/validator_set.go +++ b/types/validator_set.go @@ -749,11 +749,15 @@ func (vals *ValidatorSet) VerifyFutureCommit(newSet *ValidatorSet, chainID strin // VerifyCommitTrusting verifies that trustLevel ([1/3, 1]) of the validator // set signed this commit. +// // NOTE the given validators do not necessarily correspond to the validator set // for this commit, but there may be some intersection. +// +// Panics if trustLevel is invalid. func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, blockID BlockID, height int64, commit *Commit, trustLevel tmmath.Fraction) error { + // sanity check if trustLevel.Numerator*3 < trustLevel.Denominator || // < 1/3 trustLevel.Numerator > trustLevel.Denominator { // > 1 panic(fmt.Sprintf("trustLevel must be within [1/3, 1], given %v", trustLevel)) @@ -766,9 +770,15 @@ func (vals *ValidatorSet) VerifyCommitTrusting(chainID string, blockID BlockID, var ( talliedVotingPower int64 seenVals = make(map[int]int, len(commit.Signatures)) // validator index -> commit index - votingPowerNeeded = (vals.TotalVotingPower() * trustLevel.Numerator) / trustLevel.Denominator ) + // Safely calculate voting power needed. + totalVotingPowerMulByNumerator, overflow := safeMul(vals.TotalVotingPower(), trustLevel.Numerator) + if overflow { + return errors.New("int64 overflow while calculating voting power needed. please provide smaller trustLevel numerator") + } + votingPowerNeeded := totalVotingPowerMulByNumerator / trustLevel.Denominator + for idx, commitSig := range commit.Signatures { if commitSig.Absent() { continue // OK, some signatures can be absent. @@ -912,7 +922,7 @@ func RandValidatorSet(numValidators int, votingPower int64) (*ValidatorSet, []Pr } /////////////////////////////////////////////////////////////////////////////// -// safe addition/subtraction +// safe addition/subtraction/multiplication func safeAdd(a, b int64) (int64, bool) { if b > 0 && a > math.MaxInt64-b { @@ -953,3 +963,33 @@ func safeSubClip(a, b int64) int64 { } return c } + +func safeMul(a, b int64) (int64, bool) { + if a == 0 || b == 0 { + return 0, false + } + + absOfB := b + if b < 0 { + absOfB = -b + } + + var ( + c = a + overflow bool + ) + + for absOfB > 1 { + c, overflow = safeAdd(c, a) + if overflow { + return c, true + } + absOfB-- + } + + if (b < 0 && a > 0) || (b < 0 && a < 0) { + return -c, false + } + + return c, false +} diff --git a/types/validator_set_test.go b/types/validator_set_test.go index 2a6dff9f6..3dbdbabed 100644 --- a/types/validator_set_test.go +++ b/types/validator_set_test.go @@ -1365,7 +1365,47 @@ func TestVerifyCommitTrusting(t *testing.T) { assert.NoError(t, err) } } +} + +func TestVerifyCommitTrustingErrorsOnOverflow(t *testing.T) { + var ( + blockID = makeBlockIDRandom() + voteSet, valSet, vals = randVoteSet(1, 1, PrecommitType, 1, MaxTotalVotingPower) + commit, err = MakeCommit(blockID, 1, 1, voteSet, vals, time.Now()) + ) + require.NoError(t, err) + err = valSet.VerifyCommitTrusting("test_chain_id", blockID, commit.Height, commit, + tmmath.Fraction{Numerator: 25, Denominator: 55}) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "int64 overflow") + } +} + +func TestSafeMul(t *testing.T) { + testCases := []struct { + a int64 + b int64 + c int64 + overflow bool + }{ + 0: {0, 0, 0, false}, + 1: {1, 0, 0, false}, + 2: {2, 3, 6, false}, + 3: {2, -3, -6, false}, + 4: {-2, -3, 6, false}, + 5: {-2, 3, -6, false}, + 6: {math.MaxInt64, 1, math.MaxInt64, false}, + 7: {math.MaxInt64 / 2, 2, math.MaxInt64 - 1, false}, + 8: {math.MaxInt64 / 2, 3, -1, true}, + 9: {math.MaxInt64, 2, -1, true}, + } + + for i, tc := range testCases { + c, overflow := safeMul(tc.a, tc.b) + assert.Equal(t, tc.c, c, "#%d", i) + assert.Equal(t, tc.overflow, overflow, "#%d", i) + } } //---------------------