Wallet: Access to the account can be lost after enabling 2FA

[Kouprin]

near/near-wallet#2387

[MaximusHaximus]

I'm not sure I understand the full implications of this change; can you think of any reasons why the existing contract doesn't already have ignore_state set on the init and explicitly fails if init is called when there is existing state?

[zavodil]

I can confirm this bug, it already killed one account.

Another way to fix it in the wallet:

• prevent user to deploy 2FA if contract state exists
• add a feature to clear the contract state using wallet (there is a contract by @ilblackdragon)
  It will protect developer from overwriting his contract state by adding 2FA as described in support channel.

[Kouprin]

I can confirm this bug, it already killed one 1 account.

Another way to fix it in the wallet:

• prevent user to deploy 2FA if contract state exists
• add a feature to clear the contract state using wallet
  This may protect developer from overwriting his contract state by adding 2FA as described in support channel.

Also the wallet may check if init of 2FA was successful to continue deleting keys. This solution is absolutely must if the contract will not be updated, but even with contract update it would be good to have to keep users protected.

[Kouprin]

I'm not sure I understand the full implications of this change; can you think of any reasons why the existing contract doesn't already have ignore_state set on the init and explicitly fails if init is called when there is existing state?

I've asked @evgenykuzyakov and he confirmed that ignore_state directive solves the wallet issue.

From my perspective it may be risky if previous contract affected the state somehow, filling persistent UnorderedMaps with arbitrary data. However, this problem is related to any contract at NEAR, not multisig only. I'd prefer to have a directive clear_state that clears the state forcibly to resolve all issues based on possibility of reusing state.

Also @ilblackdragon may have some thoughts as he's code author.

[MaximusHaximus]

@austinabell Do you have any insights into this change as far as unintended consequences go?

[austinabell]

@austinabell Do you have any insights into this change as far as unintended consequences go?

My only concern is that since the state is not validated in any way, if some external tool relies on the multisig, the owner could just overwrite the current state to require only one signature for example and then sign and pass anything. Perhaps the solution involves trying to deserialize into the multisig state and if it is a valid multisig state you don't allow overwriting it? This doesn't solve the issue, but minimizes the chances of this happening by accident?

Let me know if you think this is a valid concern or if I am missing something about how the owner could just overwrite the current state/requirements based on how this is actually used.

[Kouprin]

@austinabell Do you have any insights into this change as far as unintended consequences go?

My only concern is that since the state is not validated in any way, if some external tool relies on the multisig, the owner could just overwrite the current state to require only one signature for example and then sign and pass anything. Perhaps the solution involves trying to deserialize into the multisig state and if it is a valid multisig state you don't allow overwriting it? This doesn't solve the issue, but minimizes the chances of this happening by accident?

Let me know if you think this is a valid concern or if I am missing something about how the owner could just overwrite the current state/requirements based on how this is actually used.

Looks valid to me. As I said above, the state may be filled with arbitrary data that may be interpreted by contract as some instructions (exploits).

However, there is not an issue of the contract itself. Both versions of the contract - with ignore_state or without - are correct. The root of the issue is how the contract is used.

Regarding wallet, the contract version without ignore_state kills accounts because of how procedure of 2FA enabling is implemented. Currently, it doesn't check success of initialization. However, if we will simply add this check to the wallet, the following problem appear. It would be impossible to enable 2FA after any contract deployed before, even in case of re-enabling 2FA.

Regarding exploits, it's still possible for attacker to write custom version of multisig and then imitate 2FA enabling with malicious contract. So @austinabell your approach is possible today, even while official contract don't have ignore_state. As long as we don't have a contract hashes library that we may trust, the attacker can imitate any contract behavior.

Overall, it looks like political decision. Impossibility to deploy the contract after the state is initialized makes the contract less useful, but a little bit more "safe". The main question is how wallet should process this, and it shouldn't kill accounts in any way. :)