diff --git a/contracts/savings/Savings.sol b/contracts/savings/Savings.sol index 3f9a68e..38db003 100644 --- a/contracts/savings/Savings.sol +++ b/contracts/savings/Savings.sol @@ -95,6 +95,7 @@ contract Savings is BaseSavings, SavingsEIP3009 { function initializeStoredAssets() external restricted reinitializer(2) { if (block.timestamp - lastUpdate > MAX_STORED_ASSETS_INIT_STALENESS) revert StaleAccrual(); storedAssets = IERC20Metadata(asset()).balanceOf(address(this)); + lastUpdate = uint40(block.timestamp); } /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -115,6 +116,12 @@ contract Savings is BaseSavings, SavingsEIP3009 { _; } + /// @notice Reverts when shares exist but `storedAssets` was never seeded (non-atomic upgrade) + modifier onlyInitialized() { + if (storedAssets == 0 && totalSupply() != 0) revert NotInitialized(); + _; + } + /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// CONTRACT LOGIC //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ @@ -124,6 +131,10 @@ contract Savings is BaseSavings, SavingsEIP3009 { /// can never inflate the accrual base. function _accrue() internal returns (uint256 newTotalAssets) { uint256 currentBalance = storedAssets; + if (paused > 0) { + lastUpdate = uint40(block.timestamp); + return currentBalance; + } newTotalAssets = _computeUpdatedAssets(currentBalance, block.timestamp - lastUpdate); lastUpdate = uint40(block.timestamp); uint256 earned = newTotalAssets - currentBalance; @@ -156,6 +167,7 @@ contract Savings is BaseSavings, SavingsEIP3009 { /// @dev Returns the projection of `storedAssets` rather than `IERC20.balanceOf(self)`. /// Direct ERC20 transfers to this contract do not affect this value. function totalAssets() public view override returns (uint256) { + if (paused > 0) return storedAssets; return _computeUpdatedAssets(storedAssets, block.timestamp - lastUpdate); } @@ -184,15 +196,37 @@ contract Savings is BaseSavings, SavingsEIP3009 { //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ /// @inheritdoc ERC4626Upgradeable - function deposit(uint256 assets, address receiver) public override whenNotPaused returns (uint256 shares) { + function deposit( + uint256 assets, + address receiver + ) + public + override + whenNotPaused + onlyInitialized + returns (uint256 shares) + { uint256 newTotalAssets = _accrue(); + uint256 maxAssets = maxDeposit(receiver); + if (assets > maxAssets) revert ERC4626ExceededMaxDeposit(receiver, assets, maxAssets); shares = _convertToShares(assets, newTotalAssets, Math.Rounding.Floor); _deposit(_msgSender(), receiver, assets, shares); } /// @inheritdoc ERC4626Upgradeable - function mint(uint256 shares, address receiver) public override whenNotPaused returns (uint256 assets) { + function mint( + uint256 shares, + address receiver + ) + public + override + whenNotPaused + onlyInitialized + returns (uint256 assets) + { uint256 newTotalAssets = _accrue(); + uint256 maxShares = maxMint(receiver); + if (shares > maxShares) revert ERC4626ExceededMaxMint(receiver, shares, maxShares); assets = _convertToAssets(shares, newTotalAssets, Math.Rounding.Ceil); _deposit(_msgSender(), receiver, assets, shares); } @@ -206,9 +240,12 @@ contract Savings is BaseSavings, SavingsEIP3009 { public override whenNotPaused + onlyInitialized returns (uint256 shares) { uint256 newTotalAssets = _accrue(); + uint256 maxAssets = maxWithdraw(owner); + if (assets > maxAssets) revert ERC4626ExceededMaxWithdraw(owner, assets, maxAssets); shares = _convertToShares(assets, newTotalAssets, Math.Rounding.Ceil); _withdraw(_msgSender(), receiver, owner, assets, shares); } @@ -222,9 +259,12 @@ contract Savings is BaseSavings, SavingsEIP3009 { public override whenNotPaused + onlyInitialized returns (uint256 assets) { uint256 newTotalAssets = _accrue(); + uint256 maxShares = maxRedeem(owner); + if (shares > maxShares) revert ERC4626ExceededMaxRedeem(owner, shares, maxShares); assets = _convertToAssets(shares, newTotalAssets, Math.Rounding.Floor); _withdraw(_msgSender(), receiver, owner, assets, shares); } @@ -253,6 +293,7 @@ contract Savings is BaseSavings, SavingsEIP3009 { ) external whenNotPaused + onlyInitialized returns (uint256 shares) { _consumeDepositAuthorization( @@ -260,6 +301,7 @@ contract Savings is BaseSavings, SavingsEIP3009 { ); uint256 newTotalAssets = _accrue(); + if (assets > maxDeposit(receiver)) revert ERC4626ExceededMaxDeposit(receiver, assets, maxDeposit(receiver)); shares = _convertToShares(assets, newTotalAssets, Math.Rounding.Floor); IEIP3009(asset()) .receiveWithAuthorization(owner, address(this), assets, validAfter, validBefore, nonce, tokenSignature); @@ -339,10 +381,13 @@ contract Savings is BaseSavings, SavingsEIP3009 { bytes memory signature ) internal + onlyInitialized returns (uint256 assets) { _consumeRedeemAuthorization(address(this), owner, receiver, shares, validAfter, validBefore, nonce, signature); uint256 newTotalAssets = _accrue(); + uint256 maxShares = maxRedeem(owner); + if (shares > maxShares) revert ERC4626ExceededMaxRedeem(owner, shares, maxShares); assets = _convertToAssets(shares, newTotalAssets, Math.Rounding.Floor); storedAssets -= assets; _burn(owner, shares); diff --git a/contracts/utils/Errors.sol b/contracts/utils/Errors.sol index 943a021..6ee3525 100644 --- a/contracts/utils/Errors.sol +++ b/contracts/utils/Errors.sol @@ -59,3 +59,4 @@ error SlippageTooHigh(); error InsufficientFunds(); error Undercollateralized(); error StaleAccrual(); +error NotInitialized(); diff --git a/docs/audits/savings-fix/Bailsec - Parallel Protocol - Savings Fix - 1st Report.pdf b/docs/audits/savings-fix/Bailsec - Parallel Protocol - Savings Fix - 1st Report.pdf new file mode 100644 index 0000000..66b673d Binary files /dev/null and b/docs/audits/savings-fix/Bailsec - Parallel Protocol - Savings Fix - 1st Report.pdf differ diff --git a/docs/audits/savings-fix/Bailsec - Parallel Protocol - Savings Fix - Final Report.pdf b/docs/audits/savings-fix/Bailsec - Parallel Protocol - Savings Fix - Final Report.pdf new file mode 100644 index 0000000..b179f2c Binary files /dev/null and b/docs/audits/savings-fix/Bailsec - Parallel Protocol - Savings Fix - Final Report.pdf differ diff --git a/scripts/SetSavingRoles.s.sol b/scripts/SetSavingRoles.s.sol index fd59f1d..185068c 100644 --- a/scripts/SetSavingRoles.s.sol +++ b/scripts/SetSavingRoles.s.sol @@ -21,11 +21,12 @@ contract SetSavingRoles is BaseScript { keeperSelectors[0] = Savings.setRate.selector; accessManager.setTargetFunctionRole(saving, keeperSelectors, Roles.KEEPER_ROLE); - bytes4[] memory governorSelectors = new bytes4[](4); + bytes4[] memory governorSelectors = new bytes4[](5); governorSelectors[0] = SavingsNameable.setNameAndSymbol.selector; governorSelectors[1] = Savings.setMaxRate.selector; governorSelectors[2] = UUPSUpgradeable.upgradeToAndCall.selector; governorSelectors[3] = Savings.recoverSurplus.selector; + governorSelectors[4] = Savings.initializeStoredAssets.selector; accessManager.setTargetFunctionRole(saving, governorSelectors, Roles.GOVERNOR_ROLE); accessManager.grantRole(Roles.USDp_MINTER_ROLE, address(saving), 0); diff --git a/tests/fuzz/Savings.t.sol b/tests/fuzz/Savings.t.sol index c23e20a..bf5135b 100644 --- a/tests/fuzz/Savings.t.sol +++ b/tests/fuzz/Savings.t.sol @@ -4,6 +4,7 @@ pragma solidity 0.8.28; import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import { IERC20Errors } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import { ERC4626Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC4626Upgradeable.sol"; import { IAccessManaged } from "contracts/utils/AccessManagedUpgradeable.sol"; import { EIP3009 } from "contracts/savings/EIP3009.sol"; import { Savings } from "contracts/savings/Savings.sol"; @@ -690,7 +691,11 @@ contract SavingsTest is Fixture, FunctionUtils { ); vm.startPrank(alice); - vm.expectRevert(abi.encodeWithSelector(IERC20Errors.ERC20InsufficientBalance.selector, alice, shares, shares + 1)); + vm.expectRevert( + abi.encodeWithSelector( + ERC4626Upgradeable.ERC4626ExceededMaxWithdraw.selector, alice, withdrawableAmount + 1, withdrawableAmount + ) + ); saving.withdraw(withdrawableAmount + 1, receiver, alice); uint256 sharesBurnt = saving.withdraw(withdrawableAmount, receiver, alice); vm.stopPrank(); @@ -1020,6 +1025,22 @@ contract SavingsTest is Fixture, FunctionUtils { saving.redeemWithAuthorization(redeemShares, bob, alice, 0, deadline, nonce, v, r, s); } + function test_RedeemWithAuthorization_RevertWhen_ExceedsMaxRedeem() public { + _deposit(100 * BASE_18, alice, alice, 0); + uint256 shares = saving.balanceOf(alice); + uint256 tooMany = shares + 1; + + bytes32 nonce = bytes32("redeem_max"); + uint256 deadline = block.timestamp + 1 hours; + (uint8 v, bytes32 r, bytes32 s) = _signRedeemAuth(1, alice, alice, tooMany, 0, deadline, nonce); + + vm.prank(bob); + vm.expectRevert( + abi.encodeWithSelector(ERC4626Upgradeable.ERC4626ExceededMaxRedeem.selector, alice, tooMany, shares) + ); + saving.redeemWithAuthorization(tooMany, alice, alice, 0, deadline, nonce, v, r, s); + } + function test_TransferWithAuthorization_Savings() public { _deposit(100 * BASE_18, alice, alice, 0); uint256 shares = saving.balanceOf(alice); diff --git a/tests/units/Savings.t.sol b/tests/units/Savings.t.sol index 06a4ea5..d8f4297 100644 --- a/tests/units/Savings.t.sol +++ b/tests/units/Savings.t.sol @@ -154,6 +154,37 @@ contract SavingsUpgradeTest is Fixture { assertEq(saving.storedAssets(), IERC20(address(tokenP)).balanceOf(address(saving))); } + function test_initializeStoredAssets_reAnchorsLastUpdate() public { + skip(20 minutes); + vm.prank(governor); + saving.initializeStoredAssets(); + assertEq(saving.lastUpdate(), block.timestamp); + } + + function test_nonAtomicUpgrade_revertsEntryPointsUntilInitialized() public { + SavingsNameable savingProxy = _deployLegacySavings(); + _depositInSavings(savingProxy, 100e18, alice); + + SavingsNameable newImpl = new SavingsNameable(); + vm.prank(governor); + UUPSUpgradeable(address(savingProxy)).upgradeToAndCall(address(newImpl), ""); + + assertEq(savingProxy.storedAssets(), 0, "storedAssets left unseeded by non-atomic upgrade"); + assertGt(savingProxy.totalSupply(), 0); + + deal(address(tokenP), alice, 1e18); + vm.startPrank(alice); + IERC20(address(tokenP)).approve(address(savingProxy), 1e18); + vm.expectRevert(Errors.NotInitialized.selector); + savingProxy.deposit(1e18, alice); + vm.stopPrank(); + + uint256 aliceShares = savingProxy.balanceOf(alice); + vm.prank(alice); + vm.expectRevert(Errors.NotInitialized.selector); + savingProxy.redeem(aliceShares, alice, alice); + } + function test_upgradeFromLegacy_postUpgradePauseUnpauseWorks() public { SavingsNameable savingProxy = _deployLegacySavings(); _upgradeSavings(savingProxy); @@ -633,6 +664,48 @@ contract SavingsTogglePauseAccrueTest is Fixture { assertEq(saving.totalAssets(), expectedAtPause, "yield must be settled at pause time"); } + function test_totalAssets_doesNotProjectYieldWhilePaused() public { + vm.prank(guardian); + saving.pause(); + uint256 storedAtPause = saving.storedAssets(); + + skip(30 days); + + assertEq(saving.totalAssets(), storedAtPause, "totalAssets must not project yield while paused"); + assertEq(saving.totalAssets(), saving.storedAssets()); + } + + function test_setRate_whilePaused_doesNotMintPausedYield() public { + vm.prank(guardian); + saving.pause(); + uint256 storedAtPause = saving.storedAssets(); + uint256 balAtPause = IERC20(address(tokenP)).balanceOf(address(saving)); + + skip(30 days); + + vm.prank(guardian); + saving.setRate(_rate); + + assertEq(saving.storedAssets(), storedAtPause, "no paused yield minted into storedAssets"); + assertEq(IERC20(address(tokenP)).balanceOf(address(saving)), balAtPause, "no tokenP minted while paused"); + assertEq(saving.lastUpdate(), block.timestamp, "lastUpdate advanced to now"); + } + + function test_setMaxRate_whilePaused_doesNotMintPausedYield() public { + vm.prank(guardian); + saving.pause(); + uint256 storedAtPause = saving.storedAssets(); + uint256 balAtPause = IERC20(address(tokenP)).balanceOf(address(saving)); + + skip(30 days); + + vm.prank(governor); + saving.setMaxRate(_maxRate); + + assertEq(saving.storedAssets(), storedAtPause, "no paused yield minted into storedAssets"); + assertEq(IERC20(address(tokenP)).balanceOf(address(saving)), balAtPause, "no tokenP minted while paused"); + } + function test_unpause_dropsPausedWindowYield() public { vm.prank(guardian); saving.pause(); diff --git a/tests/utils/ConfigAccessManager.sol b/tests/utils/ConfigAccessManager.sol index 8c9d854..d1f85e7 100644 --- a/tests/utils/ConfigAccessManager.sol +++ b/tests/utils/ConfigAccessManager.sol @@ -51,11 +51,12 @@ abstract contract ConfigAccessManager is Helper { } function getGovernorSavingsSelectorAccess() internal pure returns (bytes4[] memory) { - bytes4[] memory selectors = new bytes4[](4); + bytes4[] memory selectors = new bytes4[](5); selectors[0] = SavingsNameable.setNameAndSymbol.selector; selectors[1] = Savings.setMaxRate.selector; selectors[2] = UUPSUpgradeable.upgradeToAndCall.selector; selectors[3] = Savings.recoverSurplus.selector; + selectors[4] = Savings.initializeStoredAssets.selector; return selectors; }