From 3880ad63a1957e3f8a82e2899022e19c89e88d45 Mon Sep 17 00:00:00 2001 From: Josselin Feist Date: Mon, 28 Nov 2022 15:21:37 +0100 Subject: [PATCH] Minor improvements + testcase --- slither/core/slither_core.py | 23 +- .../0.8.10/reentrancy_filtered_comments.sol | 22 ++ ...red_comments.sol.0.8.10.ReentrancyEth.json | 231 ++++++++++++++++++ tests/test_detectors.py | 3 + 4 files changed, 267 insertions(+), 12 deletions(-) create mode 100644 tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol create mode 100644 tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol.0.8.10.ReentrancyEth.json diff --git a/slither/core/slither_core.py b/slither/core/slither_core.py index eb5b40d0d..691699067 100644 --- a/slither/core/slither_core.py +++ b/slither/core/slither_core.py @@ -76,8 +76,6 @@ class SlitherCore(Context): self._ignore_ranges: defaultdict[str, defaultdict[str, List[(int, int)]]] = defaultdict( lambda: defaultdict(lambda: []) ) - # Track which files for _ignore_ranges have been processed (a processed file may have no entries in _ignore_ranges). - self._processed_ignores: Set[str] = set() self._compilation_units: List[SlitherCompilationUnit] = [] @@ -159,7 +157,7 @@ class SlitherCore(Context): def filename(self, filename: str): self._filename = filename - def add_source_code(self, path): + def add_source_code(self, path: str) -> None: """ :param path: :return: @@ -170,6 +168,8 @@ class SlitherCore(Context): with open(path, encoding="utf8", newline="") as f: self.source_code[path] = f.read() + self.parse_ignore_comments(path) + @property def markdown_root(self) -> str: return self._markdown_root @@ -292,12 +292,10 @@ class SlitherCore(Context): ################################################################################### ################################################################################### - def parse_ignore_comments(self, file: str): + def parse_ignore_comments(self, file: str) -> None: # The first time we check a file, find all start/end ignore comments and memoize them. line_number = 1 while True: - if file in self._processed_ignores: - break line_text = self.crytic_compile.get_code_from_line(file, line_number) if line_text is None: @@ -317,9 +315,10 @@ class SlitherCore(Context): # First item in the array, or the prior item is fully populated. self._ignore_ranges[file][check].append((line_number, float("inf"))) else: - raise Exception( - "consecutive slither-disable-starts without slither-disable-end" + logger.error( + f"Consecutive slither-disable-starts without slither-disable-end in {file}#{line_number}" ) + return if end_match: ignored = end_match[0].split(",") @@ -327,13 +326,14 @@ class SlitherCore(Context): for check in ignored: vals = self._ignore_ranges[file][check] if len(vals) == 0 or vals[-1][1] != float("inf"): - raise Exception("slither-disable-end without slither-disable-start") + logger.error( + f"slither-disable-end without slither-disable-start in {file}#{line_number}" + ) + return self._ignore_ranges[file][check][-1] = (vals[-1][0], line_number) line_number += 1 - self._processed_ignores.add(file) - def has_ignore_comment(self, r: Dict) -> bool: """ Check if the result has an ignore comment in the file or on the preceding line, in which @@ -354,7 +354,6 @@ class SlitherCore(Context): ) for file, lines in mapping_elements_with_lines: - self.parse_ignore_comments(file) # Check if result is within an ignored range. ignore_ranges = self._ignore_ranges[file][r["check"]] + self._ignore_ranges[file]["all"] diff --git a/tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol b/tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol new file mode 100644 index 000000000..b036bd7fd --- /dev/null +++ b/tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol @@ -0,0 +1,22 @@ +interface Receiver{ + function send_funds() payable external; +} + +contract TestWithBug{ + mapping(address => uint) balances; + + function withdraw(uint amount) public{ + require(amount <= balances[msg.sender]); + Receiver(msg.sender).send_funds{value: amount}(); + balances[msg.sender] -= amount; + } + + // slither-disable-start all + function withdrawFiltered(uint amount) public{ + require(amount <= balances[msg.sender]); + Receiver(msg.sender).send_funds{value: amount}(); + balances[msg.sender] -= amount; + } + // slither-disable-end all +} + diff --git a/tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol.0.8.10.ReentrancyEth.json b/tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol.0.8.10.ReentrancyEth.json new file mode 100644 index 000000000..c9754292b --- /dev/null +++ b/tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol.0.8.10.ReentrancyEth.json @@ -0,0 +1,231 @@ +[ + [ + { + "elements": [ + { + "type": "function", + "name": "withdraw", + "source_mapping": { + "start": 133, + "length": 194, + "filename_relative": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "is_dependency": false, + "lines": [ + 8, + 9, + 10, + 11, + 12 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "TestWithBug", + "source_mapping": { + "start": 67, + "length": 534, + "filename_relative": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "withdraw(uint256)" + } + }, + { + "type": "node", + "name": "Receiver(msg.sender).send_funds{value: amount}()", + "source_mapping": { + "start": 231, + "length": 48, + "filename_relative": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "is_dependency": false, + "lines": [ + 10 + ], + "starting_column": 10, + "ending_column": 58 + }, + "type_specific_fields": { + "parent": { + "type": "function", + "name": "withdraw", + "source_mapping": { + "start": 133, + "length": 194, + "filename_relative": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "is_dependency": false, + "lines": [ + 8, + 9, + 10, + 11, + 12 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "TestWithBug", + "source_mapping": { + "start": 67, + "length": 534, + "filename_relative": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "withdraw(uint256)" + } + } + }, + "additional_fields": { + "underlying_type": "external_calls" + } + }, + { + "type": "node", + "name": "balances[msg.sender] -= amount", + "source_mapping": { + "start": 290, + "length": 30, + "filename_relative": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "is_dependency": false, + "lines": [ + 11 + ], + "starting_column": 10, + "ending_column": 40 + }, + "type_specific_fields": { + "parent": { + "type": "function", + "name": "withdraw", + "source_mapping": { + "start": 133, + "length": 194, + "filename_relative": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "is_dependency": false, + "lines": [ + 8, + 9, + 10, + 11, + 12 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "TestWithBug", + "source_mapping": { + "start": 67, + "length": 534, + "filename_relative": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "withdraw(uint256)" + } + } + }, + "additional_fields": { + "underlying_type": "variables_written", + "variable_name": "balances" + } + } + ], + "description": "Reentrancy in TestWithBug.withdraw(uint256) (tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#8-12):\n\tExternal calls:\n\t- Receiver(msg.sender).send_funds{value: amount}() (tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#10)\n\tState variables written after the call(s):\n\t- balances[msg.sender] -= amount (tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#11)\n\tTestWithBug.balances (tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#6) can be used in cross function reentrancies:\n\t- TestWithBug.withdraw(uint256) (tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#8-12)\n\t- TestWithBug.withdrawFiltered(uint256) (tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#15-19)\n", + "markdown": "Reentrancy in [TestWithBug.withdraw(uint256)](tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#L8-L12):\n\tExternal calls:\n\t- [Receiver(msg.sender).send_funds{value: amount}()](tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#L10)\n\tState variables written after the call(s):\n\t- [balances[msg.sender] -= amount](tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#L11)\n\t[TestWithBug.balances](tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#L6) can be used in cross function reentrancies:\n\t- [TestWithBug.withdraw(uint256)](tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#L8-L12)\n\t- [TestWithBug.withdrawFiltered(uint256)](tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#L15-L19)\n", + "first_markdown_element": "tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol#L8-L12", + "id": "176d2b5b09c260c72fd638ff8b5db4709df3ff3eb253daa1cfde254c8299fb94", + "check": "reentrancy-eth", + "impact": "High", + "confidence": "Medium" + } + ] +] \ No newline at end of file diff --git a/tests/test_detectors.py b/tests/test_detectors.py index f57e94803..810589bb0 100644 --- a/tests/test_detectors.py +++ b/tests/test_detectors.py @@ -362,7 +362,10 @@ ALL_TEST_OBJECTS = [ "DAO.sol", "0.4.25", ), + # Test the nonReentrant filtering Test(all_detectors.ReentrancyEth, "reentrancy_with_non_reentrant.sol", "0.8.10"), + # Test parse_ignore_comments + Test(all_detectors.ReentrancyEth, "reentrancy_filtered_comments.sol", "0.8.10"), Test( all_detectors.UninitializedStorageVars, "uninitialized_storage_pointer.sol",