Fix vulnerability with mapMasternodeOrphanObjects (#1512)

* fix vulnerability with mapMasternodeOrphanObjects

The vulnerability is that a malicious node can send a lot of NetMsgType::MNGOVERNANCEOBJECT messages which refer to many arbitrary MN's. In this case, mapMasternodeOrphanObjects will grow unrestrictedly.

* MN collateral moved to governance-object.cpp; ban score applied to misbehaving nodes

* recursive locks removed

* check for the mn collateral code segregated to a separate function

* CheckCollateral implementation moved to cpp

src/governance-object.cpp

@@ -427,6 +427,7 @@ std::string CGovernanceObject::GetDataAsString()
 
 void CGovernanceObject::UpdateLocalValidity()
 {
+    LOCK(cs_main);
     // THIS DOES NOT CHECK COLLATERAL, THIS IS CHECKED UPON ORIGINAL ARRIVAL
     fCachedLocalValidity = IsValidLocally(strLocalValidityError, false);
 };
@@ -469,8 +470,17 @@ bool CGovernanceObject::IsValidLocally(std::string& strError, bool& fMissingMast
             std::string strOutpoint = vinMasternode.prevout.ToStringShort();
             masternode_info_t infoMn = mnodeman.GetMasternodeInfo(vinMasternode);
             if(!infoMn.fInfoValid) {
+
+                CMasternode::CollateralStatus err = CMasternode::CheckCollateral(GetMasternodeVin());
+                if (err == CMasternode::COLLATERAL_OK) {
                     fMissingMasternode = true;
                     strError = "Masternode not found: " + strOutpoint;
+                } else if (err == CMasternode::COLLATERAL_UTXO_NOT_FOUND) {
+                    strError = "Failed to find Masternode UTXO, missing masternode=" + GetMasternodeVin().prevout.ToStringShort() + "\n";
+                } else if (err == CMasternode::COLLATERAL_INVALID_AMOUNT) {
+                    strError = "Masternode UTXO should have 1000 DASH, missing masternode=" + GetMasternodeVin().prevout.ToStringShort() + "\n";
+                }
+
                 return false;
             }
 
@@ -579,7 +589,7 @@ bool CGovernanceObject::IsCollateralValid(std::string& strError, bool& fMissingC
 
     // GET CONFIRMATIONS FOR TRANSACTION
 
-    LOCK(cs_main);
+    AssertLockHeld(cs_main);
     int nConfirmationsIn = GetIXConfirmations(nCollateralHash);
     if (nBlockHash != uint256()) {
         BlockMap::iterator mi = mapBlockIndex.find(nBlockHash);

src/governance.cpp

@@ -206,14 +206,27 @@ void CGovernanceManager::ProcessMessage(CNode* pfrom, std::string& strCommand, C
 
         if(!fIsValid) {
             if(fMasternodeMissing) {
-                mapMasternodeOrphanObjects.insert(std::make_pair(nHash, object_time_pair_t(govobj, GetAdjustedTime() + GOVERNANCE_ORPHAN_EXPIRATION_TIME)));
+
+                int& count = mapMasternodeOrphanCounter[govobj.GetMasternodeVin().prevout];
+                if (count >= 10) {
+                    LogPrint("gobject", "MNGOVERNANCEOBJECT -- Too many orphan objects, missing masternode=%s\n", govobj.GetMasternodeVin().prevout.ToStringShort());
+                    // ask for this object again in 2 minutes
+                    CInv inv(MSG_GOVERNANCE_OBJECT, govobj.GetHash());
+                    pfrom->AskFor(inv);
+                    return;
+                }
+
+                count++;
+                ExpirationInfo info(pfrom->GetId(), GetAdjustedTime() + GOVERNANCE_ORPHAN_EXPIRATION_TIME);
+                mapMasternodeOrphanObjects.insert(std::make_pair(nHash, object_info_pair_t(govobj, info)));
                 LogPrintf("MNGOVERNANCEOBJECT -- Missing masternode for: %s, strError = %s\n", strHash, strError);
             } else if(fMissingConfirmations) {
                 AddPostponedObject(govobj);
                 LogPrintf("MNGOVERNANCEOBJECT -- Not enough fee confirmations for: %s, strError = %s\n", strHash, strError);
             } else {
                 LogPrintf("MNGOVERNANCEOBJECT -- Governance object is invalid - %s\n", strError);
-                // TODO: apply node's ban score if object is invalid
+                // apply node's ban score
+                Misbehaving(pfrom->GetId(), 20);
             }
 
             return;
@@ -1023,31 +1036,32 @@ void CGovernanceManager::CheckMasternodeOrphanObjects()
     LOCK2(cs_main, cs);
     int64_t nNow = GetAdjustedTime();
     CRateChecksGuard guard(false, *this);
-    object_time_m_it it = mapMasternodeOrphanObjects.begin();
+    object_info_m_it it = mapMasternodeOrphanObjects.begin();
     while(it != mapMasternodeOrphanObjects.end()) {
-        object_time_pair_t& pair = it->second;
+        object_info_pair_t& pair = it->second;
         CGovernanceObject& govobj = pair.first;
 
-        if(pair.second < nNow) {
+        if(pair.second.nExpirationTime >= nNow) {
-            mapMasternodeOrphanObjects.erase(it++);
-            continue;
-        }
-
             string strError;
             bool fMasternodeMissing = false;
             bool fConfirmationsMissing = false;
             bool fIsValid = govobj.IsValidLocally(strError, fMasternodeMissing, fConfirmationsMissing, true);
-        if(!fIsValid) {
+
-            if(!fMasternodeMissing) {
+            if(fIsValid) {
-                mapMasternodeOrphanObjects.erase(it++);
+                AddGovernanceObject(govobj);
-            }
+            } else if(fMasternodeMissing) {
-            else {
                 ++it;
-            }
                 continue;
             }
+        } else {
+            // apply node's ban score
+            Misbehaving(pair.second.idFrom, 20);
+        }
+
+        auto it_count = mapMasternodeOrphanCounter.find(govobj.GetMasternodeVin().prevout);
+        if(--it_count->second == 0)
+            mapMasternodeOrphanCounter.erase(it_count);
 
-        AddGovernanceObject(govobj);
         mapMasternodeOrphanObjects.erase(it++);
     }
 }

src/governance.h

@@ -26,7 +26,14 @@ class CGovernanceVote;
 
 extern CGovernanceManager governance;
 
-typedef std::pair<CGovernanceObject, int64_t> object_time_pair_t;
+struct ExpirationInfo {
+    ExpirationInfo(int64_t _nExpirationTime, int _idFrom) : nExpirationTime(_nExpirationTime), idFrom(_idFrom) {}
+
+    int64_t nExpirationTime;
+    NodeId idFrom;
+};
+
+typedef std::pair<CGovernanceObject, ExpirationInfo> object_info_pair_t;
 
 static const int RATE_BUFFER_SIZE = 5;
 
@@ -197,17 +204,19 @@ public: // Types
 
     typedef txout_m_t::const_iterator txout_m_cit;
 
+    typedef std::map<COutPoint, int> txout_int_m_t;
+
     typedef std::set<uint256> hash_s_t;
 
     typedef hash_s_t::iterator hash_s_it;
 
     typedef hash_s_t::const_iterator hash_s_cit;
 
-    typedef std::map<uint256, object_time_pair_t> object_time_m_t;
+    typedef std::map<uint256, object_info_pair_t> object_info_m_t;
 
-    typedef object_time_m_t::iterator object_time_m_it;
+    typedef object_info_m_t::iterator object_info_m_it;
 
-    typedef object_time_m_t::const_iterator object_time_m_cit;
+    typedef object_info_m_t::const_iterator object_info_m_cit;
 
     typedef std::map<uint256, int64_t> hash_time_m_t;
 
@@ -237,7 +246,8 @@ private:
     //   value - expiration time for deleted objects
     hash_time_m_t mapErasedGovernanceObjects;
 
-    object_time_m_t mapMasternodeOrphanObjects;
+    object_info_m_t mapMasternodeOrphanObjects;
+    txout_int_m_t mapMasternodeOrphanCounter;
 
     object_m_t mapPostponedObjects;
     hash_s_t setAdditionalRelayObjects;

src/masternode.cpp

@@ -161,6 +161,31 @@ arith_uint256 CMasternode::CalculateScore(const uint256& blockHash)
     return (hash3 > hash2 ? hash3 - hash2 : hash2 - hash3);
 }
 
+CMasternode::CollateralStatus CMasternode::CheckCollateral(CTxIn vin)
+{
+    int nHeight;
+    return CheckCollateral(vin, nHeight);
+}
+
+CMasternode::CollateralStatus CMasternode::CheckCollateral(CTxIn vin, int& nHeight)
+{
+    AssertLockHeld(cs_main);
+
+    CCoins coins;
+    if(!pcoinsTip->GetCoins(vin.prevout.hash, coins) ||
+       (unsigned int)vin.prevout.n>=coins.vout.size() ||
+       coins.vout[vin.prevout.n].IsNull()) {
+        return COLLATERAL_UTXO_NOT_FOUND;
+    }
+
+    if(coins.vout[vin.prevout.n].nValue != 1000 * COIN) {
+        return COLLATERAL_INVALID_AMOUNT;
+    }
+
+    nHeight = coins.nHeight;
+    return COLLATERAL_OK;
+}
+
 void CMasternode::Check(bool fForce)
 {
     LOCK(cs);
@@ -180,10 +205,8 @@ void CMasternode::Check(bool fForce)
         TRY_LOCK(cs_main, lockMain);
         if(!lockMain) return;
 
-        CCoins coins;
+        CollateralStatus err = CheckCollateral(vin);
-        if(!pcoinsTip->GetCoins(vin.prevout.hash, coins) ||
+        if (err == COLLATERAL_UTXO_NOT_FOUND) {
-           (unsigned int)vin.prevout.n>=coins.vout.size() ||
-           coins.vout[vin.prevout.n].IsNull()) {
             nActiveState = MASTERNODE_OUTPOINT_SPENT;
             LogPrint("masternode", "CMasternode::Check -- Failed to find Masternode UTXO, masternode=%s\n", vin.prevout.ToStringShort());
             return;
@@ -632,18 +655,19 @@ bool CMasternodeBroadcast::CheckOutpoint(int& nDos)
             return false;
         }
 
-        CCoins coins;
+        int nHeight;
-        if(!pcoinsTip->GetCoins(vin.prevout.hash, coins) ||
+        CollateralStatus err = CheckCollateral(vin, nHeight);
-           (unsigned int)vin.prevout.n>=coins.vout.size() ||
+        if (err == COLLATERAL_UTXO_NOT_FOUND) {
-           coins.vout[vin.prevout.n].IsNull()) {
             LogPrint("masternode", "CMasternodeBroadcast::CheckOutpoint -- Failed to find Masternode UTXO, masternode=%s\n", vin.prevout.ToStringShort());
             return false;
         }
-        if(coins.vout[vin.prevout.n].nValue != 1000 * COIN) {
+
+        if (err == COLLATERAL_INVALID_AMOUNT) {
             LogPrint("masternode", "CMasternodeBroadcast::CheckOutpoint -- Masternode UTXO should have 1000 DASH, masternode=%s\n", vin.prevout.ToStringShort());
             return false;
         }
-        if(chainActive.Height() - coins.nHeight + 1 < Params().GetConsensus().nMasternodeMinimumConfirmations) {
+
+        if(chainActive.Height() - nHeight + 1 < Params().GetConsensus().nMasternodeMinimumConfirmations) {
             LogPrintf("CMasternodeBroadcast::CheckOutpoint -- Masternode UTXO must have at least %d confirmations, masternode=%s\n",
                     Params().GetConsensus().nMasternodeMinimumConfirmations, vin.prevout.ToStringShort());
             // maybe we miss few blocks, let this mnb to be checked again later

src/masternode.h

@@ -173,6 +173,12 @@ public:
         MASTERNODE_POSE_BAN
     };
 
+    enum CollateralStatus {
+        COLLATERAL_OK,
+        COLLATERAL_UTXO_NOT_FOUND,
+        COLLATERAL_INVALID_AMOUNT
+    };
+
     CTxIn vin;
     CService addr;
     CPubKey pubKeyCollateralAddress;
@@ -262,6 +268,8 @@ public:
 
     bool UpdateFromNewBroadcast(CMasternodeBroadcast& mnb);
 
+    static CollateralStatus CheckCollateral(CTxIn vin);
+    static CollateralStatus CheckCollateral(CTxIn vin, int& nHeight);
     void Check(bool fForce = false);
 
     bool IsBroadcastedWithin(int nSeconds) { return GetAdjustedTime() - sigTime < nSeconds; }

src/rpc/governance.cpp

@@ -153,9 +153,12 @@ UniValue gobject(const UniValue& params, bool fHelp)
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Trigger and watchdog objects need not be prepared (however only masternodes can create them)");
         }
 
+        {
+            LOCK(cs_main);
             std::string strError = "";
             if(!govobj.IsValidLocally(strError, false))
                 throw JSONRPCError(RPC_INTERNAL_ERROR, "Governance object is not valid - " + govobj.GetHash().ToString() + " - " + strError);
+        }
 
         CWalletTx wtx;
         if(!pwalletMain->GetBudgetSystemCollateralTX(wtx, govobj.GetHash(), govobj.GetMinCollateralFee(), false)) {
@@ -256,10 +259,13 @@ UniValue gobject(const UniValue& params, bool fHelp)
         std::string strError = "";
         bool fMissingMasternode;
         bool fMissingConfirmations;
+        {
+            LOCK(cs_main);
             if(!govobj.IsValidLocally(strError, fMissingMasternode, fMissingConfirmations, true) && !fMissingConfirmations) {
                 LogPrintf("gobject(submit) -- Object submission rejected because object is not valid - hash = %s, strError = %s\n", strHash, strError);
                 throw JSONRPCError(RPC_INTERNAL_ERROR, "Governance object is not valid - " + strHash + " - " + strError);
             }
+        }
 
         // RELAY THIS OBJECT
         // Reject if rate check fails but don't update buffer