Lighthouse: Eth1 data underflow

[paulhauner]

https://github.com/sigp/lighthouse/blob/192380cb58c4e0803f9b787d033bc78fd0e9ea49/eth2/state_processing/src/per_block_processing.rs#L380-L383

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 https://github.com/rust-lang/rust-clippy/pull/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! 💡