From 9988d89c11da3823ac320073f697a3f0efacce72 Mon Sep 17 00:00:00 2001 From: Raphael Date: Mon, 11 Nov 2024 10:32:24 +0100 Subject: [PATCH 01/10] test: denomination arithmetic --- .../src/tests/curves/arithmetic.rs | 132 ++++++++++++++++++ .../src/tests/curves/mod.rs | 1 + 2 files changed, 133 insertions(+) create mode 100644 pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs diff --git a/pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs b/pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs new file mode 100644 index 0000000000..092bb7f823 --- /dev/null +++ b/pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs @@ -0,0 +1,132 @@ +use crate::{ + curves::convert_to_fixed, + mock::{runtime::Test, Float}, +}; +use frame_support::assert_ok; +use sp_runtime::ArithmeticError; + +#[test] +fn test_convert_to_fixed_basic() { + let x = 1000u128; + let denomination = 2u8; // 10^2 = 100 + + let result = convert_to_fixed::(x, denomination).unwrap(); + // Test runtime uses I75F53 for CurveParameterTypeOf, which is what we'll cover + // in testing. + let expected = Float::from_num(10); // 1000 / 100 = 10 + + assert_eq!(result, expected); +} + +#[test] +fn test_convert_to_fixed_with_remainder() { + let x = 1050u128; + let denomination = 2u8; // 10^2 = 100 + + let result = convert_to_fixed::(x, denomination).unwrap(); + let expected = Float::from_num(10.5); // 1050 / 100 = 10.5 + + assert_eq!(result, expected); +} + +#[test] +fn test_convert_to_fixed_smaller_than_denomination() { + let x = 1050u128; + let denomination = 6u8; // 10^6 = 1000000 + + let result = convert_to_fixed::(x, denomination).unwrap(); + let expected = Float::from_num(0.00105); // 1050 / 1000000 = 0.00105 + + assert_eq!(result, expected); +} + +#[test] +fn test_convert_to_fixed_large_value() { + let x = 1_000_000_000_000_000u128; + let denomination = 12u8; // 10^12 = 1_000_000_000_000 + + let result = convert_to_fixed::(x, denomination).unwrap(); + let expected = Float::from_num(1000); // 1_000_000_000_000_000 / 1_000_000_000_000 = 1000 + + assert_eq!(result, expected); +} + +#[test] +fn test_convert_to_fixed_small_denomination() { + let x = 12345u128; + let denomination = 1u8; // 10^1 = 10 + + let result = convert_to_fixed::(x, denomination).unwrap(); + let expected = Float::from_num(1234.5); // 12345 / 10 = 1234.5 + + assert_eq!(result, expected); +} + +#[test] +fn test_convert_to_fixed_overflow() { + let x = u128::MAX; + let denomination = 0u8; // 10^0 = 1, no scaling + + let result = convert_to_fixed::(x, denomination); + assert!(result.is_err()); + assert_eq!(result.unwrap_err(), ArithmeticError::Overflow); +} + +#[test] +fn test_convert_to_fixed_denomination_overflow() { + let x = 1000u128; + let denomination = 128u8; // 10^128 overflows and results in division by zero + + let result = convert_to_fixed::(x, denomination); + assert!(result.is_err()); + assert_eq!(result.unwrap_err(), ArithmeticError::Overflow); +} + +#[test] +fn test_convert_to_fixed_overflow_avoided() { + let x = u128::MAX; // around 3.4e+38 + let denomination = 17u8; // I75F53 should handle around 1.8e+22, 38 - 23 -> 17 + + let result = convert_to_fixed::(x, denomination); + assert_ok!(result); +} + +#[test] +fn test_convert_to_fixed_handles_large_denomination() { + let x = u128::MAX; // around 3.4e+38 + let denomination = 22u8; // I75F53 should handle around 1.8e+22 + + let result = convert_to_fixed::(x, denomination); + assert_ok!(result); +} + +#[test] +fn test_convert_to_fixed_too_large_denomination() { + let x = u128::MAX; // around 3.4e+38 + let denomination = 30u8; // I75F53 should handle around 1.8e+22, this can't be represented + + let result = convert_to_fixed::(x, denomination); + assert_ok!(result); +} + +#[test] +fn test_convert_to_fixed_zero_denomination() { + let x = 1000u128; + let denomination = 0u8; // 10^0 = 1 + + let result = convert_to_fixed::(x, denomination).unwrap(); + let expected = Float::from_num(1000); // 1000 / 1 = 1000 + + assert_eq!(result, expected); +} + +#[test] +fn test_convert_to_fixed_zero_input() { + let x = 0u128; + let denomination = 10u8; // 10^10 = large divisor + + let result = convert_to_fixed::(x, denomination).unwrap(); + let expected = Float::from_num(0); // 0 / any number = 0 + + assert_eq!(result, expected); +} diff --git a/pallets/pallet-bonded-coins/src/tests/curves/mod.rs b/pallets/pallet-bonded-coins/src/tests/curves/mod.rs index 8c863a1a26..766bee4a60 100644 --- a/pallets/pallet-bonded-coins/src/tests/curves/mod.rs +++ b/pallets/pallet-bonded-coins/src/tests/curves/mod.rs @@ -1,3 +1,4 @@ +mod arithmetic; mod lmsr; mod polynomial; mod square_root; From 36f4d07bcfc74247bf3c27a25710adabbbde873b Mon Sep 17 00:00:00 2001 From: Raphael Date: Mon, 11 Nov 2024 10:39:50 +0100 Subject: [PATCH 02/10] fix: avoid dropping decimals in fixed conversion --- pallets/pallet-bonded-coins/src/curves/mod.rs | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/curves/mod.rs b/pallets/pallet-bonded-coins/src/curves/mod.rs index e0a5fe2e06..e375a14448 100644 --- a/pallets/pallet-bonded-coins/src/curves/mod.rs +++ b/pallets/pallet-bonded-coins/src/curves/mod.rs @@ -111,6 +111,31 @@ pub(crate) fn convert_to_fixed( let decimals = 10u128 .checked_pow(u32::from(denomination)) .ok_or(ArithmeticError::Overflow)?; - let scaled_x = x.checked_div(decimals).ok_or(ArithmeticError::DivisionByZero)?; - scaled_x.checked_to_fixed().ok_or(ArithmeticError::Overflow) + + // Divide `x` to separate the quotient and remainder + let quotient = x.checked_div(decimals).ok_or(ArithmeticError::DivisionByZero)?; + let remainder = x.checked_rem_euclid(decimals).ok_or(ArithmeticError::DivisionByZero)?; + + // Convert quotient to fixed-point + // This can overflow if capacity(u128) / 10^denomination > capacity(fixed) + let fixed_quotient: CurveParameterTypeOf = quotient.checked_to_fixed().ok_or(ArithmeticError::Overflow)?; + + // we can skip handling the remainder if it's 0 + if remainder == 0 { + Ok(fixed_quotient) + } else { + // Convert remainder to fixed-point and scale it down by `decimals` + let fixed_remainder = remainder + // This can overflow if 10^denomination > capacity(float) + .checked_to_fixed::>() + .ok_or(ArithmeticError::Overflow)? + // This WILL overflow if 10^denomination > capacity(float) + .checked_div(decimals.checked_to_fixed().ok_or(ArithmeticError::Overflow)?) + .ok_or(ArithmeticError::DivisionByZero)?; + // Combine both parts + fixed_quotient + // This WILL overflow if x / 10^denomination > capacity(float) + .checked_add(fixed_remainder) + .ok_or(ArithmeticError::Overflow) + } } From 9807ea3d10e3b407626f5726ec15724183956f1b Mon Sep 17 00:00:00 2001 From: Raphael Date: Mon, 11 Nov 2024 11:29:09 +0100 Subject: [PATCH 03/10] fix: adress overflow issues on refund_account --- pallets/pallet-bonded-coins/src/lib.rs | 39 ++++++++++++++++++++------ 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/lib.rs b/pallets/pallet-bonded-coins/src/lib.rs index 1ecf49bc0c..285ee60b0a 100644 --- a/pallets/pallet-bonded-coins/src/lib.rs +++ b/pallets/pallet-bonded-coins/src/lib.rs @@ -35,11 +35,11 @@ pub mod pallet { }; use frame_system::pallet_prelude::*; use parity_scale_codec::FullCodec; - use sp_arithmetic::ArithmeticError; + use sp_arithmetic::{traits::CheckedRem, ArithmeticError}; use sp_runtime::{ traits::{ - Bounded, CheckedDiv, CheckedMul, One, SaturatedConversion, Saturating, StaticLookup, UniqueSaturatedInto, - Zero, + Bounded, CheckedAdd, CheckedDiv, CheckedMul, One, SaturatedConversion, Saturating, StaticLookup, + UniqueSaturatedInto, Zero, }, BoundedVec, }; @@ -681,12 +681,35 @@ pub mod pallet { sum.saturating_add(T::Fungibles::total_issuance(id)) }); - let amount = burnt - .checked_mul(&total_collateral_issuance) - .ok_or(ArithmeticError::Overflow)? // TODO: do we need a fallback if this fails? + defensive_assert!( + sum_of_issuances >= burnt, + "burnt amount exceeds the total supply of all bonded currencies" + ); + + // divide first to avoid overflow, remainder will be handled seperately + let divisible_part = total_collateral_issuance + .checked_div(&sum_of_issuances) + .ok_or(Error::::Internal)? // because sum_of_issuances >= burnt > 0, this is theoretically impossible + .checked_mul(&burnt) + // An overflow is also theoretically impossible, as burnt <= sum_of_issuances + .ok_or(Error::::Internal)?; + + // for the remainder, we multiply first and divide next + let remainder_part = total_collateral_issuance + .checked_rem(&sum_of_issuances) + .ok_or(Error::::Internal)? // Should only fail on sum_of_issuances == 0, same as above + .checked_mul(&burnt) + // This can indeed overflow if (sum_of_issuances - 1) * burnt > capacity(u128) + .ok_or(ArithmeticError::Overflow)? .checked_div(&sum_of_issuances) - .ok_or(Error::::NothingToRefund)?; // should be impossible - how would we be able to burn funds if the sum of total - // supplies is 0? + .ok_or(Error::::Internal)?; // same as above + + // total amount is the sum of divisible part and remainder part + let amount = divisible_part + .checked_add(&remainder_part) + // Also theoretically impossible, as this sum would be smaller than total_collateral_issuance as long as + // burnt <= sum_of_issuances + .ok_or(Error::::Internal)?; if amount.is_zero() || T::CollateralCurrencies::can_deposit( From 4b8d2242b6a961a7ccff718ab5bdea28c87b29fe Mon Sep 17 00:00:00 2001 From: Raphael Date: Mon, 11 Nov 2024 13:34:18 +0100 Subject: [PATCH 04/10] refactor: convert_to_fixed --- pallets/pallet-bonded-coins/src/curves/mod.rs | 42 +++++++++++-------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/curves/mod.rs b/pallets/pallet-bonded-coins/src/curves/mod.rs index e375a14448..194ea6c8e4 100644 --- a/pallets/pallet-bonded-coins/src/curves/mod.rs +++ b/pallets/pallet-bonded-coins/src/curves/mod.rs @@ -112,30 +112,38 @@ pub(crate) fn convert_to_fixed( .checked_pow(u32::from(denomination)) .ok_or(ArithmeticError::Overflow)?; - // Divide `x` to separate the quotient and remainder - let quotient = x.checked_div(decimals).ok_or(ArithmeticError::DivisionByZero)?; - let remainder = x.checked_rem_euclid(decimals).ok_or(ArithmeticError::DivisionByZero)?; + // Scale down x by the denomination using integer division, truncating any + // fractional parts of the result for now. Will overflow if down-scaling is not + // sufficient to bring x to a range representable in the fixed point format. + // This can happen if capacity(u128) / 10^denomination > capacity(fixed). + let mut scaled_x: CurveParameterTypeOf = x + .checked_div(decimals) + .ok_or(ArithmeticError::DivisionByZero)? + .checked_to_fixed() + .ok_or(ArithmeticError::Overflow)?; - // Convert quotient to fixed-point - // This can overflow if capacity(u128) / 10^denomination > capacity(fixed) - let fixed_quotient: CurveParameterTypeOf = quotient.checked_to_fixed().ok_or(ArithmeticError::Overflow)?; + // Next we handle the remainder of the division + let remainder = x.checked_rem_euclid(decimals).ok_or(ArithmeticError::DivisionByZero)?; - // we can skip handling the remainder if it's 0 - if remainder == 0 { - Ok(fixed_quotient) - } else { - // Convert remainder to fixed-point and scale it down by `decimals` - let fixed_remainder = remainder + // If the remainder is not 0, convert to fixed, scale it down and add the + // resulting fractional part to the previous result + if remainder > 0 { + // Convert remainder to fixed-point and scale it down by `decimals`, + let fractional = remainder // This can overflow if 10^denomination > capacity(float) .checked_to_fixed::>() .ok_or(ArithmeticError::Overflow)? // This WILL overflow if 10^denomination > capacity(float) .checked_div(decimals.checked_to_fixed().ok_or(ArithmeticError::Overflow)?) .ok_or(ArithmeticError::DivisionByZero)?; + // Combine both parts - fixed_quotient - // This WILL overflow if x / 10^denomination > capacity(float) - .checked_add(fixed_remainder) - .ok_or(ArithmeticError::Overflow) - } + scaled_x = scaled_x + // Overflow is theoretically impossible as we are adding a number < 1 to a fixed point where the fractional + // part is 0. + .checked_add(fractional) + .ok_or(ArithmeticError::Overflow)?; + }; + + Ok(scaled_x) } From 694334c178b2082d19b0f1e27a703585b9f865ae Mon Sep 17 00:00:00 2001 From: Raphael Date: Tue, 12 Nov 2024 10:51:43 +0100 Subject: [PATCH 05/10] fix: mitigate overflow on denominations exceeding the capacity of fixed --- pallets/pallet-bonded-coins/src/curves/mod.rs | 17 ++++++++++------- pallets/pallet-bonded-coins/src/lib.rs | 4 ++-- .../src/tests/curves/arithmetic.rs | 8 ++++---- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/curves/mod.rs b/pallets/pallet-bonded-coins/src/curves/mod.rs index 194ea6c8e4..cf39726cf0 100644 --- a/pallets/pallet-bonded-coins/src/curves/mod.rs +++ b/pallets/pallet-bonded-coins/src/curves/mod.rs @@ -5,6 +5,7 @@ pub(crate) mod square_root; use parity_scale_codec::{Decode, Encode, MaxEncodedLen}; use scale_info::TypeInfo; use sp_arithmetic::ArithmeticError; +use sp_runtime::traits::CheckedConversion; use sp_std::ops::{AddAssign, BitOrAssign, ShlAssign}; use substrate_fixed::traits::{Fixed, FixedSigned, ToFixed}; @@ -104,10 +105,10 @@ fn calculate_accumulated_passive_issuance(passive_issuance: &[Ba .fold(Balance::from_num(0), |sum, x| sum.saturating_add(*x)) } -pub(crate) fn convert_to_fixed( - x: u128, - denomination: u8, -) -> Result, ArithmeticError> { +pub(crate) fn convert_to_fixed(x: u128, denomination: u8) -> Result, ArithmeticError> +where + as Fixed>::Bits: TryFrom, +{ let decimals = 10u128 .checked_pow(u32::from(denomination)) .ok_or(ArithmeticError::Overflow)?; @@ -130,11 +131,13 @@ pub(crate) fn convert_to_fixed( if remainder > 0 { // Convert remainder to fixed-point and scale it down by `decimals`, let fractional = remainder - // This can overflow if 10^denomination > capacity(float) + // This can overflow if 10^denomination > capacity(fixed) .checked_to_fixed::>() .ok_or(ArithmeticError::Overflow)? - // This WILL overflow if 10^denomination > capacity(float) - .checked_div(decimals.checked_to_fixed().ok_or(ArithmeticError::Overflow)?) + // This would overflow if 10^denomination exceeds the capacity of the _underlying integer_ of the fixed + // (e.g., i128 for an I75F53). However, there is no denomination that is representable in a u128 but not in + // an i128. + .checked_div_int(decimals.checked_into().ok_or(ArithmeticError::Overflow)?) .ok_or(ArithmeticError::DivisionByZero)?; // Combine both parts diff --git a/pallets/pallet-bonded-coins/src/lib.rs b/pallets/pallet-bonded-coins/src/lib.rs index 285ee60b0a..07f0c56cb2 100644 --- a/pallets/pallet-bonded-coins/src/lib.rs +++ b/pallets/pallet-bonded-coins/src/lib.rs @@ -256,7 +256,7 @@ pub mod pallet { #[pallet::call] impl Pallet where - as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign, + as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign + TryFrom, { #[pallet::call_index(0)] #[pallet::weight(Weight::from_parts(10_000, 0) + T::DbWeight::get().writes(1))] @@ -817,7 +817,7 @@ pub mod pallet { impl Pallet where - as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign, + as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign + TryFrom, { fn calculate_collateral( low: CurveParameterTypeOf, diff --git a/pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs b/pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs index 092bb7f823..dba5be8de2 100644 --- a/pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs +++ b/pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs @@ -94,16 +94,16 @@ fn test_convert_to_fixed_overflow_avoided() { #[test] fn test_convert_to_fixed_handles_large_denomination() { let x = u128::MAX; // around 3.4e+38 - let denomination = 22u8; // I75F53 should handle around 1.8e+22 + let denomination = 22u8; // I75F53 should handle around 1.8e+22; this is the maximum safe denomination let result = convert_to_fixed::(x, denomination); assert_ok!(result); } #[test] -fn test_convert_to_fixed_too_large_denomination() { - let x = u128::MAX; // around 3.4e+38 - let denomination = 30u8; // I75F53 should handle around 1.8e+22, this can't be represented +fn test_convert_to_fixed_very_large_denomination() { + let x = 10u128.pow(31); // multiple of denomination should not result in overflow of remainder + let denomination = 30u8; // I75F53 should handle around 1.8e+22, this can lead to overflow let result = convert_to_fixed::(x, denomination); assert_ok!(result); From e94104e29ba16db3f7626488de705a92c00068b2 Mon Sep 17 00:00:00 2001 From: Raphael Date: Tue, 12 Nov 2024 15:00:03 +0100 Subject: [PATCH 06/10] feat: constant to limit maximum allowed denomination --- pallets/pallet-bonded-coins/src/lib.rs | 12 ++++++++++-- pallets/pallet-bonded-coins/src/mock.rs | 6 ++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/lib.rs b/pallets/pallet-bonded-coins/src/lib.rs index 07f0c56cb2..3270b2385d 100644 --- a/pallets/pallet-bonded-coins/src/lib.rs +++ b/pallets/pallet-bonded-coins/src/lib.rs @@ -129,11 +129,16 @@ pub mod pallet { /// The maximum number of currencies allowed for a single pool. #[pallet::constant] type MaxCurrencies: Get; - /// The deposit required for each bonded currency. #[pallet::constant] type MaxStringLength: Get; + /// The maximum denomination that bonded currencies can use. To ensure + /// safe operation, this should be set so that 10^MaxDenomination < + /// 2^CurveParameterType::int_nbits(). + #[pallet::constant] + type MaxDenomination: Get; + /// The deposit required for each bonded currency. #[pallet::constant] type DepositPerCurrency: Get>; @@ -270,10 +275,12 @@ pub mod pallet { ) -> DispatchResult { let who = T::PoolCreateOrigin::ensure_origin(origin)?; - let currency_length = currencies.len(); + ensure!(denomination <= T::MaxDenomination::get(), Error::::InvalidInput); let checked_curve = curve.try_into().map_err(|_| Error::::InvalidInput)?; + let currency_length = currencies.len(); + let current_asset_id = NextAssetId::::get(); let (currency_ids, next_asset_id) = Self::generate_sequential_asset_ids(current_asset_id, currency_length)?; @@ -842,6 +849,7 @@ pub mod pallet { Ok(real_costs) } + fn calculate_normalized_passive_issuance( bonded_currencies: &[FungiblesAssetIdOf], denomination: u8, diff --git a/pallets/pallet-bonded-coins/src/mock.rs b/pallets/pallet-bonded-coins/src/mock.rs index 7bd25ab5c6..2f030c0c61 100644 --- a/pallets/pallet-bonded-coins/src/mock.rs +++ b/pallets/pallet-bonded-coins/src/mock.rs @@ -213,6 +213,7 @@ pub mod runtime { pub const CurrencyDeposit: Balance = 500; pub const MaxCurrencies: u32 = 50; pub const CollateralAssetId: u32 = u32::MAX; + pub const MaxDenomination: u8 = 22; } impl pallet_bonded_coins::Config for Test { @@ -232,6 +233,7 @@ pub mod runtime { type PoolId = AccountId; type RuntimeEvent = RuntimeEvent; type RuntimeHoldReason = RuntimeHoldReason; + type MaxDenomination = MaxDenomination; } #[derive(Clone, Default)] @@ -289,8 +291,8 @@ pub mod runtime { .chain(collateral_assets) .collect(); - // NextAssetId is set to the maximum value of all collateral/bonded currency ids, plus one. - // If no currencies are created, it's set to 0. + // NextAssetId is set to the maximum value of all collateral/bonded currency + // ids, plus one. If no currencies are created, it's set to 0. let next_asset_id = all_assets.iter().map(|(id, ..)| id).max().map_or(0, |id| id + 1); pallet_assets::GenesisConfig:: { From 08b71c4c8966febb0c17df070b7b3e941f77115b Mon Sep 17 00:00:00 2001 From: Raphael Flechtner <39338561+rflechtner@users.noreply.github.com> Date: Thu, 14 Nov 2024 13:13:46 +0100 Subject: [PATCH 07/10] refactor: use U256 for infallible scaling (#796) Co-authored-by: Adel Golghalyani <48685760+Ad96el@users.noreply.github.com> --- pallets/pallet-bonded-coins/src/curves/mod.rs | 63 +++++++------------ pallets/pallet-bonded-coins/src/lib.rs | 56 +++++++---------- .../src/tests/curves/arithmetic.rs | 11 ++-- 3 files changed, 54 insertions(+), 76 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/curves/mod.rs b/pallets/pallet-bonded-coins/src/curves/mod.rs index cf39726cf0..f957f615e2 100644 --- a/pallets/pallet-bonded-coins/src/curves/mod.rs +++ b/pallets/pallet-bonded-coins/src/curves/mod.rs @@ -5,6 +5,7 @@ pub(crate) mod square_root; use parity_scale_codec::{Decode, Encode, MaxEncodedLen}; use scale_info::TypeInfo; use sp_arithmetic::ArithmeticError; +use sp_core::U256; use sp_runtime::traits::CheckedConversion; use sp_std::ops::{AddAssign, BitOrAssign, ShlAssign}; use substrate_fixed::traits::{Fixed, FixedSigned, ToFixed}; @@ -107,46 +108,28 @@ fn calculate_accumulated_passive_issuance(passive_issuance: &[Ba pub(crate) fn convert_to_fixed(x: u128, denomination: u8) -> Result, ArithmeticError> where - as Fixed>::Bits: TryFrom, + as Fixed>::Bits: TryFrom, // TODO: make large integer type configurable in runtime { - let decimals = 10u128 - .checked_pow(u32::from(denomination)) + let decimals = U256::from(10) + .checked_pow(denomination.into()) .ok_or(ArithmeticError::Overflow)?; - - // Scale down x by the denomination using integer division, truncating any - // fractional parts of the result for now. Will overflow if down-scaling is not - // sufficient to bring x to a range representable in the fixed point format. - // This can happen if capacity(u128) / 10^denomination > capacity(fixed). - let mut scaled_x: CurveParameterTypeOf = x - .checked_div(decimals) - .ok_or(ArithmeticError::DivisionByZero)? - .checked_to_fixed() - .ok_or(ArithmeticError::Overflow)?; - - // Next we handle the remainder of the division - let remainder = x.checked_rem_euclid(decimals).ok_or(ArithmeticError::DivisionByZero)?; - - // If the remainder is not 0, convert to fixed, scale it down and add the - // resulting fractional part to the previous result - if remainder > 0 { - // Convert remainder to fixed-point and scale it down by `decimals`, - let fractional = remainder - // This can overflow if 10^denomination > capacity(fixed) - .checked_to_fixed::>() - .ok_or(ArithmeticError::Overflow)? - // This would overflow if 10^denomination exceeds the capacity of the _underlying integer_ of the fixed - // (e.g., i128 for an I75F53). However, there is no denomination that is representable in a u128 but not in - // an i128. - .checked_div_int(decimals.checked_into().ok_or(ArithmeticError::Overflow)?) - .ok_or(ArithmeticError::DivisionByZero)?; - - // Combine both parts - scaled_x = scaled_x - // Overflow is theoretically impossible as we are adding a number < 1 to a fixed point where the fractional - // part is 0. - .checked_add(fractional) - .ok_or(ArithmeticError::Overflow)?; - }; - - Ok(scaled_x) + // Convert to U256 so we have enough bits to perform lossless scaling. + let mut x_u256 = U256::from(x); + // Shift left to produce the representation that our fixed type would have (but + // with extra integer bits that would potentially not fit in the fixed type). + // This function can panic in theory, but only if frac_nbits() would be larger + // than 256 - and no Fixed of that size exists. + x_u256.shl_assign(CurveParameterTypeOf::::frac_nbits()); + // Perform division. Due to the shift the precision/truncation is identical to + // division on the fixed type. + x_u256 = x_u256.checked_div(decimals).ok_or(ArithmeticError::DivisionByZero)?; + // Try conversion to integer type underlying the fixed type (e.g., i128 for a + // I75F53). If this overflows, there is nothing we can do; even the scaled value + // does not fit in the fixed type. + let truncated = x_u256.checked_into().ok_or(ArithmeticError::Overflow)?; + // Cast the integer as a fixed. We can do this because we've already applied the + // correct bit shift above. + let fixed = as Fixed>::from_bits(truncated); + // Return the result of scaled conversion to fixed. + Ok(fixed) } diff --git a/pallets/pallet-bonded-coins/src/lib.rs b/pallets/pallet-bonded-coins/src/lib.rs index 3270b2385d..9f7c5ce432 100644 --- a/pallets/pallet-bonded-coins/src/lib.rs +++ b/pallets/pallet-bonded-coins/src/lib.rs @@ -35,11 +35,11 @@ pub mod pallet { }; use frame_system::pallet_prelude::*; use parity_scale_codec::FullCodec; - use sp_arithmetic::{traits::CheckedRem, ArithmeticError}; + use sp_arithmetic::{traits::CheckedAdd, ArithmeticError}; + use sp_core::U256; use sp_runtime::{ traits::{ - Bounded, CheckedAdd, CheckedDiv, CheckedMul, One, SaturatedConversion, Saturating, StaticLookup, - UniqueSaturatedInto, Zero, + Bounded, CheckedConversion, One, SaturatedConversion, Saturating, StaticLookup, UniqueSaturatedInto, Zero, }, BoundedVec, }; @@ -261,7 +261,8 @@ pub mod pallet { #[pallet::call] impl Pallet where - as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign + TryFrom, + as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign + TryFrom, + CollateralCurrenciesBalanceOf: Into + TryFrom, // TODO: make large integer type configurable { #[pallet::call_index(0)] #[pallet::weight(Weight::from_parts(10_000, 0) + T::DbWeight::get().writes(1))] @@ -668,13 +669,14 @@ pub mod pallet { // With amount = max_value(), this trait implementation burns the reducible // balance on the account and returns the actual amount burnt - let burnt = T::Fungibles::burn_from( + let burnt: U256 = T::Fungibles::burn_from( asset_id.clone(), &who, Bounded::max_value(), WithdrawalPrecision::BestEffort, Fortitude::Force, - )?; + )? + .into(); if burnt.is_zero() { // no funds available to be burnt on account; nothing to do here @@ -684,38 +686,28 @@ pub mod pallet { let sum_of_issuances = pool_details .bonded_currencies .into_iter() - .fold(FungiblesBalanceOf::::zero(), |sum, id| { - sum.saturating_add(T::Fungibles::total_issuance(id)) - }); + .fold(U256::from(0), |sum, id| { + sum.saturating_add(T::Fungibles::total_issuance(id).into()) + }) + // Add the burnt amount back to the sum of total supplies + .checked_add(burnt) + .ok_or(ArithmeticError::Overflow)?; defensive_assert!( sum_of_issuances >= burnt, "burnt amount exceeds the total supply of all bonded currencies" ); - // divide first to avoid overflow, remainder will be handled seperately - let divisible_part = total_collateral_issuance - .checked_div(&sum_of_issuances) - .ok_or(Error::::Internal)? // because sum_of_issuances >= burnt > 0, this is theoretically impossible - .checked_mul(&burnt) - // An overflow is also theoretically impossible, as burnt <= sum_of_issuances - .ok_or(Error::::Internal)?; - - // for the remainder, we multiply first and divide next - let remainder_part = total_collateral_issuance - .checked_rem(&sum_of_issuances) - .ok_or(Error::::Internal)? // Should only fail on sum_of_issuances == 0, same as above - .checked_mul(&burnt) - // This can indeed overflow if (sum_of_issuances - 1) * burnt > capacity(u128) + let amount: CollateralCurrenciesBalanceOf = burnt + .checked_mul(total_collateral_issuance.into()) + // As long as the balance type is half the size of a U256, this won't overflow. .ok_or(ArithmeticError::Overflow)? - .checked_div(&sum_of_issuances) - .ok_or(Error::::Internal)?; // same as above - - // total amount is the sum of divisible part and remainder part - let amount = divisible_part - .checked_add(&remainder_part) - // Also theoretically impossible, as this sum would be smaller than total_collateral_issuance as long as - // burnt <= sum_of_issuances + .checked_div(sum_of_issuances) + // Because sum_of_issuances >= burnt > 0, this is theoretically impossible + .ok_or(Error::::Internal)? + .checked_into() + // Also theoretically impossible, as the result must be <= total_collateral_issuance + // if burnt <= sum_of_issuances, which should always hold true .ok_or(Error::::Internal)?; if amount.is_zero() @@ -824,7 +816,7 @@ pub mod pallet { impl Pallet where - as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign + TryFrom, + as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign + TryFrom, { fn calculate_collateral( low: CurveParameterTypeOf, diff --git a/pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs b/pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs index dba5be8de2..fdb44a5c54 100644 --- a/pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs +++ b/pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs @@ -75,7 +75,7 @@ fn test_convert_to_fixed_overflow() { #[test] fn test_convert_to_fixed_denomination_overflow() { let x = 1000u128; - let denomination = 128u8; // 10^128 overflows and results in division by zero + let denomination = 128u8; // 10^128 overflows let result = convert_to_fixed::(x, denomination); assert!(result.is_err()); @@ -102,11 +102,14 @@ fn test_convert_to_fixed_handles_large_denomination() { #[test] fn test_convert_to_fixed_very_large_denomination() { - let x = 10u128.pow(31); // multiple of denomination should not result in overflow of remainder let denomination = 30u8; // I75F53 should handle around 1.8e+22, this can lead to overflow - let result = convert_to_fixed::(x, denomination); - assert_ok!(result); + // multiple of denomination would not result in remainder = 0 + assert_ok!(convert_to_fixed::(10u128.pow(31), denomination)); + + // non-multiples of denomination could lead to overflow of remainder + assert_ok!(convert_to_fixed::(11u128.pow(31), denomination)); + assert_ok!(convert_to_fixed::(10u128.pow(29), denomination)); } #[test] From bf42e603b513aa745ad9f408982c2756724a5171 Mon Sep 17 00:00:00 2001 From: Raphael Date: Thu, 14 Nov 2024 14:16:00 +0100 Subject: [PATCH 08/10] chore: document safe denomination limit --- pallets/pallet-bonded-coins/src/lib.rs | 9 +++++---- pallets/pallet-bonded-coins/src/mock.rs | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/lib.rs b/pallets/pallet-bonded-coins/src/lib.rs index 9f7c5ce432..4dd02fd672 100644 --- a/pallets/pallet-bonded-coins/src/lib.rs +++ b/pallets/pallet-bonded-coins/src/lib.rs @@ -35,7 +35,7 @@ pub mod pallet { }; use frame_system::pallet_prelude::*; use parity_scale_codec::FullCodec; - use sp_arithmetic::{traits::CheckedAdd, ArithmeticError}; + use sp_arithmetic::ArithmeticError; use sp_core::U256; use sp_runtime::{ traits::{ @@ -133,9 +133,10 @@ pub mod pallet { #[pallet::constant] type MaxStringLength: Get; - /// The maximum denomination that bonded currencies can use. To ensure - /// safe operation, this should be set so that 10^MaxDenomination < - /// 2^CurveParameterType::int_nbits(). + /// The maximum denomination that bonded currencies can use. This should + /// be configured so that + /// 10^MaxDenomination < 2^CurveParameterType::frac_nbits() + /// as larger denominations could result in truncation. #[pallet::constant] type MaxDenomination: Get; diff --git a/pallets/pallet-bonded-coins/src/mock.rs b/pallets/pallet-bonded-coins/src/mock.rs index 2f030c0c61..b78aced149 100644 --- a/pallets/pallet-bonded-coins/src/mock.rs +++ b/pallets/pallet-bonded-coins/src/mock.rs @@ -213,7 +213,7 @@ pub mod runtime { pub const CurrencyDeposit: Balance = 500; pub const MaxCurrencies: u32 = 50; pub const CollateralAssetId: u32 = u32::MAX; - pub const MaxDenomination: u8 = 22; + pub const MaxDenomination: u8 = 15; } impl pallet_bonded_coins::Config for Test { From 3a2306b615b0bab8fff0a07d5542e5e501c52287 Mon Sep 17 00:00:00 2001 From: Raphael Date: Thu, 14 Nov 2024 14:16:24 +0100 Subject: [PATCH 09/10] feat: add integrity test hook --- pallets/pallet-bonded-coins/src/lib.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pallets/pallet-bonded-coins/src/lib.rs b/pallets/pallet-bonded-coins/src/lib.rs index 4dd02fd672..1859130d9e 100644 --- a/pallets/pallet-bonded-coins/src/lib.rs +++ b/pallets/pallet-bonded-coins/src/lib.rs @@ -179,6 +179,18 @@ pub mod pallet { #[pallet::pallet] pub struct Pallet(_); + #[pallet::hooks] + impl Hooks> for Pallet { + fn integrity_test() { + assert!( + 2u128.pow(T::CurveParameterType::frac_nbits()) > 10u128.pow(T::MaxDenomination::get().into()), + "In order to prevent truncation of balances, `MaxDenomination` should be configured such \ + that the maximum scaling factor `10^MaxDenomination` is smaller than the fractional \ + capacity `2^frac_nbits` of `CurveParameterType`", + ); + } + } + #[pallet::storage] #[pallet::getter(fn pools)] pub(crate) type Pools = StorageMap<_, Twox64Concat, T::PoolId, PoolDetailsOf, OptionQuery>; From a86d1c0a0ae2e67ec26c3eaedf4df2f88d6b0a8b Mon Sep 17 00:00:00 2001 From: Raphael Date: Thu, 14 Nov 2024 15:09:26 +0100 Subject: [PATCH 10/10] test: additional integrity test to check for denomination that overflows U256 --- pallets/pallet-bonded-coins/src/lib.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pallets/pallet-bonded-coins/src/lib.rs b/pallets/pallet-bonded-coins/src/lib.rs index 1859130d9e..ea89fbc152 100644 --- a/pallets/pallet-bonded-coins/src/lib.rs +++ b/pallets/pallet-bonded-coins/src/lib.rs @@ -182,8 +182,13 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { fn integrity_test() { + let scaling_factor = U256::from(10).checked_pow(T::MaxDenomination::get().into()).expect( + "`MaxDenomination` is set so high that the resulting scaling factor cannot be represented. / + Any attempt to mint or burn on a pool where `10^denomination > 2^256` _WILL_ fail.", + ); + assert!( - 2u128.pow(T::CurveParameterType::frac_nbits()) > 10u128.pow(T::MaxDenomination::get().into()), + U256::from(2).pow(T::CurveParameterType::frac_nbits().into()) > scaling_factor, "In order to prevent truncation of balances, `MaxDenomination` should be configured such \ that the maximum scaling factor `10^MaxDenomination` is smaller than the fractional \ capacity `2^frac_nbits` of `CurveParameterType`",