Confusing Usage of `payable`

[alex-ppg]

Description

The payable modifier appears to be enforced by the interfaces of @solidstate's IERC721, however, the actual SolidStateERC721 implementation contains a dedicated hook system to prohibit non-zero msg.value transactions.

Recommendation

The payable modifier should either be removed from the interface definition or the internal hook system should be removed (i.e. _handleApproveMessageValue) to actually take advantage of the gas optimization provided by payable.

Rationale

Currently, the system simply imposes additional checks on itself significantly increasing the gas cost of the contract whilst retaining the same behaviour as if payable was never defined. A little bit more background as to why payable is specified in the interface would greatly help in judging the correct action.

To note, removing payable is still compliant with the EIP-721 standard. For more background, consult first bullet point in: https://eips.ethereum.org/EIPS/eip-721#caveats

[ItsNickBarry]

The design takes into account three goals:

1. Provide basic contracts that conform to the standard, exhibiting sensible default behavior where applicable.
2. Allow the developer to implement additional behavior through overrides, within the confines of the standard.
3. Enforce the SS code style conventions that we've determined work best for upgradeable contracts - in this case, that means avoiding marking external functions as virtual, which prevents a developer from changing a nonpayable function to payable.

The "handle message value" callback system is the only approach I've found that addresses all three of these points.

I agree that the pattern is unusual, and even disagreeable, but not confusing. Most developers don't even seem to be aware that ERC721 allows payable functions, so I think supporting them here might be educational for some people. The gas concerns seem negligible, in my opinion. Much more costly is the "safe" transfer acceptance check, which is a built-in reentrancy vulnerability.

[alex-ppg]

While I understand the points raised, most if not all EIP-721 implementations do not use the payable modifier. This means we are introducing additional steps and code to accommodate for an obscure "feature" that remains unutilized by the SolidState implementation as well as users of it.

The OpenZeppelin implementation clearly defines the functions as "non-payable" whilst the solmate implementation simply defines them as virtual (which does break the SS code style convention). The former of the two is cited as a direct influence on whether https://github.com/ethereum/solidity/issues/11253 will be accepted which permits a payable interface function to be "downgraded" to a "non-payable" implementation and would solve the issue at hand.

I think that until https://github.com/ethereum/solidity/issues/11253 is properly accepted into the language, the interface should define the functions as "non-payable". Code such as the following I have observed in security audits is very counter-intuitive and prone to errors:

function transferFrom(address from, address to, uint256 tokenId) public payable virtual override {
    require(msg.value == 0, "ERC721: transfer must have no value");
    // ...
}

Alternatively, given that the only "standard" solution right now is to install @solidstate and another dependency to gain access to a non-payable IERC721, I advise the inclusion of both interfaces in the repository with users utilizing whichever they wish.