Skip to content

Exclude address(0) in ERC721 approvals #4405

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Exclude address(0) in ERC721 approvals #4405

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 20 additions & 13 deletions contracts/token/ERC721/ERC721.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,6 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
*/
function approve(address to, uint256 tokenId) public virtual {
address owner = ownerOf(tokenId);
if (to == owner) {
revert ERC721InvalidOperator(owner);
}

if (_msgSender() != owner && !isApprovedForAll(owner, _msgSender())) {
revert ERC721InvalidApprover(_msgSender());
Expand All @@ -131,7 +128,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
function getApproved(uint256 tokenId) public view virtual returns (address) {
_requireMinted(tokenId);

return _tokenApprovals[tokenId];
return _getApproved(tokenId);
}

/**
Expand Down Expand Up @@ -220,16 +217,22 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
return _ownerOf(tokenId) != address(0);
}

/**
* @dev Returns the approved address for `tokenId`. Returns 0 if `tokenId` is not minted.
*/
function _getApproved(uint256 tokenId) internal view virtual returns (address) {
return _tokenApprovals[tokenId];
}

/**
* @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 = ownerOf(tokenId);
return (spender == owner || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender);
address owner = _ownerOf(tokenId);
return (owner != address(0) &&
(spender == owner ||
isApprovedForAll(owner, spender) ||
(_getApproved(tokenId) == spender && spender != address(0))));
Comment on lines +232 to +235
Copy link
Collaborator

@Amxx Amxx Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No actionnable here, but for the record I'm not a big fan of the linter's style.

}

/**
Expand Down Expand Up @@ -386,8 +389,12 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
* Emits an {Approval} event.
*/
function _approve(address to, uint256 tokenId) internal virtual {
address owner = ownerOf(tokenId);
Copy link
Collaborator

@Amxx Amxx Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should note that ownerOf is already called in the public approve.

If we want to minimize duplicated sloads, we could use a construction similar to _update's condition. This function is almost never used though, so optimising it is not as critical as transferFrom.

if (to == owner || to == address(0)) {
revert ERC721InvalidOperator(to);
}
_tokenApprovals[tokenId] = to;
emit Approval(ownerOf(tokenId), to, tokenId);
emit Approval(owner, to, tokenId);
}

/**
Expand All @@ -396,8 +403,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
* Emits an {ApprovalForAll} event.
*/
function _setApprovalForAll(address owner, address operator, bool approved) internal virtual {
if (owner == operator) {
revert ERC721InvalidOperator(owner);
if (operator == owner || operator == address(0)) {
revert ERC721InvalidOperator(operator);
}
_operatorApprovals[owner][operator] = approved;
emit ApprovalForAll(owner, operator, approved);
Expand Down