[SEC] Unchecked overflow in beaconstate.check_attestation_inclusion

[montyly]

Severity: Low
Difficulty: High

Description

An unchecked overflow in beaconstate.check_attestation_inclusion can lead the first epoch to accept incorrect attestations.

When receiving an attestation, the eth 2.0 spec checks for:

assert data.slot + MIN_ATTESTATION_INCLUSION_DELAY <= state.slot <= data.slot + SLOTS_PER_EPOCH

beaconstate.check_attestation_inclusion performs the operations without overflow checks:
https://github.com/status-im/nim-beacon-chain/blob/3433c77c35f3025729ac9204ab6b91ea136dce9e/beacon_chain/spec/beaconstate.nim#L539-L547

If current_slot is below 32 and data.slot is the max uint64:

• data.slot + MIN_ATTESTATION_INCLUSION_DELAY will overflow, and data.slot + MIN_ATTESTATION_INCLUSION_DELAY <= current_slot will return true
• data.slot + SLOTS_PER_EPOCH will overflow, while current_slot <= data.slot + SLOTS_PER_EPOCH will still return true

As a result, the attestation will be considered as valid by the nbc nodes, while it will be considered as invalid by the other eth 2.0 clients.

This attack can only happen on the first epoch.

Exploit Scenario

On the first epoch, Eve spams the network with invalid attestation. The attestation are accepted by the nbc nodes and rejected by the other nodes. As a result, the nbc nodes spend resources on invalid attestations.

Recommendation

Short term, check for overflow in beaconstate.check_attestation_inclusion.

Long term, identify all the integers that can be controlled by an attacker, and carefully checks the related operations for overflow and underflow

[tersec]

#1600 addresses this.

[montyly]

Fixed in #1600