Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: reentrant guards #1107

Merged
merged 4 commits into from
Feb 14, 2025
Merged

fix: reentrant guards #1107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5792bac
feat: guards on external functions
8sunyuan Feb 14, 2025
9ef44d7
chore: fmt
8sunyuan Feb 14, 2025
9b80e82
fix: remove guard from redelegate
wadealexc Feb 14, 2025
00c4dc5
feat: additional reentrancy protection for EPM and SM
wadealexc Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions src/contracts/core/DelegationManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}
Expand All @@ -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());

Expand All @@ -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());
Expand Down Expand Up @@ -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];

Expand Down Expand Up @@ -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.
Expand All @@ -252,7 +255,7 @@ contract DelegationManager is
address staker,
uint256 curDepositShares,
uint64 beaconChainSlashingFactorDecrease
) external onlyEigenPodManager {
) external onlyEigenPodManager nonReentrant {
if (!isDelegated(staker)) {
return;
}
Expand Down Expand Up @@ -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],
Expand Down
17 changes: 10 additions & 7 deletions src/contracts/core/StrategyManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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);
}

Expand All @@ -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);
Expand All @@ -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
Expand All @@ -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
Expand Down
12 changes: 6 additions & 6 deletions src/contracts/pods/EigenPodManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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
Expand Down Expand Up @@ -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());
Expand All @@ -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);
}
Expand All @@ -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());
Expand Down Expand Up @@ -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);
}
Expand Down
Loading