fix: try catch out of gas edge case (#971)

script/releases/v1.0.2-slashing/1-eoa.s.sol

@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity ^0.8.12;
+
+import {EOADeployer} from "zeus-templates/templates/EOADeployer.sol";
+import "../Env.sol";
+
+import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
+import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
+import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
+
+// Just upgrade StrategyManager
+contract Deploy is EOADeployer {
+    using Env for *;
+
+    function _runAsEOA() internal override {
+        vm.startBroadcast();
+        deployImpl({
+            name: type(StrategyManager).name,
+            deployedTo: address(new StrategyManager({
+                _delegation: Env.proxy.delegationManager(),
+                _pauserRegistry: Env.impl.pauserRegistry()
+            }))
+        });
+
+        vm.stopBroadcast();
+    }
+
+    function testDeploy() public virtual {
+        _runAsEOA();
+        _validateNewImplAddresses(false);
+        _validateImplConstructors();
+        _validateImplsInitialized();
+    }
+
+
+    /// @dev Validate that the `Env.impl` addresses are updated to be distinct from what the proxy
+    /// admin reports as the current implementation address.
+    ///
+    /// Note: The upgrade script can call this with `areMatching == true` to check that these impl
+    /// addresses _are_ matches.
+    function _validateNewImplAddresses(bool areMatching) internal view {
+        function (address, address, string memory) internal pure assertion =
+            areMatching ? _assertMatch : _assertNotMatch;
+
+
+        assertion(
+            _getProxyImpl(address(Env.proxy.strategyManager())),
+            address(Env.impl.strategyManager()),
+            "strategyManager impl failed"
+        );
+    }
+
+    /// @dev Validate the immutables set in the new implementation constructors
+    function _validateImplConstructors() internal view {
+        StrategyManager strategyManager = Env.impl.strategyManager();
+        assertTrue(strategyManager.delegation() == Env.proxy.delegationManager(), "sm.dm invalid");
+        assertTrue(strategyManager.pauserRegistry() == Env.impl.pauserRegistry(), "sm.pR invalid");
+    }
+
+    /// @dev Call initialize on all deployed implementations to ensure initializers are disabled
+    function _validateImplsInitialized() internal {
+        bytes memory errInit = "Initializable: contract is already initialized";
+
+        StrategyManager strategyManager = Env.impl.strategyManager();
+        vm.expectRevert(errInit);
+        strategyManager.initialize(address(0), address(0), 0);
+    }
+
+    /// @dev Query and return `proxyAdmin.getProxyImplementation(proxy)`
+    function _getProxyImpl(address proxy) internal view returns (address) {
+        return ProxyAdmin(Env.proxyAdmin()).getProxyImplementation(ITransparentUpgradeableProxy(proxy));
+    }
+
+    /// @dev Query and return `proxyAdmin.getProxyAdmin(proxy)`
+    function _getProxyAdmin(address proxy) internal view returns (address) {
+        return ProxyAdmin(Env.proxyAdmin()).getProxyAdmin(ITransparentUpgradeableProxy(proxy));
+    }
+
+    function _assertMatch(address a, address b, string memory err) private pure {
+        assertEq(a, b, err);
+    }
+
+    function _assertNotMatch(address a, address b, string memory err) private pure {
+        assertNotEq(a, b, err);
+    }
+}

script/releases/v1.0.2-slashing/2-multisig.s.sol

@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity ^0.8.12;
+
+import {Deploy} from "./1-eoa.s.sol";
+import "../Env.sol";
+
+import {MultisigBuilder} from "zeus-templates/templates/MultisigBuilder.sol";
+import "zeus-templates/utils/Encode.sol";
+
+import {TimelockController} from "@openzeppelin/contracts/governance/TimelockController.sol";
+
+contract Queue is MultisigBuilder, Deploy {
+    using Env for *;
+    using Encode for *;
+
+    function _runAsMultisig() prank(Env.opsMultisig()) internal virtual override {
+        bytes memory calldata_to_executor = _getCalldataToExecutor();
+
+        TimelockController timelock = Env.timelockController();
+        timelock.schedule({
+            target: Env.executorMultisig(),
+            value: 0,
+            data: calldata_to_executor,
+            predecessor: 0,
+            salt: 0,
+            delay: timelock.getMinDelay()
+        });
+    }
+
+     /// @dev Get the calldata to be sent from the timelock to the executor
+    function _getCalldataToExecutor() internal returns (bytes memory) {
+        MultisigCall[] storage executorCalls = Encode.newMultisigCalls()
+            /// core/
+            .append({
+                to: Env.proxyAdmin(),
+                data: Encode.proxyAdmin.upgrade({
+                    proxy: address(Env.proxy.strategyManager()),
+                    impl: address(Env.impl.strategyManager())
+                })
+            });
+
+        return Encode.gnosisSafe.execTransaction({
+            from: address(Env.timelockController()),
+            to: address(Env.multiSendCallOnly()),
+            op: Encode.Operation.DelegateCall,
+            data: Encode.multiSend(executorCalls)
+        });
+    }
+
+    function testScript() public virtual {
+        runAsEOA();
+
+        TimelockController timelock = Env.timelockController();
+        bytes memory calldata_to_executor = _getCalldataToExecutor();
+        bytes32 txHash = timelock.hashOperation({
+            target: Env.executorMultisig(),
+            value: 0,
+            data: calldata_to_executor,
+            predecessor: 0,
+            salt: 0
+        });
+
+        // Check that the upgrade does not exist in the timelock
+        assertFalse(timelock.isOperationPending(txHash), "Transaction should NOT be queued.");
+
+        execute();
+
+        // Check that the upgrade has been added to the timelock
+        assertTrue(timelock.isOperationPending(txHash), "Transaction should be queued.");
+    }
+}

script/releases/v1.0.2-slashing/3-execute.s.sol

@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity ^0.8.12;
+
+import "../Env.sol";
+import {Queue} from "./2-multisig.s.sol";
+
+import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
+import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
+
+contract Execute is Queue {
+    using Env for *;
+
+    function _runAsMultisig() prank(Env.protocolCouncilMultisig()) internal override(Queue) {
+        bytes memory calldata_to_executor = _getCalldataToExecutor();
+
+        TimelockController timelock = Env.timelockController();
+        timelock.execute({
+            target: Env.executorMultisig(),
+            value: 0,
+            payload: calldata_to_executor,
+            predecessor: 0,
+            salt: 0
+        });
+    }
+
+    function testScript() public virtual override(Queue){
+        // 0. Deploy Impls
+        runAsEOA();
+
+        TimelockController timelock = Env.timelockController();
+        bytes memory calldata_to_executor = _getCalldataToExecutor();
+        bytes32 txHash = timelock.hashOperation({
+            target: Env.executorMultisig(),
+            value: 0,
+            data: calldata_to_executor,
+            predecessor: 0,
+            salt: 0
+        });
+        assertFalse(timelock.isOperationPending(txHash), "Transaction should NOT be queued.");
+
+        // 1. Queue Upgrade
+        Queue._runAsMultisig();
+        _unsafeResetHasPranked(); // reset hasPranked so we can use it again
+
+        // 2. Warp past delay
+        vm.warp(block.timestamp + timelock.getMinDelay()); // 1 tick after ETA
+        assertEq(timelock.isOperationReady(txHash), true, "Transaction should be executable.");
+
+        // 3- execute
+        execute();
+
+        assertTrue(timelock.isOperationDone(txHash), "Transaction should be complete.");
+
+        // 4. Validate
+        _validateNewImplAddresses(true);
+        _validateProxyConstructors();
+    }
+
+    function _validateProxyConstructors() internal view {
+        StrategyManager strategyManager = Env.proxy.strategyManager();
+        assertTrue(strategyManager.delegation() == Env.proxy.delegationManager(), "sm.dm invalid");
+        assertTrue(strategyManager.pauserRegistry() == Env.impl.pauserRegistry(), "sm.pR invalid");
+    }
+}

script/releases/v1.0.2-slashing/upgrade.json

@@ -0,0 +1,19 @@
+{
+    "name": "slashing-patch",
+    "from": "1.0.1",
+    "to": "1.0.2",
+    "phases": [
+        {
+            "type": "eoa",
+            "filename": "1-eoa.s.sol"
+        },
+        {
+            "type": "multisig",
+            "filename": "2-multisig.s.sol"
+        },
+        {
+            "type": "multisig",
+            "filename": "3-execute.s.sol"
+        }
+    ]
+}

src/contracts/core/StrategyManager.sol

@@ -143,7 +143,7 @@ contract StrategyManager is
     /// @inheritdoc IStrategyManager
     function burnShares(IStrategy strategy, uint256 sharesToBurn) external onlyDelegationManager {
         // burning shares is functionally the same as withdrawing but with different destination address
-        try strategy.withdraw(DEFAULT_BURN_ADDRESS, strategy.underlyingToken(), sharesToBurn) {} catch {}
+        strategy.withdraw(DEFAULT_BURN_ADDRESS, strategy.underlyingToken(), sharesToBurn);
     }
 
     /// @inheritdoc IStrategyManager

src/test/unit/StrategyManagerUnit.t.sol

@@ -1421,6 +1421,7 @@ contract StrategyManagerUnitTests_withdrawSharesAsTokens is StrategyManagerUnitT
     ) external filterFuzzedAddressInputs(staker) {
         cheats.assume(staker != address(this));
         cheats.assume(staker != address(0));
+        cheats.assume(staker != address(dummyStrat));
         cheats.assume(depositAmount > 0 && depositAmount < dummyToken.totalSupply() && depositAmount < sharesAmount);
         IStrategy strategy = dummyStrat;
         IERC20 token = dummyToken;
@@ -1457,10 +1458,10 @@ contract StrategyManagerUnitTests_burnShares is StrategyManagerUnitTests {
     }
 
     /**
-     * @notice deposits a single strategy and withdrawSharesAsTokens() function reverts when sharesAmount is
-     * higher than depositAmount
+     * @notice deposits a single strategy and withdrawSharesAsTokens(). Tests that we revert when we
+     * burn more than expected
      */
-    function testFuzz_ShareAmountTooHigh(
+    function testFuzz_RevertShareAmountTooHigh(
         address staker,
         uint256 depositAmount,
         uint256 sharesToBurn
@@ -1471,15 +1472,9 @@ contract StrategyManagerUnitTests_burnShares is StrategyManagerUnitTests {
         IERC20 token = dummyToken;
         _depositIntoStrategySuccessfully(strategy, staker, depositAmount);
 
-        uint256 strategyBalanceBefore = token.balanceOf(address(strategy));
-        uint256 burnAddressBalanceBefore = token.balanceOf(strategyManager.DEFAULT_BURN_ADDRESS());
         cheats.prank(address(delegationManagerMock));
+        cheats.expectRevert(IStrategyErrors.WithdrawalAmountExceedsTotalDeposits.selector);
         strategyManager.burnShares(strategy, sharesToBurn);
-        uint256 strategyBalanceAfter = token.balanceOf(address(strategy));
-        uint256 burnAddressBalanceAfter = token.balanceOf(strategyManager.DEFAULT_BURN_ADDRESS());
-
-        assertEq(burnAddressBalanceBefore, burnAddressBalanceAfter, "burnAddressBalanceBefore != burnAddressBalanceAfter");
-        assertEq(strategyBalanceBefore, strategyBalanceAfter, "strategyBalanceBefore != strategyBalanceAfter");
     }
 
     function testFuzz_SingleStrategyDeposited(
@@ -1518,7 +1513,7 @@ contract StrategyManagerUnitTests_burnShares is StrategyManagerUnitTests {
     }
 
     /// @notice check that balances are unchanged with a reverting token but burnShares doesn't revert
-    function testFuzz_tryCatchWithRevertToken(
+    function testFuzz_revertTryCatchWithRevertToken(
         address staker,
         uint256 depositAmount,
         uint256 sharesToBurn
@@ -1533,15 +1528,9 @@ contract StrategyManagerUnitTests_burnShares is StrategyManagerUnitTests {
         cheats.etch(address(token), address(revertToken).code);
         ERC20_SetTransferReverting_Mock(address(token)).setTransfersRevert(true);
 
-        uint256 strategyBalanceBefore = token.balanceOf(address(strategy));
-        uint256 burnAddressBalanceBefore = token.balanceOf(strategyManager.DEFAULT_BURN_ADDRESS());
+        cheats.expectRevert("SafeERC20: low-level call failed");
         cheats.prank(address(delegationManagerMock));
         strategyManager.burnShares(strategy, sharesToBurn);
-        uint256 strategyBalanceAfter = token.balanceOf(address(strategy));
-        uint256 burnAddressBalanceAfter = token.balanceOf(strategyManager.DEFAULT_BURN_ADDRESS());
-
-        assertEq(burnAddressBalanceBefore, burnAddressBalanceAfter, "burnAddressBalanceBefore != burnAddressBalanceAfter");
-        assertEq(strategyBalanceBefore, strategyBalanceAfter, "strategyBalanceBefore != strategyBalanceAfter");
     }
 }