Proxy contracts reset of the version and initializers ahead of the migration #2125
Conversation
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
✅ Deploy Preview for kleros-v2-testnet 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
contracts/src/arbitration/SortitionModuleNeo.sol (1)
37-49: Add missingreinitialize()entrypoint or update migration script
SortitionModuleNeodefinesinitialize(_owner, _core, _minStakingTime, _maxDrawingTime, _rng, _maxStakePerJuror, _maxTotalStaked) external reinitializer(2)butcontracts/deploy/upgrade-all.ts(line 105) callsreinitialize()with no matching function. Either:
- In
contracts/src/arbitration/SortitionModuleNeo.sol, add a no-op wrapper:/// @dev No-op reinitializer to bump initializer version to 2 for migration tooling. function reinitialize() external reinitializer(2) {}- Or change the script to invoke
initialize(...)with the required arguments instead ofreinitialize().contracts/src/arbitration/evidence/EvidenceModule.sol (1)
61-63: Fix typo in NatSpec.
“responsability” → “responsibility”.- /// @param _externalDisputeID Unique identifier for this dispute outside Kleros. It's the submitter responsability to submit the right evidence group ID. + /// @param _externalDisputeID Unique identifier for this dispute outside Kleros. It's the submitter responsibility to submit the right evidence group ID.contracts/src/arbitration/devtools/KlerosCoreRuler.sol (1)
514-520: Unchecked ETH send: may silently fail and strand funds
sendreturns false on failure and is ignored. Usecalland check success.- if (round.feeToken == NATIVE_CURRENCY) { - // The dispute fees were paid in ETH - payable(account).send(feeReward); - } else { + if (round.feeToken == NATIVE_CURRENCY) { + (bool ok, ) = payable(account).call{value: feeReward}(""); + if (!ok) revert TransferFailed(); + } else {contracts/src/arbitration/university/KlerosCoreUniversity.sol (3)
800-809: ETH transfers via send() ignore failures — funds can get stuck.
send() returns a bool that’s unchecked here; if false, fees remain in the contract while state assumes they were paid.Apply:
- // The dispute fees were paid in ETH - payable(owner).send(round.totalFeesForJurors); + // The dispute fees were paid in ETH + (bool ok, ) = payable(owner).call{value: round.totalFeesForJurors}(""); + if (!ok) revert TransferFailed();
858-865: Unchecked send() to juror — possible silent payout failure.
Same issue as above.- // The dispute fees were paid in ETH - payable(account).send(feeReward); + // The dispute fees were paid in ETH + (bool ok, ) = payable(account).call{value: feeReward}(""); + if (!ok) revert TransferFailed();
890-896: Unchecked send() on leftover payout to owner.
Same failure mode.- // The dispute fees were paid in ETH - payable(owner).send(leftoverFeeReward); + // The dispute fees were paid in ETH + (bool ok, ) = payable(owner).call{value: leftoverFeeReward}(""); + if (!ok) revert TransferFailed();
🧹 Nitpick comments (14)
contracts/src/proxy/Initializable.sol (1)
96-99: Docstring regression: reference should be to reinitializer(1), not initializer().The sentence is now self-referential and contradicts OZ 4.9 docs. It should read “Similar to
reinitializer(1)...”.Apply:
- * Similar to `initializer()`, except that functions marked with `initializer` can be nested in the context of a + * Similar to `reinitializer(1)`, except that functions marked with `initializer` can be nested in the context of acontracts/src/proxy/mock/by-inheritance/UpgradedByInheritance.sol (1)
64-66: Comment no longer matches behavior; clarify intent for the “bad” V3 initializer.
Now using initializer (not a wrong reinitializer version). Either adjust the comment or revert to an intentionally wrong reinitializer to preserve the test’s purpose.Option A (keep initializer, fix comment):
- // Wrong reinitializer version. + // Intentionally wrong: using initializer at V3 should revert on already-initialized proxy.Option B (restore “wrong reinitializer version” semantics):
-function initializeV3() external initializer { +function initializeV3() external reinitializer(1) { // Wrong reinitializer version. }Please confirm which behavior your tests expect.
contracts/src/gateway/ForeignGateway.sol (2)
85-90: Harden initializer with zero-address checksPrevent foot-guns at deployment time.
Apply:
function initialize( address _owner, address _veaOutbox, uint256 _homeChainID, address _homeGateway ) external initializer { + if (_owner == address(0) || _veaOutbox == address(0) || _homeGateway == address(0)) revert InvalidAddress(); owner = _owner; veaOutbox = _veaOutbox; homeChainID = _homeChainID; homeGateway = _homeGateway; localDisputeID = 1; } +error InvalidAddress();
74-79: Fix copy-paste docstringReferences
PolicyRegistryinstead ofForeignGateway.Apply:
-/// @dev Constructs the `PolicyRegistry` contract. +/// @dev Constructs the `ForeignGateway` contract.contracts/src/arbitration/DisputeTemplateRegistry.sol (1)
43-45: Initializer switch: verify upgrade scripts and add basic validation
- Ensure scripts no longer call
initialize2()and won’t attempt to re-runinitialize().- Optional: guard
_owner != address(0)to avoid bricking governance.function initialize(address _owner) external initializer { - owner = _owner; + if (_owner == address(0)) revert OwnerZeroAddress(); + owner = _owner; } +error OwnerZeroAddress();contracts/src/gateway/HomeGateway.sol (3)
76-82: Add zero-address checks to initializerReduces risk of misconfiguration at deploy time.
) external initializer { - owner = _owner; - arbitrator = _arbitrator; - veaInbox = _veaInbox; - foreignChainID = _foreignChainID; - foreignGateway = _foreignGateway; - feeToken = _feeToken; + if (_owner == address(0) || address(_arbitrator) == address(0) || address(_veaInbox) == address(0) || _foreignGateway == address(0)) revert InvalidAddress(); + owner = _owner; + arbitrator = _arbitrator; + veaInbox = _veaInbox; + foreignChainID = _foreignChainID; + foreignGateway = _foreignGateway; + feeToken = _feeToken; } +error InvalidAddress();
61-68: Fix copy-paste docstringMentions
PolicyRegistryinstead ofHomeGateway.-/// @dev Constructs the `PolicyRegistry` contract. +/// @dev Constructs the `HomeGateway` contract.
187-189: ERC20 allowance pattern: prefer safe wrapperUsing raw
increaseAllowancecan break with non-standard tokens. Prefer the SafeERC20 wrapper if available in your library.contracts/src/arbitration/devtools/KlerosCoreRuler.sol (2)
178-207: Initializer switch: add basic validations and confirm scripts
- Ensure no script still calls removed
initialize2().- Optional: validate
_ownerand_pinakionare non-zero.function initialize(address _owner, IERC20 _pinakion, uint256[4] memory _courtParameters) external initializer { - owner = _owner; - pinakion = _pinakion; + if (_owner == address(0) || address(_pinakion) == address(0)) revert InvalidAddress(); + owner = _owner; + pinakion = _pinakion; } +error InvalidAddress();
621-623: Guard against division by zero in rate conversionIf
rateInEthhasn’t been set, this reverts due to division by zero.function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) { - return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / currencyRates[_toToken].rateInEth; + uint64 rate = currencyRates[_toToken].rateInEth; + if (rate == 0) revert InvalidCurrencyRate(); + return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / rate; } +error InvalidCurrencyRate();contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
45-54: Initializer switch: add input validation and confirm base initializer semantics
- Validate
_owner,_core,_poh,_wNativeto prevent misconfiguration.- Confirm
__DisputeKitClassicBase_initializeuses the correct initializer guard under the new pattern.) external initializer { - __DisputeKitClassicBase_initialize(_owner, _core, _wNative, _jumpDisputeKitID); + if (_owner == address(0) || address(_core) == address(0) || address(_poh) == address(0) || _wNative == address(0)) revert InvalidAddress(); + __DisputeKitClassicBase_initialize(_owner, _core, _wNative, _jumpDisputeKitID); poh = _poh; singleDrawPerJuror = true; } +error InvalidAddress();contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
119-135: Guard against empty _voteIDs to avoid OOB revert.
Accessing _voteIDs[0] will revert on empty arrays; add an explicit check for clearer errors.Apply:
function castVoteShutter( @@ - ) external { + ) external { + if (_voteIDs.length == 0) revert EmptyVoteIDs(); Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; address juror = dispute.rounds[dispute.rounds.length - 1].votes[_voteIDs[0]].account; @@ callerIsJuror = false; }Also add:
@@ error EmptyRecoveryCommit(); +error EmptyVoteIDs();contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
440-444: Guard against zero exchange rate to avoid division by zero.
convertEthToTokenAmount divides by rateInEth without a non-zero guarantee.Minimal checks:
function changeCurrencyRates(IERC20 _feeToken, uint64 _rateInEth, uint8 _rateDecimals) external onlyByOwner { + if (_rateInEth == 0) revert InvalidCurrencyRate(); currencyRates[_feeToken].rateInEth = _rateInEth; currencyRates[_feeToken].rateDecimals = _rateDecimals; emit NewCurrencyRate(_feeToken, _rateInEth, _rateDecimals); } @@ function convertEthToTokenAmount(IERC20 _toToken, uint256 _amountInEth) public view returns (uint256) { - return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / currencyRates[_toToken].rateInEth; + uint64 rate = currencyRates[_toToken].rateInEth; + if (rate == 0) revert InvalidCurrencyRate(); + return (_amountInEth * 10 ** currencyRates[_toToken].rateDecimals) / rate; } @@ error TransferFailed(); + error InvalidCurrencyRate();If you prefer to keep this PR scoped, I can open a follow-up issue/PR with these guards.
Also applies to: 1062-1064, 1172-1212
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
80-87: Initializer-only: confirm configurability of jumpDisputeKitID post-deploy.
Reinitializer was removed; jumpDisputeKitID can no longer be adjusted after init. If operationally acceptable, fine; otherwise add an owner-only setter.Do you still need to reconfigure jumpDisputeKitID in production? If yes, I can propose a minimal owner-only setter in a follow-up PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
contracts/deploy/upgrade-all.ts(1 hunks)contracts/src/arbitration/DisputeTemplateRegistry.sol(2 hunks)contracts/src/arbitration/KlerosCore.sol(2 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(1 hunks)contracts/src/arbitration/PolicyRegistry.sol(2 hunks)contracts/src/arbitration/SortitionModule.sol(2 hunks)contracts/src/arbitration/SortitionModuleNeo.sol(1 hunks)contracts/src/arbitration/devtools/KlerosCoreRuler.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol(2 hunks)contracts/src/arbitration/evidence/EvidenceModule.sol(2 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(2 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(2 hunks)contracts/src/gateway/ForeignGateway.sol(1 hunks)contracts/src/gateway/HomeGateway.sol(1 hunks)contracts/src/proxy/Initializable.sol(1 hunks)contracts/src/proxy/mock/by-inheritance/UpgradedByInheritance.sol(2 hunks)contracts/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T19:34:58.018Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#2107
File: contracts/src/arbitration/university/KlerosCoreUniversity.sol:1083-1092
Timestamp: 2025-09-03T19:34:58.018Z
Learning: KlerosCoreUniversity and SortitionModuleUniversity do not have phases, unlike KlerosCoreBase and SortitionModuleBase. Therefore, validateStake in the University contracts will never return StakingResult.Delayed, only Successful or other failure states.
Applied to files:
contracts/src/arbitration/university/SortitionModuleUniversity.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). (11)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- 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: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (29)
contracts/src/arbitration/KlerosCoreNeo.sol (1)
12-12: Version bump to 2.0.0 — LGTM.contracts/src/arbitration/SortitionModuleNeo.sol (1)
10-10: Version bump to 2.0.0 — LGTM.contracts/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol (1)
26-29: Switching toinitializerchanges upgrade behavior—ensure fresh proxies.Calling
initializeron an already-initialized proxy will revert. If tests/scripts previously relied onreinitializer(1), confirm they deploy fresh proxies or reset state accordingly.contracts/src/arbitration/PolicyRegistry.sol (1)
10-10: Version bump to 2.0.0 looks good.
Consistent with the PR objective to reset versions.contracts/src/arbitration/evidence/EvidenceModule.sol (1)
12-12: Version bump to 2.0.0 acknowledged.
Matches the migration plan.contracts/src/proxy/mock/by-inheritance/UpgradedByInheritance.sol (1)
23-26: Switch to initializer() for V1 is fine.
Base init once, additional stages via reinitializer in descendants.contracts/src/gateway/ForeignGateway.sol (2)
13-13: Version string consistencyOther modules in this PR bump
versionto "2.0.0". If intentional to keep gateways at "0.8.0", ignore; otherwise align.
79-90: Confirmed no scripted initialize/reinitialize calls
Searches across all deploy/upgrade scripts found no invocations of ForeignGateway.initialize or any reinitializer variants—no upgrade scripts will attempt to rerun initialize on existing proxies.contracts/src/arbitration/DisputeTemplateRegistry.sol (1)
11-11: Version bump to 2.0.0: LGTMChange is straightforward and consistent with the migration theme.
contracts/src/gateway/HomeGateway.sol (2)
17-17: Version string consistencyConsider bumping to "2.0.0" to match other modules, unless intentionally pinned.
68-82: Initialize calls absent in deploy scripts Verified no references to initialize(), initialize2(), or reinitialize() on HomeGateway across deployment/migration scripts.contracts/src/arbitration/devtools/KlerosCoreRuler.sol (1)
16-16: Version bump to 2.0.0: LGTMNo issues spotted.
contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol (1)
21-21: Version bump to 2.0.0: LGTMConsistent with the migration.
contracts/src/arbitration/university/SortitionModuleUniversity.sol (2)
15-15: Version bump to 2.0.0 — LGTM.
Consistent with the migration objective.
90-93: Verify proxy initialization and upgrade scripts
- No
reinitializer(1)remains incontracts/src(onlyreinitializer(2)in the Neo modules).- No deploy/upgrade scripts found under
contracts/deployor top-levelscripts; ensure your deployment tooling resets proxies and omits anyreinitializer(1)calls.contracts/src/arbitration/SortitionModule.sol (2)
10-10: Version bump to 2.0.0 — LGTM.
Aligned with other arbitration contracts.
27-35: Initializer-only path: confirm deploy/upgrade code no longer invokes initialize4/reinitialize. Repo scan: no call-sites invoking initialize4()/reinitialize() were found in deploy scripts, but initialize4() still exists as a NOP in sources (src/arbitration/SortitionModule.sol, src/arbitration/SortitionModuleNeo.sol) and appears in compiled deployment ABIs under contracts/deployments/*. Verify/update any upgrade scripts, encoded initializer payloads (upgradeToAndCall data), and CI/deployment pipelines so they call the new initialize(...) initializer (or appropriate reinitializer version) instead of initialize4/reinitialize.contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (2)
14-14: Version bump to 2.0.0 — LGTM.
Matches the migration plan.
30-37: Initializer-only: ensurejumpDisputeKitIDis immutable.
Search incontracts/srcfound noreinitializecalls anddeploy/scriptsdirs are absent—manually confirm no off-repo scripts, tests, or governance processes require post-deploy updates tojumpDisputeKitID.contracts/src/arbitration/KlerosCore.sol (2)
11-11: Version bump to 2.0.0 — LGTM.
34-60: Verify removal ofreinitializeand ensure_wNativeis always passed
Double-check that no deployment scripts or tooling still callKlerosCore.reinitialize(address)and that everyinitialize(...)invocation includes the correct_wNativeargument.contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (3)
15-15: Version bump to 2.0.0 — LGTM.
64-71: Initializer-only path confirmed safe: No.reinitialize(calls found anywhere in the repository.
3-3: No changes needed – Hardhat and Foundry are configured with solc 0.8.30, satisfying the ≥ 0.8.28 requirement.contracts/src/arbitration/university/KlerosCoreUniversity.sol (2)
18-18: Version bump to 2.0.0 — LGTM.
200-253: Incorrect — KlerosCoreUniversity already uses reinitializer(1).
initialize(...) is declared external reinitializer(1) in contracts/src/arbitration/university/KlerosCoreUniversity.sol, so the initializer-vs-reinitializer migration concern in the original comment is incorrect.Likely an incorrect or invalid review comment.
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (2)
30-30: Version bump to 2.0.0 — LGTM.
47-53: Switch to initializer — verify deploy/upgrade flow.
DisputeKitGated now uses initializer (no reinitializer). Ensure upgrades never call initialize() on an already-initialized proxy — init only at proxy construction or via a reinitializer/migration function invoked during upgrade. I found no direct occurrences of upgrade scripts calling initialize() for DisputeKitGated, but double-check deployment/upgrade scripts and CI (e.g. contracts/scripts/, deployments/) — start with contracts/scripts/utils/contracts.ts and the proxy init in contracts/test/foundry/KlerosCore_TestBase.sol.contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
31-31: Version bump to 2.0.0 — LGTM.
| await upgrade(disputeTemplateRegistry, "reinitialize", []); | ||
| await upgrade(evidence, "reinitialize", []); | ||
| await upgrade(core, "reinitialize", [wETH.address]); | ||
| await upgrade(policyRegistry, "initialize2", []); | ||
| await upgrade(sortition, "initialize4", []); | ||
| await upgrade(policyRegistry, "reinitialize", []); | ||
| await upgrade(sortition, "reinitialize", []); | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Initializer calls may not match target contract ABIs.
You changed four calls to initializer: "reinitialize" (some with empty args). Ensure each target contract actually exposes function reinitialize(...) external with matching arity; several modules in this PR migrated to single-use initializer, not reinitializer functions. Otherwise upgrades will revert at upgradeToAndCall.
Run this audit to confirm ABI/usage alignment:
If any target lacks a matching reinitialize, either:
- Add a minimal
reinitializewrapper (preferred for migration), or - Adjust
initializerandargshere per contract to invoke the actual available entry point.
🏁 Script executed:
#!/bin/bash
# Contracts the script touches
targets=("DisputeTemplateRegistry" "EvidenceModule" "PolicyRegistry" "SortitionModule" "KlerosCore" "KlerosCoreNeo" "SortitionModuleNeo")
echo "== Searching for reinitialize entry points =="
for t in "${targets[@]}"; do
fd -t f -a "contracts/src/**/${t}.sol" | while read -r f; do
echo "-- $f"
rg -nP 'function\s+reinitialize\s*\(' -C2 "$f" || echo " (no reinitialize found)"
rg -nP 'function\s+initialize\s*\(' -C1 "$f" || true
done
done
echo "== Show how getContractNamesFromNetwork maps names per network =="
fd -a 'contracts.ts|contracts.tsx|contracts.js|contracts.mjs' scripts -t f 2>/dev/null
rg -n 'getContractNamesFromNetwork|getContractNames' -C3 scripts || trueLength of output: 1171
🏁 Script executed:
#!/bin/bash
# Find all Solidity files under contracts/ and search for reinitialize and initialize functions
echo "== Solidity files under contracts/ =="
find contracts -type f -name '*.sol' | sed 's/^/-- /'
echo
echo "== Search each for reinitialize and initialize entry points =="
for f in $(find contracts -type f -name '*.sol'); do
echo "-- $f"
rg -nP 'function\s+reinitialize\s*\(' -C2 "$f" || echo " (no reinitialize found)"
rg -nP 'function\s+initialize\s*\(' -C2 "$f" || true
doneLength of output: 49090
Replace reinitialize calls with actual initializers and correct args
At contracts/deploy/upgrade-all.ts (lines 101–106), the script invokes reinitialize, but none of the target implementations define such a function. Either add lightweight reinitialize wrappers or, more directly, call their existing initialize(...) entry points with the proper parameters:
- DisputeTemplateRegistry, EvidenceModule, PolicyRegistry:
initialize(owner) - SortitionModule:
initialize(owner, core, maxDrawingTime, rng) - KlerosCore:
initialize(owner, guardian, sortitionModuleAddress, wETH.address)
🤖 Prompt for AI Agents
In contracts/deploy/upgrade-all.ts around lines 101 to 106, the script calls a
non-existent reinitialize(...) on several upgraded implementations; replace
those calls with the actual initialize(...) entrypoints and correct parameters:
call DisputeTemplateRegistry.initialize(owner),
EvidenceModule.initialize(owner), PolicyRegistry.initialize(owner),
SortitionModule.initialize(owner, core.address, maxDrawingTime, rng.address) and
KlerosCore.initialize(owner, guardian, sortition.address, wETH.address). Ensure
the variables used (owner, guardian, core/sortition addresses, maxDrawingTime,
rng) are in scope and pass the addresses where the initialize signatures expect
them.
| function initialize(address _owner) external initializer { | ||
| owner = _owner; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add zero-address guard in initialize().
Prevents locking upgrades/ownership if miscalled.
function initialize(address _owner) external initializer {
- owner = _owner;
+ if (_owner == address(0)) revert ZeroAddress();
+ owner = _owner;
}And declare the error:
error OwnerOnly();
+error ZeroAddress();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function initialize(address _owner) external initializer { | |
| owner = _owner; | |
| } | |
| // --- around existing error declarations --- | |
| error OwnerOnly(); | |
| error ZeroAddress(); | |
| // --- in contracts/src/arbitration/evidence/EvidenceModule.sol --- | |
| function initialize(address _owner) external initializer { | |
| if (_owner == address(0)) revert ZeroAddress(); | |
| owner = _owner; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/evidence/EvidenceModule.sol around lines 40-42,
initialize(address _owner) currently sets owner directly; add a zero-address
guard and declare a custom error. Declare an error like InvalidZeroAddress()
near the top of the contract (or with other custom errors), then in initialize()
check that _owner != address(0) and revert with InvalidZeroAddress() if it is;
finally proceed to set owner = _owner. This prevents accidental locking and
follows project error conventions.
| function initialize(address _owner) external initializer { | ||
| owner = _owner; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard initialize() against zero owner and confirm initializer-vs-reinitializer migration plan.
- Using initializer blocks reuse on already-initialized proxies; ensure migration uses fresh proxies or a dedicated reinitializer for upgrades.
- Also guard against accidentally bricking the proxy with owner = address(0).
Apply:
function initialize(address _owner) external initializer {
- owner = _owner;
+ if (_owner == address(0)) revert ZeroAddress();
+ owner = _owner;
}Add the error:
error OwnerOnly();
+error ZeroAddress();Optional: also validate in changeOwner:
function changeOwner(address _owner) external onlyByOwner {
- owner = _owner;
+ if (_owner == address(0)) revert ZeroAddress();
+ owner = _owner;
}Run to sanity-check deploy/migration scripts:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Any remaining initialize2/3/4 calls in deploy scripts?"
rg -n -C2 -g 'contracts/deploy/**' 'initialize[234]\s*\(' || true
echo "Places calling initialize(...) in deploy scripts:"
rg -n -C2 -g 'contracts/deploy/**' '\.initialize\s*\(' || true
echo "Contracts still using reinitializer():"
rg -n --type=sol 'reinitializer\s*\(' || trueLength of output: 540
Guard initialize and changeOwner against zero address & confirm initializer vs reinitializer migration
- In initialize():
function initialize(address _owner) external initializer { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; } - Define the new error:
error OwnerOnly(); +error ZeroAddress(); - Also in changeOwner():
function changeOwner(address _owner) external onlyByOwner { - owner = _owner; + if (_owner == address(0)) revert ZeroAddress(); + owner = _owner; } - Confirm your migration/deployment strategy—no
reinitializerusage was detected in contracts/deploy/**, so ensure you either deploy fresh proxies or add dedicated reinitializer functions for future upgrades.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function initialize(address _owner) external initializer { | |
| owner = _owner; | |
| } | |
| // --- add this alongside your other custom errors --- | |
| error OwnerOnly(); | |
| error ZeroAddress(); | |
| // ... | |
| function initialize(address _owner) external initializer { | |
| if (_owner == address(0)) revert ZeroAddress(); | |
| owner = _owner; | |
| } | |
| // ... | |
| function changeOwner(address _owner) external onlyByOwner { | |
| if (_owner == address(0)) revert ZeroAddress(); | |
| owner = _owner; | |
| } |
🤖 Prompt for AI Agents
In contracts/src/arbitration/PolicyRegistry.sol around lines 50-52, add explicit
checks to prevent setting the owner to the zero address in both initialize() and
changeOwner(): declare a custom error (e.g., ZeroAddress()) at the top of the
contract and revert with that error when _owner or newOwner == address(0);
ensure changeOwner also enforces appropriate access control (onlyOwner) if not
already enforced; finally, validate your deployment/migration approach: either
deploy fresh proxies that call initialize() once or add and document a
reinitializer(uint256) function for future upgrades (don’t rely on initializer
when planning reinitializer-based upgrades).
PR-Codex overview
This PR focuses on updating the initialization methods in various smart contracts from
reinitializertoinitializer, alongside version updates to2.0.0. This change enhances contract upgradeability and improves the initialization process.Detailed summary
reinitializer(1)toinitializerin multiple contracts.2.0.0in several contracts.initialize2()andinitialize4()functions in some contracts.Summary by CodeRabbit