[Fix] Add additional input/output checks to Execution verification
#2511
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Meshiest
approved these changes
Jul 25, 2024
gluax
approved these changes
Jul 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This PR adds additional safety checks to Execution verification. Previously we weren't explicitly checking the number of transition inputs/outputs; we were passing through the inputs and outputs to the Varuna proof verification as the guarantee. The problem is that the Varuna verifier pads the inputs with
Field::zero. This means that malicious parties can craft a mutated transactions by taking an honest execution transaction, adding an extraOutput::ExternalRecord(Field::zero))output that uses the same proof (and different fee), and crafting a new transaction with a new ID that performs the same state transition.We fix this by adding an explicit check against the number if inputs and the number of outputs in each transition.
TLDR:
This "attack" can NOT perform malicious state changes. It will be able to cause the honest transaction to be rejected, but still perform the same state changes. The risk is that this may cause a victim to be enticed/confused into re-executing their transaction.
The CI run is here.
Test Plan
A test has been added to ensure that the mutated transaction fails to verify due to the newly added check against the number of outputs.
Related Issues
Partially related to possible transaction mutations discussed here - #2451.