Merge branch 'horizon' into tmigone/horizon-interfaces

Author: tmigone

packages/horizon/audits/The Graph Horizon Missed Issues Initial Report.pdf

@@ -358,7 +358,7 @@ contract HorizonStakingExtension is HorizonStakingBase, IHorizonStakingExtension
         // Process non-zero-allocation rewards tracking
         if (alloc.tokens > 0) {
             // Distribute rewards if proof of indexing was presented by the indexer or operator
-            if (isIndexerOrOperator && _poi != 0) {
+            if (isIndexerOrOperator && _poi != 0 && epochs > 0) {
                 _distributeRewards(_allocationID, alloc.indexer);
             } else {
                 _updateRewards(alloc.subgraphDeploymentID);

packages/horizon/contracts/staking/HorizonStakingExtension.sol

@@ -123,6 +123,7 @@ interface IDisputeManager {
      * @param tokens The amount of tokens deposited by the fisherman
      * @param allocationId The allocation id
      * @param poi The POI
+     * @param blockNumber The block number for which the POI was calculated
      * @param stakeSnapshot The stake snapshot of the indexer at the time of the dispute
      * @param cancellableAt The timestamp when the dispute can be cancelled
      */
@@ -133,6 +134,7 @@ interface IDisputeManager {
         uint256 tokens,
         address allocationId,
         bytes32 poi,
+        uint256 blockNumber,
         uint256 stakeSnapshot,
         uint256 cancellableAt
     );
@@ -458,9 +460,10 @@ interface IDisputeManager {
      *
      * @param allocationId The allocation to dispute
      * @param poi The Proof of Indexing (POI) being disputed
+     * @param blockNumber The block number for which the POI was calculated
      * @return The dispute id
      */
-    function createIndexingDispute(address allocationId, bytes32 poi) external returns (bytes32);
+    function createIndexingDispute(address allocationId, bytes32 poi, uint256 blockNumber) external returns (bytes32);
 
     /**
      * @notice Creates and auto-accepts a legacy dispute.

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

@@ -121,12 +121,16 @@ contract DisputeManager is
     }
 
     /// @inheritdoc IDisputeManager
-    function createIndexingDispute(address allocationId, bytes32 poi) external override returns (bytes32) {
+    function createIndexingDispute(
+        address allocationId,
+        bytes32 poi,
+        uint256 blockNumber
+    ) external override returns (bytes32) {
         // Get funds from fisherman
         _graphToken().pullTokens(msg.sender, disputeDeposit);
 
         // Create a dispute
-        return _createIndexingDisputeWithAllocation(msg.sender, disputeDeposit, allocationId, poi);
+        return _createIndexingDisputeWithAllocation(msg.sender, disputeDeposit, allocationId, poi, blockNumber);
     }
 
     /// @inheritdoc IDisputeManager
@@ -341,11 +345,7 @@ contract DisputeManager is
 
     /// @inheritdoc IDisputeManager
     function getStakeSnapshot(address indexer) external view override returns (uint256) {
-        IHorizonStaking.Provision memory provision = _graphStaking().getProvision(
-            indexer,
-            address(_getSubgraphService())
-        );
-        return _getStakeSnapshot(indexer, provision.tokens);
+        return _getStakeSnapshot(indexer);
     }
 
     /// @inheritdoc IDisputeManager
@@ -395,13 +395,6 @@ contract DisputeManager is
         // Get the indexer that signed the attestation
         address indexer = getAttestationIndexer(_attestation);
 
-        // The indexer is disputable
-        IHorizonStaking.Provision memory provision = _graphStaking().getProvision(
-            indexer,
-            address(_getSubgraphService())
-        );
-        require(provision.tokens != 0, DisputeManagerZeroTokens());
-
         // Create a disputeId
         bytes32 disputeId = keccak256(
             abi.encodePacked(
@@ -416,8 +409,11 @@ contract DisputeManager is
         // Only one dispute at a time
         require(!isDisputeCreated(disputeId), DisputeManagerDisputeAlreadyCreated(disputeId));
 
+        // The indexer is disputable
+        uint256 stakeSnapshot = _getStakeSnapshot(indexer);
+        require(stakeSnapshot != 0, DisputeManagerZeroTokens());
+
         // Store dispute
-        uint256 stakeSnapshot = _getStakeSnapshot(indexer, provision.tokens);
         uint256 cancellableAt = block.timestamp + disputePeriod;
         disputes[disputeId] = Dispute(
             indexer,
@@ -451,16 +447,18 @@ contract DisputeManager is
      * @param _deposit Amount of tokens staked as deposit
      * @param _allocationId Allocation disputed
      * @param _poi The POI being disputed
+     * @param _blockNumber The block number for which the POI was calculated
      * @return The dispute id
      */
     function _createIndexingDisputeWithAllocation(
         address _fisherman,
         uint256 _deposit,
         address _allocationId,
-        bytes32 _poi
+        bytes32 _poi,
+        uint256 _blockNumber
     ) private returns (bytes32) {
         // Create a disputeId
-        bytes32 disputeId = keccak256(abi.encodePacked(_allocationId, _poi));
+        bytes32 disputeId = keccak256(abi.encodePacked(_allocationId, _poi, _blockNumber));
 
         // Only one dispute for an allocationId at a time
         require(!isDisputeCreated(disputeId), DisputeManagerDisputeAlreadyCreated(disputeId));
@@ -472,11 +470,10 @@ contract DisputeManager is
         require(indexer != address(0), DisputeManagerIndexerNotFound(_allocationId));
 
         // The indexer must be disputable
-        IHorizonStaking.Provision memory provision = _graphStaking().getProvision(indexer, address(subgraphService_));
-        require(provision.tokens != 0, DisputeManagerZeroTokens());
+        uint256 stakeSnapshot = _getStakeSnapshot(indexer);
+        require(stakeSnapshot != 0, DisputeManagerZeroTokens());
 
         // Store dispute
-        uint256 stakeSnapshot = _getStakeSnapshot(indexer, provision.tokens);
         uint256 cancellableAt = block.timestamp + disputePeriod;
         disputes[disputeId] = Dispute(
             alloc.indexer,
@@ -497,6 +494,7 @@ contract DisputeManager is
             _deposit,
             _allocationId,
             _poi,
+            _blockNumber,
             stakeSnapshot,
             cancellableAt
         );
@@ -685,18 +683,19 @@ contract DisputeManager is
      * @dev A few considerations:
      * - We include both indexer and delegators stake.
      * - Thawing stake is not excluded from the snapshot.
-     * - Delegators stake is capped at the delegation ratio to prevent delegators from inflating the snapshot
-     *   to increase the indexer slash amount.
      *
      * Note that the snapshot can be inflated by delegators front-running the dispute creation with a delegation
      * to the indexer. Given the snapshot is a cap, the dispute outcome is uncertain and considering the cost of capital
      * and slashing risk, this is not a concern.
      * @param _indexer Indexer address
-     * @param _indexerStake Indexer's stake
      * @return Total stake snapshot
      */
-    function _getStakeSnapshot(address _indexer, uint256 _indexerStake) private view returns (uint256) {
-        uint256 delegatorsStake = _graphStaking().getDelegationPool(_indexer, address(_getSubgraphService())).tokens;
-        return _indexerStake + delegatorsStake;
+    function _getStakeSnapshot(address _indexer) private view returns (uint256) {
+        address subgraphService = address(_getSubgraphService());
+
+        IHorizonStaking.Provision memory provision = _graphStaking().getProvision(_indexer, subgraphService);
+        uint256 delegatorsStake = _graphStaking().getDelegationPool(_indexer, subgraphService).tokens;
+
+        return provision.tokens + delegatorsStake;
     }
 }

packages/subgraph-service/contracts/DisputeManager.sol

@@ -70,9 +70,9 @@ contract DisputeManagerTest is SubgraphServiceSharedTest {
         assertEq(address(disputeManager.subgraphService()), _subgraphService, "Subgraph service should be set.");
     }
 
-    function _createIndexingDispute(address _allocationId, bytes32 _poi) internal returns (bytes32) {
+    function _createIndexingDispute(address _allocationId, bytes32 _poi, uint256 _blockNumber) internal returns (bytes32) {
         (, address fisherman, ) = vm.readCallers();
-        bytes32 expectedDisputeId = keccak256(abi.encodePacked(_allocationId, _poi));
+        bytes32 expectedDisputeId = keccak256(abi.encodePacked(_allocationId, _poi, _blockNumber));
         uint256 disputeDeposit = disputeManager.disputeDeposit();
         uint256 beforeFishermanBalance = token.balanceOf(fisherman);
         IAllocation.State memory alloc = subgraphService.getAllocation(_allocationId);
@@ -89,13 +89,14 @@ contract DisputeManagerTest is SubgraphServiceSharedTest {
             fisherman,
             disputeDeposit,
             _allocationId,
-            _poi,
+            _poi,   
+            _blockNumber,
             stakeSnapshot,
             cancellableAt
         );
 
         // Create the indexing dispute
-        bytes32 _disputeId = disputeManager.createIndexingDispute(_allocationId, _poi);
+        bytes32 _disputeId = disputeManager.createIndexingDispute(_allocationId, _poi, _blockNumber);
 
         // Check that the dispute was created and that it has the correct ID
         assertTrue(disputeManager.isDisputeCreated(_disputeId), "Dispute should be created.");

packages/subgraph-service/test/unit/disputeManager/DisputeManager.t.sol

@@ -29,7 +29,7 @@ contract DisputeManagerDisputeTest is DisputeManagerTest {
 
     function test_Dispute_Accept_RevertIf_SlashZeroTokens(uint256 tokens) public useIndexer useAllocation(tokens) {
         resetPrank(users.fisherman);
-        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI101"));
+        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI101"), block.number);
 
         // attempt to accept dispute with 0 tokens slashed
         resetPrank(users.arbitrator);

packages/subgraph-service/test/unit/disputeManager/disputes/disputes.t.sol

@@ -18,7 +18,7 @@ contract DisputeManagerIndexingAcceptDisputeTest is DisputeManagerTest {
         tokensSlash = bound(tokensSlash, 1, uint256(maxSlashingPercentage).mulPPM(tokens));
 
         resetPrank(users.fisherman);
-        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));
+        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"), block.number);
 
         resetPrank(users.arbitrator);
         _acceptDispute(disputeID, tokensSlash);
@@ -31,7 +31,7 @@ contract DisputeManagerIndexingAcceptDisputeTest is DisputeManagerTest {
         tokensSlash = bound(tokensSlash, 1, uint256(maxSlashingPercentage).mulPPM(tokens));
 
         resetPrank(users.fisherman);
-        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));
+        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"), block.number);
 
         resetPrank(users.arbitrator);
         // clear subgraph service address from storage
@@ -48,7 +48,7 @@ contract DisputeManagerIndexingAcceptDisputeTest is DisputeManagerTest {
         tokensSlash = bound(tokensSlash, 1, uint256(maxSlashingPercentage).mulPPM(tokens));
 
         resetPrank(users.fisherman);
-        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));
+        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"), block.number);
 
         resetPrank(users.arbitrator);
         _acceptDispute(disputeID, tokensSlash);
@@ -61,7 +61,7 @@ contract DisputeManagerIndexingAcceptDisputeTest is DisputeManagerTest {
         tokensSlash = bound(tokensSlash, 1, uint256(maxSlashingPercentage).mulPPM(tokens));
 
         resetPrank(users.fisherman);
-        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));
+        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"), block.number);
 
         // attempt to accept dispute as fisherman
         resetPrank(users.fisherman);
@@ -75,7 +75,7 @@ contract DisputeManagerIndexingAcceptDisputeTest is DisputeManagerTest {
     ) public useIndexer useAllocation(tokens) {
         resetPrank(users.fisherman);
         tokensSlash = bound(tokensSlash, uint256(maxSlashingPercentage).mulPPM(tokens) + 1, type(uint256).max);
-        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI101"));
+        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI101"), block.number);
 
         // max slashing percentage is 50%
         resetPrank(users.arbitrator);

packages/subgraph-service/test/unit/disputeManager/disputes/indexing/accept.t.sol

@@ -13,7 +13,7 @@ contract DisputeManagerIndexingCancelDisputeTest is DisputeManagerTest {
 
     function test_Indexing_Cancel_Dispute(uint256 tokens) public useIndexer useAllocation(tokens) {
         resetPrank(users.fisherman);
-        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));
+        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"), block.number);
 
         // skip to end of dispute period
         uint256 disputePeriod = disputeManager.disputePeriod();
@@ -26,7 +26,7 @@ contract DisputeManagerIndexingCancelDisputeTest is DisputeManagerTest {
         uint256 tokens
     ) public useIndexer useAllocation(tokens) {
         resetPrank(users.fisherman);
-        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));
+        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"), block.number);
 
         resetPrank(users.arbitrator);
         vm.expectRevert(abi.encodeWithSelector(IDisputeManager.DisputeManagerNotFisherman.selector));
@@ -37,15 +37,15 @@ contract DisputeManagerIndexingCancelDisputeTest is DisputeManagerTest {
         uint256 tokens
     ) public useIndexer useAllocation(tokens) {
         resetPrank(users.fisherman);
-        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));
+        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"), block.number);
 
         vm.expectRevert(abi.encodeWithSelector(IDisputeManager.DisputeManagerDisputePeriodNotFinished.selector));
         disputeManager.cancelDispute(disputeID);
     }
 
     function test_Indexing_Cancel_After_DisputePeriodIncreased(uint256 tokens) public useIndexer useAllocation(tokens) {
         resetPrank(users.fisherman);
-        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));
+        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"), block.number);
 
         // change the dispute period to a higher value
         uint256 oldDisputePeriod = disputeManager.disputePeriod();
@@ -62,7 +62,7 @@ contract DisputeManagerIndexingCancelDisputeTest is DisputeManagerTest {
 
     function test_Indexing_Cancel_After_DisputePeriodDecreased(uint256 tokens) public useIndexer useAllocation(tokens) {
         resetPrank(users.fisherman);
-        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));
+        bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"), block.number);
 
         // change the dispute period to a lower value
         uint256 oldDisputePeriod = disputeManager.disputePeriod();