maci icon indicating copy to clipboard operation
maci copied to clipboard

Coordinator message processing fails to tally votes properly in case key change or key deactivation performed

Open ognjenkurtic opened this issue 2 years ago • 4 comments

During the implementation of ElGamal/rerandomization features we encountered an issue with message processing. Messages are processed in batches in reverse order due to a security issue ( as explained here). Current message processing implementation introduces issues for both the original protocol and the ElGamal modification.

Example:

Messages in a batch are: 1. Change key 2. Vote with a new key

Since message processing is done in reverse, message processor encounters the voting message first but, as the key change hasn't occurred yet, the vote is considered as no-op. The key change message is then processed, but it is not very useful as the key is not used in the past. The same issue appears with new key generation messages in our updated protocol.

This issue (#717) is linked to the tests on "elgamal" branch that display behavior corresponding to this issue.

TODO comments that link to this issue can be found in test files that are showcasing this behavior: /cli/tests/vanilla/testKeyChange.sh /integrationTests/ts/ tests /suites.test.ts

ognjenkurtic avatar Aug 21 '23 08:08 ognjenkurtic

A possible fix is modifying the message processing in a way that the messages are not stored in the message tree but in a blockchain manner where each message is hashed with the previous message and the "root hash| is the last hash in the chain. That way the correct ordering of the messages will be maintained and the potentially expensive message tree could be removed.

aleksandar-veljkovic avatar Aug 21 '23 11:08 aleksandar-veljkovic

It should also be checked if the security issue linked in the description is still relevant with the elgamal flow - if not, messages could be processed in the order they were published which would fix the problem.

ognjenkurtic avatar Aug 21 '23 12:08 ognjenkurtic

Hey @ognjenkurtic, thanks for reporting this!

Messages are processed in batches in reverse order due to a security issue ( as explained here)

I'm seeing a "403 Forbidden" on this doc - could you please grant public access to this?

Current message processing implementation introduces issues for both the original protocol and the ElGamal modification.

So you're saying this is a bug with the current MACI implementation. @ctrlc03 @baumstern are you aware of this?

samajammin avatar Oct 31 '23 20:10 samajammin

Hey @ognjenkurtic, thanks for reporting this!

Messages are processed in batches in reverse order due to a security issue ( as explained here)

I'm seeing a "403 Forbidden" on this doc - could you please grant public access to this?

Current message processing implementation introduces issues for both the original protocol and the ElGamal modification.

So you're saying this is a bug with the current MACI implementation. @ctrlc03 @baumstern are you aware of this?

I believe the document that is referenced here is this: https://hackmd.io/AP6zPSgtThWxx6pjXY7R8A#Order-of-message-processing. That is the reason why currently messages are processed in reverse order, and this makes key changes not work correctly. Though you could send one message, then send a second one with a new public key, and the same nonce, and your key would be changed and this message counted as valid (as it being processed in reverse order it would be the first)

ctrlc03 avatar Dec 01 '23 16:12 ctrlc03

Closing this issue as documentation around key change and tests have been added, as well as the el gamal flow being replaced by #1566

ctrlc03 avatar Jun 14 '24 05:06 ctrlc03