From d41861e6cfd95ba7b33518927342244390699521 Mon Sep 17 00:00:00 2001 From: Feist Josselin Date: Fri, 18 Mar 2022 19:43:44 +0100 Subject: [PATCH] Add support for enhanced analyses through code comments (#1089) * Add support for enhanced analyses through code comments - Parse /// ... on variable declaration - Disable reentrancy for variables marked as 'sec:non-reentrant' - Create a new detector looking for wrong access control ('@custom:security write-protection="some_func") --- slither/core/variables/variable.py | 20 +- slither/detectors/all_detectors.py | 1 + .../detectors/functions/protected_variable.py | 81 +++++++ slither/slithir/operations/high_level_call.py | 4 + .../variables/variable_declaration.py | 20 ++ .../protected-vars/0.8.2/comment.sol | 35 +++ .../comment.sol.0.8.2.ProtectedVariables.json | 223 ++++++++++++++++++ .../reentrancy-no-eth/0.8.2/comment.sol | 35 +++ ...sol.0.8.2.ReentrancyReadBeforeWritten.json | 3 + tests/test_detectors.py | 10 + 10 files changed, 431 insertions(+), 1 deletion(-) create mode 100644 slither/detectors/functions/protected_variable.py create mode 100644 tests/detectors/protected-vars/0.8.2/comment.sol create mode 100644 tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json create mode 100644 tests/detectors/reentrancy-no-eth/0.8.2/comment.sol create mode 100644 tests/detectors/reentrancy-no-eth/0.8.2/comment.sol.0.8.2.ReentrancyReadBeforeWritten.json diff --git a/slither/core/variables/variable.py b/slither/core/variables/variable.py index c0a5fe0ffc..5f94bc9222 100644 --- a/slither/core/variables/variable.py +++ b/slither/core/variables/variable.py @@ -10,7 +10,7 @@ if TYPE_CHECKING: from slither.core.expressions.expression import Expression - +# pylint: disable=too-many-instance-attributes class Variable(SourceMapping): def __init__(self): super().__init__() @@ -21,6 +21,8 @@ def __init__(self): self._visibility: Optional[str] = None self._is_constant = False self._is_immutable: bool = False + self._is_reentrant: bool = True + self._write_protection: Optional[List[str]] = None @property def is_scalar(self) -> bool: @@ -90,6 +92,22 @@ def is_constant(self) -> bool: def is_constant(self, is_cst: bool): self._is_constant = is_cst + @property + def is_reentrant(self) -> bool: + return self._is_reentrant + + @is_reentrant.setter + def is_reentrant(self, is_reentrant: bool): + self._is_reentrant = is_reentrant + + @property + def write_protection(self) -> Optional[List[str]]: + return self._write_protection + + @write_protection.setter + def write_protection(self, write_protection: List[str]): + self._write_protection = write_protection + @property def visibility(self) -> Optional[str]: """ diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 62143ad3f1..e287c258f6 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -81,3 +81,4 @@ from .statements.write_after_write import WriteAfterWrite from .statements.msg_value_in_loop import MsgValueInLoop from .statements.delegatecall_in_loop import DelegatecallInLoop +from .functions.protected_variable import ProtectedVariables diff --git a/slither/detectors/functions/protected_variable.py b/slither/detectors/functions/protected_variable.py new file mode 100644 index 0000000000..0c1341d3ca --- /dev/null +++ b/slither/detectors/functions/protected_variable.py @@ -0,0 +1,81 @@ +""" +Module detecting suicidal contract + +A suicidal contract is an unprotected function that calls selfdestruct +""" +from typing import List + +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.core.declarations import Function, Contract +from slither.utils.output import Output + + +class ProtectedVariables(AbstractDetector): + + ARGUMENT = "protected-vars" + HELP = "Detected unprotected variables" + IMPACT = DetectorClassification.HIGH + CONFIDENCE = DetectorClassification.HIGH + + WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#protected-variables" + + WIKI_TITLE = "Protected Variables" + WIKI_DESCRIPTION = "Detect unprotected variable that are marked protected" + + # region wiki_exploit_scenario + WIKI_EXPLOIT_SCENARIO = """ +```solidity +contract Buggy{ + + /// @custom:security write-protection="onlyOwner()" + address owner; + + function set_protected() public onlyOwner(){ + owner = msg.sender; + } + + function set_not_protected() public{ + owner = msg.sender; + } +} +``` +`owner` must be always written by function using `onlyOwner` (`write-protection="onlyOwner()"`), however anyone can call `set_not_protected`. +""" + # endregion wiki_exploit_scenario + + WIKI_RECOMMENDATION = "Add access controls to the vulnerable function" + + def _analyze_function(self, function: Function, contract: Contract) -> List[Output]: + results = [] + + for state_variable_written in function.state_variables_written: + if state_variable_written.write_protection: + for function_sig in state_variable_written.write_protection: + function_protection = contract.get_function_from_signature(function_sig) + if not function_protection: + function_protection = contract.get_modifier_from_signature(function_sig) + if not function_protection: + self.logger.error(f"{function_sig} not found") + continue + if function_protection not in function.all_internal_calls(): + info = [ + function, + " should have ", + function_protection, + " to protect ", + state_variable_written, + "\n", + ] + + res = self.generate_result(info) + results.append(res) + return results + + def _detect(self): + """Detect the suicidal functions""" + results = [] + for contract in self.compilation_unit.contracts_derived: + for function in contract.functions_entry_points: + results += self._analyze_function(function, contract) + + return results diff --git a/slither/slithir/operations/high_level_call.py b/slither/slithir/operations/high_level_call.py index 0cac823bac..ff72a0899d 100644 --- a/slither/slithir/operations/high_level_call.py +++ b/slither/slithir/operations/high_level_call.py @@ -128,6 +128,10 @@ def can_reenter(self, callstack=None): callstack = callstack + [self.function] if self.function.can_reenter(callstack): return True + if isinstance(self.destination, Variable): + if not self.destination.is_reentrant: + return False + return True def can_send_eth(self): diff --git a/slither/solc_parsing/variables/variable_declaration.py b/slither/solc_parsing/variables/variable_declaration.py index 8bfe65e120..119604ca44 100644 --- a/slither/solc_parsing/variables/variable_declaration.py +++ b/slither/solc_parsing/variables/variable_declaration.py @@ -1,4 +1,5 @@ import logging +import re from typing import Dict from slither.solc_parsing.declarations.caller_context import CallerContextExpression @@ -103,6 +104,23 @@ def reference_id(self) -> int: """ return self._reference_id + def _handle_comment(self, attributes: Dict): + if "documentation" in attributes and "text" in attributes["documentation"]: + + candidates = attributes["documentation"]["text"].split(",") + + for candidate in candidates: + if "@custom:security non-reentrant" in candidate: + self._variable.is_reentrant = False + + write_protection = re.search( + r'@custom:security write-protection="([\w, ()]*)"', candidate + ) + if write_protection: + if self._variable.write_protection is None: + self._variable.write_protection = [] + self._variable.write_protection.append(write_protection.group(1)) + def _analyze_variable_attributes(self, attributes: Dict): if "visibility" in attributes: self._variable.visibility = attributes["visibility"] @@ -145,6 +163,8 @@ def _init_from_declaration(self, var: Dict, init: bool): # pylint: disable=too- if attributes["mutability"] == "immutable": self._variable.is_immutable = True + self._handle_comment(attributes) + self._analyze_variable_attributes(attributes) if self._is_compact_ast: diff --git a/tests/detectors/protected-vars/0.8.2/comment.sol b/tests/detectors/protected-vars/0.8.2/comment.sol new file mode 100644 index 0000000000..e06ff3902c --- /dev/null +++ b/tests/detectors/protected-vars/0.8.2/comment.sol @@ -0,0 +1,35 @@ +interface I{ + function external_call() external; +} + +contract ReentrancyAndWrite{ + + /// @custom:security non-reentrant + /// @custom:security write-protection="onlyOwner()" + I external_contract; + + modifier onlyOwner(){ + // lets assume there is an access control + _; + } + + mapping(address => uint) balances; + + function withdraw() public{ + uint balance = balances[msg.sender]; + + external_contract.external_call(); + + balances[msg.sender] = 0; + payable(msg.sender).transfer(balance); + } + + function set_protected() public onlyOwner(){ + external_contract = I(msg.sender); + } + + function set_not_protected() public{ + external_contract = I(msg.sender); + } +} + diff --git a/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json b/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json new file mode 100644 index 0000000000..3b1d1eb07f --- /dev/null +++ b/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json @@ -0,0 +1,223 @@ +[ + [ + { + "elements": [ + { + "type": "function", + "name": "set_not_protected", + "source_mapping": { + "start": 653, + "length": 85, + "filename_used": "/GENERIC_PATH", + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 31, + 32, + 33 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "ReentrancyAndWrite", + "source_mapping": { + "start": 55, + "length": 685, + "filename_used": "/GENERIC_PATH", + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "set_not_protected()" + } + }, + { + "type": "function", + "name": "onlyOwner", + "source_mapping": { + "start": 210, + "length": 88, + "filename_used": "/GENERIC_PATH", + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 11, + 12, + 13, + 14 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "ReentrancyAndWrite", + "source_mapping": { + "start": 55, + "length": 685, + "filename_used": "/GENERIC_PATH", + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "onlyOwner()" + } + }, + { + "type": "variable", + "name": "external_contract", + "source_mapping": { + "start": 184, + "length": 19, + "filename_used": "/GENERIC_PATH", + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 9 + ], + "starting_column": 5, + "ending_column": 24 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "ReentrancyAndWrite", + "source_mapping": { + "start": 55, + "length": 685, + "filename_used": "/GENERIC_PATH", + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34 + ], + "starting_column": 1, + "ending_column": 2 + } + } + } + } + ], + "description": "ReentrancyAndWrite.set_not_protected() (tests/detectors/protected-vars/0.8.2/comment.sol#31-33) should have ReentrancyAndWrite.onlyOwner() (tests/detectors/protected-vars/0.8.2/comment.sol#11-14) to protect ReentrancyAndWrite.external_contract (tests/detectors/protected-vars/0.8.2/comment.sol#9)\n", + "markdown": "[ReentrancyAndWrite.set_not_protected()](tests/detectors/protected-vars/0.8.2/comment.sol#L31-L33) should have [ReentrancyAndWrite.onlyOwner()](tests/detectors/protected-vars/0.8.2/comment.sol#L11-L14) to protect [ReentrancyAndWrite.external_contract](tests/detectors/protected-vars/0.8.2/comment.sol#L9)\n", + "first_markdown_element": "tests/detectors/protected-vars/0.8.2/comment.sol#L31-L33", + "id": "3f3bc8c8a9b3e23482f47f1133aceaed81c2c781c6aaf25656a8e578c9f6cb0e", + "check": "protected-vars", + "impact": "High", + "confidence": "High" + } + ] +] \ No newline at end of file diff --git a/tests/detectors/reentrancy-no-eth/0.8.2/comment.sol b/tests/detectors/reentrancy-no-eth/0.8.2/comment.sol new file mode 100644 index 0000000000..e06ff3902c --- /dev/null +++ b/tests/detectors/reentrancy-no-eth/0.8.2/comment.sol @@ -0,0 +1,35 @@ +interface I{ + function external_call() external; +} + +contract ReentrancyAndWrite{ + + /// @custom:security non-reentrant + /// @custom:security write-protection="onlyOwner()" + I external_contract; + + modifier onlyOwner(){ + // lets assume there is an access control + _; + } + + mapping(address => uint) balances; + + function withdraw() public{ + uint balance = balances[msg.sender]; + + external_contract.external_call(); + + balances[msg.sender] = 0; + payable(msg.sender).transfer(balance); + } + + function set_protected() public onlyOwner(){ + external_contract = I(msg.sender); + } + + function set_not_protected() public{ + external_contract = I(msg.sender); + } +} + diff --git a/tests/detectors/reentrancy-no-eth/0.8.2/comment.sol.0.8.2.ReentrancyReadBeforeWritten.json b/tests/detectors/reentrancy-no-eth/0.8.2/comment.sol.0.8.2.ReentrancyReadBeforeWritten.json new file mode 100644 index 0000000000..5825bcacc6 --- /dev/null +++ b/tests/detectors/reentrancy-no-eth/0.8.2/comment.sol.0.8.2.ReentrancyReadBeforeWritten.json @@ -0,0 +1,3 @@ +[ + [] +] \ No newline at end of file diff --git a/tests/test_detectors.py b/tests/test_detectors.py index 07fdd096b3..f8374e1c9c 100644 --- a/tests/test_detectors.py +++ b/tests/test_detectors.py @@ -116,6 +116,11 @@ def id_test(test_item: Test): "DAO.sol", "0.4.25", ), + Test( + all_detectors.ReentrancyReadBeforeWritten, + "comment.sol", + "0.8.2", + ), Test( all_detectors.ReentrancyReadBeforeWritten, "no-reentrancy-staticcall.sol", @@ -1262,6 +1267,11 @@ def id_test(test_item: Test): "delegatecall_loop.sol", "0.8.0", ), + Test( + all_detectors.ProtectedVariables, + "comment.sol", + "0.8.2", + ), ]