From ace1103462dafa65c4b95dfce81f2796d9903c9a Mon Sep 17 00:00:00 2001
From: Henry de Valence <hdevalence@hdevalence.ca>
Date: Thu, 19 Nov 2020 19:13:52 -0800
Subject: [PATCH] consensus: fix bug in tx input/output presence check

Making this check's match statement exhaustive revealed a bug similar to
the previous commit.  The logic in the spec is written in terms of
numbers, but our data is internally represented in terms of enums
(ADTs).  This kind of cross-representation rule translation is a bug
surface, which we can avoid by converting to counts and summing up.  (We
should use one style at a time).
---
 zebra-consensus/src/error.rs             |  7 +++-
 zebra-consensus/src/transaction.rs       |  2 +-
 zebra-consensus/src/transaction/check.rs | 48 ++++++++++++++++++------
 3 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs
index 2f5458d58..caa2f2edb 100644
--- a/zebra-consensus/src/error.rs
+++ b/zebra-consensus/src/error.rs
@@ -43,8 +43,11 @@ pub enum TransactionError {
     #[error("transaction version number MUST be >= 4")]
     WrongVersion,
 
-    #[error("at least one of tx_in_count, nShieldedSpend, and nJoinSplit MUST be nonzero")]
-    NoTransfer,
+    #[error("must have at least one input: transparent, shielded spend, or joinsplit")]
+    NoInputs,
+
+    #[error("must have at least one output: transparent, shielded output, or joinsplit")]
+    NoOutputs,
 
     #[error("if there are no Spends or Outputs, the value balance MUST be 0.")]
     BadBalance,
diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs
index b3fe435bb..ef2f5377a 100644
--- a/zebra-consensus/src/transaction.rs
+++ b/zebra-consensus/src/transaction.rs
@@ -130,7 +130,7 @@ where
                         }
                     }
 
-                    check::some_money_is_spent(&tx)?;
+                    check::has_inputs_and_outputs(&tx)?;
                     check::any_coinbase_inputs_no_transparent_outputs(&tx)?;
 
                     let sighash = tx.sighash(
diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs
index 9761d4454..3060220f5 100644
--- a/zebra-consensus/src/transaction/check.rs
+++ b/zebra-consensus/src/transaction/check.rs
@@ -25,28 +25,52 @@ pub fn validate_joinsplit_sig(
         .map_err(TransactionError::Ed25519)
 }
 
-/// Check that at least one of tx_in_count, nShieldedSpend, and nJoinSplit MUST
-/// be non-zero.
+/// Checks that the transaction has inputs and outputs.
+///
+/// More specifically:
+///
+/// * at least one of tx_in_count, nShieldedSpend, and nJoinSplit MUST be non-zero.
+/// * at least one of tx_out_count, nShieldedOutput, and nJoinSplit MUST be non-zero.
 ///
 /// https://zips.z.cash/protocol/canopy.pdf#txnencodingandconsensus
-pub fn some_money_is_spent(tx: &Transaction) -> Result<(), TransactionError> {
+pub fn has_inputs_and_outputs(tx: &Transaction) -> Result<(), TransactionError> {
+    // The consensus rule is written in terms of numbers, but our transactions
+    // hold enum'd data. Mixing pattern matching and numerical checks is risky,
+    // so convert everything to counts and sum up.
     match tx {
         Transaction::V4 {
             inputs,
-            joinsplit_data: Some(joinsplit_data),
-            shielded_data: Some(shielded_data),
+            outputs,
+            joinsplit_data,
+            shielded_data,
             ..
         } => {
-            if !inputs.is_empty()
-                || joinsplit_data.joinsplits().count() > 0
-                || shielded_data.spends().count() > 0
-            {
-                Ok(())
+            let tx_in_count = inputs.len();
+            let tx_out_count = outputs.len();
+            let n_joinsplit = joinsplit_data
+                .as_ref()
+                .map(|d| d.joinsplits().count())
+                .unwrap_or(0);
+            let n_shielded_spend = shielded_data
+                .as_ref()
+                .map(|d| d.spends().count())
+                .unwrap_or(0);
+            let n_shielded_output = shielded_data
+                .as_ref()
+                .map(|d| d.outputs().count())
+                .unwrap_or(0);
+
+            if tx_in_count + n_shielded_spend + n_joinsplit == 0 {
+                Err(TransactionError::NoInputs)
+            } else if tx_out_count + n_shielded_output + n_joinsplit == 0 {
+                Err(TransactionError::NoOutputs)
             } else {
-                Err(TransactionError::NoTransfer)
+                Ok(())
             }
         }
-        _ => Err(TransactionError::WrongVersion),
+        Transaction::V1 { .. } | Transaction::V2 { .. } | Transaction::V3 { .. } => {
+            unreachable!("tx version is checked first")
+        }
     }
 }