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

Reuse of Monero receiving address #4500

Closed
rating89us opened this issue Sep 7, 2020 · 7 comments · Fixed by #6236
Closed

Reuse of Monero receiving address #4500

rating89us opened this issue Sep 7, 2020 · 7 comments · Fixed by #6236

Comments

@rating89us
Copy link
Contributor

rating89us commented Sep 7, 2020

In bisq, each Monero account supports a single Monero receiving address. Even though reusing a Monero address is not as dangerous as reusing a Bitcoin address (because Monero address is never stored in the public blockchain), this practice can still compromise user's privacy if there is off-chain linking.

Instead of asking for a single receiving Monero address, the Monero account in bisq could ask for the public spend key and the private (secret) view key of the Monero wallet. By having these two keys, bisq would be able to generate a new subaddress for each new transaction, avoiding address reuse and increasing user privacy. Take a look at this javascript solution in https://github.com/knaccc/subaddress-js.

@chimp1984
Copy link
Contributor

Good suggestion! Do you know a XMR dev with java skills for doing the implementation?

@rating89us
Copy link
Contributor Author

rating89us commented Sep 9, 2020

@knaccc
Copy link

knaccc commented Sep 9, 2020

Here is my pure-Java subaddress implementation:

https://github.com/knaccc/subaddress-java

@chimp1984
Copy link
Contributor

For anyone starting to work on that: Please keep in mind that we do not want to add new library dependencies if there is any way to avoid that. Otherwise it will require an audit for those libraries and their sub-dependencies, which easily ends up in a stalled project due lack of resources to do that.

@chimp1984
Copy link
Contributor

This feature becomes more important now in the context of the transaction key reuse vulnerability. By avoiding using the same address we reduce the vulnerability surface. See monero-project/research-lab#103 and https://www.getmonero.org/pl/2018/09/25/a-post-mortum-of-the-burning-bug.html for more details.

I started to work on that but it turned out that its quite some effort so I left it incomplete.

Here is an overview about the issues which makes it non-trivial to integrate it into Bisq (its account management and trade protocol related, not XMR subaddress related). This info should be helpful for any dev who picks that task up.

We create the AssetAccount and AssetAccountPayload by the PaymentMethod not by the currency. This makes it difficult to create a special account class for XMR to add the privateViewKeyHex field and manage the different behaviour. The AssetForm class should also be subclassed and add a checkbox if subaddresses should be used and if so show an additional input field.
To work around that we could add the UI elements to AssetForm in case XMR is selected. Not great to add custom features to the generic asset class but seems to be the least problematic solution.

We must not store the privateViewKeyHex in AssetAccountPayload as this data is exchanged with the peer in the protocol, that must contain public data only. An alternative to store the privateViewKeyHex could be the UserPayload class. I am not sure if keeping the mainAddress in the address field in the AssetAccountPayload carries some risk. I guess not.

We changed a while back the way how the AccountPayload is passed to the peer in the trade protocol. The Contract has now null values for the AccountPayloads and the peers are exchanging that later in the trade proccess by a dedicated message (ShareBuyerPaymentAccountMessage).
Here we need to create the subaddress and create a new PaymentAccountPayload object with that subaddress as address instead of the mainAddress and send that to the peer. When creating the subaddress we need to persist the incremented index. I guess the UserPayload class might be again the best option.
The privateViewKeyHex input field would need a validation method for verifying correct input.

I stopped at that point, so not sure if there are more issues. I have a patch of my work-in-progress code. I will share it with the dev who works on it.

For the subaddress generation I added the bisq.core.xmr.knaccc.monero.address.WalletAddress class.

@ghost
Copy link

ghost commented Jun 1, 2022

I am working on this, already have a fully functional POC undergoing testing and revision.

@chimp1984
Copy link
Contributor

I am working on this, already have a fully functional POC undergoing testing and revision.

Great. Invite me for review once there is a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants