Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TRST audit fixes for DisputeManager contract #1075

Open
wants to merge 3 commits into
base: horizon
Choose a base branch
from
Open

TRST audit fixes for DisputeManager contract #1075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d75947
fix: dispute manager refactor (TRST-M07 / TRST-M08)
tmigone Nov 29, 2024
0a31b58
fix: ensure subgraph service is set before using it (TRST-R10)
tmigone Dec 9, 2024
85de5bd
fix: documentation error (TRST-R09)
tmigone Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
231 changes: 116 additions & 115 deletions packages/subgraph-service/contracts/DisputeManager.sol
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion packages/subgraph-service/contracts/interfaces/IDisputeManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ interface IDisputeManager {
error DisputeManagerDisputeNotPending(IDisputeManager.DisputeStatus status);
error DisputeManagerDisputeAlreadyCreated(bytes32 disputeId);
error DisputeManagerDisputePeriodNotFinished();
error DisputeManagerDisputeInConflict(bytes32 disputeId);
error DisputeManagerMustAcceptRelatedDispute(bytes32 disputeId, bytes32 relatedDisputeId);
error DisputeManagerIndexerNotFound(address allocationId);
error DisputeManagerNonMatchingSubgraphDeployment(bytes32 subgraphDeploymentId1, bytes32 subgraphDeploymentId2);
Expand All @@ -180,6 +181,7 @@ interface IDisputeManager {
bytes32 responseCID2,
bytes32 subgraphDeploymentId2
);
error DisputeManagerSubgraphServiceNotSet();

function setDisputePeriod(uint64 disputePeriod) external;

Expand All @@ -202,7 +204,7 @@ interface IDisputeManager {

function createIndexingDispute(address allocationId, bytes32 poi) external returns (bytes32);

function acceptDispute(bytes32 disputeId, uint256 tokensSlash) external;
function acceptDispute(bytes32 disputeId, uint256 tokensSlash, bool acceptDisputeInConflict) external;

function rejectDispute(bytes32 disputeId) external;

Expand Down
309 changes: 242 additions & 67 deletions packages/subgraph-service/test/disputeManager/DisputeManager.t.sol
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions packages/subgraph-service/test/disputeManager/disputes/disputes.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ contract DisputeManagerDisputeTest is DisputeManagerTest {
IDisputeManager.DisputeManagerInvalidDispute.selector,
disputeID
));
disputeManager.acceptDispute(disputeID, tokensSlash);
disputeManager.acceptDispute(disputeID, tokensSlash, false);
}

function test_Dispute_Accept_RevertIf_SlashZeroTokens(
Expand All @@ -40,7 +40,7 @@ contract DisputeManagerDisputeTest is DisputeManagerTest {
resetPrank(users.arbitrator);
uint256 maxTokensToSlash = uint256(maxSlashingPercentage).mulPPM(tokens);
vm.expectRevert(abi.encodeWithSelector(IDisputeManager.DisputeManagerInvalidTokensSlash.selector, 0, maxTokensToSlash));
disputeManager.acceptDispute(disputeID, 0);
disputeManager.acceptDispute(disputeID, 0, false);
}

function test_Dispute_Reject_RevertIf_DisputeDoesNotExist(
Expand Down
36 changes: 33 additions & 3 deletions packages/subgraph-service/test/disputeManager/disputes/indexing/accept.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,37 @@ contract DisputeManagerIndexingAcceptDisputeTest is DisputeManagerTest {
bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));

resetPrank(users.arbitrator);
_acceptDispute(disputeID, tokensSlash);
_acceptDispute(disputeID, tokensSlash, false);
}

function test_Indexing_Accept_Dispute_RevertWhen_SubgraphServiceNotSet(
uint256 tokens,
uint256 tokensSlash
) public useIndexer useAllocation(tokens) {
tokensSlash = bound(tokensSlash, 1, uint256(maxSlashingPercentage).mulPPM(tokens));

resetPrank(users.fisherman);
bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));

resetPrank(users.arbitrator);
// clear subgraph service address from storage
_setStorage_SubgraphService(address(0));

vm.expectRevert(abi.encodeWithSelector(IDisputeManager.DisputeManagerSubgraphServiceNotSet.selector));
disputeManager.acceptDispute(disputeID, tokensSlash, false);
}

function test_Indexing_Accept_Dispute_OptParam(
uint256 tokens,
uint256 tokensSlash
) public useIndexer useAllocation(tokens) {
tokensSlash = bound(tokensSlash, 1, uint256(maxSlashingPercentage).mulPPM(tokens));

resetPrank(users.fisherman);
bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));

resetPrank(users.arbitrator);
_acceptDispute(disputeID, tokensSlash, true);
}

function test_Indexing_Accept_RevertIf_CallerIsNotArbitrator(
Expand All @@ -39,7 +69,7 @@ contract DisputeManagerIndexingAcceptDisputeTest is DisputeManagerTest {
// attempt to accept dispute as fisherman
resetPrank(users.fisherman);
vm.expectRevert(abi.encodeWithSelector(IDisputeManager.DisputeManagerNotArbitrator.selector));
disputeManager.acceptDispute(disputeID, tokensSlash);
disputeManager.acceptDispute(disputeID, tokensSlash, false);
}

function test_Indexing_Accept_RevertWhen_SlashingOverMaxSlashPercentage(
Expand All @@ -59,6 +89,6 @@ contract DisputeManagerIndexingAcceptDisputeTest is DisputeManagerTest {
maxTokensToSlash
);
vm.expectRevert(expectedError);
disputeManager.acceptDispute(disputeID, tokensSlash);
disputeManager.acceptDispute(disputeID, tokensSlash, false);
}
}
35 changes: 24 additions & 11 deletions packages/subgraph-service/test/disputeManager/disputes/indexing/create.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,28 @@ import { IDisputeManager } from "../../../../contracts/interfaces/IDisputeManage
import { DisputeManagerTest } from "../../DisputeManager.t.sol";

contract DisputeManagerIndexingCreateDisputeTest is DisputeManagerTest {

/*
* TESTS
*/

function test_Indexing_Create_Dispute(
function test_Indexing_Create_Dispute(uint256 tokens) public useIndexer useAllocation(tokens) {
resetPrank(users.fisherman);
_createIndexingDispute(allocationID, bytes32("POI1"));
}

function test_Indexing_Create_Dispute_RevertWhen_SubgraphServiceNotSet(
uint256 tokens
) public useIndexer useAllocation(tokens) {
resetPrank(users.fisherman);
_createIndexingDispute(allocationID, bytes32("POI1"));

// clear subgraph service address from storage
_setStorage_SubgraphService(address(0));

// // Approve the dispute deposit
token.approve(address(disputeManager), disputeDeposit);

vm.expectRevert(abi.encodeWithSelector(IDisputeManager.DisputeManagerSubgraphServiceNotSet.selector));
disputeManager.createIndexingDispute(allocationID, bytes32("POI2"));
}

function test_Indexing_Create_MultipleDisputes() public {
Expand All @@ -33,7 +45,12 @@ contract DisputeManagerIndexingCreateDisputeTest is DisputeManagerTest {
_createProvision(indexer, tokens, maxSlashingPercentage, disputePeriod);
_register(indexer, abi.encode("url", "geoHash", address(0)));
uint256 allocationIDPrivateKey = uint256(keccak256(abi.encodePacked(i)));
bytes memory data = _createSubgraphAllocationData(indexer, subgraphDeployment, allocationIDPrivateKey, tokens);
bytes memory data = _createSubgraphAllocationData(
indexer,
subgraphDeployment,
allocationIDPrivateKey,
tokens
);
_startService(indexer, data);
allocationIDPrivateKeys[i] = allocationIDPrivateKey;
}
Expand All @@ -48,7 +65,7 @@ contract DisputeManagerIndexingCreateDisputeTest is DisputeManagerTest {
uint256 tokens
) public useIndexer useAllocation(tokens) {
resetPrank(users.fisherman);
bytes32 disputeID =_createIndexingDispute(allocationID, bytes32("POI1"));
bytes32 disputeID = _createIndexingDispute(allocationID, bytes32("POI1"));

// Create another dispute with different fisherman
address otherFisherman = makeAddr("otherFisherman");
Expand Down Expand Up @@ -78,9 +95,7 @@ contract DisputeManagerIndexingCreateDisputeTest is DisputeManagerTest {
vm.stopPrank();
}

function test_Indexing_Create_RevertIf_AllocationDoesNotExist(
uint256 tokens
) public useFisherman {
function test_Indexing_Create_RevertIf_AllocationDoesNotExist(uint256 tokens) public useFisherman {
tokens = bound(tokens, disputeDeposit, 10_000_000_000 ether);
token.approve(address(disputeManager), tokens);
bytes memory expectedError = abi.encodeWithSelector(
Expand All @@ -92,9 +107,7 @@ contract DisputeManagerIndexingCreateDisputeTest is DisputeManagerTest {
vm.stopPrank();
}

function test_Indexing_Create_RevertIf_IndexerIsBelowStake(
uint256 tokens
) public useIndexer useAllocation(tokens) {
function test_Indexing_Create_RevertIf_IndexerIsBelowStake(uint256 tokens) public useIndexer useAllocation(tokens) {
// Close allocation
bytes memory data = abi.encode(allocationID);
_stopService(users.indexer, data);
Expand Down
39 changes: 36 additions & 3 deletions packages/subgraph-service/test/disputeManager/disputes/query/accept.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,40 @@ contract DisputeManagerQueryAcceptDisputeTest is DisputeManagerTest {
bytes32 disputeID = _createQueryDispute(attestationData);

resetPrank(users.arbitrator);
_acceptDispute(disputeID, tokensSlash);
_acceptDispute(disputeID, tokensSlash, false);
}

function test_Query_Accept_Dispute_RevertWhen_SubgraphServiceNotSet(
uint256 tokens,
uint256 tokensSlash
) public useIndexer useAllocation(tokens) {
tokensSlash = bound(tokensSlash, 1, uint256(maxSlashingPercentage).mulPPM(tokens));

resetPrank(users.fisherman);
Attestation.Receipt memory receipt = _createAttestationReceipt(requestCID, responseCID, subgraphDeploymentId);
bytes memory attestationData = _createAtestationData(receipt, allocationIDPrivateKey);
bytes32 disputeID = _createQueryDispute(attestationData);

resetPrank(users.arbitrator);
// clear subgraph service address from storage
_setStorage_SubgraphService(address(0));
vm.expectRevert(abi.encodeWithSelector(IDisputeManager.DisputeManagerSubgraphServiceNotSet.selector));
disputeManager.acceptDispute(disputeID, tokensSlash, false);
}

function test_Query_Accept_Dispute_OptParam(
uint256 tokens,
uint256 tokensSlash
) public useIndexer useAllocation(tokens) {
tokensSlash = bound(tokensSlash, 1, uint256(maxSlashingPercentage).mulPPM(tokens));

resetPrank(users.fisherman);
Attestation.Receipt memory receipt = _createAttestationReceipt(requestCID, responseCID, subgraphDeploymentId);
bytes memory attestationData = _createAtestationData(receipt, allocationIDPrivateKey);
bytes32 disputeID = _createQueryDispute(attestationData);

resetPrank(users.arbitrator);
_acceptDispute(disputeID, tokensSlash, true);
}

function test_Query_Accept_RevertIf_CallerIsNotArbitrator(
Expand All @@ -47,7 +80,7 @@ contract DisputeManagerQueryAcceptDisputeTest is DisputeManagerTest {

// attempt to accept dispute as fisherman
vm.expectRevert(abi.encodeWithSelector(IDisputeManager.DisputeManagerNotArbitrator.selector));
disputeManager.acceptDispute(disputeID, tokensSlash);
disputeManager.acceptDispute(disputeID, tokensSlash, false);
}

function test_Query_Accept_RevertWhen_SlashingOverMaxSlashPercentage(
Expand All @@ -70,6 +103,6 @@ contract DisputeManagerQueryAcceptDisputeTest is DisputeManagerTest {
maxTokensToSlash
);
vm.expectRevert(expectedError);
disputeManager.acceptDispute(disputeID, tokensSlash);
disputeManager.acceptDispute(disputeID, tokensSlash, false);
}
}
17 changes: 16 additions & 1 deletion packages/subgraph-service/test/disputeManager/disputes/query/create.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,28 @@ contract DisputeManagerQueryCreateDisputeTest is DisputeManagerTest {
* TESTS
*/

function test_Query_Create_Dispute(uint256 tokens) public useIndexer useAllocation(tokens) {
function test_Query_Create_Dispute_Only(uint256 tokens) public useIndexer useAllocation(tokens) {
resetPrank(users.fisherman);
Attestation.Receipt memory receipt = _createAttestationReceipt(requestCID, responseCID, subgraphDeploymentId);
bytes memory attestationData = _createAtestationData(receipt, allocationIDPrivateKey);
_createQueryDispute(attestationData);
}

function test_Query_Create_Dispute_RevertWhen_SubgraphServiceNotSet(uint256 tokens) public useIndexer useAllocation(tokens) {
resetPrank(users.fisherman);
Attestation.Receipt memory receipt = _createAttestationReceipt(requestCID, responseCID, subgraphDeploymentId);
bytes memory attestationData = _createAtestationData(receipt, allocationIDPrivateKey);

// clear subgraph service address from storage
_setStorage_SubgraphService(address(0));

// // Approve the dispute deposit
token.approve(address(disputeManager), disputeDeposit);

vm.expectRevert(abi.encodeWithSelector(IDisputeManager.DisputeManagerSubgraphServiceNotSet.selector));
disputeManager.createQueryDispute(attestationData);
}

function test_Query_Create_MultipleDisputes_DifferentFisherman(
uint256 tokens
) public useIndexer useAllocation(tokens) {
Expand Down
54 changes: 46 additions & 8 deletions packages/subgraph-service/test/disputeManager/disputes/queryConflict/accept.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ contract DisputeManagerQueryConflictAcceptDisputeTest is DisputeManagerTest {
* TESTS
*/

function test_Query_Conflict_Accept_Dispute(
function test_Query_Conflict_Accept_Dispute_Draw_Other(
uint256 tokens,
uint256 tokensSlash
) public useIndexer useAllocation(tokens) {
Expand All @@ -33,11 +33,49 @@ contract DisputeManagerQueryConflictAcceptDisputeTest is DisputeManagerTest {
allocationIDPrivateKey
);

uint256 fishermanBalanceBefore = token.balanceOf(users.fisherman);

resetPrank(users.fisherman);
(bytes32 disputeID1,) = _createQueryDisputeConflict(attestationData1, attestationData2);
(bytes32 disputeID1, ) = _createQueryDisputeConflict(attestationData1, attestationData2);

resetPrank(users.arbitrator);
_acceptDispute(disputeID1, tokensSlash);
_acceptDispute(disputeID1, tokensSlash, false);

uint256 fishermanRewardPercentage = disputeManager.fishermanRewardCut();
uint256 fishermanReward = tokensSlash.mulPPM(fishermanRewardPercentage);
uint256 fishermanBalanceAfter = token.balanceOf(users.fisherman);

assertEq(fishermanBalanceAfter, fishermanBalanceBefore + fishermanReward);
}

function test_Query_Conflict_Accept_Dispute_Accept_Other(
uint256 tokens,
uint256 tokensSlash
) public useIndexer useAllocation(tokens) {
tokensSlash = bound(tokensSlash, 1, uint256(maxSlashingPercentage).mulPPM(tokens));

(bytes memory attestationData1, bytes memory attestationData2) = _createConflictingAttestations(
requestCID,
subgraphDeployment,
responseCID1,
responseCID2,
allocationIDPrivateKey,
allocationIDPrivateKey
);

uint256 fishermanBalanceBefore = token.balanceOf(users.fisherman);

resetPrank(users.fisherman);
(bytes32 disputeID1, ) = _createQueryDisputeConflict(attestationData1, attestationData2);

resetPrank(users.arbitrator);
_acceptDispute(disputeID1, tokensSlash, true);

uint256 fishermanRewardPercentage = disputeManager.fishermanRewardCut();
uint256 fishermanReward = tokensSlash.mulPPM(fishermanRewardPercentage);
uint256 fishermanBalanceAfter = token.balanceOf(users.fisherman);

assertEq(fishermanBalanceAfter, fishermanBalanceBefore + fishermanReward * 2);
}

function test_Query_Conflict_Accept_RevertIf_CallerIsNotArbitrator(
Expand All @@ -56,12 +94,12 @@ contract DisputeManagerQueryConflictAcceptDisputeTest is DisputeManagerTest {
);

resetPrank(users.fisherman);
(bytes32 disputeID1,) = _createQueryDisputeConflict(attestationData1, attestationData2);
(bytes32 disputeID1, ) = _createQueryDisputeConflict(attestationData1, attestationData2);

// attempt to accept dispute as fisherman
resetPrank(users.fisherman);
vm.expectRevert(abi.encodeWithSelector(IDisputeManager.DisputeManagerNotArbitrator.selector));
disputeManager.acceptDispute(disputeID1, tokensSlash);
disputeManager.acceptDispute(disputeID1, tokensSlash, false);
}

function test_Query_Conflict_Accept_RevertWhen_SlashingOverMaxSlashPercentage(
Expand All @@ -80,17 +118,17 @@ contract DisputeManagerQueryConflictAcceptDisputeTest is DisputeManagerTest {
);

resetPrank(users.fisherman);
(bytes32 disputeID1,) = _createQueryDisputeConflict(attestationData1, attestationData2);
(bytes32 disputeID1, ) = _createQueryDisputeConflict(attestationData1, attestationData2);

// max slashing percentage is 50%
resetPrank(users.arbitrator);
uint256 maxTokensToSlash = uint256(maxSlashingPercentage).mulPPM(tokens);
bytes memory expectedError = abi.encodeWithSelector(
IDisputeManager.DisputeManagerInvalidTokensSlash.selector,
IDisputeManager.DisputeManagerInvalidTokensSlash.selector,
tokensSlash,
maxTokensToSlash
);
vm.expectRevert(expectedError);
disputeManager.acceptDispute(disputeID1, tokensSlash);
disputeManager.acceptDispute(disputeID1, tokensSlash, false);
}
}
13 changes: 3 additions & 10 deletions packages/subgraph-service/test/disputeManager/disputes/queryConflict/reject.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,11 @@ import { IDisputeManager } from "../../../../contracts/interfaces/IDisputeManage
import { DisputeManagerTest } from "../../DisputeManager.t.sol";

contract DisputeManagerQueryConflictRejectDisputeTest is DisputeManagerTest {

/*
* TESTS
*/

function test_Query_Conflict_Reject_Revert(
uint256 tokens
) public useIndexer useAllocation(tokens) {
function test_Query_Conflict_Reject_Revert(uint256 tokens) public useIndexer useAllocation(tokens) {
bytes32 requestCID = keccak256(abi.encodePacked("Request CID"));
bytes32 responseCID1 = keccak256(abi.encodePacked("Response CID 1"));
bytes32 responseCID2 = keccak256(abi.encodePacked("Response CID 2"));
Expand All @@ -29,14 +26,10 @@ contract DisputeManagerQueryConflictRejectDisputeTest is DisputeManagerTest {
);

resetPrank(users.fisherman);
(bytes32 disputeID1, bytes32 disputeID2) = _createQueryDisputeConflict(attestationData1, attestationData2);
(bytes32 disputeID1, ) = _createQueryDisputeConflict(attestationData1, attestationData2);

resetPrank(users.arbitrator);
vm.expectRevert(abi.encodeWithSelector(
IDisputeManager.DisputeManagerMustAcceptRelatedDispute.selector,
disputeID1,
disputeID2
));
vm.expectRevert(abi.encodeWithSelector(IDisputeManager.DisputeManagerDisputeInConflict.selector, disputeID1));
disputeManager.rejectDispute(disputeID1);
}
}
Loading
Loading