The Open Network - Audit 1 (Phase 2)

Malicious actor can receive a valid block, alter the `block->data->deps` array and rebroadcast the block to other nodes. New block will be valid for catchain nodes but invalid for BCP. The block validation works this way: 1. Overlay gets the block and calls `CatChainReceiverImpl::receive_message_from_overlay(src, data)` 2. `catchain_block` is fetched from `data`, the rest is treated `payload` 3. `CatChainReceiverImpl::receive_block(src, catchain_block, payload)` is called 4. `CatChainReceiverImpl::validate_block_sync(block, payload)` is called 5. `CatChainReceivedBlock::pre_validate_block(block, payload)` is called - `block->incarnation` is verified - `block->src` is verified - `block->data->deps.size()` is verified to be not more than `max_deps` - `block->data->prev->src` is verified to be the same as `block->src` - `block->data->prev->height + 1 == block->height` is verified - it is verified that there are no `deps` from the same source - it is verified that `payload` is not empty - **`block->data->deps` array is not checked and not covered by signature** 6. For each `deps` and `prev` as argument `CatChainReceiverImpl::validate_block_sync(dep)` is called - Signature (by `dep.src`) of `block_dep_id` is checked. `block_dep_id = { incarnation, source_hash(dep.src), dep.height, dep.data_hash }` 8. Signature (by `src`) of `block_id` is checked. `block_id = { incarnation, source_hash(src), height, hash(payload) }` of original block 9. `CatChainReceiverImpl::create_block(block, payload)` is called and updates `blocks_[hash(block_id)]` This can lead to network stalling without ability to punish the malicious actor. For example: 1. Malicious actor can clear the `deps` array of received block and force `CatChainReceiverImpl::synchronize_with()` all other nodes. They will accept the modified block and `CatChainReceiverImpl::deliver_block()` immediately to `ValidatorSessionImpl::preprocess_block()` with undefined result. 2. Malicious actor can replace one `dep` with unexisting correctly signed `block_id` and push it to other nodes. They will accept it, will never be able to deliver that block to `ValidatorSession`, and will reject the same block with correct `deps` from other nodes as duplicate.

Malicious actor can create many valid blocks to overflow internal structures of honest nodes and provoke their crash. 1. Many blocks with increasing `height` can be formed and sent by the malicious actor 2. Honest node Overlay gets the block and calls `CatChainReceiverImpl::receive_message_from_overlay(src, data)` 3. `CatChainReceiverImpl::receive_block()` is called 4. `CatChainReceiverImpl::create_block()` is called 5. `CatChainReceivedBlock::create()` allocates memory in heap Old blocks unloading is not implemented in general.

According to [Whitepaper](https://test.ton.org/catchain.pdf) the Catchain blocks form a directed acyclic graph using their `deps`. The Catchain code doesn't ensure that. Let's assume: 1. Malicious actor A creates a BlockA and shares its `block_id_A (incarnation, hash(src), height, hash(payload)` with malicious actor B 2. Malicious actor B creates a BlockB, shares its `block_id_B` with A 3. Malicious actor A uses `block_id_B` as part of `BlockA->data->deps` and broadcasts the block in the Catchain 4. Malicious actor B uses `block_id_A` as part of `BlockB->data->deps` and broadcasts the block in the Catchain 5. Honest node C gets two blocks that depend on each other Currently the code works correctly in this situation.

`CatChainReceivedBlockImpl` has fields `block_deps_`, `deps_`, `rev_deps_`, `pending_deps_`. - `block_deps_` holds pointers to blocks from other nodes - `deps_` holds **heights** of this block source chain pieces - `rev_deps_` holds pointers to blocks that depend on this one - `pending_deps_` holds the number of full blocks required to `CatChainReceiverImpl::run_block()` `deps_` name is misleading.

`CatChainReceivedBlockImpl` has fields `data_`, `hash_`, `payload_` and `data_hash_`. `data_` field is unused. `data_hash_` in fact holds `payload_` hash.

Catchain expects every full block received to have not more than `max_deps` dependencies. `max_deps` is 4 at this moment. However, when full block is sent to network, the `deps` size is not checked.

```cpp=85 if (!blocks_.size()) { ``` `blocks_.empty()` can be used instead of `!blocks_.size()`.

`map::emplace()` returns `std::pair<iterator,bool>`. `iterator` can be used to avoid the call to `map::find()`.

Indexing `blamed_sources_[src_id]` is done without validation of `src_id` correctness.

```cpp=35 auto B = *top_blocks_.get_random(); CHECK(B != nullptr); top_blocks_.remove(B->hash()); if (B->source() == sources_.size() || !blamed_sources_[B->source()]) { ``` The code allows `B->source() == sources_.size()`, however, all the insertions to `top_blocks_` ensure the valid `src_`.