From 42a8c7abea0cc2cc9b444ccbc18cc474eb99587b Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Wed, 23 Apr 2025 02:42:47 +0200 Subject: [PATCH 1/2] feat: feature deactivation on demand --- README.md | 4 + scripts/DeployProxy.s.sol | 3 +- specs/feature_activation.md | 100 +++++++++++++ src/core/SSVBasedApps.sol | 11 ++ src/core/interfaces/IProtocolManager.sol | 1 + src/core/interfaces/IStrategyManager.sol | 2 + src/core/libraries/ProtocolStorageLib.sol | 5 + src/core/modules/ProtocolManager.sol | 8 ++ src/core/modules/StrategyManager.sol | 28 ++++ src/middleware/README.md | 11 +- test/SSVBasedApps.t.sol | 6 +- test/helpers/Setup.t.sol | 3 +- test/modules/ProtocolManager.t.sol | 121 ++++++++++++++++ test/modules/StrategyManager.t.sol | 168 ++++++++++++++++++++++ 14 files changed, 461 insertions(+), 10 deletions(-) create mode 100644 specs/feature_activation.md diff --git a/README.md b/README.md index e1b8a1b9..5faf5f0a 100644 --- a/README.md +++ b/README.md @@ -122,6 +122,10 @@ These functions verify balances and authorize the caller to retrieve their accum   +## :gear: _Feature activation_ + +[Feature activation](./specs/feature_activation.md) + ## :page_facing_up: _Whitepaper_ [Whitepaper](https://ssv.network/wp-content/uploads/2025/01/SSV2.0-Based-Applications-Protocol-1.pdf) diff --git a/scripts/DeployProxy.s.sol b/scripts/DeployProxy.s.sol index 8ef0810d..9c680762 100644 --- a/scripts/DeployProxy.s.sol +++ b/scripts/DeployProxy.s.sol @@ -32,7 +32,8 @@ contract DeployProxy is Script { withdrawalExpireTime: 3 days, obligationTimelockPeriod: 14 days, obligationExpireTime: 3 days, - maxFeeIncrement: 500 + maxFeeIncrement: 500, + disabledFeatures: 0 }); bytes memory initData = abi.encodeWithSelector( diff --git a/specs/feature_activation.md b/specs/feature_activation.md new file mode 100644 index 00000000..0c634a96 --- /dev/null +++ b/specs/feature_activation.md @@ -0,0 +1,100 @@ +# Feature Flags in StrategyManager + +## Purpose + +Introduce a compact, on‑chain mechanism to enable or disable selected features of the `StrategyManager` contract without a full redeploy. By packing multiple boolean toggles into a single `uint32`, the design: + +- Minimizes storage footprint and gas cost. +- Provides an upgrade‑friendly switchboard for safety (e.g., emergency pause). +- Centralizes feature management under DAO control. + +## Technical Explanation + +- **Storage**: A `uint32 disabledFeatures` field in `ProtocolStorageLib.Data`. +- **Bitmask Layout**: + - Bit 0 (LSB) → Slashing Disabled (`SLASHING_DISABLED = 1 << 0`) + - Bit 1 → Withdrawals Disabled (`WITHDRAWALS_DISABLED = 1 << 1`) + - Further bits reserved for future toggles. + +- **Checks**: Two internal functions in `StrategyManager`: + ```solidity + function checkSlashingAllowed() internal view { + if (ProtocolStorageLib.load().disabledFeatures & SLASHING_DISABLED != 0) + revert SlashingDisabled(); + } + + function checkWithdrawalsAllowed() internal view { + if (ProtocolStorageLib.load().disabledFeatures & WITHDRAWALS_DISABLED != 0) + revert WithdrawalsDisabled(); + } + ``` + - Called at the entry points of: + - `slash(...)` + - `proposeWithdrawal(...)` + - `finalizeWithdrawal(...)` + - `proposeWithdrawalETH(...)` + - `finalizeWithdrawalETH(...)` + +## Authorized Accounts + +- Only the **DAO (owner)** can update the entire `disabledFeatures` bitmask via: + ```solidity + function updateDisabledFeatures(uint32 disabledFeatures) external onlyOwner; + ``` +- No per-feature granularity: toggles are applied in bulk. + +## Current Features That Can Be Enabled/Disabled + +| Bit Position | Feature | Constant | Description | +|:------------:|:-------------------|:----------------------|:------------------------------------------| +| 0 | Slashing | `SLASHING_DISABLED` | Stops all calls to `slash(...)` | +| 1 | Withdrawals | `WITHDRAWALS_DISABLED`| Stops all withdrawal proposals and finalizations | + +## Usage & Examples + +1. **Disable slashing only**: + ```js + // binary: 0b...01 → 1 + SSVBasedApps.updateDisabledFeatures(1); + // `slash(...)` now reverts with `SlashingDisabled()`. + ``` +2. **Re-enable slashing, disable withdrawals**: + ```js + // binary: 0b...10 → 2 + SSVBasedApps.updateDisabledFeatures(2); + // `slash(...)` resumes; `proposeWithdrawal(...)` and `finalizeWithdrawal(...)` revert. + ``` +3. **Disable both**: + ```js + SSVBasedApps.updateDisabledFeatures(3); + ``` +4. **Full example:** + ```js + // bit-definitions + const SLASHING_DISABLED = 1 << 0; // 0b0001 + const WITHDRAWALS_DISABLED = 1 << 1; // 0b0010 + + // Suppose you want to disable only withdrawals: + let flags = 0; + flags |= WITHDRAWALS_DISABLED; // flags === 0b0010 + + // Later you decide to also disable slashing: + flags |= SLASHING_DISABLED; // flags === 0b0011 + + // To re-enable withdrawals but keep slashing disabled: + flags &= ~WITHDRAWALS_DISABLED; // flags === 0b0001 + + // Finally, send the update on-chain: + await SSVBasedApps.updateDisabledFeatures(flags); + ``` + +## Future Extensions + +- Reserve bits 2–31 for other purposes: + - Feature disabling + - Emergency pause (global) + + +--- +*This document outlines the bitmask‑driven feature gating mechanism for `StrategyManager`. It ensures rapid reaction to on‑chain emergencies and fine‑grained control over critical operations.* + diff --git a/src/core/SSVBasedApps.sol b/src/core/SSVBasedApps.sol index cc2001fd..33656ade 100644 --- a/src/core/SSVBasedApps.sol +++ b/src/core/SSVBasedApps.sol @@ -115,6 +115,7 @@ contract SSVBasedApps is sp.obligationTimelockPeriod = config.obligationTimelockPeriod; sp.obligationExpireTime = config.obligationExpireTime; sp.maxShares = config.maxShares; + sp.disabledFeatures = config.disabledFeatures; emit MaxFeeIncrementSet(sp.maxFeeIncrement); } @@ -331,6 +332,12 @@ contract SSVBasedApps is _delegateTo(SSVCoreModules.SSV_PROTOCOL_MANAGER); } + function updateDisabledFeatures( + uint32 disabledFeatures + ) external onlyOwner { + _delegateTo(SSVCoreModules.SSV_PROTOCOL_MANAGER); + } + // ***************************** // ** Section: External Views ** // ***************************** @@ -533,6 +540,10 @@ contract SSVBasedApps is return ProtocolStorageLib.load().obligationExpireTime; } + function disabledFeatures() external view returns (uint32) { + return ProtocolStorageLib.load().disabledFeatures; + } + function getVersion() external pure returns (string memory) { return "0.0.1"; } diff --git a/src/core/interfaces/IProtocolManager.sol b/src/core/interfaces/IProtocolManager.sol index 7d22b049..c67bf647 100644 --- a/src/core/interfaces/IProtocolManager.sol +++ b/src/core/interfaces/IProtocolManager.sol @@ -10,6 +10,7 @@ interface IProtocolManager { event StrategyMaxSharesUpdated(uint256 maxShares); event WithdrawalExpireTimeUpdated(uint32 withdrawalExpireTime); event WithdrawalTimelockPeriodUpdated(uint32 withdrawalTimelockPeriod); + event DisabledFeaturesUpdated(uint32 disabledFeatures); function updateFeeExpireTime(uint32 value) external; function updateFeeTimelockPeriod(uint32 value) external; diff --git a/src/core/interfaces/IStrategyManager.sol b/src/core/interfaces/IStrategyManager.sol index fe4d770c..d053b563 100644 --- a/src/core/interfaces/IStrategyManager.sol +++ b/src/core/interfaces/IStrategyManager.sol @@ -188,7 +188,9 @@ interface IStrategyManager { error ObligationAlreadySet(); error ObligationHasNotBeenCreated(); error RequestTimeExpired(); + error SlashingDisabled(); error TimelockNotElapsed(); error TokenNotSupportedByBApp(address token); error WithdrawTransferFailed(); + error WithdrawalsDisabled(); } diff --git a/src/core/libraries/ProtocolStorageLib.sol b/src/core/libraries/ProtocolStorageLib.sol index 8fba0bd6..2e38c3c0 100644 --- a/src/core/libraries/ProtocolStorageLib.sol +++ b/src/core/libraries/ProtocolStorageLib.sol @@ -12,6 +12,11 @@ library ProtocolStorageLib { uint32 obligationTimelockPeriod; uint32 obligationExpireTime; uint32 maxFeeIncrement; + // each bit, starting from the LSB, represents a DISABLED feature + // bit 0 = slashingDisabled + // bit 1 = withdrawalsDisabled + // ... + uint32 disabledFeatures; uint256 maxShares; } diff --git a/src/core/modules/ProtocolManager.sol b/src/core/modules/ProtocolManager.sol index db1a8146..e3fbeef3 100644 --- a/src/core/modules/ProtocolManager.sol +++ b/src/core/modules/ProtocolManager.sol @@ -5,6 +5,9 @@ import { IProtocolManager } from "@ssv/src/core/interfaces/IProtocolManager.sol" import { ProtocolStorageLib } from "@ssv/src/core/libraries/ProtocolStorageLib.sol"; contract ProtocolManager is IProtocolManager { + uint32 private constant SLASHING_DISABLED = 1 << 0; + uint32 private constant WITHDRAWALS_DISABLED = 1 << 1; + function updateFeeTimelockPeriod(uint32 feeTimelockPeriod) external { ProtocolStorageLib.load().feeTimelockPeriod = feeTimelockPeriod; emit FeeTimelockPeriodUpdated(feeTimelockPeriod); @@ -52,4 +55,9 @@ contract ProtocolManager is IProtocolManager { ProtocolStorageLib.load().maxFeeIncrement = maxFeeIncrement; emit StrategyMaxFeeIncrementUpdated(maxFeeIncrement); } + + function updateDisabledFeatures(uint32 disabledFeatures) external { + ProtocolStorageLib.load().disabledFeatures = disabledFeatures; + emit DisabledFeaturesUpdated(disabledFeatures); + } } diff --git a/src/core/modules/StrategyManager.sol b/src/core/modules/StrategyManager.sol index a5685030..981360fd 100644 --- a/src/core/modules/StrategyManager.sol +++ b/src/core/modules/StrategyManager.sol @@ -18,6 +18,9 @@ import { IBasedApp } from "@ssv/src/middleware/interfaces/IBasedApp.sol"; contract StrategyManager is ReentrancyGuardTransient, IStrategyManager { using SafeERC20 for IERC20; + uint32 private constant SLASHING_DISABLED = 1 << 0; + uint32 private constant WITHDRAWALS_DISABLED = 1 << 1; + /// @notice Checks if the caller is the strategy owner /// @param strategyId The ID of the strategy /// @param s The CoreStorageLib data @@ -253,6 +256,8 @@ contract StrategyManager is ReentrancyGuardTransient, IStrategyManager { address token, uint256 amount ) external { + checkWithdrawalsAllowed(); + if (token == ETH_ADDRESS) revert InvalidToken(); _proposeWithdrawal(strategyId, token, amount); } @@ -264,6 +269,8 @@ contract StrategyManager is ReentrancyGuardTransient, IStrategyManager { uint32 strategyId, IERC20 token ) external nonReentrant { + checkWithdrawalsAllowed(); + uint256 amount = _finalizeWithdrawal(strategyId, address(token)); token.safeTransfer(msg.sender, amount); @@ -281,12 +288,15 @@ contract StrategyManager is ReentrancyGuardTransient, IStrategyManager { /// @param strategyId The ID of the strategy. /// @param amount The amount of ETH to withdraw. function proposeWithdrawalETH(uint32 strategyId, uint256 amount) external { + checkWithdrawalsAllowed(); _proposeWithdrawal(strategyId, ETH_ADDRESS, amount); } /// @notice Finalize the ETH withdrawal after the timelock period has passed. /// @param strategyId The ID of the strategy. function finalizeWithdrawalETH(uint32 strategyId) external nonReentrant { + checkWithdrawalsAllowed(); + uint256 amount = _finalizeWithdrawal(strategyId, ETH_ADDRESS); payable(msg.sender).transfer(amount); @@ -772,6 +782,7 @@ contract StrategyManager is ReentrancyGuardTransient, IStrategyManager { ) external nonReentrant { if (amount == 0) revert InvalidAmount(); CoreStorageLib.Data storage s = CoreStorageLib.load(); + checkSlashingAllowed(); if (!s.registeredBApps[bApp]) { revert IBasedAppManager.BAppNotRegistered(); @@ -911,4 +922,21 @@ contract StrategyManager is ReentrancyGuardTransient, IStrategyManager { s.slashingFund[msg.sender][token] -= amount; } + + function checkSlashingAllowed() internal view { + if ( + ProtocolStorageLib.load().disabledFeatures & SLASHING_DISABLED != 0 + ) { + revert SlashingDisabled(); + } + } + + function checkWithdrawalsAllowed() internal view { + if ( + ProtocolStorageLib.load().disabledFeatures & WITHDRAWALS_DISABLED != + 0 + ) { + revert WithdrawalsDisabled(); + } + } } diff --git a/src/middleware/README.md b/src/middleware/README.md index 66bbed2a..785a6526 100644 --- a/src/middleware/README.md +++ b/src/middleware/README.md @@ -1,4 +1,4 @@ -# :construction_worker: :closed_lock_with_key: __Middleware Contracts__ +# :construction_worker: :closed_lock_with_key: **Middleware Contracts** ## Modules & Examples for BApp Development @@ -78,11 +78,10 @@ Ideal for BApps that want to avoid excessive reward costs. ## :page_facing_up: To Have: Examples -* **BLS Strategy Example**: Demonstrates how to implement and use the BLS verification module securely. +- **BLS Strategy Example**: Demonstrates how to implement and use the BLS verification module securely. -* **ECDSA Strategy Example**: Showcases a lighter alternative using ECDSA verification. +- **ECDSA Strategy Example**: Showcases a lighter alternative using ECDSA verification. -* **Capped Strategy Example**: Implements a BApp that limits deposits to 100k SSV. - -* **Whitelist Manager Example**: Uses whitelist-based permissions instead of owner-based control. +- **Capped Strategy Example**: Implements a BApp that limits deposits to 100k SSV. +- **Whitelist Manager Example**: Uses whitelist-based permissions instead of owner-based control. diff --git a/test/SSVBasedApps.t.sol b/test/SSVBasedApps.t.sol index 31439204..a1d57371 100644 --- a/test/SSVBasedApps.t.sol +++ b/test/SSVBasedApps.t.sol @@ -163,7 +163,8 @@ contract SSVBasedAppsTest is Setup, Ownable2StepUpgradeable { withdrawalExpireTime: 3 days, obligationTimelockPeriod: 14 days, obligationExpireTime: 3 days, - maxShares: 1e50 + maxShares: 1e50, + disabledFeatures: 0 }); vm.expectRevert( abi.encodeWithSelector( @@ -192,7 +193,8 @@ contract SSVBasedAppsTest is Setup, Ownable2StepUpgradeable { withdrawalExpireTime: 3 days, obligationTimelockPeriod: 14 days, obligationExpireTime: 3 days, - maxFeeIncrement: 10_001 + maxFeeIncrement: 10_001, + disabledFeatures: 0 }); vm.expectRevert( abi.encodeWithSelector( diff --git a/test/helpers/Setup.t.sol b/test/helpers/Setup.t.sol index 1ee4e99b..a5871ac6 100644 --- a/test/helpers/Setup.t.sol +++ b/test/helpers/Setup.t.sol @@ -109,7 +109,8 @@ contract Setup is Test { withdrawalExpireTime: 3 days, obligationTimelockPeriod: 14 days, obligationExpireTime: 3 days, - maxShares: 1e50 + maxShares: 1e50, + disabledFeatures: 0 }); bytes memory data = abi.encodeWithSelector( diff --git a/test/modules/ProtocolManager.t.sol b/test/modules/ProtocolManager.t.sol index d9d8ff97..32094cb6 100644 --- a/test/modules/ProtocolManager.t.sol +++ b/test/modules/ProtocolManager.t.sol @@ -5,6 +5,11 @@ import { Ownable2StepUpgradeable } from "@openzeppelin/contracts-upgradeable/acc import { ETH_ADDRESS } from "@ssv/src/core/libraries/ValidationLib.sol"; import { Setup } from "@ssv/test/helpers/Setup.t.sol"; +import { IProtocolManager } from "@ssv/src/core/interfaces/IProtocolManager.sol"; +import { IBasedAppManager } from "@ssv/src/core/interfaces/IBasedAppManager.sol"; +import { IStrategyManager } from "@ssv/src/core/interfaces/IStrategyManager.sol"; +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import { SSVBasedApps } from "@ssv/src/core/SSVBasedApps.sol"; contract ProtocolManagerTest is Setup, Ownable2StepUpgradeable { function testUpdateFeeTimelockPeriod() public { @@ -192,4 +197,120 @@ contract ProtocolManagerTest is Setup, Ownable2StepUpgradeable { ); proxiedManager.updateMaxFeeIncrement(501); } + + /// @notice By default, no features should be disabled + function testDefaultDisabledFeaturesIsZero() public { + assertEq( + proxiedManager.disabledFeatures(), + 0, + "default disabledFeatures should be zero" + ); + } + + /// @notice The initializer should respect `config.disabledFeatures` + function testInitializeDisabledFeaturesFromConfig() public { + // Override config in Setup + config.disabledFeatures = 3; // slashing & withdrawals disabled + + // Re-deploy a fresh proxy with the modified config + bytes memory initData = abi.encodeWithSelector( + implementation.initialize.selector, + address(OWNER), + IBasedAppManager(basedAppsManagerMod), + IStrategyManager(strategyManagerMod), + IProtocolManager(protocolManagerMod), + config + ); + ERC1967Proxy proxy = new ERC1967Proxy( + address(implementation), + initData + ); + SSVBasedApps proxiedManager = SSVBasedApps(payable(address(proxy))); + + // It should read back exactly what we set + assertEq( + proxiedManager.disabledFeatures(), + 3, + "initializer did not set disabledFeatures from config" + ); + } + + /// @notice Only the owner can update the feature mask + function testUpdateFeatureDisabledFlagsAsOwner() public { + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(2); + assertEq( + proxiedManager.disabledFeatures(), + 2, + "owner update of disabledFeatures failed" + ); + } + + /// @notice Updating the flags should emit DisabledFeaturesUpdated + function testEmitDisabledFeaturesUpdatedEvent() public { + vm.prank(OWNER); + vm.expectEmit(true, false, false, true); + emit IProtocolManager.DisabledFeaturesUpdated(5); + proxiedManager.updateDisabledFeatures(5); + } + + function testRevertUpdateDisabledFeaturesWithNonOwner() public { + vm.prank(ATTACKER); + vm.expectRevert( + abi.encodeWithSelector( + OwnableUnauthorizedAccount.selector, + address(ATTACKER) + ) + ); + proxiedManager.updateDisabledFeatures(1); + } + + function testSetIndividualDisabledFeatureBits() public { + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(1 << 0); + assertEq( + proxiedManager.disabledFeatures(), + 1, + "slashingDisabled bit not set correctly" + ); + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(1 << 1); + assertEq( + proxiedManager.disabledFeatures(), + 2, + "withdrawalsDisabled bit not set correctly" + ); + } + + function testClearFlags() public { + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(3); + assertEq(proxiedManager.disabledFeatures(), 3, "mask precondition"); + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(0); + assertEq(proxiedManager.disabledFeatures(), 0, "flags not cleared"); + } + + function testCombinedFlags() public { + uint32 mask = (1 << 0) | (1 << 2) | (1 << 4); + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(mask); + assertEq( + proxiedManager.disabledFeatures(), + mask, + "combined mask mismatch" + ); + } + + function testOtherParamsUnaffectedByFeatureMask() public { + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(type(uint32).max); + vm.prank(OWNER); + proxiedManager.updateFeeTimelockPeriod(2 days); + assertEq( + proxiedManager.feeTimelockPeriod(), + 2 days, + "feeTimelockPeriod should update despite flags" + ); + } } diff --git a/test/modules/StrategyManager.t.sol b/test/modules/StrategyManager.t.sol index f27fe6c3..52875e80 100644 --- a/test/modules/StrategyManager.t.sol +++ b/test/modules/StrategyManager.t.sol @@ -2678,4 +2678,172 @@ contract StrategyManagerTest is UtilsTest, BasedAppsManagerTest { newWithdrawalAmount ); } + + function testSlashWhenEnabled() public { + uint32 pct = proxiedManager.maxPercentage(); + // register & opt‐in + testStrategyOptInToBApp(pct); + // deposit 1 token + vm.startPrank(USER1); + erc20mock.approve(address(proxiedManager), 1); + proxiedManager.depositERC20(STRATEGY1, erc20mock, 1); + vm.stopPrank(); + // slash from bApp1 + vm.prank(address(bApp1)); + proxiedManager.slash( + STRATEGY1, + address(bApp1), + address(erc20mock), + 1, + "" + ); + // after slash, strategy balance should be zero + uint256 bal = proxiedManager.strategyTotalBalance( + STRATEGY1, + address(erc20mock) + ); + assertEq(bal, 0, "Strategy balance should have decreased by 1"); + } + + function testSlashRevertsWhenDisabled() public { + // disable slashing + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(1 << 0); + // now any slash call must revert + vm.prank(address(bApp1)); + vm.expectRevert( + abi.encodeWithSelector(IStrategyManager.SlashingDisabled.selector) + ); + proxiedManager.slash( + STRATEGY1, + address(bApp1), + address(erc20mock), + 1, + "" + ); + } + + function testSlashSucceedsAfterReenable() public { + // re‐enable slashing + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(0); + // prepare valid slash (opt-in + deposit) + uint32 pct = proxiedManager.maxPercentage(); + testStrategyOptInToBApp(pct); + vm.startPrank(USER1); + erc20mock.approve(address(proxiedManager), 1); + proxiedManager.depositERC20(STRATEGY1, erc20mock, 1); + vm.stopPrank(); + // should no longer revert + vm.prank(address(bApp1)); + proxiedManager.slash( + STRATEGY1, + address(bApp1), + address(erc20mock), + 1, + "" + ); + // confirm balance dropped + uint256 bal = proxiedManager.strategyTotalBalance( + STRATEGY1, + address(erc20mock) + ); + assertEq(bal, 0, "Slash should succeed once re-enabled"); + } + + function testProposeAndFinalizeWithdrawalWhenEnabled() public { + // set up a deposit + testCreateStrategyAndSingleDeposit(100); + // propose withdrawal + vm.prank(USER1); + proxiedManager.proposeWithdrawal(STRATEGY1, address(erc20mock), 50); + // fast-forward timelock + vm.warp(block.timestamp + proxiedManager.withdrawalTimelockPeriod()); + // finalize and check balances + vm.prank(USER1); + proxiedManager.finalizeWithdrawal(STRATEGY1, erc20mock); + uint256 remaining = proxiedManager.strategyAccountShares( + STRATEGY1, + USER1, + address(erc20mock) + ); + assertEq( + remaining, + 50, + "User should have 50 tokens left in the strategy" + ); + } + + function testProposeWithdrawalRevertsWhenDisabled() public { + // disable withdrawals (bit 1) + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(1 << 1); + + // attempt to propose an ERC20 withdrawal + vm.prank(USER1); + vm.expectRevert( + abi.encodeWithSelector( + IStrategyManager.WithdrawalsDisabled.selector + ) + ); + proxiedManager.proposeWithdrawal(STRATEGY1, address(erc20mock), 1); + } + + function testFinalizeWithdrawalRevertsWhenDisabled() public { + // first get a pending withdrawal + testProposeWithdrawalFromStrategy(); + + // disable withdrawals + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(1 << 1); + + // now finalize should revert + vm.warp(block.timestamp + proxiedManager.withdrawalTimelockPeriod()); + vm.prank(USER1); + vm.expectRevert( + abi.encodeWithSelector( + IStrategyManager.WithdrawalsDisabled.selector + ) + ); + proxiedManager.finalizeWithdrawal(STRATEGY1, erc20mock); + } + + function testProposeWithdrawalETHRevertsWhenDisabled() public { + // deposit some ETH first + testCreateStrategyETHAndDepositETH(); + + // disable withdrawals + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(1 << 1); + + // attempt to propose an ETH withdrawal + vm.prank(USER1); + vm.expectRevert( + abi.encodeWithSelector( + IStrategyManager.WithdrawalsDisabled.selector + ) + ); + proxiedManager.proposeWithdrawalETH(STRATEGY1, 1 ether); + } + + function testFinalizeWithdrawalETHRevertsWhenDisabled() public { + // get a pending ETH withdrawal + testProposeWithdrawalETHFromStrategy(0.5 ether); + + // disable withdrawals + vm.prank(OWNER); + proxiedManager.updateDisabledFeatures(1 << 1); + + // warp past timelock + vm.warp(block.timestamp + proxiedManager.withdrawalTimelockPeriod()); + + // now finalize should revert + vm.prank(USER1); + vm.expectRevert( + abi.encodeWithSelector( + IStrategyManager.WithdrawalsDisabled.selector + ) + ); + proxiedManager.finalizeWithdrawalETH(STRATEGY1); + } } From f807abfcb3e8d57a4259cbf6d197a8c9c0d7ee09 Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Wed, 23 Apr 2025 12:38:46 +0200 Subject: [PATCH 2/2] chore: prefix internal functions, minor changes --- {specs => doc}/feature_activation.md | 4 ++-- src/core/modules/StrategyManager.sol | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) rename {specs => doc}/feature_activation.md (97%) diff --git a/specs/feature_activation.md b/doc/feature_activation.md similarity index 97% rename from specs/feature_activation.md rename to doc/feature_activation.md index 0c634a96..ffcd0d01 100644 --- a/specs/feature_activation.md +++ b/doc/feature_activation.md @@ -18,12 +18,12 @@ Introduce a compact, on‑chain mechanism to enable or disable selected features - **Checks**: Two internal functions in `StrategyManager`: ```solidity - function checkSlashingAllowed() internal view { + function _checkSlashingAllowed() internal view { if (ProtocolStorageLib.load().disabledFeatures & SLASHING_DISABLED != 0) revert SlashingDisabled(); } - function checkWithdrawalsAllowed() internal view { + function _checkWithdrawalsAllowed() internal view { if (ProtocolStorageLib.load().disabledFeatures & WITHDRAWALS_DISABLED != 0) revert WithdrawalsDisabled(); } diff --git a/src/core/modules/StrategyManager.sol b/src/core/modules/StrategyManager.sol index 981360fd..9107da8c 100644 --- a/src/core/modules/StrategyManager.sol +++ b/src/core/modules/StrategyManager.sol @@ -256,7 +256,7 @@ contract StrategyManager is ReentrancyGuardTransient, IStrategyManager { address token, uint256 amount ) external { - checkWithdrawalsAllowed(); + _checkWithdrawalsAllowed(); if (token == ETH_ADDRESS) revert InvalidToken(); _proposeWithdrawal(strategyId, token, amount); @@ -269,7 +269,7 @@ contract StrategyManager is ReentrancyGuardTransient, IStrategyManager { uint32 strategyId, IERC20 token ) external nonReentrant { - checkWithdrawalsAllowed(); + _checkWithdrawalsAllowed(); uint256 amount = _finalizeWithdrawal(strategyId, address(token)); @@ -288,14 +288,14 @@ contract StrategyManager is ReentrancyGuardTransient, IStrategyManager { /// @param strategyId The ID of the strategy. /// @param amount The amount of ETH to withdraw. function proposeWithdrawalETH(uint32 strategyId, uint256 amount) external { - checkWithdrawalsAllowed(); + _checkWithdrawalsAllowed(); _proposeWithdrawal(strategyId, ETH_ADDRESS, amount); } /// @notice Finalize the ETH withdrawal after the timelock period has passed. /// @param strategyId The ID of the strategy. function finalizeWithdrawalETH(uint32 strategyId) external nonReentrant { - checkWithdrawalsAllowed(); + _checkWithdrawalsAllowed(); uint256 amount = _finalizeWithdrawal(strategyId, ETH_ADDRESS); @@ -780,9 +780,9 @@ contract StrategyManager is ReentrancyGuardTransient, IStrategyManager { uint256 amount, bytes calldata data ) external nonReentrant { + _checkSlashingAllowed(); if (amount == 0) revert InvalidAmount(); CoreStorageLib.Data storage s = CoreStorageLib.load(); - checkSlashingAllowed(); if (!s.registeredBApps[bApp]) { revert IBasedAppManager.BAppNotRegistered(); @@ -923,7 +923,7 @@ contract StrategyManager is ReentrancyGuardTransient, IStrategyManager { s.slashingFund[msg.sender][token] -= amount; } - function checkSlashingAllowed() internal view { + function _checkSlashingAllowed() internal view { if ( ProtocolStorageLib.load().disabledFeatures & SLASHING_DISABLED != 0 ) { @@ -931,7 +931,7 @@ contract StrategyManager is ReentrancyGuardTransient, IStrategyManager { } } - function checkWithdrawalsAllowed() internal view { + function _checkWithdrawalsAllowed() internal view { if ( ProtocolStorageLib.load().disabledFeatures & WITHDRAWALS_DISABLED != 0