Skip to content

Commit 9b56999

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 442289b commit 9b56999

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)),
@@ -254,6 +254,23 @@ contract AllocationManagerUnitTests is EigenLayerUnitTestSetup, IAllocationManag
254254
console.log("Success!".green().bold());
255255
}
256256

257+
/// @dev Check that the deallocation queue is in ascending order of effectBlocks
258+
function _checkDeallocationQueueOrder(address operator, IStrategy strategy, uint256 numDeallocations) internal view {
259+
uint32 lastEffectBlock = 0;
260+
261+
for (uint256 i = 0; i < numDeallocations; ++i) {
262+
bytes32 operatorSetKey = allocationManager.deallocationQueueAtIndex(operator, strategy, i);
263+
Allocation memory allocation = allocationManager.getAllocation(operator, OperatorSetLib.decode(operatorSetKey), strategy);
264+
265+
assertTrue(
266+
lastEffectBlock <= allocation.effectBlock,
267+
"Deallocation queue is not in ascending order of effectBlocks"
268+
);
269+
270+
lastEffectBlock = allocation.effectBlock;
271+
}
272+
}
273+
257274
function _checkSlashableStake(
258275
OperatorSet memory operatorSet,
259276
address operator,
@@ -1918,6 +1935,194 @@ contract AllocationManagerUnitTests_ModifyAllocations is AllocationManagerUnitTe
19181935
allocationManager.modifyAllocations(defaultOperator, allocateParams);
19191936
}
19201937

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

0 commit comments

Comments
 (0)