Detector/encode packed collision

CONTRIBUTING.md

@@ -79,10 +79,12 @@ How do I know what kind of test(s) to write?
 
 For each new detector, at least one regression tests must be present.
 
-1. Create a test in `tests/e2e/detectors`
-2. Update `ALL_TEST` in `tests/e2e/detectors/test_detectors.py`
-3. Run `python tests/e2e/detectors/test_detectors.py --generate`. This will generate the json artifacts in `tests/expected_json`. Add the generated files to git. If updating an existing detector, identify the respective json artifacts and then delete them, or run `python ./tests/test_detectors.py --overwrite` instead.
-4. Run `pytest tests/e2e/detectors/test_detectors.py` and check that everything worked.
+1. Create a folder in `tests/e2e/detectors/test_data` with the detector's argument name.
+2. Create a test contract in `tests/e2e/detectors/test_data/<detector_name>/`.
+3. Update `ALL_TEST` in `tests/e2e/detectors/test_detectors.py`
+4. Run `python tests/e2e/detectors/test_detectors.py --compile` to create a zip file of the compilation artifacts.
+5. `pytest tests/e2e/detectors/test_detectors.py --insta update-new`. This will generate a snapshot of the detector output in `tests/e2e/detectors/snapshots/`. If updating an existing detector, run `pytest tests/e2e/detectors/test_detectors.py --insta review` and accept or reject the updates.
+6. Run `pytest tests/e2e/detectors/test_detectors.py` to ensure everything worked. Then, add and commit the files to git.
 
 > ##### Helpful commands for detector tests
 >

slither/detectors/all_detectors.py

@@ -89,3 +89,4 @@
 from .functions.permit_domain_signature_collision import DomainSeparatorCollision
 from .functions.codex import Codex
 from .functions.cyclomatic_complexity import CyclomaticComplexity
+from .operations.encode_packed import EncodePackedCollision

slither/detectors/operations/encode_packed.py

@@ -0,0 +1,104 @@
+"""
+Module detecting usage of more than one dynamic type in abi.encodePacked() arguments which could lead to collision
+"""
+
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.core.declarations.solidity_variables import SolidityFunction
+from slither.slithir.operations import SolidityCall
+from slither.analyses.data_dependency.data_dependency import is_tainted
+from slither.core.solidity_types import ElementaryType
+from slither.core.solidity_types import ArrayType
+
+
+def _is_dynamic_type(arg):
+    """
+    Args:
+        arg (function argument)
+    Returns:
+        Bool
+    """
+    if isinstance(arg.type, ElementaryType) and (arg.type.name in ["string", "bytes"]):
+        return True
+    if isinstance(arg.type, ArrayType) and arg.type.length is None:
+        return True
+
+    return False
+
+
+def _detect_abi_encodePacked_collision(contract):
+    """
+    Args:
+        contract (Contract)
+    Returns:
+        list((Function), (list (Node)))
+    """
+    ret = []
+    # pylint: disable=too-many-nested-blocks
+    for f in contract.functions_and_modifiers_declared:
+        for n in f.nodes:
+            for ir in n.irs:
+                if isinstance(ir, SolidityCall) and ir.function == SolidityFunction(
+                    "abi.encodePacked()"
+                ):
+                    dynamic_type_count = 0
+                    for arg in ir.arguments:
+                        if is_tainted(arg, contract) and _is_dynamic_type(arg):
+                            dynamic_type_count += 1
+                        elif dynamic_type_count > 1:
+                            ret.append((f, n))
+                            dynamic_type_count = 0
+                        else:
+                            dynamic_type_count = 0
+                    if dynamic_type_count > 1:
+                        ret.append((f, n))
+    return ret
+
+
+class EncodePackedCollision(AbstractDetector):
+    """
+    Detect usage of more than one dynamic type in abi.encodePacked() arguments which could to collision
+    """
+
+    ARGUMENT = "encode-packed-collision"
+    HELP = "ABI encodePacked Collision"
+    IMPACT = DetectorClassification.HIGH
+    CONFIDENCE = DetectorClassification.HIGH
+
+    WIKI = (
+        "https://github.com/crytic/slither/wiki/Detector-Documentation#abi-encodePacked-collision"
+    )
+
+    WIKI_TITLE = "ABI encodePacked Collision"
+    WIKI_DESCRIPTION = """Detect collision due to dynamic type usages in `abi.encodePacked`"""
+
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+contract Sign {
+    function get_hash_for_signature(string name, string doc) external returns(bytes32) {
+        return keccak256(abi.encodePacked(name, doc));
+    }
+}
+```
+Bob calls `get_hash_for_signature` with (`bob`, `This is the content`). The hash returned is used as an ID.
+Eve creates a collision with the ID using (`bo`, `bThis is the content`) and compromises the system.
+"""
+    WIKI_RECOMMENDATION = """Do not use more than one dynamic type in `abi.encodePacked()`
+(see the [Solidity documentation](https://solidity.readthedocs.io/en/v0.5.10/abi-spec.html?highlight=abi.encodePacked#non-standard-packed-modeDynamic)). 
+Use `abi.encode()`, preferably."""
+
+    def _detect(self):
+        """Detect usage of more than one dynamic type in abi.encodePacked(..) arguments which could lead to collision"""
+        results = []
+        for c in self.compilation_unit.contracts:
+            values = _detect_abi_encodePacked_collision(c)
+            for func, node in values:
+                info = [
+                    func,
+                    " calls abi.encodePacked() with multiple dynamic arguments:\n\t- ",
+                    node,
+                    "\n",
+                ]
+                json = self.generate_result(info)
+                results.append(json)
+
+        return results

tests/e2e/detectors/snapshots/detectors__detector_EncodePackedCollision_0_7_6_encode_packed_collision_sol__0.txt

@@ -0,0 +1,15 @@
+EncodePackedCollision.bad4(bytes,bytes) (tests/e2e/detectors/test_data/encode-packed-collision/0.7.6/encode_packed_collision.sol#34-36) calls abi.encodePacked() with multiple dynamic arguments:
+	- packed = abi.encodePacked(a,a2,a3,a) (tests/e2e/detectors/test_data/encode-packed-collision/0.7.6/encode_packed_collision.sol#35)
+
+EncodePackedCollision.bad2(string,uint256[]) (tests/e2e/detectors/test_data/encode-packed-collision/0.7.6/encode_packed_collision.sol#24-26) calls abi.encodePacked() with multiple dynamic arguments:
+	- packed = abi.encodePacked(stra,arra) (tests/e2e/detectors/test_data/encode-packed-collision/0.7.6/encode_packed_collision.sol#25)
+
+EncodePackedCollision.bad3_get_hash_for_signature(string,string) (tests/e2e/detectors/test_data/encode-packed-collision/0.7.6/encode_packed_collision.sol#29-31) calls abi.encodePacked() with multiple dynamic arguments:
+	- keccak256(bytes)(abi.encodePacked(name,doc)) (tests/e2e/detectors/test_data/encode-packed-collision/0.7.6/encode_packed_collision.sol#30)
+
+EncodePackedCollision.bad0(string,string) (tests/e2e/detectors/test_data/encode-packed-collision/0.7.6/encode_packed_collision.sol#14-16) calls abi.encodePacked() with multiple dynamic arguments:
+	- packed = abi.encodePacked(stra,strb) (tests/e2e/detectors/test_data/encode-packed-collision/0.7.6/encode_packed_collision.sol#15)
+
+EncodePackedCollision.bad1(string,bytes) (tests/e2e/detectors/test_data/encode-packed-collision/0.7.6/encode_packed_collision.sol#19-21) calls abi.encodePacked() with multiple dynamic arguments:
+	- packed = abi.encodePacked(stra,bytesa) (tests/e2e/detectors/test_data/encode-packed-collision/0.7.6/encode_packed_collision.sol#20)
+

tests/e2e/detectors/test_data/encode-packed-collision/0.7.6/encode_packed_collision.sol

@@ -0,0 +1,78 @@
+contract ABIencodePacked{
+
+  uint a;
+  string str1 = "a";
+  string str2 = "bc";
+  bytes _bytes = "hello world";
+  uint[] arr;
+  uint[2] arr2;
+  string[3] str_arr3; /* This nested dynamic type is not supported in abi.encodePacked mode by solc */
+  string[] str_array; /* This nested dynamic type is not supported in abi.encodePacked mode by solc */
+  bytes[] bytes_array; /* This nested dynamic type and tuples are not supported in abi.encodePacked mode by solc */
+
+  /* Two dynamic types */
+  function bad0(string calldata stra, string calldata strb) external{
+    bytes memory packed = abi.encodePacked(stra, strb);
+  }
+
+  /* Two dynamic types */
+  function bad1(string calldata stra, bytes calldata bytesa) external{
+    bytes memory packed = abi.encodePacked(stra, bytesa);
+  }
+
+  /* Two dynamic types */
+  function bad2(string calldata stra, uint[] calldata arra) external{
+    bytes memory packed = abi.encodePacked(stra, arra);
+  }
+
+  /* Two dynamic types */
+  function bad3_get_hash_for_signature(string calldata name, string calldata doc) external returns (bytes32) {                              
+        return keccak256(abi.encodePacked(name, doc));
+  }
+
+  /* Two dynamic types between non dynamic types */
+  function bad4(bytes calldata a2, bytes calldata a3) external {
+    bytes memory packed = abi.encodePacked(a, a2, a3, a);
+  }
+
+  /* Two dynamic types but static values*/
+  function good0() external{
+    bytes memory packed = abi.encodePacked(str1, str2);
+  }
+
+  /* Two dynamic types but static values*/
+  function good1() external{
+    bytes memory packed = abi.encodePacked(str1, _bytes);
+  }
+
+  /* Two dynamic types but static values*/
+  function good2() external{
+    bytes memory packed = abi.encodePacked(str1, arr);
+  }
+
+  /* No dynamic types */
+  function good3() external{
+    bytes memory packed = abi.encodePacked(a);
+  }
+
+  /* One dynamic type */
+  function good4() external{
+    bytes memory packed = abi.encodePacked(str1);
+  }
+
+  /* One dynamic type */
+  function good5() external{
+    bytes memory packed = abi.encodePacked(a, str1);
+  }
+
+  /* One dynamic type */
+  function good6() external{
+    bytes memory packed = abi.encodePacked(str1, arr2);
+  }
+
+  /* Two dynamic types but not consecutive*/
+  function good7(string calldata a, uint b, string calldata c) external{
+    bytes memory packed = abi.encodePacked(a, b, c);
+  }
+}
+

tests/e2e/detectors/test_detectors.py

@@ -1,4 +1,3 @@
-import json
 import os
 from pathlib import Path
 import sys
@@ -8,6 +7,7 @@
 from crytic_compile import CryticCompile, save_to_zip
 from crytic_compile.utils.zip import load_from_zip
 
+from solc_select import solc_select
 
 from slither import Slither
 from slither.detectors.abstract_detector import AbstractDetector
@@ -33,7 +33,6 @@ def __init__(
         """
         self.detector = detector
         self.test_file = test_file
-        self.expected_result = test_file + "." + solc_ver + "." + detector.__name__ + ".json"
         self.solc_ver = solc_ver
         if additional_files is None:
             self.additional_files = []
@@ -44,6 +43,10 @@ def __init__(
 def set_solc(test_item: Test):  # pylint: disable=too-many-lines
     # hacky hack hack to pick the solc version we want
     env = dict(os.environ)
+
+    if not solc_select.artifact_path(test_item.solc_ver).exists():
+        print("Installing solc version", test_item.solc_ver)
+        solc_select.install_artifacts([test_item.solc_ver])
     env["SOLC_VERSION"] = test_item.solc_ver
     os.environ.clear()
     os.environ.update(env)
@@ -1636,28 +1639,19 @@ def id_test(test_item: Test):
         "LowCyclomaticComplexity.sol",
         "0.8.16",
     ),
+    Test(
+        all_detectors.EncodePackedCollision,
+        "encode_packed_collision.sol",
+        "0.7.6",
+    ),
 ]
 
-
-def get_all_tests() -> List[Test]:
-    # installed_solcs = set(get_installed_solc_versions())
-    # required_solcs = {test.solc_ver for test in ALL_TEST_OBJECTS}
-    # missing_solcs = list(required_solcs - installed_solcs)
-    # if missing_solcs:
-    #     install_solc_versions(missing_solcs)
-
-    return ALL_TEST_OBJECTS
-
-
-ALL_TESTS = get_all_tests()
-
 GENERIC_PATH = "/GENERIC_PATH"
 
 TEST_DATA_DIR = Path(__file__).resolve().parent / "test_data"
 
-
 # pylint: disable=too-many-locals
-@pytest.mark.parametrize("test_item", ALL_TESTS, ids=id_test)
+@pytest.mark.parametrize("test_item", ALL_TEST_OBJECTS, ids=id_test)
 def test_detector(test_item: Test, snapshot):
     test_dir_path = Path(
         TEST_DATA_DIR,
@@ -1681,38 +1675,6 @@ def test_detector(test_item: Test, snapshot):
     assert snapshot() == actual_output
 
 
-def _generate_test(test_item: Test, skip_existing=False):
-    test_dir_path = Path(
-        TEST_DATA_DIR,
-        test_item.detector.ARGUMENT,
-        test_item.solc_ver,
-    ).as_posix()
-    test_file_path = Path(test_dir_path, test_item.test_file).as_posix()
-    expected_result_path = Path(test_dir_path, test_item.expected_result).absolute().as_posix()
-
-    if skip_existing:
-        if os.path.isfile(expected_result_path):
-            return
-
-    set_solc(test_item)
-    sl = Slither(test_file_path)
-    sl.register_detector(test_item.detector)
-    results = sl.run_detectors()
-
-    results_as_string = json.dumps(results)
-    test_file_path = test_file_path.replace("\\", "\\\\")
-    results_as_string = results_as_string.replace(test_file_path, GENERIC_PATH)
-
-    for additional_file in test_item.additional_files:
-        additional_path = Path(test_dir_path, additional_file).absolute().as_posix()
-        additional_path = additional_path.replace("\\", "\\\\")
-        results_as_string = results_as_string.replace(additional_path, GENERIC_PATH)
-
-    results = json.loads(results_as_string)
-    with open(expected_result_path, "w", encoding="utf8") as f:
-        f.write(json.dumps(results, indent=4))
-
-
 def _generate_compile(test_item: Test, skip_existing=False):
     test_dir_path = Path(
         TEST_DATA_DIR,
@@ -1733,13 +1695,9 @@ def _generate_compile(test_item: Test, skip_existing=False):
 
 if __name__ == "__main__":
     if len(sys.argv) != 2:
-        print("To generate the json artifacts run\n\tpython tests/test_detectors.py --generate")
-    elif sys.argv[1] == "--generate":
-        for next_test in ALL_TESTS:
-            _generate_test(next_test, skip_existing=True)
-    elif sys.argv[1] == "--overwrite":
-        for next_test in ALL_TESTS:
-            _generate_test(next_test)
+        print(
+            "To generate the zip artifacts run\n\tpython tests/e2e/tests/test_detectors.py --compile"
+        )
     elif sys.argv[1] == "--compile":
-        for next_test in ALL_TESTS:
+        for next_test in ALL_TEST_OBJECTS:
             _generate_compile(next_test, skip_existing=True)