From 222be287cbdd624dcadb250a2b077dcc5c15b74b Mon Sep 17 00:00:00 2001
From: Juan Manuel Rodriguez Defago <juanma.rdefago@gmail.com>
Date: Wed, 26 Feb 2025 03:37:30 -0300
Subject: [PATCH 1/8] fix: add extra parameters to contract events (OZ CR-XX)
---
.../interfaces/internal/IHorizonStakingMain.sol | 4 +++-
.../horizon/contracts/staking/HorizonStaking.sol | 2 +-
.../horizon-staking/HorizonStakingShared.t.sol | 2 +-
.../contracts/SubgraphService.sol | 9 ++++++++-
.../contracts/interfaces/ISubgraphService.sol | 4 ++++
.../test/subgraphService/SubgraphService.t.sol | 16 ++++++++++++----
6 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
index 4fc677b83..f5bcc95c1 100644
--- a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
+++ b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
@@ -189,12 +189,14 @@ interface IHorizonStakingMain {
* @param verifier The address of the verifier
* @param delegator The address of the delegator
* @param tokens The amount of tokens undelegated
+ * @param tokens The amount of shares undelegated
*/
event TokensUndelegated(
address indexed serviceProvider,
address indexed verifier,
address indexed delegator,
- uint256 tokens
+ uint256 tokens,
+ uint256 shares
);
/**
diff --git a/packages/horizon/contracts/staking/HorizonStaking.sol b/packages/horizon/contracts/staking/HorizonStaking.sol
index c495a7914..cc2c5f97c 100644
--- a/packages/horizon/contracts/staking/HorizonStaking.sol
+++ b/packages/horizon/contracts/staking/HorizonStaking.sol
@@ -947,7 +947,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
pool.thawingNonce
);
- emit TokensUndelegated(_serviceProvider, _verifier, msg.sender, tokens);
+ emit TokensUndelegated(_serviceProvider, _verifier, msg.sender, tokens, _shares);
return thawRequestId;
}
diff --git a/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol b/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol
index 86817c144..0d84ba708 100644
--- a/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol
+++ b/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol
@@ -1010,7 +1010,7 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
calcValues.thawRequestId
);
vm.expectEmit();
- emit IHorizonStakingMain.TokensUndelegated(serviceProvider, verifier, delegator, calcValues.tokens);
+ emit IHorizonStakingMain.TokensUndelegated(serviceProvider, verifier, delegator, calcValues.tokens, shares);
if (legacy) {
staking.undelegate(serviceProvider, shares);
} else if (thawRequestType == IHorizonStakingTypes.ThawRequestType.Delegation) {
diff --git a/packages/subgraph-service/contracts/SubgraphService.sol b/packages/subgraph-service/contracts/SubgraphService.sol
index edf918771..752cc9e3f 100644
--- a/packages/subgraph-service/contracts/SubgraphService.sol
+++ b/packages/subgraph-service/contracts/SubgraphService.sol
@@ -522,7 +522,14 @@ contract SubgraphService is
}
}
- emit QueryFeesCollected(indexer, _signedRav.rav.payer, tokensCollected, tokensCurators);
+ emit QueryFeesCollected(
+ indexer,
+ _signedRav.rav.payer,
+ allocationId,
+ subgraphDeploymentId,
+ tokensCollected,
+ tokensCurators
+ );
return tokensCollected;
}
diff --git a/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol b/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol
index 6ae78f5b7..af2a2a953 100644
--- a/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol
+++ b/packages/subgraph-service/contracts/interfaces/ISubgraphService.sol
@@ -34,12 +34,16 @@ interface ISubgraphService is IDataServiceFees {
* @notice Emitted when a subgraph service collects query fees from Graph Payments
* @param serviceProvider The address of the service provider
* @param payer The address paying for the query fees
+ * @param allocationId The id of the allocation
+ * @param subgraphDeploymentId The id of the subgraph deployment
* @param tokensCollected The amount of tokens collected
* @param tokensCurators The amount of tokens curators receive
*/
event QueryFeesCollected(
address indexed serviceProvider,
address indexed payer,
+ address indexed allocationId,
+ bytes32 subgraphDeploymentId,
uint256 tokensCollected,
uint256 tokensCurators
);
diff --git a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol
index 2053083c8..3d8fd1b7d 100644
--- a/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol
+++ b/packages/subgraph-service/test/subgraphService/SubgraphService.t.sol
@@ -278,9 +278,10 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest {
bytes memory _data
) private returns (uint256 paymentCollected) {
IGraphTallyCollector.SignedRAV memory signedRav = abi.decode(_data, (IGraphTallyCollector.SignedRAV));
- Allocation.State memory allocation = subgraphService.getAllocation(
- address(uint160(uint256(signedRav.rav.collectionId)))
- );
+ address allocationId = address(uint160(uint256(signedRav.rav.collectionId)));
+ Allocation.State memory allocation = subgraphService.getAllocation(allocationId);
+ bytes32 subgraphDeploymentId = allocation.subgraphDeploymentId;
+
address payer = graphTallyCollector.isAuthorized(signedRav.rav.payer, _recoverRAVSigner(signedRav))
? signedRav.rav.payer
: address(0);
@@ -298,7 +299,14 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest {
uint256 tokensCurators = (paymentCollected - tokensProtocol).mulPPMRoundUp(queryFeeData.curationCut);
vm.expectEmit(address(subgraphService));
- emit ISubgraphService.QueryFeesCollected(_indexer, payer, paymentCollected, tokensCurators);
+ emit ISubgraphService.QueryFeesCollected(
+ _indexer,
+ payer,
+ allocationId,
+ subgraphDeploymentId,
+ paymentCollected,
+ tokensCurators
+ );
return paymentCollected;
}
From 5ea383dba39bcd59d5184bf588742b7618db395e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tom=C3=A1s=20Migone?= <tomas@edgeandnode.com>
Date: Fri, 7 Mar 2025 15:51:05 -0300
Subject: [PATCH 2/8] fix: add collection id to rav typehash (OZ CR-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
---
.../contracts/payments/collectors/GraphTallyCollector.sol | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/packages/horizon/contracts/payments/collectors/GraphTallyCollector.sol b/packages/horizon/contracts/payments/collectors/GraphTallyCollector.sol
index 73f887527..b8e724f2a 100644
--- a/packages/horizon/contracts/payments/collectors/GraphTallyCollector.sol
+++ b/packages/horizon/contracts/payments/collectors/GraphTallyCollector.sol
@@ -28,7 +28,7 @@ contract GraphTallyCollector is EIP712, GraphDirectory, Authorizable, IGraphTall
/// @notice The EIP712 typehash for the ReceiptAggregateVoucher struct
bytes32 private constant EIP712_RAV_TYPEHASH =
keccak256(
- "ReceiptAggregateVoucher(address payer,address serviceProvider,address dataService,uint64 timestampNs,uint128 valueAggregate,bytes metadata)"
+ "ReceiptAggregateVoucher(bytes32 collectionId,address payer,address serviceProvider,address dataService,uint64 timestampNs,uint128 valueAggregate,bytes metadata)"
);
/// @notice Tracks the amount of tokens already collected by a data service from a payer to a receiver.
@@ -189,6 +189,7 @@ contract GraphTallyCollector is EIP712, GraphDirectory, Authorizable, IGraphTall
keccak256(
abi.encode(
EIP712_RAV_TYPEHASH,
+ _rav.collectionId,
_rav.payer,
_rav.serviceProvider,
_rav.dataService,
From 64f4d77b41a0c7cbcfb80a38fd94b403965ae6e1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tom=C3=A1s=20Migone?= <tomas@edgeandnode.com>
Date: Fri, 7 Mar 2025 16:00:47 -0300
Subject: [PATCH 3/8] fix: rename misleading custom error name (OZ CR-XX)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
---
.../contracts/interfaces/internal/IHorizonStakingMain.sol | 5 +++++
packages/horizon/contracts/staking/HorizonStaking.sol | 2 +-
packages/horizon/test/staking/slash/slash.t.sol | 4 +---
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
index f5bcc95c1..e76239ee9 100644
--- a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
+++ b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
@@ -510,6 +510,11 @@ interface IHorizonStakingMain {
*/
error HorizonStakingLegacySlashFailed();
+ /**
+ * @notice Thrown when there attempting to slash a provision with no tokens to slash.
+ */
+ error HorizonStakingNoTokensToSlash();
+
// -- Functions --
/**
diff --git a/packages/horizon/contracts/staking/HorizonStaking.sol b/packages/horizon/contracts/staking/HorizonStaking.sol
index cc2c5f97c..2407e7d21 100644
--- a/packages/horizon/contracts/staking/HorizonStaking.sol
+++ b/packages/horizon/contracts/staking/HorizonStaking.sol
@@ -410,7 +410,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
Provision storage prov = _provisions[serviceProvider][verifier];
DelegationPoolInternal storage pool = _getDelegationPool(serviceProvider, verifier);
uint256 tokensProvisionTotal = prov.tokens + pool.tokens;
- require(tokensProvisionTotal != 0, HorizonStakingInsufficientTokens(tokensProvisionTotal, tokens));
+ require(tokensProvisionTotal != 0, HorizonStakingNoTokensToSlash());
uint256 tokensToSlash = MathUtils.min(tokens, tokensProvisionTotal);
diff --git a/packages/horizon/test/staking/slash/slash.t.sol b/packages/horizon/test/staking/slash/slash.t.sol
index 3f67a9e59..d621a24e9 100644
--- a/packages/horizon/test/staking/slash/slash.t.sol
+++ b/packages/horizon/test/staking/slash/slash.t.sol
@@ -97,9 +97,7 @@ contract HorizonStakingSlashTest is HorizonStakingTest {
function testSlash_RevertWhen_NoProvision(uint256 tokens, uint256 slashTokens) public useIndexer useStake(tokens) {
vm.assume(slashTokens > 0);
bytes memory expectedError = abi.encodeWithSelector(
- IHorizonStakingMain.HorizonStakingInsufficientTokens.selector,
- 0,
- slashTokens
+ IHorizonStakingMain.HorizonStakingNoTokensToSlash.selector
);
vm.expectRevert(expectedError);
vm.startPrank(subgraphDataServiceAddress);
From 3447bd955bd522d5aec512b759ddc3c7d3d80216 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tom=C3=A1s=20Migone?= <tomas@edgeandnode.com>
Date: Fri, 7 Mar 2025 16:14:50 -0300
Subject: [PATCH 4/8] fix: prevent indexer lockout in subgraph service (OZ
CR-02)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
---
.../contracts/SubgraphService.sol | 2 +-
.../subgraphService/provision/accept.t.sol | 23 +++++++++++++++----
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/packages/subgraph-service/contracts/SubgraphService.sol b/packages/subgraph-service/contracts/SubgraphService.sol
index 752cc9e3f..de76cfb73 100644
--- a/packages/subgraph-service/contracts/SubgraphService.sol
+++ b/packages/subgraph-service/contracts/SubgraphService.sol
@@ -148,7 +148,7 @@ contract SubgraphService is
function acceptProvisionPendingParameters(
address indexer,
bytes calldata
- ) external override onlyAuthorizedForProvision(indexer) onlyRegisteredIndexer(indexer) whenNotPaused {
+ ) external override onlyAuthorizedForProvision(indexer) whenNotPaused {
_checkProvisionTokens(indexer);
_acceptProvisionParameters(indexer);
emit ProvisionPendingParametersAccepted(indexer);
diff --git a/packages/subgraph-service/test/subgraphService/provision/accept.t.sol b/packages/subgraph-service/test/subgraphService/provision/accept.t.sol
index 4d421b979..6ddc2f953 100644
--- a/packages/subgraph-service/test/subgraphService/provision/accept.t.sol
+++ b/packages/subgraph-service/test/subgraphService/provision/accept.t.sol
@@ -34,11 +34,24 @@ contract SubgraphServiceProvisionAcceptTest is SubgraphServiceTest {
_acceptProvision(users.indexer, "");
}
- function test_SubgraphService_Provision_Accept_RevertWhen_NotRegistered() public useIndexer {
- vm.expectRevert(
- abi.encodeWithSelector(ISubgraphService.SubgraphServiceIndexerNotRegistered.selector, users.indexer)
- );
- subgraphService.acceptProvisionPendingParameters(users.indexer, "");
+ function test_SubgraphService_Provision_Accept_When_NotRegistered(
+ uint256 tokens,
+ uint32 newVerifierCut,
+ uint64 newDisputePeriod
+ ) public useIndexer {
+ tokens = bound(tokens, minimumProvisionTokens, MAX_TOKENS);
+ vm.assume(newVerifierCut >= fishermanRewardPercentage);
+ vm.assume(newVerifierCut <= MAX_PPM);
+ newDisputePeriod = uint64(bound(newDisputePeriod, disputePeriod, MAX_WAIT_PERIOD));
+
+ // Setup indexer but dont register
+ _createProvision(users.indexer, tokens, fishermanRewardPercentage, disputePeriod);
+
+ // Update parameters with new values
+ _setProvisionParameters(users.indexer, address(subgraphService), newVerifierCut, newDisputePeriod);
+
+ // Accept provision and check parameters
+ _acceptProvision(users.indexer, "");
}
function test_SubgraphService_Provision_Accept_RevertWhen_NotAuthorized() public {
From f81d8ff91457598b578f6846fe66a8e29198d6df Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tom=C3=A1s=20Migone?= <tomas@edgeandnode.com>
Date: Fri, 21 Mar 2025 14:38:50 -0300
Subject: [PATCH 5/8] fix: limit allowed thawing period range in the subgraph
service (OZ M-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
---
.../contracts/SubgraphService.sol | 8 ++++---
.../subgraphService/provision/accept.t.sol | 24 +++++++++++++------
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/packages/subgraph-service/contracts/SubgraphService.sol b/packages/subgraph-service/contracts/SubgraphService.sol
index de76cfb73..703fdc625 100644
--- a/packages/subgraph-service/contracts/SubgraphService.sol
+++ b/packages/subgraph-service/contracts/SubgraphService.sol
@@ -431,12 +431,14 @@ contract SubgraphService is
// -- Data service parameter getters --
/**
* @notice Getter for the accepted thawing period range for provisions
+ * The accepted range is just the dispute period defined by {DisputeManager-getDisputePeriod}
* @dev This override ensures {ProvisionManager} uses the thawing period from the {DisputeManager}
- * @return The minimum thawing period which is defined by {DisputeManager-getDisputePeriod}
- * @return The maximum is unbounded
+ * @return The minimum thawing period - the dispute period
+ * @return The maximum thawing period - the dispute period
*/
function _getThawingPeriodRange() internal view override returns (uint64, uint64) {
- return (_disputeManager().getDisputePeriod(), DEFAULT_MAX_THAWING_PERIOD);
+ uint64 disputePeriod = _disputeManager().getDisputePeriod();
+ return (disputePeriod, disputePeriod);
}
/**
diff --git a/packages/subgraph-service/test/subgraphService/provision/accept.t.sol b/packages/subgraph-service/test/subgraphService/provision/accept.t.sol
index 6ddc2f953..36b19732e 100644
--- a/packages/subgraph-service/test/subgraphService/provision/accept.t.sol
+++ b/packages/subgraph-service/test/subgraphService/provision/accept.t.sol
@@ -17,14 +17,19 @@ contract SubgraphServiceProvisionAcceptTest is SubgraphServiceTest {
uint256 tokens,
uint32 newVerifierCut,
uint64 newDisputePeriod
- ) public useIndexer {
+ ) public {
tokens = bound(tokens, minimumProvisionTokens, MAX_TOKENS);
vm.assume(newVerifierCut >= fishermanRewardPercentage);
vm.assume(newVerifierCut <= MAX_PPM);
- newDisputePeriod = uint64(bound(newDisputePeriod, disputePeriod, MAX_WAIT_PERIOD));
+ newDisputePeriod = uint64(bound(newDisputePeriod, 1, MAX_WAIT_PERIOD));
+
+ // Set the dispute period to the new value
+ resetPrank(users.governor);
+ disputeManager.setDisputePeriod(newDisputePeriod);
// Setup indexer
- _createProvision(users.indexer, tokens, fishermanRewardPercentage, disputePeriod);
+ resetPrank(users.indexer);
+ _createProvision(users.indexer, tokens, fishermanRewardPercentage, newDisputePeriod);
_register(users.indexer, abi.encode("url", "geoHash", address(0)));
// Update parameters with new values
@@ -38,14 +43,19 @@ contract SubgraphServiceProvisionAcceptTest is SubgraphServiceTest {
uint256 tokens,
uint32 newVerifierCut,
uint64 newDisputePeriod
- ) public useIndexer {
+ ) public {
tokens = bound(tokens, minimumProvisionTokens, MAX_TOKENS);
vm.assume(newVerifierCut >= fishermanRewardPercentage);
vm.assume(newVerifierCut <= MAX_PPM);
- newDisputePeriod = uint64(bound(newDisputePeriod, disputePeriod, MAX_WAIT_PERIOD));
+ newDisputePeriod = uint64(bound(newDisputePeriod, 1, MAX_WAIT_PERIOD));
+
+ // Set the dispute period to the new value
+ resetPrank(users.governor);
+ disputeManager.setDisputePeriod(newDisputePeriod);
// Setup indexer but dont register
- _createProvision(users.indexer, tokens, fishermanRewardPercentage, disputePeriod);
+ resetPrank(users.indexer);
+ _createProvision(users.indexer, tokens, fishermanRewardPercentage, newDisputePeriod);
// Update parameters with new values
_setProvisionParameters(users.indexer, address(subgraphService), newVerifierCut, newDisputePeriod);
@@ -114,7 +124,7 @@ contract SubgraphServiceProvisionAcceptTest is SubgraphServiceTest {
"thawingPeriod",
newDisputePeriod,
disputePeriod,
- type(uint64).max
+ disputePeriod
)
);
subgraphService.acceptProvisionPendingParameters(users.indexer, "");
From 3adab908745fcf5630dab024a775d723c5add7b1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tom=C3=A1s=20Migone?= <tomas@edgeandnode.com>
Date: Fri, 21 Mar 2025 16:24:00 -0300
Subject: [PATCH 6/8] fix: save last parameter staged timestamp on provisions
(OZ M-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
---
.../interfaces/internal/IHorizonStakingTypes.sol | 3 +++
.../horizon/contracts/staking/HorizonStaking.sol | 2 ++
.../horizon-staking/HorizonStakingShared.t.sol | 14 +++++++++++++-
packages/horizon/test/staking/slash/slash.t.sol | 4 +---
4 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/packages/horizon/contracts/interfaces/internal/IHorizonStakingTypes.sol b/packages/horizon/contracts/interfaces/internal/IHorizonStakingTypes.sol
index 6c3420cc7..5a9e7c055 100644
--- a/packages/horizon/contracts/interfaces/internal/IHorizonStakingTypes.sol
+++ b/packages/horizon/contracts/interfaces/internal/IHorizonStakingTypes.sol
@@ -22,6 +22,8 @@ interface IHorizonStakingTypes {
* @param createdAt Timestamp when the provision was created
* @param maxVerifierCutPending Pending value for `maxVerifierCut`. Verifier needs to accept it before it becomes active.
* @param thawingPeriodPending Pending value for `thawingPeriod`. Verifier needs to accept it before it becomes active.
+ * @param lastParametersStagedAt Timestamp when the provision parameters were last staged. Can be used by data service implementation to
+ * implement arbitrary parameter update logic.
* @param thawingNonce Value of the current thawing nonce. Thaw requests with older nonces are invalid.
*/
struct Provision {
@@ -33,6 +35,7 @@ interface IHorizonStakingTypes {
uint64 createdAt;
uint32 maxVerifierCutPending;
uint64 thawingPeriodPending;
+ uint256 lastParametersStagedAt;
uint256 thawingNonce;
}
diff --git a/packages/horizon/contracts/staking/HorizonStaking.sol b/packages/horizon/contracts/staking/HorizonStaking.sol
index 2407e7d21..4d9022f7e 100644
--- a/packages/horizon/contracts/staking/HorizonStaking.sol
+++ b/packages/horizon/contracts/staking/HorizonStaking.sol
@@ -215,6 +215,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
if ((prov.maxVerifierCutPending != newMaxVerifierCut) || (prov.thawingPeriodPending != newThawingPeriod)) {
prov.maxVerifierCutPending = newMaxVerifierCut;
prov.thawingPeriodPending = newThawingPeriod;
+ prov.lastParametersStagedAt = block.timestamp;
emit ProvisionParametersStaged(serviceProvider, verifier, newMaxVerifierCut, newThawingPeriod);
}
}
@@ -716,6 +717,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
createdAt: uint64(block.timestamp),
maxVerifierCutPending: _maxVerifierCut,
thawingPeriodPending: _thawingPeriod,
+ lastParametersStagedAt: 0,
thawingNonce: 0
});
diff --git a/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol b/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol
index 0d84ba708..f34bbc850 100644
--- a/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol
+++ b/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol
@@ -355,6 +355,7 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
assertEq(afterProvision.createdAt, uint64(block.timestamp));
assertEq(afterProvision.maxVerifierCutPending, maxVerifierCut);
assertEq(afterProvision.thawingPeriodPending, thawingPeriod);
+ assertEq(afterProvision.lastParametersStagedAt, 0);
assertEq(afterProvision.thawingNonce, 0);
assertEq(afterServiceProvider.tokensStaked, beforeServiceProvider.tokensStaked);
assertEq(afterServiceProvider.tokensProvisioned, tokens + beforeServiceProvider.tokensProvisioned);
@@ -387,6 +388,7 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
assertEq(afterProvision.maxVerifierCut, beforeProvision.maxVerifierCut);
assertEq(afterProvision.thawingPeriod, beforeProvision.thawingPeriod);
assertEq(afterProvision.createdAt, beforeProvision.createdAt);
+ assertEq(afterProvision.lastParametersStagedAt, beforeProvision.lastParametersStagedAt);
assertEq(afterProvision.maxVerifierCutPending, beforeProvision.maxVerifierCutPending);
assertEq(afterProvision.thawingPeriodPending, beforeProvision.thawingPeriodPending);
assertEq(afterProvision.thawingNonce, beforeProvision.thawingNonce);
@@ -461,6 +463,7 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
assertEq(afterProvision.createdAt, beforeProvision.createdAt);
assertEq(afterProvision.maxVerifierCutPending, beforeProvision.maxVerifierCutPending);
assertEq(afterProvision.thawingPeriodPending, beforeProvision.thawingPeriodPending);
+ assertEq(afterProvision.lastParametersStagedAt, beforeProvision.lastParametersStagedAt);
assertEq(afterProvision.thawingNonce, beforeProvision.thawingNonce);
assertEq(thawRequestId, expectedThawRequestId);
assertEq(afterThawRequest.shares, thawingShares);
@@ -546,6 +549,7 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
assertEq(afterProvision.createdAt, beforeProvision.createdAt);
assertEq(afterProvision.maxVerifierCutPending, beforeProvision.maxVerifierCutPending);
assertEq(afterProvision.thawingPeriodPending, beforeProvision.thawingPeriodPending);
+ assertEq(afterProvision.lastParametersStagedAt, beforeProvision.lastParametersStagedAt);
assertEq(afterProvision.thawingNonce, beforeProvision.thawingNonce);
assertEq(afterServiceProvider.tokensStaked, beforeServiceProvider.tokensStaked);
assertEq(
@@ -672,6 +676,7 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
assertEq(afterProvision.createdAt, beforeValues.provision.createdAt);
assertEq(afterProvision.maxVerifierCutPending, beforeValues.provision.maxVerifierCutPending);
assertEq(afterProvision.thawingPeriodPending, beforeValues.provision.thawingPeriodPending);
+ assertEq(afterProvision.lastParametersStagedAt, beforeValues.provision.lastParametersStagedAt);
assertEq(afterProvision.thawingNonce, beforeValues.provision.thawingNonce);
// assert: provision new verifier
@@ -753,7 +758,9 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
Provision memory beforeProvision = staking.getProvision(serviceProvider, verifier);
// setProvisionParameters
- if (beforeProvision.maxVerifierCut != maxVerifierCut || beforeProvision.thawingPeriod != thawingPeriod) {
+ bool paramsChanged = beforeProvision.maxVerifierCut != maxVerifierCut ||
+ beforeProvision.thawingPeriod != thawingPeriod;
+ if (paramsChanged) {
vm.expectEmit();
emit IHorizonStakingMain.ProvisionParametersStaged(
serviceProvider,
@@ -776,6 +783,10 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
assertEq(afterProvision.createdAt, beforeProvision.createdAt);
assertEq(afterProvision.maxVerifierCutPending, maxVerifierCut);
assertEq(afterProvision.thawingPeriodPending, thawingPeriod);
+ assertEq(
+ afterProvision.lastParametersStagedAt,
+ paramsChanged ? block.timestamp : beforeProvision.lastParametersStagedAt
+ );
assertEq(afterProvision.thawingNonce, beforeProvision.thawingNonce);
}
@@ -812,6 +823,7 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
assertEq(afterProvision.thawingPeriod, beforeProvision.thawingPeriodPending);
assertEq(afterProvision.thawingPeriod, afterProvision.thawingPeriodPending);
assertEq(afterProvision.createdAt, beforeProvision.createdAt);
+ assertEq(afterProvision.lastParametersStagedAt, beforeProvision.lastParametersStagedAt);
assertEq(afterProvision.thawingNonce, beforeProvision.thawingNonce);
}
diff --git a/packages/horizon/test/staking/slash/slash.t.sol b/packages/horizon/test/staking/slash/slash.t.sol
index d621a24e9..89dfe55d5 100644
--- a/packages/horizon/test/staking/slash/slash.t.sol
+++ b/packages/horizon/test/staking/slash/slash.t.sol
@@ -96,9 +96,7 @@ contract HorizonStakingSlashTest is HorizonStakingTest {
function testSlash_RevertWhen_NoProvision(uint256 tokens, uint256 slashTokens) public useIndexer useStake(tokens) {
vm.assume(slashTokens > 0);
- bytes memory expectedError = abi.encodeWithSelector(
- IHorizonStakingMain.HorizonStakingNoTokensToSlash.selector
- );
+ bytes memory expectedError = abi.encodeWithSelector(IHorizonStakingMain.HorizonStakingNoTokensToSlash.selector);
vm.expectRevert(expectedError);
vm.startPrank(subgraphDataServiceAddress);
staking.slash(users.indexer, slashTokens, 0, subgraphDataServiceAddress);
From 2f80dc4014bd1b252551aead2df5952005cdc1a1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tom=C3=A1s=20Migone?= <tomas@edgeandnode.com>
Date: Fri, 21 Mar 2025 17:23:17 -0300
Subject: [PATCH 7/8] fix: ensure invalid thaw requests are ignored by
getThawedTokens (OZ L-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
---
.../internal/IHorizonStakingMain.sol | 4 +-
.../contracts/staking/HorizonStaking.sol | 4 +-
.../contracts/staking/HorizonStakingBase.sol | 18 +++--
.../HorizonStakingShared.t.sol | 6 +-
.../horizon/test/staking/provision/thaw.t.sol | 72 +++++++++++++++++++
5 files changed, 93 insertions(+), 11 deletions(-)
diff --git a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
index e76239ee9..619397439 100644
--- a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
+++ b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
@@ -256,6 +256,7 @@ interface IHorizonStakingMain {
* @param shares The amount of shares being thawed
* @param thawingUntil The timestamp until the stake is thawed
* @param thawRequestId The ID of the thaw request
+ * @param nonce The nonce of the thaw request
*/
event ThawRequestCreated(
IHorizonStakingTypes.ThawRequestType indexed requestType,
@@ -264,7 +265,8 @@ interface IHorizonStakingMain {
address owner,
uint256 shares,
uint64 thawingUntil,
- bytes32 thawRequestId
+ bytes32 thawRequestId,
+ uint256 nonce
);
/**
diff --git a/packages/horizon/contracts/staking/HorizonStaking.sol b/packages/horizon/contracts/staking/HorizonStaking.sol
index 4d9022f7e..a8f7adbaf 100644
--- a/packages/horizon/contracts/staking/HorizonStaking.sol
+++ b/packages/horizon/contracts/staking/HorizonStaking.sol
@@ -1065,7 +1065,8 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
_owner,
_shares,
_thawingUntil,
- thawRequestId
+ thawRequestId,
+ _thawingNonce
);
return thawRequestId;
}
@@ -1173,6 +1174,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
uint256 tokens = 0;
bool validThawRequest = thawRequest.thawingNonce == thawingNonce;
if (validThawRequest) {
+ // sharesThawing cannot be zero if there is a valid thaw request so the next division is safe
tokens = (thawRequest.shares * tokensThawing) / sharesThawing;
tokensThawing = tokensThawing - tokens;
sharesThawing = sharesThawing - thawRequest.shares;
diff --git a/packages/horizon/contracts/staking/HorizonStakingBase.sol b/packages/horizon/contracts/staking/HorizonStakingBase.sol
index 5ccbddfa2..4f9f6a00d 100644
--- a/packages/horizon/contracts/staking/HorizonStakingBase.sol
+++ b/packages/horizon/contracts/staking/HorizonStakingBase.sol
@@ -182,14 +182,18 @@ abstract contract HorizonStakingBase is
bytes32 thawRequestId = thawRequestList.head;
while (thawRequestId != bytes32(0)) {
ThawRequest storage thawRequest = _getThawRequest(requestType, thawRequestId);
- if (thawRequest.thawingUntil <= block.timestamp) {
- uint256 tokens = (thawRequest.shares * tokensThawing) / sharesThawing;
- tokensThawing = tokensThawing - tokens;
- sharesThawing = sharesThawing - thawRequest.shares;
- thawedTokens = thawedTokens + tokens;
- } else {
- break;
+ if (thawRequest.thawingNonce == prov.thawingNonce) {
+ if (thawRequest.thawingUntil <= block.timestamp) {
+ // sharesThawing cannot be zero if there is a valid thaw request so the next division is safe
+ uint256 tokens = (thawRequest.shares * tokensThawing) / sharesThawing;
+ tokensThawing = tokensThawing - tokens;
+ sharesThawing = sharesThawing - thawRequest.shares;
+ thawedTokens = thawedTokens + tokens;
+ } else {
+ break;
+ }
}
+
thawRequestId = thawRequest.next;
}
return thawedTokens;
diff --git a/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol b/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol
index f34bbc850..ada3b1370 100644
--- a/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol
+++ b/packages/horizon/test/shared/horizon-staking/HorizonStakingShared.t.sol
@@ -428,7 +428,8 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
serviceProvider,
thawingShares,
uint64(block.timestamp + beforeProvision.thawingPeriod),
- expectedThawRequestId
+ expectedThawRequestId,
+ beforeProvision.thawingNonce
);
vm.expectEmit(address(staking));
emit IHorizonStakingMain.ProvisionThawed(serviceProvider, verifier, tokens);
@@ -1019,7 +1020,8 @@ abstract contract HorizonStakingSharedTest is GraphBaseTest {
beneficiary,
calcValues.thawingShares,
calcValues.thawingUntil,
- calcValues.thawRequestId
+ calcValues.thawRequestId,
+ beforeValues.pool.thawingNonce
);
vm.expectEmit();
emit IHorizonStakingMain.TokensUndelegated(serviceProvider, verifier, delegator, calcValues.tokens, shares);
diff --git a/packages/horizon/test/staking/provision/thaw.t.sol b/packages/horizon/test/staking/provision/thaw.t.sol
index a16dd7253..46c666618 100644
--- a/packages/horizon/test/staking/provision/thaw.t.sol
+++ b/packages/horizon/test/staking/provision/thaw.t.sol
@@ -162,4 +162,76 @@ contract HorizonStakingThawTest is HorizonStakingTest {
);
vm.assertEq(thawedTokens, thawAmount * thawSteps);
}
+
+ function testThaw_GetThawedTokens_AfterProvisionFullySlashed(
+ uint256 amount,
+ uint64 thawingPeriod,
+ uint256 thawAmount
+ ) public useIndexer useProvision(amount, 0, thawingPeriod) {
+ // thaw some funds so there are some shares thawing and tokens thawing
+ thawAmount = bound(thawAmount, 1, amount);
+ _thaw(users.indexer, subgraphDataServiceAddress, thawAmount);
+
+ // skip to after the thawing period has passed
+ skip(thawingPeriod + 1);
+
+ // slash all of it
+ resetPrank(subgraphDataServiceAddress);
+ _slash(users.indexer, subgraphDataServiceAddress, amount, 0);
+
+ // get the thawed tokens - should be zero now as the pool was reset
+ uint256 thawedTokens = staking.getThawedTokens(
+ ThawRequestType.Provision,
+ users.indexer,
+ subgraphDataServiceAddress,
+ users.indexer
+ );
+ vm.assertEq(thawedTokens, 0);
+ }
+
+ function testThaw_GetThawedTokens_AfterRecoveringProvision(
+ uint256 amount,
+ uint64 thawingPeriod,
+ uint256 thawAmount
+ ) public useIndexer useProvision(amount, 0, thawingPeriod) {
+ // thaw some funds so there are some shares thawing and tokens thawing
+ thawAmount = bound(thawAmount, 1, amount);
+ _thaw(users.indexer, subgraphDataServiceAddress, thawAmount);
+
+ // skip to after the thawing period has passed
+ skip(thawingPeriod + 1);
+
+ // slash all of it
+ resetPrank(subgraphDataServiceAddress);
+ _slash(users.indexer, subgraphDataServiceAddress, amount, 0);
+
+ // get the thawed tokens - should be zero now as the pool was reset
+ uint256 thawedTokens = staking.getThawedTokens(
+ ThawRequestType.Provision,
+ users.indexer,
+ subgraphDataServiceAddress,
+ users.indexer
+ );
+ vm.assertEq(thawedTokens, 0);
+
+ // put some funds back in
+ resetPrank(users.indexer);
+ _stake(amount);
+ _addToProvision(users.indexer, subgraphDataServiceAddress, amount);
+
+ // thaw some more funds
+ _thaw(users.indexer, subgraphDataServiceAddress, thawAmount);
+
+ // skip to after the thawing period has passed
+ skip(block.timestamp + thawingPeriod + 1);
+
+ // get the thawed tokens - should be the amount we thawed
+ thawedTokens = staking.getThawedTokens(
+ ThawRequestType.Provision,
+ users.indexer,
+ subgraphDataServiceAddress,
+ users.indexer
+ );
+ vm.assertEq(thawedTokens, thawAmount);
+ }
}
From d0132874b88f7e5d1929897cefb8546a9e5f48db Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tom=C3=A1s=20Migone?= <tomas@edgeandnode.com>
Date: Fri, 21 Mar 2025 17:51:08 -0300
Subject: [PATCH 8/8] fix: document possible temporary thaw requests blockage
when thawing period is shortened (OZ L-02)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
---
.../interfaces/internal/IHorizonStakingMain.sol | 6 ++++--
packages/horizon/contracts/staking/HorizonStaking.sol | 9 +++++++--
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
index 619397439..5c100e162 100644
--- a/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
+++ b/packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol
@@ -666,7 +666,8 @@ interface IHorizonStakingMain {
/**
* @notice Remove tokens from a provision and move them back to the service provider's idle stake.
* @dev The parameter `nThawRequests` can be set to a non zero value to fulfill a specific number of thaw
- * requests in the event that fulfilling all of them results in a gas limit error.
+ * requests in the event that fulfilling all of them results in a gas limit error. Otherwise, the function
+ * will attempt to fulfill all thaw requests until the first one that is not yet expired is found.
*
* Requirements:
* - Must have previously initiated a thaw request using {thaw}.
@@ -790,7 +791,8 @@ interface IHorizonStakingMain {
/**
* @notice Withdraw undelegated tokens from a provision after thawing.
* @dev The parameter `nThawRequests` can be set to a non zero value to fulfill a specific number of thaw
- * requests in the event that fulfilling all of them results in a gas limit error.
+ * requests in the event that fulfilling all of them results in a gas limit error. Otherwise, the function
+ * will attempt to fulfill all thaw requests until the first one that is not yet expired is found.
* @dev If the delegation pool was completely slashed before withdrawing, calling this function will fulfill
* the thaw requests with an amount equal to zero.
*
diff --git a/packages/horizon/contracts/staking/HorizonStaking.sol b/packages/horizon/contracts/staking/HorizonStaking.sol
index a8f7adbaf..0b6bcd615 100644
--- a/packages/horizon/contracts/staking/HorizonStaking.sol
+++ b/packages/horizon/contracts/staking/HorizonStaking.sol
@@ -800,7 +800,8 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
/**
* @notice Remove tokens from a provision and move them back to the service provider's idle stake.
* @dev The parameter `nThawRequests` can be set to a non zero value to fulfill a specific number of thaw
- * requests in the event that fulfilling all of them results in a gas limit error.
+ * requests in the event that fulfilling all of them results in a gas limit error. Otherwise, the function
+ * will attempt to fulfill all thaw requests until the first one that is not yet expired is found.
* @param _serviceProvider The service provider address
* @param _verifier The verifier address
* @param _nThawRequests The number of thaw requests to fulfill. Set to 0 to fulfill all thaw requests.
@@ -956,7 +957,8 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
/**
* @notice Withdraw undelegated tokens from a provision after thawing.
* @dev The parameter `nThawRequests` can be set to a non zero value to fulfill a specific number of thaw
- * requests in the event that fulfilling all of them results in a gas limit error.
+ * requests in the event that fulfilling all of them results in a gas limit error. Otherwise, the function
+ * will attempt to fulfill all thaw requests until the first one that is not yet expired is found.
* @dev If the delegation pool was completely slashed before withdrawing, calling this function will fulfill
* the thaw requests with an amount equal to zero.
* @param _requestType The type of thaw request (Provision or Delegation).
@@ -1073,6 +1075,9 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
/**
* @notice Traverses a thaw request list and fulfills expired thaw requests.
+ * @dev Note that the list is traversed by creation date not by thawing until date. Traversing will stop
+ * when the first thaw request that is not yet expired is found even if later thaw requests have expired. This
+ * could happen for example when the thawing period is shortened.
* @param _params The parameters for fulfilling thaw requests
* @return The amount of thawed tokens
* @return The amount of tokens still thawing