Merge pull request #285 from morpho-org/revert/refactor/remove-expira…

…tion

Revert "Merge pull request #259 from morpho-org/refactor/remove-expir…

README.md

@@ -30,7 +30,7 @@ For more information about this use case, see the [Rewards](#rewards) section.
 
 All actions that may be against users' interests (e.g. enabling a market with a high exposure, increasing the fee) are subject to a timelock of minimum 12 hours.
 If set, the `guardian` can revoke the action during the timelock except for the fee increase.
-After the timelock, the action can be executed by anyone.
+After the timelock, the action can be executed by anyone until 3 days have passed.
 
 ### Roles
 
@@ -95,9 +95,9 @@ Below is a typical example of how this use case would take place:
 
 - Transfer rewards from the vault to the rewards distributor using the `transferRewards` function.
 
-	NB: Anyone can transfer rewards from the vault to the rewards distributor unless it is unset.
+	NB: Anyone can transfer rewards from the vault to the rewards distributor unless it is unset. 
 	Thus, this step might be already performed by some third-party.
-    Note: the amount of rewards transferred is calculated based on the balance in the reward asset of the vault.
+    Note: the amount of rewards transferred is calculated based on the balance in the reward asset of the vault. 
 	In case the reward asset is the vault’s asset, the vault’s idle liquidity is automatically subtracted to prevent stealing idle liquidity.
 
 - Compute the new root for the vault’s rewards distributor, submit it, wait for the timelock (if any), accept the root, and let vault depositors claim their rewards according to the vault manager’s rewards re-distribution strategy.

src/MetaMorpho.sol

@@ -151,10 +151,14 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph
     /// @dev Makes sure conditions are met to accept a pending value.
     /// @dev Reverts if:
     /// - there's no pending value;
-    /// - the timelock has not elapsed since the pending value has been submitted.
-    modifier afterTimelock(uint256 submittedAt) {
+    /// - the timelock has not elapsed since the pending value has been submitted;
+    /// - the timelock has expired since the pending value has been submitted.
+    modifier withinTimelockWindow(uint256 submittedAt) {
         if (submittedAt == 0) revert ErrorsLib.NoPendingValue();
         if (block.timestamp < submittedAt + timelock) revert ErrorsLib.TimelockNotElapsed();
+        if (block.timestamp > submittedAt + timelock + ConstantsLib.TIMELOCK_EXPIRATION) {
+            revert ErrorsLib.TimelockExpirationExceeded();
+        }
 
         _;
     }
@@ -428,22 +432,22 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph
     }
 
     /// @notice Accepts the `pendingTimelock`.
-    function acceptTimelock() external afterTimelock(pendingTimelock.submittedAt) {
+    function acceptTimelock() external withinTimelockWindow(pendingTimelock.submittedAt) {
         _setTimelock(pendingTimelock.value);
     }
 
     /// @notice Accepts the `pendingFee`.
-    function acceptFee() external afterTimelock(pendingFee.submittedAt) {
+    function acceptFee() external withinTimelockWindow(pendingFee.submittedAt) {
         _setFee(pendingFee.value);
     }
 
     /// @notice Accepts the `pendingGuardian`.
-    function acceptGuardian() external afterTimelock(pendingGuardian.submittedAt) {
+    function acceptGuardian() external withinTimelockWindow(pendingGuardian.submittedAt) {
         _setGuardian(pendingGuardian.value);
     }
 
     /// @notice Accepts the pending cap of the market defined by `id`.
-    function acceptCap(Id id) external afterTimelock(pendingCap[id].submittedAt) {
+    function acceptCap(Id id) external withinTimelockWindow(pendingCap[id].submittedAt) {
         _setCap(id, pendingCap[id].value);
     }
 

src/libraries/ConstantsLib.sol

@@ -6,6 +6,10 @@ pragma solidity ^0.8.0;
 /// @custom:contact security@morpho.org
 /// @notice Library exposing constants.
 library ConstantsLib {
+    /// @dev The delay after a timelock ends after which the owner must submit a parameter again.
+    /// It guarantees users that the owner only accepts parameters submitted recently.
+    uint256 internal constant TIMELOCK_EXPIRATION = 3 days;
+
     /// @dev The maximum delay of a timelock.
     uint256 internal constant MAX_TIMELOCK = 2 weeks;
 

src/libraries/ErrorsLib.sol

@@ -60,6 +60,9 @@ library ErrorsLib {
     /// @notice Thrown when the timelock is not elapsed.
     error TimelockNotElapsed();
 
+    /// @notice Thrown when the timelock expiration is exceeded.
+    error TimelockExpirationExceeded();
+
     /// @notice Thrown when too many markets are in the withdraw queue.
     error MaxQueueSizeExceeded();
 

test/forge/TimelockTest.sol

@@ -135,6 +135,19 @@ contract TimelockTest is IntegrationTest {
         vault.acceptTimelock();
     }
 
+    function testAcceptTimelockTimelockExpirationExceeded(uint256 timelock, uint256 elapsed) public {
+        timelock = bound(timelock, ConstantsLib.MIN_TIMELOCK, TIMELOCK - 1);
+        elapsed = bound(elapsed, TIMELOCK + ConstantsLib.TIMELOCK_EXPIRATION + 1, type(uint64).max);
+
+        vm.prank(OWNER);
+        vault.submitTimelock(timelock);
+
+        vm.warp(block.timestamp + elapsed);
+
+        vm.expectRevert(ErrorsLib.TimelockExpirationExceeded.selector);
+        vault.acceptTimelock();
+    }
+
     function testSubmitFeeDecreased(uint256 fee) public {
         fee = bound(fee, 0, FEE - 1);
 
@@ -207,6 +220,19 @@ contract TimelockTest is IntegrationTest {
         vault.acceptFee();
     }
 
+    function testAcceptFeeTimelockExpirationExceeded(uint256 fee, uint256 elapsed) public {
+        fee = bound(fee, FEE + 1, ConstantsLib.MAX_FEE);
+        elapsed = bound(elapsed, TIMELOCK + ConstantsLib.TIMELOCK_EXPIRATION + 1, type(uint64).max);
+
+        vm.prank(OWNER);
+        vault.submitFee(fee);
+
+        vm.warp(block.timestamp + elapsed);
+
+        vm.expectRevert(ErrorsLib.TimelockExpirationExceeded.selector);
+        vault.acceptFee();
+    }
+
     function testSubmitGuardian() public {
         address guardian = makeAddr("Guardian2");
 
@@ -290,6 +316,20 @@ contract TimelockTest is IntegrationTest {
         vault.acceptGuardian();
     }
 
+    function testAcceptGuardianTimelockExpirationExceeded(uint256 elapsed) public {
+        elapsed = bound(elapsed, TIMELOCK + ConstantsLib.TIMELOCK_EXPIRATION + 1, type(uint64).max);
+
+        address guardian = makeAddr("Guardian2");
+
+        vm.prank(OWNER);
+        vault.submitGuardian(guardian);
+
+        vm.warp(block.timestamp + elapsed);
+
+        vm.expectRevert(ErrorsLib.TimelockExpirationExceeded.selector);
+        vault.acceptGuardian();
+    }
+
     function testSubmitCapDecreased(uint256 cap) public {
         cap = bound(cap, 0, CAP - 1);
 
@@ -374,4 +414,22 @@ contract TimelockTest is IntegrationTest {
         vm.expectRevert(ErrorsLib.TimelockNotElapsed.selector);
         vault.acceptCap(allMarkets[1].id());
     }
+
+    function testAcceptCapTimelockExpirationExceeded(uint256 timelock, uint256 elapsed) public {
+        timelock = bound(timelock, ConstantsLib.MIN_TIMELOCK, ConstantsLib.MAX_TIMELOCK);
+
+        vm.assume(timelock != vault.timelock());
+
+        _setTimelock(timelock);
+
+        elapsed = bound(elapsed, timelock + ConstantsLib.TIMELOCK_EXPIRATION + 1, type(uint64).max);
+
+        vm.startPrank(CURATOR);
+        vault.submitCap(allMarkets[1], CAP);
+
+        vm.warp(block.timestamp + elapsed);
+
+        vm.expectRevert(ErrorsLib.TimelockExpirationExceeded.selector);
+        vault.acceptCap(allMarkets[1].id());
+    }
 }

test/metamorpho_tests.tree

@@ -64,9 +64,12 @@
         ├── when block.timestamp < pendingTimelock.submittedAt + timelock
         │   └── revert with TimelockNotElapsed()
         └── when block.timestamp >= pendingTimelock.submittedAt + timelock
-            ├── it should set timelock to pendingTimelock.value
-            ├── it should emit SetTimelock(pendingTimelock.value)
-            └── it should delete pendingTimelock
+            ├── when block.timestamp > pendingTimelock.submittedAt + timelock + TIMELOCK_EXPIRATION
+            │   └── revert with TIMELOCK_EXPIRATION_EXCEEDED
+            └── whenblock.timestamp <= pendingTimelock.submittedAt + timelock + TIMELOCK_EXPIRATION
+                ├── it should set timelock to pendingTimelock.value
+                ├── it should emit SetTimelock(pendingTimelock.value)
+                └── it should delete pendingTimelock
 
 .
 └── submitFee(uint256 newFee) external
@@ -96,10 +99,13 @@
         ├── when block.timestamp < pendingFee.submittedAt + timelock
         │   └── revert with TimelockNotElapsed()
         └── when block.timestamp >= pendingFee.submittedAt + timelock
-            ├── it should accrue fees
-            ├── it should set fee to pendingFee.value
-            ├── it should emit SetFee(pendingFee.value)
-            └── it should delete pendingFee
+            ├── when block.timestamp > pendingFee.submittedAt + timelock + TIMELOCK_EXPIRATION
+            │   └── revert with TIMELOCK_EXPIRATION_EXCEEDED
+            └── whenblock.timestamp <= pendingFee.submittedAt + timelock + TIMELOCK_EXPIRATION
+                ├── it should accrue fees
+                ├── it should set fee to pendingFee.value
+                ├── it should emit SetFee(pendingFee.value)
+                └── it should delete pendingFee
 
 .
 └── setFeeRecipient(address newFeeRecipient) external
@@ -140,9 +146,12 @@
         ├── when block.timestamp < pendingGuardian.submittedAt + timelock
         │   └── revert with TimelockNotElapsed()
         └── when block.timestamp >= pendingGuardian.submittedAt + timelock
-            ├── it should set guardian to pendingGuardian
-            ├── it should emit SetGuardian(pendingGuardian)
-            └── it should delete pendingGuardian
+            ├── when block.timestamp > pendingGuardian.submittedAt + timelock + TIMELOCK_EXPIRATION
+            │   └── revert with TIMELOCK_EXPIRATION_EXCEEDED
+            └── whenblock.timestamp <= pendingGuardian.submittedAt + timelock + TIMELOCK_EXPIRATION
+                ├── it should set guardian to pendingGuardian
+                ├── it should emit SetGuardian(pendingGuardian)
+                └── it should delete pendingGuardian
 
 
 /* ONLY CURATOR FUNCTIONS */
@@ -184,16 +193,19 @@
         ├── when block.timestamp < pendingCap[id].submittedAt + timelock
         │   └── revert with TimelockNotElapsed()
         └── when block.timestamp >= pendingCap[id].submittedAt + timelock
-            ├── when supplyCap > 0 and marketConfig.withdrawRank == 0
-            │   ├── it should push id to supplyQueue
-            │   ├── it should push id to withdrawQueue
-            │   ├── if supplyQueue.length > MAX_QUEUE_SIZE
-            │   │   └── revert with MaxQueueSizeExceeded()
-            │   └── if withdrawQueue.length > MAX_QUEUE_SIZE
-            │       └── revert with MaxQueueSizeExceeded()
-            ├── it should set config[id].cap to pendingCap[id].value
-            ├── it should emit SetCap(id, pendingCap[id].value)
-            └── it should delete pendingCap[id]
+            ├── when block.timestamp > pendingCap[id].submittedAt + timelock + TIMELOCK_EXPIRATION
+            │   └── revert with TIMELOCK_EXPIRATION_EXCEEDED
+            └── whenblock.timestamp <= pendingCap[id].submittedAt + timelock + TIMELOCK_EXPIRATION
+                ├── when supplyCap > 0 and marketConfig.withdrawRank == 0
+                │   ├── it should push id to supplyQueue
+                │   ├── it should push id to withdrawQueue
+                │   ├── if supplyQueue.length > MAX_QUEUE_SIZE
+                │   │   └── revert with MaxQueueSizeExceeded()
+                │   └── if withdrawQueue.length > MAX_QUEUE_SIZE
+                │       └── revert with MaxQueueSizeExceeded()
+                ├── it should set config[id].cap to pendingCap[id].value
+                ├── it should emit SetCap(id, pendingCap[id].value)
+                └── it should delete pendingCap[id]
 
 
 /* ONLY ALLOCATOR FUNCTIONS */