From 2cbbb706ac80bc2e0ed27e2d728749013670072b Mon Sep 17 00:00:00 2001 From: webthethird Date: Tue, 20 Dec 2022 09:33:15 -0600 Subject: [PATCH 01/10] Handle custom upgradeability comments for contract definition --- slither/solc_parsing/declarations/contract.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/slither/solc_parsing/declarations/contract.py b/slither/solc_parsing/declarations/contract.py index b7f938d1df..ed39bd1b6a 100644 --- a/slither/solc_parsing/declarations/contract.py +++ b/slither/solc_parsing/declarations/contract.py @@ -699,6 +699,16 @@ def delete_content(self): self._usingForNotParsed = [] self._customErrorParsed = [] + def _handle_comment(self, attributes: Dict): + if "documentation" in attributes and "text" in attributes["documentation"]: + candidates = attributes["documentation"]["text"].replace("\n", ",").split(",") + + for candidate in candidates: + if "@custom:security isProxy" in candidate: + self._contract._is_upgradeable_proxy = True + if "@custom:security isUpgradeable" in candidate: + self._contract._is_upgradeable = True + # endregion ################################################################################### ################################################################################### From 61c67d88011587a1b2cadf32323146e295288627 Mon Sep 17 00:00:00 2001 From: webthethird Date: Tue, 20 Dec 2022 09:49:34 -0600 Subject: [PATCH 02/10] Add `upgradeable_version` property to Contract class --- slither/core/declarations/contract.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/slither/core/declarations/contract.py b/slither/core/declarations/contract.py index eb2ac9a2e3..dc77eb8661 100644 --- a/slither/core/declarations/contract.py +++ b/slither/core/declarations/contract.py @@ -88,6 +88,7 @@ def __init__(self, compilation_unit: "SlitherCompilationUnit", scope: "FileScope self._is_upgradeable: Optional[bool] = None self._is_upgradeable_proxy: Optional[bool] = None + self._upgradeable_version: Optional[str] = None self.is_top_level = False # heavily used, so no @property @@ -1246,6 +1247,14 @@ def is_upgradeable_proxy(self) -> bool: return self._is_upgradeable_proxy return self._is_upgradeable_proxy + @property + def upgradeable_version(self) -> Optional[str]: + return self._upgradeable_version + + @upgradeable_version.setter + def upgradeable_version(self, version_name: str): + self._upgradeable_version = version_name + # endregion ################################################################################### ################################################################################### From f1e653fb186cb9c6a65dc8d054bd0d9f1c8d7cfd Mon Sep 17 00:00:00 2001 From: webthethird Date: Tue, 20 Dec 2022 10:12:47 -0600 Subject: [PATCH 03/10] Call `handle_comment` in init method --- slither/solc_parsing/declarations/contract.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/slither/solc_parsing/declarations/contract.py b/slither/solc_parsing/declarations/contract.py index ed39bd1b6a..5a51a142e7 100644 --- a/slither/solc_parsing/declarations/contract.py +++ b/slither/solc_parsing/declarations/contract.py @@ -65,8 +65,10 @@ def __init__(self, slither_parser: "SlitherCompilationUnitSolc", contract: Contr # Export info if self.is_compact_ast: self._contract.name = self._data["name"] + self._handle_comment(self._data) else: self._contract.name = self._data["attributes"][self.get_key()] + self._handle_comment(self._data["attributes"]) self._contract.id = self._data["id"] From 5b14dae8b6694ebee28a5b7fce69a543a1ea0803 Mon Sep 17 00:00:00 2001 From: webthethird Date: Tue, 20 Dec 2022 10:15:29 -0600 Subject: [PATCH 04/10] Parse version name from custom comment --- slither/solc_parsing/declarations/contract.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/slither/solc_parsing/declarations/contract.py b/slither/solc_parsing/declarations/contract.py index 5a51a142e7..05bd9551d7 100644 --- a/slither/solc_parsing/declarations/contract.py +++ b/slither/solc_parsing/declarations/contract.py @@ -1,4 +1,5 @@ import logging +import re from typing import List, Dict, Callable, TYPE_CHECKING, Union, Set from slither.core.declarations import Modifier, Event, EnumContract, StructureContract, Function @@ -711,6 +712,12 @@ def _handle_comment(self, attributes: Dict): if "@custom:security isUpgradeable" in candidate: self._contract._is_upgradeable = True + version_name = re.search( + r'@custom:version name="([\w, .]*)"', candidate + ) + if version_name: + self._contract.upgradeable_version = version_name.group(1) + # endregion ################################################################################### ################################################################################### From afced5cc929abf4563f6df1b252320e7b4c42cbd Mon Sep 17 00:00:00 2001 From: webthethird Date: Tue, 20 Dec 2022 14:35:03 -0600 Subject: [PATCH 05/10] Fix test errors caused by `attributes["documentation"] = None"` --- slither/solc_parsing/declarations/contract.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/slither/solc_parsing/declarations/contract.py b/slither/solc_parsing/declarations/contract.py index 05bd9551d7..124e5b9c26 100644 --- a/slither/solc_parsing/declarations/contract.py +++ b/slither/solc_parsing/declarations/contract.py @@ -703,7 +703,11 @@ def delete_content(self): self._customErrorParsed = [] def _handle_comment(self, attributes: Dict): - if "documentation" in attributes and "text" in attributes["documentation"]: + if ( + "documentation" in attributes + and attributes["documentation"] is not None + and "text" in attributes["documentation"] + ): candidates = attributes["documentation"]["text"].replace("\n", ",").split(",") for candidate in candidates: From 5458a614cf91f6cd337046342efdba335bde3662 Mon Sep 17 00:00:00 2001 From: webthethird Date: Tue, 20 Dec 2022 14:47:43 -0600 Subject: [PATCH 06/10] Fix formatting --- slither/solc_parsing/declarations/contract.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/slither/solc_parsing/declarations/contract.py b/slither/solc_parsing/declarations/contract.py index 124e5b9c26..4e8a915607 100644 --- a/slither/solc_parsing/declarations/contract.py +++ b/slither/solc_parsing/declarations/contract.py @@ -704,9 +704,9 @@ def delete_content(self): def _handle_comment(self, attributes: Dict): if ( - "documentation" in attributes - and attributes["documentation"] is not None - and "text" in attributes["documentation"] + "documentation" in attributes + and attributes["documentation"] is not None + and "text" in attributes["documentation"] ): candidates = attributes["documentation"]["text"].replace("\n", ",").split(",") @@ -716,9 +716,7 @@ def _handle_comment(self, attributes: Dict): if "@custom:security isUpgradeable" in candidate: self._contract._is_upgradeable = True - version_name = re.search( - r'@custom:version name="([\w, .]*)"', candidate - ) + version_name = re.search(r'@custom:version name="([\w, .]*)"', candidate) if version_name: self._contract.upgradeable_version = version_name.group(1) From c44cf18831ceb789b5131b6722b90b80667a6640 Mon Sep 17 00:00:00 2001 From: webthethird Date: Tue, 20 Dec 2022 15:31:55 -0600 Subject: [PATCH 07/10] Add setters for properties --- slither/core/declarations/contract.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/slither/core/declarations/contract.py b/slither/core/declarations/contract.py index dc77eb8661..a90e2591eb 100644 --- a/slither/core/declarations/contract.py +++ b/slither/core/declarations/contract.py @@ -1222,6 +1222,10 @@ def is_upgradeable(self) -> bool: break return self._is_upgradeable + @is_upgradeable.setter + def is_upgradeable(self, upgradeable: bool): + self._is_upgradeable = upgradeable + @property def is_upgradeable_proxy(self) -> bool: from slither.core.cfg.node import NodeType @@ -1247,6 +1251,10 @@ def is_upgradeable_proxy(self) -> bool: return self._is_upgradeable_proxy return self._is_upgradeable_proxy + @is_upgradeable_proxy.setter + def is_upgradeable_proxy(self, upgradeable_proxy: bool): + self._is_upgradeable_proxy = upgradeable_proxy + @property def upgradeable_version(self) -> Optional[str]: return self._upgradeable_version From 762806c52f2a825ca3dfcfcdd8f3a9c23c922765 Mon Sep 17 00:00:00 2001 From: webthethird Date: Tue, 20 Dec 2022 15:32:29 -0600 Subject: [PATCH 08/10] Fix access to protected member --- slither/solc_parsing/declarations/contract.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/slither/solc_parsing/declarations/contract.py b/slither/solc_parsing/declarations/contract.py index 4e8a915607..1766e76175 100644 --- a/slither/solc_parsing/declarations/contract.py +++ b/slither/solc_parsing/declarations/contract.py @@ -712,9 +712,9 @@ def _handle_comment(self, attributes: Dict): for candidate in candidates: if "@custom:security isProxy" in candidate: - self._contract._is_upgradeable_proxy = True + self._contract.is_upgradeable_proxy = True if "@custom:security isUpgradeable" in candidate: - self._contract._is_upgradeable = True + self._contract.is_upgradeable = True version_name = re.search(r'@custom:version name="([\w, .]*)"', candidate) if version_name: From c75278f2f554f3597f2461bd2f0eadd7943ff746 Mon Sep 17 00:00:00 2001 From: Josselin Feist Date: Wed, 21 Dec 2022 14:42:37 +0100 Subject: [PATCH 09/10] Improvements + tests --- slither/solc_parsing/declarations/contract.py | 6 ++-- tests/custom_comments/upgrade.sol | 16 ++++++++++ tests/test_features.py | 29 +++++++++++++++---- 3 files changed, 43 insertions(+), 8 deletions(-) create mode 100644 tests/custom_comments/upgrade.sol diff --git a/slither/solc_parsing/declarations/contract.py b/slither/solc_parsing/declarations/contract.py index 1766e76175..ead5b20a26 100644 --- a/slither/solc_parsing/declarations/contract.py +++ b/slither/solc_parsing/declarations/contract.py @@ -702,7 +702,7 @@ def delete_content(self): self._usingForNotParsed = [] self._customErrorParsed = [] - def _handle_comment(self, attributes: Dict): + def _handle_comment(self, attributes: Dict) -> None: if ( "documentation" in attributes and attributes["documentation"] is not None @@ -711,12 +711,12 @@ def _handle_comment(self, attributes: Dict): candidates = attributes["documentation"]["text"].replace("\n", ",").split(",") for candidate in candidates: - if "@custom:security isProxy" in candidate: + if "@custom:security isDelegatecallProxy" in candidate: self._contract.is_upgradeable_proxy = True if "@custom:security isUpgradeable" in candidate: self._contract.is_upgradeable = True - version_name = re.search(r'@custom:version name="([\w, .]*)"', candidate) + version_name = re.search(r'@custom:version name=([\w-]+)', candidate) if version_name: self._contract.upgradeable_version = version_name.group(1) diff --git a/tests/custom_comments/upgrade.sol b/tests/custom_comments/upgrade.sol new file mode 100644 index 0000000000..96192df0bc --- /dev/null +++ b/tests/custom_comments/upgrade.sol @@ -0,0 +1,16 @@ +/// @custom:security isDelegatecallProxy +contract Proxy{ + +} + +/// @custom:security isUpgradeable +/// @custom:version name=version-0 +contract V0{ + +} + +/// @custom:security isUpgradeable +/// @custom:version name=version_1 +contract V1{ + +} \ No newline at end of file diff --git a/tests/test_features.py b/tests/test_features.py index c06ee96ce8..4b4564b504 100644 --- a/tests/test_features.py +++ b/tests/test_features.py @@ -9,7 +9,7 @@ from slither.detectors.abstract_detector import AbstractDetector -def _run_all_detectors(slither: Slither): +def _run_all_detectors(slither: Slither) -> None: detectors = [getattr(all_detectors, name) for name in dir(all_detectors)] detectors = [d for d in detectors if inspect.isclass(d) and issubclass(d, AbstractDetector)] @@ -19,7 +19,7 @@ def _run_all_detectors(slither: Slither): slither.run_detectors() -def test_node(): +def test_node() -> None: # hardhat must have been installed in tests/test_node_modules # For the CI its done through the github action config @@ -27,7 +27,7 @@ def test_node(): _run_all_detectors(slither) -def test_collision(): +def test_collision() -> None: standard_json = SolcStandardJson() standard_json.add_source_file("./tests/collisions/a.sol") @@ -39,14 +39,33 @@ def test_collision(): _run_all_detectors(slither) -def test_cycle(): +def test_cycle() -> None: slither = Slither("./tests/test_cyclic_import/a.sol") _run_all_detectors(slither) -def test_funcion_id_rec_structure(): +def test_funcion_id_rec_structure() -> None: solc_select.switch_global_version("0.8.0", always_install=True) slither = Slither("./tests/function_ids/rec_struct-0.8.sol") for compilation_unit in slither.compilation_units: for function in compilation_unit.functions: assert function.solidity_signature + + +def test_upgradeable_comments() -> None: + solc_select.switch_global_version("0.8.10", always_install=True) + slither = Slither("./tests/custom_comments/upgrade.sol") + compilation_unit = slither.compilation_units[0] + proxy = compilation_unit.get_contract_from_name("Proxy")[0] + + assert proxy.is_upgradeable_proxy + + v0 = compilation_unit.get_contract_from_name("V0")[0] + + assert v0.is_upgradeable + print(v0.upgradeable_version) + assert v0.upgradeable_version == "version-0" + + v1 = compilation_unit.get_contract_from_name("V1")[0] + assert v0.is_upgradeable + assert v1.upgradeable_version == "version_1" From e353256f5257c0663f0999c7900d3a22466696f3 Mon Sep 17 00:00:00 2001 From: Josselin Feist Date: Wed, 21 Dec 2022 14:47:33 +0100 Subject: [PATCH 10/10] Black --- slither/solc_parsing/declarations/contract.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/slither/solc_parsing/declarations/contract.py b/slither/solc_parsing/declarations/contract.py index ead5b20a26..c509258e98 100644 --- a/slither/solc_parsing/declarations/contract.py +++ b/slither/solc_parsing/declarations/contract.py @@ -716,7 +716,7 @@ def _handle_comment(self, attributes: Dict) -> None: if "@custom:security isUpgradeable" in candidate: self._contract.is_upgradeable = True - version_name = re.search(r'@custom:version name=([\w-]+)', candidate) + version_name = re.search(r"@custom:version name=([\w-]+)", candidate) if version_name: self._contract.upgradeable_version = version_name.group(1)