[False-Positive]: Detector "suicidal.py" needs to improve.
- 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
Assessment
This issue has not been assessed yet.