search

status-im / nimbus-eth2

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

[SEC] Underflow in state_transition_block.process_operations can lead to accept invalid blocks #1542

Closed montyly closed 3 years ago

montyly commented 4 years ago

Difficulty: Medium Severity: Medium

Description

A lack of underflow checks can lead nimbus nodes to accept invalid blocks.

A block will include the deposits that were made, and:

The eth2 specification states that the number of deposits made is checked to the minimal value between MAX_DEPOSITS, and deposit_count - eth1_deposit_index:

def process_operations(state: BeaconState, body: BeaconBlockBody) -> None:
    # Verify that outstanding deposits are processed up to the maximum number of deposits
    assert len(body.deposits) == min(MAX_DEPOSITS, state.eth1_data.deposit_count - state.eth1_deposit_index)

This check is performed in nimbus in : https://github.com/status-im/nim-beacon-chain/blob/3433c77c35f3025729ac9204ab6b91ea136dce9e/beacon_chain/spec/state_transition_block.nim#L294-L296

state.eth1_data.deposit_count and state.eth1_deposit_index are uint64. If state.eth1_data.deposit_count is lower than state.eth1_deposit_index, the subtraction will underflow, and req_deposits will be MAX_DEPOSITS. If there are MAX_DEPOSITS deposits, the block will be accepted by the nbc nodes.

The eth2 specification states that:

State transitions that cause a uint64 overflow or underflow are also considered invalid.

As a result, an attacker can craft a malicious block that will be accepted by the nbc nodes, and rejected by the other ones.

Exploit Scenario

Eve is the proposer. She sends an malicious block that is accepted by all the nbc nodes, and rejected by the other eth 2.0 clients. As a result, the nbc nodes progress on an invalid path.

Mitigation Recommendation

Short term, reject the block if state.eth1_data.deposit_count < state.eth1_deposit_index.

Long term, list all the potential underflow and overflow from the eth 2.0 specification, and carefully design nbc to be robust against those.

tersec commented 4 years ago

In general, the spec hasn't worried much about overflow, apparently figuring that it's far enough away it's not an immediate concern, and there was a concerted effort for some months to replace constructs such as if foo - bar > baz: with if foo > baz + bar for exactly this sort of underflow concern. Probably some other cases still exist and they should be sought out, indeed.

montyly commented 3 years ago

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