Danoctavian/sc 1162/implement generalized withdrawal queue manager #118

danoctavian · 2024-06-21T20:00:24Z

No description provided.

danoctavian · 2024-06-24T06:24:29Z

src/LSDStakingNode.sol

@@ -17,6 +17,8 @@ interface ILSDStakingNodeEvents {
    event DepositToEigenlayer(IERC20 indexed asset, IStrategy indexed strategy, uint256 amount, uint256 eigenShares);


can skip this file for now we're probably not going to include this in this release

danoctavian · 2024-06-24T06:58:13Z

src/StakingNodesManager.sol

+    /// @param nodeId The ID of the node processing the withdrawal.
+    /// @param amountToReinvest Amount of ETH to reinvest into ynETH.
+    /// @param amountToQueue Amount of ETH to send to the withdrawal queue.
+    function processPrincipalWithdrawalsForNode(


this permissioned potentially can be improved.

Gives full control currently for allocating funds back into pool vs withdrawal. This is the most flexible from our PoV.

Some potential changes:

make it supply ETH to withdrawal vault based on deficit
make it allocate funds for another stakingNode to better support predictably reallocation between 2 stakingNodes. (eg. move 100 validators from StakingNode 1 to StakingNode 2)

danoctavian · 2024-06-24T09:49:11Z

src/WithdrawalQueueManager.sol

+    //----------------------------------  WITHDRAWAL REQUESTS  -----------------------------
+    //--------------------------------------------------------------------------------------
+
+    function requestWithdrawal(uint256 amount) external nonReentrant {


What this abstraction doesn't support is the ability to request redemption in a particular asset or set off assets (eg. i request to exit ynLSDe in sfrxETH and rETH, maybe in a certain ratio).

This is enough for ynETH -> ETH.

We can spend more time thinking about that or extended this contract with an additional function when that use case arises.

The trickiness there is availability of said assets, as you'd be able to do that, given that you don't do any rebalancing only if those assets are available in the protocol, and that requires extra tracking and ability to inspect the state of "withdrawable" array of assets at the given time of requestWithdrawal and "book" them. This is for a token like ynLSDe, doesn't really apply for this release.

danoctavian · 2024-06-24T09:52:30Z

test/scenarios/fork/StakingNodeProofSubmission-Scenario.spec.sol

+import "forge-std/console.sol";
+
+
+contract StakingNodeTestBase is ScenarioBaseTest, ProofParsingV1 {


looking for opinions on how to structure these tests which run off of the existing deployment better.

This is for example one use case, since for proofs tests the most straighforward way is to test the existing real deployment and leave some validators "hanging" to test against them.

danoctavian · 2024-06-24T09:54:54Z

test/scenarios/fork/StakingNodeProofSubmission-Scenario.spec.sol

+import "forge-std/console.sol";
+
+
+contract StakingNodeTestBase is ScenarioBaseTest, ProofParsingV1 {


we have a more general problem with tests:

Tests that execute all tests on a fresh deployment vs tests that assume the deployment is the existing deployment.

The latter is the most accurate, since we roll the upgrade for ynETH on top of the existing deployment.

"Fresh" integration tests on a fresh deployment have the advantage that we can push the system to any configuration, while the mainnet one keeps moving underneat you (may break tests randomly, won't be able to reproduce certain system states without lots of prank-ing and etching which introduce error-prone additional mutations)

johnnyonline · 2024-06-25T11:06:07Z

src/WithdrawalQueueManager.sol

+    //--------------------------------------------------------------------------------------
+
+    function requestWithdrawal(uint256 amount) external nonReentrant {
+        if (amount <= 0) {


amount (uint256) cant be smaller than 0.

johnnyonline · 2024-06-25T11:06:48Z

src/WithdrawalQueueManager.sol

+            revert AmountMustBeGreaterThanZero();
+        }
+
+        redeemableAsset.transferFrom(msg.sender, address(this), amount);


Ignoring return value - use safeTransferFrom().

johnnyonline · 2024-06-25T11:07:15Z

src/WithdrawalQueueManager.sol

+        redeemableAsset.transferFrom(msg.sender, address(this), amount);
+
+        uint256 currentRate = redemptionAssetsVault.redemptionRate();
+        uint256 tokenId = _tokenIdCounter++;


++_tokenIdCounter will be more gas efficient.

johnnyonline · 2024-06-25T11:08:07Z

src/WithdrawalQueueManager.sol

+        _burn(tokenId);
+        redeemableAsset.burn(request.amount);
+
+        uint256 unitOfAccountAmount = calculateRedemptionAmount(request.amount, request.redemptionRateAtRequestTime);


Redundant calculation, can be done once (see L166).

johnnyonline · 2024-06-25T11:08:39Z

src/WithdrawalQueueManager.sol

+            revert InsufficientBalance(currentBalance, unitOfAccountAmount);
+        }
+
+        redemptionAssetsVault.transferRedemptionAssets(receiver, netUnitOfAccountAmount);


At this point, we check two times that the balance is sufficient, is this necessary?

johnnyonline · 2024-06-25T11:13:53Z

src/StakingNode.sol

+        uint256 sharesAmount
+    ) external onlyOperator returns (bytes32[] memory fullWithdrawalRoots) {
+
+        IDelegationManager delegationManager = IDelegationManager(address(stakingNodesManager.delegationManager()));


Redundant casts. can simply call stakingNodesManager.delegationManager(). Also redundant state variable though, used only once.

yep, lets remove the above line and replace line 367 with:

fullWithdrawalRoots = stakingNodesManager.delegationManager().queueWithdrawals(params);

johnnyonline · 2024-06-25T11:14:14Z

src/StakingNode.sol

+            }
+        }
+
+        IDelegationManager delegationManager = IDelegationManager(address(stakingNodesManager.delegationManager()));


Redundant casts. can simply call stakingNodesManager.delegationManager(). Also redundant state variable though, used only once.

indeed, lets simplify by removing the above line (412) and replacing line 422 with:

stakingNodesManager.delegationManager().completeQueuedWithdrawals( withdrawals, tokens, middlewareTimesIndexes, receiveAsTokens );

johnnyonline · 2024-06-25T11:14:54Z

src/StakingNode.sol

+        // 2. For beaconChainETHStrategy, the DelegationManager calls _withdrawSharesAsTokens interacts with the EigenPodManager.withdrawSharesAsTokens
+        // 3. Finally, the EigenPodManager calls withdrawRestakedBeaconChainETH on the EigenPod of this StakingNode to finalize the withdrawal.
+        // 4. the EigenPod decrements withdrawableRestakedExecutionLayerGwei and send the ETH to address(this)
+        delegationManager.completeQueuedWithdrawals(withdrawals, tokens, middlewareTimesIndexes, receiveAsTokens);


Noob question - will it always revert? because receive function limits sender to delayedWithdrawalRouter.

yeah i needed to fix it up!

johnnyonline · 2024-06-25T11:17:13Z

src/StakingNode.sol

+        unverifiedStakedETH += amount;
+    }
+
+    function deallocateStakedETH(uint256 amount) external payable onlyStakingNodesManager {


Should this be payable?

correct, no

johnnyonline · 2024-06-25T11:17:47Z

src/StakingNode.sol

+        withdrawnValidatorPrincipal -= amount;
+
+
+        (bool success, ) = address(stakingNodesManager).call{value: amount}("");


Can use msg.sender instead of address(stakingNodesManager).

xhad

Just a few tests that are failing, but otherwise this seems like a very solid base heading into the audit. My suggestion is that we should lean more heavily this time and collaboratively on the offchain and admin processes to ensure we have a very reliable secure system in place. Glad to hear you're up for taking the lead, but I'm def here to help and want to assist where you think is best.

xhad · 2024-07-01T05:57:47Z

script/Actors.sol

+                // TODO: fix for mainnet deployment
+                WITHDRAWAL_MANAGER: mainnetWallets.YNDev


Has this TODO been resolved?

No, we need a Withdrawal Manager Multisig created and decide the treshold and composition of that.

xhad · 2024-07-01T06:31:28Z

src/interfaces/IynETH.sol

@@ -3,8 +3,9 @@ pragma solidity ^0.8.24;

 import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
 import {IERC20Permit} from "lib/openzeppelin-contracts/contracts/token/ERC20/extensions/IERC20Permit.sol";


Unused Import

xhad · 2024-07-01T06:33:18Z

src/StakingNode.sol

@@ -4,6 +4,8 @@ pragma solidity ^0.8.24;
 import {ReentrancyGuardUpgradeable} from "lib/openzeppelin-contracts-upgradeable/contracts/utils/ReentrancyGuardUpgradeable.sol";
 import {BeaconChainProofs} from "lib/eigenlayer-contracts/src/contracts/libraries/BeaconChainProofs.sol";
 import {IDelegationManager } from "lib/eigenlayer-contracts/src/contracts/interfaces/IDelegationManager.sol";
+import {IEigenPodManager } from "lib/eigenlayer-contracts/src/contracts/interfaces/IEigenPodManager.sol";
+import {IDelayedWithdrawalRouter } from "lib/eigenlayer-contracts/src/contracts/interfaces/IDelayedWithdrawalRouter.sol";


IDelayedWithdrawRouter is unused

xhad · 2024-07-01T06:33:44Z

src/StakingNode.sol

@@ -12,7 +14,10 @@ import {IEigenPodManager} from "lib/eigenlayer-contracts/src/contracts/interface
 import {IStakingNodesManager} from "src/interfaces/IStakingNodesManager.sol";
 import {IStakingNode} from "src/interfaces/IStakingNode.sol";
 import {RewardsType} from "src/interfaces/IRewardsDistributor.sol";
+import {IERC20} from "lib/openzeppelin-contracts/contracts/interfaces/IERC20.sol";
+import {ONE_GWEI, DEFAULT_VALIDATOR_STAKE} from "src/Constants.sol";


ONE_GWEI is unused

xhad · 2024-07-01T06:39:43Z

src/StakingNode.sol

@@ -111,6 +135,10 @@ contract StakingNode is IStakingNode, StakingNodeEvents, ReentrancyGuardUpgradea
        nodeId = init.nodeId;
    }

+    function initializeV2(uint256 initialUnverifiedStakedETH) external onlyStakingNodesManager reinitializer(2) {
+        unverifiedStakedETH = initialUnverifiedStakedETH;
+    }


Q: How is the unverified staked ETH calculated?

The unverifiedStakedETH should ideally be 0 on upgrade time.

yieldnest-protocol/src/StakingNodesManager.sol

Line 400 in 4b93773

node.initializeV2(0);

We MUST ensure all validators are verified at time of upgrade to perform it as shown above.

xhad · 2024-07-02T14:44:25Z

src/HoleskyStakingNodesManager.sol

+import {IStakingNode} from "src/interfaces/IStakingNode.sol";
+import {IStakingNodesManager} from "src/interfaces/IStakingNodesManager.sol";
+
+contract HoleskyStakingNodesManager is StakingNodesManager {


I'm curious why we have HoleskyStakingNodesManager. Is this because of Eigenlayer verison difference between the networks?

Holesky has StakingNode with id with a fixed number of dangling validators by convention that remain unverified.

That number should be what the variable is initialized with and as with Mainnet, no other validators should be staked once that value is set to correspond to the beacon chain reality and on-chain reality.

For example, if we agree to have 20 unverified validators, we will initialize the unverifiedStakedETH variable with 20 * 32 ether.

xhad · 2024-07-02T14:47:27Z

src/StakingNode.sol


+import "forge-std/console.sol";


good to remove this prior to the audit

will clean all these up and turn this into a a full PR not just draft.

xhad · 2024-07-02T14:57:15Z

src/StakingNodesManager.sol

@@ -17,6 +17,9 @@ import {IStakingNode} from "src/interfaces/IStakingNode.sol";
 import {IStakingNodesManager} from "src/interfaces/IStakingNodesManager.sol";
 import {IynETH} from "src/interfaces/IynETH.sol";

+import "forge-std/console.sol";


good to remove prior to audit

xhad · 2024-07-02T15:04:36Z

src/WithdrawalQueueManager.sol

+import {IRedeemableAsset} from "src/interfaces/IRedeemableAsset.sol";
+import {IRedemptionAssetsVault} from "src/interfaces/IRedemptionAssetsVault.sol";
+
+import "forge-std/console.sol";


note to remove before audit

xhad · 2024-07-02T15:08:31Z

src/WithdrawalQueueManager.sol

+        address withdrawalQueueAdmin;
+        uint256 withdrawalFee;
+        address feeReceiver;
+


danoctavian · 2024-07-03T07:50:02Z

Just a few tests that are failing, but otherwise this seems like a very solid base heading into the audit. My suggestion is that we should lean more heavily this time and collaboratively on the offchain and admin processes to ensure we have a very reliable secure system in place. Glad to hear you're up for taking the lead, but I'm def here to help and want to assist where you think is best.

Great!

danoctavian · 2024-07-03T20:42:00Z

src/StakingNode.sol

+            // Decrease unverifiedStakedETH by DEFAULT_VALIDATOR_STAKE regardless if the current balance of the validator
+            // Since unverifiedStakedETH was increased by DEFAULT_VALIDATOR_STAKE when the validator was staked
+            // within the Beacon Chain
+            unverifiedStakedETH -= DEFAULT_VALIDATOR_STAKE;


there's a bug here.

if you decrease this by 32ETH, but verifyWithdrawalCredentials yields for example 0 shares, because validator was withdrawn, balance drops potentially without justification. assets may still be in the system.

danoctavian · 2024-07-05T14:14:39Z

src/StakingNodesManager.sol

+    ) 
+        public 
+        onlyRole(WITHDRAWAL_MANAGER_ROLE) 
+    {


there's a bug in totalAssets() as a followup to withdrawals.

What goes into witdrawal is not counted into TVL.

This is incorrect, it must be counted until the ynETH is burned.

danoctavian · 2024-07-05T15:34:15Z

src/StakingNodesManager.sol

@@ -119,6 +127,8 @@ contract StakingNodesManager is

    bool public validatorRegistrationPaused;

+    address public withdrawalAssetsVault;


Please pay great attention to memory layouts and that being respected when reviewed given that we use Proxy contracts for all.

kahuna099 · 2024-07-11T13:22:48Z

src/WithdrawalQueueManager.sol

+            revert FeePercentageExceedsLimit();
+        }
+        withdrawalFee = feePercentage;
+        emit WithdrawalFeeUpdated(feePercentage);


just like the above and below functions include the old and new value in the event args.

kahuna099 · 2024-07-11T13:29:24Z

src/WithdrawalQueueManager.sol

+     * @param index The index of the withdrawal request.
+     * @return True if the request is finalized, false otherwise.
+     */
+    function withdrawalRequestIsFinalized(uint256 index) public view returns (bool) {


this function will return true for nonexistent withdrawal requests.

If you pass in a nonexistent index, then WithdrawalRequest memory request = withdrawalRequests[index]; will return an empty WithdrawalRequest. Then isFinalized will check return block.timestamp >= request.creationTimestamp + secondsToFinalization;, which will become some-timestamp >= 0 + secondsToFianlization. And since secondsToFinalization is very very likely to be less than block.timestamp, the isFinalized check will return true, even though this withdrawal request does not exist.

fix: if (request.creationTimestamp == 0) revert WithdrawalRequestDoesNotExist(index);

kahuna099 · 2024-07-11T13:30:24Z

src/WithdrawalQueueManager.sol

+     * @param tokenId The token ID of the withdrawal request.
+     * @return request The withdrawal request details.
+     */
+    function withdrawalRequest(uint256 tokenId) public view returns (WithdrawalRequest memory request) {


look at the above comment, but it might make sense to just revert here if no withdrawal request with this id exists, instead of returning an empty WithdrawalRequest

src/ynETH.sol

kahuna099 · 2024-07-11T13:36:14Z

src/ynETH.sol

+        // ETH can be returned either by the stakinNodesManager or by the redemptionAssetsVault
+        if (!(msg.sender == address(stakingNodesManager)
+            || msg.sender == (address(stakingNodesManager.redemptionAssetsVault())))) {
+


there is nothing happening here, you are missing a revert in the if-body. Right now anybody can call processWithdrawnETH (not that that is such a big problem, but it shouldn't be allowed)

Also, just a personal preference but i think the if-condition is more readable if you write it like:

if (msg.sender != address(stakingNodesManager && msg.sender != address(stakingNodesManager.redemptionAssetsVault())) {

xhad · 2024-07-11T16:43:03Z

Just noticed this, a minor suggestion. Perhaps we can resolve all of the compiler warnings running the test suite prior to audit.

Compiler run successful with warnings:
Warning (2519): This declaration shadows an existing declaration.
   --> test/scenarios/fork/ynETHWithdrawals.t.sol:378:9:
    |
378 |         uint256 _secondsToFinalization = ynETHWithdrawalQueueManager.MAX_SECONDS_TO_FINALIZATION() + 1;
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Note: The shadowed declaration is here:
   --> test/scenarios/fork/ynETHWithdrawals.t.sol:375:76:
    |
375 |     function testSetSecondsToFinalizationSecondsToFinalizationExceedsLimit(uint256 _secondsToFinalization) public {
    |                                                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Warning (5667): Unused function parameter. Remove or comment out the variable name to silence this warning.
   --> test/utils/StakingNodeTestBase.sol:139:50:
    |
139 |     function setupForVerifyAndProcessWithdrawals(uint256 nodeId, string memory path) public {
    |                                                  ^^^^^^^^^^^^^^

Warning (2072): Unused local variable.
   --> test/utils/StakingNodeTestBase.sol:224:9:
    |
224 |         uint256 unverifiedStakedETH = stakingNodeInstance.getUnverifiedStakedETH();
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^

Warning (2072): Unused local variable.
   --> test/utils/StakingNodeTestBase.sol:225:9:
    |
225 |         int256 shares = eigenPodManager.podOwnerShares(address(stakingNodeInstance));
    |         ^^^^^^^^^^^^^

Warning (5667): Unused function parameter. Remove or comment out the variable name to silence this warning.
   --> test/scenarios/fork/ynETHWithdrawals.t.sol:375:76:
    |
375 |     function testSetSecondsToFinalizationSecondsToFinalizationExceedsLimit(uint256 _secondsToFinalization) public {

danoctavian · 2024-07-12T13:40:27Z

src/interfaces/IStakingNode.sol

+    ) external;
+
+    function queueWithdrawals(
+        uint256 sharesAmount


Question from @kahuna099

Why not withdraw
podOwnerShares[podOwner]

and have 0 params?

danoctavian commented Jun 24, 2024

View reviewed changes

danoctavian requested review from xhad, gearcat0 and johnnyonline June 24, 2024 09:55

johnnyonline reviewed Jun 25, 2024

View reviewed changes

danoctavian force-pushed the danoctavian/sc-1162/implement-generalized-withdrawal-queue-manager branch 2 times, most recently from df2bc0a to 6429a93 Compare June 30, 2024 22:28

xhad reviewed Jul 2, 2024

View reviewed changes

danoctavian closed this Jul 3, 2024

danoctavian reopened this Jul 3, 2024

danoctavian commented Jul 3, 2024

View reviewed changes

danoctavian force-pushed the danoctavian/sc-1162/implement-generalized-withdrawal-queue-manager branch 2 times, most recently from f576c48 to 82a8d61 Compare July 5, 2024 13:15

danoctavian commented Jul 5, 2024

View reviewed changes

danoctavian force-pushed the danoctavian/sc-1162/implement-generalized-withdrawal-queue-manager branch from 8351e67 to 3ef6bf8 Compare July 6, 2024 00:20

kahuna099 reviewed Jul 11, 2024

View reviewed changes

src/ynETH.sol Show resolved Hide resolved

kahuna099 reviewed Jul 11, 2024

View reviewed changes

danoctavian marked this pull request as ready for review July 12, 2024 08:27

danoctavian commented Jul 12, 2024

View reviewed changes

danoctavian added 28 commits October 1, 2024 20:02

add state assertions

5844c3d

enable state measurement

992e791

factor in rewards

7b0409f

fix assertions for multi validator test

592ceea

refactor test with utility functions up top

a6253bd

add test for ETH capture in new eigenpod

dd78564

add queueWithdrawals test

ef808e3

add test for queuedWithdrawals

7397fe3

finish completeQueuedWithdrawals test

72c4170

convert test to handle multiple validators

1d8b9db

fix test for slashing and withdrawals

08e0976

fix test for slashing with negative shares

ae0d8d4

remove bad code

9948232

remove commented out code

3c6f64e

remove unused scenario files

3cab514

fix version upgrade test

05b44a9

wip processPrincipalWithdrawals test

66a9463

move out StakingNodeBase

55f877c

fix validator creation

51794d5

extend StakingNodeTestBase

6343d56

disable test for stakingNodesManager

620c613

remove ProofUtils

090d51a

remove all test/data/ files related to proofs

68e78e6

delete scenario test

ef705d1

do formatting

c05b938

reduce number of validators in test to reduce gas

48cf1ba

reduce setup gas usage

3b27c9d

delete preprod holesky

2b540ab

xhad approved these changes Oct 2, 2024

View reviewed changes

danoctavian merged commit 87ec4ad into release-candidate Oct 2, 2024
1 check passed

		@@ -17,6 +17,8 @@ interface ILSDStakingNodeEvents {
		event DepositToEigenlayer(IERC20 indexed asset, IStrategy indexed strategy, uint256 amount, uint256 eigenShares);

		import "forge-std/console.sol";


		contract StakingNodeTestBase is ScenarioBaseTest, ProofParsingV1 {

		withdrawnValidatorPrincipal -= amount;


		(bool success, ) = address(stakingNodesManager).call{value: amount}("");

		// TODO: fix for mainnet deployment
		WITHDRAWAL_MANAGER: mainnetWallets.YNDev

		@@ -3,8 +3,9 @@ pragma solidity ^0.8.24;

		import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
		import {IERC20Permit} from "lib/openzeppelin-contracts/contracts/token/ERC20/extensions/IERC20Permit.sol";

		@@ -119,6 +127,8 @@ contract StakingNodesManager is

		bool public validatorRegistrationPaused;

		address public withdrawalAssetsVault;

Danoctavian/sc 1162/implement generalized withdrawal queue manager #118

Danoctavian/sc 1162/implement generalized withdrawal queue manager #118

Conversation

danoctavian commented Jun 21, 2024

Choose a reason for hiding this comment

danoctavian Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

danoctavian Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kahuna099 Jul 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xhad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danoctavian commented Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danoctavian Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xhad commented Jul 11, 2024

Choose a reason for hiding this comment

danoctavian Jun 24, 2024 •

edited

Loading

danoctavian Jun 24, 2024 •

edited

Loading

kahuna099 Jul 13, 2024 •

edited

Loading

danoctavian commented Jul 3, 2024 •

edited

Loading

danoctavian Jul 5, 2024 •

edited

Loading