Horizon OZ2 audit - pending fixes

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
     );
 
     /**
@@ -508,6 +510,11 @@ interface IHorizonStakingMain {
      */
     error HorizonStakingLegacySlashFailed();
 
+    /**
+     * @notice Thrown when there attempting to slash a provision with no tokens to slash.
+     */
+    error HorizonStakingNoTokensToSlash();
+
     // -- Functions --
 
     /**

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

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) {

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

packages/subgraph-service/contracts/DisputeManager.sol

@@ -502,7 +502,7 @@ contract DisputeManager is
 
     /**
      * @notice Make the subgraph service contract slash the indexer and reward the fisherman.
-     * Give the fisherman a reward equal to the fishermanRewardPercentage of slashed amount
+     * Give the fisherman a reward equal to the fishermanRewardCut of slashed amount
      * @param _indexer Address of the indexer
      * @param _tokensSlash Amount of tokens to slash from the indexer
      * @param _tokensStakeSnapshot Snapshot of the indexer's stake at the time of the dispute creation
@@ -568,8 +568,8 @@ contract DisputeManager is
     }
 
     /**
-     * @notice Set the percent reward that the fisherman gets when slashing occurs.
-     * @dev Update the reward percentage to `_percentage`
+     * @notice Set the reward cut that the fisherman gets when slashing occurs.
+     * @dev Update the reward cut to `_fishermanRewardCut`
      * @param _fishermanRewardCut The fisherman reward cut, in PPM
      */
     function _setFishermanRewardCut(uint32 _fishermanRewardCut) private {
@@ -582,8 +582,8 @@ contract DisputeManager is
     }
 
     /**
-     * @notice Set the maximum percentage that can be used for slashing indexers.
-     * @param _maxSlashingCut Max percentage slashing for disputes, in PPM
+     * @notice Set the maximum cut that can be used for slashing indexers.
+     * @param _maxSlashingCut Max slashing cut, in PPM
      */
     function _setMaxSlashingCut(uint32 _maxSlashingCut) private {
         require(PPMMath.isValidPPM(_maxSlashingCut), DisputeManagerInvalidMaxSlashingCut(_maxSlashingCut));

packages/subgraph-service/contracts/SubgraphService.sol

@@ -227,7 +227,7 @@ contract SubgraphService is
             _allocations.get(allocationId).indexer == indexer,
             SubgraphServiceAllocationNotAuthorized(indexer, allocationId)
         );
-        _closeAllocation(allocationId);
+        _closeAllocation(allocationId, false);
         emit ServiceStopped(indexer, data);
     }
 
@@ -250,6 +250,13 @@ contract SubgraphService is
      *
      * @param indexer The address of the indexer
      * @param paymentType The type of payment to collect as defined in {IGraphPayments}
+     * @param data Encoded data:
+     *    - For query fees:
+     *      - IGraphTallyCollector.SignedRAV `signedRav`: The signed RAV
+     *    - For indexing rewards:
+     *      - address `allocationId`: The id of the allocation
+     *      - bytes32 `poi`: The POI being presented
+     *      - bytes32 `publicPOI`: The public POI associated with the presented POI
      */
     /// @inheritdoc IDataService
     function collect(
@@ -275,12 +282,12 @@ contract SubgraphService is
             );
             paymentCollected = _collectQueryFees(signedRav);
         } else if (paymentType == IGraphPayments.PaymentTypes.IndexingRewards) {
-            (address allocationId, bytes32 poi) = abi.decode(data, (address, bytes32));
+            (address allocationId, bytes32 poi, bytes32 publicPOI) = abi.decode(data, (address, bytes32, bytes32));
             require(
                 _allocations.get(allocationId).indexer == indexer,
                 SubgraphServiceAllocationNotAuthorized(indexer, allocationId)
             );
-            paymentCollected = _collectIndexingRewards(allocationId, poi, _delegationRatio);
+            paymentCollected = _collectIndexingRewards(allocationId, poi, publicPOI, _delegationRatio);
         } else {
             revert SubgraphServiceInvalidPaymentType(paymentType);
         }
@@ -306,7 +313,7 @@ contract SubgraphService is
         Allocation.State memory allocation = _allocations.get(allocationId);
         require(allocation.isStale(maxPOIStaleness), SubgraphServiceCannotForceCloseAllocation(allocationId));
         require(!allocation.isAltruistic(), SubgraphServiceAllocationIsAltruistic(allocationId));
-        _closeAllocation(allocationId);
+        _closeAllocation(allocationId, true);
     }
 
     /// @inheritdoc ISubgraphService
@@ -522,7 +529,14 @@ contract SubgraphService is
             }
         }
 
-        emit QueryFeesCollected(indexer, _signedRav.rav.payer, tokensCollected, tokensCurators);
+        emit QueryFeesCollected(
+            indexer,
+            _signedRav.rav.payer,
+            allocationId,
+            subgraphDeploymentId,
+            tokensCollected,
+            tokensCurators
+        );
         return tokensCollected;
     }
 

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

packages/subgraph-service/contracts/utilities/AllocationManager.sol

@@ -61,6 +61,7 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca
      * @param tokensIndexerRewards The amount of tokens collected for the indexer
      * @param tokensDelegationRewards The amount of tokens collected for delegators
      * @param poi The POI presented
+     * @param publicPOI The public POI associated with the presented POI
      * @param currentEpoch The current epoch
      */
     event IndexingRewardsCollected(
@@ -71,6 +72,7 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca
         uint256 tokensIndexerRewards,
         uint256 tokensDelegationRewards,
         bytes32 poi,
+        bytes32 publicPOI,
         uint256 currentEpoch
     );
 
@@ -96,12 +98,14 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca
      * @param allocationId The id of the allocation
      * @param subgraphDeploymentId The id of the subgraph deployment
      * @param tokens The amount of tokens allocated
+     * @param forceClosed Whether the allocation was force closed
      */
     event AllocationClosed(
         address indexed indexer,
         address indexed allocationId,
         bytes32 indexed subgraphDeploymentId,
-        uint256 tokens
+        uint256 tokens,
+        bool forceClosed
     );
 
     /**
@@ -261,12 +265,14 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca
      *
      * @param _allocationId The id of the allocation to collect rewards for
      * @param _poi The POI being presented
+     * @param _publicPOI The public POI associated with the presented POI
      * @param _delegationRatio The delegation ratio to consider when locking tokens
      * @return The amount of tokens collected
      */
     function _collectIndexingRewards(
         address _allocationId,
         bytes32 _poi,
+        bytes32 _publicPOI,
         uint32 _delegationRatio
     ) internal returns (uint256) {
         Allocation.State memory allocation = _allocations.get(_allocationId);
@@ -332,12 +338,13 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca
             tokensIndexerRewards,
             tokensDelegationRewards,
             _poi,
+            _publicPOI,
             currentEpoch
         );
 
-        // Check if the indexer is over-allocated and close the allocation if necessary
+        // Check if the indexer is over-allocated and force close the allocation if necessary
         if (_isOverAllocated(allocation.indexer, _delegationRatio)) {
-            _closeAllocation(_allocationId);
+            _closeAllocation(_allocationId, true);
         }
 
         return tokensRewards;
@@ -415,8 +422,9 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca
      * Emits a {AllocationClosed} event
      *
      * @param _allocationId The id of the allocation to be closed
+     * @param _forceClosed Whether the allocation was force closed
      */
-    function _closeAllocation(address _allocationId) internal {
+    function _closeAllocation(address _allocationId, bool _forceClosed) internal {
         Allocation.State memory allocation = _allocations.get(_allocationId);
 
         // Take rewards snapshot to prevent other allos from counting tokens from this allo
@@ -433,7 +441,13 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca
             _subgraphAllocatedTokens[allocation.subgraphDeploymentId] -
             allocation.tokens;
 
-        emit AllocationClosed(allocation.indexer, _allocationId, allocation.subgraphDeploymentId, allocation.tokens);
+        emit AllocationClosed(
+            allocation.indexer,
+            _allocationId,
+            allocation.subgraphDeploymentId,
+            allocation.tokens,
+            _forceClosed
+        );
     }
 
     /**

packages/subgraph-service/test/shared/SubgraphServiceShared.t.sol

@@ -145,7 +145,8 @@ abstract contract SubgraphServiceSharedTest is HorizonStakingSharedTest {
             _indexer,
             allocationId,
             allocation.subgraphDeploymentId,
-            allocation.tokens
+            allocation.tokens,
+            false
         );
         emit IDataService.ServiceStopped(_indexer, _data);
 

packages/subgraph-service/test/subgraphService/SubgraphService.t.sol

@@ -160,7 +160,8 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest {
             allocation.indexer,
             _allocationId,
             allocation.subgraphDeploymentId,
-            allocation.tokens
+            allocation.tokens,
+            true
         );
 
         // close stale allocation
@@ -179,6 +180,7 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest {
 
     struct IndexingRewardsData {
         bytes32 poi;
+        bytes32 publicPOI;
         uint256 tokensIndexerRewards;
         uint256 tokensDelegationRewards;
     }
@@ -278,9 +280,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 +301,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;
     }
@@ -314,7 +324,7 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest {
     function _handleIndexingRewardsCollection(
         bytes memory _data
     ) private returns (uint256 paymentCollected, address allocationId, IndexingRewardsData memory indexingRewardsData) {
-        (allocationId, indexingRewardsData.poi) = abi.decode(_data, (address, bytes32));
+        (allocationId, indexingRewardsData.poi, indexingRewardsData.publicPOI) = abi.decode(_data, (address, bytes32, bytes32));
         Allocation.State memory allocation = subgraphService.getAllocation(allocationId);
 
         // Calculate accumulated tokens, this depends on the rewards manager which we have mocked
@@ -348,6 +358,7 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest {
             indexingRewardsData.tokensIndexerRewards,
             indexingRewardsData.tokensDelegationRewards,
             indexingRewardsData.poi,
+            indexingRewardsData.publicPOI,
             epochManager.currentEpoch()
         );
 

packages/subgraph-service/test/subgraphService/allocation/forceClose.t.sol

@@ -36,7 +36,7 @@ contract SubgraphServiceAllocationForceCloseTest is SubgraphServiceTest {
             // Skip forward
             skip(timeBetweenPOIs);
 
-            bytes memory data = abi.encode(allocationID, bytes32("POI1"));
+            bytes memory data = abi.encode(allocationID, bytes32("POI1"), bytes32("PUBLIC_POI1"));
             _collect(users.indexer, IGraphPayments.PaymentTypes.IndexingRewards, data);
         }
 
@@ -61,7 +61,7 @@ contract SubgraphServiceAllocationForceCloseTest is SubgraphServiceTest {
 
             resetPrank(users.indexer);
 
-            bytes memory data = abi.encode(allocationID, bytes32("POI1"));
+            bytes memory data = abi.encode(allocationID, bytes32("POI1"), bytes32("PUBLIC_POI1"));
             _collect(users.indexer, IGraphPayments.PaymentTypes.IndexingRewards, data);
 
             resetPrank(permissionlessBob);

packages/subgraph-service/test/subgraphService/allocation/resize.t.sol

@@ -39,7 +39,7 @@ contract SubgraphServiceAllocationResizeTest is SubgraphServiceTest {
         vm.roll(block.number + EPOCH_LENGTH);
 
         IGraphPayments.PaymentTypes paymentType = IGraphPayments.PaymentTypes.IndexingRewards;
-        bytes memory data = abi.encode(allocationID, bytes32("POI1"));
+        bytes memory data = abi.encode(allocationID, bytes32("POI1"), bytes32("PUBLIC_POI1"));
         _collect(users.indexer, paymentType, data);
         _addToProvision(users.indexer, resizeTokens);
         _resizeAllocation(users.indexer, allocationID, resizeTokens);