fix(core)!: fix potential panic for sidechain merkle root with incorr…

…ect length (#3788)

Description
---
Fixes potential panic if a side-chain Merkle root of incorrect length is accepted by the blockchain.

Motivation and Context
---
Using a fixed 32-byte array forces any implementor to produce or validate the correct number of bytes. Esp. now that const generics are available, for performance and correctness, all hashes in the system should be fixed to 32 bytes and not held in a dynamic heap allocated vector.

I've changed the merkle_root type in SideChainCheckpoint because this is a relatively minor change.

This is a chain storage breaking change. 

How Has This Been Tested?
---
Existing tests, manually

applications/tari_app_grpc/src/conversions/output_features.rs

@@ -22,7 +22,10 @@
 
 use std::convert::{TryFrom, TryInto};
 
-use tari_common_types::types::{Commitment, PublicKey};
+use tari_common_types::{
+    array::copy_into_fixed_array,
+    types::{Commitment, PublicKey},
+};
 use tari_core::transactions::transaction::{
     AssetOutputFeatures,
     MintNonFungibleFeatures,
@@ -176,10 +179,8 @@ impl TryFrom<grpc::SideChainCheckpointFeatures> for SideChainCheckpointFeatures
                 PublicKey::from_bytes(c).map_err(|err| format!("committee member was not a valid public key: {}", err))
             })
             .collect::<Result<_, _>>()?;
+        let merkle_root = copy_into_fixed_array(&value.merkle_root).map_err(|_| "Invalid merkle_root length")?;
 
-        Ok(Self {
-            merkle_root: value.merkle_root.as_bytes().to_vec(),
-            committee,
-        })
+        Ok(Self { merkle_root, committee })
     }
 }

applications/tari_console_wallet/src/automation/commands.rs

@@ -35,7 +35,7 @@ use log::*;
 use sha2::Sha256;
 use strum_macros::{Display, EnumIter, EnumString};
 use tari_common::GlobalConfig;
-use tari_common_types::{emoji::EmojiId, transaction::TxId, types::PublicKey};
+use tari_common_types::{array::copy_into_fixed_array, emoji::EmojiId, transaction::TxId, types::PublicKey};
 use tari_comms::{
     connectivity::{ConnectivityEvent, ConnectivityRequester},
     multiaddr::Multiaddr,
@@ -818,18 +818,17 @@ pub async fn command_runner(
                     _ => Err(CommandError::Argument),
                 }?;
 
-                let unique_ids: Vec<Vec<u8>> = parsed.args[1..]
+                let unique_ids = parsed.args[1..]
                     .iter()
                     .map(|arg| {
                         let s = arg.to_string();
                         if let Some(s) = s.strip_prefix("0x") {
-                            let r: Vec<u8> = Hex::from_hex(s).unwrap();
-                            r
+                            Hex::from_hex(s).map_err(|_| CommandError::Argument)
                         } else {
-                            s.into_bytes()
+                            Ok(s.into_bytes())
                         }
                     })
-                    .collect();
+                    .collect::<Result<Vec<Vec<u8>>, _>>()?;
 
                 let mut asset_manager = wallet.asset_manager.clone();
                 let asset = asset_manager.get_owned_asset_by_pub_key(&public_key).await?;
@@ -856,18 +855,14 @@ pub async fn command_runner(
 
                 let merkle_root = match parsed.args[1] {
                     ParsedArgument::Text(ref root) => {
-                        let s = root.to_string();
-                        match &s[0..2] {
-                            "0x" => {
-                                let s = s[2..].to_string();
-                                let r: Vec<u8> = Hex::from_hex(&s).unwrap();
-                                Ok(r)
-                            },
-                            _ => Ok(s.into_bytes()),
-                        }
+                        let bytes = match &root[0..2] {
+                            "0x" => Vec::<u8>::from_hex(&root[2..]).map_err(|_| CommandError::Argument)?,
+                            _ => Vec::<u8>::from_hex(root).map_err(|_| CommandError::Argument)?,
+                        };
+                        copy_into_fixed_array(&bytes).map_err(|_| CommandError::Argument)?
                     },
-                    _ => Err(CommandError::Argument),
-                }?;
+                    _ => return Err(CommandError::Argument),
+                };
 
                 let committee: Vec<PublicKey> = parsed.args[2..]
                     .iter()
@@ -879,7 +874,7 @@ pub async fn command_runner(
 
                 let mut asset_manager = wallet.asset_manager.clone();
                 let (tx_id, transaction) = asset_manager
-                    .create_initial_asset_checkpoint(&public_key, &merkle_root, &committee)
+                    .create_initial_asset_checkpoint(&public_key, merkle_root, &committee)
                     .await?;
                 let _result = transaction_service
                     .submit_transaction(

applications/tari_console_wallet/src/grpc/wallet_grpc_server.rs

@@ -71,7 +71,10 @@ use tari_app_grpc::{
         TransferResult,
     },
 };
-use tari_common_types::types::{BlockHash, PublicKey, Signature};
+use tari_common_types::{
+    array::copy_into_fixed_array,
+    types::{BlockHash, PublicKey, Signature},
+};
 use tari_comms::{types::CommsPublicKey, CommsNode};
 use tari_core::transactions::{
     tari_amount::MicroTari,
@@ -655,12 +658,11 @@ impl wallet_server::Wallet for WalletGrpcServer {
             .collect::<Result<_, _>>()
             .map_err(|err| Status::invalid_argument(format!("Committee did not contain valid pub keys:{}", err)))?;
 
+        let merkle_root = copy_into_fixed_array(&message.merkle_root)
+            .map_err(|_| Status::invalid_argument("Merkle root has an incorrect length"))?;
+
         let (tx_id, transaction) = asset_manager
-            .create_initial_asset_checkpoint(
-                &asset_public_key,
-                message.merkle_root.as_slice(),
-                committee_public_keys.as_slice(),
-            )
+            .create_initial_asset_checkpoint(&asset_public_key, merkle_root, committee_public_keys.as_slice())
             .await
             .map_err(|e| Status::internal(e.to_string()))?;
 
@@ -691,11 +693,14 @@ impl wallet_server::Wallet for WalletGrpcServer {
                 Status::invalid_argument(format!("Next committee did not contain valid pub keys:{}", err))
             })?;
 
+        let merkle_root = copy_into_fixed_array(&message.merkle_root)
+            .map_err(|_| Status::invalid_argument("Incorrect merkle root length"))?;
+
         let (tx_id, transaction) = asset_manager
             .create_follow_on_asset_checkpoint(
                 &asset_public_key,
                 message.unique_id.as_slice(),
-                message.merkle_root.as_slice(),
+                merkle_root,
                 committee_public_keys.as_slice(),
             )
             .await

base_layer/common_types/Cargo.toml

@@ -7,10 +7,12 @@ version = "0.27.3"
 edition = "2018"
 
 [dependencies]
-rand = "0.8"
 tari_crypto = { git = "https://github.com/tari-project/tari-crypto.git", branch = "main" }
-serde = { version = "1.0.106", features = ["derive"] }
-tokio = { version = "1.11", features = ["time", "sync"] }
-lazy_static = "1.4.0"
+tari_utilities = "^0.3"
+
 digest = "0.9.0"
+lazy_static = "1.4.0"
+rand = "0.8"
+serde = { version = "1.0.106", features = ["derive"] }
 thiserror = "1.0.29"
+tokio = { version = "1.11", features = ["time", "sync"] }

base_layer/common_types/src/array.rs

@@ -0,0 +1,32 @@
+//  Copyright 2022, The Tari Project
+//
+//  Redistribution and use in source and binary forms, with or without modification, are permitted provided that the
+//  following conditions are met:
+//
+//  1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following
+//  disclaimer.
+//
+//  2. Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the
+//  following disclaimer in the documentation and/or other materials provided with the distribution.
+//
+//  3. Neither the name of the copyright holder nor the names of its contributors may be used to endorse or promote
+//  products derived from this software without specific prior written permission.
+//
+//  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+//  INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+//  DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+//  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+//  SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+//  WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
+//  USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+use tari_utilities::ByteArrayError;
+
+pub fn copy_into_fixed_array<T: Default + Copy, const SZ: usize>(elems: &[T]) -> Result<[T; SZ], ByteArrayError> {
+    if elems.len() != SZ {
+        return Err(ByteArrayError::IncorrectLength);
+    }
+    let mut buf = [T::default(); SZ];
+    buf.copy_from_slice(&elems[0..SZ]);
+    Ok(buf)
+}

base_layer/common_types/src/lib.rs

@@ -20,6 +20,7 @@
 // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
 // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+pub mod array;
 pub mod chain_metadata;
 pub mod emoji;
 pub mod luhn;

base_layer/common_types/src/types/mod.rs

@@ -38,6 +38,8 @@ use tari_crypto::{
 pub const BLOCK_HASH_LENGTH: usize = 32;
 pub type BlockHash = Vec<u8>;
 
+pub type FixedHash = [u8; BLOCK_HASH_LENGTH];
+
 /// Define the explicit Signature implementation for the Tari base layer. A different signature scheme can be
 /// employed by redefining this type.
 pub type Signature = RistrettoSchnorr;

base_layer/core/src/proto/transaction.rs

@@ -27,7 +27,7 @@ use std::{
     sync::Arc,
 };
 
-use tari_common_types::types::{BlindingFactor, BulletRangeProof, Commitment, PublicKey};
+use tari_common_types::types::{BlindingFactor, BulletRangeProof, Commitment, PublicKey, BLOCK_HASH_LENGTH};
 use tari_crypto::{
     script::{ExecutionStack, TariScript},
     tari_utilities::{ByteArray, ByteArrayError},
@@ -400,7 +400,14 @@ impl TryFrom<proto::types::SideChainCheckpointFeatures> for SideChainCheckpointF
     type Error = String;
 
     fn try_from(value: proto::types::SideChainCheckpointFeatures) -> Result<Self, Self::Error> {
-        let merkle_root = value.merkle_root.as_bytes().to_vec();
+        if value.merkle_root.len() != BLOCK_HASH_LENGTH {
+            return Err(format!(
+                "Invalid side chain checkpoint merkle length {}",
+                value.merkle_root.len()
+            ));
+        }
+        let mut merkle_root = [0u8; BLOCK_HASH_LENGTH];
+        merkle_root.copy_from_slice(&value.merkle_root[0..BLOCK_HASH_LENGTH]);
         let committee = value
             .committee
             .into_iter()

base_layer/core/src/transactions/transaction/output_features.rs

@@ -29,7 +29,7 @@ use std::{
 };
 
 use serde::{Deserialize, Serialize};
-use tari_common_types::types::{Commitment, PublicKey};
+use tari_common_types::types::{Commitment, FixedHash, PublicKey};
 use tari_utilities::ByteArray;
 
 use super::OutputFeaturesVersion;
@@ -181,7 +181,7 @@ impl OutputFeatures {
     pub fn for_checkpoint(
         parent_public_key: PublicKey,
         unique_id: Vec<u8>,
-        merkle_root: Vec<u8>,
+        merkle_root: FixedHash,
         committee: Vec<PublicKey>,
         is_initial: bool,
     ) -> OutputFeatures {
@@ -321,7 +321,7 @@ mod test {
                 asset_owner_commitment: Default::default(),
             }),
             sidechain_checkpoint: Some(SideChainCheckpointFeatures {
-                merkle_root: vec![1u8; 32],
+                merkle_root: [1u8; 32],
                 committee: iter::repeat_with(PublicKey::default).take(50).collect(),
             }),
         }

base_layer/core/src/transactions/transaction/side_chain_checkpoint_features.rs

@@ -20,28 +20,24 @@
 //  WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
 //  USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-use std::io::{Error, ErrorKind, Read, Write};
+use std::io::{Error, Read, Write};
 
 use integer_encoding::VarInt;
 use serde::{Deserialize, Serialize};
-use tari_common_types::types::PublicKey;
+use tari_common_types::types::{FixedHash, PublicKey};
 use tari_crypto::keys::PublicKey as PublicKeyTrait;
 
 use crate::consensus::{ConsensusDecoding, ConsensusEncoding, ConsensusEncodingSized, MaxSizeVec};
 
 #[derive(Debug, Clone, Hash, PartialEq, Deserialize, Serialize, Eq)]
 pub struct SideChainCheckpointFeatures {
-    // TODO: This should be fixed size [u8;32]
-    pub merkle_root: Vec<u8>,
+    pub merkle_root: FixedHash,
     pub committee: Vec<PublicKey>,
 }
 
 impl ConsensusEncoding for SideChainCheckpointFeatures {
     fn consensus_encode<W: Write>(&self, writer: &mut W) -> Result<usize, Error> {
-        if self.merkle_root.len() != 32 {
-            return Err(Error::new(ErrorKind::InvalidInput, "merkle_root must be 32 bytes"));
-        }
-        writer.write_all(&self.merkle_root[0..32])?;
+        self.merkle_root.consensus_encode(writer)?;
         let mut written = 32;
         written += self.committee.consensus_encode(writer)?;
         Ok(written)
@@ -56,7 +52,7 @@ impl ConsensusEncodingSized for SideChainCheckpointFeatures {
 
 impl ConsensusDecoding for SideChainCheckpointFeatures {
     fn consensus_decode<R: Read>(reader: &mut R) -> Result<Self, Error> {
-        let merkle_root = <[u8; 32] as ConsensusDecoding>::consensus_decode(reader)?.to_vec();
+        let merkle_root = FixedHash::consensus_decode(reader)?;
 
         const MAX_COMMITTEE_KEYS: usize = 50;
         let committee = MaxSizeVec::<PublicKey, MAX_COMMITTEE_KEYS>::consensus_decode(reader)?;
@@ -78,7 +74,7 @@ mod test {
     #[test]
     fn it_encodes_and_decodes_correctly() {
         let subject = SideChainCheckpointFeatures {
-            merkle_root: vec![1u8; 32],
+            merkle_root: [1u8; 32],
             committee: iter::repeat_with(PublicKey::default).take(50).collect(),
         };
 
@@ -88,22 +84,11 @@ mod test {
     #[test]
     fn it_fails_for_too_many_committee_pks() {
         let subject = SideChainCheckpointFeatures {
-            merkle_root: vec![1u8; 32],
+            merkle_root: [1u8; 32],
             committee: iter::repeat_with(PublicKey::default).take(51).collect(),
         };
 
         let err = check_consensus_encoding_correctness(subject).unwrap_err();
         assert_eq!(err.kind(), ErrorKind::InvalidInput);
     }
-
-    #[test]
-    fn it_fails_for_incorrect_merkle_root_length() {
-        let subject = SideChainCheckpointFeatures {
-            merkle_root: vec![1u8; 31],
-            committee: vec![],
-        };
-
-        let err = check_consensus_encoding_correctness(subject).unwrap_err();
-        assert_eq!(err.kind(), ErrorKind::InvalidInput);
-    }
 }

base_layer/wallet/src/assets/asset_manager.rs

@@ -23,7 +23,7 @@
 use log::*;
 use tari_common_types::{
     transaction::TxId,
-    types::{Commitment, PublicKey},
+    types::{Commitment, FixedHash, PublicKey},
 };
 use tari_core::transactions::transaction::{OutputFeatures, OutputFlags, TemplateParameter, Transaction};
 
@@ -151,7 +151,7 @@ impl<T: OutputManagerBackend + 'static> AssetManager<T> {
     pub async fn create_initial_asset_checkpoint(
         &mut self,
         asset_pub_key: PublicKey,
-        merkle_root: Vec<u8>,
+        merkle_root: FixedHash,
         committee_pub_keys: Vec<PublicKey>,
     ) -> Result<(TxId, Transaction), WalletError> {
         let output = self
@@ -199,7 +199,7 @@ impl<T: OutputManagerBackend + 'static> AssetManager<T> {
         &mut self,
         asset_pub_key: PublicKey,
         unique_id: Vec<u8>,
-        merkle_root: Vec<u8>,
+        merkle_root: FixedHash,
         committee_pub_keys: Vec<PublicKey>,
     ) -> Result<(TxId, Transaction), WalletError> {
         let output = self