XRPL-Labs / Xaman-Issue-Tracker

Bugs, improvements, suggestions & release progress (Project boards)
https://xumm.app
14 stars 9 forks source link

[FEATURE] Only allow MasterKey disable account flag set after a verified valid signature #480

Open KoenPaas opened 9 months ago

KoenPaas commented 9 months ago

During testing Multi Sign xApp there was a case of someone who was able to delete the MasterKey with an invalid multi sign list using a third party service.

He is now unable to sign any transactions effectively black holing his account.

To prevent this from happening, I propose the following:

Only allow to set the master key disable after the user has ever created a valid on ledger signature for that account,

This can be done on numerous ways:

  1. Do a 'no op' Account Set transaction to prove a valid signature with a success result.
  2. Check on ledger if a valid signature has been provided in the past.
  3. Check our own backend if a valid signature has been provided.

It will prevents users deleting a MasterKey with an invalid Regular Key or Multi Sign List.

WietseWind commented 9 months ago

Just to make sure I get this right:

  1. "delete the MasterKey"
  2. "invalid multi sign list"

And then:

  1. "master key disable after"
  2. "valid on ledger signature for that account"

For this, we'd want to check the Xaman backend for signatures. This way:

  1. We can rely on SignIn signatures, allowing for signatures not to be part of on ledger history, to keep it quantum safe
  2. We don't need our backend to potentially scrape 32 (max multi signers) accounts

TL;DR:

  1. I understand this for Regular Key and this is easy to implement
  2. I see challenges & significant downsides doing this for MultiSign

@KoenPaas @WillXUMM comments appreciated.

KoenPaas commented 9 months ago

The primary objective is to safeguard the MasterKey from accidental deletion by average users without appropriate checks. Currently, our method involves displaying a popup, which, unfortunately, can be easily dismissed without a full grasp of the potential consequences. To bolster security measures, the proposed solution suggests implementing a test as the default method, ensuring that the MasterKey cannot be disabled inadvertently. This test should be the primary safeguard, replacing the current reliance on a potentially dismissible popup.

Despite signatures being valid, the ledger might still reject a transaction. The xrpl.org documentation advises verifying this on the ledger using a no-op transaction. For instance, if a signer has the master key disabled, a tefMASTER_DISABLED error will be thrown, demonstrating a scenario where a combination of valid signature is not accepted by the ledger.

On the MultiSign backend, my approach for checking the current SignerList or any changes involves examining the AccountSequence. This number signifies the last sequence at which the account was altered. Subsequently, we can verify whether, after that date, a valid transaction has been successfully submitted to the ledger, utilizing either MultiSign or a regular key.

For users expressing concerns about Quantum Safety, we can offer an external tool outside of Xaman specifically designed for deleting the master key. This ensures that while users engaged in regular signing activities or those with MultiSign functionality are provided with flexibility, the average user is protected from inadvertently configuring invalid account settings.

WietseWind commented 9 months ago

With deletion of the master key you refer to disabling the master?

And the verification you propose is to only allow master deletion after at least one multi sign sequence has succesfully been performed?

KoenPaas commented 9 months ago

Yes indeed disable the master after one multi sign sequence. Then we know they will not lose access. Same could go for regular key.