From da9f3dc31be528393a0ecb44ed3da7d75385fcb8 Mon Sep 17 00:00:00 2001 From: Yash Patil <40046473+ypatil12@users.noreply.github.com> Date: Mon, 3 Feb 2025 12:15:46 -0500 Subject: [PATCH] fix: ep negative shares bug (#1033) * fix: ep negative shares bug * fix: comments * test: add integration tests for neg shares * chore: remove logs * chore: use already calculated delta * chore: use stable foundry release in CI --- .github/workflows/certora-prover.yml | 2 +- .github/workflows/coverage.yml | 2 +- .github/workflows/deploy-local.yml | 2 +- .github/workflows/format.yml | 2 +- .github/workflows/storage-report.yml | 2 +- .github/workflows/testinparallel.yml | 2 +- src/contracts/pods/EigenPodManager.sol | 10 +- src/test/integration/IntegrationBase.t.sol | 8 +- .../upgrade/EigenPod_Negative_Shares.t.sol | 133 ++++++++++++++++++ src/test/integration/users/User.t.sol | 6 +- src/test/integration/users/User_M2.t.sol | 2 +- src/test/unit/EigenPodManagerUnit.t.sol | 64 +++++++++ 12 files changed, 222 insertions(+), 13 deletions(-) create mode 100644 src/test/integration/tests/upgrade/EigenPod_Negative_Shares.t.sol diff --git a/.github/workflows/certora-prover.yml b/.github/workflows/certora-prover.yml index 39a62830a5..5987dc1807 100644 --- a/.github/workflows/certora-prover.yml +++ b/.github/workflows/certora-prover.yml @@ -32,7 +32,7 @@ jobs: - name: Install Foundry uses: foundry-rs/foundry-toolchain@v1 with: - version: nightly + version: stable - name: Install forge dependencies run: forge install - name: Install python diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index 82f78a7941..535d51034f 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -50,7 +50,7 @@ jobs: - name: Install Foundry uses: foundry-rs/foundry-toolchain@v1 with: - version: nightly + version: stable - name: Run coverage run: forge coverage --report lcov env: diff --git a/.github/workflows/deploy-local.yml b/.github/workflows/deploy-local.yml index 33b29c10a6..25f9dd9a6a 100644 --- a/.github/workflows/deploy-local.yml +++ b/.github/workflows/deploy-local.yml @@ -30,7 +30,7 @@ jobs: - name: Install Foundry uses: foundry-rs/foundry-toolchain@v1 with: - version: nightly + version: stable - name: Run forge install run: forge install - name: Start anvil and deploy diff --git a/.github/workflows/format.yml b/.github/workflows/format.yml index 1869ccf191..0edb734c11 100644 --- a/.github/workflows/format.yml +++ b/.github/workflows/format.yml @@ -18,7 +18,7 @@ jobs: - name: Install Foundry uses: foundry-rs/foundry-toolchain@v1 with: - version: nightly + version: stable - name: Run forge fmt run: | forge fmt --check src/contracts diff --git a/.github/workflows/storage-report.yml b/.github/workflows/storage-report.yml index d4665b0155..f95b2099e9 100644 --- a/.github/workflows/storage-report.yml +++ b/.github/workflows/storage-report.yml @@ -16,7 +16,7 @@ jobs: - name: Install Foundry uses: foundry-rs/foundry-toolchain@v1 with: - version: nightly + version: stable - name: "Generate and prepare the storage reports for current branch" run: | diff --git a/.github/workflows/testinparallel.yml b/.github/workflows/testinparallel.yml index 82006ade75..fd4c63be13 100644 --- a/.github/workflows/testinparallel.yml +++ b/.github/workflows/testinparallel.yml @@ -34,7 +34,7 @@ jobs: - name: Install Foundry uses: foundry-rs/foundry-toolchain@v1 with: - version: nightly + version: stable - name: Run Forge build run: | diff --git a/src/contracts/pods/EigenPodManager.sol b/src/contracts/pods/EigenPodManager.sol index c1ffc9dea4..03d304e387 100644 --- a/src/contracts/pods/EigenPodManager.sol +++ b/src/contracts/pods/EigenPodManager.sol @@ -262,8 +262,14 @@ contract EigenPodManager is if (updatedDepositShares <= 0) { return (0, 0); } - - return (prevDepositShares < 0 ? 0 : uint256(prevDepositShares), shares); + // If we have gone from negative to positive shares, return (0, positive delta) + else if (prevDepositShares < 0) { + return (0, uint256(updatedDepositShares)); + } + // Else, return true previous shares and added shares + else { + return (uint256(prevDepositShares), shares); + } } /// @dev Calculates the proportion a pod owner's restaked balance has decreased, and diff --git a/src/test/integration/IntegrationBase.t.sol b/src/test/integration/IntegrationBase.t.sol index 95910a00e2..1afb808beb 100644 --- a/src/test/integration/IntegrationBase.t.sol +++ b/src/test/integration/IntegrationBase.t.sol @@ -2017,7 +2017,13 @@ abstract contract IntegrationBase is IntegrationDeployer, TypeImporter { } function _calcNativeETHOperatorShareDelta(User staker, int shareDelta) internal view returns (int) { - int curPodOwnerShares = eigenPodManager.podOwnerDepositShares(address(staker)); + // TODO: Maybe we update parent method to have an M2 and Slashing version? + int curPodOwnerShares; + if (!isUpgraded) { + curPodOwnerShares = IEigenPodManager_DeprecatedM2(address(eigenPodManager)).podOwnerShares(address(staker)); + } else { + curPodOwnerShares = eigenPodManager.podOwnerDepositShares(address(staker)); + } int newPodOwnerShares = curPodOwnerShares + shareDelta; if (curPodOwnerShares <= 0) { diff --git a/src/test/integration/tests/upgrade/EigenPod_Negative_Shares.t.sol b/src/test/integration/tests/upgrade/EigenPod_Negative_Shares.t.sol new file mode 100644 index 0000000000..0897123c93 --- /dev/null +++ b/src/test/integration/tests/upgrade/EigenPod_Negative_Shares.t.sol @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: BUSL-1.1 +pragma solidity ^0.8.27; + +import "src/test/integration/UpgradeTest.t.sol"; + +contract Integration_Upgrade_EigenPod_Negative_Shares is UpgradeTest { + function _init() internal override { + _configAssetTypes(HOLDS_ETH); + _configUserTypes(DEFAULT); + } + + function testFuzz_deposit_delegate_updateBalance_upgrade_completeAsShares( + uint24 _rand + ) public rand(_rand) { + /// 0. Create an operator and staker with some underlying assets + (User staker, IStrategy[] memory strategies, uint256[] memory tokenBalances) = _newRandomStaker(); + (User operator,,) = _newRandomOperator(); + uint256[] memory shares = _calculateExpectedShares(strategies, tokenBalances); + + /// 1. Deposit into strategies + staker.depositIntoEigenlayer(strategies, tokenBalances); + + /// 2. Delegate to operator + staker.delegateTo(operator); + + /// 3. Queue a withdrawal for all shares + IDelegationManagerTypes.Withdrawal[] memory withdrawals = staker.queueWithdrawals(strategies, shares); + IDelegationManagerTypes.Withdrawal memory withdrawal = withdrawals[0]; + + /// 4. Update balance randomly (can be positive or negative) + (int256[] memory tokenDeltas, int256[] memory balanceUpdateShareDelta,) = _randBalanceUpdate(staker, strategies); + staker.updateBalances(strategies, tokenDeltas); + + /// 5. Upgrade contracts + _upgradeEigenLayerContracts(); + + /// 6. Complete the withdrawal as shares + _rollBlocksForCompleteWithdrawals(withdrawals); + staker.completeWithdrawalAsShares(withdrawal); + + // Manually complete checks since we could still negative shares prior to the upgrade, causing a revert in the share check + (uint256[] memory expectedOperatorShareDelta, int256[] memory expectedStakerShareDelta) = + _getPostWithdrawalExpectedShareDeltas(balanceUpdateShareDelta[0], withdrawal); + assert_WithdrawalNotPending(delegationManager.calculateWithdrawalRoot(withdrawal), "staker withdrawal should no longer be pending"); + assert_Snap_Unchanged_TokenBalances(staker, "staker should not have any change in underlying token balances"); + assert_Snap_Added_OperatorShares(operator, withdrawal.strategies, expectedOperatorShareDelta, "operator should have received shares"); + assert_Snap_Delta_StakerShares(staker, strategies, expectedStakerShareDelta, "staker should have received expected shares"); + } + + function testFuzz_deposit_delegate_updateBalance_upgrade_completeAsTokens( + uint24 _rand + ) public rand(_rand) { + /// 0. Create an operator and staker with some underlying assets + (User staker, IStrategy[] memory strategies, uint256[] memory tokenBalances) = _newRandomStaker(); + (User operator,,) = _newRandomOperator(); + uint256[] memory shares = _calculateExpectedShares(strategies, tokenBalances); + + /// 1. Deposit into strategies + staker.depositIntoEigenlayer(strategies, tokenBalances); + + /// 2. Delegate to operator + staker.delegateTo(operator); + + /// 3. Queue a withdrawal for all shares + IDelegationManagerTypes.Withdrawal[] memory withdrawals = staker.queueWithdrawals(strategies, shares); + IDelegationManagerTypes.Withdrawal memory withdrawal = withdrawals[0]; + + /// 4. Update balance randomly (can be positive or negative) + (int256[] memory tokenDeltas, int256[] memory balanceUpdateShareDelta,) = _randBalanceUpdate(staker, strategies); + staker.updateBalances(strategies, tokenDeltas); + + /// 5. Upgrade contracts + _upgradeEigenLayerContracts(); + + /// 6. Complete the withdrawal as shares + _rollBlocksForCompleteWithdrawals(withdrawals); + IERC20[] memory tokens = staker.completeWithdrawalAsTokens(withdrawal); + uint256[] memory expectedTokens = _getPostWithdrawalExpectedTokenDeltas(balanceUpdateShareDelta[0], withdrawal); + + // Manually complete checks since we could still negative shares prior to the upgrade, causing a revert in the share check + assert_WithdrawalNotPending(delegationManager.calculateWithdrawalRoot(withdrawal), "staker withdrawal should no longer be pending"); + assert_Snap_Added_TokenBalances(staker, tokens, expectedTokens, "staker should have received expected tokens"); + assert_Snap_Unchanged_OperatorShares(operator, "operator shares should not have changed"); + + // If we had a positive balance update, then the staker shares should not have changed + if (balanceUpdateShareDelta[0] > 0) { + assert_Snap_Unchanged_Staker_DepositShares(staker, "staker shares should not have changed"); + } + // Else, the staker shares should have increased by the magnitude of the negative share delta + else { + int256[] memory expectedStakerShareDelta = new int256[](1); + expectedStakerShareDelta[0] = -balanceUpdateShareDelta[0]; + assert_Snap_Delta_StakerShares(staker, strategies, expectedStakerShareDelta, "staker should have received expected shares"); + } + } + + + + function _getPostWithdrawalExpectedShareDeltas( + int256 balanceUpdateShareDelta, + IDelegationManagerTypes.Withdrawal memory withdrawal + ) internal pure returns (uint256[] memory, int256[] memory) { + uint256[] memory operatorShareDelta = new uint256[](1); + int256[] memory stakerShareDelta = new int256[](1); + // The staker share delta is the withdrawal scaled shares since it can go from negative to positive + stakerShareDelta[0] = int256(withdrawal.scaledShares[0]); + + if (balanceUpdateShareDelta > 0) { + // If balanceUpdateShareDelta is positive, then the operator delta is the withdrawal scaled shares + operatorShareDelta[0] = withdrawal.scaledShares[0]; + } else { + // Operator shares never went negative, so we can just add the withdrawal scaled shares and the negative share delta + operatorShareDelta[0] = uint256(int256(withdrawal.scaledShares[0]) + balanceUpdateShareDelta); + } + + return (operatorShareDelta, stakerShareDelta); + } + + function _getPostWithdrawalExpectedTokenDeltas( + int256 balanceUpdateShareDelta, + IDelegationManagerTypes.Withdrawal memory withdrawal + ) internal pure returns (uint256[] memory) { + uint256[] memory expectedTokenDeltas = new uint256[](1); + if (balanceUpdateShareDelta > 0) { + // If we had a positive balance update, then the expected token delta is the withdrawal scaled shares + expectedTokenDeltas[0] = withdrawal.scaledShares[0]; + } else { + // If we had a negative balance update, then the expected token delta is the withdrawal scaled shares plus the negative share delta + expectedTokenDeltas[0] = uint256(int256(withdrawal.scaledShares[0]) + balanceUpdateShareDelta); + } + return expectedTokenDeltas; + } +} diff --git a/src/test/integration/users/User.t.sol b/src/test/integration/users/User.t.sol index 370ad2461d..4cd4e4881a 100644 --- a/src/test/integration/users/User.t.sol +++ b/src/test/integration/users/User.t.sol @@ -458,9 +458,9 @@ contract User is Logger, IDelegationManagerTypes, IAllocationManagerTypes { if (strat == BEACONCHAIN_ETH_STRAT) { tokens[i] = NATIVE_ETH; - // If we're withdrawing native ETH as tokens, stop ALL validators - // and complete a checkpoint - if (receiveAsTokens) { + // If we're withdrawing native ETH as tokens && do not have negative shares + // stop ALL validators and complete a checkpoint + if (receiveAsTokens && eigenPodManager.podOwnerDepositShares(address(this)) >= 0) { console.log("- exiting all validators and completing checkpoint"); _exitValidators(getActiveValidators()); diff --git a/src/test/integration/users/User_M2.t.sol b/src/test/integration/users/User_M2.t.sol index 3ec5fbc614..aaee7efe62 100644 --- a/src/test/integration/users/User_M2.t.sol +++ b/src/test/integration/users/User_M2.t.sol @@ -137,7 +137,7 @@ contract User_M2 is User { // If any balance update has occured, a checkpoint will pick it up _startCheckpoint(); if (pod.activeValidatorCount() != 0) { - _completeCheckpoint(); + _completeCheckpoint_M2(); } } else { uint256 tokens = uint256(delta); diff --git a/src/test/unit/EigenPodManagerUnit.t.sol b/src/test/unit/EigenPodManagerUnit.t.sol index 9f5848f8b3..b0fd414e3d 100644 --- a/src/test/unit/EigenPodManagerUnit.t.sol +++ b/src/test/unit/EigenPodManagerUnit.t.sol @@ -185,6 +185,22 @@ contract EigenPodManagerUnitTests_StakeTests is EigenPodManagerUnitTests { contract EigenPodManagerUnitTests_ShareUpdateTests is EigenPodManagerUnitTests { + // Wrapper contract that exposes the internal `_calculateChangeInDelegatableShares` function + EigenPodManagerWrapper public eigenPodManagerWrapper; + + function setUp() virtual override public { + super.setUp(); + + // Upgrade eigenPodManager to wrapper + eigenPodManagerWrapper = new EigenPodManagerWrapper( + ethPOSMock, + eigenPodBeacon, + IDelegationManager(address(delegationManagerMock)), + pauserRegistry + ); + eigenLayerProxyAdmin.upgrade(ITransparentUpgradeableProxy(payable(address(eigenPodManager))), address(eigenPodManagerWrapper)); + } + /******************************************************************************* Add Shares Tests *******************************************************************************/ @@ -223,6 +239,54 @@ contract EigenPodManagerUnitTests_ShareUpdateTests is EigenPodManagerUnitTests { assertEq(eigenPodManager.podOwnerDepositShares(defaultStaker), int256(shares), "Incorrect number of shares added"); } + function test_addShares_negativeInitial() public { + _initializePodWithShares(defaultStaker, -1); + + cheats.prank(address(delegationManagerMock)); + + (uint256 prevDepositShares, uint256 addedShares) = eigenPodManager.addShares( + defaultStaker, + beaconChainETHStrategy, + 5 + ); + + assertEq(prevDepositShares, 0); + assertEq(addedShares, 4); + } + + function testFuzz_addShares_negativeSharesInitial(int256 sharesToStart, int256 sharesToAdd) public { + cheats.assume(sharesToStart < 0); + cheats.assume(sharesToAdd >= 0); + + _initializePodWithShares(defaultStaker, sharesToStart); + int256 expectedDepositShares = sharesToStart + sharesToAdd; + + cheats.prank(address(delegationManagerMock)); + + cheats.expectEmit(true, true, true, true, address(eigenPodManager)); + emit PodSharesUpdated(defaultStaker, sharesToAdd); + cheats.expectEmit(true, true, true, true, address(eigenPodManager)); + emit NewTotalShares(defaultStaker, expectedDepositShares); + + (uint256 prevDepositShares, uint256 addedShares) = eigenPodManager.addShares( + defaultStaker, + beaconChainETHStrategy, + uint256(sharesToAdd) + ); + + // validate that prev shares return 0 since we started from a negative balance + assertEq(prevDepositShares, 0); + + // If we now have positive shares, expect return + if (expectedDepositShares > 0) { + assertEq(addedShares, uint256(expectedDepositShares)); + } + // We still have negative shares, return 0 + else { + assertEq(addedShares, 0); + } + } + /******************************************************************************* Remove Shares Tests ******************************************************************************/