Merge pull request #3104 from AleoHQ/feat/simple-proposal-transmissio…

…n-check

[HackerOne-2310746] Add fee check to transmissions fetched in batch proposal.

node/bft/ledger-service/src/ledger.rs

@@ -191,8 +191,8 @@ impl<N: Network, C: ConsensusStorage<N>> LedgerService<N> for CoreLedgerService<
         }
     }
 
-    /// Ensures the given transmission ID matches the given transmission.
-    fn ensure_transmission_id_matches(
+    /// Ensures that the given transmission is not a fee and matches the given transmission ID.
+    fn ensure_transmission_is_well_formed(
         &self,
         transmission_id: TransmissionID<N>,
         transmission: &mut Transmission<N>,
@@ -202,6 +202,7 @@ impl<N: Network, C: ConsensusStorage<N>> LedgerService<N> for CoreLedgerService<
             (TransmissionID::Transaction(expected_transaction_id), Transmission::Transaction(transaction_data)) => {
                 match transaction_data.clone().deserialize_blocking() {
                     Ok(transaction) => {
+                        // Ensure the transaction ID matches the expected transaction ID.
                         if transaction.id() != expected_transaction_id {
                             bail!(
                                 "Received mismatching transaction ID  - expected {}, found {}",
@@ -210,6 +211,11 @@ impl<N: Network, C: ConsensusStorage<N>> LedgerService<N> for CoreLedgerService<
                             );
                         }
 
+                        // Ensure the transaction is not a fee transaction.
+                        if transaction.is_fee() {
+                            bail!("Received a fee transaction in a transmission");
+                        }
+
                         // Update the transmission with the deserialized transaction.
                         *transaction_data = Data::Object(transaction);
                     }

node/bft/ledger-service/src/mock.rs

@@ -143,8 +143,8 @@ impl<N: Network> LedgerService<N> for MockLedgerService<N> {
         Ok(false)
     }
 
-    /// Ensures the given transmission ID matches the given transmission.
-    fn ensure_transmission_id_matches(
+    /// Ensures that the given transmission is not a fee and matches the given transmission ID.
+    fn ensure_transmission_is_well_formed(
         &self,
         transmission_id: TransmissionID<N>,
         _transmission: &mut Transmission<N>,

node/bft/ledger-service/src/prover.rs

@@ -124,8 +124,8 @@ impl<N: Network> LedgerService<N> for ProverLedgerService<N> {
         bail!("Transmission '{transmission_id}' does not exist in prover")
     }
 
-    /// Ensures the given transmission ID matches the given transmission.
-    fn ensure_transmission_id_matches(
+    /// Ensures that the given transmission is not a fee and matches the given transmission ID.
+    fn ensure_transmission_is_well_formed(
         &self,
         _transmission_id: TransmissionID<N>,
         _transmission: &mut Transmission<N>,

node/bft/ledger-service/src/traits.rs

@@ -78,8 +78,8 @@ pub trait LedgerService<N: Network>: Debug + Send + Sync {
     /// Returns `true` if the ledger contains the given transmission ID.
     fn contains_transmission(&self, transmission_id: &TransmissionID<N>) -> Result<bool>;
 
-    /// Ensures the given transmission ID matches the given transmission.
-    fn ensure_transmission_id_matches(
+    /// Ensures that the given transmission is not a fee and matches the given transmission ID.
+    fn ensure_transmission_is_well_formed(
         &self,
         transmission_id: TransmissionID<N>,
         transmission: &mut Transmission<N>,

node/bft/ledger-service/src/translucent.rs

@@ -135,7 +135,7 @@ impl<N: Network, C: ConsensusStorage<N>> LedgerService<N> for TranslucentLedgerS
     }
 
     /// Always succeeds.
-    fn ensure_transmission_id_matches(
+    fn ensure_transmission_is_well_formed(
         &self,
         _transmission_id: TransmissionID<N>,
         _transmission: &mut Transmission<N>,

node/bft/src/primary.rs

@@ -569,7 +569,16 @@ impl<N: Network> Primary<N> {
         }
 
         // If the peer is ahead, use the batch header to sync up to the peer.
-        let transmissions = self.sync_with_batch_header_from_peer(peer_ip, &batch_header).await?;
+        let mut transmissions = self.sync_with_batch_header_from_peer(peer_ip, &batch_header).await?;
+
+        // Check that the transmission ids match and are not fee transactions.
+        for (transmission_id, transmission) in transmissions.iter_mut() {
+            // If the transmission is not well-formed, then return early.
+            if let Err(err) = self.ledger.ensure_transmission_is_well_formed(*transmission_id, transmission) {
+                debug!("Batch propose from '{peer_ip}' contains an invalid transmission - {err}",);
+                return Ok(());
+            }
+        }
 
         // Ensure the batch is for the current round.
         // This method must be called after fetching previous certificates (above),

node/bft/src/worker.rs

@@ -428,8 +428,8 @@ impl<N: Network> Worker<N> {
         let exists = self.pending.get(transmission_id).unwrap_or_default().contains(&peer_ip);
         // If the peer IP exists, finish the pending request.
         if exists {
-            // Ensure the transmission ID matches the transmission.
-            match self.ledger.ensure_transmission_id_matches(transmission_id, &mut transmission) {
+            // Ensure the transmission is not a fee and matches the transmission ID.
+            match self.ledger.ensure_transmission_is_well_formed(transmission_id, &mut transmission) {
                 Ok(()) => {
                     // Remove the transmission ID from the pending queue.
                     self.pending.remove(transmission_id, Some(transmission));
@@ -516,7 +516,7 @@ mod tests {
             fn get_committee_lookback_for_round(&self, round: u64) -> Result<Committee<N>>;
             fn contains_certificate(&self, certificate_id: &Field<N>) -> Result<bool>;
             fn contains_transmission(&self, transmission_id: &TransmissionID<N>) -> Result<bool>;
-            fn ensure_transmission_id_matches(
+            fn ensure_transmission_is_well_formed(
                 &self,
                 transmission_id: TransmissionID<N>,
                 transmission: &mut Transmission<N>,
@@ -609,7 +609,7 @@ mod tests {
         let mut mock_ledger = MockLedger::default();
         mock_ledger.expect_current_committee().returning(move || Ok(committee.clone()));
         mock_ledger.expect_get_committee_lookback_for_round().returning(move |_| Ok(committee_clone.clone()));
-        mock_ledger.expect_ensure_transmission_id_matches().returning(|_, _| Ok(()));
+        mock_ledger.expect_ensure_transmission_is_well_formed().returning(|_, _| Ok(()));
         let ledger: Arc<dyn LedgerService<CurrentNetwork>> = Arc::new(mock_ledger);
         // Initialize the storage.
         let storage = Storage::<CurrentNetwork>::new(ledger.clone(), Arc::new(BFTMemoryService::new()), 1);