From b02cc3e98b57c488d485b8fdeba733af9e36995d Mon Sep 17 00:00:00 2001 From: Michael Sun <35479365+8sunyuan@users.noreply.github.com> Date: Fri, 14 Feb 2025 15:13:10 -0500 Subject: [PATCH] fix: reentrant guards (#1107) **Motivation:** Concerns about reentrancy in the DelegationManager and interactions of completed withdrawals which can call untrusted ERC20 transfers **Modifications:** Added reentrant guards across external functions **Result:** Preventing cross-function reentrancy in the DelegationManager --------- Co-authored-by: wadealexc --- src/contracts/core/DelegationManager.sol | 19 +++++++++++-------- src/contracts/core/StrategyManager.sol | 17 ++++++++++------- src/contracts/pods/EigenPodManager.sol | 12 ++++++------ 3 files changed, 27 insertions(+), 21 deletions(-) diff --git a/src/contracts/core/DelegationManager.sol b/src/contracts/core/DelegationManager.sol index 2e7828adc5..eb756d1973 100644 --- a/src/contracts/core/DelegationManager.sol +++ b/src/contracts/core/DelegationManager.sol @@ -96,7 +96,7 @@ contract DelegationManager is address initDelegationApprover, uint32 allocationDelay, string calldata metadataURI - ) external { + ) external nonReentrant { require(!isDelegated(msg.sender), ActivelyDelegated()); allocationManager.setAllocationDelay(msg.sender, allocationDelay); @@ -110,7 +110,10 @@ contract DelegationManager is } /// @inheritdoc IDelegationManager - function modifyOperatorDetails(address operator, address newDelegationApprover) external checkCanCall(operator) { + function modifyOperatorDetails( + address operator, + address newDelegationApprover + ) external checkCanCall(operator) nonReentrant { require(isOperator(operator), OperatorNotRegistered()); _setDelegationApprover(operator, newDelegationApprover); } @@ -126,7 +129,7 @@ contract DelegationManager is address operator, SignatureWithExpiry memory approverSignatureAndExpiry, bytes32 approverSalt - ) public { + ) public nonReentrant { require(!isDelegated(msg.sender), ActivelyDelegated()); require(isOperator(operator), OperatorNotRegistered()); @@ -145,7 +148,7 @@ contract DelegationManager is /// @inheritdoc IDelegationManager function undelegate( address staker - ) public returns (bytes32[] memory withdrawalRoots) { + ) public nonReentrant returns (bytes32[] memory withdrawalRoots) { // Check that the `staker` can undelegate require(isDelegated(staker), NotActivelyDelegated()); require(!isOperator(staker), OperatorsCannotUndelegate()); @@ -176,7 +179,7 @@ contract DelegationManager is /// @inheritdoc IDelegationManager function queueWithdrawals( QueuedWithdrawalParams[] calldata params - ) external onlyWhenNotPaused(PAUSED_ENTER_WITHDRAWAL_QUEUE) returns (bytes32[] memory) { + ) external onlyWhenNotPaused(PAUSED_ENTER_WITHDRAWAL_QUEUE) nonReentrant returns (bytes32[] memory) { bytes32[] memory withdrawalRoots = new bytes32[](params.length); address operator = delegatedTo[msg.sender]; @@ -228,7 +231,7 @@ contract DelegationManager is IStrategy strategy, uint256 prevDepositShares, uint256 addedShares - ) external onlyStrategyManagerOrEigenPodManager { + ) external onlyStrategyManagerOrEigenPodManager nonReentrant { /// Note: Unlike `decreaseDelegatedShares`, we don't return early if the staker has no operator. /// This is because `_increaseDelegation` updates the staker's deposit scaling factor, which we /// need to do even if not delegated. @@ -252,7 +255,7 @@ contract DelegationManager is address staker, uint256 curDepositShares, uint64 beaconChainSlashingFactorDecrease - ) external onlyEigenPodManager { + ) external onlyEigenPodManager nonReentrant { if (!isDelegated(staker)) { return; } @@ -282,7 +285,7 @@ contract DelegationManager is IStrategy strategy, uint64 prevMaxMagnitude, uint64 newMaxMagnitude - ) external onlyAllocationManager { + ) external onlyAllocationManager nonReentrant { /// forgefmt: disable-next-item uint256 operatorSharesSlashed = SlashingLib.calcSlashedAmount({ operatorShares: operatorShares[operator][strategy], diff --git a/src/contracts/core/StrategyManager.sol b/src/contracts/core/StrategyManager.sol index c1b5bbada0..8d320a12b0 100644 --- a/src/contracts/core/StrategyManager.sol +++ b/src/contracts/core/StrategyManager.sol @@ -117,7 +117,7 @@ contract StrategyManager is address staker, IStrategy strategy, uint256 depositSharesToRemove - ) external onlyDelegationManager returns (uint256) { + ) external onlyDelegationManager nonReentrant returns (uint256) { (, uint256 sharesAfter) = _removeDepositShares(staker, strategy, depositSharesToRemove); return sharesAfter; } @@ -127,7 +127,7 @@ contract StrategyManager is address staker, IStrategy strategy, uint256 shares - ) external onlyDelegationManager returns (uint256, uint256) { + ) external onlyDelegationManager nonReentrant returns (uint256, uint256) { return _addShares(staker, strategy, shares); } @@ -137,12 +137,15 @@ contract StrategyManager is IStrategy strategy, IERC20 token, uint256 shares - ) external onlyDelegationManager { + ) external onlyDelegationManager nonReentrant { strategy.withdraw(staker, token, shares); } /// @inheritdoc IShareManager - function increaseBurnableShares(IStrategy strategy, uint256 addedSharesToBurn) external onlyDelegationManager { + function increaseBurnableShares( + IStrategy strategy, + uint256 addedSharesToBurn + ) external onlyDelegationManager nonReentrant { (, uint256 currentShares) = EnumerableMap.tryGet(burnableShares, address(strategy)); EnumerableMap.set(burnableShares, address(strategy), currentShares + addedSharesToBurn); emit BurnableSharesIncreased(strategy, addedSharesToBurn); @@ -162,14 +165,14 @@ contract StrategyManager is /// @inheritdoc IStrategyManager function setStrategyWhitelister( address newStrategyWhitelister - ) external onlyOwner { + ) external onlyOwner nonReentrant { _setStrategyWhitelister(newStrategyWhitelister); } /// @inheritdoc IStrategyManager function addStrategiesToDepositWhitelist( IStrategy[] calldata strategiesToWhitelist - ) external onlyStrategyWhitelister { + ) external onlyStrategyWhitelister nonReentrant { uint256 strategiesToWhitelistLength = strategiesToWhitelist.length; for (uint256 i = 0; i < strategiesToWhitelistLength; ++i) { // change storage and emit event only if strategy is not already in whitelist @@ -183,7 +186,7 @@ contract StrategyManager is /// @inheritdoc IStrategyManager function removeStrategiesFromDepositWhitelist( IStrategy[] calldata strategiesToRemoveFromWhitelist - ) external onlyStrategyWhitelister { + ) external onlyStrategyWhitelister nonReentrant { uint256 strategiesToRemoveFromWhitelistLength = strategiesToRemoveFromWhitelist.length; for (uint256 i = 0; i < strategiesToRemoveFromWhitelistLength; ++i) { // change storage and emit event only if strategy is already in whitelist diff --git a/src/contracts/pods/EigenPodManager.sol b/src/contracts/pods/EigenPodManager.sol index e6ec820e17..921fefba91 100644 --- a/src/contracts/pods/EigenPodManager.sol +++ b/src/contracts/pods/EigenPodManager.sol @@ -66,7 +66,7 @@ contract EigenPodManager is } /// @inheritdoc IEigenPodManager - function createPod() external onlyWhenNotPaused(PAUSED_NEW_EIGENPODS) returns (address) { + function createPod() external onlyWhenNotPaused(PAUSED_NEW_EIGENPODS) nonReentrant returns (address) { require(!hasPod(msg.sender), EigenPodAlreadyExists()); // deploy a pod if the sender doesn't have one already IEigenPod pod = _deployPod(); @@ -79,7 +79,7 @@ contract EigenPodManager is bytes calldata pubkey, bytes calldata signature, bytes32 depositDataRoot - ) external payable onlyWhenNotPaused(PAUSED_NEW_EIGENPODS) { + ) external payable onlyWhenNotPaused(PAUSED_NEW_EIGENPODS) nonReentrant { IEigenPod pod = ownerToPod[msg.sender]; if (address(pod) == address(0)) { //deploy a pod if the sender doesn't have one already @@ -148,7 +148,7 @@ contract EigenPodManager is address staker, IStrategy strategy, uint256 depositSharesToRemove - ) external onlyDelegationManager returns (uint256) { + ) external onlyDelegationManager nonReentrant returns (uint256) { require(strategy == beaconChainETHStrategy, InvalidStrategy()); int256 updatedShares = podOwnerDepositShares[staker] - int256(depositSharesToRemove); require(updatedShares >= 0, SharesNegative()); @@ -169,7 +169,7 @@ contract EigenPodManager is address staker, IStrategy strategy, uint256 shares - ) external onlyDelegationManager returns (uint256, uint256) { + ) external onlyDelegationManager nonReentrant returns (uint256, uint256) { require(strategy == beaconChainETHStrategy, InvalidStrategy()); return _addShares(staker, shares); } @@ -185,7 +185,7 @@ contract EigenPodManager is IStrategy strategy, IERC20, uint256 shares - ) external onlyDelegationManager { + ) external onlyDelegationManager nonReentrant { require(strategy == beaconChainETHStrategy, InvalidStrategy()); require(staker != address(0), InputAddressZero()); require(int256(shares) > 0, SharesNegative()); @@ -226,7 +226,7 @@ contract EigenPodManager is } /// @inheritdoc IShareManager - function increaseBurnableShares(IStrategy, uint256 addedSharesToBurn) external onlyDelegationManager { + function increaseBurnableShares(IStrategy, uint256 addedSharesToBurn) external onlyDelegationManager nonReentrant { burnableETHShares += addedSharesToBurn; emit BurnableETHSharesIncreased(addedSharesToBurn); }