Scalars not being reduced before generating Monero sub-addresses

[Boog900]

Description

When entering a private view key to generate sub-addresses Bisq is not reducing the scalar mod l

Version

1.9.9

Steps to reproduce

• go to create a new account, select Monero
• select use subaddresses and enter:
  • Main address: 44KbPP1SBynZGeMwVS335c4bnDeGFsHdyZVyFNYe1CkkDsVfSa1FJcofLNCtNELAtiQ8Fyv2D2ku2UX1exLVaRroKviVSFs
  • Private view key: 42594aba0e809490dec97b2dfaf64f7ae5bef1c2d19af636eb84544773df5b5f

Expected behaviour

This should be an error as the scalar is not reduced or the scalar should be reduced and then the sub address at index(0,1) should be:

887PFuHPnZWS4YxsV5QiUpjeQ3MV1ZAqgehzC3tGoG543QH8hYsvUHABW7VrU7TC7YbWFZHf211mvP1PgkKni5mMTxotufL

Actual behaviour

the sub address at index(0,1) is shown as:

835abuQd633VLwzEc9PqARaXYgSuMaXdq1Y3KfSo2Sjw3pYfeMKoJUFLEPvsh9cUgMT75m8pgFXvLSDWbDovSob45B4HETs

additional info

if you want the full wallet information:
Private view key: 42594aba0e809490dec97b2dfaf64f7ae5bef1c2d19af636eb84544773df5b5f
Private spend key: c41905a68310309043250478e61e451652f8584d59471f54c7492f4b20e8ae0b

you can enter theses keys here: https://xmr.llcoins.net/addresstests.html to show the main wallet address is truly:
44KbPP1SBynZGeMwVS335c4bnDeGFsHdyZVyFNYe1CkkDsVfSa1FJcofLNCtNELAtiQ8Fyv2D2ku2UX1exLVaRroKviVSFs

[boring-cyborg]

Thanks for opening your first issue here!

Be sure to follow the issue template. Your issue will be reviewed by a maintainer and labeled for further action.

[ghost]

For subaddress generation we are using https://github.com/knaccc/subaddress-java provided by the monero community.
See original discussion here.

When running the above main address and private view key through subaddress-java it produces the same results as Bisq.

subaddress for account index 0, subaddress index 1: 835abuQd633VLwzEc9PqARaXYgSuMaXdq1Y3KfSo2Sjw3pYfeMKoJUFLEPvsh9cUgMT75m8pgFXvLSDWbDovSob45B4HETs

@knaccc do you think there is an issue here?

[Boog900]

Edit:

Wait no im wrong Bisq isn't even reducing the scalar after that function and im guessing this has precursor that scalars passed in must be reduced or it'll give a bogus output?

For context im trying to help the person who lost their funds but with that function giving random outputs for non-canonical scalars I think the funds are defiantly bunt

[ghost]

I would like to hear an opinion from someone else in the Monero community such as @knaccc or @woodser.

[ghost]

Non-issue, per @kayabaNerve comment on matrix.

[kayabaNerve]

@jmacxx Sorry for the confusion. I didn't need anything more from Bisq in order to help hanshan. This should still be an open issue though.

Bisq, nor subaddress-java, nor the underlying NEM crypto library, isn't performing input validation. If a public view key is entered, as done here, an invalid subaddress derivation will be generated and an invalid scalar multiplication will occur. Bisq needs to check an actual public spend key and an actual private view key were entered. Ideally, when Bisq accepts these fields, the user also specifies their wallet's main address which Bisq checks the keys against.

[ghost]

Thanks for clarifying, #6611 is the request for improved validation.

[Boog900]

The way i see it is there are 2 issues here:

1. someone entering a private view key which is not related to the public view key of the address entered
2. someone entering a view key which is a non-reduced scalar

#6611 and the pr fixing this issue are only properly addressing issue 1, I think it will also be a good idea to check if the scalar is fully reduced.

note: the PR fixing #6611 does actually fix issue 2 but only because this gives incorrect outputs for non-reduced scalars, I feel a more explicit check would be beneficial

[ghost]

@Boog900 Would it make sense to for you to make a PR fix in https://github.com/knaccc/subaddress-java ? Then Bisq can upgrade its copy of that code.

[Boog900]

I'll reopen this issue and get round to it but I can't see any PR to that repo getting merged any time soon, the last update was 2 and half years ago