Ignore private __gap variables in shadowing detectors (#1117)

* Don't consider shadowing of private __gap variables in state and abstract shadowing detectors
crytic · Mar 18, 2022 · c57e272 · c57e272
1 parent dc0cec2
commit c57e272
Show file tree

Hide file tree

Showing 13 changed files with 294 additions and 1 deletion.
diff --git a/slither/core/solidity_types/__init__.py b/slither/core/solidity_types/__init__.py
@@ -3,4 +3,5 @@
 from .function_type import FunctionType
 from .mapping_type import MappingType
 from .user_defined_type import UserDefinedType
+from .type import Type
 from .type_information import TypeInformation
diff --git a/slither/detectors/shadowing/abstract.py b/slither/detectors/shadowing/abstract.py
@@ -8,6 +8,7 @@
 from slither.core.variables.state_variable import StateVariable
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
 from slither.utils.output import Output, AllSupportedOutput
+from .common import is_upgradable_gap_variable
 
 
 def detect_shadowing(contract: Contract) -> List[List[StateVariable]]:
@@ -19,6 +20,9 @@ def detect_shadowing(contract: Contract) -> List[List[StateVariable]]:
 
     var: StateVariable
     for var in contract.state_variables_declared:
+        if is_upgradable_gap_variable(contract, var):
+            continue
+
         shadow: List[StateVariable] = [v for v in variables_fathers if v.name == var.name]
         if shadow:
             ret.append([var] + shadow)

diff --git a/slither/detectors/shadowing/common.py b/slither/detectors/shadowing/common.py
@@ -0,0 +1,30 @@
+from slither.core.declarations import Contract
+from slither.core.variables.state_variable import StateVariable
+from slither.core.solidity_types import ArrayType, ElementaryType
+
+
+def is_upgradable_gap_variable(contract: Contract, variable: StateVariable) -> bool:
+    """Helper function that returns true if 'variable' is a gap variable used
+    for upgradable contracts. More specifically, the function returns true if:
+     - variable is named "__gap"
+     - it is a uint256 array declared at the end of the contract
+     - it has private visibility"""
+
+    # Return early on if the variable name is != gap to avoid iterating over all the state variables
+    if variable.name != "__gap":
+        return False
+
+    declared_variable_ordered = [
+        v for v in contract.state_variables_ordered if v in contract.state_variables_declared
+    ]
+
+    if not declared_variable_ordered:
+        return False
+
+    variable_type = variable.type
+    return (
+        declared_variable_ordered[-1] is variable
+        and isinstance(variable_type, ArrayType)
+        and variable_type.type == ElementaryType("uint256")
+        and variable.visibility == "private"
+    )
diff --git a/slither/detectors/shadowing/state.py b/slither/detectors/shadowing/state.py
@@ -3,16 +3,22 @@
 """
 
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.core.declarations import Contract
+from .common import is_upgradable_gap_variable
 
 
-def detect_shadowing(contract):
+def detect_shadowing(contract: Contract):
     ret = []
     variables_fathers = []
     for father in contract.inheritance:
         if any(f.is_implemented for f in father.functions + father.modifiers):
             variables_fathers += father.state_variables_declared
 
     for var in contract.state_variables_declared:
+        # Ignore __gap variables for updatable contracts
+        if is_upgradable_gap_variable(contract, var):
+            continue
+
         shadow = [v for v in variables_fathers if v.name == var.name]
         if shadow:
             ret.append([var] + shadow)

diff --git a/tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol b/tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol
@@ -0,0 +1,8 @@
+pragma solidity ^0.7.5;
+contract BaseContract{
+    uint256[50] private __gap;
+}
+
+contract DerivedContract is BaseContract{
+    uint256[50] public __gap;
+}
diff --git a/...rs/shadowing-abstract/0.7.5/public_gap_variable.sol.0.7.5.ShadowingAbstractDetection.json b/...rs/shadowing-abstract/0.7.5/public_gap_variable.sol.0.7.5.ShadowingAbstractDetection.json
@@ -0,0 +1,95 @@
+[
+    [
+        {
+            "elements": [
+                {
+                    "type": "variable",
+                    "name": "__gap",
+                    "source_mapping": {
+                        "start": 127,
+                        "length": 24,
+                        "filename_used": "/GENERIC_PATH",
+                        "filename_relative": "tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol",
+                        "filename_absolute": "/GENERIC_PATH",
+                        "filename_short": "tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol",
+                        "is_dependency": false,
+                        "lines": [
+                            7
+                        ],
+                        "starting_column": 5,
+                        "ending_column": 29
+                    },
+                    "type_specific_fields": {
+                        "parent": {
+                            "type": "contract",
+                            "name": "DerivedContract",
+                            "source_mapping": {
+                                "start": 81,
+                                "length": 73,
+                                "filename_used": "/GENERIC_PATH",
+                                "filename_relative": "tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol",
+                                "filename_absolute": "/GENERIC_PATH",
+                                "filename_short": "tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol",
+                                "is_dependency": false,
+                                "lines": [
+                                    6,
+                                    7,
+                                    8
+                                ],
+                                "starting_column": 1,
+                                "ending_column": 2
+                            }
+                        }
+                    }
+                },
+                {
+                    "type": "variable",
+                    "name": "__gap",
+                    "source_mapping": {
+                        "start": 51,
+                        "length": 25,
+                        "filename_used": "/GENERIC_PATH",
+                        "filename_relative": "tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol",
+                        "filename_absolute": "/GENERIC_PATH",
+                        "filename_short": "tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol",
+                        "is_dependency": false,
+                        "lines": [
+                            3
+                        ],
+                        "starting_column": 5,
+                        "ending_column": 30
+                    },
+                    "type_specific_fields": {
+                        "parent": {
+                            "type": "contract",
+                            "name": "BaseContract",
+                            "source_mapping": {
+                                "start": 24,
+                                "length": 55,
+                                "filename_used": "/GENERIC_PATH",
+                                "filename_relative": "tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol",
+                                "filename_absolute": "/GENERIC_PATH",
+                                "filename_short": "tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol",
+                                "is_dependency": false,
+                                "lines": [
+                                    2,
+                                    3,
+                                    4
+                                ],
+                                "starting_column": 1,
+                                "ending_column": 2
+                            }
+                        }
+                    }
+                }
+            ],
+            "description": "DerivedContract.__gap (tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol#7) shadows:\n\t- BaseContract.__gap (tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol#3)\n",
+            "markdown": "[DerivedContract.__gap](tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol#L7) shadows:\n\t- [BaseContract.__gap](tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol#L3)\n",
+            "first_markdown_element": "tests/detectors/shadowing-abstract/0.7.5/public_gap_variable.sol#L7",
+            "id": "8f81b2b4b3285fe96f0b580cdd2144cc6cf6808d970ba68878b9901744069c4c",
+            "check": "shadowing-abstract",
+            "impact": "Medium",
+            "confidence": "High"
+        }
+    ]
+]
diff --git a/tests/detectors/shadowing-abstract/0.7.5/shadowing_state_variable.sol b/tests/detectors/shadowing-abstract/0.7.5/shadowing_state_variable.sol
@@ -0,0 +1,8 @@
+pragma solidity ^0.7.5;
+contract BaseContract{
+    uint256[50] private __gap;
+}
+
+contract DerivedContract is BaseContract{
+    uint256[50] private __gap;
+}
diff --git a/...adowing-abstract/0.7.5/shadowing_state_variable.sol.0.7.5.ShadowingAbstractDetection.json b/...adowing-abstract/0.7.5/shadowing_state_variable.sol.0.7.5.ShadowingAbstractDetection.json
@@ -0,0 +1,3 @@
+[
+    []
+]
diff --git a/tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol b/tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol
@@ -0,0 +1,10 @@
+pragma solidity ^0.7.5;
+contract BaseContract{
+    uint256[50] private __gap;
+    function f() external {}
+}
+
+contract DerivedContract is BaseContract{
+    uint256[50] public __gap;
+    function g() external {}
+}
diff --git a/tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol.0.7.5.StateShadowing.json b/tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol.0.7.5.StateShadowing.json
@@ -0,0 +1,97 @@
+[
+    [
+        {
+            "elements": [
+                {
+                    "type": "variable",
+                    "name": "__gap",
+                    "source_mapping": {
+                        "start": 156,
+                        "length": 24,
+                        "filename_used": "/GENERIC_PATH",
+                        "filename_relative": "tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol",
+                        "filename_absolute": "/GENERIC_PATH",
+                        "filename_short": "tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol",
+                        "is_dependency": false,
+                        "lines": [
+                            8
+                        ],
+                        "starting_column": 5,
+                        "ending_column": 29
+                    },
+                    "type_specific_fields": {
+                        "parent": {
+                            "type": "contract",
+                            "name": "DerivedContract",
+                            "source_mapping": {
+                                "start": 110,
+                                "length": 102,
+                                "filename_used": "/GENERIC_PATH",
+                                "filename_relative": "tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol",
+                                "filename_absolute": "/GENERIC_PATH",
+                                "filename_short": "tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol",
+                                "is_dependency": false,
+                                "lines": [
+                                    7,
+                                    8,
+                                    9,
+                                    10
+                                ],
+                                "starting_column": 1,
+                                "ending_column": 2
+                            }
+                        }
+                    }
+                },
+                {
+                    "type": "variable",
+                    "name": "__gap",
+                    "source_mapping": {
+                        "start": 51,
+                        "length": 25,
+                        "filename_used": "/GENERIC_PATH",
+                        "filename_relative": "tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol",
+                        "filename_absolute": "/GENERIC_PATH",
+                        "filename_short": "tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol",
+                        "is_dependency": false,
+                        "lines": [
+                            3
+                        ],
+                        "starting_column": 5,
+                        "ending_column": 30
+                    },
+                    "type_specific_fields": {
+                        "parent": {
+                            "type": "contract",
+                            "name": "BaseContract",
+                            "source_mapping": {
+                                "start": 24,
+                                "length": 84,
+                                "filename_used": "/GENERIC_PATH",
+                                "filename_relative": "tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol",
+                                "filename_absolute": "/GENERIC_PATH",
+                                "filename_short": "tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol",
+                                "is_dependency": false,
+                                "lines": [
+                                    2,
+                                    3,
+                                    4,
+                                    5
+                                ],
+                                "starting_column": 1,
+                                "ending_column": 2
+                            }
+                        }
+                    }
+                }
+            ],
+            "description": "DerivedContract.__gap (tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol#8) shadows:\n\t- BaseContract.__gap (tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol#3)\n",
+            "markdown": "[DerivedContract.__gap](tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol#L8) shadows:\n\t- [BaseContract.__gap](tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol#L3)\n",
+            "first_markdown_element": "tests/detectors/shadowing-state/0.7.5/public_gap_variable.sol#L8",
+            "id": "8f81b2b4b3285fe96f0b580cdd2144cc6cf6808d970ba68878b9901744069c4c",
+            "check": "shadowing-state",
+            "impact": "High",
+            "confidence": "High"
+        }
+    ]
+]
diff --git a/tests/detectors/shadowing-state/0.7.5/shadowing_state_variable.sol b/tests/detectors/shadowing-state/0.7.5/shadowing_state_variable.sol
@@ -0,0 +1,8 @@
+pragma solidity ^0.7.5;
+contract BaseContract{
+    uint256[50] private __gap;
+}
+
+contract DerivedContract is BaseContract{
+    uint256[50] private __gap;
+}
diff --git a/tests/detectors/shadowing-state/0.7.5/shadowing_state_variable.sol.0.7.5.StateShadowing.json b/tests/detectors/shadowing-state/0.7.5/shadowing_state_variable.sol.0.7.5.StateShadowing.json
@@ -0,0 +1,3 @@
+[
+    []
+]
diff --git a/tests/test_detectors.py b/tests/test_detectors.py
@@ -635,6 +635,16 @@ def id_test(test_item: Test):
         "shadowing_abstract.sol",
         "0.5.16",
     ),
+    Test(
+        all_detectors.ShadowingAbstractDetection,
+        "shadowing_state_variable.sol",
+        "0.7.5",
+    ),
+    Test(
+        all_detectors.ShadowingAbstractDetection,
+        "public_gap_variable.sol",
+        "0.7.5",
+    ),
     Test(
         all_detectors.StateShadowing,
         "shadowing_state_variable.sol",
@@ -650,6 +660,16 @@ def id_test(test_item: Test):
         "shadowing_state_variable.sol",
         "0.6.11",
     ),
+    Test(
+        all_detectors.StateShadowing,
+        "shadowing_state_variable.sol",
+        "0.7.5",
+    ),
+    Test(
+        all_detectors.StateShadowing,
+        "public_gap_variable.sol",
+        "0.7.5",
+    ),
     Test(
         all_detectors.StateShadowing,
         "shadowing_state_variable.sol",
-Original file line number
+Diff line change
@@ -0,0 +1,3 @@
+    [
+        []
+    ]