[Provers L01] Change Ownable to Ownable2Step in StateProverPointer#88
[Provers L01] Change Ownable to Ownable2Step in StateProverPointer#88
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated numeric counts in a snapshot JSON and changed StateProverPointer to use OpenZeppelin's Ownable2Step; added a test covering two-step ownership transfer. Changes
Sequence Diagram(s)(No sequence diagrams generated.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/contracts/StateProverPointer.sol (2)
17-23:⚠️ Potential issue | 🟠 Major
renounceOwnership()is not overridden — single-step permanent ownership loss is still possible.
Ownable2StepinheritsrenounceOwnership()fromOwnableunchanged. Any call torenounceOwnership()immediately and irrevocably sets the owner toaddress(0), skipping the two-step guard entirely. If the owner is a multisig or governance contract that accidentally includes arenounceOwnership()call in a batch, the pointer is permanently bricked — no furthersetImplementationAddressis ever possible, forcing the expensive downstream migration the PR is explicitly trying to avoid.There is still no two-step verification for renouncing ownership, i.e. transferring ownership to the zero address. If there is no need to renounce ownership, it is safer to override
renounceOwnership()to revert when called.🔒 Proposed fix — disable renounceOwnership
constructor(address _initialOwner) Ownable(_initialOwner) {} + /// `@notice` Renouncing ownership is disabled; use transferOwnership/acceptOwnership instead. + function renounceOwnership() public override onlyOwner { + revert("renounceOwnership disabled"); + } + /// `@notice` Returns the address of the current StateProver implementation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/StateProverPointer.sol` around lines 17 - 23, The contract StateProverPointer currently inherits renounceOwnership() from Ownable via Ownable2Step which allows immediate permanent ownership loss; override renounceOwnership() in StateProverPointer to revert instead (e.g., revert("renounceOwnership disabled")) so the two‑step transfer semantics cannot be bypassed, and ensure this prevents accidental bricking that would block calls like setImplementationAddress; reference renounceOwnership(), StateProverPointer, Ownable2Step and setImplementationAddress when locating where to add the override.
56-62:⚠️ Potential issue | 🟡 MinorUnguarded high-level
version()call on the current implementation can brick upgrades.Line 58 calls
IStateProver(currentImplementationAddress).version()directly via a high-level call that will revert and propagate if the current implementation'sversion()reverts (e.g., malicious or broken proxy). By contrast, the new-address path at lines 48–52 correctly uses a protectedstaticcall. If the current implementation ever becomes non-callable,setImplementationAddressbecomes permanently blocked.🛡️ Proposed fix — use try/catch for the old version lookup
- uint256 oldVersion = IStateProver(currentImplementationAddress).version(); - if (newVersion <= oldVersion) { - revert NonIncreasingVersion(newVersion, oldVersion); + try IStateProver(currentImplementationAddress).version() returns (uint256 oldVersion) { + if (newVersion <= oldVersion) { + revert NonIncreasingVersion(newVersion, oldVersion); + } + } catch { + revert InvalidImplementationAddress(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/StateProverPointer.sol` around lines 56 - 62, The direct high-level call IStateProver(currentImplementationAddress).version() inside the upgrade path can revert and permanently block setImplementationAddress; replace it with a guarded try/catch around the external call (or perform a low-level staticcall) so a failing/ malicious old implementation does not prevent upgrades. Concretely, in the function that reads implementationAddress() (e.g., setImplementationAddress), wrap the version lookup in: try IStateProver(currentImplementationAddress).version() returns (uint256 oldVersion) { if (newVersion <= oldVersion) revert NonIncreasingVersion(newVersion, oldVersion); } catch { /* ignore failure and allow upgrade to proceed */ } so failures in the old contract don’t propagate and block upgrades.
🧹 Nitpick comments (2)
src/contracts/StateProverPointer.sol (2)
43-66: Consider emitting an event when the implementation is successfully updated.
setImplementationAddresswrites critical state (new implementation address + code hash) but emits no event. Without an event, off-chain monitors, indexers, and cross-chain verifiers have no log-based signal when the pointer changes — they must resort to polling storage.📡 Suggested event
+ event ImplementationAddressUpdated( + address indexed previousImplementation, + address indexed newImplementation, + uint256 newVersion + ); function setImplementationAddress(address _newImplementationAddress) external onlyOwner { ... _implementationAddress = _newImplementationAddress; _setCodeHash(_newImplementationAddress.codehash); + emit ImplementationAddressUpdated(currentImplementationAddress, _newImplementationAddress, newVersion); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/StateProverPointer.sol` around lines 43 - 66, Add an event declaration (e.g., ImplementationUpdated(address indexed implementation, uint256 version, bytes32 codeHash)) to the contract and emit it at the end of setImplementationAddress after updating _implementationAddress and _setCodeHash; include the new implementation address, the decoded newVersion, and the new code hash (_newImplementationAddress.codehash) so off-chain consumers can detect pointer changes via logs; ensure the event name and field names match your project's naming conventions and are declared at contract scope.
2-2: Pragma pinned to0.8.30; latest release is0.8.34.Solidity
0.8.34was released on February 18, 2026 as a bugfix release that patches an important bug of high severity affecting clearing of storage and transient storage variables in the IR pipeline. The bug affects compiler versions0.8.28through0.8.33and only manifests when compiling with the IR pipeline; since--via-iris not enabled by default, projects that have not explicitly opted into it are not affected. This contract does not use transient storage withdelete, so it is not affected by the bug. Nonetheless, consider bumping the pragma to0.8.34(or a minimum range like^0.8.30) to stay on a supported, patched compiler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/StateProverPointer.sol` at line 2, The pragma is pinned to 0.8.30; update the solidity version specifier in StateProverPointer (the pragma line) to a patched compiler release or a safe range—either bump it to 0.8.34 or use a caret range like ^0.8.30—to ensure you compile with a supported bugfixed compiler; modify the pragma statement accordingly in StateProverPointer.sol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/contracts/StateProverPointer.sol`:
- Around line 17-23: The contract StateProverPointer currently inherits
renounceOwnership() from Ownable via Ownable2Step which allows immediate
permanent ownership loss; override renounceOwnership() in StateProverPointer to
revert instead (e.g., revert("renounceOwnership disabled")) so the two‑step
transfer semantics cannot be bypassed, and ensure this prevents accidental
bricking that would block calls like setImplementationAddress; reference
renounceOwnership(), StateProverPointer, Ownable2Step and
setImplementationAddress when locating where to add the override.
- Around line 56-62: The direct high-level call
IStateProver(currentImplementationAddress).version() inside the upgrade path can
revert and permanently block setImplementationAddress; replace it with a guarded
try/catch around the external call (or perform a low-level staticcall) so a
failing/ malicious old implementation does not prevent upgrades. Concretely, in
the function that reads implementationAddress() (e.g.,
setImplementationAddress), wrap the version lookup in: try
IStateProver(currentImplementationAddress).version() returns (uint256
oldVersion) { if (newVersion <= oldVersion) revert
NonIncreasingVersion(newVersion, oldVersion); } catch { /* ignore failure and
allow upgrade to proceed */ } so failures in the old contract don’t propagate
and block upgrades.
---
Nitpick comments:
In `@src/contracts/StateProverPointer.sol`:
- Around line 43-66: Add an event declaration (e.g.,
ImplementationUpdated(address indexed implementation, uint256 version, bytes32
codeHash)) to the contract and emit it at the end of setImplementationAddress
after updating _implementationAddress and _setCodeHash; include the new
implementation address, the decoded newVersion, and the new code hash
(_newImplementationAddress.codehash) so off-chain consumers can detect pointer
changes via logs; ensure the event name and field names match your project's
naming conventions and are declared at contract scope.
- Line 2: The pragma is pinned to 0.8.30; update the solidity version specifier
in StateProverPointer (the pragma line) to a patched compiler release or a safe
range—either bump it to 0.8.34 or use a caret range like ^0.8.30—to ensure you
compile with a supported bugfixed compiler; modify the pragma statement
accordingly in StateProverPointer.sol.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snapshots/verifyBroadcastMessage.jsonsrc/contracts/StateProverPointer.soltest/StateProverPointer.t.sol
|
|
||
| import {StorageSlot} from "@openzeppelin/contracts/utils/StorageSlot.sol"; | ||
| import {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol"; | ||
| import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; |
There was a problem hiding this comment.
is this import still needed?
There was a problem hiding this comment.
in order to use Ownable2Steps we need to import both Ownable2Steps and Ownable, because the constructor takes the Ownable:
constructor(address _initialOwner) Ownable(_initialOwner) {}
3 participants
StateProverPointer is governed by a single owner that can update the active StateProver via setImplementationAddress. Since this address selection directly determines which implementation is trusted for proof verification, an accidental or incorrect ownership transfer can permanently remove the ability to patch or upgrade proof verification on a given chain.
The contract currently inherits OpenZeppelin Ownable, where transferOwnership(newOwner) immediately assigns ownership to newOwner without requiring any action from the recipient. In practice, governance transitions for public infrastructure often involve multisigs, DAOs, or staged operational handoffs across many chains; a single-step transfer increases the chance that an incorrect address (wrong chain deployment, misconfigured multisig, incorrect governance executor, etc.) receives ownership.
If ownership is transferred to an address that cannot operate as owner (or to an unintended address), the pointer can become effectively bricked:
The implementation can no longer be updated to fix bugs or respond to vulnerabilities.
Downstream integrators that depend on the pointer’s stability may be forced into coordinated migrations, which is operationally expensive and error-prone for widely-used infrastructure.
This PR updates the StateProverPointer to use
Ownable2Stepinstead ofOwnableSummary by CodeRabbit
New Features
Tests
Chores