Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
pavlenex opened this issue Feb 28, 2024 · 8 comments
Open

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

pavlenex opened this issue Feb 28, 2024 · 8 comments
Labels
Discussion Wallet Related to internal BTCPay wallet

Comments

@pavlenex
Copy link
Contributor

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.

@pavlenex pavlenex added Discussion Wallet Related to internal BTCPay wallet labels Feb 28, 2024
@ndeet
Copy link
Contributor

ndeet commented Feb 28, 2024

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
Copy link
Contributor Author

pavlenex commented Feb 28, 2024

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
Copy link
Member

Kukks commented Feb 28, 2024

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
Copy link
Contributor Author

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 #4304 (comment))

@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?) #4304 (comment) 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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

pavlenex commented Mar 1, 2024

@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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Wallet Related to internal BTCPay wallet
Projects
None yet
Development

No branches or pull requests

5 participants