search

status-im / nimbus-eth2

Nim implementation of the Ethereum Beacon Chain
https://nimbus.guide
Other
543 stars 233 forks source link

[SEC] Unchecked overflow in beaconstate.check_attestation_inclusion #1580

Closed montyly closed 3 years ago

montyly commented 4 years ago

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:

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 commented 4 years ago

https://github.com/status-im/nim-beacon-chain/pull/1600 addresses this.

montyly commented 3 years ago

Fixed in https://github.com/status-im/nimbus-eth2/pull/1600