From 916f83ffb098188fc16bf8d7e4a8ed5a67a14ac2 Mon Sep 17 00:00:00 2001 From: Adel Golghalyani <48685760+Ad96el@users.noreply.github.com> Date: Sat, 11 Jan 2025 22:22:26 +0100 Subject: [PATCH 1/6] fix: ensure at least one currency in pool (#844) ## fixes [#3747](https://github.com/KILTprotocol/ticket/issues/3747) --- pallets/pallet-bonded-coins/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pallets/pallet-bonded-coins/src/lib.rs b/pallets/pallet-bonded-coins/src/lib.rs index a4a63b659..469d02089 100644 --- a/pallets/pallet-bonded-coins/src/lib.rs +++ b/pallets/pallet-bonded-coins/src/lib.rs @@ -315,6 +315,8 @@ pub mod pallet { Slippage, /// The calculated collateral is zero. ZeroCollateral, + /// A pool has to contain at least one bonded currency. + ZeroBondedCurrency, } #[pallet::call] @@ -372,6 +374,7 @@ pub mod pallet { let checked_curve = curve.try_into().map_err(|_| Error::::InvalidInput)?; let currency_length = currencies.len(); + ensure!(!currency_length.is_zero(), Error::::ZeroBondedCurrency); let currency_ids = T::NextAssetIds::try_get(currency_length.saturated_into()) .map_err(|e| e.into()) From 957e162519785f20aa6a676db6f79da0e2ed441b Mon Sep 17 00:00:00 2001 From: Raphael Flechtner <39338561+rflechtner@users.noreply.github.com> Date: Thu, 6 Mar 2025 09:20:53 +0100 Subject: [PATCH 2/6] fix: only destroy funds on refund if collateral received is BelowMinimum (#866) Adopts the recommended resolution of audit issue #2. --- pallets/pallet-bonded-coins/src/lib.rs | 5 +- pallets/pallet-bonded-coins/src/mock.rs | 37 +++++- .../src/tests/transactions/refund_account.rs | 110 +++++++++++++++++- 3 files changed, 145 insertions(+), 7 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/lib.rs b/pallets/pallet-bonded-coins/src/lib.rs index 469d02089..d4592b4e7 100644 --- a/pallets/pallet-bonded-coins/src/lib.rs +++ b/pallets/pallet-bonded-coins/src/lib.rs @@ -64,7 +64,7 @@ pub mod pallet { Create as CreateFungibles, Destroy as DestroyFungibles, Inspect as InspectFungibles, Mutate as MutateFungibles, }, - tokens::{Fortitude, Precision as WithdrawalPrecision, Preservation, Provenance}, + tokens::{DepositConsequence, Fortitude, Precision as WithdrawalPrecision, Preservation, Provenance}, AccountTouch, }, Hashable, Parameter, @@ -1119,8 +1119,7 @@ pub mod pallet { if amount.is_zero() || T::Collaterals::can_deposit(pool_details.collateral.clone(), &who, amount, Provenance::Extant) - .into_result() - .is_err() + == DepositConsequence::BelowMinimum { // Funds are burnt but the collateral received is not sufficient to be deposited // to the account. This is tolerated as otherwise we could have edge cases where diff --git a/pallets/pallet-bonded-coins/src/mock.rs b/pallets/pallet-bonded-coins/src/mock.rs index 21d1887fb..a78b10ae1 100644 --- a/pallets/pallet-bonded-coins/src/mock.rs +++ b/pallets/pallet-bonded-coins/src/mock.rs @@ -58,6 +58,7 @@ pub mod runtime { weights::constants::RocksDbWeight, }; use frame_system::{EnsureRoot, EnsureSigned}; + use pallet_assets::FrozenBalance; use sp_core::U256; use sp_runtime::{ traits::{BlakeTwo256, IdentifyAccount, IdentityLookup, Verify}, @@ -69,7 +70,7 @@ pub mod runtime { self as pallet_bonded_coins, traits::NextAssetIds, types::{Locks, PoolStatus}, - Config, DepositBalanceOf, FungiblesAssetIdOf, PoolDetailsOf, + AccountIdOf, Config, DepositBalanceOf, FungiblesAssetIdOf, FungiblesBalanceOf, PoolDetailsOf, }; pub type Hash = sp_core::H256; @@ -190,6 +191,28 @@ pub mod runtime { } } + /// Store freezes for the assets pallet. + #[storage_alias] + pub type Freezes = StorageDoubleMap< + Assets, + Blake2_128Concat, + FungiblesAssetIdOf, + Blake2_128Concat, + AccountIdOf, + FungiblesBalanceOf, + OptionQuery, + >; + + pub struct FreezesHook; + + impl FrozenBalance for FreezesHook { + fn died(_asset: AssetId, _who: &AccountId) {} + + fn frozen_balance(asset: AssetId, who: &AccountId) -> Option { + Freezes::::get(asset, who) + } + } + frame_support::construct_runtime!( pub enum Test { @@ -279,7 +302,7 @@ pub mod runtime { type Currency = Balances; type Extra = (); type ForceOrigin = EnsureRoot; - type Freezer = (); + type Freezer = FreezesHook; type MetadataDepositBase = ConstU128<0>; type MetadataDepositPerByte = ConstU128<0>; type RemoveItemsLimit = ConstU32<5>; @@ -355,6 +378,7 @@ pub mod runtime { // pool_id, PoolDetails pools: Vec<(AccountId, PoolDetailsOf)>, collaterals: Vec, + freezes: Vec<(AssetId, AccountId, Balance)>, } impl ExtBuilder { @@ -378,6 +402,11 @@ pub mod runtime { self } + pub(crate) fn with_freezes(mut self, freezes: Vec<(AssetId, AccountId, Balance)>) -> Self { + self.freezes = freezes; + self + } + pub(crate) fn build(self) -> sp_io::TestExternalities { let mut storage = frame_system::GenesisConfig::::default().build_storage().unwrap(); @@ -448,6 +477,10 @@ pub mod runtime { }); NextAssetId::::set(next_asset_id); + + self.freezes.iter().for_each(|(asset_id, account, amount)| { + Freezes::::set(asset_id, account, Some(*amount)); + }); }); ext diff --git a/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs b/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs index d94d04446..427e1e010 100644 --- a/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs +++ b/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs @@ -17,9 +17,9 @@ // If you feel like getting in touch with us, you can do so at info@botlabs.org use frame_support::{ assert_err, assert_ok, - traits::fungibles::{Create, Inspect, Mutate}, + traits::fungibles::{roles::Inspect as InspectRole, Create, Inspect, Mutate}, }; -use frame_system::{pallet_prelude::OriginFor, RawOrigin}; +use frame_system::{pallet_prelude::OriginFor, ConsumerLimits, RawOrigin}; use sp_runtime::TokenError; use crate::{ @@ -266,6 +266,112 @@ fn refund_below_min_balance() { }); } +#[test] +fn refund_account_fails_when_account_blocked() { + let pool_details = generate_pool_details( + vec![DEFAULT_BONDED_CURRENCY_ID], + get_linear_bonding_curve(), + false, + Some(PoolStatus::Refunding), + Some(ACCOUNT_00), + None, + None, + None, + ); + let pool_id: AccountIdOf = calculate_pool_id(&[DEFAULT_BONDED_CURRENCY_ID]); + + ExtBuilder::default() + .with_native_balances(vec![(ACCOUNT_01, ONE_HUNDRED_KILT)]) + .with_pools(vec![(pool_id.clone(), pool_details)]) + .with_collaterals(vec![DEFAULT_COLLATERAL_CURRENCY_ID]) + .with_bonded_balance(vec![ + (DEFAULT_COLLATERAL_CURRENCY_ID, pool_id.clone(), ONE_HUNDRED_KILT), + (DEFAULT_COLLATERAL_CURRENCY_ID, ACCOUNT_01, ONE_HUNDRED_KILT), + (DEFAULT_BONDED_CURRENCY_ID, ACCOUNT_01, ONE_HUNDRED_KILT), + ]) + .build_and_execute_with_sanity_tests(|| { + let origin: OriginFor = RawOrigin::Signed(ACCOUNT_01).into(); + + Assets::block( + RawOrigin::Signed(Assets::owner(DEFAULT_COLLATERAL_CURRENCY_ID).unwrap()).into(), + DEFAULT_COLLATERAL_CURRENCY_ID, + ACCOUNT_01, + ) + .expect("Failed to block account for test"); + + // Ensure the refund_account call fails due to failing can_deposit check + assert_err!( + BondingPallet::refund_account(origin, pool_id.clone(), ACCOUNT_01, 0, 1), + TokenError::Blocked + ); + }); +} + +#[test] +fn refund_account_fails_if_account_cannot_be_created() { + let pool_details = generate_pool_details( + vec![DEFAULT_BONDED_CURRENCY_ID], + get_linear_bonding_curve(), + false, + Some(PoolStatus::Refunding), + Some(ACCOUNT_00), + None, + None, + None, + ); + let pool_id: AccountIdOf = calculate_pool_id(&[DEFAULT_BONDED_CURRENCY_ID]); + + // get MaxConsumers value + let max_consumers: usize = ::MaxConsumers::max_consumers() + .try_into() + .expect(""); + // Collateral must be non-sufficient for these tests to work, so we'll create a + // new collateral asset in the test + let collateral_id = DEFAULT_BONDED_CURRENCY_ID + 1; + + ExtBuilder::default() + .with_native_balances(vec![ + (ACCOUNT_01, ONE_HUNDRED_KILT), + (pool_id.clone(), ONE_HUNDRED_KILT), + ]) + .with_pools(vec![(pool_id.clone(), pool_details)]) + .with_collaterals(vec![DEFAULT_COLLATERAL_CURRENCY_ID]) + .with_bonded_balance(vec![ + (DEFAULT_COLLATERAL_CURRENCY_ID, pool_id.clone(), ONE_HUNDRED_KILT), + (DEFAULT_BONDED_CURRENCY_ID, ACCOUNT_01, ONE_HUNDRED_KILT), + ]) + // Freeze some funds for ACCOUNT_01 so refund can only be partial + .with_freezes(vec![(DEFAULT_BONDED_CURRENCY_ID, ACCOUNT_01, 100_000)]) + .build_and_execute_with_sanity_tests(|| { + // switch pool's collateral to a non-sufficient one + assert_ok!(>::create(collateral_id, ACCOUNT_00, false, 1)); + Pools::::mutate(&pool_id, |details| details.as_mut().unwrap().collateral = collateral_id); + // make sure pool account holds collateral - this should work because it also + // holds the (sufficient) original collateral + assert_ok!(Assets::mint_into(collateral_id, &pool_id, ONE_HUNDRED_KILT)); + // create non-sufficient assets to increase ACCOUNT_01 consumers count + // the assets pallet requires at least two references to be available for + // creating an account, but creates only one, meaning we create max_consumers - + // 2 currencies (resulting in max_consumers - 1 consumers because we created one + // previously) + for i in 1..max_consumers - 1 { + let i_u32: u32 = i.try_into().expect("Failed to convert to u32"); + let asset_id = collateral_id + i_u32; + assert_ok!(>::create(asset_id, ACCOUNT_00, false, 1)); + assert_ok!(Assets::mint_into(asset_id, &ACCOUNT_01, ONE_HUNDRED_KILT)); + } + + let origin: OriginFor = RawOrigin::Signed(ACCOUNT_01).into(); + + // Collateral is non-sufficient and thus would create additional consumer + // references on ACCOUNT_01, which is too many + assert_err!( + BondingPallet::refund_account(origin, pool_id.clone(), ACCOUNT_01, 0, 1), + TokenError::CannotCreate + ); + }); +} + #[test] fn refund_account_fails_when_pool_not_refunding() { let pool_details = generate_pool_details( From 7e481bbe49761c6efdb6f4db938a2ca7ce9f4393 Mon Sep 17 00:00:00 2001 From: Raphael Flechtner <39338561+rflechtner@users.noreply.github.com> Date: Thu, 6 Mar 2025 10:21:42 +0100 Subject: [PATCH 3/6] fix: increase MaxConsumers to allow accounts to hold more than 15 different bonded coins (#868) fix for audit issue #3 --- .../src/merkle_proofs/v0/provider_state/tests.rs | 2 +- crates/kilt-dip-primitives/src/verifier/parachain/v0/mock.rs | 2 +- pallets/attestation/src/mock.rs | 2 +- pallets/ctype/src/mock.rs | 2 +- pallets/delegation/src/mock.rs | 2 +- pallets/did/src/mock.rs | 2 +- pallets/pallet-asset-switch/src/mock.rs | 2 +- pallets/pallet-bonded-coins/src/mock.rs | 2 +- pallets/pallet-configuration/src/mock.rs | 2 +- pallets/pallet-deposit-storage/src/deposit/mock.rs | 2 +- pallets/pallet-deposit-storage/src/fungible/tests/mock.rs | 2 +- pallets/pallet-deposit-storage/src/mock.rs | 2 +- pallets/pallet-did-lookup/src/mock.rs | 2 +- pallets/pallet-dip-consumer/src/mock.rs | 2 +- pallets/pallet-dip-provider/src/mock.rs | 2 +- pallets/pallet-inflation/src/mock.rs | 2 +- pallets/pallet-migration/src/mock.rs | 2 +- pallets/pallet-relay-store/src/mock.rs | 2 +- pallets/pallet-web3-names/src/mock.rs | 2 +- pallets/parachain-staking/src/mock.rs | 2 +- pallets/public-credentials/src/mock.rs | 2 +- runtimes/common/src/dip/deposit/mock.rs | 2 +- runtimes/common/src/dip/mock.rs | 2 +- runtimes/common/src/fees.rs | 2 +- runtimes/kestrel/src/lib.rs | 2 +- runtimes/peregrine/src/system/mod.rs | 2 +- runtimes/spiritnet/src/system/mod.rs | 2 +- 27 files changed, 27 insertions(+), 27 deletions(-) diff --git a/crates/kilt-dip-primitives/src/merkle_proofs/v0/provider_state/tests.rs b/crates/kilt-dip-primitives/src/merkle_proofs/v0/provider_state/tests.rs index 9afab8b9f..31b814643 100644 --- a/crates/kilt-dip-primitives/src/merkle_proofs/v0/provider_state/tests.rs +++ b/crates/kilt-dip-primitives/src/merkle_proofs/v0/provider_state/tests.rs @@ -251,7 +251,7 @@ mod dip_did_proof_with_verified_relay_state_root { type Hash = H256; type Hashing = BlakeTwo256; type Lookup = IdentityLookup; - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; type Nonce = u64; type OnKilledAccount = (); type OnNewAccount = (); diff --git a/crates/kilt-dip-primitives/src/verifier/parachain/v0/mock.rs b/crates/kilt-dip-primitives/src/verifier/parachain/v0/mock.rs index d59556af1..7c7a87be8 100644 --- a/crates/kilt-dip-primitives/src/verifier/parachain/v0/mock.rs +++ b/crates/kilt-dip-primitives/src/verifier/parachain/v0/mock.rs @@ -71,7 +71,7 @@ impl frame_system::Config for TestRuntime { type Hash = H256; type Hashing = BlakeTwo256; type Lookup = IdentityLookup; - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; type Nonce = u64; type OnKilledAccount = (); type OnNewAccount = (); diff --git a/pallets/attestation/src/mock.rs b/pallets/attestation/src/mock.rs index cbf789867..288ae876c 100644 --- a/pallets/attestation/src/mock.rs +++ b/pallets/attestation/src/mock.rs @@ -261,7 +261,7 @@ pub(crate) mod runtime { type BlockLength = (); type SS58Prefix = SS58Prefix; type OnSetCode = (); - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; } parameter_types! { diff --git a/pallets/ctype/src/mock.rs b/pallets/ctype/src/mock.rs index ce6ea8b77..ad90596ed 100644 --- a/pallets/ctype/src/mock.rs +++ b/pallets/ctype/src/mock.rs @@ -100,7 +100,7 @@ pub mod runtime { type BlockLength = (); type SS58Prefix = SS58Prefix; type OnSetCode = (); - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; } parameter_types! { diff --git a/pallets/delegation/src/mock.rs b/pallets/delegation/src/mock.rs index 7c816fe25..9b53f80e6 100644 --- a/pallets/delegation/src/mock.rs +++ b/pallets/delegation/src/mock.rs @@ -247,7 +247,7 @@ pub(crate) mod runtime { type BlockLength = (); type SS58Prefix = SS58Prefix; type OnSetCode = (); - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; } parameter_types! { diff --git a/pallets/did/src/mock.rs b/pallets/did/src/mock.rs index db59b5f3f..752e45acc 100644 --- a/pallets/did/src/mock.rs +++ b/pallets/did/src/mock.rs @@ -105,7 +105,7 @@ impl frame_system::Config for Test { type BlockLength = (); type SS58Prefix = SS58Prefix; type OnSetCode = (); - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; } parameter_types! { diff --git a/pallets/pallet-asset-switch/src/mock.rs b/pallets/pallet-asset-switch/src/mock.rs index e6b868d7f..f232eb264 100644 --- a/pallets/pallet-asset-switch/src/mock.rs +++ b/pallets/pallet-asset-switch/src/mock.rs @@ -66,7 +66,7 @@ impl frame_system::Config for MockRuntime { type Hash = H256; type Hashing = BlakeTwo256; type Lookup = IdentityLookup; - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; type Nonce = u64; type OnKilledAccount = (); type OnNewAccount = (); diff --git a/pallets/pallet-bonded-coins/src/mock.rs b/pallets/pallet-bonded-coins/src/mock.rs index a78b10ae1..083982590 100644 --- a/pallets/pallet-bonded-coins/src/mock.rs +++ b/pallets/pallet-bonded-coins/src/mock.rs @@ -249,7 +249,7 @@ pub mod runtime { type Hash = Hash; type Hashing = BlakeTwo256; type Lookup = IdentityLookup; - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; type Nonce = u64; type OnKilledAccount = (); type OnNewAccount = (); diff --git a/pallets/pallet-configuration/src/mock.rs b/pallets/pallet-configuration/src/mock.rs index 26ab3933f..cee13ca52 100644 --- a/pallets/pallet-configuration/src/mock.rs +++ b/pallets/pallet-configuration/src/mock.rs @@ -79,7 +79,7 @@ pub mod runtime { type BlockLength = (); type SS58Prefix = SS58Prefix; type OnSetCode = (); - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; } parameter_types! { diff --git a/pallets/pallet-deposit-storage/src/deposit/mock.rs b/pallets/pallet-deposit-storage/src/deposit/mock.rs index 3eeb97eb5..906a76cac 100644 --- a/pallets/pallet-deposit-storage/src/deposit/mock.rs +++ b/pallets/pallet-deposit-storage/src/deposit/mock.rs @@ -73,7 +73,7 @@ impl frame_system::Config for TestRuntime { type Hash = H256; type Hashing = BlakeTwo256; type Lookup = IdentityLookup; - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; type Nonce = u64; type OnKilledAccount = (); type OnNewAccount = (); diff --git a/pallets/pallet-deposit-storage/src/fungible/tests/mock.rs b/pallets/pallet-deposit-storage/src/fungible/tests/mock.rs index 9ef33876c..2f050e923 100644 --- a/pallets/pallet-deposit-storage/src/fungible/tests/mock.rs +++ b/pallets/pallet-deposit-storage/src/fungible/tests/mock.rs @@ -75,7 +75,7 @@ impl frame_system::Config for TestRuntime { type Hash = H256; type Hashing = BlakeTwo256; type Lookup = IdentityLookup; - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; type Nonce = u64; type OnKilledAccount = (); type OnNewAccount = (); diff --git a/pallets/pallet-deposit-storage/src/mock.rs b/pallets/pallet-deposit-storage/src/mock.rs index 2ee2f4382..0e8a61451 100644 --- a/pallets/pallet-deposit-storage/src/mock.rs +++ b/pallets/pallet-deposit-storage/src/mock.rs @@ -60,7 +60,7 @@ impl frame_system::Config for TestRuntime { type Hash = H256; type Hashing = BlakeTwo256; type Lookup = IdentityLookup; - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; type Nonce = u64; type OnKilledAccount = (); type OnNewAccount = (); diff --git a/pallets/pallet-did-lookup/src/mock.rs b/pallets/pallet-did-lookup/src/mock.rs index 5ed55e1e6..fe3b31018 100644 --- a/pallets/pallet-did-lookup/src/mock.rs +++ b/pallets/pallet-did-lookup/src/mock.rs @@ -80,7 +80,7 @@ impl frame_system::Config for Test { type SystemWeightInfo = (); type SS58Prefix = SS58Prefix; type OnSetCode = (); - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; } parameter_types! { diff --git a/pallets/pallet-dip-consumer/src/mock.rs b/pallets/pallet-dip-consumer/src/mock.rs index c5d597b3c..353fa6f7a 100644 --- a/pallets/pallet-dip-consumer/src/mock.rs +++ b/pallets/pallet-dip-consumer/src/mock.rs @@ -54,7 +54,7 @@ impl frame_system::Config for TestRuntime { type Hash = H256; type Hashing = BlakeTwo256; type Lookup = IdentityLookup; - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; type Nonce = u64; type OnKilledAccount = (); type OnNewAccount = (); diff --git a/pallets/pallet-dip-provider/src/mock.rs b/pallets/pallet-dip-provider/src/mock.rs index fb5430145..d4c60666d 100644 --- a/pallets/pallet-dip-provider/src/mock.rs +++ b/pallets/pallet-dip-provider/src/mock.rs @@ -54,7 +54,7 @@ impl frame_system::Config for TestRuntime { type Hash = H256; type Hashing = BlakeTwo256; type Lookup = IdentityLookup; - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; type Nonce = u64; type OnKilledAccount = (); type OnNewAccount = (); diff --git a/pallets/pallet-inflation/src/mock.rs b/pallets/pallet-inflation/src/mock.rs index a6e7d489a..bd4bdf6b5 100644 --- a/pallets/pallet-inflation/src/mock.rs +++ b/pallets/pallet-inflation/src/mock.rs @@ -82,7 +82,7 @@ impl frame_system::Config for Test { type SystemWeightInfo = (); type SS58Prefix = SS58Prefix; type OnSetCode = (); - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; } parameter_types! { diff --git a/pallets/pallet-migration/src/mock.rs b/pallets/pallet-migration/src/mock.rs index 7e658653b..0d5d3a1c6 100644 --- a/pallets/pallet-migration/src/mock.rs +++ b/pallets/pallet-migration/src/mock.rs @@ -114,7 +114,7 @@ impl frame_system::Config for Test { type BlockLength = (); type SS58Prefix = SS58Prefix; type OnSetCode = (); - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; } parameter_types! { diff --git a/pallets/pallet-relay-store/src/mock.rs b/pallets/pallet-relay-store/src/mock.rs index 698f21984..285ed3fc8 100644 --- a/pallets/pallet-relay-store/src/mock.rs +++ b/pallets/pallet-relay-store/src/mock.rs @@ -53,7 +53,7 @@ impl frame_system::Config for TestRuntime { type Hash = H256; type Hashing = BlakeTwo256; type Lookup = IdentityLookup; - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; type Nonce = u64; type OnKilledAccount = (); type OnNewAccount = (); diff --git a/pallets/pallet-web3-names/src/mock.rs b/pallets/pallet-web3-names/src/mock.rs index 9a5971c65..4b511bf7c 100644 --- a/pallets/pallet-web3-names/src/mock.rs +++ b/pallets/pallet-web3-names/src/mock.rs @@ -114,7 +114,7 @@ pub(crate) mod runtime { type SystemWeightInfo = (); type SS58Prefix = SS58Prefix; type OnSetCode = (); - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; type RuntimeTask = RuntimeTask; } diff --git a/pallets/parachain-staking/src/mock.rs b/pallets/parachain-staking/src/mock.rs index 09e14e199..35cd18288 100644 --- a/pallets/parachain-staking/src/mock.rs +++ b/pallets/parachain-staking/src/mock.rs @@ -92,7 +92,7 @@ impl frame_system::Config for Test { type BlockLength = (); type SS58Prefix = SS58Prefix; type OnSetCode = (); - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; } parameter_types! { pub const ExistentialDeposit: Balance = 1; diff --git a/pallets/public-credentials/src/mock.rs b/pallets/public-credentials/src/mock.rs index cf3911c08..4cb3a721c 100644 --- a/pallets/public-credentials/src/mock.rs +++ b/pallets/public-credentials/src/mock.rs @@ -302,7 +302,7 @@ pub(crate) mod runtime { type BlockLength = (); type SS58Prefix = ConstU16<38>; type OnSetCode = (); - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; } impl pallet_balances::Config for Test { diff --git a/runtimes/common/src/dip/deposit/mock.rs b/runtimes/common/src/dip/deposit/mock.rs index 152903165..2389dbd35 100644 --- a/runtimes/common/src/dip/deposit/mock.rs +++ b/runtimes/common/src/dip/deposit/mock.rs @@ -52,7 +52,7 @@ impl frame_system::Config for TestRuntime { type Hash = Hash; type Hashing = Hasher; type Lookup = IdentityLookup; - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; type Nonce = Nonce; type OnKilledAccount = (); type OnNewAccount = (); diff --git a/runtimes/common/src/dip/mock.rs b/runtimes/common/src/dip/mock.rs index 74d9fad94..ffc526bd7 100644 --- a/runtimes/common/src/dip/mock.rs +++ b/runtimes/common/src/dip/mock.rs @@ -74,7 +74,7 @@ impl frame_system::Config for TestRuntime { type Hash = Hash; type Hashing = Hasher; type Lookup = IdentityLookup; - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; type Nonce = Nonce; type OnKilledAccount = (); type OnNewAccount = (); diff --git a/runtimes/common/src/fees.rs b/runtimes/common/src/fees.rs index f012182e3..237dabd6c 100644 --- a/runtimes/common/src/fees.rs +++ b/runtimes/common/src/fees.rs @@ -219,7 +219,7 @@ mod tests { type SystemWeightInfo = (); type SS58Prefix = (); type OnSetCode = (); - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; } impl pallet_balances::Config for Test { diff --git a/runtimes/kestrel/src/lib.rs b/runtimes/kestrel/src/lib.rs index 65c911f22..aaf162d5d 100644 --- a/runtimes/kestrel/src/lib.rs +++ b/runtimes/kestrel/src/lib.rs @@ -198,7 +198,7 @@ impl frame_system::Config for Runtime { type SS58Prefix = SS58Prefix; /// The set code logic, just the default since we're not a parachain. type OnSetCode = (); - type MaxConsumers = ConstU32<16>; + type MaxConsumers = ConstU32<100>; } /// Maximum number of nominators per validator. diff --git a/runtimes/peregrine/src/system/mod.rs b/runtimes/peregrine/src/system/mod.rs index 906ccacf6..fc545c722 100644 --- a/runtimes/peregrine/src/system/mod.rs +++ b/runtimes/peregrine/src/system/mod.rs @@ -93,7 +93,7 @@ impl frame_system::Config for Runtime { type SS58Prefix = ConstU16; /// The set code logic type OnSetCode = cumulus_pallet_parachain_system::ParachainSetCode; - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; } impl pallet_timestamp::Config for Runtime { diff --git a/runtimes/spiritnet/src/system/mod.rs b/runtimes/spiritnet/src/system/mod.rs index f672f9b4c..bc4a2e8e8 100644 --- a/runtimes/spiritnet/src/system/mod.rs +++ b/runtimes/spiritnet/src/system/mod.rs @@ -93,7 +93,7 @@ impl frame_system::Config for Runtime { type SS58Prefix = ConstU16; /// The set code logic type OnSetCode = cumulus_pallet_parachain_system::ParachainSetCode; - type MaxConsumers = frame_support::traits::ConstU32<16>; + type MaxConsumers = frame_support::traits::ConstU32<100>; } impl pallet_timestamp::Config for Runtime { From cc4475c68a6af9118e5b07cc5dd3d08cadb87834 Mon Sep 17 00:00:00 2001 From: Raphael Flechtner <39338561+rflechtner@users.noreply.github.com> Date: Thu, 6 Mar 2025 16:17:57 +0100 Subject: [PATCH 4/6] refactor: batch updating of asset team (#869) fixes audit issue 6 --- .../pallet-bonded-coins/src/benchmarking.rs | 22 +++---- pallets/pallet-bonded-coins/src/lib.rs | 41 +++++++------ .../src/tests/transactions/reset_team.rs | 58 ++++++++++++++++--- 3 files changed, 80 insertions(+), 41 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/benchmarking.rs b/pallets/pallet-bonded-coins/src/benchmarking.rs index 9bb68b93b..f1f962600 100644 --- a/pallets/pallet-bonded-coins/src/benchmarking.rs +++ b/pallets/pallet-bonded-coins/src/benchmarking.rs @@ -355,7 +355,7 @@ mod benchmarks { } #[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() @@ -363,17 +363,10 @@ mod benchmarks { .expect("generating account_id from origin should not fail"); make_free_for_deposit::(&account_origin); - let bonded_coin_id = T::BenchmarkHelper::calculate_bonded_asset_id(0); - create_bonded_asset::(bonded_coin_id.clone()); + let bonded_currencies = create_bonded_currencies_in_range::(c, false); let curve = get_linear_bonding_curve::>(); - let pool_id = create_pool::( - curve, - [bonded_coin_id.clone()].to_vec(), - Some(account_origin), - None, - None, - ); + let pool_id = create_pool::(curve, bonded_currencies.clone(), Some(account_origin), None, None); let admin: AccountIdOf = account("admin", 0, 0); let freezer: AccountIdOf = account("freezer", 0, 0); @@ -382,12 +375,15 @@ mod benchmarks { freezer: freezer.clone(), }; + let max_currencies = T::MaxCurrenciesPerPool::get(); #[extrinsic_call] - _(origin as T::RuntimeOrigin, pool_id, fungibles_team, 0); + _(origin as T::RuntimeOrigin, pool_id, 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())); + }); } #[benchmark] diff --git a/pallets/pallet-bonded-coins/src/lib.rs b/pallets/pallet-bonded-coins/src/lib.rs index d4592b4e7..e8ae6641f 100644 --- a/pallets/pallet-bonded-coins/src/lib.rs +++ b/pallets/pallet-bonded-coins/src/lib.rs @@ -449,18 +449,17 @@ pub mod pallet { Ok(()) } - /// Changes the managing team of a bonded currency which is issued by - /// this pool. The new team will be set to the provided team. The - /// currency index is used to select the currency that the team will - /// manage. The origin account must be a manager of the pool. + /// Changes the managing team of all bonded currencies issued by this + /// pool, setting it to the provided `team`. The origin account must be + /// a manager of the pool. /// /// # Parameters /// - `origin`: The origin of the call, requiring the caller to be a /// manager of the pool. /// - `pool_id`: The identifier of the pool. /// - `team`: The new managing team. - /// - `currency_idx`: The index of the currency in the bonded currencies - /// vector. + /// - `currency_count`: The number of bonded currencies vector linked to + /// the pool. Required for weight estimations. /// /// # Returns /// - `DispatchResult`: The result of the dispatch. @@ -469,8 +468,8 @@ pub mod pallet { /// - `Error::::PoolUnknown`: If the pool does not exist. /// - `Error::::NoPermission`: If the caller is not a manager of the /// pool. - /// - `Error::::IndexOutOfBounds`: If the currency index is out of - /// bounds. + /// - `Error::::CurrencyCount`: If the actual number of currencies in + /// the pool is larger than `currency_count`. /// - Other errors depending on the types in the config. #[pallet::call_index(1)] #[pallet::weight(T::WeightInfo::reset_team())] @@ -478,31 +477,31 @@ pub mod pallet { origin: OriginFor, pool_id: T::PoolId, team: PoolManagingTeam>, - currency_idx: u32, + currency_count: u32, ) -> DispatchResult { let who = T::DefaultOrigin::ensure_origin(origin)?; let pool_details = Pools::::get(&pool_id).ok_or(Error::::PoolUnknown)?; + let number_of_currencies = Self::get_currencies_number(&pool_details); + ensure!(number_of_currencies <= currency_count, Error::::CurrencyCount); + ensure!(pool_details.is_manager(&who), Error::::NoPermission); ensure!(pool_details.state.is_live(), Error::::PoolNotLive); - let asset_id = pool_details - .bonded_currencies - .get(currency_idx.saturated_into::()) - .ok_or(Error::::IndexOutOfBounds)?; - let pool_id_account = pool_id.into(); let PoolManagingTeam { freezer, admin } = team; - T::Fungibles::reset_team( - asset_id.to_owned(), - pool_id_account.clone(), - admin, - pool_id_account, - freezer, - ) + pool_details.bonded_currencies.into_iter().try_for_each(|asset_id| { + T::Fungibles::reset_team( + asset_id, + pool_id_account.clone(), + admin.clone(), + pool_id_account.clone(), + freezer.clone(), + ) + }) } /// Resets the manager of a pool. The new manager will be set to the diff --git a/pallets/pallet-bonded-coins/src/tests/transactions/reset_team.rs b/pallets/pallet-bonded-coins/src/tests/transactions/reset_team.rs index 6de6eda4f..b8974496e 100644 --- a/pallets/pallet-bonded-coins/src/tests/transactions/reset_team.rs +++ b/pallets/pallet-bonded-coins/src/tests/transactions/reset_team.rs @@ -51,7 +51,7 @@ fn resets_team() { admin: ACCOUNT_00, freezer: ACCOUNT_01, }, - 0 + 1 )); assert_eq!(Assets::admin(DEFAULT_BONDED_CURRENCY_ID), Some(ACCOUNT_00)); @@ -61,6 +61,50 @@ fn resets_team() { }) } +#[test] +fn resets_team_for_all() { + let currencies = vec![DEFAULT_BONDED_CURRENCY_ID, DEFAULT_BONDED_CURRENCY_ID + 1]; + + let pool_details = generate_pool_details( + currencies.clone(), + get_linear_bonding_curve(), + false, + Some(PoolStatus::Active), + Some(ACCOUNT_00), + None, + None, + None, + ); + let pool_id: AccountIdOf = calculate_pool_id(¤cies); + + ExtBuilder::default() + .with_pools(vec![(pool_id.clone(), pool_details)]) + .with_collaterals(vec![DEFAULT_COLLATERAL_CURRENCY_ID]) + .build_and_execute_with_sanity_tests(|| { + let manager_origin = RawOrigin::Signed(ACCOUNT_00).into(); + + assert_ok!(BondingPallet::reset_team( + manager_origin, + pool_id.clone(), + PoolManagingTeam { + admin: ACCOUNT_00, + freezer: ACCOUNT_01, + }, + 2 + )); + + assert_eq!(Assets::admin(currencies[0]), Some(ACCOUNT_00)); + assert_eq!(Assets::freezer(currencies[0]), Some(ACCOUNT_01)); + assert_eq!(Assets::owner(currencies[0]), Some(pool_id.clone())); + assert_eq!(Assets::issuer(currencies[0]), Some(pool_id.clone())); + + assert_eq!(Assets::admin(currencies[1]), Some(ACCOUNT_00)); + assert_eq!(Assets::freezer(currencies[1]), Some(ACCOUNT_01)); + assert_eq!(Assets::owner(currencies[1]), Some(pool_id.clone())); + assert_eq!(Assets::issuer(currencies[1]), Some(pool_id)); + }) +} + #[test] fn does_not_change_team_when_not_live() { let pool_details = generate_pool_details( @@ -89,7 +133,7 @@ fn does_not_change_team_when_not_live() { admin: ACCOUNT_00, freezer: ACCOUNT_00, }, - 0 + 1 ), BondingPalletErrors::::PoolNotLive ); @@ -130,7 +174,7 @@ fn only_manager_can_change_team() { admin: ACCOUNT_00, freezer: ACCOUNT_00, }, - 0 + 1 ), BondingPalletErrors::::NoPermission ); @@ -143,7 +187,7 @@ fn only_manager_can_change_team() { admin: ACCOUNT_00, freezer: ACCOUNT_00, }, - 0 + 1 ), BondingPalletErrors::::NoPermission ); @@ -153,7 +197,7 @@ fn only_manager_can_change_team() { } #[test] -fn handles_currency_idx_out_of_bounds() { +fn handles_currency_number_incorrect() { let pool_details = generate_pool_details( vec![DEFAULT_BONDED_CURRENCY_ID], get_linear_bonding_curve(), @@ -180,9 +224,9 @@ fn handles_currency_idx_out_of_bounds() { admin: ACCOUNT_00, freezer: ACCOUNT_00, }, - 2 + 0 ), - BondingPalletErrors::::IndexOutOfBounds + BondingPalletErrors::::CurrencyCount ); }) } From 40d0a73596fec8319072785348db605f6955af5c Mon Sep 17 00:00:00 2001 From: Raphael Date: Tue, 5 Nov 2024 16:07:00 +0100 Subject: [PATCH 5/6] feat: reset team on start_refund --- pallets/pallet-bonded-coins/src/lib.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/lib.rs b/pallets/pallet-bonded-coins/src/lib.rs index e8ae6641f..90b3c832e 100644 --- a/pallets/pallet-bonded-coins/src/lib.rs +++ b/pallets/pallet-bonded-coins/src/lib.rs @@ -1428,8 +1428,10 @@ pub mod pallet { Error::::NothingToRefund ); - let has_holders = pool_details - .bonded_currencies + // cloning here lets us avoid cloning the pool details later + let bonded_currencies = pool_details.bonded_currencies.clone(); + + let has_holders = bonded_currencies .iter() .any(|asset_id| T::Fungibles::total_issuance(asset_id.clone()) > FungiblesBalanceOf::::zero()); // no token holders to refund @@ -1440,6 +1442,18 @@ pub mod pallet { new_pool_details.state.start_refund(); Pools::::set(&pool_id, Some(new_pool_details)); + // reset team on currencies to avoid unexpected burns etc. + let pool_account = pool_id.clone().into(); + for asset_id in bonded_currencies { + T::Fungibles::reset_team( + asset_id, + pool_account.clone(), + pool_account.clone(), + pool_account.clone(), + pool_account.clone(), + )?; + } + Self::deposit_event(Event::RefundingStarted { id: pool_id }); Ok(n_currencies) From 59856a5bfdab52958ebffd995265ccd2cfa8c399 Mon Sep 17 00:00:00 2001 From: Raphael Date: Tue, 26 Nov 2024 13:43:53 +0100 Subject: [PATCH 6/6] test: reset team on refund --- .../src/tests/transactions/start_refund.rs | 54 ++++++++++++++++++- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/tests/transactions/start_refund.rs b/pallets/pallet-bonded-coins/src/tests/transactions/start_refund.rs index f26632206..d5b88660d 100644 --- a/pallets/pallet-bonded-coins/src/tests/transactions/start_refund.rs +++ b/pallets/pallet-bonded-coins/src/tests/transactions/start_refund.rs @@ -15,13 +15,13 @@ // along with this program. If not, see . // If you feel like getting in touch with us, you can do so at info@botlabs.org -use frame_support::{assert_err, assert_ok}; +use frame_support::{assert_err, assert_ok, traits::fungibles::roles::Inspect}; use frame_system::{pallet_prelude::OriginFor, RawOrigin}; use sp_runtime::traits::BadOrigin; use crate::{ mock::{runtime::*, *}, - types::PoolStatus, + types::{PoolManagingTeam, PoolStatus}, AccountIdOf, Error, Event, Pools, }; @@ -62,6 +62,56 @@ fn start_refund_works() { }); } +#[test] +fn start_refund_resets_asset_admin() { + let pool_details = generate_pool_details( + vec![DEFAULT_BONDED_CURRENCY_ID], + get_linear_bonding_curve(), + true, + Some(PoolStatus::Active), + Some(ACCOUNT_00), + Some(DEFAULT_COLLATERAL_CURRENCY_ID), + Some(ACCOUNT_00), + None, + ); + let pool_id: AccountIdOf = calculate_pool_id(&[DEFAULT_BONDED_CURRENCY_ID]); + let currency_count = 1; + + ExtBuilder::default() + .with_pools(vec![(pool_id.clone(), pool_details)]) + .with_native_balances(vec![(ACCOUNT_00, u128::MAX)]) + .with_collaterals(vec![DEFAULT_COLLATERAL_CURRENCY_ID]) + .with_bonded_balance(vec![ + (DEFAULT_COLLATERAL_CURRENCY_ID, pool_id.clone(), u128::MAX / 10), + (DEFAULT_BONDED_CURRENCY_ID, ACCOUNT_00, u128::MAX / 10), + ]) + .build() + .execute_with(|| { + let origin: OriginFor = RawOrigin::Signed(ACCOUNT_00).into(); + + assert_ok!(BondingPallet::reset_team( + origin.clone(), + pool_id.clone(), + PoolManagingTeam { + admin: ACCOUNT_00, + freezer: ACCOUNT_00 + }, + 0 + )); + + assert_eq!(Assets::admin(DEFAULT_BONDED_CURRENCY_ID), Some(ACCOUNT_00)); + assert_eq!(Assets::freezer(DEFAULT_BONDED_CURRENCY_ID), Some(ACCOUNT_00)); + + assert_ok!(BondingPallet::start_refund(origin, pool_id.clone(), currency_count)); + + // Verify that the pool state has been updated to refunding + assert_eq!(Pools::::get(&pool_id).unwrap().state, PoolStatus::Refunding); + + assert_eq!(Assets::admin(DEFAULT_BONDED_CURRENCY_ID), Some(pool_id.clone())); + assert_eq!(Assets::freezer(DEFAULT_BONDED_CURRENCY_ID), Some(pool_id)); + }); +} + #[test] fn start_refund_fails_when_pool_not_live() { let pool_details = generate_pool_details(