From: Matt Corallo <git@bluematt.me>
Date: Wed, 26 Jan 2022 00:10:19 +0000 (+0000)
Subject: Avoid overflow in addition when checking counterparty feerates
X-Git-Tag: v0.0.105~23^2
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=refs%2Fheads%2F2022-01-fuzz-overflow;p=rust-lightning

Avoid overflow in addition when checking counterparty feerates

This is harmless outside of debug builds - the feerate will
overflow causing it to either spuriously fail the first check, or
correctly pass it and fail the second check. In debug builds,
however, it panics due to integer overflow.

Found by the `full_stack_target` fuzz test in the
Chaincode-provided continuous fuzzing. Thanks Chaincode!
---

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 4b1f4a7d1..8daea4d75 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -870,14 +870,6 @@ impl<Signer: Sign> Channel<Signer> {
 	fn check_remote_fee<F: Deref>(fee_estimator: &F, feerate_per_kw: u32) -> Result<(), ChannelError>
 		where F::Target: FeeEstimator
 	{
-		let lower_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
-		// Some fee estimators round up to the next full sat/vbyte (ie 250 sats per kw), causing
-		// occasional issues with feerate disagreements between an initiator that wants a feerate
-		// of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. Thus, we always add 250
-		// sat/kw before the comparison here.
-		if feerate_per_kw + 250 < lower_limit {
-			return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {} (- 250)", feerate_per_kw, lower_limit)));
-		}
 		// We only bound the fee updates on the upper side to prevent completely absurd feerates,
 		// always accepting up to 25 sat/vByte or 10x our fee estimator's "High Priority" fee.
 		// We generally don't care too much if they set the feerate to something very high, but it
@@ -887,6 +879,14 @@ impl<Signer: Sign> Channel<Signer> {
 		if feerate_per_kw as u64 > upper_limit {
 			return Err(ChannelError::Close(format!("Peer's feerate much too high. Actual: {}. Our expected upper limit: {}", feerate_per_kw, upper_limit)));
 		}
+		let lower_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
+		// Some fee estimators round up to the next full sat/vbyte (ie 250 sats per kw), causing
+		// occasional issues with feerate disagreements between an initiator that wants a feerate
+		// of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. Thus, we always add 250
+		// sat/kw before the comparison here.
+		if feerate_per_kw + 250 < lower_limit {
+			return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {} (- 250)", feerate_per_kw, lower_limit)));
+		}
 		Ok(())
 	}
 
@@ -5859,6 +5859,13 @@ mod tests {
 		        "MAX_FUNDING_SATOSHIS is greater than all satoshis in existence");
 	}
 
+	#[test]
+	fn test_no_fee_check_overflow() {
+		// Previously, calling `check_remote_fee` with a fee of 0xffffffff would overflow in
+		// arithmetic, causing a panic with debug assertions enabled.
+		assert!(Channel::<InMemorySigner>::check_remote_fee(&&TestFeeEstimator { fee_est: 42 }, u32::max_value()).is_err());
+	}
+
 	struct Keys {
 		signer: InMemorySigner,
 	}