Skip to content

Certora Audit Remediations #2209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jaybuidl wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Certora Audit Remediations #2209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a3759ef
fix: stakes accounting H-01 M-02
unknownunknown1 Nov 29, 2025
b8af085
fix: malicious arbitrable M-01
unknownunknown1 Dec 6, 2025
ad3006d
fix: safeTransfer check M-03
unknownunknown1 Dec 8, 2025
104d89b
fix: whenNotPaused on transferBySortitionModule() L-02
jaybuidl Dec 19, 2025
8189a3a
fix: maxStake bypass L-06
unknownunknown1 Dec 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions contracts/src/arbitration/KlerosCore.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,10 +624,10 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
/// @notice Transfers PNK to the juror by SortitionModule.
/// @param _account The account of the juror whose PNK to transfer.
/// @param _amount The amount to transfer.
function transferBySortitionModule(address _account, uint256 _amount) external {
function transferBySortitionModule(address _account, uint256 _amount) external whenNotPaused {
if (msg.sender != address(sortitionModule)) revert SortitionModuleOnly();
// Note eligibility is checked in SortitionModule.
pinakion.safeTransfer(_account, _amount);
if (!pinakion.safeTransfer(_account, _amount)) revert TransferFailed();
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 19, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

}

/// @inheritdoc IArbitratorV2
Expand Down
11 changes: 9 additions & 2 deletions contracts/src/arbitration/SortitionModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,15 +311,13 @@ contract SortitionModule is ISortitionModule, Initializable, UUPSProxiable {
// Current phase is Staking: set stakes.
if (stakeIncrease) {
pnkDeposit = stakeChange;
totalStaked += stakeChange;
} else {
pnkWithdrawal = stakeChange;
uint256 possibleWithdrawal = juror.stakedPnk > juror.lockedPnk ? juror.stakedPnk - juror.lockedPnk : 0;
if (pnkWithdrawal > possibleWithdrawal) {
// Ensure locked tokens remain in the contract. They can only be released during Execution.
pnkWithdrawal = possibleWithdrawal;
}
totalStaked -= pnkWithdrawal;
}
return (pnkDeposit, pnkWithdrawal, StakingResult.Successful);
}
Expand Down Expand Up @@ -372,6 +370,12 @@ contract SortitionModule is ISortitionModule, Initializable, UUPSProxiable {
if (currentStake == 0) return false; // Juror has been unstaked, don't increase their stake.

uint256 newStake = currentStake + _reward;

// 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;
Comment on lines +373 to 380
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 22, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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 -1

Repository: 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 +290

Repository: 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.

}
Expand All @@ -391,8 +395,10 @@ contract SortitionModule is ISortitionModule, Initializable, UUPSProxiable {
}
// Increase juror's balance by deposited amount.
juror.stakedPnk += _pnkDeposit;
totalStaked += _pnkDeposit;
} else {
juror.stakedPnk -= _pnkWithdrawal;
totalStaked -= _pnkWithdrawal;
if (_newStake == 0) {
// Cleanup
for (uint256 i = juror.courtIDs.length; i > 0; i--) {
Expand Down Expand Up @@ -460,6 +466,7 @@ contract SortitionModule is ISortitionModule, Initializable, UUPSProxiable {
uint256 amount = getJurorLeftoverPNK(_account);
if (amount == 0) revert NotEligibleForWithdrawal();
jurors[_account].stakedPnk = 0;
totalStaked -= amount;
core.transferBySortitionModule(_account, amount);
emit LeftoverPNKWithdrawn(_account, amount);
}
Expand Down
2 changes: 1 addition & 1 deletion contracts/src/arbitration/arbitrables/ArbitrableExample.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ contract ArbitrableExample is IArbitrableV2 {
}

/// @inheritdoc IArbitrableV2
function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external override {
function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external virtual override {
uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID];
DisputeStruct storage dispute = disputes[localDisputeID];
if (msg.sender != address(arbitrator)) revert ArbitratorOnly();
Expand Down
4 changes: 2 additions & 2 deletions contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,8 +475,8 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
address payable _beneficiary,
uint256 _choice
) external returns (uint256 amount) {
(, , , bool isRuled, ) = core.disputes(_coreDisputeID);
if (!isRuled) revert DisputeNotResolved();
(, , KlerosCore.Period period, , ) = core.disputes(_coreDisputeID);
if (period != KlerosCore.Period.execution) revert DisputeNotResolved();
if (core.paused()) revert CoreIsPaused();
if (!coreDisputeIDToActive[_coreDisputeID].dispute) revert DisputeUnknownInThisDisputeKit();

Expand Down
10 changes: 9 additions & 1 deletion contracts/src/libraries/SafeERC20.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
/// To use this library you can add a `using SafeERC20 for IERC20;` statement to your contract,
/// which allows you to call the safe operations as `token.safeTransfer(...)`, etc.
library SafeERC20 {
/// @notice Emits when safeTransfer fails.
/// @param _token Token to transfer.
/// @param _to Recipient address.
/// @param _value Amount transferred.
event SafeTransferFailed(IERC20 _token, address _to, uint256 _value);

/// @notice Increases the allowance granted to `spender` by the caller.
/// @param _token Token to transfer.
/// @param _spender The address which will spend the funds.
Expand All @@ -31,7 +37,9 @@ library SafeERC20 {
/// @return Whether transfer succeeded or not.
function safeTransfer(IERC20 _token, address _to, uint256 _value) internal returns (bool) {
(bool success, bytes memory data) = address(_token).call(abi.encodeCall(IERC20.transfer, (_to, _value)));
return (success && (data.length == 0 || abi.decode(data, (bool))));
bool ok = success && (data.length == 0 || abi.decode(data, (bool)));
if (!ok) emit SafeTransferFailed(_token, _to, _value);
return ok;
}

/// @notice Calls transferFrom() without reverting.
Expand Down
52 changes: 52 additions & 0 deletions contracts/src/test/MaliciousArbitrableMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.24;

import {IArbitratorV2, IDisputeTemplateRegistry, IERC20, ArbitrableExample} from "../arbitration/arbitrables/ArbitrableExample.sol";

/// @title MaliciousArbitrableMock
/// A mock contract to check intentional rule() revert.
contract MaliciousArbitrableMock is ArbitrableExample {
bool public doRevert;

function changeBehaviour(bool _doRevert) external {
doRevert = _doRevert;
}

constructor(
IArbitratorV2 _arbitrator,
string memory _templateData,
string memory _templateDataMappings,
bytes memory _arbitratorExtraData,
IDisputeTemplateRegistry _templateRegistry,
IERC20 _weth
)
ArbitrableExample(
_arbitrator,
_templateData,
_templateDataMappings,
_arbitratorExtraData,
_templateRegistry,
_weth
)
{
doRevert = true;
}

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);
}

error RuleReverted();
}
Loading
Loading