diff --git a/slither/README.md b/slither/README.md index 891345f..6f8c1f1 100644 --- a/slither/README.md +++ b/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 diff --git a/slither/detectors/custom_upgrade_gap.py b/slither/detectors/custom_upgrade_gap.py new file mode 100644 index 0000000..e823847 --- /dev/null +++ b/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 diff --git a/slither/tests/fixtures/upgrade_gap_bad.sol b/slither/tests/fixtures/upgrade_gap_bad.sol new file mode 100644 index 0000000..21840b0 --- /dev/null +++ b/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; +} diff --git a/slither/tests/fixtures/upgrade_gap_good.sol b/slither/tests/fixtures/upgrade_gap_good.sol new file mode 100644 index 0000000..700d440 --- /dev/null +++ b/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; +} diff --git a/slither/tests/test_custom_upgrade_gap.py b/slither/tests/test_custom_upgrade_gap.py new file mode 100644 index 0000000..765635c --- /dev/null +++ b/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()