From 2541efdbcc40b7cb13a3bb514028f7e89b54e17e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 25 Apr 2022 20:12:06 +0200 Subject: [PATCH] pallet-asset: Fix transfer of a large amount of an asset (#11241) * pallet-asset: Fix transfer of a large amount of an asset Before this pr transferring a large amount of an asset would check that transferring the asset would not overflow the supply of the asset. However, it doesn't make sense to check for asset supply overflow when we just transfer from one account to another account and don't increase the supply in any way. It also required to extend the `can_deposit` method of `fungible` and `fungibles` with a `mint` parameter. If this parameter is set to `true`, it means we want to mint the amount of an asset before transferring it into an account. For `can_withdraw` we don't need to add an extra parameter, because withdrawing should never be able to underflow the supply. If that would happen, it would mean that somewhere the supply wasn't increased while increasing the balance of an account. * Update frame/assets/src/functions.rs * Update frame/assets/src/functions.rs * Update frame/assets/src/functions.rs Co-authored-by: Shawn Tabrizi * FMT Co-authored-by: Shawn Tabrizi --- substrate/frame/assets/src/functions.rs | 14 +++++++++++--- substrate/frame/assets/src/impl_fungibles.rs | 3 ++- substrate/frame/assets/src/tests.rs | 10 ++++++++++ substrate/frame/balances/src/lib.rs | 9 +++++---- .../frame/support/src/traits/tokens/fungible.rs | 14 ++++++++++---- .../frame/support/src/traits/tokens/fungibles.rs | 10 +++++++++- 6 files changed, 47 insertions(+), 13 deletions(-) diff --git a/substrate/frame/assets/src/functions.rs b/substrate/frame/assets/src/functions.rs index 0be79619e0..a6abfd9e04 100644 --- a/substrate/frame/assets/src/functions.rs +++ b/substrate/frame/assets/src/functions.rs @@ -103,16 +103,24 @@ impl, I: 'static> Pallet { Remove } + /// Returns `true` when the balance of `account` can be increased by `amount`. + /// + /// - `id`: The id of the asset that should be increased. + /// - `who`: The account of which the balance should be increased. + /// - `amount`: The amount by which the balance should be increased. + /// - `increase_supply`: Will the supply of the asset be increased by `amount` at the same time + /// as crediting the `account`. pub(super) fn can_increase( id: T::AssetId, who: &T::AccountId, amount: T::Balance, + increase_supply: bool, ) -> DepositConsequence { let details = match Asset::::get(id) { Some(details) => details, None => return DepositConsequence::UnknownAsset, }; - if details.supply.checked_add(&amount).is_none() { + if increase_supply && details.supply.checked_add(&amount).is_none() { return DepositConsequence::Overflow } if let Some(balance) = Self::maybe_balance(id, who) { @@ -283,7 +291,7 @@ impl, I: 'static> Pallet { (true, Some(dust)) => (amount, Some(dust)), _ => (debit, None), }; - Self::can_increase(id, &dest, credit).into_result()?; + Self::can_increase(id, &dest, credit, false).into_result()?; Ok((credit, maybe_burn)) } @@ -379,7 +387,7 @@ impl, I: 'static> Pallet { return Ok(()) } - Self::can_increase(id, beneficiary, amount).into_result()?; + Self::can_increase(id, beneficiary, amount, true).into_result()?; Asset::::try_mutate(id, |maybe_details| -> DispatchResult { let details = maybe_details.as_mut().ok_or(Error::::Unknown)?; diff --git a/substrate/frame/assets/src/impl_fungibles.rs b/substrate/frame/assets/src/impl_fungibles.rs index 49caac83f4..6b263bc0c7 100644 --- a/substrate/frame/assets/src/impl_fungibles.rs +++ b/substrate/frame/assets/src/impl_fungibles.rs @@ -47,8 +47,9 @@ impl, I: 'static> fungibles::Inspect<::AccountId asset: Self::AssetId, who: &::AccountId, amount: Self::Balance, + mint: bool, ) -> DepositConsequence { - Pallet::::can_increase(asset, who, amount) + Pallet::::can_increase(asset, who, amount, mint) } fn can_withdraw( diff --git a/substrate/frame/assets/src/tests.rs b/substrate/frame/assets/src/tests.rs index db0d6a5f21..50ab04111e 100644 --- a/substrate/frame/assets/src/tests.rs +++ b/substrate/frame/assets/src/tests.rs @@ -967,3 +967,13 @@ fn querying_allowance_should_work() { assert_eq!(Assets::allowance(0, &1, &2), 0); }); } + +#[test] +fn transfer_large_asset() { + new_test_ext().execute_with(|| { + let amount = u64::pow(2, 63) + 2; + assert_ok!(Assets::force_create(Origin::root(), 0, 1, true, 1)); + assert_ok!(Assets::mint(Origin::signed(1), 0, 1, amount)); + assert_ok!(Assets::transfer(Origin::signed(1), 0, 2, amount - 1)); + }) +} diff --git a/substrate/frame/balances/src/lib.rs b/substrate/frame/balances/src/lib.rs index 6bf37dfda0..0eab933f77 100644 --- a/substrate/frame/balances/src/lib.rs +++ b/substrate/frame/balances/src/lib.rs @@ -810,12 +810,13 @@ impl, I: 'static> Pallet { _who: &T::AccountId, amount: T::Balance, account: &AccountData, + mint: bool, ) -> DepositConsequence { if amount.is_zero() { return DepositConsequence::Success } - if TotalIssuance::::get().checked_add(&amount).is_none() { + if mint && TotalIssuance::::get().checked_add(&amount).is_none() { return DepositConsequence::Overflow } @@ -1093,8 +1094,8 @@ impl, I: 'static> fungible::Inspect for Pallet liquid.saturating_sub(must_remain_to_exist) } } - fn can_deposit(who: &T::AccountId, amount: Self::Balance) -> DepositConsequence { - Self::deposit_consequence(who, amount, &Self::account(who)) + fn can_deposit(who: &T::AccountId, amount: Self::Balance, mint: bool) -> DepositConsequence { + Self::deposit_consequence(who, amount, &Self::account(who), mint) } fn can_withdraw( who: &T::AccountId, @@ -1110,7 +1111,7 @@ impl, I: 'static> fungible::Mutate for Pallet { return Ok(()) } Self::try_mutate_account(who, |account, _is_new| -> DispatchResult { - Self::deposit_consequence(who, amount, &account).into_result()?; + Self::deposit_consequence(who, amount, &account, true).into_result()?; account.free += amount; Ok(()) })?; diff --git a/substrate/frame/support/src/traits/tokens/fungible.rs b/substrate/frame/support/src/traits/tokens/fungible.rs index 712103a1e8..7422a9d651 100644 --- a/substrate/frame/support/src/traits/tokens/fungible.rs +++ b/substrate/frame/support/src/traits/tokens/fungible.rs @@ -50,7 +50,11 @@ pub trait Inspect { fn reducible_balance(who: &AccountId, keep_alive: bool) -> Self::Balance; /// Returns `true` if the balance of `who` may be increased by `amount`. - fn can_deposit(who: &AccountId, amount: Self::Balance) -> DepositConsequence; + /// + /// - `who`: The account of which the balance should be increased by `amount`. + /// - `amount`: How much should the balance be increased? + /// - `mint`: Will `amount` be minted to deposit it into `account`? + fn can_deposit(who: &AccountId, amount: Self::Balance, mint: bool) -> DepositConsequence; /// Returns `Failed` if the balance of `who` may not be decreased by `amount`, otherwise /// the consequence. @@ -86,7 +90,9 @@ pub trait Mutate: Inspect { amount: Self::Balance, ) -> Result { let extra = Self::can_withdraw(&source, amount).into_result()?; - Self::can_deposit(&dest, amount.saturating_add(extra)).into_result()?; + // As we first burn and then mint, we don't need to check if `mint` fits into the supply. + // If we can withdraw/burn it, we can also mint it again. + Self::can_deposit(&dest, amount.saturating_add(extra), false).into_result()?; let actual = Self::burn_from(source, amount)?; debug_assert!( actual == amount.saturating_add(extra), @@ -216,8 +222,8 @@ impl< fn reducible_balance(who: &AccountId, keep_alive: bool) -> Self::Balance { >::reducible_balance(A::get(), who, keep_alive) } - fn can_deposit(who: &AccountId, amount: Self::Balance) -> DepositConsequence { - >::can_deposit(A::get(), who, amount) + fn can_deposit(who: &AccountId, amount: Self::Balance, mint: bool) -> DepositConsequence { + >::can_deposit(A::get(), who, amount, mint) } fn can_withdraw(who: &AccountId, amount: Self::Balance) -> WithdrawConsequence { >::can_withdraw(A::get(), who, amount) diff --git a/substrate/frame/support/src/traits/tokens/fungibles.rs b/substrate/frame/support/src/traits/tokens/fungibles.rs index 8e68b36d60..2abadf0376 100644 --- a/substrate/frame/support/src/traits/tokens/fungibles.rs +++ b/substrate/frame/support/src/traits/tokens/fungibles.rs @@ -53,10 +53,16 @@ pub trait Inspect { fn reducible_balance(asset: Self::AssetId, who: &AccountId, keep_alive: bool) -> Self::Balance; /// Returns `true` if the `asset` balance of `who` may be increased by `amount`. + /// + /// - `asset`: The asset that should be deposited. + /// - `who`: The account of which the balance should be increased by `amount`. + /// - `amount`: How much should the balance be increased? + /// - `mint`: Will `amount` be minted to deposit it into `account`? fn can_deposit( asset: Self::AssetId, who: &AccountId, amount: Self::Balance, + mint: bool, ) -> DepositConsequence; /// Returns `Failed` if the `asset` balance of `who` may not be decreased by `amount`, otherwise @@ -137,7 +143,9 @@ pub trait Mutate: Inspect { amount: Self::Balance, ) -> Result { let extra = Self::can_withdraw(asset, &source, amount).into_result()?; - Self::can_deposit(asset, &dest, amount.saturating_add(extra)).into_result()?; + // As we first burn and then mint, we don't need to check if `mint` fits into the supply. + // If we can withdraw/burn it, we can also mint it again. + Self::can_deposit(asset, &dest, amount.saturating_add(extra), false).into_result()?; let actual = Self::burn_from(asset, source, amount)?; debug_assert!( actual == amount.saturating_add(extra),