fix(token): require token existence in NonFungibleToken _approve

contracts/src/token/NonFungibleToken.compact

@@ -58,7 +58,7 @@ pragma language_version >= 0.23.0;
  * or in ledger writes. Canonicalization zeroes out the inactive branch of the Either,
  * ensuring that two values with the same active branch always resolve to the same map key
  * regardless of what data the inactive branch carries. Write paths are canonicalized in
- * `_update` (for `_owners` and `_balances`), `_approve` (for `_tokenApprovals`), and
+ * `_update` (for `_owners` and `_balances`), `_unsafeApprove` (for `_tokenApprovals`), and
  * `_setApprovalForAll` (for `_operatorApprovals`). Read paths are canonicalized in
  * `balanceOf`, `isApprovedForAll`, and `_isAuthorized`.
  *
@@ -338,7 +338,7 @@ module NonFungibleToken {
    * In the case of an external (non-contract) caller, the caller's identity is derived from the `wit_NonFungibleTokenSK`
    * witness as `persistentHash(secretKey)`.
    *
-   * @circuitInfo k=13, rows=3468
+   * @circuitInfo k=13, rows=3415
    *
    * Requirements:
    *
@@ -383,7 +383,7 @@ module NonFungibleToken {
    * In the case of an external (non-contract) caller, the caller's identity is derived from the `wit_NonFungibleTokenSK`
    * witness as `persistentHash(secretKey)`.
    *
-   * @circuitInfo k=13, rows=2936
+   * @circuitInfo k=13, rows=2947
    *
    * Requirements:
    *
@@ -439,7 +439,7 @@ module NonFungibleToken {
    * In the case of an external (non-contract) caller, the caller's identity is derived from the `wit_NonFungibleTokenSK`
    * witness as `persistentHash(secretKey)`.
    *
-   * @circuitInfo k=13, rows=4800
+   * @circuitInfo k=13, rows=4739
    *
    * Requirements:
    *
@@ -477,7 +477,7 @@ module NonFungibleToken {
    * are not currently supported. Tokens sent to a contract address may become irretrievable.
    * Once contract-to-contract calls are supported, this circuit may be deprecated.
    *
-   * @circuitInfo k=13, rows=4797
+   * @circuitInfo k=13, rows=4736
    *
    * Requirements:
    *
@@ -499,11 +499,14 @@ module NonFungibleToken {
                    ): [] {
     assertInitialized();
     assert(!Utils_isTargetZero(to), "NonFungibleToken: invalid receiver");
-    // Setting an "auth" argument enables the `_isAuthorized` check which verifies that the token exists
-    // (fromAddress != 0). Therefore, it is not needed to verify that the return value is not 0 here.
     const auth = left<Bytes<32>, ContractAddress>(_computeAccountId());
     const previousOwner = _update(to, tokenId, auth);
 
+    // The `_isAuthorized` check inside `_update` already implies the token exists, but that
+    // holds only while approvals are never recorded for unminted tokens. Assert it directly
+    // so a misused `_unsafeApprove` cannot turn this circuit into an unauthorized mint.
+    assert(!Utils_isTargetZero(previousOwner), "NonFungibleToken: nonexistent token");
+
     const canonFrom = Utils_canonicalize<Bytes<32>, ContractAddress>(fromAddress);
     assert(previousOwner == canonFrom, "NonFungibleToken: incorrect owner");
   }
@@ -643,8 +646,8 @@ module NonFungibleToken {
 
     // Execute the update
     if (!Utils_isTargetZero(disclose(fromAddress))) {
-      // Clear approval. No need to re-authorize
-      _approve(Utils_zeroAccount(), tokenId, Utils_zeroAccount());
+      // Clear approval. No need to re-authorize or re-read the owner
+      _unsafeApprove(Utils_zeroAccount(), tokenId, Utils_zeroAccount(), false);
       const canonFrom = Utils_canonicalize<Bytes<32>, ContractAddress>(fromAddress);
       const newBalance = _balances.lookup(disclose(canonFrom)) - 1 as Uint<128>;
       _balances.insert(disclose(canonFrom), disclose(newBalance));
@@ -666,7 +669,7 @@ module NonFungibleToken {
   /**
    * @description Mints `tokenId` and transfers it to `to`.
    *
-   * @circuitInfo k=11, rows=1473
+   * @circuitInfo k=11, rows=1411
    *
    * Requirements:
    *
@@ -690,7 +693,7 @@ module NonFungibleToken {
   /**
    * @description Mints `tokenId` and transfers it to `to`. It does NOT check if the recipient is a ContractAddress.
    *
-   * @circuitInfo k=11, rows=1470
+   * @circuitInfo k=11, rows=1408
    *
    * Requirements:
    *
@@ -723,7 +726,7 @@ module NonFungibleToken {
    * The approval is cleared when the token is burned.
    * This circuit does not check if the sender is authorized to operate on the token.
    *
-   * @circuitInfo k=10, rows=509
+   * @circuitInfo k=10, rows=512
    *
    * Requirements:
    *
@@ -748,7 +751,7 @@ module NonFungibleToken {
    * interactions are supported in Compact. This restriction prevents assets from being inadvertently
    * locked in contracts that cannot currently handle token receipt.
    *
-   * @circuitInfo k=12, rows=2067
+   * @circuitInfo k=11, rows=2005
    *
    * Requirements:
    *
@@ -779,7 +782,7 @@ module NonFungibleToken {
    * As opposed to {_unsafeTransferFrom}, this imposes no restrictions on the caller's identity.
    * It does NOT check if the recipient is a ContractAddress.
    *
-   * @circuitInfo k=12, rows=2064
+   * @circuitInfo k=11, rows=2002
    *
    * Requirements:
    *
@@ -815,11 +818,12 @@ module NonFungibleToken {
   /**
    * @description Approve `to` to operate on `tokenId`.
    *
-   * @circuitInfo k=11, rows=1810
+   * @circuitInfo k=11, rows=1757
    *
    * Requirements:
    *
    * - The contract is initialized.
+   * - `tokenId` must exist.
    * - If `auth` is non 0, then this function will check that `auth` is either the owner of the token,
    * or approved to operate on the token (by the owner).
    *
@@ -833,6 +837,43 @@ module NonFungibleToken {
                    tokenId: Uint<128>,
                    auth: Either<Bytes<32>, ContractAddress>
                    ): [] {
+    _unsafeApprove(to, tokenId, auth, true);
+  }
+
+  /**
+   * @description Approve `to` to operate on `tokenId`, requiring the token to exist
+   * only when `isExistenceRequired` is true.
+   *
+   * @circuitInfo k=11, rows=1870
+   *
+   * @warning Passing false skips the check that `tokenId` was minted. Both
+   * `_unsafeTransferFrom` and `_update` read that invariant back, so a planted approval
+   * can mint a token nobody authorized or outlive a later mint. Pass false only where
+   * the owner has already been read, as `_update` does.
+   *
+   * @dev Counterpart to Solidity's `_approve(to, tokenId, auth, bool emitEvent)`, where
+   * the flag also suppresses the `Approval` event. Compact has no events, so it carries
+   * the existence check alone.
+   *
+   * Requirements:
+   *
+   * - The contract is initialized.
+   * - `tokenId` must exist, unless `isExistenceRequired` is false and `auth` is zero.
+   * - If `auth` is non 0, then this function will check that `auth` is either the owner of the token,
+   * or approved to operate on the token (by the owner).
+   *
+   * @param {Either<Bytes<32>, ContractAddress>} to - The target account to approve
+   * @param {Uint<128>} tokenId - The token to approve
+   * @param {Either<Bytes<32>, ContractAddress>} auth - An account authorized to operate on all tokens held by the owner of the token
+   * @param {Boolean} isExistenceRequired - Whether `tokenId` must exist
+   * @return {[]} - Empty tuple.
+   */
+  export circuit _unsafeApprove(
+                   to: Either<Bytes<32>, ContractAddress>,
+                   tokenId: Uint<128>,
+                   auth: Either<Bytes<32>, ContractAddress>,
+                   isExistenceRequired: Boolean
+                   ): [] {
     assertInitialized();
     // Canonicalize + normalize `to`
     const canonTo = Utils_canonicalize<Bytes<32>, ContractAddress>(to);
@@ -841,11 +882,14 @@ module NonFungibleToken {
     // Canonicalize auth
     const canonAuth = Utils_canonicalize<Bytes<32>, ContractAddress>(auth);
 
-    if (!Utils_isTargetZero(disclose(canonAuth))) {
+    // Avoid reading the owner unless necessary
+    if (disclose(isExistenceRequired) || !Utils_isTargetZero(disclose(canonAuth))) {
       const owner = _requireOwned(tokenId);
 
       // We do not use _isAuthorized because single-token approvals should not be able to call approve
-      assert((owner == disclose(canonAuth) || isApprovedForAll(owner, canonAuth)),
+      assert((Utils_isTargetZero(disclose(canonAuth)) ||
+              owner == disclose(canonAuth) ||
+              isApprovedForAll(owner, canonAuth)),
              "NonFungibleToken: invalid approver"
              );
     }
@@ -856,7 +900,7 @@ module NonFungibleToken {
   /**
    * @description Approve `operator` to operate on all of `owner` tokens.
    *
-   * @circuitInfo k=11, rows=1261
+   * @circuitInfo k=11, rows=1282
    *
    * Requirements:
    *

contracts/src/token/test/mocks/MockNonFungibleToken.compact

@@ -98,6 +98,15 @@ export circuit _approve(
   return NonFungibleToken__approve(to, tokenId, auth);
 }
 
+export circuit _unsafeApprove(
+  to: Either<Bytes<32>, ContractAddress>,
+  tokenId: Uint<128>,
+  auth: Either<Bytes<32>, ContractAddress>,
+  isExistenceRequired: Boolean
+): [] {
+  return NonFungibleToken__unsafeApprove(to, tokenId, auth, isExistenceRequired);
+}
+
 export circuit _checkAuthorized(
   owner: Either<Bytes<32>, ContractAddress>,
   spender: Either<Bytes<32>, ContractAddress>,

contracts/src/token/test/nonFungibleToken.test.ts

@@ -816,6 +816,95 @@ describe('NonFungibleToken', () => {
       await token._approve(ZERO_ACCOUNT, TOKENID_1, OWNER.either);
       expect(await token.getApproved(TOKENID_1)).toEqual(ZERO_ACCOUNT);
     });
+
+    it('should throw if token does not exist and auth is zero', async () => {
+      await expect(
+        token._approve(SPENDER.either, NON_EXISTENT_TOKEN, ZERO_ACCOUNT),
+      ).rejects.toThrow('NonFungibleToken: nonexistent token');
+
+      expect(await token._getApproved(NON_EXISTENT_TOKEN)).toEqual(
+        ZERO_ACCOUNT,
+      );
+    });
+  });
+
+  describe('_unsafeApprove', () => {
+    it('should match _approve when existence is required', async () => {
+      await token._mint(OWNER.either, TOKENID_1);
+      await token._unsafeApprove(SPENDER.either, TOKENID_1, OWNER.either, true);
+      expect(await token.getApproved(TOKENID_1)).toEqual(SPENDER.either);
+    });
+
+    it('should reject a nonexistent token when existence is required', async () => {
+      await expect(
+        token._unsafeApprove(
+          SPENDER.either,
+          NON_EXISTENT_TOKEN,
+          ZERO_ACCOUNT,
+          true,
+        ),
+      ).rejects.toThrow('NonFungibleToken: nonexistent token');
+    });
+
+    it('should record an approval for a nonexistent token when existence is not required', async () => {
+      // The documented unsafe behaviour: no existence check, approval recorded.
+      await token._unsafeApprove(
+        SPENDER.either,
+        NON_EXISTENT_TOKEN,
+        ZERO_ACCOUNT,
+        false,
+      );
+      expect(await token._getApproved(NON_EXISTENT_TOKEN)).toEqual(
+        SPENDER.either,
+      );
+    });
+
+    it('should still check the approver when existence is not required', async () => {
+      await token._mint(OWNER.either, TOKENID_1);
+      await expect(
+        token._unsafeApprove(
+          SPENDER.either,
+          TOKENID_1,
+          UNAUTHORIZED.either,
+          false,
+        ),
+      ).rejects.toThrow('NonFungibleToken: invalid approver');
+    });
+
+    it('should leave an approval planted before the mint in place', async () => {
+      await token._unsafeApprove(
+        SPENDER.either,
+        TOKENID_1,
+        ZERO_ACCOUNT,
+        false,
+      );
+
+      // `_update` clears approvals only for a non-zero source, so a mint
+      // leaves the planted approval standing.
+      await token._mint(OWNER.either, TOKENID_1);
+      expect(await token.getApproved(TOKENID_1)).toEqual(SPENDER.either);
+    });
+
+    it('should not let a planted approval mint through transferFrom', async () => {
+      // Plant the approval a misused composer call would leave behind.
+      await token._unsafeApprove(
+        SPENDER.either,
+        NON_EXISTENT_TOKEN,
+        ZERO_ACCOUNT,
+        false,
+      );
+
+      // The stale approval satisfies `_isAuthorized`, so `_checkAuthorized`
+      // passes on the zero owner. The transfer must die at the previous-owner
+      // assert in `_unsafeTransferFrom` instead.
+      await token.privateState.injectSecretKey(SPENDER.secretKey);
+      await expect(
+        token.transferFrom(ZERO_ACCOUNT, SPENDER.either, NON_EXISTENT_TOKEN),
+      ).rejects.toThrow('NonFungibleToken: nonexistent token');
+
+      expect(await token._ownerOf(NON_EXISTENT_TOKEN)).toEqual(ZERO_ACCOUNT);
+      expect(await token.balanceOf(SPENDER.either)).toEqual(0n);
+    });
   });
 
   describe('_checkAuthorized', () => {
@@ -1441,6 +1530,7 @@ const circuitsToFail: FailingCircuits[] = [
   ['_requireOwned', [TOKENID_1]],
   ['_ownerOf', [TOKENID_1]],
   ['_approve', [OWNER.either, TOKENID_1, SPENDER.either]],
+  ['_unsafeApprove', [OWNER.either, TOKENID_1, SPENDER.either, true]],
   ['_checkAuthorized', [OWNER.either, SPENDER.either, TOKENID_1]],
   ['_isAuthorized', [OWNER.either, SPENDER.either, TOKENID_1]],
   ['_getApproved', [TOKENID_1]],

contracts/src/token/test/simulators/NonFungibleTokenSimulator.ts

@@ -241,6 +241,32 @@ export class NonFungibleTokenSimulator extends NonFungibleTokenSimulatorBase {
     return this.circuits.impure._approve(to, tokenId, auth);
   }
 
+  /**
+   * @description Approve `to` to operate on `tokenId`, requiring the token to exist
+   * only when `isExistenceRequired` is true.
+   *
+   * WARNING: Passing false records the approval without verifying that `tokenId` was
+   * minted. See the module docs for the invariant this breaks.
+   *
+   * @param to The target account to approve
+   * @param tokenId The token to approve
+   * @param auth An account authorized to operate on all tokens held by the owner the token
+   * @param isExistenceRequired Whether `tokenId` must exist
+   */
+  public _unsafeApprove(
+    to: Either<Uint8Array, ContractAddress>,
+    tokenId: bigint,
+    auth: Either<Uint8Array, ContractAddress>,
+    isExistenceRequired: boolean,
+  ): Promise<[]> {
+    return this.circuits.impure._unsafeApprove(
+      to,
+      tokenId,
+      auth,
+      isExistenceRequired,
+    );
+  }
+
   /**
    * @description Checks if `spender` can operate on `tokenId`, assuming the provided `owner` is the actual owner.
    * Reverts if: