Merge pull request #1344 from plotchy/dev_fp_uuc

add _disableInitializers() detection
crytic · Aug 17, 2022 · d8e526e · d8e526e
2 parents 10413a3 + cc9b463
commit d8e526e
Show file tree

Hide file tree

Showing 12 changed files with 330 additions and 5 deletions.
diff --git a/slither/detectors/statements/unprotected_upgradeable.py b/slither/detectors/statements/unprotected_upgradeable.py
@@ -22,11 +22,20 @@ def _can_be_destroyed(contract: Contract) -> List[Function]:
     return targets
 
 
-def _has_initializer_modifier(functions: List[Function]) -> bool:
+def _has_initializing_protection(functions: List[Function]) -> bool:
+    # Detects "initializer" constructor modifiers and "_disableInitializers()" constructor internal calls
+    # https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#initializing_the_implementation_contract
+
     for f in functions:
         for m in f.modifiers:
             if m.name == "initializer":
                 return True
+        for ifc in f.all_internal_calls():
+            if ifc.name == "_disableInitializers":
+                return True
+
+    # to avoid future FPs in different modifier + function naming implementations, we can also implement a broader check for state var "_initialized" being written to in the constructor
+    #   though this is still subject to naming false positives...
     return False
 
 
@@ -82,7 +91,7 @@ def _detect(self):
 
         for contract in self.compilation_unit.contracts_derived:
             if contract.is_upgradeable:
-                if not _has_initializer_modifier(contract.constructors):
+                if not _has_initializing_protection(contract.constructors):
                     functions_that_can_destroy = _can_be_destroyed(contract)
                     if functions_that_can_destroy:
                         initialize_functions = _initialize_functions(contract)

diff --git a/tests/detectors/unprotected-upgrade/0.7.6/Fixed.sol b/tests/detectors/unprotected-upgrade/0.7.6/Fixed.sol
@@ -47,6 +47,24 @@ contract Fixed2 is Initializable {
         owner = msg.sender;
     }
 
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}
+
+contract Fixed3 is Initializable {
+    address payable owner;
+
+    constructor() {
+        _disableInitializers();
+    }
+
+    function initialize() external initializer {
+        require(owner == address(0));
+        owner = payable(msg.sender);
+    }
+
     function kill() external {
         require(msg.sender == owner);
         selfdestruct(owner);

diff --git a/tests/detectors/unprotected-upgrade/0.7.6/Initializable.sol b/tests/detectors/unprotected-upgrade/0.7.6/Initializable.sol
@@ -1,5 +1,15 @@
 contract Initializable{
-      modifier initializer() {
-          _;
-      }
+    uint8 private _initialized;
+    bool private _initializing;
+
+    modifier initializer() {
+        _;
+    }
+
+    function _disableInitializers() internal virtual {
+        require(!_initializing, "Initializable: contract is initializing");
+        if (_initialized < type(uint8).max) {
+            _initialized = type(uint8).max;
+        }
+    }
 }
diff --git a/tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol b/tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol
@@ -0,0 +1,14 @@
+import "./Initializable.sol";
+
+contract Buggy is Initializable{
+    address payable owner;
+
+    function initialize() external initializer{
+        require(owner == address(0));
+        owner = payable(msg.sender);
+    }
+    function kill() external{
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}
diff --git a/tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol.0.8.15.UnprotectedUpgradeable.json b/tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol.0.8.15.UnprotectedUpgradeable.json
@@ -0,0 +1,145 @@
+[
+    [
+        {
+            "elements": [
+                {
+                    "type": "contract",
+                    "name": "Buggy",
+                    "source_mapping": {
+                        "start": 31,
+                        "length": 294,
+                        "filename_relative": "tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol",
+                        "filename_absolute": "/GENERIC_PATH",
+                        "filename_short": "tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol",
+                        "is_dependency": false,
+                        "lines": [
+                            3,
+                            4,
+                            5,
+                            6,
+                            7,
+                            8,
+                            9,
+                            10,
+                            11,
+                            12,
+                            13,
+                            14
+                        ],
+                        "starting_column": 1,
+                        "ending_column": 2
+                    }
+                },
+                {
+                    "type": "function",
+                    "name": "initialize",
+                    "source_mapping": {
+                        "start": 96,
+                        "length": 124,
+                        "filename_relative": "tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol",
+                        "filename_absolute": "/GENERIC_PATH",
+                        "filename_short": "tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol",
+                        "is_dependency": false,
+                        "lines": [
+                            6,
+                            7,
+                            8,
+                            9
+                        ],
+                        "starting_column": 5,
+                        "ending_column": 6
+                    },
+                    "type_specific_fields": {
+                        "parent": {
+                            "type": "contract",
+                            "name": "Buggy",
+                            "source_mapping": {
+                                "start": 31,
+                                "length": 294,
+                                "filename_relative": "tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol",
+                                "filename_absolute": "/GENERIC_PATH",
+                                "filename_short": "tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol",
+                                "is_dependency": false,
+                                "lines": [
+                                    3,
+                                    4,
+                                    5,
+                                    6,
+                                    7,
+                                    8,
+                                    9,
+                                    10,
+                                    11,
+                                    12,
+                                    13,
+                                    14
+                                ],
+                                "starting_column": 1,
+                                "ending_column": 2
+                            }
+                        },
+                        "signature": "initialize()"
+                    }
+                },
+                {
+                    "type": "function",
+                    "name": "kill",
+                    "source_mapping": {
+                        "start": 225,
+                        "length": 98,
+                        "filename_relative": "tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol",
+                        "filename_absolute": "/GENERIC_PATH",
+                        "filename_short": "tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol",
+                        "is_dependency": false,
+                        "lines": [
+                            10,
+                            11,
+                            12,
+                            13
+                        ],
+                        "starting_column": 5,
+                        "ending_column": 6
+                    },
+                    "type_specific_fields": {
+                        "parent": {
+                            "type": "contract",
+                            "name": "Buggy",
+                            "source_mapping": {
+                                "start": 31,
+                                "length": 294,
+                                "filename_relative": "tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol",
+                                "filename_absolute": "/GENERIC_PATH",
+                                "filename_short": "tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol",
+                                "is_dependency": false,
+                                "lines": [
+                                    3,
+                                    4,
+                                    5,
+                                    6,
+                                    7,
+                                    8,
+                                    9,
+                                    10,
+                                    11,
+                                    12,
+                                    13,
+                                    14
+                                ],
+                                "starting_column": 1,
+                                "ending_column": 2
+                            }
+                        },
+                        "signature": "kill()"
+                    }
+                }
+            ],
+            "description": "Buggy (tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol#3-14) is an upgradeable contract that does not protect its initialize functions: Buggy.initialize() (tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol#6-9). Anyone can delete the contract with: Buggy.kill() (tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol#10-13)",
+            "markdown": "[Buggy](tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol#L3-L14) is an upgradeable contract that does not protect its initialize functions: [Buggy.initialize()](tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol#L6-L9). Anyone can delete the contract with: [Buggy.kill()](tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol#L10-L13)",
+            "first_markdown_element": "tests/detectors/unprotected-upgrade/0.8.15/Buggy.sol#L3-L14",
+            "id": "d85b90230632a30f7ffb5140a791d4a9ae8b0be045c5b27175f3c477e189c08c",
+            "check": "unprotected-upgrade",
+            "impact": "High",
+            "confidence": "High"
+        }
+    ]
+]
diff --git a/tests/detectors/unprotected-upgrade/0.8.15/Fixed.sol b/tests/detectors/unprotected-upgrade/0.8.15/Fixed.sol
@@ -0,0 +1,73 @@
+import "./Initializable.sol";
+
+contract Fixed is Initializable{
+    address payable owner;
+
+    constructor() {
+        owner = payable(msg.sender);
+    }
+
+    function initialize() external initializer{
+        require(owner == address(0));
+        owner = payable(msg.sender);
+
+    }
+    function kill() external{
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+
+    function other_function() external{
+
+    }
+}
+
+contract Not_Upgradeable{
+}
+
+contract UpgradeableNoDestruct is Initializable{
+    address payable owner;
+
+    constructor() {
+        owner = payable(msg.sender);
+    }
+
+    function initialize() external initializer{
+        require(owner == address(0));
+        owner = payable(msg.sender);
+    }
+}
+
+contract Fixed2 is Initializable {
+    address payable owner;
+
+    constructor() initializer {}
+
+    function initialize() external initializer {
+        require(owner == address(0));
+        owner = payable(msg.sender);
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}
+
+contract Fixed3 is Initializable {
+    address payable owner;
+
+    constructor() {
+        _disableInitializers();
+    }
+
+    function initialize() external initializer {
+        require(owner == address(0));
+        owner = payable(msg.sender);
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}
diff --git a/tests/detectors/unprotected-upgrade/0.8.15/Fixed.sol.0.8.15.UnprotectedUpgradeable.json b/tests/detectors/unprotected-upgrade/0.8.15/Fixed.sol.0.8.15.UnprotectedUpgradeable.json
@@ -0,0 +1,3 @@
+[
+    []
+]
diff --git a/tests/detectors/unprotected-upgrade/0.8.15/Initializable.sol b/tests/detectors/unprotected-upgrade/0.8.15/Initializable.sol
@@ -0,0 +1,15 @@
+contract Initializable {
+    uint8 private _initialized;
+    bool private _initializing;
+
+    modifier initializer() {
+        _;
+    }
+
+    function _disableInitializers() internal virtual {
+        require(!_initializing, "Initializable: contract is initializing");
+        if (_initialized < type(uint8).max) {
+            _initialized = type(uint8).max;
+        }
+    }
+}
diff --git a/tests/detectors/unprotected-upgrade/0.8.15/OnlyProxy.sol b/tests/detectors/unprotected-upgrade/0.8.15/OnlyProxy.sol
@@ -0,0 +1,5 @@
+contract OnlyProxy {
+    modifier onlyProxy() {
+        _;
+    }
+}
diff --git a/tests/detectors/unprotected-upgrade/0.8.15/whitelisted.sol b/tests/detectors/unprotected-upgrade/0.8.15/whitelisted.sol
@@ -0,0 +1,15 @@
+import "./Initializable.sol";
+import "./OnlyProxy.sol";
+
+contract Whitelisted is Initializable, OnlyProxy{
+    address payable owner;
+
+    function initialize() external initializer onlyProxy {
+        owner = payable(msg.sender);
+    }
+
+    function kill() external {
+        require(msg.sender == owner);
+        selfdestruct(owner);
+    }
+}
diff --git a/...s/detectors/unprotected-upgrade/0.8.15/whitelisted.sol.0.8.15.UnprotectedUpgradeable.json b/...s/detectors/unprotected-upgrade/0.8.15/whitelisted.sol.0.8.15.UnprotectedUpgradeable.json
@@ -0,0 +1,3 @@
+[
+    []
+]
diff --git a/tests/test_detectors.py b/tests/test_detectors.py
@@ -928,6 +928,21 @@ def id_test(test_item: Test):
         "whitelisted.sol",
         "0.7.6",
     ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "Buggy.sol",
+        "0.8.15",
+    ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "Fixed.sol",
+        "0.8.15",
+    ),
+    Test(
+        all_detectors.UnprotectedUpgradeable,
+        "whitelisted.sol",
+        "0.8.15",
+    ),
     Test(
         all_detectors.ABIEncoderV2Array,
         "storage_ABIEncoderV2_array.sol",
-Original file line number
+Diff line change
@@ -0,0 +1,3 @@
+    [
+        []
+    ]