[HackerOne-2498849] Abort fee earlier in prepare_for_speculate #2452
Conversation
Executions require full verification to avoid malleability attack. See: https://github.com/AleoHQ/snarkVM/issues/2451
vicsn
force-pushed
the
only_abort_deploys_early
branch
from
ca2b6ca
to
05c0273
Compare
raychu86
changed the title
synthesizer/src/vm/finalize.rs
Outdated
| // If the transaction is an Execution, we will fully verify it. | ||
| // Executions require full verification to avoid malleability attacks. | ||
| if transaction.is_execute() { | ||
| executions.push(*transaction); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
@vicsn I don't believe we want this. This opens up the attack where malicious parties and induce load on validators by verifying many transactions for free.
To have duplicate input_id, output_ids, and tcm you need to use the same private key to create the transaction, so it would only affect their own transactions.
The malleability attack from validators can only be done via converting the Execution to a Fee transaction. Which your subsequent check already catches.
synthesizer/src/vm/finalize.rs
Outdated
| // Abort the transaction if it is a fee transaction. | ||
| if transaction.is_fee() { | ||
| aborted_transactions.push((*transaction, "Fee transactions are not allowed in speculate".to_string())); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I believe adding this check should be enough. The only malleability attack validators can do is to convert the Execution to a Fee transaction.
The FakeTransaction attack can only be done this way. Having actual input_id, output_id, and tcm duplicates can only be crafted with the same private key, so malicious validators can't simply inject a transition from another transaction into their own.
@ghostant-1017 Let me know if this makes sense and if I am missing anything.
There was a problem hiding this comment.
My understanding is that the attack entails a validator creating a completely invalid bogus transaction by adding a random transition.
I pushed a proof of concept to the tests: 8949c47 . At least at this level in the code, I confirmed that without this PR, the bogus transaction "pushes out" the honest transaction.
Sidenote: This also made me realise that malicious validators can still cause payment-free verification of their malformed executions, though these should be cheap. I'll think about a resolution.
There was a problem hiding this comment.
I stand corrected. It looks like the validators can also mutate the transactions by swapping out the fee transaction. If the fee is too low, the transaction will be aborted (if the fee is high enough, then the transaction will be rejected and the fee is consumed.
The validator might also be able to mutate the transaction fields with garbage s.t. verification will fail and the TX will be aborted.
There was a problem hiding this comment.
The validator might also be able to mutate the transaction fields with garbage s.t. verification will fail and the TX will be aborted.
Good point, I think I also see an angle where the attacker can force a deployment to be aborted. The attacker mutates the deployment's only transition, which is the fee, to change its outputs but keep its inputs. Or vice versa. Therefore the transaction_id will be different but it still partially collides in the should_abort_transaction check.
Despite this, I have a weak opinion to keep calling should_abort_transaction on deployments, as this PR does, as it is still an effective DoS protection measure.
In general, we can consider proper malleability protection measures like a cheap hmac or signature over the full transaction contents which must be the first thing verified. But I think that's a post-mainnet discussion.
There was a problem hiding this comment.
This "attack" is a bit hard to detect, but slashing is something we need in the future to mitigate misbehavior by validators.
There was a problem hiding this comment.
Reverted skipping the execution check, now only aborting Fee early and added a test for it
|
Note that this is a potential breaking change, that may affect the ongoing networks. The validators being run most likely won't be throwing around these |
vicsn
changed the title
…rt_deploys_early"" This reverts commit 1816887.
Motivation
Does not close but partially mitigates: https://github.com/AleoHQ/snarkVM/issues/2451
A malicious validator can generate an execution with mostly the same transitions, which will be aborted causing the victim's transaction to be filtered out and dropped.
Test Plan
Related PRs
This was introduced by: https://github.com/AleoHQ/snarkVM/pull/2428