Sign Up for a Code Audit Today and Get 3 Months of Skynet for Free

Malicious Validator can trigger crash of other `master_nodes` by spamming of different valid obligation votes. Different `vote.state_change.state` can be used to make `obligations_pool_entry` unique. 

Consider the call sequence:
1. `core::add_master_node_vote(vote)` gets `vote` from `quorumnet`
2. `quorum_cop::handle_vote(vote)`
    - `verify_vote_age(vote)` checks that `vote.block_height` is within 2 hours lifetime
    - `verify_vote_signature(vote)` checks that `vote.type`, `vote.group`, `vote.index_in_group`, `vote.state_change.worker_index` are valid
3. `voting_pool::add_pool_vote_if_unique(vote)`
4. `voting_pool::find_vote_pool(vote, create = true)`
5. `find_vote_in_pool(m_obligations_pool, vote, create = true)` can't find `vote` in pool because of new `vote.state_change.state`
6. `m_obligations_pool.push_back(vote)` adds new `obligations_pool_entry`

Validity of `vote.state_change.state` is never checked. `m_obligations_pool` will grow after each new vote until master node crash with "out of memory".

The same problem also occurs if validator spams with signed checkpoint votes with different `vote.checkpoint.block_hash`.

Validator can cause network DoS by spamming of obligation votes

```cpp=1631
  bool const enough    = stage.bitset >= context.transient.wait_for_handshake_bitsets.best_bitset;
```
`stage.bitset` contains the validator set that has signed the final block hash and `best_bitset` contains the most popular set of validators participated in the handshake process.

Comparing the bitset via arithmetic `>=` is misleading. The code assumes that stray messages from non-locked-in validators are rejected (that is actually checked in `handle_message()`). 

The variable name `enough` is also misleading.

Bitset arithmetic comparison is misleading

After getting the handshake bitsets the block producer prepares and sends the block template. The producer signature covers this data:
1. hashed `block_template.blob`
2. `msg.round`

When `block_template` received, this data is checked:
1. `block.POS.round == context.prepare_for_round.round` (number from 0 to 255)
2. `block.POS.validator_bitset == context.transient.wait_for_handshake_bitsets.best_bitset` (will likely be full set)
3. `msg.quorum_position == 0`
4. signature belongs to `context.prepare_for_round.quorum.workers[0]` (block producer)
5. block template can only be accepted once
6. block height is not validated.

The malicious actor can take the signed `block_template` message from previous blocks and resend it to quorumnet. If the block producer is the same, `round` is zero (likely), `handshake_bitset` is the same (likely), - then it will be accepted by all the master nodes. 

`block_template` contains the payout info and can be abused (in case of lucky payout, same template can be repeated for each height with the same producer). However, unlikely the final block will pass all the block validity checks (out of audit scope). Due to accepting `block_template` only once, the PoS network will be stalled (after all producer*round variants will be collected by malicious actor).

Signed by producer `block_template` message can be replayed

`msg.type` of type `enum POS::message_type` is filled in `quorumnet.cpp`. `pos.cpp` in each `switch (msg.type)` assumes the value is correct and doesn't contain `default:` branch.

Unexpected `msg.type` is not checked

```c=456
      if (vote.group > quorum_group::worker || vote.group < quorum_group::validator) {
          vvc.m_incorrect_voting_group = true;
          result = false;
      }
```
The code expects `vote.group` to be either `quorum_group::worker` or `quorum_group::validator`. Using of `<` and `>` is misleading and relies on `quorum_group` declaration.

Also, `verify_vote_signature()` in fact only verifies votes from validators, not workers.

Misleading Way to Check for Validator or Worker

In `verify_tx_state_change()` it is checked that:
- `STATE_CHANGE_MIN_VOTES_TO_CHANGE_STATE <= state_change.votes.size() <= STATE_CHANGE_QUORUM_SIZE` (from 7 to 10 on mainnet)
- `vote.validator_index < quorum.validators.size()`
- additionally checked that `validator_index` are stored in ascending order and don't have duplicates

However, it is not checked that `vote.validator_index < STATE_CHANGE_QUORUM_SIZE` or `quorum.validators.size() == state_change.votes.size()`. This is required by `validator_set[vote.validator_index]` statement.

No explicit check for `validator_index` in `verify_tx_state_change()`

```cpp=601
    uint64_t vote_lifetime = BLOCKS_EXPECTED_IN_HOURS(2,hf_version);
```
`vote_lifetime` is calculated in many places.

`BLOCKS_EXPECTED_IN_HOURS` can yield inaccurate result right after hardfork. For example, after V17 there are 120 blocks in hour that were in fact produced for 4 hours pre-V17.

```cpp=418
   int64_t decommission_minimum    = BLOCKS_EXPECTED_IN_HOURS(2,hf_version);
```
`decommission_minimum` is also hardcoded.

`vote_lifetime` can be inaccurate

The network is considered stalled after 255 unsuccessful attempts to generate block via PoS. The next block is allowed to use PoW.

PoS network stalled magic number

`MAX_SWARM_SIZE` and `SWARM_BUFFER` are never used in code. There is no assertion that `EXCESS_BASE >= MIN_SWARM_SIZE`.

`calc_excess()` comment mentions `MIN_SWARM_SIZE`:
```c=63
  /// The excess is calculated as the total number of mnodes above MIN_SWARM_SIZE across all swarms
```
but in fact `EXCESS_BASE` is used.

`get_excess_pool()` uses `MIN_SWARM_SIZE` to calculate excess of swarms bigger than `threshold`.

This leads to overcomplicated logic in `create_new_swarm_from_excess()`.

Swarm size constants should be refined

`UINT64_MAX` is used as a value with special meaning in `get_min_node_contribution_in_portions()`.

`UINT64_MAX` is used as a special node contribution value

"Utiliities" is supposed to be "utilities".

"Quorum worker index in was out of bounds" is supposed to be "Quorum worker index was out of bounds".

"perpare_for_round" is supposed to be "prepare_for_round".
 
"this returns true is the server" is supposed to be "this returns true if the server".

"invcation" is supposed to be "invocation".

Typos

```c=764
    if (blocks_up > 0) {

        int64_t decommission_credit_per_day = BLOCKS_EXPECTED_IN_HOURS(24,hf_version) / 30;
        credit += blocks_up * decommission_credit_per_day / BLOCKS_EXPECTED_IN_HOURS(24,hf_version);
    }
    int64_t decommission_max_credit   = BLOCKS_EXPECTED_IN_HOURS(48,hf_version);
```
The code contains the magic number 30. 

According to [Whitepaper "2.15.3.1 Block Credits"](https://www.beldex.io/whitepaper.pdf): "Each masternode is awarded 96 block credits in a day approx. The maximum block credit is 5760 blocks or 2 days approx."

Mentioning of 2 days here is misleading.

According to `master_node_rules.h`:
```c=64
  // Master node decommissioning: as master nodes stay up they earn "credits" (measured in blocks)
  // towards a future outage.  A new master node starts out with INITIAL_CREDIT, and then builds up
  // CREDIT_PER_DAY for each day the master node remains active up to a maximum of
  // DECOMMISSION_MAX_CREDIT.
```
So, "credits" is measured in blocks. All the other code also assumes that.

`TARGET_BLOCK_TIME_V17 = 30s` means there are 2880 blocks per day, that gives 96 credits per day.

Decommission credit calculation is misleading

`master_node_list.cpp` is out of audit scope. `master_node_info::can_transition_to_state()` returns `true` for invalid `proposed_state` (out of enum bounds).

`handle_obligations_vote()` doesn't check that `vote.state_change.state` is valid enum value and can create an invalid state change transaction (if enough validator signatures will be obtained).

Invalid `vote.state_change.state` can be written to blockchain

The function `process_quorums()` is too long, making it difficult to read and understand. In particular, many (logically) different actions are performed, which could be added into a separate function and called by `process_quorums()`.

Too Long Function `process_quorums()`

```c++=192
if (m_obligations_height >= height)
    {
      if (!by_pop_blocks)
      {
        LOG_ERROR("The blockchain was detached to height: " << height << ", but quorum cop has already processed votes for obligations up to " << m_obligations_height);
        LOG_ERROR("This implies a reorg occured that was over " << REORG_SAFETY_BUFFER_BLOCKS << ". This should rarely happen! Please report this to the devs.");
      }
      m_obligations_height = height;
    }
```
The error message is misleading. In particular, there is a reference to `REORG_SAFETY_BUFFER_BLOCKS` that is not used at all in the check.

Misleading Error message

`handle_checkpoint_vote()` always returns `true`. 

`handle_obligations_vote()` returns `false` if failed to `core.handle_incoming_tx()`. In other cases returns `true`, including the case if failed `cryptonote::add_master_node_state_change_to_tx_extra()`.

As a result, `handle_vote()` fails if the vote is invalid, but doesn't tell if correct vote was properly handled or not.

`quorum_cop::handle_vote` return value meaning is unclear

`quorum` argument is not used by `handle_checkpoint_vote()` implementation.

`quorum` argument is not used in `handle_checkpoint_vote()`

The management of the master node swarms is defined and implemented in `master_node_swarm.cpp`. But it is only updated by `master_node_lists.cpp` in the function `update_from_block()` and it is never used to get master nodes from the swarm. Therefore, the need for the whole swarm management and update is unclear and not documented in any official Beldex document.

Master Node Swarms are not used by the code in audit scope

```cpp=292
    /// 4. If there is a swarm with less than MIN_SWARM_SIZE, decommission that swarm.
    if (swarm_to_mnodes.size() > 1)
    {
      while (true)
```
The code finds small swarm, decommissions it and `assign_mnodes()` to the smallest swarms left. The check `swarm_to_mnodes.size() > 1` is done only once, but `assign_mnodes()` assumes `swarm_to_mnodes` is not empty.

The code crashes if for some reason there were two 1-sized swarms in `swarm_to_mnodes`.

Risk to decommission the last swarm in `calc_swarm_changes()`

```cpp=184
  /// Assign each mnode from mnode_pubkeys into the FILL_SWARM_LOWER_PERCENTILE percentile of swarms
```
The comment mentions `FILL_SWARM_LOWER_PERCENTILE`, however, in fact the function get `percentile` argument that can also be `DECOMMISSIONED_REDISTRIBUTION_LOWER_PERCENTILE`. 

Comment doesn't reflect the code of `assign_mnodes()`

```cpp=279
          LOG_PRINT_L2("Stolen 1 mnode from " << excess_mnode.public_key << " and donated to " << swarm.swarm_id);
```
`public_key` is used instead of `swarm_id`.

Inaccurate log message in `calc_swarm_changes()`

```cpp=55
uint64_t get_locked_key_image_unlock_height(cryptonote::network_type nettype, uint64_t node_register_height, uint64_t curr_height,uint8_t version)
{
  uint64_t blocks_to_lock = staking_num_lock_blocks(nettype,version);
  uint64_t result         = curr_height + (blocks_to_lock / 2);      
  return result;
}
```
`node_register_height` argument is unused by the function.

According to [Whitepaper "2.8 Authenticated Collateralization"](https://www.beldex.io/whitepaper.pdf): "The initial collateral by a masternode is locked for a specific block-height of 21,600 blocks."
That is 7.5 days with average block production speed of 2880 blocks per day, 1 block every 30 seconds.

According to `master_node_rules.h:staking_num_lock_blocks()` the `blocks_to_lock` is `BLOCKS_EXPECTED_IN_DAYS(30,hf_version)`. But `get_locked_key_image_unlock_height()` `result` uses half of it.

`master_node_list.cpp` that uses `get_locked_key_image_unlock_height()` is out of audit scope.

Unclear unlock height calculation algorithm

The arguments `m_nettype` in function `get_stacking_requirement()` and `node_register_height` in `get_locked_key_image_unlock_height()` are not used.

Unused Argument 

In the function `get_stacking_requirement()`, a magic number (i.e., 56500) is used to decide whether the staking requirement is `COIN * 100000` or `COIN * 10000`. 

Beldex

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

24

0

7

0

17