Skip to content

Conversation

@UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Feb 3, 2021

A malicious quorum can sign a clsig with some (random) hash and some future height, up to tip + SIGN_HEIGHT_OFFSET (anything above it won't pass VerifyRecoveredSig/SelectQuorumForSigning, see tests for fake_clsig2). Such a clsig not only does not lock the chain but it also prevents new clsigs (for heights up to that future one) from forming or being accepted, see tests for fake_clsig3 (fails without b010bc9). Chances to form such a quorum are close to 0 e.g. 1.29e-38 if you control 1500 MNs (see Success probability of creating a malicious ChainLock here for more info), so it's not a critical issue by any means but I feel like it still makes sense to reduce any potential impact of such an event when possible.

Note: the issue is only for future clsigs, the best clsig with a known block is not affected, no reorgs below the already locked block are possible.

Copy link

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! 👍 See suggestions-3978 for a few potential (trivial) tweaks in the test. In any case it looks like we need a rebase here after #3976 before an approval (if we merge that one first).

EDIT: I guess you can also just skip the MSG_CLSIG commits. Looks like thats something we could anyway refactor in an other PR by adding islock/clsig support to mininode because its also used in other places i.e. duplicated code.

@UdjinM6 UdjinM6 marked this pull request as draft February 10, 2021 09:52
UdjinM6 and others added 3 commits February 15, 2021 12:26
…igs when it makes sense

A malicious quorum can sign a clsig with some (random) hash and some future height. This will basically disable clsig logic for all heights up to that future one without actually locking the chain. Note that the best known clsig with a known block is not affected by the issue, no conflicts below that block are allowed. Using the best clsig with a known block instead of a pure best clsig (with no known blocks) fixes the issue.
@UdjinM6 UdjinM6 marked this pull request as ready for review February 15, 2021 10:08
@UdjinM6
Copy link
Author

UdjinM6 commented Feb 15, 2021

Rebased

Copy link

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@PastaPastaPasta
Copy link
Member

I'm not sure how I feel about this... Really, I'd say this is the best case for a malicious chainlock. If someone forms a malicious chainlock, the best outcome is the chain halting, because that prevents any attacker from actually making any money or anything off of the attack.

@UdjinM6
Copy link
Author

UdjinM6 commented Feb 17, 2021

@PastaPastaPasta I don't understand... can you elaborate pls?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants