near-wallet icon indicating copy to clipboard operation
near-wallet copied to clipboard

Unable to remove 2FA from near wallet

Open hcho112 opened this issue 2 years ago • 2 comments

Problem

Unable to remove 2FA from my account hcho112.near and getting this: image

https://explorer.mainnet.near.org/transactions/FC5QGmsiR8SY2dGdpsAZPqGG1pwYKbVeShXUKEm9Z6nv

Expected Behavior Should be able to remove 2FA

Steps to reproduce

  1. authenticate on near wallet
  2. click Disable button on Two-Factor-Authentication
  3. insert account id and click Disable 2FA button

hcho112 avatar Aug 03 '22 23:08 hcho112

Thanks for reporting @hcho112 . So the reason you're facing this is because we need the current contract state keys to cleanup your account state when you disable 2fa by deploying and calling clean on the cleanup contract with the state keys. We need to do this so that you can deploy other code to your account safely without bricking your account after disabling 2fa.

There's a 50kb limit on view_state calls on non archival rpc nodes. This is why we attempt to delete all your requests before calling view_state so that your state falls under 50kb when the requests are deleted. The issue is that the multisig contract has a hardcoded 15 minute cooldown period after requests are created for them to be deletable. So what's happening is that we're unable to delete the requests and your state is still > 50kb so the view_state call fails so we can't get the state keys to call clean with and thus can't cleanup your state when you disable so we throw. In practice this means that in the worst case you need to disable 2fa 15 minutes after your last transactions if they added requests in the state amounting to > 50kb.

Some ideas to improve this would be if the cleanup contract did not need to know the keys when cleaning up, eliminating our dependency on view_state (not sure if this is possible) or if the multisig contract did not have the 15 minute cooldown period. Both improvements would require changes to the core contracts the wallet uses and the latter would not affect folks that already have multisig deployed.

We could pursue some improvement along above lines but the multisig functionality is being completely eliminated from the wallet anyway as part of the migration so I think what we have now is fine until then and is a lot better than what we had before since its a 15 minute inconvenience instead of bricking users' accounts.

esaminu avatar Aug 04 '22 17:08 esaminu

I think we should inform users what type of issue it is, and inform users that there is a need to wait 15 minutes. Added an issue https://github.com/near/near-wallet/issues/2885

marcinbodnar avatar Aug 16 '22 18:08 marcinbodnar

Thanks for reporting @hcho112 . So the reason you're facing this is because we need the current contract state keys to cleanup your account state when you disable 2fa by deploying and calling clean on the cleanup contract with the state keys. We need to do this so that you can deploy other code to your account safely without bricking your account after disabling 2fa.

There's a 50kb limit on view_state calls on non archival rpc nodes. This is why we attempt to delete all your requests before calling view_state so that your state falls under 50kb when the requests are deleted. The issue is that the multisig contract has a hardcoded 15 minute cooldown period after requests are created for them to be deletable. So what's happening is that we're unable to delete the requests and your state is still > 50kb so the view_state call fails so we can't get the state keys to call clean with and thus can't cleanup your state when you disable so we throw. In practice this means that in the worst case you need to disable 2fa 15 minutes after your last transactions if they added requests in the state amounting to > 50kb.

Some ideas to improve this would be if the cleanup contract did not need to know the keys when cleaning up, eliminating our dependency on view_state (not sure if this is possible) or if the multisig contract did not have the 15 minute cooldown period. Both improvements would require changes to the core contracts the wallet uses and the latter would not affect folks that already have multisig deployed.

We could pursue some improvement along above lines but the multisig functionality is being completely eliminated from the wallet anyway as part of the migration so I think what we have now is fine until then and is a lot better than what we had before since its a 15 minute inconvenience instead of bricking users' accounts.

Majority of times that we will get the State of contract ... is too large to be viewed error will be when a user tries to go through the Disable Multisig flow, but instead of verifying the 2FA code, cancels the popup.

As @esaminu mentioned, the error comes from the fact that the state on the account is over >50 Kb because we must deploy the contract state cleaner contract as part of the request to disable 2FA(this contract's base64 encoding is very large: see the DeployContract Action's code argument) and therefore this request must be deleted in order to once again carry out the Disable Multisig flow which requires a 15 minute cooldown.

Instead of carrying out the Disable Multisig flow as the app would normally carry it out, we can instead carefully look at the past 3-4 requests on the 2fa contract to see if a recent request is indeed a request to disable the multisig on the account. If we find such a request, instead of creating a new request to disable 2FA, we can have the near-contract-helper send a 2fa Code to validate that specific request and execute it therefore mitigating any sort of mandatory cooldowns.

This requires a few changes in the near-contract-helper repo as currently it does not except a specific request_id to validate against, but it is definitely possible. Just want to be sure this does not pose any security risks before I proceed with a pull request for it. What do you think @esaminu?

roshaans avatar Aug 29 '22 15:08 roshaans

We resolved this issue by showing a message to the user to wait 15 minutes before attempting another attempt, instead of trying to mitigate the issue on the client side as it would have been a bit of an involved process.

roshaans avatar Sep 21 '22 07:09 roshaans