Set subnet creator as owner + add Ownable facet to contracts (#785)

Co-authored-by: raulk <[email protected]> Co-authored-by: raulk <[email protected]>
consensus-shipyard · Mar 14, 2024 · 0babe1b · 0babe1b
1 parent e7fdce2
commit 0babe1b
Show file tree

Hide file tree

Showing 19 changed files with 189 additions and 38 deletions.
diff --git a/.github/workflows/contracts-sast.yaml b/.github/workflows/contracts-sast.yaml
@@ -42,7 +42,7 @@ jobs:
         run: cargo install aderyn
 
       - name: Run aderyn
-        run: cd contracts && aderyn ./
+        run: cd contracts && aderyn ./ -o report.json
 
       - name: Check results
         run: cd contracts && ./tools/check_aderyn.sh

diff --git a/contracts/.gitignore b/contracts/.gitignore
@@ -31,6 +31,7 @@ lcov.info
 
 # Aderyn scanner
 report.md
+report.json
 
 #vim
 *.un~
diff --git a/contracts/audit-resolve.json b/contracts/audit-resolve.json
@@ -55,6 +55,10 @@
         "1096544|json5": {
             "decision": "ignore",
             "madeAt": 1708963077846
+        },
+        "1096644|browserify-sign": {
+            "decision": "ignore",
+            "madeAt": 1710415772020
         }
     },
     "rules": {},

diff --git a/contracts/scripts/deploy-gateway.template.ts b/contracts/scripts/deploy-gateway.template.ts
@@ -92,6 +92,7 @@ export async function deploy(libs: { [key in string]: string }) {
             libs: xnetMessagingFacetLibs,
         },
         { name: 'TopDownFinalityFacet', libs: topDownFinalityFacetLibs },
+        { name: 'OwnershipFacet', libs: {} },
     ]
 
     for (const facet of facets) {

diff --git a/contracts/scripts/deploy-registry.template.ts b/contracts/scripts/deploy-registry.template.ts
@@ -90,6 +90,7 @@ export async function deploy() {
         { name: 'SubnetGetterFacet', libs: {} },
         { name: 'DiamondLoupeFacet', libs: {} },
         { name: 'DiamondCutFacet', libs: {} },
+        { name: 'OwnershipFacet', libs: {} },
     ]
 
     for (const facet of facets) {

diff --git a/contracts/scripts/deploy-sa-diamond.ts b/contracts/scripts/deploy-sa-diamond.ts
@@ -42,6 +42,7 @@ async function deploySubnetActorDiamond(
         { name: 'SubnetActorRewardFacet', libs: rewarderFacetLibs },
         { name: 'SubnetActorCheckpointingFacet', libs: checkpointerFacetLibs },
         { name: 'SubnetActorPauseFacet', libs: pauserFacetLibs },
+        { name: 'OwnershipFacet', libs: {} },
     ]
     // The `facetCuts` variable is the FacetCut[] that contains the functions to add during diamond deployment
     const facetCuts = []

diff --git a/contracts/scripts/python/build_selector_library.py b/contracts/scripts/python/build_selector_library.py
@@ -70,6 +70,7 @@ def main():
          'src/GatewayDiamond.sol',
          'src/SubnetActorDiamond.sol',
          'src/SubnetRegistryDiamond.sol',
+         'src/OwnershipFacet.sol',
          'src/diamond/DiamondCutFacet.sol',
          'src/diamond/DiamondLoupeFacet.sol',
          'src/gateway/GatewayGetterFacet.sol',

diff --git a/contracts/src/OwnershipFacet.sol b/contracts/src/OwnershipFacet.sol
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.23;
+
+import {LibDiamond} from "./lib/LibDiamond.sol";
+
+contract OwnershipFacet {
+    function transferOwnership(address _newOwner) external {
+        LibDiamond.transferOwnership(_newOwner);
+    }
+
+    function owner() external view returns (address owner_) {
+        owner_ = LibDiamond.contractOwner();
+    }
+}
diff --git a/contracts/src/SubnetActorDiamond.sol b/contracts/src/SubnetActorDiamond.sol
@@ -15,7 +15,6 @@ import {SubnetIDHelper} from "./lib/SubnetIDHelper.sol";
 import {LibStaking} from "./lib/LibStaking.sol";
 import {IERC20} from "openzeppelin-contracts/token/ERC20/IERC20.sol";
 import {SupplySourceHelper} from "./lib/SupplySourceHelper.sol";
-
 error FunctionNotFound(bytes4 _functionSelector);
 
 contract SubnetActorDiamond {
@@ -38,7 +37,7 @@ contract SubnetActorDiamond {
         SubnetID parentId;
     }
 
-    constructor(IDiamond.FacetCut[] memory _diamondCut, ConstructorParams memory params) {
+    constructor(IDiamond.FacetCut[] memory _diamondCut, ConstructorParams memory params, address owner) {
         if (params.ipcGatewayAddr == address(0)) {
             revert GatewayCannotBeZero();
         }
@@ -58,7 +57,7 @@ contract SubnetActorDiamond {
 
         params.supplySource.validate();
 
-        LibDiamond.setContractOwner(msg.sender);
+        LibDiamond.setContractOwner(owner);
         LibDiamond.diamondCut({_diamondCut: _diamondCut, _init: address(0), _calldata: new bytes(0)});
 
         LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();

diff --git a/contracts/src/SubnetRegistryDiamond.sol b/contracts/src/SubnetRegistryDiamond.sol
@@ -8,6 +8,7 @@ import {IERC165} from "./interfaces/IERC165.sol";
 import {SubnetRegistryActorStorage} from "./lib/LibSubnetRegistryStorage.sol";
 import {GatewayCannotBeZero, FacetCannotBeZero} from "./errors/IPCErrors.sol";
 import {LibDiamond} from "./lib/LibDiamond.sol";
+
 error FunctionNotFound(bytes4 _functionSelector);
 
 contract SubnetRegistryDiamond {

diff --git a/contracts/src/lib/LibDiamond.sol b/contracts/src/lib/LibDiamond.sol
@@ -7,6 +7,7 @@ import {IDiamond} from "../interfaces/IDiamond.sol";
 library LibDiamond {
     bytes32 public constant DIAMOND_STORAGE_POSITION = keccak256("libdiamond.lib.diamond.storage");
 
+    error InvalidAddress();
     error NotOwner();
     error NoBytecodeAtAddress(address _contractAddress, string _message);
     error IncorrectFacetCutAction(IDiamondCut.FacetCutAction _action);
@@ -24,6 +25,8 @@ library LibDiamond {
     error CannotRemoveFunctionThatDoesNotExist(bytes4 _selector);
     error CannotRemoveImmutableFunction(bytes4 _selector);
 
+    event OwnershipTransferred(address oldOwner, address newOwner);
+
     struct FacetAddressAndSelectorPosition {
         address facetAddress;
         uint16 selectorPosition;
@@ -38,6 +41,17 @@ library LibDiamond {
         address contractOwner;
     }
 
+    /**
+     * @dev Transfers ownership of the contract to a new account (`newOwner`).
+     * Can only be called by the current owner.
+     */
+    function transferOwnership(address newOwner) internal onlyOwner {
+        if (newOwner == address(0)) {
+            revert InvalidAddress();
+        }
+        setContractOwner(newOwner);
+    }
+
     function diamondStorage() internal pure returns (DiamondStorage storage ds) {
         bytes32 position = DIAMOND_STORAGE_POSITION;
         assembly {
@@ -47,7 +61,10 @@ library LibDiamond {
 
     function setContractOwner(address _newOwner) internal {
         DiamondStorage storage ds = diamondStorage();
+
+        address oldOwner = ds.contractOwner;
         ds.contractOwner = _newOwner;
+        emit OwnershipTransferred(oldOwner, _newOwner);
     }
 
     function contractOwner() internal view returns (address contractOwner_) {

diff --git a/contracts/src/subnetregistry/RegisterSubnetFacet.sol b/contracts/src/subnetregistry/RegisterSubnetFacet.sol
@@ -58,7 +58,7 @@ contract RegisterSubnetFacet is ReentrancyGuard {
         });
 
         // slither-disable-next-line reentrancy-benign
-        subnetAddr = address(new SubnetActorDiamond(diamondCut, _params));
+        subnetAddr = address(new SubnetActorDiamond(diamondCut, _params, msg.sender));
 
         //nonces start with 1, similar to eip 161
         ++s.userNonces[msg.sender];

diff --git a/contracts/test/IntegrationTestBase.sol b/contracts/test/IntegrationTestBase.sol
@@ -35,6 +35,8 @@ import {SubnetRegistryDiamond} from "../src/SubnetRegistryDiamond.sol";
 import {RegisterSubnetFacet} from "../src/subnetregistry/RegisterSubnetFacet.sol";
 import {SubnetGetterFacet} from "../src/subnetregistry/SubnetGetterFacet.sol";
 
+import {OwnershipFacet} from "../src/OwnershipFacet.sol";
+
 import {DiamondLoupeFacet} from "../src/diamond/DiamondLoupeFacet.sol";
 import {DiamondCutFacet} from "../src/diamond/DiamondCutFacet.sol";
 import {SupplySourceHelper} from "../src/lib/SupplySourceHelper.sol";
@@ -89,18 +91,21 @@ contract TestRegistry is Test, TestParams {
     bytes4[] registerSubnetGetterFacetSelectors;
     bytes4[] registerCutterSelectors;
     bytes4[] registerLouperSelectors;
+    bytes4[] registerOwnershipSelectors;
 
     SubnetRegistryDiamond registryDiamond;
     DiamondLoupeFacet registryLouper;
     DiamondCutFacet registryCutter;
     RegisterSubnetFacet registrySubnetFacet;
     SubnetGetterFacet registrySubnetGetterFacet;
+    OwnershipFacet ownershipFacet;
 
     constructor() {
         registerSubnetFacetSelectors = SelectorLibrary.resolveSelectors("RegisterSubnetFacet");
         registerSubnetGetterFacetSelectors = SelectorLibrary.resolveSelectors("SubnetGetterFacet");
         registerCutterSelectors = SelectorLibrary.resolveSelectors("DiamondCutFacet");
         registerLouperSelectors = SelectorLibrary.resolveSelectors("DiamondLoupeFacet");
+        registerOwnershipSelectors = SelectorLibrary.resolveSelectors("OwnershipFacet");
     }
 }
 
@@ -116,6 +121,8 @@ contract TestGatewayActor is Test, TestParams {
     bytes4[] gwCutterSelectors;
     bytes4[] gwLoupeSelectors;
 
+    bytes4[] gwOwnershipSelectors;
+
     GatewayDiamond gatewayDiamond;
 
     constructor() {
@@ -128,6 +135,8 @@ contract TestGatewayActor is Test, TestParams {
         gwMessengerSelectors = SelectorLibrary.resolveSelectors("GatewayMessengerFacet");
         gwCutterSelectors = SelectorLibrary.resolveSelectors("DiamondCutFacet");
         gwLoupeSelectors = SelectorLibrary.resolveSelectors("DiamondLoupeFacet");
+
+        gwOwnershipSelectors = SelectorLibrary.resolveSelectors("OwnershipFacet");
     }
 }
 
@@ -140,6 +149,7 @@ contract TestSubnetActor is Test, TestParams {
     bytes4[] saManagerMockedSelectors;
     bytes4[] saCutterSelectors;
     bytes4[] saLouperSelectors;
+    bytes4[] saOwnershipSelectors;
 
     SubnetActorDiamond saDiamond;
     SubnetActorMock saMock;
@@ -153,6 +163,7 @@ contract TestSubnetActor is Test, TestParams {
         saManagerMockedSelectors = SelectorLibrary.resolveSelectors("SubnetActorMock");
         saCutterSelectors = SelectorLibrary.resolveSelectors("DiamondCutFacet");
         saLouperSelectors = SelectorLibrary.resolveSelectors("DiamondLoupeFacet");
+        saOwnershipSelectors = SelectorLibrary.resolveSelectors("OwnershipFacet");
     }
 
     function defaultSubnetActorParamsWith(
@@ -285,8 +296,9 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,
         GatewayMessengerFacet messenger = new GatewayMessengerFacet();
         DiamondCutFacet cutter = new DiamondCutFacet();
         DiamondLoupeFacet louper = new DiamondLoupeFacet();
+        OwnershipFacet ownership = new OwnershipFacet();
 
-        IDiamond.FacetCut[] memory gwDiamondCut = new IDiamond.FacetCut[](8);
+        IDiamond.FacetCut[] memory gwDiamondCut = new IDiamond.FacetCut[](9);
 
         gwDiamondCut[0] = (
             IDiamond.FacetCut({
@@ -352,6 +364,13 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,
             })
         );
 
+        gwDiamondCut[8] = (
+            IDiamond.FacetCut({
+                facetAddress: address(ownership),
+                action: IDiamond.FacetCutAction.Add,
+                functionSelectors: gwOwnershipSelectors
+            })
+        );
         gatewayDiamond = new GatewayDiamond(gwDiamondCut, params);
 
         return gatewayDiamond;
@@ -363,9 +382,10 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,
         address manager,
         address pauser,
         address rewarder,
-        address checkpointer
+        address checkpointer,
+        address ownership
     ) public returns (SubnetActorDiamond) {
-        IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](5);
+        IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](6);
 
         diamondCut[0] = (
             IDiamond.FacetCut({
@@ -407,7 +427,15 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,
             })
         );
 
-        saDiamond = new SubnetActorDiamond(diamondCut, params);
+        diamondCut[5] = (
+            IDiamond.FacetCut({
+                facetAddress: ownership,
+                action: IDiamond.FacetCutAction.Add,
+                functionSelectors: saOwnershipSelectors
+            })
+        );
+
+        saDiamond = new SubnetActorDiamond(diamondCut, params, address(this));
         return saDiamond;
     }
 
@@ -419,8 +447,9 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,
         SubnetActorCheckpointingFacet checkpointer = new SubnetActorCheckpointingFacet();
         DiamondLoupeFacet louper = new DiamondLoupeFacet();
         DiamondCutFacet cutter = new DiamondCutFacet();
+        OwnershipFacet ownership = new OwnershipFacet();
 
-        IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](7);
+        IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](8);
 
         diamondCut[0] = (
             IDiamond.FacetCut({
@@ -478,7 +507,15 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,
             })
         );
 
-        SubnetActorDiamond diamond = new SubnetActorDiamond(diamondCut, params);
+        diamondCut[7] = (
+            IDiamond.FacetCut({
+                facetAddress: address(ownership),
+                action: IDiamond.FacetCutAction.Add,
+                functionSelectors: saOwnershipSelectors
+            })
+        );
+
+        SubnetActorDiamond diamond = new SubnetActorDiamond(diamondCut, params, address(this));
 
         return diamond;
     }
@@ -534,8 +571,9 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,
     function createMockedSubnetActorWithGateway(address gw) public returns (SubnetActorDiamond) {
         SubnetActorMock mockedManager = new SubnetActorMock();
         SubnetActorGetterFacet getter = new SubnetActorGetterFacet();
+        OwnershipFacet ownership = new OwnershipFacet();
 
-        IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](2);
+        IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](3);
 
         diamondCut[0] = (
             IDiamond.FacetCut({
@@ -553,9 +591,17 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,
             })
         );
 
+        diamondCut[2] = (
+            IDiamond.FacetCut({
+                facetAddress: address(ownership),
+                action: IDiamond.FacetCutAction.Add,
+                functionSelectors: saOwnershipSelectors
+            })
+        );
+
         SubnetActorDiamond.ConstructorParams memory params = defaultSubnetActorParamsWith(gw);
 
-        SubnetActorDiamond d = new SubnetActorDiamond(diamondCut, params);
+        SubnetActorDiamond d = new SubnetActorDiamond(diamondCut, params, address(this));
 
         return d;
     }
@@ -564,12 +610,13 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,
     function createSubnetRegistry(
         SubnetRegistryDiamond.ConstructorParams memory params
     ) public returns (SubnetRegistryDiamond) {
-        IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](4);
+        IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](5);
 
         DiamondCutFacet regCutFacet = new DiamondCutFacet();
         DiamondLoupeFacet regLoupeFacet = new DiamondLoupeFacet();
         RegisterSubnetFacet regSubnetFacet = new RegisterSubnetFacet();
         SubnetGetterFacet regGetterFacet = new SubnetGetterFacet();
+        OwnershipFacet ownership = new OwnershipFacet();
 
         diamondCut[0] = (
             IDiamond.FacetCut({
@@ -600,6 +647,14 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,
             })
         );
 
+        diamondCut[4] = (
+            IDiamond.FacetCut({
+                facetAddress: address(ownership),
+                action: IDiamond.FacetCutAction.Add,
+                functionSelectors: registerOwnershipSelectors
+            })
+        );
+
         SubnetRegistryDiamond newSubnetRegistry = new SubnetRegistryDiamond(diamondCut, params);
         emit SubnetRegistryCreated(address(newSubnetRegistry));
         return newSubnetRegistry;