Skip to content

Commit

Permalink
Merge pull request #57 from yieldnest/feature/fix/claimDelayedWithdra…
Browse files Browse the repository at this point in the history 
…wals-audit-fix

Fix issue audit-6.4 Withdrawals That Are Not Self-Claimed Break the Accounting
  • Loading branch information
@danoctavian
danoctavian authored Mar 27, 2024
2 parents 7fdab4b + 983f392 commit 037a183
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 53 deletions.
69 changes: 34 additions & 35 deletions src/StakingNode.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ interface StakingNodeEvents {
event Delegated(address indexed operator, bytes32 approverSalt);
event Undelegated(address indexed operator);
event ClaimedDelayedWithdrawal(uint256 claimedAmount, uint256 withdrawnValidatorPrincipal, uint256 allocatedETH);
event WithdrawalsProcessed(uint256 claimedAmount, uint256 totalValidatorPrincipal, uint256 allocatedETH);
event ETHReceived(address sender, uint256 value);
event WithdrawnBeforeRestaking(uint256 eigenPodBalance);
}

/**
Expand All @@ -41,7 +44,7 @@ contract StakingNode is IStakingNode, StakingNodeEvents, ReentrancyGuardUpgradea
error MismatchedOracleBlockNumberAndValidatorIndexLengths(uint256 oracleBlockNumberLength, uint256 validatorIndexLength);
error MismatchedValidatorIndexAndProofsLengths(uint256 validatorIndexLength, uint256 proofsLength);
error MismatchedProofsAndValidatorFieldsLengths(uint256 proofsLength, uint256 validatorFieldsLength);

error UnexpectedETHBalance(uint256 claimedAmount, uint256 expectedETHBalance);

//--------------------------------------------------------------------------------------
//---------------------------------- CONSTANTS ---------------------------------------
Expand Down Expand Up @@ -81,6 +84,7 @@ contract StakingNode is IStakingNode, StakingNodeEvents, ReentrancyGuardUpgradea
if (msg.sender != address(stakingNodesManager.delayedWithdrawalRouter())) {
revert ETHDepositorNotDelayedWithdrawalRouter();
}
emit ETHReceived(msg.sender, msg.value);
}

constructor() {
Expand Down Expand Up @@ -127,52 +131,47 @@ contract StakingNode is IStakingNode, StakingNodeEvents, ReentrancyGuardUpgradea
* validators sweep them to the withdrawal address
*/
function withdrawBeforeRestaking() external onlyAdmin {

uint256 eigenPodBalance = address(eigenPod).balance;
emit WithdrawnBeforeRestaking(eigenPodBalance);
eigenPod.withdrawBeforeRestaking();
}

/// @notice Retrieves and processes withdrawals that have been queued in the EigenPod, transferring them to the StakingNode.
/// @param maxNumWithdrawals the upper limit of queued withdrawals to process in a single transaction.
/// @dev Ideally, you should call this with "maxNumWithdrawals" set to the total number of unclaimed withdrawals.
/// However, if the queue becomes too large to handle in one transaction, you can specify a smaller number.
function claimDelayedWithdrawals(uint256 maxNumWithdrawals, uint256 withdrawnValidatorPrincipal) public nonReentrant onlyAdmin {

if (withdrawnValidatorPrincipal > allocatedETH) {
revert WithdrawalPrincipalAmountTooHigh(withdrawnValidatorPrincipal, allocatedETH);
}
/**
* @notice Processes withdrawals by verifying the node's balance and transferring ETH to the StakingNodesManager.
* @dev This function checks if the node's current balance matches the expected balance and then transfers the ETH to the StakingNodesManager.
* @param totalValidatorPrincipal The total principal amount of the validators.
* @param expectedETHBalance The expected balance of the node after withdrawals.
*/
function processWithdrawals(
uint256 totalValidatorPrincipal,
uint256 expectedETHBalance
) public nonReentrant onlyAdmin {

IDelayedWithdrawalRouter delayedWithdrawalRouter = stakingNodesManager.delayedWithdrawalRouter();
uint256 claimableAmount = address(this).balance;

uint256 totalClaimable = 0;
IDelayedWithdrawalRouter.DelayedWithdrawal[] memory claimableWithdrawals = delayedWithdrawalRouter.getClaimableUserDelayedWithdrawals(address(this));
for (uint256 i = 0; i < claimableWithdrawals.length; i++) {
totalClaimable += claimableWithdrawals[i].amount;
if (totalValidatorPrincipal > allocatedETH) {
revert WithdrawalPrincipalAmountTooHigh(totalValidatorPrincipal, allocatedETH);
}

if (totalClaimable < withdrawnValidatorPrincipal) {
revert ValidatorPrincipalExceedsTotalClaimable(withdrawnValidatorPrincipal, totalClaimable);
if (totalValidatorPrincipal > claimableAmount) {
revert ValidatorPrincipalExceedsTotalClaimable(totalValidatorPrincipal, claimableAmount);
}

// only claim if we have active unclaimed withdrawals
// the ETH funds are sent to address(this) and trigger the receive() function
if (totalClaimable > 0) {

uint256 balanceBefore = address(this).balance;
delayedWithdrawalRouter.claimDelayedWithdrawals(address(this), maxNumWithdrawals);
uint256 balanceAfter = address(this).balance;

uint256 claimedAmount = balanceAfter - balanceBefore;

if (totalClaimable > claimedAmount) {
revert ClaimAmountTooLow(totalClaimable, claimedAmount);
}
// substract validator principal
allocatedETH -= withdrawnValidatorPrincipal;

stakingNodesManager.processWithdrawnETH{value: claimedAmount}(nodeId, withdrawnValidatorPrincipal);
emit ClaimedDelayedWithdrawal(claimedAmount, claimedAmount, allocatedETH);
// This check ensures that the actual balance of the contract matches the expected balance after withdrawals.
// Ensures that the totalValidatorPrincipal is not out of sync with the address(this).balance
// by the time it reaches the on-chain
if (expectedETHBalance != claimableAmount) {
revert UnexpectedETHBalance(claimableAmount, expectedETHBalance);
}
// substract validator principal
allocatedETH -= totalValidatorPrincipal;

stakingNodesManager.processWithdrawnETH{value: claimableAmount}(nodeId, totalValidatorPrincipal);
emit WithdrawalsProcessed(claimableAmount, totalValidatorPrincipal, allocatedETH);
}


//--------------------------------------------------------------------------------------
//---------------------------------- VERIFICATION AND DELEGATION --------------------
//--------------------------------------------------------------------------------------
Expand Down
24 changes: 17 additions & 7 deletions src/StakingNodesManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,13 @@ import {IynETH} from "./interfaces/IynETH.sol";

interface StakingNodesManagerEvents {
event StakingNodeCreated(address indexed nodeAddress, address indexed podAddress);
event ValidatorRegistered(uint256 nodeId, bytes signature, bytes pubKey, bytes32 depositRoot);
event ValidatorRegistered(uint256 nodeId, bytes signature, bytes pubKey, bytes32 depositRoot, bytes withdrawalCredentials);
event MaxNodeCountUpdated(uint256 maxNodeCount);
event ValidatorRegistrationPausedSet(bool isPaused);
event WithdrawnETHProcessed(uint256 nodeId, uint256 value, uint256 withdrawnValidatorPrincipal, uint256 rewards);
event RegisteredStakingNodeImplementationContract(address upgradeableBeaconAddress, address implementationContract);
event UpgradedStakingNodeImplementationContract(address implementationContract, uint256 nodesCount);
event NodeInitialized(address nodeAddress, uint64 initializedVersion);
}

contract StakingNodesManager is
Expand Down Expand Up @@ -91,10 +95,6 @@ contract StakingNodesManager is
IynETH public ynETH;
IRewardsDistributor public rewardsDistributor;

Validator[] public validators;

bool public validatorRegistrationPaused;

/**
/**
* @notice Each node in the StakingNodesManager manages an EigenPod.
Expand All @@ -110,8 +110,11 @@ contract StakingNodesManager is
IStakingNode[] public nodes;
uint256 public maxNodeCount;

Validator[] public validators;
mapping(bytes pubkey => bool) usedValidators;

bool public validatorRegistrationPaused;

//--------------------------------------------------------------------------------------
//---------------------------------- INITIALIZATION ----------------------------------
//--------------------------------------------------------------------------------------
Expand Down Expand Up @@ -285,7 +288,8 @@ contract StakingNodesManager is
nodeId,
validator.signature,
validator.publicKey,
depositDataRoot
depositDataRoot,
withdrawalCredentials
);
}

Expand Down Expand Up @@ -357,8 +361,8 @@ contract StakingNodesManager is

// update to the newly upgraded version.
initializedVersion = node.getInitializedVersion();
emit NodeInitialized(address(node), initializedVersion);
}

// NOTE: for future versions add additional if clauses that initialize the node
// for the next version while keeping the previous initializers
}
Expand All @@ -373,6 +377,8 @@ contract StakingNodesManager is
}

upgradeableBeacon = new UpgradeableBeacon(_implementationContract, address(this));

emit RegisteredStakingNodeImplementationContract(address(upgradeableBeacon), _implementationContract);
}

function upgradeStakingNodeImplementation(address _implementationContract)
Expand All @@ -388,6 +394,8 @@ contract StakingNodesManager is
for (uint256 i = 0; i < nodes.length; i++) {
initializeStakingNode(nodes[i]);
}

emit UpgradedStakingNodeImplementationContract(_implementationContract, nodes.length);
}

/// @notice Sets the maximum number of staking nodes allowed
Expand All @@ -414,6 +422,8 @@ contract StakingNodesManager is
revert TransferFailed();
}

emit WithdrawnETHProcessed(nodeId, msg.value, withdrawnValidatorPrincipal, rewards);

ynETH.processWithdrawnETH{value: withdrawnValidatorPrincipal}();
}

Expand Down
3 changes: 1 addition & 2 deletions src/interfaces/IStakingNode.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@ interface IStakingNode {
function delegate(address operator) external;
function undelegate() external;
function withdrawBeforeRestaking() external;
function claimDelayedWithdrawals(uint256 maxNumWithdrawals, uint PendingWithdrawnValidatorPrincipal) external;

function processWithdrawals(uint256 totalValidatorPrincipal, uint256 expectedETHBalance) external;

function implementation() external view returns (address);

Expand Down
27 changes: 19 additions & 8 deletions test/foundry/integration/StakingNode.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,11 @@ contract StakingNodeEigenPod is StakingNodeTestBase {
uint256 withdrawalDelayBlocks = delayedWithdrawalRouter.withdrawalDelayBlocks();
vm.roll(block.number + withdrawalDelayBlocks + 1);

delayedWithdrawalRouter.claimDelayedWithdrawals(address(stakingNodeInstance), type(uint256).max);

uint256 balanceBeforeClaim = address(consensusLayerReceiver).balance;
vm.prank(actors.STAKING_NODES_ADMIN);
stakingNodeInstance.claimDelayedWithdrawals(type(uint256).max, 0);
stakingNodeInstance.processWithdrawals(0, address(stakingNodeInstance).balance);
uint256 balanceAfterClaim = address(consensusLayerReceiver).balance;
uint256 rewardsAmount = balanceAfterClaim - balanceBeforeClaim;

Expand Down Expand Up @@ -145,9 +147,11 @@ contract StakingNodeWithdrawWithoutRestaking is StakingNodeTestBase {
uint256 withdrawalDelayBlocks = delayedWithdrawalRouter.withdrawalDelayBlocks();
vm.roll(block.number + withdrawalDelayBlocks + 1);

delayedWithdrawalRouter.claimDelayedWithdrawals(address(stakingNodeInstance), type(uint256).max);

uint256 balanceBeforeClaim = address(consensusLayerReceiver).balance;
vm.prank(actors.STAKING_NODES_ADMIN);
stakingNodeInstance.claimDelayedWithdrawals(type(uint256).max, 0);
stakingNodeInstance.processWithdrawals(0, address(stakingNodeInstance).balance);
uint256 balanceAfterClaim = address(consensusLayerReceiver).balance;
uint256 rewardsAmount = balanceAfterClaim - balanceBeforeClaim;

Expand All @@ -172,9 +176,11 @@ contract StakingNodeWithdrawWithoutRestaking is StakingNodeTestBase {
uint256 withdrawalDelayBlocks = delayedWithdrawalRouter.withdrawalDelayBlocks();
vm.roll(block.number + withdrawalDelayBlocks + 1);

delayedWithdrawalRouter.claimDelayedWithdrawals(address(stakingNodeInstance), type(uint256).max);

uint256 balanceBeforeClaim = address(consensusLayerReceiver).balance;
vm.prank(actors.STAKING_NODES_ADMIN);
stakingNodeInstance.claimDelayedWithdrawals(type(uint256).max, 0);
stakingNodeInstance.processWithdrawals(0, address(stakingNodeInstance).balance);
uint256 balanceAfterClaim = address(consensusLayerReceiver).balance;
uint256 rewardsAmount = balanceAfterClaim - balanceBeforeClaim;

Expand Down Expand Up @@ -203,13 +209,15 @@ contract StakingNodeWithdrawWithoutRestaking is StakingNodeTestBase {
IDelayedWithdrawalRouter delayedWithdrawalRouter = stakingNodesManager.delayedWithdrawalRouter();
vm.roll(block.number + delayedWithdrawalRouter.withdrawalDelayBlocks() + 1);

delayedWithdrawalRouter.claimDelayedWithdrawals(address(stakingNodeInstance), type(uint256).max);

uint256 balanceBeforeClaim = address(consensusLayerReceiver).balance;

uint256 withdrawnValidators = activeValidators - 1;
uint256 validatorPrincipal = withdrawnValidators * 32 ether;

vm.prank(actors.STAKING_NODES_ADMIN);
stakingNodeInstance.claimDelayedWithdrawals(type(uint256).max, validatorPrincipal);
stakingNodeInstance.processWithdrawals(validatorPrincipal, address(stakingNodeInstance).balance);
uint256 balanceAfterClaim = address(consensusLayerReceiver).balance;
uint256 rewardsAmount = balanceAfterClaim - balanceBeforeClaim;

Expand Down Expand Up @@ -241,15 +249,16 @@ contract StakingNodeWithdrawWithoutRestaking is StakingNodeTestBase {
IDelayedWithdrawalRouter delayedWithdrawalRouter = stakingNodesManager.delayedWithdrawalRouter();
vm.roll(block.number + delayedWithdrawalRouter.withdrawalDelayBlocks() + 1);

delayedWithdrawalRouter.claimDelayedWithdrawals(address(stakingNodeInstance), type(uint256).max);

uint256 tooLargeValidatorPrincipal = validatorPrincipal;

// Attempt to claim withdrawals with a validator principal that exceeds total claimable amount
vm.prank(actors.STAKING_NODES_ADMIN);
vm.expectRevert(abi.encodeWithSelector(StakingNode.ValidatorPrincipalExceedsTotalClaimable.selector, tooLargeValidatorPrincipal, rewardsSweeped));
stakingNodeInstance.claimDelayedWithdrawals(type(uint256).max, tooLargeValidatorPrincipal); // Exceeding by 1 ether
stakingNodeInstance.processWithdrawals(tooLargeValidatorPrincipal, address(stakingNodeInstance).balance);
}


function testWithdrawalPrincipalAmountTooHigh() public {

uint256 activeValidators = 5;
Expand All @@ -272,13 +281,15 @@ contract StakingNodeWithdrawWithoutRestaking is StakingNodeTestBase {
IDelayedWithdrawalRouter delayedWithdrawalRouter = stakingNodesManager.delayedWithdrawalRouter();
vm.roll(block.number + delayedWithdrawalRouter.withdrawalDelayBlocks() + 1);

delayedWithdrawalRouter.claimDelayedWithdrawals(address(stakingNodeInstance), type(uint256).max);

uint256 tooLargeValidatorPrincipal = validatorPrincipal + 1 ether;
uint256 expectedAllocatedETH = depositAmount;

// Attempt to claim withdrawals with a validator principal that exceeds total claimable amount
vm.prank(actors.STAKING_NODES_ADMIN);
vm.expectRevert(abi.encodeWithSelector(StakingNode.WithdrawalPrincipalAmountTooHigh.selector, tooLargeValidatorPrincipal, expectedAllocatedETH));
stakingNodeInstance.claimDelayedWithdrawals(type(uint256).max, tooLargeValidatorPrincipal); // Exceeding by 1 ether
stakingNodeInstance.processWithdrawals(tooLargeValidatorPrincipal, address(stakingNodeInstance).balance);
}
}

Expand Down Expand Up @@ -363,7 +374,7 @@ contract StakingNodeVerifyWithdrawalCredentials is StakingNodeTestBase {

vm.prank(actors.STAKING_NODES_ADMIN);
vm.expectRevert();
stakingNodeInstance.claimDelayedWithdrawals(type(uint256).max, 1);
stakingNodeInstance.processWithdrawals(1, address(stakingNodeInstance).balance);
}

function testDelegateFailWhenNotAdmin() public {
Expand Down
5 changes: 4 additions & 1 deletion test/foundry/scenarios/ynETH.spec.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,12 +299,15 @@ contract YnETHScenarioTest8 is IntegrationBaseTest, YnETHScenarioTest3 {
assertEq(claimableDelayedWithdrawalsWarp.length, 1);
assertEq(claimableDelayedWithdrawalsWarp[0].amount, amount, "claimableDelayedWithdrawalsWarp[0].amount != 3 ether");


withdrawalRouter.claimDelayedWithdrawals(address(stakingNode), type(uint256).max);

// We can now claim the delayedWithdrawal
uint256 withdrawnValidatorPrincipal = stakingNode.getETHBalance();
vm.prank(address(actors.STAKING_NODES_ADMIN));

// Divided the withdrawnValidatorPrincipal by 2 to simulate the rewards distribution
stakingNode.claimDelayedWithdrawals(1, withdrawnValidatorPrincipal / 2);
stakingNode.processWithdrawals(withdrawnValidatorPrincipal / 2, address(stakingNode).balance);

// Get the rewards receiver addresses from the rewards distributor
IRewardsDistributor rewardsDistributor = IRewardsDistributor(stakingNodesManager.rewardsDistributor());
Expand Down

0 comments on commit 037a183

Please sign in to comment.