Security Audit: Concurrency and blockHash Consistency Fixes

consensus/XDPoS/engines/engine_v2/engine.go

@@ -766,8 +766,8 @@ func (x *XDPoS_v2) VerifyBlockInfo(blockChainReader consensus.ChainReader, block
 		}
 	} else {
 		// If blockHeader present, then its value shall consistent with what's provided in the blockInfo
-		if blockHeader.Hash() != blockInfo.Hash {
-			log.Warn("[VerifyBlockInfo] BlockHeader and blockInfo mismatch", "BlockInfoHash", blockInfo.Hash.Hex(), "BlockHeaderHash", blockHeader.Hash())
+		if blockHeader.Hash() != blockInfo.Hash && blockHeader.HashNoValidator() != blockInfo.Hash {
+			log.Warn("[VerifyBlockInfo] BlockHeader and blockInfo mismatch", "BlockInfoHash", blockInfo.Hash.Hex(), "BlockHeaderHash", blockHeader.Hash(), "BlockHeaderHashNoValidator", blockHeader.HashNoValidator())
 			return errors.New("[VerifyBlockInfo] Provided blockheader does not match what's in the blockInfo")
 		}
 	}
@@ -830,7 +830,7 @@ func (x *XDPoS_v2) verifyQC(blockChainReader consensus.ChainReader, quorumCert *
 
 	var wg sync.WaitGroup
 	wg.Add(len(signatures))
-	var haveError error
+	errChan := make(chan error, len(signatures))
 
 	for _, signature := range signatures {
 		go func(sig types.Signature) {
@@ -841,21 +841,24 @@ func (x *XDPoS_v2) verifyQC(blockChainReader consensus.ChainReader, quorumCert *
 			}), sig, epochInfo.Masternodes)
 			if err != nil {
 				log.Error("[verifyQC] Error while verfying QC message signatures", "Error", err)
-				haveError = errors.New("error while verfying QC message signatures")
+				errChan <- errors.New("error while verfying QC message signatures")
 				return
 			}
 			if !verified {
 				log.Warn("[verifyQC] Signature not verified doing QC verification", "QC", quorumCert)
-				haveError = errors.New("fail to verify QC due to signature mis-match")
+				errChan <- errors.New("fail to verify QC due to signature mis-match")
 				return
 			}
 		}(signature)
 	}
 	wg.Wait()
+	close(errChan)
 	elapsed := time.Since(start)
 	log.Debug("[verifyQC] time verify message signatures of qc", "elapsed", elapsed)
-	if haveError != nil {
-		return haveError
+	for err := range errChan {
+		if err != nil {
+			return err
+		}
 	}
 	epochSwitchNumber := epochInfo.EpochSwitchBlockInfo.Number.Uint64()
 	gapNumber := epochSwitchNumber - epochSwitchNumber%x.config.Epoch - x.config.Gap

consensus/XDPoS/engines/engine_v2/forensics.go

@@ -8,6 +8,7 @@ import (
 	"reflect"
 	"strconv"
 	"strings"
+	"sync"
 
 	"github.com/XinFinOrg/XDPoSChain/common"
 	"github.com/XinFinOrg/XDPoSChain/consensus"
@@ -24,6 +25,7 @@ const (
 
 // Forensics instance. Placeholder for future properties to be added
 type Forensics struct {
+	mu                  sync.RWMutex
 	HighestCommittedQCs []types.QuorumCert
 	forensicsFeed       event.Feed
 	scope               event.SubscriptionScope
@@ -63,20 +65,26 @@ func (f *Forensics) SetCommittedQCs(headers []types.Header, incomingQC types.Quo
 			return err
 		}
 		if i != 0 {
-			if decodedExtraField.QuorumCert.ProposedBlockInfo.Hash != headers[i-1].Hash() {
-				log.Error("[SetCommittedQCs] Headers shall be on the same chain and in the right order", "parentHash", h.ParentHash.Hex(), "headers[i-1].Hash()", headers[i-1].Hash().Hex())
+			prevHash := headers[i-1].Hash()
+			prevHashNoVal := headers[i-1].HashNoValidator()
+			if decodedExtraField.QuorumCert.ProposedBlockInfo.Hash != prevHash && decodedExtraField.QuorumCert.ProposedBlockInfo.Hash != prevHashNoVal {
+				log.Error("[SetCommittedQCs] Headers shall be on the same chain and in the right order", "headers[i-1].Hash()", prevHash.Hex(), "headers[i-1].HashNoValidator()", prevHashNoVal.Hex(), "QC.Hash", decodedExtraField.QuorumCert.ProposedBlockInfo.Hash.Hex())
 				return errors.New("headers shall be on the same chain and in the right order")
 			} else if i == len(headers)-1 { // The last header shall be pointed by the incoming QC
-				if incomingQC.ProposedBlockInfo.Hash != h.Hash() {
-					log.Error("[SetCommittedQCs] incomingQc is not pointing at the last header received", "hash", h.Hash().Hex(), "incomingQC.ProposedBlockInfo.Hash", incomingQC.ProposedBlockInfo.Hash.Hex())
+				currentHash := h.Hash()
+				currentHashNoVal := h.HashNoValidator()
+				if incomingQC.ProposedBlockInfo.Hash != currentHash && incomingQC.ProposedBlockInfo.Hash != currentHashNoVal {
+					log.Error("[SetCommittedQCs] incomingQc is not pointing at the last header received", "hash", currentHash.Hex(), "hashNoVal", currentHashNoVal.Hex(), "incomingQC.ProposedBlockInfo.Hash", incomingQC.ProposedBlockInfo.Hash.Hex())
 					return errors.New("incomingQc is not pointing at the last header received")
 				}
 			}
 		}
 
 		committedQCs = append(committedQCs, *decodedExtraField.QuorumCert)
 	}
+	f.mu.Lock()
 	f.HighestCommittedQCs = append(committedQCs, incomingQC)
+	f.mu.Unlock()
 	return nil
 }
 
@@ -90,7 +98,10 @@ func (f *Forensics) ProcessForensics(chain consensus.ChainReader, engine *XDPoS_
 	return nil
 	log.Debug("Received a QC in forensics", "QC", incomingQC)
 	// Clone the values to a temporary variable
-	highestCommittedQCs := f.HighestCommittedQCs
+	f.mu.RLock()
+	highestCommittedQCs := make([]types.QuorumCert, len(f.HighestCommittedQCs))
+	copy(highestCommittedQCs, f.HighestCommittedQCs)
+	f.mu.RUnlock()
 	if len(highestCommittedQCs) != NUM_OF_FORENSICS_QC {
 		log.Error("[ProcessForensics] HighestCommittedQCs value not set", "incomingQcProposedBlockHash", incomingQC.ProposedBlockInfo.Hash, "incomingQcProposedBlockNumber", incomingQC.ProposedBlockInfo.Number.Uint64(), "incomingQcProposedBlockRound", incomingQC.ProposedBlockInfo.Round)
 		return errors.New("HighestCommittedQCs value not set")
@@ -398,7 +409,10 @@ func (f *Forensics) ProcessVoteEquivocation(chain consensus.ChainReader, engine
 	return nil
 	log.Debug("Received a vote in forensics", "vote", incomingVote)
 	// Clone the values to a temporary variable
-	highestCommittedQCs := f.HighestCommittedQCs
+	f.mu.RLock()
+	highestCommittedQCs := make([]types.QuorumCert, len(f.HighestCommittedQCs))
+	copy(highestCommittedQCs, f.HighestCommittedQCs)
+	f.mu.RUnlock()
 	if len(highestCommittedQCs) != NUM_OF_FORENSICS_QC {
 		log.Error("[ProcessVoteEquivocation] HighestCommittedQCs value not set", "incomingVoteProposedBlockHash", incomingVote.ProposedBlockInfo.Hash, "incomingVoteProposedBlockNumber", incomingVote.ProposedBlockInfo.Number.Uint64(), "incomingVoteProposedBlockRound", incomingVote.ProposedBlockInfo.Round)
 		return errors.New("HighestCommittedQCs value not set")

core/types/block.go

@@ -98,12 +98,17 @@ type headerMarshaling struct {
 	Hash       common.Hash `json:"hash"` // adds call to Hash() in MarshalJSON
 }
 
-// Hash returns the block hash of the header, which is simply the keccak256 hash of its
+// Hash returns the block hash of the header, which is the keccak256 hash of its
 // RLP encoding.
 func (h *Header) Hash() common.Hash {
 	return rlpHash(h)
 }
 
+// HashWithValidator returns the block hash of the header including the validator signature.
+func (h *Header) HashWithValidator() common.Hash {
+	return rlpHash(h)
+}
+
 // HashNoNonce returns the hash which is used as input for the proof-of-work search.
 func (h *Header) HashNoNonce() common.Hash {
 	return rlpHash([]interface{}{
@@ -123,7 +128,7 @@ func (h *Header) HashNoNonce() common.Hash {
 	})
 }
 
-// HashNoNonce returns the hash which is used as input for the proof-of-work search.
+// HashNoValidator returns the block hash of the header, excluding the validator signature.
 func (h *Header) HashNoValidator() common.Hash {
 	return rlpHash([]interface{}{
 		h.ParentHash,