double check old audit issues

[hansieodendaal]

We need to revisit the audit and ensure that we have not reintroduced any of the same findings they had

https://www.coinspect.com/blog/tari-security-audit/

Report in pdf format

[hansieodendaal]

Reviewing TARI-001 to TARI-010

1. TARI-001 (Block producer can delay competing miners)
   mitigated

   • No single write lock anymore when processing new blocks.
   • async fn check_min_block_difficulty prevents cheap blocks to be created.
   • Peers are banned for malicious behaviour.

2. TARI-002 (Attackers can prevent block processing)

   mitigated

   • No single write lock anymore when processing new blocks.
   • async fn check_min_block_difficulty prevents cheap blocks to be created.
   • Peers are banned for malicious behaviour.

3. TARI-003 (Nodes waste resources validating fake orphan blocks)

   mitigated

   • No single write lock anymore when processing new blocks.
   • async fn check_min_block_difficulty prevents cheap blocks to be created.
   • Peers are banned for malicious behaviour.

4. TARI-004 (Attackers can spam mempool at no cost)

   NOT mitigated

   • A local mempool check if tx_fee.as_u64() < self.unconfirmed_pool.config.min_fee was implemented, but does not mitigate the issue that an attacker can craft a transaction that will pass all validation checks but fail on the last one. As an example, an attacker can future date a minimal value UTXO to themselves, then create a multitude transactions that spend that same UTXO now and broadcast it to the mempool. The very last chack when validating a mempool transaction is to do verify_timelocks(&body, height)?;, thereby succeeding in the attack because all the expensive validation checks have been done before.

5. TARI-005 (Attackers can mine several blocks at once by abusing ExtraField data)

   mitigated

   • Only one merge mining hash is allowed otherwise we return a validatio error

6. TARI-006 (Attackers can mine several blocks at once by using multiple Monero coinbase transactions)

   mitigated

   • Only one merge mining hash is allowed otherwise we return a validatio error, and it contains the depth/position
   • The path bitmap is not used anymore.
   • The position must correspond to the number of aux chains

7. TARI-007 (Malicious script makes validators waste computations)

   mitigated

   • Signature and public key iterators can only move forward making sure they are perfectly aligned and use minimum computation cycles.

8. TARI-008 (Attackers can create the same block with different hashes)

   mitigated

   • Only one merge mining hash is allowed otherwise we return a validatio error, and it contains the depth/position
   • The path bitmap is not used anymore.
   • The position must correspond to the number of aux chains

9. TARI-009 (Attackers can halt long-syncing by inflating block headers)

   mitigated

   • Inflating block headers by appending data to the PoW buffer is not possible as now pub pow_data: PowData is size limited with

  pub type PowData = MaxSizeBytes<{ NOT_BEFORE_PROOF_BYTES_SIZE }>

1. TARI-010 (Attackers can halt long-sync by appending trailing data to Proof of Work buffer)

   mitigated

   • Appending data to the PoW buffer is not possible as now pub pow_data: PowData is size limited with

  pub type PowData = MaxSizeBytes<{ NOT_BEFORE_PROOF_BYTES_SIZE }>

[hansieodendaal]

Reviewing TARI-011 to TARI-020

1. TARI-011 (Attackers can crash nodes by manipulating proof of work data)

   mitigated

   • depth is bounded to branch length

2. TARI-012 (Proof of work can be copy-pasted)

   mitigated

   • input hash is returned, depth == 0 not trusted anymore

3. TARI-013 (Attackers can force sync on a peer eternally)

   mitigated

   • all inconsistecies when compared to advertized metadata upon which the initial sync descision is based result in a peer ban

4. TARI-014 (Miner can ban competition)

   mitigated

   • validator_node_mr field in the header is not malleable anymore

5. TARI-015 (Known bad blocks are validated)

   mitigated

   • Bad blocks are consistently checked accross the board. A peer thar sends a known bad block will be banned.

6. TARI-016 (Attackers can halt synchronization)

   mitigated

   • Sending a repeated header results in a ban

7. TARI-017 (Attackers can avoid ban on horizon sync)

   mitigated

   • Any peers misbehaviour during sync gets the peer banned

8. TARI-018 (Consensus is dependent on platform)

   mitigated

   • Default Tari applications only support 64bit only and will block when running on 32 bit applications

9. TARI-019 (Attackers can crash a node by forcing over allocation)

   mitigated

   • The following pattern has been adopted where applicable:
     • A VarInt is the first data received in deserialization. This is checked against an upper limit for the byte stream.
     • Type MaxSizeVec has been introduced to kerb any vector overallocation.

10. TARI-020 (Default output selection criteria can be abused to increase the sender's fee)

    mitigated

    • Two config settings are employed to kerb this:

    /// If a large amount of tiny valued uT UTXOs are used as inputs to a transaction, the fee may be larger than the
    /// transaction amount. Set this value to `false` to allow spending of "dust" UTXOs for small valued transactions.
    pub prevent_fee_gt_amount: bool,
    /// Ignores dust below this value, value in micro MinoTari
    pub dust_ignore_value: u64,

[SWvheerden]

I disagree about TARI-004 (Attackers can spam mempool at no cost)

The issue is about providing a spam amount of 0 fee transactions. And thus flooding the mempool with valid transactions.

The Tari network has limits on the number of messages that can be sent and a min fee for transactions. Beyond that a node has to validate the tx before it can be propagated to the network.

[delta1]

Is your min fee for transactions a consensus parameter or a mempool policy?

[hansieodendaal]

Reviewing TARI-021 to TARI-030

1. TARI-021 (Attackers can crash nodes by broadcasting JOIN messages)

   PARTIALLY mitigated

   • MultiaddressesWithStats fn sort_addresses truncates all addresses above size MAX_ADDRESSES, but this is done after vector allocation.
   • addresses: Vec<MultiaddrWithStats> could be replaced with a MaxSizeVec:

   pub struct MultiaddressesWithStats {
       addresses: Vec<MultiaddrWithStats>,
   }

   with

   pub(crate) type SizedMultiaddresses = MaxSizeVec<MultiaddrWithStats, MAX_ADDRESSES>;
   pub struct MultiaddressesWithStats {
       addresses: SizedMultiaddresses,
   }