Skip to content

Commit 87f9d08

Browse files
jaybuidljaybuidl
committed
feat: upgrade proxies validation, natspec reformat for UUPSProxiable and UUPSProxy
1 parent b4d925a commit 87f9d08

20 files changed

+182
-201
lines changed

‎contracts/package.json

+8-6
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
"start-local": "hardhat node --tags Arbitration,HomeArbitrable --hostname 0.0.0.0",
2828
"deploy": "hardhat deploy",
2929
"deploy-local": "hardhat deploy --tags Arbitration,HomeArbitrable --network localhost",
30+
"validate-upgrades": "openzeppelin-upgrades-core validate --exclude 'src/proxy/mock/**/*.sol' --exclude 'src/test/**/*.sol' artifacts/build-info",
3031
"simulate": "hardhat simulate:all",
3132
"simulate-local": "hardhat simulate:all --network localhost",
3233
"viem:generate-devnet": "NODE_NO_WARNINGS=1 wagmi generate -c wagmi.config.devnet.ts",
@@ -69,6 +70,7 @@
6970
"@nomicfoundation/hardhat-chai-matchers": "^2.0.8",
7071
"@nomicfoundation/hardhat-ethers": "^3.0.8",
7172
"@nomiclabs/hardhat-solhint": "^4.0.1",
73+
"@openzeppelin/upgrades-core": "^1.41.0",
7274
"@typechain/ethers-v6": "^0.5.1",
7375
"@typechain/hardhat": "^9.1.0",
7476
"@types/chai": "^4.3.20",
@@ -80,23 +82,23 @@
8082
"dotenv": "^16.4.5",
8183
"eslint": "^9.15.0",
8284
"ethereumjs-util": "^7.1.5",
83-
"ethers": "^6.13.4",
85+
"ethers": "^6.13.5",
8486
"graphql": "^16.9.0",
8587
"graphql-request": "^7.1.2",
86-
"hardhat": "2.22.16",
88+
"hardhat": "2.22.18",
8789
"hardhat-contract-sizer": "^2.10.0",
8890
"hardhat-deploy": "^0.14.0",
8991
"hardhat-deploy-ethers": "^0.4.2",
90-
"hardhat-deploy-tenderly": "^0.2.0",
92+
"hardhat-deploy-tenderly": "^0.2.1",
9193
"hardhat-docgen": "^1.3.0",
92-
"hardhat-gas-reporter": "^2.2.1",
94+
"hardhat-gas-reporter": "^2.2.2",
9395
"hardhat-tracer": "^3.1.0",
9496
"hardhat-watcher": "^2.5.0",
9597
"node-fetch": "^3.3.2",
9698
"pino": "^8.21.0",
9799
"pino-pretty": "^10.3.1",
98100
"prettier": "^3.3.3",
99-
"prettier-plugin-solidity": "^1.4.1",
101+
"prettier-plugin-solidity": "^1.4.2",
100102
"shelljs": "^0.8.5",
101103
"solhint-plugin-prettier": "^0.1.0",
102104
"solidity-coverage": "^0.8.13",
@@ -107,7 +109,7 @@
107109
"dependencies": {
108110
"@chainlink/contracts": "^1.3.0",
109111
"@kleros/vea-contracts": "^0.4.0",
110-
"@openzeppelin/contracts": "^5.1.0",
112+
"@openzeppelin/contracts": "^5.2.0",
111113
"viem": "^2.21.48"
112114
}
113115
}

‎contracts/src/arbitration/DisputeTemplateRegistry.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ contract DisputeTemplateRegistry is IDisputeTemplateRegistry, UUPSProxiable, Ini
3333
// * Constructor * //
3434
// ************************************* //
3535

36-
/// @dev Constructor, initializing the implementation to reduce attack surface.
36+
/// @custom:oz-upgrades-unsafe-allow constructor
3737
constructor() {
3838
_disableInitializers();
3939
}

‎contracts/src/arbitration/KlerosCore.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ contract KlerosCore is KlerosCoreBase {
2020
// * Constructor * //
2121
// ************************************* //
2222

23-
/// @dev Constructor, initializing the implementation to reduce attack surface.
23+
/// @custom:oz-upgrades-unsafe-allow constructor
2424
constructor() {
2525
_disableInitializers();
2626
}

‎contracts/src/arbitration/KlerosCoreNeo.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ contract KlerosCoreNeo is KlerosCoreBase {
2828
// * Constructor * //
2929
// ************************************* //
3030

31-
/// @dev Constructor, initializing the implementation to reduce attack surface.
31+
/// @custom:oz-upgrades-unsafe-allow constructor
3232
constructor() {
3333
_disableInitializers();
3434
}

‎contracts/src/arbitration/PolicyRegistry.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ contract PolicyRegistry is UUPSProxiable, Initializable {
4040
// * Constructor * //
4141
// ************************************* //
4242

43-
/// @dev Constructor, initializing the implementation to reduce attack surface.
43+
/// @custom:oz-upgrades-unsafe-allow constructor
4444
constructor() {
4545
_disableInitializers();
4646
}

‎contracts/src/arbitration/SortitionModule.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ contract SortitionModule is SortitionModuleBase {
2121
// * Constructor * //
2222
// ************************************* //
2323

24-
/// @dev Constructor, initializing the implementation to reduce attack surface.
24+
/// @custom:oz-upgrades-unsafe-allow constructor
2525
constructor() {
2626
_disableInitializers();
2727
}

‎contracts/src/arbitration/SortitionModuleNeo.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ contract SortitionModuleNeo is SortitionModuleBase {
2929
// * Constructor * //
3030
// ************************************* //
3131

32-
/// @dev Constructor, initializing the implementation to reduce attack surface.
32+
/// @custom:oz-upgrades-unsafe-allow constructor
3333
constructor() {
3434
_disableInitializers();
3535
}

‎contracts/src/arbitration/devtools/KlerosCoreRuler.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ contract KlerosCoreRuler is IArbitratorV2, UUPSProxiable, Initializable {
166166
// * Constructor * //
167167
// ************************************* //
168168

169-
/// @dev Constructor, initializing the implementation to reduce attack surface.
169+
/// @custom:oz-upgrades-unsafe-allow constructor
170170
constructor() {
171171
_disableInitializers();
172172
}

‎contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ contract DisputeKitClassic is DisputeKitClassicBase {
2323
// * Constructor * //
2424
// ************************************* //
2525

26-
/// @dev Constructor, initializing the implementation to reduce attack surface.
26+
/// @custom:oz-upgrades-unsafe-allow constructor
2727
constructor() {
2828
_disableInitializers();
2929
}

‎contracts/src/arbitration/dispute-kits/DisputeKitGated.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ contract DisputeKitGated is DisputeKitClassicBase {
4646
// * Constructor * //
4747
// ************************************* //
4848

49-
/// @dev Constructor, initializing the implementation to reduce attack surface.
49+
/// @custom:oz-upgrades-unsafe-allow constructor
5050
constructor() {
5151
_disableInitializers();
5252
}

‎contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ contract DisputeKitSybilResistant is DisputeKitClassicBase {
3636
// * Constructor * //
3737
// ************************************* //
3838

39-
/// @dev Constructor, initializing the implementation to reduce attack surface.
39+
/// @custom:oz-upgrades-unsafe-allow constructor
4040
constructor() {
4141
_disableInitializers();
4242
}

‎contracts/src/arbitration/evidence/EvidenceModule.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ contract EvidenceModule is IEvidence, Initializable, UUPSProxiable {
3737
// * Constructor * //
3838
// ************************************* //
3939

40-
/// @dev Constructor, initializing the implementation to reduce attack surface.
40+
/// @custom:oz-upgrades-unsafe-allow constructor
4141
constructor() {
4242
_disableInitializers();
4343
}

‎contracts/src/arbitration/university/KlerosCoreUniversity.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ contract KlerosCoreUniversity is IArbitratorV2, UUPSProxiable, Initializable {
182182
// * Constructor * //
183183
// ************************************* //
184184

185-
/// @dev Constructor, initializing the implementation to reduce attack surface.
185+
/// @custom:oz-upgrades-unsafe-allow constructor
186186
constructor() {
187187
_disableInitializers();
188188
}

‎contracts/src/arbitration/university/SortitionModuleUniversity.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ contract SortitionModuleUniversity is ISortitionModuleUniversity, UUPSProxiable,
6868
// * Constructor * //
6969
// ************************************* //
7070

71-
/// @dev Constructor, initializing the implementation to reduce attack surface.
71+
/// @custom:oz-upgrades-unsafe-allow constructor
7272
constructor() {
7373
_disableInitializers();
7474
}

‎contracts/src/gateway/ForeignGateway.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ contract ForeignGateway is IForeignGateway, UUPSProxiable, Initializable {
7373
// * Constructor * //
7474
// ************************************* //
7575

76-
/// @dev Constructor, initializing the implementation to reduce attack surface.
76+
/// @custom:oz-upgrades-unsafe-allow constructor
7777
constructor() {
7878
_disableInitializers();
7979
}

‎contracts/src/gateway/HomeGateway.sol

+1-1
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ contract HomeGateway is IHomeGateway, UUPSProxiable, Initializable {
5959
// * Constructor * //
6060
// ************************************* //
6161

62-
/// @dev Constructor, initializing the implementation to reduce attack surface.
62+
/// @custom:oz-upgrades-unsafe-allow constructor
6363
constructor() {
6464
_disableInitializers();
6565
}

‎contracts/src/kleros-v1/kleros-liquid-xdai/xKlerosLiquidV2.sol

+1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import "../../gateway/interfaces/IForeignGateway.sol";
1616
/// @dev This contract is an adaption of Mainnet's KlerosLiquid (https://github.com/kleros/kleros/blob/69cfbfb2128c29f1625b3a99a3183540772fda08/contracts/kleros/KlerosLiquid.sol)
1717
/// for xDai chain. Notice that variables referring to ETH values in this contract, will hold the native token values of the chain on which xKlerosLiquid is deployed.
1818
/// When this contract gets deployed on xDai chain, ETH variables will hold xDai values.
19+
/// @custom:oz-upgrades-unsafe-allow external-library-linking
1920
contract xKlerosLiquidV2 is Initializable, ITokenController, IArbitratorV2 {
2021
// ************************************* //
2122
// * Enums / Structs * //

‎contracts/src/proxy/UUPSProxiable.sol

+46-72
Original file line numberDiff line numberDiff line change
@@ -1,56 +1,41 @@
11
//SPDX-License-Identifier: MIT
2-
// Adapted from <https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.9.0/contracts/proxy/utils/UUPSUpgradeable.sol>
3-
4-
/**
5-
* @authors: [@malatrax]
6-
* @reviewers: []
7-
* @auditors: []
8-
* @bounties: []
9-
* @deployments: []
10-
*/
2+
113
pragma solidity 0.8.24;
124

13-
/**
14-
* @title UUPS Proxiable
15-
* @author Simon Malatrait <[email protected]>
16-
* @dev This contract implements an upgradeability mechanism designed for UUPS proxies.
17-
* The functions included here can perform an upgrade of an UUPS Proxy, when this contract is set as the implementation behind such a proxy.
18-
*
19-
* IMPORTANT: A UUPS proxy requires its upgradeability functions to be in the implementation as opposed to the transparent proxy.
20-
* This means that if the proxy is upgraded to an implementation that does not support this interface, it will no longer be upgradeable.
21-
*
22-
* A security mechanism ensures that an upgrade does not turn off upgradeability accidentally, although this risk is
23-
* reinstated if the upgrade retains upgradeability but removes the security mechanism, e.g. by replacing
24-
* `UUPSProxiable` with a custom implementation of upgrades.
25-
*
26-
* The `_authorizeUpgrade` function must be overridden to include access restriction to the upgrade mechanism.
27-
*/
5+
/// @title UUPS Proxiable
6+
/// @author Simon Malatrait <[email protected]>
7+
/// @dev This contract implements an upgradeability mechanism designed for UUPS proxies.
8+
/// @dev Adapted from <https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.9.0/contracts/proxy/utils/UUPSUpgradeable.sol>
9+
/// The functions included here can perform an upgrade of an UUPS Proxy, when this contract is set as the implementation behind such a proxy.
10+
///
11+
/// IMPORTANT: A UUPS proxy requires its upgradeability functions to be in the implementation as opposed to the transparent proxy.
12+
/// This means that if the proxy is upgraded to an implementation that does not support this interface, it will no longer be upgradeable.
13+
///
14+
/// A security mechanism ensures that an upgrade does not turn off upgradeability accidentally, although this risk is
15+
/// reinstated if the upgrade retains upgradeability but removes the security mechanism, e.g. by replacing
16+
/// `UUPSProxiable` with a custom implementation of upgrades.
17+
///
18+
/// The `_authorizeUpgrade` function must be overridden to include access restriction to the upgrade mechanism.
2819
abstract contract UUPSProxiable {
2920
// ************************************* //
3021
// * Event * //
3122
// ************************************* //
3223

33-
/**
34-
* Emitted when the `implementation` has been successfully upgraded.
35-
* @param newImplementation Address of the new implementation the proxy is now forwarding calls to.
36-
*/
24+
/// @dev Emitted when the `implementation` has been successfully upgraded.
25+
/// @param newImplementation Address of the new implementation the proxy is now forwarding calls to.
3726
event Upgraded(address indexed newImplementation);
3827

3928
// ************************************* //
4029
// * Error * //
4130
// ************************************* //
4231

43-
/**
44-
* @dev The call is from an unauthorized context.
45-
*/
32+
/// @dev The call is from an unauthorized context.
4633
error UUPSUnauthorizedCallContext();
4734

48-
/**
49-
* @dev The storage `slot` is unsupported as a UUID.
50-
*/
35+
/// @dev The storage `slot` is unsupported as a UUID.
5136
error UUPSUnsupportedProxiableUUID(bytes32 slot);
5237

53-
/// The `implementation` is not UUPS-compliant
38+
/// @dev The `implementation` is not UUPS-compliant
5439
error InvalidImplementation(address implementation);
5540

5641
/// Failed Delegated call
@@ -60,48 +45,40 @@ abstract contract UUPSProxiable {
6045
// * Storage * //
6146
// ************************************* //
6247

63-
/**
64-
* @dev Storage slot with the address of the current implementation.
65-
* This is the keccak-256 hash of "eip1967.proxy.implementation" subtracted by 1, and is
66-
* validated in the constructor.
67-
* NOTE: bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1)
68-
*/
48+
/// @dev Storage slot with the address of the current implementation.
49+
/// @dev This is the keccak-256 hash of "eip1967.proxy.implementation" subtracted by 1, and is
50+
/// @dev validated in the constructor.
51+
/// @dev NOTE: bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1)
6952
bytes32 private constant IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
7053

71-
/**
72-
* @dev Storage variable of the proxiable contract address.
73-
* It is used to check whether or not the current call is from the proxy.
74-
*/
54+
/// @dev Storage variable of the proxiable contract address.
55+
/// @dev It is used to check whether or not the current call is from the proxy.
56+
/// @custom:oz-upgrades-unsafe-allow state-variable-immutable
7557
address private immutable __self = address(this);
7658

7759
// ************************************* //
7860
// * Governance * //
7961
// ************************************* //
8062

81-
/**
82-
* @dev Function that should revert when `msg.sender` is not authorized to upgrade the contract.
83-
* @dev Called by {upgradeToAndCall}.
84-
*/
63+
/// @dev Function that should revert when `msg.sender` is not authorized to upgrade the contract.
64+
/// @dev Called by {upgradeToAndCall}.
8565
function _authorizeUpgrade(address newImplementation) internal virtual;
8666

8767
// ************************************* //
8868
// * State Modifiers * //
8969
// ************************************* //
9070

91-
/**
92-
* @dev Upgrade mechanism including access control and UUPS-compliance.
93-
* @param newImplementation Address of the new implementation contract.
94-
* @param data Data used in a delegate call to `newImplementation` if non-empty. This will typically be an encoded
95-
* function call, and allows initializing the storage of the proxy like a Solidity constructor.
96-
*
97-
* @dev Reverts if the execution is not performed via delegatecall or the execution
98-
* context is not of a proxy with an ERC1967-compliant implementation pointing to self.
99-
*/
71+
/// @dev Upgrade mechanism including access control and UUPS-compliance.
72+
/// @param newImplementation Address of the new implementation contract.
73+
/// @param data Data used in a delegate call to `newImplementation` if non-empty. This will typically be an encoded
74+
/// function call, and allows initializing the storage of the proxy like a Solidity constructor.
75+
/// @dev Reverts if the execution is not performed via delegatecall or the execution
76+
/// context is not of a proxy with an ERC1967-compliant implementation pointing to self.
10077
function upgradeToAndCall(address newImplementation, bytes memory data) public payable virtual {
10178
_authorizeUpgrade(newImplementation);
10279

103-
/* Check that the execution is being performed through a delegatecall call and that the execution context is
104-
a proxy contract with an implementation (as defined in ERC1967) pointing to self. */
80+
// Check that the execution is being performed through a delegatecall call and that the execution context is
81+
// a proxy contract with an implementation (as defined in ERC1967) pointing to self.
10582
if (address(this) == __self || _getImplementation() != __self) {
10683
revert UUPSUnauthorizedCallContext();
10784
}
@@ -118,6 +95,7 @@ abstract contract UUPSProxiable {
11895

11996
if (data.length != 0) {
12097
// The return data is not checked (checking, in case of success, that the newImplementation code is non-empty if the return data is empty) because the authorized callee is trusted.
98+
/// @custom:oz-upgrades-unsafe-allow delegatecall
12199
(bool success, ) = newImplementation.delegatecall(data);
122100
if (!success) {
123101
revert FailedDelegateCall();
@@ -132,14 +110,12 @@ abstract contract UUPSProxiable {
132110
// * Public Views * //
133111
// ************************************* //
134112

135-
/**
136-
* @dev Implementation of the ERC1822 `proxiableUUID` function. This returns the storage slot used by the
137-
* implementation. It is used to validate the implementation's compatibility when performing an upgrade.
138-
*
139-
* IMPORTANT: A proxy pointing at a proxiable contract should not be considered proxiable itself, because this risks
140-
* bricking a proxy that upgrades to it, by delegating to itself until out of gas. Thus it is critical that this
141-
* function revert if invoked through a proxy. This is guaranteed by the if statement.
142-
*/
113+
/// @dev Implementation of the ERC1822 `proxiableUUID` function. This returns the storage slot used by the
114+
/// implementation. It is used to validate the implementation's compatibility when performing an upgrade.
115+
///
116+
/// IMPORTANT: A proxy pointing at a proxiable contract should not be considered proxiable itself, because this risks
117+
/// bricking a proxy that upgrades to it, by delegating to itself until out of gas. Thus it is critical that this
118+
/// function revert if invoked through a proxy. This is guaranteed by the if statement.
143119
function proxiableUUID() external view virtual returns (bytes32) {
144120
if (address(this) != __self) {
145121
// Must not be called through delegatecall
@@ -148,10 +124,8 @@ abstract contract UUPSProxiable {
148124
return IMPLEMENTATION_SLOT;
149125
}
150126

151-
/**
152-
* @notice Returns the version of the contract.
153-
* @return Version string.
154-
*/
127+
/// @dev Returns the version of the implementation.
128+
/// @return Version string.
155129
function version() external view virtual returns (string memory);
156130

157131
// ************************************* //

0 commit comments

Comments
 (0)