ON-793: fixed _hasNonRevocableRole function

OriumNetwork · May 9, 2024 · 9532d9d · 9532d9d
1 parent c77f75f
commit 9532d9d
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 39 deletions.
diff --git a/contracts/ERC7432/NftRolesRegistryVault.sol b/contracts/ERC7432/NftRolesRegistryVault.sol
@@ -13,7 +13,7 @@ contract NftRolesRegistryVault is IERC7432 {
         bytes data;
     }
 
-    bytes32[] public allowedRoles = [keccak256('UNIQUE_ROLE')];
+    bytes32[] public allowedRoles;
 
     // roleId => isAllowed
     mapping(bytes32 => bool) public isRoleAllowed;
@@ -28,6 +28,7 @@ contract NftRolesRegistryVault is IERC7432 {
     mapping(address => mapping(address => mapping(address => bool))) public tokenApprovals;
 
     constructor() {
+        allowedRoles = [keccak256('UNIQUE_ROLE')];
         for (uint256 i = 0; i < allowedRoles.length; i++) {
             isRoleAllowed[allowedRoles[i]] = true;
         }
@@ -98,7 +99,7 @@ contract NftRolesRegistryVault is IERC7432 {
     function unlockToken(address _tokenAddress, uint256 _tokenId) external override {
         address originalOwner = originalOwners[_tokenAddress][_tokenId];
 
-        require(_isLocked(_tokenAddress, _tokenId), 'NftRolesRegistryVault: NFT is locked');
+        require(!_hasNonRevocableRole(_tokenAddress, _tokenId), 'NftRolesRegistryVault: NFT is locked');
 
         require(
             originalOwner == msg.sender || isRoleApprovedForAll(_tokenAddress, originalOwner, msg.sender),
@@ -159,10 +160,9 @@ contract NftRolesRegistryVault is IERC7432 {
         uint256 _tokenId,
         bytes32 _roleId
     ) external view returns (bool revocable_) {
-        if (roles[_tokenAddress][_tokenId][_roleId].expirationDate > block.timestamp) {
-            return roles[_tokenAddress][_tokenId][_roleId].revocable;
-        }
-        return false;
+        return
+            roles[_tokenAddress][_tokenId][_roleId].expirationDate > block.timestamp &&
+            roles[_tokenAddress][_tokenId][_roleId].revocable;
     }
 
     function isRoleApprovedForAll(address _tokenAddress, address _owner, address _operator) public view returns (bool) {
@@ -224,13 +224,16 @@ contract NftRolesRegistryVault is IERC7432 {
         revert('NftRolesRegistryVault: role does not exist or sender is not approved');
     }
 
-    /// @notice Checks whether an NFT is locked.
+    /// @notice Checks whether an NFT has at least one non-revocable role.
     /// @param _tokenAddress The token address.
     /// @param _tokenId The token identifier.
-    /// @return True if the NFT is locked.
-    function _isLocked(address _tokenAddress, uint256 _tokenId) internal view returns (bool) {
+    /// @return true if the NFT is locked.
+    function _hasNonRevocableRole(address _tokenAddress, uint256 _tokenId) internal view returns (bool) {
         for (uint256 i = 0; i < allowedRoles.length; i++) {
-            if (roles[_tokenAddress][_tokenId][allowedRoles[i]].expirationDate > block.timestamp) {
+            if (
+                !roles[_tokenAddress][_tokenId][allowedRoles[i]].revocable &&
+                roles[_tokenAddress][_tokenId][allowedRoles[i]].expirationDate > block.timestamp
+            ) {
                 return true;
             }
         }

diff --git a/test/ERC7432/NftRolesRegistryVault.ts b/test/ERC7432/NftRolesRegistryVault.ts
@@ -30,9 +30,9 @@ describe('NftRolesRegistryVault', () => {
     anotherUser = signers[2]
   }
 
-  async function depositNftAndGrantRole({ recipient = AddressZero }) {
+  async function depositNftAndGrantRole({ recipient = AddressZero, revocable = role.revocable }) {
     await MockErc721Token.connect(owner).approve(NftRolesRegistryVault.address, role.tokenId)
-    await expect(NftRolesRegistryVault.connect(owner).grantRole({ ...role, recipient }))
+    await expect(NftRolesRegistryVault.connect(owner).grantRole({ ...role, recipient, revocable }))
       .to.emit(NftRolesRegistryVault, 'RoleGranted')
       .withArgs(
         role.tokenAddress,
@@ -41,7 +41,7 @@ describe('NftRolesRegistryVault', () => {
         owner.address,
         recipient,
         role.expirationDate,
-        role.revocable,
+        revocable,
         role.data,
       )
       .to.emit(MockErc721Token, 'Transfer')
@@ -265,37 +265,42 @@ describe('NftRolesRegistryVault', () => {
   })
 
   describe('unlockToken', () => {
-    beforeEach(async () => {
-      await depositNftAndGrantRole({ recipient: recipient.address })
+    describe('when NFT is not deposited', () => {
+      it('should revert if token is not deposited', async () => {
+        await depositNftAndGrantRole({ recipient: recipient.address, revocable: false })
+        await expect(
+          NftRolesRegistryVault.connect(owner).unlockToken(role.tokenAddress, role.tokenId),
+        ).to.be.revertedWith('NftRolesRegistryVault: NFT is locked')
+      })
     })
 
-    it('should revert if token is not deposited', async () => {
-      await expect(
-        NftRolesRegistryVault.connect(owner).unlockToken(role.tokenAddress, role.tokenId + 1),
-      ).to.be.revertedWith('NftRolesRegistryVault: NFT is locked')
-    })
+    describe('when NFT is deposited', () => {
+      beforeEach(async () => {
+        await depositNftAndGrantRole({ recipient: recipient.address })
+      })
 
-    it('should revert if sender is not original owner or approved', async () => {
-      await expect(
-        NftRolesRegistryVault.connect(anotherUser).unlockToken(role.tokenAddress, role.tokenId),
-      ).to.be.revertedWith('NftRolesRegistryVault: sender must be owner or approved')
-    })
+      it('should revert if sender is not original owner or approved', async () => {
+        await expect(
+          NftRolesRegistryVault.connect(anotherUser).unlockToken(role.tokenAddress, role.tokenId),
+        ).to.be.revertedWith('NftRolesRegistryVault: sender must be owner or approved')
+      })
 
-    it('should unlock token if sender is owner and NFT is not locked', async () => {
-      await expect(NftRolesRegistryVault.connect(owner).unlockToken(role.tokenAddress, role.tokenId))
-        .to.emit(NftRolesRegistryVault, 'TokenUnlocked')
-        .withArgs(owner.address, role.tokenAddress, role.tokenId)
-        .to.emit(MockErc721Token, 'Transfer')
-        .withArgs(NftRolesRegistryVault.address, owner.address, role.tokenId)
-    })
+      it('should unlock token if sender is owner and NFT is not locked', async () => {
+        await expect(NftRolesRegistryVault.connect(owner).unlockToken(role.tokenAddress, role.tokenId))
+          .to.emit(NftRolesRegistryVault, 'TokenUnlocked')
+          .withArgs(owner.address, role.tokenAddress, role.tokenId)
+          .to.emit(MockErc721Token, 'Transfer')
+          .withArgs(NftRolesRegistryVault.address, owner.address, role.tokenId)
+      })
 
-    it('should unlock token if sender is approved and NFT is not locked', async () => {
-      await NftRolesRegistryVault.connect(owner).setRoleApprovalForAll(role.tokenAddress, anotherUser.address, true)
-      await expect(NftRolesRegistryVault.connect(anotherUser).unlockToken(role.tokenAddress, role.tokenId))
-        .to.emit(NftRolesRegistryVault, 'TokenUnlocked')
-        .withArgs(owner.address, role.tokenAddress, role.tokenId)
-        .to.emit(MockErc721Token, 'Transfer')
-        .withArgs(NftRolesRegistryVault.address, owner.address, role.tokenId)
+      it('should unlock token if sender is approved and NFT is not locked', async () => {
+        await NftRolesRegistryVault.connect(owner).setRoleApprovalForAll(role.tokenAddress, anotherUser.address, true)
+        await expect(NftRolesRegistryVault.connect(anotherUser).unlockToken(role.tokenAddress, role.tokenId))
+          .to.emit(NftRolesRegistryVault, 'TokenUnlocked')
+          .withArgs(owner.address, role.tokenAddress, role.tokenId)
+          .to.emit(MockErc721Token, 'Transfer')
+          .withArgs(NftRolesRegistryVault.address, owner.address, role.tokenId)
+      })
     })
   })