Adopt ERC-5313 by renaming governor to owner
#2112
Conversation
WalkthroughProject-wide rename of governance surface from “governor” to “owner,” updating storage, modifiers, errors, initializers, upgrade authorization, and public method names across arbitration, RNG, gateways, proxies, v1 adapters, scripts, and tests. Adds a local Chainlink VRF base (owner-aware) and replaces the Hardhat task “change-governor” with “change-owner.” Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant Hardhat as Hardhat CLI
participant Task as change-owner task
participant C as Contracts (Core, DK, Resolver, Registries, RNG, Proxy, Sortition, Evidence)
Dev->>Hardhat: npx hardhat change-owner --newOwner <addr> [--coreType base|neo|university]
Hardhat->>Task: Load task (validate params)
Task->>Task: Resolve coreType, getContracts()
loop For each contract in set
Task->>C: changeOwner(newOwner)
C-->>Task: tx receipt / error
Task->>Dev: Log per-contract status
end
Task->>Dev: Done
sequenceDiagram
autonumber
participant User
participant RNG as ChainlinkRNG (owner-based)
participant VRF as VRFConsumerBaseV2Plus
participant Coord as VRF Coordinator
User->>RNG: requestRandomness()
RNG->>Coord: request via s_vrfCoordinator
Coord-->>VRF: rawFulfillRandomWords(requestId, words)
VRF->>RNG: fulfillRandomWords(requestId, words)
RNG-->>User: randomness delivered
Note over RNG,VRF: Admin ops gated by owner (changeOwner, set params)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
jaybuidl
added
Type: Enhancement ✨
Compatibility: ABI change 🗯
jaybuidl
force-pushed
the
refactor/governor-to-owner
branch
from
b2807a3 to
ff8d216
Compare
jaybuidl
force-pushed
the
refactor/governor-to-owner
branch
from
ff8d216 to
d0a9a80
Compare
|
✅ 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-university 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. |
✅ Deploy Preview for kleros-v2-testnet-devtools 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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
contracts/src/token/Faucet.sol (1)
55-62: Reentrancy window in request(): set state before external call (CEI) and use SafeERC20.A malicious/non-standard token could reenter before
withdrewAlreadyis set. Move the state write before the transfer.Apply:
require( !withdrewAlready[msg.sender], "You have used this faucet already. If you need more tokens, please use another address." ); - token.transfer(msg.sender, amount); - withdrewAlready[msg.sender] = true; + withdrewAlready[msg.sender] = true; // CEI: effects before interactions + token.safeTransfer(msg.sender, amount);Optional hardening: add
nonReentrantviaReentrancyGuardif you expect interacting with arbitrary tokens.contracts/scripts/utils/execution.ts (1)
23-39: Bug: Wrong transaction type; sendTransaction called with ContractTransaction.ContractTransaction represents an already-sent tx. Here you need a TransactionRequest/PopulatedTransaction. Also ensure tx.to is defined.
Apply this diff within the function:
-export const execute = async (tx: ContractTransaction) => { +export const execute = async (tx: TransactionRequest) => { const hre = require("hardhat"); const { ethers } = hre; - const contract = await ethers.getContractAt(governableAbi, tx.to); + if (!tx.to) throw new Error("tx.to is required"); + const contract = await ethers.getContractAt(governableAbi, tx.to); const owner = await contract.owner(); const isContract = (await ethers.provider.getCode(owner)).length > 2; if (isContract) { // Don't execute, just log the tx. It must be submitted for execution separately. - const { to, value, data } = tx; + const { to, value, data } = tx; transactions.push(transaction({ to, value, data })); console.log("tx = %O", tx); } else { // Execute the tx const signer = (await ethers.getSigners())[0]; - await signer.sendTransaction(tx); + if (signer.address.toLowerCase() !== owner.toLowerCase()) { + throw new Error(`Signer ${signer.address} does not match owner ${owner}`); + } + const { to, value, data } = tx; + await signer.sendTransaction({ to, value, data }); } };And add the missing type import at the top of the file:
// at Line 1 import { type TransactionRequest } from "ethers";contracts/src/arbitration/devtools/KlerosCoreRuler.sol (3)
333-337: Disallow zero rate to avoid division-by-zero.Harden the setter; it’s owner-only but prevents foot-guns.
function changeCurrencyRates(IERC20 _feeToken, uint64 _rateInEth, uint8 _rateDecimals) external onlyByOwner { - currencyRates[_feeToken].rateInEth = _rateInEth; + if (_rateInEth == 0) revert InvalidCurrencyRate(); + currencyRates[_feeToken].rateInEth = _rateInEth; currencyRates[_feeToken].rateDecimals = _rateDecimals; emit NewCurrencyRate(_feeToken, _rateInEth, _rateDecimals); }
523-529: ETH transfer uses.sendand ignores failure; fix payout + idempotency.
.sendreturns false on failure and you still incrementsumFeeRewardPaid, desyncing accounting; repeat calls try to pay again. Use.calland require success; optionally mark the round as paid.- if (round.feeToken == NATIVE_CURRENCY) { - // The dispute fees were paid in ETH - payable(account).send(feeReward); - } else { + if (round.feeToken == NATIVE_CURRENCY) { + // The dispute fees were paid in ETH + (bool ok, ) = payable(account).call{value: feeReward}(""); + if (!ok) revert TransferFailed(); + } else { // The dispute fees were paid in ERC20 round.feeToken.safeTransfer(account, feeReward); }Optional: add a
bool paid;inRoundand guardexecuteto be callable once per round.
432-471: Validate_numberOfChoices > 0before modulo.Avoid modulo by zero in automatic-random mode.
Minimal guard (either in
createDisputeor_autoRule):- function _createDispute( + function _createDispute( uint256 _numberOfChoices, bytes memory _extraData, IERC20 _feeToken, uint256 _feeAmount ) internal returns (uint256 disputeID) { + if (_numberOfChoices == 0) revert InvalidNumberOfChoices();Add (outside this hunk):
error InvalidNumberOfChoices();contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (1)
80-92: Unsafe bytes length guard before assembly reads (potential OOB read).You read at offsets 0x80 and 0xA0 but only check length >= 160. Reading the 5th word at 0xA0 requires length >= 192. Fix guard and conditionally read tokenId.
Apply this diff:
- // Need at least 160 bytes to safely read the parameters - if (_extraData.length < 160) return (address(0), false, 0); + // Need at least 160 bytes to read packedTokenGateAndFlag; tokenId at 160 requires 192 bytes. + if (_extraData.length < 160) return (address(0), false, 0); @@ - let packedTokenGateIsERC1155 := mload(add(_extraData, 0x80)) // 4th parameter at offset 128 - tokenId := mload(add(_extraData, 0xA0)) // 5th parameter at offset 160 (moved up) + let packedTokenGateIsERC1155 := mload(add(_extraData, 0x80)) // 4th parameter at offset 128 + // Read tokenId only if present (length >= 192) to avoid OOB read. + if iszero(lt(mload(_extraData), 192)) { + tokenId := mload(add(_extraData, 0xA0)) // 5th parameter at offset 160 + }contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
68-70: reinitialize() lacks access control; any EOA can permanently set wNative.Add onlyByOwner to prevent a one-shot hostile reinit.
-function reinitialize(address _wNative) external reinitializer(9) { +function reinitialize(address _wNative) external reinitializer(9) onlyByOwner { wNative = _wNative; }contracts/src/arbitration/SortitionModuleBase.sol (1)
156-161: Disallow zero RNG; generating-phase call is good.Reject address(0) to prevent bricking passPhase().
-function changeRandomNumberGenerator(IRNG _rng) external onlyByOwner { - rng = _rng; +function changeRandomNumberGenerator(IRNG _rng) external onlyByOwner { + require(address(_rng) != address(0), "RngZeroAddress"); + rng = _rng; if (phase == Phase.generating) { rng.requestRandomness(); } }contracts/src/arbitration/arbitrables/ArbitrableExample.sol (1)
129-133: Use SafeERC20 helpers; drop boolean checks.safeTransferFrom/safeIncreaseAllowance revert on failure; current checks are redundant and brittle.
- if (!weth.safeTransferFrom(msg.sender, address(this), _feeInWeth)) revert TransferFailed(); - if (!weth.increaseAllowance(address(arbitrator), _feeInWeth)) revert AllowanceIncreaseFailed(); + weth.safeTransferFrom(msg.sender, address(this), _feeInWeth); + weth.safeIncreaseAllowance(address(arbitrator), _feeInWeth);Optionally remove now-unused errors TransferFailed/AllowanceIncreaseFailed.
contracts/src/rng/RNGWithFallback.sol (2)
30-41: Constructor: validate _owner and _consumerAvoid deploying with zero owner/consumer which would brick governance or usage.
constructor(address _owner, address _consumer, uint256 _fallbackTimeoutSeconds, IRNG _rng) { if (address(_rng) == address(0)) revert InvalidDefaultRNG(); - owner = _owner; + if (_owner == address(0) || _consumer == address(0)) revert ZeroAddress(); + owner = _owner; consumer = _consumer; fallbackTimeoutSeconds = _fallbackTimeoutSeconds; rng = _rng; }Also declare once:
+error ZeroAddress();
97-101: Fallback before any request: tighten conditionIf receiveRandomness() is called before requestRandomness(), requestTimestamp==0 and fallback may trigger immediately. Gate on requestTimestamp != 0.
-if (randomNumber == 0 && block.timestamp > requestTimestamp + fallbackTimeoutSeconds) { +if (randomNumber == 0 && requestTimestamp != 0 && block.timestamp > requestTimestamp + fallbackTimeoutSeconds) { randomNumber = uint256(blockhash(block.number - 1)); emit RNGFallback(); }contracts/src/rng/ChainlinkRNG.sol (1)
48-51: Missing ConsumerOnly error declarationSame issue for ConsumerOnly().
+error ConsumerOnly();contracts/src/arbitration/university/KlerosCoreUniversity.sol (2)
678-741: Reentrancy risk inexecute: guard and adhere to CEI.
executeperforms external calls (ERC20 transfers and ETH sends via helper functions) while mutating shared round state. Without a reentrancy guard, a malicious token could reenter and corrupt repartitions or accounting.Apply a lightweight nonReentrant guard and keep state updates-before-interactions where possible:
+ uint256 private _reentrancyLock; // 0 = unlocked, 1 = locked + + modifier nonReentrant() { + require(_reentrancyLock == 0, "REENTRANCY"); + _reentrancyLock = 1; + _; + _reentrancyLock = 0; + } ... - function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external { + function execute(uint256 _disputeID, uint256 _round, uint256 _iterations) external nonReentrant { Round storage round; { Dispute storage dispute = disputes[_disputeID]; if (dispute.period != Period.execution) revert NotExecutionPeriod(); round = dispute.rounds[_round]; } uint256 start = round.repartitions; uint256 end = round.repartitions + _iterations; ... round.repartitions = end; for (uint256 i = start; i < end; i++) { ... } if (round.pnkPenalties != pnkPenaltiesInRound) { round.pnkPenalties = pnkPenaltiesInRound; } }
789-806: ETH transfers use.sendand ignore failures — funds can get stuck.All ETH payouts use
sendwithout checking the boolean result. If a recipient is a contract, payouts can fail silently; accounting is still advanced, causing unrecoverable leftovers.Switch to
.call{value: ...}("")and require success (or use a specific revert):- payable(owner).send(round.totalFeesForJurors); + (bool okFeeToOwner, ) = payable(owner).call{value: round.totalFeesForJurors}(""); + if (!okFeeToOwner) revert TransferFailed(); ... - payable(account).send(feeReward); + (bool okFeeToJuror, ) = payable(account).call{value: feeReward}(""); + if (!okFeeToJuror) revert TransferFailed(); ... - payable(owner).send(leftoverFeeReward); + (bool okLeftover, ) = payable(owner).call{value: leftoverFeeReward}(""); + if (!okLeftover) revert TransferFailed();This aligns ETH behavior with SafeERC20’s revert-on-failure semantics and prevents silent inconsistencies.
Also applies to: 846-853, 869-895
🧹 Nitpick comments (77)
contracts/src/token/Faucet.sol (5)
21-23: Switch to custom error and drop redundant cast in onlyByOwner (align with repo, save gas).Use
OwnerOnly()and avoidaddress(owner)cast.Apply:
-modifier onlyByOwner() { - require(address(owner) == msg.sender, "Access not allowed: Owner only."); - _; -} +modifier onlyByOwner() { + if (msg.sender != owner) revert OwnerOnly(); + _; +}Add once near the top of the file:
error OwnerOnly();
30-33: Owner initialization OK; consider emitting event on deployment.Optional: emit
OwnershipTransferred(address(0), msg.sender)for indexability.
39-41: Validate non-zero owner and emit OwnershipTransferred on change.Prevents bricking and aids monitoring.
Apply:
-function changeOwner(address _owner) public onlyByOwner { - owner = _owner; -} +function changeOwner(address _owner) public onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + address old = owner; + owner = _owner; + emit OwnershipTransferred(old, _owner); +}Add once:
error ZeroAddress(); event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
43-45: Emit AmountChanged for off-chain visibility (optional).Also consider sanity-checking
_amount > 0if zero would stall UX.Apply:
-function changeAmount(uint256 _amount) public onlyByOwner { - amount = _amount; -} +function changeAmount(uint256 _amount) public onlyByOwner { + uint256 old = amount; + amount = _amount; + emit AmountChanged(old, _amount); +}Add once:
event AmountChanged(uint256 oldAmount, uint256 newAmount);
68-70: Prefer explicit uint256 for return type.Minor consistency/readability.
Apply:
-function balance() public view returns (uint) { +function balance() public view returns (uint256) { return token.balanceOf(address(this)); }contracts/scripts/viemTest.ts (1)
18-18: Minor: prefer await over then() for readability and error surfacing.
Using await keeps stack traces cleaner in scripts.Apply this diff:
- await disputeKit.read.owner().then(console.log); + const owner = await disputeKit.read.owner(); + console.log(owner);contracts/scripts/utils/tx-builder.ts (2)
3-3: Nit: rename owner constant to safeAddress to avoid confusion with contract owner().
Clarifies intent in script metadata and URL.Apply this diff:
-const owner = "0x66e8DE9B42308c6Ca913D1EE041d6F6fD037A57e"; +const safeAddress = "0x66e8DE9B42308c6Ca913D1EE041d6F6fD037A57e"; @@ - createdFromSafeAddress: owner, + createdFromSafeAddress: safeAddress, @@ -export const transactionBuilderUrl = `https://app.safe.global/apps/open?safe=arb1:${owner}&appUrl=https%3A%2F%2Fapps-portal.safe.global%2Ftx-builder`; +export const transactionBuilderUrl = `https://app.safe.global/apps/open?safe=arb1:${safeAddress}&appUrl=https%3A%2F%2Fapps-portal.safe.global%2Ftx-builder`;Also applies to: 15-15, 45-45
45-45: Optional: derive the Safe network prefix instead of hardcoding 'arb1'.
Prevents mismatch if this utility is reused on other chains (e.g., Arbitrum Sepolia).Example outside this hunk:
const safePrefixByChainId: Record<number, string> = { [arbitrum.id]: "arb1", // add other chains if needed }; export const transactionBuilderUrl = `https://app.safe.global/apps/open?safe=${safePrefixByChainId[arbitrum.id]}:${safeAddress}&appUrl=` + encodeURIComponent("https://apps-portal.safe.global/tx-builder");contracts/CHANGELOG.md (1)
15-15: Style: tighten wording per LanguageTool suggestion.
Replace “in order to comply” with “to comply.”Apply this diff:
-- **Breaking:** Rename `governor` to `owner` in order to comply with the lightweight ownership standard [ERC-5313](https://eipsinsight.com/ercs/erc-5313) ([#2112](https://github.com/kleros/kleros-v2/issues/2112)) +- **Breaking:** Rename `governor` to `owner` to comply with the lightweight ownership standard [ERC-5313](https://eipsinsight.com/ercs/erc-5313) ([#2112](https://github.com/kleros/kleros-v2/issues/2112))contracts/scripts/utils/execution.ts (1)
27-31: Optional: Gracefully handle non-ownable targets.Wrap owner() with try/catch and default to EOA execution if the call reverts; log a warning so the script is robust against legacy contracts lacking owner().
contracts/src/proxy/mock/by-inheritance/UpgradedByInheritance.sol (1)
23-25: Nit: Prevent zero-address owner.Consider rejecting address(0) to avoid locking upgrades in mocks that depend on authorization.
Proposed tweak:
- function initialize(address _owner) external virtual reinitializer(1) { - owner = _owner; + function initialize(address _owner) external virtual reinitializer(1) { + require(_owner != address(0), "owner = 0"); + owner = _owner;contracts/src/proxy/mock/UUPSUpgradeableMocks.sol (1)
31-35: Init assigns owner — OK. Optional zero-address guard.Add a check to prevent setting owner to address(0) in tests that rely on authorization.
- function initialize(address _owner) external { + function initialize(address _owner) external { require(!initialized, "Contract instance has already been initialized"); - owner = _owner; + require(_owner != address(0), "owner = 0"); + owner = _owner; initialized = true; }contracts/src/arbitration/arbitrables/DisputeResolver.sol (2)
48-53: Emit event and validate new owner in changeOwner.Add an OwnershipTransferred-like event and forbid address(0) unless renounce is intended.
Apply within this function:
- function changeOwner(address _owner) external { + function changeOwner(address _owner) external { if (owner != msg.sender) revert OwnerOnly(); - owner = _owner; + if (_owner == address(0)) revert InvalidOwner(); + address _prev = owner; + owner = _owner; + emit OwnershipTransferred(_prev, _owner); }And add outside this hunk:
// near the Errors section event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); error InvalidOwner();
55-63: Optional: Emit change events for arbitrator and template registry.Helps indexers and off-chain consumers react to governance changes.
Proposed additions:
- function changeArbitrator(IArbitratorV2 _arbitrator) external { + function changeArbitrator(IArbitratorV2 _arbitrator) external { if (owner != msg.sender) revert OwnerOnly(); - arbitrator = _arbitrator; + arbitrator = _arbitrator; + emit ArbitratorChanged(_arbitrator); } - function changeTemplateRegistry(IDisputeTemplateRegistry _templateRegistry) external { + function changeTemplateRegistry(IDisputeTemplateRegistry _templateRegistry) external { if (owner != msg.sender) revert OwnerOnly(); - templateRegistry = _templateRegistry; + templateRegistry = _templateRegistry; + emit TemplateRegistryChanged(_templateRegistry); }Plus event declarations:
event ArbitratorChanged(IArbitratorV2 indexed arbitrator); event TemplateRegistryChanged(IDisputeTemplateRegistry indexed templateRegistry);contracts/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol (2)
26-29: Guard against zero-address owner in initializer.Avoid bricking upgradeability by ensuring
_owner != address(0).- function initialize(address _owner) external virtual reinitializer(1) { - owner = _owner; + function initialize(address _owner) external virtual reinitializer(1) { + require(_owner != address(0), "Owner is zero"); + owner = _owner; counter = 1; }
31-33: Use custom error for upgrade auth for gas and repo-wide consistency.Most contracts in this PR use custom errors (e.g., OwnerOnly()). Mirror that here.
- function _authorizeUpgrade(address) internal view override { - require(owner == msg.sender, "No privilege to upgrade"); - } + function _authorizeUpgrade(address) internal view override { + if (owner != msg.sender) revert OwnerOnly(); + }Add once at contract scope (outside the selected lines):
error OwnerOnly();contracts/scripts/changeOwner.ts (4)
48-63: Skip no-op updates and surface current owners before sending txs.Pre-read
owner(); if equal tonewOwner, log and skip. Saves gas and avoids noisy txs.- const updateOwner = async (contractName: string, contractInstance: any) => { + const updateOwner = async (contractName: string, contractInstance: any) => { print.info(`Changing owner for ${contractName}`); const spinner = print.spin(`Executing transaction for ${contractName}...`); try { - const tx = await contractInstance.changeOwner(newOwner); + const current = await contractInstance.owner?.(); + if (current && current.toLowerCase() === newOwner.toLowerCase()) { + spinner.succeed(`${contractName}: already owned by ${newOwner}`); + return; + } + const tx = await contractInstance.changeOwner(newOwner); await tx.wait(); spinner.succeed(`Owner changed for ${contractName}, tx hash: ${tx.hash}`); } catch (error) {
8-16: Input validation is good; consider an optional --dry-run for safety.Add a
--dry-runflag to only print planned calls without broadcasting transactions (useful on prod networks).
65-75: Optional: batch in parallel with rate limiting.If signer/provider can handle it,
Promise.allSettledcan reduce wall time; keep sequential as default to avoid nonce contention.
28-34: Ensure coreType parsing is resilient.Current mapping is fine. If you want friendlier UX, accept aliases (“base/core”, “neo/classic”, “uni”). Not required.
contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol (3)
41-43: Constructor: prevent zero-address owner.Avoid deploying with an unusable owner.
- constructor(address _owner, IKlerosCore _core) { - owner = _owner; + constructor(address _owner, IKlerosCore _core) { + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; core = _core; }Add once (outside the selected lines):
error ZeroAddress();
49-53: Emit event on owner change (observability).Events help off-chain tooling and audits track admin actions. ERC‑5313 doesn’t mandate events, but they’re de facto standard.
- function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + address prev = owner; + owner = _owner; + emit OwnerChanged(prev, _owner); }Add once (outside the selected lines):
event OwnerChanged(address indexed previousOwner, address indexed newOwner);
57-59: Emit event on core change.Track core pointer updates for monitoring and incident response.
- function changeCore(IKlerosCore _core) external onlyByOwner { - core = _core; + function changeCore(IKlerosCore _core) external onlyByOwner { + IKlerosCore prev = core; + core = _core; + emit CoreChanged(prev, _core); }Add once (outside the selected lines):
event CoreChanged(IKlerosCore indexed previousCore, IKlerosCore indexed newCore);contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (4)
151-158: Initializer: validate non-zero owner.Prevents accidental renounce-at-deploy via initializer.
function __DisputeKitClassicBase_initialize( address _owner, KlerosCore _core, address _wNative ) internal onlyInitializing { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; core = _core; wNative = _wNative; }Add once (outside the selected lines):
error ZeroAddress();
173-177: Owner change: add event and zero-address guard.Improves auditability; avoids bricking admin by mistake.
- function changeOwner(address payable _owner) external onlyByOwner { - owner = _owner; + function changeOwner(address payable _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + address prev = owner; + owner = _owner; + emit OwnerChanged(prev, _owner); }Add once (outside the selected lines):
event OwnerChanged(address indexed previousOwner, address indexed newOwner);
181-183: Core change: emit event.Track governance-sensitive pointer updates on-chain.
- function changeCore(address _core) external onlyByOwner { - core = KlerosCore(_core); + function changeCore(address _core) external onlyByOwner { + KlerosCore prev = core; + core = KlerosCore(_core); + emit CoreChanged(prev, core); }Add once (outside the selected lines):
event CoreChanged(KlerosCore indexed previousCore, KlerosCore indexed newCore);
164-171: Optional: emit event for executeOwnerProposal.Admin-call-throughs benefit from an event with destination, amount, and calldata hash for forensics.
Example (outside selected lines):
event OwnerCallExecuted(address indexed destination, uint256 amount, bytes data); /// inside executeOwnerProposal, after success check: emit OwnerCallExecuted(_destination, _amount, _data);contracts/src/arbitration/KlerosCoreBase.sol (4)
93-93: Add ownership change event + zero-address guard to harden ERC‑5313 adoption
- Emit an OwnershipChanged-style event when owner changes (helps indexers and ops).
- Guard against setting owner to zero.
Apply within changeOwner():
- function changeOwner(address payable _owner) external onlyByOwner { - owner = _owner; - } + function changeOwner(address payable _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + address previousOwner = owner; + owner = _owner; + emit OwnerChanged(previousOwner, _owner); + }Add outside the selected range:
// In the Events section event OwnerChanged(address indexed previousOwner, address indexed newOwner); // In the Errors section error ZeroAddress();Also applies to: 281-286, 1216-1218
195-208: Validate initializer inputs (_owner, _guardian, contracts) to prevent accidental brickingAdd non-zero checks for critical addresses before assignment.
- function __KlerosCoreBase_initialize( + function __KlerosCoreBase_initialize( address _owner, address _guardian, IERC20 _pinakion, address _jurorProsecutionModule, IDisputeKit _disputeKit, bool _hiddenVotes, uint256[4] memory _courtParameters, uint256[4] memory _timesPerPeriod, bytes memory _sortitionExtraData, ISortitionModule _sortitionModuleAddress, address _wNative ) internal onlyInitializing { + if (_owner == address(0) || _guardian == address(0) || address(_pinakion) == address(0)) revert ZeroAddress(); + if (address(_disputeKit) == address(0) || address(_sortitionModuleAddress) == address(0) || _wNative == address(0)) revert ZeroAddress(); owner = _owner;
794-797: Leftovers redirected to owner — confirm intended treasury flowBehavioral change routes 100% of “no coherent juror” leftovers to owner. Confirm this aligns with current ops/treasury policy.
272-279: Action Required: Clean up stale governor-era references in docs and artifactsThe new
executeOwnerProposalandonlyByOwnerguard are correctly in place in Solidity code, and there are no remaining references in live contracts. However, legacy “governor” terminology persists in various external artifacts and documentation. Please update or remove these stale references to avoid confusion:• Subgraph ABI migrations
– subgraph/core-neo/abi-migrations/SortitionModuleNeo.json (changeGovernor)
– subgraph/core-neo/abi-migrations/KlerosCoreNeo.json (GovernorOnly, changeGovernor, executeGovernorProposal)
– subgraph/core/abi-migrations/KlerosCore.json (GovernorOnly, changeGovernor, executeGovernorProposal)
– subgraph/core/abi-migrations/SortitionModule.json (changeGovernor)• Specifications
– contracts/specifications/arbitrator.md: lines callingunpause()gated byonlyByGovernor, and related examples• Audit metrics
– contracts/audit/METRICS.md: numerous entries (e.g.executeGovernorProposal,changeGovernor, various “onlyByGovernor” rows)• Deployment JSONs
– contracts/deployments/**/…/*.json: ABIs and metadata in chiadoDevnet, gnosischain, chiado, etc., listingchangeGovernorandexecuteGovernorProposalfunctionsRecommendation:
- Regenerate subgraph schemas or migration files to reflect
executeOwnerProposal/onlyByOwner.- Update specification and audit docs to replace “governor” terminology with “owner”.
- Clean up deployment ABIs (or version them) to remove obsolete governor-era methods.
These changes are non-blocking for on-chain functionality but will align documentation and tooling with the updated access control.
contracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.sol (1)
145-145: Add OwnershipChanged event + zero-owner guard for parity and ops visibilityEmit an event when changing owner; prevent setting to zero address.
- modifier onlyByOwner() { - require(owner == msg.sender); + modifier onlyByOwner() { + require(owner == msg.sender, "OwnerOnly"); }- function changeOwner(address _owner) external onlyByOwner { - owner = _owner; - } + function changeOwner(address _owner) external onlyByOwner { + require(_owner != address(0), "ZeroAddress"); + address previousOwner = owner; + owner = _owner; + emit OwnerChanged(previousOwner, _owner); + }Add outside the selected range:
event OwnerChanged(address indexed previousOwner, address indexed newOwner);Also applies to: 200-204, 276-281
contracts/src/kleros-v1/kleros-liquid/KlerosLiquidToV2Governor.sol (1)
29-42: Add OwnershipChanged event + zero-owner guard; keep interface parityEmit event on changeOwner; forbid zero.
- modifier onlyByOwner() { - require(owner == msg.sender); + modifier onlyByOwner() { + require(owner == msg.sender, "OwnerOnly"); }- function changeOwner(address _owner) external onlyByOwner { - owner = _owner; - } + function changeOwner(address _owner) external onlyByOwner { + require(_owner != address(0), "ZeroAddress"); + address previousOwner = owner; + owner = _owner; + emit OwnerChanged(previousOwner, _owner); + }Add outside the selected range:
event OwnerChanged(address indexed previousOwner, address indexed newOwner);Also applies to: 71-75
contracts/src/gateway/HomeGateway.sol (1)
32-32: Ownership initialization/change: add event + zero checks
- Validate _owner != address(0) on initialize/changeOwner.
- Emit event on owner changes for observability.
- function initialize( + function initialize( address _owner, IArbitratorV2 _arbitrator, IVeaInbox _veaInbox, uint256 _foreignChainID, address _foreignGateway, IERC20 _feeToken ) external reinitializer(1) { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner;- function changeOwner(address _owner) external onlyByOwner { - owner = _owner; - } + function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + address previousOwner = owner; + owner = _owner; + emit OwnerChanged(previousOwner, _owner); + }Add outside the selected range:
event OwnerChanged(address indexed previousOwner, address indexed newOwner); error ZeroAddress();Also applies to: 69-76, 96-100
contracts/src/gateway/ForeignGateway.sol (1)
104-109: Use onlyByOwner consistently (avoid inline checks) + add zero-owner guardUnify ACL by applying the modifier to changeOwner and changeHomeGateway; add zero check for owner.
- function changeOwner(address _owner) external { - if (owner != msg.sender) revert OwnerOnly(); - owner = _owner; - } + function changeOwner(address _owner) external onlyByOwner { + if (_owner == address(0)) revert ZeroAddress(); + address previousOwner = owner; + owner = _owner; + emit OwnerChanged(previousOwner, _owner); + }- function changeHomeGateway(address _homeGateway) external { - if (owner != msg.sender) revert OwnerOnly(); - homeGateway = _homeGateway; - } + function changeHomeGateway(address _homeGateway) external onlyByOwner { + homeGateway = _homeGateway; + }Add outside the selected range:
event OwnerChanged(address indexed previousOwner, address indexed newOwner); error ZeroAddress();Also applies to: 124-126
contracts/src/arbitration/devtools/KlerosCoreRuler.sol (4)
88-88: ERC-5313 alignment looks good; consider explicit interface conformance.Public
ownerexposes the standard surface. Optionally implement anIERC5313interface (owner() view) to make conformance explicit across the repo.
228-235: Allow funding the forwarded call in the same tx.If you intend to forward ETH that arrives with this tx, mark the function payable.
-function executeOwnerProposal(address _destination, uint256 _amount, bytes memory _data) external onlyByOwner { +function executeOwnerProposal(address _destination, uint256 _amount, bytes memory _data) external payable onlyByOwner {
243-247: Consider zero-address check for token.Optionally prevent setting
pinakiontoaddress(0), unless intentional for tests.
674-674: Custom error is fine; optionally include the caller for better traces.Consider
error OwnerOnly(address caller);and emitrevert OwnerOnly(msg.sender);inonlyByOwner.contracts/src/proxy/mock/by-rewrite/UpgradedByRewriteV2.sol (1)
31-33: Prefer custom error over string for gas and consistency.Match the rest of the codebase’s error style.
- function _authorizeUpgrade(address) internal view override { - require(owner == msg.sender, "No privilege to upgrade"); - } + error OwnerOnly(); + function _authorizeUpgrade(address) internal view override { + if (owner != msg.sender) revert OwnerOnly(); + }contracts/src/arbitration/SortitionModuleNeo.sol (1)
65-71: Emit events on governance parameter changes for traceability.State changes to staking caps lack events; add events to aid off-chain monitoring and audits.
Apply within-range diffs:
- function changeMaxStakePerJuror(uint256 _maxStakePerJuror) external onlyByOwner { - maxStakePerJuror = _maxStakePerJuror; - } + function changeMaxStakePerJuror(uint256 _maxStakePerJuror) external onlyByOwner { + uint256 prev = maxStakePerJuror; + maxStakePerJuror = _maxStakePerJuror; + emit MaxStakePerJurorChanged(prev, _maxStakePerJuror); + } - function changeMaxTotalStaked(uint256 _maxTotalStaked) external onlyByOwner { - maxTotalStaked = _maxTotalStaked; - } + function changeMaxTotalStaked(uint256 _maxTotalStaked) external onlyByOwner { + uint256 prev = maxTotalStaked; + maxTotalStaked = _maxTotalStaked; + emit MaxTotalStakedChanged(prev, _maxTotalStaked); + }Add these declarations near storage:
event MaxStakePerJurorChanged(uint256 previous, uint256 current); event MaxTotalStakedChanged(uint256 previous, uint256 current);contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
90-93: Initializer: add zero-address checks for owner/coreDefensive checks avoid bricked proxies or accidental zero owner/core.
Apply:
function initialize(address _owner, KlerosCoreUniversity _core) external reinitializer(1) { - owner = _owner; - core = _core; + if (_owner == address(0)) revert ZeroAddress(); + if (address(_core) == address(0)) revert ZeroAddress(); + owner = _owner; + core = _core; }And declare the error:
error OwnerOnly(); +error ZeroAddress(); error KlerosCoreOnly();contracts/test/proxy/index.ts (1)
82-82: Comment update accurateNote about
ownerslot being zeroed in implementation context is correct rationale for the revert.Consider adding a one-liner: “implementations must never be used directly” to reinforce best practice.
contracts/test/evidence/index.ts (1)
83-87: Direct owner handover flow is fine; consider asserting an OwnershipChanged event if emittedIf ModeratedEvidenceModule emits an event on ownership transfer, add an assertion to strengthen the test. If no event exists, consider adding one repo-wide for better auditability.
contracts/src/arbitration/evidence/EvidenceModule.sol (1)
39-42: Guard against zero owner on initializationDisallow initializing with address(0) to avoid bricking upgrades/governance if misconfigured.
function initialize(address _owner) external reinitializer(1) { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; }Add once in the Errors section:
error ZeroAddress();contracts/src/arbitration/KlerosCoreNeo.sol (2)
89-91: Emit an event on juror NFT changeState changes without events are hard to track onchain. Emit a JurorNftChanged event.
function changeJurorNft(IERC721 _jurorNft) external onlyByOwner { - jurorNft = _jurorNft; + jurorNft = _jurorNft; + emit JurorNftChanged(_jurorNft); }Add once near other events:
event JurorNftChanged(IERC721 jurorNft);
96-98: Emit an event on whitelist updatesHelps indexers and offchain services react.
function changeArbitrableWhitelist(address _arbitrable, bool _allowed) external onlyByOwner { - arbitrableWhitelist[_arbitrable] = _allowed; + arbitrableWhitelist[_arbitrable] = _allowed; + emit ArbitrableWhitelistChanged(_arbitrable, _allowed); }Add once near other events:
event ArbitrableWhitelistChanged(address indexed arbitrable, bool allowed);contracts/src/arbitration/DisputeTemplateRegistry.sol (2)
41-45: Add zero-address guard in initializerPrevents accidental bricking on deployment.
function initialize(address _owner) external reinitializer(1) { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; }Also add once in Errors:
error ZeroAddress();
61-65: Emit OwnershipChanged and consider 2-step transferEmitting an event is important for monitoring. Optionally move to a 2-step flow to reduce fat-finger risk.
function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + address prev = owner; + owner = _owner; + emit OwnershipChanged(prev, _owner); }Add once near top:
event OwnershipChanged(address indexed previousOwner, address indexed newOwner);If you prefer 2-step, I can draft Ownable2Step-style helpers without pulling OZ.
contracts/src/rng/BlockhashRNG.sol (4)
18-18: Nit: comment says “withdraw funds” though contract has no withdrawalsUpdate to “address that can administer this RNG” to avoid confusion.
43-50: Constructor: validate inputsGuard against zero addresses to avoid lost admin/consumer.
constructor(address _owner, address _consumer, uint256 _lookaheadTime) { - owner = _owner; - consumer = _consumer; + if (_owner == address(0) || _consumer == address(0)) revert ZeroAddress(); + owner = _owner; + consumer = _consumer; lookaheadTime = _lookaheadTime; }Add once near bottom:
error ZeroAddress();
56-60: Emit event and validate on owner changeEmit OwnershipChanged and prevent setting to zero unless intentionally supporting renounce.
function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + address prev = owner; + owner = _owner; + emit OwnershipChanged(prev, _owner); }Add once:
event OwnershipChanged(address indexed previousOwner, address indexed newOwner);
64-66: Emit event on consumer changeImproves observability for integrations.
function changeConsumer(address _consumer) external onlyByOwner { - consumer = _consumer; + if (_consumer == address(0)) revert ZeroAddress(); + address prev = consumer; + consumer = _consumer; + emit ConsumerChanged(prev, _consumer); }Add once:
event ConsumerChanged(address indexed previousConsumer, address indexed newConsumer);contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
61-66: Validate inputs in initialize() (non-zero owner/core/wNative).Defensive checks avoid bricking proxies on bad params.
function initialize(address _owner, KlerosCore _core, address _wNative) external reinitializer(1) { - __DisputeKitClassicBase_initialize(_owner, _core, _wNative); + require(_owner != address(0), "OwnerZeroAddress"); + require(address(_core) != address(0), "CoreZeroAddress"); + require(_wNative != address(0), "WNativeZeroAddress"); + __DisputeKitClassicBase_initialize(_owner, _core, _wNative); }contracts/src/arbitration/evidence/ModeratedEvidenceModule.sol (5)
54-55: Track ownership changes with an event.Emit a standard event on owner updates to aid indexing and off-chain tooling.
- address public owner; // The address that can make governance changes to the parameters of the contract. + address public owner; // The address that can make governance changes to the parameters of the contract. + event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
64-66: Prefer custom error over string revert; align modifier naming.Use a custom error for gas savings and consider consistent naming (onlyByOwner vs onlyOwner) across the repo.
-modifier onlyOwner() { - require(msg.sender == owner, "The caller must be the owner"); - _; -} +modifier onlyOwner() { + if (msg.sender != owner) revert OwnerOnly(); + _; +}Add outside this range if missing:
- error OwnerOnly();
95-116: Constructor: validate non-zero owner (and optionally templateRegistry).Prevent accidental lockout or broken registry references.
constructor( IArbitratorV2 _arbitrator, - address _owner, + address _owner, IDisputeTemplateRegistry _templateRegistry, @@ ) { - arbitrator = _arbitrator; - owner = _owner; - templateRegistry = _templateRegistry; + arbitrator = _arbitrator; + require(_owner != address(0), "OwnerZeroAddress"); + owner = _owner; + require(address(_templateRegistry) != address(0), "TemplateRegistryZeroAddress"); + templateRegistry = _templateRegistry;
135-139: changeOwner(): add zero-address guard and emit OwnershipTransferred.Brings it closer to common ownership patterns and observability.
-function changeOwner(address _owner) external onlyOwner { - owner = _owner; -} +function changeOwner(address _owner) external onlyOwner { + require(_owner != address(0), "OwnerZeroAddress"); + emit OwnershipTransferred(owner, _owner); + owner = _owner; +}
153-158: Fix docstring for _bondTimeout (copy-paste error).Parameter describes a time window, not a “multiplier.”
-/// @param _bondTimeout Multiplier of arbitration fees that must be paid as fee stake. In basis points. +/// @param _bondTimeout The time window (in seconds) during which the last moderation status can be challenged.contracts/src/rng/RandomizerRNG.sol (2)
53-62: Constructor: validate inputs to avoid misconfiguration.Ensure owner/consumer/randomizer are non-zero.
-constructor(address _owner, address _consumer, IRandomizer _randomizer) { - owner = _owner; - consumer = _consumer; - randomizer = _randomizer; +constructor(address _owner, address _consumer, IRandomizer _randomizer) { + require(_owner != address(0) && _consumer != address(0) && address(_randomizer) != address(0), "ZeroAddress"); + owner = _owner; + consumer = _consumer; + randomizer = _randomizer; callbackGasLimit = 50000; }
68-72: Emit events and validate inputs for governance actions.Improve auditability and safety for owner ops.
+event OwnerChanged(address indexed previousOwner, address indexed newOwner); +event ConsumerChanged(address indexed previousConsumer, address indexed newConsumer); +event RandomizerSet(address indexed randomizer); +event CallbackGasLimitSet(uint256 previous, uint256 current); function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + require(_owner != address(0), "ZeroAddress"); + emit OwnerChanged(owner, _owner); + owner = _owner; } @@ function changeConsumer(address _consumer) external onlyByOwner { - consumer = _consumer; + require(_consumer != address(0), "ZeroAddress"); + emit ConsumerChanged(consumer, _consumer); + consumer = _consumer; } @@ function setCallbackGasLimit(uint256 _callbackGasLimit) external onlyByOwner { - callbackGasLimit = _callbackGasLimit; + emit CallbackGasLimitSet(callbackGasLimit, _callbackGasLimit); + callbackGasLimit = _callbackGasLimit; } @@ function setRandomizer(address _randomizer) external onlyByOwner { - randomizer = IRandomizer(_randomizer); + require(_randomizer != address(0), "ZeroAddress"); + randomizer = IRandomizer(_randomizer); + emit RandomizerSet(_randomizer); } @@ function randomizerWithdraw(uint256 _amount) external onlyByOwner { randomizer.clientWithdrawTo(msg.sender, _amount); + // optionally emit an event for bookkeeping + // emit RandomizerWithdrawn(msg.sender, _amount); }Also applies to: 76-79, 82-84, 88-90, 92-96
contracts/src/arbitration/SortitionModuleBase.sol (2)
102-116: Initializer: check non-zero owner/core/rng.Avoid deploying unusable modules.
function __SortitionModuleBase_initialize( address _owner, KlerosCore _core, @@ ) internal onlyInitializing { - owner = _owner; - core = _core; + require(_owner != address(0), "OwnerZeroAddress"); + require(address(_core) != address(0), "CoreZeroAddress"); + owner = _owner; + core = _core; @@ - rng = _rng; + require(address(_rng) != address(0), "RngZeroAddress"); + rng = _rng;
136-140: changeOwner(): add zero-address guard and event emission (for consistency).-function changeOwner(address _owner) external onlyByOwner { - owner = _owner; -} +event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); +function changeOwner(address _owner) external onlyByOwner { + require(_owner != address(0), "OwnerZeroAddress"); + emit OwnershipTransferred(owner, _owner); + owner = _owner; +}contracts/src/arbitration/PolicyRegistry.sol (2)
33-37: Guard owner checks + emit an ownership-change eventAdd zero-address guard and emit an event for observability when enforcing owner-only access across upgrades and policy writes.
Apply:
modifier onlyByOwner() { - if (owner != msg.sender) revert OwnerOnly(); + if (owner != msg.sender) revert OwnerOnly(); _; }And add (outside this hunk):
+// Events +event OwnerChanged(address indexed previousOwner, address indexed newOwner);
93-94: Custom error present — align ZeroAddress reuseYou already use OwnerOnly(); consider reusing a shared ZeroAddress() (see suggestions above) for consistency with other contracts in this PR.
contracts/src/arbitration/KlerosGovernor.sol (2)
83-86: Modifier name misleading: it enforces self-call, not “owner”onlyByOwner checks msg.sender == address(this). That’s intentional for KlerosGovernor, but the rename makes it easy to misread as EOA ownership. Consider renaming to onlyByContract/onlyBySelf or add a clarifying NatSpec.
-modifier onlyByOwner() { - if (address(this) != msg.sender) revert OwnerOnly(); +/// @dev Restricts calls to internal (contract-self) calls only. +modifier onlyByContract() { + if (address(this) != msg.sender) revert OwnerOnly(); }Then update call sites accordingly.
148-153: Emit events on governance parameter changesEmitting events for deposits/timeouts improves monitoring and off-chain tooling.
Add events and emit:
+event SubmissionDepositChanged(uint256 oldValue, uint256 newValue); +event SubmissionTimeoutChanged(uint256 oldValue, uint256 newValue); +event ExecutionTimeoutChanged(uint256 oldValue, uint256 newValue); +event WithdrawTimeoutChanged(uint256 oldValue, uint256 newValue); function changeSubmissionDeposit(uint256 _submissionBaseDeposit) external onlyByOwner { - submissionBaseDeposit = _submissionBaseDeposit; + emit SubmissionDepositChanged(submissionBaseDeposit, _submissionBaseDeposit); + submissionBaseDeposit = _submissionBaseDeposit; } ... function changeSubmissionTimeout(uint256 _submissionTimeout) external onlyByOwner duringSubmissionPeriod { - submissionTimeout = _submissionTimeout; + emit SubmissionTimeoutChanged(submissionTimeout, _submissionTimeout); + submissionTimeout = _submissionTimeout; } ... function changeExecutionTimeout(uint256 _executionTimeout) external onlyByOwner { - executionTimeout = _executionTimeout; + emit ExecutionTimeoutChanged(executionTimeout, _executionTimeout); + executionTimeout = _executionTimeout; } ... function changeWithdrawTimeout(uint256 _withdrawTimeout) external onlyByOwner { - withdrawTimeout = _withdrawTimeout; + emit WithdrawTimeoutChanged(withdrawTimeout, _withdrawTimeout); + withdrawTimeout = _withdrawTimeout; }Also applies to: 157-159, 163-165, 169-171
contracts/src/rng/RNGWithFallback.sol (1)
69-71: changeConsumer(): guard zero + emitSafer and observable.
-event ConsumerChanged(address indexed previousConsumer, address indexed newConsumer); +event ConsumerChanged(address indexed previousConsumer, address indexed newConsumer); function changeConsumer(address _consumer) external onlyByOwner { - consumer = _consumer; + if (_consumer == address(0)) revert ZeroAddress(); + emit ConsumerChanged(consumer, _consumer); + consumer = _consumer; }contracts/test/foundry/KlerosCore.t.sol (5)
36-153: Unify “owner” wiring (preferownerovermsg.senderin constructors).You already store
owner = msg.sender(Line 72). For clarity and to avoid accidental drift ifownerever differs frommsg.senderwithin a test, passownerexplicitly to contracts that take an owner. At minimum, update BlockHashRNG instantiations in setUp.- rng = new BlockHashRNG(msg.sender, address(sortitionModule), rngLookahead); + rng = new BlockHashRNG(owner, address(sortitionModule), rngLookahead);
165-246: Useownerin assertions for consistency (instead ofmsg.sender).This makes the intent explicit and reduces mental mapping when reading tests.
- assertEq(core.owner(), msg.sender, "Wrong owner"); + assertEq(core.owner(), owner, "Wrong owner"); ... - assertEq(disputeKit.owner(), msg.sender, "Wrong DK owner"); + assertEq(disputeKit.owner(), owner, "Wrong DK owner"); ... - assertEq(sortitionModule.owner(), msg.sender, "Wrong SM owner"); + assertEq(sortitionModule.owner(), owner, "Wrong SM owner");Also applies to: 231-246, 237-241
835-848: Stop prank to avoid bleed-over within the function.Not strictly required because the test ends soon after, but adding
vm.stopPrank()improves readability and avoids surprises if more calls are appended later.vm.startPrank(owner); core.addNewDisputeKit(disputeKit); core.addNewDisputeKit(disputeKit); core.addNewDisputeKit(disputeKit); core.addNewDisputeKit(disputeKit); core.addNewDisputeKit(disputeKit); extraData = abi.encodePacked(uint256(50), uint256(41), uint256(6)); + vm.stopPrank();
1268-1290: Snapshot proxy owner assertions: small consistency polish.Prefer asserting
snapshotProxy.owner() == ownerto mirror constructor wiring and earlier suggestion.- assertEq(snapshotProxy.owner(), msg.sender, "Wrong owner"); + assertEq(snapshotProxy.owner(), owner, "Wrong owner");
3038-3051: RNGWithFallback: unify owner passing.Minor consistency nit: pass
ownerrather thanmsg.senderwhen constructing RNGWithFallback (mirrors earlier RNG suggestion).- rngFallback = new RNGWithFallback(msg.sender, address(sortitionModule), fallbackTimeout, rngMock); + rngFallback = new RNGWithFallback(owner, address(sortitionModule), fallbackTimeout, rngMock);Also applies to: 3079-3085
contracts/src/arbitration/university/KlerosCoreUniversity.sol (4)
586-612: Ensure the picked juror matches the requested_jurorin University mode.You set a transient juror but don’t validate that
disputeKit.draw()actually returned_juror. For pedagogical determinism, consider enforcing equality (or reverting with a clear reason).- address drawnAddress = disputeKit.draw(_disputeID, iteration); + address drawnAddress = disputeKit.draw(_disputeID, iteration); if (drawnAddress == address(0)) { revert NoJurorDrawn(); } + if (drawnAddress != _juror) { + revert NoJurorDrawn(); // or introduce a dedicated error (e.g., RequestedJurorNotEligible) + }
1050-1052: Guard against unset currency rates in conversions.
arbitrationCost(_extraData, _feeToken)will divide by zero if the rate is not set yet. Provide a clearer failure.+ error CurrencyRateNotSet(); ... - function arbitrationCost(bytes calldata _extraData, IERC20 _feeToken) public view override returns (uint256 cost) { - cost = convertEthToTokenAmount(_feeToken, arbitrationCost(_extraData)); + function arbitrationCost(bytes calldata _extraData, IERC20 _feeToken) public view override returns (uint256 cost) { + if (currencyRates[_feeToken].rateInEth == 0) revert CurrencyRateNotSet(); + cost = convertEthToTokenAmount(_feeToken, arbitrationCost(_extraData)); } ... - function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) { - return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / currencyRates[_toToken].rateInEth; + function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) { + uint64 rate = currencyRates[_toToken].rateInEth; + if (rate == 0) revert CurrencyRateNotSet(); + return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / rate; }Also applies to: 928-931
275-277: Emit an ownership change event for better observability.Optional, but emitting
OwnerChanged(oldOwner, newOwner)helps indexers and UI.+ event OwnerChanged(address indexed previousOwner, address indexed newOwner); ... function changeOwner(address payable _owner) external onlyByOwner { - owner = _owner; + address prev = owner; + owner = _owner; + emit OwnerChanged(prev, _owner); }
1162-1162: Typo in error name (InvalidDisputKitParent).Looks unused and misspelled. Rename to
InvalidDisputeKitParentor remove to avoid confusion.-error InvalidDisputKitParent(); +// error InvalidDisputeKitParent(); // (unused) remove or correct if intended
| import { task } from "hardhat/config"; | ||
| import { prompt, print } from "gluegun"; | ||
| import { Cores, getContracts } from "./utils/contracts"; | ||
| import { isAddress } from "viem"; | ||
|
|
||
| const { bold } = print.colors; | ||
|
|
||
| task("change-owner", "Changes the owner for all the contracts") | ||
| .addPositionalParam("newOwner", "The address of the new owner") | ||
| .addOptionalParam("coreType", "The type of core to use between base, neo, university (default: base)", Cores.BASE) | ||
| .setAction(async (taskArgs, hre) => { | ||
| const newOwner = taskArgs.newOwner; | ||
| if (!isAddress(newOwner)) { | ||
| throw new Error("Invalid owner address provided"); | ||
| } | ||
| print.highlight(`💣 Changing owner to ${bold(newOwner)}`); | ||
|
|
||
| const { confirm } = await prompt.ask({ | ||
| type: "confirm", | ||
| name: "confirm", | ||
| message: "Are you sure you want to proceed?", | ||
| }); | ||
| if (!confirm) { | ||
| console.log("Operation cancelled by user."); | ||
| return; | ||
| } | ||
|
|
||
| const coreType = Cores[taskArgs.coreType.toUpperCase() as keyof typeof Cores]; | ||
| if (coreType === undefined) { | ||
| console.error("Invalid core type, must be one of base, neo, university"); | ||
| return; | ||
| } | ||
| console.log("Using core type %s", coreType); | ||
|
|
||
| const { | ||
| core, | ||
| disputeKitClassic, | ||
| disputeResolver, | ||
| disputeTemplateRegistry, | ||
| policyRegistry, | ||
| chainlinkRng, | ||
| randomizerRng, | ||
| snapshotProxy, | ||
| sortition, | ||
| evidence, | ||
| } = await getContracts(hre, coreType); | ||
|
|
||
| const updateOwner = async (contractName: string, contractInstance: any) => { | ||
| print.info(`Changing owner for ${contractName}`); | ||
|
|
||
| const spinner = print.spin(`Executing transaction for ${contractName}...`); | ||
| try { | ||
| const tx = await contractInstance.changeOwner(newOwner); | ||
| await tx.wait(); | ||
| spinner.succeed(`Owner changed for ${contractName}, tx hash: ${tx.hash}`); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| spinner.fail(`Failed to change owner for ${contractName}: ${error.message}`); | ||
| } else { | ||
| spinner.fail(`Failed to change owner for ${contractName}: ${String(error)}`); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| await updateOwner("KlerosCore", core); | ||
| await updateOwner("DisputeKitClassic", disputeKitClassic); | ||
| await updateOwner("DisputeResolver", disputeResolver); | ||
| await updateOwner("DisputeTemplateRegistry", disputeTemplateRegistry); | ||
| await updateOwner("PolicyRegistry", policyRegistry); | ||
| await updateOwner("KlerosCoreSnapshotProxy", snapshotProxy); | ||
| await updateOwner("SortitionModule", sortition); | ||
| await updateOwner("EvidenceModule", evidence); | ||
| if (chainlinkRng) await updateOwner("ChainlinkRNG", chainlinkRng); | ||
| if (randomizerRng) await updateOwner("RandomizerRNG", randomizerRng); | ||
|
|
||
| print.success("Owner changed successfully"); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Repo-wide verification: detect leftover governor APIs.
Run the following to find any stragglers like changeGovernor, onlyByGovernor, or GovernorOnly still referenced by scripts or contracts.
🏁 Script executed:
#!/bin/bash
# Find lingering governor-era identifiers.
rg -nP --type=sol --type=ts --type=js -C2 '\b(changeGovernor|onlyByGovernor|GovernorOnly|governor\(\))\b' contracts | sed -n '1,200p'Length of output: 161
I’ve updated the search to use file globs instead of unsupported --type=sol. Please run this to catch any leftover governor-era identifiers:
🏁 Script executed:
#!/bin/bash
# Find lingering governor-era identifiers in .sol, .ts, and .js under contracts/
rg -nP -C2 '\b(changeGovernor|onlyByGovernor|GovernorOnly|governor\(\))\b' -g '*.sol' -g '*.ts' -g '*.js' contracts | sed -n '1,200p'Length of output: 13931
Regenerate or Exclude Stale ABI Definitions
We’ve confirmed dozens of changeGovernor and GovernorOnly occurrences—but all are coming from the auto-generated viem ABI files under
contracts/deployments/testnet.viem.tscontracts/deployments/mainnet.viem.ts
These aren’t hand‐written references in your scripts or contracts themselves, but stale ABI definitions. You should either:
- Regenerate the deployments/ABI files so they no longer include the old governor methods, or
- Exclude
contracts/deployments/**from your grep checks (e.g. add-g '!contracts/deployments/**') and optionally remove them from version control if they’re fully generated at build time.
Please update accordingly to ensure no leftover governor APIs remain in your committed source.
| @@ -151,7 +151,7 @@ contract DisputeResolver is IArbitrableV2 { | |||
| // * Errors * // | |||
| // ************************************* // | |||
|
|
|||
| error GovernorOnly(); | |||
| error OwnerOnly(); | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Error rename to OwnerOnly — LGTM.
Ensure all revert sites across repo now use OwnerOnly().
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '\bGovernorOnly\(\)'
rg -nP -C2 '\bOwnerOnly\(\)'Length of output: 277312
🏁 Script executed:
#!/bin/bash
# Search across Solidity files for old and new error and modifier names
rg -nP --type=sol '\bGovernorOnly\(\)'
rg -nP --type=sol '\bOwnerOnly\(\)'
rg -nP --type=sol '\bonlyByGovernor\b'
rg -nP --type=sol '\bonlyByOwner\b'
rg -nP --type=sol 'revert\s+GovernorOnly\(\)'
rg -nP --type=sol 'revert\s+OwnerOnly\(\)'Length of output: 418
#!/usr/bin/env bash
# Verify no remaining references to the old error
rg -nP '\bGovernorOnly\(\)' -g 'contracts/**/*.sol' -C2 || echo "No GovernorOnly() found"
# Verify all revert sites now use OwnerOnly()
rg -nP 'revert\s+OwnerOnly\(\)' -g 'contracts/**/*.sol' -C2 || echo "No revert OwnerOnly() found"
# Also check for any direct OwnerOnly() usages outside revert()
rg -nP '\bOwnerOnly\(\)' -g 'contracts/**/*.sol' -C2#!/usr/bin/env bash
set -e
# Search for any remaining references to the old error
echo "Searching for revert GovernorOnly()..."
rg -nP 'revert\s+GovernorOnly\(\)' -g 'contracts/**/*.sol' -C2 || echo "✔ No revert GovernorOnly() found"
echo "Searching for any GovernorOnly() usage..."
rg -nP '\bGovernorOnly\(' -g 'contracts/**/*.sol' -C2 || echo "✔ No GovernorOnly() found"
# Confirm that revert sites now use OwnerOnly()
echo "Searching for revert OwnerOnly()..."
rg -nP 'revert\s+OwnerOnly\(\)' -g 'contracts/**/*.sol' -C2 || echo "✖ No revert OwnerOnly() usage found"
echo "Searching for OwnerOnly() usage outside definition..."
rg -nP '\bOwnerOnly\(' -g 'contracts/**/*.sol' -C2🏁 Script executed:
#!/bin/bash
# Search for any remaining references to the old GovernorOnly error
rg -nP 'revert\s+GovernorOnly\(' -C2 || echo "✔ No revert GovernorOnly() found"
rg -nP '\bGovernorOnly\(' -C2 || echo "✔ No GovernorOnly() found"
# Verify revert sites now use OwnerOnly()
rg -nP 'revert\s+OwnerOnly\(' -C2 || echo "✖ No revert OwnerOnly() usage found"
# Search for any OwnerOnly() references
rg -nP '\bOwnerOnly\(' -C2 || echo "✔ No OwnerOnly() found outside definition"Length of output: 277383
Inconsistent rename — OwnerOnly added but GovernorOnly still used (action required)
The rename to OwnerOnly() exists in DisputeResolver.sol but the repo still declares/uses GovernorOnly(); update all remaining uses or revert the rename.
- Files needing attention:
- contracts/src/arbitration/arbitrables/DisputeResolver.sol — defines
error OwnerOnly();. - contracts/src/arbitration/KlerosCoreBase.sol —
onlyByGovernorrevertsGovernorOnly()anderror GovernorOnly()is declared; update to OwnerOnly() (or restore GovernorOnly in DisputeResolver).
- contracts/src/arbitration/arbitrables/DisputeResolver.sol — defines
Suggested minimal change (apply in KlerosCoreBase.sol):
Before:
modifier onlyByGovernor() {
if (governor != msg.sender) revert GovernorOnly();
_;
}
...
error GovernorOnly();
After:
modifier onlyByGovernor() {
if (governor != msg.sender) revert OwnerOnly();
_;
}
...
error OwnerOnly();
Please run a repository-wide search for GovernorOnly( and revert GovernorOnly( and replace consistently, or revert the single-file rename if that was unintended.
🤖 Prompt for AI Agents
In contracts/src/arbitration/KlerosCoreBase.sol around the modifier
onlyByGovernor (and the error declaration near it), the code still reverts
GovernorOnly() and declares error GovernorOnly(), while DisputeResolver.sol
defines error OwnerOnly(); update KlerosCoreBase.sol to use OwnerOnly()
consistently: change the revert in onlyByGovernor to revert OwnerOnly() and
rename the error declaration to error OwnerOnly(); then run a repo-wide search
for any remaining uses of GovernorOnly( or revert GovernorOnly( and replace them
with OwnerOnly( to keep naming consistent (or revert the DisputeResolver rename
if you prefer the original GovernorOnly name).
| /// @param _owner The owner's address. | ||
| /// @param _pinakion The address of the token contract. | ||
| /// @param _courtParameters Numeric parameters of General court (minStake, alpha, feeForJuror and jurorsForCourtJump respectively). | ||
| function initialize( | ||
| address _governor, | ||
| address _owner, | ||
| IERC20 _pinakion, | ||
| uint256[4] memory _courtParameters | ||
| ) external reinitializer(1) { | ||
| governor = _governor; | ||
| owner = _owner; | ||
| pinakion = _pinakion; | ||
|
|
||
| // FORKING_COURT |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against zero owner on initialization.
Avoid accidental bricking by ensuring _owner != address(0).
- function initialize(
- address _owner,
+ function initialize(
+ address _owner,
IERC20 _pinakion,
uint256[4] memory _courtParameters
) external reinitializer(1) {
- owner = _owner;
+ if (_owner == address(0)) revert InvalidOwner();
+ owner = _owner;
pinakion = _pinakion;Add this error (outside this hunk):
error InvalidOwner();🤖 Prompt for AI Agents
In contracts/src/arbitration/devtools/KlerosCoreRuler.sol around lines 175 to
186, initialization does not guard against a zero owner which can brick the
contract; add a custom error declaration `InvalidOwner()` near the top-level
declarations (outside this hunk) and then in initialize(...) validate that
`_owner != address(0)` and revert with `InvalidOwner()` if it is zero (e.g., `if
(_owner == address(0)) revert InvalidOwner();`) before assigning owner and other
state.
| /// @dev Changes the `owner` storage variable. | ||
| /// @param _owner The new value for the `owner` storage variable. | ||
| function changeOwner(address payable _owner) external onlyByOwner { | ||
| owner = _owner; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden ownership transfer: zero-address guard + event + param rename.
Prevents loss of control and improves traceability.
-/// @dev Changes the `owner` storage variable.
-/// @param _owner The new value for the `owner` storage variable.
-function changeOwner(address payable _owner) external onlyByOwner {
- owner = _owner;
+/// @dev Transfers ownership.
+/// @param _newOwner The new owner address.
+function changeOwner(address _newOwner) external onlyByOwner {
+ if (_newOwner == address(0)) revert InvalidOwner();
+ address _old = owner;
+ owner = _newOwner;
+ emit OwnershipTransferred(_old, _newOwner);
}Add this event (outside this hunk, in the events section):
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);🤖 Prompt for AI Agents
In contracts/src/arbitration/devtools/KlerosCoreRuler.sol around lines 237 to
241, the changeOwner function should prevent setting the owner to the zero
address, emit an OwnershipTransferred event, and use a clearer param name; add
the OwnershipTransferred event declaration in the events section (as specified),
rename the function parameter from _owner to newOwner, require(newOwner !=
address(0)) to guard against zero-address transfers, set owner =
payable(newOwner), and emit OwnershipTransferred(previousOwner, newOwner) where
previousOwner is the original owner stored before assignment.
| currencyRates[_feeToken].feePaymentAccepted = _accepted; | ||
| emit AcceptedFeeToken(_feeToken, _accepted); | ||
| } |
There was a problem hiding this comment.
Prevent enabling a token with unset/invalid rate.
Enabling before setting a non-zero rate causes divide-by-zero in convertEthToTokenAmount.
function changeAcceptedFeeTokens(IERC20 _feeToken, bool _accepted) external onlyByOwner {
- currencyRates[_feeToken].feePaymentAccepted = _accepted;
+ if (_accepted) {
+ if (currencyRates[_feeToken].rateInEth == 0) revert InvalidCurrencyRate();
+ }
+ currencyRates[_feeToken].feePaymentAccepted = _accepted;
emit AcceptedFeeToken(_feeToken, _accepted);
}Add (outside this hunk, if not present):
error InvalidCurrencyRate();🤖 Prompt for AI Agents
In contracts/src/arbitration/devtools/KlerosCoreRuler.sol around lines 325-327,
enabling a fee token currently allows setting feePaymentAccepted=true even if
the token's rate is unset/zero, which causes a divide-by-zero in
convertEthToTokenAmount; add an error declaration `InvalidCurrencyRate()` at the
top-level of the file (outside this hunk) if not present, and inside the
function before setting currencyRates[_feeToken].feePaymentAccepted perform a
check that the stored rate for _feeToken is non-zero (revert
InvalidCurrencyRate() if it is zero) so enabling is only allowed when a valid
non-zero rate exists.
| /// @dev Changes the owner of the contract. | ||
| /// @param _owner The new owner. | ||
| function changeOwner(address _owner) external onlyByOwner { | ||
| owner = _owner; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
changeOwner(): add zero-address guard + emit event
Track ownership and prevent zero.
-function changeOwner(address _owner) external onlyByOwner {
- owner = _owner;
+event OwnerChanged(address indexed previousOwner, address indexed newOwner);
+function changeOwner(address _owner) external onlyByOwner {
+ if (_owner == address(0)) revert ZeroAddress();
+ emit OwnerChanged(owner, _owner);
+ owner = _owner;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @dev Changes the owner of the contract. | |
| /// @param _owner The new owner. | |
| function changeOwner(address _owner) external onlyByOwner { | |
| owner = _owner; | |
| } | |
| /// @dev Changes the owner of the contract. | |
| /// @param _owner The new owner. | |
| event OwnerChanged(address indexed previousOwner, address indexed newOwner); | |
| function changeOwner(address _owner) external onlyByOwner { | |
| if (_owner == address(0)) revert ZeroAddress(); | |
| emit OwnerChanged(owner, _owner); | |
| owner = _owner; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/rng/ChainlinkRNG.sol around lines 86 to 90, the changeOwner
function lacks a guard against setting the zero address and does not emit an
ownership change event; add a require check to prevent _owner == address(0),
store the previous owner in a local variable, update owner only after the check,
and emit an OwnershipTransferred (or OwnerChanged) event with (previousOwner,
_owner) so ownership changes are tracked on-chain.
| function changeVrfCoordinator(address _vrfCoordinator) external onlyByOwner { | ||
| s_vrfCoordinator = IVRFCoordinatorV2Plus(_vrfCoordinator); | ||
| emit CoordinatorSet(_vrfCoordinator); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
changeVrfCoordinator(): add zero-address guard (mirror base)
Avoid accidentally zeroing coordinator; keep semantics aligned with base’s setCoordinator.
function changeVrfCoordinator(address _vrfCoordinator) external onlyByOwner {
- s_vrfCoordinator = IVRFCoordinatorV2Plus(_vrfCoordinator);
- emit CoordinatorSet(_vrfCoordinator);
+ if (_vrfCoordinator == address(0)) revert ZeroAddress();
+ s_vrfCoordinator = IVRFCoordinatorV2Plus(_vrfCoordinator);
+ emit CoordinatorSet(_vrfCoordinator);
}📝 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 changeVrfCoordinator(address _vrfCoordinator) external onlyByOwner { | |
| s_vrfCoordinator = IVRFCoordinatorV2Plus(_vrfCoordinator); | |
| emit CoordinatorSet(_vrfCoordinator); | |
| } | |
| function changeVrfCoordinator(address _vrfCoordinator) external onlyByOwner { | |
| if (_vrfCoordinator == address(0)) revert ZeroAddress(); | |
| s_vrfCoordinator = IVRFCoordinatorV2Plus(_vrfCoordinator); | |
| emit CoordinatorSet(_vrfCoordinator); | |
| } |
🤖 Prompt for AI Agents
In contracts/src/rng/ChainlinkRNG.sol around lines 100 to 103, the
changeVrfCoordinator function lacks a zero-address guard; add a require that
_vrfCoordinator != address(0) (with an appropriate revert message) before
assigning s_vrfCoordinator and emitting CoordinatorSet so the function mirrors
the base setCoordinator semantics and prevents accidentally zeroing the
coordinator.
| modifier onlyByOwner() { | ||
| if (owner != msg.sender) revert OwnerOnly(); | ||
| _; | ||
| } |
There was a problem hiding this comment.
Missing custom error declarations
Modifiers revert OwnerOnly()/ConsumerOnly() but errors aren’t declared in this file; this won’t compile.
Add at the end:
+error OwnerOnly();
+error ConsumerOnly();📝 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.
| modifier onlyByOwner() { | |
| if (owner != msg.sender) revert OwnerOnly(); | |
| _; | |
| } | |
| modifier onlyByOwner() { | |
| if (owner != msg.sender) revert OwnerOnly(); | |
| _; | |
| } | |
| error OwnerOnly(); | |
| error ConsumerOnly(); |
🤖 Prompt for AI Agents
In contracts/src/rng/RNGWithFallback.sol around lines 47 to 50, the modifier
uses revert OwnerOnly()/ConsumerOnly() but these custom errors are not declared
in the file; add declarations for both custom errors at the end of the
contract/file (e.g., declare error OwnerOnly(); and error ConsumerOnly();) so
the compiler recognizes them and the modifiers can revert using those custom
errors.
| /// @dev Change the owner | ||
| /// @param _newOwner Address of the new owner | ||
| function changeOwner(address _newOwner) external onlyByOwner { | ||
| owner = _newOwner; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
changeOwner(): add zero-address guard + event
Emit ownership change for off-chain consumers; guard zero.
-function changeOwner(address _newOwner) external onlyByOwner {
- owner = _newOwner;
+event OwnerChanged(address indexed previousOwner, address indexed newOwner);
+function changeOwner(address _newOwner) external onlyByOwner {
+ if (_newOwner == address(0)) revert ZeroAddress();
+ emit OwnerChanged(owner, _newOwner);
+ owner = _newOwner;
}📝 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 Change the owner | |
| /// @param _newOwner Address of the new owner | |
| function changeOwner(address _newOwner) external onlyByOwner { | |
| owner = _newOwner; | |
| } | |
| /// @dev Change the owner | |
| /// @param _newOwner Address of the new owner | |
| event OwnerChanged(address indexed previousOwner, address indexed newOwner); | |
| function changeOwner(address _newOwner) external onlyByOwner { | |
| if (_newOwner == address(0)) revert ZeroAddress(); | |
| emit OwnerChanged(owner, _newOwner); | |
| owner = _newOwner; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/rng/RNGWithFallback.sol around lines 61 to 65, the changeOwner
function lacks a zero-address guard and does not emit an event; add a require to
prevent setting owner to address(0) and emit an event (e.g.,
OwnershipTransferred(address indexed previousOwner, address indexed newOwner))
when owner is changed. Ensure the event is declared in the contract and include
the previous owner and new owner in the emitted event just before updating the
owner storage.
| function withdraw() public onlyByOwner { | ||
| token.transfer(owner, token.balanceOf(address(this))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use SafeERC20 and emit Withdrawn; bare transfer may silently fail on non-standard ERC-20s.
This ensures correct handling of tokens that return no/false values.
Apply:
-function withdraw() public onlyByOwner {
- token.transfer(owner, token.balanceOf(address(this)));
-}
+function withdraw() public onlyByOwner {
+ uint256 bal = token.balanceOf(address(this));
+ token.safeTransfer(owner, bal);
+ emit Withdrawn(owner, bal);
+}Add once (top-level):
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
event Withdrawn(address indexed to, uint256 amount);And inside the contract:
using SafeERC20 for IERC20;🤖 Prompt for AI Agents
In contracts/src/token/Faucet.sol around lines 47 to 49, the withdraw() uses
bare token.transfer which can silently fail for non-standard ERC‑20s; import and
use OpenZeppelin SafeERC20, declare the Withdrawn event, add "using SafeERC20
for IERC20;" at contract scope, change the transfer call to
token.safeTransfer(owner, amount) where amount = token.balanceOf(address(this)),
and emit Withdrawn(owner, amount) after the transfer. Also ensure the token
variable is typed as IERC20 and add the import statement at the top of the file.
Adoption of ERC-5313.
PR-Codex overview
This PR focuses on renaming the
governorvariable and related functions toowneracross multiple contracts and scripts, aligning with the lightweight ownership standard ERC-5313.Detailed summary
governortoownerin multiple contracts.onlyByGovernortoonlyByOwner.GovernorOnlytoOwnerOnly.Summary by CodeRabbit