feat: use setMinAssetsLockDuration on escrow initialize

lidofinance · Feb 4, 2025 · 30015d3 · 30015d3
1 parent adf77b2
commit 30015d3
Show file tree

Hide file tree

Showing 6 changed files with 67 additions and 21 deletions.
diff --git a/contracts/Escrow.sol b/contracts/Escrow.sol
@@ -140,7 +140,7 @@ contract Escrow is ISignallingEscrow, IRageQuitEscrow {
         }
         _checkCallerIsDualGovernance();
 
-        _escrowState.initialize(minAssetsLockDuration);
+        _escrowState.initialize(minAssetsLockDuration, MAX_MIN_ASSETS_LOCK_DURATION);
 
         ST_ETH.approve(address(WST_ETH), type(uint256).max);
         ST_ETH.approve(address(WITHDRAWAL_QUEUE), type(uint256).max);
@@ -310,6 +310,7 @@ contract Escrow is ISignallingEscrow, IRageQuitEscrow {
     /// @param newMinAssetsLockDuration The new minimum lock duration to be set.
     function setMinAssetsLockDuration(Duration newMinAssetsLockDuration) external {
         _checkCallerIsDualGovernance();
+        _escrowState.checkSignallingEscrow();
         _escrowState.setMinAssetsLockDuration(newMinAssetsLockDuration, MAX_MIN_ASSETS_LOCK_DURATION);
     }
 

diff --git a/contracts/libraries/EscrowState.sol b/contracts/libraries/EscrowState.sol
@@ -71,11 +71,16 @@ library EscrowState {
 
     /// @notice Initializes the Escrow state to SignallingEscrow.
     /// @param self The context of the Escrow State library.
-    /// @param minAssetsLockDuration The minimum assets lock duration.
-    function initialize(Context storage self, Duration minAssetsLockDuration) internal {
+    /// @param minAssetsLockDuration The initial minimum assets lock duration.
+    /// @param maxMinAssetsLockDuration Sanity check upper bound for min assets lock duration.
+    function initialize(
+        Context storage self,
+        Duration minAssetsLockDuration,
+        Duration maxMinAssetsLockDuration
+    ) internal {
         _checkState(self, State.NotInitialized);
         _setState(self, State.SignallingEscrow);
-        _setMinAssetsLockDuration(self, minAssetsLockDuration);
+        setMinAssetsLockDuration(self, minAssetsLockDuration, maxMinAssetsLockDuration);
     }
 
     /// @notice Starts the rage quit process.
@@ -116,7 +121,8 @@ library EscrowState {
         ) {
             revert InvalidMinAssetsLockDuration(newMinAssetsLockDuration);
         }
-        _setMinAssetsLockDuration(self, newMinAssetsLockDuration);
+        self.minAssetsLockDuration = newMinAssetsLockDuration;
+        emit MinAssetsLockDurationSet(newMinAssetsLockDuration);
     }
 
     // ---
@@ -205,12 +211,4 @@ library EscrowState {
         self.state = newState;
         emit EscrowStateChanged(prevState, newState);
     }
-
-    /// @notice Sets the minimum assets lock duration.
-    /// @param self The context of the Escrow State library.
-    /// @param newMinAssetsLockDuration The new minimum assets lock duration.
-    function _setMinAssetsLockDuration(Context storage self, Duration newMinAssetsLockDuration) private {
-        self.minAssetsLockDuration = newMinAssetsLockDuration;
-        emit MinAssetsLockDurationSet(newMinAssetsLockDuration);
-    }
 }
diff --git a/docs/specification.md b/docs/specification.md
@@ -849,6 +849,8 @@ The `Escrow` instance is intended to be used behind [minimal proxy contracts](ht
 - MUST be called using the proxy contract.
 - MUST be called by the `DualGovernance` contract.
 - MUST NOT have been initialized previously.
+- `minAssetsLockDuration` MUST NOT exceed `Escrow.MAX_MIN_ASSETS_LOCK_DURATION`.
+- `minAssetsLockDuration` MUST NOT be equal to previous value (by default `0`).
 
 ---
 

diff --git a/test/unit/Escrow.t.sol b/test/unit/Escrow.t.sol
@@ -135,20 +135,23 @@ contract EscrowUnitTests is UnitTest {
     // initialize()
     // ---
 
-    function test_initialize_HappyPath() external {
+    function testFuzz_initialize_HappyPath(Duration minAssetLockDuration) external {
+        vm.assume(minAssetLockDuration > Durations.ZERO);
+        vm.assume(minAssetLockDuration <= _maxMinAssetsLockDuration);
+
         vm.expectEmit();
         emit EscrowStateLib.EscrowStateChanged(EscrowState.NotInitialized, EscrowState.SignallingEscrow);
         vm.expectEmit();
-        emit EscrowStateLib.MinAssetsLockDurationSet(Durations.ZERO);
+        emit EscrowStateLib.MinAssetsLockDurationSet(minAssetLockDuration);
 
         vm.expectCall(address(_stETH), abi.encodeCall(IERC20.approve, (address(_wstETH), type(uint256).max)));
         vm.expectCall(address(_stETH), abi.encodeCall(IERC20.approve, (address(_withdrawalQueue), type(uint256).max)));
 
         Escrow escrowInstance =
-            _createInitializedEscrowProxy({minWithdrawalsBatchSize: 100, minAssetsLockDuration: Durations.ZERO});
+            _createInitializedEscrowProxy({minWithdrawalsBatchSize: 100, minAssetsLockDuration: minAssetLockDuration});
 
         assertEq(escrowInstance.MIN_WITHDRAWALS_BATCH_SIZE(), 100);
-        assertEq(escrowInstance.getMinAssetsLockDuration(), Durations.ZERO);
+        assertEq(escrowInstance.getMinAssetsLockDuration(), minAssetLockDuration);
         assertTrue(escrowInstance.getEscrowState() == EscrowState.SignallingEscrow);
 
         IEscrow.SignallingEscrowDetails memory signallingEscrowDetails = escrowInstance.getSignallingEscrowDetails();
@@ -1190,6 +1193,18 @@ contract EscrowUnitTests is UnitTest {
         _escrow.setMinAssetsLockDuration(newMinAssetsLockDuration);
     }
 
+    function test_setMinAssetsLockDuration_RevertOn_RageQuitState() external {
+        Duration newMinAssetsLockDuration = Durations.from(1);
+
+        _transitToRageQuit();
+
+        vm.expectRevert(
+            abi.encodeWithSelector(EscrowStateLib.UnexpectedEscrowState.selector, EscrowState.RageQuitEscrow)
+        );
+        vm.prank(_dualGovernance);
+        _escrow.setMinAssetsLockDuration(newMinAssetsLockDuration);
+    }
+
     // ---
     // withdrawETH()
     // ---

diff --git a/test/unit/libraries/EscrowState.t.sol b/test/unit/libraries/EscrowState.t.sol
@@ -17,14 +17,20 @@ contract EscrowStateUnitTests is UnitTest {
     // initialize()
     // ---
 
-    function testFuzz_initialize_happyPath(Duration minAssetsLockDuration) external {
+    function testFuzz_initialize_happyPath(
+        Duration minAssetsLockDuration,
+        Duration maxMinAssetsLockDuration
+    ) external {
+        vm.assume(minAssetsLockDuration > Durations.ZERO);
+        vm.assume(minAssetsLockDuration <= maxMinAssetsLockDuration);
+
         _context.state = State.NotInitialized;
 
         vm.expectEmit();
         emit EscrowState.EscrowStateChanged(State.NotInitialized, State.SignallingEscrow);
         emit EscrowState.MinAssetsLockDurationSet(minAssetsLockDuration);
 
-        EscrowState.initialize(_context, minAssetsLockDuration);
+        EscrowState.initialize(_context, minAssetsLockDuration, maxMinAssetsLockDuration);
 
         checkContext({
             state: State.SignallingEscrow,
@@ -35,12 +41,35 @@ contract EscrowStateUnitTests is UnitTest {
         });
     }
 
-    function testFuzz_initialize_RevertOn_InvalidState(Duration minAssetsLockDuration) external {
+    function testFuzz_initialize_RevertOn_InvalidState(
+        Duration minAssetsLockDuration,
+        Duration maxMinAssetsLockDuration
+    ) external {
+        vm.assume(minAssetsLockDuration <= maxMinAssetsLockDuration);
         _context.state = State.SignallingEscrow;
 
         vm.expectRevert(abi.encodeWithSelector(EscrowState.UnexpectedEscrowState.selector, State.SignallingEscrow));
 
-        EscrowState.initialize(_context, minAssetsLockDuration);
+        EscrowState.initialize(_context, minAssetsLockDuration, maxMinAssetsLockDuration);
+    }
+
+    function testFuzz_initalize_RevertOn_InvalidMinAssetLockDuration_ZeroDuration(Duration maxMinAssetsLockDuration)
+        external
+    {
+        vm.expectRevert(abi.encodeWithSelector(EscrowState.InvalidMinAssetsLockDuration.selector, 0));
+        EscrowState.initialize(_context, Durations.ZERO, maxMinAssetsLockDuration);
+    }
+
+    function testFuzz_initalize_RevertOn_InvalidMinAssetLockDuration_ExceedMaxDuration(
+        Duration maxMinAssetsLockDuration
+    ) external {
+        vm.assume(
+            maxMinAssetsLockDuration > Durations.ZERO
+                && maxMinAssetsLockDuration < Durations.from(MAX_DURATION_VALUE - 1)
+        );
+        Duration duration = maxMinAssetsLockDuration + Durations.from(1);
+        vm.expectRevert(abi.encodeWithSelector(EscrowState.InvalidMinAssetsLockDuration.selector, duration));
+        EscrowState.initialize(_context, duration, maxMinAssetsLockDuration);
     }
 
     // ---

diff --git a/test/utils/SetupDeployment.sol b/test/utils/SetupDeployment.sol
@@ -178,6 +178,7 @@ abstract contract SetupDeployment is Test {
         dgDeployConfig.FIRST_SEAL_RAGE_QUIT_SUPPORT = PercentsD16.fromBasisPoints(3_00); // 3%
         dgDeployConfig.SECOND_SEAL_RAGE_QUIT_SUPPORT = PercentsD16.fromBasisPoints(15_00); // 15%
         dgDeployConfig.MIN_ASSETS_LOCK_DURATION = Durations.from(5 hours);
+        dgDeployConfig.MAX_MIN_ASSETS_LOCK_DURATION = Durations.from(365 days);
         dgDeployConfig.VETO_SIGNALLING_MIN_DURATION = Durations.from(3 days);
         dgDeployConfig.VETO_SIGNALLING_MAX_DURATION = Durations.from(30 days);
         dgDeployConfig.VETO_SIGNALLING_MIN_ACTIVE_DURATION = Durations.from(5 hours);