diff --git a/contracts/interfaces/ISavings.sol b/contracts/interfaces/ISavings.sol index fe73627..014223c 100644 --- a/contracts/interfaces/ISavings.sol +++ b/contracts/interfaces/ISavings.sol @@ -62,15 +62,19 @@ interface ISavings is IEIP3009 { // solhint-disable-next-line func-name-mixedcase function DOMAIN_SEPARATOR() external view returns (bytes32); - function estimatedAPR() external view returns (uint256 apr); + function estimatedAPY() external view returns (uint256 apy); function computeUpdatedAssets(uint256 totalAssets, uint256 exp) external view returns (uint256); - function togglePause() external; + function pause() external; + + function unpause() external; function toggleTrusted(address trustedAddress) external; function setRate(uint208 newRate) external; function setMaxRate(uint256 newMaxRate) external; + + function recoverSurplus(address to) external returns (uint256 surplus); } diff --git a/contracts/interfaces/ISetters.sol b/contracts/interfaces/ISetters.sol index 8f841a6..17b514b 100644 --- a/contracts/interfaces/ISetters.sol +++ b/contracts/interfaces/ISetters.sol @@ -78,8 +78,13 @@ interface ISettersGovernor { /// @author Cooper Labs /// @custom:contact security@cooperlabs.xyz interface ISettersGuardian { - /// @notice Changes the pause status for mint or burn transactions for `collateral` - function togglePause(address collateral, ActionType action) external; + /// @notice Pauses `action` for `collateral` (or pauses redemption protocol-wide when + /// `action == Redeem`). Reverts if the action is already paused. + function pause(address collateral, ActionType action) external; + + /// @notice Unpauses `action` for `collateral` (or unpauses redemption protocol-wide when + /// `action == Redeem`). Reverts if the action is not paused. + function unpause(address collateral, ActionType action) external; /// @notice Sets the mint or burn fees for `collateral` function setFees(address collateral, uint64[] memory xFee, int64[] memory yFee, bool mint) external; diff --git a/contracts/parallelizer/configs/DiamondInitializer.sol b/contracts/parallelizer/configs/DiamondInitializer.sol index 45d0234..7f1c428 100644 --- a/contracts/parallelizer/configs/DiamondInitializer.sol +++ b/contracts/parallelizer/configs/DiamondInitializer.sol @@ -30,15 +30,15 @@ contract DiamondInitializer { LibSetters.setFees(collateral.token, collateral.xMintFee, collateral.yMintFee, true); // Burn fees LibSetters.setFees(collateral.token, collateral.xBurnFee, collateral.yBurnFee, false); - LibSetters.togglePause(collateral.token, ActionType.Mint); - LibSetters.togglePause(collateral.token, ActionType.Burn); + LibSetters.unpause(collateral.token, ActionType.Mint); + LibSetters.unpause(collateral.token, ActionType.Burn); LibSetters.setStablecoinCap(collateral.token, 100_000_000 ether); if (collateral.targetMax) LibOracle.updateOracle(collateral.token); } // setRedemptionCurveParams if (_redemptionSetup.xRedeemFee.length > 0) { - LibSetters.togglePause(address(0), ActionType.Redeem); + LibSetters.unpause(address(0), ActionType.Redeem); LibSetters.setRedemptionCurveParams(_redemptionSetup.xRedeemFee, _redemptionSetup.yRedeemFee); } } diff --git a/contracts/parallelizer/configs/Test.sol b/contracts/parallelizer/configs/Test.sol index 0559c10..0902ddc 100644 --- a/contracts/parallelizer/configs/Test.sol +++ b/contracts/parallelizer/configs/Test.sol @@ -69,7 +69,7 @@ contract Test { yMintFee[3] = int64(uint64(BASE_12 - 1)); LibSetters.setFees(eurA.collateral, xMintFee, yMintFee, true); - LibSetters.togglePause(eurA.collateral, ActionType.Mint); + LibSetters.unpause(eurA.collateral, ActionType.Mint); uint64[] memory xBurnFee = new uint64[](4); xBurnFee[0] = uint64(BASE_9); @@ -85,7 +85,7 @@ contract Test { yBurnFee[3] = int64(uint64(MAX_BURN_FEE - 1)); LibSetters.setFees(eurA.collateral, xBurnFee, yBurnFee, false); - LibSetters.togglePause(eurA.collateral, ActionType.Burn); + LibSetters.unpause(eurA.collateral, ActionType.Burn); // Setup second collateral LibSetters.addCollateral(eurB.collateral); @@ -121,7 +121,7 @@ contract Test { yMintFee[3] = int64(uint64(BASE_12 - 1)); LibSetters.setFees(eurB.collateral, xMintFee, yMintFee, true); - LibSetters.togglePause(eurB.collateral, ActionType.Mint); + LibSetters.unpause(eurB.collateral, ActionType.Mint); xBurnFee = new uint64[](4); xBurnFee[0] = uint64(BASE_9); @@ -137,7 +137,7 @@ contract Test { yBurnFee[3] = int64(uint64(MAX_BURN_FEE - 1)); LibSetters.setFees(eurB.collateral, xBurnFee, yBurnFee, false); - LibSetters.togglePause(eurB.collateral, ActionType.Burn); + LibSetters.unpause(eurB.collateral, ActionType.Burn); // Setup third collateral LibSetters.addCollateral(eurY.collateral); @@ -173,7 +173,7 @@ contract Test { yMintFee[3] = int64(uint64(BASE_12 - 1)); LibSetters.setFees(eurY.collateral, xMintFee, yMintFee, true); - LibSetters.togglePause(eurY.collateral, ActionType.Mint); + LibSetters.unpause(eurY.collateral, ActionType.Mint); xBurnFee = new uint64[](4); xBurnFee[0] = uint64(BASE_9); @@ -189,7 +189,7 @@ contract Test { yBurnFee[3] = int64(uint64(MAX_BURN_FEE - 1)); LibSetters.setFees(eurY.collateral, xBurnFee, yBurnFee, false); - LibSetters.togglePause(eurY.collateral, ActionType.Burn); + LibSetters.unpause(eurY.collateral, ActionType.Burn); // Set no hard limits on stablecoin minting per collateral LibSetters.setStablecoinCap(eurA.collateral, type(uint256).max); @@ -197,6 +197,6 @@ contract Test { LibSetters.setStablecoinCap(eurY.collateral, type(uint256).max); // Redeem - LibSetters.togglePause(eurA.collateral, ActionType.Redeem); + LibSetters.unpause(eurA.collateral, ActionType.Redeem); } } diff --git a/contracts/parallelizer/facets/SettersGuardian.sol b/contracts/parallelizer/facets/SettersGuardian.sol index a6a3a65..809fec4 100644 --- a/contracts/parallelizer/facets/SettersGuardian.sol +++ b/contracts/parallelizer/facets/SettersGuardian.sol @@ -15,8 +15,13 @@ import "../Storage.sol"; /// https://github.com/AngleProtocol/angle-transmuter/blob/main/contracts/transmuter/facets/SettersGuardian.sol contract SettersGuardian is AccessManagedModifiers, ISettersGuardian { /// @inheritdoc ISettersGuardian - function togglePause(address collateral, ActionType pausedType) external restricted { - LibSetters.togglePause(collateral, pausedType); + function pause(address collateral, ActionType action) external restricted { + LibSetters.pause(collateral, action); + } + + /// @inheritdoc ISettersGuardian + function unpause(address collateral, ActionType action) external restricted { + LibSetters.unpause(collateral, action); } /// @inheritdoc ISettersGuardian diff --git a/contracts/parallelizer/libraries/LibSetters.sol b/contracts/parallelizer/libraries/LibSetters.sol index 1d2cc9f..7c72224 100644 --- a/contracts/parallelizer/libraries/LibSetters.sol +++ b/contracts/parallelizer/libraries/LibSetters.sol @@ -183,25 +183,45 @@ library LibSetters { ONLY GUARDIAN ACTIONS //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ - /// @notice Internal version of `togglePause` - function togglePause(address collateral, ActionType action) internal { - uint8 isLive; + /// @notice Internal version of `pause` — sets the action's live flag to `0` + /// @dev Reverts with `AlreadyPaused` if the action is already paused so a no-op governance + /// call cannot pass silently + function pause(address collateral, ActionType action) internal { + _setPauseState(collateral, action, 0); + } + + /// @notice Internal version of `unpause` — sets the action's live flag to `1` + /// @dev Reverts with `NotPaused` if the action is already unpaused so a no-op governance + /// call cannot pass silently + function unpause(address collateral, ActionType action) internal { + _setPauseState(collateral, action, 1); + } + + /// @dev Shared accessor for `pause` and `unpause`. `targetIsLive` is `0` to pause and `1` + /// to unpause. Reverts on no-op transitions. + function _setPauseState(address collateral, ActionType action, uint8 targetIsLive) private { if (action == ActionType.Mint || action == ActionType.Burn) { Collateral storage collatInfo = s.transmuterStorage().collaterals[collateral]; if (collatInfo.decimals == 0) revert NotCollateral(); + uint8 currentIsLive = action == ActionType.Mint ? collatInfo.isMintLive : collatInfo.isBurnLive; + if (currentIsLive == targetIsLive) { + if (targetIsLive == 0) revert AlreadyPaused(); + revert NotPaused(); + } if (action == ActionType.Mint) { - isLive = 1 - collatInfo.isMintLive; - collatInfo.isMintLive = isLive; + collatInfo.isMintLive = targetIsLive; } else { - isLive = 1 - collatInfo.isBurnLive; - collatInfo.isBurnLive = isLive; + collatInfo.isBurnLive = targetIsLive; } } else { ParallelizerStorage storage ts = s.transmuterStorage(); - isLive = 1 - ts.isRedemptionLive; - ts.isRedemptionLive = isLive; + if (ts.isRedemptionLive == targetIsLive) { + if (targetIsLive == 0) revert AlreadyPaused(); + revert NotPaused(); + } + ts.isRedemptionLive = targetIsLive; } - emit PauseToggled(collateral, uint256(action), isLive == 0); + emit PauseToggled(collateral, uint256(action), targetIsLive == 0); } /// @notice Internal version of `setFees` diff --git a/contracts/savings/Savings.sol b/contracts/savings/Savings.sol index dcb7df9..38db003 100644 --- a/contracts/savings/Savings.sol +++ b/contracts/savings/Savings.sol @@ -35,7 +35,14 @@ contract Savings is BaseSavings, SavingsEIP3009 { /// @notice Checks whether the address is trusted to set the rate mapping(address => uint256) public isTrustedUpdater; - uint256[48] private __gap; + /// @notice Tracked balance of `asset` backing share holders + /// @dev Distinct from `IERC20(asset()).balanceOf(address(this))`: only updated by `deposit`/`mint`, + /// `withdraw`/`redeem`, the EIP-3009 authorized variants and `_accrue`. Direct ERC20 transfers + /// to this contract are *not* counted as backing, neutralizing donation/inflation attacks. + /// The surplus (`balanceOf(self) - storedAssets`) can be retrieved via `recoverSurplus`. + uint256 public storedAssets; + + uint256[47] private __gap; /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// EVENTS @@ -46,6 +53,7 @@ contract Savings is BaseSavings, SavingsEIP3009 { event ToggledPause(uint128 pauseStatus); event ToggledTrusted(address indexed trustedAddress, uint256 trustedStatus); event RateUpdated(uint256 newRate); + event SurplusRecovered(address indexed to, uint256 amount); /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// INITIALIZATION @@ -77,6 +85,17 @@ contract Savings is BaseSavings, SavingsEIP3009 { __AccessManaged_init(_authority); _setNameAndSymbol(name_, symbol_); _deposit(msg.sender, address(this), 10 ** (asset_.decimals()) / divizer, BASE_18 / divizer); + lastUpdate = uint40(block.timestamp); + } + + /// @notice One-shot reinitializer for upgrades that introduce `storedAssets` + /// @dev Seeds `storedAssets` with the current ERC20 balance held by the contract, so existing + /// legitimately deposited assets remain backing. Any subsequent direct transfer is treated as a + /// donation surplus and ignored by `totalAssets()` until `recoverSurplus` is called. + function initializeStoredAssets() external restricted reinitializer(2) { + if (block.timestamp - lastUpdate > MAX_STORED_ASSETS_INIT_STALENESS) revert StaleAccrual(); + storedAssets = IERC20Metadata(asset()).balanceOf(address(this)); + lastUpdate = uint40(block.timestamp); } /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -97,18 +116,31 @@ contract Savings is BaseSavings, SavingsEIP3009 { _; } + /// @notice Reverts when shares exist but `storedAssets` was never seeded (non-atomic upgrade) + modifier onlyInitialized() { + if (storedAssets == 0 && totalSupply() != 0) revert NotInitialized(); + _; + } + /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// CONTRACT LOGIC //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ /// @notice Accrues interest to this contract by minting tokenPs + /// @dev Uses the tracked `storedAssets` rather than `IERC20.balanceOf(self)` so that donations + /// can never inflate the accrual base. function _accrue() internal returns (uint256 newTotalAssets) { - uint256 currentBalance = super.totalAssets(); + uint256 currentBalance = storedAssets; + if (paused > 0) { + lastUpdate = uint40(block.timestamp); + return currentBalance; + } newTotalAssets = _computeUpdatedAssets(currentBalance, block.timestamp - lastUpdate); lastUpdate = uint40(block.timestamp); uint256 earned = newTotalAssets - currentBalance; if (earned > 0) { ITokenP(asset()).mint(address(this), earned); + storedAssets = newTotalAssets; emit Accrued(earned); } } @@ -132,8 +164,11 @@ contract Savings is BaseSavings, SavingsEIP3009 { //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ /// @inheritdoc ERC4626Upgradeable + /// @dev Returns the projection of `storedAssets` rather than `IERC20.balanceOf(self)`. + /// Direct ERC20 transfers to this contract do not affect this value. function totalAssets() public view override returns (uint256) { - return _computeUpdatedAssets(super.totalAssets(), block.timestamp - lastUpdate); + if (paused > 0) return storedAssets; + return _computeUpdatedAssets(storedAssets, block.timestamp - lastUpdate); } /// @inheritdoc ERC4626Upgradeable @@ -161,15 +196,37 @@ contract Savings is BaseSavings, SavingsEIP3009 { //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ /// @inheritdoc ERC4626Upgradeable - function deposit(uint256 assets, address receiver) public override whenNotPaused returns (uint256 shares) { + function deposit( + uint256 assets, + address receiver + ) + public + override + whenNotPaused + onlyInitialized + returns (uint256 shares) + { uint256 newTotalAssets = _accrue(); + uint256 maxAssets = maxDeposit(receiver); + if (assets > maxAssets) revert ERC4626ExceededMaxDeposit(receiver, assets, maxAssets); shares = _convertToShares(assets, newTotalAssets, Math.Rounding.Floor); _deposit(_msgSender(), receiver, assets, shares); } /// @inheritdoc ERC4626Upgradeable - function mint(uint256 shares, address receiver) public override whenNotPaused returns (uint256 assets) { + function mint( + uint256 shares, + address receiver + ) + public + override + whenNotPaused + onlyInitialized + returns (uint256 assets) + { uint256 newTotalAssets = _accrue(); + uint256 maxShares = maxMint(receiver); + if (shares > maxShares) revert ERC4626ExceededMaxMint(receiver, shares, maxShares); assets = _convertToAssets(shares, newTotalAssets, Math.Rounding.Ceil); _deposit(_msgSender(), receiver, assets, shares); } @@ -183,9 +240,12 @@ contract Savings is BaseSavings, SavingsEIP3009 { public override whenNotPaused + onlyInitialized returns (uint256 shares) { uint256 newTotalAssets = _accrue(); + uint256 maxAssets = maxWithdraw(owner); + if (assets > maxAssets) revert ERC4626ExceededMaxWithdraw(owner, assets, maxAssets); shares = _convertToShares(assets, newTotalAssets, Math.Rounding.Ceil); _withdraw(_msgSender(), receiver, owner, assets, shares); } @@ -199,9 +259,12 @@ contract Savings is BaseSavings, SavingsEIP3009 { public override whenNotPaused + onlyInitialized returns (uint256 assets) { uint256 newTotalAssets = _accrue(); + uint256 maxShares = maxRedeem(owner); + if (shares > maxShares) revert ERC4626ExceededMaxRedeem(owner, shares, maxShares); assets = _convertToAssets(shares, newTotalAssets, Math.Rounding.Floor); _withdraw(_msgSender(), receiver, owner, assets, shares); } @@ -230,6 +293,7 @@ contract Savings is BaseSavings, SavingsEIP3009 { ) external whenNotPaused + onlyInitialized returns (uint256 shares) { _consumeDepositAuthorization( @@ -237,9 +301,11 @@ contract Savings is BaseSavings, SavingsEIP3009 { ); uint256 newTotalAssets = _accrue(); + if (assets > maxDeposit(receiver)) revert ERC4626ExceededMaxDeposit(receiver, assets, maxDeposit(receiver)); shares = _convertToShares(assets, newTotalAssets, Math.Rounding.Floor); IEIP3009(asset()) .receiveWithAuthorization(owner, address(this), assets, validAfter, validBefore, nonce, tokenSignature); + storedAssets += assets; _mint(receiver, shares); emit Deposit(owner, receiver, assets, shares); } @@ -315,11 +381,15 @@ contract Savings is BaseSavings, SavingsEIP3009 { bytes memory signature ) internal + onlyInitialized returns (uint256 assets) { _consumeRedeemAuthorization(address(this), owner, receiver, shares, validAfter, validBefore, nonce, signature); uint256 newTotalAssets = _accrue(); + uint256 maxShares = maxRedeem(owner); + if (shares > maxShares) revert ERC4626ExceededMaxRedeem(owner, shares, maxShares); assets = _convertToAssets(shares, newTotalAssets, Math.Rounding.Floor); + storedAssets -= assets; _burn(owner, shares); SafeERC20.safeTransfer(IERC20Metadata(asset()), receiver, assets); emit Withdraw(msg.sender, receiver, owner, assets, shares); @@ -377,14 +447,44 @@ contract Savings is BaseSavings, SavingsEIP3009 { function _setNameAndSymbol(string memory newName, string memory newSymbol) internal virtual { } + /// @inheritdoc ERC4626Upgradeable + /// @dev Mirrors deposits into `storedAssets` so direct transfers stay outside the backing. + function _deposit( + address caller, + address receiver, + uint256 assets, + uint256 shares + ) + internal + override + { + super._deposit(caller, receiver, assets, shares); + storedAssets += assets; + } + + /// @inheritdoc ERC4626Upgradeable + /// @dev Mirrors withdrawals out of `storedAssets`. + function _withdraw( + address caller, + address receiver, + address owner, + uint256 assets, + uint256 shares + ) + internal + override + { + storedAssets -= assets; + super._withdraw(caller, receiver, owner, assets, shares); + } + /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// HELPERS //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ - /// @notice Provides an estimated Annual Percentage Rate for base depositors on this contract - function estimatedAPR() external view returns (uint256 apr) { - // 365 days = 31536000 seconds - return _computeUpdatedAssets(BASE_18, 31_536_000) - BASE_18; + /// @notice Provides an estimated Annual Percentage Yield for base depositors on this contract + function estimatedAPY() external view returns (uint256 apy) { + return _computeUpdatedAssets(BASE_18, SECONDS_PER_YEAR) - BASE_18; } /// @notice Wrapper on top of the `computeUpdatedAssets` function @@ -396,12 +496,27 @@ contract Savings is BaseSavings, SavingsEIP3009 { GOVERNANCE //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ - /// @notice Pauses the contract - function togglePause() external restricted { + /// @notice Pauses the contract — blocks `deposit`, `mint`, `withdraw`, `redeem` and the + /// EIP-3009 authorized variants + /// @dev Reverts if already paused, so a no-op governance call cannot pass silently. Accrues + /// outstanding yield before flipping the flag so holders are settled the moment the vault stops + /// accepting interactions, removing the stale-window deferral. + function pause() external restricted { + if (paused == 1) revert AlreadyPaused(); _accrue(); - uint8 pauseStatus = 1 - paused; - paused = pauseStatus; - emit ToggledPause(pauseStatus); + paused = 1; + emit ToggledPause(1); + } + + /// @notice Unpauses the contract + /// @dev Reverts if not paused, so a no-op governance call cannot pass silently. Advances + /// `lastUpdate` to the unpause timestamp so the paused interval is dropped rather than minted: + /// pausing halts emission. `pause()` already settles yield up to the pause moment. + function unpause() external restricted { + if (paused == 0) revert NotPaused(); + lastUpdate = uint40(block.timestamp); + paused = 0; + emit ToggledPause(0); } /// @notice Toggles an address @@ -434,4 +549,22 @@ contract Savings is BaseSavings, SavingsEIP3009 { } emit MaxRateUpdated(newMaxRate); } + + /// @notice Transfers any `asset` balance held by the contract that is not part of the tracked + /// `storedAssets` (i.e. donated or otherwise sent directly) to `to` + /// @dev Scope is limited to the underlying `asset()`: it recovers the `balanceOf(self) - storedAssets` + /// surplus only, and never any third-party ERC20 mistakenly sent here (use a dedicated rescue path for + /// those). Restricted to governance. Cannot drain assets backing share holders since it returns `0` + /// whenever the actual balance does not exceed `storedAssets`. + /// @param to Recipient of the recovered surplus + /// @return surplus Amount of `asset()` transferred out (0 when there is no surplus) + function recoverSurplus(address to) external restricted returns (uint256 surplus) { + if (to == address(0)) revert ZeroAddress(); + uint256 actualBalance = IERC20Metadata(asset()).balanceOf(address(this)); + uint256 stored = storedAssets; + if (actualBalance <= stored) return 0; + surplus = actualBalance - stored; + IERC20Metadata(asset()).safeTransfer(to, surplus); + emit SurplusRecovered(to, surplus); + } } diff --git a/contracts/utils/Constants.sol b/contracts/utils/Constants.sol index cee95fc..c9a09bf 100644 --- a/contracts/utils/Constants.sol +++ b/contracts/utils/Constants.sol @@ -38,6 +38,8 @@ uint256 constant BASE_36 = 1e36; uint256 constant MAX_BURN_FEE = 999_000_000; uint256 constant MAX_MINT_FEE = BASE_12 - 1; uint256 constant MAX_PAYEES = 10; +uint256 constant SECONDS_PER_YEAR = 365 days; +uint256 constant MAX_STORED_ASSETS_INIT_STALENESS = 30 minutes; /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// REENTRANT diff --git a/contracts/utils/Errors.sol b/contracts/utils/Errors.sol index 9740d8f..6ee3525 100644 --- a/contracts/utils/Errors.sol +++ b/contracts/utils/Errors.sol @@ -44,6 +44,8 @@ error OdosSwapFailed(); error OracleUpdateFailed(); error SurplusBufferRatioNotSet(); error Paused(); +error AlreadyPaused(); +error NotPaused(); error ReentrantCall(); error RemoveFacetAddressMustBeZeroAddress(address _facetAddress); error TooBigAmountIn(); @@ -56,3 +58,5 @@ error SwapError(); error SlippageTooHigh(); error InsufficientFunds(); error Undercollateralized(); +error StaleAccrual(); +error NotInitialized(); diff --git a/docs/audits/savings-fix/Bailsec - Parallel Protocol - Savings Fix - 1st Report.pdf b/docs/audits/savings-fix/Bailsec - Parallel Protocol - Savings Fix - 1st Report.pdf new file mode 100644 index 0000000..66b673d Binary files /dev/null and b/docs/audits/savings-fix/Bailsec - Parallel Protocol - Savings Fix - 1st Report.pdf differ diff --git a/docs/audits/savings-fix/Bailsec - Parallel Protocol - Savings Fix - Final Report.pdf b/docs/audits/savings-fix/Bailsec - Parallel Protocol - Savings Fix - Final Report.pdf new file mode 100644 index 0000000..b179f2c Binary files /dev/null and b/docs/audits/savings-fix/Bailsec - Parallel Protocol - Savings Fix - Final Report.pdf differ diff --git a/docs/audits/savings-fix/Cyfrin - Parallel Protocol - Savings Fix - 1st Report.pdf b/docs/audits/savings-fix/Cyfrin - Parallel Protocol - Savings Fix - 1st Report.pdf new file mode 100644 index 0000000..3867a3c Binary files /dev/null and b/docs/audits/savings-fix/Cyfrin - Parallel Protocol - Savings Fix - 1st Report.pdf differ diff --git a/docs/audits/savings-fix/Cyfrin - Parallel Protocol - Savings Fix - Final Report.pdf b/docs/audits/savings-fix/Cyfrin - Parallel Protocol - Savings Fix - Final Report.pdf new file mode 100644 index 0000000..09e5b98 Binary files /dev/null and b/docs/audits/savings-fix/Cyfrin - Parallel Protocol - Savings Fix - Final Report.pdf differ diff --git a/foundry.toml b/foundry.toml index 053ce55..26e9336 100644 --- a/foundry.toml +++ b/foundry.toml @@ -6,7 +6,7 @@ test = 'tests' script = 'scripts' cache_path = 'cache-forge' gas_reports = ["*"] -via_ir = true +via_ir = false sizes = true evm_version = "cancun" optimizer = true diff --git a/scripts/SetParallelizerRoles.s.sol b/scripts/SetParallelizerRoles.s.sol index 51f1820..0196c0e 100644 --- a/scripts/SetParallelizerRoles.s.sol +++ b/scripts/SetParallelizerRoles.s.sol @@ -10,13 +10,14 @@ contract SetParallelizerRoles is BaseScript { address parallelizer = 0x1250304F66404cd153fA39388DDCDAec7E0f1707; function run() public broadcast { - bytes4[] memory guardianSelectors = new bytes4[](6); - guardianSelectors[0] = ISettersGuardian.togglePause.selector; - guardianSelectors[1] = ISettersGuardian.setFees.selector; - guardianSelectors[2] = ISettersGuardian.setRedemptionCurveParams.selector; - guardianSelectors[3] = ISettersGuardian.toggleWhitelist.selector; - guardianSelectors[4] = ISettersGuardian.setStablecoinCap.selector; - guardianSelectors[5] = IDiamondEtherscan.setDummyImplementation.selector; + bytes4[] memory guardianSelectors = new bytes4[](7); + guardianSelectors[0] = ISettersGuardian.pause.selector; + guardianSelectors[1] = ISettersGuardian.unpause.selector; + guardianSelectors[2] = ISettersGuardian.setFees.selector; + guardianSelectors[3] = ISettersGuardian.setRedemptionCurveParams.selector; + guardianSelectors[4] = ISettersGuardian.toggleWhitelist.selector; + guardianSelectors[5] = ISettersGuardian.setStablecoinCap.selector; + guardianSelectors[6] = IDiamondEtherscan.setDummyImplementation.selector; accessManager.setTargetFunctionRole(parallelizer, guardianSelectors, Roles.GUARDIAN_ROLE); bytes4[] memory governorSelectors = new bytes4[](11); diff --git a/scripts/SetSavingRoles.s.sol b/scripts/SetSavingRoles.s.sol index ca5fd5b..185068c 100644 --- a/scripts/SetSavingRoles.s.sol +++ b/scripts/SetSavingRoles.s.sol @@ -11,19 +11,22 @@ contract SetSavingRoles is BaseScript { address saving = 0x9B3a8f7CEC208e247d97dEE13313690977e24459; function run() public broadcast { - bytes4[] memory guardianSelectors = new bytes4[](2); - guardianSelectors[0] = Savings.togglePause.selector; - guardianSelectors[1] = Savings.toggleTrusted.selector; + bytes4[] memory guardianSelectors = new bytes4[](3); + guardianSelectors[0] = Savings.pause.selector; + guardianSelectors[1] = Savings.unpause.selector; + guardianSelectors[2] = Savings.toggleTrusted.selector; accessManager.setTargetFunctionRole(saving, guardianSelectors, Roles.GUARDIAN_ROLE); bytes4[] memory keeperSelectors = new bytes4[](1); keeperSelectors[0] = Savings.setRate.selector; accessManager.setTargetFunctionRole(saving, keeperSelectors, Roles.KEEPER_ROLE); - bytes4[] memory governorSelectors = new bytes4[](3); + bytes4[] memory governorSelectors = new bytes4[](5); governorSelectors[0] = SavingsNameable.setNameAndSymbol.selector; governorSelectors[1] = Savings.setMaxRate.selector; governorSelectors[2] = UUPSUpgradeable.upgradeToAndCall.selector; + governorSelectors[3] = Savings.recoverSurplus.selector; + governorSelectors[4] = Savings.initializeStoredAssets.selector; accessManager.setTargetFunctionRole(saving, governorSelectors, Roles.GOVERNOR_ROLE); accessManager.grantRole(Roles.USDp_MINTER_ROLE, address(saving), 0); diff --git a/scripts/generated/DummyDiamondImplementation.sol b/scripts/generated/DummyDiamondImplementation.sol index 78f42cf..c439866 100644 --- a/scripts/generated/DummyDiamondImplementation.sol +++ b/scripts/generated/DummyDiamondImplementation.sol @@ -161,7 +161,9 @@ contract DummyDiamondImplementation { function setRedemptionCurveParams(uint64[] memory xFee, int64[] memory yFee) external { } - function togglePause(address collateral, uint8 pausedType) external { } + function pause(address collateral, uint8 action) external { } + + function unpause(address collateral, uint8 action) external { } function toggleWhitelist(uint8 whitelistType, address who) external { } diff --git a/scripts/generated/DummyDiamondImplementationSidechain.sol b/scripts/generated/DummyDiamondImplementationSidechain.sol index 0797d64..6bcb412 100644 --- a/scripts/generated/DummyDiamondImplementationSidechain.sol +++ b/scripts/generated/DummyDiamondImplementationSidechain.sol @@ -166,7 +166,9 @@ contract DummyDiamondImplementation { function setStablecoinCap(address collateral, uint256 stablecoinCap) external { } - function togglePause(address collateral, uint8 pausedType) external { } + function pause(address collateral, uint8 action) external { } + + function unpause(address collateral, uint8 action) external { } function toggleWhitelist(uint8 whitelistType, address who) external { } diff --git a/tests/fuzz/Redeem.t.sol b/tests/fuzz/Redeem.t.sol index c43d81a..60ca6c2 100644 --- a/tests/fuzz/Redeem.t.sol +++ b/tests/fuzz/Redeem.t.sol @@ -1770,8 +1770,8 @@ contract RedeemTest is Fixture, FunctionUtils { address attacker = vm.addr(10); vm.startPrank(guardian); - parallelizer.togglePause(address(eurB), ActionType.Redeem); - parallelizer.togglePause(address(eurY), ActionType.Redeem); + parallelizer.pause(address(eurB), ActionType.Redeem); + parallelizer.unpause(address(eurY), ActionType.Redeem); uint64[] memory xRedemption = new uint64[](1); xRedemption[0] = uint64(0); int64[] memory yRedemption = new int64[](1); diff --git a/tests/fuzz/Savings.t.sol b/tests/fuzz/Savings.t.sol index 84d3213..bf5135b 100644 --- a/tests/fuzz/Savings.t.sol +++ b/tests/fuzz/Savings.t.sol @@ -4,8 +4,10 @@ pragma solidity 0.8.28; import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import { IERC20Errors } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import { ERC4626Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC4626Upgradeable.sol"; import { IAccessManaged } from "contracts/utils/AccessManagedUpgradeable.sol"; import { EIP3009 } from "contracts/savings/EIP3009.sol"; +import { Savings } from "contracts/savings/Savings.sol"; import { UD60x18, ud, pow, powu, unwrap } from "@prb/math/UD60x18.sol"; @@ -59,7 +61,7 @@ contract SavingsTest is Fixture, FunctionUtils { _deposit(BASE_18, alice, alice, 0); vm.startPrank(guardian); - saving.togglePause(); + saving.pause(); vm.startPrank(alice); vm.expectRevert(Errors.Paused.selector); @@ -89,6 +91,29 @@ contract SavingsTest is Fixture, FunctionUtils { assertEq(saving.isTrustedUpdater(alice), 1); } + /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + DONATION ATTACK + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ + + /// @notice Locks in the invariant the storedAssets fix protects: an honest depositor's + /// redemption is strictly equal to the preview computed before the donation, + /// regardless of how large the donation is. + function testFuzz_DonationAttackInflationLockedDown(uint256 donation) public { + donation = bound(donation, 1, 1_000_000_000e18); + + (uint256 honestShares,) = _deposit(BASE_18, alice, alice, 0); + uint256 baselinePreview = saving.previewRedeem(honestShares); + + deal(address(tokenP), address(this), donation); + tokenP.transfer(address(saving), donation); + + assertEq(saving.previewRedeem(honestShares), baselinePreview, "preview unchanged"); + + vm.prank(alice); + uint256 received = saving.redeem(honestShares, alice, alice); + assertEq(received, baselinePreview, "received == baseline (no donation captured)"); + } + /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// APRS //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ @@ -107,7 +132,7 @@ contract SavingsTest is Fixture, FunctionUtils { (BASE_18 * unwrap(powu(ud(BASE_18 + rate / BASE_9), 365 days))) / unwrap(powu(ud(BASE_18), 365 days)) - BASE_18; _assertApproxEqRelDecimalWithTolerance( - saving.estimatedAPR(), estimatedAPR, estimatedAPR, _MAX_PERCENTAGE_DEVIATION * 5000, 18 + saving.estimatedAPY(), estimatedAPR, estimatedAPR, _MAX_PERCENTAGE_DEVIATION * 5000, 18 ); } @@ -131,7 +156,7 @@ contract SavingsTest is Fixture, FunctionUtils { (BASE_18 * unwrap(powu(ud(BASE_18 + rate / BASE_9), 365 days))) / unwrap(powu(ud(BASE_18), 365 days)) - BASE_18; _assertApproxEqRelDecimalWithTolerance( - saving.estimatedAPR(), estimatedAPR, estimatedAPR, _MAX_PERCENTAGE_DEVIATION * 5000, 18 + saving.estimatedAPY(), estimatedAPR, estimatedAPR, _MAX_PERCENTAGE_DEVIATION * 5000, 18 ); vm.startPrank(guardian); @@ -666,7 +691,11 @@ contract SavingsTest is Fixture, FunctionUtils { ); vm.startPrank(alice); - vm.expectRevert(abi.encodeWithSelector(IERC20Errors.ERC20InsufficientBalance.selector, alice, shares, shares + 1)); + vm.expectRevert( + abi.encodeWithSelector( + ERC4626Upgradeable.ERC4626ExceededMaxWithdraw.selector, alice, withdrawableAmount + 1, withdrawableAmount + ) + ); saving.withdraw(withdrawableAmount + 1, receiver, alice); uint256 sharesBurnt = saving.withdraw(withdrawableAmount, receiver, alice); vm.stopPrank(); @@ -936,7 +965,7 @@ contract SavingsTest is Fixture, FunctionUtils { (bytes memory savingsSig, bytes memory tokenSig) = _signDepositAuth(1, alice, alice, amount, 0, deadline, nonce); vm.prank(guardian); - saving.togglePause(); + saving.pause(); vm.prank(bob); vm.expectRevert(Errors.Paused.selector); @@ -996,6 +1025,22 @@ contract SavingsTest is Fixture, FunctionUtils { saving.redeemWithAuthorization(redeemShares, bob, alice, 0, deadline, nonce, v, r, s); } + function test_RedeemWithAuthorization_RevertWhen_ExceedsMaxRedeem() public { + _deposit(100 * BASE_18, alice, alice, 0); + uint256 shares = saving.balanceOf(alice); + uint256 tooMany = shares + 1; + + bytes32 nonce = bytes32("redeem_max"); + uint256 deadline = block.timestamp + 1 hours; + (uint8 v, bytes32 r, bytes32 s) = _signRedeemAuth(1, alice, alice, tooMany, 0, deadline, nonce); + + vm.prank(bob); + vm.expectRevert( + abi.encodeWithSelector(ERC4626Upgradeable.ERC4626ExceededMaxRedeem.selector, alice, tooMany, shares) + ); + saving.redeemWithAuthorization(tooMany, alice, alice, 0, deadline, nonce, v, r, s); + } + function test_TransferWithAuthorization_Savings() public { _deposit(100 * BASE_18, alice, alice, 0); uint256 shares = saving.balanceOf(alice); diff --git a/tests/fuzz/Swap.t.sol b/tests/fuzz/Swap.t.sol index 32f426e..cc46807 100644 --- a/tests/fuzz/Swap.t.sol +++ b/tests/fuzz/Swap.t.sol @@ -144,8 +144,8 @@ contract SwapTest is Fixture, FunctionUtils { _updateOracles(latestOracleValue); vm.startPrank(guardian); - parallelizer.togglePause(_collaterals[fromToken], Storage.ActionType.Mint); - parallelizer.togglePause(_collaterals[fromToken], Storage.ActionType.Burn); + parallelizer.pause(_collaterals[fromToken], Storage.ActionType.Mint); + parallelizer.pause(_collaterals[fromToken], Storage.ActionType.Burn); vm.stopPrank(); vm.startPrank(alice); @@ -173,8 +173,8 @@ contract SwapTest is Fixture, FunctionUtils { _updateOracles(latestOracleValue); vm.startPrank(guardian); - parallelizer.togglePause(_collaterals[fromToken], Storage.ActionType.Mint); - parallelizer.togglePause(_collaterals[fromToken], Storage.ActionType.Burn); + parallelizer.pause(_collaterals[fromToken], Storage.ActionType.Mint); + parallelizer.pause(_collaterals[fromToken], Storage.ActionType.Burn); vm.stopPrank(); vm.startPrank(alice); diff --git a/tests/mock/SavingsLegacyMock.sol b/tests/mock/SavingsLegacyMock.sol new file mode 100644 index 0000000..bb52c16 --- /dev/null +++ b/tests/mock/SavingsLegacyMock.sol @@ -0,0 +1,165 @@ +// SPDX-License-Identifier: BUSL-1.1 +pragma solidity 0.8.28; + +import { ERC4626Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC4626Upgradeable.sol"; +import { ERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol"; +import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; +import { Math } from "@openzeppelin/contracts/utils/math/Math.sol"; + +import { ITokenP } from "contracts/interfaces/ITokenP.sol"; +import { BaseSavings } from "contracts/savings/BaseSavings.sol"; +import { SavingsEIP3009 } from "contracts/savings/SavingsEIP3009.sol"; + +import "contracts/utils/Constants.sol"; +import "contracts/utils/Errors.sol"; + +/// @title SavingsLegacyMock +/// @notice Mirrors the pre-storedAssets Savings + SavingsNameable storage and behaviour, used +/// solely as the "before" implementation in upgrade tests. Storage layout matches the production +/// SavingsNameable contract on `audit/eip3009-hotfix-fees` *before* this PR introduced the +/// `storedAssets` slot: +/// slot 0: rate (uint208) + lastUpdate (uint40) + paused (uint8) +/// slot 1: maxRate +/// slot 2: isTrustedUpdater +/// slots 3..50: __gap (48 entries) +/// slot 51: __name +/// slot 52: __symbol +/// slots 53..100: __gapNameable (48 entries) +contract SavingsLegacyMock is BaseSavings, SavingsEIP3009 { + using Math for uint256; + + uint208 public rate; + uint40 public lastUpdate; + uint8 public paused; + uint256 public maxRate; + mapping(address => uint256) public isTrustedUpdater; + uint256[48] private __gap; + + string internal __name; + string internal __symbol; + uint256[48] private __gapNameable; + + function initialize( + address _authority, + IERC20Metadata asset_, + string memory name_, + string memory symbol_, + uint256 divizer + ) + public + initializer + { + if (address(_authority) == address(0)) revert ZeroAddress(); + __ERC4626_init(asset_); + __ERC20_init(name_, symbol_); + __UUPSUpgradeable_init(); + __AccessManaged_init(_authority); + __name = name_; + __symbol = symbol_; + _deposit(msg.sender, address(this), 10 ** (asset_.decimals()) / divizer, BASE_18 / divizer); + } + + modifier whenNotPaused() { + if (paused > 0) revert Paused(); + _; + } + + /// @notice Legacy implementation: reads raw ERC20 balance (the source of the donation + /// vulnerability) and projects it through the rate formula. + function totalAssets() public view override returns (uint256) { + return _computeUpdatedAssets(super.totalAssets(), block.timestamp - lastUpdate); + } + + function name() public view override(ERC20Upgradeable, IERC20Metadata) returns (string memory) { + return __name; + } + + function symbol() public view override(ERC20Upgradeable, IERC20Metadata) returns (string memory) { + return __symbol; + } + + function deposit(uint256 assets, address receiver) public override whenNotPaused returns (uint256 shares) { + uint256 newTotalAssets = _accrue(); + shares = _convertToShares(assets, newTotalAssets, Math.Rounding.Floor); + _deposit(_msgSender(), receiver, assets, shares); + } + + function redeem( + uint256 shares, + address receiver, + address owner + ) + public + override + whenNotPaused + returns (uint256 assets) + { + uint256 newTotalAssets = _accrue(); + assets = _convertToAssets(shares, newTotalAssets, Math.Rounding.Floor); + _withdraw(_msgSender(), receiver, owner, assets, shares); + } + + function decimals() public view override(ERC4626Upgradeable, ERC20Upgradeable) returns (uint8) { + return super.decimals(); + } + + function _accrue() internal returns (uint256 newTotalAssets) { + uint256 currentBalance = super.totalAssets(); + newTotalAssets = _computeUpdatedAssets(currentBalance, block.timestamp - lastUpdate); + lastUpdate = uint40(block.timestamp); + uint256 earned = newTotalAssets - currentBalance; + if (earned > 0) { + ITokenP(asset()).mint(address(this), earned); + } + } + + function _computeUpdatedAssets(uint256 currentBalance, uint256 exp) internal view returns (uint256) { + uint256 ratePerSecond = rate; + if (exp == 0 || ratePerSecond == 0) return currentBalance; + uint256 expMinusOne = exp - 1; + uint256 expMinusTwo = exp > 2 ? exp - 2 : 0; + uint256 basePowerTwo = (ratePerSecond * ratePerSecond + HALF_BASE_27) / BASE_27; + uint256 basePowerThree = (basePowerTwo * ratePerSecond + HALF_BASE_27) / BASE_27; + uint256 secondTerm = (exp * expMinusOne * basePowerTwo) / 2; + uint256 thirdTerm = (exp * expMinusOne * expMinusTwo * basePowerThree) / 6; + return (currentBalance * (BASE_27 + ratePerSecond * exp + secondTerm + thirdTerm)) / BASE_27; + } + + function _convertToShares(uint256 assets, Math.Rounding rounding) internal view override returns (uint256 shares) { + return _convertToShares(assets, totalAssets(), rounding); + } + + function _convertToShares( + uint256 assets, + uint256 newTotalAssets, + Math.Rounding rounding + ) + internal + view + returns (uint256 shares) + { + uint256 supply = totalSupply(); + return (assets == 0 || supply == 0) + ? assets.mulDiv(BASE_18, 10 ** (IERC20Metadata(asset()).decimals()), rounding) + : assets.mulDiv(supply, newTotalAssets, rounding); + } + + function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view override returns (uint256 assets) { + return _convertToAssets(shares, totalAssets(), rounding); + } + + function _convertToAssets( + uint256 shares, + uint256 newTotalAssets, + Math.Rounding rounding + ) + internal + view + returns (uint256 assets) + { + uint256 supply = totalSupply(); + return (supply == 0) + ? shares.mulDiv(10 ** (IERC20Metadata(asset()).decimals()), BASE_18, rounding) + : shares.mulDiv(newTotalAssets, supply, rounding); + } +} diff --git a/tests/units/Savings.t.sol b/tests/units/Savings.t.sol index 8bf3b5b..d8f4297 100644 --- a/tests/units/Savings.t.sol +++ b/tests/units/Savings.t.sol @@ -1,24 +1,29 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity 0.8.28; +import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import { IERC20 } from "@openzeppelin/contracts/interfaces/IERC20.sol"; import { IERC20Metadata } from "@openzeppelin/contracts/interfaces/IERC20Metadata.sol"; +import { UUPSUpgradeable } from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; import { IAccessManaged } from "contracts/utils/AccessManagedUpgradeable.sol"; +import { Savings } from "contracts/savings/Savings.sol"; import { SavingsNameable } from "contracts/savings/nameable/SavingsNameable.sol"; -import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import { Vm } from "@forge-std/Vm.sol"; - +import "contracts/utils/Errors.sol" as Errors; import "../Fixture.sol"; +import { SavingsLegacyMock } from "../mock/SavingsLegacyMock.sol"; + +contract SavingsUpgradeTest is Fixture { + uint256 internal constant _initDeposit = 1e18; -contract SavingsNameableUpgradeTest is Fixture { function setUp() public override { super.setUp(); saving = SavingsNameable(deploySavings(governor, address(tokenP), address(accessManager))); vm.label(address(saving), "saving"); - // grant access to required functions for governor role vm.startPrank(governor); accessManager.setTargetFunctionRole(address(saving), getGovernorSavingsSelectorAccess(), GOVERNOR_ROLE); vm.stopPrank(); @@ -35,7 +40,7 @@ contract SavingsNameableUpgradeTest is Fixture { assertEq(saving.totalSupply(), Constants.BASE_18); assertEq(saving.maxRate(), 0); assertEq(saving.paused(), 0); - assertEq(saving.lastUpdate(), 0); + assertEq(saving.lastUpdate(), block.timestamp); assertEq(saving.rate(), 0); } @@ -59,7 +64,7 @@ contract SavingsNameableUpgradeTest is Fixture { assertEq(saving.totalSupply(), Constants.BASE_18); assertEq(saving.maxRate(), 0); assertEq(saving.paused(), 0); - assertEq(saving.lastUpdate(), 0); + assertEq(saving.lastUpdate(), block.timestamp); assertEq(saving.rate(), 0); } @@ -69,6 +74,355 @@ contract SavingsNameableUpgradeTest is Fixture { vm.expectRevert(abi.encodeWithSelector(IAccessManaged.AccessManagedUnauthorized.selector, alice)); saving.upgradeToAndCall(newSavingsImpl, ""); } + + function test_upgradeFromLegacy_neutralizesDonationAttack() public { + SavingsNameable savingProxy = _deployLegacySavings(); + _depositInSavings(savingProxy, 100e18, alice); + + uint256 priceBeforeDonation = savingProxy.previewRedeem(1e18); + + uint256 donation = 10_000_000e18; + deal(address(tokenP), address(this), donation); + IERC20(address(tokenP)).transfer(address(savingProxy), donation); + + assertGt( + savingProxy.previewRedeem(1e18), + priceBeforeDonation * 1000, + "legacy: donation must inflate share price" + ); + + _upgradeSavings(savingProxy); + + assertGe( + savingProxy.storedAssets(), + _initDeposit + 100e18 + donation, + "post-upgrade storedAssets includes pre-upgrade donation as backing" + ); + + uint256 priceAfterUpgrade = savingProxy.previewRedeem(1e18); + + uint256 newDonation = 50_000_000e18; + deal(address(tokenP), address(this), newDonation); + IERC20(address(tokenP)).transfer(address(savingProxy), newDonation); + + assertEq(savingProxy.previewRedeem(1e18), priceAfterUpgrade, "new donation must not move price"); + assertEq(savingProxy.totalAssets(), savingProxy.storedAssets(), "totalAssets tracks storedAssets"); + + vm.prank(governor); + assertEq(savingProxy.recoverSurplus(treasury), newDonation, "new donation recoverable"); + } + + function test_upgradeFromLegacy_existingDepositorsCanStillRedeem() public { + SavingsNameable savingProxy = _deployLegacySavings(); + _depositInSavings(savingProxy, 100e18, alice); + + _upgradeSavings(savingProxy); + + uint256 aliceShares = savingProxy.balanceOf(alice); + vm.prank(alice); + uint256 received = savingProxy.redeem(aliceShares, alice, alice); + assertGt(received, 0, "alice can redeem after upgrade"); + assertEq(savingProxy.balanceOf(alice), 0, "shares burned"); + } + + function test_upgradeFromLegacy_initializeStoredAssetsRevertsWhenCalledTwice() public { + SavingsNameable savingProxy = _deployLegacySavings(); + _upgradeSavings(savingProxy); + + vm.prank(governor); + vm.expectRevert(bytes4(keccak256("InvalidInitialization()"))); + savingProxy.initializeStoredAssets(); + } + + function test_initializeStoredAssets_RevertWhen_CallerUnauthorized() public { + vm.prank(alice); + vm.expectRevert(abi.encodeWithSelector(IAccessManaged.AccessManagedUnauthorized.selector, alice)); + saving.initializeStoredAssets(); + } + + function test_initializeStoredAssets_RevertWhen_AccrualStale() public { + skip(30 minutes + 1); + vm.prank(governor); + vm.expectRevert(Errors.StaleAccrual.selector); + saving.initializeStoredAssets(); + } + + function test_initializeStoredAssets_SucceedsWithinFreshnessWindow() public { + skip(30 minutes); + vm.prank(governor); + saving.initializeStoredAssets(); + assertEq(saving.storedAssets(), IERC20(address(tokenP)).balanceOf(address(saving))); + } + + function test_initializeStoredAssets_reAnchorsLastUpdate() public { + skip(20 minutes); + vm.prank(governor); + saving.initializeStoredAssets(); + assertEq(saving.lastUpdate(), block.timestamp); + } + + function test_nonAtomicUpgrade_revertsEntryPointsUntilInitialized() public { + SavingsNameable savingProxy = _deployLegacySavings(); + _depositInSavings(savingProxy, 100e18, alice); + + SavingsNameable newImpl = new SavingsNameable(); + vm.prank(governor); + UUPSUpgradeable(address(savingProxy)).upgradeToAndCall(address(newImpl), ""); + + assertEq(savingProxy.storedAssets(), 0, "storedAssets left unseeded by non-atomic upgrade"); + assertGt(savingProxy.totalSupply(), 0); + + deal(address(tokenP), alice, 1e18); + vm.startPrank(alice); + IERC20(address(tokenP)).approve(address(savingProxy), 1e18); + vm.expectRevert(Errors.NotInitialized.selector); + savingProxy.deposit(1e18, alice); + vm.stopPrank(); + + uint256 aliceShares = savingProxy.balanceOf(alice); + vm.prank(alice); + vm.expectRevert(Errors.NotInitialized.selector); + savingProxy.redeem(aliceShares, alice, alice); + } + + function test_upgradeFromLegacy_postUpgradePauseUnpauseWorks() public { + SavingsNameable savingProxy = _deployLegacySavings(); + _upgradeSavings(savingProxy); + + vm.prank(governor); + accessManager.setTargetFunctionRole(address(savingProxy), getGuardianSavingsSelectorAccess(), GUARDIAN_ROLE); + + vm.prank(guardian); + savingProxy.pause(); + deal(address(tokenP), alice, 1e18); + vm.startPrank(alice); + IERC20(address(tokenP)).approve(address(savingProxy), 1e18); + vm.expectRevert(Errors.Paused.selector); + savingProxy.deposit(1e18, alice); + vm.stopPrank(); + + vm.prank(guardian); + savingProxy.unpause(); + } + + /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + HELPERS + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ + + function _deployLegacySavings() internal returns (SavingsNameable savingProxy) { + vm.startPrank(governor); + + deal(address(tokenP), governor, _initDeposit); + + SavingsLegacyMock legacyImpl = new SavingsLegacyMock(); + address futureProxy = vm.computeCreateAddress(governor, vm.getNonce(governor)); + IERC20(address(tokenP)).approve(futureProxy, _initDeposit); + + savingProxy = SavingsNameable( + address( + new ERC1967Proxy( + address(legacyImpl), + abi.encodeWithSelector( + legacyImpl.initialize.selector, accessManager, IERC20Metadata(address(tokenP)), name, symbol, 1 + ) + ) + ) + ); + + bytes4[] memory selectors = new bytes4[](1); + selectors[0] = UUPSUpgradeable.upgradeToAndCall.selector; + accessManager.setTargetFunctionRole(address(savingProxy), selectors, GOVERNOR_ROLE); + + vm.stopPrank(); + + vm.label(address(savingProxy), "savingProxy"); + } + + function _depositInSavings(SavingsNameable savingProxy, uint256 amount, address depositor) internal { + deal(address(tokenP), depositor, amount); + vm.startPrank(depositor); + IERC20(address(tokenP)).approve(address(savingProxy), amount); + savingProxy.deposit(amount, depositor); + vm.stopPrank(); + } + + function _upgradeSavings(SavingsNameable savingProxy) internal { + SavingsNameable newImpl = new SavingsNameable(); + vm.prank(governor); + UUPSUpgradeable(address(savingProxy)).upgradeToAndCall( + address(newImpl), + abi.encodeWithSelector(Savings.initializeStoredAssets.selector) + ); + + assertEq( + savingProxy.storedAssets(), + IERC20(address(tokenP)).balanceOf(address(savingProxy)), + "post-upgrade storedAssets == balance" + ); + } +} + +contract SavingsDonationAttackTest is Fixture { + function setUp() public override { + super.setUp(); + + saving = SavingsNameable(deploySavings(governor, address(tokenP), address(accessManager))); + vm.label(address(saving), "saving"); + + vm.startPrank(governor); + accessManager.setTargetFunctionRole(address(saving), getGovernorSavingsSelectorAccess(), GOVERNOR_ROLE); + accessManager.setTargetFunctionRole(address(saving), getGuardianSavingsSelectorAccess(), GUARDIAN_ROLE); + saving.setMaxRate(type(uint256).max); + vm.stopPrank(); + } + + /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + DONATION ATTACK + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ + + function test_DonationAttackIsNeutralized() public { + uint256 attackerDeposit = 1e18; + uint256 attackerShares = _depositInSavings(attackerDeposit, alice); + + uint256 priceBefore = saving.previewRedeem(1e18); + uint256 totalAssetsBefore = saving.totalAssets(); + uint256 storedBefore = saving.storedAssets(); + + uint256 donation = 225_000_000e18; + deal(address(tokenP), address(this), donation); + IERC20(address(tokenP)).transfer(address(saving), donation); + + assertEq(saving.totalAssets(), totalAssetsBefore, "totalAssets unchanged by donation"); + assertEq(saving.previewRedeem(1e18), priceBefore, "share price unchanged"); + assertEq(saving.storedAssets(), storedBefore, "storedAssets unchanged"); + + uint256 expectedOut = saving.previewRedeem(attackerShares); + vm.prank(alice); + uint256 received = saving.redeem(attackerShares, alice, alice); + assertEq(received, expectedOut, "redeem returns preview"); + assertLe(received, attackerDeposit + 1, "attacker did not capture donation"); + + uint256 surplus = IERC20(address(tokenP)).balanceOf(address(saving)) - saving.storedAssets(); + assertEq(surplus, donation, "donation persists as recoverable surplus"); + } + + function test_DonationAttackDormantAccrualIsAccountedNotSurplus() public { + _depositInSavings(1e18, alice); + + vm.startPrank(guardian); + uint208 ratePerSecond = uint208(uint256(BASE_27) / SECONDS_PER_YEAR / 100); // ~1% APY + saving.setRate(ratePerSecond); + vm.stopPrank(); + + skip(39 days); + + uint256 storedBefore = saving.storedAssets(); + _depositInSavings(1, alice); // any interaction triggers _accrue + + assertGt(saving.storedAssets(), storedBefore, "accrual increases storedAssets"); + assertEq( + IERC20(address(tokenP)).balanceOf(address(saving)) - saving.storedAssets(), + 0, + "minted accrual is backing, not surplus" + ); + } + + /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + RECOVER SURPLUS + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ + + function test_RecoverSurplusReturnsZeroWhenNoDonation() public { + _depositInSavings(1e18, alice); + + vm.prank(governor); + uint256 recovered = saving.recoverSurplus(treasury); + assertEq(recovered, 0, "no surplus expected"); + assertEq(IERC20(address(tokenP)).balanceOf(treasury), 0, "treasury untouched"); + } + + function test_RecoverSurplusDrainsDonationWithoutAffectingDepositors() public { + uint256 honestShares = _depositInSavings(1e18, alice); + uint256 storedBeforeDonation = saving.storedAssets(); + + uint256 donation = 5_000_000e18; + deal(address(tokenP), address(this), donation); + IERC20(address(tokenP)).transfer(address(saving), donation); + + vm.expectEmit(true, false, false, true, address(saving)); + emit Savings.SurplusRecovered(treasury, donation); + vm.prank(governor); + uint256 recovered = saving.recoverSurplus(treasury); + + assertEq(recovered, donation, "recovered amount"); + assertEq(IERC20(address(tokenP)).balanceOf(treasury), donation, "treasury credited"); + assertEq(saving.storedAssets(), storedBeforeDonation, "storedAssets stable"); + assertEq( + IERC20(address(tokenP)).balanceOf(address(saving)), + saving.storedAssets(), + "vault balance == storedAssets after recover" + ); + + vm.prank(alice); + uint256 received = saving.redeem(honestShares, alice, alice); + assertGt(received, 0, "alice can still redeem"); + } + + function test_RecoverSurplusRevertsOnZeroAddress() public { + deal(address(tokenP), address(saving), 100e18); + + vm.prank(governor); + vm.expectRevert(Errors.ZeroAddress.selector); + saving.recoverSurplus(address(0)); + } + + function test_RecoverSurplusRevertsWhenCallerNotGovernor() public { + deal(address(tokenP), address(saving), 100e18); + + vm.prank(alice); + vm.expectRevert(abi.encodeWithSelector(IAccessManaged.AccessManagedUnauthorized.selector, alice)); + saving.recoverSurplus(treasury); + } + + /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + PAUSE / UNPAUSE + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ + + function test_PauseRevertsWhenAlreadyPaused() public { + vm.startPrank(guardian); + saving.pause(); + vm.expectRevert(Errors.AlreadyPaused.selector); + saving.pause(); + vm.stopPrank(); + } + + function test_UnpauseRevertsWhenNotPaused() public { + vm.prank(guardian); + vm.expectRevert(Errors.NotPaused.selector); + saving.unpause(); + } + + function test_PauseThenUnpauseRestoresInteractions() public { + _depositInSavings(1e18, alice); + + vm.startPrank(guardian); + saving.pause(); + saving.unpause(); + vm.stopPrank(); + + _depositInSavings(1e18, alice); + } + + /*////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + HELPERS + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////*/ + + function _depositInSavings(uint256 amount, address depositor) internal returns (uint256 shares) { + deal(address(tokenP), depositor, amount); + vm.startPrank(depositor); + IERC20(address(tokenP)).approve(address(saving), amount); + shares = saving.deposit(amount, depositor); + vm.stopPrank(); + } } contract SavingsInitializeValidationTest is Fixture { @@ -144,7 +498,7 @@ contract SavingsMaxViewsPauseTest is Fixture { function _pause() internal { vm.prank(guardian); - saving.togglePause(); + saving.pause(); assertEq(saving.paused(), 1, "savings must be paused"); } @@ -164,7 +518,7 @@ contract SavingsMaxViewsPauseTest is Fixture { _pause(); vm.prank(guardian); - saving.togglePause(); + saving.unpause(); assertEq(saving.paused(), 0, "savings must be unpaused"); assertEq(saving.maxDeposit(alice), type(uint256).max); @@ -296,47 +650,87 @@ contract SavingsTogglePauseAccrueTest is Fixture { vm.stopPrank(); } - function test_togglePause_accruesYieldOnPause() public { + function test_pause_accruesYieldOnPause() public { uint256 assetsBefore = saving.totalAssets(); skip(1 days); uint256 expectedAtPause = saving.computeUpdatedAssets(assetsBefore, 1 days); assertGt(expectedAtPause, assetsBefore, "yield must have accrued during the pre-pause window"); vm.prank(guardian); - saving.togglePause(); + saving.pause(); assertEq(saving.paused(), 1); assertEq(saving.lastUpdate(), block.timestamp, "lastUpdate must snapshot the pause timestamp"); assertEq(saving.totalAssets(), expectedAtPause, "yield must be settled at pause time"); } - function test_togglePause_advancesLastUpdateOnUnpause() public { + function test_totalAssets_doesNotProjectYieldWhilePaused() public { + vm.prank(guardian); + saving.pause(); + uint256 storedAtPause = saving.storedAssets(); + + skip(30 days); + + assertEq(saving.totalAssets(), storedAtPause, "totalAssets must not project yield while paused"); + assertEq(saving.totalAssets(), saving.storedAssets()); + } + + function test_setRate_whilePaused_doesNotMintPausedYield() public { + vm.prank(guardian); + saving.pause(); + uint256 storedAtPause = saving.storedAssets(); + uint256 balAtPause = IERC20(address(tokenP)).balanceOf(address(saving)); + + skip(30 days); + + vm.prank(guardian); + saving.setRate(_rate); + + assertEq(saving.storedAssets(), storedAtPause, "no paused yield minted into storedAssets"); + assertEq(IERC20(address(tokenP)).balanceOf(address(saving)), balAtPause, "no tokenP minted while paused"); + assertEq(saving.lastUpdate(), block.timestamp, "lastUpdate advanced to now"); + } + + function test_setMaxRate_whilePaused_doesNotMintPausedYield() public { vm.prank(guardian); - saving.togglePause(); + saving.pause(); + uint256 storedAtPause = saving.storedAssets(); + uint256 balAtPause = IERC20(address(tokenP)).balanceOf(address(saving)); + + skip(30 days); + + vm.prank(governor); + saving.setMaxRate(_maxRate); + + assertEq(saving.storedAssets(), storedAtPause, "no paused yield minted into storedAssets"); + assertEq(IERC20(address(tokenP)).balanceOf(address(saving)), balAtPause, "no tokenP minted while paused"); + } + + function test_unpause_dropsPausedWindowYield() public { + vm.prank(guardian); + saving.pause(); uint40 lastUpdateAtPause = saving.lastUpdate(); uint256 assetsAtPause = saving.totalAssets(); skip(7 days); - uint256 expectedAfterPauseWindow = saving.computeUpdatedAssets(assetsAtPause, 7 days); - vm.prank(guardian); - saving.togglePause(); + saving.unpause(); assertEq(saving.paused(), 0); assertEq(saving.lastUpdate(), block.timestamp, "lastUpdate must advance to the unpause timestamp"); assertGt(saving.lastUpdate(), lastUpdateAtPause); - assertEq(saving.totalAssets(), expectedAfterPauseWindow, "unpause must settle pause-window yield in one shot"); + assertEq(saving.totalAssets(), assetsAtPause, "pausing halts emission: the paused window is not minted"); } - function test_togglePause_firstInteractionAfterUnpauseEarnsNoStaleYield() public { + function test_unpause_firstInteractionAfterUnpauseEarnsNoStaleYield() public { vm.prank(guardian); - saving.togglePause(); + saving.pause(); skip(30 days); vm.prank(guardian); - saving.togglePause(); + saving.unpause(); uint256 assetsAtUnpause = saving.totalAssets(); diff --git a/tests/units/Setters.t.sol b/tests/units/Setters.t.sol index 6b5b299..ed5e2e5 100644 --- a/tests/units/Setters.t.sol +++ b/tests/units/Setters.t.sol @@ -17,25 +17,25 @@ import "contracts/utils/Errors.sol" as Errors; import { Fixture } from "../Fixture.sol"; -contract Test_Setters_TogglePause is Fixture { +contract Test_Setters_Pause is Fixture { function test_RevertWhen_NotGuardian() public { vm.expectRevert(abi.encodeWithSelector(Errors.AccessManagedUnauthorized.selector, governor)); hoax(governor); - parallelizer.togglePause(address(eurA), ActionType.Mint); + parallelizer.pause(address(eurA), ActionType.Mint); vm.expectRevert(abi.encodeWithSelector(Errors.AccessManagedUnauthorized.selector, alice)); hoax(alice); - parallelizer.togglePause(address(eurA), ActionType.Mint); + parallelizer.pause(address(eurA), ActionType.Mint); vm.expectRevert(abi.encodeWithSelector(Errors.AccessManagedUnauthorized.selector, bob)); hoax(bob); - parallelizer.togglePause(address(eurA), ActionType.Mint); + parallelizer.pause(address(eurA), ActionType.Mint); } function test_RevertWhen_NotCollateral() public { vm.expectRevert(Errors.NotCollateral.selector); hoax(guardian); - parallelizer.togglePause(address(tokenP), ActionType.Mint); + parallelizer.pause(address(tokenP), ActionType.Mint); } function test_PauseMint() public { @@ -43,7 +43,7 @@ contract Test_Setters_TogglePause is Fixture { emit LibSetters.PauseToggled(address(eurA), uint256(ActionType.Mint), true); hoax(guardian); - parallelizer.togglePause(address(eurA), ActionType.Mint); + parallelizer.pause(address(eurA), ActionType.Mint); assert(parallelizer.isPaused(address(eurA), ActionType.Mint)); @@ -59,7 +59,7 @@ contract Test_Setters_TogglePause is Fixture { emit LibSetters.PauseToggled(address(eurA), uint256(ActionType.Burn), true); hoax(guardian); - parallelizer.togglePause(address(eurA), ActionType.Burn); + parallelizer.pause(address(eurA), ActionType.Burn); assert(parallelizer.isPaused(address(eurA), ActionType.Burn)); @@ -75,7 +75,7 @@ contract Test_Setters_TogglePause is Fixture { emit LibSetters.PauseToggled(address(eurA), uint256(ActionType.Redeem), true); hoax(guardian); - parallelizer.togglePause(address(eurA), ActionType.Redeem); + parallelizer.pause(address(eurA), ActionType.Redeem); assert(parallelizer.isPaused(address(eurA), ActionType.Redeem)); diff --git a/tests/utils/ConfigAccessManager.sol b/tests/utils/ConfigAccessManager.sol index 774656a..d1f85e7 100644 --- a/tests/utils/ConfigAccessManager.sol +++ b/tests/utils/ConfigAccessManager.sol @@ -42,18 +42,21 @@ abstract contract ConfigAccessManager is Helper { } function getGuardianSavingsSelectorAccess() internal pure returns (bytes4[] memory) { - bytes4[] memory selectors = new bytes4[](3); - selectors[0] = Savings.togglePause.selector; - selectors[1] = Savings.toggleTrusted.selector; - selectors[2] = Savings.setRate.selector; + bytes4[] memory selectors = new bytes4[](4); + selectors[0] = Savings.pause.selector; + selectors[1] = Savings.unpause.selector; + selectors[2] = Savings.toggleTrusted.selector; + selectors[3] = Savings.setRate.selector; return selectors; } function getGovernorSavingsSelectorAccess() internal pure returns (bytes4[] memory) { - bytes4[] memory selectors = new bytes4[](3); + bytes4[] memory selectors = new bytes4[](5); selectors[0] = SavingsNameable.setNameAndSymbol.selector; selectors[1] = Savings.setMaxRate.selector; selectors[2] = UUPSUpgradeable.upgradeToAndCall.selector; + selectors[3] = Savings.recoverSurplus.selector; + selectors[4] = Savings.initializeStoredAssets.selector; return selectors; } @@ -81,13 +84,14 @@ abstract contract ConfigAccessManager is Helper { } function getParallelizerGuardianSelectorAccess() internal pure returns (bytes4[] memory) { - bytes4[] memory selectors = new bytes4[](6); - selectors[0] = SettersGuardian.togglePause.selector; - selectors[1] = SettersGuardian.setFees.selector; - selectors[2] = SettersGuardian.setRedemptionCurveParams.selector; - selectors[3] = SettersGuardian.toggleWhitelist.selector; - selectors[4] = SettersGuardian.setStablecoinCap.selector; - selectors[5] = DiamondEtherscan.setDummyImplementation.selector; + bytes4[] memory selectors = new bytes4[](7); + selectors[0] = SettersGuardian.pause.selector; + selectors[1] = SettersGuardian.unpause.selector; + selectors[2] = SettersGuardian.setFees.selector; + selectors[3] = SettersGuardian.setRedemptionCurveParams.selector; + selectors[4] = SettersGuardian.toggleWhitelist.selector; + selectors[5] = SettersGuardian.setStablecoinCap.selector; + selectors[6] = DiamondEtherscan.setDummyImplementation.selector; return selectors; }