btcpayserver / btcpayserver

Accept Bitcoin payments. Free, open-source & self-hosted, Bitcoin payment processor.
https://btcpayserver.org/
MIT License
6.3k stars 1.64k forks source link

Allow non-admins to configure watch-only wallet policy? #5798

Open pavlenex opened 6 months ago

pavlenex commented 6 months ago

Currently, we have a Server Setting > Policy that allows Server owner to enable or disable the ability for users to create hot wallets. The reason this is set is to prevent any custodian risks for third-party host.

We introduced a watch-only wallet where the wallet is created, and the key is wiped after the user agrees it's backed up.

I am really worried on how this has been playing out in practice, considering a lot of people don't back up the wallets. The reason why this is dangerous is that users not backing up a wallet could receive payments over a long period of time, and would figure out they cannot spend them only when it's too late. As a server owner, personally I would like to outsource any wallet creation externally, this at least ensures an outsourced risk for server owner.

We've been discussing about this for a while, but never decided how to handle it, and I think the simplest solution is to just disable hot wallet and watch only wallet by default for anyone but admins, unless they're enabled in Server Policy.

Perhaps there are better way to handle this, so I am opening it up for a discussion, but imo it's an issue that should be addressed sooner rather than later.

ndeet commented 6 months ago

I agree the case of "Create a new wallet" -> "watch-only wallet", we should have a way to disable that functionality. I never used that feature tbh as I either use a hot-wallet or the "Connect existing wallet" options where you put an xpub or wallet export (for which users should fairly sure have private keys).

On the other hand they are promted to write down the seedwords like with a hot wallet so they should be aware of it in theory? Is this an issue reported by users?

pavlenex commented 6 months ago

No, no user reported this issue, but as I noted above, some users are registering on mainnet demo nonchalantly , believing BTCPay is a wallet they need to use to pay a merchant to, so somehow they find our demo instance, create an account, store they create a wallet (since watch-only is only option), top it up and try to pay a merchant and they can't. Yes it's funny but it can be problematic for other user-groups as well.

The reason why introduced hot wallets is to ensure we have balance between security and convenience for third-party hosts, but spending from such wallets in general is pain so unsure how beneficial it is.

I am not against killing it and directing users towards external wallets vs hot wallets at all as long as it doesn't break things and there's a migration path for existing users.

Kukks commented 6 months ago

I am against killing it, and would instead opt to add various means to ensure they actually backed up the wallet. a seed word confirmation, a wallet file download, etc

pavlenex commented 6 months ago

We don't have to kill it, but there's no reason why we can't: A) Ensure there's a server policy to disable it (I'd disable it by default tbh) and force external wallets usage B) Check mnemonic (the reason why we didn't do this is because @NicolasDorier was against it https://github.com/btcpayserver/btcpayserver/issues/4304#issuecomment-1480648538)

@NicolasDorier do you still think that checking mnemonic to ensure user has backed up a wallet (especially in a case of a hot wallet is not a good improvement?) https://github.com/btcpayserver/btcpayserver/issues/4304#issuecomment-1480648538 I still stand by what I said back then, it's a common practice, it's done once and ensures a backup has been made.

dennisreimann commented 6 months ago

Imho it'd be a good option to not generate the watch-only wallet inside BTCPay Server and give users only the option to bring an existing wallet via xpub/output descriptor for that purpose. We'd eliminate the risk mentioned and ensure (at least for most of the cases) that this feature is used as intended.

NicolasDorier commented 6 months ago

I am against forcing backing up mnemonic, but I think it is a good idea to not allow cold wallets to be created by non-admins.

pavlenex commented 6 months ago

@NicolasDorier How about we do mnemonic check only for watch-only wallet (not the hot wallet), and add a server policy to enable/disable watch-only?

This way we don't cripple the hot wallet creation but ensure that proper backup is done for watch-only since you only get one chance to back it up, then we wipe the seed, I really think it's important that we communicate the dangers of this even if it means through friction, it's better to have friction than think you can back up later and keep receiving funds only to realize you can't spent them.

dennisreimann commented 6 months ago

How about we do mnemonic check only for watch-only wallet (not the hot wallet), and add a server policy to enable/disable watch-only?

Tbh I see two problems with this:

A) The user has to enter/verify the seed on a device other than the hardware wallet. We strongly discourage importing wallets that way, I think we should refrain from doing it as a means of confirming a watch-only wallet.

B) It'd be inconsistent with the create/generate flow, where it would make even more sense to have it imho.

igorsilvacripto commented 3 weeks ago

Y'all! I see that I can't create a hot wallet. I'm the owner of the store but it says I'm not an administrator. I have the vault open and running. Store owner. It's also connected to WooCommerce. And I can't run the wallet rescan either because i says I'm not a server admin.

What should I do about that?