[SEC] Unbounded queued attestations allows to crash nbc nodes

[montyly]

Unbounded queued attestations allows to crash nbc nodes

Severity: Medium Difficulty: Low

When an attestation is received, it will be stored as unresolved if its related block is not found: https://github.com/status-im/nim-beacon-chain/blob/3433c77c35f3025729ac9204ab6b91ea136dce9e/beacon_chain/attestation_pool.nim#L81-L85

If the attestation is for future checkpoints, it will be queued: https://github.com/status-im/nim-beacon-chain/blob/3433c77c35f3025729ac9204ab6b91ea136dce9e/beacon_chain/fork_choice/fork_choice.nim#L191-L195

Both operations lead to store unbounded attestations.

As a result an attacker can trigger an out-of-memory on nbc nodes by sending malicious attestations.

Nodes on machine with limited resources, such as raspberry pi, will have a higher likelihood to crash

This issue is similar to https://github.com/status-im/nim-beacon-chain/issues/1553

Exploit Scenario

Eve generates hundred of thousand of malicious attestations and spam the network. All the nbc nodes crash by being out-of-memory

Recommendation

Short term, limit the number of attestations that can be queued in pool.unresolved and queued_attestations.

Long term, thoroughly document every queued mechanism and take into consideration the impact of malicious actors on the queued attestations.

[tersec]

https://github.com/status-im/nim-beacon-chain/pull/1568 removes the unresolved attestation queue entirely.

[tersec]

As regards future checkpoints, those code's only reached for resolved attestations, which have been verified to follow all the protocol rules. In particular, this means at most one attestation per active validator per epoch, and it's an attestation that, in theory should be picked up by an aggregator and a block proposer too.

The flow is that https://github.com/status-im/nimbus-eth2/blob/91741326cc529f02817a1887180316a969b76998/beacon_chain/attestation_pool.nim#L91-L101

appears to be the only place in nbc which calls fork choice's on_attestation. In turn, addForkChoiceVotes is called only from: https://github.com/status-im/nimbus-eth2/blob/91741326cc529f02817a1887180316a969b76998/beacon_chain/attestation_pool.nim#L204-L228

To the extent this might be exploitable, it would require a flaw in the attestation extended validation and resolving code, not the queuing mechanism as such, of which this would be relatively minor fallout.

This extended validation checks, in particular, that it's not from the future: https://github.com/status-im/nimbus-eth2/blob/91741326cc529f02817a1887180316a969b76998/beacon_chain/attestation_aggregation.nim#L175-L180 and https://github.com/status-im/nimbus-eth2/blob/91741326cc529f02817a1887180316a969b76998/beacon_chain/attestation_aggregation.nim#L303-L307

which check that it's not from the future: https://github.com/status-im/nimbus-eth2/blob/91741326cc529f02817a1887180316a969b76998/beacon_chain/attestation_aggregation.nim#L78-L86

[montyly]

While the attack scenario on queued_attestations is indeed less likely than on pool.unresolved, I would still recommend considering adding a limit to the queue. Especially as the codebase is still evolving.

This would prevent a scenario where a node is late in the synchronization or has a weak view of the network, and end up collecting a lot of attestations.

[arnetheduck]

While there have been various improvements to the attestation queueing, including a priority queue that dynamically dropped attestations based on load and other metrics, the current batched attestation queue performs a similar task, pruning the queue when the node can no longer keep up, thus putting an additional bound on the queue