ChainInfo: reject blocks with overflowing transaction fee sums

Author: Eligioo

Cargo.lock

@@ -37,3 +37,6 @@ nimiq-primitives = { workspace = true, features = ["coin", "key-nibbles", "polic
 nimiq-serde = { workspace = true }
 nimiq-transaction = { workspace = true }
 nimiq-vrf = { workspace = true }
+
+[dev-dependencies]
+nimiq-keys = { workspace = true }

blockchain-interface/Cargo.toml

@@ -1,8 +1,8 @@
 use std::{fmt::Formatter, ops::RangeFrom};
 
 use nimiq_block::{
-    Block, BlockType, MacroBlock, MacroHeader, MicroBlock, MicroHeader, MicroJustification,
-    TendermintProof,
+    Block, BlockError, BlockType, MacroBlock, MacroHeader, MicroBlock, MicroHeader,
+    MicroJustification, TendermintProof,
 };
 use nimiq_database_value_derive::DbSerializable;
 use nimiq_hash::Blake2bHash;
@@ -58,24 +58,30 @@ impl ChainInfo {
     }
 
     /// Creates a new ChainInfo for a block given its predecessor.
+    ///
+    /// Returns an error if the transaction fee sum overflows.
     pub fn from_block(
         block: Block,
         prev_info: &ChainInfo,
         prev_missing_range: Option<RangeFrom<KeyNibbles>>,
-    ) -> Self {
+    ) -> Result<Self, BlockError> {
         assert_eq!(prev_info.head.block_number(), block.block_number() - 1);
 
         // Reset the transaction fee accumulator if this is the first block of a batch. Otherwise,
         // just add the transactions fees of this block to the accumulator.
+        let block_fees = block.sum_transaction_fees()?;
         let cum_tx_fees = if Policy::is_macro_block_at(prev_info.head.block_number()) {
-            block.sum_transaction_fees()
+            block_fees
         } else {
-            prev_info.cum_tx_fees + block.sum_transaction_fees()
+            prev_info
+                .cum_tx_fees
+                .checked_add(block_fees)
+                .ok_or(BlockError::TransactionFeeOverflow)?
         };
 
         let prunable = !block.is_election();
 
-        ChainInfo {
+        Ok(ChainInfo {
             on_main_chain: false,
             main_chain_successor: None,
             head: block,
@@ -84,7 +90,7 @@ impl ChainInfo {
             history_tree_len: 0,
             prunable,
             prev_missing_range,
-        }
+        })
     }
 
     /// Sets the value for the cumulative historic transaction size and the prunable flag
@@ -188,3 +194,70 @@ impl<'de> Visitor<'de> for ChainHeadVisitor {
         Ok(block)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use nimiq_block::{BlockError, MicroBlock, MicroBody, MicroHeader};
+    use nimiq_keys::Address;
+    use nimiq_primitives::{coin::Coin, networks::NetworkId, policy::Policy};
+    use nimiq_transaction::ExecutedTransaction;
+
+    use super::*;
+
+    fn tx_with_fee(fee: Coin) -> ExecutedTransaction {
+        ExecutedTransaction::Ok(nimiq_transaction::Transaction::new_basic(
+            Address::default(),
+            Address::from([1u8; 20]),
+            Coin::ZERO,
+            fee,
+            1,
+            NetworkId::UnitAlbatross,
+        ))
+    }
+
+    #[test]
+    fn test_chain_info_cumulative_fee_overflow() {
+        // Use block_number 2 so it's NOT a macro block predecessor, triggering the
+        // cumulative addition path.
+        let prev_block = Block::Micro(MicroBlock {
+            header: MicroHeader {
+                network: NetworkId::UnitAlbatross,
+                version: Policy::max_supported_version(),
+                block_number: 2,
+                timestamp: 0,
+                ..Default::default()
+            },
+            justification: None,
+            body: None,
+        });
+
+        let mut prev_info = ChainInfo::new(prev_block, true);
+        prev_info.cum_tx_fees = Coin::MAX;
+
+        // Create a block with fee = 1. Adding to MAX should overflow.
+        let transactions = vec![tx_with_fee(Coin::from_u64_unchecked(1))];
+        let micro_body = MicroBody {
+            equivocation_proofs: vec![],
+            transactions,
+        };
+        let block = Block::Micro(MicroBlock {
+            header: MicroHeader {
+                network: NetworkId::UnitAlbatross,
+                version: Policy::max_supported_version(),
+                block_number: 3,
+                timestamp: 1,
+                ..Default::default()
+            },
+            justification: None,
+            body: Some(micro_body),
+        });
+
+        assert!(
+            matches!(
+                ChainInfo::from_block(block, &prev_info, None),
+                Err(BlockError::TransactionFeeOverflow)
+            ),
+            "Expected TransactionFeeOverflow for cumulative fee overflow",
+        );
+    }
+}

blockchain-interface/src/chain_info.rs

@@ -122,7 +122,7 @@ impl Blockchain {
 
         read_txn.close();
 
-        let chain_info = ChainInfo::from_block(block, &prev_info, prev_missing_range);
+        let chain_info = ChainInfo::from_block(block, &prev_info, prev_missing_range)?;
 
         // Extend, rebranch or just store the block depending on the chain ordering.
         let result = match chain_order {

blockchain/src/blockchain/push.rs

@@ -495,7 +495,7 @@ impl Blockchain {
         }
 
         // Create a ChainInfo for the proposed block.
-        let chain_info = ChainInfo::from_block(block.clone(), &prev_info, prev_missing_range);
+        let chain_info = ChainInfo::from_block(block.clone(), &prev_info, prev_missing_range)?;
 
         let read_txn = self.read_transaction();
         // First the common ancestor of the two chains needs to be found.

blockchain/src/blockchain/verify.rs

@@ -114,7 +114,7 @@ impl LightBlockchain {
         }
 
         // Create the chain info for the new block.
-        let chain_info = ChainInfo::from_block(block, &prev_info, None);
+        let chain_info = ChainInfo::from_block(block, &prev_info, None)?;
 
         // More chain ordering.
         let result = match chain_order {

light-blockchain/src/push.rs

@@ -234,19 +234,24 @@ impl Block {
 
     /// Returns the sum of the fees of all of the transactions in the block. If the block is a Macro
     /// block it just returns zero, since Macro blocks don't contain any transactions.
-    pub fn sum_transaction_fees(&self) -> Coin {
+    ///
+    /// Returns an error if the sum of fees would overflow `Coin::MAX`.
+    pub fn sum_transaction_fees(&self) -> Result<Coin, BlockError> {
         match self {
-            Block::Macro(_) => Coin::ZERO,
+            Block::Macro(_) => Ok(Coin::ZERO),
             Block::Micro(block) => block
                 .body
                 .as_ref()
                 .map(|ex| {
                     ex.transactions
                         .iter()
                         .map(|tx| tx.get_raw_transaction().fee)
-                        .sum()
+                        .try_fold(Coin::ZERO, |acc, fee| {
+                            acc.checked_add(fee)
+                                .ok_or(BlockError::TransactionFeeOverflow)
+                        })
                 })
-                .unwrap_or(Coin::ZERO),
+                .unwrap_or(Ok(Coin::ZERO)),
         }
     }
 

primitives/block/src/block.rs

@@ -89,4 +89,7 @@ pub enum BlockError {
 
     #[error("Skip block contains a non empty body")]
     InvalidSkipBlockBody,
+
+    #[error("Transaction fee sum overflow")]
+    TransactionFeeOverflow,
 }

primitives/block/src/lib.rs

@@ -8,13 +8,14 @@ use nimiq_collections::BitSet;
 use nimiq_hash::Hash;
 use nimiq_keys::{Address, Ed25519PublicKey as SchnorrPublicKey, Ed25519Signature, KeyPair};
 use nimiq_primitives::{
+    coin::Coin,
     networks::NetworkId,
     policy::Policy,
     slots_allocation::{Validator, Validators, ValidatorsBuilder},
 };
 use nimiq_test_log::test;
 use nimiq_test_utils::blockchain::{generate_transactions, validator_address};
-use nimiq_transaction::ExecutedTransaction;
+use nimiq_transaction::{ExecutedTransaction, Transaction};
 
 /// Test blocks use timestamp 0; this allows up to 1 second into the future.
 const TEST_MAX_TIMESTAMP: u64 = 1000;
@@ -636,6 +637,72 @@ fn test_verify_micro_block_body_fork_proofs() {
     );
 }
 
+/// Helper to create a transaction with a specific fee and a unique recipient.
+/// The transaction is not signed and won't pass signature verification, but is
+/// sufficient for fee sum tests.
+fn tx_with_fee(fee: Coin, unique_byte: u8) -> ExecutedTransaction {
+    ExecutedTransaction::Ok(Transaction::new_basic(
+        Address::default(),
+        Address::from([unique_byte; 20]),
+        Coin::from_u64_unchecked(1),
+        fee,
+        1,
+        NetworkId::UnitAlbatross,
+    ))
+}
+
+#[test]
+fn test_sum_transaction_fees_overflow() {
+    let high_fee = Coin::from_u64_unchecked(Policy::TOTAL_SUPPLY);
+
+    // 5 transactions each with fee = TOTAL_SUPPLY should overflow Coin::MAX
+    let transactions: Vec<ExecutedTransaction> =
+        (0..5).map(|i| tx_with_fee(high_fee, i + 1)).collect();
+
+    let micro_body = MicroBody {
+        equivocation_proofs: vec![],
+        transactions,
+    };
+
+    let block = Block::Micro(MicroBlock {
+        header: MicroHeader {
+            network: NetworkId::UnitAlbatross,
+            version: Policy::max_supported_version(),
+            block_number: 1,
+            timestamp: 0,
+            body_root: micro_body.hash(),
+            ..Default::default()
+        },
+        justification: Some(MicroJustification::Micro(Ed25519Signature::default())),
+        body: Some(micro_body),
+    });
+
+    assert_eq!(
+        block.sum_transaction_fees(),
+        Err(BlockError::TransactionFeeOverflow),
+    );
+}
+
+#[test]
+fn test_sum_transaction_fees_normal() {
+    let fee = Coin::from_u64_unchecked(1000);
+    let transactions: Vec<ExecutedTransaction> = (0..3).map(|i| tx_with_fee(fee, i + 1)).collect();
+
+    let block = Block::Micro(MicroBlock {
+        header: MicroHeader::default(),
+        justification: None,
+        body: Some(MicroBody {
+            equivocation_proofs: vec![],
+            transactions,
+        }),
+    });
+
+    assert_eq!(
+        block.sum_transaction_fees(),
+        Ok(Coin::from_u64_unchecked(3000)),
+    );
+}
+
 #[test]
 fn test_verify_election_macro_body() {
     let mut macro_header = MacroHeader {