diff --git a/slither/core/slither_core.py b/slither/core/slither_core.py index 01af779164..6916990671 100644 --- a/slither/core/slither_core.py +++ b/slither/core/slither_core.py @@ -71,6 +71,12 @@ def __init__(self): self._show_ignored_findings = False + # Maps from file to detector name to the start/end ranges for that detector. + # Infinity is used to signal a detector has no end range. + self._ignore_ranges: defaultdict[str, defaultdict[str, List[(int, int)]]] = defaultdict( + lambda: defaultdict(lambda: []) + ) + self._compilation_units: List[SlitherCompilationUnit] = [] self._contracts: List[Contract] = [] @@ -151,7 +157,7 @@ def filename(self) -> Optional[str]: 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: @@ -162,6 +168,8 @@ def add_source_code(self, path): 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 @@ -284,9 +292,52 @@ def offset_to_definitions(self, filename_str: str, offset: int) -> Set[Source]: ################################################################################### ################################################################################### + 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: + + line_text = self.crytic_compile.get_code_from_line(file, line_number) + if line_text is None: + break + + start_regex = r"^\s*//\s*slither-disable-start\s*([a-zA-Z0-9_,-]*)" + end_regex = r"^\s*//\s*slither-disable-end\s*([a-zA-Z0-9_,-]*)" + start_match = re.findall(start_regex, line_text.decode("utf8")) + end_match = re.findall(end_regex, line_text.decode("utf8")) + + if start_match: + ignored = start_match[0].split(",") + if ignored: + for check in ignored: + vals = self._ignore_ranges[file][check] + if len(vals) == 0 or vals[-1][1] != float("inf"): + # First item in the array, or the prior item is fully populated. + self._ignore_ranges[file][check].append((line_number, float("inf"))) + else: + logger.error( + f"Consecutive slither-disable-starts without slither-disable-end in {file}#{line_number}" + ) + return + + if end_match: + ignored = end_match[0].split(",") + if ignored: + for check in ignored: + vals = self._ignore_ranges[file][check] + if len(vals) == 0 or vals[-1][1] != float("inf"): + 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 + def has_ignore_comment(self, r: Dict) -> bool: """ - Check if the result has an ignore comment on the proceeding line, in which case, it is not valid + Check if the result has an ignore comment in the file or on the preceding line, in which + case, it is not valid """ if not self.crytic_compile: return False @@ -303,6 +354,15 @@ def has_ignore_comment(self, r: Dict) -> bool: ) for file, lines in mapping_elements_with_lines: + + # Check if result is within an ignored range. + ignore_ranges = self._ignore_ranges[file][r["check"]] + self._ignore_ranges[file]["all"] + for start, end in ignore_ranges: + # The full check must be within the ignore range to be ignored. + if start < lines[0] and end > lines[-1]: + return True + + # Check for next-line matchers. ignore_line_index = min(lines) - 1 ignore_line_text = self.crytic_compile.get_code_from_line(file, ignore_line_index) if ignore_line_text: @@ -324,7 +384,7 @@ def valid_result(self, r: Dict) -> bool: - All its source paths belong to the source path filtered - Or a similar result was reported and saved during a previous run - The --exclude-dependencies flag is set and results are only related to dependencies - - There is an ignore comment on the preceding line + - There is an ignore comment on the preceding line or in the file """ # Remove duplicate due to the multiple compilation support 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 0000000000..b036bd7fd0 --- /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 0000000000..c9754292bd --- /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 f57e94803a..810589bb06 100644 --- a/tests/test_detectors.py +++ b/tests/test_detectors.py @@ -362,7 +362,10 @@ def id_test(test_item: Test): "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",