Release #2076
Release #2076
Conversation
…use of @wagmi/cli
Feat: Dispute Kits helper in contracts package
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Enabled Hardhat viaIR compilation with solc v0.8.30
bumped @kleros/vea-contracts to v0.7.0
chore: viaIR compilation enabled for Foundry with explicit solc v0.8.30
… deprecated for later removal
feat: court llms.txt
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughLarge rewrite across contracts, scripts, tests, and web app: ownership changed from governor to owner; RNG interface refactored to IRNG with RNGWithFallback and timestamped BlockhashRNG; SortitionTrees library added; SortitionModule rebuilt; KlerosCore rewritten with new APIs; dispute kits updated (coherence split, jump DK, Shutter recovery commits); Neo variants removed; deployments/scripts/tests updated; web adds gated-token validation and multi-DK voting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Sortition as SortitionModule
participant RNGF as RNGWithFallback
participant RNG as Primary IRNG
participant BH as BlockhashRNG
Caller->>Sortition: passPhase()
Sortition->>RNGF: requestRandomness()
RNGF->>RNG: requestRandomness()
note over RNGF: store requestTimestamp
Sortition->>Sortition: later: passPhase()
Sortition->>RNGF: receiveRandomness()
alt Primary ready before timeout
RNGF->>RNG: receiveRandomness()
RNG-->>RNGF: randomNumber!=0
RNGF-->>Sortition: randomNumber
else Timeout or zero
RNGF-->>Sortition: blockhash-based randomNumber
note over RNGF: emit RNGFallback
end
Sortition->>Sortition: Drawing with randomNumber
Sortition->>Sortition: draw(courtID, disputeID, nonce)
Sortition-->>Caller: drawnAddress, fromSubcourtID
sequenceDiagram
autonumber
actor User
participant Core as KlerosCore
participant DK as IDisputeKit
participant SM as SortitionModule
User->>Core: appeal(disputeID, choice) fund
Core->>DK: getNbVotesAfterAppeal(prevDK, currentNbVotes)
DK-->>Core: nbVotesNext
Core->>DK: earlyCourtJump(disputeID)?
alt Jump needed
Core->>Core: select new court / DK (getJumpDisputeKitID)
else No jump
Core->>Core: stay in court/DK
end
Core->>SM: postDrawHook(disputeID, roundID)
Core-->>User: AppealDecision / (optional) CourtJump, DisputeKitJump
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150+ minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-neo-devtools failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/src/arbitration/KlerosCoreBase.sol (3)
94-104: Owner storage OK; add EIP-173-style event and zero-address guard.Track ownership changes and prevent accidental burn on init/change.
address public owner; // The owner of the contract. + event OwnerChanged(address indexed previousOwner, address indexed newOwner);And emit in initializer/changeOwner (see dedicated diffs below). Also consider whether
guardianmay be zero by design; if not, guard similarly.
196-209: Initializer: validate inputs and emit ownership event.Prevent zero owner; record initial transfer for indexers.
function __KlerosCoreBase_initialize( - address _owner, + address _owner, address _guardian, @@ ) internal onlyInitializing { - owner = _owner; + if (_owner == address(0)) revert OwnerZeroAddress(); + owner = _owner; + emit OwnerChanged(address(0), _owner); guardian = _guardian;Add error at the bottom:
+ error OwnerZeroAddress();
452-456: Remove redundant event declaration and add zero‐rate validation
TheNewCurrencyRateevent is declared inIArbitratorV2and inherited—no need to re‐declare it inKlerosCoreBase.sol. You must, however, prevent a zero rate (which would later cause a division‐by‐zero inconvertEthToTokenAmount). Incontracts/src/arbitration/KlerosCoreBase.sol, updatechangeCurrencyRatesto:function changeCurrencyRates(IERC20 _feeToken, uint64 _rateInEth, uint8 _rateDecimals) external onlyByGovernor { + if (_rateInEth == 0) revert InvalidRate(); currencyRates[_feeToken].rateInEth = _rateInEth; currencyRates[_feeToken].rateDecimals = _rateDecimals; emit NewCurrencyRate(_feeToken, _rateInEth, _rateDecimals); }And add at the bottom of the file:
+ error InvalidRate();
♻️ Duplicate comments (4)
contracts/CHANGELOG.md (3)
29-29: Clarify exact pragma range and scope (“interfaces only” likely inaccurate).Recommend stating the exact range and reflecting actual scope (interfaces and RNG). Please verify across the repo before applying.
- - Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083)) + - Widen the allowed pragma to `>=0.8.0 <0.9.0` for interfaces (and RNG contracts if applicable) ([#2083](https://github.com/kleros/kleros-v2/issues/2083))To verify scope:
#!/bin/bash rg -nP '^\s*pragma\s+solidity\s+[^;]+;' -g 'contracts/**' -C0
7-7: Avoid broken release links for an unpublished tag.If 0.13.0 isn’t live yet, comment out its link and optionally mark the section as “Unreleased.” Update when published.
-## [0.13.0] - 2025-08-07 (Not published yet) +## [0.13.0] - Unreleased-[0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0 +<!-- [0.13.0]: add after publishing -->To check link availability:
#!/bin/bash curl -s -o /dev/null -w "%{http_code}\n" \ "https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0"Also applies to: 156-157
31-31: Fix constant name: ONE_BASIS_POINT (singular).Public-facing docs must match the code. Use the correct identifier.
- - Consolidate the constant `ALPHA_DIVISOR` with `ONE_BASIS_POINTS` ([#2090](https://github.com/kleros/kleros-v2/issues/2090)) + - Replace `ALPHA_DIVISOR` with `ONE_BASIS_POINT` ([#2090](https://github.com/kleros/kleros-v2/issues/2090))contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
137-140: Add active-dispute guard in notJumped (prevents accidental reads of dispute 0).Accessing disputes[coreDisputeIDToLocal[_coreDisputeID]] without validating activity can read the zero-initialized dispute. Guard with coreDisputeIDToActive first.
modifier notJumped(uint256 _coreDisputeID) { - if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); + if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); _; }
🧹 Nitpick comments (28)
contracts/src/arbitration/KlerosCoreBase.sol (3)
63-68: Keep arrays aligned; consider sanity checks on lengths.Adding
drawnJurorFromCourtIDsparallel todrawnJurorsis fine. Ensure all push/pop sites keep lengths equal; add an internal assert in execute paths to detect divergence early.+ // Sanity: keep arrays aligned (dev-only; remove if gas-sensitive) + assert(round.drawnJurorFromCourtIDs.length == round.drawnJurors.length);
315-320: Adding new DK: validate address and uniqueness.Check non-zero and that the address has code; optional: prevent duplicate insertion.
- function addNewDisputeKit(IDisputeKit _disputeKitAddress) external onlyByOwner { + function addNewDisputeKit(IDisputeKit _disputeKitAddress) external onlyByOwner { + if (address(_disputeKitAddress) == address(0)) revert InvalidAddress(); uint256 disputeKitID = disputeKits.length;
342-382: Court creation flow looks good; tiny nit on casting.
- Flow: validate, push, enable DKs, ensure Classic enabled, set fields, create tree, link parent — solid.
- Nit:
uint96 courtID = uint96(courts.length);is fine given practical limits; add an assert for future-proofing.- uint96 courtID = uint96(courts.length); + uint256 _newId = courts.length; + assert(_newId < type(uint96).max); + uint96 courtID = uint96(_newId);Also applies to: 346-354, 367-368
contracts/CHANGELOG.md (3)
15-15: Tighten phrasing and use canonical ERC link.Replace “in order to comply” with “to comply” and point to eips.ethereum.org.
- - **Breaking:** Rename `governor` to `owner` in order to comply with the lightweight ownership standard [ERC-5313](https://eipsinsight.com/ercs/erc-5313) ([#2112](https://github.com/kleros/kleros-v2/issues/2112)) + - **Breaking:** Rename `governor` to `owner` to comply with the lightweight ownership standard [ERC-5313](https://eips.ethereum.org/EIPS/eip-5313) ([#2112](https://github.com/kleros/kleros-v2/issues/2112))
21-21: Spell and style: “bypass” (one word).Minor wording cleanup.
- - Allow stake changes to by-pass delayed stakes when initiated by the SortitionModule by setting the `_noDelay` parameter to `true` in `SortitionModule.validateStake()` ([#2107](https://github.com/kleros/kleros-v2/issues/2107)) + - Allow stake changes to bypass delayed stakes when initiated by the SortitionModule by setting `_noDelay` to `true` in `SortitionModule.validateStake()` ([#2107](https://github.com/kleros/kleros-v2/issues/2107))
41-41: Grammar: “advance to the Voting period.”Improves readability.
- - Do not pass to Voting period if all the commits are cast because it breaks the current Shutter auto-reveal process. ([#2085](https://github.com/kleros/kleros-v2/issues/2085)) + - Do not advance to the Voting period if all commits are cast, as it breaks the current Shutter auto‑reveal process. ([#2085](https://github.com/kleros/kleros-v2/issues/2085))contracts/src/libraries/SortitionTrees.sol (4)
170-171: Typo in NatSpec: “subtract,” not “substract.”Public docs should be clean.
- /// @param _plusOrMinus Whether to add (true) or substract (false). - /// @param _value The value to add or substract. + /// @param _plusOrMinus Whether to add (true) or subtract (false). + /// @param _value The value to add or subtract.
3-3: Align pragma with toolchain or widen consistently.Hardhat/Foundry target 0.8.30 per CHANGELOG. Consider
^0.8.30or>=0.8.0 <0.9.0for consistency with interfaces.-pragma solidity ^0.8.24; +pragma solidity ^0.8.30; // or: pragma solidity >=0.8.0 <0.9.0;
206-211: Update linter directive (“solium” → “solhint”).Solium is deprecated; use solhint directive to suppress the inline assembly warning.
- // solium-disable-line security/no-inline-assembly + // solhint-disable-next-line no-inline-assembly
217-224: Mirror linter directive for unpack as well.- // solium-disable-line security/no-inline-assembly + // solhint-disable-next-line no-inline-assemblycontracts/test/sortition/index.ts (4)
315-334: Reduce flakiness of weighted draw test.With only 100 samples, randomness modulo arithmetic could underperform the 80% threshold sporadically. Increase draws (e.g., 1_000) or relax the threshold a bit (e.g., > 70%) to stabilize CI.
- const numDraws = 100; + const numDraws = 1000; - expect(draws[getTestAddress(1).toLowerCase()]).to.be.greaterThan(numDraws * 0.8); + expect(draws[getTestAddress(1).toLowerCase()]).to.be.greaterThan(numDraws * 0.7);
631-647: Make the MaxUint256 “gas limit” test deterministic.Catching an error by matching
/gas/iis brittle across EVMs/providers. Either assert the call does not revert or skip if environment-dependent.- try { - await sortitionTree.set(0, juror, maxStake); - expect(await sortitionTree.stakeOf(0, juror)).to.equal(maxStake); - } catch (error) { - // Expected to fail due to gas limits, not due to overflow - expect(error).to.match(/gas/i); - } + await expect(sortitionTree.set(0, juror, maxStake)).to.not.be.reverted; + expect(await sortitionTree.stakeOf(0, juror)).to.equal(maxStake);
509-519: Tighten the expectation on returned court IDs or document rationale.Allowing ranges like
[0,1]and[1,2]hides regressions. If the algorithm should return the exact source court, assert equality; otherwise, add a brief comment explaining why multiple values are valid.
15-17: Add a tiny helper to standardize casting in assertions.Optional convenience to reduce repetition.
// Helper function to create a test juror address const getTestAddress = (index: number): string => accounts[index % accounts.length].address; + // Helper for BigInt casting in ethers v6 assertions + const bn = (v: number | bigint) => BigInt(v);contracts/src/arbitration/interfaces/ISortitionModule.sol (1)
33-40: Clarify setStakePenalty return semantics.The name
availablePenaltyis ambiguous. If it represents “remaining penalty to apply in other courts,” document it here, or consider renaming in a future major bump. Ensure implementations return the remainder, not the applied amount.contracts/src/arbitration/university/SortitionModuleUniversity.sol (4)
115-118: Mark createTree as overriding (and consider restricting caller).If
ISortitionModuleUniversitydeclarescreateTree, addoverride. Also consideronlyByCoreif creation should be core-driven.- function createTree(uint96 _courtID, bytes memory _extraData) external { + function createTree(uint96 _courtID, bytes memory _extraData) external override /* onlyByCore */ {
246-287: _setStake: minor safety and clarity improvements.
- Consider guarding
juror.stakedPnk -= _pnkWithdrawal;with an assert (or rely on upstream invariant) to avoid underflow if callers change.- Event: consider making
_courtIDauint96inStakeSetfor consistency.- event StakeSet(address indexed _address, uint256 _courtID, uint256 _amount, uint256 _amountAllCourts); + event StakeSet(address indexed _address, uint96 _courtID, uint256 _amount, uint256 _amountAllCourts);
69-76: Initialize: add zero-address checks (optional).Add sanity checks for
_ownerand_coreto fail fast on bad deployments.function initialize(address _owner, KlerosCoreUniversity _core) external reinitializer(1) { - owner = _owner; - core = _core; + if (_owner == address(0)) revert OwnerOnly(); // or a dedicated ZeroAddress error + owner = _owner; + if (address(_core) == address(0)) revert KlerosCoreOnly(); // or a dedicated ZeroAddress error + core = _core; }Also applies to: 88-93
348-351: draw: returnfromSubcourtIDexplicitly (even if zero).Tiny readability improvement; no behavior change.
-function draw(uint96, uint256, uint256) public view override returns (address drawnAddress, uint96 fromSubcourtID) { - drawnAddress = transientJuror; -} +function draw(uint96 _courtID, uint256, uint256) public view override returns (address drawnAddress, uint96 fromSubcourtID) { + drawnAddress = transientJuror; + fromSubcourtID = _courtID; // educational mock: echo back +}contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (5)
173-177: Normalize changeOwner signature to match storage type.owner is address; changeOwner takes address payable. Make types consistent.
- function changeOwner(address payable _owner) external onlyByOwner { + function changeOwner(address _owner) external onlyByOwner { owner = _owner; }
260-278: Explicitly bound-check vote IDs to avoid Panic reverts.Accessing round.votes[_voteIDs[i]] without bounds checks can trigger a low-level Panic. Emit a clear error instead.
for (uint256 i = 0; i < _voteIDs.length; i++) { + if (_voteIDs[i] >= round.votes.length) revert VoteIDOutOfBounds(); if (round.votes[_voteIDs[i]].account != msg.sender) revert JurorHasToOwnTheVote(); round.votes[_voteIDs[i]].commit = _commit; }Also add at the bottom error list:
+ error VoteIDOutOfBounds();
320-328: Same bound-check for castVote loop.for (uint256 i = 0; i < _voteIDs.length; i++) { + if (_voteIDs[i] >= round.votes.length) revert VoteIDOutOfBounds(); if (round.votes[_voteIDs[i]].account != _juror) revert JurorHasToOwnTheVote(); if (hiddenVotes && round.votes[_voteIDs[i]].commit != voteHash) revert HashDoesNotMatchHiddenVoteCommitment(); if (round.votes[_voteIDs[i]].voted) revert VoteAlreadyCast(); ... }
164-171: Bubble revert reasons from executeOwnerProposal for easier debugging.Optional: on failure, bubble returndata instead of a generic UnsuccessfulCall. Keeps revert diagnostics.
function executeOwnerProposal(address _destination, uint256 _amount, bytes memory _data) external onlyByOwner { - (bool success, ) = _destination.call{value: _amount}(_data); - if (!success) revert UnsuccessfulCall(); + (bool success, bytes memory returndata) = _destination.call{value: _amount}(_data); + if (!success) { + assembly { + revert(add(returndata, 0x20), mload(returndata)) + } + } }
663-681: Fix returned name typo (totalCommited → totalCommitted).ABI names matter for tooling/UX; align spelling.
- uint256 totalCommited, + uint256 totalCommitted,contracts/src/test/SortitionTreesMock.sol (2)
58-62: Avoid external self-calls (this.set) inside loops to save gas and reentrancy surface.Refactor to an internal helper and call it directly.
- function set(uint96 _courtID, address _account, uint256 _value) external { + function set(uint96 _courtID, address _account, uint256 _value) external { + _set(_courtID, _account, _value); + } + + function _set(uint96 _courtID, address _account, uint256 _value) internal { TreeKey key = CourtID.wrap(_courtID).toTreeKey(); bytes32 stakePathID = SortitionTrees.toStakePathID(_account, _courtID); SortitionTrees.set(trees[key], _value, stakePathID); } /// @dev Set stake for a juror across multiple courts (for testing hierarchy) function setStakeInHierarchy(uint96[] calldata _courtIDs, address _account, uint256 _value) external { for (uint256 i = 0; i < _courtIDs.length; i++) { - this.set(_courtIDs[i], _account, _value); + _set(_courtIDs[i], _account, _value); } }
69-73: Avoid external self-calls (this.stakeOf) in getters.Use an internal helper to read directly.
- function getStakesAcrossCourts( + function getStakesAcrossCourts( address _account, uint96[] calldata _courtIDs ) external view returns (uint256[] memory stakes) { stakes = new uint256[](_courtIDs.length); for (uint256 i = 0; i < _courtIDs.length; i++) { - stakes[i] = this.stakeOf(_courtIDs[i], _account); + stakes[i] = _stakeOf(_courtIDs[i], _account); } } + + function _stakeOf(uint96 _courtID, address _account) internal view returns (uint256) { + TreeKey key = CourtID.wrap(_courtID).toTreeKey(); + bytes32 stakePathID = SortitionTrees.toStakePathID(_account, _courtID); + return SortitionTrees.stakeOf(trees[key], stakePathID); + }contracts/src/arbitration/SortitionModuleBase.sol (2)
303-319: Docstring mismatch: setStakePenalty is a penalty, not a reward.Fix comment to avoid confusion.
-/// @dev Update the state of the stakes with a PNK reward deposit, called by KC during rewards execution. +/// @dev Apply a PNK penalty (deduction) to the juror’s stake, called by KC during rewards execution.
202-208: executeDelayedStakes: no-op handling when nothing pending.Current revert uses delayedStakeWriteIndex < delayedStakeReadIndex. When equal (common “empty” case after full drain), revert would not trigger, but loop becomes 0-iter, which is fine. If you prefer explicit UX, consider reverting also on equality.
- if (delayedStakeWriteIndex < delayedStakeReadIndex) revert NoDelayedStakeToExecute(); + if (delayedStakeWriteIndex + 1 <= delayedStakeReadIndex) revert NoDelayedStakeToExecute();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
contracts/CHANGELOG.md(2 hunks)contracts/src/arbitration/KlerosCoreBase.sol(27 hunks)contracts/src/arbitration/SortitionModuleBase.sol(13 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(14 hunks)contracts/src/arbitration/interfaces/ISortitionModule.sol(3 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(11 hunks)contracts/src/libraries/SortitionTrees.sol(1 hunks)contracts/src/test/SortitionModuleMock.sol(1 hunks)contracts/src/test/SortitionTreesMock.sol(1 hunks)contracts/test/sortition/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`.
Applied to files:
contracts/src/arbitration/SortitionModuleBase.solcontracts/src/arbitration/KlerosCoreBase.sol
📚 Learning: 2024-11-19T16:09:41.467Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:3-17
Timestamp: 2024-11-19T16:09:41.467Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently.
Applied to files:
contracts/src/arbitration/KlerosCoreBase.sol
🪛 LanguageTool
contracts/CHANGELOG.md
[style] ~15-~15: Consider a more concise word here.
Context: ...reaking:** Rename governor to owner in order to comply with the lightweight ownership s...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (23)
contracts/src/arbitration/KlerosCoreBase.sol (17)
171-179: Access-control rename to Owner LGTM.
226-227: Tree creation ordering OK.Creating FORKING and GENERAL trees early matches usage later.
Also applies to: 239-240
261-272: Pause/unpause semantics consistent.Guardian-or-owner for pause and owner-only for unpause is coherent.
392-417: Parameter update validations LGTM.Parent/children
minStakeconstraints enforced correctly.
423-438: enableDisputeKits: good guardrails on Classic DK.
467-468: OnError.Revert choice is correct for user-facing staking.
470-477: By-sortition staking path: good separation and no-delay.Using
OnError.Returnavoids bricking the module loop.
610-618: Track subcourt for penalty: good; add bounds fallback already handled.Using
fromSubcourtID != 0 ? fromSubcourtID : dispute.courtIDis safe if DK returns 0 for “none”.
643-651: Jump helpers integration LGTM.Emitting
CourtJumpon actual jump is correct.
939-957: appealCost: jump logic OK; TODO on General Court fork should be actionable.Returning
NON_PAYABLE_AMOUNTis a sentinel; either implement or open a tracked issue so integrators can handle it predictably.
1045-1051: isDisputeKitJumping: early exit is fine.
1065-1076: New jump helpers read cleanly; solid separation of concerns.Also applies to: 1078-1098
1114-1118: ONE_BASIS_POINT migration LGTM.Replacing legacy divisor with basis-point constant is correct and gas-neutral.
Also applies to: 1125-1126
1141-1150: _setStake signature/usage: good extension for no-delay and error policy.Also applies to: 1162-1164
1236-1238: Error renames align with owner/guardian migration.
443-446: AcceptedFeeToken already declared in IArbitratorV2.sol
The event is defined in the IArbitratorV2 interface (contracts/src/arbitration/interfaces/IArbitratorV2.sol:30) and imported by KlerosCoreBase.sol, so no additional declaration is needed.Likely an incorrect or invalid review comment.
565-567: Confirm commit-period gating in Shutter and add edge-case tests
Ensure removing the “all commits cast” shortcut doesn’t break Shutter auto-reveal. Add tests covering:
- Early full-commit pass in KlerosCoreBase (commit→vote transition when all commits are in before window end)
- Commit gating in KlerosCoreUniversity (University flow)
- Short commit windows and timing edges (warp scenarios)
contracts/src/test/SortitionModuleMock.sol (2)
6-6: Import looks correct and scoped.
12-12: LGTM: mock reads SortitionTrees state via typed key.Access pattern aligns with the new library; return values preserved.
contracts/src/arbitration/interfaces/ISortitionModule.sol (1)
51-56: Resolved — draw 2-tuple is propagated and consumed.
- Interfaces updated: ISortitionModule.draw and IDisputeKit.draw now return (address, uint96). (contracts/src/arbitration/interfaces/ISortitionModule.sol, contracts/src/arbitration/interfaces/IDisputeKit.sol)
- Sortition implements the 2-tuple: SortitionModuleBase.draw returns (address, uint96) and sources the second value from the trees. (contracts/src/arbitration/SortitionModuleBase.sol)
- Dispute kits forward it: DisputeKitClassicBase.draw forwards (drawnAddress, fromSubcourtID) from sortitionModule.draw. (contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol)
- Core consumes it: KlerosCoreBase.draw receives (drawnAddress, fromSubcourtID) and pushes fromSubcourtID into round.drawnJurorFromCourtIDs. (contracts/src/arbitration/KlerosCoreBase.sol)
No further changes required.
contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
331-339: Leftover PNK withdrawal: good state-change-before-external-call pattern.Setting
stakedPnk = 0beforetransferBySortitionModuleprevents reentrancy double-withdrawals. LGTM.contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
615-622: Confirm isAppealFunded semantics when exactly one choice is funded.Function name/docs suggest “funding finished prematurely when losing side didn’t fund.” Current condition returns true only if fundedChoices.length == 0. Should it also return true when fundedChoices.length == 1 and loser window elapsed?
contracts/src/arbitration/SortitionModuleBase.sol (1)
481-489: draw should not depend on stale randomNumber across Drawing phases.If Drawing spans multiple disputes, ensure randomNumber is updated every Generating phase and cannot persist across cycles unexpectedly. Consider zeroing randomNumber when entering staking or after Drawing completes.
| function createTree(uint96 _courtID, bytes memory _extraData) external; | ||
|
|
||
| function validateStake( | ||
| address _account, | ||
| uint96 _courtID, | ||
| uint256 _newStake | ||
| uint256 _newStake, | ||
| bool _noDelay | ||
| ) external returns (uint256 pnkDeposit, uint256 pnkWithdrawal, StakingResult stakingResult); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
createTree/validateStake signature changes look consistent; ensure downstream updates.
All implementers/call sites must adopt (uint96 _courtID, bytes _extraData) and extra _noDelay param. Recommend verifying overrides and mocks compile against this interface.
Run:
🏁 Script executed:
#!/bin/bash
# Validate overrides across the repo
rg -nP 'interface\s+ISortitionModule\b' -C1
rg -nP 'function\s+createTree\s*\(\s*uint96\s*_courtID,\s*bytes' -C2
rg -nP 'validateStake\s*\(\s*address\s*_account,\s*uint96\s*_courtID,\s*uint256\s*_newStake,\s*bool\s*_noDelay' -C2
rg -nP '(contract|abstract).*SortitionModule.*\b(createTree|validateStake)\b' -C3Length of output: 317853
Fix ISortitionModule signature mismatch — update implementations & callers to the new signatures (createTree(uint96, bytes) and validateStake(..., bool _noDelay))
rg shows no uses of the new signatures; the repo still uses createTree(bytes32, bytes) and validateStake(address,uint96,uint256). Update these locations at minimum:
- contracts/src/arbitration/SortitionModuleBase.sol — change createTree/validateStake signatures and internals.
- contracts/src/arbitration/SortitionModule.sol — update overrides to match interface.
- contracts/src/arbitration/KlerosCoreBase.sol — update calls to sortitionModule.createTree(...) and sortitionModule.validateStake(..., _noDelay).
- contracts/deployments/** and any tests/mocks that rely on the old ABI — regenerate/update.
🤖 Prompt for AI Agents
In contracts/src/arbitration/interfaces/ISortitionModule.sol around lines 16–23
the interface signatures changed to createTree(uint96 _courtID, bytes
_extraData) and validateStake(address _account, uint96 _courtID, uint256
_newStake, bool _noDelay) returns (uint256 pnkDeposit, uint256 pnkWithdrawal,
StakingResult stakingResult); update the implementations and callers
accordingly: in contracts/src/arbitration/SortitionModuleBase.sol and
contracts/src/arbitration/SortitionModule.sol change the function declarations
to match the new signatures and adjust internal logic to accept a uint96 courtID
for createTree and handle the added _noDelay boolean and the three-value return
from validateStake; in contracts/src/arbitration/KlerosCoreBase.sol update all
calls to sortitionModule.createTree(...) and sortitionModule.validateStake(...,
_noDelay) to pass a uint96 courtID and the new _noDelay flag and to capture the
pnkDeposit/pnkWithdrawal/stakingResult return values; finally update any
deployment scripts, generated ABIs, tests and mocks under
contracts/deployments/** and test files to the new ABI signatures and regenerate
artifacts so builds/tests use the updated interface.
| /// @dev Allows the owner to call anything on behalf of the contract. | ||
| /// @param _destination The destination of the call. | ||
| /// @param _amount The value sent with the call. | ||
| /// @param _data The data sent with the call. | ||
| function executeGovernorProposal( | ||
| address _destination, | ||
| uint256 _amount, | ||
| bytes memory _data | ||
| ) external onlyByGovernor { | ||
| function executeOwnerProposal(address _destination, uint256 _amount, bytes memory _data) external onlyByOwner { | ||
| (bool success, ) = _destination.call{value: _amount}(_data); | ||
| if (!success) revert UnsuccessfulCall(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Owner arbitrary-call: add event and reentrancy guard.
This function can send ETH and call arbitrary code; emit an audit trail and block reentrancy.
- function executeOwnerProposal(address _destination, uint256 _amount, bytes memory _data) external onlyByOwner {
+ event OwnerCallExecuted(address indexed destination, uint256 value, bytes data, bool success);
+ function executeOwnerProposal(address _destination, uint256 _amount, bytes memory _data) external onlyByOwner nonReentrant {
(bool success, ) = _destination.call{value: _amount}(_data);
if (!success) revert UnsuccessfulCall();
+ emit OwnerCallExecuted(_destination, _amount, _data, success);
}See guard scaffold at the end of this review for a minimal nonReentrant modifier.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/src/arbitration/KlerosCoreBase.sol around lines 273 to 281, the
executeOwnerProposal allows sending ETH and calling arbitrary code but lacks an
event and reentrancy protection; add a nonReentrant modifier to the contract
(e.g., using a private uint256 _reentrancyLock with checks and updates) and
apply it to executeOwnerProposal, declare a descriptive event (e.g.,
OwnerExecutedProposal(address indexed destination, uint256 amount, bytes data,
bool success)) and emit it after the call (including success/failure info), and
ensure the modifier prevents reentrancy by setting and clearing the lock and
reverting on reentrancy attempts.
| /// @dev Changes the `owner` storage variable. | ||
| /// @param _owner The new value for the `owner` storage variable. | ||
| function changeOwner(address payable _owner) external onlyByOwner { | ||
| owner = _owner; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ownership change: emit event and reject zero-address unless “renounce” is intended.
If renouncing is wanted, expose a dedicated renounceOwner(); otherwise prevent zero.
- function changeOwner(address payable _owner) external onlyByOwner {
- owner = _owner;
+ function changeOwner(address payable _owner) external onlyByOwner {
+ if (_owner == address(0)) revert OwnerZeroAddress();
+ emit OwnerChanged(owner, _owner);
+ owner = _owner;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @dev Changes the `owner` storage variable. | |
| /// @param _owner The new value for the `owner` storage variable. | |
| function changeOwner(address payable _owner) external onlyByOwner { | |
| owner = _owner; | |
| } | |
| /// @dev Changes the `owner` storage variable. | |
| /// @param _owner The new value for the `owner` storage variable. | |
| function changeOwner(address payable _owner) external onlyByOwner { | |
| if (_owner == address(0)) revert OwnerZeroAddress(); | |
| emit OwnerChanged(owner, _owner); | |
| owner = _owner; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/KlerosCoreBase.sol around lines 282 to 286, the
changeOwner function currently assigns owner directly; update it to (1) reject
the zero address with a require(check) to prevent accidental renouncement unless
explicitly intended, (2) emit an OwnershipTransferred (or OwnerChanged) event
with previous and new owner values, and (3) if renouncement should be supported,
implement a separate renounceOwner() function that sets owner to address(0) and
emits the same event; ensure onlyByOwner remains enforced.
| /// @dev Changes the `guardian` storage variable. | ||
| /// @param _guardian The new value for the `guardian` storage variable. | ||
| function changeGuardian(address _guardian) external onlyByGovernor { | ||
| function changeGuardian(address _guardian) external onlyByOwner { | ||
| guardian = _guardian; | ||
| } | ||
|
|
||
| /// @dev Changes the `pinakion` storage variable. | ||
| /// @param _pinakion The new value for the `pinakion` storage variable. | ||
| function changePinakion(IERC20 _pinakion) external onlyByGovernor { | ||
| function changePinakion(IERC20 _pinakion) external onlyByOwner { | ||
| pinakion = _pinakion; | ||
| } | ||
|
|
||
| /// @dev Changes the `jurorProsecutionModule` storage variable. | ||
| /// @param _jurorProsecutionModule The new value for the `jurorProsecutionModule` storage variable. | ||
| function changeJurorProsecutionModule(address _jurorProsecutionModule) external onlyByGovernor { | ||
| function changeJurorProsecutionModule(address _jurorProsecutionModule) external onlyByOwner { | ||
| jurorProsecutionModule = _jurorProsecutionModule; | ||
| } | ||
|
|
||
| /// @dev Changes the `_sortitionModule` storage variable. | ||
| /// Note that the new module should be initialized for all courts. | ||
| /// @param _sortitionModule The new value for the `sortitionModule` storage variable. | ||
| function changeSortitionModule(ISortitionModule _sortitionModule) external onlyByGovernor { | ||
| function changeSortitionModule(ISortitionModule _sortitionModule) external onlyByOwner { | ||
| sortitionModule = _sortitionModule; | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Change-XXX setters: add basic validation.
changeGuardian: prevent setting owner as guardian (optional) or zero if not allowed.changePinakion: disallow zero; consider preventingNATIVE_CURRENCY.changeJurorProsecutionModule/changeSortitionModule: require contract code present; for sortition, verify the module is initialized for all courts or document the exact migration procedure.
- function changePinakion(IERC20 _pinakion) external onlyByOwner {
+ function changePinakion(IERC20 _pinakion) external onlyByOwner {
+ if (address(_pinakion) == address(0)) revert InvalidAddress();
pinakion = _pinakion;
}
@@
- function changeSortitionModule(ISortitionModule _sortitionModule) external onlyByOwner {
- sortitionModule = _sortitionModule;
+ function changeSortitionModule(ISortitionModule _sortitionModule) external onlyByOwner {
+ if (address(_sortitionModule) == address(0)) revert InvalidAddress();
+ sortitionModule = _sortitionModule;
}Add:
+ error InvalidAddress();Please confirm where the “initialize for all courts” step is executed during upgrade.
🏁 Script executed:
#!/bin/bash
set -e
# Verify existing validation in changeXXX functions
rg -n -C3 "function changeGuardian" -g "contracts/src/arbitration/KlerosCoreBase.sol"
rg -n -C3 "function changePinakion" -g "contracts/src/arbitration/KlerosCoreBase.sol"
rg -n -C3 "function changeJurorProsecutionModule" -g "contracts/src/arbitration/KlerosCoreBase.sol"
rg -n -C3 "function changeSortitionModule" -g "contracts/src/arbitration/KlerosCoreBase.sol"
# Check court creation and sortition module initialization
rg -n -C3 "function createCourt" -g "contracts/src/arbitration/KlerosCoreBase.sol"
rg -n -C3 "initialize" -g "contracts/src/arbitration/KlerosCoreBase.sol"Length of output: 5036
🏁 Script executed:
#!/bin/bash
set -e
echo "--- sortitionModule.initialize callers ---"
rg -nP "sortitionModule\\.initialize" -g contracts/src/arbitration/KlerosCoreBase.sol || echo "none found"
echo "--- createCourt definition and body ---"
rg -nP -C10 "function createCourt" -g contracts/src/arbitration/KlerosCoreBase.sol
echo "--- __KlerosCoreBase_initialize implementation ---"
rg -nP -C5 "__KlerosCoreBase_initialize" -g contracts/src/arbitration/KlerosCoreBase.solLength of output: 3595
🏁 Script executed:
#!/bin/bash
set -e
echo "----- createCourt implementation (lines 310–380) -----"
sed -n '310,380p' contracts/src/arbitration/KlerosCoreBase.sol
echo
echo "----- All calls to sortitionModule. within file -----"
rg -n "sortitionModule\." -g contracts/src/arbitration/KlerosCoreBase.sol || echo "none found"Length of output: 5392
Add non-zero address checks to all setters and document sortition module migration
- In
changeGuardian,changePinakion,changeJurorProsecutionModuleandchangeSortitionModule, addand declareif (address(_…Parameter…) == address(0)) revert InvalidAddress();
alongside the other custom errors.error InvalidAddress(); - After swapping in a new
sortitionModule, existing courts must each callas part of your upgrade or migration script—no such initialization currently exists insortitionModule.createTree(courtID, _sortitionExtraData);changeSortitionModule. Document or implement where and how you’ll initialize the new module for all pre-existing courts.
| // [0, 1] value that determines how coherent the juror was in this round, in basis points. | ||
| uint256 degreeOfCoherence = disputeKit.getDegreeOfCoherence( | ||
| uint256 coherence = disputeKit.getDegreeOfCoherencePenalty( | ||
| _params.disputeID, | ||
| _params.round, | ||
| _params.repartition, | ||
| _params.feePerJurorInRound, | ||
| _params.pnkAtStakePerJurorInRound | ||
| ); | ||
| if (degreeOfCoherence > ALPHA_DIVISOR) { | ||
| // Make sure the degree doesn't exceed 1, though it should be ensured by the dispute kit. | ||
| degreeOfCoherence = ALPHA_DIVISOR; | ||
|
|
||
| // Guard against degree exceeding 1, though it should be ensured by the dispute kit. | ||
| if (coherence > ONE_BASIS_POINT) { | ||
| coherence = ONE_BASIS_POINT; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Penalty path: coherence clamp good; consider reentrancy posture.
External calls occur before state sync and token sends (unlock, setStakePenalty, potential ETH send to owner). Add nonReentrant on execute() or move all state writes before external interactions where feasible.
Additionally, message comment says “breaks CEI” near round.pnkPenalties update. If keeping the cached write, guard the whole execute() with nonReentrant (see scaffold below).
Also applies to: 773-786, 797-803, 805-816
| // [0, 1] value that determines how coherent the juror was in this round, in basis points. | ||
| uint256 degreeOfCoherence = disputeKit.getDegreeOfCoherence( | ||
| (uint256 pnkCoherence, uint256 feeCoherence) = disputeKit.getDegreeOfCoherenceReward( | ||
| _params.disputeID, | ||
| _params.round, | ||
| _params.repartition % _params.numberOfVotesInRound, | ||
| _params.feePerJurorInRound, | ||
| _params.pnkAtStakePerJurorInRound | ||
| ); | ||
|
|
||
| // Make sure the degree doesn't exceed 1, though it should be ensured by the dispute kit. | ||
| if (degreeOfCoherence > ALPHA_DIVISOR) { | ||
| degreeOfCoherence = ALPHA_DIVISOR; | ||
| // Guard against degree exceeding 1, though it should be ensured by the dispute kit. | ||
| if (pnkCoherence > ONE_BASIS_POINT) { | ||
| pnkCoherence = ONE_BASIS_POINT; | ||
| } | ||
| if (feeCoherence > ONE_BASIS_POINT) { | ||
| feeCoherence = ONE_BASIS_POINT; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rewards path: coherent split by PNK/fees is correct; minor hardening.
- Clamp to ONE_BASIS_POINT is good.
- Consider reentrancy guard on
execute()due to ETH/WERC20 sends to jurors/owner. - Optional: emit a single “RoundSettled” summary event at end to ease analytics.
Also applies to: 845-855, 857-863, 869-872, 874-893
🤖 Prompt for AI Agents
In contracts/src/arbitration/KlerosCoreBase.sol around lines 827 to 842, the
coherence clamp is fine but the execute() path performs ETH/WERC20 transfers to
jurors/owner and needs reentrancy hardening: add a reentrancy guard (e.g.,
inherit OpenZeppelin ReentrancyGuard and mark execute() nonReentrant or
implement an internal mutex), ensure checks-effects-interactions by performing
all state updates (marking round settled, updating balances/flags) before any
external token/ETH transfers, and emit a single RoundSettled summary event at
the end of the function summarizing amounts and recipients for easier analytics.
| function createTree(uint96 _courtID, bytes memory _extraData) external override onlyByCore { | ||
| TreeKey key = CourtID.wrap(_courtID).toTreeKey(); | ||
| uint256 K = _extraDataToTreeK(_extraData); | ||
| require(tree.K == 0, "Tree already exists."); | ||
| require(K > 1, "K must be greater than one."); | ||
| tree.K = K; | ||
| tree.nodes.push(0); | ||
| sortitionSumTrees.createTree(key, K); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate K ≥ 2 when creating trees (defensive invariant).
Creating a K=0/1 tree breaks sum-tree invariants. Guard before createTree.
function createTree(uint96 _courtID, bytes memory _extraData) external override onlyByCore {
TreeKey key = CourtID.wrap(_courtID).toTreeKey();
uint256 K = _extraDataToTreeK(_extraData);
+ if (K < 2) revert KMustBeGreaterThanOne();
sortitionSumTrees.createTree(key, K);
}Add error:
error NotDrawingPhase();
+error KMustBeGreaterThanOne();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function createTree(uint96 _courtID, bytes memory _extraData) external override onlyByCore { | |
| TreeKey key = CourtID.wrap(_courtID).toTreeKey(); | |
| uint256 K = _extraDataToTreeK(_extraData); | |
| require(tree.K == 0, "Tree already exists."); | |
| require(K > 1, "K must be greater than one."); | |
| tree.K = K; | |
| tree.nodes.push(0); | |
| sortitionSumTrees.createTree(key, K); | |
| } | |
| // … elsewhere in the file, alongside other error declarations | |
| error NotDrawingPhase(); | |
| error KMustBeGreaterThanOne(); | |
| function createTree(uint96 _courtID, bytes memory _extraData) external override onlyByCore { | |
| TreeKey key = CourtID.wrap(_courtID).toTreeKey(); | |
| uint256 K = _extraDataToTreeK(_extraData); | |
| if (K < 2) revert KMustBeGreaterThanOne(); | |
| sortitionSumTrees.createTree(key, K); | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/SortitionModuleBase.sol around lines 185 to 189,
the computed tree arity K from _extraDataToTreeK(_extraData) is not validated,
allowing K=0 or K=1 which breaks sum-tree invariants; add a guard immediately
after computing K to require K >= 2 and revert with a clear error (either
require(K >= 2, "K must be >= 2") or a custom error) before calling
sortitionSumTrees.createTree(key, K) so creation never proceeds with invalid K.
| function setStakePenalty( | ||
| address _account, | ||
| uint96 _courtID, | ||
| uint256 _penalty | ||
| ) external override onlyByCore returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty) { | ||
| Juror storage juror = jurors[_account]; | ||
| availablePenalty = _penalty; | ||
| newCourtStake = _stakeOf(_account, _courtID); | ||
| if (juror.stakedPnk < _penalty) { | ||
| availablePenalty = juror.stakedPnk; | ||
| } | ||
|
|
||
| if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply. | ||
|
|
||
| uint256 currentStake = _stakeOf(_account, _courtID); | ||
| uint256 newStake = 0; | ||
| if (currentStake >= availablePenalty) { | ||
| newStake = currentStake - availablePenalty; | ||
| } | ||
| _setStake(_account, _courtID, 0, availablePenalty, newStake); | ||
| pnkBalance = juror.stakedPnk; // updated by _setStake() | ||
| newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix penalty application to preserve accounting invariants and return remainder.
Current code withdraws _penalty from stakedPnk even when the court stake is smaller, leaving stakedPnk < sum of per-court stakes and returning an availablePenalty that equals the applied amount, not the remainder. Clamp applied penalty to the lesser of (a) total balance available, (b) the court stake; then return the leftover for upstream cascading.
- ) external override onlyByCore returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty) {
- Juror storage juror = jurors[_account];
- availablePenalty = _penalty;
- newCourtStake = _stakeOf(_account, _courtID);
- if (juror.stakedPnk < _penalty) {
- availablePenalty = juror.stakedPnk;
- }
-
- if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply.
-
- uint256 currentStake = _stakeOf(_account, _courtID);
- uint256 newStake = 0;
- if (currentStake >= availablePenalty) {
- newStake = currentStake - availablePenalty;
- }
- _setStake(_account, _courtID, 0, availablePenalty, newStake);
- pnkBalance = juror.stakedPnk; // updated by _setStake()
- newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake()
- }
+ ) external override onlyByCore returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty) {
+ Juror storage juror = jurors[_account];
+ // Max we can take overall in this tx
+ uint256 maxApplicable = _penalty <= juror.stakedPnk ? _penalty : juror.stakedPnk;
+ uint256 currentStake = _stakeOf(_account, _courtID);
+ // Apply only what this court can cover
+ uint256 applied = currentStake <= maxApplicable ? currentStake : maxApplicable;
+ if (applied == 0) {
+ return (juror.stakedPnk, currentStake, maxApplicable); // nothing applied here; leftover equals maxApplicable
+ }
+ uint256 newStake = currentStake - applied;
+ _setStake(_account, _courtID, 0, applied, newStake);
+ pnkBalance = juror.stakedPnk; // updated by _setStake()
+ newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake()
+ availablePenalty = maxApplicable - applied; // remainder to be applied to other courts upstream
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function setStakePenalty( | |
| address _account, | |
| uint96 _courtID, | |
| uint256 _penalty | |
| ) external override onlyByCore returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty) { | |
| Juror storage juror = jurors[_account]; | |
| availablePenalty = _penalty; | |
| newCourtStake = _stakeOf(_account, _courtID); | |
| if (juror.stakedPnk < _penalty) { | |
| availablePenalty = juror.stakedPnk; | |
| } | |
| if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply. | |
| uint256 currentStake = _stakeOf(_account, _courtID); | |
| uint256 newStake = 0; | |
| if (currentStake >= availablePenalty) { | |
| newStake = currentStake - availablePenalty; | |
| } | |
| _setStake(_account, _courtID, 0, availablePenalty, newStake); | |
| pnkBalance = juror.stakedPnk; // updated by _setStake() | |
| newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake() | |
| } | |
| function setStakePenalty( | |
| address _account, | |
| uint96 _courtID, | |
| uint256 _penalty | |
| ) external override onlyByCore returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty) { | |
| Juror storage juror = jurors[_account]; | |
| // Max we can take overall in this tx | |
| uint256 maxApplicable = _penalty <= juror.stakedPnk ? _penalty : juror.stakedPnk; | |
| uint256 currentStake = _stakeOf(_account, _courtID); | |
| // Apply only what this court can cover | |
| uint256 applied = currentStake <= maxApplicable ? currentStake : maxApplicable; | |
| if (applied == 0) { | |
| return (juror.stakedPnk, currentStake, maxApplicable); // nothing applied here; leftover equals maxApplicable | |
| } | |
| uint256 newStake = currentStake - applied; | |
| _setStake(_account, _courtID, 0, applied, newStake); | |
| pnkBalance = juror.stakedPnk; // updated by _setStake() | |
| newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake() | |
| availablePenalty = maxApplicable - applied; // remainder to be applied to other courts upstream | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/university/SortitionModuleUniversity.sol around
lines 197-219, the function currently withdraws the full requested _penalty from
stakedPnk regardless of the court-specific stake, breaking accounting and
returning the wrong availablePenalty; fix by computing an appliedPenalty =
min(_penalty, juror.stakedPnk, currentStake) (clamp to available PNK and the
court stake), call _setStake to withdraw only appliedPenalty from the
juror/court, then set pnkBalance = juror.stakedPnk and newCourtStake =
_stakeOf(_account, _courtID) after the update, and return availablePenalty =
_penalty - appliedPenalty so the remainder can cascade upstream.
| // While it still has children | ||
| uint256 treeIndex = 0; | ||
| while ((_tree.K * treeIndex) + 1 < _tree.nodes.length) { | ||
| for (uint256 i = 1; i <= _tree.K; i++) { | ||
| // Loop over children. | ||
| uint256 nodeIndex = (_tree.K * treeIndex) + i; | ||
| uint256 nodeValue = _tree.nodes[nodeIndex]; | ||
|
|
||
| if (currentDrawnNumber >= nodeValue) { | ||
| // Go to the next child. | ||
| currentDrawnNumber -= nodeValue; | ||
| } else { | ||
| // Pick this child. | ||
| treeIndex = nodeIndex; | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
draw(): out-of-bounds read risk and potential infinite loop.
Iterating i=1..K unconditionally reads children that may not exist yet (nodeIndex >= nodes.length), which reverts. If no child consumes the draw, the while loop can spin. Fix by bounds-checking each child and failing safely.
- while ((_tree.K * treeIndex) + 1 < _tree.nodes.length) {
- for (uint256 i = 1; i <= _tree.K; i++) {
- // Loop over children.
- uint256 nodeIndex = (_tree.K * treeIndex) + i;
- uint256 nodeValue = _tree.nodes[nodeIndex];
-
- if (currentDrawnNumber >= nodeValue) {
- // Go to the next child.
- currentDrawnNumber -= nodeValue;
- } else {
- // Pick this child.
- treeIndex = nodeIndex;
- break;
- }
- }
- }
+ while ((_tree.K * treeIndex) + 1 < _tree.nodes.length) {
+ bool picked = false;
+ for (uint256 i = 1; i <= _tree.K; i++) {
+ uint256 nodeIndex = (_tree.K * treeIndex) + i;
+ if (nodeIndex >= _tree.nodes.length) continue; // missing child slot => value 0
+ uint256 nodeValue = _tree.nodes[nodeIndex];
+ if (currentDrawnNumber < nodeValue) {
+ treeIndex = nodeIndex;
+ picked = true;
+ break;
+ }
+ currentDrawnNumber -= nodeValue;
+ }
+ if (!picked) revert InvalidTreeState(); // defensive: sums must match
+ }Add the new error:
error TreeAlreadyExists();
error KMustBeGreaterThanOne();
+ error InvalidTreeState();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // While it still has children | |
| uint256 treeIndex = 0; | |
| while ((_tree.K * treeIndex) + 1 < _tree.nodes.length) { | |
| for (uint256 i = 1; i <= _tree.K; i++) { | |
| // Loop over children. | |
| uint256 nodeIndex = (_tree.K * treeIndex) + i; | |
| uint256 nodeValue = _tree.nodes[nodeIndex]; | |
| if (currentDrawnNumber >= nodeValue) { | |
| // Go to the next child. | |
| currentDrawnNumber -= nodeValue; | |
| } else { | |
| // Pick this child. | |
| treeIndex = nodeIndex; | |
| break; | |
| } | |
| } | |
| } | |
| // While it still has children | |
| uint256 treeIndex = 0; | |
| while ((_tree.K * treeIndex) + 1 < _tree.nodes.length) { | |
| bool picked = false; | |
| for (uint256 i = 1; i <= _tree.K; i++) { | |
| uint256 nodeIndex = (_tree.K * treeIndex) + i; | |
| if (nodeIndex >= _tree.nodes.length) continue; // missing child slot ⇒ value 0 | |
| uint256 nodeValue = _tree.nodes[nodeIndex]; | |
| if (currentDrawnNumber < nodeValue) { | |
| // Pick this child. | |
| treeIndex = nodeIndex; | |
| picked = true; | |
| break; | |
| } | |
| // Otherwise subtract and continue. | |
| currentDrawnNumber -= nodeValue; | |
| } | |
| if (!picked) revert InvalidTreeState(); // defensive: sums must match | |
| } |
| // While it still has children | |
| uint256 treeIndex = 0; | |
| while ((_tree.K * treeIndex) + 1 < _tree.nodes.length) { | |
| for (uint256 i = 1; i <= _tree.K; i++) { | |
| // Loop over children. | |
| uint256 nodeIndex = (_tree.K * treeIndex) + i; | |
| uint256 nodeValue = _tree.nodes[nodeIndex]; | |
| if (currentDrawnNumber >= nodeValue) { | |
| // Go to the next child. | |
| currentDrawnNumber -= nodeValue; | |
| } else { | |
| // Pick this child. | |
| treeIndex = nodeIndex; | |
| break; | |
| } | |
| } | |
| } | |
| error TreeAlreadyExists(); | |
| error KMustBeGreaterThanOne(); | |
| error InvalidTreeState(); |
🤖 Prompt for AI Agents
In contracts/src/libraries/SortitionTrees.sol around lines 66 to 83, the loop
unconditionally indexes children and can read past nodes.length or loop forever
if no child consumes the draw; add a bounds check before computing/reading
nodeValue (skip or treat missing child as zero), and if you detect nodeIndex >=
_tree.nodes.length then revert with a new descriptive error (e.g.,
NoChildAtIndex) or handle by breaking out safely; ensure the while loop can
always exit by either selecting a valid child or reverting when no children
exist; also add the new error declaration near the top of the contract/library.
| const nodes = await sortitionTree.getTreeNodes(testCase.courtID); | ||
| expect(nodes.length).to.equal(1); | ||
| expect(nodes[0]).to.equal(0); // Root starts at 0 | ||
|
|
||
| const stack = await sortitionTree.getTreeStack(testCase.courtID); | ||
| expect(stack.length).to.equal(0); // Empty stack initially | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix BigInt vs number mismatches in assertions (ethers v6 returns BigInt).
Many contract reads return BigInt; several expects compare to JS numbers, causing fragile or failing tests. Prefer 0n/100n or cast with BigInt()/Number() consistently.
Examples below; please apply the same pattern across the file.
@@
- expect(nodes[0]).to.equal(0); // Root starts at 0
+ expect(nodes[0]).to.equal(0n); // Root starts at 0
- expect(await sortitionTree.stakeOf(0, juror)).to.equal(stake);
- expect(await sortitionTree.getRootSum(0)).to.equal(stake);
+ expect(await sortitionTree.stakeOf(0, juror)).to.equal(BigInt(stake));
+ expect(await sortitionTree.getRootSum(0)).to.equal(BigInt(stake));
- const expectedTotal = stakes.reduce((sum, stake) => sum + stake, 0);
+ const expectedTotal = stakes.reduce((sum, stake) => sum + BigInt(stake), 0n);
expect(await sortitionTree.getRootSum(0)).to.equal(expectedTotal);
- expect(await sortitionTree.getRootSum(0)).to.equal(200 + newStake);
+ expect(await sortitionTree.getRootSum(0)).to.equal(200n + BigInt(newStake));
- expect(await sortitionTree.getNodeIndex(0, juror)).to.equal(0); // Should be cleared
+ expect(await sortitionTree.getNodeIndex(0, juror)).to.equal(0n); // Should be cleared
- expect(await sortitionTree.stakeOf(0, juror4)).to.equal(150);
+ expect(await sortitionTree.stakeOf(0, juror4)).to.equal(150n);
- expect(courtID).to.equal(0);
+ expect(Number(courtID)).to.equal(0);
- const stakes = await sortitionTree.getStakesAcrossCourts(juror, courtIDs);
- for (const retrievedStake of stakes) {
- expect(retrievedStake).to.equal(stake);
- }
+ const stakes = await sortitionTree.getStakesAcrossCourts(juror, courtIDs);
+ for (const retrievedStake of stakes) {
+ expect(retrievedStake).to.equal(BigInt(stake));
+ }
- expect(await sortitionTree.getRootSum(0)).to.equal(700);
+ expect(await sortitionTree.getRootSum(0)).to.equal(700n);Also applies to: 144-151, 162-165, 179-181, 191-204, 205-213, 221-224, 241-244, 268-270, 280-283, 289-295, 309-314, 331-334, 341-353, 375-378, 390-396, 411-415, 445-446, 475-479, 556-559, 569-587, 609-614, 677-681, 692-695
🤖 Prompt for AI Agents
In contracts/test/sortition/index.ts around lines 99-105 (and also update the
ranges 144-151, 162-165, 179-181, 191-204, 205-213, 221-224, 241-244, 268-270,
280-283, 289-295, 309-314, 331-334, 341-353, 375-378, 390-396, 411-415, 445-446,
475-479, 556-559, 569-587, 609-614, 677-681, 692-695) assertions compare BigInt
results from ethers v6 to JS numbers; change those expectations to use BigInt
literals (e.g., 0n, 100n) or cast the contract return explicitly with
BigInt()/Number() consistently so types match. For each expect(...)
.to.equal(...) replace numeric RHS with BigInt literals or wrap the LHS in
Number(...) if you intend JS numbers, ensuring consistency across all listed
lines, and update any length/element comparisons likewise to compare BigInt to
BigInt or numbers to numbers.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
39-55: Add zero-address validation for _poh and _wNative.
Prevent silent misconfiguration by validating inputs before calling the base initializer.function initialize( address _owner, KlerosCore _core, IProofOfHumanity _poh, address _wNative, uint256 _jumpDisputeKitID ) external reinitializer(1) { + require(address(_poh) != address(0), "DKSR: POH zero"); + require(_wNative != address(0), "DKSR: wNative zero"); __DisputeKitClassicBase_initialize(_owner, _core, _wNative, _jumpDisputeKitID); poh = _poh; singleDrawPerJuror = true; }Confirmed
reinitializer(1)does not conflict with any base or sibling initializers.contracts/deploy/00-home-chain-arbitration.ts (1)
142-146: Fix dependency tag: script depends on RNGWithFallback, not ChainlinkRNGThis deploy retrieves RNGWithFallback (Line 53) and will fail if it hasn’t been deployed yet. Update the dependency tag to ensure ordering.
-deployArbitration.dependencies = ["ChainlinkRNG"]; +deployArbitration.dependencies = ["RNGWithFallback"];
♻️ Duplicate comments (9)
contracts/CHANGELOG.md (3)
30-30: Clarify pragma widening scope and exact range.Reflect actual range and include RNG contracts if applicable.
- Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083)) + Widen pragma to `>=0.8.0 <0.9.0` for interfaces and RNG contracts ([#2083](https://github.com/kleros/kleros-v2/issues/2083))
32-32: Fix constant name and wording.Use singular and a clearer action verb.
- Consolidate the constant `ALPHA_DIVISOR` with `ONE_BASIS_POINTS` ([#2090](https://github.com/kleros/kleros-v2/issues/2090)) + Replace `ALPHA_DIVISOR` with `ONE_BASIS_POINT` ([#2090](https://github.com/kleros/kleros-v2/issues/2090))
158-159: Remove unpublished 0.13.0 release link to avoid 404s.Keep the anchor commented until the tag is live.
-[0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0 +<!-- [0.13.0]: (add after publishing) -->Run to confirm availability before uncommenting:
#!/usr/bin/env bash curl -s -o /dev/null -w "%{http_code}\n" \ "https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0"contracts/src/arbitration/interfaces/IDisputeKit.sol (1)
83-96: Fix NatSpec: “reward” → “penalty” (still unresolved).The @return text for the penalty accessor incorrectly says “reward”.
-/// @return pnkCoherence The degree of coherence in basis points for the dispute PNK reward. +/// @return pnkCoherence The degree of coherence in basis points for the dispute PNK penalty. function getDegreeOfCoherencePenalty(contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
138-141: Guard against inactive/unknown disputes in notJumped.Accessing disputes[coreDisputeIDToLocal[_coreDisputeID]] without an active check can read an uninitialized struct and underflow on rounds.length - 1. Add the active guard.
modifier notJumped(uint256 _coreDisputeID) { + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); _; }contracts/src/arbitration/KlerosCoreBase.sol (4)
758-773: Coherence clamping is good; consider adding reentrancy protection.The penalty execution correctly clamps coherence to ONE_BASIS_POINT and properly calculates penalties. However, the function performs external calls before completing all state updates.
Based on the team's preference for gas optimization (as noted in learnings), the current implementation is acceptable. The comment at line 746 acknowledges the CEI pattern break. Consider adding a
nonReentrantmodifier to the mainexecute()function if reentrancy becomes a concern.
273-280: Add event emission and consider reentrancy protection for owner's arbitrary calls.This function allows arbitrary external calls with ETH value. While the owner is trusted, it's good practice to emit an event for auditability and consider reentrancy protection.
+ event OwnerCallExecuted(address indexed destination, uint256 value, bytes data, bool success); function executeOwnerProposal(address _destination, uint256 _amount, bytes memory _data) external onlyByOwner { (bool success, ) = _destination.call{value: _amount}(_data); if (!success) revert UnsuccessfulCall(); + emit OwnerCallExecuted(_destination, _amount, _data, success); }
282-286: Validate owner address and emit ownership change event.The function should prevent setting a zero address (unless renouncement is intended) and emit an event for transparency.
+ event OwnerChanged(address indexed previousOwner, address indexed newOwner); function changeOwner(address payable _owner) external onlyByOwner { + if (_owner == address(0)) revert InvalidAddress(); + address previousOwner = owner; owner = _owner; + emit OwnerChanged(previousOwner, _owner); }Additionally, define the error:
error InvalidAddress();
288-311: Add zero address validation for critical setter functions.These setter functions should validate against zero addresses to prevent accidental misconfigurations.
function changeGuardian(address _guardian) external onlyByOwner { + if (_guardian == address(0)) revert InvalidAddress(); guardian = _guardian; } function changePinakion(IERC20 _pinakion) external onlyByOwner { + if (address(_pinakion) == address(0)) revert InvalidAddress(); pinakion = _pinakion; } function changeJurorProsecutionModule(address _jurorProsecutionModule) external onlyByOwner { + if (_jurorProsecutionModule == address(0)) revert InvalidAddress(); jurorProsecutionModule = _jurorProsecutionModule; } function changeSortitionModule(ISortitionModule _sortitionModule) external onlyByOwner { + if (address(_sortitionModule) == address(0)) revert InvalidAddress(); sortitionModule = _sortitionModule; }Note: When changing the sortition module, ensure all existing courts are properly initialized with the new module through migration scripts.
🧹 Nitpick comments (19)
contracts/CHANGELOG.md (5)
22-22: Spelling: “bypass” (no hyphen).- Allow stake changes to by-pass delayed stakes when initiated by the SortitionModule by setting the `_noDelay` parameter to `true` in `SortitionModule.validateStake()` ([#2107](https://github.com/kleros/kleros-v2/issues/2107)) + Allow stake changes to bypass delayed stakes when initiated by the SortitionModule by setting `_noDelay` to `true` in `SortitionModule.validateStake()` ([#2107](https://github.com/kleros/kleros-v2/issues/2107))
25-25: Clarify what switches to timestamps.Blockhash requires block numbers; if the timeout calculation (not the hash lookup) is what moved to timestamps, say so to avoid confusion.
- Use `block.timestamp` rather than `block.number` for `BlockhashRNG` for better reliability on Arbitrum as block production is sporadic depending on network conditions. ([#2054](https://github.com/kleros/kleros-v2/issues/2054)) + Use timestamp-based timeouts instead of block-number-based ones in `BlockhashRNG` to improve reliability on Arbitrum, where block production can be sporadic. ([#2054](https://github.com/kleros/kleros-v2/issues/2054))
43-43: Grammar/clarity: “advance to the Voting period.”- Do not pass to Voting period if all the commits are cast because it breaks the current Shutter auto-reveal process. ([#2085](https://github.com/kleros/kleros-v2/issues/2085)) + Do not advance to the Voting period when all commits are already cast; this preserves the current Shutter auto‑reveal process. ([#2085](https://github.com/kleros/kleros-v2/issues/2085))
15-15: Style + canonical ERC link.Concise phrasing and link to the official EIP page.
- **Breaking:** Rename `governor` to `owner` in order to comply with the lightweight ownership standard [ERC-5313](https://eipsinsight.com/ercs/erc-5313) ([#2112](https://github.com/kleros/kleros-v2/issues/2112)) + **Breaking:** Rename `governor` to `owner` to comply with [ERC‑5313](https://eips.ethereum.org/EIPS/eip-5313) ([#2112](https://github.com/kleros/kleros-v2/issues/2112))
31-31: Confirm whether a separate penalty function/API was added.AI notes suggest splitting coherence into reward/penalty functions. If true, document it explicitly here.
Proposed addendum:
- Add
getDegreeOfCoherencePenalty()(separate penalty path) and align docs accordingly (#2090)contracts/src/arbitration/interfaces/IDisputeKit.sol (2)
46-55: Add missing NatSpec for new return value in draw.Document fromSubcourtID to match the expanded signature.
/// @param _nonce Nonce. -/// @return drawnAddress The drawn address. +/// @return drawnAddress The drawn address. +/// @return fromSubcourtID The originating subcourt ID of the drawn juror. function draw(
124-132: Confirm TODO or remove param.The _previousDisputeKit parameter is marked TODO for removal. Either remove it now (breaking change) or track a follow-up to deprecate it in the next major.
contracts/deploy/00-home-chain-arbitration-neo.ts (1)
39-44: Nonce-based precompute: add a sanity check.Given the reliance on nonce ordering, assert the deployed KlerosCoreNeo address equals the precomputed klerosCoreAddress after deployment to fail fast if nonces shift.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)
168-175: Emit event on successful owner call (optional).Add an event to audit privileged calls.
function executeOwnerProposal(address _destination, uint256 _amount, bytes memory _data) external onlyByOwner { (bool success, ) = _destination.call{value: _amount}(_data); if (!success) revert UnsuccessfulCall(); + emit OwnerCallExecuted(_destination, _amount, _data); }Add this event in the events section:
event OwnerCallExecuted(address indexed destination, uint256 value, bytes data);
177-194: Ownership/admin setters: add events and align types.
- changeOwner takes address payable but stores address; make the param address for consistency.
- Emit events for changeOwner/changeCore/changeJumpDisputeKitID.
-/// @param _owner The new value for the `owner` storage variable. -function changeOwner(address payable _owner) external onlyByOwner { - owner = _owner; -} +/// @param _owner The new value for the `owner` storage variable. +function changeOwner(address _owner) external onlyByOwner { + owner = _owner; + emit OwnerChanged(_owner); +} - function changeCore(address _core) external onlyByOwner { - core = KlerosCore(_core); -} + function changeCore(address _core) external onlyByOwner { + core = KlerosCore(_core); + emit CoreChanged(_core); +} - function changeJumpDisputeKitID(uint256 _jumpDisputeKitID) external onlyByOwner { - jumpDisputeKitID = _jumpDisputeKitID; -} + function changeJumpDisputeKitID(uint256 _jumpDisputeKitID) external onlyByOwner { + jumpDisputeKitID = _jumpDisputeKitID; + emit JumpDisputeKitIDChanged(_jumpDisputeKitID); +}Add events:
event OwnerChanged(address indexed newOwner); event CoreChanged(address indexed newCore); event JumpDisputeKitIDChanged(uint256 indexed newJumpDisputeKitID);
650-656: Fix typo and phrasing in NatSpec (“kid” → “kit”).-/// @dev Returns the dispute kid ID be used after court jump by Kleros Core. +/// @dev Returns the dispute kit ID to be used after a court jump by Kleros Core.contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
45-56: Initializer wiring: OK; consider adding zero-address checks.
Owner/core/wNative/jumpDisputeKitID are forwarded as expected. Add validation to hard-fail on bad params.Apply this diff:
function initialize( address _owner, KlerosCore _core, address _wNative, uint256 _jumpDisputeKitID ) external reinitializer(1) { - __DisputeKitClassicBase_initialize(_owner, _core, _wNative, _jumpDisputeKitID); + require(_owner != address(0), "DKShutter: owner zero"); + require(address(_core) != address(0), "DKShutter: core zero"); + require(_wNative != address(0), "DKShutter: wNative zero"); + __DisputeKitClassicBase_initialize(_owner, _core, _wNative, _jumpDisputeKitID); }contracts/test/foundry/KlerosCore_Voting.t.sol (3)
14-123: Commit/reveal happy-path and guards look solid.
Good coverage of period gating, ownership checks, and hash mismatches.
- Consider asserting nbVoters/choiceCount when fetched to avoid unused locals.
161-270: Direct voting flow: OK; stabilize magic numbers.
Assertions/readability would benefit from named constants for choices (e.g., YES=1, NO=2).Apply this diff snippet at top of the test or within the function:
- vm.prank(staker1); + uint256 constant NO = 2; + vm.prank(staker1); ... - disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ"); + disputeKit.castVote(disputeID, voteIDs, NO, 0, "XYZ");
387-486: Derive newDkID dynamically instead of hard-coding
Replace the hard-coded2with the length getter, e.g.:- uint256 newDkID = 2; + // Use Core’s getter to avoid relying on fixed ordering + uint256 newDkID = core.getDisputeKitsLength() - 1;Drop the TODO once this is in place. Optionally, assert the proxy’s implementation matches
address(dkLogic)after initialization to enforce UUPS invariants.contracts/deploy/00-home-chain-arbitration.ts (1)
44-49: Precomputed KlerosCore address is brittle—add a sanity check after deployIf any extra tx slips in, SortitionModule would be initialized with a wrong core address. Add a post-deploy assert to fail fast.
const core = await hre.ethers.getContract<KlerosCore>("KlerosCore"); + // Sanity check: precomputed vs deployed proxy address must match + if ((klerosCoreAddress ?? "").toLowerCase() !== core.target.toLowerCase()) { + throw new Error( + `KlerosCore address mismatch: precomputed ${klerosCoreAddress} vs deployed ${core.target}` + ); + }contracts/test/foundry/KlerosCore_TestBase.sol (3)
117-124: Prefer abi.encodeCall over abi.encodeWithSignature for safetyStronger type safety and refactor resilience.
- bytes memory initDataDk = abi.encodeWithSignature( - "initialize(address,address,address,uint256)", - owner, - address(proxyCore), - address(wNative), - DISPUTE_KIT_CLASSIC - ); + bytes memory initDataDk = abi.encodeCall( + DisputeKitClassic.initialize, + (owner, address(proxyCore), address(wNative), DISPUTE_KIT_CLASSIC) + ); - bytes memory initDataSm = abi.encodeWithSignature( - "initialize(address,address,uint256,uint256,address)", - owner, - address(proxyCore), - minStakingTime, - maxDrawingTime, - rng - ); + bytes memory initDataSm = abi.encodeCall( + SortitionModuleMock.initialize, + (owner, address(proxyCore), minStakingTime, maxDrawingTime, address(rng)) + ); - bytes memory initDataRegistry = abi.encodeWithSignature("initialize(address)", owner); + bytes memory initDataRegistry = abi.encodeCall(DisputeTemplateRegistry.initialize, (owner));Also applies to: 128-136, 165-167
112-114: Initialize BlockHashRNG with a clear zero consumer, then set itMake the intent explicit and avoid passing an uninitialized variable cast to address.
- rngLookahead = 30; - rng = new BlockHashRNG(msg.sender, address(sortitionModule), rngLookahead); + rngLookahead = 30; + rng = new BlockHashRNG(msg.sender, address(0), rngLookahead); ... vm.prank(owner); rng.changeConsumer(address(sortitionModule));Also applies to: 137-141
188-213: Avoid double external call in _createStandardCourtMicro-opt to store children locally before indexing.
- return uint96(core.getCourtChildren(parent)[core.getCourtChildren(parent).length - 1]); + uint256[] memory children = core.getCourtChildren(parent); + return uint96(children[children.length - 1]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
contracts/CHANGELOG.md(2 hunks)contracts/deploy/00-home-chain-arbitration-neo.ts(6 hunks)contracts/deploy/00-home-chain-arbitration.ts(5 hunks)contracts/src/arbitration/KlerosCoreBase.sol(27 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(14 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol(3 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(4 hunks)contracts/test/foundry/KlerosCore_Appeals.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Initialization.t.sol(1 hunks)contracts/test/foundry/KlerosCore_TestBase.sol(1 hunks)contracts/test/foundry/KlerosCore_Voting.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/test/foundry/KlerosCore_Appeals.t.sol
- contracts/test/foundry/KlerosCore_Initialization.t.sol
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-03T22:48:32.951Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#0
File: :0-0
Timestamp: 2025-09-03T22:48:32.951Z
Learning: In the Kleros v2 codebase, the team prioritizes gas optimization over strict CEI pattern compliance when dealing with trusted contracts. For penalty execution logic, they prefer batching storage writes (`round.pnkPenalties`) rather than updating incrementally after each penalty calculation to save gas costs, as the risk is extremely low between trusted contracts.
Applied to files:
contracts/src/arbitration/KlerosCoreBase.sol
📚 Learning: 2024-11-19T16:09:41.467Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:3-17
Timestamp: 2024-11-19T16:09:41.467Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently.
Applied to files:
contracts/src/arbitration/KlerosCoreBase.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`.
Applied to files:
contracts/src/arbitration/KlerosCoreBase.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
Applied to files:
contracts/deploy/00-home-chain-arbitration-neo.ts
📚 Learning: 2024-12-07T11:39:10.927Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.
Applied to files:
contracts/deploy/00-home-chain-arbitration-neo.tscontracts/deploy/00-home-chain-arbitration.ts
🧬 Code graph analysis (2)
contracts/deploy/00-home-chain-arbitration-neo.ts (1)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)
contracts/deploy/00-home-chain-arbitration.ts (3)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)contracts/deployments/devnet.viem.ts (1)
klerosCoreAddress(12320-12322)contracts/deployments/testnet.viem.ts (1)
klerosCoreAddress(10190-10192)
🪛 LanguageTool
contracts/CHANGELOG.md
[style] ~15-~15: Consider a more concise word here.
Context: ...reaking:** Rename governor to owner in order to comply with the lightweight ownership s...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (39)
contracts/src/arbitration/interfaces/IDisputeKit.sol (3)
3-3: Pragma range LGTM.Compatible with implementing contracts using ^0.8.24.
73-82: Reward coherence API LGTM.Signature and dual return (PNK/fee) are consistent with implementations.
119-123: earlyCourtJump view added — LGTM.Interface matches implementations returning false by default.
contracts/deploy/00-home-chain-arbitration-neo.ts (4)
48-59: RNGWithFallback wiring LGTM.Passing rngWithFallback.target into SortitionModuleNeo matches the new consumer model.
96-101: Consumer update flow LGTM.Idempotent changeConsumer guarded by an equality check.
128-149: DK wiring and ID derivation LGTM.Constructor args and post-add ID computation align with the new jump semantics.
158-160: Keep ChainlinkRNG as the dependency. The00-rng-chainlink.tsscript, tagged"ChainlinkRNG", deploys both the ChainlinkRNG and RNGWithFallback contracts; there is no script tagged"RNGWithFallback".Likely an incorrect or invalid review comment.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
237-257: draw path LGTM.Tuple return and 0-address handling align with ISortitionModule and interface.
765-787: Typed errors LGTM.Clear, specific, and used consistently across code paths.
contracts/src/arbitration/KlerosCoreBase.sol (6)
171-174: LGTM! Consistent governance terminology update.The modifier rename from
onlyByGovernortoonlyByOwneris correctly implemented with the appropriate error check.
176-179: LGTM! Guardian/owner access modifier properly updated.The combined guardian/owner modifier correctly checks both roles and throws the appropriate renamed error.
827-842: Coherence split for PNK and fees is correctly implemented.The rewards calculation properly uses separate coherence values for PNK and fees, both clamped to ONE_BASIS_POINT. This provides more granular control over reward distribution.
856-863: Good implementation of reward staking with fallback mechanism.The code attempts to stake PNK rewards directly through
sortitionModule.setStakeReward()and falls back to a direct transfer if staking fails. This is a thoughtful design that ensures jurors always receive their rewards.
643-651: Court jump logic refactoring looks good.The extraction of court and dispute kit jump logic into
_getCourtAndDisputeKitJumps()improves code organization and maintainability.
1065-1102: Well-structured helper functions for court jumps.The new
_isCourtJumping()and_getCourtAndDisputeKitJumps()functions properly encapsulate the jump logic, making it more testable and maintainable. The fallback to DISPUTE_KIT_CLASSIC when a compatible kit isn't found is a good safety measure.contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (4)
31-31: Version bump is consistent with release.Version correctly updated from 0.12.0 to 0.13.0.
65-72: Initializer properly updated for owner-based governance.The initializer correctly accepts
_ownerinstead of_governorand includes the new_jumpDisputeKitIDparameter for court jump functionality.
74-76: Reinitializer correctly implements jump kit ID update.The reinitializer(10) properly updates the
jumpDisputeKitIDstorage variable, enabling the dispute kit jump functionality.
83-86: Upgrade authorization correctly updated to owner-based control.The
_authorizeUpgradefunction now usesonlyByOwnermodifier, consistent with the governance terminology refactor.contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (4)
30-30: Version bump is consistent across dispute kits.Version correctly updated to 0.13.0, matching other dispute kit implementations.
46-53: Initializer properly implements owner governance and jump kit functionality.The function signature and implementation correctly use
_ownerand include_jumpDisputeKitIDfor court jump support.
55-57: Reinitializer correctly updates jump dispute kit ID.The reinitializer(10) properly sets the
jumpDisputeKitIDvariable.
64-67: Access control properly updated for owner-based upgrades.The
_authorizeUpgradefunction correctly uses theonlyByOwnermodifier.contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (4)
14-14: Version consistently updated across all dispute kits.Version 0.13.0 aligns with the other dispute kit implementations.
30-37: Classic dispute kit initializer properly updated.The initializer correctly implements the owner-based governance model and includes the jump dispute kit functionality.
39-41: Reinitializer implementation is consistent.The reinitializer(10) properly updates the
jumpDisputeKitID.
48-51: Upgrade control properly restricted to owner.The
_authorizeUpgradefunction correctly enforces owner-only access.contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (2)
21-21: Version bump looks good.
Matches the broader SDK/contract versioning in the PR.
61-63: Owner-only upgrade gate: OK.
UUPS authorization correctly updated to owner.contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (2)
15-15: Version bump acknowledged.
Consistent with related DKs.
67-69: Owner-only upgrade gate: OK.
UUPS authorization correctly restricted to owner.contracts/test/foundry/KlerosCore_Voting.t.sol (4)
125-159: Timeout checks for commit period: OK.
Reverts and NewPeriod emission expectations align with timing.
272-298: Vote-period timeout path: OK.
Covers VotePeriodNotPassed and transition to appeal with proper events.
299-327: Ruling check: OK.
Covers (ruling, tied, overridden) tuple after a batch vote.
329-386: Quick pass period on single committed vote: OK.
Good assertion that period advances based on committed/cast completeness.contracts/deploy/00-home-chain-arbitration.ts (2)
90-95: LGTM: consumer wiring uses idempotent changeConsumerChecking rngWithFallback.consumer() before change prevents unnecessary txs and aligns with the new RNGWithFallback API.
53-57: LGTM: SortitionModule now takes RNGWithFallback targetConstructor args reflect the new RNG integration; min/max timing args remain intact.
contracts/test/foundry/KlerosCore_TestBase.sol (2)
103-106: Verify PNK initial supply assumptionsTransferring
totalSupply - 2 ethertomsg.senderpresumes where the initial supply lives. If PNK already mints tomsg.sender, this is a no-op; if not, it may revert. Confirm PNK’s constructor behavior and adjust (mint/deal) accordingly.
142-156: LGTM: Core initialization wiring is consistent with new ownership/RNG modelParameters, DK, sortition module, and WNative wiring look coherent for the updated interfaces.
| /// @dev Gets the degree of coherence of a particular voter. This function is called by Kleros Core in order to determine the amount of the penalty. | ||
| /// @param _coreDisputeID The ID of the dispute in Kleros Core, not in the Dispute Kit. | ||
| /// @param _coreRoundID The ID of the round in Kleros Core, not in the Dispute Kit. | ||
| /// @param _voteID The ID of the vote. | ||
| /// @return pnkCoherence The degree of coherence in basis points for the dispute PNK reward. | ||
| function getDegreeOfCoherencePenalty( | ||
| uint256 _coreDisputeID, | ||
| uint256 _coreRoundID, | ||
| uint256 _voteID, | ||
| uint256 /* _feePerJuror */, | ||
| uint256 /* _pnkAtStakePerJuror */ | ||
| ) external view override returns (uint256 pnkCoherence) { | ||
| return _getDegreeOfCoherence(_coreDisputeID, _coreRoundID, _voteID); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix NatSpec: “reward” → “penalty” in penalty accessor.
-/// @return pnkCoherence The degree of coherence in basis points for the dispute PNK reward.
+/// @return pnkCoherence The degree of coherence in basis points for the dispute PNK penalty.
function getDegreeOfCoherencePenalty(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @dev Gets the degree of coherence of a particular voter. This function is called by Kleros Core in order to determine the amount of the penalty. | |
| /// @param _coreDisputeID The ID of the dispute in Kleros Core, not in the Dispute Kit. | |
| /// @param _coreRoundID The ID of the round in Kleros Core, not in the Dispute Kit. | |
| /// @param _voteID The ID of the vote. | |
| /// @return pnkCoherence The degree of coherence in basis points for the dispute PNK reward. | |
| function getDegreeOfCoherencePenalty( | |
| uint256 _coreDisputeID, | |
| uint256 _coreRoundID, | |
| uint256 _voteID, | |
| uint256 /* _feePerJuror */, | |
| uint256 /* _pnkAtStakePerJuror */ | |
| ) external view override returns (uint256 pnkCoherence) { | |
| return _getDegreeOfCoherence(_coreDisputeID, _coreRoundID, _voteID); | |
| } | |
| /// @dev Gets the degree of coherence of a particular voter. This function is called by Kleros Core in order to determine the amount of the penalty. | |
| /// @param _coreDisputeID The ID of the dispute in Kleros Core, not in the Dispute Kit. | |
| /// @param _coreRoundID The ID of the round in Kleros Core, not in the Dispute Kit. | |
| /// @param _voteID The ID of the vote. | |
| /// @return pnkCoherence The degree of coherence in basis points for the dispute PNK penalty. | |
| function getDegreeOfCoherencePenalty( | |
| uint256 _coreDisputeID, | |
| uint256 _coreRoundID, | |
| uint256 _voteID, | |
| uint256 /* _feePerJuror */, | |
| uint256 /* _pnkAtStakePerJuror */ | |
| ) external view override returns (uint256 pnkCoherence) { | |
| return _getDegreeOfCoherence(_coreDisputeID, _coreRoundID, _voteID); | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol around lines
546 to 560, the NatSpec for getDegreeOfCoherencePenalty incorrectly says "PNK
reward"; update the function comment and @return description to say "PNK
penalty" (or "penalty" instead of "reward") so the documentation matches the
function purpose and name; make no other code changes.
| function reinitialize(uint256 _jumpDisputeKitID) external reinitializer(10) { | ||
| jumpDisputeKitID = _jumpDisputeKitID; | ||
| } |
There was a problem hiding this comment.
Critical: reinitialize is publicly callable; lets anyone set jumpDisputeKitID once.
This is an owner-governed parameter; the first arbitrary caller can lock in an unintended DK ID.
Apply this diff to restrict and reuse base logic (emits/validation if present):
-function reinitialize(uint256 _jumpDisputeKitID) external reinitializer(10) {
- jumpDisputeKitID = _jumpDisputeKitID;
-}
+function reinitialize(uint256 _jumpDisputeKitID)
+ external
+ reinitializer(10)
+ onlyByOwner
+{
+ // Prefer the base mutator if available to keep events/validation consistent
+ // e.g., DisputeKitClassicBase.changeJumpDisputeKitID(uint256)
+ changeJumpDisputeKitID(_jumpDisputeKitID);
+}If changeJumpDisputeKitID is not external, set via an internal helper that mirrors its checks and emits an event.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function reinitialize(uint256 _jumpDisputeKitID) external reinitializer(10) { | |
| jumpDisputeKitID = _jumpDisputeKitID; | |
| } | |
| function reinitialize(uint256 _jumpDisputeKitID) | |
| external | |
| reinitializer(10) | |
| onlyByOwner | |
| { | |
| // Prefer the base mutator if available to keep events/validation consistent | |
| // e.g., DisputeKitClassicBase.changeJumpDisputeKitID(uint256) | |
| changeJumpDisputeKitID(_jumpDisputeKitID); | |
| } |
| /// @dev Returns the dispute kid ID be used after court jump by Kleros Core. | ||
| /// @return The ID of the dispute kit in Kleros Core disputeKits array. | ||
| function getJumpDisputeKitID() external view returns (uint256); | ||
|
|
There was a problem hiding this comment.
Fix typo and phrasing in NatSpec (“kid” → “kit”).
-/// @dev Returns the dispute kid ID be used after court jump by Kleros Core.
+/// @dev Returns the dispute kit ID to be used after a court jump by Kleros Core.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @dev Returns the dispute kid ID be used after court jump by Kleros Core. | |
| /// @return The ID of the dispute kit in Kleros Core disputeKits array. | |
| function getJumpDisputeKitID() external view returns (uint256); | |
| /// @dev Returns the dispute kit ID to be used after a court jump by Kleros Core. | |
| /// @return The ID of the dispute kit in Kleros Core disputeKits array. | |
| function getJumpDisputeKitID() external view returns (uint256); |
🤖 Prompt for AI Agents
In contracts/src/arbitration/interfaces/IDisputeKit.sol around lines 133 to 136,
the NatSpec comment contains a typo and awkward phrasing ("kid" → "kit" and
unclear wording); update the doc comment to read something like: "/// @dev
Returns the dispute kit ID to be used after a court jump by Kleros Core." and
ensure the @return description remains accurate: "/// @return The ID of the
dispute kit in Kleros Core disputeKits array."
| import {KlerosCore_TestBase} from "./KlerosCore_TestBase.sol"; | ||
| import {KlerosCoreBase} from "../../src/arbitration/KlerosCoreBase.sol"; | ||
| import {DisputeKitClassic, DisputeKitClassicBase} from "../../src/arbitration/dispute-kits/DisputeKitClassic.sol"; | ||
| import {IDisputeKit} from "../../src/arbitration/interfaces/IDisputeKit.sol"; | ||
| import {UUPSProxy} from "../../src/proxy/UUPSProxy.sol"; | ||
| import "../../src/libraries/Constants.sol"; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify DisputeKitClassicBase import path.
Base type appears imported from DisputeKitClassic.sol; in other files it lives in DisputeKitClassicBase.sol. Ensure this resolves to the intended symbol.
🏁 Script executed:
#!/bin/bash
# Show where DisputeKitClassicBase is defined and how it's imported across the repo.
rg -nP 'contract\s+DisputeKitClassicBase\b|import\s+\{[^}]*DisputeKitClassicBase' -C2Length of output: 279793
Fix DisputeKitClassicBase import path
In contracts/test/foundry/KlerosCore_Voting.t.sol, replace the combined import from DisputeKitClassic.sol with separate imports—DisputeKitClassic remains in DisputeKitClassic.sol, but DisputeKitClassicBase must come from DisputeKitClassicBase.sol.
-import {DisputeKitClassic, DisputeKitClassicBase} from "../../src/arbitration/dispute-kits/DisputeKitClassic.sol";
+import {DisputeKitClassic} from "../../src/arbitration/dispute-kits/DisputeKitClassic.sol";
+import {DisputeKitClassicBase} from "../../src/arbitration/dispute-kits/DisputeKitClassicBase.sol";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import {KlerosCore_TestBase} from "./KlerosCore_TestBase.sol"; | |
| import {KlerosCoreBase} from "../../src/arbitration/KlerosCoreBase.sol"; | |
| import {DisputeKitClassic, DisputeKitClassicBase} from "../../src/arbitration/dispute-kits/DisputeKitClassic.sol"; | |
| import {IDisputeKit} from "../../src/arbitration/interfaces/IDisputeKit.sol"; | |
| import {UUPSProxy} from "../../src/proxy/UUPSProxy.sol"; | |
| import "../../src/libraries/Constants.sol"; | |
| import {KlerosCore_TestBase} from "./KlerosCore_TestBase.sol"; | |
| import {KlerosCoreBase} from "../../src/arbitration/KlerosCoreBase.sol"; | |
| import {DisputeKitClassic} from "../../src/arbitration/dispute-kits/DisputeKitClassic.sol"; | |
| import {DisputeKitClassicBase} from "../../src/arbitration/dispute-kits/DisputeKitClassicBase.sol"; | |
| import {IDisputeKit} from "../../src/arbitration/interfaces/IDisputeKit.sol"; | |
| import {UUPSProxy} from "../../src/proxy/UUPSProxy.sol"; | |
| import "../../src/libraries/Constants.sol"; |
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Voting.t.sol around lines 4 to 10, the
current combined import pulls DisputeKitClassic and DisputeKitClassicBase from
DisputeKitClassic.sol but DisputeKitClassicBase actually lives in
DisputeKitClassicBase.sol; update the imports so DisputeKitClassic is imported
from ../../src/arbitration/dispute-kits/DisputeKitClassic.sol and
DisputeKitClassicBase is imported from
../../src/arbitration/dispute-kits/DisputeKitClassicBase.sol (keeping other
imports unchanged).
Support for recovery hash in Shutter DK
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
contracts/hardhat.config.ts (2)
16-24: Load dotenv before requiring task scriptsRequire’d task files may read env at import time. Move
dotenv.config()above therequire("./scripts/...")lines to ensure env is available.-import * as dotenv from "dotenv"; +import * as dotenv from "dotenv"; +dotenv.config(); @@ -require("./scripts/populatePolicyRegistry"); -require("./scripts/populateCourts"); +require("./scripts/populatePolicyRegistry"); +require("./scripts/populateCourts"); require("./scripts/changeOwner"); @@ -dotenv.config();
302-306: Watcher path likely wrong (sources are in ./src)
paths.sourcesis"./src"but watcher watches"./contracts". It may miss changes.- files: ["./contracts"], + files: ["./src"],contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
139-146: Bounds-check _voteIDs before indexingcastVoteShutter reads _voteIDs[0] without verifying length, causing an index-out-of-bounds revert instead of the intended EmptyVoteIDs error.
Apply:
function castVoteShutter( uint256 _coreDisputeID, uint256[] calldata _voteIDs, uint256 _choice, uint256 _salt, string memory _justification ) external { + if (_voteIDs.length == 0) revert EmptyVoteIDs(); Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; address juror = dispute.rounds[dispute.rounds.length - 1].votes[_voteIDs[0]].account; ... }contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
123-139: Guard against empty vote set incastVoteShutter.Indexing
_voteIDs[0]will revert with out-of-bounds; add an explicit check and a dedicated error for clarity.Apply:
function castVoteShutter( @@ - ) external { + ) external { + if (_voteIDs.length == 0) revert EmptyVoteSet(); Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; address juror = dispute.rounds[dispute.rounds.length - 1].votes[_voteIDs[0]].account; @@ callerIsJuror = false; }And add the error (see bottom of file diff below).
♻️ Duplicate comments (6)
contracts/CHANGELOG.md (2)
161-161: Remove unpublished 0.13.0 release link (still 404).Tag
@kleros/kleros-v2-contracts@0.13.0is not present as of 2025-09-04; keep this commented until published. (github.com)Apply:
-[0.13.0]: https://github.com/kleros/kleros-v2/releases/tag/@kleros%2Fkleros-v2-contracts@0.13.0 +<!-- [0.13.0]: (add after publishing) -->
26-26: Fix constant name and wording.The constant is
ONE_BASIS_POINT(singular); “replace” is clearer than “consolidate.”-- Consolidate the constant `ALPHA_DIVISOR` with `ONE_BASIS_POINTS` ([#2090](https://github.com/kleros/kleros-v2/issues/2090)) +- Replace `ALPHA_DIVISOR` with `ONE_BASIS_POINT` ([#2090](https://github.com/kleros/kleros-v2/issues/2090))contracts/hardhat.config.ts (1)
35-35: Expose optimizer runs via env; keep default 2000Improves Hardhat/Foundry parity and CI flexibility. This echoes a prior suggestion.
- runs: 2000, + runs: Number(process.env.SOLC_OPTIMIZER_RUNS ?? 2000),contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
547-553: Fix NatSpec: penalty docstring says “reward” (duplicate of prior review)Update @return description to “PNK penalty”.
-/// @return pnkCoherence The degree of coherence in basis points for the dispute PNK reward. +/// @return pnkCoherence The degree of coherence in basis points for the dispute PNK penalty.
138-141: Guard notJumped with “active” check (duplicate of prior review)Add NotActiveForCoreDisputeID before checking jumped to fail fast on inactive disputes.
modifier notJumped(uint256 _coreDisputeID) { - if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); + if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); _; }contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
73-75: Critical:reinitializelacks access control; first caller can setjumpDisputeKitID.Restrict to owner and reuse the base mutator to keep events/validation consistent. This was flagged previously and remains unresolved.
Apply:
-function reinitialize(uint256 _jumpDisputeKitID) external reinitializer(10) { - jumpDisputeKitID = _jumpDisputeKitID; -} +function reinitialize(uint256 _jumpDisputeKitID) + external + reinitializer(10) + onlyByOwner +{ + changeJumpDisputeKitID(_jumpDisputeKitID); +}
🧹 Nitpick comments (23)
contracts/CHANGELOG.md (6)
7-7: Use “Unreleased” header format for not-yet-published entries.Align with Common/Keep a Changelog style.
-## [0.13.0] - 2025-08-07 (Not published yet) +## [0.13.0] - Unreleased
15-15: Tighten phrasing; prefer canonical EIP link.-- **Breaking:** Rename `governor` to `owner` in order to comply with the lightweight ownership standard [ERC-5313](https://eipsinsight.com/ercs/erc-5313) ([#2112](https://github.com/kleros/kleros-v2/issues/2112)) +- **Breaking:** Rename `governor` to `owner` to comply with lightweight ownership standard [ERC‑5313](https://eips.ethereum.org/EIPS/eip-5313) ([#2112](https://github.com/kleros/kleros-v2/issues/2112))
29-29: Clarify pragma widening scope and exact range.Current text says “interfaces only,” but PR scope appears broader. Please confirm and update.
Option A (interfaces only):
- Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083)) + Widen interface pragmas to `>=0.8.0 <0.9.0` ([#2083](https://github.com/kleros/kleros-v2/issues/2083))Option B (interfaces + RNG and others):
- Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083)) + Widen pragmas to `>=0.8.0 <0.9.0` across interfaces (and RNG contracts) ([#2083](https://github.com/kleros/kleros-v2/issues/2083))
20-21: Minor style: spell out “third parties.”- Authenticate the calls to the RNGs to prevent 3rd parties from depleting the Chainlink VRF subscription funds ([#2054](https://github.com/kleros/kleros-v2/issues/2054)) + Authenticate the calls to the RNGs to prevent third parties from depleting the Chainlink VRF subscription funds ([#2054](https://github.com/kleros/kleros-v2/issues/2054))
42-42: Spelling: “bypass” (one word).- Allow stake changes to by-pass delayed stakes when initiated by the SortitionModule by setting the `_noDelay` parameter to `true` in `SortitionModule.validateStake()` ([#2107](https://github.com/kleros/kleros-v2/issues/2107)) + Allow stake changes to bypass delayed stakes when initiated by the SortitionModule by setting the `_noDelay` parameter to `true` in `SortitionModule.validateStake()` ([#2107](https://github.com/kleros/kleros-v2/issues/2107))
46-46: Grammar: “advance to the Voting period.”- Do not pass to Voting period if all the commits are cast because it breaks the current Shutter auto-reveal process. ([#2085](https://github.com/kleros/kleros-v2/issues/2085)) + Do not advance to the Voting period if all commits are cast, as it breaks the current Shutter auto‑reveal process. ([#2085](https://github.com/kleros/kleros-v2/issues/2085))contracts/foundry.toml (2)
5-5: via_ir = true: good default; keep an escape hatch for comparisons.IR pipeline is generally beneficial, but debugging certain optimizer issues is easier with a no-IR profile.
Proposed profiles:
[profile.default] via_ir = true optimizer = true optimizer_runs = 10000 + +[profile.noir] +via_ir = false +optimizer = true +optimizer_runs = 200Build without IR when needed:
FOUNDRY_PROFILE=noir forge build.
6-7: 10k optimizer runs: split profiles to reduce CI time and keep release builds hot.10k can materially slow compiles. Use a fast CI profile and keep 10k for release/gas benchmarking.
[profile.default] solc = "0.8.30" evm_version = "cancun" via_ir = true optimizer = true optimizer_runs = 10000 + +[profile.ci] +optimizer = true +optimizer_runs = 200 +via_ir = true +# Fail on warnings in CI to catch issues early +deny_warnings = true +# Deterministic bytecode for reproducible diffs +bytecode_hash = "none"Use:
FOUNDRY_PROFILE=ci forge build..github/workflows/contracts-testing.yml (5)
49-54: Drop the redundantyarn set versionstep.
corepack prepare yarn@4.9.2 --activateis sufficient;yarn set versionmodifies repo files unnecessarily in CI.- name: Set up corepack (for yarn) run: | corepack enable corepack prepare yarn@4.9.2 --activate - yarn set version 4.9.2
29-44: Validate allowed egress; avoid hard-coded IPs and add common endpoints.
54.185.253.63:443is brittle. Consider removing it and, if needed, addapi.github.com:443andstorage.googleapis.com:443(node-gyp/prebuilds, solc mirrors).allowed-endpoints: > binaries.soliditylang.org:443 + api.github.com:443 classic.yarnpkg.com:443 github.com:443 nightly.yarnpkg.com:443 nodejs.org:443 objects.githubusercontent.com:443 registry.yarnpkg.com:443 registry.npmjs.org:443 - 54.185.253.63:443 + storage.googleapis.com:443
76-79: Double-check test invocation context.If you keep PnP, running from the repo root with a workspace target is often cleaner.
- - name: Run Hardhat tests - run: yarn test - working-directory: contracts + - name: Run Hardhat tests + run: yarn workspace @kleros/kleros-v2-contracts test
26-28: Add job-level concurrency to cancel superseded runs.Saves CI minutes on rapid pushes to the same ref.
hardhat-tests: runs-on: ubuntu-latest + concurrency: + group: hardhat-tests-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
80-144: Track Foundry enablement without large commented blocks.Consider a stub job guarded by
if: falseor a separate workflow to keep YAML tidy and make re-enablement a one-line change.Example:
foundry-tests: if: false runs-on: ubuntu-latest steps: - run: echo "Foundry temporarily disabled"contracts/hardhat.config.ts (1)
31-31: Parameterize EVM version to ease cross-network buildsKeep default “cancun” but allow override via env to unblock networks/tooling that lag.
- evmVersion: "cancun", + evmVersion: process.env.SOLC_EVM_VERSION ?? "cancun",contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (2)
168-181: Consistent encoding for hashes (nit)You mix abi.encode (full commit) and abi.encodePacked (recovery). For fixed-size (uint256,uint256) both are 64 bytes today, but using a single scheme improves auditability and reduces ambiguity.
Consider:
- return keccak256(abi.encodePacked(_choice, _salt)); + return keccak256(abi.encode(_choice, _salt));
215-229: Assembly offsets look correct; add brief invariant commentThe 160-byte length guard matches loads at offsets 128 and 160. Add a short note that layout comes from encode(courtID,minJurors,disputeKitID,packedTokenGateAndFlag,tokenId).
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
177-181: Validate non-zero owner on transfer (optional)Block accidental renouncement-by-zero here; if zero-owner is desired, make it explicit via a separate function.
function changeOwner(address payable _owner) external onlyByOwner { - owner = _owner; + if (_owner == address(0)) revert UnsuccessfulCall(); // or a new ZeroAddress() error + owner = _owner; }contracts/test/arbitration/dispute-kit-shutter.ts (3)
160-178: Read commit period from core instead of hard-coding 300sAvoid drift with deployments that tweak timesPerPeriod; fetch from the contract.
- const timesPerPeriod = [300, 300, 300, 300]; - const commitPeriod = timesPerPeriod[Period.commit]; + const court = await core.courts(courtId); + const commitPeriod = Number(court.timesPerPeriod[Period.commit] ?? 300);
138-149: Avoid assuming disputeId = 0Derive from events or a counter to be robust when tests run with other suites.
- await core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => tx.wait()); - const disputeId = 0; + const tx = await core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }); + const rc = await tx.wait(); + const disputeId = Number(rc!.logs.find((l) => l.fragment?.name === "DisputeCreation")!.args![0]);
180-203: Iterating votes via getVoteInfo: consider bounded iterationUsing try/catch sentinel is fine; optionally read votes.length via getRoundInfo to bound the loop and speed up failures.
- const maxVotes = 10; // Reasonable limit for testing + const [, , , , nbVoters] = await disputeKitShutter.getRoundInfo(disputeId, roundIndex, 0); + const maxVotes = Number(nbVoters);contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (3)
101-121: Minor API hygiene and CEI ordering incastCommitShutter.
- Add an explicit non-empty
_voteIDsguard for clearer errors.- Consider moving
recoveryCommitmentswrites after_castCommitto minimize wasted writes on revert and follow checks-effects-interactions.Apply:
function castCommitShutter( @@ - ) external notJumped(_coreDisputeID) { - if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit(); - - uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; - Dispute storage dispute = disputes[localDisputeID]; - uint256 localRoundID = dispute.rounds.length - 1; - for (uint256 i = 0; i < _voteIDs.length; i++) { - recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit; - } - - // `_castCommit()` ensures that the caller owns the vote - _castCommit(_coreDisputeID, _voteIDs, _commit); + ) external notJumped(_coreDisputeID) { + if (_voteIDs.length == 0) revert EmptyVoteSet(); + if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit(); + + // `_castCommit()` ensures that the caller owns the vote and period is valid + _castCommit(_coreDisputeID, _voteIDs, _commit); + + uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; + Dispute storage dispute = disputes[localDisputeID]; + uint256 localRoundID = dispute.rounds.length - 1; + for (uint256 i = 0; i < _voteIDs.length; i++) { + recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit; + }
192-193: Complementary error for empty vote sets.Define an explicit error for the new guards.
Apply:
- error EmptyRecoveryCommit(); + error EmptyRecoveryCommit(); + error EmptyVoteSet();
28-28: DecouplecallerIsJurorflag from external calls
In castVoteShutter (DisputeKitShutter.sol 133–138) you set a contract-widecallerIsJurorfor the entire_castVotecall, yet_castVoteinvokes external functions (core.disputesandcore.courtsin DisputeKitClassicBase.sol 309–316). If the core contract ever reenters, it could observe unintended juror-path branching in hashVote.
• Compute juror context on the fly instead: passmsg.senderintohashVote/_getExpectedVoteHashrather than relying on a transient storage flag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
.github/workflows/contracts-testing.yml(1 hunks)contracts/CHANGELOG.md(2 hunks)contracts/foundry.toml(1 hunks)contracts/hardhat.config.ts(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(15 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(7 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(7 hunks)contracts/test/arbitration/dispute-kit-shutter.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/test/arbitration/dispute-kit-shutter.ts (1)
contracts/deploy/utils/index.ts (1)
PNK(40-40)
🪛 LanguageTool
contracts/CHANGELOG.md
[style] ~15-~15: Consider a more concise word here.
Context: ...reaking:** Rename governor to owner in order to comply with the lightweight ownership s...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Dependabot
- GitHub Check: hardhat-tests
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: Mend Security Check
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (22)
contracts/CHANGELOG.md (1)
162-162: Confirm 0.12.0 link is live (it is).No change needed; just confirming this anchor works. (github.com)
contracts/foundry.toml (1)
3-4: Pragmas allow 0.8.30; manual Cancun build verification required
- All detected
pragma solidityconstraints (^0.8.24,^0.8.28,>=0.8.0 <0.9.0,^0.8.0) accept compiler version 0.8.30.- CI lacks a functioning
forge; please run locally
forge build -vv --compiler-version 0.8.30 --evm-version cancun
and validate deployments on mainnet, Gnosis, Arbitrum, Sepolia..github/workflows/contracts-testing.yml (1)
45-48: Good pinning and submodules.Pinned SHAs for actions and recursive submodules look solid.
contracts/hardhat.config.ts (1)
32-32: viaIR defaulting via env looks goodSane default with easy opt-out for coverage/perf.
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (3)
37-39: Good: Recovery-commit storage added with clear indexingPer-dispute/round/vote mapping is appropriate and the public getter simplifies tests and tooling.
75-87: Initializer as reinitializer(1): confirm upgrade pathSwitching to reinitializer(1) is fine for upgrades but will brick fresh proxies if an initializer(1) was already used differently. Confirm no proxy instances have consumed version 1 for this DK in production.
231-253: Good: gating check is minimal and gas-efficientERC-1155 vs ERC-20/721 balance checks are clean and read-only; default “no gate” path returns early.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (5)
235-249: LGTM: draw() return surface updatedReturning (drawnAddress, fromSubcourtID) aligns with ISortitionModule.draw; early-return on zero address avoids extra work.
275-287: LGTM: commit flow hardens period/active/ownership checksClear revert reasons and batched updates per vote. Event emission preserved.
321-337: LGTM: vote flow with explicit hidden-votes validationThe HashDoesNotMatchHiddenVoteCommitment and JurorHasToOwnTheVote checks are ordered sensibly; stack-too-deep workaround is clean.
651-657: LGTM: jump DK fallbackGraceful fallback to DISPUTE_KIT_CLASSIC when unset is sensible.
742-754: LGTM: default expected-hash accessorVirtual helper cleanly isolates commit source; enables Shutter overrides.
contracts/test/arbitration/dispute-kit-shutter.ts (3)
36-74: Deterministic RNG + dedicated hidden-vote court: solid setupFixture plus IncrementalNG and explicit court params create a stable testbed.
324-349: Nice: event assertions and state checks cover the happy pathCovers CommitCastShutter and VoteCast plus getVoteInfo state.
574-598: Attack-path test is valuableValidates that non-jurors cannot exploit recovery path.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (7)
15-16: Version constant update looks good.
21-23: Public recoveryCommitments mapping: OK.Clear separation by (localDisputeID, localRoundID, voteID) makes reads straightforward.
81-85: UUPS upgrade gate to owner: OK.
176-186: Expected-hash selection by context looks correct.
3-3: All target networks support EIP-1153 (transient storage)
Ethereum mainnet (Dencun/Cancun), Arbitrum One (ArbOS v20 “Atlas”), Optimism (Ecotone), Base (Ecotone), and Gnosis Chain (Pectra) all include EIP-1153 as of September 4, 2025.
60-71: reinitializer(1) safe to use: DisputeKitClassicBase does not declare any external initializer or reinitializer, so version 1 remains unconsumed and this call will succeed.
156-165: Hashing scheme alignment: verify off-chain commit generators match encodings.Juror-path uses
abi.encodePacked(_choice, _salt)while non-juror usesabi.encode(_choice, _salt, keccak256(bytes(_justification))). Confirm Shutter/off-chain tooling generates_commitand_recoveryCommitwith these exact encodings to avoid silent mismatches.
| permissions: # added using https://github.com/step-security/secure-workflows | ||
| contents: read | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Grant actions: read so the cache can restore.
When you set a restrictive permissions block, unspecified scopes default to none. actions/cache requires actions: read for restore.
-permissions: # added using https://github.com/step-security/secure-workflows
- contents: read
+permissions: # added using https://github.com/step-security/secure-workflows
+ contents: read
+ actions: read📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| permissions: # added using https://github.com/step-security/secure-workflows | |
| contents: read | |
| permissions: # added using https://github.com/step-security/secure-workflows | |
| contents: read | |
| actions: read |
🤖 Prompt for AI Agents
.github/workflows/contracts-testing.yml around lines 19 to 21: the workflow’s
permissions block only sets contents: read but omits actions: read which
prevents actions/cache from restoring; update the permissions block to
explicitly include actions: read (e.g., add "actions: read" alongside contents:
read) so the cache restore step can access the actions permission.
| - name: Cache node modules | ||
| uses: actions/cache@0c907a75c2c80ebcb7f088228285e798b750cf8f # v4.2.1 | ||
| env: | ||
| cache-name: cache-node-modules | ||
| with: | ||
| path: | | ||
| ~/.npm | ||
| **/node_modules | ||
| key: ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}-${{ hashFiles('**/package-lock.json', '**/yarn.lock') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}- | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cache the Yarn Berry artifacts instead of npm/node_modules.
With Yarn 4 (PnP by default), ~/.npm and **/node_modules are often unused. Cache .yarn/cache and install state for real wins.
- - name: Cache node modules
+ - name: Cache Yarn artifacts
uses: actions/cache@0c907a75c2c80ebcb7f088228285e798b750cf8f # v4.2.1
- env:
- cache-name: cache-node-modules
with:
- path: |
- ~/.npm
- **/node_modules
- key: ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}-${{ hashFiles('**/package-lock.json', '**/yarn.lock') }}
- restore-keys: |
- ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}-
+ path: |
+ .yarn/cache
+ .yarn/install-state.gz
+ key: ${{ runner.os }}-yarn-${{ secrets.CACHE_VERSION }}-${{ hashFiles('**/yarn.lock', '.yarnrc.yml') }}
+ restore-keys: |
+ ${{ runner.os }}-yarn-${{ secrets.CACHE_VERSION }}-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Cache node modules | |
| uses: actions/cache@0c907a75c2c80ebcb7f088228285e798b750cf8f # v4.2.1 | |
| env: | |
| cache-name: cache-node-modules | |
| with: | |
| path: | | |
| ~/.npm | |
| **/node_modules | |
| key: ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}-${{ hashFiles('**/package-lock.json', '**/yarn.lock') }} | |
| restore-keys: | | |
| ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}- | |
| - name: Cache Yarn artifacts | |
| uses: actions/cache@0c907a75c2c80ebcb7f088228285e798b750cf8f # v4.2.1 | |
| with: | |
| path: | | |
| .yarn/cache | |
| .yarn/install-state.gz | |
| key: ${{ runner.os }}-yarn-${{ secrets.CACHE_VERSION }}-${{ hashFiles('**/yarn.lock', '.yarnrc.yml') }} | |
| restore-keys: | | |
| ${{ runner.os }}-yarn-${{ secrets.CACHE_VERSION }}- |
| - name: Install contracts dependencies | ||
| run: yarn workspace @kleros/kleros-v2-contracts install | ||
|
|
There was a problem hiding this comment.
Fix install step: Yarn v4 doesn’t support yarn workspace <name> install.
This will either no-op or fail unless the workspace defines an install script. Do a root install (Berry) and optionally focus the contracts workspace.
- - name: Install contracts dependencies
- run: yarn workspace @kleros/kleros-v2-contracts install
+ - name: Install dependencies
+ run: |
+ yarn install --immutable
+ yarn workspaces focus @kleros/kleros-v2-contracts📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Install contracts dependencies | |
| run: yarn workspace @kleros/kleros-v2-contracts install | |
| - name: Install dependencies | |
| run: | | |
| yarn install --immutable | |
| yarn workspaces focus @kleros/kleros-v2-contracts |
🤖 Prompt for AI Agents
In .github/workflows/contracts-testing.yml around lines 73-75, the workflow uses
"yarn workspace @kleros/kleros-v2-contracts install", which fails under Yarn v4;
replace that step with a root install (e.g., "yarn install" or "yarn install
--immutable" for CI) and, if you want to limit deps to the contracts workspace,
follow with a workspace focus command such as "yarn workspaces focus
@kleros/kleros-v2-contracts --all" (or similar focus flags) so dependencies are
prepared for that specific workspace.
| @@ -26,28 +26,13 @@ const config: HardhatUserConfig = { | |||
| solidity: { | |||
| compilers: [ | |||
| { | |||
| version: "0.8.28", | |||
| version: "0.8.30", | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Solc 0.8.30: align across toolchains
Ensure Foundry uses the same solc_version to avoid bytecode/gas drift.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Hardhat =="
rg -n 'version:\s*"0\.8\.\d+"' contracts/hardhat.config.ts -n
echo
echo "== Foundry configs =="
fd -a "foundry.toml" | while read -r f; do
echo ">>> $f"
rg -n 'solc_version|evm_version|optimizer_runs|via_ir' "$f" || true
doneLength of output: 522
Specify solc_version in Foundry config
Add solc_version = "0.8.30" to contracts/foundry.toml to match Hardhat’s compiler version and prevent bytecode/gas drift.
🤖 Prompt for AI Agents
In contracts/foundry.toml (add a new or update existing [profile] section), set
solc_version = "0.8.30" to match the Hardhat compiler version declared in
contracts/hardhat.config.ts at line 29; update or insert the solc_version key
under the appropriate profile or the global config so Foundry uses 0.8.30 and
prevents bytecode/gas drift between toolchains.
| // * Transient Storage * // | ||
| // ************************************* // | ||
|
|
||
| bool transient callerIsJuror; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid transient “callerIsJuror” across external calls; pass explicit context instead
Using a transient flag that influences hashVote/_getExpectedVoteHash across external calls (core.disputes/courts) is fragile and reentrancy-prone. Prefer plumbing explicit context to hashing/expected-hash logic to avoid cross-call leakage.
Apply this refactor:
- bool transient callerIsJuror;
+// Remove transient state; compute juror context per-call.
- function castVoteShutter(...) external {
+ function castVoteShutter(...) external {
Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]];
address juror = dispute.rounds[dispute.rounds.length - 1].votes[_voteIDs[0]].account;
-
- callerIsJuror = juror == msg.sender;
- _castVote(_coreDisputeID, _voteIDs, _choice, _salt, _justification, juror);
- callerIsJuror = false;
+ bool isJuror = (juror == msg.sender);
+ _castVoteWithContext(_coreDisputeID, _voteIDs, _choice, _salt, _justification, juror, isJuror);
}
- function hashVote(uint256 _choice, uint256 _salt, string memory _justification)
- public view override returns (bytes32) {
- if (callerIsJuror) { return keccak256(abi.encodePacked(_choice, _salt)); }
- ...
- }
+function _hashVoteWithContext(
+ uint256 _choice, uint256 _salt, string memory _justification, bool isJuror
+) internal pure returns (bytes32) {
+ if (isJuror) return keccak256(abi.encodePacked(_choice, _salt));
+ bytes32 justificationHash = keccak256(bytes(_justification));
+ return keccak256(abi.encode(_choice, _salt, justificationHash));
+}
+
+function hashVote(uint256 _choice, uint256 _salt, string memory _justification)
+ public view override returns (bytes32) {
+ // Public helper (no juror context): default to non-juror hashing.
+ bytes32 justificationHash = keccak256(bytes(_justification));
+ return keccak256(abi.encode(_choice, _salt, justificationHash));
+}
- function _getExpectedVoteHash(...) internal view override returns (bytes32) {
- if (callerIsJuror) { return recoveryCommitments[_localDisputeID][_localRoundID][_voteID]; }
- return disputes[_localDisputeID].rounds[_localRoundID].votes[_voteID].commit;
- }
+function _getExpectedVoteHashWithContext(
+ uint256 _localDisputeID, uint256 _localRoundID, uint256 _voteID, bool isJuror
+) internal view returns (bytes32) {
+ return isJuror
+ ? recoveryCommitments[_localDisputeID][_localRoundID][_voteID]
+ : disputes[_localDisputeID].rounds[_localRoundID].votes[_voteID].commit;
+}
+function _castVoteWithContext(
+ uint256 _coreDisputeID,
+ uint256[] calldata _voteIDs,
+ uint256 _choice,
+ uint256 _salt,
+ string memory _justification,
+ address _juror,
+ bool isJuror
+) internal {
+ // copy body of _castVote but replace:
+ // bytes32 actualVoteHash = hashVote(...);
+ // with:
+ // bytes32 actualVoteHash = _hashVoteWithContext(_choice, _salt, _justification, isJuror);
+ // and _getExpectedVoteHash(...) with _getExpectedVoteHashWithContext(..., isJuror)
+}If you prefer a smaller diff, at minimum avoid setting the transient flag before external calls and compute the juror context only around hashing/expected-hash checks.
Also applies to: 149-155, 168-181, 193-202
🤖 Prompt for AI Agents
In contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol around line
44 (and also review usages at lines 149-155, 168-181, 193-202), the transient
bool callerIsJuror is set before making external calls which can leak or be
observed across reentrant/external boundaries; instead remove the transient flag
usage across external calls and pass an explicit juror-context parameter into
hashVote and _getExpectedVoteHash (or compute the juror-context locally
immediately before calling those functions). Concretely: stop setting
callerIsJuror prior to any external call, add a jurorContext parameter (or local
variable) that you compute from msg.sender/role right where
hashing/expected-hash is needed, and update hashVote/_getExpectedVoteHash
signatures and all internal call sites to accept the explicit context; if you
opt for a minimal change, ensure callerIsJuror is only assigned and used within
the same non-external-call scope (compute and consume it immediately around the
hashing checks) so it never persists across external calls.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/arbitration/SortitionModuleNeo.sol (1)
95-103: State drift risk: totalStaked updates occur before super; move after and gate on actual changeIf super._validateStake later returns a non-success result or defers the change (e.g., when _noDelay is false outside staking), totalStaked is still mutated, desynchronizing module state. Update totalStaked only after super and only if the stake actually changed now.
Apply:
- if (phase == Phase.staking) { - if (stakeIncrease) { - totalStaked += stakeChange; - } else { - totalStaked -= stakeChange; - } - } - (pnkDeposit, pnkWithdrawal, stakingResult) = super._validateStake(_account, _courtID, _newStake, _noDelay); + (pnkDeposit, pnkWithdrawal, stakingResult) = super._validateStake(_account, _courtID, _newStake, _noDelay); + // Reflect only immediate, realized stake changes. + uint256 newStakeAfter = stakeOf(_account, _courtID); + if (newStakeAfter != currentStake && (phase == Phase.staking || _noDelay)) { + if (newStakeAfter > currentStake) { + totalStaked += (newStakeAfter - currentStake); + } else { + totalStaked -= (currentStake - newStakeAfter); + } + }
🧹 Nitpick comments (4)
contracts/src/arbitration/SortitionModuleNeo.sol (4)
5-5: Avoid transitive import of IRNG to reduce couplingPrefer importing IRNG directly from its own interface file instead of re-exporting through SortitionModuleBase to prevent fragile compile-order or circular-dependency issues.
38-47: Reinitializer bump and IRNG migration—double-check upgrade path and null checks
- Confirm the reinitializer(2) version is compatible with existing proxy history for this contract. Ensure no previously-used version equals 2 (otherwise initialize would be blocked). The presence of initialize4() suggests version bumps elsewhere—please verify the intended call graph.
- Ensure _rng cannot be the zero address (if the base initializer doesn’t already validate it).
65-71: Emit events for governance changes (+ optional sanity checks)
- Emit events when maxStakePerJuror or maxTotalStaked change to aid observability and indexing.
- Optionally guard against setting values that would make staking impossible (e.g., zero) unless that’s intentional.
Apply this diff and add event declarations:
function changeMaxStakePerJuror(uint256 _maxStakePerJuror) external onlyByOwner { - maxStakePerJuror = _maxStakePerJuror; + uint256 old = maxStakePerJuror; + maxStakePerJuror = _maxStakePerJuror; + emit MaxStakePerJurorChanged(old, _maxStakePerJuror); } function changeMaxTotalStaked(uint256 _maxTotalStaked) external onlyByOwner { - maxTotalStaked = _maxTotalStaked; + uint256 old = maxTotalStaked; + maxTotalStaked = _maxTotalStaked; + emit MaxTotalStakedChanged(old, _maxTotalStaked); }Add inside the contract (near storage):
event MaxStakePerJurorChanged(uint256 previous, uint256 current); event MaxTotalStakedChanged(uint256 previous, uint256 current);
3-3: Pragma range differs from PR objectivePR notes broadening to >=0.8.0 <0.9.0. This file still pins ^0.8.24. If intentional (e.g., due to base/library constraints), ignore; otherwise, align with the repo-wide pragma or rely on tooling config.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
contracts/src/arbitration/SortitionModuleNeo.sol(5 hunks)contracts/src/libraries/Constants.sol(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/src/libraries/Constants.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: hardhat-tests
- GitHub Check: Scorecards analysis
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: hardhat-tests
🔇 Additional comments (3)
contracts/src/arbitration/SortitionModuleNeo.sol (3)
30-30: Docs: owner rename is correctParam rename in the initializer docs matches the ownership refactor.
60-62: UUPS authorization guarded by onlyByOwner—LGTM
80-82: All_validateStakecall sites updated with the new_noDelayparameter — Verified that SortitionModuleBase.sol (line 237) and SortitionModuleNeo.sol (line 102) both pass the boolean flag, matching the updated signature.
| if (juror.stakedPnk + stakeChange > maxStakePerJuror || currentStake + stakeChange > maxStakePerJuror) { | ||
| return (0, 0, StakingResult.CannotStakeMoreThanMaxStakePerJuror); | ||
| } |
There was a problem hiding this comment.
Bug: per-court limit mistakenly uses total juror stake
The check uses juror.stakedPnk (total across courts) against a per-court cap, which can incorrectly reject valid stakes in multi-court scenarios. Compare the desired per-court stake (_newStake) to maxStakePerJuror instead.
Apply:
- if (juror.stakedPnk + stakeChange > maxStakePerJuror || currentStake + stakeChange > maxStakePerJuror) {
+ if (_newStake > maxStakePerJuror) {
return (0, 0, StakingResult.CannotStakeMoreThanMaxStakePerJuror);
}Proxy contracts reset of the version and initializers ahead of the migration
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
contracts/src/proxy/mock/by-inheritance/UpgradedByInheritance.sol (1)
4-4: Align pragma in UpgradedByInheritance.sol with repo-wide constraint.
In contracts/src/proxy/mock/by-inheritance/UpgradedByInheritance.sol, change the pragma from-pragma solidity ^0.8.24; +pragma solidity >=0.8.0 <0.9.0;to match the repository’s agreed version range.
contracts/src/arbitration/devtools/KlerosCoreRuler.sol (2)
325-329: Validate rates to avoid division-by-zero in convertEthToTokenAmount()
rateInEth == 0makesconvertEthToTokenAmount()revert and can freeze ERC20 fee payments.- function changeCurrencyRates(IERC20 _feeToken, uint64 _rateInEth, uint8 _rateDecimals) external onlyByOwner { + function changeCurrencyRates(IERC20 _feeToken, uint64 _rateInEth, uint8 _rateDecimals) external onlyByOwner { + if (_rateInEth == 0) revert InvalidCurrencyRate(); + // optionally: require(_rateDecimals <= 36) to bound exponent currencyRates[_feeToken].rateInEth = _rateInEth; currencyRates[_feeToken].rateDecimals = _rateDecimals; emit NewCurrencyRate(_feeToken, _rateInEth, _rateDecimals); }Also add an error:
+ error InvalidCurrencyRate();
510-521: Avoid accounting before transfer; handle ETH send failure
sumFeeRewardPaidincrements before sending ETH and thesendresult is ignored, causing false accounting and potential stuck funds.- uint256 feeReward = round.totalFeesForJurors; - round.sumFeeRewardPaid += feeReward; - if (round.feeToken == NATIVE_CURRENCY) { - // The dispute fees were paid in ETH - payable(account).send(feeReward); - } else { - // The dispute fees were paid in ERC20 - round.feeToken.safeTransfer(account, feeReward); - } + uint256 feeReward = round.totalFeesForJurors; + if (round.feeToken == NATIVE_CURRENCY) { + // The dispute fees were paid in ETH + bool ok = payable(account).send(feeReward); // 2300 gas; no reentrancy + if (!ok) revert TransferFailed(); + } else { + // The dispute fees were paid in ERC20 + round.feeToken.safeTransfer(account, feeReward); + } + round.sumFeeRewardPaid += feeReward;contracts/src/arbitration/KlerosCoreNeo.sol (1)
31-71: Initialize: guard_jurorNftto avoid brickingsetStake().If
_jurorNftisaddress(0)or not a contract,jurorNft.balanceOfwill revert, effectively disabling staking.function initialize( - address _owner, + address _owner, address _guardian, IERC20 _pinakion, address _jurorProsecutionModule, IDisputeKit _disputeKit, bool _hiddenVotes, uint256[4] memory _courtParameters, uint256[4] memory _timesPerPeriod, bytes memory _sortitionExtraData, ISortitionModule _sortitionModuleAddress, - IERC721 _jurorNft, + IERC721 _jurorNft, address _wNative ) external reinitializer(2) { + if (address(_jurorNft) == address(0)) revert ZeroAddress(); + // Optional: also ensure `_jurorNft` is a contract to prevent EOAs. + if (address(_jurorNft).code.length == 0) revert NotAContract(); __KlerosCoreBase_initialize( _owner, _guardian, _pinakion, _jurorProsecutionModule, _disputeKit, _hiddenVotes, _courtParameters, _timesPerPeriod, _sortitionExtraData, _sortitionModuleAddress, _wNative ); jurorNft = _jurorNft; }Additions outside this hunk:
error ZeroAddress();error NotAContract();contracts/src/arbitration/SortitionModuleNeo.sol (1)
91-99: totalStaked accounting can drift outside staking phase.Updating
totalStakedonly inPhase.stakingignores immediate changes allowed via_noDelayand any immediate flows signaled bysuper._validateStake. Update after the super call based on returned deposits/withdrawals.Apply:
- if (phase == Phase.staking) { - if (stakeIncrease) { - totalStaked += stakeChange; - } else { - totalStaked -= stakeChange; - } - } - (pnkDeposit, pnkWithdrawal, stakingResult) = super._validateStake(_account, _courtID, _newStake, _noDelay); + (pnkDeposit, pnkWithdrawal, stakingResult) = super._validateStake(_account, _courtID, _newStake, _noDelay); + if (pnkDeposit > 0) { + totalStaked += pnkDeposit; + } + if (pnkWithdrawal > 0) { + totalStaked -= pnkWithdrawal; + }Also applies to: 98-99
contracts/src/arbitration/university/KlerosCoreUniversity.sol (5)
191-212: Initializer: validate non-zero critical addresses.Defensive checks prevent bricking the proxy on first init.
function initialize( address _owner, address _instructor, IERC20 _pinakion, address _jurorProsecutionModule, IDisputeKit _disputeKit, bool _hiddenVotes, uint256[4] memory _courtParameters, uint256[4] memory _timesPerPeriod, ISortitionModuleUniversity _sortitionModuleAddress ) external initializer { - owner = _owner; + if (_owner == address(0) || _instructor == address(0) || address(_pinakion) == address(0) || _jurorProsecutionModule == address(0) || address(_disputeKit) == address(0) || address(_sortitionModuleAddress) == address(0)) revert ZeroAddress(); + owner = _owner; instructor = _instructor; ... }Add to Errors:
+error ZeroAddress();
307-311: addNewDisputeKit: validate input and prevent duplicates.Check for zero address and existing registration to avoid sparse IDs pointing to dead DKs.
function addNewDisputeKit(IDisputeKit _disputeKitAddress) external onlyByOwner { + if (address(_disputeKitAddress) == address(0)) revert ZeroAddress(); + // Optional: prevent duplicates + for (uint256 i = 0; i < disputeKits.length; i++) { + if (address(disputeKits[i]) == address(_disputeKitAddress)) revert WrongDisputeKitIndex(); + } uint256 disputeKitID = disputeKits.length; disputeKits.push(_disputeKitAddress); emit DisputeKitCreated(disputeKitID, _disputeKitAddress); }
331-370: createCourt: explicit bounds-check parent court.Accessing
courts[_parent]before validating_parent < courts.lengthrisks an array OOB revert. Add a clear revert.) external onlyByOwner { - if (courts[_parent].minStake > _minStake) revert MinStakeLowerThanParentCourt(); + if (_parent >= courts.length) revert InvalidParentCourt(); + if (courts[_parent].minStake > _minStake) revert MinStakeLowerThanParentCourt(); if (_supportedDisputeKits.length == 0) revert UnsupportedDisputeKit(); if (_parent == FORKING_COURT) revert InvalidForkingCourtAsParent();And add:
+error InvalidParentCourt();
680-743: execute(): reentrancy window and CEI inconsistency.The loop makes external calls (DK, sortitionModule, token/ETH transfers) before persisting
round.pnkPenalties. Add reentrancy protection and update state before outward interactions where possible.
- Add ReentrancyGuard (upgradeable) and mark
executeasnonReentrant.- Alternatively keep local caches (as done) but persist critical state before transfers, or separate penalty/reward phases into two transactions.
Example (high-level, requires import and initializer wiring):
-import {Initializable} from "../../proxy/Initializable.sol"; +import {Initializable} from "../../proxy/Initializable.sol"; +import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; -contract KlerosCoreUniversity is IArbitratorV2, UUPSProxiable, Initializable { +contract KlerosCoreUniversity is IArbitratorV2, UUPSProxiable, Initializable, ReentrancyGuardUpgradeable { function initialize(... ) external initializer { ... + __ReentrancyGuard_init(); } -function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external { +function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external nonReentrant { ... - round.repartitions = end; + round.repartitions = end; // persist before external calls ... }
431-444: Enforce non-zero rates when accepting or setting fee tokens
– In changeAcceptedFeeTokens (contracts/src/arbitration/university/KlerosCoreUniversity.sol:431–434), addif (_accepted && currencyRates[_feeToken].rateInEth == 0) revert ZeroRate();before setting
feePaymentAccepted.
– In changeCurrencyRates (lines 440–444), addif (_rateInEth == 0) revert ZeroRate();before assigning
rateInEth.
– Declareerror ZeroRate();.
AcceptedFeeToken and NewCurrencyRate are already defined in IArbitratorV2.
♻️ Duplicate comments (11)
contracts/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol (1)
26-29: Add zero-address guard in initialize()Avoid bricking upgrades by preventing owner = address(0).
Apply:
- function initialize(address _owner) external virtual initializer { - owner = _owner; + function initialize(address _owner) external virtual initializer { + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; counter = 1; }Add the error (e.g., after state vars):
uint256[50] __gap; + error ZeroAddress();contracts/src/arbitration/PolicyRegistry.sol (1)
49-52: Guard against zero owner in initialize()Setting owner to address(0) permanently locks governance.
- function initialize(address _owner) external initializer { - owner = _owner; + function initialize(address _owner) external initializer { + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; }Also declare the error in the Errors section:
error OwnerOnly(); + error ZeroAddress();contracts/src/arbitration/DisputeTemplateRegistry.sol (1)
41-45: Initialize: add nonzero-owner checkZero owner bricks upgrades/governance.
function initialize(address _owner) external initializer { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; }contracts/src/arbitration/KlerosCoreNeo.sol (1)
85-87: Validate input and emit event inchangeJurorNft.Prevent zero address assignment and emit for off-chain indexing. This mirrors a prior suggestion.
function changeJurorNft(IERC721 _jurorNft) external onlyByOwner { - jurorNft = _jurorNft; + if (address(_jurorNft) == address(0)) revert ZeroAddress(); + jurorNft = _jurorNft; + emit JurorNftChanged(_jurorNft); }Additions outside this hunk:
event JurorNftChanged(IERC721 jurorNft);error ZeroAddress();(if not already added)contracts/src/arbitration/evidence/EvidenceModule.sol (1)
39-42: Guard against zero owner ininitialize().Zero owner locks upgrades and admin ops. This mirrors a prior suggestion.
function initialize(address _owner) external initializer { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; }Additions outside this hunk:
error ZeroAddress();contracts/src/arbitration/SortitionModule.sol (1)
28-35: Double-check callers/ABIs after removing_rngLookahead.Past comments flagged stale ABIs; ensure deploy artifacts and scripts dropped the arg and any
rngLookaheadview.#!/bin/bash # 1) Find any lingering references to rngLookahead rg -n "rngLookahead" -g '*.sol' -g '*.ts' -g '*.js' -C2 || echo "✔️ No rngLookahead refs" # 2) Calls to SortitionModule.initialize with 6+ args (old sig) rg -nP "SortitionModule.*initialize\s*\((?:[^()]|\\([^()]*\\))*\)" -g '*.ts' -g '*.js' -C2 \ | rg -n "initialize\\s*\\(([^,]*,){5}" || echo "✔️ No 6-arg initialize calls" # 3) Generated ABIs still exposing rngLookahead (initialize/changeRandomNumberGenerator/view) rg -nP '"name"\s*:\s*"rngLookahead"|"_rngLookahead"' -g '**/*.json' -g '**/*.ts' -C2 || echo "✔️ ABIs clean"contracts/src/arbitration/SortitionModuleNeo.sol (1)
84-86: Bug: per-court cap compares against total juror stake.Use
_newStake(the per-court target) instead ofjuror.stakedPnk(cross-court total).Apply:
- if (juror.stakedPnk + stakeChange > maxStakePerJuror || currentStake + stakeChange > maxStakePerJuror) { + if (_newStake > maxStakePerJuror) { return (0, 0, StakingResult.CannotStakeMoreThanMaxStakePerJuror); }contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (4)
44-44: Avoid transient global flag for juror context — reentrancy/leakage risk.A transient flag that spans external calls is fragile. Pass explicit juror context into hashing/expected-hash logic (or confine the flag strictly to a no-external-call window).
Minimal refactor (preferred):
- bool transient callerIsJuror; + // Avoid transient global flags; compute juror context per-call.Add context-aware helpers:
function _hashVoteWithContext(uint256 _choice, uint256 _salt, string memory _justification, bool isJuror) internal pure returns (bytes32) { if (isJuror) return keccak256(abi.encodePacked(_choice, _salt)); bytes32 justificationHash = keccak256(bytes(_justification)); return keccak256(abi.encode(_choice, _salt, justificationHash)); } function _getExpectedVoteHashWithContext(uint256 _localDisputeID, uint256 _localRoundID, uint256 _voteID, bool isJuror) internal view returns (bytes32) { return isJuror ? recoveryCommitments[_localDisputeID][_localRoundID][_voteID] : disputes[_localDisputeID].rounds[_localRoundID].votes[_voteID].commit; }
145-151: Juror-context flag spans external calls.
callerIsJuroris set, then_castVoteis called (which can make external calls). This invites cross-call leakage. Compute juror context locally and avoid transient global state.Apply:
- callerIsJuror = juror == msg.sender; - // `_castVote()` ensures that all the `_voteIDs` do belong to `juror` - _castVote(_coreDisputeID, _voteIDs, _choice, _salt, _justification, juror); - callerIsJuror = false; + bool isJuror = (juror == msg.sender); + _castVoteWithContext(_coreDisputeID, _voteIDs, _choice, _salt, _justification, juror, isJuror);Add (outside this hunk) a context-aware wrapper that mirrors
_castVotebut uses_hashVoteWithContextand_getExpectedVoteHashWithContextwhere hashing/verification is done.
168-176: MakehashVotedeterministic and context-free for public callers.Public
hashVoteshould not depend on transient state. Keep it deterministic (non-juror form) and expose the juror form via an internal helper.Apply:
- ) public view override returns (bytes32) { - if (callerIsJuror) { - // Caller is the juror, hash without `_justification` to facilitate recovery. - return keccak256(abi.encodePacked(_choice, _salt)); - } else { - // Caller is not the juror, hash with `_justification`. - bytes32 justificationHash = keccak256(bytes(_justification)); - return keccak256(abi.encode(_choice, _salt, justificationHash)); - } + ) public view override returns (bytes32) { + // Deterministic, context-free helper. + bytes32 justificationHash = keccak256(bytes(_justification)); + return keccak256(abi.encode(_choice, _salt, justificationHash)); }
183-198: Decouple expected-hash lookup from transient state.Use a context parameter to select between recoveryCommit and on-chain commit.
Apply:
- ) internal view override returns (bytes32) { - if (callerIsJuror) { - return recoveryCommitments[_localDisputeID][_localRoundID][_voteID]; - } else { - return disputes[_localDisputeID].rounds[_localRoundID].votes[_voteID].commit; - } - } + ) internal view override returns (bytes32) { + // Default (context-free) path: non-juror. + return disputes[_localDisputeID].rounds[_localRoundID].votes[_voteID].commit; + }And use
_getExpectedVoteHashWithContext(...)in your new_castVoteWithContext(...).
🧹 Nitpick comments (27)
contracts/src/proxy/mock/by-inheritance/UpgradedByInheritance.sol (2)
23-26: Add zero-address guard ininitializeto avoid bricking upgrades.If
owneris set toaddress(0),_authorizeUpgradebecomes uncallable forever.function initialize(address _owner) external virtual initializer { - owner = _owner; + require(_owner != address(0), "Owner cannot be zero"); + owner = _owner; counter = 1; }If you prefer custom errors:
error InvalidOwner();and
- require(_owner != address(0), "Owner cannot be zero"); + if (_owner == address(0)) revert InvalidOwner();
29-30: Use custom error for owner-only check to match PR standard and save gas.Swap string revert for
OwnerOnly()(mentioned in PR).- require(owner == msg.sender, "No privilege to upgrade"); + if (msg.sender != owner) revert OwnerOnly();If this mock should be self-contained, add:
error OwnerOnly();Otherwise import the shared error.
contracts/src/arbitration/devtools/KlerosCoreRuler.sol (4)
237-239: Emit event when changing PNK token
Helps indexers and off-chain services track token changes.- function changePinakion(IERC20 _pinakion) external onlyByOwner { - pinakion = _pinakion; - } + function changePinakion(IERC20 _pinakion) external onlyByOwner { + IERC20 old = pinakion; + pinakion = _pinakion; + emit PinakionChanged(old, _pinakion); + }Add event:
+ event PinakionChanged(IERC20 indexed _oldPinakion, IERC20 indexed _newPinakion);
257-258: Consider validating parent court ID
Optional: ensure_parent < courts.lengthfor a clearer error than a potential index panic later.
294-295: Consider validating target court ID
Optional:require(_courtID < courts.length)to avoid silent misconfiguration or index panic.
220-227: executeOwnerProposal: OK; consider emitting an event
Optional: emit something likeOwnerProposalExecuted(destination, amount, data)for traceability.contracts/src/gateway/HomeGateway.sol (3)
96-100: Emit an ownership change event (indexable traceability).Add an event and emit it on owner change to aid off-chain tooling and audits.
Apply this diff within the function:
function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + address oldOwner = owner; + owner = _owner; + emit OwnerChanged(oldOwner, _owner); }And add the event (outside this hunk, example placement near other declarations):
event OwnerChanged(address indexed previousOwner, address indexed newOwner);
104-104: Admin setters OK; consider emitting change events.Optional but useful: emit events for arbitrator/vea/gateway/fee-token changes to ease monitoring and rollbacks.
Also applies to: 110-110, 116-116, 122-122
212-212: Add a sanity check for unknown dispute IDs.Defensive: if
_disputeIDwas never set,disputeHashwill be zero and you’ll forward a message with a zero hash/relayer. Consider reverting ondisputeHash == bytes32(0).Example (outside this hunk):
if (disputeHash == bytes32(0)) revert UnknownDisputeID();And declare:
error UnknownDisputeID();contracts/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol (1)
31-33: Prefer custom errors over string requires for gas and consistencyReplace the string require with a custom error (OwnerOnly) like the rest of the codebase.
- function _authorizeUpgrade(address) internal view override { - require(owner == msg.sender, "No privilege to upgrade"); - } + error OwnerOnly(); + function _authorizeUpgrade(address) internal view override { + if (owner != msg.sender) revert OwnerOnly(); + }contracts/src/arbitration/PolicyRegistry.sol (1)
33-37: Access control error is good; consider emitting ownership changesOptional, but emitting OwnershipTransferred(prev, new) aids off-chain tracking and forensics.
+ event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); @@ - function initialize(address _owner) external initializer { + function initialize(address _owner) external initializer { if (_owner == address(0)) revert ZeroAddress(); - owner = _owner; + owner = _owner; + emit OwnershipTransferred(address(0), _owner); } @@ - function changeOwner(address _owner) external onlyByOwner { - if (_owner == address(0)) revert ZeroAddress(); - owner = _owner; + function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + emit OwnershipTransferred(owner, _owner); + owner = _owner; }Also applies to: 85-90
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
107-116: Write recovery commitments after successful _castCommitAvoid doing writes that will be reverted if
_castCommitfails; set them after the commit succeeds.- uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; - Dispute storage dispute = disputes[localDisputeID]; - uint256 localRoundID = dispute.rounds.length - 1; - for (uint256 i = 0; i < _voteIDs.length; i++) { - recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit; - } - - // `_castCommit()` ensures that the caller owns the vote - _castCommit(_coreDisputeID, _voteIDs, _commit); + // `_castCommit()` ensures that the caller owns the vote + _castCommit(_coreDisputeID, _voteIDs, _commit); + uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; + Dispute storage dispute = disputes[localDisputeID]; + uint256 localRoundID = dispute.rounds.length - 1; + for (uint256 i = 0; i < _voteIDs.length; i++) { + recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit; + }contracts/src/arbitration/DisputeTemplateRegistry.sol (1)
57-61: Emit event and validate on owner changeTrack ownership transitions and reject zero address.
- function changeOwner(address _owner) external onlyByOwner { - owner = _owner; - } + function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + address _prev = owner; + owner = _owner; + emit OwnerChanged(_prev, _owner); + }Add event (near storage or an Events section):
+ event OwnerChanged(address indexed previousOwner, address indexed newOwner);And error (see Errors section):
- error OwnerOnly(); + error OwnerOnly(); + error ZeroAddress();contracts/src/arbitration/KlerosCoreNeo.sol (3)
92-94: Emit whitelist change event (and optionally guard zero address).Event aids monitoring; zero address likely meaningless here.
function changeArbitrableWhitelist(address _arbitrable, bool _allowed) external onlyByOwner { - arbitrableWhitelist[_arbitrable] = _allowed; + if (_arbitrable == address(0)) revert ZeroAddress(); // optional + arbitrableWhitelist[_arbitrable] = _allowed; + emit ArbitrableWhitelistChanged(_arbitrable, _allowed); }Additions outside this hunk:
event ArbitrableWhitelistChanged(address arbitrable, bool allowed);- reuse
error ZeroAddress();
106-108: Fail fast ifjurorNftnot set to avoid external call decode errors.Calling
balanceOfon a zero/EOA address can revert; be explicit.function setStake(uint96 _courtID, uint256 _newStake) external override whenNotPaused { - if (jurorNft.balanceOf(msg.sender) == 0) revert NotEligibleForStaking(); + if (address(jurorNft) == address(0)) revert JurorNftNotSet(); + if (jurorNft.balanceOf(msg.sender) == 0) revert NotEligibleForStaking(); super._setStake(msg.sender, _courtID, _newStake, false, OnError.Revert); }Additions outside this hunk:
error JurorNftNotSet();
120-122: Whitelist gate — consider developer ergonomics.Good for safety. Add a dev note in docs and an event on whitelist changes so integrators can self-check why dispute creation fails.
contracts/src/arbitration/evidence/EvidenceModule.sol (1)
71-71: AddZeroAddresscustom error.Used in
initializeand future setters to standardize input validation.error OwnerOnly(); +error ZeroAddress();contracts/src/arbitration/SortitionModule.sol (2)
5-5: ImportingIRNGvia SortitionModuleBase — consistency nit.Prefer importing
IRNGfrom its dedicated interface (if available) to avoid tight coupling to the base.Would you like me to adjust imports repo-wide for consistency once we confirm the canonical IRNG path?
28-35: Initialize: ensure_rngis non-zero and a contract (or rely on base checks).If base initializer does not validate, add guards here.
function initialize( address _owner, KlerosCore _core, uint256 _minStakingTime, uint256 _maxDrawingTime, IRNG _rng ) external initializer { - __SortitionModuleBase_initialize(_owner, _core, _minStakingTime, _maxDrawingTime, _rng); + if (address(_rng) == address(0)) revert ZeroAddress(); + if (address(_rng).code.length == 0) revert NotAContract(); + __SortitionModuleBase_initialize(_owner, _core, _minStakingTime, _maxDrawingTime, _rng); }Additions outside this hunk:
error ZeroAddress();error NotAContract();contracts/src/arbitration/SortitionModuleNeo.sol (1)
61-67: Emit governance-change events for observability.Changes to caps are significant and should be traceable on-chain.
Apply:
function changeMaxStakePerJuror(uint256 _maxStakePerJuror) external onlyByOwner { - maxStakePerJuror = _maxStakePerJuror; + uint256 prev = maxStakePerJuror; + maxStakePerJuror = _maxStakePerJuror; + emit MaxStakePerJurorChanged(prev, _maxStakePerJuror); } function changeMaxTotalStaked(uint256 _maxTotalStaked) external onlyByOwner { - maxTotalStaked = _maxTotalStaked; + uint256 prev = maxTotalStaked; + maxTotalStaked = _maxTotalStaked; + emit MaxTotalStakedChanged(prev, _maxTotalStaked); }Add once (outside this hunk):
event MaxStakePerJurorChanged(uint256 previous, uint256 current); event MaxTotalStakedChanged(uint256 previous, uint256 current);contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
157-167: Doc/order mismatch inhashVoteparams.Natspec lists
_justificationbefore_salt, but the signature takes_saltfirst. Align docs for clarity.Apply:
- * @param _justification The justification for the vote - * @param _salt A random salt for commitment + * @param _salt A random salt for commitment + * @param _justification The justification for the votecontracts/src/arbitration/university/KlerosCoreUniversity.sol (6)
62-67: Add invariant checks for parallel arrays.
drawnJurorFromCourtIDsmust always mirrordrawnJurorslength and order. It’s only pushed indraw(). Please confirm no other code path pops/removes fromdrawnJurors. Consider adding an assert post-draw and pre-execute to guard against divergence.function draw(uint256 _disputeID, address _juror) external onlyByOwnerOrInstructor { ... round.drawnJurors.push(drawnAddress); round.drawnJurorFromCourtIDs.push(fromSubcourtID != 0 ? fromSubcourtID : dispute.courtID); + assert(round.drawnJurors.length == round.drawnJurorFromCourtIDs.length); ... }
265-272: executeOwnerProposal: consider emitting an event and surfacing returndata.For traceability and off-chain accounting, emit an event (destination, amount, selector). Optional: bubble up revert reason from callee to ease debugging.
function executeOwnerProposal(address _destination, uint256 _amount, bytes memory _data) external onlyByOwner { - (bool success, ) = _destination.call{value: _amount}(_data); + (bool success, bytes memory ret) = _destination.call{value: _amount}(_data); if (!success) revert UnsuccessfulCall(); + emit OwnerProposalExecuted(_destination, _amount, _data, ret); }And add:
+event OwnerProposalExecuted(address indexed destination, uint256 amount, bytes data, bytes returndata);
274-278: Owner change: optionally guard zero or explicitly support renounce.Decide whether setting owner to zero is allowed (renounce). If not, reject zero; if yes, document it and emit an event for indexers.
-function changeOwner(address payable _owner) external onlyByOwner { - owner = _owner; +function changeOwner(address payable _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + address prev = owner; + owner = _owner; + emit OwnerChanged(prev, _owner); }Add:
+event OwnerChanged(address indexed previousOwner, address indexed newOwner);
282-284: Changing instructor by instructor may be surprising.Allowing the current instructor to reassign the role could bypass owner intent. Confirm product requirement; otherwise restrict to
onlyByOwner.
588-613: Draw provenance: good addition; add sanity checks.Persisting
fromSubcourtIDenables precise penalty provenance. Consider asserting thatfromSubcourtIDrefers to an existing court (or defaulted) to catch bad DK returns.(address drawnAddress, uint96 fromSubcourtID) = disputeKit.draw(_disputeID, iteration); ... - round.drawnJurorFromCourtIDs.push(fromSubcourtID != 0 ? fromSubcourtID : dispute.courtID); + uint96 origin = fromSubcourtID != 0 ? fromSubcourtID : dispute.courtID; + if (origin >= courts.length) revert InvalidParentCourt(); + round.drawnJurorFromCourtIDs.push(origin);Also applies to: 599-607
1176-1179: Error renames align with owner model.New
OwnerOnly/OwnerOrInstructorOnlyare consistent. Consider addingZeroAddress,ZeroRate,InvalidParentCourt,NativeTransferFailedif you accept suggestions above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
contracts/deploy/upgrade-all.ts(1 hunks)contracts/src/arbitration/DisputeTemplateRegistry.sol(4 hunks)contracts/src/arbitration/KlerosCore.sol(5 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(6 hunks)contracts/src/arbitration/PolicyRegistry.sol(5 hunks)contracts/src/arbitration/SortitionModule.sol(2 hunks)contracts/src/arbitration/SortitionModuleNeo.sol(4 hunks)contracts/src/arbitration/devtools/KlerosCoreRuler.sol(10 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(7 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(7 hunks)contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol(3 hunks)contracts/src/arbitration/evidence/EvidenceModule.sol(4 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(25 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(12 hunks)contracts/src/gateway/ForeignGateway.sol(10 hunks)contracts/src/gateway/HomeGateway.sol(10 hunks)contracts/src/proxy/Initializable.sol(1 hunks)contracts/src/proxy/mock/by-inheritance/UpgradedByInheritance.sol(2 hunks)contracts/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/src/proxy/Initializable.sol
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/src/gateway/ForeignGateway.sol
- contracts/src/arbitration/university/SortitionModuleUniversity.sol
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-03T19:34:58.018Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#2107
File: contracts/src/arbitration/university/KlerosCoreUniversity.sol:1083-1092
Timestamp: 2025-09-03T19:34:58.018Z
Learning: KlerosCoreUniversity and SortitionModuleUniversity do not have phases, unlike KlerosCoreBase and SortitionModuleBase. Therefore, validateStake in the University contracts will never return StakingResult.Delayed, only Successful or other failure states.
Applied to files:
contracts/src/arbitration/university/KlerosCoreUniversity.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
Applied to files:
contracts/src/arbitration/university/KlerosCoreUniversity.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (45)
contracts/src/proxy/mock/by-inheritance/UpgradedByInheritance.sol (2)
15-15: Renamegovernor→owner: storage-compatibility LGTM.Type and position preserved; safe for upgrades if the prior slot was
address governor.
64-66: Confirm intent:initializeV3()usesinitializer(will revert after V1 init).If this is a negative test (contract name says “V3Bad”), fine. If not, use a higher reinitializer version.
-function initializeV3() external initializer { +function initializeV3() external reinitializer(3) {contracts/src/arbitration/devtools/KlerosCoreRuler.sol (6)
214-217: UUPS upgrade authorization switch to owner looks good
No issues spotted.
16-16: Version bump acknowledged
Matches the refactor. Ensure the interface expects a public getter with this signature.
88-88: Owner storage var rename is consistent
No issues spotted.
160-163: onlyByOwner modifier is correct
Compact and uses a custom error; good.
666-666: OwnerOnly error added
Looks good and consistently used.
399-401: SafeERC20.safeTransferFrom returns bool, conditional is valid
The SafeERC20 library in src/libraries/SafeERC20.sol defines safeTransferFrom to return a bool; using it in an if condition compiles and behaves as intended. No changes required.Likely an incorrect or invalid review comment.
contracts/src/gateway/HomeGateway.sol (3)
32-32: Ownership refactor looks correct.Storage var and modifier read clean;
OwnerOnly()is defined below.Also applies to: 46-49
90-93: Upgrade gate wired to owner — good.
_authorizeUpgraderestricted byonlyByOwneris the right UUPS control.
237-250: Custom errors set looks solid and consistent.Clear, specific errors for each failure mode. Nice.
contracts/src/arbitration/PolicyRegistry.sol (2)
10-10: Version bump looks good
80-83: SetPolicy access change acknowledgedonlyByOwner applied correctly; event payload mirrors stored value.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
3-3: No action required: compiler configuration already covers all pragmasHardhat and Foundry are configured to use Solidity v0.8.30, which satisfies both
^0.8.24and^0.8.28across all contracts, so no pragma alignment is needed.contracts/src/arbitration/DisputeTemplateRegistry.sol (2)
67-78: Confirm permissionless template registration
setDisputeTemplateis unrestricted. If intended to be permissionless, all good; otherwise addonlyByOwner.
2-2: Pragma alignmentFile uses
^0.8.24while Shutter uses^0.8.28. Confirm toolchain supports both or standardize.contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (3)
30-37: Initialize: verify nonzero-owner validationEither add an explicit
_owner != address(0)check here or confirm__DisputeKitClassicBase_initializeenforces it.Optionally:
) external initializer { - __DisputeKitClassicBase_initialize(_owner, _core, _wNative, _jumpDisputeKitID); + if (_owner == address(0)) revert ZeroAddress(); + __DisputeKitClassicBase_initialize(_owner, _core, _wNative, _jumpDisputeKitID); }
43-46: LGTM: owner-gated UUPS authorizationUpgrade gate correctly switched to
onlyByOwner.
3-3: Pragma consistencyThis file
^0.8.24vs Shutter^0.8.28. Ensure single- or multi-compiler setup is intentional.contracts/src/arbitration/KlerosCoreNeo.sol (2)
12-12: Version bump acknowledged.Ensure downstream consumers (subgraphs, SDKs, frontends) expect "2.0.0".
79-81: UUPS upgrade gate switched to owner — LGTM.contracts/src/arbitration/evidence/EvidenceModule.sol (2)
24-27: Owner-only modifier — LGTM.
52-54: UUPS upgrade gate switched to owner — LGTM.contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
61-63: UUPS upgrade gate switched to owner — LGTM.contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (2)
46-53: Initialize wiring — LGTM. Add a sanity check for_jumpDisputeKitIDonly if non-zero is required.If base treats
0as valid, leave as-is.
61-63: UUPS upgrade gate switched to owner — LGTM.contracts/src/arbitration/SortitionModule.sol (1)
43-45: UUPS upgrade gate switched to owner — LGTM.contracts/src/arbitration/KlerosCore.sol (2)
11-11: Version bump acknowledged.Public version constant update to "2.0.0" looks consistent with the PR scope.
23-46: Initializer vs. upgrade path — confirm deployment/migration plan.Switching to
initializer(and removing re-init paths) means existing proxies cannot set new state (e.g.,_disputeKit) via an upgrade hook. This is fine for fresh deployments but blocks in-place migrations. Please confirm 2.x is intended for new proxies; otherwise, add a dedicatedreinitializer(N)to wire new fields safely.Also applies to: 46-59
contracts/src/arbitration/SortitionModuleNeo.sol (2)
10-10: Version bump acknowledged.Constant
version = "2.0.0"aligns with release scope.
55-59: No change needed:viewoverride is valid. Parent’s_authorizeUpgrade(address)is nonpayable and Solidity allows overriding a nonpayable function with a stricterviewmutability (nonpayable → view/pure)(ethereum.stackexchange.com).contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (5)
31-31: Version bump acknowledged.Constant
version = "2.0.0"matches release.
37-39: Recovery commitments mapping looks good.Public mapping layout for (localDisputeID, localRoundID, voteID) → bytes32 is clear and queryable.
75-87: Initializer change acknowledged.Owner-based initializer and jump DK ID wiring LGTM.
44-44: Confirm target chains support EIP-1153 (transient storage).If any deployment target lacks Cancun/1153,
transientwill fail. Confirm all target networks (L1/L2/testnets) support it or remove dependency.
93-97: No change required —viewis allowed on the override.Solidity permits tightening mutability (nonpayable → view/pure); UUPSProxiable._authorizeUpgrade is non-view, so the override
internal view override onlyByOwner { /* NOP */ }is valid. (docs.soliditylang.org)Location: contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (lines 93–97); base: contracts/src/proxy/UUPSProxiable.sol (function _authorizeUpgrade at ~line 65).
contracts/src/arbitration/university/KlerosCoreUniversity.sol (9)
11-11: LGTM on constants import and version bump.Importing
Constants.soland bumpingversionto "2.0.0" look consistent with the PR direction.Also applies to: 18-18
93-93: Ownership refactor/guards look correct.Replacing governor with owner and adding
onlyByOwner/onlyByOwnerOrInstructoris consistent and tight.Also applies to: 166-169, 176-179
258-262: UUPS authorization scoped to owner — good.Matches the repo-wide ownership model.
380-405: LGTM onchangeCourtParametersconstraints.Child/parent minStake checks and event emission look correct.
411-426: enableDisputeKits: good guardrails for Classic DK.Logic and erroring on out-of-range DK IDs look fine.
455-465: Stake API:_noDelaywiring matches University behavior.Using
falseinsetStakeandtrueinsetStakeBySortitionModulealigns with the retrieved learning (no Delayed state in University).
525-526: ONE_BASIS_POINT usage forpnkAtStakePerJuror— good.The new scaling is clear and avoids hard-coded divisors.
Also applies to: 655-656
1083-1092: _setStake: API extension looks fine; surface failure reasons consistently.
_noDelayplumbs through tovalidateStakeand errors are mapped via_stakingFailed. Looks good given University never returns Delayed.Also applies to: 1101-1126
828-870: Reward path: unlock math may leave residual locks if coherences differ.You unlock
pnkLockedusingpnkCoherence, while penalties usecoherence(from penalty path). If DK returns different values,penalty + pnkLocked != pnkAtStakePerJuror, leaving residual locked stake.Option A (recommended): base unlock on penalty coherence to ensure exact complement.
function _executeRewards(ExecuteParams memory _params) internal { ... - (uint256 pnkCoherence, uint256 feeCoherence) = disputeKit.getDegreeOfCoherenceReward(...); + (uint256 pnkCoherence, uint256 feeCoherence) = disputeKit.getDegreeOfCoherenceReward(...); + // Recompute penalty coherence for the same juror index to fully release the lock + uint256 penaltyCoherence = disputeKit.getDegreeOfCoherencePenalty( + _params.disputeID, + _params.round, + _params.repartition % _params.numberOfVotesInRound, + _params.feePerJurorInRound, + _params.pnkAtStakePerJurorInRound + ); ... - uint256 pnkLocked = (round.pnkAtStakePerJuror * pnkCoherence) / ONE_BASIS_POINT; + if (penaltyCoherence > ONE_BASIS_POINT) penaltyCoherence = ONE_BASIS_POINT; + uint256 pnkLocked = (round.pnkAtStakePerJuror * penaltyCoherence) / ONE_BASIS_POINT; ... }Option B: assert equality to catch DK inconsistencies in tests.
+require(pnkCoherence == penaltyCoherence, "CoherenceMismatch");Also replace
.sendwith.callfor ETH juror payouts:- payable(account).send(feeReward); + (bool ok, ) = payable(account).call{value: feeReward}(""); + if (!ok) revert NativeTransferFailed();Also applies to: 858-864
⛔ Skipped due to learnings
Learnt from: jaybuidl PR: kleros/kleros-v2#0 File: :0-0 Timestamp: 2025-09-03T22:48:32.951Z Learning: In the Kleros v2 codebase, the team prioritizes gas optimization over strict CEI pattern compliance when dealing with trusted contracts. For penalty execution logic, they prefer batching storage writes (`round.pnkPenalties`) rather than updating incrementally after each penalty calculation to save gas costs, as the risk is extremely low between trusted contracts.
| await upgrade(disputeTemplateRegistry, "reinitialize", []); | ||
| await upgrade(evidence, "reinitialize", []); | ||
| await upgrade(core, "reinitialize", [wETH.address]); | ||
| await upgrade(policyRegistry, "initialize2", []); | ||
| await upgrade(sortition, "initialize4", []); | ||
| await upgrade(policyRegistry, "reinitialize", []); | ||
| await upgrade(sortition, "reinitialize", []); | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Initializer mismatch: PolicyRegistry has no reinitialize(); this upgrade step will revert
The current PolicyRegistry implementation exposes initialize(address) (initializer), not reinitialize(). Calling reinitialize([]) here will fail. Either add a no-op reinitialize() to the contract or skip the initializer call during this upgrade while still deploying the new implementation.
Run this to confirm which contracts actually expose reinitialize():
Apply this minimal change (after making upgrade() accept an optional initializer as shown below):
- await upgrade(policyRegistry, "reinitialize", []);
+ await upgrade(policyRegistry);And update upgrade() to make the initializer optional and only pass it when present:
- const upgrade = async (contractName: string, initializer: string, args: any[]) => {
+ const upgrade = async (contractName: string, initializer?: string, args: any[] = []) => {
@@
- switch (contractName) {
+ const baseOpts: any =
+ contractName === "DisputeKitClassicNeo" || contractName === "DisputeResolverNeo"
+ ? { contract: contractName, newImplementation: contractNameWithoutNeo }
+ : { newImplementation: contractName };
+ if (initializer) {
+ baseOpts.initializer = initializer;
+ baseOpts.from = deployer;
+ baseOpts.args = args; // only new state
+ } else {
+ baseOpts.from = deployer;
+ }
+ switch (contractName) {
case "DisputeKitClassicNeo":
case "DisputeResolverNeo":
- await deployUpgradable(deployments, contractName, {
- contract: contractName,
- newImplementation: contractNameWithoutNeo,
- initializer,
- from: deployer,
- args, // Warning: do not reinitialize existing state variables, only the new ones
- });
+ await deployUpgradable(deployments, contractName, baseOpts);
break;
default:
- await deployUpgradable(deployments, contractName, {
- newImplementation: contractName,
- initializer,
- from: deployer,
- args, // Warning: do not reinitialize existing state variables, only the new ones
- });
+ await deployUpgradable(deployments, contractName, baseOpts);
}🏁 Script executed:
#!/bin/bash
# Verify presence of `reinitialize()` in the affected contracts
rg -nP 'contract\s+(PolicyRegistry|DisputeTemplateRegistry|SortitionModule|EvidenceModule)\b' -n -C2 contracts/src \
| sed 's/^/--- /'
echo
echo "Searching for reinitialize() definitions:"
rg -nP 'contract\s+(PolicyRegistry|DisputeTemplateRegistry|SortitionModule|EvidenceModule)\b|function\s+reinitialize\s*\(' -C2 contracts/srcLength of output: 4202
Drop or optionalize reinitialize calls for contracts without a matching function
EvidenceModule, SortitionModule, PolicyRegistry and DisputeTemplateRegistry do not define reinitialize(), so invoking it will revert. Either add a no-op reinitialize() in each upgraded contract that requires new state setup, or remove the "reinitialize"/[] arguments (e.g. await upgrade(policyRegistry)) and update upgrade() to only include initializer/args when present.
🤖 Prompt for AI Agents
In contracts/deploy/upgrade-all.ts around lines 101 to 106, the script calls
upgrade(..., "reinitialize", []) on EvidenceModule, SortitionModule,
PolicyRegistry and DisputeTemplateRegistry even though those contracts lack a
reinitialize() function, which will cause reverts; either remove the
"reinitialize"/[] args for those upgrades (e.g. change to await
upgrade(policyRegistry)) or modify the upgrade() helper to only pass an
initializer and args when provided, or alternatively add a no-op reinitialize()
to each contract that needs a placeholder; implement the chosen approach so the
upgrade call only invokes an initializer when the contract actually defines it.
| function initialize(address _owner, IERC20 _pinakion, uint256[4] memory _courtParameters) external initializer { | ||
| owner = _owner; | ||
| pinakion = _pinakion; |
There was a problem hiding this comment.
Prevent permanent lockout: reject zero owner in initialize()
A zero owner bricks all onlyByOwner functions (no path to recover).
function initialize(address _owner, IERC20 _pinakion, uint256[4] memory _courtParameters) external initializer {
- owner = _owner;
+ if (_owner == address(0)) revert OwnerOnly(); // or introduce a dedicated ZeroAddress() error
+ owner = _owner;
pinakion = _pinakion;Optionally also validate _pinakion != IERC20(address(0)) if zero is not meaningful here.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function initialize(address _owner, IERC20 _pinakion, uint256[4] memory _courtParameters) external initializer { | |
| owner = _owner; | |
| pinakion = _pinakion; | |
| function initialize(address _owner, IERC20 _pinakion, uint256[4] memory _courtParameters) external initializer { | |
| if (_owner == address(0)) revert OwnerOnly(); // or introduce a dedicated ZeroAddress() error | |
| owner = _owner; | |
| pinakion = _pinakion; | |
| // …rest of initialization… | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/devtools/KlerosCoreRuler.sol around lines 178 to
180, initialize() currently accepts a zero address owner which would permanently
disable all onlyByOwner functions; update the initializer to require(_owner !=
address(0)) and revert if zero, then set owner = _owner; additionally, if a zero
token is invalid, add require(address(_pinakion) != address(0)) before assigning
pinakion = _pinakion to prevent initializing with a null token reference.
| /// @dev Changes the `owner` storage variable. | ||
| /// @param _owner The new value for the `owner` storage variable. | ||
| function changeOwner(address payable _owner) external onlyByOwner { | ||
| owner = _owner; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Emit OwnerChanged event and guard zero address in changeOwner()
Improves observability and avoids accidental bricking.
- function changeOwner(address payable _owner) external onlyByOwner {
- owner = _owner;
- }
+ function changeOwner(address payable _owner) external onlyByOwner {
+ if (_owner == address(0)) revert OwnerOnly(); // or a dedicated ZeroAddress()
+ address old = owner;
+ owner = _owner;
+ emit OwnerChanged(old, _owner);
+ }Add event in the events section:
+ event OwnerChanged(address indexed _oldOwner, address indexed _newOwner);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/src/arbitration/devtools/KlerosCoreRuler.sol around lines 229 to
233, the changeOwner function currently sets owner without validation or
emitting an event; update it to revert if _owner is the zero address and emit an
OwnerChanged(address indexed previousOwner, address indexed newOwner) event when
owner changes. Add the OwnerChanged event declaration to the events section, add
a require(_owner != address(0), "Owner cannot be zero") guard at the start of
changeOwner, capture the previous owner in a local variable, assign the new
owner, and emit OwnerChanged(previousOwner, _owner).
| function changeAcceptedFeeTokens(IERC20 _feeToken, bool _accepted) external onlyByOwner { | ||
| currencyRates[_feeToken].feePaymentAccepted = _accepted; | ||
| emit AcceptedFeeToken(_feeToken, _accepted); | ||
| } |
There was a problem hiding this comment.
Undeclared events: AcceptedFeeToken/NewCurrencyRate
Both emits reference events that are not declared in this contract or its shown imports; this will not compile.
Add these events under the events block:
+ event AcceptedFeeToken(IERC20 indexed _feeToken, bool _accepted);
+ event NewCurrencyRate(IERC20 indexed _feeToken, uint64 _rateInEth, uint8 _rateDecimals);Also applies to: 325-329
🤖 Prompt for AI Agents
In contracts/src/arbitration/devtools/KlerosCoreRuler.sol around lines 316-319
(and also affecting 325-329), the contract emits events AcceptedFeeToken and
NewCurrencyRate that are not declared; add event declarations under the events
block: declare AcceptedFeeToken with parameters matching the emit (IERC20
indexed feeToken, bool accepted) and declare NewCurrencyRate with parameters
matching its emit (IERC20 indexed feeToken, uint256 newRate) so the emitted
events compile and include useful indexed fields for filtering.
| // * Transient Storage * // | ||
| // ************************************* // | ||
|
|
||
| bool transient callerIsJuror; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Scope juror-context flag and guard against reentrancy-driven leakage
callerIsJuror is contract-global (even if transient) and toggled around _castVote. If _castVote (or anything it calls) ever performs external calls, reentrancy could observe an unintended juror context. At minimum, save/restore the prior value to avoid nested-context bugs; ideally ensure _castVote doesn’t call untrusted targets or guard with a reentrancy pattern.
Apply scoped set/restore:
- callerIsJuror = juror == msg.sender;
- // `_castVote()` ensures that all the `_voteIDs` do belong to `juror`
- _castVote(_coreDisputeID, _voteIDs, _choice, _salt, _justification, juror);
- callerIsJuror = false;
+ bool _prev = callerIsJuror;
+ callerIsJuror = (juror == msg.sender);
+ // `_castVote()` ensures that all the `_voteIDs` do belong to `juror`
+ _castVote(_coreDisputeID, _voteIDs, _choice, _salt, _justification, juror);
+ callerIsJuror = _prev;Optionally confirm _castVote has no external/untrusted calls:
Also applies to: 119-135, 148-161, 172-182
🏁 Script executed:
#!/bin/bash
# Inspect _castVote implementation and look for external/tokens calls.
fd -a 'DisputeKitClassicBase.sol' | xargs -I{} rg -n -C3 '_castVote\s*\(' {}
fd -a 'DisputeKitClassicBase.sol' | xargs -I{} rg -n -C2 -e '\.call\(' -e '\.transfer\(' -e '\.safeTransfer' -e '\.safeTransferFrom' {}Length of output: 551
Scope callerIsJuror via save/restore
Wrap each assignment to callerIsJuror around the _castVote invocation so you save and restore its previous value (e.g.
- callerIsJuror = (juror == msg.sender);
+ bool _prev = callerIsJuror;
+ callerIsJuror = (juror == msg.sender);
_castVote(...);
- callerIsJuror = false;
+ callerIsJuror = _prev;) in DisputeKitShutter.sol at line 28 (and likewise for the blocks at lines 119–135, 148–161, 172–182). Verified no direct .call/.transfer patterns in _castVote.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol around line 28
(and for the assignment blocks at lines 119–135, 148–161, 172–182), the
transient state variable callerIsJuror is overwritten before calling _castVote
without restoring its previous value; save the current value to a local
temporary variable, assign the desired new value to callerIsJuror, invoke
_castVote, then restore callerIsJuror from the temporary variable so the
original state is preserved across the call; apply this save/restore pattern to
each indicated assignment site.
| // Transfer any residual rewards to the owner. It may happen due to partial coherence of the jurors. | ||
| if (_params.repartition == _params.numberOfVotesInRound * 2 - 1) { | ||
| uint256 leftoverPnkReward = _params.pnkPenaltiesInRound - round.sumPnkRewardPaid; | ||
| uint256 leftoverFeeReward = round.totalFeesForJurors - round.sumFeeRewardPaid; | ||
| if (leftoverPnkReward != 0 || leftoverFeeReward != 0) { | ||
| if (leftoverPnkReward != 0) { | ||
| pinakion.safeTransfer(governor, leftoverPnkReward); | ||
| pinakion.safeTransfer(owner, leftoverPnkReward); | ||
| } | ||
| if (leftoverFeeReward != 0) { | ||
| if (round.feeToken == NATIVE_CURRENCY) { | ||
| // The dispute fees were paid in ETH | ||
| payable(governor).send(leftoverFeeReward); | ||
| payable(owner).send(leftoverFeeReward); | ||
| } else { | ||
| // The dispute fees were paid in ERC20 | ||
| round.feeToken.safeTransfer(governor, leftoverFeeReward); | ||
| round.feeToken.safeTransfer(owner, leftoverFeeReward); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leftover payouts: replace .send with .call.
Same .send issue when sending leftover ETH to owner.
- payable(owner).send(leftoverFeeReward);
+ (bool ok2, ) = payable(owner).call{value: leftoverFeeReward}("");
+ if (!ok2) revert NativeTransferFailed();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Transfer any residual rewards to the owner. It may happen due to partial coherence of the jurors. | |
| if (_params.repartition == _params.numberOfVotesInRound * 2 - 1) { | |
| uint256 leftoverPnkReward = _params.pnkPenaltiesInRound - round.sumPnkRewardPaid; | |
| uint256 leftoverFeeReward = round.totalFeesForJurors - round.sumFeeRewardPaid; | |
| if (leftoverPnkReward != 0 || leftoverFeeReward != 0) { | |
| if (leftoverPnkReward != 0) { | |
| pinakion.safeTransfer(governor, leftoverPnkReward); | |
| pinakion.safeTransfer(owner, leftoverPnkReward); | |
| } | |
| if (leftoverFeeReward != 0) { | |
| if (round.feeToken == NATIVE_CURRENCY) { | |
| // The dispute fees were paid in ETH | |
| payable(governor).send(leftoverFeeReward); | |
| payable(owner).send(leftoverFeeReward); | |
| } else { | |
| // The dispute fees were paid in ERC20 | |
| round.feeToken.safeTransfer(governor, leftoverFeeReward); | |
| round.feeToken.safeTransfer(owner, leftoverFeeReward); | |
| } | |
| } | |
| // Transfer any residual rewards to the owner. It may happen due to partial coherence of the jurors. | |
| if (_params.repartition == _params.numberOfVotesInRound * 2 - 1) { | |
| uint256 leftoverPnkReward = _params.pnkPenaltiesInRound - round.sumPnkRewardPaid; | |
| uint256 leftoverFeeReward = round.totalFeesForJurors - round.sumFeeRewardPaid; | |
| if (leftoverPnkReward != 0 || leftoverFeeReward != 0) { | |
| if (leftoverPnkReward != 0) { | |
| pinakion.safeTransfer(owner, leftoverPnkReward); | |
| } | |
| if (leftoverFeeReward != 0) { | |
| if (round.feeToken == NATIVE_CURRENCY) { | |
| // The dispute fees were paid in ETH | |
| (bool ok2, ) = payable(owner).call{value: leftoverFeeReward}(""); | |
| if (!ok2) revert NativeTransferFailed(); | |
| } else { | |
| // The dispute fees were paid in ERC20 | |
| round.feeToken.safeTransfer(owner, leftoverFeeReward); | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/university/KlerosCoreUniversity.sol around lines
881 to 897, replace the unsafe payable(owner).send(leftoverFeeReward) call with
a low-level call pattern that forwards value and checks the returned success
flag; specifically use payable(owner).call{value: leftoverFeeReward}("") and
require the success result (or otherwise revert/handle failure) so the ETH
transfer cannot silently fail. Ensure no code examples are left in comments and
keep consistent error message/context when requiring success.
| /// @dev Constructs the `PolicyRegistry` contract. | ||
| /// @param _governor The governor's address. | ||
| /// @param _owner The owner's address. | ||
| /// @param _arbitrator The address of the arbitrator. |
There was a problem hiding this comment.
Fix incorrect constructor docstring; keep docs accurate.
The initializer docs still reference PolicyRegistry.
Apply this diff:
- /// @dev Constructs the `PolicyRegistry` contract.
+ /// @dev Initializes the HomeGateway contract.Also applies to: 69-69, 75-76
🤖 Prompt for AI Agents
In contracts/src/gateway/HomeGateway.sol around lines 61-63 (also applies to
lines 69 and 75-76), the constructor/initializer NatSpec comments incorrectly
reference "PolicyRegistry" instead of the HomeGateway contract and its actual
parameters; update the docstrings to accurately describe the HomeGateway
constructor/initializer, replace mentions of PolicyRegistry with HomeGateway,
and ensure the @dev and @param tags correctly describe the owner, arbitrator,
and any other parameters present at the referenced lines.
| if (feeToken != NATIVE_CURRENCY) revert FeesPaidInNativeCurrencyOnly(); | ||
| if (_params.foreignChainID != foreignChainID) revert ForeignChainIDNotSupported(); |
There was a problem hiding this comment.
Close reentrancy window before external calls (duplicate-relay risk).
You set relayedData.relayer after createDispute. If arbitrator.createDispute re-enters, the second call can slip through the “already relayed” guard. Pre-mark before any external interaction.
Apply this diff:
RelayedData storage relayedData = disputeHashtoRelayedData[disputeHash];
if (relayedData.relayer != address(0)) revert DisputeAlreadyRelayed();
+ relayedData.relayer = msg.sender;
- uint256 disputeID = arbitrator.createDispute{value: msg.value}(_params.choices, _params.extraData);
+ uint256 disputeID = arbitrator.createDispute{value: msg.value}(_params.choices, _params.extraData);
disputeIDtoHash[disputeID] = disputeHash;
disputeHashtoID[disputeHash] = disputeID;
- relayedData.relayer = msg.sender;Optional: consider a lightweight nonReentrant guard as belt-and-suspenders.
Also applies to: 146-153
🤖 Prompt for AI Agents
In contracts/src/gateway/HomeGateway.sol around lines 132-133 (and likewise
146-153), the code sets relayedData.relayer after calling
arbitrator.createDispute which leaves a reentrancy window allowing duplicate
relays; pre-mark the relayer/relay status on relayedData before any external
call so the “already relayed” guard is effective, i.e. set relayedData.relayer
(and any flag indicating relayed) immediately prior to calling createDispute,
then proceed with the external arbitrator call; optionally also add a
lightweight nonReentrant modifier to the entrypoint as an additional safeguard.
| @@ -167,8 +167,8 @@ | |||
|
|
|||
| /// @inheritdoc IHomeGateway | |||
| function relayCreateDispute(RelayCreateDisputeParams memory _params, uint256 _feeAmount) external { | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing override on overloaded relayCreateDispute (ERC20 path).
If the interface declares this overload, this won’t compile without override.
Apply this diff:
-function relayCreateDispute(RelayCreateDisputeParams memory _params, uint256 _feeAmount) external {
+function relayCreateDispute(RelayCreateDisputeParams memory _params, uint256 _feeAmount) external override {Run to confirm the interface actually declares the overload:
🏁 Script executed:
#!/bin/bash
set -eu
# Show IHomeGateway relayCreateDispute signatures
fd -t f IHomeGateway.sol | xargs -I{} rg -n -C2 'relayCreateDispute\s*\(' {}Length of output: 1056
Add missing override to ERC20 overload of relayCreateDispute
File: contracts/src/gateway/HomeGateway.sol:169
-function relayCreateDispute(RelayCreateDisputeParams memory _params, uint256 _feeAmount) external {
+function relayCreateDispute(RelayCreateDisputeParams memory _params, uint256 _feeAmount) external override {This implementation must include override to match the overloaded signature declared in IHomeGateway.sol.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function relayCreateDispute(RelayCreateDisputeParams memory _params, uint256 _feeAmount) external { | |
| function relayCreateDispute(RelayCreateDisputeParams memory _params, uint256 _feeAmount) external override { |
🤖 Prompt for AI Agents
In contracts/src/gateway/HomeGateway.sol at line 169, the function declaration
for relayCreateDispute is missing the required override specifier for the ERC20
overload declared in IHomeGateway.sol; update the function signature to include
override (e.g., add "override" or "override(IHomeGateway)" as appropriate) so it
matches the overloaded interface declaration and compiles correctly.
| if (feeToken == NATIVE_CURRENCY) revert FeesPaidInERC20Only(); | ||
| if (_params.foreignChainID != foreignChainID) revert ForeignChainIDNotSupported(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Same reentrancy window + IERC20 API assumptions; pre-mark before token ops.
Mark as relayed before safeTransferFrom, increaseAllowance, and createDispute to prevent duplicate relays on ERC20 tokens (including fee-on-transfer/777 hooks).
Apply this diff:
RelayedData storage relayedData = disputeHashtoRelayedData[disputeHash];
if (relayedData.relayer != address(0)) revert DisputeAlreadyRelayed();
+ relayedData.relayer = msg.sender;
- if (!feeToken.safeTransferFrom(msg.sender, address(this), _feeAmount)) revert TransferFailed();
- if (!feeToken.increaseAllowance(address(arbitrator), _feeAmount)) revert AllowanceIncreaseFailed();
+ if (!feeToken.safeTransferFrom(msg.sender, address(this), _feeAmount)) revert TransferFailed();
+ if (!feeToken.increaseAllowance(address(arbitrator), _feeAmount)) revert AllowanceIncreaseFailed();
uint256 disputeID = arbitrator.createDispute(_params.choices, _params.extraData, feeToken, _feeAmount);
disputeIDtoHash[disputeID] = disputeHash;
disputeHashtoID[disputeHash] = disputeID;
- relayedData.relayer = msg.sender;Also verify your SafeERC20 and IERC20 definitions support these calls/return-bools (many OZ wrappers don’t return bool, and increaseAllowance isn’t in the standard IERC20 interface).
Also applies to: 184-194, 187-189
🏁 Script executed:
#!/bin/bash
set -eu
# Confirm SafeERC20 signatures and IERC20.include increaseAllowance
rg -n -C2 'library\s+SafeERC20' **/*.sol
rg -n -C2 '\bsafeTransferFrom\s*\(' **/SafeERC20.sol
rg -n -C2 'interface\s+IERC20\b' **/*.sol
rg -n -C2 '\bincreaseAllowance\s*\(' **/*.solLength of output: 103
🏁 Script executed:
#!/bin/bash
set -eu
# List all Solidity files in the repository
rg --files --glob '*.sol'
# Search within contracts for SafeERC20 and IERC20 definitions and increaseAllowance usage
rg -n -C2 'library SafeERC20' --glob 'contracts/**/*.sol'
rg -n -C2 'interface IERC20' --glob 'contracts/**/*.sol'
rg -n -C2 'increaseAllowance' --glob 'contracts/**/*.sol'Length of output: 5218
🏁 Script executed:
#!/bin/bash
set -eu
echo "=== imports in HomeGateway.sol ==="
sed -n '1,50p' contracts/src/gateway/HomeGateway.sol | rg -n 'import ' || true
echo "=== SafeERC20.sol head ==="
sed -n '1,200p' contracts/src/libraries/SafeERC20.sol || true
echo "=== SafeERC20 function definitions ==="
rg -n -C2 'function safeTransferFrom' contracts/src/libraries/SafeERC20.sol || true
rg -n -C2 'function safeIncreaseAllowance' contracts/src/libraries/SafeERC20.sol || true
rg -n -C2 'function increaseAllowance' contracts/src/libraries/SafeERC20.sol || trueLength of output: 3954
🏁 Script executed:
#!/bin/bash
set -eu
# Verify that SafeERC20 methods are available via `using` directive
rg -n 'using SafeERC20 for' --glob 'contracts/src/gateway/HomeGateway.sol'Length of output: 157
Pre-mark relayer before ERC20 operations in HomeGateway.sol (around L170)
Assign relayedData.relayer = msg.sender immediately after the DisputeAlreadyRelayed() check to close the reentrancy window before calling safeTransferFrom/increaseAllowance/createDispute. The imported SafeERC20 library (via using SafeERC20 for IERC20;) defines both safeTransferFrom and increaseAllowance returning bool, so no interface changes are needed.
Apply this diff:
RelayedData storage relayedData = disputeHashtoRelayedData[disputeHash];
if (relayedData.relayer != address(0)) revert DisputeAlreadyRelayed();
+ relayedData.relayer = msg.sender;
- if (!feeToken.safeTransferFrom(msg.sender, address(this), _feeAmount)) revert TransferFailed();
- if (!feeToken.increaseAllowance(address(arbitrator), _feeAmount)) revert AllowanceIncreaseFailed();
+ if (!feeToken.safeTransferFrom(msg.sender, address(this), _feeAmount)) revert TransferFailed();
+ if (!feeToken.increaseAllowance(address(arbitrator), _feeAmount)) revert AllowanceIncreaseFailed();
uint256 disputeID = arbitrator.createDispute(_params.choices, _params.extraData, feeToken, _feeAmount);
- relayedData.relayer = msg.sender;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/src/gateway/HomeGateway.sol around lines 170 to 171, after the
DisputeAlreadyRelayed() check assign relayedData.relayer = msg.sender
immediately to close the reentrancy window prior to any ERC20 operations
(safeTransferFrom/increaseAllowance/createDispute); move or insert that single
assignment directly before invoking any IERC20/SafeERC20 calls so the relayer is
marked before token transfers/allowance or dispute creation occur.
…bitration-mainnet
…in Ethers/Viem helpers
No more Neo contract distinction
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
contracts/scripts/populatePolicyRegistry.ts (1)
60-61: Defaulting to testnet on Arbitrum mainnet is risky—auto-select mainnet by chainId.Prevents accidental mainnet runs using testnet policies when
--fromis omitted.- from = isDevnet(network) ? Sources.V2_DEVNET : Sources.V2_TESTNET; + if (isDevnet(network)) { + from = Sources.V2_DEVNET; + } else if (chainId === HomeChains.ARBITRUM_ONE) { + from = Sources.V2_MAINNET; + } else { + from = Sources.V2_TESTNET; + }contracts/scripts/keeperBotShutter.ts (1)
252-258: Avoid precision loss for voteIDs; use bigint end-to-end.
voteID may exceed JS safe-integer. Keep it as bigint throughout to match uint256 on-chain.- const decryptedToVoteIDs = new Map<string, number[]>(); // Map from decryptedMessage string to array of voteIDs + const decryptedToVoteIDs = new Map<string, bigint[]>(); // Map from decryptedMessage string to array of voteIDs @@ - const { voteID } = parseGraphVoteId(vote.id); + const { voteID } = parseGraphVoteId(vote.id); @@ - decryptedToVoteIDs.get(decryptedMessage)!.push(voteID); + decryptedToVoteIDs.get(decryptedMessage)!.push(voteID);And update parseGraphVoteId to return bigints and adjust logging:
-const parseGraphVoteId = (graphVoteId: string) => { - const [disputeKitID, localDisputeID, localRoundID, voteID] = graphVoteId.split("-").map(Number); - return { disputeKitID, localDisputeID, localRoundID, voteID }; -}; +const parsePart = (s: string) => { + // Accept decimal or hex "0x..." parts. + return s.startsWith("0x") ? BigInt(s) : BigInt(s); +}; +const parseGraphVoteId = (graphVoteId: string) => { + const [disputeKitID, localDisputeID, localRoundID, voteID] = graphVoteId.split("-").map(parsePart); + return { disputeKitID, localDisputeID, localRoundID, voteID }; +};And when logging:
- `Decoded message for voteIDs [${voteIDs.join(", ")}]: ${JSON.stringify({ choice: decodedMessage.choice.toString(), salt: decodedMessage.salt, justification: decodedMessage.justification }, null, 2)}` + `Decoded message for voteIDs [${voteIDs.map((v) => v.toString()).join(", ")}]: ${JSON.stringify({ choice: decodedMessage.choice.toString(), salt: decodedMessage.salt, justification: decodedMessage.justification }, null, 2)}`contracts/scripts/populateCourts.ts (2)
108-112: Bug: post-increment mutates input and returns pre-incremented valueThis mutates the source array entries and returns the old id/parent, leading to off-by-one inconsistencies. Use pure arithmetic.
- const parametersV1ToV2 = (court: Court): Court => ({ - ...court, - id: court.id++, - parent: court.parent++, - }); + const parametersV1ToV2 = (court: Court): Court => ({ + ...court, + id: court.id + 1, + parent: court.parent + 1, + });
178-196: Fix logging with BigInt valuesUsing %d with BigInt throws (cannot convert BigInt to number). Use %s or String().
- console.log("Court %d: changing minStake from %d to %d", court.id, courtPresent.minStake, court.minStake); + console.log("Court %d: changing minStake from %s to %s", court.id, courtPresent.minStake.toString(), court.minStake.toString());- console.log("Court %d: changing alpha from %d to %d", court.id, courtPresent.alpha, court.alpha); + console.log("Court %d: changing alpha from %s to %s", court.id, courtPresent.alpha.toString(), court.alpha.toString());- console.log( - "Court %d: changing feeForJuror from %d to %d", + console.log( + "Court %d: changing feeForJuror from %s to %s", court.id, - courtPresent.feeForJuror, - court.feeForJuror + courtPresent.feeForJuror.toString(), + court.feeForJuror.toString() );- console.log( - "Court %d: changing jurorsForCourtJump from %d to %d", + console.log( + "Court %d: changing jurorsForCourtJump from %s to %s", court.id, - courtPresent.jurorsForCourtJump, - court.jurorsForCourtJump + courtPresent.jurorsForCourtJump.toString(), + court.jurorsForCourtJump.toString() );Also applies to: 201-206
contracts/deploy/00-home-chain-arbitration.ts (1)
151-153: Dependency mismatch: script uses RNGWithFallback but depends on ChainlinkRNGEnsure correct deploy ordering by depending on
RNGWithFallback(or both if needed).-deployArbitration.dependencies = ["ChainlinkRNG"]; +deployArbitration.dependencies = ["RNGWithFallback"]; +# Optionally keep backward-compat: +# deployArbitration.dependencies = ["RNGWithFallback", "ChainlinkRNG"];contracts/deploy/utils/klerosCoreHelper.ts (1)
17-20: BigInt vs number mismatch will force unnecessary txs; normalizerateDecimals.
pnkRate.rateDecimalsis bigint,rateDecimalsis BigNumberish; strict-inequality with differing types will always be true. Convert withtoBigInt.Apply:
- if (pnkRate.rateInEth !== toBigInt(rateInEth) || pnkRate.rateDecimals !== rateDecimals) { + if (pnkRate.rateInEth !== toBigInt(rateInEth) || pnkRate.rateDecimals !== toBigInt(rateDecimals)) {
♻️ Duplicate comments (7)
contracts/package.json (1)
162-169: Peer “viem” ^2.24.1 may conflict with root resolution (^2.23.x) — align versions.Same concern as previously flagged; please reconcile to avoid peer warnings and subtle incompatibilities.
Option A (preferred: update root resolution):
- "viem@npm:2.x": "npm:^2.23.2" + "viem@npm:2.x": "npm:^2.24.1"Option B (if you must stay on 2.23.x, relax this peer here):
- "peerDependencies": { - "viem": "^2.24.1" - }, + "peerDependencies": { + "viem": "^2.23.0" + },contracts/test/arbitration/staking-neo.ts (1)
301-304: Chai chain: use “to.be.revertedWithCustomError”Missing “be” causes the custom error matcher not to run.
- await expect(sortition.executeDelayedStakes(10)).to.revertedWithCustomError( + await expect(sortition.executeDelayedStakes(10)).to.be.revertedWithCustomError( sortition, "NoDelayedStakeToExecute" );Apply the same fix at Lines 369–371.
Also applies to: 369-371
contracts/src/arbitration/KlerosCore.sol (1)
289-292: _authorizeUpgrade mutability mismatch (UUPS) — removeviewParent declares a non-view function; this override will not compile.
- function _authorizeUpgrade(address) internal view override onlyByOwner { + function _authorizeUpgrade(address) internal override onlyByOwner { // NOP }contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)
547-558: Fix NatSpec: “reward” → “penalty”Docstring for the penalty accessor mentions “reward”.
- /// @return pnkCoherence The degree of coherence in basis points for the dispute PNK reward. + /// @return pnkCoherence The degree of coherence in basis points for the dispute PNK penalty.
138-141: Guard against inactive disputes in notJumpedAdd the active-dispute check to prevent accidental reads of uninitialized locals.
- modifier notJumped(uint256 _coreDisputeID) { - if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); + modifier notJumped(uint256 _coreDisputeID) { + if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); + if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); _; }contracts/deploy/upgrade-all.ts (2)
74-79: Verifyreinitialize()existence for these contracts or drop the call.EvidenceModule, SortitionModule, PolicyRegistry, and DisputeTemplateRegistry previously did not define
reinitialize(). Passing it will revert at upgrade time. Either (a) remove the initializer here or (b) add a no-opreinitialize()on-chain.You can confirm and then safely remove the initializer for contracts without it:
#!/bin/bash # Which of these actually expose reinitialize()? rg -nP 'contract\s+(PolicyRegistry|DisputeTemplateRegistry|SortitionModule|EvidenceModule)\b|function\s+reinitialize\s*\(' -C2 contracts/srcIf absent, update the calls:
- await upgrade(disputeTemplateRegistry, "reinitialize", []); - await upgrade(evidence, "reinitialize", []); + await upgrade(disputeTemplateRegistry); + await upgrade(evidence); @@ - await upgrade(policyRegistry, "reinitialize", []); - await upgrade(sortition, "reinitialize", []); + await upgrade(policyRegistry); + await upgrade(sortition);
54-59: Don’t unconditionally pass an initializer; make it optional to avoid upgrade reverts.If the target implementation lacks
reinitialize, the upgrade will revert. Optionalizeinitializerand only include it indeployUpgradableoptions when provided.Apply:
- const upgrade = async (contractName: string, initializer: string, args: any[]) => { + const upgrade = async (contractName: string, initializer?: string, args: any[] = []) => { @@ - await deployUpgradable(deployments, contractName, { - newImplementation: contractName, - initializer, - from: deployer, - args, // Warning: do not reinitialize existing state variables, only the new ones - }); + const opts: any = { newImplementation: contractName, from: deployer }; + if (initializer) { + opts.initializer = initializer; + opts.args = args; // Warning: only new state + } + await deployUpgradable(deployments, contractName, opts);
🧹 Nitpick comments (47)
contracts/scripts/generateDeploymentArtifact.sh (5)
7-7: Quote script name in usage output.Prevents word-splitting if the path contains spaces.
- echo "usage: $(basename $0) network address" + echo "usage: $(basename "$0") network address"
52-55: Quote variable in-ntest.Safer and shellcheck-friendly.
-if [[ -n $apiKey ]] +if [[ -n "$apiKey" ]] then query="$query&apikey=$apiKey" fi
57-64: Harden HTTP/JSON handling and send errors to stderr.Handle curl failures/timeouts and non-JSON responses; avoid emitting non-JSON to stdout.
-result=$(curl -s "$query") -if [[ $(echo "$result" | jq -r .status) == 0 ]] -then - echo "error: contract not verified or does not exist" - abi="[]" -else - abi=$(echo "$result" | jq -r .result) -fi +result=$(curl -fsS --retry 3 --max-time 20 "$query") || { + echo "error: failed to query explorer API" >&2 + exit 2 +} +status=$(printf '%s' "$result" | jq -r '.status // empty' 2>/dev/null || true) +if [[ "$status" != "1" ]]; then + echo "error: contract not verified or does not exist" >&2 + abi="[]" +else + abi=$(printf '%s' "$result" | jq -r .result) +fi
46-49: Write unknown-network errors to stderr.Keeps stdout clean for JSON consumers.
- echo "error: unknown network $network" + echo "error: unknown network '$network'" >&2
1-4: Add bash safety flags and prerequisite checks.Improves robustness for CI and local runs.
#!/usr/bin/env bash +set -Eeuo pipefail + +# prerequisites +command -v curl >/dev/null || { echo "error: curl not found" >&2; exit 2; } +command -v jq >/dev/null || { echo "error: jq not found" >&2; exit 2; } SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"contracts/scripts/populatePolicyRegistry.ts (2)
56-56: Keep the error list in sync with enum values.Minor: derive valid options from
Sourcesto avoid drift.const valid = Object.keys(Sources).filter((k) => isNaN(Number(k))).map((k) => k.toLowerCase()); console.error(`Invalid source, must be one of ${valid.join(", ")}`);
107-110: Avoid mixingfor awaitwith.thenfor plain arrays—simplify control flow.Sequential awaits read clearer and avoid dangling promises in logs.
- for await (const policy of policiesV2) { - console.log("Populating policy for %s Court (%d): %s", policy.name, policy.court, policy.uri); - await policyRegistry.setPolicy.populateTransaction(policy.court, policy.name, policy.uri).then(execute); - } + for (const policy of policiesV2) { + console.log("Populating policy for %s Court (%d): %s", policy.name, policy.court, policy.uri); + const tx = await policyRegistry.setPolicy.populateTransaction(policy.court, policy.name, policy.uri); + await execute(tx); + }contracts/package.json (1)
160-161: Drop theisomorphic-fetchpolyfill and rely on Node ≥18’s built-infetch(or conditionally load for older versions)
contracts/package.json still lists"isomorphic-fetch"but your code only uses it inscripts/shutter.ts(imported at the top) and otherwise either uses the native globalfetch(in scripts) or explicitly importsnode-fetchingetPoliciesV1.ts(so the polyfill is redundant on Node 18+). Remove the dependency and its import, or wrap it behind a runtime check for Node <18.contracts/scripts/keeperBotShutter.ts (3)
3-3: Use type-only import to avoid unintended runtime dependency.
Change to a type-only import so the compiled JS doesn’t try to require typechain at runtime.-import { DisputeKitGatedShutter, DisputeKitShutter } from "../typechain-types"; +import type { DisputeKitGatedShutter, DisputeKitShutter } from "../typechain-types";
232-233: Guard JSON logging against BigInt serialization issues.
Event args may contain BigInt/BigNumber; JSON.stringify would throw. Coerce to strings before logging.- logger.debug(`CommitCastShutter event: ${JSON.stringify({ _encryptedVote, _identity }, null, 2)}`); + const logArgs = { + _encryptedVote: _encryptedVote?.toString?.() ?? String(_encryptedVote), + _identity: _identity?.toString?.() ?? String(_identity), + }; + logger.debug(`CommitCastShutter event: ${JSON.stringify(logArgs, null, 2)}`);
169-178: Guard against out-of-bounds local round selection.
If currentLocalRoundIndex is out of range or localRounds is empty, this will throw; return an empty vote set instead.const disputeVotes = filteredRounds.map((round) => { const dk = round.dispute.disputeKitDispute[0]; const idx = Number(dk.currentLocalRoundIndex); - const filteredLocalRounds = dk.localRounds.filter((_, i) => i === idx); - return { - coreDispute: dk.coreDispute, - votes: filteredLocalRounds[0].votes, - }; + const filteredLocalRounds = dk.localRounds.filter((_, i) => i === idx); + const votes = filteredLocalRounds[0]?.votes ?? []; + return { coreDispute: dk.coreDispute, votes }; });contracts/test/foundry/KlerosCore_Appeals.t.sol (9)
39-41: Prefer using thedisputeIDvariable over hard-coded0.Improves clarity and avoids footguns if non-zero IDs ever occur.
- (uint256 start, uint256 end) = core.appealPeriod(0); + (uint256 start, uint256 end) = core.appealPeriod(disputeID); ... - (start, end) = core.appealPeriod(0); + (start, end) = core.appealPeriod(disputeID); - assertEq(core.appealCost(0), 0.21 ether, "Wrong appealCost"); + assertEq(core.appealCost(disputeID), 0.21 ether, "Wrong appealCost"); ... - (uint256 start, uint256 end) = core.appealPeriod(0); + (uint256 start, uint256 end) = core.appealPeriod(disputeID);Also applies to: 55-61, 134-135
80-93: Tighten event assertions by pinning the emitter address.Using the emitter guards against false positives if another contract logs a structurally identical event.
Example changes (apply pattern across similar blocks):
- vm.expectEmit(true, true, true, true); + vm.expectEmit(true, true, true, true, address(disputeKit)); emit DisputeKitClassicBase.Contribution(disputeID, 0, 1, crowdfunder1, 0.21 ether); ... - vm.expectEmit(true, true, true, true); + vm.expectEmit(true, true, true, true, address(core)); emit KlerosCore.AppealDecision(disputeID, arbitrable); - vm.expectEmit(true, true, true, true); + vm.expectEmit(true, true, true, true, address(core)); emit KlerosCore.NewPeriod(disputeID, KlerosCore.Period.evidence); ... - vm.expectEmit(true, true, true, true); + vm.expectEmit(true, true, true, true, address(core)); emit KlerosCore.CourtJump(disputeID, 1, newCourtID, GENERAL_COURT); - vm.expectEmit(true, true, true, true); + vm.expectEmit(true, true, true, true, address(core)); emit KlerosCore.DisputeKitJump(disputeID, 1, newDkID, DISPUTE_KIT_CLASSIC); - vm.expectEmit(true, true, true, true); + vm.expectEmit(true, true, true, true, address(newDisputeKit)); emit DisputeKitClassicBase.DisputeCreation(disputeID, 2, newExtraData); - vm.expectEmit(true, true, true, true); + vm.expectEmit(true, true, true, true, address(core)); emit KlerosCore.AppealDecision(disputeID, arbitrable); - vm.expectEmit(true, true, true, true); + vm.expectEmit(true, true, true, true, address(core)); emit KlerosCore.NewPeriod(disputeID, KlerosCore.Period.evidence);Also applies to: 176-184, 288-299, 426-437, 498-501
31-35: De-duplicate vote ID setup and make it resilient to juror count changes.Avoid hard-coded “3” and repeated assignments.
- uint256[] memory voteIDs = new uint256[](3); - voteIDs[0] = 0; - voteIDs[1] = 1; - voteIDs[2] = 2; + uint256[] memory voteIDs = new uint256[](DEFAULT_NB_OF_JURORS); + for (uint256 i; i < voteIDs.length; ++i) { + voteIDs[i] = i; + }Apply in all similar blocks.
Also applies to: 121-125, 165-169, 273-277, 411-415, 486-490
82-99: Replace appeal-cost magic numbers with derived values.Hard-coded 0.21/0.42/0.63 ether and balances couple tests to specific parameters. Derive from
core.appealCost(disputeID)and the configured multipliers to keep tests stable if court parameters evolve.Minimal example (apply pattern where relevant):
- disputeKit.fundAppeal{value: 0.63 ether}(disputeID, 1); + uint256 baseCost = core.appealCost(disputeID); + // For current params loser == baseCost, winner == 2*baseCost. If multipliers change, compute via getters. + uint256 loserCost = baseCost; + uint256 winnerCost = baseCost * 2; + disputeKit.fundAppeal{value: loserCost * 3 / 1}(disputeID, 1) // 0.63 ether previously ... - disputeKit.fundAppeal{value: 0.42 ether}(disputeID, 2); + disputeKit.fundAppeal{value: winnerCost - baseCost}(disputeID, 2)Optionally, fetch multipliers from Core/DK (if exposed) and compute exact costs:
winnerCost = baseCost * winnerStakeMultiplier / MULTIPLIER_DIVISOR, etc.Also applies to: 175-185, 191-205, 283-299, 421-437
150-156: CapturedisputeIDfromcreateDisputereturn (if the API returns it).Improves robustness vs. relying on
0as the first ID.- uint256 disputeID = 0; ... - arbitrable.createDispute{value: feeForJuror * DEFAULT_NB_OF_JURORS}("Action"); + uint256 disputeID = + arbitrable.createDispute{value: feeForJuror * DEFAULT_NB_OF_JURORS}("Action");If
createDisputedoesn’t return an ID in this harness, ignore this.Also applies to: 397-399, 475-477
68-69: Use1 weiliteral for clarity.
1is wei by default, but1 weireads better in revert tests.- core.appeal{value: 0.21 ether - 1}(disputeID, 2, arbitratorExtraData); + core.appeal{value: 0.21 ether - 1 wei}(disputeID, 2, arbitratorExtraData);
2-2: Consider aligning pragma with repo-wide range.Repo broadens to
>=0.8.0 <0.9.0. Matching it here avoids version drift in tests.-pragma solidity ^0.8.24; +pragma solidity >=0.8.0 <0.9.0;
309-314: Optional: add a quick assertion on round count post-jump.E.g.,
assertEq(core.getNumberOfRounds(disputeID), 2)to mirror the no-jump test and catch regressions in jump flow.Also applies to: 447-452
228-233: Avoid brittle assumptions about new IDs — derive them from Core.createCourt/addNewDisputeKit do not return IDs and there are no address→ID getters in Core. Use one of the following instead of hardcoding IDs:
- No core.getDisputeKitID(address) or core.disputeKitAddressToID(address) exists.
- Use the available length getter: core.getDisputeKitsLength() — after calling core.addNewDisputeKit(newDK) compute newDkID = core.getDisputeKitsLength() - 1.
- Capture the emitted events: DisputeKitCreated(disputeKitID, address) from addNewDisputeKit and CourtCreated(courtID, ...) from createCourt (createCourt does not return the ID).
- Or add an explicit helper (e.g., getCourtsLength() or make createCourt return the new courtID) if you prefer API-first fixes.
Fix the tests at contracts/test/foundry/KlerosCore_Appeals.t.sol lines 228–233 (and the other occurrences you noted: 229–233, 369–391, 404–406).
contracts/scripts/storage-layout.ts (1)
4-9: Make contract target configurableOptional: add params (path, contract) to reuse the task for other contracts.
-task("storage-layout", "Prints the storage layout of a contract").setAction( - async ({}, hre: HardhatRuntimeEnvironment) => { +task("storage-layout", "Prints the storage layout of a contract") + .addOptionalParam("path", "Source path", "src/arbitration/KlerosCore.sol") + .addOptionalParam("name", "Contract name", "KlerosCore") + .setAction(async ({ path, name }, hre: HardhatRuntimeEnvironment) => { await hre.run("compile"); - const fqName = "src/arbitration/KlerosCore.sol:KlerosCore"; + const fqName = `${path}:${name}`;contracts/scripts/populateCourts.ts (4)
2-9: Import typechain types as type-onlyAvoids accidental runtime imports; purely compile-time types.
-import { KlerosCore, KlerosCoreUniversity } from "../typechain-types"; +import type { KlerosCore, KlerosCoreUniversity } from "../typechain-types";Also: switching to v2 mainnet courts JSON looks correct for neo→mainnet unification.
73-81: Derive allowed Sources dynamically to avoid driftKeeps the error message in sync if enum members change.
- from = Sources[taskArgs.from.toUpperCase() as keyof typeof Sources]; - if (from === undefined) { - console.error("Invalid source, must be one of v1_mainnet, v1_gnosis, v2_devnet, v2_testnet, v2_mainnet"); + from = Sources[taskArgs.from.toUpperCase() as keyof typeof Sources]; + if (from === undefined) { + const allowed = Object.keys(Sources) + .filter((k) => isNaN(Number(k))) + .map((k) => k.toLowerCase()) + .join(", "); + console.error(`Invalid source, must be one of ${allowed}`); return; }
85-91: Derive allowed core types dynamicallyPrevents mismatch with Cores definition.
- if (coreType === undefined) { - console.error("Invalid core type, must be one of base, university"); + if (coreType === undefined) { + const allowed = Object.values(Cores).map((k) => k.toLowerCase()).join(", "); + console.error(`Invalid core type, must be one of ${allowed}`); return; }
256-266: Prefer numeric literal over hex string for K valueMinor clarity: pass 5n instead of ethers.toBeHex(5) for uint256 parameter.
- ethers.toBeHex(5), // Not accessible on-chain, but has always been set to the same value so far. + 5n, // Not accessible on-chain, but has always been set to the same value so far.contracts/deployments/contractsViem.ts (1)
206-227: Mainnet dispatch LGTMOptional: keep ordering consistent with dev/test for readability (already close).
contracts/deploy/change-sortition-module-rng.ts (1)
18-22: Make the script idempotent to avoid unnecessary tx/revertsGuard the mutations by checking current values first. This prevents reverts if already set and saves gas.
- console.log(`chainlinkRng.changeConsumer(${sortitionModule.target})`); - await chainlinkRng.changeConsumer(sortitionModule.target); + const currentConsumer = await chainlinkRng.consumer(); + if (currentConsumer !== sortitionModule.target) { + console.log(`chainlinkRng.changeConsumer(${sortitionModule.target})`); + await chainlinkRng.changeConsumer(sortitionModule.target); + } - console.log(`sortitionModule.changeRandomNumberGenerator(${chainlinkRng.target})`); - await sortitionModule.changeRandomNumberGenerator(chainlinkRng.target); + const currentRng = await sortitionModule.randomNumberGenerator(); + if (currentRng !== chainlinkRng.target) { + console.log(`sortitionModule.changeRandomNumberGenerator(${chainlinkRng.target})`); + await sortitionModule.changeRandomNumberGenerator(chainlinkRng.target); + }contracts/test/arbitration/staking-neo.ts (3)
32-34: Avoid shadowing the imported PNK contract typeLocal const PNK function shadows the imported PNK type, hurting readability. Prefer importing helpers or rename locals.
Example:
- const ETH = (amount: number) => ethers.parseUnits(amount.toString()); - const PNK = ETH; + const toETH = (amount: number) => ethers.parseUnits(amount.toString()); + const toPNK = toETH;Or import ETH/PNK helpers from deploy utils.
743-747: Remove unnecessary await on contract instance
to.emit(await sortition, ...)is odd; pass the contract directly.- await expect(sortition.executeDelayedStakes(10)) - .to.emit(await sortition, "StakeSet") + await expect(sortition.executeDelayedStakes(10)) + .to.emit(sortition, "StakeSet") .withArgs(deployer, 2, PNK(2000), PNK(4000)); // 2nd delayed stake will override the first one
515-517: Fix misleading commentThe balance assertion expects a 1000 PNK decrease; comment says “No PNK transfer”.
- expect(await pnk.balanceOf(deployer)).to.be.equal(balanceBefore - PNK(1000)); // No PNK transfer + expect(await pnk.balanceOf(deployer)).to.be.equal(balanceBefore - PNK(1000)); // 1000 PNK transferred outcontracts/src/arbitration/KlerosCore.sol (1)
1299-1299: Typo in error name
InvalidDisputKitParent→InvalidDisputeKitParentfor consistency/readability.- error InvalidDisputKitParent(); + error InvalidDisputeKitParent();contracts/deploy/00-home-chain-arbitration.ts (1)
44-49: Nonce-based address precompute is brittle
nonce + 3can drift if deploy steps change. Prefer reading the proxy address from the deployment artifact after deploying, or usepredictAddresshelpers consistently.If you keep precompute, add an assertion after deploy:
- Compare computed
klerosCoreAddresswithawait deployments.get("KlerosCore").then(x => x.address)and warn if mismatched.contracts/src/arbitration/SortitionModule.sol (5)
64-96: Emit governance/config change events (add OwnerChanged)No event is emitted for owner changes, and other config setters also lack events. Emitting these improves off-chain monitoring and upgrade transparency.
Add an OwnerChanged event (and consider analogous events for other setters):
// * Events * // // ************************************* // /// @notice Emitted when a juror stakes in a court. @@ event LeftoverPNKWithdrawn(address indexed _account, uint256 _amount); + +/// @notice Emitted when the contract owner changes. +/// @param previousOwner The old owner. +/// @param newOwner The new owner. +event OwnerChanged(address indexed previousOwner, address indexed newOwner);
612-622: Define ZeroAddressOwner errorRequired by the changeOwner guard.
error OwnerOnly(); error KlerosCoreOnly(); +error ZeroAddressOwner(); error MinStakingTimeNotPassed(); error NoDisputesThatNeedJurors(); error RandomNumberNotReady();
597-606: Sanitize K from _extraData to avoid invalid tree fanoutIf _extraData provides K == 0, the tree may misbehave. Fallback to DEFAULT_K (and consider upper bound if library requires).
function _extraDataToTreeK(bytes memory _extraData) internal pure returns (uint256 K) { if (_extraData.length >= 32) { assembly { // solium-disable-line security/no-inline-assembly K := mload(add(_extraData, 0x20)) } } else { K = DEFAULT_K; } + if (K == 0) { + K = DEFAULT_K; + } }
250-252: Avoid disputesWithoutJurors underflowpostDrawHook unconditionally decrements. If hooks ever mis-order, this underflows.
function postDrawHook(uint256 /*_disputeID*/, uint256 /*_roundID*/) external override onlyByCore { - disputesWithoutJurors--; + if (disputesWithoutJurors == 0) revert NoDisputesThatNeedJurors(); + disputesWithoutJurors--; }
197-216: Consider reentrancy guard around passPhase RNG callspassPhase makes external calls to rng which could reenter (malicious RNG). Not strictly exploitable now, but a simple non-reentrancy guard would harden it.
I can propose a minimal boolean guard pattern if desired; let me know.
contracts/test/foundry/KlerosCore_Execution.t.sol (2)
23-26: Decouple tests from deprecated rngLookaheadTests warp by rngLookahead, which is marked deprecated in SortitionModule and may be removed next redeploy. This can make tests brittle.
- Replace these warps with a minimal block/time advance (e.g., vm.roll(block.number + 2) or vm.warp(block.timestamp + 1)), or derive delay from the RNG mock if applicable.
- Keep functional semantics identical; the RNG here doesn’t depend on rngLookahead from the module anymore.
I can craft a small helper (advanceForRNG()) and replace these call sites.
Also applies to: 35-38, 165-167, 255-257, 303-305, 519-521, 571-574, 611-613
273-280: Fix tiny typo in comment“her” → “here”.
-// Note that these events are emitted only after the first iteration of execute() therefore the juror has been penalized only for 1000 PNK her. +// Note that these events are emitted only after the first iteration of execute(), therefore the juror has been penalized only for 1000 PNK here.contracts/deployments/utils.ts (1)
25-29: Improve getAddress error contextIncluding the contract name (when available) eases debugging.
export function getAddress(config: ContractConfig, chainId: number): `0x${string}` { const address = config.address[chainId]; - if (!address) throw new Error(`No address found for chainId ${chainId}`); + if (!address) throw new Error(`No address for chainId ${chainId} in provided ContractConfig`); return address; }contracts/scripts/utils/contracts.ts (3)
158-163: Network list duplicated; consider extracting a constant.Minor DRY to avoid divergence between the two functions.
Example:
const BASE_NETWORKS = ["arbitrumSepoliaDevnet", "arbitrumSepolia", "arbitrum"] as const;
67-67: Unify casing:blockHashRNG→blockHashRngacross modules
Rename the key/variable everywhere (string literal"BlockHashRNG"stays untouched).In contracts/scripts/utils/contracts.ts:
- blockHashRNG: "BlockHashRNG", + blockHashRng: "BlockHashRNG",- const blockHashRNG = await ethers.getContractOrNull<BlockHashRNG>(getContractNames(coreType).blockHashRNG); + const blockHashRng = await ethers.getContractOrNull<BlockHashRNG>(getContractNames(coreType).blockHashRng);- blockHashRNG, + blockHashRng,In contracts/scripts/keeperBot.ts:
- const { chainlinkRng, randomizerRng, blockHashRNG, sortition } = await getContracts(); + const { chainlinkRng, randomizerRng, blockHashRng, sortition } = await getContracts();-} else if (currentRng === blockHashRNG?.target && blockHashRNG !== null) { - const n = await blockHashRNG.receiveRandomness.staticCall(); +} else if (currentRng === blockHashRng?.target && blockHashRng !== null) { + const n = await blockHashRng.receiveRandomness.staticCall();Verify no other occurrences remain:
rg -nP '\bblockHashRNG\b' -C2
64-64: Renamebatcher→transactionBatcherincontracts/scripts/utils/contracts.ts
Update the mapping key, variable name, and returned property to match the deployments/getters convention:- batcher: "TransactionBatcher", + transactionBatcher: "TransactionBatcher", - const batcher = await ethers.getContract<TransactionBatcher>(getContractNames(coreType).batcher); + const transactionBatcher = await ethers.getContract<TransactionBatcher>(getContractNames(coreType).transactionBatcher); - batcher, + transactionBatcher,Tests and deployment getters already expect
transactionBatcher, so this alignment eliminates naming friction.contracts/test/integration/getContractsEthers.test.ts (1)
100-117: Deduplicate contract mappings across Ethers/Viem tests.A shared fixture would prevent drift between the two files.
E.g., export a single
mainnetContractMappingfrom a test helper and import it in both suites.contracts/deploy/00-home-chain-arbitration-mainnet.ts (3)
23-23: Rename NFT: stray “Neo” in a mainnet path.If this is truly the mainnet deploy path, consider removing “Neo” from the token name/symbol.
- const nft = await deployERC721(hre, deployer, "Kleros V2 Neo Early User", "KlerosV2NeoEarlyUser"); + const nft = await deployERC721(hre, deployer, "Kleros V2 Early User", "KlerosV2EarlyUser");
102-108: Currency rate set behind try/catch is acceptable; consider a read-before-write later.Low priority. Fine as-is for one-shot deploys.
115-124: Check current whitelist state before settingMake the deployment idempotent by querying the existing flags via the public getters before calling the change functions:
- console.log(`core.changeArbitrableWhitelistEnabled(true)`); - await core.changeArbitrableWhitelistEnabled(true); + if (!(await core.arbitrableWhitelistEnabled())) { + console.log(`core.changeArbitrableWhitelistEnabled(true)`); + await core.changeArbitrableWhitelistEnabled(true); + } - console.log(`core.changeArbitrableWhitelist(${resolver.address}, true)`); - await core.changeArbitrableWhitelist(resolver.address, true); + if (!(await core.arbitrableWhitelist(resolver.address))) { + console.log(`core.changeArbitrableWhitelist(${resolver.address}, true)`); + await core.changeArbitrableWhitelist(resolver.address, true); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (39)
contracts/deploy/00-home-chain-arbitration-mainnet.ts(6 hunks)contracts/deploy/00-home-chain-arbitration.ts(5 hunks)contracts/deploy/change-sortition-module-rng.ts(2 hunks)contracts/deploy/upgrade-all.ts(3 hunks)contracts/deploy/utils/klerosCoreHelper.ts(1 hunks)contracts/deployments/contractsEthers.ts(3 hunks)contracts/deployments/contractsViem.ts(2 hunks)contracts/deployments/utils.ts(1 hunks)contracts/package.json(5 hunks)contracts/scripts/changeOwner.ts(1 hunks)contracts/scripts/generateDeploymentArtifact.sh(1 hunks)contracts/scripts/generateDeploymentsMarkdown.sh(3 hunks)contracts/scripts/keeperBot.ts(2 hunks)contracts/scripts/keeperBotShutter.ts(2 hunks)contracts/scripts/populateCourts.ts(7 hunks)contracts/scripts/populatePolicyRegistry.ts(4 hunks)contracts/scripts/storage-layout.ts(1 hunks)contracts/scripts/utils/contracts.ts(5 hunks)contracts/src/arbitration/KlerosCore.sol(4 hunks)contracts/src/arbitration/KlerosCoreBase.sol(0 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(0 hunks)contracts/src/arbitration/SortitionModule.sol(2 hunks)contracts/src/arbitration/SortitionModuleBase.sol(0 hunks)contracts/src/arbitration/SortitionModuleNeo.sol(0 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(16 hunks)contracts/src/proxy/KlerosProxies.sol(0 hunks)contracts/test/arbitration/staking-neo.ts(9 hunks)contracts/test/foundry/KlerosCore_Appeals.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Disputes.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Drawing.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Governance.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Initialization.t.sol(1 hunks)contracts/test/foundry/KlerosCore_RNG.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Staking.t.sol(1 hunks)contracts/test/foundry/KlerosCore_TestBase.sol(1 hunks)contracts/test/foundry/KlerosCore_Voting.t.sol(1 hunks)contracts/test/integration/getContractsEthers.test.ts(3 hunks)contracts/test/integration/getContractsViem.test.ts(3 hunks)
💤 Files with no reviewable changes (5)
- contracts/src/proxy/KlerosProxies.sol
- contracts/src/arbitration/SortitionModuleBase.sol
- contracts/src/arbitration/KlerosCoreNeo.sol
- contracts/src/arbitration/KlerosCoreBase.sol
- contracts/src/arbitration/SortitionModuleNeo.sol
✅ Files skipped from review due to trivial changes (1)
- contracts/scripts/generateDeploymentsMarkdown.sh
🚧 Files skipped from review as they are similar to previous changes (9)
- contracts/test/foundry/KlerosCore_Governance.t.sol
- contracts/test/foundry/KlerosCore_Initialization.t.sol
- contracts/test/foundry/KlerosCore_Voting.t.sol
- contracts/test/foundry/KlerosCore_Drawing.t.sol
- contracts/test/foundry/KlerosCore_RNG.t.sol
- contracts/test/foundry/KlerosCore_Staking.t.sol
- contracts/scripts/changeOwner.ts
- contracts/test/foundry/KlerosCore_Disputes.t.sol
- contracts/test/foundry/KlerosCore_TestBase.sol
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-12-07T11:39:10.927Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.
Applied to files:
contracts/scripts/keeperBot.tscontracts/deploy/change-sortition-module-rng.tscontracts/deploy/00-home-chain-arbitration.ts
📚 Learning: 2025-09-04T23:36:16.385Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#2126
File: contracts/src/arbitration/KlerosCore.sol:472-489
Timestamp: 2025-09-04T23:36:16.385Z
Learning: In this repo, KlerosCore emits AcceptedFeeToken and NewCurrencyRate events that are declared in contracts/src/arbitration/interfaces/IArbitratorV2.sol; implementations don’t need to redeclare these events.
Applied to files:
contracts/deploy/utils/klerosCoreHelper.ts
📚 Learning: 2024-11-19T17:18:39.007Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:3-5
Timestamp: 2024-11-19T17:18:39.007Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews.
Applied to files:
contracts/scripts/populateCourts.ts
📚 Learning: 2024-11-19T16:09:41.467Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:3-17
Timestamp: 2024-11-19T16:09:41.467Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently.
Applied to files:
contracts/scripts/populateCourts.ts
🧬 Code graph analysis (8)
contracts/test/integration/getContractsViem.test.ts (2)
contracts/deployments/contractsEthers.ts (1)
getContracts(168-274)contracts/deployments/contractsViem.ts (1)
getContracts(234-342)
contracts/scripts/populateCourts.ts (1)
contracts/scripts/utils/contracts.ts (1)
Cores(23-26)
contracts/deployments/contractsEthers.ts (1)
contracts/deployments/utils.ts (1)
getAddress(25-29)
contracts/scripts/utils/contracts.ts (3)
contracts/deployments/contractsEthers.ts (1)
getContracts(168-274)contracts/deployments/contractsViem.ts (1)
getContracts(234-342)contracts/deployments/index.ts (2)
getContracts(18-18)getContracts(19-19)
contracts/deploy/00-home-chain-arbitration.ts (4)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)contracts/deployments/mainnet.viem.ts (1)
klerosCoreAddress(9808-9810)contracts/deployments/devnet.viem.ts (1)
klerosCoreAddress(12320-12322)contracts/deployments/testnet.viem.ts (1)
klerosCoreAddress(10190-10192)
contracts/test/integration/getContractsEthers.test.ts (3)
contracts/deployments/contractsEthers.ts (1)
getContracts(168-274)contracts/deployments/contractsViem.ts (1)
getContracts(234-342)contracts/scripts/utils/contracts.ts (1)
getContracts(79-135)
contracts/test/arbitration/staking-neo.ts (1)
contracts/deploy/utils/index.ts (2)
PNK(40-40)ETH(41-41)
contracts/deploy/00-home-chain-arbitration-mainnet.ts (3)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)contracts/deployments/utils.ts (1)
deployments(3-16)contracts/deploy/utils/index.ts (2)
isDevnet(30-30)PNK(40-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (47)
contracts/scripts/generateDeploymentArtifact.sh (1)
5-9: Good fix: use numeric arg comparison.Switching to
[[ $# -lt 2 ]]is correct and avoids fragile string ops.contracts/scripts/populatePolicyRegistry.ts (3)
22-22: Enum addition LGTM.
91-93: Mainnet mapping LGTM.
7-7: Config shape consistency verified. policies.v2.mainnet.json includes the required court, name, and uri fields.contracts/package.json (3)
3-3: Version bump to 0.12.0 acknowledged.
95-99: Script renames to mainnet: ensure CI/docs/use-cases updated.Double-check any references to “mainnet-neo” in docs, runbooks, and pipelines are updated to these new script names.
47-51: Confirm Node engine floor matches toolchain (Hardhat 2.26.x, global fetch).Engines list “>=16”, while Volta pins Node 20. If consumers run Node 16, builds may fail. Consider raising to >=18 (or >=20 to match Volta).
- "engines": { - "node": ">=16.0.0" - }, + "engines": { + "node": ">=18.18.0" + },Also applies to: 134-134
contracts/scripts/keeperBotShutter.ts (1)
315-316: LGTM: contract discovery simplification.
ReturninggetContractsForCoreTypedirectly makes this script consistent with others and removes ad-hoc augmentation.contracts/scripts/keeperBot.ts (2)
279-287: Keep numeric readiness check — BlockhashRNG.receiveRandomness returns uint256 (0 when not ready)
Verified: contracts/src/rng/BlockhashRNG.sol implements receiveRandomness() returning uint256 and returns 0 when not ready (e.g. checks requestTimestamp); the existing Number(n) === 0 check is correct — do not change to a try/catch.Likely an incorrect or invalid review comment.
51-52: Sortition narrow to SortitionModule safe across all core types
The keeper bot only imports and instantiatesSortitionModule(no Neo-specific factory is generated), andgetContractsForCoreTypereturns the same ABI/interface for both base and Neo cores—castingcontracts.sortitiontoSortitionModuleis valid in every supported path.contracts/scripts/populateCourts.ts (1)
136-139: Mainnet source switch LGTMSelecting v2 mainnet JSON aligns with the neo→mainnet migration.
contracts/deployments/contractsViem.ts (1)
42-58: Mainnet config imports LGTMConsistent naming and inclusion of RNG variants and snapshot proxy.
contracts/src/arbitration/KlerosCore.sol (1)
116-172: Missing event declarations used later
AcceptedFeeTokenandNewCurrencyRateare emitted but not declared. Add events here.event LeftoverRewardSent( uint256 indexed _disputeID, uint256 indexed _roundID, uint256 _pnkAmount, uint256 _feeAmount, IERC20 _feeToken ); + event AcceptedFeeToken(IERC20 indexed _feeToken, bool _accepted); + event NewCurrencyRate(IERC20 indexed _feeToken, uint64 _rateInEth, uint8 _rateDecimals); event Paused(); event Unpaused();⛔ Skipped due to learnings
Learnt from: jaybuidl PR: kleros/kleros-v2#2126 File: contracts/src/arbitration/KlerosCore.sol:472-489 Timestamp: 2025-09-04T23:36:16.385Z Learning: In this repo, KlerosCore emits AcceptedFeeToken and NewCurrencyRate events that are declared in contracts/src/arbitration/interfaces/IArbitratorV2.sol; implementations don’t need to redeclare these events.contracts/src/arbitration/SortitionModule.sol (1)
366-387: Sync global totalStaked with all stake changes
setStakePenalty, setStakeReward and withdrawLeftoverPNK bypass validateStake and don’t adjust totalStaked, desynchronizing cap checks and reporting. Add the corresponding totalStaked increments/decrements in each flow as shown above.⛔ Skipped due to learnings
Learnt from: jaybuidl PR: kleros/kleros-v2#0 File: :0-0 Timestamp: 2025-09-03T22:48:32.951Z Learning: In the Kleros v2 codebase, the team prioritizes gas optimization over strict CEI pattern compliance when dealing with trusted contracts. For penalty execution logic, they prefer batching storage writes (`round.pnkPenalties`) rather than updating incrementally after each penalty calculation to save gas costs, as the risk is extremely low between trusted contracts.contracts/test/foundry/KlerosCore_Execution.t.sol (1)
602-653: Nice coverage for executeRuling pathEnd-to-end assertions around periods, events, and idempotency look solid.
contracts/deployments/utils.ts (1)
13-16: Rename ripple effects complete: no stale references tomainnetNeofound.contracts/deploy/upgrade-all.ts (1)
6-6: LGTM on import switch.Using
getContractNamesFromNetworkis consistent with the rest of the deploy scripts.contracts/deploy/utils/klerosCoreHelper.ts (2)
1-1: LGTM on core-type unification.Removing Neo variants matches the broader PR direction.
5-10: All changeCurrencyRate invocations include the new rateDecimals argument. No 4-argument calls remain.contracts/deployments/contractsEthers.ts (3)
41-47: Mainnet import consolidation looks correct.Configs align with the factories used below.
170-172: Remove or update all references to Neo variants
- web/src/hooks/useGenesisBlock.ts still branches on isKlerosNeo and imports KlerosCoreNeo.json
- subgraph/core-neo/** and its template reference KlerosCoreNeo.json, SortitionModuleNeo.json and related event handlers
- contracts/deployments/** include Proxy definitions and JSON artifacts for KlerosCoreNeoProxy, SortitionModuleNeoProxy, etc.
⛔ Skipped due to learnings
Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1744 File: web/src/hooks/useGenesisBlock.ts:9-31 Timestamp: 2024-11-19T05:31:48.701Z Learning: In `useGenesisBlock.ts`, within the `useEffect` hook, the conditions (`isKlerosUniversity`, `isKlerosNeo`, `isTestnetDeployment`) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
246-249: mainnet config verification complete: All*Configexports incontracts/deployments/mainnet.viem.tsinclude anaddressproperty, sogetAddress(mainnet*Config, chainId)will not throw.contracts/test/integration/getContractsViem.test.ts (3)
80-87: Mainnet mapping rename looks correct.Names align with non-Neo mainnet artifacts and with contractsViem return keys. Chainlink/Randomizer RNG being required on mainnet is consistent with deployments.
243-247: Mainnet test setup updated correctly.Switch to
deployment: "mainnet"with the Arbitrum client is appropriate.
265-265: Deployed-address verification uses mainnet mapping.Matches the updated mapping and network name.
contracts/scripts/utils/contracts.ts (8)
31-35: Docs updated to reflect BASE/UNIVERSITY only.Accurately describes the supported core types.
57-57: Error message narrowed to valid core types.Clearer guidance; LGTM.
73-79: Docs forgetContractsupdated.Matches the new core model.
81-83: Union types forcore/sortitionare correct.Covers BASE/UNIVERSITY factories.
93-94: Consistent invalid-core error.Matches earlier validation.
138-142: Docs for network inference updated.Accurately describes BASE inference.
144-149: Network inference list is correct.NEO removed; only Arbitrum variants map to BASE.
152-156: Docs forgetContractNamesFromNetworkupdated.Matches the implementation.
contracts/test/integration/getContractsEthers.test.ts (4)
100-107: Mainnet mapping normalized (non-Neo).Contract names and RNG requirements match the updated deployments.
292-294: Mainnet test now targetsmainnetdeployment.Correct switch away from
mainnetNeo.
300-301: Factory expectations updated to base factories.
KlerosCore__factoryandSortitionModule__factoryare appropriate for mainnet.
311-311: Deployed-address verification aligns with mainnet mapping.Consistent with the mapping above.
contracts/deploy/00-home-chain-arbitration-mainnet.ts (10)
9-9: Typechain imports migration looks correct.Types align with the new non-Neo contracts.
47-47: RNGWithFallback retrieval is fine; ensure it’s deployed before this script.Current dependency still references ChainlinkRNG (see Lines 158-162). Update dependencies accordingly.
50-61: SortitionModule wiring with RNGWithFallback LGTM; Core address sourcing remains the only risk.Once the address prediction is hardened (above), this block is good.
88-93: Idempotent disputeKit.changeCore guard — good.Nice state check before mutating.
95-100: Idempotent rngWithFallback.changeConsumer guard — good.This avoids unnecessary txs and reverts.
128-133: DisputeKitShutter deploy + registration: LGTM.ID via length-1n is fine in a single-run deploy.
134-141: DisputeKitGated deploy + registration: LGTM.Same pattern; looks consistent.
142-149: DisputeKitGatedShutter deploy + registration: LGTM.Correctly jumps to Shutter kit.
31-36: classicDisputeKitID invariant holds (1)
The constant DISPUTE_KIT_CLASSIC is defined as 1 in src/libraries/Constants.sol and used throughout the core initializer, so the hard-coded value is correct.
68-85: KlerosCore initializer args match deploy call — no action required.Deployment passes (owner, guardian, pinakion, jurorProsecutionModule, disputeKit, hiddenVotes, courtParameters, timesPerPeriod, sortitionExtraData, sortitionModule, wNative, jurorNft), which matches initialize(...) in contracts/src/arbitration/KlerosCore.sol; deploy snippet is in contracts/deploy/00-home-chain-arbitration-mainnet.ts.
| let klerosCoreAddress = await deployments.getOrNull("KlerosCore").then((deployment) => deployment?.address); | ||
| if (!klerosCoreAddress) { | ||
| const nonce = await ethers.provider.getTransactionCount(deployer); | ||
| klerosCoreAddress = getContractAddress(deployer, nonce + 3); // deployed on the 4th tx (nonce+3): SortitionModule Impl tx, SortitionModule Proxy tx, KlerosCore Impl tx, KlerosCore Proxy tx | ||
| console.log("calculated future KlerosCoreNeo address for nonce %d: %s", nonce + 3, klerosCoreAddress); | ||
| console.log("calculated future KlerosCore address for nonce %d: %s", nonce + 3, klerosCoreAddress); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Predicting KlerosCore via nonce+3 is fragile; also uses “latest” nonce.
Any extra tx (or pending tx not counted) breaks the predicted address, causing SortitionModule to be initialized with a wrong Core address.
Apply both safeguards:
- Use "pending" nonce.
- Assert predicted vs actual right after Core deploy to fail early.
- const nonce = await ethers.provider.getTransactionCount(deployer);
+ const nonce = await ethers.provider.getTransactionCount(deployer, "pending");
klerosCoreAddress = getContractAddress(deployer, nonce + 3);
console.log("calculated future KlerosCore address for nonce %d: %s", nonce + 3, klerosCoreAddress);Add this check right after Core deployment (after Line 85):
+ if (klerosCoreAddress?.toLowerCase() !== klerosCore.address.toLowerCase()) {
+ throw new Error(
+ `Predicted KlerosCore ${klerosCoreAddress} ≠ actual ${klerosCore.address}. SortitionModule was initialized with the wrong core address.`
+ );
+ }If SortitionModule supports post-init core wiring (e.g., changeCore/setCore), prefer passing ZeroAddress here and setting it after Core deploy.
🏁 Script executed:
#!/bin/bash
# Check if SortitionModule exposes a post-init setter for Core.
rg -nP -C2 '(contract\s+SortitionModule\b|function\s+(changeCore|setCore)\s*\()' contractsLength of output: 278987
Use “pending” nonce and fail early on address mismatch
In contracts/deploy/00-home-chain-arbitration-mainnet.ts, change the nonce lookup and add an assertion immediately after deploying KlerosCore:
// Before predicting the future KlerosCore proxy address
- const nonce = await ethers.provider.getTransactionCount(deployer);
+ const nonce = await ethers.provider.getTransactionCount(deployer, "pending");
klerosCoreAddress = getContractAddress(deployer, nonce + 3);
console.log("calculated future KlerosCore address for nonce %d: %s", nonce + 3, klerosCoreAddress);
// ...deploy KlerosCore here...
+ // Fail early if prediction was wrong
+ if (klerosCoreAddress.toLowerCase() !== klerosCore.address.toLowerCase()) {
+ throw new Error(
+ `Predicted KlerosCore ${klerosCoreAddress} ≠ actual ${klerosCore.address}`
+ );
+ }SortitionModule has no post‐init setter for core, so drop the ZeroAddress‐and‐set‐later suggestion.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let klerosCoreAddress = await deployments.getOrNull("KlerosCore").then((deployment) => deployment?.address); | |
| if (!klerosCoreAddress) { | |
| const nonce = await ethers.provider.getTransactionCount(deployer); | |
| klerosCoreAddress = getContractAddress(deployer, nonce + 3); // deployed on the 4th tx (nonce+3): SortitionModule Impl tx, SortitionModule Proxy tx, KlerosCore Impl tx, KlerosCore Proxy tx | |
| console.log("calculated future KlerosCoreNeo address for nonce %d: %s", nonce + 3, klerosCoreAddress); | |
| console.log("calculated future KlerosCore address for nonce %d: %s", nonce + 3, klerosCoreAddress); | |
| } | |
| let klerosCoreAddress = await deployments | |
| .getOrNull("KlerosCore") | |
| .then((deployment) => deployment?.address); | |
| if (!klerosCoreAddress) { | |
| // Before predicting the future KlerosCore proxy address | |
| const nonce = await ethers.provider.getTransactionCount(deployer, "pending"); | |
| klerosCoreAddress = getContractAddress( | |
| deployer, | |
| nonce + 3 | |
| ); // deployed on the 4th tx (nonce+3): SortitionModule Impl tx, SortitionModule Proxy tx, KlerosCore Impl tx, KlerosCore Proxy tx | |
| console.log( | |
| "calculated future KlerosCore address for nonce %d: %s", | |
| nonce + 3, | |
| klerosCoreAddress | |
| ); | |
| } | |
| // …now deploy KlerosCore… | |
| const klerosCore = await deployments.deploy("KlerosCore", { | |
| from: deployer, | |
| // …args… | |
| }); | |
| // Fail early if prediction was wrong | |
| if ( | |
| klerosCoreAddress.toLowerCase() !== | |
| klerosCore.address.toLowerCase() | |
| ) { | |
| throw new Error( | |
| `Predicted KlerosCore ${klerosCoreAddress} ≠ actual ${klerosCore.address}` | |
| ); | |
| } |
🤖 Prompt for AI Agents
In contracts/deploy/00-home-chain-arbitration-mainnet.ts around lines 38-43,
replace the current nonce lookup with
ethers.provider.getTransactionCount(deployer, "pending") so you compute the
future address including pending txs; immediately after deploying KlerosCore
assert that the deployment address equals getContractAddress(deployer,
pendingNonce + 3) and fail/throw if it does not to catch mismatches early; do
not use a zero address workaround for SortitionModule — ensure SortitionModule
is initialized with the correct core address during deployment rather than
setting core later.
| } | ||
|
|
||
| task("populate:policy-registry", "Populates the policy registry for each court") | ||
| .addOptionalParam( | ||
| "from", | ||
| "The source of the policies between v1_mainnet, v1_gnosis, v2_devnet, v2_testnet, v2_mainnet_neo (default: auto depending on the network)", | ||
| "The source of the policies between v1_mainnet, v1_gnosis, v2_devnet, v2_testnet, v2_mainnet (default: auto depending on the network)", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Help text claims auto-detection but defaults don’t select mainnet—update text and logic.
Either adjust text or fix the default to also detect mainnet. Prefer fixing default; see next comment.
Apply to help text:
- "The source of the policies between v1_mainnet, v1_gnosis, v2_devnet, v2_testnet, v2_mainnet (default: auto depending on the network)",
+ "The source of the policies: v1_mainnet | v1_gnosis | v2_devnet | v2_testnet | v2_mainnet (default: auto by network: devnet/testnet/mainnet)",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "The source of the policies between v1_mainnet, v1_gnosis, v2_devnet, v2_testnet, v2_mainnet (default: auto depending on the network)", | |
| "The source of the policies: v1_mainnet | v1_gnosis | v2_devnet | v2_testnet | v2_mainnet (default: auto by network: devnet/testnet/mainnet)", |
| const buildInfo = await hre.artifacts.getBuildInfo(`src/arbitration/KlerosCore.sol:KlerosCore`); | ||
| console.log(buildInfo.output.contracts["src/arbitration/KlerosCore.sol"]["KlerosCore"].storageLayout); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden against missing build info and absent storageLayout
Prevents runtime crashes and adds clearer diagnostics.
- const buildInfo = await hre.artifacts.getBuildInfo(`src/arbitration/KlerosCore.sol:KlerosCore`);
- console.log(buildInfo.output.contracts["src/arbitration/KlerosCore.sol"]["KlerosCore"].storageLayout);
+ const fqName = "src/arbitration/KlerosCore.sol:KlerosCore";
+ const buildInfo = await hre.artifacts.getBuildInfo(fqName);
+ if (!buildInfo) {
+ throw new Error(`Build info not found for ${fqName}. Try: npx hardhat clean && npx hardhat compile`);
+ }
+ const [src, name] = fqName.split(":");
+ const entry = buildInfo.output.contracts?.[src]?.[name];
+ if (!entry?.storageLayout) {
+ console.error(`Storage layout not available for ${fqName}. Ensure solc outputSelection includes "storageLayout".`);
+ return;
+ }
+ console.log(entry.storageLayout);🤖 Prompt for AI Agents
In contracts/scripts/storage-layout.ts around lines 7 to 9, the code assumes
hre.artifacts.getBuildInfo(...) and the storageLayout exist and can crash at
runtime; modify it to check that buildInfo is not null/undefined and that
buildInfo.output.contracts[...]["KlerosCore"].storageLayout exists before
accessing it, log clear diagnostic messages when either is missing (including
the artifact path used), and fail early (throw an Error or process.exit(1)) so
the script does not continue with undefined values.
| function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external whenNotPaused { | ||
| Round storage round; | ||
| { | ||
| Dispute storage dispute = disputes[_disputeID]; | ||
| if (dispute.period != Period.execution) revert NotExecutionPeriod(); | ||
|
|
||
| round = dispute.rounds[_round]; | ||
| } // stack too deep workaround | ||
|
|
||
| uint256 start = round.repartitions; | ||
| uint256 end = round.repartitions + _iterations; | ||
|
|
||
| uint256 pnkPenaltiesInRound = round.pnkPenalties; // Keep in memory to save gas. | ||
| uint256 numberOfVotesInRound = round.drawnJurors.length; | ||
| uint256 feePerJurorInRound = round.totalFeesForJurors / numberOfVotesInRound; | ||
| uint256 pnkAtStakePerJurorInRound = round.pnkAtStakePerJuror; | ||
| uint256 coherentCount; | ||
| { | ||
| IDisputeKit disputeKit = disputeKits[round.disputeKitID]; | ||
| coherentCount = disputeKit.getCoherentCount(_disputeID, _round); // Total number of jurors that are eligible to a reward in this round. | ||
| } // stack too deep workaround | ||
|
|
||
| if (coherentCount == 0) { | ||
| // We loop over the votes once as there are no rewards because it is not a tie and no one in this round is coherent with the final outcome. | ||
| if (end > numberOfVotesInRound) end = numberOfVotesInRound; | ||
| } else { | ||
| // We loop over the votes twice, first to collect the PNK penalties, and second to distribute them as rewards along with arbitration fees. | ||
| if (end > numberOfVotesInRound * 2) end = numberOfVotesInRound * 2; | ||
| } | ||
| round.repartitions = end; | ||
|
|
||
| for (uint256 i = start; i < end; i++) { | ||
| if (i < numberOfVotesInRound) { | ||
| pnkPenaltiesInRound = _executePenalties( | ||
| ExecuteParams({ | ||
| disputeID: _disputeID, | ||
| round: _round, | ||
| coherentCount: coherentCount, | ||
| numberOfVotesInRound: numberOfVotesInRound, | ||
| feePerJurorInRound: feePerJurorInRound, | ||
| pnkAtStakePerJurorInRound: pnkAtStakePerJurorInRound, | ||
| pnkPenaltiesInRound: pnkPenaltiesInRound, | ||
| repartition: i | ||
| }) | ||
| ); | ||
| } else { | ||
| _executeRewards( | ||
| ExecuteParams({ | ||
| disputeID: _disputeID, | ||
| round: _round, | ||
| coherentCount: coherentCount, | ||
| numberOfVotesInRound: numberOfVotesInRound, | ||
| feePerJurorInRound: feePerJurorInRound, | ||
| pnkAtStakePerJurorInRound: pnkAtStakePerJurorInRound, | ||
| pnkPenaltiesInRound: pnkPenaltiesInRound, | ||
| repartition: i | ||
| }) | ||
| ); | ||
| } | ||
| } | ||
| if (round.pnkPenalties != pnkPenaltiesInRound) { | ||
| round.pnkPenalties = pnkPenaltiesInRound; // Reentrancy risk: breaks Check-Effect-Interact | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Reentrancy window in execute() around external transfers
ETH transfers via safeSend and ERC20 transfers occur before persisting round.pnkPenalties; reentrancy can observe stale state and distort reward math.
Mitigations (pick one):
- Guard: add OZ ReentrancyGuardTransient and mark
executeasnonReentrant. - Or, refactor to persist state before any external transfer and pay out after.
Example guard:
- contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
+ import {ReentrancyGuardTransient} from "@openzeppelin/contracts/utils/ReentrancyGuardTransient.sol";
+ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable, ReentrancyGuardTransient {
...
- function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external whenNotPaused {
+ function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external whenNotPaused nonReentrant {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external whenNotPaused { | |
| Round storage round; | |
| { | |
| Dispute storage dispute = disputes[_disputeID]; | |
| if (dispute.period != Period.execution) revert NotExecutionPeriod(); | |
| round = dispute.rounds[_round]; | |
| } // stack too deep workaround | |
| uint256 start = round.repartitions; | |
| uint256 end = round.repartitions + _iterations; | |
| uint256 pnkPenaltiesInRound = round.pnkPenalties; // Keep in memory to save gas. | |
| uint256 numberOfVotesInRound = round.drawnJurors.length; | |
| uint256 feePerJurorInRound = round.totalFeesForJurors / numberOfVotesInRound; | |
| uint256 pnkAtStakePerJurorInRound = round.pnkAtStakePerJuror; | |
| uint256 coherentCount; | |
| { | |
| IDisputeKit disputeKit = disputeKits[round.disputeKitID]; | |
| coherentCount = disputeKit.getCoherentCount(_disputeID, _round); // Total number of jurors that are eligible to a reward in this round. | |
| } // stack too deep workaround | |
| if (coherentCount == 0) { | |
| // We loop over the votes once as there are no rewards because it is not a tie and no one in this round is coherent with the final outcome. | |
| if (end > numberOfVotesInRound) end = numberOfVotesInRound; | |
| } else { | |
| // We loop over the votes twice, first to collect the PNK penalties, and second to distribute them as rewards along with arbitration fees. | |
| if (end > numberOfVotesInRound * 2) end = numberOfVotesInRound * 2; | |
| } | |
| round.repartitions = end; | |
| for (uint256 i = start; i < end; i++) { | |
| if (i < numberOfVotesInRound) { | |
| pnkPenaltiesInRound = _executePenalties( | |
| ExecuteParams({ | |
| disputeID: _disputeID, | |
| round: _round, | |
| coherentCount: coherentCount, | |
| numberOfVotesInRound: numberOfVotesInRound, | |
| feePerJurorInRound: feePerJurorInRound, | |
| pnkAtStakePerJurorInRound: pnkAtStakePerJurorInRound, | |
| pnkPenaltiesInRound: pnkPenaltiesInRound, | |
| repartition: i | |
| }) | |
| ); | |
| } else { | |
| _executeRewards( | |
| ExecuteParams({ | |
| disputeID: _disputeID, | |
| round: _round, | |
| coherentCount: coherentCount, | |
| numberOfVotesInRound: numberOfVotesInRound, | |
| feePerJurorInRound: feePerJurorInRound, | |
| pnkAtStakePerJurorInRound: pnkAtStakePerJurorInRound, | |
| pnkPenaltiesInRound: pnkPenaltiesInRound, | |
| repartition: i | |
| }) | |
| ); | |
| } | |
| } | |
| if (round.pnkPenalties != pnkPenaltiesInRound) { | |
| round.pnkPenalties = pnkPenaltiesInRound; // Reentrancy risk: breaks Check-Effect-Interact | |
| } | |
| } | |
| // At the top of contracts/src/arbitration/KlerosCore.sol | |
| import {ReentrancyGuardTransient} from "@openzeppelin/contracts/utils/ReentrancyGuardTransient.sol"; | |
| contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable, ReentrancyGuardTransient { | |
| ... | |
| function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external whenNotPaused nonReentrant { | |
| Round storage round; | |
| { | |
| Dispute storage dispute = disputes[_disputeID]; | |
| if (dispute.period != Period.execution) revert NotExecutionPeriod(); | |
| round = dispute.rounds[_round]; | |
| } // stack too deep workaround | |
| ... | |
| if (round.pnkPenalties != pnkPenaltiesInRound) { | |
| round.pnkPenalties = pnkPenaltiesInRound; // Reentrancy risk: breaks Check-Effect-Interact | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
contracts/src/arbitration/KlerosCore.sol lines 737-801: execute() performs
external transfers (safeSend / ERC20 transfers) before persisting
round.pnkPenalties which opens a reentrancy window where external calls can
observe stale contract state and distort reward math; fix by either (A)
persisting all mutated state (e.g., update round.pnkPenalties and any other
round/dispute fields) before any external transfer and move all transfer logic
after those state writes, or (B) add OpenZeppelin's ReentrancyGuardTransient and
mark execute as nonReentrant to prevent reentrant calls; implement one approach
consistently (prefer persisting state-first, and if using a guard, import and
apply guard modifier to execute).
| function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) { | ||
| return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / currencyRates[_toToken].rateInEth; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Division-by-zero risk in convertEthToTokenAmount
If the token was not configured (rateInEth == 0), this reverts with a raw division. Fail fast with a clear error.
- function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) {
- return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / currencyRates[_toToken].rateInEth;
- }
+ function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) {
+ CurrencyRate memory rate = currencyRates[_toToken];
+ if (rate.rateInEth == 0) revert TokenNotAccepted();
+ return (_amountInEth * 10 ** rate.rateDecimals) / rate.rateInEth;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) { | |
| return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / currencyRates[_toToken].rateInEth; | |
| } | |
| function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) { | |
| CurrencyRate memory rate = currencyRates[_toToken]; | |
| if (rate.rateInEth == 0) revert TokenNotAccepted(); | |
| return (_amountInEth * 10 ** rate.rateDecimals) / rate.rateInEth; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/KlerosCore.sol around lines 1109 to 1111, the
function convertEthToTokenAmount performs a division by
currencyRates[_toToken].rateInEth without checking for zero; add a guard that
requires currencyRates[_toToken].rateInEth != 0 and revert with a clear error
message (e.g., "Token rate not configured") before performing the arithmetic so
the function fails fast instead of causing a raw division-by-zero revert.
| /// @dev Changes the owner of the contract. | ||
| /// @param _owner The new owner. | ||
| function changeOwner(address _owner) external onlyByOwner { | ||
| owner = _owner; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against zero-address owner and emit OwnerChanged
Prevent bricking upgrades/access control by setting owner to address(0). Also emit the new event.
function changeOwner(address _owner) external onlyByOwner {
- owner = _owner;
+ if (_owner == address(0)) revert ZeroAddressOwner();
+ emit OwnerChanged(owner, _owner);
+ owner = _owner;
}Add the error to the errors block (see separate comment).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/src/arbitration/SortitionModule.sol around lines 158-163, the
changeOwner function currently allows setting owner to address(0) and does not
emit an event; update it to revert when _owner == address(0) (use the new custom
error you added to the errors block), assign the owner only after the check, and
emit an OwnerChanged(address indexed previousOwner, address indexed newOwner)
event (add that event to the events section if missing) to record the change.
| function unlockStake(address _account, uint256 _relativeAmount) external override onlyByCore { | ||
| Juror storage juror = jurors[_account]; | ||
| juror.lockedPnk -= _relativeAmount; | ||
| emit StakeLocked(_account, _relativeAmount, true); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clamp unlock amount to avoid underflow
If _relativeAmount > lockedPnk, subtraction underflows. Clamp to current lock (or revert).
function unlockStake(address _account, uint256 _relativeAmount) external override onlyByCore {
Juror storage juror = jurors[_account];
- juror.lockedPnk -= _relativeAmount;
+ if (_relativeAmount > juror.lockedPnk) {
+ _relativeAmount = juror.lockedPnk;
+ }
+ juror.lockedPnk -= _relativeAmount;
emit StakeLocked(_account, _relativeAmount, true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function unlockStake(address _account, uint256 _relativeAmount) external override onlyByCore { | |
| Juror storage juror = jurors[_account]; | |
| juror.lockedPnk -= _relativeAmount; | |
| emit StakeLocked(_account, _relativeAmount, true); | |
| function unlockStake(address _account, uint256 _relativeAmount) external override onlyByCore { | |
| Juror storage juror = jurors[_account]; | |
| // Clamp to avoid underflow (or revert) if asked to unlock more than is locked | |
| if (_relativeAmount > juror.lockedPnk) { | |
| _relativeAmount = juror.lockedPnk; | |
| } | |
| juror.lockedPnk -= _relativeAmount; | |
| emit StakeLocked(_account, _relativeAmount, true); | |
| } |
| core.draw(disputeID, 7); | ||
| emit KlerosCore.NewPeriod(disputeID, KlerosCore.Period.vote); // Check that we don't have to wait for the timeout to pass the evidence period after appeal | ||
| core.passPeriod(disputeID); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing expect for event — the test currently emits from the test contract, not asserting Core’s emission.
Add vm.expectEmit before the expected event so the subsequent call is checked.
core.draw(disputeID, 7);
- emit KlerosCore.NewPeriod(disputeID, KlerosCore.Period.vote); // Check that we don't have to wait for the timeout to pass the evidence period after appeal
+ vm.expectEmit(true, true, true, true, address(core));
+ emit KlerosCore.NewPeriod(disputeID, KlerosCore.Period.vote); // Check that we don't have to wait for the timeout to pass the evidence period after appeal
core.passPeriod(disputeID);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| core.draw(disputeID, 7); | |
| emit KlerosCore.NewPeriod(disputeID, KlerosCore.Period.vote); // Check that we don't have to wait for the timeout to pass the evidence period after appeal | |
| core.passPeriod(disputeID); | |
| } | |
| core.draw(disputeID, 7); | |
| vm.expectEmit(true, true, true, true, address(core)); | |
| emit KlerosCore.NewPeriod(disputeID, KlerosCore.Period.vote); // Check that we don't have to wait for the timeout to pass the evidence period after appeal | |
| core.passPeriod(disputeID); | |
| } |
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Appeals.t.sol around lines 206 to 209, the
test currently emits the NewPeriod event from the test contract instead of
asserting that KlerosCore emitted it; add a vm.expectEmit call immediately
before the expected event emission so the test framework checks Core’s emission
(configure the topic matching as needed), then keep the subsequent emit line and
the core.passPeriod(disputeID) call so the expect covers the actual contract
call that should produce the event.
PR-Codex overview
This PR primarily focuses on renaming the
governorvariable toowneracross multiple contracts and scripts, while also updating related function names and access control modifiers. It includes adjustments to contract deployment scripts and interface definitions.Detailed summary
governortoownerin contracts and scripts.onlyByGovernortoonlyByOwner.changeGovernortochangeOwner.Summary by CodeRabbit