[False-Positive]: Block timestamp and Dangerous strict equalities

[pcaversaccio]

Describe the false alarm that Slither raise and how you know it's inaccurate:

Clone CreateX before commit pcaversaccio/createx@b60005c and run slither . with the latest Slither version 0.10.2:

CreateX._parseSalt(bytes32) (src/CreateX.sol#922-944) uses a dangerous strict equality:
        - address(bytes20(salt)) == msg.sender && bytes1(salt[20]) ==  (src/CreateX.sol#925)
CreateX._parseSalt(bytes32) (src/CreateX.sol#922-944) uses a dangerous strict equality:
        - address(bytes20(salt)) == msg.sender && bytes1(salt[20]) ==  (src/CreateX.sol#927)
CreateX._parseSalt(bytes32) (src/CreateX.sol#922-944) uses a dangerous strict equality:
        - address(bytes20(salt)) == address(0) && bytes1(salt[20]) ==  (src/CreateX.sol#931)
CreateX._parseSalt(bytes32) (src/CreateX.sol#922-944) uses a dangerous strict equality:
        - address(bytes20(salt)) == address(0) && bytes1(salt[20]) ==  (src/CreateX.sol#933)
CreateX._parseSalt(bytes32) (src/CreateX.sol#922-944) uses a dangerous strict equality:
        - bytes1(salt[20]) ==  (src/CreateX.sol#937)
CreateX._parseSalt(bytes32) (src/CreateX.sol#922-944) uses a dangerous strict equality:
        - bytes1(salt[20]) ==  (src/CreateX.sol#939)
CreateX._requireSuccessfulContractCreation(address) (src/CreateX.sol#1011-1015) uses a dangerous strict equality:
        - newContract == address(0) || newContract.code.length == 0 (src/CreateX.sol#1012)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#dangerous-strict-equalities
INFO:Detectors:
CreateX.deployCreate2Clone(bytes32,address,bytes) (src/CreateX.sol#512-543) uses timestamp for comparisons
        Dangerous comparisons:
        - proxy == address(0) (src/CreateX.sol#532)
CreateX.deployCreate3(bytes32,bytes) (src/CreateX.sol#630-646) uses timestamp for comparisons
        Dangerous comparisons:
        - proxy == address(0) (src/CreateX.sol#637)
CreateX.deployCreate3AndInit(bytes32,bytes,bytes,CreateX.Values,address) (src/CreateX.sol#686-723) uses timestamp for comparisons
        Dangerous comparisons:
        - proxy == address(0) (src/CreateX.sol#699)
CreateX._guard(bytes32) (src/CreateX.sol#886-912) uses timestamp for comparisons
        Dangerous comparisons:
        - (salt != _generateSalt()) (src/CreateX.sol#910)
CreateX._parseSalt(bytes32) (src/CreateX.sol#922-944) uses timestamp for comparisons
        Dangerous comparisons:
        - address(bytes20(salt)) == msg.sender && bytes1(salt[20]) ==  (src/CreateX.sol#925)
        - address(bytes20(salt)) == msg.sender && bytes1(salt[20]) ==  (src/CreateX.sol#927)
        - address(bytes20(salt)) == msg.sender (src/CreateX.sol#929)
        - address(bytes20(salt)) == address(0) && bytes1(salt[20]) ==  (src/CreateX.sol#931)
        - address(bytes20(salt)) == address(0) && bytes1(salt[20]) ==  (src/CreateX.sol#933)
        - address(bytes20(salt)) == address(0) (src/CreateX.sol#935)
        - bytes1(salt[20]) ==  (src/CreateX.sol#937)
        - bytes1(salt[20]) ==  (src/CreateX.sol#939)
CreateX._requireSuccessfulContractCreation(bool,address) (src/CreateX.sol#995-1005) uses timestamp for comparisons
        Dangerous comparisons:
        - ! success || newContract == address(0) || newContract.code.length == 0 (src/CreateX.sol#1002)
CreateX._requireSuccessfulContractCreation(address) (src/CreateX.sol#1011-1015) uses timestamp for comparisons
        Dangerous comparisons:
        - newContract == address(0) || newContract.code.length == 0 (src/CreateX.sol#1012)
CreateX._requireSuccessfulContractInitialisation(bool,bytes,address) (src/CreateX.sol#1023-1031) uses timestamp for comparisons
        Dangerous comparisons:
        - ! success || implementation.code.length == 0 (src/CreateX.sol#1028)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp
INFO:Slither:. analyzed (2 contracts with 86 detectors), 15 result(s) found

These false positives have not been present in the previous versions. So, I guess this is a new regression.

Examples:

Maybe Slither wants to point to the following (non-issues in my context) (link):

guardedSalt = (salt != _generateSalt()) ? keccak256(abi.encode(salt)) : salt;

_generateSalt() uses block.timestamp under the hood. So maybe the description is simply off:

CreateX._requireSuccessfulContractInitialisation(bool,bytes,address) (src/CreateX.sol#1023-1031) uses timestamp for comparisons
        Dangerous comparisons:
        - ! success || implementation.code.length == 0 (src/CreateX.sol#1028)

The same for the other warning which has no dangerous equality except if you want to refer to newContract.code.length == 0 for the codesize check maybe, but in that case the detector message must be improved IMO:

CreateX._requireSuccessfulContractCreation(address) (src/CreateX.sol#1011-1015) uses a dangerous strict equality:
        - newContract == address(0) || newContract.code.length == 0 (src/CreateX.sol#1012)

Frequency

Very Frequently

Code example to reproduce the issue:

See CreateX.

Version:

0.10.2

Relevant log output:

No response

[0xalpharush]

The first is a result of propagating taints through named returns i.e. this would have been detected prior if the code used explicit returns. #1880

We are working on #175

For inter-procedural taints, we definitely need a better way to show where the source originates

[cirethic]

got similar issue that slither . started complaining uses timestamp for comparisons on many non-timestamp variable comparisons.

I've realized that when the timestamp comparison occurs within a single function and that function is called throughout the codebase, slither may generate false-positive detections to all the variables. Perhaps it's more effective to simply highlight the line where the timestamp comparison occurs and be able to just disable-next-line on that line.