diff --git a/slither/detectors/statements/unprotected_upgradeable.py b/slither/detectors/statements/unprotected_upgradeable.py index 1916885b0e..7508f718f7 100644 --- a/slither/detectors/statements/unprotected_upgradeable.py +++ b/slither/detectors/statements/unprotected_upgradeable.py @@ -47,10 +47,14 @@ def _has_initializing_protection(functions: list[Function]) -> bool: return False +_WHITELISTED_MODIFIERS = {"onlyProxy", "onlyDelegateCall"} + + def _whitelisted_modifiers(f: Function) -> bool: - # The onlyProxy modifier prevents calling the implementation contract (must be delegatecall) + # The onlyProxy / onlyDelegateCall modifiers prevent calling the implementation contract + # directly (the call must go through delegatecall via the proxy). # https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/3dec82093ea4a490d63aab3e925fed4f692909e8/contracts/proxy/utils/UUPSUpgradeable.sol#L38-L42 - return "onlyProxy" not in [modifier.name for modifier in f.modifiers] + return not any(m.name in _WHITELISTED_MODIFIERS for m in f.modifiers) def _initialize_functions(contract: Contract) -> list[Function]: diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/OnlyDelegateCall.sol b/tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/OnlyDelegateCall.sol new file mode 100644 index 0000000000..5337332448 --- /dev/null +++ b/tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/OnlyDelegateCall.sol @@ -0,0 +1,5 @@ +contract OnlyDelegateCall { + modifier onlyDelegateCall() { + _; + } +} diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/whitelisted.sol b/tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/whitelisted.sol index d33a1816a6..db51f0da3e 100644 --- a/tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/whitelisted.sol +++ b/tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/whitelisted.sol @@ -1,5 +1,6 @@ import "./Initializable.sol"; import "./OnlyProxy.sol"; +import "./OnlyDelegateCall.sol"; contract Whitelisted is Initializable, OnlyProxy{ address owner; @@ -13,3 +14,16 @@ contract Whitelisted is Initializable, OnlyProxy{ selfdestruct(owner); } } + +contract WhitelistedDelegateCall is Initializable, OnlyDelegateCall{ + address owner; + + function initialize() external initializer onlyDelegateCall { + owner = msg.sender; + } + + function kill() external { + require(msg.sender == owner); + selfdestruct(owner); + } +} diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/whitelisted.sol-0.4.25.zip b/tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/whitelisted.sol-0.4.25.zip index 64fdf952bb..88c6f3a6c3 100644 Binary files a/tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/whitelisted.sol-0.4.25.zip and b/tests/e2e/detectors/test_data/unprotected-upgrade/0.4.25/whitelisted.sol-0.4.25.zip differ diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/OnlyDelegateCall.sol b/tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/OnlyDelegateCall.sol new file mode 100644 index 0000000000..5337332448 --- /dev/null +++ b/tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/OnlyDelegateCall.sol @@ -0,0 +1,5 @@ +contract OnlyDelegateCall { + modifier onlyDelegateCall() { + _; + } +} diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/whitelisted.sol b/tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/whitelisted.sol index c99b1929f2..007aff1860 100644 --- a/tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/whitelisted.sol +++ b/tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/whitelisted.sol @@ -1,5 +1,6 @@ import "./Initializable.sol"; import "./OnlyProxy.sol"; +import "./OnlyDelegateCall.sol"; contract Whitelisted is Initializable, OnlyProxy{ address payable owner; @@ -13,3 +14,16 @@ contract Whitelisted is Initializable, OnlyProxy{ selfdestruct(owner); } } + +contract WhitelistedDelegateCall is Initializable, OnlyDelegateCall{ + address payable owner; + + function initialize() external initializer onlyDelegateCall { + owner = msg.sender; + } + + function kill() external { + require(msg.sender == owner); + selfdestruct(owner); + } +} diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/whitelisted.sol-0.5.16.zip b/tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/whitelisted.sol-0.5.16.zip index 0700332b73..a14606d2da 100644 Binary files a/tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/whitelisted.sol-0.5.16.zip and b/tests/e2e/detectors/test_data/unprotected-upgrade/0.5.16/whitelisted.sol-0.5.16.zip differ diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/OnlyDelegateCall.sol b/tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/OnlyDelegateCall.sol new file mode 100644 index 0000000000..5337332448 --- /dev/null +++ b/tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/OnlyDelegateCall.sol @@ -0,0 +1,5 @@ +contract OnlyDelegateCall { + modifier onlyDelegateCall() { + _; + } +} diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/whitelisted.sol b/tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/whitelisted.sol index c99b1929f2..007aff1860 100644 --- a/tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/whitelisted.sol +++ b/tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/whitelisted.sol @@ -1,5 +1,6 @@ import "./Initializable.sol"; import "./OnlyProxy.sol"; +import "./OnlyDelegateCall.sol"; contract Whitelisted is Initializable, OnlyProxy{ address payable owner; @@ -13,3 +14,16 @@ contract Whitelisted is Initializable, OnlyProxy{ selfdestruct(owner); } } + +contract WhitelistedDelegateCall is Initializable, OnlyDelegateCall{ + address payable owner; + + function initialize() external initializer onlyDelegateCall { + owner = msg.sender; + } + + function kill() external { + require(msg.sender == owner); + selfdestruct(owner); + } +} diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/whitelisted.sol-0.6.11.zip b/tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/whitelisted.sol-0.6.11.zip index 5b24376e09..c2c54c4df3 100644 Binary files a/tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/whitelisted.sol-0.6.11.zip and b/tests/e2e/detectors/test_data/unprotected-upgrade/0.6.11/whitelisted.sol-0.6.11.zip differ diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/OnlyDelegateCall.sol b/tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/OnlyDelegateCall.sol new file mode 100644 index 0000000000..5337332448 --- /dev/null +++ b/tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/OnlyDelegateCall.sol @@ -0,0 +1,5 @@ +contract OnlyDelegateCall { + modifier onlyDelegateCall() { + _; + } +} diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/whitelisted.sol b/tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/whitelisted.sol index c99b1929f2..007aff1860 100644 --- a/tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/whitelisted.sol +++ b/tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/whitelisted.sol @@ -1,5 +1,6 @@ import "./Initializable.sol"; import "./OnlyProxy.sol"; +import "./OnlyDelegateCall.sol"; contract Whitelisted is Initializable, OnlyProxy{ address payable owner; @@ -13,3 +14,16 @@ contract Whitelisted is Initializable, OnlyProxy{ selfdestruct(owner); } } + +contract WhitelistedDelegateCall is Initializable, OnlyDelegateCall{ + address payable owner; + + function initialize() external initializer onlyDelegateCall { + owner = msg.sender; + } + + function kill() external { + require(msg.sender == owner); + selfdestruct(owner); + } +} diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/whitelisted.sol-0.7.6.zip b/tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/whitelisted.sol-0.7.6.zip index d1d3ce9e81..fd35ab8bba 100644 Binary files a/tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/whitelisted.sol-0.7.6.zip and b/tests/e2e/detectors/test_data/unprotected-upgrade/0.7.6/whitelisted.sol-0.7.6.zip differ diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/OnlyDelegateCall.sol b/tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/OnlyDelegateCall.sol new file mode 100644 index 0000000000..5337332448 --- /dev/null +++ b/tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/OnlyDelegateCall.sol @@ -0,0 +1,5 @@ +contract OnlyDelegateCall { + modifier onlyDelegateCall() { + _; + } +} diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/whitelisted.sol b/tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/whitelisted.sol index dafeff691d..f9b3748778 100644 --- a/tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/whitelisted.sol +++ b/tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/whitelisted.sol @@ -1,5 +1,6 @@ import "./Initializable.sol"; import "./OnlyProxy.sol"; +import "./OnlyDelegateCall.sol"; contract Whitelisted is Initializable, OnlyProxy{ address payable owner; @@ -13,3 +14,16 @@ contract Whitelisted is Initializable, OnlyProxy{ selfdestruct(owner); } } + +contract WhitelistedDelegateCall is Initializable, OnlyDelegateCall{ + address payable owner; + + function initialize() external initializer onlyDelegateCall { + owner = payable(msg.sender); + } + + function kill() external { + require(msg.sender == owner); + selfdestruct(owner); + } +} diff --git a/tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/whitelisted.sol-0.8.15.zip b/tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/whitelisted.sol-0.8.15.zip index 4afbcf0760..4299acb32c 100644 Binary files a/tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/whitelisted.sol-0.8.15.zip and b/tests/e2e/detectors/test_data/unprotected-upgrade/0.8.15/whitelisted.sol-0.8.15.zip differ