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 4 commits into
base: horizon
Choose a base branch
from
+751 −211
Open

TRST audit fixes for DisputeManager contract #1075

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 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
25d22e4
fix: added new function for accept dispute in conflict that includes …
Maikol Dec 13, 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
185 changes: 84 additions & 101 deletions packages/subgraph-service/contracts/DisputeManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ contract DisputeManager is
// Maximum value for fisherman reward cut in PPM
uint32 public constant MAX_FISHERMAN_REWARD_CUT = 500000;

// Minimum value for dispute deposit
uint256 public constant MIN_DISPUTE_DEPOSIT = 1e18; // 1 GRT

// -- Modifiers --

/**
Expand Down Expand Up @@ -140,7 +143,7 @@ contract DisputeManager is
*/
function createIndexingDispute(address allocationId, bytes32 poi) external override returns (bytes32) {
// Get funds from fisherman
_pullFishermanDeposit();
_graphToken().pullTokens(msg.sender, disputeDeposit);

// Create a dispute
return _createIndexingDisputeWithAllocation(msg.sender, disputeDeposit, allocationId, poi);
Expand All @@ -158,7 +161,7 @@ contract DisputeManager is
*/
function createQueryDispute(bytes calldata attestationData) external override returns (bytes32) {
// Get funds from fisherman
_pullFishermanDeposit();
_graphToken().pullTokens(msg.sender, disputeDeposit);

// Create a dispute
return
Expand All @@ -174,10 +177,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.
* Two linked disputes will be created and if the arbitrator resolve one, the other
* one will be automatically resolved.
* one will be automatically resolved. Note that:
* - it's not possible to reject a conflicting query dispute as by definition at least one
* of the attestations is incorrect.
* - if both attestations are proven to be incorrect, the arbitrator can slash the indexer twice.
* 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
Expand Down Expand Up @@ -205,10 +212,23 @@ contract DisputeManager is
)
);

// Get funds from fisherman
_graphToken().pullTokens(msg.sender, disputeDeposit);

// 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;
Expand All @@ -228,54 +248,59 @@ contract DisputeManager is
* @dev Accept a dispute with Id `disputeId`
* @param disputeId Id of the dispute to be accepted
* @param tokensSlash Amount of tokens to slash from the indexer
* @param acceptDisputeInConflict If the dispute is in conflict, accept the conflicting dispute. Otherwise
* it will be drawn automatically. Ignored if the dispute is not in conflict.
*/
function acceptDispute(
bytes32 disputeId,
uint256 tokensSlash
uint256 tokensSlash,
bool acceptDisputeInConflict
) external override onlyArbitrator onlyPendingDispute(disputeId) {
Dispute storage dispute = disputes[disputeId];

// store the dispute status
dispute.status = IDisputeManager.DisputeStatus.Accepted;

// Slash
uint256 tokensToReward = _slashIndexer(dispute.indexer, tokensSlash, dispute.stakeSnapshot);

// Give the fisherman their reward and their deposit back
_graphToken().pushTokens(dispute.fisherman, tokensToReward + dispute.deposit);
_acceptDispute(disputeId, dispute, tokensSlash);

if (_isDisputeInConflict(dispute)) {
rejectDispute(dispute.relatedDisputeId);
Dispute storage relatedDispute = disputes[dispute.relatedDisputeId];
if (acceptDisputeInConflict) {
_acceptDispute(dispute.relatedDisputeId, relatedDispute, tokensSlash);
} else {
_drawDispute(dispute.relatedDisputeId, relatedDispute);
}
}
}

emit DisputeAccepted(disputeId, dispute.indexer, dispute.fisherman, dispute.deposit + tokensToReward);
/**
* @notice The arbitrator rejects a dispute as being invalid.
* Note that conflicting query disputes cannot be rejected.
* @dev Reject a dispute with Id `disputeId`
* @param disputeId Id of the dispute to be rejected
*/
function rejectDispute(bytes32 disputeId) external override onlyArbitrator onlyPendingDispute(disputeId) {
Dispute storage dispute = disputes[disputeId];
require(!_isDisputeInConflict(dispute), DisputeManagerDisputeInConflict(disputeId));
_rejectDispute(disputeId, dispute);
}

/**
* @notice The arbitrator draws dispute.
* Note that drawing a conflicting query dispute should not be possible however it is allowed
* to give arbitrators greater flexibility when resolving disputes.
* @dev Ignore a dispute with Id `disputeId`
* @param disputeId Id of the dispute to be disregarded
*/
function drawDispute(bytes32 disputeId) external override onlyArbitrator onlyPendingDispute(disputeId) {
Dispute storage dispute = disputes[disputeId];
_drawDispute(disputeId, dispute);

// Return deposit to the fisherman if any
if (dispute.deposit > 0) {
_graphToken().pushTokens(dispute.fisherman, dispute.deposit);
if (_isDisputeInConflict(dispute)) {
_drawDispute(dispute.relatedDisputeId, disputes[dispute.relatedDisputeId]);
}

// resolve related dispute if any
_drawDisputeInConflict(dispute);

// store dispute status
dispute.status = IDisputeManager.DisputeStatus.Drawn;

emit DisputeDrawn(disputeId, dispute.indexer, dispute.fisherman, dispute.deposit);
}

/**
* @notice Once the dispute period ends, if the dispute status remains Pending,
* the fisherman can cancel the dispute and get back their initial deposit.
* Note that cancelling a conflicting query dispute will also cancel the related dispute.
* @dev Cancel a dispute with Id `disputeId`
* @param disputeId Id of the dispute to be cancelled
*/
Expand All @@ -284,19 +309,11 @@ contract DisputeManager is

// Check if dispute period has finished
require(dispute.createdAt + disputePeriod < block.timestamp, DisputeManagerDisputePeriodNotFinished());
_cancelDispute(disputeId, dispute);

// Return deposit to the fisherman if any
if (dispute.deposit > 0) {
_graphToken().pushTokens(dispute.fisherman, dispute.deposit);
if (_isDisputeInConflict(dispute)) {
_cancelDispute(dispute.relatedDisputeId, disputes[dispute.relatedDisputeId]);
}

// resolve related dispute if any
_cancelDisputeInConflict(dispute);

// store dispute status
dispute.status = IDisputeManager.DisputeStatus.Cancelled;

emit DisputeCancelled(disputeId, dispute.indexer, dispute.fisherman, dispute.deposit);
}

/**
Expand Down Expand Up @@ -401,29 +418,6 @@ contract DisputeManager is
return Attestation.areConflicting(attestation1, attestation2);
}

/**
* @notice The arbitrator rejects a dispute as being invalid.
* @dev Reject a dispute with Id `disputeId`
* @param disputeId Id of the dispute to be rejected
*/
function rejectDispute(bytes32 disputeId) public override onlyArbitrator onlyPendingDispute(disputeId) {
Dispute storage dispute = disputes[disputeId];

// store dispute status
dispute.status = IDisputeManager.DisputeStatus.Rejected;

// For conflicting disputes, the related dispute must be accepted
require(
!_isDisputeInConflict(dispute),
DisputeManagerMustAcceptRelatedDispute(disputeId, dispute.relatedDisputeId)
);

// Burn the fisherman's deposit
_graphToken().burnTokens(dispute.deposit);

emit DisputeRejected(disputeId, dispute.indexer, dispute.fisherman, dispute.deposit);
}

/**
* @notice Returns the indexer that signed an attestation.
* @param attestation Attestation
Expand Down Expand Up @@ -561,42 +555,33 @@ contract DisputeManager is
return disputeId;
}

/**
* @notice Draw the conflicting dispute if there is any for the one passed to this function.
* @param _dispute Dispute
* @return True if resolved
*/
function _drawDisputeInConflict(Dispute memory _dispute) private returns (bool) {
if (_isDisputeInConflict(_dispute)) {
bytes32 relatedDisputeId = _dispute.relatedDisputeId;
Dispute storage relatedDispute = disputes[relatedDisputeId];
relatedDispute.status = IDisputeManager.DisputeStatus.Drawn;
return true;
}
return false;
function _acceptDispute(bytes32 _disputeId, Dispute storage _dispute, uint256 _tokensSlashed) private {
uint256 tokensToReward = _slashIndexer(_dispute.indexer, _tokensSlashed, _dispute.stakeSnapshot);
_dispute.status = IDisputeManager.DisputeStatus.Accepted;
_graphToken().pushTokens(_dispute.fisherman, tokensToReward + _dispute.deposit);

emit DisputeAccepted(_disputeId, _dispute.indexer, _dispute.fisherman, _dispute.deposit + tokensToReward);
}

/**
* @notice Cancel the conflicting dispute if there is any for the one passed to this function.
* @param _dispute Dispute
* @return True if cancelled
*/
function _cancelDisputeInConflict(Dispute memory _dispute) private returns (bool) {
if (_isDisputeInConflict(_dispute)) {
bytes32 relatedDisputeId = _dispute.relatedDisputeId;
Dispute storage relatedDispute = disputes[relatedDisputeId];
relatedDispute.status = IDisputeManager.DisputeStatus.Cancelled;
return true;
}
return false;
function _rejectDispute(bytes32 _disputeId, Dispute storage _dispute) private {
_dispute.status = IDisputeManager.DisputeStatus.Rejected;
_graphToken().burnTokens(_dispute.deposit);

emit DisputeRejected(_disputeId, _dispute.indexer, _dispute.fisherman, _dispute.deposit);
}

/**
* @notice Pull `disputeDeposit` from fisherman account.
*/
function _pullFishermanDeposit() private {
// Transfer tokens to deposit from fisherman to this contract
_graphToken().pullTokens(msg.sender, disputeDeposit);
function _drawDispute(bytes32 _disputeId, Dispute storage _dispute) private {
_dispute.status = IDisputeManager.DisputeStatus.Drawn;
_graphToken().pushTokens(_dispute.fisherman, _dispute.deposit);

emit DisputeDrawn(_disputeId, _dispute.indexer, _dispute.fisherman, _dispute.deposit);
}

function _cancelDispute(bytes32 _disputeId, Dispute storage _dispute) private {
_dispute.status = IDisputeManager.DisputeStatus.Cancelled;
_graphToken().pushTokens(_dispute.fisherman, _dispute.deposit);

emit DisputeCancelled(_disputeId, _dispute.indexer, _dispute.fisherman, _dispute.deposit);
}

/**
Expand Down Expand Up @@ -658,7 +643,7 @@ contract DisputeManager is
* @param _disputeDeposit The dispute deposit in Graph Tokens
*/
function _setDisputeDeposit(uint256 _disputeDeposit) private {
require(_disputeDeposit != 0, DisputeManagerInvalidDisputeDeposit(_disputeDeposit));
require(_disputeDeposit >= MIN_DISPUTE_DEPOSIT, DisputeManagerInvalidDisputeDeposit(_disputeDeposit));
disputeDeposit = _disputeDeposit;
emit DisputeDepositSet(_disputeDeposit);
}
Expand Down Expand Up @@ -704,10 +689,8 @@ contract DisputeManager is
* @param _dispute Dispute
* @return True conflicting attestation dispute
*/
function _isDisputeInConflict(Dispute memory _dispute) private view returns (bool) {
bytes32 relatedId = _dispute.relatedDisputeId;
// this is so the check returns false when rejecting the related dispute.
return relatedId != 0 && disputes[relatedId].status == IDisputeManager.DisputeStatus.Pending;
function _isDisputeInConflict(Dispute storage _dispute) private view returns (bool) {
return _dispute.relatedDisputeId != bytes32(0);
}

/**
Expand Down
3 changes: 2 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 Down Expand Up @@ -202,7 +203,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
Loading
Loading