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 <[email protected]>
Layr-Labs · Feb 14, 2025 · 81fef63 · 81fef63
1 parent 067010c
commit 81fef63
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 21 deletions.
diff --git 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
@@ -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
@@ -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);
     }