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

@@ -10,7 +10,7 @@ from slither.core.solidity_types.elementary_type import ElementaryType
 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 @@ class Variable(SourceMapping):
         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 @@ class Variable(SourceMapping):
     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]:
         """

slither/detectors/all_detectors.py

@@ -81,3 +81,4 @@ from .functions.dead_code import DeadCode
 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

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

slither/slithir/operations/high_level_call.py

@@ -128,6 +128,10 @@ class HighLevelCall(Call, OperationWithLValue):
             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):

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 @@ class VariableDeclarationSolc:
         """
         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 @@ class VariableDeclarationSolc:
             if attributes["mutability"] == "immutable":
                 self._variable.is_immutable = True
 
+        self._handle_comment(attributes)
+
         self._analyze_variable_attributes(attributes)
 
         if self._is_compact_ast:

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);
+    }
+}
+

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"
+        }
+    ]
+]

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);
+    }
+}
+

tests/detectors/reentrancy-no-eth/0.8.2/comment.sol.0.8.2.ReentrancyReadBeforeWritten.json

@@ -0,0 +1,3 @@
+[
+    []
+]

tests/test_detectors.py

@@ -116,6 +116,11 @@ ALL_TEST_OBJECTS = [
         "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 @@ ALL_TEST_OBJECTS = [
         "delegatecall_loop.sol",
         "0.8.0",
     ),
+    Test(
+        all_detectors.ProtectedVariables,
+        "comment.sol",
+        "0.8.2",
+    ),
 ]