_isApprovedOrOwner approves a null spender

[elv-serban]

_isApprovedOrOwner approves a null spender:

   getApproved(tokenId) == spender

It would be safer to check for a non-null spender or at a minimum document the requirement that the caller check for it.

💻 Environment

I have tested a relatively old version but the code hasn't changed in the latest:

• solidity 0.6.16

📝 Details

getApproved returns 0 when the approval doesn't exist.

    function getApproved(uint256 tokenId) public view returns (address) {
        require(_exists(tokenId));
        return _tokenApprovals[tokenId];
    }

_isApprovedOrOwner will return true if spender is address(0) and getApproved(tokenId) return 0

    /**
     * @dev Returns whether `spender` is allowed to manage `tokenId`.
     *
     * Requirements:
     *
     * - `tokenId` must exist.
     */
    function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) {
        address owner = ERC721.ownerOf(tokenId);
        return (spender == owner || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender);
    }

🔢 Code to reproduce bug

Any call with spender = address(0) will produce the problem.

[Amxx]

Hello @elv-serban

Under what circumstances do you expect spender to be address(0)? In our codebase, ERC721._isApprovedOrOwner is always called with _msgSender() as its first argument.

Adding this check will have a cost ... and many devs already think we are doing to much of these, leading to gas waste. I'd like to fully understand the logic that would have some dev call this function with address(0) and understand the resulting risks.

[elv-serban]

The exact use case we ran into was an extension of the ERC721 that calls _isApprovedOrOwner() with the result of ecrecover() which returns 0 for a non-matching signature, thus the spender == address(0).

This use case is outside of the openzep code of course.

I know an additional require() might be too big of a hammer in this case. Maybe the best option is to add a comment stating what the caller is required to do or check since it is not immediately obvious reading the code.

[Amxx]

My recommendation would be to not use ecrecover directly. We have a ECDSA library for that.

[elv-serban]

Yes agreed. I still think it is worth adding a prerequisites or requirements section to the function comment when there is potential for mistakes like this but this is up to you.

Since there are no code changes needed, let me close this and you can decide if there is a good way to document to avoid traps.

[frangio]

This is an interesting case for keeping the checks against non-zero addresses in the code. It's definitely an easy mistake for devs to directly pass the result of ecrecover to functions without considering that it could return zero and have undesired consequences.

Personally, I would consider adding this check. I wonder how much additional runtime and deployment cost it adds, but I can't imagine it will be too much.

Will reopen to continue the discussion.