From f67398c5b48b42aa8630f5e441a37a024254d465 Mon Sep 17 00:00:00 2001
From: Oba <obatirou@gmail.com>
Date: Mon, 2 Dec 2024 18:28:30 +0100
Subject: [PATCH] fix: OOB read in parse_storage_keys on malformed inputs

---
 cairo/src/utils/transaction.cairo         |  9 ++++++
 cairo/tests/src/utils/test_transaction.py | 28 ++++++++++++++++-
 cairo/tests/utils/constants.py            | 38 ++++++++++++-----------
 3 files changed, 56 insertions(+), 19 deletions(-)

diff --git a/cairo/src/utils/transaction.cairo b/cairo/src/utils/transaction.cairo
index 68078070..c7a4571f 100644
--- a/cairo/src/utils/transaction.cairo
+++ b/cairo/src/utils/transaction.cairo
@@ -287,6 +287,10 @@ namespace Transaction {
 
         // Address
         let address_item = cast(access_list.data, RLP.Item*);
+        with_attr error_message("Invalid address length") {
+            assert [range_check_ptr] = address_item.data_len - 20;
+        }
+        let range_check_ptr = range_check_ptr + 1;
         let address = Helpers.bytes20_to_felt(address_item.data);
 
         // List<StorageKeys>
@@ -320,6 +324,11 @@ namespace Transaction {
             return ();
         }
 
+        with_attr error_message("Invalid storage key length") {
+            assert [range_check_ptr] = keys_list.data_len - 32;
+        }
+        let range_check_ptr = range_check_ptr + 1;
+
         let key = Helpers.bytes32_to_uint256(keys_list.data);
         assert [parsed_keys] = key.low;
         assert [parsed_keys + 1] = key.high;
diff --git a/cairo/tests/src/utils/test_transaction.py b/cairo/tests/src/utils/test_transaction.py
index 29c3a911..65c63a37 100644
--- a/cairo/tests/src/utils/test_transaction.py
+++ b/cairo/tests/src/utils/test_transaction.py
@@ -7,7 +7,11 @@
 from hypothesis import strategies as st
 from rlp import encode
 
-from tests.utils.constants import INVALID_TRANSACTIONS, TRANSACTIONS
+from tests.utils.constants import (
+    ACCESS_LIST_TRANSACTION,
+    INVALID_TRANSACTIONS,
+    TRANSACTIONS,
+)
 from tests.utils.errors import cairo_error
 from tests.utils.helpers import flatten_tx_access_list, rlp_encode_signed_data
 
@@ -144,6 +148,28 @@ def test_should_parse_access_list(self, cairo_run, transaction):
             expected_output = flatten_tx_access_list(transaction.get("accessList", []))
             assert output == expected_output
 
+        def test_should_panic_on_invalid_address_format(self, cairo_run):
+            rlp_structure_tx = transaction_rpc_to_rlp_structure(ACCESS_LIST_TRANSACTION)
+            # modify access list for addr to be 1 byte
+            rlp_structure_tx["accessList"] = [
+                (f"0x{bytes([1]).hex()}", storage_keys)
+                for _, storage_keys in rlp_structure_tx["accessList"]
+            ]
+            encoded_access_list = encode(rlp_structure_tx.get("accessList", []))
+            with cairo_error("Invalid address length"):
+                cairo_run("test__parse_access_list", data=list(encoded_access_list))
+
+        def test_should_panic_on_invalid_storage_key_format(self, cairo_run):
+            rlp_structure_tx = transaction_rpc_to_rlp_structure(ACCESS_LIST_TRANSACTION)
+            # modify access list for storage key to be 1 byte
+            rlp_structure_tx["accessList"] = [
+                (address, (f"0x{bytes([1]).hex()}",))
+                for address, _ in rlp_structure_tx["accessList"]
+            ]
+            encoded_access_list = encode(rlp_structure_tx.get("accessList", []))
+            with cairo_error("Invalid storage key length"):
+                cairo_run("test__parse_access_list", data=list(encoded_access_list))
+
     class TestGetTxType:
         @pytest.mark.parametrize("transaction", TRANSACTIONS)
         def test_should_return_tx_type(self, cairo_run, transaction):
diff --git a/cairo/tests/utils/constants.py b/cairo/tests/utils/constants.py
index d566b778..f0c4b6a3 100644
--- a/cairo/tests/utils/constants.py
+++ b/cairo/tests/utils/constants.py
@@ -31,6 +31,25 @@
 }
 OWNER, OTHER = signers.keys()
 
+ACCESS_LIST_TRANSACTION = {
+    "type": 1,
+    "gas": 100_000,
+    "gasPrice": 1_000_000_000,
+    "data": "0x616263646566",
+    "nonce": 34,
+    "to": "0x09616C3d61b3331fc4109a9E41a8BDB7d9776609",
+    "value": 0x5AF3107A4000,
+    "accessList": (
+        {
+            "address": "0x0000000000000000000000000000000000000001",
+            "storageKeys": (
+                "0x0100000000000000000000000000000000000000000000000000000000000000",
+            ),
+        },
+    ),
+    "chainId": CHAIN_ID,
+}
+
 # Taken from eth_account.account.Account.sign_transaction docstring
 # https://eth-account.readthedocs.io/en/stable/eth_account.html?highlight=sign_transaction#eth_account.account.Account.sign_transaction
 TRANSACTIONS = [
@@ -43,24 +62,7 @@
         "chainId": CHAIN_ID,
         "data": b"",
     },
-    {
-        "type": 1,
-        "gas": 100_000,
-        "gasPrice": 1_000_000_000,
-        "data": "0x616263646566",
-        "nonce": 34,
-        "to": "0x09616C3d61b3331fc4109a9E41a8BDB7d9776609",
-        "value": 0x5AF3107A4000,
-        "accessList": (
-            {
-                "address": "0x0000000000000000000000000000000000000001",
-                "storageKeys": (
-                    "0x0100000000000000000000000000000000000000000000000000000000000000",
-                ),
-            },
-        ),
-        "chainId": CHAIN_ID,
-    },
+    ACCESS_LIST_TRANSACTION,
     # Access list with two addresses
     {
         "type": 1,