Feat: Remove set withdrawal delay (#355)

8sunyuan · web-flow · commit fe4a8e234c84 · 2023-12-04T18:53:17.000-05:00
* feat: remove setWithdrawalDelayBlocks()

removing ability to modify withdrawalDelayBlocks, will only be initialized once

* fix: fixed delegation.initialize() calls

- Added `initializedWithdrawalDelayBlocks` to initialize params
- Added regression test for initializing&gt; MAX_WITHDRAWAL_DELAY_BLOCKS
- DelegationUnit.t.sol, small fix for internal function `_registerOperatorWith1271DelegationApprover`

* chore: emit WithdrawalDelayBlocksSet
diff --git a/script/middleware/DeployOpenEigenLayer.s.sol b/script/middleware/DeployOpenEigenLayer.s.sol
@@ -139,7 +139,7 @@ contract DeployOpenEigenLayer is Script, Test {
         eigenLayerProxyAdmin.upgradeAndCall(
             TransparentUpgradeableProxy(payable(address(delegation))),
             address(delegationImplementation),
-            abi.encodeWithSelector(DelegationManager.initialize.selector, executorMultisig, eigenLayerPauserReg, 0)
+            abi.encodeWithSelector(DelegationManager.initialize.selector, executorMultisig, eigenLayerPauserReg, 0, 0 /* withdrawalDelayBlocks */)
         );
         eigenLayerProxyAdmin.upgradeAndCall(
             TransparentUpgradeableProxy(payable(address(strategyManager))),
diff --git a/script/milestone/M2Deploy.s.sol b/script/milestone/M2Deploy.s.sol
@@ -326,7 +326,7 @@ contract M2Deploy is Script, Test {
         );
 
         cheats.expectRevert(bytes("Initializable: contract is already initialized"));
-        DelegationManager(address(delegation)).initialize(address(this), PauserRegistry(address(this)), 0);
+        DelegationManager(address(delegation)).initialize(address(this), PauserRegistry(address(this)), 0, 0);
 
         cheats.expectRevert(bytes("Initializable: contract is already initialized"));
         EigenPodManager(address(eigenPodManager)).initialize(
diff --git a/script/testing/M2_Deploy_From_Scratch.s.sol b/script/testing/M2_Deploy_From_Scratch.s.sol
@@ -82,6 +82,7 @@ contract Deployer_M2 is Script, Test {
     uint256 STRATEGY_MANAGER_INIT_PAUSED_STATUS;
     uint256 SLASHER_INIT_PAUSED_STATUS;
     uint256 DELEGATION_INIT_PAUSED_STATUS;
+    uint256 DELEGATION_WITHDRAWAL_DELAY_BLOCKS;
     uint256 EIGENPOD_MANAGER_INIT_PAUSED_STATUS;
     uint256 EIGENPOD_MANAGER_MAX_PODS;
     uint256 DELAYED_WITHDRAWAL_ROUTER_INIT_PAUSED_STATUS;
@@ -103,6 +104,7 @@ contract Deployer_M2 is Script, Test {
         STRATEGY_MANAGER_INIT_PAUSED_STATUS = stdJson.readUint(config_data, ".strategyManager.init_paused_status");
         SLASHER_INIT_PAUSED_STATUS = stdJson.readUint(config_data, ".slasher.init_paused_status");
         DELEGATION_INIT_PAUSED_STATUS = stdJson.readUint(config_data, ".delegation.init_paused_status");
+        DELEGATION_WITHDRAWAL_DELAY_BLOCKS = stdJson.readUint(config_data, ".delegation.init_withdrawal_delay_blocks");
         EIGENPOD_MANAGER_MAX_PODS = stdJson.readUint(config_data, ".eigenPodManager.max_pods");
         EIGENPOD_MANAGER_INIT_PAUSED_STATUS = stdJson.readUint(config_data, ".eigenPodManager.init_paused_status");
         DELAYED_WITHDRAWAL_ROUTER_INIT_PAUSED_STATUS = stdJson.readUint(
@@ -208,7 +210,8 @@ contract Deployer_M2 is Script, Test {
                 DelegationManager.initialize.selector,
                 executorMultisig,
                 eigenLayerPauserReg,
-                DELEGATION_INIT_PAUSED_STATUS
+                DELEGATION_INIT_PAUSED_STATUS,
+                DELEGATION_WITHDRAWAL_DELAY_BLOCKS
             )
         );
         eigenLayerProxyAdmin.upgradeAndCall(
diff --git a/script/testing/M2_deploy_from_scratch.mainnet.config.json b/script/testing/M2_deploy_from_scratch.mainnet.config.json
@@ -53,7 +53,8 @@
           },
      "delegation":
           {
-               "init_paused_status": 0
+               "init_paused_status": 0,
+               "init_withdrawal_delay_blocks": 50400
           },
      "ethPOSDepositAddress": "0x00000000219ab540356cBB839Cbe05303d7705Fa"
  }
diff --git a/src/contracts/core/DelegationManager.sol b/src/contracts/core/DelegationManager.sol
@@ -65,15 +65,18 @@ contract DelegationManager is Initializable, OwnableUpgradeable, Pausable, Deleg
 
     /**
      * @dev Initializes the addresses of the initial owner, pauser registry, and paused status.
+     * withdrawalDelayBlocks is set only once here
      */
     function initialize(
         address initialOwner,
         IPauserRegistry _pauserRegistry,
-        uint256 initialPausedStatus
+        uint256 initialPausedStatus,
+        uint256 _withdrawalDelayBlocks
     ) external initializer {
         _initializePauser(_pauserRegistry, initialPausedStatus);
         _DOMAIN_SEPARATOR = _calculateDomainSeparator();
         _transferOwnership(initialOwner);
+        _initializeWithdrawalDelayBlocks(_withdrawalDelayBlocks);
     }
 
     /*******************************************************************************
@@ -218,19 +221,6 @@ contract DelegationManager is Initializable, OwnableUpgradeable, Pausable, Deleg
         _delegate(staker, operator, approverSignatureAndExpiry, approverSalt);
     }
 
-    /**
-     * @notice Owner-only function for modifying the value of the `withdrawalDelayBlocks` variable.
-     * @param newWithdrawalDelayBlocks new value of `withdrawalDelayBlocks`.
-     */
-    function setWithdrawalDelayBlocks(uint256 newWithdrawalDelayBlocks) external onlyOwner {
-        require(
-            newWithdrawalDelayBlocks <= MAX_WITHDRAWAL_DELAY_BLOCKS,
-            "DelegationManager.setWithdrawalDelayBlocks: newWithdrawalDelayBlocks too high"
-        );
-        emit WithdrawalDelayBlocksSet(withdrawalDelayBlocks, newWithdrawalDelayBlocks);
-        withdrawalDelayBlocks = newWithdrawalDelayBlocks;
-    }
-
     /**
      * Allows the staker, the staker's operator, or that operator's delegationApprover to undelegate
      * a staker from their operator. Undelegation immediately removes ALL active shares/strategies from
@@ -771,6 +761,15 @@ contract DelegationManager is Initializable, OwnableUpgradeable, Pausable, Deleg
         }
     }
 
+    function _initializeWithdrawalDelayBlocks(uint256 _withdrawalDelayBlocks) internal {
+        require(
+            _withdrawalDelayBlocks <= MAX_WITHDRAWAL_DELAY_BLOCKS,
+            "DelegationManager._initializeWithdrawalDelayBlocks: _withdrawalDelayBlocks cannot be > MAX_WITHDRAWAL_DELAY_BLOCKS"
+        );
+        emit WithdrawalDelayBlocksSet(withdrawalDelayBlocks, _withdrawalDelayBlocks);
+        withdrawalDelayBlocks = _withdrawalDelayBlocks;
+    }
+
     /*******************************************************************************
                             VIEW FUNCTIONS
     *******************************************************************************/
diff --git a/src/test/Delegation.t.sol b/src/test/Delegation.t.sol
@@ -333,7 +333,7 @@ contract DelegationTests is EigenLayerTestHelper {
     ///         cannot be intitialized multiple times
     function testCannotInitMultipleTimesDelegation() public cannotReinit {
         //delegation has already been initialized in the Deployer test contract
-        delegation.initialize(address(this), eigenLayerPauserReg, 0);
+        delegation.initialize(address(this), eigenLayerPauserReg, 0, initializedWithdrawalDelayBlocks);
     }
 
     /// @notice This function tests to ensure that a you can't register as a delegate multiple times
@@ -370,7 +370,7 @@ contract DelegationTests is EigenLayerTestHelper {
         //delegation has already been initialized in the Deployer test contract
         vm.prank(_attacker);
         cheats.expectRevert(bytes("Initializable: contract is already initialized"));
-        delegation.initialize(_attacker, eigenLayerPauserReg, 0);
+        delegation.initialize(_attacker, eigenLayerPauserReg, 0, initializedWithdrawalDelayBlocks);
     }
 
     /// @notice This function tests that the earningsReceiver cannot be set to address(0)
diff --git a/src/test/DepositWithdraw.t.sol b/src/test/DepositWithdraw.t.sol
@@ -500,7 +500,8 @@ contract DepositWithdrawTests is EigenLayerTestHelper {
                 DelegationManager.initialize.selector,
                 eigenLayerReputedMultisig,
                 eigenLayerPauserReg,
-                0/*initialPausedStatus*/
+                0 /*initialPausedStatus*/,
+                initializedWithdrawalDelayBlocks
             )
         );
         eigenLayerProxyAdmin.upgradeAndCall(
diff --git a/src/test/EigenLayerDeployer.t.sol b/src/test/EigenLayerDeployer.t.sol
@@ -73,6 +73,7 @@ contract EigenLayerDeployer is Operators {
     uint256 public constant eigenTotalSupply = 1000e18;
     uint256 nonce = 69;
     uint256 public gasLimit = 750000;
+    uint256 initializedWithdrawalDelayBlocks = 0;
     uint32 PARTIAL_WITHDRAWAL_FRAUD_PROOF_PERIOD_BLOCKS = 7 days / 12 seconds;
     uint256 REQUIRED_BALANCE_WEI = 32 ether;
     uint64 MAX_PARTIAL_WTIHDRAWAL_AMOUNT_GWEI = 1 ether / 1e9;
@@ -268,7 +269,8 @@ contract EigenLayerDeployer is Operators {
                 DelegationManager.initialize.selector,
                 eigenLayerReputedMultisig,
                 eigenLayerPauserReg,
-                0 /*initialPausedStatus*/
+                0 /*initialPausedStatus*/,
+                initializedWithdrawalDelayBlocks
             )
         );
         eigenLayerProxyAdmin.upgradeAndCall(
diff --git a/src/test/EigenPod.t.sol b/src/test/EigenPod.t.sol
@@ -204,7 +204,8 @@ contract EigenPodTests is ProofParsing, EigenPodPausingConstants {
                 DelegationManager.initialize.selector,
                 initialOwner,
                 pauserReg,
-                0 /*initialPausedStatus*/
+                0 /*initialPausedStatus*/,
+                WITHDRAWAL_DELAY_BLOCKS
             )
         );
         eigenLayerProxyAdmin.upgradeAndCall(
diff --git a/src/test/integration/IntegrationDeployer.t.sol b/src/test/integration/IntegrationDeployer.t.sol
@@ -189,6 +189,7 @@ abstract contract IntegrationDeployer is Test, IUserDeployer {
         DelayedWithdrawalRouter delayedWithdrawalRouterImplementation = new DelayedWithdrawalRouter(eigenPodManager);
 
         // Third, upgrade the proxy contracts to point to the implementations
+        uint256 withdrawalDelayBlocks = 7 days / 12 seconds;
         // DelegationManager
         eigenLayerProxyAdmin.upgradeAndCall(
             TransparentUpgradeableProxy(payable(address(delegationManager))),
@@ -197,7 +198,8 @@ abstract contract IntegrationDeployer is Test, IUserDeployer {
                 DelegationManager.initialize.selector,
                 eigenLayerReputedMultisig, // initialOwner
                 pauserRegistry,
-                0 // initialPausedStatus
+                0 /* initialPausedStatus */,
+                withdrawalDelayBlocks
             )
         );
         // StrategyManager
@@ -237,7 +239,6 @@ abstract contract IntegrationDeployer is Test, IUserDeployer {
             )
         );
         // Delayed Withdrawal Router
-        uint256 withdrawalDelayBlocks = 7 days / 12 seconds;
         eigenLayerProxyAdmin.upgradeAndCall(
             TransparentUpgradeableProxy(payable(address(delayedWithdrawalRouter))),
             address(delayedWithdrawalRouterImplementation),
diff --git a/src/test/unit/DelegationUnit.t.sol b/src/test/unit/DelegationUnit.t.sol
@@ -44,6 +44,8 @@ contract DelegationManagerUnitTests is EigenLayerUnitTestSetup, IDelegationManag
     address defaultOperator = address(this);
     address defaultAVS = address(this);
 
+    uint256 initializedWithdrawalDelayBlocks = 50400;
+
     IStrategy public constant beaconChainETHStrategy = IStrategy(0xbeaC0eeEeeeeEEeEeEEEEeeEEeEeeeEeeEEBEaC0);
 
     // Index for flag that pauses new delegations when set.
@@ -55,6 +57,8 @@ contract DelegationManagerUnitTests is EigenLayerUnitTestSetup, IDelegationManag
     // Index for flag that pauses completing existing withdrawals when set.
     uint8 internal constant PAUSED_EXIT_WITHDRAWAL_QUEUE = 2;
 
+    uint256 public constant MAX_WITHDRAWAL_DELAY_BLOCKS = 50400;
+
     /// @notice mappings used to handle duplicate entries in fuzzed address array input
     mapping(address => uint256) public totalSharesForStrategyInArray;
     mapping(IStrategy => uint256) public delegatedSharesBefore;
@@ -70,7 +74,13 @@ contract DelegationManagerUnitTests is EigenLayerUnitTestSetup, IDelegationManag
                 new TransparentUpgradeableProxy(
                     address(delegationManagerImplementation),
                     address(eigenLayerProxyAdmin),
-                    abi.encodeWithSelector(DelegationManager.initialize.selector, address(this), pauserRegistry, 0) // 0 is initialPausedStatus
+                    abi.encodeWithSelector(
+                        DelegationManager.initialize.selector,
+                        address(this),
+                        pauserRegistry,
+                        0, // 0 is initialPausedStatus
+                        initializedWithdrawalDelayBlocks
+                    )
                 )
             )
         );
@@ -297,7 +307,7 @@ contract DelegationManagerUnitTests_Initialization_Setters is DelegationManagerU
     /// @notice Verifies that the DelegationManager cannot be iniitalized multiple times
     function test_initialize_revert_reinitialization() public {
         cheats.expectRevert("Initializable: contract is already initialized");
-        delegationManager.initialize(address(this), pauserRegistry, 0);
+        delegationManager.initialize(address(this), pauserRegistry, 0, initializedWithdrawalDelayBlocks);
     }
 
     /// @notice Verifies that the stakeRegistry cannot be set after it has already been set
@@ -306,37 +316,27 @@ contract DelegationManagerUnitTests_Initialization_Setters is DelegationManagerU
         delegationManager.setStakeRegistry(stakeRegistryMock);
     }
 
-    function testFuzz_setWithdrawalDelayBlocks_revert_notOwner(
-        address invalidCaller
-    ) public filterFuzzedAddressInputs(invalidCaller) {
-        cheats.prank(invalidCaller);
-        cheats.expectRevert("Ownable: caller is not the owner");
-        delegationManager.setWithdrawalDelayBlocks(0);
-    }
-
-    function testFuzz_setWithdrawalDelayBlocks_revert_tooLarge(uint256 newWithdrawalDelayBlocks) external {
-        // filter fuzzed inputs to disallowed amounts
-        cheats.assume(newWithdrawalDelayBlocks > delegationManager.MAX_WITHDRAWAL_DELAY_BLOCKS());
-
-        // attempt to set the `withdrawalDelayBlocks` variable
-        cheats.expectRevert("DelegationManager.setWithdrawalDelayBlocks: newWithdrawalDelayBlocks too high");
-        delegationManager.setWithdrawalDelayBlocks(newWithdrawalDelayBlocks);
-    }
-
-    function testFuzz_setWithdrawalDelayBlocks(uint256 newWithdrawalDelayBlocks) public {
-        cheats.assume(newWithdrawalDelayBlocks <= delegationManager.MAX_WITHDRAWAL_DELAY_BLOCKS());
-
-        // set the `withdrawalDelayBlocks` variable
-        uint256 previousDelayBlocks = delegationManager.withdrawalDelayBlocks();
-        cheats.expectEmit(true, true, true, true, address(delegationManager));
-        emit WithdrawalDelayBlocksSet(previousDelayBlocks, newWithdrawalDelayBlocks);
-        delegationManager.setWithdrawalDelayBlocks(newWithdrawalDelayBlocks);
-
-        // Check storage
-        assertEq(
-            delegationManager.withdrawalDelayBlocks(),
-            newWithdrawalDelayBlocks,
-            "withdrawalDelayBlocks not set correctly"
+    function testFuzz_initialize_Revert_WhenWithdrawalDelayBlocksTooLarge(uint256 withdrawalDelayBlocks) public {
+        cheats.assume(withdrawalDelayBlocks > MAX_WITHDRAWAL_DELAY_BLOCKS);
+        // Deploy DelegationManager implmentation and proxy
+        delegationManagerImplementation = new DelegationManager(strategyManagerMock, slasherMock, eigenPodManagerMock);
+        cheats.expectRevert(
+            "DelegationManager._initializeWithdrawalDelayBlocks: _withdrawalDelayBlocks cannot be > MAX_WITHDRAWAL_DELAY_BLOCKS"
+        );
+        delegationManager = DelegationManager(
+            address(
+                new TransparentUpgradeableProxy(
+                    address(delegationManagerImplementation),
+                    address(eigenLayerProxyAdmin),
+                    abi.encodeWithSelector(
+                        DelegationManager.initialize.selector,
+                        address(this),
+                        pauserRegistry,
+                        0, // 0 is initialPausedStatus
+                        withdrawalDelayBlocks
+                    )
+                )
+            )
         );
     }
 }
@@ -906,9 +906,6 @@ contract DelegationManagerUnitTests_delegateTo is DelegationManagerUnitTests {
         cheats.roll(type(uint256).max / 2);
         // filter to only *invalid* `expiry` values
         cheats.assume(expiry < block.timestamp);
-
-        address delegationApprover = cheats.addr(delegationSignerPrivateKey);
-
         // filter inputs, since this will fail when the staker is already registered as an operator
         cheats.assume(staker != defaultOperator);
 
@@ -1110,8 +1107,6 @@ contract DelegationManagerUnitTests_delegateTo is DelegationManagerUnitTests {
     ) public filterFuzzedAddressInputs(staker) {
         // filter to only valid `expiry` values
         cheats.assume(expiry >= block.timestamp);
-
-        address delegationApprover = cheats.addr(delegationSignerPrivateKey);
         // filter inputs, since this will fail when the staker is already registered as an operator
         cheats.assume(staker != defaultOperator);
 

Original file line number	Diff line number	Diff line change
`@@ -53,7 +53,8 @@`
`53`	`53`	`},`
`54`	`54`	`"delegation":`
`55`	`55`	`{`
`56`		`- "init_paused_status": 0`
	`56`	`+ "init_paused_status": 0,`
	`57`	`+ "init_withdrawal_delay_blocks": 50400`
`57`	`58`	`},`
`58`	`59`	`"ethPOSDepositAddress": "0x00000000219ab540356cBB839Cbe05303d7705Fa"`
`59`	`60`	`}`