[Bug]Malicious validators can prevent legitimate transactions from being executed.

[ghostant-1017]

https://hackerone.com/reports/2498849

Summary:

Malicious validators can prevent legitimate transactions from being executed.

Proof-of-Concept (PoC)

1. When a malicious validator receives any transaction, it can create a FakeTransaction using the following method: FakeTransaction = Transaction + sample_transiton().
2. FakeTransaction and Transaction have different TransactionIDs but share the same TransitionIDs, InputIDs, OutputIDs, and TPK. According to the current logic in the code, if the FakeTransaction is sorted before the Transaction in the DAG, it will cause the Transaction to be aborted.
3. Malicious validators broadcast the FakeTransaction externally via WorkerPing, potentially causing honest validator nodes to prioritize the FakeTransaction over the Transaction.
4. Furthermore, validators can fabricate FakeTransactions for uncommitted transactions upon proposing each proposal. If a malicious validator is chosen as the leader, it will result in all transactions in the block being aborted.

Impact

This attack can lead to a large number of legitimate transactions being aborted, and the abort_transaction_id will be recorded in the ledger, requiring users to regenerate transactions.

[vicsn]

This looks like an issue to me as well. Phrasing the issue in my own words: executions require full verification to avoid malleability attacks. There is a tradeoff between spam reduction (if we abort on partial transaction contents) v.s. censorship resistance (if we abort on the full transaction). The latter seems more important to me.

Fortunately, deployments don't have this particular malleability issue, so a quickfix can be to only call early should_abort_transaction on deployments, which are most expensive to verify: https://github.com/AleoHQ/snarkVM/pull/2452

I think a similar issue is possible when a malicious validator replaces global_state_root/proof/verifying_keys - given that those are not included in the transaction id calculation: https://github.com/AleoHQ/snarkVM/blob/mainnet/ledger/block/src/transaction/merkle.rs#L138 . This might be intentional design, to enable delegation of proofs while retaining deterministic transaction ids.

[vicsn]

After additional consideration, we have decided to only partially mitigate this for now and prioritize DoS prevention. Slashing can likely properly mitigate this censorship issue in the future.

[gkp-sumaiya]

This looks like an issue to me as well.

[HarukaMa]

IIRC we weren't considering any slashing methods; has anything changed since then?

[vicsn]

IIRC we weren't considering any slashing methods; has anything changed since then?

I view it as likely to be essential to add post-mainnet launch.

[damons]

Doesn't sound like we'd block mainnet for this. Marking as such. Feel free to argue.

[raychu86]

Should be addressed with the following fixes:

• [HackerOne-2498849] Abort fee earlier in prepare_for_speculate #2452
• [Fix] Add additional input/output checks to Execution verification #2511
• [Fix] Add checksum to TransmissionID snarkOS#3367