[Revive] Fix construction of negative zero SignedGas

prdoc/pr_10567.prdoc

@@ -0,0 +1,10 @@
+title: '[Revive] Fix construction of negative zero SignedGas'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Closes https://github.com/paritytech-secops/srlabs_findings/issues/603
+
+    This fixes an issue where a zero `SignedGas` value can be constructed that uses the `Negative` variant. The rest of the code relies on the invariant that zero `SignedGas` has always the `Positive` variant and this is also documented in the code.
+crates:
+- name: pallet-revive
+  bump: patch

substrate/frame/revive/src/metering/gas.rs

@@ -45,6 +45,17 @@ impl<T: Config> Default for SignedGas<T> {
 }
 
 impl<T: Config> SignedGas<T> {
+	/// Safely construct a negative `SignedGas` amount
+	///
+	/// Ensures the invariant that `Negative` must not be used for zero
+	pub fn safe_new_negative(amount: BalanceOf<T>) -> Self {
+		if amount == Default::default() {
+			Positive(amount)
+		} else {
+			Negative(amount)
+		}
+	}
+
 	/// Transform a weight fee into a gas amount.
 	pub fn from_weight_fee(weight_fee: BalanceOf<T>) -> Self {
 		Self::Positive(weight_fee)
@@ -65,8 +76,8 @@ impl<T: Config> SignedGas<T> {
 
 		match deposit {
 			StorageDeposit::Charge(amount) => Positive(multiplier.saturating_mul_int(*amount)),
-			StorageDeposit::Refund(amount) if *amount == Default::default() => Positive(*amount),
-			StorageDeposit::Refund(amount) => Negative(multiplier.saturating_mul_int(*amount)),
+			StorageDeposit::Refund(amount) =>
+				Self::safe_new_negative(multiplier.saturating_mul_int(*amount)),
 		}
 	}
 
@@ -108,16 +119,16 @@ impl<T: Config> SignedGas<T> {
 	pub fn saturating_add(&self, rhs: &Self) -> Self {
 		match (self, rhs) {
 			(Positive(lhs), Positive(rhs)) => Positive(lhs.saturating_add(*rhs)),
-			(Negative(lhs), Negative(rhs)) => Negative(lhs.saturating_add(*rhs)),
+			(Negative(lhs), Negative(rhs)) => Self::safe_new_negative(lhs.saturating_add(*rhs)),
 			(Positive(lhs), Negative(rhs)) =>
 				if lhs >= rhs {
 					Positive(lhs.saturating_sub(*rhs))
 				} else {
-					Negative(rhs.saturating_sub(*lhs))
+					Self::safe_new_negative(rhs.saturating_sub(*lhs))
 				},
 			(Negative(lhs), Positive(rhs)) =>
 				if lhs > rhs {
-					Negative(lhs.saturating_sub(*rhs))
+					Self::safe_new_negative(lhs.saturating_sub(*rhs))
 				} else {
 					Positive(rhs.saturating_sub(*lhs))
 				},
@@ -128,16 +139,16 @@ impl<T: Config> SignedGas<T> {
 	pub fn saturating_sub(&self, rhs: &Self) -> Self {
 		match (self, rhs) {
 			(Positive(lhs), Negative(rhs)) => Positive(lhs.saturating_add(*rhs)),
-			(Negative(lhs), Positive(rhs)) => Negative(lhs.saturating_add(*rhs)),
+			(Negative(lhs), Positive(rhs)) => Self::safe_new_negative(lhs.saturating_add(*rhs)),
 			(Positive(lhs), Positive(rhs)) =>
 				if lhs >= rhs {
 					Positive(lhs.saturating_sub(*rhs))
 				} else {
-					Negative(rhs.saturating_sub(*lhs))
+					Self::safe_new_negative(rhs.saturating_sub(*lhs))
 				},
 			(Negative(lhs), Negative(rhs)) =>
 				if lhs > rhs {
-					Negative(lhs.saturating_sub(*rhs))
+					Self::safe_new_negative(lhs.saturating_sub(*rhs))
 				} else {
 					Positive(rhs.saturating_sub(*lhs))
 				},
@@ -147,10 +158,10 @@ impl<T: Config> SignedGas<T> {
 	// Determine the minimum of two signed gas values.
 	pub fn min(&self, other: &Self) -> Self {
 		match (self, other) {
-			(Positive(_), Negative(rhs)) => Negative(*rhs),
-			(Negative(lhs), Positive(_)) => Negative(*lhs),
+			(Positive(_), Negative(rhs)) => Self::safe_new_negative(*rhs),
+			(Negative(lhs), Positive(_)) => Self::safe_new_negative(*lhs),
 			(Positive(lhs), Positive(rhs)) => Positive((*lhs).min(*rhs)),
-			(Negative(lhs), Negative(rhs)) => Negative((*lhs).max(*rhs)),
+			(Negative(lhs), Negative(rhs)) => Self::safe_new_negative((*lhs).max(*rhs)),
 		}
 	}
 }

substrate/frame/revive/src/metering/tests.rs

@@ -43,6 +43,24 @@ enum Charge {
 	D(i64),
 }
 
+#[test]
+fn test_deposit_calculation() {
+	use super::SignedGas;
+
+	ExtBuilder::default()
+		.with_next_fee_multiplier(FixedU128::from_rational(2, 1))
+		.build()
+		.execute_with(|| {
+			let deposit1 = StorageDeposit::Refund(10);
+			let gas_result1 = SignedGas::<Test>::from_adjusted_deposit_charge(&deposit1);
+			assert_eq!(gas_result1, SignedGas::Negative(BalanceOf::<Test>::from(5u32)));
+
+			let deposit2 = StorageDeposit::Refund(1);
+			let gas_result2 = SignedGas::<Test>::from_adjusted_deposit_charge(&deposit2);
+			assert_eq!(gas_result2, SignedGas::Positive(BalanceOf::<Test>::from(0u32)));
+		});
+}
+
 #[test_case(FixtureType::Solc   , "DepositPrecompile" ; "solc precompiles")]
 #[test_case(FixtureType::Resolc , "DepositPrecompile" ; "resolc precompiles")]
 #[test_case(FixtureType::Solc   , "DepositDirect" ; "solc direct")]