crytic / crytic/slither

[False-Positive]: Detector "suicidal.py" needs to improve.

Open
#2,282 1 comment 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:

The current implementation checks for direct `msg.sender` references and `onlyOwner` modifiers. For modifiers, I can see we cannot list all possible naming for related modifiers, and check "onlyOwner" is thought enough because it is the common usage in this context.

However, in many ERC-20 projects, `_msgSender()` is used to abstract `msg.sender`, especially in contracts designed for upgradability or meta-transactions.

The `is_protected` function doesn’t seem to recognize when `_msgSender()` abstracts `msg.sender`, leading to potential false positives. Functions using `_msgSender()` for owner checks, like `onlyAdmin`, are not identified as secure.

I think adding alias analysis or data-flow analysis to track `msg.sender` usage could enhance Slither's effectiveness in recognizing secure owner verification patterns and reduce false positives. Looking forward to your reply.

### Frequency

Very Frequently

### Code example to reproduce the issue:

test_selfdestruct.sol
```
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract TestSuicidal {
address public owner;

// Modifier that allows only the owner to call a function
modifier onlyAdmin() {
require(_msgSender() == owner, "Not the owner");
_;
}

constructor() {
owner = _msgSender(); // Setting the contract deployer as the owner
}

// Function to self-destruct the contract, protected by the onlyOwner modifier
function destroyContract() external onlyAdmin {
selfdestruct(payable(owner));
}

// Internal function to return the message sender
function _msgSender() internal view returns (address) {
return msg.sender;
}
}
```

### Version:

```
slither --version
0.10.0
```

### Relevant log output:

```shell
INFO:Detectors:
TestSuicidal.destroyContract() (test_selfdestruct.sol#18-20) allows anyone to destruct the contract
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#suicidal
```
```

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.