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

Fix issue 4793 - M/N multisig transaction signature #4845

Merged
merged 1 commit into from Dec 4, 2018

Conversation

5 participants
@naughtyfox
Contributor

naughtyfox commented Nov 13, 2018

This PR fixes issue #4793

The problem was in insufficient LR pairs and supplementary transactions generated. Unlike standard N/N and N-1/N cases M/N sometimes requires more transaction generated to exclude all combinations of signers that may possibly to decline current transaction.

Hence, in 2/4 wallets with signers A, B, C and D with A be a transaction creator wallet needs to pick one more signer by excluding 2-of-3 possible signers combinations. So wallet have to create transactions with following signers to exclude: BC, BD and CD.

I think it's worth to include this fix in point release as well.

PS. Huge thanks to @rbrunner7 for active collaboration and his help with testing it!

@rbrunner7

This comment has been minimized.

Contributor

rbrunner7 commented Nov 13, 2018

I can't comment about the code because I don't know enough about the technicals of Monero multisig, but I can confirm that I did extensive tests with this PR checky-picked into master HEAD as of today, and errors that I had before (reported as issue #4793) did not occur anymore, neither did new errors occur.

I tested 3/3, 2/3, 2/4 and 3/7 multisig. For the M/N variants I did test transactions where all coalition members cooperated and the wallets received all sync info / key images from all other wallets, and tests where only a subset of coalition members exchanged sync info with each other, excluding other members, i.e. 2 in the 2/4 case. Both kinds of tests worked.

I consider it highly likely that with this PR generalized M/N multisig is fully working.

uint64_t c = 1;
for (uint64_t i = 1; i <= k; ++i) {
c *= n--;

This comment has been minimized.

@anonimal

anonimal Nov 13, 2018

Contributor

c *= n--;

Seeing this reminded me of http://hmarco.org/bugs/CVE-2015-8370-Grub2-authentication-bypass.html only because it's the quick-and-dirty little things that'll get you.

TL;DR be careful for underflow if the previous throw block is ever removed and if n is ever given zero (intentionally or not).

for (const auto &lr: p.m_LR)
{
if (used_L.find(lr.m_L) != used_L.end())
continue;

This comment has been minimized.

@moneromooo-monero

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Nov 20, 2018

Contributor

Can you remove the whitespace change while you're at it ? :)

This comment has been minimized.

@naughtyfox

naughtyfox Nov 20, 2018

Contributor

oh, sorry! of course!

@@ -0,0 +1,50 @@
// Copyright (c) 2014-2018, The Monero Project

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Nov 19, 2018

Contributor

That file doesn't seem to have any old code, so 2018

This comment has been minimized.

@naughtyfox

naughtyfox Nov 20, 2018

Contributor

ok, will do it

@@ -0,0 +1,96 @@
// Copyright (c) 2014-2018, The Monero Project

This comment has been minimized.

@moneromooo-monero

@naughtyfox naughtyfox force-pushed the naughtyfox:mn-fix branch from f741910 to 6732fc7 Nov 20, 2018

@fluffypony

Reviewed

@fluffypony fluffypony merged commit 6732fc7 into monero-project:master Dec 4, 2018

7 of 10 checks passed

buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-linux-armv7 Build done.
Details
buildbot/monero-linux-armv8 Build done.
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

fluffypony added a commit that referenced this pull request Dec 4, 2018

Merge pull request #4845
6732fc7 Fix issue 4793 - M/N multisig transaction signature (naughtyfox)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment