crytic / crytic/slither

improvements to zero address check detector

Open
#1,544 1 comment 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
Python
Stars
6.4k
Forks
1.1k
PR merge metrics
No merged PRs in 30d

Description

follow on https://github.com/crytic/slither/pull/1043 https://github.com/crytic/slither/pull/1033

I think the usage of the data dependency here:
https://github.com/crytic/slither/blob/191d6d4e661cd14c715f0f7b86593b69037322c3/slither/detectors/operations/missing_zero_address_validation.py#L81-L82

Will lead to miss TP. For example the following code:

```solidity
contract C{

address owner;

modifier onlyOwner(){
require(msg.sender == owner);
_;
}

function set(address addr) public{
require(msg.sender == owner);
owner = addr;
}
}
```

Would raise a warning before the PR, and would not with these changes.

I am wondering also if we could not refactor the detector's logic - it becomes complex to follow.

Maybe we can mark all the `Constant(0)` usage as source of dependency, and check for any `==`/`!=` operations where the left variable is a parameter, and the right variable is dependent on one of the `Constant(0)` we detected?

_Originally posted by @montyly in https://github.com/crytic/slither/issues/1033#issuecomment-1034916789_

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.