fix(smart-contracts): renounce role after initialization when adding …

…a new template (#15248)

* fix typo in initializer

* add a test

* release fixed unlock 14 contract

* add back `renounceLockManager`

* add back to iface

packages/contracts/src/contracts/PublicLock/PublicLockV15.sol

@@ -3146,6 +3146,10 @@ contract MixinRoles is AccessControlUpgradeable, MixinErrors {
     return hasRole(LOCK_MANAGER_ROLE, account);
   }
 
+  // kept for backward compat
+  function renounceLockManager() public {
+    renounceRole(LOCK_MANAGER_ROLE, msg.sender);
+  }
 
   // added -1 slot for the onRoleHook address in v15
   uint256[999] private __safe_upgrade_gap;

packages/contracts/src/contracts/Unlock/UnlockV14.sol

@@ -2971,7 +2971,7 @@ contract Unlock is UnlockInitializable, UnlockOwnable {
     // claim the template so that no-one else could
     try IPublicLock(impl).initialize(address(this), 0, address(0), 0, 0, "") {
       // renounce Unlock's lock manager role that was added during initialization
-      IPublicLock(impl).revokeRole(keccak256("LOCK_MANAGER"), address(this));
+      IPublicLock(impl).renounceRole(keccak256("LOCK_MANAGER"), address(this));
     } catch {
       // failure means that the template is already initialized
     }

smart-contracts/contracts/Unlock.sol

@@ -206,7 +206,7 @@ contract Unlock is UnlockInitializable, UnlockOwnable {
     // claim the template so that no-one else could
     try IPublicLock(impl).initialize(address(this), 0, address(0), 0, 0, "") {
       // renounce Unlock's lock manager role that was added during initialization
-      IPublicLock(impl).revokeRole(keccak256("LOCK_MANAGER"), address(this));
+      IPublicLock(impl).renounceRole(keccak256("LOCK_MANAGER"), address(this));
     } catch {
       // failure means that the template is already initialized
     }

smart-contracts/contracts/interfaces/IPublicLock.sol

@@ -573,6 +573,9 @@ interface IPublicLock {
 
   function hasRole(bytes32 role, address account) external view returns (bool);
 
+  // helper function
+  function renounceLockManager() external;
+
   /** `owner()` is provided as an helper to mimick the `Ownable` contract ABI.
    * The `Ownable` logic is used by many 3rd party services to determine
    * contract ownership - e.g. who is allowed to edit metadata on Opensea.

smart-contracts/contracts/mixins/MixinRoles.sol

@@ -61,6 +61,10 @@ contract MixinRoles is AccessControlUpgradeable, MixinErrors {
     return hasRole(LOCK_MANAGER_ROLE, account);
   }
 
+  // kept for backward compat with Unlock prior v14
+  function renounceLockManager() public {
+    renounceRole(LOCK_MANAGER_ROLE, msg.sender);
+  }
 
   // added -1 slot for the onRoleHook address in v15
   uint256[999] private __safe_upgrade_gap;

smart-contracts/test/Lock/initializers.js

@@ -52,7 +52,7 @@ describe('Lock / initializers', () => {
   })
 
   describe('initializing when setting as Unlock template', () => {
-    it('admin role is revoked when adding a template', async () => {
+    it('admin role is revoked when adding a template that hasnt been initialized', async () => {
       // deploy contracts
       const PublicLock = await ethers.getContractFactory(
         'contracts/PublicLock.sol:PublicLock'
@@ -72,6 +72,12 @@ describe('Lock / initializers', () => {
         false
       )
 
+      // deployer should not be lock manager
+      await assert.equal(
+        await template.isLockManager(await deployer.getAddress()),
+        false
+      )
+
       await reverts(
         template.initialize(
           await lockOwner.getAddress(),