feat: savings donation-attack guard + parallelizer pause refactor - #27
Merged
Merged
Conversation
The audit/eip3009-hotfix-fees branch grew the Storage struct (Surplus fields) past the point where Yul can route stack variables through EVM stack slots in a few code paths, surfacing as "Variable size is 1 too deep in the stack" during compilation. Disabling via_ir at the default profile sidesteps the issue across the whole compilation graph. The dev and ci profiles already had via_ir = false, so this only changes the unprofiled forge invocation (the one CI and most local runs use).
- SECONDS_PER_YEAR (= 365 days) replaces the hardcoded 31_536_000 literal in Savings.estimatedAPY(). - AlreadyPaused / NotPaused errors enable explicit pause/unpause flows that revert on no-op governance calls instead of silently toggling.
Mirrors the Savings refactor from this branch on the per-collateral governance pause exposed by SettersGuardian. The `togglePause(collateral, action)` entry point flipped the live flag without checking the current state, so a misclicked governance call could silently invert the pause status of a critical action — the only audit signal was the emitted event. Changes: - LibSetters: new `pause(collateral, action)` and `unpause(collateral, action)` internals share a private `_setPauseState` helper. Both revert with `AlreadyPaused` / `NotPaused` on no-op transitions; the existing `PauseToggled` event is preserved for backward compatibility with indexers. - SettersGuardian: replaces the external `togglePause` with `pause` and `unpause`. - ISettersGuardian: interface updated accordingly. - DiamondInitializer / Test config: previous `togglePause` calls enabled mint/burn/redeem at deploy time, so they map cleanly to `unpause` (set the live flag from 0 to 1). - Selector wiring: `tests/utils/ConfigAccessManager.sol` and `scripts/SetParallelizerRoles.s.sol` updated to register both new selectors under the guardian role. - Generated dummy diamond implementations regenerated with the new signatures. - Tests updated to use the new API. The two no-op double-toggles in Redeem.t.sol that relied on legacy toggle semantics are kept as a pause+unpause sequence so they're now explicit about being a no-op.
The audit/eip3009-hotfix-fees branch grew the Storage struct (Surplus fields) past the point where Yul can route stack variables through EVM stack slots in a few code paths, surfacing as "Variable size is 1 too deep in the stack" during compilation. Disabling via_ir at the default profile sidesteps the issue across the whole compilation graph. The dev and ci profiles already had via_ir = false, so this only changes the unprofiled forge invocation (the one CI and most local runs use).
- SECONDS_PER_YEAR (= 365 days) replaces the hardcoded 31_536_000 literal in Savings.estimatedAPY(). - AlreadyPaused / NotPaused errors enable explicit pause/unpause flows that revert on no-op governance calls instead of silently toggling.
The Savings ERC4626 vault was vulnerable to donation/inflation attacks because totalAssets() read the raw IERC20.balanceOf(self), allowing any direct USDp transfer to inflate share value pro-rata to all holders. At low TVL or after long dormancy this turned even a 1-share position into a captureable windfall. Mitigation: - New storedAssets state variable (consumes 1 slot from __gap[48] -> [47]) tracks the only legitimate backing: deposits in, withdrawals out, accrued interest minted in. - totalAssets() now projects storedAssets through the existing rate formula instead of the raw ERC20 balance. - _accrue() reads from and writes to storedAssets so dormant accruals are properly accounted, never reclassified as surplus. - _deposit / _withdraw overridden to mirror standard ERC4626 movements into storedAssets. - depositWithAuthorization and _redeemWithAuthorization (EIP-3009 paths that bypass _deposit/_withdraw) update storedAssets directly around the receiveWithAuthorization / safeTransfer calls so the same backing accounting holds across all entry points. - initializeStoredAssets() reinitializer(2) seeds storedAssets from the current balance on upgrade so existing depositors remain backed. - recoverSurplus(to) restricted hatch transfers any balanceOf(self) - storedAssets delta (donations, accidental transfers) to a destination chosen by governance, with no effect on share holders. Companion refactors in the same surface: - togglePause split into pause()/unpause() with idempotence checks (revert AlreadyPaused / NotPaused) so a misclicked governance call cannot silently flip the pause state. - estimatedAPR renamed to estimatedAPY to match the per-second compounding semantics of _computeUpdatedAssets. - Magic literal 31_536_000 replaced by SECONDS_PER_YEAR constant. Operational note: the deployment must call upgradeToAndCall with abi.encodeWithSelector(initializeStoredAssets.selector) atomically. Skipping the reinitializer would leave storedAssets at 0, blocking all subsequent withdrawals.
Donation attack invariant fuzz lives next to the existing rate / deposit / pause fuzz tests in tests/fuzz/Savings.t.sol: - testFuzz_DonationAttackInflationLockedDown: invariant fuzz over donation amount up to 1B USDp — preview and redeem outcomes are unchanged by direct ERC20 transfers. Deterministic coverage moves to tests/units/Savings.t.sol in two contracts: - SavingsUpgradeTest groups the existing fresh-deploy + upgradeTo cases with the new upgrade-from-legacy path. Cases: donation attack succeeds on legacy then is neutralized after upgrade; existing depositors still redeem; initializeStoredAssets reinitializer cannot be reused; pause/unpause selectors function once wired into the access manager. - SavingsDonationAttackTest covers the storedAssets fix, the recoverSurplus governance hatch (drain, zero-address guard, unauthorised caller, no-donation case), the dormant-accrual accounting and the new pause/unpause idempotence checks. The upgrade path is exercised through ERC1967Proxy.upgradeToAndCall backed by a SavingsLegacyMock that mirrors the pre-fix storage layout and behaviour (BaseSavings + SavingsEIP3009 inheritance, __gap[48], no storedAssets slot). The mock is confined to tests/mock/ and never deployed in production paths.
…ause refacto(parallelizer): split togglePause into pause and unpause
…tack fix(savings): track storedAssets to prevent donation attacks
…int max-line-length
…pendence The redemption penalty is evaluated at entry-time CR on the full amount (Cyfrin I-6) and governance may install non-flat fee curves, so single-path vs split-path outcomes legitimately diverge. Cap per-call redeem CR excursion (<=5% of issuance) and widen path-independence tolerances to 1% (balances) / 0.2% (CR). Validated 25/25 runs green under FOUNDRY_PROFILE=ci.
…savings-donation Cyfrin Savings Fix — audit remediation (L-1, L-2, I-3, I-4)
…overnor for atomic upgrade
…cannot mint paused yield
…toredAssets after non-atomic upgrade
…orized deposit/redeem
…-savings-donation Bailsec Parallel Savings Fix — audit remediation (Issues 01-05, 07)
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.
Overview
Aggregation branch ("code to audit") combining two features, received via PR:
refacto(parallelizer): splittogglePauseintopause/unpausefix(savings): donation-attack guardChanges
Savings (donation-attack guard)
storedAssetstracks the real backing;totalAssets()projectsstoredAssetsinstead of the rawbalanceOf→ neutralizes donation/inflation attacks.recoverSurplus(to)recovers the untracked surplus (scope limited toasset()).togglePausesplit intopause()/unpause()with idempotence guards (AlreadyPaused/NotPaused).pause()settles yield up to the pause moment;unpause()drops the paused interval (pausing halts emission).ISavingsaligned (pause/unpause,estimatedAPY,recoverSurplus).initializeStoredAssets()reinitializer(2) to seed on upgrade —restrictedand bounded by alastUpdatefreshness window.Parallelizer (pause refactor)
togglePause(collateral, ActionType)split into explicitpause/unpause.Shared foundation
SECONDS_PER_YEARconstant,AlreadyPaused/NotPausederrors, defaultvia_ir = false.Audit remediation (Cyfrin — Parallel Savings Fix)
Final report (v2.0, 2026-06-09): all findings Resolved/Acknowledged.
Code changes addressing the report's findings:
initializeStoredAssets—initializeStoredAssets()now reverts withStaleAccrual()whenblock.timestamp - lastUpdate > MAX_STORED_ASSETS_INIT_STALENESS(30 min). With a fresh accrual there is no unaccrued-interest delta to compound against an inflatedstoredAssets. Operational step: refresh accrual (setRate) immediately before upgrading.unpause()now advanceslastUpdateto the unpause timestamp instead of_accrue()-ing the paused span, so the paused interval is dropped and never minted. Pausing now genuinely halts emission.initializeStoredAssetsaccess control — added therestrictedmodifier; only authorized governance can seedstoredAssets, independent of whether the upgrade runs atomically.initializedid not setlastUpdate—initializenow setslastUpdate = uint40(block.timestamp), bounding the first accrual window to "time since deployment" rather than "time since Unix epoch".Operational guidance (no code defect — process to follow):
_accrue()mintsTokenP, so it reverts once the AccessManager revokes the vault's minter role. When winding down, callsetRate(0)(final_accrue()succeeds) before revoking the minter role — never after, or the disabling call traps the rate.safeTransfer. After the donation guard, USDp transferred in is excluded fromtotalAssets()and is sweepable byrecoverSurplus— it never reaches savers. Saver yield is delivered only by minting atsetRate. Savings must never appear inupdatePayees/ts.payees.Pre-upgrade deployment sequence (from the report's Post-Audit Recommendations):
initializeStoredAssets.setRateimmediately before the upgrade to refresh accrual (lastUpdate≈ now), so the L-1 staleness guard passes and stale-accrual time is ~0.Tests: added
initializeStoredAssetsaccess-control + staleness coverage and updated the pause/unpause andlastUpdateassertions for the new behavior.Tests
forge buildOK. Full suite green on the source branches (372 Savings tests included). Verification on the merged state in progress.Note
PR intended for audit — do not merge until the audit is finalized.
🤖 Generated with Claude Code