Merge pull request #23 from datachainlab/audit-202411

Audit 202411

Signed-off-by: Jun Kimura <[email protected]>

crates/consensus/src/bls.rs

@@ -195,7 +195,7 @@ impl TryFrom<Signature> for BLSSignature {
     }
 }
 
-pub fn aggreate_public_key(keys: &[BLSPublicKey]) -> Result<BLSAggregatePublicKey, Error> {
+pub fn aggregate_public_key(keys: &[BLSPublicKey]) -> Result<BLSAggregatePublicKey, Error> {
     Ok(BLSAggregatePublicKey::into_aggregate(keys)?)
 }
 
@@ -204,21 +204,21 @@ pub fn fast_aggregate_verify(
     msg: H256,
     signature: BLSSignature,
 ) -> Result<bool, Error> {
-    let aggregate_pubkey = aggreate_public_key(&pubkeys)?;
+    let aggregate_pubkey = aggregate_public_key(&pubkeys)?;
     let aggregate_signature = BLSAggregateSignature::from_signature(&signature);
 
     Ok(aggregate_signature.fast_aggregate_verify_pre_aggregated(msg.as_bytes(), &aggregate_pubkey))
 }
 
-pub fn is_equal_pubkeys_and_aggreate_pub_key<const SYNC_COMMITTEE_SIZE: usize>(
+pub fn is_equal_pubkeys_and_aggregate_pub_key<const SYNC_COMMITTEE_SIZE: usize>(
     pubkeys: &Vector<PublicKey, SYNC_COMMITTEE_SIZE>,
     aggregate_pubkey: &PublicKey,
 ) -> Result<(), Error> {
     let pubkeys: Vec<BLSPublicKey> = pubkeys
         .iter()
         .map(|k| k.clone().try_into().unwrap())
         .collect();
-    let agg_pubkey: PublicKey = aggreate_public_key(&pubkeys)?.into();
+    let agg_pubkey: PublicKey = aggregate_public_key(&pubkeys)?.into();
     if aggregate_pubkey == &agg_pubkey {
         Ok(())
     } else {

crates/consensus/src/errors.rs

@@ -21,7 +21,7 @@ pub enum Error {
     InvalidBLSSignatureLenght(usize, usize),
     /// invalid bls public key length: `expected={0} actual={1}`
     InvalidBLSPublicKeyLength(usize, usize),
-    /// bls aggreate public key mismatch: `{0:?} != {1:?}`
+    /// bls aggregate public key mismatch: `{0:?} != {1:?}`
     BLSAggregatePublicKeyMismatch(PublicKey, PublicKey),
     /// invalid address length: `expected={0} actual={1}`
     InvalidAddressLength(usize, usize),

crates/consensus/src/sync_protocol.rs

@@ -1,5 +1,5 @@
 use crate::{
-    bls::{is_equal_pubkeys_and_aggreate_pub_key, PublicKey, Signature},
+    bls::{is_equal_pubkeys_and_aggregate_pub_key, PublicKey, Signature},
     errors::Error,
     internal_prelude::*,
     types::U64,
@@ -21,7 +21,7 @@ pub struct SyncCommittee<const SYNC_COMMITTEE_SIZE: usize> {
 
 impl<const SYNC_COMMITTEE_SIZE: usize> SyncCommittee<SYNC_COMMITTEE_SIZE> {
     pub fn validate(&self) -> Result<(), Error> {
-        is_equal_pubkeys_and_aggreate_pub_key(&self.pubkeys, &self.aggregate_pubkey)
+        is_equal_pubkeys_and_aggregate_pub_key(&self.pubkeys, &self.aggregate_pubkey)
     }
 }
 

crates/consensus/src/types.rs

@@ -148,7 +148,7 @@ impl<'de> serde::Deserialize<'de> for U64 {
     {
         struct MyVisitor;
 
-        impl<'de> serde::de::Visitor<'de> for MyVisitor {
+        impl serde::de::Visitor<'_> for MyVisitor {
             type Value = U64;
 
             fn expecting(&self, fmt: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {

crates/light-client-cli/src/client.rs

@@ -70,7 +70,8 @@ impl<
             verifier: Default::default(),
             genesis_time,
             genesis_validators_root,
-            trust_level: trust_level.unwrap_or(Fraction::new(2, 3)),
+            // safe to unwrap: `2/3` is valid fraction
+            trust_level: trust_level.unwrap_or(Fraction::new(2, 3).unwrap()),
         }
     }
 

crates/light-client-cli/src/context.rs

@@ -29,8 +29,8 @@ impl<
     pub fn build(network: Network, opts: Opts) -> Result<Self, Error> {
         let home_dir = opts.home_dir();
         if !home_dir.exists() {
-            info!("directory {:?} is created", home_dir);
             std::fs::create_dir(&home_dir)?;
+            info!("directory {:?} is created", home_dir);
         }
         Ok(Self {
             config: network.config(),
@@ -49,7 +49,6 @@ impl<
     }
 
     /// Store accessors
-
     pub fn get_bootstrap(
         &self,
     ) -> Result<

crates/light-client-verifier/src/consensus.rs

@@ -194,9 +194,9 @@ pub fn verify_sync_committee_attestation<
             participants,
             ctx.min_sync_committee_participants(),
         ));
-    } else if participants as u64 * ctx.signature_threshold().denominator
+    } else if participants as u64 * ctx.signature_threshold().denominator()
         < consensus_update.sync_aggregate().sync_committee_bits.len() as u64
-            * ctx.signature_threshold().numerator
+            * ctx.signature_threshold().numerator()
     {
         return Err(Error::InsufficientParticipants(
             participants as u64,
@@ -387,7 +387,7 @@ pub mod test_utils {
     use ethereum_consensus::ssz_rs::Vector;
     use ethereum_consensus::{
         beacon::{BlockNumber, Checkpoint, Epoch, Slot},
-        bls::{aggreate_public_key, PublicKey, Signature},
+        bls::{aggregate_public_key, PublicKey, Signature},
         fork::deneb,
         merkle::MerkleTree,
         preset::minimal::DenebBeaconBlock,
@@ -441,7 +441,7 @@ pub mod test_utils {
             for v in self.committee.iter() {
                 pubkeys.push(v.public_key());
             }
-            let aggregate_pubkey = aggreate_public_key(&pubkeys.to_vec()).unwrap();
+            let aggregate_pubkey = aggregate_public_key(&pubkeys.to_vec()).unwrap();
             SyncCommittee {
                 pubkeys: Vector::from_iter(pubkeys.into_iter().map(PublicKey::from)),
                 aggregate_pubkey: PublicKey::from(aggregate_pubkey),
@@ -855,7 +855,7 @@ mod tests {
         };
         use ethereum_consensus::{
             beacon::Version,
-            bls::aggreate_public_key,
+            bls::aggregate_public_key,
             config::{minimal, Config},
             fork::{
                 altair::ALTAIR_FORK_SPEC, bellatrix::BELLATRIX_FORK_SPEC, ForkParameter,
@@ -881,7 +881,7 @@ mod tests {
                 genesis_validators_root,
                 // NOTE: this is workaround. we must get the correct timestamp from beacon state.
                 minimal::get_config().min_genesis_time,
-                Fraction::new(2, 3),
+                Fraction::new(2, 3).unwrap(),
                 1729846322.into(),
             );
             assert!(verifier.validate_boostrap(&ctx, &bootstrap, None).is_ok());
@@ -897,7 +897,7 @@ mod tests {
                 .iter()
                 .map(|k| k.clone().try_into().unwrap())
                 .collect();
-            let aggregated_key = aggreate_public_key(&pubkeys).unwrap();
+            let aggregated_key = aggregate_public_key(&pubkeys).unwrap();
             let pubkey = BLSPublicKey {
                 point: aggregated_key.point,
             };
@@ -928,7 +928,7 @@ mod tests {
                 genesis_validators_root,
                 // NOTE: this is workaround. we must get the correct timestamp from beacon state.
                 minimal::get_config().min_genesis_time,
-                Fraction::new(2, 3),
+                Fraction::new(2, 3).unwrap(),
                 1729846322.into(),
             );
             assert!(verifier.validate_boostrap(&ctx, &bootstrap, None).is_ok());
@@ -1039,7 +1039,7 @@ mod tests {
                 config::minimal::get_config(),
                 Default::default(),
                 Default::default(),
-                Fraction::new(2, 3),
+                Fraction::new(2, 3).unwrap(),
                 SystemTime::now()
                     .duration_since(SystemTime::UNIX_EPOCH)
                     .unwrap()
@@ -1458,7 +1458,7 @@ mod tests {
                 config::minimal::get_config(),
                 Default::default(),
                 Default::default(),
-                Fraction::new(2, 3),
+                Fraction::new(2, 3).unwrap(),
                 SystemTime::now()
                     .duration_since(SystemTime::UNIX_EPOCH)
                     .unwrap()
@@ -1779,7 +1779,7 @@ mod tests {
                         .as_ref(),
                 ),
                 1731420304.into(),
-                Fraction::new(2, 3),
+                Fraction::new(2, 3).unwrap(),
                 SystemTime::now()
                     .duration_since(SystemTime::UNIX_EPOCH)
                     .unwrap()
@@ -1806,7 +1806,7 @@ mod tests {
                         .as_ref(),
                 ),
                 1731420304.into(),
-                Fraction::new(2, 3),
+                Fraction::new(2, 3).unwrap(),
                 SystemTime::now()
                     .duration_since(SystemTime::UNIX_EPOCH)
                     .unwrap()
@@ -1971,7 +1971,7 @@ mod tests {
                         .as_ref(),
                 ),
                 1731420304.into(),
-                Fraction::new(2, 3),
+                Fraction::new(2, 3).unwrap(),
                 SystemTime::now()
                     .duration_since(SystemTime::UNIX_EPOCH)
                     .unwrap()

crates/light-client-verifier/src/context.rs

@@ -1,3 +1,4 @@
+use crate::errors::Error;
 use ethereum_consensus::{
     beacon::{Epoch, Root, Slot},
     compute::{compute_epoch_at_slot, compute_slot_at_timestamp},
@@ -6,19 +7,36 @@ use ethereum_consensus::{
     fork::{ForkParameters, ForkSpec},
     types::U64,
 };
+
+/// Fraction is a struct representing a fraction for a threshold.
 #[derive(Clone, Default, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
 pub struct Fraction {
-    pub numerator: u64,
-    pub denominator: u64,
+    numerator: u64,
+    denominator: u64,
 }
 
 impl Fraction {
-    pub fn new(numerator: u64, denominator: u64) -> Self {
-        Self {
-            numerator,
-            denominator,
+    pub fn new(numerator: u64, denominator: u64) -> Result<Self, Error> {
+        if denominator == 0 || numerator > denominator {
+            Err(Error::InvalidFraction(Self {
+                numerator,
+                denominator,
+            }))
+        } else {
+            Ok(Self {
+                numerator,
+                denominator,
+            })
         }
     }
+
+    pub fn numerator(&self) -> u64 {
+        self.numerator
+    }
+
+    pub fn denominator(&self) -> u64 {
+        self.denominator
+    }
 }
 
 pub trait ConsensusVerificationContext {

crates/light-client-verifier/src/errors.rs

@@ -1,4 +1,4 @@
-use crate::internal_prelude::*;
+use crate::{context::Fraction, internal_prelude::*};
 use displaydoc::Display;
 use ethereum_consensus::{
     beacon::{BeaconBlockHeader, Epoch, Root, Slot},
@@ -91,6 +91,8 @@ pub enum Error {
     InvalidExecutionBlockNumberMerkleBranch(MerkleError),
     /// inconsistent next sync committee: `store:{0:?}` != `update:{1:?}`
     InconsistentNextSyncCommittee(PublicKey, PublicKey),
+    /// invalid fraction: `fraction={0:?}`
+    InvalidFraction(Fraction),
     /// other error: `{description}`
     Other { description: String },
 }

crates/light-client-verifier/src/updates.rs

@@ -77,7 +77,7 @@ pub trait ConsensusUpdate<const SYNC_COMMITTEE_SIZE: usize>:
 
     /// validate the basic properties of the update
     fn validate_basic<C: ConsensusVerificationContext>(&self, ctx: &C) -> Result<(), Error> {
-        // ensure that sync committee's aggreated key matches pubkeys
+        // ensure that sync committee's aggregated key matches pubkeys
         if let Some(next_sync_committee) = self.next_sync_committee() {
             next_sync_committee.validate()?;
         }

crates/lodestar-rpc/src/client.rs

@@ -8,7 +8,7 @@ use ethereum_consensus::beacon::Slot;
 use ethereum_consensus::sync_protocol::SyncCommitteePeriod;
 use ethereum_consensus::types::H256;
 use log::debug;
-use reqwest::{Client, StatusCode};
+use reqwest::{Client, StatusCode, Url};
 use serde::de::DeserializeOwned;
 
 type Result<T> = core::result::Result<T, Error>;
@@ -20,9 +20,25 @@ pub struct RPCClient {
 
 impl RPCClient {
     pub fn new(endpoint: impl Into<String>) -> Self {
+        let url = Url::parse(&endpoint.into()).expect("Invalid URL");
+        if url.scheme() != "http" && url.scheme() != "https" {
+            panic!("Invalid URL scheme: {}", url.scheme());
+        }
+        if url.path() != "/" {
+            panic!("Invalid URL path: {}", url.path());
+        }
+        if url.host().is_none() {
+            panic!("Invalid URL host: {}", url.host().unwrap());
+        }
+        if url.query().is_some() {
+            panic!("Invalid URL query: {}", url.query().unwrap());
+        }
+        if url.fragment().is_some() {
+            panic!("Invalid URL fragment: {}", url.fragment().unwrap());
+        }
         Self {
             http_client: reqwest::Client::new(),
-            endpoint: endpoint.into(),
+            endpoint: url.as_str().strip_suffix("/").unwrap().to_string(),
         }
     }
 
@@ -168,8 +184,8 @@ impl RPCClient {
 
 #[derive(serde::Serialize, serde::Deserialize)]
 struct InternalServerError {
-    #[serde(rename = "statusCode")]
+    #[serde(alias = "statusCode", alias = "code")]
     status_code: u64,
-    error: String,
+    error: Option<String>,
     message: String,
 }