fix(FeeVaultUpgradeTemplate): configure fee vaults via setters, not initialize#1469
Open
Wazabie wants to merge 5 commits into
Open
fix(FeeVaultUpgradeTemplate): configure fee vaults via setters, not initialize#1469Wazabie wants to merge 5 commits into
Wazabie wants to merge 5 commits into
Conversation
…alize The template upgraded each fee-vault proxy with `ProxyAdmin.upgradeAndCall(proxy, impl, initialize(recipient, min, network))`. On any chain whose fee vaults are already initialized in L2 genesis (OpenZeppelin v5 `_initialized == 1`), `initialize` carries the `initializer` modifier (version 1, not a bumped `reinitializer`), so it reverts with `InvalidInitialization()`. The OP `Proxy` masks the inner revert as "delegatecall to new implementation contract failed", `upgradeAndCall` rolls back, and the L1 task tx still succeeds — so the recipient silently never changes on L2. The pre-flight only checks L2 ProxyAdmin ownership, not the already-initialized case, making this fail confusingly. Fix: deposit `ProxyAdmin.upgrade(proxy, impl)` (no initializer call) followed by the v1.6.0 storage setters setRecipient / setMinWithdrawalAmount / setWithdrawalNetwork, each gated by the L2 ProxyAdmin owner (the aliased rootSafe these deposits run as). Works whether or not the proxy was previously initialized — same approach as RevShareCommon.configureVaultsForRevShare. Per vault: 1 upgrade + 3 setters (was 1 upgradeAndCall); _validate updated. Regenerated the FeeVaultUpgradeTemplate regression fixtures (expectedCallData + nested-multisig data-to-sign) for the new deposit pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wazabie
force-pushed
the
fix/feevault-upgrade-already-initialized
branch
from
4cf926e to
e6743c9
Compare
Wazabie
changed the title
The previous commit pasted the regenerated FeeVaultUpgradeTemplate `expectedCallData` into the wrong test: it overwrote the SingleMultisigGasConfigTemplate `expectedCallData` (line 44) while the FeeVaultUpgradeTemplate test (line 784) kept its stale upgradeAndCall calldata. Result: both regression tests failed in forge_test. - Restore SingleMultisigGasConfigTemplate `expectedCallData` to its correct value (keccak matches the archive-fork actual 0x8195483f...). - Move the new FeeVault upgrade+setters calldata to the FeeVault test's `expectedCallData` (keccak matches the archive-fork actual 0x319f8603...). - Apply forge fmt to FeeVaultUpgradeTemplate.sol (setRecipient depositCall wrapping) to fix the forge_fmt check. FeeVault `expectedDataToSign` were already regenerated in the prior commit; domain separators match the prior pinned values and struct hashes change as expected from the new root calldata. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Forks Sepolia + OP Sepolia, simulates the L1 task, replays the 14 portal deposits on L2, and asserts the fee-vault recipients actually update: the vaults go from v1.3.1 (immutable recipient, recipient() reverts) to v1.6.0 with recipient()/RECIPIENT() == the configured recipient, min=0, network=L1. This proves the upgrade+setters fix in #1469 takes effect on an already-initialized vault, where the old upgradeAndCall+initialize path silently no-ops. Skipped in CI like other *Integration tests (forge test --skip Integration); run manually with DISABLE_OVERRIDE_NONCE_CHECK=1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
FeeVaultUpgradeTemplateupgrades each fee-vault predeploy withProxyAdmin.upgradeAndCall(proxy, impl, initialize(recipient, minWithdrawal, network)).On any chain whose fee vaults are already initialized in L2 genesis (the normal case for live OP Stack chains), this silently no-ops on L2 while the L1 task tx succeeds:
initializeuses OpenZeppelin v5'sinitializermodifier (version 1, not a bumpedreinitializer). The proxies already have_initialized == 1, soinitializereverts withInvalidInitialization()(0xf92ee8a9).Proxy.upgradeToAndCallrequires the post-upgrade delegatecall to succeed, masking the inner error as"Proxy: delegatecall to new implementation contract failed".upgradeAndCallreverts → the impl-slot write rolls back → the recipient never changes.The template's pre-flight only checks L2 ProxyAdmin ownership, not the already-initialized case, so this is a confusing land-on-L1 / silently-fail-on-L2 mode.
Reproduced on-chain (Sepolia devnet
migration-src-0, chainId 420120140)upgradeAndCallon L2 reverts withProxy: delegatecall to new implementation contract failedInvalidInitialization(); all four vault proxies read_initialized = 1at the OZ-v5 namespaced slotFix
Deposit
ProxyAdmin.upgrade(proxy, impl)(no initializer call), then configure via the v1.6.0 storage setterssetRecipient/setMinWithdrawalAmount/setWithdrawalNetwork. Each setter is gated by the L2 ProxyAdmin owner — the aliased rootSafe these portal deposits already execute as — so it works whether or not the proxy was previously initialized. Same approach as the existingRevShareCommon.configureVaultsForRevSharehelper.Per vault: 1
upgrade+ 3 setters (was 1upgradeAndCall);_validate's expected action count updated accordingly.Verification
forge build✓testRegressionCallDataMatches_FeeVaultUpgradeTemplateregenerated for the new deposit pattern:expectedCallDatais now 14 deposits (2 CREATE2 deploys + 3upgrade+ 9 setters), plus the nested-multisigexpectedDataToSignfor Foundation + Council. The regenerated data-to-sign domain hashes match the previous pinned values exactly (they are override-driven / block-independent), and the calldata was verified selector-by-selector (upgradeAndCallgone,upgrade×3, 14×depositTransaction). CI's archive-fork run at the pinned block is the authoritative regression check.Follow-ups (out of scope here)
Tasks that consume this template must be re-simulated and their VALIDATION hashes regenerated, since the generated calldata changes:
src/tasks/sep/085-migrations-sop-1-fee-vault-updatemigration-src-0fee-vault task (#1463)🤖 Generated with Claude Code