-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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.
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
Exclude address(0) in ERC721 approvals #4405
Conversation
|
return (owner != address(0) && | ||
(spender == owner || | ||
isApprovedForAll(owner, spender) || | ||
(_getApproved(tokenId) == spender && spender != address(0)))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for the tests to run (curious about the gas changes).
If tests are good this LGTM.
@@ -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); |
There was a problem hiding this comment.
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 sload
s, 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.
Closing in favor of #4377 |
This PR implements a few changes to make the treatment of
address(0)
in ERC721 more uniform and consistent with the rest of the library:_isApprovedOrOwner
: Ensure we return false ifspender
isaddress(0)
. Don't revert if the token is not minted.setApprovalForAll
: Revert ifoperator
isaddress(0)
.approve
: Revert ifto
isaddress(0)
.The reasoning to reject
address(0)
is that it can show up unexpectedly by reading uninitialized values from storage, or any uninitialized variable, or as the return value of ecrecover, etc., and in the case of_isApprovedOrOwner
if the spender isaddress(0)
it could cause the function to return true in combination withgetApproved
, since that function returns 0 to indicate that no account is approved.Proposing first before writing tests.
Fixes #4136.
Closes #4169 (superceded).
Closes #4144 (superceded).