fix: subgraphService access control (OZ C-04)

packages/subgraph-service/contracts/SubgraphService.sol

@@ -221,6 +221,7 @@ contract SubgraphService is
         bytes calldata data
     ) external override onlyProvisionAuthorized(indexer) onlyRegisteredIndexer(indexer) whenNotPaused {
         address allocationId = abi.decode(data, (address));
+        require(allocations[allocationId].indexer == indexer, SubgraphServiceAllocationNotAuthorized(indexer, allocationId));
         _closeAllocation(allocationId);
         emit ServiceStopped(indexer, data);
     }
@@ -250,13 +251,13 @@ contract SubgraphService is
         address indexer,
         IGraphPayments.PaymentTypes paymentType,
         bytes calldata data
-    ) external override onlyValidProvision(indexer) onlyRegisteredIndexer(indexer) whenNotPaused {
+    ) external override onlyProvisionAuthorized(indexer) onlyValidProvision(indexer) onlyRegisteredIndexer(indexer) whenNotPaused {
         uint256 paymentCollected = 0;
 
         if (paymentType == IGraphPayments.PaymentTypes.QueryFee) {
-            paymentCollected = _collectQueryFees(data);
+            paymentCollected = _collectQueryFees(indexer, data);
         } else if (paymentType == IGraphPayments.PaymentTypes.IndexingRewards) {
-            paymentCollected = _collectIndexingRewards(data);
+            paymentCollected = _collectIndexingRewards(indexer, data);
         } else {
             revert SubgraphServiceInvalidPaymentType(paymentType);
         }
@@ -482,16 +483,23 @@ contract SubgraphService is
      * Emits a {StakeClaimLocked} event.
      * Emits a {QueryFeesCollected} event.
      *
+     * @param _indexer The address of the indexer
      * @param _data Encoded data containing a signed RAV
      */
-    function _collectQueryFees(bytes memory _data) private returns (uint256 feesCollected) {
+    function _collectQueryFees(address _indexer, bytes memory _data) private returns (uint256 feesCollected) {
         ITAPCollector.SignedRAV memory signedRav = abi.decode(_data, (ITAPCollector.SignedRAV));
         address indexer = signedRav.rav.serviceProvider;
+        require(_indexer == indexer, SubgraphServiceIndexerMismatch(indexer, _indexer));
         address allocationId = abi.decode(signedRav.rav.metadata, (address));
-        bytes32 subgraphDeploymentId = allocations.get(allocationId).subgraphDeploymentId;
+        Allocation.State memory allocation = allocations.get(allocationId);
+
+        // Not strictly necessary: if an indexer collects another's voucher, they lock their stake with no gains.
+        // We include the check to guard against potential malicious payers deceiving the indexer.
+        require(allocation.indexer == _indexer, SubgraphServiceAllocationNotAuthorized(_indexer, allocationId));
+        bytes32 subgraphDeploymentId = allocation.subgraphDeploymentId;
 
         // release expired stake claims
-        _releaseStake(indexer, 0);
+        _releaseStake(_indexer, 0);
 
         // Collect from GraphPayments
         PaymentCuts memory queryFeePaymentCuts = _getQueryFeePaymentCuts(subgraphDeploymentId);
@@ -515,7 +523,7 @@ contract SubgraphService is
             // lock stake as economic security for fees
             uint256 tokensToLock = tokensCollected * stakeToFeesRatio;
             uint256 unlockTimestamp = block.timestamp + _disputeManager().getDisputePeriod();
-            _lockStake(indexer, tokensToLock, unlockTimestamp);
+            _lockStake(_indexer, tokensToLock, unlockTimestamp);
 
             // calculate service and curator cuts
             tokensCurators = tokensCollected.mulPPMRoundUp(queryFeePaymentCuts.curationCut);
@@ -531,7 +539,7 @@ contract SubgraphService is
             }
         }
 
-        emit QueryFeesCollected(indexer, tokensCollected, tokensCurators, tokensSubgraphService);
+        emit QueryFeesCollected(_indexer, tokensCollected, tokensCurators, tokensSubgraphService);
         return tokensCollected;
     }
 

packages/subgraph-service/contracts/interfaces/ISubgraphService.sol

@@ -93,6 +93,20 @@ interface ISubgraphService is IDataServiceFees {
      */
     error SubgraphServiceInconsistentCollection(uint256 balanceBefore, uint256 balanceAfter, uint256 tokensCollected);
 
+    /**
+     * @notice @notice Thrown when the service provider in the RAV does not match the expected indexer.
+     * @param providedIndexer The address of the provided indexer.
+     * @param expectedIndexer The address of the expected indexer.
+     */
+    error SubgraphServiceIndexerMismatch(address providedIndexer, address expectedIndexer);
+
+    /**
+     * @notice Thrown when the indexer in the allocation state does not match the expected indexer.
+     * @param indexer The address of the expected indexer.
+     * @param allocationId The id of the allocation.
+     */
+    error SubgraphServiceAllocationNotAuthorized(address indexer, address allocationId);
+
     /**
      * @notice Initialize the contract
      * @param minimumProvisionTokens The minimum amount of provisioned tokens required to create an allocation

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

@@ -152,6 +152,13 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca
      */
     error AllocationManagerAllocationSameSize(address allocationId, uint256 tokens);
 
+    /**
+     * @notice Thrown when an indexer is not allocation owner
+     * @param allocationId The id of the allocation
+     * @param indexer The address of the indexer
+     */
+    error AllocationManagerNotAuthorized(address indexer, address allocationId);
+
     /**
      * @notice Initializes the contract and parent contracts
      */
@@ -250,14 +257,16 @@ abstract contract AllocationManager is EIP712Upgradeable, GraphDirectory, Alloca
      *
      * Emits a {IndexingRewardsCollected} event.
      *
+     * @param _indexer The address of the indexer
      * @param _data Encoded data containing:
      * - address `allocationId`: The id of the allocation to collect rewards for
      * - bytes32 `poi`: The POI being presented
      */
-    function _collectIndexingRewards(bytes memory _data) internal returns (uint256) {
+    function _collectIndexingRewards(address _indexer, bytes memory _data) internal returns (uint256) {
         (address allocationId, bytes32 poi) = abi.decode(_data, (address, bytes32));
 
         Allocation.State memory allocation = allocations.get(allocationId);
+        require(allocation.indexer == _indexer, AllocationManagerNotAuthorized(_indexer, allocationId));
         require(allocation.isOpen(), AllocationManagerAllocationClosed(allocationId));
 
         // Mint indexing rewards if all conditions are met

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

@@ -33,4 +33,20 @@ contract SubgraphServiceTest is SubgraphServiceSharedTest {
      * HELPERS
      */
 
+    function _createAndStartAllocation(address indexer, uint256 tokens) internal {
+        mint(indexer, tokens);
+
+        resetPrank(indexer);
+        token.approve(address(staking), tokens);
+        staking.stakeTo(indexer, tokens);
+        staking.provision(indexer, address(subgraphService), tokens, maxSlashingPercentage, disputePeriod);
+        subgraphService.register(indexer, abi.encode("url", "geoHash", address(0)));
+
+        (address newIndexerAllocationId, uint256 newIndexerAllocationKey) = makeAddrAndKey("newIndexerAllocationId");
+        bytes32 digest = subgraphService.encodeAllocationProof(indexer, newIndexerAllocationId);
+        (uint8 v, bytes32 r, bytes32 s) = vm.sign(newIndexerAllocationKey, digest);
+
+        bytes memory data = abi.encode(subgraphDeployment, tokens, newIndexerAllocationId, abi.encodePacked(r, s, v));
+        subgraphService.startService(indexer, data);
+    }
 }

packages/subgraph-service/test/subgraphService/allocate/stop.t.sol

@@ -34,6 +34,21 @@ contract SubgraphServiceAllocateStopTest is SubgraphServiceTest {
         assertEq(subgraphAllocatedTokens, 0);
     }
 
+    function testStop_RevertWhen_IndexerIsNotTheAllocationOwner(uint256 tokens) public useIndexer useAllocation(tokens) {
+        // Setup new indexer
+        address newIndexer = makeAddr("newIndexer");
+        _createAndStartAllocation(newIndexer, tokens);
+
+        // Attempt to close other indexer's allocation
+        bytes memory data = abi.encode(allocationID);
+        vm.expectRevert(abi.encodeWithSelector(
+            ISubgraphService.SubgraphServiceAllocationNotAuthorized.selector,
+            newIndexer,
+            allocationID
+        ));
+        subgraphService.stopService(newIndexer, data);
+    }
+
     function testStop_RevertWhen_NotAuthorized(uint256 tokens) public useIndexer useAllocation(tokens) {
         resetPrank(users.operator);
         bytes memory data = abi.encode(allocationID);

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

@@ -9,6 +9,7 @@ import { ITAPCollector } from "@graphprotocol/horizon/contracts/interfaces/ITAPC
 import { PPMMath } from "@graphprotocol/horizon/contracts/libraries/PPMMath.sol";
 import { ProvisionManager } from "@graphprotocol/horizon/contracts/data-service/utilities/ProvisionManager.sol";
 
+import { AllocationManager } from "../../../contracts/utilities/AllocationManager.sol";
 import { ISubgraphService } from "../../../contracts/interfaces/ISubgraphService.sol";
 import { SubgraphServiceTest } from "../SubgraphService.t.sol";
 
@@ -24,20 +25,21 @@ contract SubgraphServiceRegisterTest is SubgraphServiceTest {
      */
 
     function _getQueryFeeEncodedData(
+        address indexer,
         uint128 tokens
     ) private view returns (bytes memory) {
-        ITAPCollector.ReceiptAggregateVoucher memory rav = _getRAV(tokens);
+        ITAPCollector.ReceiptAggregateVoucher memory rav = _getRAV(indexer, tokens);
         bytes32 messageHash = tapCollector.encodeRAV(rav);
         (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, messageHash);
         bytes memory signature = abi.encodePacked(r, s, v);
         ITAPCollector.SignedRAV memory signedRAV = ITAPCollector.SignedRAV(rav, signature);
         return abi.encode(signedRAV);
     }
 
-    function _getRAV(uint128 tokens) private view returns (ITAPCollector.ReceiptAggregateVoucher memory rav) {
+    function _getRAV(address indexer, uint128 tokens) private view returns (ITAPCollector.ReceiptAggregateVoucher memory rav) {
         return ITAPCollector.ReceiptAggregateVoucher({
             dataService: address(subgraphService),
-            serviceProvider: users.indexer,
+            serviceProvider: indexer,
             timestampNs: 0,
             valueAggregate: tokens,
             metadata: abi.encode(allocationID)
@@ -78,13 +80,13 @@ contract SubgraphServiceRegisterTest is SubgraphServiceTest {
         tokensPayment = bound(tokensPayment, minimumProvisionTokens, maxTokensPayment);
         uint128 tokensPayment128 = uint128(tokensPayment);
         IGraphPayments.PaymentTypes paymentType = IGraphPayments.PaymentTypes.QueryFee;
-        bytes memory data = _getQueryFeeEncodedData(tokensPayment128);
+        bytes memory data = _getQueryFeeEncodedData(users.indexer, tokensPayment128);
 
         uint256 indexerPreviousBalance = token.balanceOf(users.indexer);
 
         _approveCollector(tokensPayment128);
         subgraphService.collect(users.indexer, paymentType, data);
-        
+
         uint256 indexerBalance = token.balanceOf(users.indexer);
         uint256 tokensProtocol = tokensPayment128.mulPPM(protocolPaymentCut);
         uint256 curationTokens = tokensPayment128.mulPPMRoundUp(curationCut);
@@ -115,4 +117,67 @@ contract SubgraphServiceRegisterTest is SubgraphServiceTest {
         ));
         subgraphService.collect(users.indexer, invalidPaymentType, "");
     }
+
+    function testCollect_RevertWhen_NotAuthorized(
+        uint256 tokens
+    ) public useIndexer useAllocation(tokens) {
+        IGraphPayments.PaymentTypes paymentType = IGraphPayments.PaymentTypes.QueryFee;
+        bytes memory data = _getQueryFeeEncodedData(users.indexer, uint128(tokens));
+        resetPrank(users.operator);
+        vm.expectRevert(abi.encodeWithSelector(
+            ProvisionManager.ProvisionManagerNotAuthorized.selector,
+            users.operator,
+            users.indexer
+        ));
+        subgraphService.collect(users.indexer, paymentType, data);
+    }
+
+    function testCollect_QueryFees_RevertWhen_IndexerIsNotAllocationOwner(
+        uint256 tokens
+    ) public useIndexer useAllocation(tokens) {
+        IGraphPayments.PaymentTypes paymentType = IGraphPayments.PaymentTypes.QueryFee;
+        // Setup new indexer
+        address newIndexer = makeAddr("newIndexer");
+        _createAndStartAllocation(newIndexer, tokens);
+        bytes memory data = _getQueryFeeEncodedData(newIndexer, uint128(tokens));
+        vm.expectRevert(abi.encodeWithSelector(
+            ISubgraphService.SubgraphServiceAllocationNotAuthorized.selector,
+            newIndexer,
+            allocationID
+        ));
+        subgraphService.collect(newIndexer, paymentType, data);
+    }
+
+    function testCollect_QueryFees_RevertWhen_CollectingOtherIndexersFees(
+        uint256 tokens
+    ) public useIndexer useAllocation(tokens) {
+        IGraphPayments.PaymentTypes paymentType = IGraphPayments.PaymentTypes.QueryFee;
+        // Setup new indexer
+        address newIndexer = makeAddr("newIndexer");
+        _createAndStartAllocation(newIndexer, tokens);
+        bytes memory data = _getQueryFeeEncodedData(users.indexer, uint128(tokens));
+        vm.expectRevert(abi.encodeWithSelector(
+            ISubgraphService.SubgraphServiceIndexerMismatch.selector,
+            users.indexer,
+            newIndexer
+        ));
+        subgraphService.collect(newIndexer, paymentType, data);
+    }
+
+    function testCollect_IndexingFees_RevertWhen_IndexerIsNotAllocationOwner(
+        uint256 tokens
+    ) public useIndexer useAllocation(tokens) {
+        IGraphPayments.PaymentTypes paymentType = IGraphPayments.PaymentTypes.IndexingRewards;
+        // Setup new indexer
+        address newIndexer = makeAddr("newIndexer");
+        _createAndStartAllocation(newIndexer, tokens);
+        bytes memory data = abi.encode(allocationID, bytes32("POI1"));
+        // Attempt to collect from other indexer's allocation
+        vm.expectRevert(abi.encodeWithSelector(
+            AllocationManager.AllocationManagerNotAuthorized.selector,
+            newIndexer,
+            allocationID
+        ));
+        subgraphService.collect(newIndexer, paymentType, data);
+    }
 }