paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

Anonymous proxy spawner should always be able to kill it, regardless of permissions. #9794

Open lamafab opened 3 years ago

lamafab commented 3 years ago

(@shawntabrizi , @bkchr told me to ping you)

Currently, a spawner of an anonymous proxy cannot kill the anonymous proxy unless it was created with Any permissions. If a spawner uses non-Any permissions, then this results in having your basic deposit locked forever, with no way of unlocking it.

Relevant code:

// ...
// Proxy call cannot remove all proxies or kill anonymous proxies unless it has full
// permissions.
Some(Call::remove_proxies { .. }) | Some(Call::kill_anonymous { .. })
    if def.proxy_type != T::ProxyType::default() =>
    false,
// ...

@rphmeier argued that the Runtime should not prevent you from doing mistakes and that it's mostly a UI issue. I do think it's reasonable to assume that when you create an anonymous proxy - and the Runtime allows you to specify a permission type - it should be able to be killed, regardless of the chosen option. The Runtime could handle it with one of those two options:

If the second options is to be implemented (or even if it's a UI issue), this will not allow currently locked-out users from unlocking the basic deposit. We have such support cases at the moment.

shawntabrizi commented 3 years ago

and what does killing an anonymous proxy entail? transferring its full balance to the owner?

This sounds like a UI issue to me.

Also your second option may not be preferable to some people. For example, I want an account which i can guarantee no one can transfer balances from. So i would make an anonymous proxy with non-transfer setting.

Forcing a user to use Any can only limit users. IMO we should not build APIs which limit options for everyone to appease a small audience of people who make mistakes.

Should we also somehow prevent users from sending their tokens to the wrong address?

xlc commented 3 years ago

Also your second option may not be preferable to some people. For example, I want an account which i can guarantee no one can transfer balances from. So i would make an anonymous proxy with non-transfer setting.

No you don't. The token will stuck there and no way out.

michalisFr commented 3 years ago

I don't think there's a UI issue here. The Polkadot-JS UI doesn't give you the option to create anonymous proxies like it does with normal proxies. You need to do it through the extrinsics page. The only UI involved there is that you issue the extrinsic using a GUI instead of doing it through a CLI or programmatically. And since the runtime doesn't prevent you from creating a proxy with a non-Any type, the extrinsics GUI doesn't either.

When the runtime allows for a situation that:

  1. Has no real use case
  2. Leads to an unsolvable problem

is it user mistake if they find themselves in that situation or an oversight on our part? IMHO it's the second.

Anonymous proxies are tricky and dangerous (and quite complex) but they don't have to be. If we can implement them in a way that can't lead to accidental lock of funds, without compromising on security, shouldn't we do it?

From the proposed solutions I agree with @lamafab that the first solution is preferred, but only because the second doesn't solve the issue for the currently locked funds.

My ideal solution would be a combination of that and what @shawntabrizi proposed (although I believe he was joking):

Reasoning: A spawner of non-Any type cannot add other proxies, so it will be the only proxy on the proxied account. So, it stands to reason that any funds in the proxied account will be the spawner's, for example, to pay fees for staking actions. This actually creates legitimate use cases for creating non-Any type proxies, without (knowingly or unknowingly) sacrificing any funds sent to the proxied account.

I'm excluding spawners of type Any from this because they can (and should) send the full balance to any account they want before killing the proxy.

If what I propose can be exploited in some way, please correct me.

lamafab commented 3 years ago

Forcing a user to use Any can only limit users. IMO we should not build APIs which limit options for everyone to appease a small audience of people who make mistakes.

I genuinely do not see any practical use case for this as long as you cannot withdraw any funds. All activities, especially governance and staking, will require (a lot of) funds; why would you ever create an anonymous proxy with non-Any permissions if this would result in having your tokens stuck there forever? Fine tuning permissions makes sense when you add new proxies to the anonymous account, not when creating it.

So, if someone wants to create a Staking-only anonymous proxy that's fine, but I also think it's reasonable to expect those funds to be returned.

  • The spawner can always kill the anonymous proxy, regardless of type
  • When the spawner type is not Any, killing the proxy also sends the full balance to the spawner.

There could be a safe option of kill_anonymous, similar to transfer_keep_alive. IMO kill_anonymous should just check whether the proxy type is Any or return an error (current behavior), resulting in the user having to call the safe version which also executes a refund.

What do you think?

hmd7ai commented 2 years ago

Hello, I was about to kill an anonymous proxy which i have created before and see the error "Txn version not supported". I should mention a created my polkadot acoount in polkadot.js via Ledger Nano x. my account linked to my hardaew wallet. I can not kill the proxy and my dot locked in there. can anyone help me? Here is the Hash: 15vzAirFFFWcEgF74jxLoCTQxh11caDYgBrdQpUAnSEaDkrz