Merge pull request #718 from clrfund/fix/recipientId

Make recipient id unique for different registries

contracts/contracts/recipientRegistry/BaseRecipientRegistry.sol

@@ -146,4 +146,19 @@ abstract contract BaseRecipientRegistry is IRecipientRegistry {
   function getRecipientCount() public view returns(uint256) {
       return slots.length - removed.length;
   }
+
+  /**
+   * @dev Make a unique recipient id for different registries
+   * @param _registry Recipient registry address
+   * @param _recipient Recipient address
+   * @param _metadata Recipient metadata
+   * @return recipient id
+   */
+   function makeRecipientId(address _registry, address _recipient, string calldata _metadata)
+    internal
+    pure
+    returns(bytes32)
+  {
+    return keccak256(abi.encodePacked(_registry, _recipient, _metadata));
+  }
 }

contracts/contracts/recipientRegistry/OptimisticRecipientRegistry.sol

@@ -97,7 +97,7 @@ contract OptimisticRecipientRegistry is Ownable, BaseRecipientRegistry {
   {
     require(_recipient != address(0), 'RecipientRegistry: Recipient address is zero');
     require(bytes(_metadata).length != 0, 'RecipientRegistry: Metadata info is empty string');
-    bytes32 recipientId = keccak256(abi.encodePacked(_recipient, _metadata));
+    bytes32 recipientId = makeRecipientId(address(this), _recipient, _metadata);
     require(recipients[recipientId].index == 0, 'RecipientRegistry: Recipient already registered');
     require(requests[recipientId].submissionTime == 0, 'RecipientRegistry: Request already submitted');
     require(msg.value == baseDeposit, 'RecipientRegistry: Incorrect deposit amount');

contracts/contracts/recipientRegistry/PermissionedRecipientRegistry.sol

@@ -97,7 +97,7 @@ contract PermissionedRecipientRegistry is Ownable, BaseRecipientRegistry {
   {
     require(_recipient != address(0), 'RecipientRegistry: Recipient address is zero');
     require(bytes(_metadata).length != 0, 'RecipientRegistry: Metadata info is empty string');
-    bytes32 recipientId = keccak256(abi.encodePacked(_recipient, _metadata));
+    bytes32 recipientId = makeRecipientId(address(this), _recipient, _metadata);
     require(recipients[recipientId].index == 0, 'RecipientRegistry: Recipient already registered');
     require(requests[recipientId].submissionTime == 0, 'RecipientRegistry: Request already submitted');
     require(msg.value == baseDeposit, 'RecipientRegistry: Incorrect deposit amount');

contracts/contracts/recipientRegistry/SimpleRecipientRegistry.sol

@@ -47,7 +47,7 @@ contract SimpleRecipientRegistry is Ownable, BaseRecipientRegistry {
   {
     require(_recipient != address(0), 'RecipientRegistry: Recipient address is zero');
     require(bytes(_metadata).length != 0, 'RecipientRegistry: Metadata info is empty string');
-    bytes32 recipientId = keccak256(abi.encodePacked(_recipient, _metadata));
+    bytes32 recipientId = makeRecipientId(address(this), _recipient, _metadata);
     uint256 recipientIndex = _addRecipient(recipientId, _recipient);
     emit RecipientAdded(recipientId, _recipient, _metadata, recipientIndex, block.timestamp);
   }

contracts/tests/recipientRegistry.ts

@@ -6,7 +6,7 @@ import { keccak256 } from '@ethersproject/solidity'
 import { gtcrEncode } from '@kleros/gtcr-encoder'
 
 import { UNIT, ZERO_ADDRESS } from '../utils/constants'
-import { getTxFee } from '../utils/contracts'
+import { getTxFee, getEventArg } from '../utils/contracts'
 import { deployContract } from '../utils/deployment'
 
 use(solidity)
@@ -18,6 +18,17 @@ async function getCurrentTime(): Promise<number> {
   return (await provider.getBlock('latest')).timestamp
 }
 
+function getRecipientId(
+  registryAddress: string,
+  address: string,
+  metadata: string
+): string {
+  return keccak256(
+    ['address', 'address', 'string'],
+    [registryAddress, address, metadata]
+  )
+}
+
 describe('Simple Recipient Registry', () => {
   const [, deployer, controller, recipient] = provider.getWallets()
 
@@ -69,10 +80,6 @@ describe('Simple Recipient Registry', () => {
     let metadata: string
     let recipientId: string
 
-    function getRecipientId(address: string, metadata: string): string {
-      return keccak256(['address', 'string'], [address, metadata])
-    }
-
     beforeEach(async () => {
       await registry.connect(controller).setMaxRecipients(MAX_RECIPIENTS)
       recipientAddress = recipient.address
@@ -81,7 +88,7 @@ describe('Simple Recipient Registry', () => {
         description: 'Description',
         imageHash: 'Ipfs imageHash',
       })
-      recipientId = getRecipientId(recipientAddress, metadata)
+      recipientId = getRecipientId(registry.address, recipientAddress, metadata)
     })
 
     it('allows owner to add recipient', async () => {
@@ -110,6 +117,7 @@ describe('Simple Recipient Registry', () => {
       const anotherRecipientAddress =
         '0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045'
       const anotherRecipientId = getRecipientId(
+        registry.address,
         anotherRecipientAddress,
         metadata
       )
@@ -256,9 +264,17 @@ describe('Simple Recipient Registry', () => {
 
       // Replace recipients
       const removedRecipient1 = '0x0000000000000000000000000000000000000001'
-      const removedRecipient1Id = getRecipientId(removedRecipient1, metadata)
+      const removedRecipient1Id = getRecipientId(
+        registry.address,
+        removedRecipient1,
+        metadata
+      )
       const removedRecipient2 = '0x0000000000000000000000000000000000000002'
-      const removedRecipient2Id = getRecipientId(removedRecipient2, metadata)
+      const removedRecipient2Id = getRecipientId(
+        registry.address,
+        removedRecipient2,
+        metadata
+      )
       await registry.removeRecipient(removedRecipient1Id)
       await registry.removeRecipient(removedRecipient2Id)
       const addedRecipient1 = '0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045'
@@ -571,10 +587,6 @@ describe('Optimistic recipient registry', () => {
     let metadata: string
     let recipientId: string
 
-    function getRecipientId(address: string, metadata: string): string {
-      return keccak256(['address', 'string'], [address, metadata])
-    }
-
     beforeEach(async () => {
       await registry.connect(controller).setMaxRecipients(MAX_RECIPIENTS)
       recipientAddress = recipient.address
@@ -583,7 +595,7 @@ describe('Optimistic recipient registry', () => {
         description: 'Description',
         imageHash: 'Ipfs imageHash',
       })
-      recipientId = getRecipientId(recipientAddress, metadata)
+      recipientId = getRecipientId(registry.address, recipientAddress, metadata)
     })
 
     it('allows anyone to submit registration request', async () => {
@@ -808,7 +820,11 @@ describe('Optimistic recipient registry', () => {
           imageHash: 'Ipfs imageHash',
         })
         recipientAddress = `0x000000000000000000000000000000000000${recipientName}`
-        recipientId = getRecipientId(recipientAddress, metadata)
+        recipientId = getRecipientId(
+          registry.address,
+          recipientAddress,
+          metadata
+        )
         if (i < MAX_RECIPIENTS) {
           await registry.addRecipient(recipientAddress, metadata, {
             value: baseDeposit,
@@ -953,5 +969,40 @@ describe('Optimistic recipient registry', () => {
         )
       ).to.equal(ZERO_ADDRESS)
     })
+
+    it('creates different recipient id for different recipient registries', async () => {
+      const txOne = await registry.addRecipient(recipientAddress, metadata, {
+        value: baseDeposit,
+      })
+      const idOne = await getEventArg(
+        txOne,
+        registry,
+        'RequestSubmitted',
+        '_recipientId'
+      )
+
+      const anotherRegistry = await deployContract(
+        deployer,
+        'OptimisticRecipientRegistry',
+        [baseDeposit, challengePeriodDuration, controller.address]
+      )
+      const txTwo = await anotherRegistry.addRecipient(
+        recipientAddress,
+        metadata,
+        {
+          value: baseDeposit,
+        }
+      )
+      const idTwo = await getEventArg(
+        txTwo,
+        anotherRegistry,
+        'RequestSubmitted',
+        '_recipientId'
+      )
+
+      expect(idOne.length).to.be.gt(0)
+      expect(idTwo.length).to.be.gt(0)
+      expect(idOne).to.not.eq(idTwo)
+    })
   })
 })