Skip to content

Commit 57a5253

Browse files
8sunyuanypatil12
8sunyuan
authored and
ypatil12
committed
fix: overwriting existing pending modifications (#1052)
* test: regression tests showing invalid state * fix: require check and update tests
1 parent 4f7c67a commit 57a5253

File tree

3 files changed

+243
-6
lines changed

3 files changed

+243
-6
lines changed

‎src/contracts/core/AllocationManager.sol‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ contract AllocationManager is
184184

185185
(StrategyInfo memory info, Allocation memory allocation) =
186186
_getUpdatedAllocation(operator, operatorSet.key(), strategy);
187-
require(allocation.pendingDiff == 0, ModificationAlreadyPending());
187+
require(allocation.effectBlock == 0, ModificationAlreadyPending());
188188

189189
// 2. Check whether the operator's allocation is slashable. If not, we allow instant
190190
// deallocation.

‎src/test/harnesses/AllocationManagerHarness.sol‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// SPDX-License-Identifier: BUSL-1.1
2+
pragma solidity ^0.8.27;
3+
4+
import "../../contracts/core/AllocationManager.sol";
5+
6+
contract AllocationManagerHarness is AllocationManager {
7+
using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque;
8+
9+
constructor(
10+
IDelegationManager _delegation,
11+
IPauserRegistry _pauserRegistry,
12+
IPermissionController _permissionController,
13+
uint32 _DEALLOCATION_DELAY,
14+
uint32 _ALLOCATION_CONFIGURATION_DELAY
15+
)
16+
AllocationManager(
17+
_delegation,
18+
_pauserRegistry,
19+
_permissionController,
20+
_DEALLOCATION_DELAY,
21+
_ALLOCATION_CONFIGURATION_DELAY
22+
)
23+
{}
24+
25+
function deallocationQueueAtIndex(
26+
address operator,
27+
IStrategy strategy,
28+
uint256 index
29+
) external view returns (bytes32) {
30+
return deallocationQueue[operator][strategy].at(index);
31+
}
32+
}

‎src/test/unit/AllocationManagerUnit.t.sol‎

Lines changed: 210 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// SPDX-License-Identifier: BUSL-1.1
22
pragma solidity ^0.8.27;
33

4-
import "src/contracts/core/AllocationManager.sol";
4+
import "src/test/harnesses/AllocationManagerHarness.sol";
55
import "src/test/utils/EigenLayerUnitTestSetup.sol";
66
import "src/test/mocks/MockAVSRegistrar.sol";
77

@@ -33,7 +33,7 @@ contract AllocationManagerUnitTests is EigenLayerUnitTestSetup, IAllocationManag
3333
/// Mocks
3434
/// -----------------------------------------------------------------------
3535

36-
AllocationManager allocationManager;
36+
AllocationManagerHarness allocationManager;
3737
ERC20PresetFixedSupply tokenMock;
3838
StrategyBase strategyMock;
3939

@@ -86,12 +86,12 @@ contract AllocationManagerUnitTests is EigenLayerUnitTestSetup, IAllocationManag
8686
address _initialOwner,
8787
IPauserRegistry _pauserRegistry,
8888
uint256 _initialPausedStatus
89-
) internal returns (AllocationManager) {
90-
return allocationManager = AllocationManager(
89+
) internal returns (AllocationManagerHarness) {
90+
return allocationManager = AllocationManagerHarness(
9191
address(
9292
new TransparentUpgradeableProxy(
9393
address(
94-
new AllocationManager(
94+
new AllocationManagerHarness(
9595
IDelegationManager(address(delegationManagerMock)),
9696
_pauserRegistry,
9797
IPermissionController(address(permissionController)),
@@ -259,6 +259,23 @@ contract AllocationManagerUnitTests is EigenLayerUnitTestSetup, IAllocationManag
259259
console.log("Success!".green().bold());
260260
}
261261

262+
/// @dev Check that the deallocation queue is in ascending order of effectBlocks
263+
function _checkDeallocationQueueOrder(address operator, IStrategy strategy, uint256 numDeallocations) internal view {
264+
uint32 lastEffectBlock = 0;
265+
266+
for (uint256 i = 0; i < numDeallocations; ++i) {
267+
bytes32 operatorSetKey = allocationManager.deallocationQueueAtIndex(operator, strategy, i);
268+
Allocation memory allocation = allocationManager.getAllocation(operator, OperatorSetLib.decode(operatorSetKey), strategy);
269+
270+
assertTrue(
271+
lastEffectBlock <= allocation.effectBlock,
272+
"Deallocation queue is not in ascending order of effectBlocks"
273+
);
274+
275+
lastEffectBlock = allocation.effectBlock;
276+
}
277+
}
278+
262279
function _checkSlashableStake(
263280
OperatorSet memory operatorSet,
264281
address operator,
@@ -1916,6 +1933,194 @@ contract AllocationManagerUnitTests_ModifyAllocations is AllocationManagerUnitTe
19161933
allocationManager.modifyAllocations(defaultOperator, allocateParams);
19171934
}
19181935

1936+
/**
1937+
* @notice Regression tests for the bugfix where pending modifications were checked by
1938+
* require(allocation.pendingDiff == 0, ModificationAlreadyPending());
1939+
* which would overwrite the effectBlock, pendingDiff if a pendingDiff
1940+
* of a deallocation was slashed to become 0.
1941+
*
1942+
* This test checks that the effectBlock, pendingDiff are not overwritten even if the pendingDiff is 0
1943+
* when attempting to modify allocations again
1944+
*/
1945+
function test_modifyAllocations_PendingDiffZero() public {
1946+
// Step 1: Allocate to the operator set
1947+
AllocateParams[] memory allocateParams = _newAllocateParams(defaultOperatorSet, 501);
1948+
cheats.prank(defaultOperator);
1949+
allocationManager.modifyAllocations(defaultOperator, allocateParams);
1950+
1951+
// Step 2: Roll blocks forward until the allocation effectBlock
1952+
cheats.roll(block.number + DEFAULT_OPERATOR_ALLOCATION_DELAY);
1953+
1954+
// Step 3: Deallocate from the operator set
1955+
AllocateParams[] memory deallocateParams = _newAllocateParams(defaultOperatorSet, 500);
1956+
cheats.prank(defaultOperator);
1957+
allocationManager.modifyAllocations(defaultOperator, deallocateParams);
1958+
1959+
Allocation memory allocation = allocationManager.getAllocation(defaultOperator, defaultOperatorSet, strategyMock);
1960+
uint32 originalEffectBlock = allocation.effectBlock;
1961+
1962+
// Step 4: Slash the operator to adjust pendingDiff to 0, slashing rounds up the amount of magnitude to slash
1963+
// so with an existing deallocation/pendingDiff of 1, it should result in a pendingDiff of 0
1964+
SlashingParams memory slashingParams = SlashingParams({
1965+
operator: defaultOperator,
1966+
operatorSetId: defaultOperatorSet.id,
1967+
strategies: defaultStrategies,
1968+
wadsToSlash: 5e17.toArrayU256(),
1969+
description: "Test slashing"
1970+
});
1971+
cheats.prank(defaultAVS);
1972+
allocationManager.slashOperator(defaultAVS, slashingParams);
1973+
allocation = allocationManager.getAllocation(defaultOperator, defaultOperatorSet, strategyMock);
1974+
assertEq(allocation.pendingDiff, 0, "Pending diff should be 0");
1975+
assertEq(allocation.effectBlock, originalEffectBlock, "Effect block should not have changed");
1976+
1977+
// Step 5: Modify allocations again (Should not be called)
1978+
AllocateParams[] memory newAllocateParams = _newAllocateParams(defaultOperatorSet, 1000);
1979+
cheats.prank(defaultOperator);
1980+
cheats.expectRevert(ModificationAlreadyPending.selector);
1981+
allocationManager.modifyAllocations(defaultOperator, newAllocateParams);
1982+
1983+
// Assert that the allocation was modified without reverting
1984+
allocation = allocationManager.getAllocation(defaultOperator, defaultOperatorSet, strategyMock);
1985+
assertEq(allocation.currentMagnitude, 250, "Allocation should be updated to 250 after slashing 50%");
1986+
1987+
// Note: These 2 assertions fail prior to the bugfix and if we kept the same
1988+
// require(allocation.pendingDiff == 0, ModificationAlreadyPending());
1989+
// in the code. The effectBlock, pendingDiff would get overwritten with the new modification
1990+
// but the deallocationQueue would now be unordered(in terms of effectBlocks) with this overwrite.
1991+
assertEq(allocation.effectBlock, originalEffectBlock, "Effect block should not have changed");
1992+
assertEq(allocation.pendingDiff, 0, "Pending diff should still be 0");
1993+
}
1994+
1995+
/**
1996+
* @notice Regression tests for the bugfix where pending modifications were checked by
1997+
* require(allocation.pendingDiff == 0, ModificationAlreadyPending());
1998+
* which would overwrite the effectBlock, pendingDiff if a pendingDiff
1999+
* of a deallocation was slashed to become 0.
2000+
*
2001+
* This test checks that the deallocationQueue is ascending ordered by effectBlocks
2002+
*/
2003+
function test_modifyAllocations_PendingDiffZero_CheckOrderedDeallocationQueue() public {
2004+
// Step 1: Register the operator to multiple operator sets
2005+
OperatorSet memory operatorSet1 = OperatorSet(defaultAVS, 1);
2006+
OperatorSet memory operatorSet2 = OperatorSet(defaultAVS, 2);
2007+
_createOperatorSet(operatorSet1, defaultStrategies);
2008+
_createOperatorSet(operatorSet2, defaultStrategies);
2009+
_registerForOperatorSet(defaultOperator, operatorSet1);
2010+
_registerForOperatorSet(defaultOperator, operatorSet2);
2011+
2012+
// Step 2: Allocate to both operator sets
2013+
AllocateParams[] memory allocateParams1 = _newAllocateParams(operatorSet1, 1001);
2014+
AllocateParams[] memory allocateParams2 = _newAllocateParams(operatorSet2, 1000);
2015+
cheats.prank(defaultOperator);
2016+
allocationManager.modifyAllocations(defaultOperator, allocateParams1);
2017+
cheats.prank(defaultOperator);
2018+
allocationManager.modifyAllocations(defaultOperator, allocateParams2);
2019+
2020+
// Step 3: Roll blocks forward until the allocation effectBlock
2021+
cheats.roll(block.number + DEFAULT_OPERATOR_ALLOCATION_DELAY);
2022+
2023+
// Step 4: Deallocate from both operator sets
2024+
AllocateParams[] memory deallocateParams1 = _newAllocateParams(operatorSet1, 1000);
2025+
AllocateParams[] memory deallocateParams2 = _newAllocateParams(operatorSet2, 0);
2026+
cheats.prank(defaultOperator);
2027+
allocationManager.modifyAllocations(defaultOperator, deallocateParams1);
2028+
// roll blocks forward so that deallocations have different effectBlocks
2029+
cheats.roll(block.number + 100);
2030+
cheats.prank(defaultOperator);
2031+
allocationManager.modifyAllocations(defaultOperator, deallocateParams2);
2032+
2033+
// Step 5: Slash the first deallocation to adjust pendingDiff to 0
2034+
SlashingParams memory slashingParams1 = SlashingParams({
2035+
operator: defaultOperator,
2036+
operatorSetId: operatorSet1.id,
2037+
strategies: defaultStrategies,
2038+
wadsToSlash: 5e17.toArrayU256(),
2039+
description: "Test slashing"
2040+
});
2041+
cheats.prank(defaultAVS);
2042+
allocationManager.slashOperator(defaultAVS, slashingParams1);
2043+
2044+
// Step 6: Modify allocations again for operatorSet1 making another deallocation and
2045+
// overwriting/increasing the effectBlock
2046+
// roll blocks forward so that deallocations have different effectBlocks
2047+
cheats.roll(block.number + 100);
2048+
// Note: this should revert but previously it would not prior to the bugfix
2049+
AllocateParams[] memory newAllocateParams1 = _newAllocateParams(operatorSet1, 400);
2050+
cheats.prank(defaultOperator);
2051+
cheats.expectRevert(ModificationAlreadyPending.selector);
2052+
allocationManager.modifyAllocations(defaultOperator, newAllocateParams1);
2053+
2054+
// Assert that the deallocationQueue is unordered for the 2 deallocations in queue
2055+
_checkDeallocationQueueOrder(defaultOperator, defaultStrategies[0], 2);
2056+
}
2057+
2058+
/**
2059+
* @notice Regression tests for the bugfix where pending modifications were checked by
2060+
* require(allocation.pendingDiff == 0, ModificationAlreadyPending());
2061+
* which would overwrite the effectBlock, pendingDiff if a pendingDiff
2062+
* of a deallocation was slashed to become 0.
2063+
*
2064+
* This test checks that the deallocationQueue is ascending ordered by effectBlocks
2065+
* In this case the new modifyAllocations call is an allocation
2066+
* where the effectBlock is increased and the deallocationQueue is unordered as well because the operator
2067+
* allocationDelay configured to be long enough.
2068+
*/
2069+
function test_modifyAllocations_PendingDiffZero_Allocation() public {
2070+
// Step 1: Register the operator to multiple operator sets
2071+
OperatorSet memory operatorSet1 = OperatorSet(defaultAVS, 1);
2072+
OperatorSet memory operatorSet2 = OperatorSet(defaultAVS, 2);
2073+
_createOperatorSet(operatorSet1, defaultStrategies);
2074+
_createOperatorSet(operatorSet2, defaultStrategies);
2075+
_registerForOperatorSet(defaultOperator, operatorSet1);
2076+
_registerForOperatorSet(defaultOperator, operatorSet2);
2077+
2078+
// Step 2: Allocate to both operator sets
2079+
AllocateParams[] memory allocateParams1 = _newAllocateParams(operatorSet1, 1001);
2080+
AllocateParams[] memory allocateParams2 = _newAllocateParams(operatorSet2, 1000);
2081+
cheats.prank(defaultOperator);
2082+
allocationManager.modifyAllocations(defaultOperator, allocateParams1);
2083+
cheats.prank(defaultOperator);
2084+
allocationManager.modifyAllocations(defaultOperator, allocateParams2);
2085+
2086+
// Step 3: Update operator allocation delay
2087+
cheats.prank(defaultOperator);
2088+
allocationManager.setAllocationDelay(defaultOperator, DEALLOCATION_DELAY + 10 days);
2089+
cheats.roll(block.number + ALLOCATION_CONFIGURATION_DELAY);
2090+
2091+
// Step 4: Deallocate from both operator sets
2092+
AllocateParams[] memory deallocateParams1 = _newAllocateParams(operatorSet1, 1000);
2093+
AllocateParams[] memory deallocateParams2 = _newAllocateParams(operatorSet2, 0);
2094+
cheats.prank(defaultOperator);
2095+
allocationManager.modifyAllocations(defaultOperator, deallocateParams1);
2096+
// roll blocks forward so that deallocations have different effectBlocks
2097+
cheats.roll(block.number + 100);
2098+
cheats.prank(defaultOperator);
2099+
allocationManager.modifyAllocations(defaultOperator, deallocateParams2);
2100+
2101+
// Step 5: Slash the first deallocation to adjust pendingDiff to 0
2102+
SlashingParams memory slashingParams1 = SlashingParams({
2103+
operator: defaultOperator,
2104+
operatorSetId: operatorSet1.id,
2105+
strategies: defaultStrategies,
2106+
wadsToSlash: 5e17.toArrayU256(),
2107+
description: "Test slashing"
2108+
});
2109+
cheats.prank(defaultAVS);
2110+
allocationManager.slashOperator(defaultAVS, slashingParams1);
2111+
2112+
// Step 6: Modify allocations again for operatorSet1 making an allocation and
2113+
// overwriting/increasing the effectBlock
2114+
// Note: this should revert but previously it would not prior to the bugfix
2115+
AllocateParams[] memory newAllocateParams1 = _newAllocateParams(operatorSet1, 5000);
2116+
cheats.prank(defaultOperator);
2117+
cheats.expectRevert(ModificationAlreadyPending.selector);
2118+
allocationManager.modifyAllocations(defaultOperator, newAllocateParams1);
2119+
2120+
// Assert that the deallocationQueue is unordered for the 2 deallocations in queue
2121+
_checkDeallocationQueueOrder(defaultOperator, defaultStrategies[0], 2);
2122+
}
2123+
19192124
function test_revert_allocateZeroMagnitude() public {
19202125
// Allocate exact same magnitude as initial allocation (0)
19212126
AllocateParams[] memory allocateParams = _randAllocateParams_DefaultOpSet();

0 commit comments

Comments
 (0)