Skip to content

Commit d300f6d

Browse files
ypatil12claude
andcommitted
fix: address Protocol Registry informationals from Certora audit (#1690)
**Motivation:** Address informational findings from the Certora audit for Protocol Registry (PR-1655). These improvements enhance validation, prevent potential issues with orphaned configurations, and improve documentation clarity. **Modifications:** 1. **Fix I-01: ship() lacks validation** - Added array length validation: `addresses.length == configs.length == names.length` - Added zero address validation: revert if any address is `address(0)` - Added new errors: `ArrayLengthMismatch()` and `InputAddressZero()` 2. **Fix I-02: Orphaned configs on name overwrite** - When re-shipping a name with a new address, the old address's `DeploymentConfig` is now deleted - Added `DeploymentConfigDeleted(address)` event to signal cleanup - Prevents orphaned configs that could cause confusion 3. **Fix I-03: configure() for unshipped addresses** - Update `configure` function to pass in a name - Added validation that the address must be a shipped deployment before allowing configuration 5. **Fix I-04: Fix misleading NatSpec** - Updated `ship()` NatSpec to document that names can be re-shipped with new addresses - Clarified that old address configs are automatically deleted when this happens - Updated `configure()` NatSpec to indicate address must be previously shipped 6. **Fix I-05: Document pauseAll blocking** - Added warning documentation that `pauseAll()` will revert if ANY pausable deployment fails - A single misconfigured deployment can block the entire protocol pause - Important for operational awareness during emergency scenarios **Result:** - `ship()` validates inputs before processing - No more orphaned configs when re-shipping names - `configure()` only works for shipped addresses - Clearer documentation on expected behavior and edge cases - Operators understand risks of `pauseAll()` failure scenarios 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent e1270da commit d300f6d

File tree

7 files changed

+677
-52
lines changed

7 files changed

+677
-52
lines changed

docs/core/ProtocolRegistry.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,17 +82,19 @@ A contract name may be passed to this function repeatedly; each time, the previo
8282
### `configure`
8383

8484
```solidity
85-
function configure(address addr, DeploymentConfig calldata config) external onlyRole(DEFAULT_ADMIN_ROLE)
85+
function configure(string calldata name, DeploymentConfig calldata config) external onlyRole(DEFAULT_ADMIN_ROLE)
8686
```
8787

8888
Updates the stored `DeploymentConfig` for a single deployment:
8989

9090
*Effects*:
91+
* Looks up the address for `name` from `_deployments`.
9192
* Overwrites `_deploymentConfigs[addr]` with the supplied configuration.
9293
* Emits `DeploymentConfigured(addr, config)`.
9394

9495
*Requirements*:
9596
* Caller must hold `DEFAULT_ADMIN_ROLE`.
97+
* `name` must have been previously shipped.
9698

9799
### `pauseAll`
98100

pkg/bindings/IProtocolRegistry/binding.go

Lines changed: 157 additions & 13 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/bindings/ProtocolRegistry/binding.go

Lines changed: 158 additions & 14 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/bindings/ProtocolRegistryStorage/binding.go

Lines changed: 157 additions & 13 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/contracts/core/ProtocolRegistry.sol

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,26 +37,37 @@ contract ProtocolRegistry is Initializable, AccessControlEnumerableUpgradeable,
3737
string[] calldata names,
3838
string calldata semanticVersion
3939
) external onlyRole(DEFAULT_ADMIN_ROLE) {
40+
// Validate array lengths match
41+
require(addresses.length == configs.length && configs.length == names.length, ArrayLengthMismatch());
42+
4043
// Update the semantic version.
4144
_updateSemanticVersion(semanticVersion);
4245
for (uint256 i = 0; i < addresses.length; ++i) {
46+
// Validate no zero addresses
47+
require(addresses[i] != address(0), InputAddressZero());
4348
// Append each provided
4449
_appendDeployment(addresses[i], configs[i], names[i]);
4550
}
4651
}
4752

4853
/// @inheritdoc IProtocolRegistry
4954
function configure(
50-
address addr,
55+
string calldata name,
5156
DeploymentConfig calldata config
5257
) external onlyRole(DEFAULT_ADMIN_ROLE) {
58+
// Verify name is a shipped deployment and get its address (O(1) lookup)
59+
(bool exists, address addr) = _deployments.tryGet(_unwrap(name.toShortString()));
60+
require(exists, DeploymentNotShipped());
5361
// Update the config
5462
_deploymentConfigs[addr] = config;
5563
// Emit the event.
5664
emit DeploymentConfigured(addr, config);
5765
}
5866

5967
/// @inheritdoc IProtocolRegistry
68+
/// @dev WARNING: This function will revert if ANY pausable deployment fails to pause.
69+
/// Ensure all deployments marked as pausable correctly implement IPausable.pauseAll().
70+
/// A single misconfigured deployment will block the entire protocol pause.
6071
function pauseAll() external onlyRole(PAUSER_ROLE) {
6172
uint256 length = totalDeployments();
6273
// Iterate over all stored deployments.
@@ -89,8 +100,17 @@ contract ProtocolRegistry is Initializable, AccessControlEnumerableUpgradeable,
89100
DeploymentConfig calldata config,
90101
string calldata name
91102
) internal {
103+
uint256 nameKey = _unwrap(name.toShortString());
104+
105+
// Clean up old config if name exists with different address
106+
(bool exists, address oldAddr) = _deployments.tryGet(nameKey);
107+
if (exists && oldAddr != addr) {
108+
delete _deploymentConfigs[oldAddr];
109+
emit DeploymentConfigDeleted(oldAddr);
110+
}
111+
92112
// Store name => address mapping
93-
_deployments.set({key: _unwrap(name.toShortString()), value: addr});
113+
_deployments.set({key: nameKey, value: addr});
94114
// Store deployment config
95115
_deploymentConfigs[addr] = config;
96116
// Emit the events.

src/contracts/interfaces/IProtocolRegistry.sol

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,16 @@
11
// SPDX-License-Identifier: BUSL-1.1
22
pragma solidity ^0.8.27;
33

4-
interface IProtocolRegistryErrors {}
4+
interface IProtocolRegistryErrors {
5+
/// @notice Thrown when array lengths don't match in ship().
6+
error ArrayLengthMismatch();
7+
8+
/// @notice Thrown when a zero address is provided.
9+
error InputAddressZero();
10+
11+
/// @notice Thrown when trying to configure an address that hasn't been shipped.
12+
error DeploymentNotShipped();
13+
}
514

615
interface IProtocolRegistryTypes {
716
/// @notice Configuration for a protocol deployment.
@@ -24,6 +33,10 @@ interface IProtocolRegistryEvents is IProtocolRegistryTypes {
2433
/// @param config The configuration for the deployment.
2534
event DeploymentConfigured(address indexed addr, DeploymentConfig config);
2635

36+
/// @notice Emitted when a deployment config is deleted due to name re-assignment.
37+
/// @param addr The address whose config was deleted.
38+
event DeploymentConfigDeleted(address indexed addr);
39+
2740
/// @notice Emitted when the semantic version is updated.
2841
/// @param previousSemanticVersion The previous semantic version.
2942
/// @param semanticVersion The new semantic version.
@@ -41,13 +54,15 @@ interface IProtocolRegistry is IProtocolRegistryErrors, IProtocolRegistryEvents
4154

4255
/// @notice Ships a list of deployments.
4356
/// @dev Only callable by the admin.
44-
/// @param addresses The addresses of the deployments to ship.
57+
/// @param addresses The addresses of the deployments to ship. Must not contain zero addresses.
4558
/// @param configs The configurations of the deployments to ship.
4659
/// @param contractNames The names of the contracts to ship.
4760
/// @param semanticVersion The semantic version to ship.
4861
/// @dev Contract names can be passed in as type(contract).name, e.g. `type(AllocationManager).name`
4962
/// @dev Contract names must be <= 31 bytes
50-
/// @dev Contract names can be overridden any number of times.
63+
/// @dev Contract names can be re-shipped with a new address. When this happens,
64+
/// the old address's config is automatically deleted to prevent orphaned configs.
65+
/// @dev All input arrays (`addresses`, `configs`, `names`) must have the same length.
5166
function ship(
5267
address[] calldata addresses,
5368
DeploymentConfig[] calldata configs,
@@ -56,16 +71,18 @@ interface IProtocolRegistry is IProtocolRegistryErrors, IProtocolRegistryEvents
5671
) external;
5772

5873
/// @notice Configures a deployment.
59-
/// @dev Only callable by the admin.
60-
/// @param addr The address of the deployment to configure.
74+
/// @dev Only callable by the admin. Name must have been previously shipped.
75+
/// @param name The name of the deployment to configure.
6176
/// @param config The configuration to set.
6277
function configure(
63-
address addr,
78+
string calldata name,
6479
DeploymentConfig calldata config
6580
) external;
6681

6782
/// @notice Pauses all deployments that support pausing.
6883
/// @dev Loops over all deployments and attempts to invoke `pauseAll()` on each contract that is marked as pausable.
84+
/// @dev WARNING: Reverts if any pausable deployment doesn't implement IPausable or if any pauseAll() call reverts.
85+
/// A single misconfigured deployment will block the entire protocol pause. Ensure registry is correctly configured.
6986
function pauseAll() external;
7087

7188
/// @notice Returns the full semantic version string of the protocol (e.g. "1.2.3").

src/test/unit/ProtocolRegistryUnit.t.sol

Lines changed: 157 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,17 +139,171 @@ contract ProtocolRegistryUnitTests is EigenLayerUnitTestSetup, IProtocolRegistry
139139
assertEq(protocolRegistry.getAddress("Contract3"), addr3);
140140
}
141141

142+
function test_ship_revert_arrayLengthMismatch_addressesConfigs() public {
143+
address[] memory addresses = new address[](2);
144+
addresses[0] = address(0x1);
145+
addresses[1] = address(0x2);
146+
147+
IProtocolRegistryTypes.DeploymentConfig[] memory configs = new IProtocolRegistryTypes.DeploymentConfig[](1);
148+
configs[0] = IProtocolRegistryTypes.DeploymentConfig({pausable: false, deprecated: false});
149+
150+
string[] memory names = new string[](2);
151+
names[0] = "A";
152+
names[1] = "B";
153+
154+
cheats.expectRevert(ArrayLengthMismatch.selector);
155+
protocolRegistry.ship(addresses, configs, names, "1.0.0");
156+
}
157+
158+
function test_ship_revert_arrayLengthMismatch_configsNames() public {
159+
address[] memory addresses = new address[](2);
160+
addresses[0] = address(0x1);
161+
addresses[1] = address(0x2);
162+
163+
IProtocolRegistryTypes.DeploymentConfig[] memory configs = new IProtocolRegistryTypes.DeploymentConfig[](2);
164+
configs[0] = IProtocolRegistryTypes.DeploymentConfig({pausable: false, deprecated: false});
165+
configs[1] = IProtocolRegistryTypes.DeploymentConfig({pausable: false, deprecated: false});
166+
167+
string[] memory names = new string[](1);
168+
names[0] = "A";
169+
170+
cheats.expectRevert(ArrayLengthMismatch.selector);
171+
protocolRegistry.ship(addresses, configs, names, "1.0.0");
172+
}
173+
174+
function test_ship_revert_zeroAddress() public {
175+
address[] memory addresses = new address[](2);
176+
addresses[0] = address(0x1);
177+
addresses[1] = address(0); // Zero address
178+
179+
IProtocolRegistryTypes.DeploymentConfig[] memory configs = new IProtocolRegistryTypes.DeploymentConfig[](2);
180+
configs[0] = IProtocolRegistryTypes.DeploymentConfig({pausable: false, deprecated: false});
181+
configs[1] = IProtocolRegistryTypes.DeploymentConfig({pausable: false, deprecated: false});
182+
183+
string[] memory names = new string[](2);
184+
names[0] = "A";
185+
names[1] = "B";
186+
187+
cheats.expectRevert(InputAddressZero.selector);
188+
protocolRegistry.ship(addresses, configs, names, "1.0.0");
189+
}
190+
191+
function test_ship_overwriteName_deletesOldConfig() public {
192+
address oldAddr = address(0x111);
193+
address newAddr = address(0x222);
194+
string memory name = "TestContract";
195+
196+
// Ship with old address
197+
address[] memory addresses1 = new address[](1);
198+
addresses1[0] = oldAddr;
199+
IProtocolRegistryTypes.DeploymentConfig[] memory configs1 = new IProtocolRegistryTypes.DeploymentConfig[](1);
200+
configs1[0] = IProtocolRegistryTypes.DeploymentConfig({pausable: true, deprecated: false});
201+
string[] memory names1 = new string[](1);
202+
names1[0] = name;
203+
protocolRegistry.ship(addresses1, configs1, names1, "1.0.0");
204+
205+
// Verify old address is set
206+
assertEq(protocolRegistry.getAddress(name), oldAddr);
207+
208+
// Ship same name with new address - should emit DeploymentConfigDeleted for old address
209+
address[] memory addresses2 = new address[](1);
210+
addresses2[0] = newAddr;
211+
IProtocolRegistryTypes.DeploymentConfig[] memory configs2 = new IProtocolRegistryTypes.DeploymentConfig[](1);
212+
configs2[0] = IProtocolRegistryTypes.DeploymentConfig({pausable: false, deprecated: true});
213+
string[] memory names2 = new string[](1);
214+
names2[0] = name;
215+
216+
cheats.expectEmit(true, true, true, true, address(protocolRegistry));
217+
emit DeploymentConfigDeleted(oldAddr);
218+
cheats.expectEmit(true, true, true, true, address(protocolRegistry));
219+
emit DeploymentShipped(newAddr, configs2[0]);
220+
221+
protocolRegistry.ship(addresses2, configs2, names2, "1.1.0");
222+
223+
// Verify new address is set
224+
assertEq(protocolRegistry.getAddress(name), newAddr);
225+
// Verify total deployments is still 1 (not 2)
226+
assertEq(protocolRegistry.totalDeployments(), 1);
227+
}
228+
229+
function test_ship_sameNameSameAddress_noDeleteEvent() public {
230+
address addr = address(0x111);
231+
string memory name = "TestContract";
232+
233+
// Ship with address
234+
address[] memory addresses1 = new address[](1);
235+
addresses1[0] = addr;
236+
IProtocolRegistryTypes.DeploymentConfig[] memory configs1 = new IProtocolRegistryTypes.DeploymentConfig[](1);
237+
configs1[0] = IProtocolRegistryTypes.DeploymentConfig({pausable: true, deprecated: false});
238+
string[] memory names1 = new string[](1);
239+
names1[0] = name;
240+
protocolRegistry.ship(addresses1, configs1, names1, "1.0.0");
241+
242+
// Ship same name with same address - should NOT emit DeploymentConfigDeleted
243+
IProtocolRegistryTypes.DeploymentConfig[] memory configs2 = new IProtocolRegistryTypes.DeploymentConfig[](1);
244+
configs2[0] = IProtocolRegistryTypes.DeploymentConfig({pausable: false, deprecated: true});
245+
246+
// We only expect DeploymentShipped, not DeploymentConfigDeleted
247+
cheats.expectEmit(true, true, true, true, address(protocolRegistry));
248+
emit DeploymentShipped(addr, configs2[0]);
249+
250+
protocolRegistry.ship(addresses1, configs2, names1, "1.1.0");
251+
}
252+
142253
/// -----------------------------------------------------------------------
143254
/// configure()
144255
/// -----------------------------------------------------------------------
145256

146257
function test_configure_OnlyOwner() public {
258+
// First ship a deployment
147259
address addr = cheats.randomAddress();
148-
IProtocolRegistryTypes.DeploymentConfig memory config = IProtocolRegistryTypes.DeploymentConfig({pausable: true, deprecated: false});
260+
address[] memory addresses = new address[](1);
261+
addresses[0] = addr;
262+
IProtocolRegistryTypes.DeploymentConfig[] memory configs = new IProtocolRegistryTypes.DeploymentConfig[](1);
263+
configs[0] = IProtocolRegistryTypes.DeploymentConfig({pausable: false, deprecated: false});
264+
string[] memory names = new string[](1);
265+
names[0] = "TestName";
266+
protocolRegistry.ship(addresses, configs, names, "1.0.0");
267+
268+
IProtocolRegistryTypes.DeploymentConfig memory newConfig =
269+
IProtocolRegistryTypes.DeploymentConfig({pausable: true, deprecated: false});
149270

150271
cheats.prank(nonOwner);
151272
cheats.expectRevert();
152-
protocolRegistry.configure(addr, config);
273+
protocolRegistry.configure("TestName", newConfig);
274+
}
275+
276+
function test_configure_revert_unshippedName() public {
277+
IProtocolRegistryTypes.DeploymentConfig memory config = IProtocolRegistryTypes.DeploymentConfig({pausable: true, deprecated: false});
278+
279+
cheats.expectRevert(DeploymentNotShipped.selector);
280+
protocolRegistry.configure("Unshipped", config);
281+
}
282+
283+
function test_configure_succeeds_shippedName() public {
284+
address shipped = address(0x111);
285+
286+
// First ship the address
287+
address[] memory addresses = new address[](1);
288+
addresses[0] = shipped;
289+
IProtocolRegistryTypes.DeploymentConfig[] memory configs = new IProtocolRegistryTypes.DeploymentConfig[](1);
290+
configs[0] = IProtocolRegistryTypes.DeploymentConfig({pausable: false, deprecated: false});
291+
string[] memory names = new string[](1);
292+
names[0] = "Test";
293+
protocolRegistry.ship(addresses, configs, names, "1.0.0");
294+
295+
// Now configure should succeed
296+
IProtocolRegistryTypes.DeploymentConfig memory newConfig =
297+
IProtocolRegistryTypes.DeploymentConfig({pausable: true, deprecated: true});
298+
299+
cheats.expectEmit(true, true, true, true, address(protocolRegistry));
300+
emit DeploymentConfigured(shipped, newConfig);
301+
302+
protocolRegistry.configure("Test", newConfig);
303+
304+
(, IProtocolRegistryTypes.DeploymentConfig memory retrievedConfig) = protocolRegistry.getDeployment("Test");
305+
assertEq(retrievedConfig.pausable, true);
306+
assertEq(retrievedConfig.deprecated, true);
153307
}
154308

155309
function test_configure_Correctness() public {
@@ -178,7 +332,7 @@ contract ProtocolRegistryUnitTests is EigenLayerUnitTestSetup, IProtocolRegistry
178332
cheats.expectEmit(true, true, true, true, address(protocolRegistry));
179333
emit DeploymentConfigured(addr, newConfig);
180334

181-
protocolRegistry.configure(addr, newConfig);
335+
protocolRegistry.configure("TestContract", newConfig);
182336

183337
(, IProtocolRegistryTypes.DeploymentConfig memory retrievedConfig) = protocolRegistry.getDeployment("TestContract");
184338
assertEq(retrievedConfig.pausable, true);

0 commit comments

Comments
 (0)