Bailsec Parallel Savings Fix — audit remediation (Issues 01-05, 07)

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,13 +293,15 @@ contract Savings is BaseSavings, SavingsEIP3009 {
   )
     external
     whenNotPaused
+    onlyInitialized
     returns (uint256 shares)
   {
     _consumeDepositAuthorization(
       address(this), owner, receiver, assets, validAfter, validBefore, nonce, savingsSignature
     );
 
     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);

contracts/utils/Errors.sol

@@ -59,3 +59,4 @@ error SlippageTooHigh();
 error InsufficientFunds();
 error Undercollateralized();
 error StaleAccrual();
+error NotInitialized();

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);

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);

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();

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;
   }