Self-approval allowed for approve

[jrkosinski]

The caller of approve(address, uint) should not be allowed to approve themselves.

This is a minor issue, but setApprovalForAll(operator, approved) disallows the caller address and the 'operator' argument from being identical. Similarly, approve(to, tokenId) should revert with the same error.

For reference, OpenZeppelin's ERC721.sol reverts when approve is called, if msg.sender and the given address are the same.

Sample code:
erc721.approve(my_addr, 1); // does not revert

erc721.setApprovalForAll(my_addr, true); //reverts with ApproveToCaller

[Vectorized]

Nice catch.

The spec according to EIP-721 for approve and setApprovalForAll are:

/// @notice Change or reaffirm the approved address for an NFT
/// @dev The zero address indicates there is no approved address.
///  Throws unless `msg.sender` is the current NFT owner, or an authorized
///  operator of the current owner.
/// @param _approved The new approved NFT controller
/// @param _tokenId The NFT to approve
function approve(address _approved, uint256 _tokenId) external payable;

/// @notice Enable or disable approval for a third party ("operator") to manage
///  all of `msg.sender`'s assets
/// @dev Emits the ApprovalForAll event. The contract MUST allow
///  multiple operators per owner.
/// @param _operator Address to add to the set of authorized operators
/// @param _approved True if the operator is approved, false to revoke approval
function setApprovalForAll(address _operator, bool _approved) external;

So technically, we can choose to either include or omit the check of approving to oneself.

Approving to oneself is essentially a no-op. Perfectly allowable in the standard.

I think we should remove the check in setApprovalToAll instead, to make it more minimalist and consistent.
setApprovalToAll is a frequently called function too -- we will be glad to shave off a bit of gas from there.

You can open a PR to remove the check. :)

Solmate does this too: https://github.com/transmissions11/solmate/blob/main/src/tokens/ERC721.sol#L76

[jrkosinski]

That is fair, I agree with that. I have a branch for the issue, I will change it to remove the check for setApprovalToAll instead.