chore(l2): forbid index slicing (#1344)

**Motivation**

We should not use direct indexing `var[idx]` or direct index slicing
`var[0..2]` and instead use safe alternatives and handle the errors.

**Description**

- `indexing_slicing = "deny"` was added to `Cargo.toml` to forbid index
slicing usage.
- All usages were removed in the `l2` crate

<!-- Link to issues: Resolves #111, Resolves #222 -->

Closes #1270

cmd/ethrex_l2/src/commands/wallet.rs

@@ -245,6 +245,12 @@ async fn get_withdraw_merkle_proof(
             .collect(),
         tx_withdrawal_hash,
     )
+    .map_err(|err| {
+        eyre::eyre!(
+            "Failed to generate merkle proof in get_withdraw_merkle_proof: {:?}",
+            err
+        )
+    })?
     .ok_or_eyre("Transaction's WithdrawalData is not in block's WithdrawalDataMerkleRoot")?;
 
     Ok((index, path))

crates/l2/Cargo.toml

@@ -42,3 +42,4 @@ path = "./l2.rs"
 [lints.clippy]
 unwrap_used = "deny"
 expect_used = "deny"
+indexing_slicing = "deny"

crates/l2/contracts/Cargo.toml

@@ -28,3 +28,4 @@ path = "./deployer.rs"
 [lints.clippy]
 unwrap_used = "deny"
 expect_used = "deny"
+indexing_slicing = "deny"

crates/l2/proposer/errors.rs

@@ -1,5 +1,6 @@
 use std::sync::mpsc::SendError;
 
+use crate::utils::merkle_tree::MerkleError;
 use crate::utils::{config::errors::ConfigError, eth_client::errors::EthClientError};
 use ethereum_types::FromStrRadixErr;
 use ethrex_core::types::BlobsBundleError;
@@ -94,6 +95,10 @@ pub enum CommitterError {
     FailedToReExecuteBlock(#[from] EvmError),
     #[error("Committer failed to send transaction: {0}")]
     FailedToSendCommitment(String),
+    #[error("Committer failed to decode deposit hash")]
+    FailedToDecodeDepositHash,
+    #[error("Committer failed to merkelize: {0}")]
+    FailedToMerkelize(#[from] MerkleError),
     #[error("Withdrawal transaction was invalid")]
     InvalidWithdrawalTransaction,
     #[error("Blob estimation failed: {0}")]

crates/l2/proposer/l1_committer.rs

@@ -117,13 +117,13 @@ impl Committer {
                 }
 
                 let withdrawal_logs_merkle_root =
-                    self.get_withdrawals_merkle_root(withdrawal_hashes);
+                    self.get_withdrawals_merkle_root(withdrawal_hashes)?;
                 let deposit_logs_hash = self.get_deposit_hash(
                     deposits
                         .iter()
                         .filter_map(|tx| tx.get_deposit_hash())
                         .collect(),
-                );
+                )?;
 
                 let state_diff = self.prepare_state_diff(
                     &block_to_commit,
@@ -180,11 +180,14 @@ impl Committer {
         Ok(withdrawals)
     }
 
-    pub fn get_withdrawals_merkle_root(&self, withdrawals_hashes: Vec<H256>) -> H256 {
+    pub fn get_withdrawals_merkle_root(
+        &self,
+        withdrawals_hashes: Vec<H256>,
+    ) -> Result<H256, CommitterError> {
         if !withdrawals_hashes.is_empty() {
-            merkelize(withdrawals_hashes)
+            merkelize(withdrawals_hashes).map_err(CommitterError::FailedToMerkelize)
         } else {
-            H256::zero()
+            Ok(H256::zero())
         }
     }
 
@@ -206,25 +209,27 @@ impl Committer {
         deposits
     }
 
-    pub fn get_deposit_hash(&self, deposit_hashes: Vec<H256>) -> H256 {
+    pub fn get_deposit_hash(&self, deposit_hashes: Vec<H256>) -> Result<H256, CommitterError> {
         if !deposit_hashes.is_empty() {
-            H256::from_slice(
+            Ok(H256::from_slice(
                 [
                     &(deposit_hashes.len() as u16).to_be_bytes(),
-                    &keccak(
+                    keccak(
                         deposit_hashes
                             .iter()
                             .map(H256::as_bytes)
                             .collect::<Vec<&[u8]>>()
                             .concat(),
                     )
-                    .as_bytes()[2..32],
+                    .as_bytes()
+                    .get(2..32)
+                    .ok_or(CommitterError::FailedToDecodeDepositHash)?,
                 ]
                 .concat()
                 .as_slice(),
-            )
+            ))
         } else {
-            H256::zero()
+            Ok(H256::zero())
         }
     }
     /// Prepare the state diff for the block.

crates/l2/proposer/l1_watcher.rs

@@ -79,14 +79,17 @@ impl L1Watcher {
 
     pub async fn get_pending_deposit_logs(&self) -> Result<Vec<H256>, L1WatcherError> {
         Ok(hex::decode(
-            &self
-                .eth_client
+            self.eth_client
                 .call(
                     self.address,
                     Bytes::copy_from_slice(&[0x35, 0x6d, 0xa2, 0x49]),
                     Overrides::default(),
                 )
-                .await?[2..],
+                .await?
+                .get(2..)
+                .ok_or(L1WatcherError::FailedToDeserializeLog(
+                    "Not a valid hex string".to_string(),
+                ))?,
         )
         .map_err(|_| L1WatcherError::FailedToDeserializeLog("Not a valid hex string".to_string()))?
         .chunks(32)
@@ -112,22 +115,28 @@ impl L1Watcher {
             self.last_block_fetched, new_last_block
         );
 
-        let logs = match self
-            .eth_client
-            .get_logs(
-                self.last_block_fetched + 1,
-                new_last_block,
-                self.address,
-                self.topics[0],
-            )
-            .await
-        {
-            Ok(logs) => logs,
-            Err(error) => {
-                warn!("Error when getting logs from L1: {}", error);
-                vec![]
-            }
-        };
+        let logs;
+        if let Some(first_topic) = self.topics.first() {
+            logs = match self
+                .eth_client
+                .get_logs(
+                    self.last_block_fetched + 1,
+                    new_last_block,
+                    self.address,
+                    *first_topic,
+                )
+                .await
+            {
+                Ok(logs) => logs,
+                Err(error) => {
+                    warn!("Error when getting logs from L1: {error}");
+                    vec![]
+                }
+            };
+        } else {
+            warn!("Error when getting logs from L1: topics vector is empty");
+            logs = vec![];
+        }
 
         debug!("Logs: {:#?}", logs);
 
@@ -158,14 +167,31 @@ impl L1Watcher {
             .unwrap_or_default();
 
         for log in logs {
-            let mint_value = format!("{:#x}", log.log.topics[1])
-                .parse::<U256>()
-                .map_err(|e| {
-                    L1WatcherError::FailedToDeserializeLog(format!(
-                        "Failed to parse mint value from log: {e:#?}"
-                    ))
-                })?;
-            let beneficiary = format!("{:#x}", log.log.topics[2].into_uint())
+            let mint_value = format!(
+                "{:#x}",
+                log.log
+                    .topics
+                    .get(1)
+                    .ok_or(L1WatcherError::FailedToDeserializeLog(
+                        "Failed to parse mint value from log: log.topics[1] out of bounds"
+                            .to_owned()
+                    ))?
+            )
+            .parse::<U256>()
+            .map_err(|e| {
+                L1WatcherError::FailedToDeserializeLog(format!(
+                    "Failed to parse mint value from log: {e:#?}"
+                ))
+            })?;
+            let beneficiary_uint = log
+                .log
+                .topics
+                .get(2)
+                .ok_or(L1WatcherError::FailedToDeserializeLog(
+                    "Failed to parse beneficiary from log: log.topics[2] out of bounds".to_owned(),
+                ))?
+                .into_uint();
+            let beneficiary = format!("{beneficiary_uint:#x}")
                 .parse::<Address>()
                 .map_err(|e| {
                     L1WatcherError::FailedToDeserializeLog(format!(

crates/l2/prover/Cargo.toml

@@ -54,3 +54,4 @@ gpu = ["risc0-zkvm/cuda"]
 [lints.clippy]
 unwrap_used = "deny"
 expect_used = "deny"
+indexing_slicing = "deny"

crates/l2/sdk/Cargo.toml

@@ -23,3 +23,4 @@ path = "src/sdk.rs"
 [lints.clippy]
 unwrap_used = "deny"
 expect_used = "deny"
+indexing_slicing = "deny"

crates/l2/sdk/src/sdk.rs

@@ -163,7 +163,14 @@ pub async fn claim_withdraw(
         let mut calldata = Vec::new();
 
         // Function selector
-        calldata.extend_from_slice(&keccak(CLAIM_WITHDRAWAL_SIGNATURE).as_bytes()[..4]);
+        calldata.extend_from_slice(
+            keccak(CLAIM_WITHDRAWAL_SIGNATURE)
+                .as_bytes()
+                .get(..4)
+                .ok_or(EthClientError::Custom(
+                    "failed to slice into the claim withdrawal signature".to_owned(),
+                ))?,
+        );
 
         // bytes32 l2WithdrawalTxHash
         calldata.extend_from_slice(l2_withdrawal_tx_hash.as_fixed_bytes());
@@ -271,6 +278,7 @@ pub async fn get_withdraw_merkle_proof(
             .collect(),
         tx_withdrawal_hash,
     )
+    .map_err(|err| EthClientError::Custom(format!("Failed to generate merkle proof: {err}")))?
     .ok_or(EthClientError::Custom(
         "Failed to generate merkle proof, element is not on the tree".to_string(),
     ))?;

crates/l2/tests/tests.rs

@@ -226,7 +226,7 @@ async fn testito() {
     println!("Claiming funds on L1");
 
     while u64::from_str_radix(
-        &eth_client
+        eth_client
             .call(
                 Address::from_str(
                     &std::env::var("ON_CHAIN_PROPOSER_ADDRESS")
@@ -238,7 +238,9 @@ async fn testito() {
                 Overrides::default(),
             )
             .await
-            .unwrap()[2..],
+            .unwrap()
+            .get(2..)
+            .unwrap(),
         16,
     )
     .unwrap()

crates/l2/utils/eth_client/mod.rs

@@ -29,7 +29,7 @@ use keccak_hash::keccak;
 use reqwest::Client;
 use secp256k1::SecretKey;
 use serde::{Deserialize, Serialize};
-use serde_json::json;
+use serde_json::{json, Value};
 use std::ops::Div;
 use tokio::time::{sleep, Instant};
 use tracing::warn;
@@ -365,7 +365,9 @@ impl EthClient {
 
         // Add the nonce just if present, otherwise the RPC will use the latest nonce
         if let Some(nonce) = transaction.nonce {
-            data["nonce"] = json!(format!("{:#x}", nonce));
+            if let Value::Object(ref mut map) = data {
+                map.insert("nonce".to_owned(), json!(format!("{nonce:#x}")));
+            }
         }
 
         let request = RpcRequest {
@@ -377,8 +379,12 @@ impl EthClient {
 
         match self.send_request(request).await {
             Ok(RpcResponse::Success(result)) => u64::from_str_radix(
-                &serde_json::from_value::<String>(result.result)
-                    .map_err(EstimateGasPriceError::SerdeJSONError)?[2..],
+                serde_json::from_value::<String>(result.result)
+                    .map_err(EstimateGasPriceError::SerdeJSONError)?
+                    .get(2..)
+                    .ok_or(EstimateGasPriceError::Custom(
+                        "Failed to slice index response in estimate_gas".to_owned(),
+                    ))?,
                 16,
             )
             .map_err(EstimateGasPriceError::ParseIntError)
@@ -398,9 +404,20 @@ impl EthClient {
                                 "Failed to hex::decode in estimate_gas".to_owned(),
                             )
                         })?;
-                        let string_length = U256::from_big_endian(&abi_decoded_error_data[36..68]);
-                        let string_data =
-                            &abi_decoded_error_data[68..68 + string_length.as_usize()];
+                        let string_length = U256::from_big_endian(
+                            abi_decoded_error_data
+                                .get(36..68)
+                                .ok_or(EthClientError::Custom(
+                                    "Failed to slice index abi_decoded_error_data in estimate_gas"
+                                        .to_owned(),
+                                ))?,
+                        );
+                        let string_data = abi_decoded_error_data
+                            .get(68..68 + string_length.as_usize())
+                            .ok_or(EthClientError::Custom(
+                                "Failed to slice index abi_decoded_error_data in estimate_gas"
+                                    .to_owned(),
+                            ))?;
                         String::from_utf8(string_data.to_vec()).map_err(|_| {
                             EthClientError::Custom(
                                 "Failed to String::from_utf8 in estimate_gas".to_owned(),
@@ -449,8 +466,12 @@ impl EthClient {
 
         match self.send_request(request).await {
             Ok(RpcResponse::Success(result)) => u64::from_str_radix(
-                &serde_json::from_value::<String>(result.result)
-                    .map_err(GetNonceError::SerdeJSONError)?[2..],
+                serde_json::from_value::<String>(result.result)
+                    .map_err(GetNonceError::SerdeJSONError)?
+                    .get(2..)
+                    .ok_or(EthClientError::Custom(
+                        "Failed to deserialize get_nonce request".to_owned(),
+                    ))?,
                 16,
             )
             .map_err(GetNonceError::ParseIntError)