Merge pull request #225 from OriginTrail/fix/identity

Fixed bug in Identity contract, added possibility to register multiple op wallets on profile creation using ProfileV2, added unit tests
OriginTrail · Feb 8, 2024 · 6447e47 · 6447e47
2 parents b9ec511 + b98857d
commit 6447e47
Show file tree

Hide file tree

Showing 13 changed files with 300 additions and 14 deletions.
diff --git a/abi/Identity.json b/abi/Identity.json
@@ -76,6 +76,24 @@
     "stateMutability": "nonpayable",
     "type": "function"
   },
+  {
+    "inputs": [
+      {
+        "internalType": "uint72",
+        "name": "identityId",
+        "type": "uint72"
+      },
+      {
+        "internalType": "address[]",
+        "name": "operationalWallets",
+        "type": "address[]"
+      }
+    ],
+    "name": "addOperationalWallets",
+    "outputs": [],
+    "stateMutability": "nonpayable",
+    "type": "function"
+  },
   {
     "inputs": [
       {

diff --git a/abi/ParametersStorage.json b/abi/ParametersStorage.json
@@ -68,6 +68,19 @@
     "stateMutability": "view",
     "type": "function"
   },
+  {
+    "inputs": [],
+    "name": "hashFunctionsLimit",
+    "outputs": [
+      {
+        "internalType": "uint16",
+        "name": "",
+        "type": "uint16"
+      }
+    ],
+    "stateMutability": "view",
+    "type": "function"
+  },
   {
     "inputs": [],
     "name": "hub",
@@ -146,6 +159,19 @@
     "stateMutability": "pure",
     "type": "function"
   },
+  {
+    "inputs": [],
+    "name": "opWalletsLimitOnProfileCreation",
+    "outputs": [
+      {
+        "internalType": "uint16",
+        "name": "",
+        "type": "uint16"
+      }
+    ],
+    "stateMutability": "view",
+    "type": "function"
+  },
   {
     "inputs": [],
     "name": "proofWindowDurationPerc",
@@ -263,6 +289,19 @@
     "stateMutability": "nonpayable",
     "type": "function"
   },
+  {
+    "inputs": [
+      {
+        "internalType": "uint16",
+        "name": "hashFunctionsLimit_",
+        "type": "uint16"
+      }
+    ],
+    "name": "setHashFunctionsLimit",
+    "outputs": [],
+    "stateMutability": "nonpayable",
+    "type": "function"
+  },
   {
     "inputs": [
       {
@@ -315,6 +354,19 @@
     "stateMutability": "nonpayable",
     "type": "function"
   },
+  {
+    "inputs": [
+      {
+        "internalType": "uint16",
+        "name": "opWalletsLimitOnProfileCreation_",
+        "type": "uint16"
+      }
+    ],
+    "name": "setOpWalletsLimitOnProfileCreation",
+    "outputs": [],
+    "stateMutability": "nonpayable",
+    "type": "function"
+  },
   {
     "inputs": [
       {

diff --git a/abi/ProfileV2.json b/abi/ProfileV2.json
@@ -118,6 +118,22 @@
     "name": "SharesTokenSymbolAlreadyExists",
     "type": "error"
   },
+  {
+    "inputs": [
+      {
+        "internalType": "uint16",
+        "name": "allowed",
+        "type": "uint16"
+      },
+      {
+        "internalType": "uint16",
+        "name": "provided",
+        "type": "uint16"
+      }
+    ],
+    "name": "TooManyOperationalWallets",
+    "type": "error"
+  },
   {
     "inputs": [
       {
@@ -220,6 +236,11 @@
         "name": "adminWallet",
         "type": "address"
       },
+      {
+        "internalType": "address[]",
+        "name": "operationalWallets",
+        "type": "address[]"
+      },
       {
         "internalType": "bytes",
         "name": "nodeId",

diff --git a/contracts/v1/Identity.sol b/contracts/v1/Identity.sol
@@ -14,7 +14,7 @@ contract Identity is Named, Versioned, ContractStatus, Initializable {
     event IdentityDeleted(uint72 indexed identityId);
 
     string private constant _NAME = "Identity";
-    string private constant _VERSION = "1.0.1";
+    string private constant _VERSION = "1.1.0";
 
     IdentityStorage public identityStorage;
 
@@ -76,13 +76,17 @@ contract Identity is Named, Versioned, ContractStatus, Initializable {
 
         IdentityStorage ids = identityStorage;
 
+        if (keyPurpose == OPERATIONAL_KEY) {
+            require(ids.identityIds(key) == 0, "Operational key is taken");
+
+            ids.setOperationalKeyIdentityId(key, identityId);
+        }
+
         bytes32 attachedKey;
         (, , attachedKey) = ids.getKey(identityId, key);
         require(attachedKey != key, "Key is already attached");
 
         ids.addKey(identityId, key, keyPurpose, keyType);
-
-        if (keyPurpose == OPERATIONAL_KEY) ids.setOperationalKeyIdentityId(key, identityId);
     }
 
     function removeKey(uint72 identityId, bytes32 key) external onlyAdmin(identityId) {
@@ -110,6 +114,31 @@ contract Identity is Named, Versioned, ContractStatus, Initializable {
         if (purpose == OPERATIONAL_KEY) ids.removeOperationalKeyIdentityId(key);
     }
 
+    function addOperationalWallets(uint72 identityId, address[] calldata operationalWallets) external onlyContracts {
+        IdentityStorage ids = identityStorage;
+
+        bytes32 operationalKey;
+        bytes32 attachedKey;
+
+        for (uint i; i < operationalWallets.length; ) {
+            operationalKey = keccak256(abi.encodePacked(operationalWallets[i]));
+
+            require(operationalKey != bytes32(0), "Key arg is empty");
+            require(ids.identityIds(operationalKey) == 0, "Operational key is taken");
+
+            ids.setOperationalKeyIdentityId(operationalKey, identityId);
+
+            (, , attachedKey) = ids.getKey(identityId, operationalKey);
+            require(attachedKey != operationalKey, "Key is already attached");
+
+            ids.addKey(identityId, operationalKey, OPERATIONAL_KEY, ECDSA);
+
+            unchecked {
+                i++;
+            }
+        }
+    }
+
     function _checkAdmin(uint72 identityId) internal view virtual {
         require(
             identityStorage.keyHasPurpose(identityId, keccak256(abi.encodePacked(msg.sender)), ADMIN_KEY),

diff --git a/contracts/v1/storage/ParametersStorage.sol b/contracts/v1/storage/ParametersStorage.sol
@@ -38,6 +38,8 @@ contract ParametersStorage is Named, Versioned, HubDependent {
 
     uint16 public updateCommitWindowDuration;
 
+    uint16 public hashFunctionsLimit;
+    uint16 public opWalletsLimitOnProfileCreation;
     uint16 public shardingTableSizeLimit;
 
     constructor(address hubAddress) HubDependent(hubAddress) {
@@ -74,6 +76,8 @@ contract ParametersStorage is Named, Versioned, HubDependent {
 
         updateCommitWindowDuration = 30 minutes;
 
+        hashFunctionsLimit = 20;
+        opWalletsLimitOnProfileCreation = 50;
         shardingTableSizeLimit = 500;
 
         // finalizationCommitsNumber
@@ -230,6 +234,18 @@ contract ParametersStorage is Named, Versioned, HubDependent {
         emit ParameterChanged("updateCommitWindowDuration", newUpdateCommitWindowDuration);
     }
 
+    function setHashFunctionsLimit(uint16 hashFunctionsLimit_) external onlyHubOwner {
+        hashFunctionsLimit = hashFunctionsLimit_;
+
+        emit ParameterChanged("hashFunctionsLimit", hashFunctionsLimit);
+    }
+
+    function setOpWalletsLimitOnProfileCreation(uint16 opWalletsLimitOnProfileCreation_) external onlyHubOwner {
+        opWalletsLimitOnProfileCreation = opWalletsLimitOnProfileCreation_;
+
+        emit ParameterChanged("opWalletsLimitOnProfileCreation", opWalletsLimitOnProfileCreation);
+    }
+
     function setShardingTableSizeLimit(uint16 shardingTableSizeLimit_) external onlyHubOwner {
         shardingTableSizeLimit = shardingTableSizeLimit_;
 

diff --git a/contracts/v2/CommitManagerV1.sol b/contracts/v2/CommitManagerV1.sol
@@ -49,8 +49,6 @@ contract CommitManagerV2 is Named, Versioned, ContractStatus, Initializable {
     ShardingTableStorageV2 public shardingTableStorage;
     StakingStorage public stakingStorage;
 
-    uint256 constant HASH_RING_SIZE = type(uint256).max;
-
     // solhint-disable-next-line no-empty-blocks
     constructor(address hubAddress) ContractStatus(hubAddress) {}
 

diff --git a/contracts/v2/Profile.sol b/contracts/v2/Profile.sol
@@ -82,16 +82,23 @@ contract ProfileV2 is Named, Versioned, ContractStatus, Initializable {
 
     function createProfile(
         address adminWallet,
+        address[] calldata operationalWallets,
         bytes calldata nodeId,
         string calldata sharesTokenName,
         string calldata sharesTokenSymbol,
         uint8 initialOperatorFee
     ) external onlyWhitelisted {
         IdentityStorage ids = identityStorage;
         ProfileStorage ps = profileStorage;
+        Identity id = identityContract;
 
         if (ids.getIdentityId(msg.sender) != 0)
             revert ProfileErrors.IdentityAlreadyExists(ids.getIdentityId(msg.sender), msg.sender);
+        if (operationalWallets.length > parametersStorage.opWalletsLimitOnProfileCreation())
+            revert ProfileErrors.TooManyOperationalWallets(
+                parametersStorage.opWalletsLimitOnProfileCreation(),
+                uint16(operationalWallets.length)
+            );
         if (nodeId.length == 0) revert ProfileErrors.EmptyNodeId();
         if (ps.nodeIdsList(nodeId)) revert ProfileErrors.NodeIdAlreadyExists(nodeId);
         if (keccak256(abi.encodePacked(sharesTokenName)) == keccak256(abi.encodePacked("")))
@@ -101,7 +108,8 @@ contract ProfileV2 is Named, Versioned, ContractStatus, Initializable {
         if (ps.sharesNames(sharesTokenName)) revert ProfileErrors.SharesTokenNameAlreadyExists(sharesTokenName);
         if (ps.sharesSymbols(sharesTokenSymbol)) revert ProfileErrors.SharesTokenSymbolAlreadyExists(sharesTokenSymbol);
 
-        uint72 identityId = identityContract.createIdentity(msg.sender, adminWallet);
+        uint72 identityId = id.createIdentity(msg.sender, adminWallet);
+        id.addOperationalWallets(identityId, operationalWallets);
 
         Shares sharesContract = new Shares(address(hub), sharesTokenName, sharesTokenSymbol);
 
@@ -130,9 +138,9 @@ contract ProfileV2 is Named, Versioned, ContractStatus, Initializable {
         bytes32 nodeAddress;
 
         UnorderedIndexableContractDynamicSetLib.Contract[] memory hashFunctions = hp.getAllHashFunctions();
-        uint256 hashFunctionsNumber = hashFunctions.length;
+        require(hashFunctions.length <= parametersStorage.hashFunctionsLimit(), "Too many hash functions!");
         uint8 hashFunctionId;
-        for (uint8 i; i < hashFunctionsNumber; ) {
+        for (uint8 i; i < hashFunctions.length; ) {
             hashFunctionId = hashFunctions[i].id;
             nodeAddress = hp.callHashFunction(hashFunctionId, nodeId);
             ps.setNodeAddress(identityId, hashFunctionId, nodeAddress);

diff --git a/contracts/v2/constants/HashRingConstants.sol b/contracts/v2/constants/HashRingConstants.sol
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.16;
+
+uint256 constant HASH_RING_SIZE = type(uint256).max;
diff --git a/contracts/v2/errors/ProfileErrors.sol b/contracts/v2/errors/ProfileErrors.sol
@@ -4,6 +4,7 @@ pragma solidity ^0.8.16;
 
 library ProfileErrors {
     error IdentityAlreadyExists(uint72 identityId, address wallet);
+    error TooManyOperationalWallets(uint16 allowed, uint16 provided);
     error EmptyNodeId();
     error NodeIdAlreadyExists(bytes nodeId);
     error EmptySharesTokenName();

diff --git a/contracts/v2/scoring/LinearSum.sol b/contracts/v2/scoring/LinearSum.sol
@@ -10,15 +10,14 @@ import {Initializable} from "../../v1/interface/Initializable.sol";
 import {IProximityScoreFunctionsPair} from "../interface/IProximityScoreFunctionsPair.sol";
 import {Named} from "../../v1/interface/Named.sol";
 import {ScaleDownLib} from "../utils/ScaleDownLibrary.sol";
+import {HASH_RING_SIZE} from "../constants/HashRingConstants.sol";
 
 contract LinearSum is IProximityScoreFunctionsPair, Indexable, Named, HubDependent, Initializable {
     event ParameterChanged(string parameterName, uint256 parameterValue);
 
     uint8 private constant _ID = 2;
     string private constant _NAME = "LinearSum";
 
-    uint256 constant HASH_RING_SIZE = type(uint256).max;
-
     HashingProxy public hashingProxy;
     ParametersStorage public parametersStorage;
 

diff --git a/contracts/v2/storage/NodeOperatorFeeChangesStorage.sol b/contracts/v2/storage/NodeOperatorFeeChangesStorage.sol
@@ -25,7 +25,7 @@ contract NodeOperatorFeeChangesStorage is Named, Versioned, HubDependent {
     constructor(address hubAddress) HubDependent(hubAddress) {}
 
     modifier onlyOnce() {
-        require(!_delayFreePeriodSet, "Function has already been executed.");
+        require(!_delayFreePeriodSet, "Fn has already been executed");
         _;
         _delayFreePeriodSet = true;
     }

diff --git a/test/v1/unit/Identity.test.ts b/test/v1/unit/Identity.test.ts
@@ -66,8 +66,8 @@ describe('@v1 @unit Identity contract', function () {
     expect(await Identity.name()).to.equal('Identity');
   });
 
-  it('The contract is version "1.0.1"', async () => {
-    expect(await Identity.version()).to.equal('1.0.1');
+  it('The contract is version "1.1.0"', async () => {
+    expect(await Identity.version()).to.equal('1.1.0');
   });
 
   it('Create an identity as a contract, expect to pass', async () => {
@@ -176,7 +176,7 @@ describe('@v1 @unit Identity contract', function () {
     expect(adminKeys.length).to.equal(2, 'Error: Failed to add operational key to identity!');
     await expect(
       AddKeyWithAdminWallet.addKey(getIdentityId, newOperationalKey, OPERATIONAL_KEY, ECDSA),
-    ).to.be.revertedWith('Key is already attached');
+    ).to.be.revertedWith('Operational key is taken');
   });
 
   it('Add an admin key to existing identity with operational wallet, expect to fail', async () => {
@@ -279,4 +279,39 @@ describe('@v1 @unit Identity contract', function () {
 
     expect(await IdentityStorage.keyHasPurpose(getIdentityId, adminKeyBytes32, ADMIN_KEY)).to.be.false;
   });
+
+  it('Create 2 identities, try to attach operational key of the other identity, expect to revert', async () => {
+    const identityId = await createIdentity(operationalKey, adminKey);
+    await createIdentity(accounts[3].address, accounts[4].address);
+
+    await expect(
+      Identity.connect(accounts[2]).addKey(
+        identityId,
+        ethers.utils.keccak256(ethers.utils.solidityPack(['address'], [accounts[3].address])),
+        OPERATIONAL_KEY,
+        ECDSA,
+      ),
+    ).to.be.revertedWith('Operational key is taken');
+  });
+
+  it('Create identity, try to attach multiple operational wallets with already existing key, expect to revert', async () => {
+    const identityId = await createIdentity(operationalKey, adminKey);
+
+    await expect(
+      Identity.addOperationalWallets(identityId, [accounts[1].address, accounts[3].address]),
+    ).to.be.revertedWith('Operational key is taken');
+  });
+
+  it('Create identity, try to attach multiple operational wallets with already taken key, expect to revert', async () => {
+    const identityId = await createIdentity(operationalKey, adminKey);
+    await createIdentity(accounts[3].address, accounts[4].address);
+
+    await expect(
+      Identity.addOperationalWallets(identityId, [accounts[3].address, accounts[5].address]),
+    ).to.be.revertedWith('Operational key is taken');
+
+    // We still can attach someones admin wallet as a key, but it shouldn't be a problem
+    await expect(Identity.addOperationalWallets(identityId, [accounts[4].address, accounts[5].address])).not.to.be
+      .reverted;
+  });
 });