[Bug-Candidate]: False positive for unprotected-upgrade detector when initializers are disabled

[bmarcot]

Describe the issue:

It seems unprotected-upgrade can be detected on contracts that have disabled initializers in their constructor. See the following example:

//SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.0;

import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

interface IUniswapV2 {
    function swapExactTokensForTokens(
        uint256 amount,
        address token0,
        address token1,
        address receiver
    ) external returns (uint256[] memory);
}

contract SlitherNotUnprotectedUpgrade is Initializable {
    address private immutable _router;

    constructor(address router) {
        _disableInitializers();
        _router = router;
    }

    function initialize() public initializer {
        // solhint-disable-prev-line no-empty-block
    }

    function swapExactTokensForTokens(
        uint256 amount,
        address token0,
        address token1,
        address receiver
    ) private returns (uint256[] memory) {
        // solhint-disable-next-line avoid-low-level-calls
        (bool success, bytes memory returndata) = _router.delegatecall(
            abi.encodeCall(IUniswapV2.swapExactTokensForTokens, (amount, token0, token1, receiver))
        );
        if (!success) {
            // solhint-disable-next-line no-inline-assembly
            assembly {
                revert(add(returndata, 32), mload(returndata))
            }
        }
        return abi.decode(returndata, (uint256[]));
    }
}

And detection by slither:

ethsec@7d5131fcbad1:/share$ slither .
SlitherNotUnprotectedUpgrade.swapExactTokensForTokens(uint256,address,address,address) (contracts/SlitherNotUnprotectedUpgrade.sol#27-44) uses delegatecall to a input-controlled function id
	- (success,returndata) = _router.delegatecall(abi.encodeCall(IUniswapV2.swapExactTokensForTokens,(amount,token0,token1,receiver))) (contracts/SlitherNotUnprotectedUpgrade.sol#34-36)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#controlled-delegatecall

It looks like OpenZeppelin introduced _disableInitializers specifically for EIP-1167 proxied contracts:

It is recommended to use this to lock implementation contracts that are designed to be called through proxies.

Ref. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/proxy/utils/Initializable.sol#L136-L143

I unit tested and I cannot call initilizer on implementation after calling _disableInitializer, so I believe this detection is a false positive. Also I think the problem lies in this code

slither/slither/detectors/statements/unprotected_upgradeable.py

Lines 25 to 39 in d8e526e

	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

I can fix if we confirm the issue.

Code example to reproduce the issue:

//SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.0;

import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

interface IUniswapV2 {
function swapExactTokensForTokens(
uint256 amount,
address token0,
address token1,
address receiver
) external returns (uint256[] memory);
}

contract SlitherNotUnprotectedUpgrade is Initializable {
address private immutable _router;

constructor(address router) {
    _disableInitializers();
    _router = router;
}

function initialize() public initializer {
    // solhint-disable-prev-line no-empty-block
}

function swapExactTokensForTokens(
    uint256 amount,
    address token0,
    address token1,
    address receiver
) private returns (uint256[] memory) {
    // solhint-disable-next-line avoid-low-level-calls
    (bool success, bytes memory returndata) = _router.delegatecall(
        abi.encodeCall(IUniswapV2.swapExactTokensForTokens, (amount, token0, token1, receiver))
    );
    if (!success) {
        // solhint-disable-next-line no-inline-assembly
        assembly {
            revert(add(returndata, 32), mload(returndata))
        }
    }
    return abi.decode(returndata, (uint256[]));
}

}

Version:

0.8.3

Relevant log output:

No response

[0xalpharush]

This should be fixed if you install slither-analyzer==0.9.0 which includes #1344.
To upgrade with pip: pip install slither-analyzer==0.9.0

[bmarcot]

Fixed in 0.9.0