Manage Proxy Interface's Use of Batch Call can lead to loss of Proxy Account because of utility.batch

[lucasvo]

utility.batch is not atomic and therefore unsafe to use when updating proxy accounts.

Here's how I just lost access to an anonymous proxy I created:

1. Create a proxy with Alice as Any
2. Go to Manage Proxies on the new Proxy
3. Do several changes:
   • Remove Alice with Any
   • Add Alice as NonTransfer
   • Add Bob as Any

This results in the following batch call to be submitted which removes the Alice and leaves you without any owner on the Proxy: https://portal.chain.centrifuge.io/#/explorer/query/0x9188ead75407372490b7d902a0ff7fbd23f82d29c1f4a3a22b70531df6cd73c7

[lucasvo]

This bug is rather severe as there is no place in the UI that it even shows you that this dialog uses a utility.batch underneath which could fail halfway through leaving the proxy in a bad state. I would propose to modify the Manage Proxy screen to only allow to do one action at a time this would make it clear to the user that they would be removing themselves before adding another user.

One could also do all addProxy operations before any removeProxy operations but this seems to come with other risks.

[lucasvo]

Alternatively, is there an atomic version of utility.batch in substrate that could be used instead?

[jacogr]

There is an atomic batchAll and the apps UI actually does use it when available in proxy management as well as other areas such as nominator/validator setup, specifically to guard against BatchInterrupted in critical operations.

See the following for the detection/use of batchAll/batch for proxies - https://github.com/polkadot-js/apps/blob/master/packages/page-accounts/src/modals/ProxyOverview.tsx#L50

In this case, looking at the portal you posted, it seem that Centrifuge doesn't have the batchAll functionality yet (it is not available under extrinsics -> utility.*), compared to e.g. Kusama/Polkadot where this is available alongside the non-atomic batch.

(It was merged into Substrate on 27 Oct, see paritytech/substrate#7188, which is post 2.0 and has not been made available as a Substrate minor yet, so I'm assuming that is why there is the on-chain functionality gap)

[lucasvo]

Thanks for the explanation, @jacogr Does the UI default back to using batch in any other places where this could lead to unexpected results? We will try to add these changes to our runtime soon but I am a bit concerned that this could backfire anywhere else.

[lucasvo]

I would propose to remove any support for batch in the UI that is unsafe.

[jacogr]

The current planning is to remove batch/batchAll detection 2 weeks after the next Substrate minor bump (alongside some other type detection), just not making functionality available at all when the batchAll is not available. (Obviously this means a minimum of 2 weeks from point A to point B, since the UI has a fixed release schedule every Monday)

This is basically what happens now, some functionality is hidden when e.g. batch is not available, so the aim is to swap this "disable functionality" to the batchAll version instead where it is used.

All the extrinsic type detection as well as "number of arguments" for older/newer/newest adds maintenance and complexity overhead for instance we still cater for even -rc4. In 2021 the Substrate release cadence will adjust, with that in mind there will be a clear "how many version back" of Substrate the UI will support (once the regular releases are in-place) - it evolves constantly.

[jacogr]

Closing this on the back of #4242 which will show a warning on all non-updated chains.

Both this warning or removal will both yield an immediate uptick in support and questions around where it is used, however since the specific issue is non-existent on chains following latest, hopefully this also have some benefit to drive a move closer to current.

[polkadot-js-bot]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.