From 8a878c1cad3433ce266962bd5833906d8fc32d6b Mon Sep 17 00:00:00 2001
From: Aditya <adityasripal@gmail.com>
Date: Tue, 19 Nov 2019 01:54:54 -0500
Subject: [PATCH] evidence: enforce ordering in DuplicateVoteEvidence (#4151)

Fixes #4143
---
 CHANGELOG_PENDING.md   |  1 +
 rpc/client/rpc_test.go |  6 +-----
 types/evidence.go      | 26 ++++++++++++++++++++++++++
 types/evidence_test.go | 14 +++++++++-----
 types/vote.go          |  8 ++------
 5 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md
index 137b7d05f..ad299e2b6 100644
--- a/CHANGELOG_PENDING.md
+++ b/CHANGELOG_PENDING.md
@@ -49,6 +49,7 @@ program](https://hackerone.com/tendermint).
 
 - Blockchain Protocol
   - [abci] \#2521 Remove `TotalTxs` and `NumTxs` from `Header`
+  - [types] [\#4151](https://github.com/tendermint/tendermint/pull/4151) Enforce ordering of votes in DuplicateVoteEvidence to be lexicographically sorted on BlockID
 
 - P2P Protocol
   - [p2p] [\3668](https://github.com/tendermint/tendermint/pull/3668) Make `SecretConnection` non-malleable
diff --git a/rpc/client/rpc_test.go b/rpc/client/rpc_test.go
index 34d41d0e1..6a7a802e2 100644
--- a/rpc/client/rpc_test.go
+++ b/rpc/client/rpc_test.go
@@ -504,11 +504,7 @@ func newEvidence(
 	vote2_.Signature, err = val.Key.PrivKey.Sign(vote2_.SignBytes(chainID))
 	require.NoError(t, err)
 
-	return types.DuplicateVoteEvidence{
-		PubKey: val.Key.PubKey,
-		VoteA:  vote,
-		VoteB:  vote2_,
-	}
+	return *types.NewDuplicateVoteEvidence(val.Key.PubKey, vote, vote2_)
 }
 
 func makeEvidences(
diff --git a/types/evidence.go b/types/evidence.go
index 63d6859e4..ac5cd3a2a 100644
--- a/types/evidence.go
+++ b/types/evidence.go
@@ -3,6 +3,7 @@ package types
 import (
 	"bytes"
 	"fmt"
+	"strings"
 
 	"github.com/pkg/errors"
 	"github.com/tendermint/tendermint/crypto/tmhash"
@@ -103,6 +104,27 @@ type DuplicateVoteEvidence struct {
 
 var _ Evidence = &DuplicateVoteEvidence{}
 
+// NewDuplicateVoteEvidence creates DuplicateVoteEvidence with right ordering given
+// two conflicting votes. If one of the votes is nil, evidence returned is nil as well
+func NewDuplicateVoteEvidence(pubkey crypto.PubKey, vote1 *Vote, vote2 *Vote) *DuplicateVoteEvidence {
+	var voteA, voteB *Vote
+	if vote1 == nil || vote2 == nil {
+		return nil
+	}
+	if strings.Compare(vote1.BlockID.Key(), vote2.BlockID.Key()) == -1 {
+		voteA = vote1
+		voteB = vote2
+	} else {
+		voteA = vote2
+		voteB = vote1
+	}
+	return &DuplicateVoteEvidence{
+		PubKey: pubkey,
+		VoteA:  voteA,
+		VoteB:  voteB,
+	}
+}
+
 // String returns a string representation of the evidence.
 func (dve *DuplicateVoteEvidence) String() string {
 	return fmt.Sprintf("VoteA: %v; VoteB: %v", dve.VoteA, dve.VoteB)
@@ -209,6 +231,10 @@ func (dve *DuplicateVoteEvidence) ValidateBasic() error {
 	if err := dve.VoteB.ValidateBasic(); err != nil {
 		return fmt.Errorf("invalid VoteB: %v", err)
 	}
+	// Enforce Votes are lexicographically sorted on blockID
+	if strings.Compare(dve.VoteA.BlockID.Key(), dve.VoteB.BlockID.Key()) >= 0 {
+		return errors.New("duplicate votes in invalid order")
+	}
 	return nil
 }
 
diff --git a/types/evidence_test.go b/types/evidence_test.go
index 3c943e38f..ff6823c2c 100644
--- a/types/evidence_test.go
+++ b/types/evidence_test.go
@@ -144,15 +144,19 @@ func TestDuplicateVoteEvidenceValidation(t *testing.T) {
 		{"Invalid vote type", func(ev *DuplicateVoteEvidence) {
 			ev.VoteA = makeVote(val, chainID, math.MaxInt64, math.MaxInt64, math.MaxInt64, 0, blockID2)
 		}, true},
+		{"Invalid vote order", func(ev *DuplicateVoteEvidence) {
+			swap := ev.VoteA.Copy()
+			ev.VoteA = ev.VoteB.Copy()
+			ev.VoteB = swap
+		}, true},
 	}
 	for _, tc := range testCases {
 		tc := tc
 		t.Run(tc.testName, func(t *testing.T) {
-			ev := &DuplicateVoteEvidence{
-				PubKey: secp256k1.GenPrivKey().PubKey(),
-				VoteA:  makeVote(val, chainID, math.MaxInt64, math.MaxInt64, math.MaxInt64, 0x02, blockID),
-				VoteB:  makeVote(val, chainID, math.MaxInt64, math.MaxInt64, math.MaxInt64, 0x02, blockID2),
-			}
+			pk := secp256k1.GenPrivKey().PubKey()
+			vote1 := makeVote(val, chainID, math.MaxInt64, math.MaxInt64, math.MaxInt64, 0x02, blockID)
+			vote2 := makeVote(val, chainID, math.MaxInt64, math.MaxInt64, math.MaxInt64, 0x02, blockID2)
+			ev := NewDuplicateVoteEvidence(pk, vote1, vote2)
 			tc.malleateEvidence(ev)
 			assert.Equal(t, tc.expectErr, ev.ValidateBasic() != nil, "Validate Basic had an unexpected result")
 		})
diff --git a/types/vote.go b/types/vote.go
index e87eb4e60..9e19841a6 100644
--- a/types/vote.go
+++ b/types/vote.go
@@ -34,13 +34,9 @@ func (err *ErrVoteConflictingVotes) Error() string {
 	return fmt.Sprintf("Conflicting votes from validator %v", err.PubKey.Address())
 }
 
-func NewConflictingVoteError(val *Validator, voteA, voteB *Vote) *ErrVoteConflictingVotes {
+func NewConflictingVoteError(val *Validator, vote1, vote2 *Vote) *ErrVoteConflictingVotes {
 	return &ErrVoteConflictingVotes{
-		&DuplicateVoteEvidence{
-			PubKey: val.PubKey,
-			VoteA:  voteA,
-			VoteB:  voteB,
-		},
+		NewDuplicateVoteEvidence(val.PubKey, vote1, vote2),
 	}
 }