dappnode · dapplion · Feb 4, 2022 · Feb 4, 2022
diff --git a/contracts/Registry.sol b/contracts/Registry.sol
@@ -1,8 +1,9 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.0;
 
-import "@openzeppelin/contracts/access/AccessControlEnumerable.sol";
-import "@openzeppelin/contracts/proxy/Clones.sol";
+import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
+import "@openzeppelin/contracts-upgradeable/access/AccessControlEnumerableUpgradeable.sol";
+import "@openzeppelin/contracts-upgradeable/proxy/ClonesUpgradeable.sol";
 import "./Repo.sol";
 
 /**
@@ -11,7 +12,7 @@ import "./Repo.sol";
  * - assign package statuses: visible, active, validated, banned, etc
  * - basic priorization between packages in the registry in the form of a sorted non-exhaustive list
  */
-contract Registry is AccessControlEnumerable {
+contract Registry is Initializable, AccessControlEnumerableUpgradeable {
   bytes32 public constant ADD_PACKAGE_ROLE = keccak256("ADD_PACKAGE_ROLE");
   bytes32 public constant SET_STATUS_ROLE = keccak256("SET_STATUS_ROLE");
   // This role should be used only for extreme cirumstances since it breaks the immutability of packages.
@@ -46,7 +47,7 @@ contract Registry is AccessControlEnumerable {
    * @dev Name to identify this registry, i.e. 'dnp.dappnode.eth'
    */
   string public registryName;
-  address public immutable repoImplementation;
+  address public repoImplementation;
 
   uint64 internal nextIdx;
   /**
@@ -90,7 +91,9 @@ contract Registry is AccessControlEnumerable {
   /**
    * @param _registryName Name to identify this registry, i.e. 'dnp.dappnode.eth'
    */
-  constructor(string memory _registryName) {
+  function initialize(string memory _registryName) public initializer {
+    __AccessControlEnumerable_init();
+
     registryName = _registryName;
     nextIdx = 1;
     bytesPerListItem = 1;
@@ -101,6 +104,7 @@ contract Registry is AccessControlEnumerable {
     _setupRole(ADD_PACKAGE_ROLE, msg.sender);
     _setupRole(SET_STATUS_ROLE, msg.sender);
     _setupRole(SET_LIST_ROLE, msg.sender);
+    _setupRole(SET_REPO_ROLE, msg.sender);
   }
 
   /**
@@ -114,7 +118,7 @@ contract Registry is AccessControlEnumerable {
     address _dev,
     uint64 _flags
   ) external onlyAddPackageRole returns (Repo) {
-    Repo repo = Repo(Clones.clone(repoImplementation));
+    Repo repo = Repo(ClonesUpgradeable.clone(repoImplementation));
 
     repo.initialize(_dev);
 
@@ -135,7 +139,7 @@ contract Registry is AccessControlEnumerable {
     string memory _version,
     string memory _contentURI
   ) external onlyAddPackageRole returns (Repo) {
-    Repo repo = Repo(Clones.clone(repoImplementation));
+    Repo repo = Repo(ClonesUpgradeable.clone(repoImplementation));
 
     // Registry must have permissions to create the first version
     repo.initialize(address(this));
@@ -189,7 +193,7 @@ contract Registry is AccessControlEnumerable {
 
   /**
    * @notice Change package repo address.
-   * Should only be used in extreme circustances to recover a useful name.
+   * Should only be used in extreme circumstances to recover a useful name.
    */
   function setPackageRepo(uint256 packageIdx, address repo) external onlyRole(SET_REPO_ROLE) {
     Package storage package = packages[packageIdx];

diff --git a/contracts/Repo.sol b/contracts/Repo.sol
@@ -31,6 +31,8 @@ contract Repo is Initializable, AccessControlEnumerableUpgradeable {
    * @notice Initialize this Repo
    */
   function initialize(address _admin) public initializer {
+    __AccessControlEnumerable_init();
+
     nextIdx = 1;
 
     _setupRole(DEFAULT_ADMIN_ROLE, _admin);

diff --git a/contracts/mocks/RegistryV2.sol b/contracts/mocks/RegistryV2.sol
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import "../Registry.sol";
+
+/**
+ * This contract is only used for a upgradability test
+ */
+contract RegistryV2Mock is Registry {
+    // upgradability test
+    uint256 public version;
+
+    // upgradability test
+    function setVersion() public {
+        version = 2;
+    }
+
+    function getVersion() external view returns (uint256) {
+        return version;
+    }
+
+}
diff --git a/hardhat.config.ts b/hardhat.config.ts
@@ -8,6 +8,7 @@ import "@nomiclabs/hardhat-ethers";
 import "@typechain/hardhat";
 import "hardhat-gas-reporter";
 import "solidity-coverage";
+import "@openzeppelin/hardhat-upgrades";
 
 // This is a sample Hardhat task. To learn how to create your own go to
 // https://hardhat.org/guides/create-task.html
@@ -26,7 +27,15 @@ task("accounts", "Prints the list of accounts", async (taskArgs, hre) => {
  * @type import('hardhat/config').HardhatUserConfig
  */
 export default {
-  solidity: "0.8.4",
+  solidity: {
+    version: "0.8.4",
+    settings: {
+      optimizer: {
+        enabled: true,
+        runs: 999999
+      }
+    }
+  },
   networks: {
     hardhat: {
       initialBaseFeePerGas: 0, // workaround from https://github.com/sc-forks/solidity-coverage/issues/652#issuecomment-896330136 . Remove when that issue is closed.

diff --git a/package.json b/package.json
@@ -2,14 +2,15 @@
   "name": "dappnode-package-manager",
   "scripts": {
     "build": "npx hardhat compile",
-    "test": "npx hardhat test",
+    "test": "npx hardhat compile && npx hardhat test",
     "deploy:registry:hardhat": "npx hardhat run scripts/deploy-registry.ts --network hardhat",
     "deploy:registry:xDai": "npx hardhat run scripts/deploy-registry.ts --network xDAI"
   },
   "devDependencies": {
     "@nomiclabs/hardhat-ethers": "^2.0.2",
     "@nomiclabs/hardhat-etherscan": "^2.1.6",
     "@nomiclabs/hardhat-waffle": "^2.0.1",
+    "@openzeppelin/hardhat-upgrades": "^1.14.0",
     "@typechain/ethers-v5": "^8.0.5",
     "@typechain/hardhat": "^3.0.0",
     "@types/chai": "^4.3.0",

diff --git a/scripts/deploy-registry.ts b/scripts/deploy-registry.ts
@@ -1,4 +1,4 @@
-import {ethers} from "hardhat";
+import {ethers,upgrades} from "hardhat";
 import {Registry} from "../typechain-types/Registry";
 
 async function main() {
@@ -11,7 +11,8 @@ async function main() {
   console.log("registryName:", registryName);
 
   const Registry = await ethers.getContractFactory("Registry");
-  const registry = (await Registry.deploy(registryName)) as Registry;
+
+  const registry = (await upgrades.deployProxy(Registry, [registryName])) as Registry;
   await registry.deployed();
 
   console.log("#######################\n");

diff --git a/scripts/migrate-from-mainnet.ts b/scripts/migrate-from-mainnet.ts
@@ -1,5 +1,5 @@
 import {Interface} from "ethers/lib/utils";
-import {ethers} from "hardhat";
+import {ethers,upgrades} from "hardhat";
 import fs from "fs";
 import {Registry} from "../typechain-types/Registry";
 import * as registryData from "./apm/registryABI";
@@ -85,7 +85,7 @@ async function main() {
 
   const Registry = await ethers.getContractFactory("Registry");
 
-  const registry = (await Registry.deploy(xDAIRegistryName)) as Registry;
+  const registry = (await upgrades.deployProxy(Registry, [xDAIRegistryName])) as Registry;
   await registry.deployed();
 
   console.log("Dappnode Registry Contract deployed to:", registry.address);

diff --git a/test/registry.ts b/test/registry.ts
@@ -1,13 +1,14 @@
 import {expect} from "chai";
-import {ethers} from "hardhat";
-import {Event} from "ethers";
+import {ethers, upgrades} from "hardhat";
+import {BigNumber, Event} from "ethers";
 import {Registry, PackageStruct} from "../typechain-types/Registry";
 import {Repo, VersionStruct} from "../typechain-types/Repo";
+import {RegistryV2Mock} from "../typechain-types/RegistryV2Mock";
 
 interface RepoPackage {
   name: string;
   dev: string;
-  flags: number;
+  flags: BigNumber;
 }
 
 describe("Registry", function () {
@@ -29,12 +30,12 @@ describe("Registry", function () {
     const newPackage: RepoPackage = {
       name: "gnosis",
       dev: addr1.address,
-      flags: 0,
+      flags: ethers.BigNumber.from(0),
     };
 
     const Registry = await ethers.getContractFactory("Registry");
-    const registry = (await Registry.deploy(registryName)) as Registry;
 
+    const registry = (await upgrades.deployProxy(Registry, [registryName])) as Registry;
     await registry.deployed();
 
     expect(await registry.registryName()).to.equal(registryName, "Wrong registryName");
@@ -91,19 +92,15 @@ describe("Registry", function () {
       contentURI: "/ipfs/Qmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
     };
 
-    const newVersion2 = {
-      version: "0.2.0-beta.0",
-      contentURI: "/ipfs/Qmbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
-    };
-
     const newPackage = {
       name: "gnosis-test",
       dev: addr1.address,
-      flags: 0,
+      flags: ethers.BigNumber.from(0),
     };
 
     const Registry = await ethers.getContractFactory("Registry");
-    const registryAdmin = (await Registry.deploy(registryName)) as Registry;
+
+    const registryAdmin = (await upgrades.deployProxy(Registry, [registryName])) as Registry;
     await registryAdmin.deployed();
 
     expect(await registryAdmin.registryName()).to.equal(registryName, "Wrong registryName");
@@ -130,6 +127,82 @@ describe("Registry", function () {
     await assertRepoVersions(repoUser, [newVersion1]);
   });
 
+  it("public.dappnode replace a malicious repo using setPackageRepo", async function () {
+    const [owner, addr1, dev] = await ethers.getSigners();
+
+    const registryName = "dnp.dappnode";
+
+    const badVersion: VersionStruct = {
+      version: "0.1.0",
+      contentURI: "/ipfs/notcorrectversion",
+    };
+
+    const badPackage: RepoPackage = {
+      name: "gnosis",
+      dev: addr1.address,
+      flags: ethers.BigNumber.from(0),
+    };
+
+    const correctPackage: RepoPackage = {
+      name: "gnosis",
+      dev: dev.address,
+      flags: ethers.BigNumber.from(0),
+    };
+
+    const correctVersion: VersionStruct = {
+      version: "0.1.0",
+      contentURI: "/ipfs/Qmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+    };
+
+    const Registry = await ethers.getContractFactory("Registry");
+
+    const registry = (await upgrades.deployProxy(Registry, [registryName])) as Registry;
+    await registry.deployed();
+
+    expect(await registry.registryName()).to.equal(registryName, "Wrong registryName");
+
+    // Publish new repo from admin account
+    const {repo: repoAddress} = await publishRepoVersion(registry, badPackage, badVersion);
+
+    // Assert registry packages
+    await assertPackages(registry, [{flags: badPackage.flags, repo: repoAddress, name: badPackage.name}])
+
+    // Replace the bad package with a new one
+    const Repo = await ethers.getContractFactory("Repo");
+    const newRepoAddress = (await upgrades.deployProxy(Repo, [dev.address],  {
+      unsafeAllow: ["constructor"],
+    })) as Repo;
+
+
+    const repoWithDev = (await ethers.getContractAt("Repo", newRepoAddress.address, dev)) as Repo;
+
+    // Publish a version on the new repo
+    const newVersionTx = await repoWithDev.newVersion(correctVersion.version, correctVersion.contentURI);
+    const newVersionReceipt = await newVersionTx.wait();
+
+    const newVersionEvent = getEvent(newVersionReceipt.events, "NewVersion");
+    expect(newVersionEvent.args!.version).to.equal(correctVersion.version, "Wrong event NewVersion.version");
+    expect(newVersionEvent.args!.contentURI).to.equal(correctVersion.contentURI, "Wrong event NewVersion.contentURI");
+
+    // Assert that there are one version in the Repo contract
+    await assertRepoVersions(repoWithDev, [correctVersion]);
+
+    // Check that the malicious package is the idx 1
+    const packageIdx = 1;
+    expect(await registry.getPackageIdx(badPackage.name)).to.be.equal(packageIdx);
+
+    // Overwrite repo address
+    const registryUser = (await ethers.getContractAt("Registry", registry.address, addr1)) as Registry;
+    expect(registryUser.setPackageRepo(packageIdx, newRepoAddress.address)).to.be.revertedWith(
+      "AccessControl: account 0x70997970c51812dc3a010c7d01b50e0d17dc79c8 is missing role 0x16bd2aca01d0d7886c05a93638707d130beb22ebb67403e39bc35ee20a0de336"
+    );
+
+    await registry.setPackageRepo(packageIdx, newRepoAddress.address);
+
+    // Assert registry packages
+    await assertPackages(registry, [{flags: correctPackage.flags, repo: newRepoAddress.address, name: correctPackage.name}])
+  });
+
   it("public.dappnode registry publish one package and set flags", async function () {
     const [owner, addr1] = await ethers.getSigners();
 
@@ -143,12 +216,12 @@ describe("Registry", function () {
     const newPackage: RepoPackage = {
       name: "gnosis",
       dev: addr1.address,
-      flags: 0,
+      flags: ethers.BigNumber.from(0),
     };
 
     const Registry = await ethers.getContractFactory("Registry");
-    const registry = (await Registry.deploy(registryName)) as Registry;
-
+
+    const registry = (await upgrades.deployProxy(Registry, [registryName])) as Registry;
     await registry.deployed();
 
     expect(await registry.registryName()).to.equal(registryName, "Wrong registryName");
@@ -194,10 +267,38 @@ describe("Registry", function () {
 
     // Assert registry packages
     await assertPackages(registry, [{flags: bannedFlag, repo: newRepoAddress, name: newPackage.name}])
+  });
+
+  it("public.dappnode registry upgradability test", async function () {
+    const registryName = "dnp.dappnode";
+
+    const Registry = await ethers.getContractFactory("Registry");
+
+    const registry = (await upgrades.deployProxy(Registry, [registryName])) as Registry;
+    await registry.deployed();
+
+    expect(await registry.registryName()).to.equal(registryName, "Wrong registryName");
 
-    // Package should have been removed from the packageIdxByName mapping
-    await expect(registry.getPackageIdx(newPackage.name)).to.be.revertedWith("REGISTRY_INEXISTENT_NAME");
-    expect(await registry.packageIdxByName(nameHash)).to.be.equal(0);
+    // Prepare upgrade
+    const RegistryV2 = await ethers.getContractFactory("RegistryV2Mock");
+    const registryV2 = RegistryV2.attach(registry.address) as RegistryV2Mock;
+
+    // Check that the contract is not yet upgraded
+    // For some reason the expect to be reverted does not work when the function selector does not exist
+    try {
+      await registryV2.setVersion();
+      throw new Error("Unreachable code");
+    } catch(error: unknown) {
+      const { message } = error as Error;
+      expect(message).to.be.equal("Transaction reverted: function selector was not recognized and there's no fallback function");
+    }
+
+    // Upgrade the contract
+    await upgrades.upgradeProxy(registry.address, RegistryV2);
+
+    // Check upgrade
+    await registryV2.setVersion();
+    expect(await registryV2.getVersion()).to.be.equal(2);
   });
 });
 
@@ -274,7 +375,7 @@ function getEvent(events: Event[] = [], eventName: string): Event {
   return event;
 }
 
-function calculateFlagValue(visible: Boolean, active: Boolean,validated: Boolean, banned: Boolean): number {
+function calculateFlagValue(visible: Boolean, active: Boolean,validated: Boolean, banned: Boolean): BigNumber {
   const value = Number(visible) + Number(active)*2 + Number(validated)*4 + Number(banned)*8;
-  return value;
+  return ethers.BigNumber.from(value);
 }