fix: only destroy funds on refund if collateral is BelowMinimum

[rflechtner]

fixes https://github.com/KILTprotocol/ticket/issues/3797

Adopts the recommended resolution and checks for the reason of the can_deposit check failing. Funds (in bonded coins) are only destroyed if there is a BelowMinimum error, which indicates that the account to which the deposit should be send cannot be created because the amount is too low. This scenario can be tolerated, as this amount will likely have negligible financial value.

Other error cases will not lead to the destruction of funds, which means that in theory a refunding pool may not be able to fully refund because some account does not meet criteria for receiving funds. This can include:

• The maximum amount of Consumer references is exhausted
• The Provider references counter would overflow
• The collateral currency is frozen

Checklist:

• I have verified that the code works
  • No panics! (checked arithmetic ops, no indexing array[3] use get(3), ...)
• I have verified that the code is easy to understand
  • If not, I have left a well-balanced amount of inline comments
• I have left the code in a better state
• I have documented the changes (where applicable)
  • Either PR or Ticket to update the Docs
  • Link the PR/Ticket here

[abdulmth]

in what case into_result().is_err() would be true? 🤔

[rflechtner]

The DepositConsequence has multiple error cases and one success case. In all cases but the success case this would have been false, only the success case yields true. In other words, the transfer below can now still fail (because there's error cases we are not covering in this branch), leading to the extrinsic failing and being rolled back. As I explained in the PR description though, that would be intentional.

[rflechtner]

@abdulmth We have a test that checks the minimum balance case above this one, and I added a test here that ensures no balance is burnt when the beneficiary account cannot receive the funds due to any other reason (here: being blocked). This test fails if you revert the changes I made to the refund_account extrinsic.

[ntn-x2]

The PR addresses the problem you mention, although I am not sure why we would treat only one case as a special case for burning funds? I believe this might result in an unpleasant UX. It would see the error being ignored perhaps in a force_ extrinsic, also because this would mean that a user would pay for tx fees to end up with nothing, so if they know beforehand, they have no incentive to call this extrinsic in the first place.
Other than this, if we're still going down this route, I think one more test case which covers the only case in which the error should be ignored (i.e., BelowMinimum) should be added. Ideally, we would have one test case for each of the error variants, to make sure that we catch changes in case we modify those bits of code.

[rflechtner]

The PR addresses the problem you mention, although I am not sure why we would treat only one case as a special case for burning funds? I believe this might result in an unpleasant UX.

The early return was meant ensure that refunding can always be completed. Knowing now that this could lead to loss of significant amounts of funds we need to change it, meaning that in some scenarios the refund process can be stuck until governance steps in.

We can accept this for all cases, as you said, treating all cases the same way. I do see it as a likely scenario though that refund amounts for some users are below minimum balance, and it's clear in these cases that the financial loss would be minimal, so IMO it's okay to make an exception here. Also I don't really see how governance would fix this (besides also just burning the funds).

So as far as UX goes, I think it's better to prioritise avoiding refunds getting stuck all the time over minuscule losses of funds.

It would see the error being ignored perhaps in a force_ extrinsic, also because this would mean that a user would pay for tx fees to end up with nothing, so if they know beforehand, they have no incentive to call this extrinsic in the first place.

A force extrinsic would mean governance has to step in again, and in this case they could just burn funds directly via the assets pallet I think. The refund_account extrinsic is permissionless though, so anyone can call it for any account (e.g., the pool owner who wants to tear it down).

Other than this, if we're still going down this route, I think one more test case which covers the only case in which the error should be ignored (i.e., BelowMinimum) should be added. Ideally, we would have one test case for each of the error variants, to make sure that we catch changes in case we modify those bits of code.

There's already a test covering the BelowMinimum case, as mentioned in #866 (comment). Conditions for two other cases are a quite absurd for our case (UnknownAsset -> asset doesn't exist; Overflow -> the balance type or the sufficients counter on the account overflows), but I could add a test for the remaining case (CannotCreate -> too many consumers or no sufficients)

[ntn-x2]

Ok, your points make sense overall, although the amounts that could be burned would be directly related to the ED, so in the case of high ED (for any reason), the amount that could be burned would also be the same. But this is ok and not something we can fix anyway, as you say. Also if the extrinsic is permissionless then that solves the issues of users not willing to call the extrinsic to "do a favor" to the pool owner, so the pool owner can do so themselves.
Regarding tests, yes one test for any other case that makes sense would be good to have IMO.

[rflechtner]

Ok, your points make sense overall, although the amounts that could be burned would be directly related to the ED, so in the case of high ED (for any reason), the amount that could be burned would also be the same. But this is ok and not something we can fix anyway, as you say. Also if the extrinsic is permissionless then that solves the issues of users not willing to call the extrinsic to "do a favor" to the pool owner, so the pool owner can do so themselves.

Perfect, I'll continue as-is then!

Regarding tests, yes one test for any other case that makes sense would be good to have IMO.

I'm on it, just a heads-up that these cases only happen when the collateral is non-sufficient, which means I'm going to have to work around the assumption that exists at least for the use of the pallet in our chain that all collaterals are sufficient.

[ntn-x2]

I'm on it, just a heads-up that these cases only happen when the collateral is non-sufficient, which means I'm going to have to work around the assumption that exists at least for the use of the pallet in our chain that all collaterals are sufficient.

If the assumption is not baked into the pallet, then it is something we need to test, as it is runtime-dependent. If it is, then we can assume it won't happen hence we don't need to test.

[rflechtner]

If the assumption is not baked into the pallet, then it is something we need to test, as it is runtime-dependent. If it is, then we can assume it won't happen hence we don't need to test.

Turns out this is quite tricky to test. The only cases I could identify where this would happen are:

1. The collateral is non-sufficient but the bonded asset is sufficient. The pallet creates all bonded assets as non-sufficient, so this is a contrived, strange tests case (unless we change this behaviour, see https://github.com/KILTprotocol/ticket/issues/3798).
2. The collateral is non-sufficient and there is a hold on the account for the (also non-sufficient) bonded token, which means the refund is only partial (the free balance is refunded). But Holds/Freezes are implemented in the assets pallet only via a hook, which we don't use in the mocks, so not sure if and how I should implement a hold just for this test.

Should we skip this test for now and make a note to add this later? We're a bit pressed for time with the deadline on addressing the audit issues.

[ntn-x2]

You can skip it for now if you are sure that no weird stuff happens in any combination of cases you mentioned above, as they are all theoretically possible if the pallet is deployed on other runtimes. If you are unsure, and still want to skip the tests, just add a comment where relevant so in case anyone else decides to deploy the pallet, they must ensure the invariants you explain are upheld. What do you think?

[rflechtner]

You can skip it for now if you are sure that no weird stuff happens in any combination of cases you mentioned above, as they are all theoretically possible if the pallet is deployed on other runtimes. If you are unsure, and still want to skip the tests, just add a comment where relevant so in case anyone else decides to deploy the pallet, they must ensure the invariants you explain are upheld. What do you think?

Ok I managed to make sure the case where the MaxConsumers limit leads to failure to transfer funds is covered: 87cb2e8

This is the only case that is theoretically possible, unless we decide to change the current behaviour where bonded currencies are created as non-sufficient assets. From my POV this should be enough for this PR.

[ntn-x2]

LGTM!