Cyfrin Savings Fix — audit remediation (L-1, L-2, I-3, I-4)

contracts/savings/Savings.sol

@@ -85,13 +85,15 @@ contract Savings is BaseSavings, SavingsEIP3009 {
     __AccessManaged_init(_authority);
     _setNameAndSymbol(name_, symbol_);
     _deposit(msg.sender, address(this), 10 ** (asset_.decimals()) / divizer, BASE_18 / divizer);
+    lastUpdate = uint40(block.timestamp);
   }
 
   /// @notice One-shot reinitializer for upgrades that introduce `storedAssets`
   /// @dev Seeds `storedAssets` with the current ERC20 balance held by the contract, so existing
   /// legitimately deposited assets remain backing. Any subsequent direct transfer is treated as a
   /// donation surplus and ignored by `totalAssets()` until `recoverSurplus` is called.
-  function initializeStoredAssets() external reinitializer(2) {
+  function initializeStoredAssets() external restricted reinitializer(2) {
+    if (block.timestamp - lastUpdate > MAX_STORED_ASSETS_INIT_STALENESS) revert StaleAccrual();
     storedAssets = IERC20Metadata(asset()).balanceOf(address(this));
   }
 
@@ -462,12 +464,12 @@ contract Savings is BaseSavings, SavingsEIP3009 {
   }
 
   /// @notice Unpauses the contract
-  /// @dev Reverts if not paused, so a no-op governance call cannot pass silently. Accrues
-  /// outstanding yield before flipping the flag so `lastUpdate` advances to the unpause timestamp,
-  /// eliminating the stale gap that would otherwise persist until the first interaction.
+  /// @dev Reverts if not paused, so a no-op governance call cannot pass silently. Advances
+  /// `lastUpdate` to the unpause timestamp so the paused interval is dropped rather than minted:
+  /// pausing halts emission. `pause()` already settles yield up to the pause moment.
   function unpause() external restricted {
     if (paused == 0) revert NotPaused();
-    _accrue();
+    lastUpdate = uint40(block.timestamp);
     paused = 0;
     emit ToggledPause(0);
   }

contracts/utils/Constants.sol

@@ -39,6 +39,7 @@ uint256 constant MAX_BURN_FEE = 999_000_000;
 uint256 constant MAX_MINT_FEE = BASE_12 - 1;
 uint256 constant MAX_PAYEES = 10;
 uint256 constant SECONDS_PER_YEAR = 365 days;
+uint256 constant MAX_STORED_ASSETS_INIT_STALENESS = 30 minutes;
 
 /*//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
                                                      REENTRANT                                                      

contracts/utils/Errors.sol

@@ -58,3 +58,4 @@ error SwapError();
 error SlippageTooHigh();
 error InsufficientFunds();
 error Undercollateralized();
+error StaleAccrual();

tests/units/Savings.t.sol

@@ -40,7 +40,7 @@ contract SavingsUpgradeTest is Fixture {
     assertEq(saving.totalSupply(), Constants.BASE_18);
     assertEq(saving.maxRate(), 0);
     assertEq(saving.paused(), 0);
-    assertEq(saving.lastUpdate(), 0);
+    assertEq(saving.lastUpdate(), block.timestamp);
     assertEq(saving.rate(), 0);
   }
 
@@ -64,7 +64,7 @@ contract SavingsUpgradeTest is Fixture {
     assertEq(saving.totalSupply(), Constants.BASE_18);
     assertEq(saving.maxRate(), 0);
     assertEq(saving.paused(), 0);
-    assertEq(saving.lastUpdate(), 0);
+    assertEq(saving.lastUpdate(), block.timestamp);
     assertEq(saving.rate(), 0);
   }
 
@@ -129,10 +129,31 @@ contract SavingsUpgradeTest is Fixture {
     SavingsNameable savingProxy = _deployLegacySavings();
     _upgradeSavings(savingProxy);
 
+    vm.prank(governor);
     vm.expectRevert(bytes4(keccak256("InvalidInitialization()")));
     savingProxy.initializeStoredAssets();
   }
 
+  function test_initializeStoredAssets_RevertWhen_CallerUnauthorized() public {
+    vm.prank(alice);
+    vm.expectRevert(abi.encodeWithSelector(IAccessManaged.AccessManagedUnauthorized.selector, alice));
+    saving.initializeStoredAssets();
+  }
+
+  function test_initializeStoredAssets_RevertWhen_AccrualStale() public {
+    skip(30 minutes + 1);
+    vm.prank(governor);
+    vm.expectRevert(Errors.StaleAccrual.selector);
+    saving.initializeStoredAssets();
+  }
+
+  function test_initializeStoredAssets_SucceedsWithinFreshnessWindow() public {
+    skip(30 minutes);
+    vm.prank(governor);
+    saving.initializeStoredAssets();
+    assertEq(saving.storedAssets(), IERC20(address(tokenP)).balanceOf(address(saving)));
+  }
+
   function test_upgradeFromLegacy_postUpgradePauseUnpauseWorks() public {
     SavingsNameable savingProxy = _deployLegacySavings();
     _upgradeSavings(savingProxy);
@@ -612,23 +633,21 @@ contract SavingsTogglePauseAccrueTest is Fixture {
     assertEq(saving.totalAssets(), expectedAtPause, "yield must be settled at pause time");
   }
 
-  function test_unpause_advancesLastUpdateOnUnpause() public {
+  function test_unpause_dropsPausedWindowYield() public {
     vm.prank(guardian);
     saving.pause();
     uint40 lastUpdateAtPause = saving.lastUpdate();
     uint256 assetsAtPause = saving.totalAssets();
 
     skip(7 days);
 
-    uint256 expectedAfterPauseWindow = saving.computeUpdatedAssets(assetsAtPause, 7 days);
-
     vm.prank(guardian);
     saving.unpause();
 
     assertEq(saving.paused(), 0);
     assertEq(saving.lastUpdate(), block.timestamp, "lastUpdate must advance to the unpause timestamp");
     assertGt(saving.lastUpdate(), lastUpdateAtPause);
-    assertEq(saving.totalAssets(), expectedAfterPauseWindow, "unpause must settle pause-window yield in one shot");
+    assertEq(saving.totalAssets(), assetsAtPause, "pausing halts emission: the paused window is not minted");
   }
 
   function test_unpause_firstInteractionAfterUnpauseEarnsNoStaleYield() public {