[Fix] Check certificate validity early.#3537
Conversation
Before recursively fetching previous certificates. Closes ProvableHQ#2935.
|
Asked Kai some questions in DM about testing this. |
|
Quick update: I was able to reproduce the attack. I will verify that the attack does not work anymore with the proposed changes soon. Here are some observations I made:
|
There was a problem hiding this comment.
I could verify the fix works (yay!), but, now that I have a better understanding of the code, I also left a few more notes on the PR.
To verify, this is the error message I get when honest nodes receive the attack:
Cannot store a certificate from '127.0.0.1:5004' - Certificate '2671926863586933361463317741031072687577528356414354042732451240158089381291field' for round 5 does not meet quorum requirements
Another thing we should consider, not in this PR but sometime in the future, is removing the recursion entirely, because there could still be valid cases where you receive a fairly long certificate chain. Alternatively, we could ensure that the number of recursions always stays below some small bound.
For example, it might be more elegant to have a data structure that maps certificate ID to unprocessed certificates that depend on it. Whenever we accept a batch, we check that mapping for certificates to process next.
Nice! That's indeed the error I was expecting to happen: the certificate doesn't have enough signatures. Under the correct super-majority assumption, the validator would be unable to obtain enough signatures. |
|
I have a branch here that adds unit tests, but it requires a small change to snarkVM. We could merge this PR now and I will add the tests with another PR. I tested it extensively, and it should land in staging sooner than later, considering it prevents a possible attack. |
Before recursively fetching previous certificates.
Closes #2935.
Since it looks like we had working code to run the exploit, it would be good to revive it and make sure it no longer crashes the validator.