fix: staking audit fixes by 0xChin · Pull Request #892 · defi-wonderland/optimism

0xChin · 2026-03-09T14:47:38Z

Fixes some findings from the Cantina's audit

Finding 1 - setAllowedStaker callable during pause (documented as intentional)
Finding 7 - lastUpdate unnecessarily reset on partial unstake (now only resets on full unstake)
Finding 8 - One-step transferOwnership risks permanent loss (now two-step with acceptOwnership)
Finding 10 - Beneficiary can reset staker's lastUpdate via allowlist removal (documented trust assumption)

Cantina audit finding #7: _decreasePeData unconditionally reset lastUpdate on every decrease, penalizing partial unstakers by resetting their staking weight. Now lastUpdate is only reset when effectiveStake reaches zero. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Cantina audit finding #8: transferOwnership now nominates a pending owner who must call acceptOwnership() to finalize the transfer, preventing irrevocable ownership loss from incorrect addresses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…edStaker Cantina audit finding #10: when a beneficiary removes a staker from their allowlist, the staker's lastUpdate is reset via _increasePeData, losing accumulated staking weight. Document this as an inherent trust assumption of the delegation model. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

coderabbitai · 2026-03-09T14:48:09Z

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Implements a two-step ownership transfer in PolicyEngineStaking: transferOwnership now nominates a pendingOwner and emits OwnershipTransferStarted; acceptOwnership finalizes the transfer, emits OwnershipTransferred, and clears pendingOwner. Adds pendingOwner() getter and PolicyEngineStaking_NotPendingOwner error. Introduces public owner and pendingOwner state variables (replacing the explicit owner() getter), updates storage layout and ABI, adjusts _decreasePeData to only update lastUpdate when effective stake becomes zero, and updates/extends tests to cover nomination/acceptance and lastUpdate behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'fix: staking audit fixes' is vague and generic, using non-descriptive language that does not convey the specific changes made in the changeset.	Consider a more specific title like 'fix: implement two-step ownership transfer and refine unstake logic' that highlights the main changes.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description clearly relates to the changeset, detailing four specific audit findings being addressed including two-step ownership transfer, lastUpdate logic, and documented trust assumptions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/staking-audit-fixes

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

cubic-dev-ai

No issues found across 3 files

cubic-dev-ai

1 issue found across 1 file (changes from recent commits).

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/contracts-bedrock/src/periphery/staking/PolicyEngineStaking.sol">

<violation number="1" location="packages/contracts-bedrock/src/periphery/staking/PolicyEngineStaking.sol:198">
P1: Custom agent: **Solidity Standards**

Bug: `_owner` is already overwritten to `msg.sender` before the event is emitted, so `OwnershipTransferred` will log `previousOwner == newOwner == msg.sender`, losing the actual previous owner. Cache the old owner before mutating state.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

cubic-dev-ai

0 issues found across 2 files (changes from recent commits).

coderabbitai

🧹 Nitpick comments (2)

packages/contracts-bedrock/src/periphery/staking/PolicyEngineStaking.sol (1)
183-201: Consider clearing _pendingOwner when nominating a new one.

If transferOwnership is called multiple times with different addresses, the previous pending owner is silently overwritten. This is likely intentional but worth noting. Additionally, if ownership needs to be canceled, there's no explicit mechanism (though calling transferOwnership(address(0)) would revert).

If cancellation is desired, consider allowing transferOwnership to accept address(0) to clear pending state, or add a separate cancelOwnershipTransfer() function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/contracts-bedrock/src/periphery/staking/PolicyEngineStaking.sol`
around lines 183 - 201, transferOwnership currently overwrites _pendingOwner
without a way to cancel a pending nomination; update the logic to allow clearing
a pending owner or add an explicit cancel function: either (A) permit
transferOwnership(address(0)) to clear _pendingOwner by removing the revert on
zero address and set _pendingOwner = address(0) with an emitted event (e.g.,
OwnershipTransferCancelled) so callers can cancel a nomination, or (B) add a new
onlyOwner function cancelOwnershipTransfer() that sets _pendingOwner =
address(0) and emits a cancellation event; ensure acceptOwnership still checks
_pendingOwner and behavior around emitting
OwnershipTransferStarted/OwnershipTransferred remains consistent.
packages/contracts-bedrock/test/periphery/staking/PolicyEngineStaking.t.sol (1)
23-31: Missing OwnershipTransferred event declaration.

OwnershipTransferStarted was added but OwnershipTransferred is used in test_acceptOwnership_succeeds (line 90) without being declared locally in the test init contract. While it compiles (inherited from interface), other events are declared locally for consistency.
Proposed fix
     event Paused();
     event Unpaused();
     event OwnershipTransferStarted(address indexed previousOwner, address indexed newOwner);
+    event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/contracts-bedrock/test/periphery/staking/PolicyEngineStaking.t.sol`
around lines 23 - 31, The test contract is missing a local declaration for the
OwnershipTransferred event used in test_acceptOwnership_succeeds; add a matching
event declaration (e.g., event OwnershipTransferred(address indexed
previousOwner, address indexed newOwner);) alongside the existing
OwnershipTransferStarted, BeneficiarySet, etc., so the test and local
declarations are consistent with the inherited interface and the
test_acceptOwnership_succeeds assertion.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/contracts-bedrock/src/periphery/staking/PolicyEngineStaking.sol`:
- Around line 183-201: transferOwnership currently overwrites _pendingOwner
without a way to cancel a pending nomination; update the logic to allow clearing
a pending owner or add an explicit cancel function: either (A) permit
transferOwnership(address(0)) to clear _pendingOwner by removing the revert on
zero address and set _pendingOwner = address(0) with an emitted event (e.g.,
OwnershipTransferCancelled) so callers can cancel a nomination, or (B) add a new
onlyOwner function cancelOwnershipTransfer() that sets _pendingOwner =
address(0) and emits a cancellation event; ensure acceptOwnership still checks
_pendingOwner and behavior around emitting
OwnershipTransferStarted/OwnershipTransferred remains consistent.

In `@packages/contracts-bedrock/test/periphery/staking/PolicyEngineStaking.t.sol`:
- Around line 23-31: The test contract is missing a local declaration for the
OwnershipTransferred event used in test_acceptOwnership_succeeds; add a matching
event declaration (e.g., event OwnershipTransferred(address indexed
previousOwner, address indexed newOwner);) alongside the existing
OwnershipTransferStarted, BeneficiarySet, etc., so the test and local
declarations are consistent with the inherited interface and the
test_acceptOwnership_succeeds assertion.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: defi-wonderland/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2faa8a23-1365-477e-bb8b-b5bc9eb139bd

📥 Commits

Reviewing files that changed from the base of the PR and between a4bfa26 and ccda33e.

📒 Files selected for processing (3)

packages/contracts-bedrock/interfaces/periphery/staking/IPolicyEngineStaking.sol
packages/contracts-bedrock/src/periphery/staking/PolicyEngineStaking.sol
packages/contracts-bedrock/test/periphery/staking/PolicyEngineStaking.t.sol

cubic-dev-ai

0 issues found across 4 files (changes from recent commits).

cubic-dev-ai

0 issues found across 1 file (changes from recent commits).

cubic-dev-ai

0 issues found across 2 files (changes from recent commits).

cubic-dev-ai

0 issues found across 4 files (changes from recent commits).

0xChin · 2026-03-09T16:23:48Z

Closing to be opened as external PR.

0xChin and others added 6 commits March 9, 2026 11:13

docs(staking): document pause bypass behavior in setAllowedStaker

2ab8b9f

chore: undo version change

c65305a

chore: upgrade natspec

9479d15

cubic-dev-ai bot approved these changes Mar 9, 2026

View reviewed changes

chore: update event emission order

0b84413

cubic-dev-ai bot reviewed Mar 9, 2026

View reviewed changes

Comment thread packages/contracts-bedrock/src/periphery/staking/PolicyEngineStaking.sol Outdated

0xChin added 2 commits March 9, 2026 12:05

fix: wrong event args

4ba124f

chore: add interface comments

ccda33e

cubic-dev-ai bot approved these changes Mar 9, 2026

View reviewed changes

fix: pre-pr

dad0c75

coderabbitai bot reviewed Mar 9, 2026

View reviewed changes

coderabbitai bot approved these changes Mar 9, 2026

View reviewed changes

cubic-dev-ai bot approved these changes Mar 9, 2026

View reviewed changes

0xChin added 2 commits March 9, 2026 12:54

refactor: make ownable private functions public

99cd3dd

fix: pre-pr

fb67d87

cubic-dev-ai bot approved these changes Mar 9, 2026

View reviewed changes

feat(ownable): reset pending owner on zeroaddress ownership transfer

e825114

cubic-dev-ai bot approved these changes Mar 9, 2026

View reviewed changes

0xChin closed this Mar 9, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: staking audit fixes#892

fix: staking audit fixes#892
0xChin wants to merge 13 commits intodevelopfrom
fix/staking-audit-fixes

0xChin commented Mar 9, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Mar 9, 2026 •

edited

Loading

Review failed

Walkthrough

Estimated code review effort

❌ Failed checks (1 inconclusive)

Uh oh!

cubic-dev-ai bot left a comment

Uh oh!

cubic-dev-ai bot left a comment

Uh oh!

Uh oh!

cubic-dev-ai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

cubic-dev-ai bot left a comment

Uh oh!

cubic-dev-ai bot left a comment

Uh oh!

cubic-dev-ai bot left a comment

Uh oh!

cubic-dev-ai bot left a comment

Uh oh!

0xChin commented Mar 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

0xChin commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Estimated code review effort

❌ Failed checks (1 inconclusive)

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

0xChin commented Mar 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

0xChin commented Mar 9, 2026 •

edited

Loading

coderabbitai bot commented Mar 9, 2026 •

edited

Loading