From 570e2223b9443fd681f1c8395405e8aae8180d94 Mon Sep 17 00:00:00 2001
From: Stan Bondi <sdbondi@users.noreply.github.com>
Date: Mon, 6 Sep 2021 15:18:32 +0400
Subject: [PATCH] fix: ban header sync peer if no headers provided (#3297)

Description
---
Adds ban in header sync if peer has indicated that they have a better chain but is unable to provide headers

Motivation and Context
---
A peer can advertise a higher chain metadata and then return no
headers. This case is likely malicious. In this PR, this is handled
by banning the peer allowing the node to attempt a sync with another peer.

How Has This Been Tested?
---
Manually
---
 .../core/src/base_node/sync/header_sync/error.rs   |  2 ++
 .../src/base_node/sync/header_sync/synchronizer.rs | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/base_layer/core/src/base_node/sync/header_sync/error.rs b/base_layer/core/src/base_node/sync/header_sync/error.rs
index aee8cf22e7..3e846b85e6 100644
--- a/base_layer/core/src/base_node/sync/header_sync/error.rs
+++ b/base_layer/core/src/base_node/sync/header_sync/error.rs
@@ -63,4 +63,6 @@ pub enum BlockHeaderSyncError {
     InvalidProtocolResponse(String),
     #[error("Headers did not form a chain. Expected {actual} to equal the previous hash {expected}")]
     ChainLinkBroken { actual: String, expected: String },
+    #[error("Invalid remote peer behaviour: {0}")]
+    InvalidRemotePeerBehaviour(String),
 }
diff --git a/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs b/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs
index fdd1ab6d64..dda56cbc09 100644
--- a/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs
+++ b/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs
@@ -127,6 +127,11 @@ impl<'a, B: BlockchainBackend + 'static> HeaderSynchronizer<'a, B> {
                     warn!(target: LOG_TARGET, "Chain split not found for peer {}.", peer);
                     self.ban_peer_long(peer, BanReason::ChainSplitNotFound).await?;
                 },
+                Err(err @ BlockHeaderSyncError::InvalidRemotePeerBehaviour(_)) => {
+                    warn!(target: LOG_TARGET, "{}", err);
+                    self.ban_peer_long(node_id, BanReason::PeerMisbehaviour(err.to_string()))
+                        .await?;
+                },
                 Err(err @ BlockHeaderSyncError::InvalidBlockHeight { .. }) => {
                     warn!(target: LOG_TARGET, "{}", err);
                     self.ban_peer_long(node_id, BanReason::GeneralHeaderSyncFailure(err))
@@ -382,6 +387,13 @@ impl<'a, B: BlockchainBackend + 'static> HeaderSynchronizer<'a, B> {
             peer
         );
 
+        if headers.is_empty() {
+            return Err(BlockHeaderSyncError::InvalidRemotePeerBehaviour(format!(
+                "Remote peer {} advertised a better chain but did not return headers",
+                peer
+            )));
+        }
+
         if fork_hash_index >= block_hashes.len() as u32 {
             let _ = self
                 .ban_peer_long(peer.clone(), BanReason::SplitHashGreaterThanHashes {
@@ -677,6 +689,8 @@ enum BanReason {
     GeneralHeaderSyncFailure(BlockHeaderSyncError),
     #[error("Peer did not respond timeously during RPC negotiation")]
     RpcNegotiationTimedOut,
+    #[error("Peer misbehaviour: {0}")]
+    PeerMisbehaviour(String),
 }
 
 struct ChainSplitInfo {