From 29abf422257c643a3fcfeffe2b72682e49dc363d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20Stu=CC=88ber?= <15174476+TorstenStueber@users.noreply.github.com> Date: Mon, 8 Dec 2025 06:48:51 -0300 Subject: [PATCH 1/3] Avoid construction of negative zero SignedGas --- substrate/frame/revive/src/metering/gas.rs | 10 ++++++++-- substrate/frame/revive/src/metering/tests.rs | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/substrate/frame/revive/src/metering/gas.rs b/substrate/frame/revive/src/metering/gas.rs index 4e51c82c57ade..0ef35f7a24ac9 100644 --- a/substrate/frame/revive/src/metering/gas.rs +++ b/substrate/frame/revive/src/metering/gas.rs @@ -65,8 +65,14 @@ 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) => { + let scaled_amount = multiplier.saturating_mul_int(*amount); + if scaled_amount == Default::default() { + Positive(scaled_amount) + } else { + Negative(scaled_amount) + } + }, } } 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")] From 10ed27c6bc40470acd585b3ee0e5d682ac7924b1 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 8 Dec 2025 09:55:04 +0000 Subject: [PATCH 2/3] Update from github-actions[bot] running command 'prdoc --audience runtime_dev --bump patch' --- prdoc/pr_10567.prdoc | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 prdoc/pr_10567.prdoc 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 From 923dcc576773f816ad51bbac5bd66f7630889579 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20Stu=CC=88ber?= <15174476+TorstenStueber@users.noreply.github.com> Date: Mon, 8 Dec 2025 09:00:23 -0300 Subject: [PATCH 3/3] Make negative signed gas construction more defensive --- substrate/frame/revive/src/metering/gas.rs | 39 ++++++++++++---------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/substrate/frame/revive/src/metering/gas.rs b/substrate/frame/revive/src/metering/gas.rs index 0ef35f7a24ac9..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,14 +76,8 @@ impl SignedGas { match deposit { StorageDeposit::Charge(amount) => Positive(multiplier.saturating_mul_int(*amount)), - StorageDeposit::Refund(amount) => { - let scaled_amount = multiplier.saturating_mul_int(*amount); - if scaled_amount == Default::default() { - Positive(scaled_amount) - } else { - Negative(scaled_amount) - } - }, + StorageDeposit::Refund(amount) => + Self::safe_new_negative(multiplier.saturating_mul_int(*amount)), } } @@ -114,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)) }, @@ -134,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)) }, @@ -153,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)), } } }