diff --git a/prdoc/pr_10567.prdoc b/prdoc/pr_10567.prdoc new file mode 100644 index 0000000000000..4ce93b38e4b23 --- /dev/null +++ b/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 diff --git a/substrate/frame/revive/src/metering/gas.rs b/substrate/frame/revive/src/metering/gas.rs index 4e51c82c57ade..293be5d22a2f4 100644 --- a/substrate/frame/revive/src/metering/gas.rs +++ b/substrate/frame/revive/src/metering/gas.rs @@ -45,6 +45,17 @@ impl Default for SignedGas { } impl SignedGas { + /// Safely construct a negative `SignedGas` amount + /// + /// Ensures the invariant that `Negative` must not be used for zero + pub fn safe_new_negative(amount: BalanceOf) -> 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) -> Self { Self::Positive(weight_fee) @@ -65,8 +76,8 @@ impl SignedGas { 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 SignedGas { 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 SignedGas { 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 SignedGas { // 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)), } } } diff --git a/substrate/frame/revive/src/metering/tests.rs b/substrate/frame/revive/src/metering/tests.rs index 35089c593cc04..23614230b6256 100644 --- a/substrate/frame/revive/src/metering/tests.rs +++ b/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::::from_adjusted_deposit_charge(&deposit1); + assert_eq!(gas_result1, SignedGas::Negative(BalanceOf::::from(5u32))); + + let deposit2 = StorageDeposit::Refund(1); + let gas_result2 = SignedGas::::from_adjusted_deposit_charge(&deposit2); + assert_eq!(gas_result2, SignedGas::Positive(BalanceOf::::from(0u32))); + }); +} + #[test_case(FixtureType::Solc , "DepositPrecompile" ; "solc precompiles")] #[test_case(FixtureType::Resolc , "DepositPrecompile" ; "resolc precompiles")] #[test_case(FixtureType::Solc , "DepositDirect" ; "solc direct")]