add transfer owner to admin facet

sarcophagus-org · Oct 19, 2023 · 9b8ff62 · 9b8ff62
1 parent 271bc6f
commit 9b8ff62
Show file tree

Hide file tree

Showing 9 changed files with 121 additions and 46 deletions.
diff --git a/contracts/facets/AdminFacet.sol b/contracts/facets/AdminFacet.sol
@@ -20,15 +20,16 @@ contract AdminFacet {
     error CannotSetZeroValue();
 
     /// @notice Caller must be the admin address
-    error CallerIsNotAdmin();
+    error CallerIsNotAdminOrOwner();
 
     /// @notice Provided address cannot be zero address
     error ZeroAddress();
 
+    /// @notice Modifier to enforce caller is admin or contract owner
     modifier onlyAdmin {
         AppStorage storage s = LibAppStorage.getAppStorage();
-        if (msg.sender != s.admin) {
-            revert CallerIsNotAdmin();
+        if (msg.sender != s.admin && msg.sender != LibDiamond.contractOwner()) {
+            revert CallerIsNotAdminOrOwner();
         }
         _;
     }
@@ -105,4 +106,16 @@ contract AdminFacet {
         s.admin = newAdmin;
         emit AdminTransferred(msg.sender, newAdmin);
     }
+
+    /// @notice Transfers diamond owner to new owner.
+    /// @param newOwner to set
+    function transferDiamondOwner(address newOwner) external {
+        LibDiamond.enforceIsContractOwner();
+        LibDiamond.setContractOwner(newOwner);
+    }
+
+    /// @notice Returns current owner of Diamond contract.
+    function getDiamondOwner() external view returns (address) {
+        return LibDiamond.contractOwner();
+    }
 }
diff --git a/test/facets/admin/setCursedBondPercentage.spec.ts b/test/facets/admin/setCursedBondPercentage.spec.ts
@@ -10,28 +10,41 @@ import {
 import { BigNumber } from "ethers";
 import time from "../../utils/time";
 import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
-import { getArchaeologistFreeBondSarquitos } from "../helpers/bond";
 
 const { deployments, ethers } = require("hardhat");
 
 describe("AdminFacet.setCursedBondPercentage", () => {
   let deployer: SignerWithAddress;
 
-  context("when caller is not the admin", async () => {
+  context("after deployment", async () => {
     beforeEach(async () => {
       await deployments.fixture();
-      const { adminFacet } = await getContracts();
+    });
+
+    it("defaults the admin and owner to the deployer", async () => {
+      const { adminFacet, viewStateFacet } = await getContracts();
       deployer = await ethers.getNamedSigner("deployer");
-      const signers = await ethers.getSigners();
-      await adminFacet.connect(deployer).transferAdmin(signers[1].address);
+      const adminAddress = await viewStateFacet.connect(deployer).getAdmin();
+      const diamondOwnerAddress = await adminFacet
+        .connect(deployer)
+        .getDiamondOwner();
+      expect(deployer.address).to.eq(adminAddress);
+      expect(deployer.address).to.eq(diamondOwnerAddress);
+    });
+  });
+
+  context("when caller is not the admin", async () => {
+    beforeEach(async () => {
+      await deployments.fixture();
     });
 
     it("reverts with the correct error message", async () => {
       const { adminFacet } = await getContracts();
-      const setTx = adminFacet.connect(deployer).setCursedBondPercentage(200);
+      const signers = await ethers.getSigners();
+      const setTx = adminFacet.connect(signers[1]).setCursedBondPercentage(200);
       await expect(setTx).to.be.revertedWithCustomError(
         adminFacet,
-        "CallerIsNotAdmin"
+        "CallerIsNotAdminOrOwner"
       );
     });
   });

diff --git a/test/facets/admin/setEmbalmerClaimWindow.spec.ts b/test/facets/admin/setEmbalmerClaimWindow.spec.ts
@@ -11,18 +11,15 @@ describe("AdminFacet.setEmbalmerClaimWindow", () => {
   context("when caller is not the admin", async () => {
     beforeEach(async () => {
       await deployments.fixture();
-      const { adminFacet } = await getContracts();
-      deployer = await ethers.getNamedSigner("deployer");
-      const signers = await ethers.getSigners();
-      await adminFacet.connect(deployer).transferAdmin(signers[1].address);
     });
 
     it("reverts with the correct error message", async () => {
       const { adminFacet } = await getContracts();
-      const setTx = adminFacet.connect(deployer).setEmbalmerClaimWindow(200);
+      const signers = await ethers.getSigners();
+      const setTx = adminFacet.connect(signers[1]).setEmbalmerClaimWindow(200);
       await expect(setTx).to.be.revertedWithCustomError(
         adminFacet,
-        "CallerIsNotAdmin"
+        "CallerIsNotAdminOrOwner"
       );
     });
   });

diff --git a/test/facets/admin/setExpirationThreshold.spec.ts b/test/facets/admin/setExpirationThreshold.spec.ts
@@ -11,19 +11,15 @@ describe("AdminFacet.setExpirationThreshold", () => {
   context("when caller is not the admin", async () => {
     beforeEach(async () => {
       await deployments.fixture();
-      const { adminFacet } = await getContracts();
-      deployer = await ethers.getNamedSigner("deployer");
-      const signers = await ethers.getSigners();
-      await adminFacet.connect(deployer).transferAdmin(signers[1].address);
     });
 
     it("reverts with the correct error message", async () => {
       const { adminFacet } = await getContracts();
-      const deployer = await ethers.getNamedSigner("deployer");
-      const setTx = adminFacet.connect(deployer).setExpirationThreshold(200);
+      const signers = await ethers.getSigners();
+      const setTx = adminFacet.connect(signers[1]).setExpirationThreshold(200);
       await expect(setTx).to.be.revertedWithCustomError(
         adminFacet,
-        "CallerIsNotAdmin"
+        "CallerIsNotAdminOrOwner"
       );
     });
   });

diff --git a/test/facets/admin/setGracePeriod.spec.ts b/test/facets/admin/setGracePeriod.spec.ts
@@ -11,19 +11,15 @@ describe("AdminFacet.setGracePeriod", () => {
   context("when caller is not the admin", async () => {
     beforeEach(async () => {
       await deployments.fixture();
-      const { adminFacet } = await getContracts();
-      deployer = await ethers.getNamedSigner("deployer");
-      const signers = await ethers.getSigners();
-      await adminFacet.connect(deployer).transferAdmin(signers[1].address);
     });
 
     it("reverts with the correct error message", async () => {
       const { adminFacet } = await getContracts();
-      const deployer = await ethers.getNamedSigner("deployer");
-      const setTx = adminFacet.connect(deployer).setGracePeriod(200);
+      const signers = await ethers.getSigners();
+      const setTx = adminFacet.connect(signers[1]).setGracePeriod(200);
       await expect(setTx).to.be.revertedWithCustomError(
         adminFacet,
-        "CallerIsNotAdmin"
+        "CallerIsNotAdminOrOwner"
       );
     });
   });

diff --git a/test/facets/admin/setProtocolFeeBasePercentage.spec.ts b/test/facets/admin/setProtocolFeeBasePercentage.spec.ts
@@ -11,21 +11,17 @@ describe("AdminFacet.setProtocolFeeBasePercentage", () => {
   context("when caller is not the admin", async () => {
     beforeEach(async () => {
       await deployments.fixture();
-      const { adminFacet } = await getContracts();
-      deployer = await ethers.getNamedSigner("deployer");
-      const signers = await ethers.getSigners();
-      await adminFacet.connect(deployer).transferAdmin(signers[1].address);
     });
 
     it("reverts with the correct error message", async () => {
       const { adminFacet } = await getContracts();
-      const deployer = await ethers.getNamedSigner("deployer");
+      const signers = await ethers.getSigners();
       const setTx = adminFacet
-        .connect(deployer)
+        .connect(signers[1])
         .setProtocolFeeBasePercentage(200);
       await expect(setTx).to.be.revertedWithCustomError(
         adminFacet,
-        "CallerIsNotAdmin"
+        "CallerIsNotAdminOrOwner"
       );
     });
   });

diff --git a/test/facets/admin/transferAdmin.spec.ts b/test/facets/admin/transferAdmin.spec.ts
@@ -8,13 +8,16 @@ import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
 describe("AdminFacet.transferAdmin", () => {
   let deployer: SignerWithAddress;
 
-  context("when caller is not the admin", async () => {
+  context("when caller is not the admin or owner", async () => {
     beforeEach(async () => {
       await deployments.fixture();
       const { adminFacet } = await getContracts();
       deployer = await ethers.getNamedSigner("deployer");
       const signers = await ethers.getSigners();
       await adminFacet.connect(deployer).transferAdmin(signers[1].address);
+      await adminFacet
+        .connect(deployer)
+        .transferDiamondOwner(signers[1].address);
     });
 
     it("reverts with the correct error message", async () => {
@@ -25,7 +28,7 @@ describe("AdminFacet.transferAdmin", () => {
         .transferAdmin(deployer.address);
       await expect(setTx).to.be.revertedWithCustomError(
         adminFacet,
-        "CallerIsNotAdmin"
+        "CallerIsNotAdminOrOwner"
       );
     });
   });
@@ -45,6 +48,18 @@ describe("AdminFacet.transferAdmin", () => {
     expect(adminAddress).to.eq(signers[1].address);
   });
 
+  it("transfers the admin successfully when caller is owner but not admin", async () => {
+    const { viewStateFacet, adminFacet } = await getContracts();
+    const signers = await ethers.getSigners();
+    await adminFacet.connect(deployer).transferAdmin(signers[1].address);
+
+    // Caller is now not the admin, but the owner
+    await adminFacet.connect(deployer).transferAdmin(signers[2].address);
+
+    const adminAddress = await viewStateFacet.connect(deployer).getAdmin();
+    expect(adminAddress).to.eq(signers[2].address);
+  });
+
   it("emits transferAdmin event", async () => {
     const { viewStateFacet, adminFacet } = await getContracts();
     const signers = await ethers.getSigners();

diff --git a/test/facets/admin/transferDiamondOwner.spec.ts b/test/facets/admin/transferDiamondOwner.spec.ts
@@ -0,0 +1,53 @@
+import { accountGenerator } from "../helpers/accounts";
+import { getContracts } from "../helpers/contracts";
+import { expect } from "chai";
+
+const { deployments, ethers } = require("hardhat");
+import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
+
+describe("AdminFacet.transferDiamondOwner", () => {
+  let deployer: SignerWithAddress;
+
+  context("when caller is not the owner", async () => {
+    beforeEach(async () => {
+      await deployments.fixture();
+      const { adminFacet } = await getContracts();
+      deployer = await ethers.getNamedSigner("deployer");
+      const signers = await ethers.getSigners();
+      await adminFacet.connect(deployer).transferDiamondOwner(signers[1].address);
+    });
+
+    it("reverts", async () => {
+      const { adminFacet } = await getContracts();
+      const deployer = await ethers.getNamedSigner("deployer");
+      const setTx = adminFacet
+        .connect(deployer)
+        .transferDiamondOwner(deployer.address);
+      await expect(setTx).to.be.reverted;
+    });
+  });
+
+  beforeEach(async () => {
+    deployer = await ethers.getNamedSigner("deployer");
+    await deployments.fixture();
+    accountGenerator.index = 0;
+  });
+
+  it("transfers the owner successfully", async () => {
+    const { viewStateFacet, adminFacet } = await getContracts();
+    const signers = await ethers.getSigners();
+    await adminFacet.connect(deployer).transferDiamondOwner(signers[1].address);
+
+    const ownerAddress = await adminFacet.connect(deployer).getDiamondOwner();
+    expect(ownerAddress).to.eq(signers[1].address);
+  });
+
+  it("does allow owner address to be the zero address", async () => {
+    const { adminFacet } = await getContracts();
+    const deployer = await ethers.getNamedSigner("deployer");
+    const setTx = adminFacet
+      .connect(deployer)
+      .transferDiamondOwner(ethers.constants.AddressZero);
+    await expect(setTx).not.to.be.reverted;
+  });
+});
diff --git a/test/facets/admin/withdrawProtocolFees.spec.ts b/test/facets/admin/withdrawProtocolFees.spec.ts
@@ -12,21 +12,17 @@ describe("AdminFacet.withdrawProtocolFees", () => {
   context("when caller is not the admin", async () => {
     beforeEach(async () => {
       await deployments.fixture();
-      const { adminFacet } = await getContracts();
-      deployer = await ethers.getNamedSigner("deployer");
-      const signers = await ethers.getSigners();
-      await adminFacet.connect(deployer).transferAdmin(signers[1].address);
     });
 
     it("reverts with the correct error message", async () => {
       const { adminFacet } = await getContracts();
-      const deployer = await ethers.getNamedSigner("deployer");
+      const signers = await ethers.getSigners();
       const setTx = adminFacet
-        .connect(deployer)
+        .connect(signers[1])
         .withdrawProtocolFees(deployer.address);
       await expect(setTx).to.be.revertedWithCustomError(
         adminFacet,
-        "CallerIsNotAdmin"
+        "CallerIsNotAdminOrOwner"
       );
     });
   });