From b6130557428fe732a67f10148a59410888e43ead Mon Sep 17 00:00:00 2001 From: Gav Date: Tue, 18 Apr 2023 13:45:14 +0100 Subject: [PATCH 1/9] Fix: Incorrect implementation of can_reserve check --- frame/balances/src/impl_currency.rs | 7 +------ frame/balances/src/tests/currency_tests.rs | 10 ++++++++++ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/frame/balances/src/impl_currency.rs b/frame/balances/src/impl_currency.rs index 329ea289f966e..4812d173fa3de 100644 --- a/frame/balances/src/impl_currency.rs +++ b/frame/balances/src/impl_currency.rs @@ -483,12 +483,7 @@ where /// /// Always `true` if value to be reserved is zero. fn can_reserve(who: &T::AccountId, value: Self::Balance) -> bool { - if value.is_zero() { - return true - } - Self::account(who).free.checked_sub(&value).map_or(false, |new_balance| { - Self::ensure_can_withdraw(who, value, WithdrawReasons::RESERVE, new_balance).is_ok() - }) + value <= >::reducible_balance(who, Protect, Force) } fn reserved_balance(who: &T::AccountId) -> Self::Balance { diff --git a/frame/balances/src/tests/currency_tests.rs b/frame/balances/src/tests/currency_tests.rs index 034c92f65689b..e25b122c1fcf0 100644 --- a/frame/balances/src/tests/currency_tests.rs +++ b/frame/balances/src/tests/currency_tests.rs @@ -1180,6 +1180,16 @@ fn named_reserve_should_work() { }); } +#[test] +fn reserve_must_succeed_if_can_reserve_does() { + ExtBuilder::default().build_and_execute_with(|| { + let _ = Balances::deposit_creating(&1, 1); + let _ = Balances::deposit_creating(&2, 2); + assert!(Balances::can_reserve(&1, 1) == Balances::reserve(&1, 1).is_ok()); + assert!(Balances::can_reserve(&2, 1) == Balances::reserve(&2, 1).is_ok()); + }); +} + #[test] fn reserved_named_to_yourself_should_work() { ExtBuilder::default().build_and_execute_with(|| { From 94389bf02488f9ee5595a88ce359bbc7c9e22e14 Mon Sep 17 00:00:00 2001 From: Gav Date: Tue, 18 Apr 2023 13:45:58 +0100 Subject: [PATCH 2/9] Fix: Incorrect migration of consumer counting for existing accounts with frozen amounts --- frame/balances/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 74aec1f2d259b..03a8c8ddfb73d 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -784,7 +784,7 @@ pub mod pallet { return false } a.flags.set_new_logic(); - if !a.reserved.is_zero() || !a.frozen.is_zero() { + if !a.reserved.is_zero() { if system::Pallet::::providers(who) == 0 { // Gah!! We have no provider refs :( // This shouldn't practically happen, but we need a failsafe anyway: let's give From 70e497b4220231e18e9f0df77ca2149edce77aff Mon Sep 17 00:00:00 2001 From: Gav Date: Tue, 18 Apr 2023 14:02:55 +0100 Subject: [PATCH 3/9] Fix: Inconsistent implementation between assets can_deposit and new_account --- frame/assets/src/functions.rs | 2 +- frame/assets/src/tests.rs | 21 ++++++++++++++++++++- frame/system/src/lib.rs | 13 ++++++++++++- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index af9e269acf8bc..1e10e0066e851 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -138,7 +138,7 @@ impl, I: 'static> Pallet { if amount < details.min_balance { return DepositConsequence::BelowMinimum } - if !details.is_sufficient && !frame_system::Pallet::::can_inc_consumer(who) { + if !details.is_sufficient && !frame_system::Pallet::::can_accrue_consumers(who, 2) { return DepositConsequence::CannotCreate } if details.is_sufficient && details.sufficients.checked_add(1).is_none() { diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index afd224ad66642..38e7a07f8973a 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -21,7 +21,7 @@ use super::*; use crate::{mock::*, Error}; use frame_support::{ assert_noop, assert_ok, - traits::{fungibles::InspectEnumerable, Currency}, + traits::{fungibles::InspectEnumerable, Currency, tokens::Preservation::Protect}, }; use pallet_balances::Error as BalancesError; use sp_io::storage; @@ -33,6 +33,25 @@ fn asset_ids() -> Vec { s } +#[test] +fn transfer_should_never_burn() { + new_test_ext().execute_with(|| { + assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, false, 1)); + Balances::make_free_balance_be(&1, 100); + Balances::make_free_balance_be(&2, 100); + + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); + assert_eq!(Assets::balance(0, 1), 100); + + while System::inc_consumers(&2).is_ok() {} + let _ = System::dec_consumers(&2); + // Exactly one consumer ref remaining. + + let _ = >::transfer(0, &1, &2, 50, Protect); + assert_eq!(Assets::balance(0, 1) + Assets::balance(0, 2), 100); + }); +} + #[test] fn basic_minting_should_work() { new_test_ext().execute_with(|| { diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 88291c326edba..f7271a828105a 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -1222,7 +1222,18 @@ impl Pallet { a.consumers == 0 || a.providers > 1 } - /// True if the account has at least one provider reference. + /// True if the account has at least one provider reference and adding `amount` consumer + /// references would not take it above the the maximum. + pub fn can_accrue_consumers(who: &T::AccountId, amount: u32) -> bool { + let a = Account::::get(who); + match a.consumers.checked_add(amount) { + Some(c) => a.providers > 0 && c <= T::MaxConsumers::max_consumers(), + None => false, + } + } + + /// True if the account has at least one provider reference and fewer consumer references than + /// the maximum. pub fn can_inc_consumer(who: &T::AccountId) -> bool { let a = Account::::get(who); a.providers > 0 && a.consumers < T::MaxConsumers::max_consumers() From 924ddd100ed514d02218af038be54eb446b27124 Mon Sep 17 00:00:00 2001 From: Gav Date: Tue, 18 Apr 2023 19:40:57 +0100 Subject: [PATCH 4/9] Fixes --- frame/assets/src/tests.rs | 2 +- frame/balances/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index 38e7a07f8973a..02a02728a52c8 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -21,7 +21,7 @@ use super::*; use crate::{mock::*, Error}; use frame_support::{ assert_noop, assert_ok, - traits::{fungibles::InspectEnumerable, Currency, tokens::Preservation::Protect}, + traits::{fungibles::InspectEnumerable, tokens::Preservation::Protect, Currency}, }; use pallet_balances::Error as BalancesError; use sp_io::storage; diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 03a8c8ddfb73d..7653292f6267b 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -784,7 +784,7 @@ pub mod pallet { return false } a.flags.set_new_logic(); - if !a.reserved.is_zero() { + if !a.reserved.is_zero() && a.frozen.is_zero() { if system::Pallet::::providers(who) == 0 { // Gah!! We have no provider refs :( // This shouldn't practically happen, but we need a failsafe anyway: let's give From cd8ad70d71c8baefec436cda180f3ea6233fc05c Mon Sep 17 00:00:00 2001 From: Gav Date: Tue, 18 Apr 2023 19:44:59 +0100 Subject: [PATCH 5/9] Fixes --- frame/balances/src/impl_currency.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/frame/balances/src/impl_currency.rs b/frame/balances/src/impl_currency.rs index 4812d173fa3de..8e726ffa3683f 100644 --- a/frame/balances/src/impl_currency.rs +++ b/frame/balances/src/impl_currency.rs @@ -483,7 +483,14 @@ where /// /// Always `true` if value to be reserved is zero. fn can_reserve(who: &T::AccountId, value: Self::Balance) -> bool { - value <= >::reducible_balance(who, Protect, Force) + if value.is_zero() { + return true + } + Self::account(who).free.checked_sub(&value).map_or(false, |new_balance| { + new_balance >= T::ExistentialDeposit::get() && + Self::ensure_can_withdraw(who, value, WithdrawReasons::RESERVE, new_balance) + .is_ok() + }) } fn reserved_balance(who: &T::AccountId) -> Self::Balance { From 05aeac137483c5f26d7276ebeabd1d4f518e9417 Mon Sep 17 00:00:00 2001 From: Gav Date: Wed, 19 Apr 2023 12:49:06 +0100 Subject: [PATCH 6/9] Another fix --- frame/assets/src/tests.rs | 31 ++++++++++++++++------ frame/balances/src/tests/fungible_tests.rs | 25 +++++++++++++++++ frame/system/src/lib.rs | 4 ++- 3 files changed, 51 insertions(+), 9 deletions(-) diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index 02a02728a52c8..64d54ec446d30 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -76,10 +76,7 @@ fn minting_too_many_insufficient_assets_fails() { Balances::make_free_balance_be(&1, 100); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 1, 1, 100)); - assert_noop!( - Assets::mint(RuntimeOrigin::signed(1), 2, 1, 100), - Error::::UnavailableConsumer - ); + assert_noop!(Assets::mint(RuntimeOrigin::signed(1), 2, 1, 100), TokenError::CannotCreate); Balances::make_free_balance_be(&2, 1); assert_ok!(Assets::transfer(RuntimeOrigin::signed(1), 0, 2, 100)); @@ -97,10 +94,7 @@ fn minting_insufficient_asset_with_deposit_should_work_when_consumers_exhausted( Balances::make_free_balance_be(&1, 100); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 1, 1, 100)); - assert_noop!( - Assets::mint(RuntimeOrigin::signed(1), 2, 1, 100), - Error::::UnavailableConsumer - ); + assert_noop!(Assets::mint(RuntimeOrigin::signed(1), 2, 1, 100), TokenError::CannotCreate); assert_ok!(Assets::touch(RuntimeOrigin::signed(1), 2)); assert_eq!(Balances::reserved_balance(&1), 10); @@ -1341,3 +1335,24 @@ fn asset_create_and_destroy_is_reverted_if_callback_fails() { ); }); } + +#[test] +fn poc_double_spending() { + new_test_ext().execute_with(|| { + // Only run PoC when the system pallet is enabled, since the underlying bug is in the + // system pallet it won't work with BalancesAccountStore + // Start with a balance of 100 + Balances::force_set_balance(RuntimeOrigin::root(), 1, 100).unwrap(); + // Emulate a sufficient, in reality this could be reached by transferring a sufficient + // asset to the account + assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); + // Spend the same balance multiple times + assert_ok!(Balances::transfer_all(RuntimeOrigin::signed(1), 1337, false)); + assert_ok!(Balances::transfer_all(RuntimeOrigin::signed(1), 1337, false)); + assert_ok!(Balances::transfer_all(RuntimeOrigin::signed(1), 1337, false)); + + assert_eq!(Balances::free_balance(&1), 0); + assert_eq!(Balances::free_balance(&1337), 100); + }); +} diff --git a/frame/balances/src/tests/fungible_tests.rs b/frame/balances/src/tests/fungible_tests.rs index 128086885391f..4d0d61b4ff9c2 100644 --- a/frame/balances/src/tests/fungible_tests.rs +++ b/frame/balances/src/tests/fungible_tests.rs @@ -397,3 +397,28 @@ fn unholding_frees_hold_slot() { assert_ok!(Balances::hold(&TestId::Baz, &1, 10)); }); } + +#[test] +fn poc_double_spending() { + ExtBuilder::default() + .existential_deposit(1) + .monied(true) + .build_and_execute_with(|| { + // Only run PoC when the system pallet is enabled, since the underlying bug is in the + // system pallet it won't work with BalancesAccountStore + if UseSystem::get() { + // Start with a balance of 100 + >::set_balance(&1, 100); + // Emulate a sufficient, in reality this could be reached by transferring a + // sufficient asset to the account + System::inc_sufficients(&1); + // Spend the same balance multiple times + assert_ok!(>::transfer(&1, &1337, 100, Expendable)); + assert_eq!(Balances::free_balance(&1), 0); + assert_noop!( + >::transfer(&1, &1337, 100, Expendable), + TokenError::FundsUnavailable + ); + } + }); +} diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index f7271a828105a..1d24cf5ba33cf 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -1690,8 +1690,10 @@ impl StoredMap for Pallet { let is_default = account.data == T::AccountData::default(); let mut some_data = if is_default { None } else { Some(account.data) }; let result = f(&mut some_data)?; - if Self::providers(k) > 0 { + if Self::providers(k) > 0 || Self::sufficients(k) > 0 { Account::::mutate(k, |a| a.data = some_data.unwrap_or_default()); + } else { + Account::::remove(k) } Ok(result) } From af41a3d29fa7bc38b37c6ab561f73ef8bd732a0b Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Wed, 19 Apr 2023 18:58:38 +0100 Subject: [PATCH 7/9] Update tests.rs --- frame/assets/src/tests.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index 64d54ec446d30..b47fb6486cb27 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -1337,7 +1337,7 @@ fn asset_create_and_destroy_is_reverted_if_callback_fails() { } #[test] -fn poc_double_spending() { +fn multiple_transfer_alls_work_ok() { new_test_ext().execute_with(|| { // Only run PoC when the system pallet is enabled, since the underlying bug is in the // system pallet it won't work with BalancesAccountStore @@ -1350,7 +1350,6 @@ fn poc_double_spending() { // Spend the same balance multiple times assert_ok!(Balances::transfer_all(RuntimeOrigin::signed(1), 1337, false)); assert_ok!(Balances::transfer_all(RuntimeOrigin::signed(1), 1337, false)); - assert_ok!(Balances::transfer_all(RuntimeOrigin::signed(1), 1337, false)); assert_eq!(Balances::free_balance(&1), 0); assert_eq!(Balances::free_balance(&1337), 100); From 07119c0cedbdf50fb823d291561f879c87796776 Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Wed, 19 Apr 2023 18:59:29 +0100 Subject: [PATCH 8/9] Update fungible_tests.rs --- frame/balances/src/tests/fungible_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/balances/src/tests/fungible_tests.rs b/frame/balances/src/tests/fungible_tests.rs index 4d0d61b4ff9c2..57d6f0b6fab74 100644 --- a/frame/balances/src/tests/fungible_tests.rs +++ b/frame/balances/src/tests/fungible_tests.rs @@ -399,7 +399,7 @@ fn unholding_frees_hold_slot() { } #[test] -fn poc_double_spending() { +fn sufficients_work_properly_with_reference_counting() { ExtBuilder::default() .existential_deposit(1) .monied(true) From 5916c41fcca99ffea638ec824dd20533eb262d65 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Thu, 20 Apr 2023 19:16:38 +0800 Subject: [PATCH 9/9] Use `can_accrue_consumers` in the body of `can_inc_consumer` --- frame/system/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 1d24cf5ba33cf..6bbebb870594c 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -1235,8 +1235,7 @@ impl Pallet { /// True if the account has at least one provider reference and fewer consumer references than /// the maximum. pub fn can_inc_consumer(who: &T::AccountId) -> bool { - let a = Account::::get(who); - a.providers > 0 && a.consumers < T::MaxConsumers::max_consumers() + Self::can_accrue_consumers(who, 1) } /// Deposits an event into this block's event record.