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 issue audit-6.4 Withdrawals That Are Not Self-Claimed Break the Accounting #57

Merged
merged 1 commit into from
Mar 27, 2024
Merged

Fix issue audit-6.4 Withdrawals That Are Not Self-Claimed Break the Accounting #57

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
69 changes: 34 additions & 35 deletions src/StakingNode.sol
View file
Open in desktop
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
Open in desktop
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);
xhad marked this conversation as resolved.
Show resolved Hide resolved
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;

/**
xhad marked this conversation as resolved.
Show resolved Hide resolved
/**
* @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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading