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 @@
 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]:
         """

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

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 @@ 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):

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:

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