Staking refactor including bug fixes #2021
Conversation
… removed SortitionModule refs
Neo NFT checking moved from CoreNeo to VaultNeo
…ck rather a returned error enum
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis update introduces a major architectural overhaul of the arbitration, staking, and vault system. The legacy SortitionModule and related contracts are removed, replaced by a modular architecture featuring new StakeController, Vault, and SortitionSumTree contracts, each with dedicated interfaces. The deployment scripts, core contracts, dispute kits, and associated proxies are refactored to integrate these new components and their upgraded initialization and interaction patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant Deployer
participant Vault
participant StakeController
participant SortitionSumTree
participant KlerosCore
participant DisputeKit
participant RNG
Deployer->>Vault: Deploy (governor, token, stakeController, core)
Deployer->>SortitionSumTree: Deploy (governor, stakeController)
Deployer->>StakeController: Deploy (governor, core, vault, sortitionSumTree, timings, RNG)
Deployer->>KlerosCore: Deploy (governor, guardian, prosecution, disputeKit, stakeController, vault, ...)
Deployer->>DisputeKit: Deploy (core)
KlerosCore->>StakeController: setStake / setStakeDelayed / lockStake / unlockStake / penalties
StakeController->>Vault: deposit / withdraw / lockTokens / unlockTokens / applyPenalty
StakeController->>SortitionSumTree: setStake / draw
StakeController->>RNG: Request random number (during phase change)
StakeController->>Vault: transferReward (for juror rewards)
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🔭 Outside diff range comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
210-236: 🛠️ Refactor suggestionUnchecked assumptions around
_nonceuniqueness and juror re-draws.
stakeController.drawis invoked with an externally supplied_nonce, but no contract-side check guarantees that the same(disputeID, nonce)pair is not reused.
Reusing the nonce would let the caller “re-roll” until a desired juror passes_postDrawCheck, compromising randomness.Guard recommendation:
+// inside draw(..) +require(core.isNonceUnused(_coreDisputeID, _nonce), "Nonce already used"); +core.setNonceUsed(_coreDisputeID, _nonce);(or equivalent tracking inside this DK).
Also consider emitting an event for every successful draw to simplify off-chain auditing.
🧹 Nitpick comments (18)
contracts/src/arbitration/Vault.sol (1)
40-44: Minor:_authorizeUpgrademarkedviewunnecessarily.OpenZeppelin’s
_authorizeUpgradeis non-view; addingviewis allowed but unusual and prevents emitting events or writing state inside the hook in the future.
Consider removingviewfor consistency with OZ patterns.contracts/src/arbitration/StakeController.sol (1)
61-65: Same remark as inVault: avoid declaring_authorizeUpgradeasview.Leaving it non-view keeps the door open for future state changes (e.g., upgrade counters) without breaking the signature hierarchy.
contracts/src/arbitration/interfaces/ISortitionSumTree.sol (2)
10-18: Missing IERC-165 support for interface discoveryMost consumer contracts rely on ERC-165 to verify that a target supports an interface.
Consider addingtype(ISortitionSumTree).interfaceIdto a dedicated constants library or extend the base implementation withsupportsInterface(bytes4).
47-58: Docstring vs. signature mismatch (stakeOf)The second overload comment says “stake of a juror in a court by tree key and stake path ID” but the parameter name
_courtIDhas been replaced by_key.
Clarify wording to avoid confusion.contracts/deploy/00-home-chain-arbitration-v2.ts (3)
48-50: Comment typo and outdated nameThe inline comment still refers to “SortitionModule” and contains a duplicate comma. Minor, but can mislead future maintainers.
69-71: Hard-coded timing constants hamper test flexibility
minStakingTime/maxDrawingTimeare set once based onisDevnet.
Expose them through environment variables or script parameters so integration tests can tweak timings without editing code.
121-127: RNG access-control variable naming inconsistencyThe TODO comment notes that
sortitionModuleon RNG is misleading.
Consider opening an issue + migration plan now; otherwise this confusion will persist into production deployments.contracts/src/arbitration/interfaces/IVault.sol (2)
46-55:applyPenaltyreturn values under-specifiedDocstring says the first value “including locked PNK”, but callers also need the unlocked remaining balance to decide whether additional penalties can be applied.
Consider returning(uint256 unlockedBalance, uint256 lockedBalance, uint256 actualPenalty)for clarity.
14-20: Event suite lacksPenaltyAppliedgranularityUsing a generic
Penaltyevent makes it hard to index who triggered the action (e.g. core vs governor).
Add anindexed address initiatorfield or emit a dedicatedPenaltyApplied(address indexed juror, address indexed caller, uint256 amount).contracts/src/arbitration/KlerosCoreNeo.sol (1)
95-97: Minor gas saving by caching the whitelist flagThe storage read is performed twice (check + implicit return). Cache it once.
- if (!arbitrableWhitelist[msg.sender]) revert ArbitrableNotWhitelisted(); - return super._createDispute(_numberOfChoices, _extraData, _feeToken, _feeAmount); + bool allowed = arbitrableWhitelist[msg.sender]; + if (!allowed) revert ArbitrableNotWhitelisted(); + return super._createDispute(_numberOfChoices, _extraData, _feeToken, _feeAmount);contracts/src/arbitration/KlerosCore.sol (1)
63-67: Doc-comment noiseAn empty block with a verbose comment adds little value—condense it.
- /// @notice Access Control to perform implementation upgrades (UUPS Proxiable) - /// Only the governor can perform upgrades (`onlyByGovernor`) + /// @notice UUPS upgrade authorization – restricted via `onlyByGovernor`contracts/src/arbitration/VaultNeo.sol (2)
128-132: Move thetotalDepositeddecrement after the callSafer (and clearer) to update state only after
super._withdrawsucceeds.- totalDeposited -= _amount; - return super._withdraw(_to, _amount); + uint256 pnkAmount = super._withdraw(_to, _amount); + totalDeposited -= _amount; + return pnkAmount;
102-120: Micro-optimisation: unchecked arithmetic & cached balanceSaves ~200 gas and removes a redundant addition overflow check that’s already guarded.
- uint256 currentUserDeposit = jurorBalances[_from].deposited; - if (currentUserDeposit + _amount > maxDepositPerUser) { + uint256 currentUserDeposit = jurorBalances[_from].deposited; + unchecked { + uint256 newTotal = currentUserDeposit + _amount; + if (newTotal > maxDepositPerUser) { revert ExceedsMaxDepositPerUser(); - } + } + }contracts/src/arbitration/SortitionSumTreeBase.sol (1)
246-255: Safer default for tree branching factor
DEFAULT_Kis silently used when_extraDatais shorter than 32 bytes. Consider validating thatDEFAULT_K > 1(or re-using theInvalidTreeKrevert) to avoid accidentally deploying 1-ary trees due to mis-configured extraData.contracts/src/arbitration/KlerosCoreBase.sol (1)
471-483: Docstring & signature driftThe NatSpec comment mentions
_depositPreFunded, but the parameter list no longer contains it.
Please update the comment to avoid confusion for integrators.contracts/src/arbitration/VaultBase.sol (1)
82-99: Governance setters emit no eventsChanging
governor,stakeController, orcoreis security-sensitive; on-chain monitoring tools rely on events.
Emit specific events (e.g.GovernorChanged(oldGov, newGov)) in each setter.contracts/src/arbitration/StakeControllerBase.sol (2)
276-286: Event naming inconsistency may break off-chain parsersThe comment hints that
emit StakeLockedis mis-named for backwards compatibility.
If you intentionally keep the old name, drop the comment or add a dedicated eventTokensLockedto avoid confusion.
402-425: Micro-optimisation: replace O(n) search with indexed removalWhen the number of courts per juror grows,
_removeCourt’s linear scan becomes expensive.
Maintaining an additionalmapping(courtId ⇒ index)lets you delete in O(1) while preserving constant gas per call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
contracts/deploy/00-home-chain-arbitration-v2-neo.ts(1 hunks)contracts/deploy/00-home-chain-arbitration-v2.ts(1 hunks)contracts/deploy/utils/klerosCoreHelper.ts(1 hunks)contracts/src/arbitration/KlerosCore.sol(1 hunks)contracts/src/arbitration/KlerosCoreBase.sol(19 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(2 hunks)contracts/src/arbitration/SortitionModule.sol(0 hunks)contracts/src/arbitration/SortitionModuleBase.sol(0 hunks)contracts/src/arbitration/SortitionModuleNeo.sol(0 hunks)contracts/src/arbitration/SortitionSumTree.sol(1 hunks)contracts/src/arbitration/SortitionSumTreeBase.sol(1 hunks)contracts/src/arbitration/StakeController.sol(1 hunks)contracts/src/arbitration/StakeControllerBase.sol(1 hunks)contracts/src/arbitration/Vault.sol(1 hunks)contracts/src/arbitration/VaultBase.sol(1 hunks)contracts/src/arbitration/VaultNeo.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(7 hunks)contracts/src/arbitration/interfaces/ISortitionSumTree.sol(1 hunks)contracts/src/arbitration/interfaces/IStakeController.sol(1 hunks)contracts/src/arbitration/interfaces/IVault.sol(1 hunks)contracts/src/libraries/Constants.sol(1 hunks)contracts/src/test/SortitionModuleMock.sol(1 hunks)
💤 Files with no reviewable changes (3)
- contracts/src/arbitration/SortitionModule.sol
- contracts/src/arbitration/SortitionModuleBase.sol
- contracts/src/arbitration/SortitionModuleNeo.sol
🧰 Additional context used
🧬 Code Graph Analysis (1)
contracts/deploy/00-home-chain-arbitration-v2-neo.ts (4)
contracts/deployments/utils.ts (1)
deployments(3-16)contracts/deploy/utils/deployTokens.ts (2)
deployERC20AndFaucet(6-17)deployERC721(55-67)contracts/deploy/utils/getContractOrDeploy.ts (2)
getContractOrDeploy(6-20)getContractOrDeployUpgradable(22-36)contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)
🪛 Biome (1.9.4)
contracts/deploy/utils/klerosCoreHelper.ts
[error] 1-1: Declarations inside of a import declaration may not have duplicates
a second declaration of KlerosCore is not allowed
KlerosCore is first declared here
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: SonarCloud
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (12)
contracts/src/test/SortitionModuleMock.sol (2)
13-17: LGTM: Proper migration to new SortitionSumTree architecture.The changes correctly update the mock to inherit from
SortitionSumTreeinstead of the deprecatedSortitionModule, maintaining the same testing interface for the refactored architecture.
18-22: LGTM: Function maintains testing interface.The function correctly accesses the
sortitionSumTreesmapping and tree properties, maintaining the same testing interface after the architectural migration.contracts/src/libraries/Constants.sol (1)
23-27: LGTM: Proper deprecation of OnError enum.The deprecation of the
OnErrorenum aligns with the architectural refactor to the new StakeController system.contracts/src/arbitration/SortitionSumTree.sol (5)
5-6: LGTM: Proper imports for modular architecture.The imports correctly reference the base contract and interface, supporting the new modular design that separates tree operations from stake management.
8-12: LGTM: Clear contract documentation and versioning.The documentation clearly defines the contract's focused responsibility within the new modular architecture. The version "2.0.0" appropriately reflects the major architectural changes.
18-21: LGTM: Proper upgradeable contract constructor.The constructor correctly disables initializers following OpenZeppelin best practices for upgradeable contracts.
26-28: LGTM: Proper initialization for upgradeable contract.The initialize function correctly implements the upgradeable contract pattern, taking necessary parameters and delegating to the base contract initializer.
36-38: LGTM: Secure upgrade authorization.The upgrade authorization correctly restricts implementation upgrades to the governor only, following secure UUPS proxy pattern best practices.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
623-629:❌ Incorrect review comment
Possible parameter-order mismatch with
getJurorBalance→ risk of wrong insolvency checks.Only
totalLockedandtotalStakedare used, but the tuple de-structuring assumes they are returned in positions 1 and 3:(, uint256 totalLocked, , uint256 totalStaked, , ) = ...Verify the actual signature in
IStakeController:function getJurorBalance(address juror, uint96 courtID) external view returns (uint256 staked, uint256 locked, …)If the order is
(staked, locked, …),totalLockedandtotalStakedare being swapped, letting insolvent jurors slip through.
Please cross-check and swap the indices if needed.
🏁 Script executed:
#!/bin/bash # Locate and print the signature of getJurorBalance for confirmation rg -n "function getJurorBalance" -A2Length of output: 83834
🏁 Script executed:
#!/bin/bash # Find the IStakeController interface to confirm getJurorBalance signature rg -l "interface IStakeController" -n src/arbitration/interfacesLength of output: 133
🏁 Script executed:
#!/bin/bash # Search for IStakeController interface to confirm getJurorBalance signature rg -n "interface IStakeController" -A2 contracts/src/arbitrationLength of output: 374
🏁 Script executed:
#!/bin/bash # Retrieve the getJurorBalance signature from IStakeController rg -n "function getJurorBalance" -A2 contracts/src/arbitration/interfaces/IStakeController.solLength of output: 186
🏁 Script executed:
#!/bin/bash # Print the full signature of getJurorBalance including its returns sed -n '160,200p' contracts/src/arbitration/interfaces/IStakeController.solLength of output: 1543
Ignore the getJurorBalance order mismatch – the tuple indices are correct
IConfirmed that IStakeController.getJurorBalance is declared as:
function getJurorBalance( address _juror, uint96 _courtID ) external view returns ( uint256 availablePnk, // 1 uint256 lockedPnk, // 2 uint256 penaltyPnk, // 3 uint256 totalStaked, // 4 uint256 stakedInCourt, // 5 uint256 nbCourts // 6 );The destructuring in DisputeKitClassicBase.sol:
(, uint256 totalLocked, , uint256 totalStaked, , ) = core.stakeController().getJurorBalance(_juror, courtID);correctly maps
lockedPnk→totalLocked(2nd slot) andtotalStaked→totalStaked(4th slot). No swap is needed.Likely an incorrect or invalid review comment.
contracts/src/arbitration/KlerosCore.sol (1)
22-44: Confirm initializer versioningThis contract uses
initializer, while other modules usereinitializer(n).
If an existing proxy has already consumed itsinitializer, this call will revert. Ensure the proxy is always fresh or switch toreinitializer(2)(or the next slot).contracts/src/arbitration/KlerosCoreBase.sol (1)
779-805: Unchecked external call may revert entire penalty loop
vault.withdrawis wrapped intry/catch, good.
vault.transferReward(used both here and in_executeRewards) isn’t – a single vault failure would revert the wholeexecute()call and block dispute finalisation.
Wrap it similarly or make the vault return a boolean to keep liveness.contracts/src/arbitration/StakeControllerBase.sol (1)
176-199: Gas/overflow safety inexecuteDelayedStakesloop
actualIterationsarithmetic can overflow for very large_iterations(> 2⁵⁶).- Processing user-supplied
_iterationswithout an upper cap can still make the call run out-of-gas even if_iterationsis small but each inner call is expensive.Consider:
- Using
uncheckedwith explicit bounds checks to save gas and avoid overflow.- Hard-cap
actualIterationsto e.g.500per call.
| import { BigNumberish, toBigInt } from "ethers"; | ||
|
|
||
| export const changeCurrencyRate = async ( | ||
| core: KlerosCore | KlerosCoreNeo | KlerosCoreRuler | KlerosCoreUniversity, | ||
| core: KlerosCore | KlerosCoreNeo | KlerosCoreRuler | KlerosCoreUniversity | KlerosCore, |
There was a problem hiding this comment.
Fix duplicate type in parameter annotation.
The core parameter type annotation contains a duplicate KlerosCore type in the union.
Apply this diff to remove the duplicate:
- core: KlerosCore | KlerosCoreNeo | KlerosCoreRuler | KlerosCoreUniversity | KlerosCore,
+ core: KlerosCore | KlerosCoreNeo | KlerosCoreRuler | KlerosCoreUniversity,📝 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: KlerosCore | KlerosCoreNeo | KlerosCoreRuler | KlerosCoreUniversity | KlerosCore, | |
| core: KlerosCore | KlerosCoreNeo | KlerosCoreRuler | KlerosCoreUniversity, |
🤖 Prompt for AI Agents
In contracts/deploy/utils/klerosCoreHelper.ts at line 5, the type annotation for
the parameter 'core' includes a duplicate 'KlerosCore' in the union type. Remove
the repeated 'KlerosCore' so that each type appears only once in the union.
| @@ -2,7 +1,8 @@ | |||
| import { KlerosCore, KlerosCoreNeo, KlerosCoreRuler, KlerosCoreUniversity, KlerosCore } from "../../typechain-types"; | |||
There was a problem hiding this comment.
Fix duplicate import causing syntax error.
The import statement contains a duplicate KlerosCore import, which will cause a compilation error.
Apply this diff to remove the duplicate:
-import { KlerosCore, KlerosCoreNeo, KlerosCoreRuler, KlerosCoreUniversity, KlerosCore } from "../../typechain-types";
+import { KlerosCore, KlerosCoreNeo, KlerosCoreRuler, KlerosCoreUniversity } from "../../typechain-types";🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Declarations inside of a import declaration may not have duplicates
a second declaration of KlerosCore is not allowed
KlerosCore is first declared here
(parse)
🤖 Prompt for AI Agents
In contracts/deploy/utils/klerosCoreHelper.ts at line 1, the import statement
includes a duplicate import of KlerosCore, causing a syntax error. Remove the
repeated KlerosCore from the import list so that each imported item appears only
once.
| enum StakingResult { | ||
| Successful, | ||
| StakingTransferFailed, | ||
| UnstakingTransferFailed, | ||
| CannotStakeInMoreCourts, | ||
| CannotStakeInThisCourt, | ||
| CannotStakeLessThanMinStake, | ||
| CannotStakeMoreThanMaxStakePerJuror, | ||
| CannotStakeMoreThanMaxTotalStaked, | ||
| CannotStakeZeroWhenNoStake | ||
| Delayed, | ||
| StakingTransferFailed, // DEPRECATED | ||
| UnstakingTransferFailed, // DEPRECATED | ||
| CannotStakeInMoreCourts, // DEPRECATED | ||
| CannotStakeInThisCourt, // DEPRECATED | ||
| CannotStakeLessThanMinStake, // DEPRECATED | ||
| CannotStakeMoreThanMaxStakePerJuror, // DEPRECATED | ||
| CannotStakeMoreThanMaxTotalStaked, // DEPRECATED | ||
| CannotStakeZeroWhenNoStake // DEPRECATED | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM: StakingResult enum updated for new architecture.
The addition of Delayed and deprecation of legacy values aligns with the new StakeController system. The Delayed result likely supports deferred stake execution during certain phases.
Verify that deprecated StakingResult values are not used in the new code:
🏁 Script executed:
#!/bin/bash
# Description: Check for usage of deprecated StakingResult values in the new codebase
# Search for usage of deprecated StakingResult values
echo "Checking for usage of deprecated StakingResult values..."
rg -A 2 "StakingResult\.(StakingTransferFailed|UnstakingTransferFailed|CannotStakeInMoreCourts|CannotStakeInThisCourt|CannotStakeLessThanMinStake|CannotStakeMoreThanMaxStakePerJuror|CannotStakeMoreThanMaxTotalStaked|CannotStakeZeroWhenNoStake)" --type solLength of output: 408
🏁 Script executed:
#!/bin/bash
# Description: Re-check usage of deprecated StakingResult values across all Solidity files
echo "Searching for deprecated StakingResult usages in .sol files..."
rg -n "StakingResult\.(StakingTransferFailed|UnstakingTransferFailed|CannotStakeInMoreCourts|CannotStakeInThisCourt|CannotStakeLessThanMinStake|CannotStakeMoreThanMaxStakePerJuror|CannotStakeMoreThanMaxTotalStaked|CannotStakeZeroWhenNoStake)" -g "*.sol" || trueLength of output: 2125
Update deprecated StakingResult usages
The verification shows that deprecated enum values are still referenced in the codebase. Please replace or remove these to align with the new StakeController model:
• contracts/src/arbitration/university/SortitionModuleUniversity.sol:153
• contracts/src/arbitration/university/KlerosCoreUniversity.sol:1056, 1060, 1075, 1081, 1091–1095
Ensure you migrate logic to use the new success/delayed result paths (or other appropriate outcomes) and drop reliance on the deprecated cases.
🤖 Prompt for AI Agents
In contracts/src/libraries/Constants.sol around lines 29 to 40, the
StakingResult enum has deprecated values that are still used in the codebase.
Locate all references to these deprecated enum values in
contracts/src/arbitration/university/SortitionModuleUniversity.sol at line 153
and in contracts/src/arbitration/university/KlerosCoreUniversity.sol between
lines 1056 and 1095. Replace or remove these deprecated enum usages by updating
the logic to use the new StakingResult values such as Successful or Delayed,
ensuring the code aligns with the new StakeController architecture and no longer
relies on deprecated cases.
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| pragma solidity 0.8.24; | |||
|
|
|||
| import {KlerosCore, KlerosCoreBase, IDisputeKit, ISortitionModule} from "../KlerosCore.sol"; | |||
| import {KlerosCore, KlerosCoreBase, IDisputeKit, IStakeController} from "../KlerosCore.sol"; | |||
There was a problem hiding this comment.
IStakeController very likely sits in its own interface file, not in KlerosCore.sol.
Importing it from ../KlerosCore.sol will fail unless that file also declares the interface – which would be surprising given the new modular split.
Switch the import to the dedicated interface path to avoid a compile-time error.
-import {KlerosCore, KlerosCoreBase, IDisputeKit, IStakeController} from "../KlerosCore.sol";
+import {KlerosCore, KlerosCoreBase, IDisputeKit} from "../KlerosCore.sol";
+import {IStakeController} from "../interfaces/IStakeController.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, KlerosCoreBase, IDisputeKit, IStakeController} from "../KlerosCore.sol"; | |
| import {KlerosCore, KlerosCoreBase, IDisputeKit} from "../KlerosCore.sol"; | |
| import {IStakeController} from "../interfaces/IStakeController.sol"; |
🤖 Prompt for AI Agents
In contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol at line 5,
the import statement incorrectly imports IStakeController from KlerosCore.sol,
but IStakeController likely resides in its own interface file. Update the import
path to point to the dedicated interface file for IStakeController instead of
../KlerosCore.sol to prevent compile-time errors.
| /// @dev Initializer (constructor equivalent for upgradable contracts). | ||
| /// @param _governor The governor's address. | ||
| /// @param _pnk The address of the PNK token contract. | ||
| /// @param _stakeController The address of the stake controller. | ||
| /// @param _core The address of the KlerosCore contract. | ||
| function initialize( | ||
| address _governor, | ||
| IERC20 _pnk, | ||
| address _stakeController, | ||
| address _core | ||
| ) external reinitializer(1) { | ||
| __VaultBase_initialize(_governor, _pnk, _stakeController, _core); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use initializer for the first init – reinitializer(1) is misleading here.
This is the very first initialisation of the proxy; using reinitializer(1) suggests the contract was already initialised with version 0, which is not the case and can cause confusion when adding future re-initialisers.
- ) external reinitializer(1) {
+ ) external initializer {No functional change, but it aligns with OZ’s upgradeable-contract conventions.
📝 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 Initializer (constructor equivalent for upgradable contracts). | |
| /// @param _governor The governor's address. | |
| /// @param _pnk The address of the PNK token contract. | |
| /// @param _stakeController The address of the stake controller. | |
| /// @param _core The address of the KlerosCore contract. | |
| function initialize( | |
| address _governor, | |
| IERC20 _pnk, | |
| address _stakeController, | |
| address _core | |
| ) external reinitializer(1) { | |
| __VaultBase_initialize(_governor, _pnk, _stakeController, _core); | |
| } | |
| /// @dev Initializer (constructor equivalent for upgradable contracts). | |
| /// @param _governor The governor's address. | |
| /// @param _pnk The address of the PNK token contract. | |
| /// @param _stakeController The address of the stake controller. | |
| /// @param _core The address of the KlerosCore contract. | |
| function initialize( | |
| address _governor, | |
| IERC20 _pnk, | |
| address _stakeController, | |
| address _core | |
| ) external initializer { | |
| __VaultBase_initialize(_governor, _pnk, _stakeController, _core); | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/Vault.sol around lines 22 to 34, replace the
`reinitializer(1)` modifier on the initialize function with the `initializer`
modifier. This change correctly marks this function as the first initialization
step for the upgradeable contract, avoiding confusion about prior initialization
versions and aligning with OpenZeppelin's upgradeable contract conventions.
| function _setStake(address _account, uint96 _courtID, uint256 _newStake) internal returns (bool success) { | ||
| if (_courtID == FORKING_COURT || _courtID >= courts.length) { | ||
| _stakingFailed(_onError, StakingResult.CannotStakeInThisCourt); // Staking directly into the forking court is not allowed. | ||
| return false; | ||
| revert StakingNotPossibleInThisCourt(); | ||
| } | ||
| if (_newStake != 0 && _newStake < courts[_courtID].minStake) { | ||
| _stakingFailed(_onError, StakingResult.CannotStakeLessThanMinStake); // Staking less than the minimum stake is not allowed. | ||
| return false; | ||
| revert StakingLessThanCourtMinStake(); | ||
| } | ||
| (uint256 pnkDeposit, uint256 pnkWithdrawal, StakingResult stakingResult) = sortitionModule.setStake( | ||
| (uint256 pnkDeposit, uint256 pnkWithdrawal, StakingResult stakingResult) = stakeController.validateStake( | ||
| _account, | ||
| _courtID, | ||
| _newStake, | ||
| _alreadyTransferred | ||
| _newStake | ||
| ); | ||
| if (stakingResult != StakingResult.Successful) { | ||
| _stakingFailed(_onError, stakingResult); | ||
| return false; | ||
| if (stakingResult == StakingResult.Delayed) { | ||
| return true; | ||
| } | ||
| success = true; | ||
| if (pnkDeposit > 0) { | ||
| if (!pinakion.safeTransferFrom(_account, address(this), pnkDeposit)) { | ||
| _stakingFailed(_onError, StakingResult.StakingTransferFailed); | ||
| return false; | ||
| try vault.deposit(_account, pnkDeposit) {} catch { | ||
| success = false; | ||
| } | ||
| } | ||
| if (pnkWithdrawal > 0) { | ||
| if (!pinakion.safeTransfer(_account, pnkWithdrawal)) { | ||
| _stakingFailed(_onError, StakingResult.UnstakingTransferFailed); | ||
| return false; | ||
| try vault.withdraw(_account, pnkWithdrawal) {} catch { | ||
| success = false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| /// @dev It may revert depending on the _onError parameter. | ||
| function _stakingFailed(OnError _onError, StakingResult _result) internal pure virtual { | ||
| if (_onError == OnError.Return) return; | ||
| if (_result == StakingResult.StakingTransferFailed) revert StakingTransferFailed(); | ||
| if (_result == StakingResult.UnstakingTransferFailed) revert UnstakingTransferFailed(); | ||
| if (_result == StakingResult.CannotStakeInMoreCourts) revert StakingInTooManyCourts(); | ||
| if (_result == StakingResult.CannotStakeInThisCourt) revert StakingNotPossibleInThisCourt(); | ||
| if (_result == StakingResult.CannotStakeLessThanMinStake) revert StakingLessThanCourtMinStake(); | ||
| if (_result == StakingResult.CannotStakeZeroWhenNoStake) revert StakingZeroWhenNoStake(); | ||
| if (success) { | ||
| stakeController.setStake(_account, _courtID, _newStake, pnkDeposit, pnkWithdrawal); | ||
| } | ||
| return success; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Partial side-effects leave vault & trees out-of-sync when a deposit/withdraw fails
_setStake
- executes
vault.depositandvault.withdrawinsidetry/catch, - flips
successtofalseon either failure, but - still returns after leaving the vault in an intermediate state (e.g., deposit succeeded, withdrawal failed).
- Moreover,
stakeController.setStakeis not wrapped intry/catch; a revert here undoes none of the previous vault operations.
Consider an “all-or-nothing” approach:
- success = true;
- if (pnkDeposit > 0) {
- try vault.deposit(_account, pnkDeposit) {} catch {
- success = false;
- }
- }
- if (pnkWithdrawal > 0) {
- try vault.withdraw(_account, pnkWithdrawal) {} catch {
- success = false;
- }
- }
- if (success) {
- stakeController.setStake(_account, _courtID, _newStake, pnkDeposit, pnkWithdrawal);
- }
+ // optimistic – revert everything on first failure
+ if (pnkDeposit > 0) vault.deposit(_account, pnkDeposit);
+ if (pnkWithdrawal > 0) vault.withdraw(_account, pnkWithdrawal);
+
+ // update trees only after vault succeeded
+ stakeController.setStake(_account, _courtID, _newStake, pnkDeposit, pnkWithdrawal);
+ success = true;This guarantees state consistency between the vault and the sortition trees.
📝 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 _setStake(address _account, uint96 _courtID, uint256 _newStake) internal returns (bool success) { | |
| if (_courtID == FORKING_COURT || _courtID >= courts.length) { | |
| _stakingFailed(_onError, StakingResult.CannotStakeInThisCourt); // Staking directly into the forking court is not allowed. | |
| return false; | |
| revert StakingNotPossibleInThisCourt(); | |
| } | |
| if (_newStake != 0 && _newStake < courts[_courtID].minStake) { | |
| _stakingFailed(_onError, StakingResult.CannotStakeLessThanMinStake); // Staking less than the minimum stake is not allowed. | |
| return false; | |
| revert StakingLessThanCourtMinStake(); | |
| } | |
| (uint256 pnkDeposit, uint256 pnkWithdrawal, StakingResult stakingResult) = sortitionModule.setStake( | |
| (uint256 pnkDeposit, uint256 pnkWithdrawal, StakingResult stakingResult) = stakeController.validateStake( | |
| _account, | |
| _courtID, | |
| _newStake, | |
| _alreadyTransferred | |
| _newStake | |
| ); | |
| if (stakingResult != StakingResult.Successful) { | |
| _stakingFailed(_onError, stakingResult); | |
| return false; | |
| if (stakingResult == StakingResult.Delayed) { | |
| return true; | |
| } | |
| success = true; | |
| if (pnkDeposit > 0) { | |
| if (!pinakion.safeTransferFrom(_account, address(this), pnkDeposit)) { | |
| _stakingFailed(_onError, StakingResult.StakingTransferFailed); | |
| return false; | |
| try vault.deposit(_account, pnkDeposit) {} catch { | |
| success = false; | |
| } | |
| } | |
| if (pnkWithdrawal > 0) { | |
| if (!pinakion.safeTransfer(_account, pnkWithdrawal)) { | |
| _stakingFailed(_onError, StakingResult.UnstakingTransferFailed); | |
| return false; | |
| try vault.withdraw(_account, pnkWithdrawal) {} catch { | |
| success = false; | |
| } | |
| } | |
| return true; | |
| } | |
| /// @dev It may revert depending on the _onError parameter. | |
| function _stakingFailed(OnError _onError, StakingResult _result) internal pure virtual { | |
| if (_onError == OnError.Return) return; | |
| if (_result == StakingResult.StakingTransferFailed) revert StakingTransferFailed(); | |
| if (_result == StakingResult.UnstakingTransferFailed) revert UnstakingTransferFailed(); | |
| if (_result == StakingResult.CannotStakeInMoreCourts) revert StakingInTooManyCourts(); | |
| if (_result == StakingResult.CannotStakeInThisCourt) revert StakingNotPossibleInThisCourt(); | |
| if (_result == StakingResult.CannotStakeLessThanMinStake) revert StakingLessThanCourtMinStake(); | |
| if (_result == StakingResult.CannotStakeZeroWhenNoStake) revert StakingZeroWhenNoStake(); | |
| if (success) { | |
| stakeController.setStake(_account, _courtID, _newStake, pnkDeposit, pnkWithdrawal); | |
| } | |
| return success; | |
| } | |
| function _setStake(address _account, uint96 _courtID, uint256 _newStake) internal returns (bool success) { | |
| if (_courtID == FORKING_COURT || _courtID >= courts.length) { | |
| revert StakingNotPossibleInThisCourt(); | |
| } | |
| if (_newStake != 0 && _newStake < courts[_courtID].minStake) { | |
| revert StakingLessThanCourtMinStake(); | |
| } | |
| (uint256 pnkDeposit, uint256 pnkWithdrawal, StakingResult stakingResult) = stakeController.validateStake( | |
| _account, | |
| _courtID, | |
| _newStake | |
| ); | |
| if (stakingResult == StakingResult.Delayed) { | |
| return true; | |
| } | |
| // optimistic – revert everything on first failure | |
| if (pnkDeposit > 0) vault.deposit(_account, pnkDeposit); | |
| if (pnkWithdrawal > 0) vault.withdraw(_account, pnkWithdrawal); | |
| // update trees only after vault succeeded | |
| stakeController.setStake(_account, _courtID, _newStake, pnkDeposit, pnkWithdrawal); | |
| success = true; | |
| return success; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/KlerosCoreBase.sol around lines 1081 to 1111, the
_setStake function currently performs vault.deposit and vault.withdraw calls
inside try/catch blocks but allows partial success, leading to inconsistent
vault and stakeController states. To fix this, refactor the function to
implement an all-or-nothing approach by first validating all operations can
succeed, then performing deposit and withdrawal without try/catch, ensuring that
if any operation fails, the entire transaction reverts and no partial state
changes occur. Also, wrap stakeController.setStake in the same transaction flow
so that any revert there also rolls back vault changes, maintaining state
consistency between the vault and sortition trees.
| /// @inheritdoc IVault | ||
| function lockTokens(address _account, uint256 _amount) external virtual override onlyStakeController { | ||
| jurorBalances[_account].locked += _amount; | ||
| emit Lock(_account, _amount); | ||
| } | ||
|
|
||
| /// @inheritdoc IVault | ||
| function unlockTokens(address _account, uint256 _amount) external virtual override onlyStakeController { | ||
| jurorBalances[_account].locked -= _amount; | ||
| emit Unlock(_account, _amount); | ||
| } |
There was a problem hiding this comment.
Lack of invariant checks when locking / unlocking
lockTokens and unlockTokens blindly mutate locked. This allows the stake-controller to:
- lock more than the juror’s deposited balance, or
- unlock more than what is currently locked.
Both will revert due to under-/over-flow only at the second operation (when available balance is read) and produce confusing error sources.
Add explicit require-style guards:
require(jurorBalances[_account].deposited >= jurorBalances[_account].locked + _amount, "Lock exceeds deposit");
require(jurorBalances[_account].locked >= _amount, "Unlock exceeds locked");🤖 Prompt for AI Agents
In contracts/src/arbitration/VaultBase.sol around lines 148 to 158, the
lockTokens and unlockTokens functions modify the locked balance without checks,
allowing locking beyond deposited balance or unlocking more than locked. Add
require statements in lockTokens to ensure jurorBalances[_account].deposited is
at least jurorBalances[_account].locked plus _amount, and in unlockTokens to
ensure jurorBalances[_account].locked is at least _amount, preventing invalid
state changes and providing clear error messages.
| function _withdraw(address _to, uint256 _amount) internal virtual returns (uint256 pnkAmount) { | ||
| if (_amount == 0) revert InvalidAmount(); | ||
|
|
||
| JurorBalance storage balance = jurorBalances[_to]; | ||
|
|
||
| // Check available balance (deposited - locked - penalties) for the user | ||
| uint256 available = getAvailableBalance(_to); | ||
| if (_amount > available) revert InsufficientAvailableBalance(); | ||
|
|
||
| balance.deposited -= _amount; | ||
| pnk.safeTransfer(_to, _amount); // Vault sends PNK to user | ||
|
|
||
| emit Withdraw(_to, _amount); | ||
| return _amount; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against re-entrancy in _withdraw
_withdraw decreases the user balance and immediately performs an external safeTransfer.
Add a simple ReentrancyGuard or switch to the pull mechanism (withdraw returns amount, user calls claim) to eliminate the remaining re-entrancy surface.
Even though PNK is assumed to be well-behaved, the vault should stay asset-agnostic.
🤖 Prompt for AI Agents
In contracts/src/arbitration/VaultBase.sol around lines 132 to 146, the
_withdraw function updates the user's balance and then calls an external
safeTransfer, which exposes a re-entrancy risk. To fix this, apply a
ReentrancyGuard modifier to the _withdraw function to prevent re-entrant calls,
or refactor the withdrawal logic into a two-step pull mechanism where _withdraw
only records the amount to withdraw and a separate claim function performs the
transfer. This ensures the contract remains asset-agnostic and secure against
re-entrancy attacks.
| /// @inheritdoc IVault | ||
| function deposit(address _from, uint256 _amount) external virtual override onlyCore { | ||
| _deposit(_from, _amount); | ||
| } | ||
|
|
||
| /// @dev Internal implementation of deposit. | ||
| /// @param _from The user address for the deposit. | ||
| /// @param _amount The amount of PNK to deposit. | ||
| function _deposit(address _from, uint256 _amount) internal virtual { | ||
| if (_amount == 0) revert InvalidAmount(); | ||
|
|
||
| // Transfer PNK from the user to the vault | ||
| // The Vault must be approved by _from to transfer PNK to the vault | ||
| pnk.safeTransferFrom(_from, address(this), _amount); | ||
| jurorBalances[_from].deposited += _amount; | ||
|
|
||
| emit Deposit(_from, _amount); | ||
| } |
There was a problem hiding this comment.
Move state-mutations before external token transfer to follow CEI pattern
_deposit updates deposited after calling safeTransferFrom.
A malicious/non-standard ERC-20 could re-enter via the callback hooks available in some tokens, leaving state inconsistent.
Swap the order to “checks → effects → interactions” for consistency with the rest of the contract (e.g. _withdraw already follows that order).
- pnk.safeTransferFrom(_from, address(this), _amount);
- jurorBalances[_from].deposited += _amount;
+ jurorBalances[_from].deposited += _amount;
+ pnk.safeTransferFrom(_from, address(this), _amount);📝 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.
| /// @inheritdoc IVault | |
| function deposit(address _from, uint256 _amount) external virtual override onlyCore { | |
| _deposit(_from, _amount); | |
| } | |
| /// @dev Internal implementation of deposit. | |
| /// @param _from The user address for the deposit. | |
| /// @param _amount The amount of PNK to deposit. | |
| function _deposit(address _from, uint256 _amount) internal virtual { | |
| if (_amount == 0) revert InvalidAmount(); | |
| // Transfer PNK from the user to the vault | |
| // The Vault must be approved by _from to transfer PNK to the vault | |
| pnk.safeTransferFrom(_from, address(this), _amount); | |
| jurorBalances[_from].deposited += _amount; | |
| emit Deposit(_from, _amount); | |
| } | |
| /// @dev Internal implementation of deposit. | |
| /// @param _from The user address for the deposit. | |
| /// @param _amount The amount of PNK to deposit. | |
| function _deposit(address _from, uint256 _amount) internal virtual { | |
| if (_amount == 0) revert InvalidAmount(); | |
| // Transfer PNK from the user to the vault | |
| // The Vault must be approved by _from to transfer PNK to the vault | |
| jurorBalances[_from].deposited += _amount; | |
| pnk.safeTransferFrom(_from, address(this), _amount); | |
| emit Deposit(_from, _amount); | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/VaultBase.sol around lines 104 to 121, the _deposit
function updates the jurorBalances state after calling the external token
transfer function pnk.safeTransferFrom, which risks reentrancy issues. To fix
this, reorder the code to update the jurorBalances mapping before calling
safeTransferFrom, following the checks-effects-interactions pattern to prevent
state inconsistencies from malicious token contracts.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/src/arbitration/StakeControllerBase.sol (1)
269-274: Past security issue has been resolved ✓The
setStakeDelayedfunction now correctly includes theonlyByCoremodifier, addressing the previous DOS attack vulnerability where anyone could push arbitrary entries intodelayedStakes.
🧹 Nitpick comments (2)
contracts/src/arbitration/StakeControllerBase.sol (2)
279-279: Fix misleading commentThe comment mentions that the event name might be misleading but then incorrectly states "should be StakeLocked" when the event is already named
StakeLocked.- emit StakeLocked(_account, _amount); // Event name might be misleading, should be StakeLocked. Preserved for compatibility if so. + emit StakeLocked(_account, _amount);
402-406: Minor optimization: Early return is redundantThe early return check for empty array at line 405 is unnecessary since the for loop at line 409 will naturally handle empty arrays by not executing any iterations.
function _removeCourt(uint96[] storage _stakedCourts, uint96 _courtID) internal { uint256 length = _stakedCourts.length; - if (length == 0) { - return; // Nothing to remove - } uint256 courtIndexToRemove = type(uint256).max; // Sentinel value indicates not found
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/arbitration/StakeControllerBase.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
contracts/src/arbitration/StakeControllerBase.sol (3)
81-101: Well-structured initialization patternThe initialization function properly uses the
onlyInitializingmodifier and correctly sets all storage variables, including settingdelayedStakeReadIndexto 1 to skip index 0 as documented.
177-199: Robust error handling in delayed stake executionThe function properly validates phase and index bounds, uses try-catch to handle failures gracefully, and emits events for failed operations. The deletion of processed stakes prevents re-execution.
255-260: Efficient court management logicThe code correctly manages the
stakedCourtIDsarray by adding courts when staking from zero and removing them when unstaking to zero, maintaining an accurate list of courts where the juror has active stakes.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
contracts/src/arbitration/KlerosCoreBase.sol (1)
1097-1109: Partial vault success can desynchronise state (repeat of earlier review)
Same issue as previously flagged: a failed deposit/withdraw keeps the vault and trees out-of-sync.
🧹 Nitpick comments (3)
contracts/src/arbitration/StakeControllerBase.sol (3)
202-206: Off-by-one makes_iterations> pending items revertThe
actualIterationsformula is overly tricky and can underflow when
delayedStakeWriteIndex == delayedStakeReadIndex + _iterations - 1.A clearer, safer pattern:
uint256 remaining = delayedStakeWriteIndex - delayedStakeReadIndex + 1; uint256 actual = _iterations > remaining ? remaining : _iterations;
290-301: Locked/unlocked stake events mis-named
emit StakeLocked/StakeUnlockedevents are emitted insidelockStake/unlockStakebut the event names declared areStakeLocked/StakeUnlocked.
The comment “Event name might be misleading” hints at legacy renaming – clean this up to avoid confusion for indexers.
424-447:_removeCourtis O(n) per delete – consider swap-and-pop with index mapFrequent stake changes can make this hot-path.
Storingmapping(uint96 => uint256 index)next to the array allows O(1) removals without scanning.This saves gas for large multi-court jurors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/src/arbitration/KlerosCoreBase.sol(19 hunks)contracts/src/arbitration/SortitionSumTree.sol(1 hunks)contracts/src/arbitration/StakeControllerBase.sol(1 hunks)contracts/src/libraries/Constants.sol(1 hunks)contracts/src/test/SortitionModuleMock.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/src/test/SortitionModuleMock.sol
- contracts/src/libraries/Constants.sol
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- 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
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (1)
contracts/src/arbitration/SortitionSumTree.sol (1)
248-260: 🛠️ Refactor suggestion
_stakePathIDToAccountassembly mis-aligns the recovered addressThe loop copies the 20 address bytes starting at offset
0x0c, thenmload(ptr)returns a left-aligned 32-byte word, so the resulting value contains 12 zero bytes at the end and only the high 20 bytes of the address – i.e.,address(mload(ptr))≠ original address for most inputs.A cheaper and safer alternative:
- assembly { - // copy address bytes - ... - account := mload(ptr) - } + account = address(uint160(uint256(_stakePathID)));This uses native slicing and avoids manual memory writes.
Likely an incorrect or invalid review comment.
| bytes32 stakePathID = _accountAndCourtIDToStakePathID(_account, _courtID); | ||
| bool finished = false; | ||
| uint96 currentCourtID = _courtID; | ||
| KlerosCoreBase core = stakeController.core(); | ||
|
|
||
| while (!finished) { | ||
| _set(bytes32(uint256(currentCourtID)), _newStake, stakePathID); | ||
| if (currentCourtID == GENERAL_COURT) { | ||
| finished = true; | ||
| } else { | ||
| // Fetch parent court ID. Ensure core.courts() is accessible and correct. | ||
| (uint96 parentCourtID, , , , , , ) = core.courts(currentCourtID); | ||
| if (parentCourtID == currentCourtID) { | ||
| // Avoid infinite loop if parent is self (e.g. for general court already handled or misconfiguration) | ||
| finished = true; | ||
| } else { | ||
| currentCourtID = parentCourtID; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Parent-court stake is overwritten, breaking aggregate stake accounting
setStake propagates _newStake up the court hierarchy but passes the same _newStake each time.
When a juror stakes in two sibling courts, the second update will overwrite – not add to – the existing node in the parent tree, causing the juror’s total stake in the parent (and further ancestors) to be under-counted.
- _set(bytes32(uint256(currentCourtID)), _newStake, stakePathID);
+ // Aggregate stake in ancestor courts.
+ uint256 aggregated = _newStake + stakeOf(bytes32(uint256(currentCourtID)), stakePathID);
+ _set(bytes32(uint256(currentCourtID)), aggregated, stakePathID);A related edge-case: if the juror later stakes directly in the parent court, a different stakePathID will be used, duplicating the juror’s leaf and double-counting their weight.
Consider:
- Deriving
stakePathIDfrom the ancestor court inside the loop; and - Storing a per-juror aggregation map to avoid O(n²) reads.
📝 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.
| bytes32 stakePathID = _accountAndCourtIDToStakePathID(_account, _courtID); | |
| bool finished = false; | |
| uint96 currentCourtID = _courtID; | |
| KlerosCoreBase core = stakeController.core(); | |
| while (!finished) { | |
| _set(bytes32(uint256(currentCourtID)), _newStake, stakePathID); | |
| if (currentCourtID == GENERAL_COURT) { | |
| finished = true; | |
| } else { | |
| // Fetch parent court ID. Ensure core.courts() is accessible and correct. | |
| (uint96 parentCourtID, , , , , , ) = core.courts(currentCourtID); | |
| if (parentCourtID == currentCourtID) { | |
| // Avoid infinite loop if parent is self (e.g. for general court already handled or misconfiguration) | |
| finished = true; | |
| } else { | |
| currentCourtID = parentCourtID; | |
| } | |
| } | |
| } | |
| } | |
| bytes32 stakePathID = _accountAndCourtIDToStakePathID(_account, _courtID); | |
| bool finished = false; | |
| uint96 currentCourtID = _courtID; | |
| KlerosCoreBase core = stakeController.core(); | |
| while (!finished) { | |
| // Aggregate stake in ancestor courts. | |
| uint256 aggregated = _newStake + stakeOf(bytes32(uint256(currentCourtID)), stakePathID); | |
| _set(bytes32(uint256(currentCourtID)), aggregated, stakePathID); | |
| if (currentCourtID == GENERAL_COURT) { | |
| finished = true; | |
| } else { | |
| // Fetch parent court ID. Ensure core.courts() is accessible and correct. | |
| (uint96 parentCourtID, , , , , , ) = core.courts(currentCourtID); | |
| if (parentCourtID == currentCourtID) { | |
| // Avoid infinite loop if parent is self (e.g. for general court already handled or misconfiguration) | |
| finished = true; | |
| } else { | |
| currentCourtID = parentCourtID; | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/SortitionSumTree.sol around lines 118 to 138, the
setStake function incorrectly propagates the same _newStake value up the court
hierarchy, overwriting parent court stakes instead of aggregating them, which
causes under-counting when a juror stakes in multiple sibling courts. To fix
this, modify the loop to derive the stakePathID based on the current ancestor
courtID inside the loop, and accumulate the stakes per juror in a mapping to
aggregate their total stake at each ancestor court, preventing overwrites and
double-counting. This approach ensures correct aggregation of stakes up the
hierarchy and avoids redundant reads.
| uint256 actualIterations = (delayedStakeReadIndex + _iterations) - 1 > delayedStakeWriteIndex | ||
| ? (delayedStakeWriteIndex - delayedStakeReadIndex) + 1 | ||
| : _iterations; | ||
| uint256 newDelayedStakeReadIndex = delayedStakeReadIndex + actualIterations; | ||
|
|
||
| for (uint256 i = delayedStakeReadIndex; i < newDelayedStakeReadIndex; i++) { | ||
| DelayedStake storage delayedStake = delayedStakes[i]; | ||
| if (delayedStake.account == address(0)) continue; | ||
|
|
||
| // Let KlerosCore coordinate stake update and vault deposit/withdrawal. | ||
| try core.setStakeByController(delayedStake.account, delayedStake.courtID, delayedStake.stake) { | ||
| // NOP | ||
| } catch (bytes memory data) { | ||
| emit DelayedStakeSetFailed(data); | ||
| } | ||
| delete delayedStakes[i]; | ||
| } | ||
| delayedStakeReadIndex = newDelayedStakeReadIndex; | ||
| } | ||
|
|
There was a problem hiding this comment.
executeDelayedStakes ignores failure signal – corrupted stakes remain
core.setStakeByController returns bool success, but the loop deletes the entry regardless of the value, making it impossible to retry failed stakes.
- try core.setStakeByController(...) {
- // NOP
- } catch (bytes memory data) {
+ bool ok;
+ try core.setStakeByController(...) returns (bool _ok) {
+ ok = _ok;
+ } catch (bytes memory data) {
emit DelayedStakeSetFailed(data);
+ ok = false;
}
- delete delayedStakes[i];
+ if (ok) {
+ delete delayedStakes[i];
+ }This ensures failed items stay queued for manual inspection or re-execution.
📝 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.
| uint256 actualIterations = (delayedStakeReadIndex + _iterations) - 1 > delayedStakeWriteIndex | |
| ? (delayedStakeWriteIndex - delayedStakeReadIndex) + 1 | |
| : _iterations; | |
| uint256 newDelayedStakeReadIndex = delayedStakeReadIndex + actualIterations; | |
| for (uint256 i = delayedStakeReadIndex; i < newDelayedStakeReadIndex; i++) { | |
| DelayedStake storage delayedStake = delayedStakes[i]; | |
| if (delayedStake.account == address(0)) continue; | |
| // Let KlerosCore coordinate stake update and vault deposit/withdrawal. | |
| try core.setStakeByController(delayedStake.account, delayedStake.courtID, delayedStake.stake) { | |
| // NOP | |
| } catch (bytes memory data) { | |
| emit DelayedStakeSetFailed(data); | |
| } | |
| delete delayedStakes[i]; | |
| } | |
| delayedStakeReadIndex = newDelayedStakeReadIndex; | |
| } | |
| uint256 actualIterations = (delayedStakeReadIndex + _iterations) - 1 > delayedStakeWriteIndex | |
| ? (delayedStakeWriteIndex - delayedStakeReadIndex) + 1 | |
| : _iterations; | |
| uint256 newDelayedStakeReadIndex = delayedStakeReadIndex + actualIterations; | |
| for (uint256 i = delayedStakeReadIndex; i < newDelayedStakeReadIndex; i++) { | |
| DelayedStake storage delayedStake = delayedStakes[i]; | |
| if (delayedStake.account == address(0)) continue; | |
| bool ok; | |
| // Let KlerosCore coordinate stake update and vault deposit/withdrawal. | |
| try core.setStakeByController(delayedStake.account, delayedStake.courtID, delayedStake.stake) returns (bool _ok) { | |
| ok = _ok; | |
| } catch (bytes memory data) { | |
| emit DelayedStakeSetFailed(data); | |
| ok = false; | |
| } | |
| if (ok) { | |
| delete delayedStakes[i]; | |
| } | |
| } | |
| delayedStakeReadIndex = newDelayedStakeReadIndex; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/StakeControllerBase.sol around lines 202 to 221,
the executeDelayedStakes function calls core.setStakeByController which returns
a bool indicating success, but the code deletes the delayedStake entry
regardless of success, preventing retries on failure. Modify the loop to check
the returned bool success value; only delete delayedStakes[i] and advance
delayedStakeReadIndex if the call succeeds. If it fails, emit the failure event
but keep the stake entry intact so it can be retried later.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
contracts/deploy/00-home-chain-arbitration.ts (4)
60-66: Variable name mismatches new architecture
sortitionModuleV2is a leftover from the removed SortitionModule and now wraps aSortitionSumTree. Rename for clarity to avoid mental friction:-const sortitionModuleV2 = await deployUpgradable(deployments, "SortitionSumTree", { +const sortitionSumTree = await deployUpgradable(deployments, "SortitionSumTree", {Then update the downstream references.
92-108: Duplicate name collision in the deployment artefacts
You register the proxy under"KlerosCore"twice (klerosCoreV2deploy + latergetContract<KlerosCore>("KlerosCore")).
If the first call is skipped because an artefact with that name already exists,klerosCoreV2will beundefined, breaking every subsequent reference.Safer pattern:
const klerosCoreProxy = await deployUpgradable(/* … */, { deterministicDeployment: false, log: true }); const core = klerosCoreProxy;…and avoid a second
deployments.getcall altogether.
129-136: Silent catch hides failed currency-rate setup
Catching any error and onlyconsole.error-ing swallows revert reasons and will happily continue with an inconsistent state.Either:
await changeCurrencyRate(/* … */); // let it throwor log and
process.exit(1)so CI fails and the deployment script halts.
145-150: Console logs use proxies but omit implementations – useful for verification
Consider logging both proxy and implementation addresses so on-chain verifiers (e.g. Tenderly / Sourcify) can hook automatically:console.log(`📦 Vault(proxy): ${vault.address} impl: ${(await deployments.get("Vault_Implementation")).address}`);This tiny addition saves time when debugging main-net deployments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/deploy/00-home-chain-arbitration.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Mend Security Check
🔇 Additional comments (1)
contracts/deploy/00-home-chain-arbitration.ts (1)
68-86: Passing predicted addresses to the initializer can hard-revert
IfVault,StakeController, orSortitionSumTreecall into each other in their initializers (even a simplerequire(isContract(addr))), the deployment will revert because the predicted contract’s code is still empty at that block.
Please double-check initializers and, if necessary, split the process:
- Deploy contracts with temporary zero/placeholder addresses.
- After all proxies exist, call explicit
setXfunctions (already present on many Kleros contracts) to wire them together.That keeps deployment atomic without assuming silent initializers.
| // Calculate future addresses for circular dependencies | ||
| const nonce = await ethers.provider.getTransactionCount(deployer); | ||
|
|
||
| const vaultAddress = getContractAddress(deployer, nonce + 1); // deployed on the 2nd tx (nonce+1): Vault Impl tx, Vault Proxy tx | ||
| console.log("calculated future Vault address for nonce %d: %s", nonce + 1, vaultAddress); | ||
|
|
||
| const stakeControllerAddress = getContractAddress(deployer, nonce + 5); // deployed on the 6th tx (nonce+5): Vault Impl tx, Vault Proxy tx, SortitionModule Impl tx, SortitionModule Proxy tx,, StakeController Impl tx, StakeController Proxy tx | ||
| console.log("calculated future StakeController address for nonce %d: %s", nonce + 5, stakeControllerAddress); | ||
|
|
||
| const klerosCoreAddress = getContractAddress(deployer, nonce + 7); // deployed on the 8th tx (nonce+7): Vault Impl tx, Vault Proxy tx, SortitionModule Impl tx, SortitionModule Proxy tx, StakeController Impl tx, StakeController Proxy tx, KlerosCore Impl tx, KlerosCore Proxy tx | ||
| console.log("calculated future KlerosCore address for nonce %d: %s", nonce + 7, klerosCoreAddress); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Manual nonce arithmetic is fragile and will break on the first extra transaction
Hard-coding nonce + {1‥7} assumes that no additional tx slips in between the calculated point and the actual deployment (e.g. a library deploy, an implicit OZ admin‐upgrade deploy, or even a gas‐price retry).
Once the assumption is violated every pre-computed address is wrong, bricking the whole deployment.
Consider replacing the manual math with:
predictAddresshelpers available in OZ Upgrades, or- deploying the dependency first (e.g. Vault impl only), fetching its actual proxy address with
deployments.getand passing that concrete value to the next deploy step.
This turns the circular-dependency problem into a deterministic two-phase deploy without relying on fragile nonce bookkeeping.
🤖 Prompt for AI Agents
In contracts/deploy/00-home-chain-arbitration.ts around lines 42 to 53, the
current approach uses manual nonce arithmetic to predict future contract
addresses, which is fragile and can break if any extra transactions occur before
deployment. To fix this, replace the manual nonce calculations with a more
reliable method such as using OpenZeppelin Upgrades' `predictAddress` helpers or
deploy each dependency sequentially, retrieving the actual deployed proxy
address via `deployments.get` before passing it to the next deployment step.
This ensures deterministic address resolution without relying on fragile nonce
assumptions.
| const vault = await deployUpgradable(deployments, "Vault", { | ||
| from: deployer, | ||
| args: [deployer, pnk.target, stakeControllerAddress, klerosCoreAddress], | ||
| log: true, | ||
| }); // nonce (implementation), nonce + 1 (proxy) | ||
|
|
||
| // Deploy SortitionSumTree | ||
| const sortitionModuleV2 = await deployUpgradable(deployments, "SortitionSumTree", { | ||
| from: deployer, | ||
| args: [deployer, stakeControllerAddress], | ||
| log: true, | ||
| }); // nonce + 2 (implementation), nonce + 3 (proxy) |
There was a problem hiding this comment.
pnk.target may point to the implementation, not the proxy
In v6 ethers, .target is the address of the contract instance you created, in this case the implementation returned by OZ’s deployer, not the TransparentUpgradeableProxy users will interact with.
The Vault’s constructor is expected to hold a reference to the tradeable PNK token (proxy). Pass the proxy address instead:
-args: [deployer, pnk.target, stakeControllerAddress, klerosCoreAddress],
+const pnkAddress = await pnk.getAddress(); // proxy address
+args: [deployer, pnkAddress, stakeControllerAddress, klerosCoreAddress],Same remark applies later to rng.target and core.target. Mixing .target, .address, and .getAddress() is error-prone; pick one consistently (ideally .getAddress() which always gives the proxy).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/deploy/00-home-chain-arbitration.ts around lines 54 to 65, the code
uses pnk.target which points to the implementation contract address instead of
the proxy address required by the Vault constructor. To fix this, replace
pnk.target with the proxy address of the PNK token, ideally using
pnk.getAddress() for consistency and correctness. Apply the same change for
rng.target and core.target later in the file, ensuring all contract addresses
passed are proxy addresses obtained via .getAddress() to avoid confusion and
errors.
…Sortition Sum Tree, deploy scripts cleanup
jaybuidl
force-pushed
the
refactor/staking-contracts
branch
from
cf077e4 to
d99def1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
contracts/deploy/00-home-chain-arbitration.ts (2)
42-52: 🛠️ Refactor suggestionManual nonce arithmetic issue already flagged – still unresolved
Pre-computing addresses with
nonce + {1‥7}is as brittle as before. Any extra tx (e.g., library, retry, admin-upgrade) invalidates every predicted address and bricks the deployment. Please switch to deterministic helpers (predictAddress) or a two-phase deployment approach as proposed earlier.
54-58:⚠️ Potential issueUsing
.targetgives implementation addresses, not proxies
pnk.target,rng.target, and similar properties still refer to the implementation contract returned by OZ upgrades, not the proxy the rest of the system expects. Useawait pnk.getAddress()/await rng.getAddress()consistently.Also applies to: 79-82
contracts/src/arbitration/SortitionSumTree.sol (1)
121-137:⚠️ Potential issueParent-court stake aggregation bug persists
_set(bytes32(uint256(currentCourtID)), _newStake, …)overwrites the ancestor node instead of aggregating sibling stakes. Staking twice in two sibling courts will under-count in parents and break weighted draws.contracts/src/arbitration/KlerosCoreBase.sol (1)
1086-1109:⚠️ Potential issueTwo long-standing staking issues still present
stakingResult == Delayedpath silently returns without queuing the stake (setStakeDelayed), so the request is lost.- Deposit / withdraw are performed in isolation; a failure in either leaves Vault & trees out-of-sync. Adopt the all-or-nothing flow previously suggested.
contracts/src/arbitration/StakeController.sol (1)
96-116:⚠️ Potential issueInitializer still lacks basic sanity checks
Zero addresses or nonsense timing values can brick the controller. Add simple
requireguards (_governor != address(0),_maxDrawingTime > _minStakingTime, etc.) to fail fast.
🧹 Nitpick comments (1)
contracts/src/arbitration/SortitionSumTree.sol (1)
248-260: Safe-packing nit – zero the high 12 bytes
_stakePathIDToAccountleaves the high 12 bytes of the 32-byte word uninitialised beforemload, relying on previous memory being zero. Zero them explicitly to avoid edge-case garbage.- let ptr := mload(0x40) + let ptr := mload(0x40) + mstore(ptr, 0) // clear 32 bytes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/deploy/00-home-chain-arbitration.ts(3 hunks)contracts/src/arbitration/KlerosCoreBase.sol(19 hunks)contracts/src/arbitration/SortitionSumTree.sol(1 hunks)contracts/src/arbitration/StakeController.sol(1 hunks)contracts/src/test/SortitionModuleMock.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/src/test/SortitionModuleMock.sol
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: Mend Security Check
| function setJurorInactive(address _account) external override onlyByCore returns (uint256 pnkToWithdraw) { | ||
| JurorStake storage currentJurorStake = jurorStakes[_account]; | ||
| uint96[] storage stakedCourtIDs = currentJurorStake.stakedCourtIDs; | ||
| while (stakedCourtIDs.length > 0) { | ||
| uint96 courtID = stakedCourtIDs[0]; | ||
| uint256 currentStakeInCourt = currentJurorStake.stakes[courtID]; | ||
| if (phase == Phase.staking) { | ||
| setStake(_account, courtID, 0, 0, currentStakeInCourt); | ||
| } else { | ||
| setStakeDelayed(_account, courtID, 0); | ||
| } | ||
| } | ||
| if (phase == Phase.staking) { | ||
| pnkToWithdraw = vault.getAvailableBalance(_account); | ||
| emit JurorSetInactive(_account, false); | ||
| } else { | ||
| pnkToWithdraw = 0; | ||
| emit JurorSetInactive(_account, true); | ||
| } |
There was a problem hiding this comment.
Infinite loop when marking juror inactive outside staking phase
setJurorInactive iterates while (stakedCourtIDs.length > 0) but, in non-staking phases, setStakeDelayed does not shrink stakedCourtIDs. The length never decreases → infinite loop & out-of-gas.
Fix idea:
- } else {
- setStakeDelayed(_account, courtID, 0);
+ } else {
+ setStakeDelayed(_account, courtID, 0);
+ _removeCourt(stakedCourtIDs, courtID); // ensure progressCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/src/arbitration/StakeController.sol around lines 320 to 338, the
while loop in setJurorInactive causes an infinite loop outside the staking phase
because setStakeDelayed does not remove court IDs from stakedCourtIDs, so its
length never decreases. To fix this, modify the loop to remove the processed
courtID from stakedCourtIDs after calling setStakeDelayed, ensuring the array
shrinks and the loop terminates properly.
|
Code Climate has analyzed commit 6c28f88 and detected 0 issues on this pull request. View more on Code Climate. |
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
contracts/src/arbitration/Vault.sol (1)
31-31: Useinitializerfor the first init –reinitializer(1)is misleading here.This is the very first initialisation of the proxy; using
reinitializer(1)suggests the contract was already initialised with version 0, which is not the case and can cause confusion when adding future re-initialisers.- ) external reinitializer(1) { + ) external initializer {contracts/src/arbitration/VaultNeo.sol (1)
23-23:totalDepositedcan get out-of-syncThe counter is only updated in
_deposit/_withdraw. If the base contract slashes or force-withdraws funds (e.g., penalties), the cap invariant breaks and future deposits may be blocked.
Ensure every path that changesjurorBalances[..].depositedalso updatestotalDeposited, or compute the total on-demand.contracts/src/arbitration/SortitionSumTree.sol (1)
118-138: Parent-court stake is overwritten, breaking aggregate stake accounting
setStakepropagates_newStakeup the court hierarchy but passes the same_newStakeeach time.
When a juror stakes in two sibling courts, the second update will overwrite – not add to – the existing node in the parent tree, causing the juror's total stake in the parent (and further ancestors) to be under-counted.- _set(bytes32(uint256(currentCourtID)), _newStake, stakePathID); + // Aggregate stake in ancestor courts. + uint256 aggregated = _newStake + stakeOf(bytes32(uint256(currentCourtID)), stakePathID); + _set(bytes32(uint256(currentCourtID)), aggregated, stakePathID);contracts/src/arbitration/KlerosCoreBase.sol (3)
1091-1094: Delayed stake never enqueued – stake silently discarded
_setStakeexits early whenstakingResult == StakingResult.Delayed, but it does not callstakeController.setStakeDelayed, so the juror's request is lost.- if (stakingResult == StakingResult.Delayed) { - return true; - } + if (stakingResult == StakingResult.Delayed) { + stakeController.setStakeDelayed(_account, _courtID, _newStake); + return true; + }
1095-1109: Partial side-effects leave vault & trees out-of-sync when a deposit/withdraw fails
_setStakeexecutesvault.depositandvault.withdrawinsidetry/catch, flipssuccesstofalseon either failure, but still returns after leaving the vault in an intermediate state. Moreover,stakeController.setStakeis not wrapped intry/catch; a revert here undoes none of the previous vault operations.Consider an "all-or-nothing" approach:
- success = true; - if (pnkDeposit > 0) { - try vault.deposit(_account, pnkDeposit) {} catch { - success = false; - } - } - if (pnkWithdrawal > 0) { - try vault.withdraw(_account, pnkWithdrawal) {} catch { - success = false; - } - } - if (success) { - stakeController.setStake(_account, _courtID, _newStake, pnkDeposit, pnkWithdrawal); - } + // optimistic – revert everything on first failure + if (pnkDeposit > 0) vault.deposit(_account, pnkDeposit); + if (pnkWithdrawal > 0) vault.withdraw(_account, pnkWithdrawal); + + // update trees only after vault succeeded + stakeController.setStake(_account, _courtID, _newStake, pnkDeposit, pnkWithdrawal); + success = true;
778-803: Penalty flow unlocks then withdraws – may withdraw more than available
setJurorPenaltyunlocks_penaltyPNK, reducing the juror's locked balance. Immediately after, inactive-juror handling may attemptvault.withdraw(pnkToWithdraw), which can include the just-unlocked amount. IfapplyPenaltyalready burned the balance,withdrawcan revert or drain zero-balance.Recommend:
- Compute
available = vault.getAvailableBalance(account)after the penalty;- Cap
pnkToWithdraw = min(pnkToWithdraw, available);- Abort withdrawal when
available == 0.contracts/src/arbitration/StakeController.sol (2)
96-116: Missing parameter validation in initialize functionThe initialize function lacks validation for critical parameters, allowing zero addresses and invalid timing values. This matches a previously identified concern.
Add require statements to ensure all address parameters are non-zero and that timing parameters are valid:
function initialize( address _governor, KlerosCoreBase _core, IVault _vault, ISortitionSumTree _sortition, uint256 _minStakingTime, uint256 _maxDrawingTime, RNG _rng, uint256 _rngLookahead ) external initializer { + require(_governor != address(0) && address(_core) != address(0) && address(_vault) != address(0), "Zero addr"); + require(address(_sortition) != address(0) && address(_rng) != address(0), "Zero addr"); + require(_rngLookahead > 0 && _maxDrawingTime > _minStakingTime, "Bad timing params"); governor = _governor; core = _core; vault = _vault;
320-339: Potential infinite loop in setJurorInactive functionThe while loop in
setJurorInactivecan cause an infinite loop outside the staking phase becausesetStakeDelayeddoes not remove court IDs fromstakedCourtIDs, preventing the loop from terminating.The issue occurs because when
phase != Phase.staking,setStakeDelayedis called but doesn't modify thestakedCourtIDsarray, whilesetStake(called during staking phase) properly removes courts via_removeCourt. This creates an infinite loop during non-staking phases.
🧹 Nitpick comments (5)
contracts/src/arbitration/SortitionSumTree.sol (1)
59-61: Remove duplicate constructor section comment.There are two identical "Constructor" section comments (lines 59-61 and 60-61).
- // ************************************* // - // * Constructor * // - // ************************************* // - // ************************************* // // * Constructor * // // ************************************* //contracts/src/arbitration/StakeController.sol (4)
202-204: Optimize arithmetic calculation in executeDelayedStakesThe arithmetic for calculating
actualIterationscan be simplified and made more readable.- uint256 actualIterations = (delayedStakeReadIndex + _iterations) - 1 > delayedStakeWriteIndex - ? (delayedStakeWriteIndex - delayedStakeReadIndex) + 1 - : _iterations; + uint256 maxPossibleIterations = delayedStakeWriteIndex - delayedStakeReadIndex + 1; + uint256 actualIterations = _iterations > maxPossibleIterations ? maxPossibleIterations : _iterations;
212-217: Consider logging more details for failed delayed stakesThe current error handling only emits the raw error data, which may not provide sufficient context for debugging failed stake operations.
try core.setStakeByController(delayedStake.account, delayedStake.courtID, delayedStake.stake) { // NOP } catch (bytes memory data) { - emit DelayedStakeSetFailed(data); + emit DelayedStakeSetFailed(delayedStake.account, delayedStake.courtID, delayedStake.stake, data); }Note: This would require updating the event definition in the interface as well.
300-300: Remove outdated comment about event nameThe comment suggests the event name might be misleading and should be
StakeLocked, but the actual event being emitted is alreadyStakeLocked, making this comment incorrect.vault.lockTokens(_account, _amount); - emit StakeLocked(_account, _amount); // Event name might be misleading, should be StakeLocked. Preserved for compatibility if so. + emit StakeLocked(_account, _amount);
164-167: Consider phase validation in changeRandomNumberGeneratorThe function only handles the
generatingphase but doesn't validate or handle other phases that might also need special consideration when changing RNG parameters.Consider adding validation or handling for other phases:
function changeRandomNumberGenerator(RNG _rng, uint256 _rngLookahead) external onlyByGovernor { rng = _rng; rngLookahead = _rngLookahead; if (phase == Phase.generating) { rng.requestRandomness(block.number + rngLookahead); randomNumberRequestBlock = block.number; + } else if (phase == Phase.drawing && randomNumber == 0) { + // Reset if we're in drawing phase but don't have a random number yet + phase = Phase.staking; + lastPhaseChange = block.timestamp; + emit NewPhase(phase); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
contracts/src/arbitration/KlerosCore.sol(1 hunks)contracts/src/arbitration/KlerosCoreBase.sol(19 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(2 hunks)contracts/src/arbitration/SortitionSumTree.sol(1 hunks)contracts/src/arbitration/StakeController.sol(1 hunks)contracts/src/arbitration/Vault.sol(1 hunks)contracts/src/arbitration/VaultBase.sol(1 hunks)contracts/src/arbitration/VaultNeo.sol(1 hunks)contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/src/arbitration/KlerosCore.sol
- contracts/src/arbitration/KlerosCoreNeo.sol
- contracts/src/arbitration/VaultBase.sol
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (19)
contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol (3)
5-5: LGTM - Interface import updated correctly.The import has been properly updated from
ISortitionModuletoIStakeControllerto align with the new architecture.
8-8: LGTM - Function signature updated correctly.The interface method has been updated from
sortitionModule()tostakeController()with the appropriate return type change.
70-70:✅ Verification successful
Verify tuple destructuring matches new interface.
The tuple destructuring has been updated to extract
totalStakedfrom a different position. Ensure this matches the return structure ofIStakeController.getJurorBalance().
🏁 Script executed:
#!/bin/bash # Description: Verify the return structure of getJurorBalance in IStakeController interface # Expected: Should show the tuple structure and position of totalStaked ast-grep --pattern $'function getJurorBalance($_, $_) external view returns ($_);'Length of output: 83
🏁 Script executed:
#!/bin/bash # Locate the IStakeController interface file echo "Searching for IStakeController interface..." rg -l "^interface IStakeController" -n # Extract the getJurorBalance function signature across the repo echo "Extracting getJurorBalance signature..." rg "function getJurorBalance" -A2 -nLength of output: 85057
All clear: tuple destructuring is correct
The proxy’sbalanceOfnow callscore.sortitionModule().getJurorBalance(_account, 0), which returns(uint256 totalStaked, uint256 totalLocked, uint256 stakedInCourt, uint256 nbCourts)The current destructuring
(totalStaked, , , )correctly extracts the
totalStakedvalue. No changes needed.contracts/src/arbitration/VaultNeo.sol (4)
58-58: LGTM - Appropriate reinitializer version.Using
reinitializer(2)is correct here as this contract extends the base vault functionality with additional features.
104-106: LGTM - NFT gating logic is correct.The NFT eligibility check correctly validates that the depositor owns at least one NFT when the depositNft is configured.
109-114: LGTM - Per-user deposit limit enforced correctly.The logic correctly checks current user deposits plus new amount against the maximum limit.
117-119: LGTM - Total deposit cap enforced correctly.The total deposit cap check is implemented properly before allowing the deposit.
contracts/src/arbitration/SortitionSumTree.sol (3)
71-71: LGTM - Correct use of initializer for first initialization.Unlike the Vault contracts, this correctly uses
initializerfor the first initialization.
103-110: LGTM - Tree creation logic is sound.The validation checks for existing trees and minimum K value are appropriate, and the initial root node setup is correct.
154-156: LGTM - Proper handling of empty trees.The function correctly returns address(0) when no jurors are staked in the tree.
contracts/src/arbitration/KlerosCoreBase.sol (7)
7-8: LGTM - Interface imports updated correctly.The imports have been properly updated to use the new
IStakeControllerandIVaultinterfaces.
96-97: LGTM - Storage variables updated for new architecture.The storage has been correctly updated to use
stakeControllerandvaultinstead of the deprecated sortition module.
180-183: LGTM - Access control modifier updated correctly.The modifier has been appropriately updated from
onlySortitionModuletoonlyStakeController.
475-481: LGTM - Function renamed and signature updated appropriately.The function has been correctly renamed from
setStakeBySortitionModuletosetStakeByControllerwith appropriate access control.
611-611: LGTM - Stake locking updated to use stakeController.The call has been correctly updated to use
stakeController.lockStakeinstead of the old sortition module.
851-851: LGTM - Stake unlocking updated to use stakeController.The call has been correctly updated to use
stakeController.unlockStake.
858-858: LGTM - Reward transfer updated to use vault.The reward transfer has been correctly updated to use
vault.transferReward.contracts/src/arbitration/StakeController.sol (2)
423-447: Well-implemented array element removalThe
_removeCourtfunction correctly implements the swap-and-pop pattern for efficient array element removal while maintaining O(1) complexity.
246-247:❌ Incorrect review comment
Verify MAX_STAKE_PATHS constant is properly imported
The code references
MAX_STAKE_PATHSconstant but it's not defined in this contract. Ensure this constant is properly imported from the Constants library.
🏁 Script executed:
#!/bin/bash # Description: Verify MAX_STAKE_PATHS constant is defined in Constants library # Expected: Find the constant definition in the imported Constants file rg -A 2 "MAX_STAKE_PATHS" contracts/src/libraries/Constants.solLength of output: 229
🏁 Script executed:
#!/bin/bash # Check if Constants library is imported in StakeController.sol rg -n "import.*Constants" contracts/src/arbitration/StakeController.sol # Display the beginning of the file to inspect imports head -n 40 contracts/src/arbitration/StakeController.solLength of output: 1632
No missing import for MAX_STAKE_PATHS
The
MAX_STAKE_PATHSconstant is defined incontracts/src/libraries/Constants.sol(uint256 constant MAX_STAKE_PATHS = 4;) and is properly imported inStakeController.solon line 13. No changes are necessary.Likely an incorrect or invalid review comment.
PR-Codex overview
This PR focuses on refactoring the Kleros arbitration system by replacing the
SortitionModulewith a newSortitionSumTreearchitecture, enhancing theVaultandStakeController, and updating interfaces and contracts for improved functionality and clarity.Detailed summary
SortitionModule.sol,SortitionModuleNeo.sol, andSortitionModuleBase.sol.SortitionSumTree.solfor sortition operations.SortitionModuleMockto inherit fromSortitionSumTree.KlerosCoreandKlerosCoreNeoto useStakeController.VaultandVaultNeofor handling deposits with NFT-gated features.IStakeController,IVault).changeCurrencyRateto includeKlerosCorein its parameters.Summary by CodeRabbit
New Features
Refactor
Removals
Bug Fixes
Chores