[NCC-E005955-XVE] zebra-chain: Inconsistent error management in Add and Sub for Height

[mpguerra]

Motivation

We want to track all of the findings from the zebra audit.

Details

Some arithmetic operations for Height are implemented in zebra-chain/src/block/height.rs for both Height and i32

zebra/zebra-chain/src/block/height.rs

Lines 68 to 100 in 5a88fe7

	impl Add<Height> for Height {
	type Output = Option<Height>;
	fn add(self, rhs: Height) -> Option<Height> {
	// We know that both values are positive integers. Therefore, the result is
	// positive, and we can skip the conversions. The checked_add is required,
	// because the result may overflow.
	let height = self.0.checked_add(rhs.0)?;
	let height = Height(height);
	if height <= Height::MAX && height >= Height::MIN {
	Some(height)
	} else {
	None
	}
	}
	}
	impl Sub<Height> for Height {
	type Output = i32;
	/// Panics if the inputs or result are outside the valid i32 range.
	fn sub(self, rhs: Height) -> i32 {
	// We construct heights from integers without any checks,
	// so the inputs or result could be out of range.
	let lhs = i32::try_from(self.0)
	.expect("out of range input `self`: inputs should be valid Heights");
	let rhs =
	i32::try_from(rhs.0).expect("out of range input `rhs`: inputs should be valid Heights");
	lhs.checked_sub(rhs)
	.expect("out of range result: valid input heights should yield a valid result")
	}
	}

The Add function handles overflow with an Option , but Sub will panic. The Sub function later in the file for i32 subtraction returns an Option as well, making the panic behavior an outlier. Panics should be used as a last resort, when there is no possibility of recovery. Otherwise, an attacker may attempt to intentionally trigger a panic as part of a denial-of-service attack. Additionally, the Sub function does not enforce the same constraint checks shown on Line 73 above, unlike the other functions in the same file.

[mpguerra]

Hey team! Please add your planning poker estimate with Zenhub @arya2 @conradoplg @dconnolly @oxarbitrage @teor2345 @upbqdn

[teor2345]

The Add function handles overflow with an Option , but Sub will panic. The Sub function later in the file for i32 subtraction returns an Option as well, making the panic behavior an outlier.

We don't think this panic is possible with valid Heights, due to the range of the data types involved:
#6330 (comment)

However, since some code (particularly RPC code) might construct invalid heights, we're going to do some fixes anyway.

Additionally, the Sub function does not enforce the same constraint checks shown on Line 73 above, unlike the other functions in the same file.

The output type is not a Height, it's the difference between two heights. So the height range limits do not apply. But we'll make some fixes to make this clearer.

[mpguerra]

Do we need to review the estimate on this one? Seems like it ended up being bigger than a "size 2" issue. I don't necessarily want us to actually change the estimate retroactively but it might be interesting to discuss during the retro :)

[teor2345]

This refactor has already helped me discover a potential bug in my progress bar code.