Enable arithmetic lint in rate-limiter

[michaelsproul]

The rate-limiter is called prior to application-level checks on request validity, so we should be careful to avoid integer overflow in its code, as it is operating on untrusted data.

There is integer overflow here:

lighthouse/beacon_node/lighthouse_network/src/rpc/rate_limiter.rs

Lines 416 to 426 in 029b4f2

	pub fn allows(
	&mut self,
	time_since_start: Duration,
	key: &Key,
	tokens: u64,
	) -> Result<(), RateLimitedErr> {
	let time_since_start = time_since_start.as_nanos() as u64;
	let tau = self.tau;
	let t = self.t;
	// how long does it take to replenish these tokens
	let additional_time = t * tokens;

The tokens value is derived from untrusted user input:

lighthouse/beacon_node/lighthouse_network/src/rpc/rate_limiter.rs

Lines 327 to 333 in 029b4f2

	pub fn allows<Item: RateLimiterItem>(
	&mut self,
	peer_id: &PeerId,
	request: &Item,
	) -> Result<(), RateLimitedErr> {
	let time_since_start = self.init_time.elapsed();
	let tokens = request.max_responses().max(1);

The count from the request is untrusted and can be set arbitrarily:

lighthouse/beacon_node/lighthouse_network/src/rpc/protocol.rs

Lines 794 to 811 in 029b4f2

	pub fn max_responses(&self) -> u64 {
	match self {
	RequestType::Status(_) => 1,
	RequestType::Goodbye(_) => 0,
	RequestType::BlocksByRange(req) => *req.count(),
	RequestType::BlocksByRoot(req) => req.block_roots().len() as u64,
	RequestType::BlobsByRange(req) => req.max_blobs_requested::<E>(),
	RequestType::BlobsByRoot(req) => req.blob_ids.len() as u64,
	RequestType::DataColumnsByRoot(req) => req.data_column_ids.len() as u64,
	RequestType::DataColumnsByRange(req) => req.max_requested::<E>(),
	RequestType::Ping(_) => 1,
	RequestType::MetaData(_) => 1,
	RequestType::LightClientBootstrap(_) => 1,
	RequestType::LightClientOptimisticUpdate => 1,
	RequestType::LightClientFinalityUpdate => 1,
	RequestType::LightClientUpdatesByRange(req) => req.count,
	}
	}

This was briefly fixed for BlobsByRange requests on unstable when we implemented a size check during decoding, but will regress again when this PR is merged:

• Fork aware max values in rpc #6847

The fix that existed in unstable was coincidental, so I think it is better if we address the root cause and avoid overflowing integer arithmetic in the rate limiter altogether. We can probably replace most uses of +, -, * by their saturating_ equivalents to get the right semantics.

We can also enable the arithmetic_side_effects lint, which we use in consensus crates. See:

• Expand checked arithmetic to more crates #1013

This issue is not deemed security-sensitive because other mechanisms exist to defend the node from peers that would bypass the rate limiter. Any request with an invalid count will be rejected by application-level checks and result in the peer being quickly banned.

Thanks to @gln for reporting this bug as part of the Immunefi Ethereum Attackathon.

[jimmygchen]

Addressed in #7025 🎉