Skip to content

Fix potential DoS vector for masternode payments #2071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
UdjinM6 merged 2 commits into from
May 26, 2018
Merged

Fix potential DoS vector for masternode payments #2071

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b06abb
Skip only already verified mn payments vote duplicates
UdjinM6 May 7, 2018
8818b50
Update mn payment vote stats only if mn payment vote sig is ok
UdjinM6 May 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/masternode-payments.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,8 @@ void CMasternodePayments::ProcessMessage(CNode* pfrom, const std::string& strCom

auto res = mapMasternodePaymentVotes.emplace(nHash, vote);

// Avoid processing same vote multiple times
if(!res.second) {
// Avoid processing same vote multiple times if it was already verified earlier
if(!res.second && res.first->second.IsVerified()) {
Copy link

@sidhujag sidhujag May 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually solve the problem. If an attacker uses a bad signature it will pass and add to mapMasternodePaymentVotes yet vchsig will be non-empty and this check is just a NOP. Don't you need to move this under the sig check like I did in my initial PR?

Copy link

@sidhujag sidhujag May 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see so vchsig will be cleared anyway and this will basically reprocess if it was a bad vote that was stored and for a good vote vchsig would have been filled later so it returns right away, makes sense thanks!

LogPrint("mnpayments", "MASTERNODEPAYMENTVOTE -- hash=%s, nBlockHeight=%d/%d seen\n",
nHash.ToString(), vote.nBlockHeight, nCachedBlockHeight);
return;
Expand All @@ -375,11 +375,6 @@ void CMasternodePayments::ProcessMessage(CNode* pfrom, const std::string& strCom
return;
}

if(!UpdateLastVote(vote)) {
LogPrintf("MASTERNODEPAYMENTVOTE -- masternode already voted, masternode=%s\n", vote.masternodeOutpoint.ToStringShort());
return;
}

masternode_info_t mnInfo;
if(!mnodeman.GetMasternodeInfo(vote.masternodeOutpoint, mnInfo)) {
// mn was not found, so we can't check vote, some info is probably missing
Expand Down Expand Up @@ -407,6 +402,11 @@ void CMasternodePayments::ProcessMessage(CNode* pfrom, const std::string& strCom
return;
}

if(!UpdateLastVote(vote)) {
LogPrintf("MASTERNODEPAYMENTVOTE -- masternode already voted, masternode=%s\n", vote.masternodeOutpoint.ToStringShort());
return;
}

CTxDestination address1;
ExtractDestination(vote.payee, address1);
CBitcoinAddress address2(address1);
Expand Down