Skip to content

Commit 13c74c6

Browse files
0xChinclaude
0xChin
and
claude
authored
fix: staking audit fixes (ethereum-optimism#19449)
* docs(staking): document pause bypass behavior in setAllowedStaker * fix(staking): only reset lastUpdate on full unstake in _decreasePeData 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> * feat(staking): implement two-step ownership transfer 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> * docs(staking): document lastUpdate reset trust assumption in setAllowedStaker 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> * chore: undo version change * chore: upgrade natspec * chore: update event emission order * fix: wrong event args * chore: add interface comments * fix: pre-pr * refactor: make ownable private functions public * fix: pre-pr * feat(ownable): reset pending owner on zeroaddress ownership transfer * refactor: inherit OZ's ownable and use helper contract for mapping storage slot * fix: ci --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent a8fa09e commit 13c74c6

File tree

10 files changed

+329
-91
lines changed

10 files changed

+329
-91
lines changed

‎packages/contracts-bedrock/interfaces/periphery/staking/IPolicyEngineStaking.sol‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,14 @@ interface IPolicyEngineStaking is ISemver {
3434
/// @notice Emitted when ownership is transferred.
3535
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
3636

37-
/// @notice Thrown when the caller is not the owner.
38-
error PolicyEngineStaking_OnlyOwner();
37+
/// @notice Emitted when a new owner is nominated via `transferOwnership`.
38+
event OwnershipTransferStarted(address indexed previousOwner, address indexed newOwner);
39+
40+
/// @notice Thrown when the caller is not authorized (OZ Ownable).
41+
error OwnableUnauthorizedAccount(address account);
42+
43+
/// @notice Thrown when an invalid owner is provided (OZ Ownable).
44+
error OwnableInvalidOwner(address owner);
3945

4046
/// @notice Thrown when the staking is paused.
4147
error PolicyEngineStaking_Paused();
@@ -69,10 +75,24 @@ interface IPolicyEngineStaking is ISemver {
6975
/// @notice Returns the contract owner.
7076
function owner() external view returns (address);
7177

72-
/// @notice Transfers ownership of the contract to a new account. Only callable by owner.
73-
/// @param _newOwner The address of the new owner.
78+
/// @notice Returns the pending owner nominated via `transferOwnership`.
79+
function pendingOwner() external view returns (address);
80+
81+
/// @notice Starts the ownership transfer of the contract to a new account. Replaces the
82+
/// pending transfer if there is one. The nominated address must call
83+
/// `acceptOwnership` to finalize the transfer (two-step pattern). Only callable
84+
/// by owner.
85+
///
86+
/// @param _newOwner The address of the nominated owner.
7487
function transferOwnership(address _newOwner) external;
7588

89+
/// @notice Accepts ownership after being nominated via `transferOwnership`.
90+
/// Only callable by the pending owner.
91+
function acceptOwnership() external;
92+
93+
/// @notice Renounces ownership of the contract. Only callable by owner.
94+
function renounceOwnership() external;
95+
7696
/// @notice Returns whether the contract is paused.
7797
function paused() external view returns (bool);
7898

@@ -114,6 +134,7 @@ interface IPolicyEngineStaking is ISemver {
114134
/// @notice Sets whether a staker can set the caller as beneficiary. When disallowing,
115135
/// if the staker's current beneficiary is the caller, their stake attribution is
116136
/// moved back to the staker (beneficiary reset to self).
137+
/// This function is callable even when the contract is paused.
117138
///
118139
/// @param _staker The staker address.
119140
/// @param _allowed Whether the staker is allowed to set the caller as beneficiary.

‎packages/contracts-bedrock/interfaces/universal/IOwnable.sol‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,12 @@ pragma solidity ^0.8.0;
66
interface IOwnable {
77
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
88

9+
/// @dev The caller account is not authorized to perform an operation.
10+
error OwnableUnauthorizedAccount(address account);
11+
12+
/// @dev The owner is not a valid owner account. (eg. `address(0)`)
13+
error OwnableInvalidOwner(address owner);
14+
915
function owner() external view returns (address);
1016
function renounceOwnership() external;
1117
function transferOwnership(address newOwner) external; // nosemgrep

‎packages/contracts-bedrock/scripts/checks/interfaces/main.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,9 @@ var excludeContracts = []string{
3333
// Constructor inheritance differences
3434
"IL2ProxyAdmin",
3535

36+
// OZ v4/v5 Ownable mismatch: IOwnable has v5 errors, AddressManager uses v4 Ownable
37+
"IAddressManager",
38+
3639
// TODO: Interfaces that need to be fixed
3740
"IInitializable", "IOptimismMintableERC20", "ILegacyMintableERC20",
3841
"KontrolCheatsBase", "IResolvedDelegateProxy",
@@ -41,7 +44,7 @@ var excludeContracts = []string{
4144
// excludeSourceContracts is a list of contracts that are allowed to not have interfaces
4245
var excludeSourceContracts = []string{
4346
// Base contracts with no external functions
44-
"CrossDomainOwnable", "CrossDomainOwnable2", "CrossDomainOwnable3", "CrossDomainMessengerLegacySpacer0", "CrossDomainMessengerLegacySpacer1",
47+
"CrossDomainOwnable", "CrossDomainOwnable2", "CrossDomainOwnable3", "CrossDomainMessengerLegacySpacer0", "CrossDomainMessengerLegacySpacer1", "PolicyEngineStakingMapping",
4548

4649
// Helper contracts
4750
"SafeSend", "EventLogger", "StorageSetter", "DisputeMonitorHelper", "GameHelper",

‎packages/contracts-bedrock/snapshots/abi/PolicyEngineStaking.json‎

Lines changed: 69 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,13 @@
2828
"stateMutability": "view",
2929
"type": "function"
3030
},
31+
{
32+
"inputs": [],
33+
"name": "acceptOwnership",
34+
"outputs": [],
35+
"stateMutability": "nonpayable",
36+
"type": "function"
37+
},
3138
{
3239
"inputs": [
3340
{
@@ -122,6 +129,26 @@
122129
"stateMutability": "view",
123130
"type": "function"
124131
},
132+
{
133+
"inputs": [],
134+
"name": "pendingOwner",
135+
"outputs": [
136+
{
137+
"internalType": "address",
138+
"name": "",
139+
"type": "address"
140+
}
141+
],
142+
"stateMutability": "view",
143+
"type": "function"
144+
},
145+
{
146+
"inputs": [],
147+
"name": "renounceOwnership",
148+
"outputs": [],
149+
"stateMutability": "nonpayable",
150+
"type": "function"
151+
},
125152
{
126153
"inputs": [
127154
{
@@ -217,7 +244,7 @@
217244
"inputs": [
218245
{
219246
"internalType": "address",
220-
"name": "_newOwner",
247+
"name": "newOwner",
221248
"type": "address"
222249
}
223250
],
@@ -341,6 +368,25 @@
341368
"name": "EffectiveStakeChanged",
342369
"type": "event"
343370
},
371+
{
372+
"anonymous": false,
373+
"inputs": [
374+
{
375+
"indexed": true,
376+
"internalType": "address",
377+
"name": "previousOwner",
378+
"type": "address"
379+
},
380+
{
381+
"indexed": true,
382+
"internalType": "address",
383+
"name": "newOwner",
384+
"type": "address"
385+
}
386+
],
387+
"name": "OwnershipTransferStarted",
388+
"type": "event"
389+
},
344390
{
345391
"anonymous": false,
346392
"inputs": [
@@ -411,23 +457,40 @@
411457
"type": "event"
412458
},
413459
{
414-
"inputs": [],
415-
"name": "PolicyEngineStaking_InsufficientStake",
460+
"inputs": [
461+
{
462+
"internalType": "address",
463+
"name": "owner",
464+
"type": "address"
465+
}
466+
],
467+
"name": "OwnableInvalidOwner",
468+
"type": "error"
469+
},
470+
{
471+
"inputs": [
472+
{
473+
"internalType": "address",
474+
"name": "account",
475+
"type": "address"
476+
}
477+
],
478+
"name": "OwnableUnauthorizedAccount",
416479
"type": "error"
417480
},
418481
{
419482
"inputs": [],
420-
"name": "PolicyEngineStaking_NoStake",
483+
"name": "PolicyEngineStaking_InsufficientStake",
421484
"type": "error"
422485
},
423486
{
424487
"inputs": [],
425-
"name": "PolicyEngineStaking_NotAllowedToSetBeneficiary",
488+
"name": "PolicyEngineStaking_NoStake",
426489
"type": "error"
427490
},
428491
{
429492
"inputs": [],
430-
"name": "PolicyEngineStaking_OnlyOwner",
493+
"name": "PolicyEngineStaking_NotAllowedToSetBeneficiary",
431494
"type": "error"
432495
},
433496
{

‎packages/contracts-bedrock/snapshots/abi/PolicyEngineStakingMapping.json‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
[
2+
{
3+
"inputs": [],
4+
"name": "PE_DATA_SLOT",
5+
"outputs": [
6+
{
7+
"internalType": "bytes32",
8+
"name": "",
9+
"type": "bytes32"
10+
}
11+
],
12+
"stateMutability": "view",
13+
"type": "function"
14+
},
15+
{
16+
"inputs": [
17+
{
18+
"internalType": "address",
19+
"name": "account",
20+
"type": "address"
21+
}
22+
],
23+
"name": "peData",
24+
"outputs": [
25+
{
26+
"internalType": "uint128",
27+
"name": "effectiveStake",
28+
"type": "uint128"
29+
},
30+
{
31+
"internalType": "uint128",
32+
"name": "lastUpdate",
33+
"type": "uint128"
34+
}
35+
],
36+
"stateMutability": "view",
37+
"type": "function"
38+
}
39+
]

‎packages/contracts-bedrock/snapshots/semver-lock.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -240,8 +240,8 @@
240240
"sourceCodeHash": "0x955bd0c9b47e43219865e4e92abf28d916c96de20cbdf2f94c8ab14d02083759"
241241
},
242242
"src/periphery/staking/PolicyEngineStaking.sol:PolicyEngineStaking": {
243-
"initCodeHash": "0xc0c04b0dddbf7831bf5abfccc6a569d92f9b7ab0ec53278f6d1cf7113041d59d",
244-
"sourceCodeHash": "0x998ddc9f24e3c85b1d588c263838261442edaad1dde5424fd55c2d4e1243592a"
243+
"initCodeHash": "0x0dd679c6e955a4bf4af7d98da8c42ac7fd48c5c28af4c61eaeddb782690d6223",
244+
"sourceCodeHash": "0x24fe77ae2b0dacad1f61f6dd65026ce3a34df7c4d0331eadc661b2e062d6bb54"
245245
},
246246
"src/safe/DeputyPauseModule.sol:DeputyPauseModule": {
247247
"initCodeHash": "0x18422b48c4901ed6fd9338d76d3c5aecfff9a7add34b05c6e21c23d0011ed6bf",

‎packages/contracts-bedrock/snapshots/storageLayout/PolicyEngineStaking.json‎

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,34 +4,41 @@
44
"label": "peData",
55
"offset": 0,
66
"slot": "0",
7-
"type": "mapping(address => struct PolicyEngineStaking.PEData)"
7+
"type": "mapping(address => struct PolicyEngineStakingMapping.PEData)"
8+
},
9+
{
10+
"bytes": "20",
11+
"label": "_owner",
12+
"offset": 0,
13+
"slot": "1",
14+
"type": "address"
15+
},
16+
{
17+
"bytes": "20",
18+
"label": "_pendingOwner",
19+
"offset": 0,
20+
"slot": "2",
21+
"type": "address"
822
},
923
{
1024
"bytes": "32",
1125
"label": "allowlist",
1226
"offset": 0,
13-
"slot": "1",
27+
"slot": "3",
1428
"type": "mapping(address => mapping(address => bool))"
1529
},
1630
{
1731
"bytes": "32",
1832
"label": "stakingData",
1933
"offset": 0,
20-
"slot": "2",
34+
"slot": "4",
2135
"type": "mapping(address => struct PolicyEngineStaking.StakedData)"
2236
},
2337
{
2438
"bytes": "1",
2539
"label": "paused",
2640
"offset": 0,
27-
"slot": "3",
41+
"slot": "5",
2842
"type": "bool"
29-
},
30-
{
31-
"bytes": "20",
32-
"label": "_owner",
33-
"offset": 1,
34-
"slot": "3",
35-
"type": "address"
3643
}
3744
]

‎packages/contracts-bedrock/snapshots/storageLayout/PolicyEngineStakingMapping.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
[
2+
{
3+
"bytes": "32",
4+
"label": "peData",
5+
"offset": 0,
6+
"slot": "0",
7+
"type": "mapping(address => struct PolicyEngineStakingMapping.PEData)"
8+
}
9+
]

0 commit comments

Comments
 (0)