Eth1 data underflow

[paulhauner]

lighthouse/eth2/state_processing/src/per_block_processing.rs

Lines 380 to 383 in 192380c

	let expected_deposit_len = std::cmp::min(
	T::MaxDeposits::to_u64(),
	state.eth1_data.deposit_count - state.eth1_deposit_index,
	);

If we accept that state.eth1_data is user-supplied input, I believe this subtraction is an underflow risk.

@michaelsproul, I'm keen to hear your thoughts.

[michaelsproul]

state.eth1_data is decided by majority voting, so the network would have to be pretty screwed for it to take on a malicious value. That said, better to be safe than sorry, we may as well change this to a saturating_sub.

More broadly, I was thinking of writing a clippy lint to detect and ban all plain additions and subtractions, which we could roll out across the types/state_processing crates. The logic being that we almost never want the wrapping behaviour of an overflowing add, and should be explicit (use wrapping_add) where we do.

[paulhauner]

I was thinking of writing a clippy lint to detect and ban all plain additions and subtractions

I'm also keen to give this a go.

[pscott]

@michaelsproul would this lint make the cut : https://rust-lang.github.io/rust-clippy/master/#integer_arithmetic ?

[michaelsproul]

Yeah, it kind of does, I started working with it on Thursday (and fixed a bug rust-lang/rust-clippy#4585). I’m now splitting it into one lint for detecting operations that can overflow, and another for detecting operations that have boundary issues (div or mod by 0, shift by >word size).

[ackintosh]

I'm working on this issue, will file a PR to close this. 😃

[ackintosh]

Hmm... should we return Err immediately if underflow is occurred in the subtraction instead of proceed the process with 0?

It looks a critical error if the underflow (state.eth1_data.deposit_count < state.eth1_deposit_index) is really happened but I'm not sure due to the lack of understanding about the process. 🤔💦

[michaelsproul]

On overflow the spec raises an exception, so you're right, we should error here rather than proceeding with 0 (see spec). It'll mean that no blocks can be processed until a manual intervention, but the attack requires ~50% of proposers to be compromised, which gets us into social fork territory anyway.

[ackintosh]

Thank you for the details! 💡