crytic / crytic/slither

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

Open
#2,425 2 comments 0 reactions 0 assignees View on GitHub
false-positive
Dominant language
Python
Stars
6.4k
Forks
1.1k
PR merge metrics
No merged PRs in 30d

Description

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

Clone [`CreateX`](https://github.com/pcaversaccio/createx) before commit https://github.com/pcaversaccio/createx/commit/b60005c97cc7365a36b7fe74c75b4bcd32d3f165 and run `slither .` with the latest Slither version `0.10.2`:

```console
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:

![image](https://github.com/crytic/slither/assets/25297591/db0121e6-e6ba-40db-85b2-31b8bad03777)

![image](https://github.com/crytic/slither/assets/25297591/89c02a50-3d1a-418e-94f9-fb53fed41823)

Maybe Slither wants to point to the following (non-issues in my context) ([link](https://github.com/pcaversaccio/createx/blob/b6184f50bf3ec288aead6060057e8ecdc7adef9e/src/CreateX.sol#L910)):

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

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

```console
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:

```console
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`](https://github.com/pcaversaccio/createx/blob/main/src/CreateX.sol).

### Version:

`0.10.2`

### Relevant log output:

_No response_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.