Fix underflow panic in vesting and HTLC insufficient funds error path

When a vesting or HTLC contract had total_amount > balance (reachable
via the 32-byte creation format), the InsufficientFunds error
construction computed self.balance - min_cap which panics on
underflow. In the block processing path this poisons the blockchain
RwLock, making the node permanently unusable.

Use saturating_sub for the error field arithmetic and add validation
that total_amount <= tx_value during contract creation.

Author: jsdanielh

primitives/account/src/account/htlc_contract.rs

@@ -79,8 +79,8 @@ impl HashedTimeLockedContract {
 
                 if new_balance < min_cap {
                     return Err(AccountError::InsufficientFunds {
-                        balance: self.balance - min_cap,
-                        needed: self.balance - new_balance,
+                        balance: self.balance.saturating_sub(min_cap),
+                        needed: self.balance.saturating_sub(new_balance),
                     });
                 }
 

primitives/account/src/account/vesting_contract.rs

@@ -51,8 +51,8 @@ impl VestingContract {
 
         if new_balance < min_cap {
             return Err(AccountError::InsufficientFunds {
-                balance: self.balance - min_cap,
-                needed: self.balance - new_balance,
+                balance: self.balance.saturating_sub(min_cap),
+                needed: self.balance.saturating_sub(new_balance),
             });
         }
 

primitives/account/tests/vesting_contract.rs

@@ -200,14 +200,14 @@ fn it_can_create_contract_from_transaction() {
     assert_eq!(contract.step_amount, 50.try_into().unwrap());
     assert_eq!(contract.total_amount, 100.try_into().unwrap());
 
-    // Transaction 3
+    // Transaction 3: valid 32-byte format (total_amount <= tx_value)
     let mut data: Vec<u8> = Vec::with_capacity(Address::SIZE + 32);
     let owner = Address::from([0u8; 20]);
     Serialize::serialize_to_writer(&owner, &mut data);
     Serialize::serialize_to_writer(&0u64.to_be_bytes(), &mut data);
     Serialize::serialize_to_writer(&100u64.to_be_bytes(), &mut data);
     Serialize::serialize_to_writer(&Coin::try_from(50).unwrap(), &mut data);
-    Serialize::serialize_to_writer(&Coin::try_from(150).unwrap(), &mut data);
+    Serialize::serialize_to_writer(&Coin::try_from(80).unwrap(), &mut data);
     tx.recipient_data = data;
     tx.recipient = tx.contract_creation_address();
 
@@ -232,9 +232,59 @@ fn it_can_create_contract_from_transaction() {
     assert_eq!(contract.start_time, 0);
     assert_eq!(contract.time_step, 100);
     assert_eq!(contract.step_amount, 50.try_into().unwrap());
-    assert_eq!(contract.total_amount, 150.try_into().unwrap());
+    assert_eq!(contract.total_amount, 80.try_into().unwrap());
+
+    // Transaction 4: 32-byte format with total_amount > tx_value (rejected)
+    let mut data: Vec<u8> = Vec::with_capacity(Address::SIZE + 32);
+    Serialize::serialize_to_writer(&owner, &mut data);
+    Serialize::serialize_to_writer(&0u64.to_be_bytes(), &mut data);
+    Serialize::serialize_to_writer(&100u64.to_be_bytes(), &mut data);
+    Serialize::serialize_to_writer(&Coin::try_from(50).unwrap(), &mut data);
+    Serialize::serialize_to_writer(&Coin::try_from(150).unwrap(), &mut data);
+    tx.recipient_data = data;
+    tx.recipient = tx.contract_creation_address();
+
+    let mut tx_logger = TransactionLog::empty();
+    let result = accounts.test_create_new_contract::<VestingContract>(
+        &tx,
+        Coin::ZERO,
+        &block_state,
+        &mut tx_logger,
+        true,
+    );
+    assert_eq!(
+        result,
+        Err(AccountError::InvalidTransaction(
+            TransactionError::InvalidData
+        ))
+    );
+
+    // Transaction 5: 32-byte format with step_amount > total_amount (rejected)
+    let mut data: Vec<u8> = Vec::with_capacity(Address::SIZE + 32);
+    Serialize::serialize_to_writer(&owner, &mut data);
+    Serialize::serialize_to_writer(&0u64.to_be_bytes(), &mut data);
+    Serialize::serialize_to_writer(&100u64.to_be_bytes(), &mut data);
+    Serialize::serialize_to_writer(&Coin::try_from(80).unwrap(), &mut data);
+    Serialize::serialize_to_writer(&Coin::try_from(50).unwrap(), &mut data);
+    tx.recipient_data = data;
+    tx.recipient = tx.contract_creation_address();
 
-    // Transaction 4: invalid data
+    let mut tx_logger = TransactionLog::empty();
+    let result = accounts.test_create_new_contract::<VestingContract>(
+        &tx,
+        Coin::ZERO,
+        &block_state,
+        &mut tx_logger,
+        true,
+    );
+    assert_eq!(
+        result,
+        Err(AccountError::InvalidTransaction(
+            TransactionError::InvalidData
+        ))
+    );
+
+    // Transaction 6: invalid data length
     tx.recipient_data = Vec::with_capacity(Address::SIZE + 2);
     Serialize::serialize_to_writer(&owner, &mut tx.recipient_data);
     Serialize::serialize_to_writer(&0u16.to_be_bytes(), &mut tx.recipient_data);
@@ -481,6 +531,134 @@ fn reserve_release_balance_works() {
     assert!(result.is_ok());
 }
 
+#[test]
+fn total_amount_exceeds_balance_panics_on_reserve_balance() {
+    // Attack scenario: attacker uses the 32-byte vesting creation format to set
+    // total_amount > transaction.value. This creates a contract where min_cap() can
+    // exceed self.balance. When an outgoing transaction triggers can_change_balance(),
+    // the error path computes `self.balance - min_cap` which panics on underflow.
+    //
+    // In the mempool path (reserve_balance), this runs inside a tokio::task::spawn,
+    // so the panic kills only the verification task — not the whole node process.
+    // However, in the block processing path (commit_outgoing_transaction), the panic
+    // unwinds through Blockchain::push which holds an RwLockUpgradableReadGuard.
+    // The poisoned lock makes the blockchain permanently unusable, effectively
+    // crashing the node.
+
+    let mut rng = test_rng(true);
+    let key_owner = KeyPair::generate(&mut rng);
+    let key_recipient = KeyPair::generate(&mut rng);
+
+    // Create a vesting contract where total_amount (2000) > balance (1000).
+    // This is reachable via the 32-byte CreationTransactionData format which
+    // deserializes total_amount from attacker-controlled recipient_data without
+    // validating total_amount <= tx_value.
+    let vesting_contract = VestingContract {
+        balance: Coin::from_u64_unchecked(1000),
+        owner: Address::from(&key_owner.public),
+        start_time: 0,
+        time_step: u64::MAX, // Ensures 0 steps elapsed, so min_cap = total_amount
+        step_amount: Coin::from_u64_unchecked(1),
+        total_amount: Coin::from_u64_unchecked(2000), // > balance!
+    };
+
+    let accounts = TestCommitRevert::with_initial_state(&[
+        (
+            Address::from(&key_owner.public),
+            Account::Basic(BasicAccount {
+                balance: Coin::from_u64_unchecked(1000),
+            }),
+        ),
+        (
+            Address([1u8; 20]),
+            Account::Vesting(vesting_contract.clone()),
+        ),
+    ]);
+
+    let mut db_txn = accounts.env().write_transaction();
+    let sender_address = Address::from(&key_owner);
+    let data_store = accounts.data_store(&sender_address);
+
+    // Block time 0 with time_step=MAX means 0 steps have elapsed, so
+    // min_cap = total_amount = 2000 > balance = 1000.
+    let block_state = BlockState::new(1, 0);
+
+    let mut reserved_balance = ReservedBalance::new(sender_address.clone());
+
+    // Outgoing transaction for 1 luna — the amount doesn't matter,
+    // any outgoing tx triggers the panic because min_cap > balance.
+    let tx = make_signed_transaction(key_owner.clone(), key_recipient.clone(), 1);
+
+    // This panics in can_change_balance at:
+    //   balance: self.balance - min_cap  →  1000 - 2000  →  underflow panic
+    //
+    // After the fix, this should return Err(AccountError::InsufficientFunds)
+    // without panicking.
+    let result = vesting_contract.reserve_balance(
+        &tx,
+        &mut reserved_balance,
+        &block_state,
+        data_store.read(&mut db_txn),
+    );
+    assert!(
+        result.is_err(),
+        "Expected InsufficientFunds error, got: {:?}",
+        result
+    );
+}
+
+#[test]
+fn total_amount_exceeds_balance_panics_on_commit_outgoing() {
+    // Same root cause as above, but triggered via commit_outgoing_transaction —
+    // the block processing path. This is the more dangerous path because it runs
+    // while holding the blockchain RwLock, poisoning it on panic.
+
+    let mut rng = test_rng(true);
+    let key_owner = KeyPair::generate(&mut rng);
+    let key_recipient = KeyPair::generate(&mut rng);
+
+    let mut vesting_contract = VestingContract {
+        balance: Coin::from_u64_unchecked(1000),
+        owner: Address::from(&key_owner.public),
+        start_time: 0,
+        time_step: u64::MAX,
+        step_amount: Coin::from_u64_unchecked(1),
+        total_amount: Coin::from_u64_unchecked(2000),
+    };
+
+    let accounts = TestCommitRevert::with_initial_state(&[
+        (
+            Address::from(&key_owner.public),
+            Account::Basic(BasicAccount {
+                balance: Coin::from_u64_unchecked(1000),
+            }),
+        ),
+        (
+            Address([1u8; 20]),
+            Account::Vesting(vesting_contract.clone()),
+        ),
+    ]);
+
+    let block_state = BlockState::new(1, 0);
+    let tx = make_signed_transaction(key_owner.clone(), key_recipient.clone(), 1);
+
+    // This panics in can_change_balance during commit_outgoing_transaction.
+    // After the fix, this should return Err(AccountError::InsufficientFunds).
+    let mut tx_logger = TransactionLog::empty();
+    let result = accounts.test_commit_outgoing_transaction(
+        &mut vesting_contract,
+        &tx,
+        &block_state,
+        &mut tx_logger,
+        true,
+    );
+    assert!(
+        result.is_err(),
+        "Expected InsufficientFunds error, got: {:?}",
+        result
+    );
+}
+
 #[test]
 fn can_reserve_balance_after_time_step() {
     // -----------------------------------

primitives/transaction/src/account/vesting_contract.rs

@@ -159,6 +159,12 @@ impl CreationTransactionData {
                     step_amount,
                     total_amount,
                 } = CreationTransactionData32::deserialize_all(data)?;
+                if total_amount > tx_value {
+                    return Err(TransactionError::InvalidData);
+                }
+                if step_amount > total_amount {
+                    return Err(TransactionError::InvalidData);
+                }
                 CreationTransactionData {
                     owner,
                     start_time,

primitives/transaction/tests/vesting_contract_verify.rs

@@ -107,7 +107,7 @@ fn it_can_verify_creation_transaction() {
     transaction.recipient = transaction.contract_creation_address();
     assert_eq!(
         AccountType::verify_incoming_transaction(&transaction),
-        Ok(())
+        Err(TransactionError::InvalidData)
     );
 }