optimistic governance upgrade spell - #174
Conversation
|
/plamen-audit |
|
Plamen audit started in |
![reserve-auditor-bot[bot]](https://avatars.githubusercontent.com/u/34616001?s=60&v=4){kind=small}
There was a problem hiding this comment.
Plamen Audit
Mode: thorough | Status: static-only degraded | High: 1 Medium: 6 Low: 4 Informational: 2
🔴 High (1)
[H-01] Folio control can move to successor-vault governance before representative migrated supply exists [CONTESTED]
Severity: High
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:90-139, contracts/spells/GovernanceSpell_04_17_2026.sol:153-213, contracts/spells/GovernanceSpell_04_17_2026.sol:274-291
Confidence: HIGH (multiple code traces confirmed the missing readiness gate; Static Analysis: no dedicated detector; PoC: SKIPPED/BLOCKED)
Description:
The migration spell can deploy a fresh successor staking vault, then move Folio governance authority and future fee routing to a governor backed by that vault without enforcing that representative old-vault stake has migrated.
Step 1 deploys a new vault and checks its asset/admin invariants, but it does not seed shares, migrate deposits, check totalSupply(), or require delegated voting power:
IStakingVault oldStakingVault = IStakingVault(stakingVaultGovernor.token());
address newUnderlying = oldStakingVault.asset();
require(newUnderlying != address(0), UpgradeError(21));
...
require(IStakingVault(newDeployment.stakingVault).asset() == newUnderlying, UpgradeError(23));
require(newDeployment.stakingVault != address(oldStakingVault), UpgradeError(24));Step 2 accepts the successor vault, checks only that it reports version 1.0.0, and then rotates the Folio fee recipient, rebalance authority, proxy-admin ownership, and final admin role to the newly deployed Folio-governance timelock:
require(keccak256(bytes(IVersioned(address(newStakingVault)).version())) == VERSION_1_0_0, UpgradeError(3));
_rotateFeeRecipients(folio, oldFolioGovernor.token(), address(newStakingVault));
for (uint256 i = folio.getRoleMemberCount(REBALANCE_MANAGER); i > 0; i--) {
address rebalanceManager = folio.getRoleMember(REBALANCE_MANAGER, i - 1);
folio.revokeRole(REBALANCE_MANAGER, rebalanceManager);
}
folio.grantRole(REBALANCE_MANAGER, newDeployment.newTimelock);
folioProxyAdmin.transferOwnership(newDeployment.newTimelock);
folio.revokeRole(DEFAULT_ADMIN_ROLE, msg.sender);
folio.grantRole(DEFAULT_ADMIN_ROLE, newDeployment.newTimelock);
folio.renounceRole(DEFAULT_ADMIN_ROLE, address(this));The spell reconstructs governance ratios from the old vault and old governor, but it does not verify the new vault's current or historical supply before Folio control moves:
uint256 pastSupply = stakingVault.getPastTotalSupply(stakingVault.clock() - 1);
uint256 proposalThresholdWithSupply = governor.proposalThreshold();
proposalThreshold = (proposalThresholdWithSupply * 1e18 + pastSupply - 1) / pastSupply;
require(proposalThreshold >= 0.0001e18 && proposalThreshold <= 0.1e18, UpgradeError(15));The added tests assert that the new Folio governor uses the successor vault, but they create proposal power by directly dealing staking-vault shares to a proposer after the upgrade. That validates proposal creation, not actual holder migration before upgradeFolio() transfers authority.
Impact:
If the old Folio timelock executes upgradeFolio() before broad honest stake migrates to the successor vault, a tiny or early successor-vault constituency can become the effective electorate for the new Folio-governance timelock. That timelock receives Folio admin authority, proxy-admin ownership, and rebalance authority, so governance capture during the low-supply window can affect privileged Folio operations after the configured voting and timelock delays.
The same timing window also routes future Folio fee shares to the successor vault before its supply is representative. Early/current successor-vault stakers can receive a disproportionate stream of post-upgrade rewards if fees accrue while migrated supply is sparse.
The issue is sequencing dependent. A migration runbook that pre-seeds representative supply and confirms delegated voting power before executing Step 2 can mitigate the attack path, but the changed spell does not enforce those preconditions on-chain.
PoC Result:
Verification result: contested with strong code-trace evidence. forge build passed. The scoped fork tests did not run to completion locally because fork RPC environment variables were unset, and no passing exploit PoC was produced.
Recommendation:
Add an explicit on-chain readiness gate before upgradeFolio() transfers Folio roles, proxy-admin ownership, or fee recipients to the new governance system. At minimum, validate that the supplied successor vault is the intended successor for the old staking vault and that it has representative migrated voting power. Consider requiring minimum successor-vault supply/delegated votes relative to recent old-vault supply, separating fee-recipient rotation from authority rotation, or recording a Step 1 readiness manifest that Step 2 must prove.
🟠 Medium (6)
[M-01] Successor staking-vault optimistic startRebalance route executes from a timelock without REBALANCE_MANAGER [VERIFIED]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:L90-L123, contracts/spells/GovernanceSpell_04_17_2026.sol:L194-L201, test/spells/GovernanceSpell_04_17_2026/GenericGovernanceSpell_04_17_2026.t.sol:L227-L238, test/spells/GovernanceSpell_04_17_2026/GenericGovernanceSpell_04_17_2026.t.sol:L389-L398
Confidence: HIGH (source trace complete; build passed; PoC execution blocked by fork environment)
Description:
The spell allows Folio.startRebalance.selector to be configured in the successor staking-vault governor during Step 1, but Step 2 grants the Folio REBALANCE_MANAGER role only to the separate per-Folio governance timelock. In the shared-vault flow, the added tests also assert that Folio governors and timelocks are distinct from the staking-vault governance system, then only check optimistic proposal creation.
baseParams.selectorData = optimisticSelectorData;
baseParams.optimisticProposers = optimisticProposers;folio.grantRole(REBALANCE_MANAGER, newDeployment.newTimelock);
require(folio.getRoleMemberCount(REBALANCE_MANAGER) == 1, UpgradeError(7));
require(folio.getRoleMember(REBALANCE_MANAGER, 0) == newDeployment.newTimelock, UpgradeError(8));The changed tests configure the exact selector under the successor staking-vault deployment:
selectors[0] = Folio.startRebalance.selector;
selectorData[i] = IOptimisticSelectorRegistry.SelectorData({ target: folios[i], selectors: selectors });Because optimistic execution for the successor staking-vault governor is performed by the successor staking-vault timelock, the target Folio observes the wrong caller at startRebalance(). The proposal can be created, but the configured execution route is not authorized by the role state produced by upgradeFolio().
Impact:
The successor staking-vault optimistic rebalance fast path can be configured and proposed but fail at execution. Operators may rely on proposal-creation coverage as evidence that optimistic rebalancing is live, while time-sensitive rebalances still require a different governance path or emergency repair.
PoC Result:
Verified by complete source trace. forge build passed. The focused fork test path could not execute locally because required fork RPC environment variables were missing, so no executable PoC pass was obtained.
Recommendation:
Do not register Folio.startRebalance.selector in the successor staking-vault governor unless that timelock is intentionally granted REBALANCE_MANAGER on every target Folio. Prefer registering rebalance selectors only in the per-Folio governor whose timelock receives the role, and extend tests to execute the optimistic proposal rather than stopping at proposal creation.
[M-02] Old staking-vault retirement has no on-chain dependent-Folio completion guard [CONTESTED]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:L218-L230
Confidence: MEDIUM (source trace complete; impact depends on migration staging and dependent-Folio state)
Description:
retireOldStakingVault() permanently retires an old staking vault once ownership has been transferred to the spell, but the function does not check whether every Folio that depends on that old staking vault has completed migration. The comments explicitly place dependent-Folio enumeration off-chain.
/// @dev Enumeration of dependent Folios is off-chain: current governance is responsible for
/// confirming no Folio governor still uses this StakingVault as its voting token.
function retireOldStakingVault(IOwnableStakingVault oldStakingVault) public {
require(oldStakingVault.owner() == address(this), UpgradeError(13));
oldStakingVault.setUnstakingDelay(0);
oldStakingVault.setRewardRatio(1 days);
oldStakingVault.renounceOwnership();
require(oldStakingVault.owner() == address(0), UpgradeError(14));
}The function is public, so once the owner-staged precondition exists, any caller can finalize retirement. This matters when a shared old staking vault backs multiple Folios and one dependent Folio fails to complete upgradeFolio() because of state/configuration differences such as fee-recipient topology.
Impact:
If ownership is staged before all dependent Folios have migrated, the old staking vault can be placed into a terminal unowned state while a dependent Folio still relies on it as its voting token. That can leave the remaining Folio on retired governance infrastructure and remove owner-gated maintenance options.
PoC Result:
Contested source-trace finding. Existing changed tests cover the happy path where retirement happens after successful upgrades, but local fork execution was blocked by RPC/env issues and a Foundry client panic. No production fork proved or refuted the unsafe staging sequence.
Recommendation:
Add an on-chain retirement guard that binds retirement to explicit dependent-Folio completion, or split the API so retirement can only be called with a verified migration manifest whose entries are checked on-chain. At minimum, make ownership transfer and retirement atomic after all upgradeFolio() calls have succeeded, and add negative tests showing retirement cannot proceed while any dependent Folio still points at the old staking vault.
[M-03] Execution-time old-vault supply can brick threshold reconstruction [CONTESTED]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:L274-L291
Confidence: MEDIUM (arithmetic boundary is source-backed; production supply state was not fork-verified)
Description:
The spell reconstructs proposal-threshold and quorum ratios for the new governance deployment by sampling old staking-vault historical supply at execution time. The threshold calculation divides by pastSupply without an explicit nonzero floor and then requires the reconstructed ratio to fall inside a hard-coded pass band.
uint256 pastSupply = stakingVault.getPastTotalSupply(stakingVault.clock() - 1);
uint256 proposalThresholdWithSupply = governor.proposalThreshold();
proposalThreshold = (proposalThresholdWithSupply * 1e18 + pastSupply - 1) / pastSupply;
require(proposalThreshold >= 0.0001e18 && proposalThreshold <= 0.1e18, UpgradeError(15));When pastSupply == 0, the expression reverts. With dust supply, rounding can push the reconstructed threshold above 0.1e18; with very high supply relative to the old absolute threshold, it can fall below 0.0001e18. Because both deployment paths call _baseDeploymentParams(), this can block successor vault deployment or Folio governance deployment at execution time.
Impact:
The migration can be bricked by old-vault supply state at the sampled prior clock. This can delay or prevent upgrades and can amplify the old-vault retirement risk if operational steps assume migration completion while threshold reconstruction is reverting.
PoC Result:
Contested source-trace finding. Arithmetic boundary checks were computed during verification, including zero-supply division and pass-band cliff cases, but no Solidity fork PoC was executed because targeted fork tests did not run in the local environment.
Recommendation:
Use the same defensive supply floor or bounded conversion model as the old governance system, and validate the old-vault supply pass band before scheduling execution. Consider accepting explicit threshold/quorum parameters guarded by preflight assertions, or reverting earlier with a clear diagnostic if historical supply is zero or outside supported bounds.
[M-04] Strict fee-recipient topology can hard-stop upgradeFolio [CONTESTED]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:L305-L326
Confidence: MEDIUM (revert conditions are source-backed; production fee tables were not fork-verified)
Description:
_rotateFeeRecipients() assumes the current Folio fee-recipient table contains exactly one old-staking-vault recipient and no existing new-staking-vault recipient. Any valid production fee table outside that topology causes upgradeFolio() to revert before role, proxy-admin, and admin migration complete.
for (uint256 i; i < recipients.length; i++) {
address recipient = recipients[i].recipient;
if (recipient == oldStakingVault) {
oldStakingVaultRecipientCount++;
oldStakingVaultRecipientIndex = i;
}
require(recipient != newStakingVault, UpgradeError(19));
}
require(oldStakingVaultRecipientCount == 1, UpgradeError(20));This is a fail-fast invariant, but it is enforced during the upgrade transaction rather than preflighted or handled as a migration repair case. If a production Folio has no old-vault recipient, multiple old-vault entries, or a pre-existing new-vault entry, the entire Folio upgrade stops at fee-recipient rotation.
Impact:
A dependent Folio can remain on old governance because upgradeFolio() cannot pass its fee-recipient migration step. In a shared-vault migration, this liveness failure can combine with premature old-vault retirement and leave one dependent Folio behind.
PoC Result:
Contested source-trace finding. The revert paths are explicit in changed code, but production fee-recipient tables were not successfully fork-validated in this environment. Existing tests assert the intended topology for configured fixtures only.
Recommendation:
Preflight all production fee-recipient tables before execution and add a migration path for each expected topology. If exactly-one-old/no-new-vault is a required invariant, enforce it in a separate read-only checker used before scheduling the proposal, and add negative tests for no-old, duplicate-old, and existing-new-vault recipient cases.
[M-05] Fee routing can start before successor-vault supply is representative [CONTESTED]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:L189-L190, contracts/spells/GovernanceSpell_04_17_2026.sol:L305-L326
Confidence: MEDIUM (fee-routing state is source-backed; material loss depends on supply and fee timing)
Description:
upgradeFolio() rotates Folio fee recipients from the old staking vault to the new staking vault without checking that the successor vault has representative migrated supply. The fee route changes before any on-chain guard confirms that old-vault holders have migrated or delegated into the successor vault.
// rotate Folio fee recipients from old staking vault to new staking vault
_rotateFeeRecipients(folio, oldFolioGovernor.token(), address(newStakingVault));recipients[oldStakingVaultRecipientIndex].recipient = newStakingVault;
_sortFeeRecipients(recipients);
folio.setFeeRecipients(recipients);If Folio fees are distributed after this rotation while successor-vault supply is zero, sparse, or dominated by early stakers, fee rewards can accrue to the current successor-vault constituency rather than the intended migrated old-vault holder set. This is especially material when combined with H-01 or L-01.
Impact:
Future Folio fee shares can be routed into a successor vault before the economic constituency is representative. Early or sparse successor-vault stakers may receive rewards disproportionate to the old holder set, and fee distribution can become dependent on off-chain migration timing.
PoC Result:
Contested source-trace finding. The changed code performs the fee-recipient rotation without a supply check, but verification did not execute a forked reward-distribution harm PoC due local fork blockers.
Recommendation:
Gate fee-recipient rotation on an explicit successor-vault readiness condition, such as minimum migrated supply, expected holder/delegation checkpoint, or an operational delay after migration opens. Alternatively, delay fee-recipient rotation until after representative migration is confirmed, and add tests covering fee distribution before and after the readiness gate.
[M-06] Optimistic governance selectors, proposers, and veto parameters are caller-configured during migration [CONTESTED]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:L244-L269, test/spells/GovernanceSpell_04_17_2026/GenericGovernanceSpell_04_17_2026.t.sol:L379-L398, test/spells/GovernanceSpell_04_17_2026/GenericGovernanceSpell_04_17_2026.t.sol:L523-L529
Confidence: MEDIUM (configuration pass-through is source-backed; harmful final selector/proposer set was not proven)
Description:
The spell copies optimistic governance parameters, selector data, and optimistic proposers directly from migration calldata into the deployment parameters. The changed tests show both Folio.setName.selector and Folio.startRebalance.selector as configured selector examples, and the helper uses short optimistic veto timing values.
baseParams.optimisticParams = optimisticParams;
baseParams.selectorData = optimisticSelectorData;
baseParams.optimisticProposers = optimisticProposers;return IReserveOptimisticGovernor.OptimisticGovernanceParams({
vetoDelay: 1 seconds,
vetoPeriod: 1 days,
vetoThreshold: 0.05e18
});This finding is configuration-dependent and falls within the protocol's stated operational bounds for caller-selected optimistic governance settings: governance can choose selectors, proposers, and veto windows. The risk is that the spell itself does not enforce a selector allowlist, minimum conservative veto settings, or a production-specific review boundary for sensitive fast paths.
Impact:
If final migration calldata registers sensitive selectors, assigns a semi-trusted optimistic proposer, or sets weak veto parameters, optimistic governance can expose a faster state-change path than intended. The concrete impact depends on the configured selector; for example, rebalance selectors can affect market operations, and administrative selectors can affect Folio metadata or parameters.
PoC Result:
Contested source-trace finding. Pass-through configuration was verified in changed code, but no harmful final payload or unsafe optimistic execution was proven. Local changed fork tests were blocked by missing RPC env vars, and existing tests demonstrate proposal creation rather than unsafe execution.
Recommendation:
Add spell-level validation for optimistic configuration. At minimum, enforce an allowlist of permitted selectors per target, minimum veto delay/period/threshold values, and reviewed proposer addresses. Add tests that execute optimistic proposals for every allowed selector and verify that disallowed sensitive selectors are rejected by the spell before deployment.
There was a problem hiding this comment.
🟡 Low (4)
[L-01] Fee-recipient migration is not bound to successor-vault reward-token coverage [CONTESTED]
Severity: Low
Severity adjusted from Medium because the direct bad-calldata path requires trusted governance migration calldata to violate the stated migration assumptions.
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:90-123, contracts/spells/GovernanceSpell_04_17_2026.sol:181-190, contracts/spells/GovernanceSpell_04_17_2026.sol:305-326
Confidence: MEDIUM (code trace confirmed; fork execution blocked; final reward-token payload and production dependent-Folio count unverified)
Description:
The migration spell rotates a Folio fee-recipient entry from the old staking vault to newStakingVault, but it does not verify that the Folio token is either the new vault's native asset or a registered reward token in that vault. Step 1 accepts the successor vault reward-token list directly from calldata, while Step 2 checks only that the supplied vault reports version 1.0.0 before moving fee routing.
IReserveOptimisticGovernorDeployer.NewStakingVaultParams
memory newStakingVaultParams = IReserveOptimisticGovernorDeployer.NewStakingVaultParams({
underlying: IERC20Metadata(newUnderlying),
rewardTokens: rewardTokens,
rewardHalfLife: 3.5 days,
unstakingDelay: 1 weeks
});require(keccak256(bytes(IVersioned(address(newStakingVault)).version())) == VERSION_1_0_0, UpgradeError(3));
_rotateFeeRecipients(folio, oldFolioGovernor.token(), address(newStakingVault));For shared-vault migrations, this creates a guardrail gap: a non-underlying Folio can have its future fee shares routed to a vault that does not accrue that Folio token to stakers. The issue can also appear as a liveness/configuration problem when the successor vault reward-token set is full or incomplete.
Impact:
Folio fee shares can be minted to the successor vault without being distributed through the intended staking-vault reward accounting path. If the omitted token is added later, distribution timing may favor the then-current successor-vault supply rather than the intended migrated holder set. In the worst operational case, a full reward-token set can make repair require removing another reward token or redeploying/replanning the migration.
PoC Result:
Contested by code trace. Build succeeded, but no dedicated omitted-reward-token PoC was written, and the changed fork tests were blocked by missing fork RPC environment variables. The evidence supports the missing invariant, while material user impact depends on final migration calldata, fee amounts, and successor-vault reward-token state.
Recommendation:
Before rotating fee recipients, require every Folio being migrated to be either the successor vault asset or a registered reward token. For shared-vault migrations, validate the full dependent-Folio inventory against the vault's reward-token capacity before Step 1 is accepted or before any Step 2 fee routing is executed.
[L-02] upgradeFolio accepts unbound migration authority addresses [CONTESTED]
Severity: Low
Severity adjusted from Medium because exploitation requires trusted governance to execute incorrect migration calldata.
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:153-182, contracts/spells/GovernanceSpell_04_17_2026.sol:203-206
Confidence: MEDIUM (missing binding checks are visible in changed code; wrong-payload execution was not mechanically run)
Description:
upgradeFolio() accepts several authority-bearing addresses from calldata and validates them mostly in isolation. The old governor only needs to have msg.sender as its timelock, the new staking vault only needs to return the expected version string, and the proxy admin only needs to be owned by the spell. The function does not prove that the old governor is the canonical governor for the supplied Folio, that the new staking vault is the intended successor for that old vault, or that the supplied proxy admin administers the supplied Folio proxy.
require(oldFolioGovernor.timelock() == msg.sender, UpgradeError(1));
...
require(keccak256(bytes(IVersioned(address(newStakingVault)).version())) == VERSION_1_0_0, UpgradeError(3));require(folioProxyAdmin.owner() == address(this), UpgradeError(9));
folioProxyAdmin.transferOwnership(newDeployment.newTimelock);
require(folioProxyAdmin.owner() == newDeployment.newTimelock, UpgradeError(10));Because the same caller-supplied identities drive threshold reconstruction, guardian validation, fee-recipient rotation, role migration, and proxy-admin ownership transfer, a single wrong address in a governance payload can split or misdirect Folio authority.
Impact:
A malformed migration proposal can move Folio roles to a governance system backed by the wrong vault, rotate fee flow using the wrong old-vault identity, or transfer ownership of an unrelated proxy admin. Correct payload construction avoids the issue, but the spell does not enforce the cross-object relationships on-chain.
PoC Result:
Partial and contested by code trace. The missing relationship checks are present in the changed code, but no negative test was executed with mismatched governor, vault, Folio, and proxy-admin parameters. Existing fork tests cover the intended happy path and were not executable locally due fork RPC blockers.
Recommendation:
Bind all migration authorities before making external deployment or ownership-transfer calls. At minimum, validate that the old governor token matches the expected old staking vault for the supplied Folio migration, the new staking vault matches the deterministic Step 1 deployment or an approved successor registry entry, the new staking vault invariants match the old vault and Folio set, and the supplied proxy admin is the actual admin for the supplied Folio proxy.
[L-03] Permissionless Step 1 can pre-consume deterministic successor deployments [CONTESTED]
Severity: Low
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:90-123
Confidence: MEDIUM (permissionless changed entry point confirmed; deterministic collision behavior traced through dependency context but not executed against the spell)
Description:
deploySuccessorStakingVault() is intentionally public and permissionless. It forwards deployment parameters and a caller-supplied nonce to the reserve-governor deployer to create the successor vault/governance system.
function deploySuccessorStakingVault(
IFolioGovernor stakingVaultGovernor,
IReserveOptimisticGovernor.OptimisticGovernanceParams calldata optimisticParams,
IOptimisticSelectorRegistry.SelectorData[] calldata optimisticSelectorData,
address[] calldata optimisticProposers,
address[] calldata guardians,
address[] calldata rewardTokens,
bytes32 deploymentNonce
) public returns (NewDeployment memory newDeployment) {
...
) = governorDeployer.deployWithNewStakingVault(baseParams, newStakingVaultParams, deploymentNonce);
}Dependency-context tracing showed the deployer uses deterministic salts based on the deployer-side caller, deployment parameters, and nonce. Since all external callers reach the deployer through the spell contract, a third party with the exact future calldata can pre-execute Step 1 for the same tuple. A later queued governance batch that expects to perform the same deterministic deployment may then revert because the contracts already exist.
Impact:
By itself, Step 1 does not mutate the old Folio or old staking vault. The practical impact is migration disruption: a queued or scripted batch can be forced to handle an already-consumed deterministic deployment tuple, delaying later Folio upgrades or requiring runbook changes. This can amplify higher-severity migration sequencing problems when old-vault ownership or retirement steps are staged separately.
PoC Result:
Contested by code trace. Build succeeded, and local deterministic-deployment tests in adjacent dependency tests passed, but no direct duplicate-tuple PoC was added for the changed spell. Fork-dependent migration tests were blocked before proving the operational sequence.
Recommendation:
Treat Step 1 as a pre-execution step, not as an operation embedded in a later all-or-nothing governance batch. If deterministic pre-consumption is not acceptable, add an explicit registry of approved deployments and make later steps accept an already-deployed matching successor after verifying all invariants, or bind the deployment salt to an authorized executor/runbook identity.
[L-04] Guardian continuity is subset-only and deployer shared guardian is not old-set validated [CONTESTED]
Severity: Low
Severity adjusted for the omission-only path because it requires trusted governance to choose an incomplete guardian list. The shared-guardian expansion path remains configuration-dependent because the spell does not validate that default guardian against the old timelock set.
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:235-269, contracts/spells/GovernanceSpell_04_17_2026.sol:294-302
Confidence: MEDIUM (subset-only validation confirmed; final production guardian intent and shared-guardian trust boundary unverified)
Description:
The spell passes optimistic governance settings and additional guardians into the new deployment, but guardian validation only checks each supplied guardian independently. The code explicitly documents that it does not confirm completeness of the old canceller set.
baseParams.selectorData = optimisticSelectorData;
baseParams.optimisticProposers = optimisticProposers;
_validateGuardians(oldGovernor, guardians);
baseParams.additionalGuardians = guardians;/// @dev Does NOT confirm `guardians` is the complete set of CANCELLER_ROLE members
function _validateGuardians(IFolioGovernor oldGovernor, address[] memory guardians) internal view {
TimelockController oldTimelock = TimelockController(payable(oldGovernor.timelock()));
for (uint256 i; i < guardians.length; i++) {
require(guardians[i] != address(0) && guardians[i] != address(oldGovernor), UpgradeError(17));
require(oldTimelock.hasRole(CANCELLER_ROLE, guardians[i]), UpgradeError(18));
}
}Dependency-context review also showed the deployer grants a shared guardian in addition to the supplied guardian list. The changed spell does not validate whether that shared guardian was present in the old timelock's canceller set.
Impact:
New optimistic governance can launch with a cancellation boundary that is weaker, incomplete, or expanded relative to the old governance setup. This is most relevant when combined with sensitive optimistic selectors or short veto settings. If the final migration payload intentionally narrows guardians, the issue is a governance discretion risk; if the shared guardian is outside the old safety boundary, the spell provides no local proof that this expansion is intended for every migrated Folio.
PoC Result:
Contested by code trace. The subset-only check is confirmed, and changed tests configure optimistic params/selectors/proposers, but no fork run proved the final production guardian set or unsafe optimistic execution. Offline changed fork tests failed in setup because required fork RPC variables were unset.
Recommendation:
Make the intended guardian policy explicit in the spell. If continuity is required, compare the supplied guardian set to the old timelock's canceller set, excluding only documented addresses. If a shared guardian must always be added, validate that it is either already an old canceller or explicitly allowlisted per chain/Folio with comments and tests covering the expected role membership.
|
/plamen-audit |
There was a problem hiding this comment.
Plamen Audit
Mode: thorough | Status: static-only degraded | High: 1 Medium: 7 Low: 4 Informational: 2
Full report: reserve-protocol__reserve-index-dtf-pr174.md
High (1)
[H-01] Underpopulated successor governance receives Folio authority [VERIFIED]
Severity: High
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:151,177-219
Confidence: HIGH (multiple independent code reviews confirmed the missing on-chain readiness check; Static Analysis: not applicable to this cross-contract governance state; PoC: NOT_EXECUTED due Foundry runner panic before assertions)
Description:
upgradeFolio() moves Folio control to a newly deployed governor/timelock that uses newStakingVault as its voting token. The changed code documents that callers must wait until the successor vault has enough new stake, but that condition is not enforced before the Folio's fee recipient, rebalance manager, proxy admin, and default admin authority are transferred.
/// @dev IMPORTANT: Do not call until the `newStakingVault` has been sufficiently populated by new stake
function upgradeFolio(
Folio folio,
FolioProxyAdmin folioProxyAdmin,
IStakingVault newStakingVault,
IFolioGovernor oldFolioGovernor,
IReserveOptimisticGovernor.OptimisticGovernanceParams calldata optimisticParams,
address[] calldata optimisticProposers,
address[] calldata guardians,
bytes32 deploymentNonce
) public returns (NewDeployment memory newDeployment) {
require(oldFolioGovernor.timelock() == msg.sender, UpgradeError(1));The runtime checks validate version compatibility, Folio/vault eligibility, deployment success, and current Folio staging shape. They do not check successor-vault total supply, delegated votes, quorum-bearing distribution, or an old-to-new stake migration ratio.
require(keccak256(bytes(IVersioned(address(newStakingVault)).version())) == VERSION_1_0_0, UpgradeError(3));
require(
address(folio) == newStakingVault.asset() ||
newStakingVault.rewardTokenRegistry().isRegistered(address(folio)),
UpgradeError(28)
);
newDeployment.stakingVault = address(newStakingVault);
(newDeployment.newGovernor, newDeployment.newTimelock, newDeployment.newSelectorRegistry) = governorDeployer
.deployWithExistingStakingVault(baseParams, address(newStakingVault), deploymentNonce);
require(newDeployment.newTimelock != address(0), UpgradeError(2));After those checks, the function rotates privileged Folio control to the new timelock.
_rotateFeeRecipients(folio, oldFolioGovernor.token(), address(newStakingVault));
for (uint256 i = folio.getRoleMemberCount(REBALANCE_MANAGER); i > 0; i--) {
address rebalanceManager = folio.getRoleMember(REBALANCE_MANAGER, i - 1);
folio.revokeRole(REBALANCE_MANAGER, rebalanceManager);
}
folio.grantRole(REBALANCE_MANAGER, newDeployment.newTimelock);
require(folioProxyAdmin.owner() == address(this), UpgradeError(9));
folioProxyAdmin.transferOwnership(newDeployment.newTimelock);
folio.revokeRole(DEFAULT_ADMIN_ROLE, msg.sender);
folio.grantRole(DEFAULT_ADMIN_ROLE, newDeployment.newTimelock);
folio.renounceRole(DEFAULT_ADMIN_ROLE, address(this));If the old Folio timelock executes the upgrade while the successor vault is empty, dust-funded, or dominated by a small early holder, the new Folio governance system can inherit full operational control before meaningful stake migration has happened. The caller restriction means this is not permissionless: the old timelock must execute the upgrade too early. The missing on-chain readiness guard still leaves the critical migration invariant enforced only by off-chain process.
Impact:
A dust-funded or attacker-skewed successor electorate can become the authority behind the Folio's new timelock. That authority receives rebalance-manager control, proxy-admin ownership, default-admin control, and future fee-recipient routing. In the worst case, a first or dominant successor-vault staker can pass privileged governance actions that would not have passed after broad honest stake migration, affecting Folio administration, upgrades, and rebalance control for the full Folio.
PoC Result:
Verification preserved the finding as code-confirmed but did not execute a passing PoC. Forge compilation succeeded, but existing scoped fork tests compiled and then Foundry panicked before assertions. Production successor-vault stake and vote distribution was unavailable.
Recommendation:
Make stake-readiness an explicit on-chain precondition of upgradeFolio() or split authority transfer into a second callable step that can only execute after readiness is proven. At minimum, require governance payloads to provide and validate minimum newStakingVault.totalSupply() or past supply, minimum delegated voting power, minimum successor-vault supply relative to the old vault, and an optional per-Folio proof tying the selected successor vault to the staged migration.
Medium (7)
[M-01] Existing-vault path accepts unproven vault provenance or electorate [CONTESTED]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:177-188
Confidence: MEDIUM (multiple review artifacts confirmed the code path; Static Analysis: fallback only; PoC: SKIPPED/BLOCKED)
Description:
upgradeFolio() can deploy Folio governance on a caller-supplied existing staking vault after checking only the vault version and whether the Folio is either the vault asset or globally registered in the vault's reward-token registry. Unlike the new-vault deployment path, this existing-vault path does not prove that the supplied vault was deployed by the spell, has the intended electorate, has the intended admin/provenance, or is configured as the expected successor for the Folio.
require(keccak256(bytes(IVersioned(address(newStakingVault)).version())) == VERSION_1_0_0, UpgradeError(3));
require(
address(folio) == newStakingVault.asset() ||
newStakingVault.rewardTokenRegistry().isRegistered(address(folio)),
UpgradeError(28)
);
newDeployment.stakingVault = address(newStakingVault);
(newDeployment.newGovernor, newDeployment.newTimelock, newDeployment.newSelectorRegistry) = governorDeployer
.deployWithExistingStakingVault(baseParams, address(newStakingVault), deploymentNonce);This is not a permissionless attacker path: the old Folio timelock must call upgradeFolio(). The risk is that a malformed or stale migration payload can attach a Folio to a compatible but unintended existing vault whose voters or administrators are not the intended successor governance set.
Impact:
If an incorrect existing vault is selected, the wrong electorate can receive Folio governance authority and can compound reward-tracking issues described in M-02. In the worst case this can misassign control of a high-value Folio to a vault that is not the intended migrated governance token. Severity remains Medium because the path requires old-governance payload execution and no production wrong-vault payload was proven.
PoC Result:
No executable PoC was produced. Verification preserved the mechanism by code trace, but no fork or custom test executed a harmful wrong-existing-vault payload. Existing scoped tests compiled, then fork execution failed before assertions due the local Foundry/runtime environment.
Recommendation:
Bind the existing-vault path to a provenance-safe successor. At minimum, require a vault-local or deployer-provided proof that the vault is the intended successor for the Folio, and validate expected admin/electorate properties before transferring Folio roles. If only spell-deployed successors are intended, remove or restrict the arbitrary existing-vault path.
[M-02] Non-asset Folio fees can route to a vault without vault-local reward tracking [CONTESTED]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:179-196,338-340
Confidence: MEDIUM (code path and reward-accounting mismatch supported; Static Analysis: fallback only; PoC: SKIPPED/BLOCKED)
Description:
For non-asset Folios, upgradeFolio() accepts a staking vault if the Folio is globally registered in the vault's reward-token registry. It does not verify that this specific vault locally tracks the Folio token as a reward token before rotating future Folio fee recipients to the vault.
require(
address(folio) == newStakingVault.asset() ||
newStakingVault.rewardTokenRegistry().isRegistered(address(folio)),
UpgradeError(28)
);
_rotateFeeRecipients(folio, oldFolioGovernor.token(), address(newStakingVault));recipients[oldStakingVaultRecipientIndex].recipient = newStakingVault;
_sortFeeRecipients(recipients);
folio.setFeeRecipients(recipients);Verification found that the global registry and a vault's local reward-token set are separate concepts in the called staking-vault implementation. Future Folio fee shares can therefore be minted to the selected vault while the vault's local reward-accrual loop does not process that Folio token.
Impact:
Successor-vault stakers may not accrue newly minted Folio fee shares until later governance/admin remediation adds the Folio token to the vault-local reward set. The permanent-loss framing was not proven; verification softened the impact to delayed or remediation-dependent fee distribution. This can still be material for large fee streams, especially if M-01 causes the selected vault to be the wrong or weakly governed existing vault.
PoC Result:
No full harm PoC executed. Verification confirmed the code path and the local/global reward-tracking mismatch by source trace. Fork tests failed before assertions, and artifact rules prevented adding a dedicated temporary test harness.
Recommendation:
For non-asset Folios, require vault-local reward tracking before rotating fee recipients. For example, check that address(folio) appears in the selected vault's local reward-token list, or add/configure the reward token as part of the same controlled migration path before any fees can be minted to the new vault.
[M-03] Public old-vault retirement relies on off-chain dependent-Folio completeness [CONTESTED]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:224-234
Confidence: MEDIUM (public post-staging state transition confirmed; Static Analysis: fallback only; PoC: SKIPPED/BLOCKED)
Description:
retireOldStakingVault() is public once the old staking vault owner has been staged to the spell. The function explicitly leaves dependent-Folio enumeration to off-chain governance, but it does not enforce that every Folio still using the old vault has migrated before permanently changing the old vault's exit/reward settings and renouncing ownership.
/// @dev Enumeration of dependent Folios is off-chain: current governance is responsible for
/// confirming no Folio governor still uses this StakingVault as its voting token.
function retireOldStakingVault(IOwnableStakingVault oldStakingVault) public {
require(oldStakingVault.owner() == address(this), UpgradeError(13));
oldStakingVault.setUnstakingDelay(0);
oldStakingVault.setRewardRatio(1 days);
oldStakingVault.renounceOwnership();
require(oldStakingVault.owner() == address(0), UpgradeError(14));
}The existing shared-vault happy-path test upgrades both dependent Folios before retirement, but the on-chain spell itself only checks owner() == address(this). If ownership staging and final retirement are split, or if a dependent Folio is missed, any caller can finalize retirement after staging.
Impact:
A still-dependent Folio can be left relying on an ownerless old vault with zero unstaking delay and changed reward runoff parameters. This can impair recovery and governance assumptions for Folios that still use the old staking vault as their voting token. The finding is Medium because exploitability depends on governance sequencing or an incomplete dependent-Folio inventory.
PoC Result:
No dedicated PoC executed. Verification built the scoped spell and confirmed the source trace, but closest fork tests failed before assertions because fork RPC variables were missing or Foundry fork setup failed locally.
Recommendation:
Avoid relying solely on off-chain completeness at the irreversible retirement step. Prefer a migration registry or explicit list/hash of dependent Folios that must be marked upgraded before retirement. If on-chain enumeration is impractical, batch ownership staging and retirement only after machine-checked off-chain payload assertions prove all dependent Folios have migrated.
[M-04] Old-vault retirement can strand or distort residual reward runoff [CONTESTED]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:228-234
Confidence: MEDIUM (retirement mechanics and reward-tail boundary supported; Static Analysis: fallback only; PoC: SKIPPED/BLOCKED)
Description:
The retirement function sets the old vault's unstaking delay to zero, changes the reward half-life to one day, and immediately renounces ownership. It does not check whether residual reward balances have fully run off, whether users have claimed, or whether old-vault supply will remain nonzero during the runoff window.
require(oldStakingVault.owner() == address(this), UpgradeError(13));
oldStakingVault.setUnstakingDelay(0);
oldStakingVault.setRewardRatio(1 days);
oldStakingVault.renounceOwnership();
require(oldStakingVault.owner() == address(0), UpgradeError(14));Verification traced the old vault reward mechanics as context: reward handout remains time-based after the ratio update, and zero total supply causes reward handout to return zero. With zero-delay exits, old stakers can leave before residual rewards finish streaming, potentially leaving idle balances or allowing later/remaining supply to receive an outsized share.
Impact:
Original old-vault stakers can receive less than their expected residual reward value if retirement occurs with nonzero reward balances and users exit before runoff completes. The issue is not proven as immediate permanent loss; materiality depends on live residual balances, exit timing, and whether supply drops to zero or becomes concentrated.
PoC Result:
No full token-flow PoC executed. Verification confirmed the changed retirement sequence and concrete one-day half-life boundary by code trace, but live residual balances were unavailable and no writable PoC harness could be added under artifact discipline.
Recommendation:
Gate retirement on reward-runoff readiness. Options include requiring zero or below-threshold residual reward balances, enforcing a minimum runoff/claim window before renouncing ownership, or splitting zero-delay exit enablement from owner renouncement so governance can remediate residual reward accounting if needed.
[M-05] Threshold derivation can revert when old-vault past supply is zero or tiny [PARTIAL]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:292-305
Confidence: HIGH (boundary arithmetic confirmed by source trace; Static Analysis: fallback only; PoC: SKIPPED/BLOCKED)
Description:
Both migration paths derive new standard-governance parameters from the old governor through _proposalThresholdAndQuorum(). The function divides by the old staking vault's raw historical supply before checking for a zero or tiny denominator.
uint256 pastSupply = stakingVault.getPastTotalSupply(stakingVault.clock() - 1);
uint256 proposalThresholdWithSupply = governor.proposalThreshold();
proposalThreshold = (proposalThresholdWithSupply * 1e18 + pastSupply - 1) / pastSupply;
require(proposalThreshold >= 0.0001e18 && proposalThreshold <= 0.1e18, UpgradeError(15));If pastSupply is zero, the calculation reverts before the explicit UpgradeError(15) bounds check. If pastSupply is tiny, a one-token old proposal threshold can exceed the hardcoded 10% upper bound and revert. Verification's boundary calculation showed that pastSupply below 10 with a one-token threshold can fail the upper cap.
Impact:
Successor-vault deployment or Folio governance upgrade can fail before deployment, role rotation, or fee-recipient migration completes. This is a liveness risk rather than direct fund loss. It can also create operational timing pressure: waiting for users to migrate out of the old vault can reduce old-vault supply, while executing too early can worsen the successor-governance readiness risk described in H-01.
PoC Result:
No executable Foundry PoC ran. Verification confirmed the arithmetic boundary by direct code trace and local calculation. Existing scoped fork tests compiled but did not execute assertions because the fork runner failed locally.
Recommendation:
Add explicit precondition checks before division. Revert with a clear migration-specific error when pastSupply == 0, and validate that the old governor's threshold/quorum settings are convertible under the new bounds before beginning deployment. Consider allowing governance payloads to supply audited override parameters when old-vault supply is below a safe denominator floor.
[M-06] Selector-only optimistic whitelist permits arbitrary startRebalance parameters [CONTESTED]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:175,277-284
Confidence: MEDIUM (selector-only surface confirmed; economic harm unproven; Static Analysis: fallback only; PoC: SKIPPED/BLOCKED)
Description:
upgradeFolio() installs optimistic selector data for Folio.startRebalance, giving optimistic governance a fast path for that function after migration. The spell constrains only the target Folio and the four-byte selector; it does not constrain rebalance parameters such as token lists, limits, price ranges, TTL, auction launcher behavior, or other economic inputs.
baseParams.selectorData = _startRebalanceSelectorData(folio);function _startRebalanceSelectorData(
Folio folio
) internal pure returns (IOptimisticSelectorRegistry.SelectorData[] memory selectorData) {
selectorData = new IOptimisticSelectorRegistry.SelectorData[](1);
bytes4[] memory selectors = new bytes4[](1);
selectors[0] = Folio.startRebalance.selector;
selectorData[0] = IOptimisticSelectorRegistry.SelectorData({ target: address(folio), selectors: selectors });
}Dependency validation reviewed during verification checks target and selector, not calldata arguments. Once the new timelock becomes the sole rebalance manager, a configured optimistic proposer can submit arbitrary ABI-valid startRebalance calldata unless guardians or veto voters stop the proposal.
Impact:
A malicious, compromised, or misconfigured optimistic proposer can attempt a harmful rebalance faster than standard governance if cancellation/veto defenses fail. The severity is Medium because exploitability depends on proposer trust, guardian completeness, veto participation, and a concrete economically harmful rebalance that passes Folio bounds. The risk is amplified by H-01 and L-01.
PoC Result:
No harm PoC executed. Verification confirmed the selector-only permission path and rejected overstatements: optimistic governance cannot call arbitrary Folio methods through this whitelist, and literal zero veto supply auto-cancels optimistic proposals in dependency context. Existing tests only asserted proposal creation and did not execute harmful rebalance parameters.
Recommendation:
Treat optimistic startRebalance as a parameterized capability, not just a selector. Either disable optimistic proposers for Folio upgrades by default, require full guardian carryover and successor-vote readiness before enabling the selector, or add a payload-generation/validation layer that restricts startRebalance arguments to an approved economic envelope.
[M-07] Supplied FolioProxyAdmin is not proven to administer the supplied Folio [CONTESTED]
Severity: Medium
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:157-165,210-212
Confidence: MEDIUM (binding gap confirmed; wrong-admin payload not executed; Static Analysis: fallback only; PoC: SKIPPED/BLOCKED)
Description:
upgradeFolio() accepts folio and folioProxyAdmin as independent parameters. It checks only that the supplied folioProxyAdmin is owned by the spell, then transfers that supplied admin to the new timelock. It does not prove that the supplied proxy admin is actually the admin for the supplied Folio proxy.
function upgradeFolio(
Folio folio,
FolioProxyAdmin folioProxyAdmin,
IStakingVault newStakingVault,
IFolioGovernor oldFolioGovernor,
IReserveOptimisticGovernor.OptimisticGovernanceParams calldata optimisticParams,
address[] calldata optimisticProposers,
address[] calldata guardians,
bytes32 deploymentNonce
) public returns (NewDeployment memory newDeployment) {require(folioProxyAdmin.owner() == address(this), UpgradeError(9));
folioProxyAdmin.transferOwnership(newDeployment.newTimelock);
require(folioProxyAdmin.owner() == newDeployment.newTimelock, UpgradeError(10));If a migration payload stages the wrong proxy admin to the spell, upgradeFolio() can transfer that unrelated admin while the real proxy admin for the target Folio remains controlled by old governance or another owner.
Impact:
The Folio's role ownership can be migrated while upgrade authority is split or left behind, complicating emergency upgrades and post-migration remediation. This is an operational/configuration risk rather than a permissionless exploit, but the impact can be material for high-value Folios if payload construction mispairs Folio and proxy-admin addresses.
PoC Result:
No wrong-admin payload PoC executed. Verification found no negative test for wrong proxy-admin binding in the changed suite. Fork tests could not execute assertions locally, and no new PoC file was written due artifact-write restrictions.
Recommendation:
Add a binding check between the supplied folio and folioProxyAdmin before transferring ownership. If the proxy/admin architecture cannot expose that relation on-chain, require deterministic off-chain payload assertions that query the actual proxy admin for each Folio immediately before proposal submission and execution.
There was a problem hiding this comment.
Plamen Audit: Low Findings
Full report: reserve-protocol__reserve-index-dtf-pr174.md
Low (4)
[L-01] Guardian carryover validates only a subset of old cancellers [UNVERIFIED]
Severity: Low
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:308-316
Confidence: MEDIUM (source property confirmed, Static Analysis: N/A, PoC: SKIPPED)
Description:
The spell validates that every supplied guardian was a canceller on the old timelock, but it intentionally does not validate that all important old cancellers are carried forward. This means a migration payload can omit old cancellation authorities while still passing the spell's local checks.
/// Require `guardians` is a subset of the old timelock's CANCELLER_ROLE members (excl old governor)
/// @dev Does NOT confirm `guardians` is the complete set of CANCELLER_ROLE members
function _validateGuardians(IFolioGovernor oldGovernor, address[] memory guardians) internal view {
TimelockController oldTimelock = TimelockController(payable(oldGovernor.timelock()));
for (uint256 i; i < guardians.length; i++) {
require(guardians[i] != address(0) && guardians[i] != address(oldGovernor), UpgradeError(17));
require(oldTimelock.hasRole(CANCELLER_ROLE, guardians[i]), UpgradeError(18));
}
}Impact:
If the migration payload omits an operationally important canceller, the upgraded governance system may have weaker emergency cancellation coverage than the old one. This is primarily an operational and defense-in-depth risk, and its practical severity depends on the live guardian policy, the shared Guardian's coverage, and whether optimistic proposers are enabled.
PoC Result:
Verification confirmed the subset-only source property. No executable PoC or production role diff was available; the existing verifier recorded this as a preserved, contested enabler rather than a mechanically proven standalone exploit.
Recommendation:
Add a migration-time completeness check or explicit payload assertion for the expected old canceller set. If full carryover is intentionally not required, require the proposal payload or deployment checklist to document omitted cancellers and why the shared Guardian plus supplied guardians are sufficient.
[L-02] Supplied oldFolioGovernor is not directly bound to the Folio [UNVERIFIED]
Severity: Low
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:157-196, contracts/spells/GovernanceSpell_04_17_2026.sol:241-305
Confidence: MEDIUM (source trace confirmed, Static Analysis: N/A, PoC: SKIPPED)
Description:
upgradeFolio() receives both the Folio and the old Folio governor as independent parameters. The spell checks that the supplied governor's timelock is the caller, and separately checks that the caller is a current Folio admin, but it does not directly prove that the supplied governor is the canonical governor for the supplied Folio. The supplied governor is then used to derive the old staking vault, proposal threshold, quorum, guardian set, and the fee-recipient address that should be replaced.
require(oldFolioGovernor.timelock() == msg.sender, UpgradeError(1));
...
_rotateFeeRecipients(folio, oldFolioGovernor.token(), address(newStakingVault));IStakingVault oldStakingVault = IStakingVault(oldGovernor.token());
...
) = _proposalThresholdAndQuorum(oldStakingVault, oldGovernor);
...
_validateGuardians(oldGovernor, guardians);Impact:
A malformed migration payload could derive governance parameters or fee-recipient rotation from the wrong same-timelock governor. Many wrong-token cases may revert during fee-recipient rotation, but same-timelock or otherwise ambiguous configurations remain a payload-review risk.
PoC Result:
Verification did not execute a wrong-governor payload. The verifier found no changed-suite negative test for this case, and fork execution was blocked by the local Foundry runtime panic before assertions.
Recommendation:
Bind the supplied governor to the supplied Folio through an on-chain relation if one exists. If the protocol architecture cannot expose such a relation, add deterministic deployment-script checks that assert oldFolioGovernor.token() is the expected old staking vault fee recipient for the specific Folio and that the governor/timelock pair is not merely a same-timelock governor for another Folio.
[L-03] Strict staging and fee-recipient shape can block migration [UNVERIFIED]
Severity: Low
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:190-193, contracts/spells/GovernanceSpell_04_17_2026.sol:210, contracts/spells/GovernanceSpell_04_17_2026.sol:320-336
Confidence: MEDIUM (liveness source trace preserved, Static Analysis: N/A, PoC: SKIPPED)
Description:
The migration spell requires a very specific staging shape before it will complete the Folio upgrade. The Folio must have exactly two default admins, those admins must be the spell and old timelock, the supplied proxy admin must already be owned by the spell, the old staking vault must appear exactly once as a fee recipient, and the new staking vault must not already appear as a fee recipient.
require(folio.getRoleMemberCount(DEFAULT_ADMIN_ROLE) == 2, UpgradeError(4));
require(folio.hasRole(DEFAULT_ADMIN_ROLE, address(this)), UpgradeError(5));
require(folio.hasRole(DEFAULT_ADMIN_ROLE, msg.sender), UpgradeError(6));
...
require(folioProxyAdmin.owner() == address(this), UpgradeError(9));for (uint256 i; i < recipients.length; i++) {
address recipient = recipients[i].recipient;
if (recipient == oldStakingVault) {
oldStakingVaultRecipientCount++;
oldStakingVaultRecipientIndex = i;
}
require(recipient != newStakingVault, UpgradeError(19));
}
require(oldStakingVaultRecipientCount == 1, UpgradeError(20));Impact:
If the live Folio admin set, proxy-admin staging, or fee-recipient table differs from the expected shape at execution time, the migration can revert and leave governance in a partially staged operational state that must be remediated by another proposal. This is a liveness and execution-risk issue, not a direct asset-theft path.
PoC Result:
No dedicated production-state liveness PoC was executed. The existing verification preserved the issue as a code-trace liveness risk, with production staging and fee-recipient tables unavailable.
Recommendation:
Add preflight payload checks that read the exact Folio admin set, proxy-admin owner, and fee-recipient table at the target fork block before proposal submission. Consider exposing a view helper or dry-run script that returns the specific UpgradeError that would be hit for each Folio.
[L-04] Full rebalance-manager revocation loop is role-cardinality sensitive [UNVERIFIED]
Severity: Low
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:198-207
Confidence: LOW (source trace preserved, Static Analysis: N/A, PoC: SKIPPED)
Description:
During upgradeFolio(), the spell revokes every current REBALANCE_MANAGER member in a single transaction and then grants the role to the new timelock. This loop is correct for ordinary role-cardinality assumptions, but the spell does not bound or pre-check the number of existing role members. If a production Folio has a larger-than-expected manager set, migration gas cost and execution reliability become dependent on that live cardinality.
for (uint256 i = folio.getRoleMemberCount(REBALANCE_MANAGER); i > 0; i--) {
address rebalanceManager = folio.getRoleMember(REBALANCE_MANAGER, i - 1);
folio.revokeRole(REBALANCE_MANAGER, rebalanceManager);
}
folio.grantRole(REBALANCE_MANAGER, newDeployment.newTimelock);
require(folio.getRoleMemberCount(REBALANCE_MANAGER) == 1, UpgradeError(7));
require(folio.getRoleMember(REBALANCE_MANAGER, 0) == newDeployment.newTimelock, UpgradeError(8));Impact:
Unexpectedly high REBALANCE_MANAGER cardinality can make a migration transaction more expensive or, in the extreme, fail because too many revocations must be executed atomically. The practical risk is low if production Folios maintain a small manager set.
PoC Result:
No gas-bound or fork-state PoC was executed. Existing verifier artifacts preserve this as a low-confidence liveness risk because production role counts and gas limits were not measured.
Recommendation:
Add a preflight check that records getRoleMemberCount(REBALANCE_MANAGER) for every target Folio before proposal submission. If any target has an unexpectedly large manager set, split cleanup into a separate proposal or add a bounded migration helper that can be executed in controlled batches.
There was a problem hiding this comment.
Plamen Audit: Informational Findings
Full report: reserve-protocol__reserve-index-dtf-pr174.md
Informational (2)
[I-01] BSC fork coverage depends on provider and key compatibility [UNVERIFIED]
Severity: Informational
Location: .github/workflows/test.yml:73-76, test/base/BaseTest.sol:161-169
Confidence: MEDIUM (test-infrastructure source trace, Static Analysis: N/A, PoC: SKIPPED)
Description:
The PR adds BSC fork RPC support to CI and test setup, but the workflow constructs the BSC Alchemy URL using the same secret name as Ethereum mainnet. This may be intentional if the secret is an Alchemy app key that supports both networks, but it creates a coverage dependency on provider and key compatibility that is not enforced by the repository itself.
env:
FORK_RPC_MAINNET: "https://eth-mainnet.g.alchemy.com/v2/${{ secrets.ALCHEMY_MAINNET_KEY }}"
FORK_RPC_BASE: "https://base-mainnet.g.alchemy.com/v2/${{ secrets.ALCHEMY_BASE_KEY }}"
FORK_RPC_BSC: "https://bnb-mainnet.g.alchemy.com/v2/${{ secrets.ALCHEMY_MAINNET_KEY }}"if (target == ForkNetwork.ETHEREUM) {
forkRpc = vm.envString("FORK_RPC_MAINNET");
} else if (target == ForkNetwork.BASE) {
forkRpc = vm.envString("FORK_RPC_BASE");
} else if (target == ForkNetwork.BSC) {
forkRpc = vm.envString("FORK_RPC_BSC");
}Impact:
If the mainnet Alchemy key is not authorized for the BSC endpoint, CI may fail BSC fork tests or silently reduce effective coverage if those tests are skipped in other environments. This is test-infrastructure risk only and does not directly affect on-chain spell execution.
PoC Result:
Verifier artifacts could not validate the secret or provider configuration locally. Fork tests compiled but did not execute assertions in the local environment because Foundry panicked during fork setup.
Recommendation:
Use a distinct ALCHEMY_BSC_KEY or document that ALCHEMY_MAINNET_KEY is intentionally a multi-chain app key. Add a lightweight CI preflight that fails clearly when FORK_RPC_BSC cannot serve the required BSC fork block.
[I-02] Reserve-governor dependency upgrade requires separate diff review [UNVERIFIED]
Severity: Informational
Location: package.json, pnpm-lock.yaml, remappings.txt:4
Confidence: MEDIUM (dependency/remapping source trace, Static Analysis: N/A, PoC: SKIPPED)
Description:
The PR introduces the optimistic-governance spell and points imports at the external @reserve-protocol/reserve-governor package. The lockfile pins that package to a specific GitHub commit, and remappings.txt maps Solidity imports into node_modules. Because the audit scope is PR diff-only and local artifacts record that an old/new dependency diff review was unavailable, the dependency change should be treated as requiring a separate dependency review rather than being implicitly covered by the spell review.
@reserve-protocol/reserve-governor/=node_modules/@reserve-protocol/reserve-governor/
'@reserve-protocol/reserve-governor':
specifier: github:reserve-protocol/reserve-governor#3e6c3af0e3b6db36c9135f9206f65ec20c0fd4f0
version: https://codeload.github.com/reserve-protocol/reserve-governor/tar.gz/3e6c3af0e3b6db36c9135f9206f65ec20c0fd4f0
Impact:
The spell's safety relies on external deployer, governor, selector-registry, and staking-vault behavior from the pinned dependency. Any security-relevant changes in that dependency, especially optimistic proposal validation and staking-vault reward accounting, should be reviewed as part of release readiness.
PoC Result:
No standalone on-chain exploit is asserted. Existing report-index evidence records this as a tooling and dependency-review gap, not a confirmed contract vulnerability.
Recommendation:
Perform and archive a dependency diff review for @reserve-protocol/reserve-governor at commit 3e6c3af0e3b6db36c9135f9206f65ec20c0fd4f0, with explicit attention to deployer authorization, optimistic selector validation, timelock role grants, cancellation/veto semantics, and staking-vault reward-token accounting.
… upgradeFolio()" This reverts commit 9d685b7.
There was a problem hiding this comment.
Plamen Audit: Informational Findings
Full report: reserve-protocol__reserve-index-dtf-pr174.md
Informational (3)
[I-01] NewGovernanceDeployment omits old-system context for monitoring [LOW CONFIDENCE]
Severity: Informational
Location: contracts/spells/GovernanceSpell_04_17_2026.sol:67, contracts/spells/GovernanceSpell_04_17_2026.sol:139, contracts/spells/GovernanceSpell_04_17_2026.sol:215
Confidence: LOW (multiple analysis sources confirmed; Static Analysis: N; PoC: SKIPPED)
Description:
The changed spell emits NewGovernanceDeployment after both deployment paths, but the event only contains the new deployment tuple.
event NewGovernanceDeployment(NewDeployment newDeployment);
emit NewGovernanceDeployment(newDeployment);Impact:
No on-chain security impact was verified. Monitoring, incident response, dashboards, and reconciliation may need to parse calldata or maintain off-chain metadata to associate a new deployment with the old system it replaced.
PoC Result:
Verification classified this as monitoring-only and low confidence by code trace.
Recommendation:
Add a second event or expand event parameters with indexed old-system context for each deployment path, such as old governor, old timelock, old staking vault, Folio address where applicable, and new deployment addresses.
[I-02] BSC fork RPC workflow uses the mainnet Alchemy secret in a BNB endpoint [CONFIG]
Severity: Informational
Location: .github/workflows/test.yml:73-77, test/base/BaseTest.sol:167-168
Confidence: MEDIUM (multiple analysis sources confirmed; Static Analysis: N; PoC: SKIPPED)
Description:
The changed CI workflow adds FORK_RPC_BSC, but points the BNB endpoint at the same secret used for Ethereum mainnet.
FORK_RPC_MAINNET: "https://eth-mainnet.g.alchemy.com/v2/${{ secrets.ALCHEMY_MAINNET_KEY }}"
FORK_RPC_BASE: "https://base-mainnet.g.alchemy.com/v2/${{ secrets.ALCHEMY_BASE_KEY }}"
FORK_RPC_BSC: "https://bnb-mainnet.g.alchemy.com/v2/${{ secrets.ALCHEMY_MAINNET_KEY }}"Impact:
BSC-specific migration tests may not execute reliably in CI if the mainnet Alchemy key is not enabled for BNB. This is a coverage/configuration issue, not a production contract issue.
PoC Result:
Verification classified this as config-only coverage risk. The audit environment did not have the secret value, so endpoint authorization was not proven or disproven.
Recommendation:
Use a dedicated BNB-capable secret, for example ALCHEMY_BSC_KEY, and make the workflow fail clearly if BSC fork configuration is absent.
[I-03] Cancun EVM target change needs deployment-chain compatibility confirmation [CONFIG]
Severity: Informational
Location: foundry.toml:14
Confidence: LOW (multiple analysis sources confirmed; Static Analysis: N; PoC: SKIPPED)
Description:
The changed Foundry configuration sets the compiler EVM target to Cancun.
evm_version = "cancun"The PR includes fork coverage for Ethereum, Base, and BSC, but the audit did not verify deployment-chain bytecode compatibility or whether every target chain and deployment environment supports Cancun-targeted bytecode at the intended execution block.
Impact:
If a target deployment chain or tooling path is not Cancun-compatible, deployment or fork execution can fail for configuration reasons. No production exploit path was identified, and forge build passed.
PoC Result:
Verification classified this as deployment-compatibility/config-only. Fork tests did not run locally due missing RPC environment variables, so chain-specific compatibility was not mechanically confirmed.
Recommendation:
Document target fork/EVM support assumptions for Ethereum, Base, and BSC, and run chain-specific fork tests in CI with the same evm_version before executing the spell.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a governance upgrade spell contract with deploy/upgrade/retire entrypoints, updates CI and tooling for BSC forks and Cancun EVM, and adds forked tests across Ethereum, Base, and BSC validating deployment, fee-recipient migration, governance wiring, proposal behavior, and old-vault retirement. ChangesGovernance Spell Upgrade & Testing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/spells/GovernanceSpell_04_17_2026/GenericGovernanceSpell_04_17_2026.t.sol (1)
325-340: ⚡ Quick winAdd one reject-path assertion for optimistic selectors.
This only proves that the whitelisted
startRebalancepath succeeds. It never proves that an authorized optimistic proposer is blocked from usingproposeOptimisticon any other Folio selector, so a selector-registry widening regression would still pass this suite. A single negative case here with something likeFolio.setNamewould close that gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/spells/GovernanceSpell_04_17_2026/GenericGovernanceSpell_04_17_2026.t.sol` around lines 325 - 340, Add a negative test that ensures optimistic proposers cannot use proposeOptimistic for non-whitelisted selectors: build calldata for Folio.setName (use _singleCall(address(folio), 0, _setNameCalldata() / appropriate bytes) to get targets/values/calldatas), then vm.prank(optimisticProposer) and assert that governor.proposeOptimistic(...) reverts or is rejected for that proposal (i.e., does not create an optimistic proposal); reference functions/identifiers: Folio.setName, _singleCall, governor.proposeOptimistic, and optimisticProposer.contracts/spells/GovernanceSpell_04_17_2026.sol (1)
220-228: Bundle the ownership handoff and retirement in the same governance action.Once ownership has been transferred to the spell, this public method is callable by anyone. Since the “all dependent Folios are upgraded” check is explicitly off-chain, splitting those two steps across transactions leaves a public race window.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/spells/GovernanceSpell_04_17_2026.sol` around lines 220 - 228, The retireOldStakingVault function is public and, once ownership has been transferred to the spell, can be invoked by anyone—creating a race between ownership handoff and retirement; restrict this so retirement only occurs as part of the same authorized governance action. Update retireOldStakingVault (and any caller path) to require an on-chain governance-only caller (use the contract's existing governance-only modifier or access-control check) or make the retirement internal and invoke it atomically from the governance execution entrypoint so the IOwnableStakingVault.owner() == address(this) check and the calls setUnstakingDelay, setRewardRatio, renounceOwnership, and emit StakingVaultRetired happen in the same authorized transaction. Ensure you reference the IOwnableStakingVault instance and the retireOldStakingVault function when applying the access-control change so the handoff and retirement cannot be split across transactions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/spells/GovernanceSpell_04_17_2026.sol`:
- Around line 175-177: upgradeFolio() currently trusts
governorDeployer.deployWithExistingStakingVault(...) and proceeds to rotate
Folio ownership without verifying the returned governor wiring; add explicit
runtime checks after the deploy call to confirm newDeployment.newGovernor is
non-zero and that the on-chain governor actually points to
newDeployment.newTimelock and newDeployment.newStakingVault (and optionally
newSelectorRegistry) before performing any proxy/admin transfers. Implement this
by calling the governor's public view/accessor functions (e.g., the governor
contract's timelock and stakingVault/accessor methods) or reading the expected
storage via the governor interface, compare them to newDeployment.newTimelock
and address(newStakingVault), and revert with UpgradeError if they mismatch;
keep these checks immediately after deployWithExistingStakingVault and before
any ownership/role rotations.
- Around line 317-332: The loop currently reverts if newStakingVault is already
in recipients; instead detect when recipient == newStakingVault and merge the
oldStakingVault's fee share into that existing entry: track both
oldStakingVaultRecipientIndex and newStakingVaultRecipientIndex (or a flag when
found), after the loop require oldStakingVault exists, then if newStakingVault
was found add recipients[oldStakingVaultRecipientIndex].portion (or equivalent
amount field) to recipients[newStakingVaultRecipientIndex].portion, remove the
oldStakingVault entry from recipients (or collapse entries appropriately), then
call _sortFeeRecipients(recipients) and folio.setFeeRecipients(recipients);
otherwise when newStakingVault not present, preserve the current replacement
logic that assigns recipients[oldStakingVaultRecipientIndex].recipient =
newStakingVault before sorting and setting.
---
Nitpick comments:
In `@contracts/spells/GovernanceSpell_04_17_2026.sol`:
- Around line 220-228: The retireOldStakingVault function is public and, once
ownership has been transferred to the spell, can be invoked by anyone—creating a
race between ownership handoff and retirement; restrict this so retirement only
occurs as part of the same authorized governance action. Update
retireOldStakingVault (and any caller path) to require an on-chain
governance-only caller (use the contract's existing governance-only modifier or
access-control check) or make the retirement internal and invoke it atomically
from the governance execution entrypoint so the IOwnableStakingVault.owner() ==
address(this) check and the calls setUnstakingDelay, setRewardRatio,
renounceOwnership, and emit StakingVaultRetired happen in the same authorized
transaction. Ensure you reference the IOwnableStakingVault instance and the
retireOldStakingVault function when applying the access-control change so the
handoff and retirement cannot be split across transactions.
In
`@test/spells/GovernanceSpell_04_17_2026/GenericGovernanceSpell_04_17_2026.t.sol`:
- Around line 325-340: Add a negative test that ensures optimistic proposers
cannot use proposeOptimistic for non-whitelisted selectors: build calldata for
Folio.setName (use _singleCall(address(folio), 0, _setNameCalldata() /
appropriate bytes) to get targets/values/calldatas), then
vm.prank(optimisticProposer) and assert that governor.proposeOptimistic(...)
reverts or is rejected for that proposal (i.e., does not create an optimistic
proposal); reference functions/identifiers: Folio.setName, _singleCall,
governor.proposeOptimistic, and optimisticProposer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c7c528f6-7e9a-46b6-a88e-85148aefb4f3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.github/workflows/test.ymlcontracts/spells/GovernanceSpell_04_17_2026.solfoundry.tomlpackage.jsonremappings.txttest/base/BaseTest.soltest/spells/GovernanceSpell_04_17_2026/GenericGovernanceSpell_04_17_2026.t.soltest/spells/GovernanceSpell_04_17_2026/GovernanceSpellBase_04_17_2026.t.soltest/spells/GovernanceSpell_04_17_2026/GovernanceSpellBsc_04_17_2026.t.soltest/spells/GovernanceSpell_04_17_2026/GovernanceSpellEthereum_04_17_2026.t.sol
| (newDeployment.newGovernor, newDeployment.newTimelock, newDeployment.newSelectorRegistry) = governorDeployer | ||
| .deployWithExistingStakingVault(baseParams, address(newStakingVault), deploymentNonce); | ||
| require(newDeployment.newTimelock != address(0), UpgradeError(2)); |
There was a problem hiding this comment.
Validate the returned governor wiring before rotating Folio ownership.
upgradeFolio() trusts deployWithExistingStakingVault() and immediately starts moving proxy/admin control, but it never checks that the returned governor actually points at newTimelock and newStakingVault. If the deployer returns a mismatched pair, this can complete the handoff to an unusable governance setup.
Suggested guardrails
(newDeployment.newGovernor, newDeployment.newTimelock, newDeployment.newSelectorRegistry) = governorDeployer
.deployWithExistingStakingVault(baseParams, address(newStakingVault), deploymentNonce);
require(newDeployment.newTimelock != address(0), UpgradeError(2));
+ require(IFolioGovernor(newDeployment.newGovernor).timelock() == newDeployment.newTimelock, UpgradeError(25));
+ require(IFolioGovernor(newDeployment.newGovernor).token() == address(newStakingVault), UpgradeError(28));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (newDeployment.newGovernor, newDeployment.newTimelock, newDeployment.newSelectorRegistry) = governorDeployer | |
| .deployWithExistingStakingVault(baseParams, address(newStakingVault), deploymentNonce); | |
| require(newDeployment.newTimelock != address(0), UpgradeError(2)); | |
| (newDeployment.newGovernor, newDeployment.newTimelock, newDeployment.newSelectorRegistry) = governorDeployer | |
| .deployWithExistingStakingVault(baseParams, address(newStakingVault), deploymentNonce); | |
| require(newDeployment.newTimelock != address(0), UpgradeError(2)); | |
| require(IFolioGovernor(newDeployment.newGovernor).timelock() == newDeployment.newTimelock, UpgradeError(25)); | |
| require(IFolioGovernor(newDeployment.newGovernor).token() == address(newStakingVault), UpgradeError(28)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/spells/GovernanceSpell_04_17_2026.sol` around lines 175 - 177,
upgradeFolio() currently trusts
governorDeployer.deployWithExistingStakingVault(...) and proceeds to rotate
Folio ownership without verifying the returned governor wiring; add explicit
runtime checks after the deploy call to confirm newDeployment.newGovernor is
non-zero and that the on-chain governor actually points to
newDeployment.newTimelock and newDeployment.newStakingVault (and optionally
newSelectorRegistry) before performing any proxy/admin transfers. Implement this
by calling the governor's public view/accessor functions (e.g., the governor
contract's timelock and stakingVault/accessor methods) or reading the expected
storage via the governor interface, compare them to newDeployment.newTimelock
and address(newStakingVault), and revert with UpgradeError if they mismatch;
keep these checks immediately after deployWithExistingStakingVault and before
any ownership/role rotations.
| for (uint256 i; i < recipients.length; i++) { | ||
| address recipient = recipients[i].recipient; | ||
|
|
||
| if (recipient == oldStakingVault) { | ||
| oldStakingVaultRecipientCount++; | ||
| oldStakingVaultRecipientIndex = i; | ||
| } | ||
|
|
||
| require(recipient != newStakingVault, UpgradeError(19)); | ||
| } | ||
|
|
||
| require(oldStakingVaultRecipientCount == 1, UpgradeError(20)); | ||
|
|
||
| recipients[oldStakingVaultRecipientIndex].recipient = newStakingVault; | ||
| _sortFeeRecipients(recipients); | ||
| folio.setFeeRecipients(recipients); |
There was a problem hiding this comment.
Merge an existing successor-vault fee share instead of hard-reverting.
This path reverts as soon as newStakingVault is already present in feeRecipients, which blocks any rollout that pre-seeds the successor vault with a fee share before the cutover. The surrounding test helper already tracks newVaultFeePortionBefore, so this looks like a supported scenario rather than an invalid one.
One way to collapse the old/new entries into a single recipient
function _rotateFeeRecipients(Folio folio, address oldStakingVault, address newStakingVault) internal {
IFolio.FeeRecipient[] memory recipients = _feeRecipients(folio);
- uint256 oldStakingVaultRecipientCount;
- uint256 oldStakingVaultRecipientIndex;
+ uint256 oldStakingVaultRecipientCount;
+ uint96 migratedPortion;
+ uint96 existingNewPortion;
for (uint256 i; i < recipients.length; i++) {
address recipient = recipients[i].recipient;
if (recipient == oldStakingVault) {
oldStakingVaultRecipientCount++;
- oldStakingVaultRecipientIndex = i;
+ migratedPortion += recipients[i].portion;
}
-
- require(recipient != newStakingVault, UpgradeError(19));
+ if (recipient == newStakingVault) {
+ existingNewPortion += recipients[i].portion;
+ }
}
require(oldStakingVaultRecipientCount == 1, UpgradeError(20));
- recipients[oldStakingVaultRecipientIndex].recipient = newStakingVault;
- _sortFeeRecipients(recipients);
- folio.setFeeRecipients(recipients);
+ IFolio.FeeRecipient[] memory nextRecipients =
+ new IFolio.FeeRecipient[](existingNewPortion == 0 ? recipients.length : recipients.length - 1);
+ uint256 k;
+ for (uint256 i; i < recipients.length; i++) {
+ address recipient = recipients[i].recipient;
+ if (recipient == oldStakingVault || recipient == newStakingVault) continue;
+ nextRecipients[k++] = recipients[i];
+ }
+ nextRecipients[k] = IFolio.FeeRecipient({
+ recipient: newStakingVault,
+ portion: migratedPortion + existingNewPortion
+ });
+ _sortFeeRecipients(nextRecipients);
+ folio.setFeeRecipients(nextRecipients);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/spells/GovernanceSpell_04_17_2026.sol` around lines 317 - 332, The
loop currently reverts if newStakingVault is already in recipients; instead
detect when recipient == newStakingVault and merge the oldStakingVault's fee
share into that existing entry: track both oldStakingVaultRecipientIndex and
newStakingVaultRecipientIndex (or a flag when found), after the loop require
oldStakingVault exists, then if newStakingVault was found add
recipients[oldStakingVaultRecipientIndex].portion (or equivalent amount field)
to recipients[newStakingVaultRecipientIndex].portion, remove the oldStakingVault
entry from recipients (or collapse entries appropriately), then call
_sortFeeRecipients(recipients) and folio.setFeeRecipients(recipients); otherwise
when newStakingVault not present, preserve the current replacement logic that
assigns recipients[oldStakingVaultRecipientIndex].recipient = newStakingVault
before sorting and setting.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/spells/GovernanceSpell_04_17_2026/GenericGovernanceSpell_04_17_2026.t.sol (1)
580-593: ⚡ Quick winFail fast on unsupported folio versions.
This helper currently routes every non-
4.0.0folio through the newerstartRebalanceABI. That weakens the spell’s4.0.0/5.0.0restriction, because a future config on some other version could still pass this harness if the selector happens to match.Suggested change
function _startRebalanceCalldata(Folio folio) internal view returns (bytes memory calldata_) { + bytes32 version = _folioVersion(folio); IFolio.RebalanceLimits memory limits = IFolio.RebalanceLimits({ low: 1, spot: 1, high: 1 }); - if (_folioVersion(folio) == FOLIO_VERSION_4_0_0) { + if (version == FOLIO_VERSION_4_0_0) { address[] memory v4Tokens = new address[](0); IFolio.WeightRange[] memory weights = new IFolio.WeightRange[](0); IFolio.PriceRange[] memory prices = new IFolio.PriceRange[](0); return abi.encodeWithSelector(START_REBALANCE_4_0_0, v4Tokens, weights, prices, limits, 0, 1); } + + assertEq(version, keccak256("5.0.0"), "unsupported folio version"); IFolio.TokenRebalanceParams[] memory tokens = new IFolio.TokenRebalanceParams[](0); calldata_ = abi.encodeCall(Folio.startRebalance, (tokens, limits, 0, 1)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/spells/GovernanceSpell_04_17_2026/GenericGovernanceSpell_04_17_2026.t.sol` around lines 580 - 593, The helper _startRebalanceCalldata currently treats any non-FOLIO_VERSION_4_0_0 as the newer ABI, which allows unexpected folio versions to slip through; update _startRebalanceCalldata to explicitly branch on supported versions only: keep the existing branch for FOLIO_VERSION_4_0_0 (using START_REBALANCE_4_0_0), add an explicit branch for the supported newer version (e.g., FOLIO_VERSION_5_0_0) that returns abi.encodeCall(Folio.startRebalance, (tokens, limits, 0, 1)), and otherwise revert (or require) with a clear error for unsupported folio versions; use _folioVersion, FOLIO_VERSION_4_0_0, FOLIO_VERSION_5_0_0, START_REBALANCE_4_0_0 and Folio.startRebalance to locate and implement the checks.test/spells/GovernanceSpell_04_17_2026/GovernanceSpellEthereum_04_17_2026.t.sol (1)
163-243: ⚡ Quick winExercise proposal creation on the shared-vault path too.
This test proves migration and role wiring, but it never verifies that either newly deployed folio governor can still create both standard and optimistic proposals after both folios point at the same successor vault. A shared-vault-specific wiring bug would still pass here.
Suggested change
- GovernanceSpell_04_17_2026.NewDeployment memory firstFolioDep = _upgradeFolio( + address firstOptimisticProposer = makeAddr(string.concat(firstLabel, "-folio-opt")); + address firstStandardProposer = makeAddr(string.concat(firstLabel, "-std")); + GovernanceSpell_04_17_2026.NewDeployment memory firstFolioDep = _upgradeFolio( firstCfg, IStakingVault(stakingVaultDep.newStakingVault), - makeAddr(string.concat(firstLabel, "-folio-opt")), + firstOptimisticProposer, keccak256(abi.encode(firstLabel, "folio")) ); @@ assertEq( IFolioGovernor(firstFolioDep.newGovernor).token(), stakingVaultDep.newStakingVault, string.concat(firstLabel, " folio governor should use the upgraded staking vault") ); + _assertCanCreateBothProposalTypes( + IReserveOptimisticGovernorLike(firstFolioDep.newGovernor), + IStakingVault(stakingVaultDep.newStakingVault), + firstCfg.folio, + firstStandardProposer, + firstOptimisticProposer + ); @@ - GovernanceSpell_04_17_2026.NewDeployment memory secondFolioDep = _upgradeFolio( + address secondOptimisticProposer = makeAddr(string.concat(secondLabel, "-folio-opt")); + address secondStandardProposer = makeAddr(string.concat(secondLabel, "-std")); + GovernanceSpell_04_17_2026.NewDeployment memory secondFolioDep = _upgradeFolio( secondCfg, IStakingVault(stakingVaultDep.newStakingVault), - makeAddr(string.concat(secondLabel, "-folio-opt")), + secondOptimisticProposer, keccak256(abi.encode(secondLabel, "folio")) ); @@ assertEq( IFolioGovernor(secondFolioDep.newGovernor).token(), stakingVaultDep.newStakingVault, string.concat(secondLabel, " folio governor should use the upgraded staking vault") ); + _assertCanCreateBothProposalTypes( + IReserveOptimisticGovernorLike(secondFolioDep.newGovernor), + IStakingVault(stakingVaultDep.newStakingVault), + secondCfg.folio, + secondStandardProposer, + secondOptimisticProposer + );
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@test/spells/GovernanceSpell_04_17_2026/GenericGovernanceSpell_04_17_2026.t.sol`:
- Around line 580-593: The helper _startRebalanceCalldata currently treats any
non-FOLIO_VERSION_4_0_0 as the newer ABI, which allows unexpected folio versions
to slip through; update _startRebalanceCalldata to explicitly branch on
supported versions only: keep the existing branch for FOLIO_VERSION_4_0_0 (using
START_REBALANCE_4_0_0), add an explicit branch for the supported newer version
(e.g., FOLIO_VERSION_5_0_0) that returns abi.encodeCall(Folio.startRebalance,
(tokens, limits, 0, 1)), and otherwise revert (or require) with a clear error
for unsupported folio versions; use _folioVersion, FOLIO_VERSION_4_0_0,
FOLIO_VERSION_5_0_0, START_REBALANCE_4_0_0 and Folio.startRebalance to locate
and implement the checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 396c6099-7d43-4f7c-88b8-933340a9faa1
📒 Files selected for processing (5)
contracts/spells/GovernanceSpell_04_17_2026.soltest/spells/GovernanceSpell_04_17_2026/GenericGovernanceSpell_04_17_2026.t.soltest/spells/GovernanceSpell_04_17_2026/GovernanceSpellBase_04_17_2026.t.soltest/spells/GovernanceSpell_04_17_2026/GovernanceSpellBsc_04_17_2026.t.soltest/spells/GovernanceSpell_04_17_2026/GovernanceSpellEthereum_04_17_2026.t.sol
🚧 Files skipped from review as they are similar to previous changes (2)
- test/spells/GovernanceSpell_04_17_2026/GovernanceSpellBsc_04_17_2026.t.sol
- contracts/spells/GovernanceSpell_04_17_2026.sol
2 participants
Do not merge into
mainSummary by CodeRabbit
New Features
Tests
Chores