From 11fa6259343813becb4794bf0c617995680ba765 Mon Sep 17 00:00:00 2001
From: Anton Kaliaev <anton.kalyaev@gmail.com>
Date: Tue, 14 Apr 2020 14:58:50 +0400
Subject: [PATCH] evidence: both MaxAgeDuration and MaxAgeNumBlocks need to be
 surpassed (#4667)

for evidence to be considered expired. otherwise, a cabal group can
  manipulate block time to make a particular evidence too old.

Refs https://github.com/tendermint/tendermint/issues/2565#issuecomment-432896645
Refs https://github.com/tendermint/tendermint/issues/2653
spec PR: tendermint/spec#87
---
 evidence/pool.go      |  3 +--
 evidence/pool_test.go | 10 +++++-----
 evidence/reactor.go   |  2 +-
 state/validation.go   | 21 +++++++++++----------
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/evidence/pool.go b/evidence/pool.go
index acbd4bc23..68967ede1 100644
--- a/evidence/pool.go
+++ b/evidence/pool.go
@@ -163,8 +163,7 @@ func (evpool *Pool) removeEvidence(
 
 		// Remove the evidence if it's already in a block or if it's now too old.
 		if _, ok := blockEvidenceMap[evMapKey(ev)]; ok ||
-			ageNumBlocks > params.MaxAgeNumBlocks ||
-			ageDuration > params.MaxAgeDuration {
+			(ageDuration > params.MaxAgeDuration && ageNumBlocks > params.MaxAgeNumBlocks) {
 			// remove from clist
 			evpool.evidenceList.Remove(e)
 			e.DetachPrev()
diff --git a/evidence/pool_test.go b/evidence/pool_test.go
index ba39aaa61..97694d1ff 100644
--- a/evidence/pool_test.go
+++ b/evidence/pool_test.go
@@ -58,7 +58,7 @@ func TestEvidencePool(t *testing.T) {
 
 	var (
 		valAddr      = []byte("val1")
-		height       = int64(5)
+		height       = int64(100002)
 		stateDB      = initializeValidatorState(valAddr, height)
 		evidenceDB   = dbm.NewMemDB()
 		pool         = NewPool(stateDB, evidenceDB)
@@ -66,7 +66,7 @@ func TestEvidencePool(t *testing.T) {
 	)
 
 	goodEvidence := types.NewMockEvidence(height, time.Now(), 0, valAddr)
-	badEvidence := types.NewMockEvidence(height, evidenceTime, 0, valAddr)
+	badEvidence := types.NewMockEvidence(1, evidenceTime, 0, valAddr)
 
 	// bad evidence
 	err := pool.AddEvidence(badEvidence)
@@ -134,10 +134,10 @@ func TestAddEvidence(t *testing.T) {
 		evDescription string
 	}{
 		{height, time.Now(), false, "valid evidence"},
-		{height, evidenceTime, true, "evidence created at 2019-01-01 00:00:00 +0000 UTC has expired"},
-		{int64(1), time.Now(), true, "evidence from height 1 is too old"},
+		{height, evidenceTime, false, "valid evidence (despite old time)"},
+		{int64(1), time.Now(), false, "valid evidence (despite old height)"},
 		{int64(1), evidenceTime, true,
-			"evidence from height 1 is too old & evidence created at 2019-01-01 00:00:00 +0000 UTC has expired"},
+			"evidence from height 1 (created at: 2019-01-01 00:00:00 +0000 UTC) is too old"},
 	}
 
 	for _, tc := range testCases {
diff --git a/evidence/reactor.go b/evidence/reactor.go
index 7bedeeb43..26343638a 100644
--- a/evidence/reactor.go
+++ b/evidence/reactor.go
@@ -194,7 +194,7 @@ func (evR Reactor) checkSendEvidenceMessage(
 
 	if peerHeight < evHeight { // peer is behind. sleep while he catches up
 		return nil, true
-	} else if ageNumBlocks > params.MaxAgeNumBlocks ||
+	} else if ageNumBlocks > params.MaxAgeNumBlocks &&
 		ageDuration > params.MaxAgeDuration { // evidence is too old, skip
 
 		// NOTE: if evidence is too old for an honest peer, then we're behind and
diff --git a/state/validation.go b/state/validation.go
index 41f12cc40..ccbcc72e2 100644
--- a/state/validation.go
+++ b/state/validation.go
@@ -162,18 +162,19 @@ func VerifyEvidence(stateDB dbm.DB, state State, evidence types.Evidence) error
 	var (
 		height         = state.LastBlockHeight
 		evidenceParams = state.ConsensusParams.Evidence
-	)
 
-	ageNumBlocks := height - evidence.Height()
-	if ageNumBlocks > evidenceParams.MaxAgeNumBlocks {
-		return fmt.Errorf("evidence from height %d is too old. Min height is %d",
-			evidence.Height(), height-evidenceParams.MaxAgeNumBlocks)
-	}
+		ageDuration  = state.LastBlockTime.Sub(evidence.Time())
+		ageNumBlocks = height - evidence.Height()
+	)
 
-	ageDuration := state.LastBlockTime.Sub(evidence.Time())
-	if ageDuration > evidenceParams.MaxAgeDuration {
-		return fmt.Errorf("evidence created at %v has expired. Evidence can not be older than: %v",
-			evidence.Time(), state.LastBlockTime.Add(evidenceParams.MaxAgeDuration))
+	if ageDuration > evidenceParams.MaxAgeDuration && ageNumBlocks > evidenceParams.MaxAgeNumBlocks {
+		return fmt.Errorf(
+			"evidence from height %d (created at: %v) is too old; min height is %d and evidence can not be older than %v",
+			evidence.Height(),
+			evidence.Time(),
+			height-evidenceParams.MaxAgeNumBlocks,
+			state.LastBlockTime.Add(evidenceParams.MaxAgeDuration),
+		)
 	}
 
 	valset, err := LoadValidators(stateDB, evidence.Height())