Skip to content

Commit

Permalink
Minor improvements + testcase
Browse files Browse the repository at this point in the history
  • Loading branch information
@montyly
montyly committed Nov 28, 2022
1 parent a16202a commit 3880ad6
Show file tree
Hide file tree
Showing 4 changed files with 267 additions and 12 deletions.
23 changes: 11 additions & 12 deletions slither/core/slither_core.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,6 @@ def __init__(self):
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] = []

Expand Down Expand Up @@ -159,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 @@ -170,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 @@ -292,12 +292,10 @@ def offset_to_definitions(self, filename_str: str, offset: int) -> Set[Source]:
###################################################################################
###################################################################################

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:
Expand All @@ -317,23 +315,25 @@ def parse_ignore_comments(self, file: str):
# 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(",")
if ignored:
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
Expand All @@ -354,7 +354,6 @@ def has_ignore_comment(self, r: Dict) -> bool:
)

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"]
Expand Down
22 changes: 22 additions & 0 deletions tests/detectors/reentrancy-eth/0.8.10/reentrancy_filtered_comments.sol
View file
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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 3880ad6

Please sign in to comment.