Changes suggested in the PR discussions

contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol

@@ -31,9 +31,9 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable
     function _update(
         address to,
         uint256 tokenId,
-        address operatorCheck
+        address auth
     ) internal virtual override(ERC721Consecutive, ERC721Enumerable) returns (address) {
-        return super._update(to, tokenId, operatorCheck);
+        return super._update(to, tokenId, auth);
     }
 
     function _increaseBalance(address account, uint128 amount) internal virtual override(ERC721, ERC721Enumerable) {

contracts/mocks/token/ERC721ConsecutiveMock.sol

@@ -44,9 +44,9 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes
     function _update(
         address to,
         uint256 tokenId,
-        address operatorCheck
+        address auth
     ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) returns (address) {
-        return super._update(to, tokenId, operatorCheck);
+        return super._update(to, tokenId, auth);
     }
 
     // solhint-disable-next-line func-name-mixedcase

contracts/token/ERC721/ERC721.sol

@@ -113,14 +113,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
      * @dev See {IERC721-approve}.
      */
     function approve(address to, uint256 tokenId) public virtual {
-        address owner = ownerOf(tokenId);
+        _approve(to, tokenId, _msgSender());
-        address caller = _msgSender();
-
-        if (owner != caller && !isApprovedForAll(owner, caller)) {
-            revert ERC721InvalidApprover(caller);
-        }
-
-        _approve(to, tokenId);
     }
 
     /**
@@ -153,10 +146,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
         if (to == address(0)) {
             revert ERC721InvalidReceiver(address(0));
         }
+        // Setting an "auth" arguments means that `_update` will check that the token exists (from != 0),
+        // no need to duplicate that check here.
         address previousOwner = _update(to, tokenId, _msgSender());
-        if (previousOwner == address(0)) {
+        if (previousOwner != from) {
-            revert ERC721NonexistentToken(tokenId);
-        } else if (previousOwner != from) {
             revert ERC721IncorrectOwner(from, tokenId, previousOwner);
         }
     }
@@ -179,10 +172,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
     /**
      * @dev Returns the owner of the `tokenId`. Does NOT revert if token doesn't exist
      *
-      * IMPORTANT: Any overrides to this function that add ownership of tokens not tracked by the 
+     * IMPORTANT: Any overrides to this function that add ownership of tokens not tracked by the
-      * core ERC721 logic MUST be matched with the use of {_increaseBalance} to keep balances 
+     * core ERC721 logic MUST be matched with the use of {_increaseBalance} to keep balances
-      * consistent with ownership. The invariant to preserve is that for any address `a` the value returned by 
+     * consistent with ownership. The invariant to preserve is that for any address `a` the value returned by
-      * `balanceOf(a)` must be equal to the number of tokens such that `_ownerOf(tokenId)` is `a`. 
+     * `balanceOf(a)` must be equal to the number of tokens such that `_ownerOf(tokenId)` is `a`.
      */
     function _ownerOf(uint256 tokenId) internal view virtual returns (address) {
         return _owners[tokenId];
@@ -201,37 +194,58 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
      *
      * WARNING: This function doesn't check that `owner` is the actual owner of the specified `tokenId`. Moreover, it returns true if `owner == spender` in order to skip the check where needed. Consider checking for cases where `spender == address(0)` since they could lead to unexpected behavior.
      */
-    function _isApproved(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) {
+    function _isAuthorised(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) {
-        return owner == spender || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender;
+        return
+            spender != address(0) &&
+            (owner == spender || isApprovedForAll(owner, spender) || _getApproved(tokenId) == spender);
     }
 
     /**
      * @dev Checks if `spender` can operate on `tokenId`, assuming the provided `owner` is the actual owner.
      * Reverts if `spender` has not approval for all assets of the provided `owner` nor the actual owner approved the `spender` for the specific `tokenId`.
      *
-     * WARNING: This function relies on {_isApproved}, so it doesn't check whether `owner` is the actual owner of `tokenId` and will skip the check if `spender == owner`
+     * WARNING: This function relies on {_isAuthorised}, so it doesn't check whether `owner` is the actual owner of `tokenId` and will skip the check if `spender == owner`
      */
-    function _checkApproved(address owner, address spender, uint256 tokenId) internal view virtual {
+    function _checkAuthorised(address owner, address spender, uint256 tokenId) internal view virtual {
-        if (!_isApproved(owner, spender, tokenId)) {
+        // That first check is needed because the error is different, and should as precedence over insufficient approval
+        if (owner == address(0)) {
+            revert ERC721NonexistentToken(tokenId);
+        } else if (!_isAuthorised(owner, spender, tokenId)) {
             revert ERC721InsufficientApproval(spender, tokenId);
         }
     }
 
+    /**
+     * @dev Unsafe write access to the balances, used by extensions that "mint" tokens using an {ownerOf} override.
+     *
+     * NOTE: the value is limited to type(uint128).max. This protect against _balance overflow. It is unrealistic that
+     * a uint256 would ever overflow from increments when these increments are bounded to uint128 values.
+     *
+     * WARNING: Increassing an account's balance using this function should go in pair with an override of the
+     * {_ownerOf} function that resolve the ownership of the corresponding tokens so that balances and ownerships
+     * remain consistent with one another.
+     */
+    function _increaseBalance(address account, uint128 value) internal virtual {
+        unchecked {
+            _balances[account] += value;
+        }
+    }
+
     /**
      * @dev Transfers `tokenId` from its current owner to `to`, or alternatively mints (or burns) if the current owner
      * (or `to`) is the zero address. Returns the owner of the `tokenId` before the update.
      *
-     * The `operatorCheck` argument is optional. If the value passed is non 0, then this function will check that
+     * The `auth` argument is optional. If the value passed is non 0, then this function will check that
-     * `operatorCheck` is either the owner of the token, or approved to operate on the token (by the owner).
+     * `auth` is either the owner of the token, or approved to operate on the token (by the owner).
      *
      * Emits a {Transfer} event.
      */
-    function _update(address to, uint256 tokenId, address operatorCheck) internal virtual returns (address) {
+    function _update(address to, uint256 tokenId, address auth) internal virtual returns (address) {
         address from = _ownerOf(tokenId);
 
         // Perform (optional) operator check
-        if (operatorCheck != address(0)) {
+        if (auth != address(0)) {
-            _checkApproved(from, operatorCheck, tokenId);
+            _checkAuthorised(from, auth, tokenId);
         }
 
         // Execute the update
@@ -271,8 +285,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
         if (to == address(0)) {
             revert ERC721InvalidReceiver(address(0));
         }
-        address from = _update(to, tokenId, address(0));
+        address previousOwner = _update(to, tokenId, address(0));
-        if (from != address(0)) {
+        if (previousOwner != address(0)) {
             revert ERC721InvalidSender(address(0));
         }
     }
@@ -312,8 +326,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
      * Emits a {Transfer} event.
      */
     function _burn(uint256 tokenId) internal {
-        address from = _update(address(0), tokenId, address(0));
+        address previousOwner = _update(address(0), tokenId, address(0));
-        if (from == address(0)) {
+        if (previousOwner == address(0)) {
             revert ERC721NonexistentToken(tokenId);
         }
     }
@@ -375,13 +389,18 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
     /**
      * @dev Approve `to` to operate on `tokenId`
      *
+     * The `auth` argument is optional. If the value passed is non 0, then this function will check that `auth` is
+     * either the owner of the token, or approved to operate on all tokens held by this owner.
+     *
      * Emits an {Approval} event.
      */
-    function _approve(address to, uint256 tokenId) internal virtual {
+    function _approve(address to, uint256 tokenId, address auth) internal virtual {
         address owner = ownerOf(tokenId);
-        if (to == owner) {
+
-            revert ERC721InvalidOperator(to);
+        if (auth != address(0) && owner != auth && !isApprovedForAll(owner, auth)) {
+            revert ERC721InvalidApprover(auth);
         }
+
         _tokenApprovals[tokenId] = to;
         emit Approval(owner, to, tokenId);
     }
@@ -396,7 +415,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
      * Emits an {ApprovalForAll} event.
      */
     function _setApprovalForAll(address owner, address operator, bool approved) internal virtual {
-        if (operator == owner || operator == address(0)) {
+        if (operator == address(0)) {
             revert ERC721InvalidOperator(operator);
         }
         _operatorApprovals[owner][operator] = approved;
@@ -439,20 +458,4 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er
             }
         }
     }
-
-    /**
-     * @dev Unsafe write access to the balances, used by extensions that "mint" tokens using an {ownerOf} override.
-     *
-     * NOTE: the value is limited to type(uint128).max. This protect against _balance overflow. It is unrealistic that
-     * a uint256 would ever overflow from increments when these increments are bounded to uint128 values.
-     *
-     * WARNING: Increassing an account's balance using this function should go in pair with an override of the
-     * {_ownerOf} function that resolve the ownership of the corresponding tokens so that balances and ownerships
-     * remain consistent with one another.
-     */
-    function _increaseBalance(address account, uint128 value) internal virtual {
-        unchecked {
-          _balances[account] += value;
-        }
-    }
 }

contracts/token/ERC721/extensions/ERC721Burnable.sol

@@ -19,6 +19,8 @@ abstract contract ERC721Burnable is Context, ERC721 {
      * - The caller must own `tokenId` or be an approved operator.
      */
     function burn(uint256 tokenId) public virtual {
+        // Setting an "auth" arguments means that `_update` will check that the token exists (from != 0),
+        // no need to duplicate that check here.
         _update(address(0), tokenId, _msgSender());
     }
 }

contracts/token/ERC721/extensions/ERC721Consecutive.sol

@@ -138,8 +138,8 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 {
      * WARNING: Using {ERC721Consecutive} prevents minting during construction in favor of {_mintConsecutive}.
      * After construction, {_mintConsecutive} is no longer available and minting through {_update} becomes available.
      */
-    function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) {
+    function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) {
-        address from = super._update(to, tokenId, operatorCheck);
+        address from = super._update(to, tokenId, auth);
 
         // only mint after construction
         if (from == address(0) && address(this).code.length == 0) {

contracts/token/ERC721/extensions/ERC721Enumerable.sol

@@ -76,8 +76,8 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable {
     /**
      * @dev See {ERC721-_update}.
      */
-    function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) {
+    function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) {
-        address from = super._update(to, tokenId, operatorCheck);
+        address from = super._update(to, tokenId, auth);
 
         if (from == address(0)) {
             _addTokenToAllTokensEnumeration(tokenId);

contracts/token/ERC721/extensions/ERC721Pausable.sol

@@ -30,8 +30,8 @@ abstract contract ERC721Pausable is ERC721, Pausable {
     function _update(
         address to,
         uint256 tokenId,
-        address operatorCheck
+        address auth
     ) internal virtual override whenNotPaused returns (address) {
-        return super._update(to, tokenId, operatorCheck);
+        return super._update(to, tokenId, auth);
     }
 }

contracts/token/ERC721/extensions/ERC721Royalty.sol

@@ -29,8 +29,8 @@ abstract contract ERC721Royalty is ERC2981, ERC721 {
     /**
      * @dev See {ERC721-_update}. When burning, this override will additionally clear the royalty information for the token.
      */
-    function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) {
+    function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) {
-        address from = super._update(to, tokenId, operatorCheck);
+        address from = super._update(to, tokenId, auth);
 
         if (to == address(0)) {
             _resetTokenRoyalty(tokenId);

contracts/token/ERC721/extensions/ERC721URIStorage.sol

@@ -55,7 +55,7 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 {
      * - `tokenId` must exist.
      */
     function _setTokenURI(uint256 tokenId, string memory _tokenURI) internal virtual {
-        if (!_exists(tokenId)) {
+        if (_ownerOf(tokenId) == address(0)) {
             revert ERC721NonexistentToken(tokenId);
         }
         _tokenURIs[tokenId] = _tokenURI;
@@ -68,8 +68,8 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 {
      * token-specific URI was set for the token, and if so, it deletes the token URI from
      * the storage mapping.
      */
-    function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) {
+    function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) {
-        address from = super._update(to, tokenId, operatorCheck);
+        address from = super._update(to, tokenId, auth);
 
         if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) {
             delete _tokenURIs[tokenId];

contracts/token/ERC721/extensions/ERC721Votes.sol

@@ -20,8 +20,8 @@ abstract contract ERC721Votes is ERC721, Votes {
      *
      * Emits a {IVotes-DelegateVotesChanged} event.
      */
-    function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) {
+    function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) {
-        address from = super._update(to, tokenId, operatorCheck);
+        address from = super._update(to, tokenId, auth);
 
         _transferVotingUnits(from, to, 1);
 

contracts/token/ERC721/extensions/ERC721Wrapper.sol

@@ -50,6 +50,8 @@ abstract contract ERC721Wrapper is ERC721, IERC721Receiver {
         uint256 length = tokenIds.length;
         for (uint256 i = 0; i < length; ++i) {
             uint256 tokenId = tokenIds[i];
+            // Setting an "auth" arguments means that `_update` will check that the token exists (from != 0),
+            // no need to duplicate that check here.
             _update(address(0), tokenId, _msgSender());
             // Checks were already performed at this point, and there's no way to retake ownership or approval from
             // the wrapped tokenId after this point, so it's safe to remove the reentrancy check for the next line.

test/token/ERC721/ERC721.behavior.js

@@ -524,14 +524,6 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper
         });
       });
 
-      context('when the address that receives the approval is the owner', function () {
-        it('reverts', async function () {
-          await expectRevertCustomError(this.token.approve(owner, tokenId, { from: owner }), 'ERC721InvalidOperator', [
-            owner,
-          ]);
-        });
-      });
-
       context('when the sender does not own the given token ID', function () {
         it('reverts', async function () {
           await expectRevertCustomError(
@@ -644,16 +636,6 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper
           });
         });
       });
-
-      context('when the operator is the owner', function () {
-        it('reverts', async function () {
-          await expectRevertCustomError(
-            this.token.setApprovalForAll(owner, true, { from: owner }),
-            'ERC721InvalidOperator',
-            [owner],
-          );
-        });
-      });
     });
 
     describe('getApproved', async function () {

test/token/ERC721/extensions/ERC721Consecutive.test.js

@@ -103,7 +103,7 @@ contract('ERC721Consecutive', function (accounts) {
         it('simple minting is possible after construction', async function () {
           const tokenId = sum(...batches.map(b => b.amount)) + offset;
 
-          expect(await this.token.$_exists(tokenId)).to.be.equal(false);
+          await expectRevertCustomError(this.token.ownerOf(tokenId), 'ERC721NonexistentToken', [tokenId]);
 
           expectEvent(await this.token.$_mint(user1, tokenId), 'Transfer', {
             from: constants.ZERO_ADDRESS,
@@ -115,7 +115,7 @@ contract('ERC721Consecutive', function (accounts) {
         it('cannot mint a token that has been batched minted', async function () {
           const tokenId = sum(...batches.map(b => b.amount)) + offset - 1;
 
-          expect(await this.token.$_exists(tokenId)).to.be.equal(true);
+          expect(await this.token.ownerOf(tokenId)).to.be.not.equal(constants.ZERO_ADDRESS);
 
           await expectRevertCustomError(this.token.$_mint(user1, tokenId), 'ERC721InvalidSender', [ZERO_ADDRESS]);
         });
@@ -151,13 +151,11 @@ contract('ERC721Consecutive', function (accounts) {
         it('tokens can be burned and re-minted #2', async function () {
           const tokenId = web3.utils.toBN(sum(...batches.map(({ amount }) => amount)) + offset);
 
-          expect(await this.token.$_exists(tokenId)).to.be.equal(false);
           await expectRevertCustomError(this.token.ownerOf(tokenId), 'ERC721NonexistentToken', [tokenId]);
 
           // mint
           await this.token.$_mint(user1, tokenId);
 
-          expect(await this.token.$_exists(tokenId)).to.be.equal(true);
           expect(await this.token.ownerOf(tokenId), user1);
 
           // burn
@@ -167,7 +165,6 @@ contract('ERC721Consecutive', function (accounts) {
             tokenId,
           });
 
-          expect(await this.token.$_exists(tokenId)).to.be.equal(false);
           await expectRevertCustomError(this.token.ownerOf(tokenId), 'ERC721NonexistentToken', [tokenId]);
 
           // re-mint
@@ -177,7 +174,6 @@ contract('ERC721Consecutive', function (accounts) {
             tokenId,
           });
 
-          expect(await this.token.$_exists(tokenId)).to.be.equal(true);
           expect(await this.token.ownerOf(tokenId), user2);
         });
       });

test/token/ERC721/extensions/ERC721Pausable.test.js

@@ -81,12 +81,6 @@ contract('ERC721Pausable', function (accounts) {
       });
     });
 
-    describe('exists', function () {
-      it('returns token existence', async function () {
-        expect(await this.token.$_exists(firstTokenId)).to.equal(true);
-      });
-    });
-
     describe('isApprovedForAll', function () {
       it('returns the approval of the operator', async function () {
         expect(await this.token.isApprovedForAll(owner, operator)).to.equal(false);

test/token/ERC721/extensions/ERC721URIStorage.test.js

@@ -86,7 +86,6 @@ contract('ERC721URIStorage', function (accounts) {
     it('tokens without URI can be burnt ', async function () {
       await this.token.$_burn(firstTokenId, { from: owner });
 
-      expect(await this.token.$_exists(firstTokenId)).to.equal(false);
       await expectRevertCustomError(this.token.tokenURI(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]);
     });
 
@@ -95,7 +94,6 @@ contract('ERC721URIStorage', function (accounts) {
 
       await this.token.$_burn(firstTokenId, { from: owner });
 
-      expect(await this.token.$_exists(firstTokenId)).to.equal(false);
       await expectRevertCustomError(this.token.tokenURI(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]);
     });
   });