Certora Audit Remediations #2209
Conversation
❌ Deploy Preview for kleros-v2-testnet failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughRefactors stake bookkeeping to update totalStaked during deposit/withdraw application, adds explicit safeTransfer failure handling and event emission, switches dispute resolution checks to Period.execution, makes ArbitrableExample.rule() virtual, adds a MaliciousArbitrableMock, and expands related tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
jaybuidl
added
Type: Bug 🐛
Type: Enhancement ✨
Package: Contracts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/test/foundry/KlerosCore_Execution.t.sol (1)
870-900: Clarify test objective for inflated totalStaked scenario.The test
test_inflatedTotalStaked_whenDelayedStakeExecute_whenJurorHasNoFundsappears to validate behavior when a juror stakes more than their PNK balance. However, the test objective and expected outcome are not entirely clear. Please add a comment explaining:
- The specific edge case being tested
- Whether this is a regression test for a previously identified issue
- The expected post-condition (is the inflated totalStaked the correct behavior, or should it be prevented?)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/src/arbitration/KlerosCore.sol(1 hunks)contracts/src/arbitration/SortitionModule.sol(2 hunks)contracts/src/arbitration/arbitrables/ArbitrableExample.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(1 hunks)contracts/src/libraries/SafeERC20.sol(2 hunks)contracts/src/test/MaliciousArbitrableMock.sol(1 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(10 hunks)contracts/test/foundry/KlerosCore_Staking.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2107
File: contracts/src/arbitration/university/KlerosCoreUniversity.sol:1083-1092
Timestamp: 2025-09-03T19:34:58.056Z
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.
📚 Learning: 2025-09-03T19:34:58.056Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2107
File: contracts/src/arbitration/university/KlerosCoreUniversity.sol:1083-1092
Timestamp: 2025-09-03T19:34:58.056Z
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/SortitionModule.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 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/SortitionModule.sol
📚 Learning: 2025-09-04T23:36:16.415Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2126
File: contracts/src/arbitration/KlerosCore.sol:472-489
Timestamp: 2025-09-04T23:36:16.415Z
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/test/foundry/KlerosCore_Execution.t.solcontracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
📚 Learning: 2024-11-19T05:31:48.701Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 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.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.sol
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.solcontracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
📚 Learning: 2024-10-07T06:18:23.427Z
Learnt from: kemuru
Repo: kleros/kleros-v2 PR: 1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-07T06:18:23.427Z
Learning: In the `kleros-v2` codebase, the property `totalResolvedDisputes` should remain and should not be renamed to `totalResolvedVotes`.
Applied to files:
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.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). (5)
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (11)
contracts/src/arbitration/SortitionModule.sol (2)
385-406: LGTM: totalStaked tracking now explicit in deposit/withdrawal flows.The refactoring correctly moves totalStaked updates from the validation phase to the application phase within
_setStake. This ensures totalStaked reflects actual token movements rather than validated intentions.
463-463: The order of totalStaked decrement in withdrawLeftoverPNK is before the transfer, not after.The
totalStaked -= amount;statement at line 463 executes beforecore.transferBySortitionModule(_account, amount);at line 464. The test in KlerosCore_Execution.t.sol (lines 488 and 507) properly validates that totalStaked is correctly updated before and after the withdrawal operation.Likely an incorrect or invalid review comment.
contracts/src/arbitration/KlerosCore.sol (1)
627-631: LGTM: Explicit transfer failure handling added.The explicit return value check for
pinakion.safeTransferstrengthens failure handling by immediately reverting withTransferFailed()when the transfer fails. This complements the SafeERC20 update that now returns a boolean and emitsSafeTransferFailed.contracts/src/arbitration/arbitrables/ArbitrableExample.sol (1)
152-152: LGTM: Marking rule() as virtual enables proper test mock extension.The addition of the
virtualmodifier allows derived contracts (like the newly introducedMaliciousArbitrableMock) to override therule()function for testing purposes without changing the base implementation.contracts/src/libraries/SafeERC20.sol (1)
18-42: LGTM: SafeTransferFailed event and return value improve transfer failure handling.The addition of the
SafeTransferFailedevent and the boolean return value fromsafeTransferenables explicit failure handling and observability. This allows callers likeKlerosCore.transferBySortitionModuleto check the result and revert with context-specific errors.contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
478-479: LGTM: Resolution check now uses Period.execution instead of isRuled flag.The change validates dispute resolution by checking the period state (
Period.execution) rather than theisRuledflag. This ensures withdrawals are aligned with the execution phase and provides a more direct validation of the dispute state.contracts/src/test/MaliciousArbitrableMock.sol (1)
1-52: LGTM: Test mock enables arbitrable revert scenario testing.The
MaliciousArbitrableMockcontract properly extendsArbitrableExampleand provides a controllable revert mechanism via thedoRevertflag. This enables testing of edge cases where the arbitrable'srule()function reverts, which is valuable for validating that withdrawal paths remain functional even when ruling execution fails.contracts/test/foundry/KlerosCore_Execution.t.sol (4)
5-12: LGTM: Test imports updated for new failure scenarios.The imports for
SafeERC20,console, andMaliciousArbitrableMocksupport the new test cases for transfer failures and arbitrable revert scenarios.
566-617: LGTM: Comprehensive test for failed fee token transfer.The
test_execute_feeToken_failedTransfertest validates the SafeTransferFailed event emission and confirms that execution continues correctly even when individual reward transfers fail. The test properly simulates a transfer failure by depleting the contract's fee token balance.
721-761: LGTM: Test validates withdrawal path when arbitrable reverts.The
test_executeRuling_arbitrableReverttest confirms that withdrawal functionality remains operational even when the arbitrable'srule()function reverts. This is critical for ensuring users can always claim their funds regardless of malicious or buggy arbitrable implementations.
1003-1055: LGTM: Helper functions improve test maintainability.The internal helper functions (
_assertJurorBalance,_stakeBalanceForJuror,_stakePnk_createDispute_moveToDrawingPhase,_drawJurors_advancePeriodToVoting,_vote_execute) reduce code duplication and improve test readability by encapsulating common test setup patterns.
| function test_setStake_totalStaked() public { | ||
| // Increase | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 4000); | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 5001); | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 1000); | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 1500); | ||
|
|
||
| assertEq(sortitionModule.totalStaked(), 6501, "Wrong totalStaked"); | ||
|
|
||
| // Decrease | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 3000); | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 2500); | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 1400); | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 1200); | ||
|
|
||
| assertEq(sortitionModule.totalStaked(), 3700, "Wrong totalStaked"); | ||
| } |
There was a problem hiding this comment.
Remove duplicate test function.
The test_setStake_totalStaked() function appears to be defined only once in the provided code (lines 144-168). However, the AI summary indicates there are two identical occurrences. Please verify and remove any duplicate definition if present, as duplicate test functions can cause compilation issues or test execution confusion.
#!/bin/bash
# Description: Check for duplicate test_setStake_totalStaked function definitions
# Search for all occurrences of the function definition
rg -nP --type=sol 'function\s+test_setStake_totalStaked\s*\(\s*\)\s+public'🤖 Prompt for AI Agents
contracts/test/foundry/KlerosCore_Staking.t.sol lines ~144-168: the
test_setStake_totalStaked function is duplicated elsewhere causing potential
compilation/test confusion; locate the other identical definition (search for
"function test_setStake_totalStaked() public") and remove the duplicate
definition, or if both are needed, rename one to a unique test name and ensure
its assertions remain correct so only a single function name exists per test;
run the test suite to verify compilation and that the intended assertions still
execute.
jaybuidl
force-pushed
the
fix/certora-audit
branch
from
a016740 to
ad3006d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contracts/src/test/MaliciousArbitrableMock.sol (2)
12-14: Consider adding access control for test predictability.The
changeBehaviour()function has no access control, which is acceptable for a test mock but could lead to unexpected test behavior if called from unintended addresses. For test predictability, consider restricting toonlyByOwnerinherited from the parent pattern.
36-49: Consider usingsuper.rule()to avoid code duplication.The override duplicates the parent's validation and state update logic. Using
super.rule()would be more maintainable and ensure the mock stays in sync with any future changes toArbitrableExample.rule().🔎 Proposed refactor
function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external override { if (doRevert) revert RuleReverted(); - - uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID]; - DisputeStruct storage dispute = disputes[localDisputeID]; - if (msg.sender != address(arbitrator)) revert ArbitratorOnly(); - if (_ruling > dispute.numberOfRulingOptions) revert RulingOutOfBounds(); - if (dispute.isRuled) revert DisputeAlreadyRuled(); - - dispute.isRuled = true; - dispute.ruling = _ruling; - - emit Ruling(IArbitratorV2(msg.sender), _arbitratorDisputeID, dispute.ruling); + ArbitrableExample.rule(_arbitratorDisputeID, _ruling); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/src/arbitration/KlerosCore.sol(1 hunks)contracts/src/arbitration/SortitionModule.sol(2 hunks)contracts/src/arbitration/arbitrables/ArbitrableExample.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(1 hunks)contracts/src/libraries/SafeERC20.sol(2 hunks)contracts/src/test/MaliciousArbitrableMock.sol(1 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(10 hunks)contracts/test/foundry/KlerosCore_Staking.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/src/arbitration/KlerosCore.sol
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2024-10-21T10:32:16.970Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1703
File: kleros-sdk/src/utils/getDispute.ts:38-40
Timestamp: 2024-10-21T10:32:16.970Z
Learning: The variables 'arbitrableChainID' and 'externalDisputeID' are required by the context to have uppercase 'ID', so they should remain unchanged even if the corresponding source properties use 'Id'.
Applied to files:
contracts/src/arbitration/arbitrables/ArbitrableExample.sol
📚 Learning: 2025-09-03T19:34:58.056Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2107
File: contracts/src/arbitration/university/KlerosCoreUniversity.sol:1083-1092
Timestamp: 2025-09-03T19:34:58.056Z
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/SortitionModule.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 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/SortitionModule.sol
📚 Learning: 2025-09-04T23:36:16.415Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2126
File: contracts/src/arbitration/KlerosCore.sol:472-489
Timestamp: 2025-09-04T23:36:16.415Z
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/test/foundry/KlerosCore_Execution.t.solcontracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
📚 Learning: 2024-11-19T05:31:48.701Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 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.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.sol
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.solcontracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
📚 Learning: 2024-10-07T06:18:23.427Z
Learnt from: kemuru
Repo: kleros/kleros-v2 PR: 1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-07T06:18:23.427Z
Learning: In the `kleros-v2` codebase, the property `totalResolvedDisputes` should remain and should not be renamed to `totalResolvedVotes`.
Applied to files:
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.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). (14)
- 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-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (16)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
478-479: Ensure dispute withdrawals only happen after execution period is reached.The check using
period != KlerosCore.Period.executionis the correct approach. Theexecutionperiod is entered exclusively when a dispute reaches its final resolution state after voting completes and all appeals expire or resolve, making it semantically equivalent to a resolved dispute. This pattern is used consistently throughout KlerosCore for controlling state-dependent operations.contracts/src/libraries/SafeERC20.sol (2)
18-22: LGTM! Event declaration is well-documented.The
SafeTransferFailedevent provides useful observability for failed transfers, enabling off-chain monitoring and debugging.
40-42: The asymmetry betweensafeTransferandsafeTransferFromis intentional and justified by usage patterns.safeTransferemits aSafeTransferFailedevent on failure because it's used for non-critical operations (reward/penalty distributions) where failure may go unhandled; conversely, all 7safeTransferFromusages explicitly check the return value and revert immediately, since it's used for critical deposit operations that must fail explicitly. The design is consistent with the risk profiles of each function's usage context.contracts/src/arbitration/arbitrables/ArbitrableExample.sol (1)
152-163: LGTM! Addingvirtualmodifier enables proper extensibility.This change correctly allows derived contracts like
MaliciousArbitrableMockto override therule()function for testing purposes, while maintaining all existing access control and validation logic.contracts/test/foundry/KlerosCore_Staking.t.sol (1)
144-168: LGTM! Test correctly validatestotalStakedaccounting.The test properly verifies the new
totalStakedbookkeeping behavior:
- Stake increases: 5001 (staker1) + 1500 (staker2) = 6501 ✓
- Stake decreases: 2500 (staker1) + 1200 (staker2) = 3700 ✓
This provides good coverage for the refactored
totalStakedtracking inSortitionModule._setStake(). The previous review comment about a duplicate function appears to be stale as only one definition is present in the provided code.contracts/src/arbitration/SortitionModule.sol (2)
385-406: LGTM!totalStakedbookkeeping correctly tracks deposit/withdrawal flows.The refactored logic properly maintains
totalStakedinvariants:
- Deposits (line 392):
totalStakedincreases in sync withjuror.stakedPnk- Withdrawals (line 395):
totalStakeddecreases in sync withjuror.stakedPnkThe else branch executes when
_pnkDeposit == 0, which handles both actual withdrawals (_pnkWithdrawal > 0) and no-op cases (_pnkWithdrawal == 0) safely since subtracting zero has no effect.
460-465: LGTM! Leftover withdrawal correctly updatestotalStaked.The sequence is correct:
- Retrieve leftover amount
- Zero out
juror.stakedPnk- Decrease
totalStakedby the withdrawn amount- Transfer tokens
This ensures
totalStakedremains consistent with actual token holdings in the contract.contracts/test/foundry/KlerosCore_Execution.t.sol (9)
101-103: Good addition: Balance consistency checks.These assertions verify that
totalStakedremains consistent with actual token balances throughout the execution flow, improving test reliability.Also applies to: 149-150, 153-153
487-498: Good test coverage for SafeERC20 failure handling.This segment properly tests the new
TransferFailederror path by artificially draining the core balance and verifying the expected revert, then restoring state to continue the test.
546-546: Verify: Vote choice changed from 2 to 1.The vote choice was modified without clear explanation. Since
staker1holds all votes, either choice should result in the same outcome, but confirm this change aligns with the test's intent.
566-617: Excellent test coverage for fee token transfer failures.This test properly validates that when
safeTransferfails due to insufficient balance, theSafeTransferFailedevent is emitted and execution completes without reverting, while correctly preventing token transfer to the juror.
721-761: Excellent security test: Withdrawal works despite malicious arbitrable.This test validates that jurors can withdraw fees and rewards even when the arbitrable contract maliciously reverts during
executeRuling(). This is critical for ensuring jurors aren't financially penalized by misbehaving arbitrables.
842-844: Intentional change: Withdrawals now independent of executeRuling.The commented-out
executeRuling()call and accompanying comment clarify that withdrawals should work even ifexecuteRuling()hasn't been called or has reverted. This aligns with the resilience testing against malicious arbitrables.
1001-1055: Excellent refactor: Internal helpers improve test maintainability.These helper functions consolidate common test patterns, reducing duplication and improving readability. The naming is clear and each function has a well-defined, single responsibility.
870-900: Test appears incomplete: Missing assertions for expected behavior.This test demonstrates a potential
totalStakedinflation scenario when delayed stakes are executed with insufficient juror funds, but lacks clear assertions:
- No verification that
totalStakedis actually inflated (lines 888, 894 just log values)- No
expectRevertfor the arithmetic overflow mentioned in comments (lines 897-898)- Final
_stakeBalanceForJuror(staker2, 20000)call at line 900 has no assertion about expected outcomeConsider adding:
assertEqorassertGtto verify totalStaked inflation afterexecuteDelayedStakesvm.expectRevertif overflow is expected when staker2 tries to stake- Or assertions proving the bug is fixed if this is a regression test
Would you like me to suggest complete assertions based on the expected behavior?
⛔ Skipped due to learnings
Learnt from: jaybuidl Repo: kleros/kleros-v2 PR: 2107 File: contracts/src/arbitration/university/KlerosCoreUniversity.sol:1083-1092 Timestamp: 2025-09-03T19:34:58.056Z 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.
5-5: LGTM! MaliciousArbitrableMock contract exists in the codebase.The new imports properly support the enhanced test coverage for SafeERC20 failure handling and malicious arbitrable behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/arbitration/KlerosCore.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T23:36:16.415Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2126
File: contracts/src/arbitration/KlerosCore.sol:472-489
Timestamp: 2025-09-04T23:36:16.415Z
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/src/arbitration/KlerosCore.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). (14)
- 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-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (1)
contracts/src/arbitration/KlerosCore.sol (1)
627-627: LGTM! Consistent pause protection for token transfers.Adding
whenNotPausedis appropriate and aligns with other token transfer functions likesetStakeandexecute.
| if (msg.sender != address(sortitionModule)) revert SortitionModuleOnly(); | ||
| // Note eligibility is checked in SortitionModule. | ||
| pinakion.safeTransfer(_account, _amount); | ||
| if (!pinakion.safeTransfer(_account, _amount)) revert TransferFailed(); |
There was a problem hiding this comment.
Verify consistent error handling for all safeTransfer calls.
The explicit return value check is appropriate given the SafeERC20 modifications. However, there are other safeTransfer calls in this file that don't check return values:
- Line 965:
pinakion.safeTransfer(owner, _params.pnkPenaltiesInRound); - Line 1023:
pinakion.safeTransfer(account, pnkReward); - Line 1045:
pinakion.safeTransfer(owner, leftoverPnkReward); - Line 1290:
_feeToken.safeTransfer(_recipient, _amount);
If SafeERC20 now returns false instead of reverting on failure, these unchecked calls could fail silently, leading to incorrect state (e.g., rewards not distributed but marked as paid).
Run this script to verify all safeTransfer usage patterns:
#!/bin/bash
# Find all safeTransfer calls in KlerosCore.sol and show context
echo "=== All safeTransfer calls in KlerosCore.sol ==="
rg -n -B2 -A2 'safeTransfer(From)?\(' contracts/src/arbitration/KlerosCore.sol
echo -e "\n=== Checking if SafeERC20 implementation returns boolean ==="
rg -n -A10 'function safeTransfer' contracts/src/libraries/SafeERC20.sol🤖 Prompt for AI Agents
In contracts/src/arbitration/KlerosCore.sol (lines ~630, 965, 1023, 1045, and
1290) some pinakion/_feeToken.safeTransfer calls are not checking the boolean
return introduced by the modified SafeERC20, causing silent failures; update
each unchecked call to validate the return and revert on false (e.g., wrap each
transfer in an if check that reverts with TransferFailed() on a false return) or
alternatively replace those calls with a helper that asserts the transfer
succeeded; apply the same pattern used at line 630 to all listed transfers so
state changes (rewards/fees) cannot be marked as paid when the token transfer
failed.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contracts/test/foundry/KlerosCore_Execution.t.sol (1)
1066-1121: Good addition of internal helpers to reduce code duplication.The helper functions improve test readability and maintainability. Consider removing the
console.logstatement at line 1085 as it adds noise to test output and is typically used only for debugging.🔎 Suggested change
function _stakeBalanceForJuror(address juror, uint256 amount) internal { - console.log("actual juror PNK balance before staking: %e", pinakion.balanceOf(juror)); vm.prank(juror); core.setStake(GENERAL_COURT, amount); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/arbitration/SortitionModule.solcontracts/test/foundry/KlerosCore_Execution.t.sol
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-03T19:34:58.056Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2107
File: contracts/src/arbitration/university/KlerosCoreUniversity.sol:1083-1092
Timestamp: 2025-09-03T19:34:58.056Z
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/SortitionModule.solcontracts/test/foundry/KlerosCore_Execution.t.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 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/SortitionModule.sol
📚 Learning: 2025-09-04T23:36:16.415Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2126
File: contracts/src/arbitration/KlerosCore.sol:472-489
Timestamp: 2025-09-04T23:36:16.415Z
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/test/foundry/KlerosCore_Execution.t.sol
📚 Learning: 2024-11-19T05:31:48.701Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 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.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.sol
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.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). (14)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (11)
contracts/test/foundry/KlerosCore_Execution.t.sol (9)
5-12: LGTM!Import additions are appropriate for the new test functionality:
SafeERC20for verifying theSafeTransferFailedevent,consolefor debug logging in edge-case tests, andMaliciousArbitrableMockfor testing malicious arbitrable behavior.
101-103: LGTM!Good addition of invariant checks verifying that
totalStakedmatches the actual token balance of the core contract before execution.
149-154: LGTM!Consistent post-execution assertions verifying that token balances and
totalStakedremain synchronized after reward distribution.
156-220: LGTM!This test effectively covers an important edge case where PNK rewards should be transferred directly to the juror's wallet when adding them to stake would exceed
maxStakePerJuror. The assertions correctly verify that stake remains unchanged while the wallet balance increases.
553-574: LGTM!Excellent addition of transfer failure testing. The approach of manually draining the core's balance to simulate insufficient funds, verifying the
TransferFailedrevert, then restoring funds for successful withdrawal effectively covers edge cases in the withdrawal flow.
612-612: No changes to review.This line is part of the existing test logic with no material modifications.
632-683: LGTM!This test effectively validates the new
SafeTransferFailedevent behavior introduced inSafeERC20.sol. It confirms that execution continues (repartitions complete) even when fee token transfers fail, with the failure being signaled via event rather than revert. This is important for system resilience.
787-827: LGTM!Critical test validating that jurors can withdraw fees and rewards even when the arbitrable contract maliciously reverts in
rule(). This ensures the arbitration system remains robust against adversarial arbitrable contracts, protecting jurors from being unable to claim their earned rewards.
908-910: LGTM!Good modification reflecting the updated withdrawal eligibility check. The comment clearly explains that
executeRuling()is now irrelevant for withdrawals, aligning with the PR's change to checkPeriod.executioninstead of requiringruled=trueinDisputeKitClassicBase.sol.contracts/src/arbitration/SortitionModule.sol (2)
391-412: ExplicittotalStakedbookkeeping is correct.Moving
totalStakedupdates into_setStake(lines 398, 401) centralizes accounting where stakes are actually applied. This ensures consistency across all code paths—deposits, withdrawals, penalties (line 356), and rewards (line 379)—and maintains the invariant thattotalStakedequals the sum of all jurors'stakedPnk.
463-472: CorrecttotalStakeddecrement for leftover withdrawals.When a juror unstakes but has locked PNK, the withdrawal is capped (lines 317-320) and
totalStakedis not decremented. Later, when leftover PNK is withdrawn via this function, line 469 properly decrementstotalStaked, closing the accounting cycle.
|
|
||
| // Transfer reward directly to the juror so the max stake is not exceeded. | ||
| if (jurors[_account].stakedPnk + _reward > maxStakePerJuror || newStake > maxStakePerJuror) { | ||
| return false; | ||
| } | ||
|
|
||
| _setStake(_account, _courtID, _reward, 0, newStake); | ||
| return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine SortitionModule.sol to verify the review comment concern
find . -name "SortitionModule.sol" -type f 2>/dev/null | head -1Repository: kleros/kleros-v2
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Read the SortitionModule.sol file to verify the review comment
head -n 450 ./contracts/src/arbitration/SortitionModule.sol | tail -n +290Repository: kleros/kleros-v2
Length of output: 6440
Missing maxTotalStaked check in reward distribution
The setStakeReward function validates per-juror limits (maxStakePerJuror) but omits the global maxTotalStaked constraint. In contrast, _validateStake enforces if (totalStaked + stakeChange > maxTotalStaked) for regular stakes. Here, rewards bypass this check, allowing _setStake to increment totalStaked beyond maxTotalStaked (line ~399-400).
Add the check before calling _setStake:
if (totalStaked + _reward > maxTotalStaked) {
return false;
}🤖 Prompt for AI Agents
In contracts/src/arbitration/SortitionModule.sol around lines 373 to 380, the
reward path in setStakeReward checks only per-juror maxStakePerJuror but misses
the global maxTotalStaked constraint, allowing totalStaked to exceed the
contract-wide cap; before calling _setStake add a guard that returns false when
totalStaked + _reward > maxTotalStaked to mirror _validateStake and prevent
rewards from pushing totalStaked past the global limit.
| function test_inflatedTotalStaked_whenDelayedStakeExecute_whenJurorHasNoFunds() public { | ||
| // pre conditions | ||
| // 1. there is a dispute in drawing phase | ||
| // 2. juror call setStake with an amount greater than his PNK balance | ||
| // 3. draw jurors, move to voting phase and execute voting | ||
| // 4. move sortition to staking phase | ||
| uint256 disputeID = 0; | ||
| uint256 amountToStake = 20000; | ||
| _stakePnk_createDispute_moveToDrawingPhase(staker1, amountToStake); | ||
|
|
||
| KlerosCore.Round memory round = core.getRoundInfo(disputeID, 0); | ||
| uint256 pnkAtStakePerJuror = round.pnkAtStakePerJuror; | ||
| _stakeBalanceForJuror(staker1, type(uint256).max); | ||
| _drawJurors_advancePeriodToVoting(disputeID); | ||
| _vote_execute(disputeID, staker1); | ||
| sortitionModule.passPhase(); // set it to staking phase | ||
| _assertJurorBalance(staker1, amountToStake, pnkAtStakePerJuror * DEFAULT_NB_OF_JURORS, amountToStake, 1); | ||
|
|
||
| console.log("totalStaked before: %e", sortitionModule.totalStaked()); | ||
|
|
||
| // execution: execute delayed stake | ||
| sortitionModule.executeDelayedStakes(1); | ||
|
|
||
| // post condition: inflated totalStaked | ||
| console.log("totalStaked after: %e", sortitionModule.totalStaked()); | ||
| _assertJurorBalance(staker1, amountToStake, pnkAtStakePerJuror * DEFAULT_NB_OF_JURORS, amountToStake, 1); | ||
|
|
||
| // new juror tries to stake but totalStaked already reached type(uint256).max | ||
| // it reverts with "arithmetic underflow or overflow (0x11)" | ||
| _stakeBalanceForJuror(staker2, 20000); | ||
| } |
There was a problem hiding this comment.
Test appears incomplete and may not validate expected behavior.
Several concerns with this test:
-
Missing assertion/revert check: Line 965 states staker2's stake should revert with overflow, but there's no
vm.expectRevert()or assertion verifying this behavior. -
Unrealistic stake amount:
_stakeBalanceForJuror(staker1, type(uint256).max)at line 948 attempts to stake the maximum uint256 value. This seems intended to create a delayed stake that can't be fulfilled, but the test intent isn't clear. -
Debug logging:
console.logstatements (lines 954, 960, 1085) are typically removed from finalized tests as they add noise to test output. -
Missing post-condition assertions: The comment at line 959 says "inflated totalStaked" should occur, but there's no assertion verifying this happened.
🔎 Suggested improvements
// post condition: inflated totalStaked
console.log("totalStaked after: %e", sortitionModule.totalStaked());
_assertJurorBalance(staker1, amountToStake, pnkAtStakePerJuror * DEFAULT_NB_OF_JURORS, amountToStake, 1);
+ // Assert the inflated state
+ assertGt(sortitionModule.totalStaked(), pinakion.balanceOf(address(core)), "totalStaked should be inflated");
// new juror tries to stake but totalStaked already reached type(uint256).max
// it reverts with "arithmetic underflow or overflow (0x11)"
+ vm.expectRevert(); // Expect arithmetic overflow
_stakeBalanceForJuror(staker2, 20000);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Execution.t.sol around lines 936-966, the
test is incomplete: add an explicit vm.expectRevert(...) before the
_stakeBalanceForJuror(staker2, 20000) call to assert the overflow revert,
replace or clarify the unrealistic _stakeBalanceForJuror(staker1,
type(uint256).max) usage (either document intent or use a deterministic large
value that reproduces the delayed-stake condition), remove the console.log
calls, and add assertions that verify totalStaked changed as expected (e.g.,
assert totalStaked equals the inflated value or equals type(uint256).max after
executeDelayedStakes and that balances remain unchanged for staker1) so the test
explicitly checks the post-conditions rather than only logging them.
PR-Codex overview
This PR focuses on enhancing the functionality and error handling in the
KlerosCoreand related contracts, introducing new tests, and improving the safety of token transfers.Detailed summary
virtualmodifier torulefunction inArbitrableExample.DisputeKitClassicBase.transferBySortitionModuleto includewhenNotPausedmodifier.SafeTransferFailedevent inSafeERC20.safeTransfer.MaliciousArbitrableMockfor testing revert scenarios.KlerosCore_ExecutionTest.Summary by CodeRabbit
Bug Fixes
Tests
New Contracts (Tests)
✏️ Tip: You can customize this high-level summary in your review settings.