Skip to content

Commit

Permalink
Merge pull request #1483 from crytic/mds1-feat/disable-start
Browse files Browse the repository at this point in the history 
feat/disable start
  • Loading branch information
@montyly
montyly committed Nov 28, 2022
2 parents e292c67 + 3880ad6 commit b96beea
Show file tree
Hide file tree
Showing 4 changed files with 319 additions and 3 deletions.
66 changes: 63 additions & 3 deletions slither/core/slither_core.py
View file
Expand Up @@ -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] = []
Expand Down Expand Up @@ -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:
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -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
Expand Down
22 changes: 22 additions & 0 deletions tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol
View file
@@ -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
}

231 changes: 231 additions & 0 deletions ...etectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol.0.8.10.ReentrancyEth.json
View file
@@ -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"
}
]
]
3 changes: 3 additions & 0 deletions tests/test_detectors.py
View file
Expand Up @@ -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",
Expand Down

0 comments on commit b96beea

Please sign in to comment.