contract warns if fallback payable is defined without receive defined

[3esmit]

Description

I'm having to include this

    /**
     * @notice Fails on calls with no `msg.data` but with `msg.value`
     */
    receive() external payable {
        revert("bad call");
    }

into the smart contract so it stops warning me about not using receive. I use fallback payable, but I need msg.data, so receive()(calls with no msg.data, but msg.value) should always fail if I didn't defined a receive() function

This warning is not fair: Warning: This contract has a payable fallback function, but no receive ether function. Consider adding a receive ether function.

If I add this receive function, that only reverts, than compiler will include it on the ABI of the contract. But this conflics with the warning and with https://solidity.readthedocs.io/en/v0.7.4/security-considerations.html#take-warnings-seriously

Instead, I added the first line of fallback as require(msg.data.length > 0, "bad call").

[chriseth]

In my opinion, your explicit receive function that always reverts in a good solution - it makes it explicit what happens if you send this contract Ether. The fact that the function is present in the ABI may be unfortunate, but any other function present in the ABI could also always revert.

Would you propose to remove the warning altogether?

[3esmit]

The warning seems wrong because the intended design is not having a receive() function.

If I haven't defined receive(), than any address.transfer() to the contract should fail automatically.

Currently I am getting this warning, and I have to:

    fallback() external payable {
        require(msg.data.length > 0, "bad call");
        //(...) my fallback logic that uses msg.data, and possibly use msg.value
    }

[3esmit]

Yes, I propose to remove the warning altogether, because the warning makes it seems I am doing a bad design on the contract, while I have to do this way for the compiler understand that this ABI entrypoint does not exists.

If I am not using the "receive() external payable", or calls from .transfer(), or msg.data.lenght == 0 with msg.value > 0, than should be a proper way of doing it without rising warnings and without including in ABI an function that cannot be used.

I cannot accept this solution as "ignore the warning" because this will make all warnings ignorable. I cannot accept this solution as "include the receive that always reverts" because it will make an incorrect ABI which can confuse other developers or users.

[cameel]

Since this is somewhat related to #2691, which is now closed, I'm adding this to the design backlog to be discussed on the next call. We should make a decision about what to do with this warning.

[chriseth]

I still think the current situation is the best, but I'm open for discussion.

[axic]

The split between fallback and receive was discussed and introduced via #3198. It is a length discussions containing reasoning.

@hrkrshnn brought up that the warning is not particularly clear and could trigger people to just include an empty receive function, which is likely not what they want. We should consider improving the error message to avoid such situations.

[VladLupashevskyi]

The fallback function is used in proxy contracts and this warning is a bit annoying when you have to deal with lots of these contracts. I agree with @3esmit that such an approach to ignore this warning makes all warnings (possibly important ones) also ignorable. It would be great if there would be an option to disable this warning for a particular line.

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

[github-actions[bot]]

Hi everyone! This issue has been automatically closed due to inactivity. If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen. However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.