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

feat: prevent queuing withdrawals to other addresses #438

Merged
merged 1 commit into from
Feb 13, 2024
Merged

feat: prevent queuing withdrawals to other addresses #438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 5 additions & 8 deletions docs/core/DelegationManager.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,20 +226,18 @@ function queueWithdrawals(

Allows the caller to queue one or more withdrawals of their held shares across any strategy (in either/both the `EigenPodManager` or `StrategyManager`). If the caller is delegated to an Operator, the `shares` and `strategies` being withdrawn are immediately removed from that Operator's delegated share balances. Note that if the caller is an Operator, this still applies, as Operators are essentially delegated to themselves.

`queueWithdrawals` works very similarly to `undelegate`, except that the caller is not undelegated, and also may:
* Choose which strategies and how many shares to withdraw (as opposed to ALL shares/strategies)
* Specify a `withdrawer` to receive withdrawn funds once the withdrawal is completed
`queueWithdrawals` works very similarly to `undelegate`, except that the caller is not undelegated, and also may choose which strategies and how many shares to withdraw (as opposed to ALL shares/strategies).

All shares being withdrawn (whether via the `EigenPodManager` or `StrategyManager`) are removed while the withdrawals are in the queue.

Withdrawals can be completed by the `withdrawer` after max(`minWithdrawalDelayBlocks`, `strategyWithdrawalDelayBlocks[strategy]`) such that `strategy` represents the queued strategies to be withdrawn. Withdrawals do not require the `withdrawer` to "fully exit" from the system -- they may choose to receive their shares back in full once the withdrawal is completed (see [`completeQueuedWithdrawal`](#completequeuedwithdrawal) for details).
Withdrawals can be completed by the caller after max(`minWithdrawalDelayBlocks`, `strategyWithdrawalDelayBlocks[strategy]`) such that `strategy` represents the queued strategies to be withdrawn. Withdrawals do not require the caller to "fully exit" from the system -- they may choose to receive their shares back in full once the withdrawal is completed (see [`completeQueuedWithdrawal`](#completequeuedwithdrawal) for details).

Note that for any `strategy` s.t `StrategyManager.thirdPartyTransfersForbidden(strategy) == true` the `withdrawer` must be the same address as the `staker` as this setting disallows users to deposit or withdraw on behalf of other users. (see [`thirdPartyTransfersForbidden`](./StrategyManager.md) for details).
Note that the `QueuedWithdrawalParams` struct has a `withdrawer` field. Originally, this was used to specify an address that the withdrawal would be credited to once completed. However, `queueWithdrawals` now requires that `withdrawer == msg.sender`. Any other input is rejected.

*Effects*:
* For each withdrawal:
* If the caller is delegated to an Operator, that Operator's delegated balances are decreased according to the `strategies` and `shares` being withdrawn.
* A `Withdrawal` is queued for the `withdrawer`, tracking the strategies and shares being withdrawn
* A `Withdrawal` is queued for the caller, tracking the strategies and shares being withdrawn
* The caller's withdrawal nonce is increased
* The hash of the `Withdrawal` is marked as "pending"
* See [`EigenPodManager.removeShares`](./EigenPodManager.md#eigenpodmanagerremoveshares)
Expand All @@ -250,8 +248,7 @@ Note that for any `strategy` s.t `StrategyManager.thirdPartyTransfersForbidden(s
* For each withdrawal:
* `strategies.length` MUST equal `shares.length`
* `strategies.length` MUST NOT be equal to 0
* The `withdrawer` MUST NOT be 0
* For all strategies being withdrawn, the `withdrawer` MUST be the same address as the `staker` if `StrategyManager.thirdPartyTransfersForbidden(strategy) == true`
* The `withdrawer` MUST equal `msg.sender`
* See [`EigenPodManager.removeShares`](./EigenPodManager.md#eigenpodmanagerremoveshares)
* See [`StrategyManager.removeShares`](./StrategyManager.md#removeshares)

Expand Down
2 changes: 1 addition & 1 deletion src/contracts/core/DelegationManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ contract DelegationManager is Initializable, OwnableUpgradeable, Pausable, Deleg

for (uint256 i = 0; i < queuedWithdrawalParams.length; i++) {
require(queuedWithdrawalParams[i].strategies.length == queuedWithdrawalParams[i].shares.length, "DelegationManager.queueWithdrawal: input length mismatch");
require(queuedWithdrawalParams[i].withdrawer != address(0), "DelegationManager.queueWithdrawal: must provide valid withdrawal address");
require(queuedWithdrawalParams[i].withdrawer == msg.sender, "DelegationManager.queueWithdrawal: withdrawer must be staker");

// Remove shares from staker's strategies and place strategies/shares in queue.
// If the staker is delegated to an operator, the operator's delegated shares are also reduced
Expand Down
54 changes: 5 additions & 49 deletions src/test/Withdrawals.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,17 @@ contract WithdrawalTests is EigenLayerTestHelper {
function testWithdrawalWrapper(
address operator,
address depositor,
address withdrawer,
uint96 ethAmount,
uint96 eigenAmount,
bool withdrawAsTokens,
bool RANDAO
) public fuzzedAddress(operator) fuzzedAddress(depositor) fuzzedAddress(withdrawer) {
) public fuzzedAddress(operator) fuzzedAddress(depositor) {
cheats.assume(depositor != operator);
cheats.assume(ethAmount >= 1 && ethAmount <= 1e18);
cheats.assume(eigenAmount >= 1 && eigenAmount <= 1e18);

address withdrawer = depositor;

if (RANDAO) {
_testWithdrawalAndDeregistration(operator, depositor, withdrawer, ethAmount, eigenAmount, withdrawAsTokens);
} else {
Expand Down Expand Up @@ -244,14 +245,13 @@ contract WithdrawalTests is EigenLayerTestHelper {
function testRedelegateAfterWithdrawal(
address operator,
address depositor,
address withdrawer,
uint96 ethAmount,
uint96 eigenAmount,
bool withdrawAsShares
) public fuzzedAddress(operator) fuzzedAddress(depositor) fuzzedAddress(withdrawer) {
) public fuzzedAddress(operator) fuzzedAddress(depositor) {
cheats.assume(depositor != operator);
//this function performs delegation and subsequent withdrawal
testWithdrawalWrapper(operator, depositor, withdrawer, ethAmount, eigenAmount, withdrawAsShares, true);
testWithdrawalWrapper(operator, depositor, ethAmount, eigenAmount, withdrawAsShares, true);

cheats.prank(depositor);
delegation.undelegate(depositor);
Expand All @@ -261,50 +261,6 @@ contract WithdrawalTests is EigenLayerTestHelper {
_initiateDelegation(operator, depositor, ethAmount, eigenAmount);
}

// onlyNotFrozen modifier is not used in current DelegationManager implementation.
// commented out test case for now
// /// @notice test to see if an operator who is slashed/frozen
// /// cannot be undelegated from by their stakers.
// /// @param operator is the operator being delegated to.
// /// @param staker is the staker delegating stake to the operator.
// function testSlashedOperatorWithdrawal(address operator, address staker, uint96 ethAmount, uint96 eigenAmount)
// public
// fuzzedAddress(operator)
// fuzzedAddress(staker)
// {
// cheats.assume(staker != operator);
// testDelegation(operator, staker, ethAmount, eigenAmount);

// {
// address slashingContract = slasher.owner();

// cheats.startPrank(operator);
// slasher.optIntoSlashing(address(slashingContract));
// cheats.stopPrank();

// cheats.startPrank(slashingContract);
// slasher.freezeOperator(operator);
// cheats.stopPrank();
// }

// (IStrategy[] memory updatedStrategies, uint256[] memory updatedShares) =
// strategyManager.getDeposits(staker);

// uint256[] memory strategyIndexes = new uint256[](2);
// strategyIndexes[0] = 0;
// strategyIndexes[1] = 1;

// IERC20[] memory tokensArray = new IERC20[](2);
// tokensArray[0] = weth;
// tokensArray[0] = eigenToken;

// //initiating queued withdrawal
// cheats.expectRevert(
// bytes("StrategyManager.onlyNotFrozen: staker has been frozen and may be subject to slashing")
// );
// _testQueueWithdrawal(staker, strategyIndexes, updatedStrategies, updatedShares, staker);
// }

// Helper function to begin a delegation
function _initiateDelegation(
address operator,
Expand Down
38 changes: 18 additions & 20 deletions src/test/unit/DelegationUnit.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2828,14 +2828,16 @@ contract DelegationManagerUnitTests_queueWithdrawals is DelegationManagerUnitTes
delegationManager.queueWithdrawals(queuedWithdrawalParams);
}

function test_Revert_WhenZeroAddressWithdrawer() public {
function test_Revert_WhenNotStakerWithdrawer(address withdrawer) public {
cheats.assume(withdrawer != defaultStaker);

(IDelegationManager.QueuedWithdrawalParams[] memory queuedWithdrawalParams, , ) = _setUpQueueWithdrawalsSingleStrat({
staker: defaultStaker,
withdrawer: address(0),
withdrawer: withdrawer,
strategy: strategyMock,
withdrawalAmount: 100
});
cheats.expectRevert("DelegationManager.queueWithdrawal: must provide valid withdrawal address");
cheats.expectRevert("DelegationManager.queueWithdrawal: withdrawer must be staker");
delegationManager.queueWithdrawals(queuedWithdrawalParams);
}

Expand Down Expand Up @@ -2977,6 +2979,7 @@ contract DelegationManagerUnitTests_queueWithdrawals is DelegationManagerUnitTes
uint256 randSalt
) public filterFuzzedAddressInputs(staker){
cheats.assume(depositAmounts.length > 0 && depositAmounts.length <= 32);
cheats.assume(staker != defaultOperator);
uint256[] memory withdrawalAmounts = _fuzzWithdrawalAmounts(depositAmounts);

IStrategy[] memory strategies = _deployAndDepositIntoStrategies(staker, depositAmounts);
Expand Down Expand Up @@ -3033,7 +3036,7 @@ contract DelegationManagerUnitTests_queueWithdrawals is DelegationManagerUnitTes
uint256[] memory depositAmounts,
uint256 randSalt
) public filterFuzzedAddressInputs(staker) {
cheats.assume(staker != withdrawer && withdrawer != address(0));
cheats.assume(staker != withdrawer);
cheats.assume(depositAmounts.length > 0 && depositAmounts.length <= 32);
uint256[] memory withdrawalAmounts = _fuzzWithdrawalAmounts(depositAmounts);

Expand All @@ -3052,8 +3055,11 @@ contract DelegationManagerUnitTests_queueWithdrawals is DelegationManagerUnitTes
});

// queueWithdrawals
// NOTE: Originally, you could queue a withdrawal to a different address, which would fail with a specific error
// if third party transfers were forbidden. Now, withdrawing to a different address is forbidden regardless
// of third party transfer status.
cheats.expectRevert(
"DelegationManager._removeSharesAndQueueWithdrawal: withdrawer must be same address as staker if thirdPartyTransfersForbidden are set"
"DelegationManager.queueWithdrawal: withdrawer must be staker"
);
cheats.prank(staker);
delegationManager.queueWithdrawals(queuedWithdrawalParams);
Expand Down Expand Up @@ -3247,12 +3253,10 @@ contract DelegationManagerUnitTests_completeQueuedWithdrawal is DelegationManage
*/
function test_completeQueuedWithdrawal_SingleStratWithdrawAsTokens(
address staker,
address withdrawer,
uint256 depositAmount,
uint256 withdrawalAmount
) public filterFuzzedAddressInputs(staker) {
cheats.assume(staker != defaultOperator);
cheats.assume(withdrawer != address(0));
cheats.assume(withdrawalAmount > 0 && withdrawalAmount <= depositAmount);
_registerOperatorWithBaseDetails(defaultOperator);
(
Expand All @@ -3261,7 +3265,7 @@ contract DelegationManagerUnitTests_completeQueuedWithdrawal is DelegationManage
bytes32 withdrawalRoot
) = _setUpCompleteQueuedWithdrawalSingleStrat({
staker: staker,
withdrawer: withdrawer,
withdrawer: staker,
depositAmount: depositAmount,
withdrawalAmount: withdrawalAmount
});
Expand All @@ -3271,7 +3275,7 @@ contract DelegationManagerUnitTests_completeQueuedWithdrawal is DelegationManage

// completeQueuedWithdrawal
cheats.roll(block.number + delegationManager.getWithdrawalDelay(withdrawal.strategies));
cheats.prank(withdrawer);
cheats.prank(staker);
cheats.expectEmit(true, true, true, true, address(delegationManager));
emit WithdrawalCompleted(withdrawalRoot);
delegationManager.completeQueuedWithdrawal(withdrawal, tokens, 0 /* middlewareTimesIndex */, true);
Expand All @@ -3291,21 +3295,20 @@ contract DelegationManagerUnitTests_completeQueuedWithdrawal is DelegationManage
*/
function test_completeQueuedWithdrawal_SingleStratWithdrawAsShares(
address staker,
address withdrawer,
uint256 depositAmount,
uint256 withdrawalAmount
) public filterFuzzedAddressInputs(staker) {
cheats.assume(staker != defaultOperator);
cheats.assume(withdrawer != defaultOperator && withdrawer != address(0));
cheats.assume(withdrawalAmount > 0 && withdrawalAmount <= depositAmount);
_registerOperatorWithBaseDetails(defaultOperator);

(
IDelegationManager.Withdrawal memory withdrawal,
IERC20[] memory tokens,
bytes32 withdrawalRoot
) = _setUpCompleteQueuedWithdrawalSingleStrat({
staker: staker,
withdrawer: withdrawer,
withdrawer: staker,
depositAmount: depositAmount,
withdrawalAmount: withdrawalAmount
});
Expand All @@ -3315,19 +3318,14 @@ contract DelegationManagerUnitTests_completeQueuedWithdrawal is DelegationManage

// completeQueuedWithdrawal
cheats.roll(block.number + delegationManager.getWithdrawalDelay(withdrawal.strategies));
cheats.prank(withdrawer);
cheats.prank(staker);
cheats.expectEmit(true, true, true, true, address(delegationManager));
emit WithdrawalCompleted(withdrawalRoot);
delegationManager.completeQueuedWithdrawal(withdrawal, tokens, 0 /* middlewareTimesIndex */, false);

uint256 operatorSharesAfter = delegationManager.operatorShares(defaultOperator, withdrawal.strategies[0]);
if (staker == withdrawer) {
// Since staker is delegated, operatorShares get incremented
assertEq(operatorSharesAfter, operatorSharesBefore + withdrawalAmount, "operator shares not increased correctly");
} else {
// Since withdrawer is not the staker and isn't delegated, staker's oeprator shares are unchanged
assertEq(operatorSharesAfter, operatorSharesBefore, "operator shares should be unchanged");
}
// Since staker is delegated, operatorShares get incremented
assertEq(operatorSharesAfter, operatorSharesBefore + withdrawalAmount, "operator shares not increased correctly");
assertFalse(delegationManager.pendingWithdrawals(withdrawalRoot), "withdrawalRoot should be completed and marked false now");
}
}
Loading