diff --git a/contracts/savings/Savings.sol b/contracts/savings/Savings.sol index fc9d3be..3f9a68e 100644 --- a/contracts/savings/Savings.sol +++ b/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); } diff --git a/contracts/utils/Constants.sol b/contracts/utils/Constants.sol index 1ffe253..c9a09bf 100644 --- a/contracts/utils/Constants.sol +++ b/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 diff --git a/contracts/utils/Errors.sol b/contracts/utils/Errors.sol index 6680d72..943a021 100644 --- a/contracts/utils/Errors.sol +++ b/contracts/utils/Errors.sol @@ -58,3 +58,4 @@ error SwapError(); error SlippageTooHigh(); error InsufficientFunds(); error Undercollateralized(); +error StaleAccrual(); diff --git a/docs/audits/savings-fix/Cyfrin - Parallel Protocol - Savings Fix - 1st Report.pdf b/docs/audits/savings-fix/Cyfrin - Parallel Protocol - Savings Fix - 1st Report.pdf new file mode 100644 index 0000000..3867a3c Binary files /dev/null and b/docs/audits/savings-fix/Cyfrin - Parallel Protocol - Savings Fix - 1st Report.pdf differ diff --git a/docs/audits/savings-fix/Cyfrin - Parallel Protocol - Savings Fix - Final Report.pdf b/docs/audits/savings-fix/Cyfrin - Parallel Protocol - Savings Fix - Final Report.pdf new file mode 100644 index 0000000..09e5b98 Binary files /dev/null and b/docs/audits/savings-fix/Cyfrin - Parallel Protocol - Savings Fix - Final Report.pdf differ diff --git a/tests/units/Savings.t.sol b/tests/units/Savings.t.sol index 2198659..06a4ea5 100644 --- a/tests/units/Savings.t.sol +++ b/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,7 +633,7 @@ 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(); @@ -620,15 +641,13 @@ contract SavingsTogglePauseAccrueTest is Fixture { 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 {