refactor: bonded coins audit fixes (#873)

Changes to pallet-bonded-coins to address findings in the audit.
---
Co-authored-by: Adel Golghalyani <48685760+Ad96el@users.noreply.github.com>

Author: rflechtner

pallets/pallet-bonded-coins/README.md

@@ -17,6 +17,7 @@
 // If you feel like getting in touch with us, you can do so at <hello@kilt.io>
 use frame_benchmarking::v2::*;
 use frame_support::traits::fungibles::roles::Inspect as InspectRoles;
+use scale_info::prelude::format;
 use sp_core::U256;
 use sp_std::{
 	ops::{AddAssign, BitOrAssign, ShlAssign},
@@ -27,9 +28,11 @@ use substrate_fixed::traits::{Fixed, FixedSigned, FixedUnsigned, ToFixed};
 use crate::{
 	curves::{
 		lmsr::{LMSRParameters, LMSRParametersInput},
+		polynomial::PolynomialParameters,
 		square_root::{SquareRootParameters, SquareRootParametersInput},
 		Curve, CurveInput,
 	},
+	types::BondedCurrenciesSettings,
 	Call, CollateralAssetIdOf, CollateralBalanceOf, Config, CurveParameterTypeOf, FungiblesAssetIdOf,
 	FungiblesBalanceOf, Pallet,
 };
@@ -63,6 +66,13 @@ where
 	fn set_native_balance(_account: &<T>::AccountId, _amount: u128) {}
 }
 
+fn get_2nd_order_polynomial_curve<Float: FixedSigned>() -> Curve<Float> {
+	let m = Float::from_num(0.01);
+	let n = Float::from_num(2);
+	let o = Float::from_num(3);
+	Curve::Polynomial(PolynomialParameters { m, n, o })
+}
+
 fn get_square_root_curve<Float: FixedSigned>() -> Curve<Float> {
 	let m = Float::from_num(3);
 	let n = Float::from_num(2);
@@ -220,10 +230,13 @@ mod benchmarks {
 			owner,
 			state,
 			collateral,
-			denomination,
 			bonded_currencies: BoundedVec::truncate_from(bonded_coin_ids.clone()),
-			transferable: true,
-			min_operation_balance: 1u128.saturated_into(),
+			currencies_settings: BondedCurrenciesSettings {
+				denomination,
+				transferable: true,
+				allow_reset_team: true,
+				min_operation_balance: 1u128.saturated_into(),
+			},
 			deposit: Pallet::<T>::calculate_pool_deposit(bonded_coin_ids.len()),
 		};
 		Pools::<T>::insert(&pool_id, pool_details);
@@ -233,11 +246,11 @@ mod benchmarks {
 
 	fn generate_token_metadata<T: Config>(c: u32) -> BoundedVec<TokenMetaOf<T>, T::MaxCurrenciesPerPool> {
 		let mut token_meta = Vec::new();
-		for _ in 1..=c {
+		for i in 1..=c {
 			token_meta.push(TokenMetaOf::<T> {
 				min_balance: 1u128.saturated_into(),
-				name: BoundedVec::try_from(b"BTC".to_vec()).expect("Failed to create BoundedVec"),
-				symbol: BoundedVec::try_from(b"BTC".to_vec()).expect("Failed to create BoundedVec"),
+				name: BoundedVec::try_from(format!("Coin_{}", &i).into_bytes()).expect("Failed to create BoundedVec"),
+				symbol: BoundedVec::try_from(format!("BTC_{}", &i).into_bytes()).expect("Failed to create BoundedVec"),
 			})
 		}
 		BoundedVec::try_from(token_meta).expect("creating bounded Vec should not fail")
@@ -263,9 +276,12 @@ mod benchmarks {
 			curve,
 			collateral_id,
 			currencies,
-			10,
-			true,
-			1,
+			BondedCurrenciesSettings {
+				denomination: 10,
+				allow_reset_team: true,
+				transferable: true,
+				min_operation_balance: 1u128.saturated_into(),
+			},
 		);
 
 		// Verify
@@ -301,9 +317,12 @@ mod benchmarks {
 			curve,
 			collateral_id,
 			currencies,
-			10,
-			true,
-			1,
+			BondedCurrenciesSettings {
+				denomination: 10,
+				allow_reset_team: true,
+				transferable: true,
+				min_operation_balance: 1u128.saturated_into(),
+			},
 		);
 
 		// Verify
@@ -313,7 +332,7 @@ mod benchmarks {
 			Curve::SquareRoot(_) => {
 				assert_eq!(id, expected_pool_id);
 			}
-			_ => panic!("pool.curve is not a Polynomial function"),
+			_ => panic!("pool.curve is not a SquareRoot function"),
 		}
 	}
 
@@ -338,9 +357,12 @@ mod benchmarks {
 			curve,
 			collateral_id,
 			currencies,
-			10,
-			true,
-			1,
+			BondedCurrenciesSettings {
+				denomination: 10,
+				allow_reset_team: true,
+				transferable: true,
+				min_operation_balance: 1u128.saturated_into(),
+			},
 		);
 
 		// Verify
@@ -350,30 +372,27 @@ mod benchmarks {
 			Curve::Lmsr(_) => {
 				assert_eq!(id, expected_pool_id);
 			}
-			_ => panic!("pool.curve is not a Polynomial function"),
+			_ => panic!("pool.curve is not a LSMR curve!"),
 		}
 	}
 
 	#[benchmark]
-	fn reset_team() {
+	fn reset_team(c: Linear<1, { T::MaxCurrenciesPerPool::get() }>) {
 		let origin = T::DefaultOrigin::try_successful_origin().expect("creating origin should not fail");
 		let account_origin = origin
 			.clone()
 			.into_signer()
 			.expect("generating account_id from origin should not fail");
 		make_free_for_deposit::<T>(&account_origin);
 
-		let bonded_coin_id = T::BenchmarkHelper::calculate_bonded_asset_id(0);
-		create_bonded_asset::<T>(bonded_coin_id.clone());
+		let bonded_currencies = create_bonded_currencies_in_range::<T>(c, false);
 
 		let curve = get_linear_bonding_curve::<CurveParameterTypeOf<T>>();
-		let pool_id = create_pool::<T>(
-			curve,
-			[bonded_coin_id.clone()].to_vec(),
-			Some(account_origin),
-			None,
-			None,
-		);
+		let pool_id = create_pool::<T>(curve, bonded_currencies.clone(), Some(account_origin), None, None);
+
+		// Although these would rarely happen in practice, for benchmarking we assume
+		// the worst case where the owner must be changed as well
+		assert!(T::Fungibles::owner(bonded_currencies[0].clone()) != Some(pool_id.clone().into()));
 
 		let admin: AccountIdOf<T> = account("admin", 0, 0);
 		let freezer: AccountIdOf<T> = account("freezer", 0, 0);
@@ -382,12 +401,24 @@ mod benchmarks {
 			freezer: freezer.clone(),
 		};
 
+		let pool_id_for_call = pool_id.clone();
+
+		let max_currencies = T::MaxCurrenciesPerPool::get();
 		#[extrinsic_call]
-		_(origin as T::RuntimeOrigin, pool_id, fungibles_team, 0);
+		_(
+			origin as T::RuntimeOrigin,
+			pool_id_for_call,
+			fungibles_team,
+			max_currencies,
+		);
 
 		// Verify
-		assert_eq!(T::Fungibles::admin(bonded_coin_id.clone()), Some(admin));
-		assert_eq!(T::Fungibles::freezer(bonded_coin_id), Some(freezer));
+		bonded_currencies.iter().for_each(|asset_id| {
+			assert_eq!(T::Fungibles::admin(asset_id.clone()), Some(admin.clone()));
+			assert_eq!(T::Fungibles::freezer(asset_id.clone()), Some(freezer.clone()));
+			assert_eq!(T::Fungibles::owner(asset_id.clone()), Some(pool_id.clone().into()));
+			assert_eq!(T::Fungibles::issuer(asset_id.clone()), Some(pool_id.clone().into()));
+		});
 	}
 
 	#[benchmark]
@@ -475,7 +506,7 @@ mod benchmarks {
 		make_free_for_deposit::<T>(&account_origin);
 		set_collateral_balance::<T>(collateral_id.clone(), &account_origin, 10000u128);
 
-		let curve = get_linear_bonding_curve::<CurveParameterTypeOf<T>>();
+		let curve = get_2nd_order_polynomial_curve::<CurveParameterTypeOf<T>>();
 		let bonded_currencies = create_bonded_currencies_in_range::<T>(c, false);
 
 		let pool_id = create_pool::<T>(curve, bonded_currencies.clone(), None, None, Some(0));
@@ -605,7 +636,7 @@ mod benchmarks {
 		let start_balance = 100u128;
 		set_fungible_balance::<T>(target_asset_id.clone(), &account_origin, start_balance);
 
-		let curve = get_linear_bonding_curve::<CurveParameterTypeOf<T>>();
+		let curve = get_2nd_order_polynomial_curve::<CurveParameterTypeOf<T>>();
 
 		let pool_id = create_pool::<T>(curve, bonded_currencies, None, None, Some(0));
 		let pool_account = pool_id.clone().into();

pallets/pallet-bonded-coins/src/benchmarking.rs

@@ -166,7 +166,7 @@ pub fn balance_to_fixed<Balance, FixedType: Fixed>(
 	round_kind: Round,
 ) -> Result<FixedType, ArithmeticError>
 where
-	FixedType::Bits: TryFrom<U256>, // TODO: make large integer type configurable in runtime
+	FixedType::Bits: TryFrom<U256>,
 	Balance: TryInto<U256>,
 {
 	let decimals = U256::from(10u8)

pallets/pallet-bonded-coins/src/curves/mod.rs

@@ -34,13 +34,19 @@
 /// ### Antiderivative
 /// The indefinite integral of the cost function is:
 /// ```text
-/// C(s) = (m / 3) * s^3 + (n / 2) * s^2 + o * s
+/// C(s) = (m / 3) * s^3 + (n / 2) * s^2 + o * s = M * s^3 + N * s^2 + O * s
 /// ```
 /// Where:
 /// - `m` is the coefficient for the quadratic term,
 /// - `n` is the coefficient for the linear term,
-/// - `o` is the constant term.
+/// - `o` is the constant term,
+/// - `M` is the coefficient of the first (cubic) term in the antiderivative,
+/// - `N` is the coefficient of the second (quadratic) term in the
+///   antiderivative,
+/// - `O=o` is the coefficient of the third (linear) term in the antiderivative.
 ///
+/// Coefficients of the antiderivative `N`, `M` & `O` are used to parametrize
+/// functions in this module.
 ///
 /// `C(s)` represents the accumulated cost of purchasing or selling assets up to
 /// the current supply `s`. The integral between two supply points, `s*`
@@ -81,9 +87,11 @@ use crate::PassiveSupply;
 ///
 /// For a polynomial cost function `c(s) = 3 * s^2 + 2 * s + 2`
 ///
-/// which is resulting into the antiderivative
-/// `C(s) = (3 / 3) * s^3 + (2 / 2) * s^2 + 2 * s`
-/// the input parameters would be:
+/// which results in the antiderivative
+/// `C(s) = (3 / 3) * s^3 + (2 / 2) * s^2 + 2 * s = 1 * s^3 + 1 * s^2 + 2 * s`
+///
+/// the input parameters `M`, `N` & `O` (coefficients of the antiderivative)
+/// would be:
 /// ```rust, ignore
 /// PolynomialParametersInput {
 ///    m: 1,
@@ -152,27 +160,33 @@ where
 			.checked_add(accumulated_passive_issuance)
 			.ok_or(ArithmeticError::Overflow)?;
 
-		// Calculate high - low
-		let delta_x = high.checked_sub(low).ok_or(ArithmeticError::Underflow)?;
-
-		let high_low_mul = high.checked_mul(low).ok_or(ArithmeticError::Overflow)?;
-		let high_square = square(high)?;
-		let low_square = square(low)?;
-
-		// Factorized cubic term:  (high^2 + high * low + low^2)
-		let cubic_term = high_square
-			.checked_add(high_low_mul)
-			.ok_or(ArithmeticError::Overflow)?
-			.checked_add(low_square)
-			.ok_or(ArithmeticError::Overflow)?;
-
 		// Calculate m * (high^2 + high * low + low^2)
-		let term1 = self.m.checked_mul(cubic_term).ok_or(ArithmeticError::Overflow)?;
-
-		let high_plus_low = high.checked_add(low).ok_or(ArithmeticError::Overflow)?;
+		let term1 = if self.m == Coefficient::from_num(0u8) {
+			// if m is 0 the product is 0
+			Ok(self.m)
+		} else {
+			let high_low_mul = high.checked_mul(low).ok_or(ArithmeticError::Overflow)?;
+			let high_square = square(high)?;
+			let low_square = square(low)?;
+
+			// Factorized cubic term:  (high^2 + high * low + low^2)
+			let cubic_term = high_square
+				.checked_add(high_low_mul)
+				.ok_or(ArithmeticError::Overflow)?
+				.checked_add(low_square)
+				.ok_or(ArithmeticError::Overflow)?;
+
+			self.m.checked_mul(cubic_term).ok_or(ArithmeticError::Overflow)
+		}?;
 
 		// Calculate n * (high + low)
-		let term2 = self.n.checked_mul(high_plus_low).ok_or(ArithmeticError::Overflow)?;
+		let term2 = if self.n == Coefficient::from_num(0u8) {
+			// if n is 0 the product is 0
+			Ok(self.n)
+		} else {
+			let high_plus_low = high.checked_add(low).ok_or(ArithmeticError::Overflow)?;
+			self.n.checked_mul(high_plus_low).ok_or(ArithmeticError::Overflow)
+		}?;
 
 		// Final calculation with factored (high - low)
 		let result = term1
@@ -181,6 +195,9 @@ where
 			.checked_add(self.o)
 			.ok_or(ArithmeticError::Overflow)?;
 
+		// Calculate high - low
+		let delta_x = high.checked_sub(low).ok_or(ArithmeticError::Underflow)?;
+
 		result.checked_mul(delta_x).ok_or(ArithmeticError::Overflow)
 	}
 }

pallets/pallet-bonded-coins/src/curves/polynomial.rs

@@ -33,12 +33,19 @@
 /// ### Antiderivative
 /// The indefinite integral of the cost function is:
 /// ```text
-/// C(s) = (2/3) * m * s^(3/2) + n * s
+/// C(s) = (2/3) * m * s^(3/2) + n * s = M * s^(3/2) + N * s
 /// ```
 /// Where:
 /// - `s` is the supply of assets,
 /// - `m` is the coefficient for the square root term,
-/// - `n` is the coefficient for the linear term.
+/// - `n` is the coefficient for the constant term,
+/// - `M` is the coefficient for the first (fractional power) term in the
+///   antiderivative,
+/// - `N=n` is the coefficient of the second (linear) term in the
+///   antiderivative.
+///
+/// Coefficients of the antiderivative `N` & `M` are used to parametrize
+/// functions in this module.
 ///
 /// `C(s)` represents the total cost of purchasing or selling assets up to the
 /// current supply `s`. To calculate the incremental cost of a transaction, use
@@ -72,14 +79,19 @@ use crate::{PassiveSupply, Precision};
 /// bonding curve. This struct is used to convert the input parameters to the
 /// correct fixed-point type.
 ///
-/// The input struct assumes that the coefficients are precomputed according to
-/// the integral rules of the square root function./// ### Example
+/// The input struct expects coefficients of terms in the antiderivative of the
+/// square root function, which must be precomputed according to the integral
+/// rules of the square root function.
+///
+/// ### Example
+///
+/// For a square root cost function `c(s) = 3 * s^1/2 + 2`
 ///
-/// For a square root cost function `c(s) = 3 * s^1/2 + 2
+/// which results in the antiderivative
+/// `C(s) = (2 / 3) * 3 * s^(3/2) + 2 * s = 2s^(3/2) + 2s`
 ///
-/// which is resulting into the antiderivative
-/// `C(s) = (6 / 3) * s^(1/2) + 2 * s`
-/// the input parameters would be:
+/// the input parameters `M` & `N` (coefficients of the antiderivative) would
+/// be:
 /// ```rust, ignore
 /// SquareRootParametersInput {
 ///    m: 2,