Skip to content

consensus: peer can be marked good after a single message #2048

@ebuchman

Description

@ebuchman

Reported by an auditor.

PeerState.Stats are used to track how many votes/blockparts for a unique height a peer has given us. When they hit 10k, we mark the peer as "good" and preferentially connect to it.

We initialize PeerState.Stats with zeros, and then only update if the Height of the vote/blockpart is higher than the latest:

func (ps *PeerState) RecordVote(vote *types.Vote) int {
ps.mtx.Lock()
defer ps.mtx.Unlock()
if ps.Stats.LastVoteHeight >= vote.Height {
return ps.Stats.Votes
}

Problem is we don't validate that votes/parts contain non-negative values for height. If the first msg from a peer is a vote with negative height, we'll return the latest value, which is 0. But when we check the return value, we check % 10000 == 0, which passes for 0, so we mark the peer good right away!

Note we want to use % and not > because peers can become "ungood" probabilistically if there's lots of good peers and they should have a chance to keep proving themselves. Alternatively we could use > 10000 and just reset the counter to 0 every time a peer is marked good.

Main thing we need to fix is validating the reactor messages so we don't have negative numbers. But we should also make sure the logic is more defensive, like a modulus guarding a "marked good" should be smarter about accidentally being triggered right away.

Metadata

Metadata

Assignees

Labels

C:consensusComponent: ConsensusC:p2pComponent: P2P pkgT:bugType Bug (Confirmed)T:securityType: Security (specify priority)

Type

No type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions