OZ audit fixes - Graph Horizon

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
     );
 
     /**
@@ -254,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,
@@ -262,7 +265,8 @@ interface IHorizonStakingMain {
         address owner,
         uint256 shares,
         uint64 thawingUntil,
-        bytes32 thawRequestId
+        bytes32 thawRequestId,
+        uint256 nonce
     );
 
     /**
@@ -508,6 +512,11 @@ interface IHorizonStakingMain {
      */
     error HorizonStakingLegacySlashFailed();
 
+    /**
+     * @notice Thrown when there attempting to slash a provision with no tokens to slash.
+     */
+    error HorizonStakingNoTokensToSlash();
+
     // -- Functions --
 
     /**
@@ -657,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}.
@@ -781,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.
      *

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;
     }
 

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,

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);
         }
     }
@@ -410,7 +411,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);
 
@@ -716,6 +717,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
             createdAt: uint64(block.timestamp),
             maxVerifierCutPending: _maxVerifierCut,
             thawingPeriodPending: _thawingPeriod,
+            lastParametersStagedAt: 0,
             thawingNonce: 0
         });
 
@@ -798,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.
@@ -947,14 +950,15 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
             pool.thawingNonce
         );
 
-        emit TokensUndelegated(_serviceProvider, _verifier, msg.sender, tokens);
+        emit TokensUndelegated(_serviceProvider, _verifier, msg.sender, tokens, _shares);
         return thawRequestId;
     }
 
     /**
      * @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).
@@ -1063,13 +1067,17 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
             _owner,
             _shares,
             _thawingUntil,
-            thawRequestId
+            thawRequestId,
+            _thawingNonce
         );
         return thawRequestId;
     }
 
     /**
      * @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
@@ -1171,6 +1179,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;

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;

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);
@@ -426,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);
@@ -461,6 +464,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 +550,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 +677,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 +759,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 +784,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 +824,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);
     }
 
@@ -1007,10 +1020,11 @@ 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);
+        emit IHorizonStakingMain.TokensUndelegated(serviceProvider, verifier, delegator, calcValues.tokens, shares);
         if (legacy) {
             staking.undelegate(serviceProvider, shares);
         } else if (thawRequestType == IHorizonStakingTypes.ThawRequestType.Delegation) {

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);
+    }
 }