Arbitrary-send-eth false positive

GeraldRyan commented 2 years ago

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

0xalpharush commented 2 years ago

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.

Otto-AA commented 7 months ago

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.

crytic / slither