[False-Positive]: unchecked-lowlevel if returndata is not used

[ernestognw]

Describe the false alarm that Slither raise and how you know it's inaccurate:

Recently some changes were made to OpenZeppelins ERC2771 Forwarder in OpenZeppelin/openzeppelin-contracts#4346. These changes started to trigger to warnings as follows:

INFO:Detectors:
Address.sendValue(address,uint256) (contracts/utils/Address.sol#41-50) ignores return value by (success) = recipient.call{value: amount}() (contracts/utils/Address.sol#46)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unchecked-low-level-calls
INFO:Slither:. analyzed (447 contracts with 46 detectors), 1 result(s) found

However, these are the two functions that matched:

function sendValue(address payable recipient, uint256 amount) internal {
    ...
    (bool success, ) = recipient.call{value: amount}("");
    if (!success) {
        revert FailedInnerCall();
    }
}

function _execute(
    ForwardRequestData calldata request,
    bool requireValidRequest
) internal virtual returns (bool success) {
    ...
    uint256 currentNonce = _useNonce(signer);

    (success, ) = request.to.call{gas: request.gas, value: request.value}(
        abi.encodePacked(request.data, request.from)
    );

    _checkForwardedGas(request);

    emit ExecutedForwardRequest(signer, currentNonce, success);
   ...
}

Both cases are behaving as intended, because:

1. The sendValue function is indeed checking the success value. The only reason why I think it may be broken is because of the custom error usage.
2. The _execute function is logging the success value in ExecutedForwardRequest and the documentation for this detector recommends ensuring logging the result

EDIT: The issue seems to be related to not using the returndata instead of custom errors

Frequency

Very Frequently

Code example to reproduce the issue:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.19;

/// @dev WARNING: Simplified for demo purposes, don't use.
contract FalsePositives {
    struct ForwardRequestData {
        address from;
        address to;
        uint256 value;
        uint256 gas;
        uint48 deadline;
        bytes data;
        bytes signature;
    }

    mapping(address => uint256) private _nonces;

    event ExecutedForwardRequest(address indexed signer, uint256 nonce, bool success);

    function sendValue(address payable recipient, uint256 amount) internal {
        if (address(this).balance < amount) {
            revert();
        }

        (bool success, ) = recipient.call{value: amount}("");
        if (!success) {
            revert();
        }
    }

    function _execute(ForwardRequestData calldata request) internal virtual returns (bool success) {
        uint256 currentNonce = _nonces[request.from]++;

        (success, ) = request.to.call{gas: request.gas, value: request.value}(
            abi.encodePacked(request.data, request.from)
        );

        emit ExecutedForwardRequest(request.from, currentNonce, success);
    }
}

Version:

0.9.4 and 0.9.5

Relevant log output:

FalsePositives.sendValue(address,uint256) (contracts/metatx/FalsePositive.sol#21-30) ignores return value by (success) = recipient.call{value: amount}() (contracts/metatx/FalsePositive.sol#26)
FalsePositives._execute(FalsePositives.ForwardRequestData) (contracts/metatx/FalsePositive.sol#32-40) ignores return value by (success,None) = request.to.call{gas: request.gas,value: request.value}(abi.encodePacked(request.data,request.from)) (contracts/metatx/FalsePositive.sol#35-37)

[frangio]

In my opinion this is not related to the changes introduced in OpenZeppelin code, it was a side effect of #1861 in Slither v0.9.5. The PR says:

Now if a function call returns multiple values all must be used.

However, for unchecked-lowlevel we should only care about the use of the success boolean. Returndata is not needed and is often ignored.

[0xalpharush]

I agree we should not require returndata since it may not be used for good reason

[ernestognw]

I updated the title and the description to refer better to the issue (returndata not used). Also, as referenced in ubiquity/ubiquity-dollar#714, the false positive triggers by ignoring returndata as well (see here and here)

[0xalpharush]

Released https://github.com/crytic/slither/releases/tag/0.9.6 to address this!

[ernestognw]

Thanks @0xalpharush, I can confirm this is no longer an issue so I'm closing