slither
slither copied to clipboard
Arbitrary-send-eth false positive
Describe the issue:
UPDATE:
(2) and (3) are resolved by using detector name "arbitrary-send", not "arbitrary-send-eth". That could be made more clear in the docs since it is titled "Check: arbitrary-send-eth" on the man page, but it has been resolved
However, (1) still seems to be an issue
OP:
arbitrary-send-eth seems broken- both producing false positive (1), and not working as a detector that can be ignored or detected against individually (2) and (3).
function getPayout(address payable addressOfProposer)
public
returns (bool)
{
// Get the available allowance first amd store in uint256.
uint256 allowanceAvailable = _payoutTotals[addressOfProposer];
require(allowanceAvailable > 0, "You do not have any funds available.");
_decreasePayout(addressOfProposer, allowanceAvailable);
(bool success,) = addressOfProposer.call{value: allowanceAvailable}("");
require(success, "Failed to send eth");
emit Withdraw(addressOfProposer, allowanceAvailable);
return true;
}
(1) This code appears controlled, not sending ether to an arbitrary user address. They have to have an allowanceAvailable per contract state, yet slither produces:
PowDAO.getPayout(address) (PowDAO.sol#142-157) sends eth to arbitrary user
Dangerous calls:
- (success) = addressOfProposer.call{value: allowanceAvailable}() (PowDAO.sol#152)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#functions-that-send-ether-to-arbitrary-destinations
That's the first bug.
(2) Next, I added this line:
//slither-disable-next-line arbitrary-send-eth
(bool success,) = addressOfProposer.call{value: allowanceAvailable}("");
It still produces the high level warning
(3) Third, I ran
slither PowDAO.sol --detect arbitrary-send-eth
And I got the response
Traceback (most recent call last):
File "/home/gerald/.local/bin//slither", line 8, in <module>
sys.exit(main())
File "/home/gerald/.local/lib/python3.8/site-packages/slither/__main__.py", line 632, in main
main_impl(all_detector_classes=detectors, all_printer_classes=printers)
File "/home/gerald/.local/lib/python3.8/site-packages/slither/__main__.py", line 671, in main_impl
detector_classes = choose_detectors(args, all_detector_classes)
File "/home/gerald/.local/lib/python3.8/site-packages/slither/__main__.py", line 211, in choose_detectors
raise Exception(f"Error: {detector} is not a detector")
Exception: Error: arbitrary-send-eth is not a detector
Code example to reproduce the issue:
function getPayout(address payable addressOfProposer)
public
returns (bool)
{
// Get the available allowance first amd store in uint256.
uint256 allowanceAvailable = _payoutTotals[addressOfProposer];
require(allowanceAvailable > 0, "You do not have any funds available.");
_decreasePayout(addressOfProposer, allowanceAvailable);
//slither-disable-next-line arbitrary-send-eth
(bool success,) = addressOfProposer.call{value: allowanceAvailable}("");
require(success, "Failed to send eth");
emit Withdraw(addressOfProposer, allowanceAvailable);
return true;
}
Version:
0.8.3
Relevant log output:
PowDAO.getPayout(address) (PowDAO.sol#142-157) sends eth to arbitrary user
Dangerous calls:
- (success) = addressOfProposer.call{value: allowanceAvailable}() (PowDAO.sol#152)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#functions-that-send-ether-to-arbitrary-destinations
and
slither PowDAO.sol --detect arbitrary-send-eth
Traceback (most recent call last):
File "/home/gerald/.local/bin//slither", line 8, in <module>
sys.exit(main())
File "/home/gerald/.local/lib/python3.8/site-packages/slither/__main__.py", line 632, in main
main_impl(all_detector_classes=detectors, all_printer_classes=printers)
File "/home/gerald/.local/lib/python3.8/site-packages/slither/__main__.py", line 671, in main_impl
detector_classes = choose_detectors(args, all_detector_classes)
File "/home/gerald/.local/lib/python3.8/site-packages/slither/__main__.py", line 211, in choose_detectors
raise Exception(f"Error: {detector} is not a detector")
Exception: Error: arbitrary-send-eth is not a detector
We can look into tightening the heuristics to ignore transfers to addresses where the amount sent is based on the address i.e. used as key in a mapping or argument of a function.
I think the detector in the 0.8.3 is still named "arbitrary-send" and was renamed to "arbitrary-send-eth" on the development branch (https://github.com/crytic/slither/pull/1025), so substituting in "arbitrary-send" should work.
We can look into tightening the heuristics to ignore transfers to addresses where the amount sent is based on the address i.e. used as key in a mapping or argument of a function.
From my understanding, the detector already tries to exclude functions where the address is used as an index (eg uint256 allowanceAvailable = _payoutTotals[addressOfProposer];
). Here is the code that seems to check for this:
https://github.com/crytic/slither/blob/e3dcf1ecd3e9de60da046de471c5663ab637993a/slither/detectors/functions/arbitrary_send_eth.py#L52-L60
However, this only checks for msg.sender
, while later on is_tainted
also checks for function inputs, tx.origin and more. The solution is likely to check if a tainted value is used as an index, rather than only checking if a variable depending on msg.sender is used as an index.