fix(access): l-05 owner can be zero

contracts/src/access/Ownable.compact

@@ -21,15 +21,10 @@ pragma language_version >= 0.21.0;
  * if a contract is granted ownership, the owner contract cannot directly call the protected
  * circuit.
  *
- * @notice This module does offer experimental circuits that allow ownership to be granted
- * to contract addresses (`_unsafeTransferOwnership` and `_unsafeUncheckedTransferOwnership`).
- * Note that the circuit names are very explicit ("unsafe") with these experimental circuits.
- * Until contract-to-contract calls are supported,
- * there is no direct way for a contract to call circuits of other contracts
- * or transfer ownership back to a user.
- *
- * @notice The unsafe circuits are planned to become deprecated once contract-to-contract calls
- * are supported.
+ * @notice Ownership transfers to contract addresses are not currently supported because
+ * `assertOnlyOwner` cannot authenticate contract address callers. The platform does not
+ * provide a mechanism to identify the calling contract. Until contract-to-contract calls
+ * are supported, ownership is restricted to public key accounts.
  */
 module Ownable {
   import CompactStandardLibrary;
@@ -119,7 +114,7 @@ module Ownable {
    * @param {Either<ZswapCoinPublicKey, ContractAddress>} newOwner - The new owner.
    * @returns {[]} Empty tuple.
    */
-  export circuit _unsafeTransferOwnership(newOwner: Either<ZswapCoinPublicKey, ContractAddress>): [] {
+  circuit _unsafeTransferOwnership(newOwner: Either<ZswapCoinPublicKey, ContractAddress>): [] {
     Initializable_assertInitialized();
     assertOnlyOwner();
     assert(!Utils_isKeyOrAddressZero(newOwner), "Ownable: invalid new owner");
@@ -206,7 +201,7 @@ module Ownable {
    * @param {Either<ZswapCoinPublicKey, ContractAddress>} newOwner - The new owner.
    * @returns {[]} Empty tuple.
    */
-  export circuit _unsafeUncheckedTransferOwnership(
+  circuit _unsafeUncheckedTransferOwnership(
                    newOwner: Either<ZswapCoinPublicKey, ContractAddress>
                    ): [] {
     Initializable_assertInitialized();

contracts/src/access/test/Ownable.test.ts

@@ -18,11 +18,6 @@ const isBadInit = false;
 
 let ownable: OwnableSimulator;
 
-const newOwnerTypes = [
-  ['contract', Z_OWNER_CONTRACT],
-  ['pubkey', Z_NEW_OWNER],
-] as const;
-
 const zeroTypes = [
   ['contract', utils.ZERO_ADDRESS],
   ['pubkey', utils.ZERO_KEY],
@@ -55,10 +50,8 @@ describe('Ownable', () => {
       ['owner', []],
       ['assertOnlyOwner', []],
       ['transferOwnership', [Z_OWNER]],
-      ['_unsafeTransferOwnership', [Z_OWNER]],
       ['renounceOwnership', []],
       ['_transferOwnership', [Z_OWNER]],
-      ['_unsafeUncheckedTransferOwnership', [Z_OWNER]],
     ];
     it.each(
       circuitsToFail,
@@ -154,57 +147,6 @@ describe('Ownable', () => {
       });
     });
 
-    describe('_unsafeTransferOwnership', () => {
-      describe.each(
-        newOwnerTypes,
-      )('when the owner is a %s', (type, newOwner) => {
-        it('should transfer ownership', () => {
-          ownable.as(OWNER)._unsafeTransferOwnership(newOwner);
-          expect(ownable.owner()).toEqual(newOwner);
-
-          // Original owner
-          expect(() => {
-            ownable.as(OWNER).assertOnlyOwner();
-          }).toThrow('Ownable: caller is not the owner');
-
-          if (type === 'pubkey') {
-            // New owner
-            expect(() => {
-              ownable.as(NEW_OWNER).assertOnlyOwner();
-            }).not.toThrow();
-          }
-        });
-      });
-
-      it('should fail when unauthorized transfers ownership', () => {
-        expect(() => {
-          ownable.as(UNAUTHORIZED)._unsafeTransferOwnership(Z_NEW_OWNER);
-        }).toThrow('Ownable: caller is not the owner');
-      });
-
-      it('should fail when transferring to zero (pk)', () => {
-        expect(() => {
-          ownable.as(OWNER)._unsafeTransferOwnership(utils.ZERO_KEY);
-        }).toThrow('Ownable: invalid new owner');
-      });
-
-      it('should fail when transferring to zero (contract)', () => {
-        expect(() => {
-          ownable.as(OWNER)._unsafeTransferOwnership(utils.ZERO_ADDRESS);
-        }).toThrow('Ownable: invalid new owner');
-      });
-
-      it('should transfer multiple times', () => {
-        ownable.as(OWNER)._unsafeTransferOwnership(Z_NEW_OWNER);
-
-        ownable.as(NEW_OWNER)._unsafeTransferOwnership(Z_OWNER);
-
-        ownable.as(OWNER)._unsafeTransferOwnership(Z_OWNER_CONTRACT);
-
-        expect(ownable.owner()).toEqual(Z_OWNER_CONTRACT);
-      });
-    });
-
     describe('renounceOwnership', () => {
       it('should renounce ownership', () => {
         expect(ownable.owner()).toEqual(Z_OWNER);
@@ -274,41 +216,5 @@ describe('Ownable', () => {
       });
     });
 
-    describe('_unsafeUncheckedTransferOwnership', () => {
-      describe.each(newOwnerTypes)('when the owner is a %s', (_, newOwner) => {
-        it('should transfer ownership', () => {
-          ownable._unsafeUncheckedTransferOwnership(newOwner);
-          expect(ownable.owner()).toEqual(newOwner);
-        });
-      });
-
-      it('should enforce permissions after transfer (pk)', () => {
-        ownable._unsafeUncheckedTransferOwnership(Z_NEW_OWNER);
-
-        // Original owner
-        expect(() => {
-          ownable.as(OWNER).assertOnlyOwner();
-        }).toThrow('Ownable: caller is not the owner');
-
-        expect(() => {
-          ownable.as(UNAUTHORIZED).assertOnlyOwner();
-        }).toThrow('Ownable: caller is not the owner');
-
-        // New owner
-        expect(() => {
-          ownable.as(NEW_OWNER).assertOnlyOwner();
-        }).not.toThrow();
-      });
-
-      it('should transfer multiple times', () => {
-        ownable.as(OWNER)._unsafeUncheckedTransferOwnership(Z_NEW_OWNER);
-
-        ownable.as(NEW_OWNER)._unsafeUncheckedTransferOwnership(Z_OWNER);
-
-        ownable.as(OWNER)._unsafeUncheckedTransferOwnership(Z_OWNER_CONTRACT);
-
-        expect(ownable.owner()).toEqual(Z_OWNER_CONTRACT);
-      });
-    });
   });
 });

contracts/src/access/test/mocks/MockOwnable.compact

@@ -29,10 +29,6 @@ export circuit transferOwnership(newOwner: Either<ZswapCoinPublicKey, ContractAd
   return Ownable_transferOwnership(newOwner);
 }
 
-export circuit _unsafeTransferOwnership(newOwner: Either<ZswapCoinPublicKey, ContractAddress>): [] {
-  return Ownable__unsafeTransferOwnership(newOwner);
-}
-
 export circuit renounceOwnership(): [] {
   return Ownable_renounceOwnership();
 }
@@ -45,6 +41,3 @@ export circuit _transferOwnership(newOwner: Either<ZswapCoinPublicKey, ContractA
   return Ownable__transferOwnership(newOwner);
 }
 
-export circuit _unsafeUncheckedTransferOwnership(newOwner: Either<ZswapCoinPublicKey, ContractAddress>): [] {
-  return Ownable__unsafeUncheckedTransferOwnership(newOwner);
-}

contracts/src/access/test/simulators/OwnableSimulator.ts

@@ -69,16 +69,6 @@ export class OwnableSimulator extends OwnableSimulatorBase {
     this.circuits.impure.transferOwnership(newOwner);
   }
 
-  /**
-   * @description Unsafe variant of `transferOwnership`.
-   * @param newOwner - The new owner.
-   */
-  public _unsafeTransferOwnership(
-    newOwner: Either<ZswapCoinPublicKey, ContractAddress>,
-  ) {
-    this.circuits.impure._unsafeTransferOwnership(newOwner);
-  }
-
   /**
    * @description Leaves the contract without an owner.
    * It will not be possible to call `assertOnlyOnwer` circuits anymore.
@@ -107,13 +97,4 @@ export class OwnableSimulator extends OwnableSimulatorBase {
     this.circuits.impure._transferOwnership(newOwner);
   }
 
-  /**
-   * @description Unsafe variant of `_transferOwnership`.
-   * @param newOwner - The new owner.
-   */
-  public _unsafeUncheckedTransferOwnership(
-    newOwner: Either<ZswapCoinPublicKey, ContractAddress>,
-  ) {
-    this.circuits.impure._unsafeUncheckedTransferOwnership(newOwner);
-  }
 }