From 8f1af3e610d2fdd379b2e66e5be466013e123be2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Migone?= Date: Fri, 29 Nov 2024 17:25:45 -0300 Subject: [PATCH] fix: require fisherman deposit for conflicting query disputes (TRST-M07) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomás Migone --- .../contracts/DisputeManager.sol | 25 +- .../test/disputeManager/DisputeManager.t.sol | 214 +++++++++++++----- 2 files changed, 184 insertions(+), 55 deletions(-) diff --git a/packages/subgraph-service/contracts/DisputeManager.sol b/packages/subgraph-service/contracts/DisputeManager.sol index 58c93a756..73062e0a9 100644 --- a/packages/subgraph-service/contracts/DisputeManager.sol +++ b/packages/subgraph-service/contracts/DisputeManager.sol @@ -174,10 +174,14 @@ contract DisputeManager is * @notice Create query disputes for two conflicting attestations. * A conflicting attestation is a proof presented by two different indexers * where for the same request on a subgraph the response is different. - * For this type of dispute the fisherman is not required to present a deposit - * as one of the attestation is considered to be right. + * For this type of dispute it's safe to assume one of the attestations is incorrect + * so the fisherman's deposit would not be necesarry, however to prevent spam attacks + * we still require it. * Two linked disputes will be created and if the arbitrator resolve one, the other * one will be automatically resolved. + * Requirements: + * - fisherman must have previously approved this contract to pull `disputeDeposit` amount + * of tokens from their balance. * @param attestationData1 First attestation data submitted * @param attestationData2 Second attestation data submitted * @return DisputeId1, DisputeId2 @@ -205,10 +209,23 @@ contract DisputeManager is ) ); + // Get funds from fisherman + _pullFishermanDeposit(); + // Create the disputes // The deposit is zero for conflicting attestations - bytes32 dId1 = _createQueryDisputeWithAttestation(fisherman, 0, attestation1, attestationData1); - bytes32 dId2 = _createQueryDisputeWithAttestation(fisherman, 0, attestation2, attestationData2); + bytes32 dId1 = _createQueryDisputeWithAttestation( + fisherman, + disputeDeposit / 2, + attestation1, + attestationData1 + ); + bytes32 dId2 = _createQueryDisputeWithAttestation( + fisherman, + disputeDeposit / 2, + attestation2, + attestationData2 + ); // Store the linked disputes to be resolved disputes[dId1].relatedDisputeId = dId2; diff --git a/packages/subgraph-service/test/disputeManager/DisputeManager.t.sol b/packages/subgraph-service/test/disputeManager/DisputeManager.t.sol index 0d99aec0b..464f202e6 100644 --- a/packages/subgraph-service/test/disputeManager/DisputeManager.t.sol +++ b/packages/subgraph-service/test/disputeManager/DisputeManager.t.sol @@ -26,7 +26,7 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { vm.stopPrank(); } - modifier useFisherman { + modifier useFisherman() { vm.startPrank(users.fisherman); _; vm.stopPrank(); @@ -88,7 +88,7 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { // Create the indexing dispute bytes32 _disputeId = disputeManager.createIndexingDispute(_allocationId, _poi); - + // Check that the dispute was created and that it has the correct ID assertTrue(disputeManager.isDisputeCreated(_disputeId), "Dispute should be created."); assertEq(expectedDisputeId, _disputeId, "Dispute ID should match"); @@ -99,20 +99,32 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { assertEq(dispute.fisherman, fisherman, "Fisherman should match"); assertEq(dispute.deposit, disputeDeposit, "Deposit should match"); assertEq(dispute.relatedDisputeId, bytes32(0), "Related dispute ID should be empty"); - assertEq(uint8(dispute.disputeType), uint8(IDisputeManager.DisputeType.IndexingDispute), "Dispute type should be indexing"); - assertEq(uint8(dispute.status), uint8(IDisputeManager.DisputeStatus.Pending), "Dispute status should be pending"); + assertEq( + uint8(dispute.disputeType), + uint8(IDisputeManager.DisputeType.IndexingDispute), + "Dispute type should be indexing" + ); + assertEq( + uint8(dispute.status), + uint8(IDisputeManager.DisputeStatus.Pending), + "Dispute status should be pending" + ); assertEq(dispute.createdAt, block.timestamp, "Created at should match"); assertEq(dispute.stakeSnapshot, stakeSnapshot, "Stake snapshot should match"); // Check that the fisherman was charged the dispute deposit uint256 afterFishermanBalance = token.balanceOf(fisherman); - assertEq(afterFishermanBalance, beforeFishermanBalance - disputeDeposit, "Fisherman should be charged the dispute deposit"); + assertEq( + afterFishermanBalance, + beforeFishermanBalance - disputeDeposit, + "Fisherman should be charged the dispute deposit" + ); return _disputeId; } function _createQueryDispute(bytes memory _attestationData) internal returns (bytes32) { - (, address fisherman,) = vm.readCallers(); + (, address fisherman, ) = vm.readCallers(); Attestation.State memory attestation = Attestation.parse(_attestationData); address indexer = disputeManager.getAttestationIndexer(attestation); bytes32 expectedDisputeId = keccak256( @@ -130,7 +142,7 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { // Approve the dispute deposit token.approve(address(disputeManager), disputeDeposit); - + vm.expectEmit(address(disputeManager)); emit IDisputeManager.QueryDisputeCreated( expectedDisputeId, @@ -141,9 +153,9 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { _attestationData, stakeSnapshot ); - + bytes32 _disputeID = disputeManager.createQueryDispute(_attestationData); - + // Check that the dispute was created and that it has the correct ID assertTrue(disputeManager.isDisputeCreated(_disputeID), "Dispute should be created."); assertEq(expectedDisputeId, _disputeID, "Dispute ID should match"); @@ -154,15 +166,27 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { assertEq(dispute.fisherman, fisherman, "Fisherman should match"); assertEq(dispute.deposit, disputeDeposit, "Deposit should match"); assertEq(dispute.relatedDisputeId, bytes32(0), "Related dispute ID should be empty"); - assertEq(uint8(dispute.disputeType), uint8(IDisputeManager.DisputeType.QueryDispute), "Dispute type should be query"); - assertEq(uint8(dispute.status), uint8(IDisputeManager.DisputeStatus.Pending), "Dispute status should be pending"); + assertEq( + uint8(dispute.disputeType), + uint8(IDisputeManager.DisputeType.QueryDispute), + "Dispute type should be query" + ); + assertEq( + uint8(dispute.status), + uint8(IDisputeManager.DisputeStatus.Pending), + "Dispute status should be pending" + ); assertEq(dispute.createdAt, block.timestamp, "Created at should match"); assertEq(dispute.stakeSnapshot, stakeSnapshot, "Stake snapshot should match"); // Check that the fisherman was charged the dispute deposit uint256 afterFishermanBalance = token.balanceOf(fisherman); - assertEq(afterFishermanBalance, beforeFishermanBalance - disputeDeposit, "Fisherman should be charged the dispute deposit"); - + assertEq( + afterFishermanBalance, + beforeFishermanBalance - disputeDeposit, + "Fisherman should be charged the dispute deposit" + ); + return _disputeID; } @@ -174,11 +198,12 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { uint256 stakeSnapshot1; uint256 stakeSnapshot2; } + function _createQueryDisputeConflict( bytes memory attestationData1, bytes memory attestationData2 ) internal returns (bytes32, bytes32) { - (, address fisherman,) = vm.readCallers(); + (, address fisherman, ) = vm.readCallers(); BeforeValues_CreateQueryDisputeConflict memory beforeValues; beforeValues.attestation1 = Attestation.parse(attestationData1); @@ -187,6 +212,11 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { beforeValues.indexer2 = disputeManager.getAttestationIndexer(beforeValues.attestation2); beforeValues.stakeSnapshot1 = disputeManager.getStakeSnapshot(beforeValues.indexer1); beforeValues.stakeSnapshot2 = disputeManager.getStakeSnapshot(beforeValues.indexer2); + + uint256 beforeFishermanBalance = token.balanceOf(fisherman); + + // Approve the dispute deposit + token.approve(address(disputeManager), disputeDeposit); bytes32 expectedDisputeId1 = keccak256( abi.encodePacked( @@ -213,7 +243,7 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { expectedDisputeId1, beforeValues.indexer1, fisherman, - 0, + disputeDeposit / 2, beforeValues.attestation1.subgraphDeploymentId, attestationData1, beforeValues.stakeSnapshot1 @@ -223,13 +253,16 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { expectedDisputeId2, beforeValues.indexer2, fisherman, - 0, + disputeDeposit / 2, beforeValues.attestation2.subgraphDeploymentId, attestationData2, beforeValues.stakeSnapshot2 ); - (bytes32 _disputeId1, bytes32 _disputeId2) = disputeManager.createQueryDisputeConflict(attestationData1, attestationData2); + (bytes32 _disputeId1, bytes32 _disputeId2) = disputeManager.createQueryDisputeConflict( + attestationData1, + attestationData2 + ); // Check that the disputes were created and that they have the correct IDs assertTrue(disputeManager.isDisputeCreated(_disputeId1), "Dispute 1 should be created."); @@ -241,23 +274,47 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { IDisputeManager.Dispute memory dispute1 = _getDispute(_disputeId1); assertEq(dispute1.indexer, beforeValues.indexer1, "Indexer 1 should match"); assertEq(dispute1.fisherman, fisherman, "Fisherman 1 should match"); - assertEq(dispute1.deposit, 0, "Deposit 1 should match"); + assertEq(dispute1.deposit, disputeDeposit / 2, "Deposit 1 should match"); assertEq(dispute1.relatedDisputeId, _disputeId2, "Related dispute ID 1 should be the id of the other dispute"); - assertEq(uint8(dispute1.disputeType), uint8(IDisputeManager.DisputeType.QueryDispute), "Dispute type 1 should be query"); - assertEq(uint8(dispute1.status), uint8(IDisputeManager.DisputeStatus.Pending), "Dispute status 1 should be pending"); + assertEq( + uint8(dispute1.disputeType), + uint8(IDisputeManager.DisputeType.QueryDispute), + "Dispute type 1 should be query" + ); + assertEq( + uint8(dispute1.status), + uint8(IDisputeManager.DisputeStatus.Pending), + "Dispute status 1 should be pending" + ); assertEq(dispute1.createdAt, block.timestamp, "Created at 1 should match"); assertEq(dispute1.stakeSnapshot, beforeValues.stakeSnapshot1, "Stake snapshot 1 should match"); IDisputeManager.Dispute memory dispute2 = _getDispute(_disputeId2); assertEq(dispute2.indexer, beforeValues.indexer2, "Indexer 2 should match"); assertEq(dispute2.fisherman, fisherman, "Fisherman 2 should match"); - assertEq(dispute2.deposit, 0, "Deposit 2 should match"); + assertEq(dispute2.deposit, disputeDeposit / 2, "Deposit 2 should match"); assertEq(dispute2.relatedDisputeId, _disputeId1, "Related dispute ID 2 should be the id of the other dispute"); - assertEq(uint8(dispute2.disputeType), uint8(IDisputeManager.DisputeType.QueryDispute), "Dispute type 2 should be query"); - assertEq(uint8(dispute2.status), uint8(IDisputeManager.DisputeStatus.Pending), "Dispute status 2 should be pending"); + assertEq( + uint8(dispute2.disputeType), + uint8(IDisputeManager.DisputeType.QueryDispute), + "Dispute type 2 should be query" + ); + assertEq( + uint8(dispute2.status), + uint8(IDisputeManager.DisputeStatus.Pending), + "Dispute status 2 should be pending" + ); assertEq(dispute2.createdAt, block.timestamp, "Created at 2 should match"); assertEq(dispute2.stakeSnapshot, beforeValues.stakeSnapshot2, "Stake snapshot 2 should match"); + // Check that the fisherman was charged the dispute deposit + uint256 afterFishermanBalance = token.balanceOf(fisherman); + assertEq( + afterFishermanBalance, + beforeFishermanBalance - disputeDeposit, + "Fisherman should be charged the dispute deposit" + ); + return (_disputeId1, _disputeId2); } @@ -271,14 +328,23 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { uint256 fishermanReward = _tokensSlash.mulPPM(fishermanRewardPercentage); vm.expectEmit(address(disputeManager)); - emit IDisputeManager.DisputeAccepted(_disputeId, dispute.indexer, dispute.fisherman, dispute.deposit + fishermanReward); + emit IDisputeManager.DisputeAccepted( + _disputeId, + dispute.indexer, + dispute.fisherman, + dispute.deposit + fishermanReward + ); // Accept the dispute disputeManager.acceptDispute(_disputeId, _tokensSlash); // Check fisherman's got their reward and their deposit (if any) back uint256 fishermanExpectedBalance = fishermanPreviousBalance + fishermanReward + disputeDeposit; - assertEq(token.balanceOf(fisherman), fishermanExpectedBalance, "Fisherman should get their reward and deposit back"); + assertEq( + token.balanceOf(fisherman), + fishermanExpectedBalance, + "Fisherman should get their reward and deposit back" + ); // Check indexer was slashed by the correct amount uint256 expectedIndexerTokensAvailable; @@ -287,16 +353,28 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { } else { expectedIndexerTokensAvailable = indexerTokensAvailable - _tokensSlash; } - assertEq(staking.getProviderTokensAvailable(dispute.indexer, address(subgraphService)), expectedIndexerTokensAvailable, "Indexer should be slashed by the correct amount"); + assertEq( + staking.getProviderTokensAvailable(dispute.indexer, address(subgraphService)), + expectedIndexerTokensAvailable, + "Indexer should be slashed by the correct amount" + ); // Check dispute status dispute = _getDispute(_disputeId); - assertEq(uint8(dispute.status), uint8(IDisputeManager.DisputeStatus.Accepted), "Dispute status should be accepted"); + assertEq( + uint8(dispute.status), + uint8(IDisputeManager.DisputeStatus.Accepted), + "Dispute status should be accepted" + ); // If there's a related dispute, check that it was rejected if (dispute.relatedDisputeId != bytes32(0)) { IDisputeManager.Dispute memory relatedDispute = _getDispute(dispute.relatedDisputeId); - assertEq(uint8(relatedDispute.status), uint8(IDisputeManager.DisputeStatus.Rejected), "Related dispute status should be rejected"); + assertEq( + uint8(relatedDispute.status), + uint8(IDisputeManager.DisputeStatus.Rejected), + "Related dispute status should be rejected" + ); } } @@ -317,7 +395,11 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { assertEq(token.balanceOf(fisherman), fishermanExpectedBalance, "Fisherman should receive their deposit back."); // Check that indexer was not slashed - assertEq(staking.getProviderTokensAvailable(dispute.indexer, address(subgraphService)), indexerTokensAvailable, "Indexer should not be slashed"); + assertEq( + staking.getProviderTokensAvailable(dispute.indexer, address(subgraphService)), + indexerTokensAvailable, + "Indexer should not be slashed" + ); // Check dispute status dispute = _getDispute(_disputeId); @@ -326,7 +408,11 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { // If there's a related dispute, check that it was drawn too if (dispute.relatedDisputeId != bytes32(0)) { IDisputeManager.Dispute memory relatedDispute = _getDispute(dispute.relatedDisputeId); - assertEq(uint8(relatedDispute.status), uint8(IDisputeManager.DisputeStatus.Drawn), "Related dispute status should be drawn"); + assertEq( + uint8(relatedDispute.status), + uint8(IDisputeManager.DisputeStatus.Drawn), + "Related dispute status should be drawn" + ); } } @@ -346,11 +432,19 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { assertEq(token.balanceOf(users.fisherman), fishermanPreviousBalance, "Fisherman should lose the deposit."); // Check that indexer was not slashed - assertEq(staking.getProviderTokensAvailable(dispute.indexer, address(subgraphService)), indexerTokensAvailable, "Indexer should not be slashed"); + assertEq( + staking.getProviderTokensAvailable(dispute.indexer, address(subgraphService)), + indexerTokensAvailable, + "Indexer should not be slashed" + ); // Check dispute status dispute = _getDispute(_disputeId); - assertEq(uint8(dispute.status), uint8(IDisputeManager.DisputeStatus.Rejected), "Dispute status should be rejected"); + assertEq( + uint8(dispute.status), + uint8(IDisputeManager.DisputeStatus.Rejected), + "Dispute status should be rejected" + ); // Checl related id is empty assertEq(dispute.relatedDisputeId, bytes32(0), "Related dispute ID should be empty"); } @@ -373,19 +467,35 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { // Check that the fisherman got their deposit back uint256 fishermanExpectedBalance = fishermanPreviousBalance + dispute.deposit; - assertEq(token.balanceOf(users.fisherman), fishermanExpectedBalance, "Fisherman should receive their deposit back."); + assertEq( + token.balanceOf(users.fisherman), + fishermanExpectedBalance, + "Fisherman should receive their deposit back." + ); // Check that indexer was not slashed - assertEq(staking.getProviderTokensAvailable(dispute.indexer, address(subgraphService)), indexerTokensAvailable, "Indexer should not be slashed"); + assertEq( + staking.getProviderTokensAvailable(dispute.indexer, address(subgraphService)), + indexerTokensAvailable, + "Indexer should not be slashed" + ); // Check dispute status dispute = _getDispute(_disputeId); - assertEq(uint8(dispute.status), uint8(IDisputeManager.DisputeStatus.Cancelled), "Dispute status should be cancelled"); + assertEq( + uint8(dispute.status), + uint8(IDisputeManager.DisputeStatus.Cancelled), + "Dispute status should be cancelled" + ); // If there's a related dispute, check that it was cancelled too if (dispute.relatedDisputeId != bytes32(0)) { IDisputeManager.Dispute memory relatedDispute = _getDispute(dispute.relatedDisputeId); - assertEq(uint8(relatedDispute.status), uint8(IDisputeManager.DisputeStatus.Cancelled), "Related dispute status should be cancelled"); + assertEq( + uint8(relatedDispute.status), + uint8(IDisputeManager.DisputeStatus.Cancelled), + "Related dispute status should be cancelled" + ); } } @@ -398,11 +508,12 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { bytes32 responseCID, bytes32 subgraphDeploymentId ) internal pure returns (Attestation.Receipt memory receipt) { - return Attestation.Receipt({ - requestCID: requestCID, - responseCID: responseCID, - subgraphDeploymentId: subgraphDeploymentId - }); + return + Attestation.Receipt({ + requestCID: requestCID, + responseCID: responseCID, + subgraphDeploymentId: subgraphDeploymentId + }); } function _createConflictingAttestations( @@ -446,15 +557,16 @@ contract DisputeManagerTest is SubgraphServiceSharedTest { uint256 createdAt, uint256 stakeSnapshot ) = disputeManager.disputes(_disputeId); - return IDisputeManager.Dispute({ - indexer: indexer, - fisherman: fisherman, - deposit: deposit, - relatedDisputeId: relatedDisputeId, - disputeType: disputeType, - status: status, - createdAt: createdAt, - stakeSnapshot: stakeSnapshot - }); + return + IDisputeManager.Dispute({ + indexer: indexer, + fisherman: fisherman, + deposit: deposit, + relatedDisputeId: relatedDisputeId, + disputeType: disputeType, + status: status, + createdAt: createdAt, + stakeSnapshot: stakeSnapshot + }); } }