Remove redundant ProxyAdmin from factory, operator role

yieldnest · Aug 27, 2024 · 359aa12 · 359aa12
1 parent b469c5b
commit 359aa12
Show file tree

Hide file tree

Showing 10 changed files with 36 additions and 54 deletions.
diff --git a/Makefile b/Makefile
@@ -5,6 +5,4 @@ account :; cast wallet import $(ACCOUNT_NAME) --interactive
 factory :; forge script script/DeployVaultFactory.sol:DeployVaultFactory \
 	--account $(ACCOUNT_NAME) \
 	--rpc-url $(RPC_URL) \
-	--broadcast \
-	--etherscan-api-key $(ETHERSCAN_KEY) \
-	--verify
+	--broadcast
diff --git a/src/ISingleVault.sol b/src/ISingleVault.sol
@@ -4,12 +4,18 @@ pragma solidity ^0.8.24;
 import {IERC20, IERC4626, IAccessControl} from "src/Common.sol";
 
 interface ISingleVault is IERC20, IERC4626, IAccessControl {
+    error AssetZeroAddress();
+    error NameEmpty();
+    error SymbolEmpty();
+    error AdminZeroAddress();
+    error ProposersEmpty();
+    error ExecutorsEmpty();
+
     function initialize(
         IERC20 asset_,
         string memory name_,
         string memory symbol_,
         address admin_,
-        address operator_,
         uint256 minDelay_,
         address[] calldata proposers_,
         address[] calldata executors_

diff --git a/src/SingleVault.sol b/src/SingleVault.sol
@@ -6,13 +6,14 @@ import {
     AccessControlUpgradeable,
     ReentrancyGuardUpgradeable,
     TimelockControllerUpgradeable,
-    IERC20
+    IERC20,
+    IERC4626
 } from "src/Common.sol";
 
 import {ISingleVault} from "src/ISingleVault.sol";
 
+
 contract SingleVault is ISingleVault, ERC4626Upgradeable, TimelockControllerUpgradeable, ReentrancyGuardUpgradeable {
-    bytes32 public constant OPERATOR_ROLE = keccak256("OPERATOR_ROLE");
 
     constructor() {
         _disableInitializers();
@@ -24,7 +25,6 @@ contract SingleVault is ISingleVault, ERC4626Upgradeable, TimelockControllerUpgr
      * @param name_ The name of the vault.
      * @param symbol_ The symbol of the vault.
      * @param admin_ The address of the admin.
-     * @param operator_ The address of the operator.
      * @param minDelay_ The minimum delay for timelock.
      * @param proposers_ The addresses of the proposers.
      * @param executors_ The addresses of the executors.
@@ -34,38 +34,32 @@ contract SingleVault is ISingleVault, ERC4626Upgradeable, TimelockControllerUpgr
         string memory name_,
         string memory symbol_,
         address admin_,
-        address operator_,
         uint256 minDelay_,
         address[] calldata proposers_,
         address[] calldata executors_
     ) public initializer {
-        _verifyParamsAreValid(asset_, name_, symbol_, admin_, operator_, proposers_, executors_);
-
+        _verifyParamsAreValid(asset_, name_, symbol_, admin_, proposers_, executors_);
         __TimelockController_init(minDelay_, proposers_, executors_, admin_);
         __ERC20_init(name_, symbol_);
         __ERC4626_init(asset_);
         __AccessControl_init();
         __ReentrancyGuard_init();
-
         _grantRole(DEFAULT_ADMIN_ROLE, admin_);
-        _grantRole(OPERATOR_ROLE, operator_);
     }
 
     function _verifyParamsAreValid(
         IERC20 asset_,
         string memory name_,
         string memory symbol_,
         address admin_,
-        address operator_,
         address[] memory proposers_,
         address[] memory executors_
     ) internal pure {
-        require(asset_ != IERC20(address(0)), "Asset cannot be zero address");
-        require(bytes(name_).length > 0, "Name cannot be empty");
-        require(bytes(symbol_).length > 0, "Symbol cannot be empty");
-        require(admin_ != address(0), "Admin cannot be zero address");
-        require(operator_ != address(0), "Operator cannot be zero address");
-        require(proposers_.length > 0, "Proposers cannot be empty");
-        require(executors_.length > 0, "Executors cannot be empty");
+        if (asset_ == IERC20(address(0))) revert AssetZeroAddress();
+        if (bytes(name_).length == 0) revert NameEmpty();
+        if (bytes(symbol_).length == 0) revert SymbolEmpty();
+        if (admin_ == address(0)) revert AdminZeroAddress();
+        if (proposers_.length == 0) revert ProposersEmpty();
+        if (executors_.length == 0) revert ExecutorsEmpty();
     }
 }
diff --git a/src/VaultFactory.sol b/src/VaultFactory.sol
@@ -12,14 +12,14 @@ import {
 contract VaultFactory is AccessControlUpgradeable {
     string public constant version = "0.1.0";
 
-    ProxyAdmin public proxyAdmin;
     TimelockController public timelock;
 
     address public singleVaultImpl;
     address public multiVaultImpl;
 
     struct Vault {
         address vault;
+        address timelock;
         string name;
         string symbol;
         VaultType vaultType;
@@ -54,8 +54,10 @@ contract VaultFactory is AccessControlUpgradeable {
         address admin
     ) {
         _grantRole(DEFAULT_ADMIN_ROLE, admin);
+        // NOTES: There are two timelocks. This timelock is for vault upgrades but
+        // the vault is the second timelock controller, which has the same proposers and executors
+        // as the proxy admins
         timelock = new TimelockController(minDelay, proposers, executors, admin);
-        proxyAdmin = new ProxyAdmin(address(timelock));
         singleVaultImpl = singleVaultImpl_;
     }
 
@@ -65,7 +67,6 @@ contract VaultFactory is AccessControlUpgradeable {
      * @param name_ The name of the vault.
      * @param symbol_ The symbol of the vault.
      * @param admin_ The address of the admin.
-     * @param operator_ The address of the operator.
      * @param minDelay_ The timelock delay for transactions.
      * @param proposers_ Array of transaction proposers.
      * @param executors_ Array of transaction executors.
@@ -76,24 +77,22 @@ contract VaultFactory is AccessControlUpgradeable {
         string memory name_,
         string memory symbol_,
         address admin_,
-        address operator_,
         uint256 minDelay_,
         address[] memory proposers_,
         address[] memory executors_
     ) public onlyRole(DEFAULT_ADMIN_ROLE) returns (address) {
         if (vaults[symbol_].vault != address(0)) revert SymbolUsed();
 
-        string memory funcSig = "initialize(address,string,string,address,address,uint256,address[],address[])";
+        string memory funcSig = "initialize(address,string,string,address,uint256,address[],address[])";
 
         TransparentUpgradeableProxy proxy = new TransparentUpgradeableProxy(
             singleVaultImpl,
-            address(proxyAdmin),
+            address(timelock),
             abi.encodeWithSignature(
-                funcSig, asset_, name_, symbol_, admin_, operator_, minDelay_, proposers_, executors_
+                funcSig, asset_, name_, symbol_, admin_, minDelay_, proposers_, executors_
             )
         );
-
-        vaults[symbol_] = Vault(address(proxy), name_, symbol_, VaultType.SingleAsset);
+        vaults[symbol_] = Vault(address(proxy), address(timelock), name_, symbol_, VaultType.SingleAsset);
         emit NewVault(address(proxy), name_, symbol_, VaultType.SingleAsset);
         return address(proxy);
     }

diff --git a/test/factory/create.t.sol b/test/factory/create.t.sol
@@ -28,25 +28,17 @@ contract CreateTest is Test, LocalActors, TestConstants {
     function testCreateSingleVault() public {
         vm.startPrank(ADMIN);
         address vault =
-            factory.createSingleVault(asset, VAULT_NAME, VAULT_SYMBOL, ADMIN, OPERATOR, minDelay, proposers, executors);
-        (address vaultAddress,,,) = factory.vaults(VAULT_SYMBOL);
+            factory.createSingleVault(asset, VAULT_NAME, VAULT_SYMBOL, ADMIN, minDelay, proposers, executors);
+        (address vaultAddress,,,,) = factory.vaults(VAULT_SYMBOL);
         assertEq(vaultAddress, vault, "Vault address should match the expected address");
     }
 
     function testVaultFactoryAdmin() public view {
         assertTrue(factory.hasRole(factory.DEFAULT_ADMIN_ROLE(), ADMIN));
     }
 
-    function testProxyOwner() public view {
-        ProxyAdmin proxyAdmin = ProxyAdmin(factory.proxyAdmin());
-        address proxyOwner = proxyAdmin.owner();
-        address timelock = address(factory.timelock());
-
-        assertEq(proxyOwner, timelock);
-    }
-
     function skip_testCreateSingleVaultRevertsIfNotAdmin() public {
         vm.expectRevert(abi.encodeWithSelector(bytes4(keccak256("AccessControl: must have admin role"))));
-        factory.createSingleVault(asset, VAULT_NAME, VAULT_SYMBOL, ADMIN, OPERATOR, minDelay, proposers, executors);
+        factory.createSingleVault(asset, VAULT_NAME, VAULT_SYMBOL, ADMIN, minDelay, proposers, executors);
     }
 }
diff --git a/test/single/access.t.sol b/test/single/access.t.sol
@@ -24,7 +24,6 @@ contract AccessControlTest is Test, LocalActors, TestConstants {
             VAULT_NAME,
             VAULT_SYMBOL,
             ADMIN,
-            OPERATOR,
             0, // time delay
             deployFactory.getProposers(),
             deployFactory.getExecutors()
@@ -38,8 +37,8 @@ contract AccessControlTest is Test, LocalActors, TestConstants {
 
     function testAdminCanGrantRole() public {
         vm.startPrank(ADMIN);
-        vault.grantRole(vault.OPERATOR_ROLE(), OPERATOR);
-        assertEq(vault.hasRole(vault.OPERATOR_ROLE(), OPERATOR), true);
+        vault.grantRole(vault.DEFAULT_ADMIN_ROLE(), OPERATOR);
+        assertEq(vault.hasRole(vault.DEFAULT_ADMIN_ROLE(), OPERATOR), true);
     }
 
     function skip_testNonAdminCannotGrantRole() public {
@@ -49,13 +48,13 @@ contract AccessControlTest is Test, LocalActors, TestConstants {
                 IAccessControl.AccessControlUnauthorizedAccount.selector, UNAUTHORIZED, vault.DEFAULT_ADMIN_ROLE()
             )
         );
-        vault.grantRole(vault.OPERATOR_ROLE(), address(4));
+        vault.grantRole(vault.DEFAULT_ADMIN_ROLE(), address(4));
     }
 
     function testAdminCanRevokeRole() public {
         vm.startPrank(ADMIN);
-        vault.revokeRole(vault.OPERATOR_ROLE(), OPERATOR);
-        assertFalse(vault.hasRole(vault.OPERATOR_ROLE(), OPERATOR));
+        vault.revokeRole(vault.DEFAULT_ADMIN_ROLE(), OPERATOR);
+        assertFalse(vault.hasRole(vault.DEFAULT_ADMIN_ROLE(), OPERATOR));
     }
 
     function skip_testNonAdminCannotRevokeRole() public {
@@ -65,6 +64,6 @@ contract AccessControlTest is Test, LocalActors, TestConstants {
                 IAccessControl.AccessControlUnauthorizedAccount.selector, UNAUTHORIZED, vault.DEFAULT_ADMIN_ROLE()
             )
         );
-        vault.revokeRole(vault.OPERATOR_ROLE(), OPERATOR);
+        vault.revokeRole(vault.DEFAULT_ADMIN_ROLE(), OPERATOR);
     }
 }
diff --git a/test/single/admin.t.sol b/test/single/admin.t.sol
@@ -32,7 +32,6 @@ contract TimelockTest is Test, LocalActors, TestConstants {
             VAULT_NAME,
             VAULT_SYMBOL,
             ADMIN,
-            OPERATOR,
             0, // admin tx time delay
             deployFactory.getProposers(),
             deployFactory.getExecutors()

diff --git a/test/single/deposit.t.sol b/test/single/deposit.t.sol
@@ -24,7 +24,6 @@ contract DepositTest is Test, LocalActors, TestConstants {
             VAULT_NAME,
             VAULT_SYMBOL,
             ADMIN,
-            OPERATOR,
             0, // time delay
             deployFactory.getProposers(),
             deployFactory.getExecutors()

diff --git a/test/single/initialize.t.sol b/test/single/initialize.t.sol
@@ -27,7 +27,6 @@ contract InitializeTest is Test, LocalActors, TestConstants {
             VAULT_NAME,
             VAULT_SYMBOL,
             ADMIN,
-            OPERATOR,
             0, // time delay
             deployFactory.getProposers(),
             deployFactory.getExecutors()
@@ -41,12 +40,11 @@ contract InitializeTest is Test, LocalActors, TestConstants {
             address(vaultImplementation),
             address(this),
             abi.encodeWithSignature(
-                "initialize(address,string,string,address,address,uint256,address[],address[])",
+                "initialize(address,string,string,address,uint256,address[],address[])",
                 asset,
                 VAULT_NAME,
                 VAULT_SYMBOL,
                 ADMIN,
-                OPERATOR,
                 0,
                 deployFactory.getProposers(),
                 deployFactory.getExecutors()
@@ -56,7 +54,6 @@ contract InitializeTest is Test, LocalActors, TestConstants {
         ISingleVault newVault = ISingleVault(address(proxy));
 
         assertEq(newVault.asset(), address(asset));
-        assertEq(newVault.hasRole(vault.OPERATOR_ROLE(), OPERATOR), true);
         assertEq(newVault.hasRole(vault.DEFAULT_ADMIN_ROLE(), ADMIN), true);
         assertEq(newVault.symbol(), VAULT_SYMBOL);
         assertEq(newVault.name(), VAULT_NAME);

diff --git a/test/single/withdraw.t.sol b/test/single/withdraw.t.sol
@@ -26,7 +26,6 @@ contract WithdrawTest is Test, LocalActors, TestConstants {
             VAULT_NAME,
             VAULT_SYMBOL,
             ADMIN,
-            OPERATOR,
             0, // admin tx time delay
             deployFactory.getProposers(),
             deployFactory.getExecutors()