From 983f392a4aad6ad4be6ac39c0a37d2a4a8c1aa3b Mon Sep 17 00:00:00 2001 From: danoctavian Date: Tue, 26 Mar 2024 17:26:51 +0200 Subject: [PATCH] Fix issue audit-6.4 --- src/StakingNode.sol | 69 +++++++++++----------- src/StakingNodesManager.sol | 24 +++++--- src/interfaces/IStakingNode.sol | 3 +- test/foundry/integration/StakingNode.t.sol | 27 ++++++--- test/foundry/scenarios/ynETH.spec.sol | 5 +- 5 files changed, 75 insertions(+), 53 deletions(-) diff --git a/src/StakingNode.sol b/src/StakingNode.sol index 25dd1a720..21ea89fa3 100644 --- a/src/StakingNode.sol +++ b/src/StakingNode.sol @@ -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); } /** @@ -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 --------------------------------------- @@ -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() { @@ -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 -------------------- //-------------------------------------------------------------------------------------- diff --git a/src/StakingNodesManager.sol b/src/StakingNodesManager.sol index c20ed9568..9792aba26 100644 --- a/src/StakingNodesManager.sol +++ b/src/StakingNodesManager.sol @@ -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 @@ -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. @@ -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 ---------------------------------- //-------------------------------------------------------------------------------------- @@ -285,7 +288,8 @@ contract StakingNodesManager is nodeId, validator.signature, validator.publicKey, - depositDataRoot + depositDataRoot, + withdrawalCredentials ); } @@ -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 } @@ -373,6 +377,8 @@ contract StakingNodesManager is } upgradeableBeacon = new UpgradeableBeacon(_implementationContract, address(this)); + + emit RegisteredStakingNodeImplementationContract(address(upgradeableBeacon), _implementationContract); } function upgradeStakingNodeImplementation(address _implementationContract) @@ -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 @@ -414,6 +422,8 @@ contract StakingNodesManager is revert TransferFailed(); } + emit WithdrawnETHProcessed(nodeId, msg.value, withdrawnValidatorPrincipal, rewards); + ynETH.processWithdrawnETH{value: withdrawnValidatorPrincipal}(); } diff --git a/src/interfaces/IStakingNode.sol b/src/interfaces/IStakingNode.sol index 604f5301a..3be485e84 100644 --- a/src/interfaces/IStakingNode.sol +++ b/src/interfaces/IStakingNode.sol @@ -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); diff --git a/test/foundry/integration/StakingNode.t.sol b/test/foundry/integration/StakingNode.t.sol index 958cae888..c873f28b6 100644 --- a/test/foundry/integration/StakingNode.t.sol +++ b/test/foundry/integration/StakingNode.t.sol @@ -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; @@ -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; @@ -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; @@ -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; @@ -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; @@ -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); } } @@ -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 { diff --git a/test/foundry/scenarios/ynETH.spec.sol b/test/foundry/scenarios/ynETH.spec.sol index 749fab4cc..c4bf33ac0 100644 --- a/test/foundry/scenarios/ynETH.spec.sol +++ b/test/foundry/scenarios/ynETH.spec.sol @@ -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());