Add Slither detector for upgrade storage gaps

slither/README.md

@@ -11,6 +11,7 @@ Custom Slither static analysis detectors that mirror the Foundry test checks.
 | `custom-flash-loan-detector` | Low | High | Functions with names containing `swap`/`liquidate`/`redeem`/`execute` that read external state and write state atomically — classic flash loan attack surface | ```solidity\nfunction swap(address tokenIn, address tokenOut) external {\n    uint256 price = IUniswapV2Pair(pair).getReserves()[0];  // read external\n    uint256 amountOut = price * amountIn / 1e18;              // write state\n    IERC20(tokenOut).transfer(msg.sender, amountOut);         // external call\n}\n``` | `slither . --detect custom-flash-loan-detector` |
 | `custom-oracle-manipulation-detector` | Medium | High | Spot price usage (`getPrice`, `latestPrice`, `spotPrice`) without TWAP, single-source oracle without fallback, price-dependent state changes on uncached values | ```solidity\nfunction getAmountOut(uint256 amountIn) public view returns (uint256) {\n    uint256 price = pair.getReserves()[0];      // spot price, no TWAP\n    return price * amountIn / 1e18;\n}\n\nfunction executeTrade() external {\n    uint256 price = getAmountOut(1000);          // reads spot → state change\n    // vulnerable to flash loan manipulation\n}\n``` | `slither . --detect custom-oracle-manipulation-detector` |
 | `custom-governance-detector` | Medium | High | `execute`/`propose` functions without timelock delay, governance parameter changes (`setFee`, `setDelay`, `updateThreshold`) without `onlyGovernance` modifier | ```solidity\nfunction execute(uint256 proposalId) external {\n    // no timelock check — executes immediately\n    targets[proposalId].call(calldata[proposalId]);\n}\n\nfunction setFee(uint256 newFee) external {       // public, no modifier\n    fee = newFee;                                 // critical param change\n}\n``` | `slither . --detect custom-governance-detector` |
+| `custom-upgrade-gap` | Medium | Medium | Upgradeable contracts that append storage variables after `__gap`, which can corrupt proxy storage layouts after upgrades | ```solidity\nuint256[49] private __gap;\nuint256 public newFeeBps; // appended after gap\n``` | `slither . --detect custom-upgrade-gap` |
 
 ## Usage
 
@@ -25,6 +26,15 @@ slither . --config slither.config.json
 slither . --detect custom-reentrancy-detector
 ```
 
+### Run the upgrade-gap detector tests
+```bash
+python3 -m unittest discover -s slither/tests -q
+```
+
+The upgrade-gap fixtures model the intended pattern:
+- `upgrade_gap_bad.sol` appends `newFeeBps` after `__gap` and should be flagged.
+- `upgrade_gap_good.sol` places `newFeeBps` before a shrunken gap and should be ignored.
+
 ### Run with SARIF output
 ```bash
 slither . --config slither.config.json --sarif output.sarif
@@ -39,6 +49,7 @@ slither . --config slither.config.json --sarif output.sarif
 | `custom-flash-loan-detector` | [`slither/detectors/custom_flash_loan.py`](slither/detectors/custom_flash_loan.py) |
 | `custom-oracle-manipulation-detector` | [`slither/detectors/custom_oracle.py`](slither/detectors/custom_oracle.py) |
 | `custom-governance-detector` | [`slither/detectors/custom_governance.py`](slither/detectors/custom_governance.py) |
+| `custom-upgrade-gap` | [`slither/detectors/custom_upgrade_gap.py`](slither/detectors/custom_upgrade_gap.py) |
 
 ## Confidence vs Impact
 

slither/detectors/custom_upgrade_gap.py

@@ -0,0 +1,92 @@
+"""Custom Slither detector for upgradeable storage gap regressions.
+
+Detects upgradeable contracts that declare new state variables after a
+reserved `__gap` array. In OpenZeppelin-style upgradeable contracts, new
+storage belongs before the gap and the gap should shrink by the same slot
+width; appending after `__gap` defeats that convention and can mask layout
+regressions.
+"""
+try:
+    from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+except ModuleNotFoundError:
+    class DetectorClassification:
+        MEDIUM = "medium"
+
+    class AbstractDetector:
+        pass
+
+
+UPGRADEABLE_BASE_PATTERNS = (
+    "initializable",
+    "uupsupgradeable",
+    "transparentupgradeableproxy",
+    "erc1967",
+    "upgradeable",
+)
+
+
+def _name_of(item) -> str:
+    return str(getattr(item, "name", item)).lower()
+
+
+def _declared_state_variables(contract) -> list:
+    return list(getattr(contract, "state_variables_declared", None) or contract.state_variables)
+
+
+def _is_storage_gap(variable) -> bool:
+    return _name_of(variable) == "__gap"
+
+
+def _is_persistent_state(variable) -> bool:
+    if _is_storage_gap(variable):
+        return False
+    if getattr(variable, "is_constant", False) or getattr(variable, "is_immutable", False):
+        return False
+    return True
+
+
+def is_upgradeable_contract(contract) -> bool:
+    names = [_name_of(contract)]
+    names.extend(_name_of(base) for base in getattr(contract, "inheritance", []))
+    return any(pattern in name for name in names for pattern in UPGRADEABLE_BASE_PATTERNS)
+
+
+def find_gap_order_violations(contract) -> list:
+    """Return state variables declared after `__gap` in an upgradeable contract."""
+    if getattr(contract, "is_interface", False) or not is_upgradeable_contract(contract):
+        return []
+
+    violations = []
+    saw_gap = False
+    for variable in _declared_state_variables(contract):
+        if _is_storage_gap(variable):
+            saw_gap = True
+            continue
+        if saw_gap and _is_persistent_state(variable):
+            violations.append(variable)
+    return violations
+
+
+class CustomUpgradeGapDetector(AbstractDetector):
+    ARGUMENT = "custom-upgrade-gap"
+    HELP = "Detects upgradeable storage variables appended after __gap"
+    IMPACT = DetectorClassification.MEDIUM
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = "https://github.com/kcolbchain/audit-checklist/wiki/Upgrade-Gap-Detector"
+    WIKI_TITLE = "Custom Upgrade Gap Detector"
+
+    def _detect(self) -> list:
+        results = []
+        for contract in self.compilation_unit.contracts:
+            for variable in find_gap_order_violations(contract):
+                info = [
+                    "Upgradeable storage variable declared after __gap: ",
+                    f"{variable.name} in {contract.name}\n",
+                    "\nMove the new variable before __gap and shrink the gap array by the same slot width.\n",
+                ]
+                res = self.generate_result(info)
+                res.add(variable)
+                results.append(res)
+
+        return results

slither/tests/fixtures/upgrade_gap_bad.sol

@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+contract Initializable {}
+
+contract UUPSUpgradeable is Initializable {}
+
+contract BadUpgradeGap is UUPSUpgradeable {
+    uint256 public existingValue;
+    uint256[50] private __gap;
+
+    uint256 public newFeeBps;
+}

slither/tests/fixtures/upgrade_gap_good.sol

@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+contract Initializable {}
+
+contract UUPSUpgradeable is Initializable {}
+
+contract GoodUpgradeGap is UUPSUpgradeable {
+    uint256 public existingValue;
+    uint256 public newFeeBps;
+
+    uint256[49] private __gap;
+}

slither/tests/test_custom_upgrade_gap.py

@@ -0,0 +1,79 @@
+import importlib.util
+import pathlib
+import types
+import unittest
+
+
+ROOT = pathlib.Path(__file__).resolve().parents[2]
+DETECTOR_PATH = ROOT / "slither" / "detectors" / "custom_upgrade_gap.py"
+
+spec = importlib.util.spec_from_file_location("custom_upgrade_gap", DETECTOR_PATH)
+custom_upgrade_gap = importlib.util.module_from_spec(spec)
+assert spec.loader is not None
+spec.loader.exec_module(custom_upgrade_gap)
+
+
+def variable(name, type_name="uint256"):
+    return types.SimpleNamespace(
+        name=name,
+        type=type_name,
+        is_constant=False,
+        is_immutable=False,
+    )
+
+
+def contract(name, bases, state_variables):
+    return types.SimpleNamespace(
+        name=name,
+        inheritance=[types.SimpleNamespace(name=base) for base in bases],
+        state_variables_declared=state_variables,
+        state_variables=state_variables,
+        is_interface=False,
+    )
+
+
+class CustomUpgradeGapDetectorTest(unittest.TestCase):
+    def test_flags_variable_declared_after_gap(self):
+        candidate = contract(
+            "BadUpgradeGap",
+            ["UUPSUpgradeable"],
+            [
+                variable("existingValue"),
+                variable("__gap", "uint256[50]"),
+                variable("newFeeBps"),
+            ],
+        )
+
+        violations = custom_upgrade_gap.find_gap_order_violations(candidate)
+
+        self.assertEqual([item.name for item in violations], ["newFeeBps"])
+
+    def test_ignores_variable_before_shrunken_gap(self):
+        candidate = contract(
+            "GoodUpgradeGap",
+            ["UUPSUpgradeable"],
+            [
+                variable("existingValue"),
+                variable("newFeeBps"),
+                variable("__gap", "uint256[49]"),
+            ],
+        )
+
+        self.assertEqual(custom_upgrade_gap.find_gap_order_violations(candidate), [])
+
+    def test_ignores_non_upgradeable_contracts(self):
+        candidate = contract(
+            "PlainContract",
+            [],
+            [
+                variable("__gap", "uint256[50]"),
+                variable("newFeeBps"),
+            ],
+        )
+
+        self.assertFalse(custom_upgrade_gap.is_upgradeable_contract(candidate))
+        self.assertEqual(custom_upgrade_gap.find_gap_order_violations(candidate), [])
+
+
+if __name__ == "__main__":
+    unittest.main()