Check existential deposit when creating an account during swap

[MOZGIII]

We have discovered an issue with account creation in both pallet_currency_swap and precompile_currency_swap in that they can both lead to the loss of funds when the amount swapped is smaller that the To currency existential deposit.

Currently, the account gets dusted before it is created.
We should change is the following ways:

1. Use try_mutate_account if possible (maybe we can't though).
2. Handle the case where the value swapped is less than the existential deposit of the currency, and fail the swap operation. In substrate this is done usually at the pallet call level, so we should do the same: add the check to the pallet_currency_swap and precompile_currency_swap calls.

[MOZGIII]

Upd: we should not use try_mutate_account as it is supposed to be private by design

[dmitrylavrenov]

Spent some time to investigate possible root causes. Looks like the behaviour is clear now.

We have key data objects:

• source - the initiator address of swap operation.
• pot_from - the account key for the currency to swap from.
• destination - the target address of swap operation.
• pot_to - the account key for the currency to swap from.
• balance- balances amount to be swapped.
• FromCurrency - from balances space.
• ToCurrency - to balances space.

Current flow consists of the following:

1. Update source and obtain the withdrawed_imbalance with

withdrawed_imbalance  = FromCurrency::withdraw(&source, balance...

2. Update pot_to and obtain the outgoing_imbalance with

outgoing_imbalance  = ToCurrency::withdraw(&pot_to, T::BalanceConverter::convert(balance)...

3. Update pot_from with FromCurrency::resolve_into_existing(&pot_from, withdrawed_imbalance).
4. Update destination with ToCurrency::resolve_creating(&to, deposited_imbalance).

We have an error in case swapped balance is less than ED of ToCurrency space and the destination account doesn't exist.

Let's check resolve_creating code:

/// Similar to deposit_creating, only accepts a `NegativeImbalance` and returns nothing on
/// success.
    fn resolve_creating(who: &AccountId, value: Self::NegativeImbalance) {
	let v = value.peek();
	drop(value.offset(Self::deposit_creating(who, v)));
    }

So, deposit_creating is called. resolve_creating doesn't return any error.
Let's check deposit_creating code:

fn deposit_creating(who: &T::AccountId, value: Self::Balance) -> Self::PositiveImbalance {
	if value.is_zero() {
		return Self::PositiveImbalance::zero()
	}

	Self::try_mutate_account(
		who,
		|account, is_new| -> Result<Self::PositiveImbalance, DispatchError> {
			let ed = T::ExistentialDeposit::get();
			ensure!(value >= ed || !is_new, Error::<T, I>::ExistentialDeposit);

			// defensive only: overflow should never happen, however in case it does, then this
			// operation is a no-op.
			account.free = match account.free.checked_add(&value) {
				Some(x) => x,
				None => return Ok(Self::PositiveImbalance::zero()),
			};

			Self::deposit_event(Event::Deposit { who: who.clone(), amount: value });
			Ok(PositiveImbalance::new(value))
		},
	)
	.unwrap_or_else(|_| Self::PositiveImbalance::zero())
}

As we can see, in case value balance is less than ED than try_mutate_account returns an Error::<T, I>::ExistentialDeposit. But next we have .unwrap_or_else(|_| Self::PositiveImbalance::zero()). As a result, resolve_creating just returns PositiveImbalance::zero() in case ExistentialDeposit error. But the call returns success.

I thinks in this case #694 makes sense as we need to estimate swapped balance before doing any tokens operations and make sure that the resulted amount is ge than ED.

By the way, deposit_creating method could return result to avoid confusions and mistakes like that one.

[MOZGIII]

Well done outlining the situation!

I think the better option would be to define our own trait on top of Currency that does this resolve_creating right. The API should just expose the error and be more explicit about it.
It is essential that we do not drop the non-zero imbalance, and the current resolve_creating with do just that. What a dull thing to make.

We should investigate whether the new fungible traits help here, and possible switch to them. However, if they don't improve the situation - we can add our own.

---

This will the correct long-term solution.

In the short term we could just add those extra checks temporarily and then we could remove them once we switch to the better low level building block - however this just seems like extra work when we can already add custom trait ext and solve it there.

Overall, the currently proposed silution is way more of a hack than fixing the currency API to expose a better resolve_creating version.

[MOZGIII]

Yep, checked the fungible, and it has this part done right: https://github.com/paritytech/substrate/blob/63246b699d7e2645c8b12aae46f8f0765c682183/frame/support/src/traits/tokens/fungible/regular.rs#L465
Although it has its own serious issues...

So, maybe just switch to the new fungible traits?