[Bug] Due to changes from PR #3217, BFT sync logic is not safe, 2 malicious validators can send invalid block_locators and blocks to other nodes and halt the network

[feezybabee]

https://hackerone.com/reports/2469178

Summary:

BFT sync logic is not safe after PR #3217 ; 2 malicious validators can send invalid block_locators and blocks to other nodes, and the other nodes will accept them and get halted.

Steps To Reproduce:

1. Use this commit
2. Run script ./devnet.sh with 8 validators.
3. Check the state of all nodes

Proof-of-Concept (PoC)

This attack only requires two validator nodes and is independent of the amount staked by the validators, so it doesn't violate the Byzantine assumption.

1. Validator0 and Validator1 are two malicious nodes. Validator0 is the main attacker, while for Validator1, we only need to use its private key, referred to as partner_pk. The purpose of using this private key is to generate a BatchCertificate that can pass validation checks.
2. Assuming Validator0's current height is current_height and current_round, Validator0 can create a Block for height current_height + 1 using its own private_key and the partner_key. This Block can pass validation checks performed by other nodes' check_next_block function and be successfully advanced by other nodes using advance_next_block. Refer to the code here.
3. Validator0 will increase the frequency of PrimaryPing (interval 5s => 1s), ensuring that other nodes detect the Block which is current_height + 1.
4. When Validator0 receives an Event::BlockRequest from other nodes, it can send the incorrect Block to them.
5. When other nodes receive the BlockResponse, the malicious Block will pass all checks and eventually be advanced using advance_to_next_block, causing the ledger to be in an incorrect state.

It's worth noting that the malicious Block may not pass sync_certificate_with_blocks, but since the attack is sustainable, other nodes will eventually end up in an incorrect state.

Impact

Two malicious validators can halt the network. Recommendation to mitigate this: add quorum check for Subdag when check_next_block.

[raychu86]

This PR should address the issue - https://github.com/AleoHQ/snarkOS/pull/3232.

The finding was related to another issue we were observing in devnet, but this change should address both concerns.

@ghostant-1017 Please confirm if you believe this fix is sufficient!

[ghostant-1017]

Hi @raychu86 , The attack doesn't work only for validators which is syncing. In fact, it works for ALL SYNCED validators. The attacker will always generate block which height is latest_height + 1, so that other validators will find themselves are behind, and send BlockRequest to the attacker. Please reevaluate the severity of the report. Thank you!

[raychu86]

@ghostant-1017 Happy to re-evaluate the report security, lets correspond in HackerOne regarding that.

In the meantime, we have another addition to this fix in the form of https://github.com/AleoHQ/snarkOS/pull/3233 that addresses an edge case. Would love you have your review of this as well.

[raychu86]

Closing with https://github.com/AleoHQ/snarkOS/pull/3232