Skip to content

Complex DK jump sequences bug fix and extended testing #2159

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

Merged
merged 5 commits into from
Oct 14, 2025
+399 −82
Merged

Complex DK jump sequences bug fix and extended testing #2159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
00be694
fix(DK): allow recurring DK
unknownunknown1 Oct 6, 2025
536e61c
test: withdrawFeesAndRewards() coverage for complex DK jump sequence
jaybuidl Oct 8, 2025
374b4f2
refactor: active/jump checks, no more _round param in withdrawFeesAnd…
jaybuidl Oct 13, 2025
f9fd944
fix: removed unnecessary isActive modifier on the shutter DKs
jaybuidl Oct 14, 2025
a0a3a67
Merge pull request #2174 from kleros/refactor/active-dk
jaybuidl Oct 14, 2025
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
110 changes: 58 additions & 52 deletions contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
struct Dispute {
Round[] rounds; // Rounds of the dispute. 0 is the default round, and [1, ..n] are the appeal rounds.
uint256 numberOfChoices; // The number of choices jurors have when voting. This does not include choice `0` which is reserved for "refuse to arbitrate".
bool jumped; // True if dispute jumped to a parent dispute kit and won't be handled by this DK anymore.
mapping(uint256 => uint256) coreRoundIDToLocal; // Maps id of the round in the core contract to the index of the round of related local dispute.
bytes extraData; // Extradata for the dispute.
uint256[10] __gap; // Reserved slots for future upgrades.
Expand Down Expand Up @@ -54,6 +53,11 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
uint256[10] __gap; // Reserved slots for future upgrades.
}

struct Active {
bool dispute; // True if at least one round in the dispute has been active on this Dispute Kit. False if the dispute is unknown to this Dispute Kit.
bool currentRound; // True if the dispute's current round is active on this Dispute Kit. False if the dispute has jumped to another Dispute Kit.
}

// ************************************* //
// * Storage * //
// ************************************* //
Expand All @@ -67,7 +71,7 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
Dispute[] public disputes; // Array of the locally created disputes.
mapping(uint256 => uint256) public coreDisputeIDToLocal; // Maps the dispute ID in Kleros Core to the local dispute ID.
bool public singleDrawPerJuror; // Whether each juror can only draw once per dispute, false by default.
mapping(uint256 coreDisputeID => bool) public coreDisputeIDToActive; // True if this dispute kit is active for this core dispute ID.
mapping(uint256 coreDisputeID => Active) public coreDisputeIDToActive; // Active status of the dispute and the current round.
address public wNative; // The wrapped native token for safeSend().
uint256 public jumpDisputeKitID; // The ID of the dispute kit in Kleros Core disputeKits array that the dispute should switch to after the court jump, in case the new court doesn't support this dispute kit.

Expand Down Expand Up @@ -106,17 +110,10 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi

/// @notice To be emitted when the contributed funds are withdrawn.
/// @param _coreDisputeID The identifier of the dispute in the Arbitrator contract.
/// @param _coreRoundID The identifier of the round in the Arbitrator contract.
/// @param _choice The choice that is being funded.
/// @param _contributor The address of the contributor.
/// @param _amount The amount withdrawn.
event Withdrawal(
uint256 indexed _coreDisputeID,
uint256 indexed _coreRoundID,
uint256 _choice,
address indexed _contributor,
uint256 _amount
);
event Withdrawal(uint256 indexed _coreDisputeID, uint256 _choice, address indexed _contributor, uint256 _amount);

/// @notice To be emitted when a choice is fully funded for an appeal.
/// @param _coreDisputeID The identifier of the dispute in the Arbitrator contract.
Expand All @@ -138,8 +135,9 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
_;
}

modifier notJumped(uint256 _coreDisputeID) {
if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK();
modifier isActive(uint256 _coreDisputeID) {
if (!coreDisputeIDToActive[_coreDisputeID].dispute) revert DisputeUnknownInThisDisputeKit();
if (!coreDisputeIDToActive[_coreDisputeID].currentRound) revert DisputeJumpedToAnotherDisputeKit();
_;
}

Expand Down Expand Up @@ -206,28 +204,37 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
bytes calldata _extraData,
uint256 /*_nbVotes*/
) external override onlyByCore {
uint256 localDisputeID = disputes.length;
Dispute storage dispute = disputes.push();
uint256 localDisputeID;
Dispute storage dispute;
Active storage active = coreDisputeIDToActive[_coreDisputeID];
if (active.dispute) {
// The dispute has already been created in this DK in a previous round. E.g. if DK1 jumps to DK2 and then back to DK1.
localDisputeID = coreDisputeIDToLocal[_coreDisputeID];
dispute = disputes[localDisputeID];
} else {
// The dispute has not been created in this DK yet.
localDisputeID = disputes.length;
dispute = disputes.push();
coreDisputeIDToLocal[_coreDisputeID] = localDisputeID;
}

active.dispute = true;
active.currentRound = true;
dispute.numberOfChoices = _numberOfChoices;
dispute.extraData = _extraData;
dispute.jumped = false; // Possibly true if this DK has jumped in a previous round.

// New round in the Core should be created before the dispute creation in DK.
// KlerosCore.Round must have been already created.
dispute.coreRoundIDToLocal[core.getNumberOfRounds(_coreDisputeID) - 1] = dispute.rounds.length;
dispute.rounds.push().tied = true;

Round storage round = dispute.rounds.push();
round.tied = true;

coreDisputeIDToLocal[_coreDisputeID] = localDisputeID;
coreDisputeIDToActive[_coreDisputeID] = true;
emit DisputeCreation(_coreDisputeID, _numberOfChoices, _extraData);
}

/// @inheritdoc IDisputeKit
function draw(
uint256 _coreDisputeID,
uint256 _nonce
) external override onlyByCore notJumped(_coreDisputeID) returns (address drawnAddress, uint96 fromSubcourtID) {
) external override onlyByCore isActive(_coreDisputeID) returns (address drawnAddress, uint96 fromSubcourtID) {
uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID];
Dispute storage dispute = disputes[localDisputeID];
uint256 localRoundID = dispute.rounds.length - 1;
Expand Down Expand Up @@ -266,11 +273,10 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
uint256 _coreDisputeID,
uint256[] calldata _voteIDs,
bytes32 _commit
) internal notJumped(_coreDisputeID) {
) internal isActive(_coreDisputeID) {
(, , KlerosCore.Period period, , ) = core.disputes(_coreDisputeID);
if (period != KlerosCore.Period.commit) revert NotCommitPeriod();
if (_commit == bytes32(0)) revert EmptyCommit();
if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID();

Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]];
Round storage round = dispute.rounds[dispute.rounds.length - 1];
Expand Down Expand Up @@ -313,11 +319,10 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
uint256 _salt,
string memory _justification,
address _juror
) internal notJumped(_coreDisputeID) {
) internal isActive(_coreDisputeID) {
(, , KlerosCore.Period period, , ) = core.disputes(_coreDisputeID);
if (period != KlerosCore.Period.vote) revert NotVotePeriod();
if (_voteIDs.length == 0) revert EmptyVoteIDs();
if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID();

uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID];
Dispute storage dispute = disputes[localDisputeID];
Expand Down Expand Up @@ -364,10 +369,9 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
/// Note that the surplus deposit will be reimbursed.
/// @param _coreDisputeID Index of the dispute in Kleros Core.
/// @param _choice A choice that receives funding.
function fundAppeal(uint256 _coreDisputeID, uint256 _choice) external payable notJumped(_coreDisputeID) {
function fundAppeal(uint256 _coreDisputeID, uint256 _choice) external payable isActive(_coreDisputeID) {
Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]];
if (_choice > dispute.numberOfChoices) revert ChoiceOutOfBounds();
if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID();

(uint256 appealPeriodStart, uint256 appealPeriodEnd) = core.appealPeriod(_coreDisputeID);
if (block.timestamp < appealPeriodStart || block.timestamp >= appealPeriodEnd) revert NotAppealPeriod();
Expand Down Expand Up @@ -417,7 +421,7 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi

if (core.isDisputeKitJumping(_coreDisputeID)) {
// Don't create a new round in case of a jump, and remove local dispute from the flow.
dispute.jumped = true;
coreDisputeIDToActive[_coreDisputeID].currentRound = false;
} else {
// Don't subtract 1 from length since both round arrays haven't been updated yet.
dispute.coreRoundIDToLocal[coreRoundID + 1] = dispute.rounds.length;
Expand All @@ -433,48 +437,50 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi

/// @notice Allows those contributors who attempted to fund an appeal round to withdraw any reimbursable fees or rewards after the dispute gets resolved.
/// @dev Withdrawals are not possible if the core contract is paused.
/// @dev It can be called after the dispute has jumped to another dispute kit.
/// @param _coreDisputeID Index of the dispute in Kleros Core contract.
/// @param _beneficiary The address whose rewards to withdraw.
/// @param _coreRoundID The round in the Kleros Core contract the caller wants to withdraw from.
/// @param _choice The ruling option that the caller wants to withdraw from.
/// @return amount The withdrawn amount.
function withdrawFeesAndRewards(
uint256 _coreDisputeID,
address payable _beneficiary,
uint256 _coreRoundID,
uint256 _choice
) external returns (uint256 amount) {
(, , , bool isRuled, ) = core.disputes(_coreDisputeID);
if (!isRuled) revert DisputeNotResolved();
if (core.paused()) revert CoreIsPaused();
if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID();
if (!coreDisputeIDToActive[_coreDisputeID].dispute) revert DisputeUnknownInThisDisputeKit();

Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]];
Round storage round = dispute.rounds[dispute.coreRoundIDToLocal[_coreRoundID]];
(uint256 finalRuling, , ) = core.currentRuling(_coreDisputeID);

if (!round.hasPaid[_choice]) {
// Allow to reimburse if funding was unsuccessful for this ruling option.
amount = round.contributions[_beneficiary][_choice];
} else {
// Funding was successful for this ruling option.
if (_choice == finalRuling) {
// This ruling option is the ultimate winner.
amount = round.paidFees[_choice] > 0
? (round.contributions[_beneficiary][_choice] * round.feeRewards) / round.paidFees[_choice]
: 0;
} else if (!round.hasPaid[finalRuling]) {
// The ultimate winner was not funded in this round. In this case funded ruling option(s) are reimbursed.
amount =
(round.contributions[_beneficiary][_choice] * round.feeRewards) /
(round.paidFees[round.fundedChoices[0]] + round.paidFees[round.fundedChoices[1]]);
for (uint256 i = 0; i < dispute.rounds.length; i++) {
Round storage round = dispute.rounds[i];

if (!round.hasPaid[_choice]) {
// Allow to reimburse if funding was unsuccessful for this ruling option.
amount += round.contributions[_beneficiary][_choice];
} else {
// Funding was successful for this ruling option.
if (_choice == finalRuling) {
// This ruling option is the ultimate winner.
amount += round.paidFees[_choice] > 0
? (round.contributions[_beneficiary][_choice] * round.feeRewards) / round.paidFees[_choice]
: 0;
} else if (!round.hasPaid[finalRuling]) {
// The ultimate winner was not funded in this round. In this case funded ruling option(s) are reimbursed.
amount +=
(round.contributions[_beneficiary][_choice] * round.feeRewards) /
(round.paidFees[round.fundedChoices[0]] + round.paidFees[round.fundedChoices[1]]);
}
}
round.contributions[_beneficiary][_choice] = 0;
}
round.contributions[_beneficiary][_choice] = 0;

if (amount != 0) {
_beneficiary.safeSend(amount, wNative);
emit Withdrawal(_coreDisputeID, _coreRoundID, _choice, _beneficiary, amount);
emit Withdrawal(_coreDisputeID, _choice, _beneficiary, amount);
}
}
Comment on lines 438 to 485
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Withdrawals: guard unknown disputes and handle edge cases explicitly

  • Good: allows withdrawals after jumps; iterates all rounds; zeroes contributions.
  • Edge case: else-if branch assumes two funded choices; enforce fundedChoices.length == 2 to avoid accidental OOB or skew if rules ever change.
-                } else if (!round.hasPaid[finalRuling]) {
+                } else if (!round.hasPaid[finalRuling]) {
+                    // Expect exactly 2 funded choices in this design.
+                    if (round.fundedChoices.length < 2) continue;
                     amount +=
                         (round.contributions[_beneficiary][_choice] * round.feeRewards) /
                         (round.paidFees[round.fundedChoices[0]] + round.paidFees[round.fundedChoices[1]]);
                 }

Also consider reverting with DisputeInconsistentFunding() if length != 2 to surface data inconsistencies early.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol around lines
438 to 485, the else-if branch assumes round.fundedChoices has exactly two
entries when summing paidFees by indexing [0] and [1]; add an explicit guard
that round.fundedChoices.length == 2 before accessing those indices and if not
true revert with DisputeInconsistentFunding() (or an existing appropriate error)
to surface inconsistent funding state; place this check immediately before the
division that uses fundedChoices and use the guarded branch to compute the
reimbursement, otherwise revert.


Expand Down Expand Up @@ -748,11 +754,11 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi

error OwnerOnly();
error KlerosCoreOnly();
error DisputeJumpedToParentDK();
error DisputeJumpedToAnotherDisputeKit();
error DisputeUnknownInThisDisputeKit();
error UnsuccessfulCall();
error NotCommitPeriod();
error EmptyCommit();
error NotActiveForCoreDisputeID();
error JurorHasToOwnTheVote();
error NotVotePeriod();
error EmptyVoteIDs();
Expand Down
4 changes: 2 additions & 2 deletions contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ contract DisputeKitGatedShutter is DisputeKitClassicBase {
bytes32 _recoveryCommit,
bytes32 _identity,
bytes calldata _encryptedVote
) external notJumped(_coreDisputeID) {
) external {
if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit();

uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID];
Expand All @@ -128,7 +128,7 @@ contract DisputeKitGatedShutter is DisputeKitClassicBase {
recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit;
}

// `_castCommit()` ensures that the caller owns the vote
// `_castCommit()` ensures that the caller owns the vote and that dispute is active
_castCommit(_coreDisputeID, _voteIDs, _commit);
emit CommitCastShutter(_coreDisputeID, msg.sender, _commit, _recoveryCommit, _identity, _encryptedVote);
}
Expand Down
4 changes: 2 additions & 2 deletions contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ contract DisputeKitShutter is DisputeKitClassicBase {
bytes32 _recoveryCommit,
bytes32 _identity,
bytes calldata _encryptedVote
) external notJumped(_coreDisputeID) {
) external {
if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit();

uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID];
Expand All @@ -112,7 +112,7 @@ contract DisputeKitShutter is DisputeKitClassicBase {
recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit;
}

// `_castCommit()` ensures that the caller owns the vote
// `_castCommit()` ensures that the caller owns the vote and that dispute is active
_castCommit(_coreDisputeID, _voteIDs, _commit);
emit CommitCastShutter(_coreDisputeID, msg.sender, _commit, _recoveryCommit, _identity, _encryptedVote);
}
Expand Down
1 change: 1 addition & 0 deletions contracts/src/arbitration/interfaces/IDisputeKit.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ interface IDisputeKit {

/// @notice Creates a local dispute and maps it to the dispute ID in the Core contract.
/// @dev Access restricted to Kleros Core only.
/// @dev The new `KlerosCore.Round` must be created before calling this function.
/// @param _coreDisputeID The ID of the dispute in Kleros Core, not in the Dispute Kit.
/// @param _numberOfChoices Number of choices of the dispute
/// @param _extraData Additional info about the dispute, for possible use in future dispute kits.
Expand Down
Loading
Loading