upgradeability in Processor Endpoint - #162
Conversation
saratnt
left a comment
saratnt
left a comment
There was a problem hiding this comment.
Additional comments:
- The go bindings should be updated.
- NOTICES still lists @openzeppelin/contracts at ^5.3.0 and gained no entries
for the two new direct deps. - From Claude: "
contracts/.openzeppelin/has no defined home in the repo: not committed, not
gitignored, not mentioned in the README or in
docs/design/UPGRADABLE_CONTRACTS_DESIGN.md.
The first deploy against a real network writes the upgrades plugin's manifest
there — the record of each proxy, its current implementation, and that
implementation's storage layout. upgradeProxy reads it to check the new
implementation's layout against the deployed one
(hardhat-upgrades/dist/utils/validate-impl.js:31 →
getStorageLayoutForAddress). The layout isn't recoverable from chain, so
without the manifest an upgrade fails with Deployment at address ... is not registered — including when run from CI or by anyone who didn't do the deploy.
Right now it surfaces as untracked output and is easy to lose.
Three things:
- Commit the manifests for public networks (OZ's recommendation) so the layout
history is a repo artifact rather than a per-machine file. - Gitignore
contracts/.openzeppelin/unknown-31337.jsononly — not
unknown-*.horizen-l3-testnet's chain id isn't in the plugin's
network-name map, so its manifest is anunknown-<chainId>.jsontoo, and a
blanket pattern would ignore exactly the one worth keeping. - Note it in the README's Upgrading section: the manifest must be present and
current before running an upgrade script, and
upgrades.forceImport(proxy, Factory, { kind: 'uups' })re-registers a proxy
if it's lost — with the caveat that it takes the layout from whatever source
is currently compiled, so it's only correct if that matches what's deployed."
| tokenAllowlist = _tokenAllowlist; | ||
| feeCollector = payable(updateStatusOperator); | ||
| _grantRole(UPDATE_STATUS_ROLE, updateStatusOperator); | ||
| _grantRole(ADMIN, admin); |
There was a problem hiding this comment.
From Claude: The ADMIN role cannot be rotated or revoked. ProcessorEndpointExtension.sol:93-99
grants ADMIN but never grants DEFAULT_ADMIN_ROLE and never calls
_setRoleAdmin(ADMIN, ...), so getRoleAdmin(ADMIN) == DEFAULT_ADMIN_ROLE, held
by nobody.
So a compromised ADMIN key can never be revoked, a lost one freezes
upgradeability forever, and renounceRole bricks it.
Grant DEFAULT_ADMIN_ROLE to admin, or
_setRoleAdmin(ADMIN, ADMIN).
| /// - `state-variable-immutable`: `_extension` stays `immutable`, set once per implementation | ||
| /// deployment in the constructor rather than through `initialize` — see the `_extension` doc | ||
| /// comment on `ProcessorEndpoint` for why that is safe under a proxy. | ||
| export const PROCESSOR_ENDPOINT_UPGRADE_OPTIONS = { |
There was a problem hiding this comment.
From Claude:
unsafeAllow is applied per-deployment, not per-declaration
All three exceptions are blanket-approved for the whole validation run. state-variable-immutable is checked against every immutable in the contract (upgrades-core/dist/validate/run.js:358), so it currently covers _extension but will silently pass a future immutable that should have been proxy state.
All three can be narrowed to source annotations, which also documents each exception where it applies:
- /// @Custom:oz-upgrades-unsafe-allow state-variable-immutable on _extension (ProcessorEndpoint.sol:42)
- /// @Custom:oz-upgrades-unsafe-allow missing-initializer-call on initialize (ProcessorEndpoint.sol:70) — the check does skipCheck('missing-initializer-call', contractInitializer) on the function definition (upgrades-core/dist/validate/run/initializer.js:206-207), so this one is narrowable too
- /// @Custom:oz-upgrades-unsafe-allow delegatecall on _delegateToExtensionCall (ProcessorEndpoint.sol:234) — only this one. delegateToExtension uses inline assembly, and the validator matches only Solidity-level .delegatecall() calls (typeIdentifier /^t_function_baredelegatecall/, run.js:44-46); there is no Yul/InlineAssembly handling, so annotating it would be a no-op.
With all three annotated, PROCESSOR_ENDPOINT_UPGRADE_OPTIONS is empty and can be dropped from its 6 call sites (scripts/deploy/{all,processorEndpoint}.ts, scripts/upgrade/processorEndpoint.ts, test/ProcessorEndpoint/{constructor.spec,fixture}.ts). Move the existing doc comment from util.ts into the contract next to each annotation — it's the only written record of why each exception is safe.
|
|
||
| const processorEndpoint = await ethers.getContractAt('ProcessorEndpoint', proxyAddress); | ||
| const extensionAddress = | ||
| process.env.PROCESSOR_ENDPOINT_EXTENSION || (await processorEndpoint.extension()); |
There was a problem hiding this comment.
From Claude: PROCESSOR_ENDPOINT_EXTENSION defaults to await processorEndpoint.extension().
Change ProcessorEndpointExtension.sol, forget the env var, and you deploy a
new implementation delegating to the old extension bytecode — no revert, no
plugin check. The script prints the address, but a print isn't a guard.
Compare the locally compiled extension bytecode against
getCode(extensionAddress) and refuse on mismatch.
2 participants
No description provided.