From 1c9a97cc8e90a19a7217281c321d99b157b6ee4e Mon Sep 17 00:00:00 2001 From: "John McClure (pickleback)" Date: Wed, 30 Oct 2024 19:59:33 -0500 Subject: [PATCH] Evaluate whether a constant vault share price is an appropriate invariant - Removed rebalancing from sandwich tests since the situation is supposed to be atomic - Keeping share price constant has the exact same effect as not distributing losses. - This results in bad UX + a DOS vector for small position sizes. - In the case of bonds devaluing via huge LP, this loss should affect others. - No need to handle the case where a redemption would be zero, with distributed losses this isn't possible. - Vault share price decreases on deposit, remains constant on redemption (for base atomic case). --- contracts/Everlong.sol | 61 ++++++---------- test/integration/CloseImmatureLongs.t.sol | 2 +- test/integration/PartialClosures.t.sol | 2 +- test/integration/Sandwich.t.sol | 33 ++++++--- test/playgrounds/VaultSharePrice.t.sol | 69 +++++++++++++++++++ .../VaultSharePriceManipulation.t.sol | 2 +- test/units/EverlongERC4626.t.sol | 14 ---- 7 files changed, 118 insertions(+), 65 deletions(-) create mode 100644 test/playgrounds/VaultSharePrice.t.sol rename test/{integration => playgrounds}/VaultSharePriceManipulation.t.sol (99%) diff --git a/contracts/Everlong.sol b/contracts/Everlong.sol index 127fa8d..fbf73c2 100644 --- a/contracts/Everlong.sol +++ b/contracts/Everlong.sol @@ -97,7 +97,7 @@ contract Everlong is IEverlong { /// @notice Maximum slippage allowed when closing longs with Hyperdrive. /// @dev Represented as a percentage with 1e18 signifying 100%. - uint256 public constant maxCloseLongSlippage = 0.001e18; + uint256 public constant maxCloseLongSlippage = 0.0001e18; // ───────────────────────── Immutables ────────────────────── @@ -229,6 +229,8 @@ contract Everlong is IEverlong { /// @notice Returns an approximate lower bound on the amount of assets /// received from redeeming the specified amount of shares. + /// @dev Losses and gains to the portfolio since the last `_totalAssets` + /// update are applied proportionally. /// @param _shares Amount of shares to redeem. /// @return assets Amount of assets that will be received. function previewRedeem( @@ -237,36 +239,28 @@ contract Everlong is IEverlong { // Convert the share amount to assets. assets = convertToAssets(_shares); - // TODO: Hold the vault share price constant. - // - // Apply losses incurred by the portfolio. - uint256 losses = _calculatePortfolioLosses().mulDivUp( - assets, - _totalAssets - ); - - // If the losses from closing immature positions exceeds the assets - // owed to the redeemer, set the assets owed to zero. - if (losses > assets) { - // NOTE: We return zero since `previewRedeem` must not revert. - assets = 0; - } - // Decrement the assets owed to the redeemer by the amount of losses - // incurred from closing immature positions. - else { + // Calculate and apply unrealized portfolio losses. + // Losses are rounded up. + uint256 lastTotalAssets = _totalAssets; + uint256 currentTotalAssets = _calculateTotalAssets(); + if (_totalAssets > currentTotalAssets) { unchecked { - assets -= losses; + assets -= (lastTotalAssets - currentTotalAssets).mulDivUp( + assets, + lastTotalAssets + ); } } } - /// @dev Attempt rebalancing after a deposit if idle is above max. + /// @dev Increase Everlong's total assets by the amount deposited. function _afterDeposit(uint256 _assets, uint256) internal virtual override { // Add the deposit to Everlong's assets. _totalAssets += _assets; } - /// @dev Frees sufficient assets for a withdrawal by closing positions. + /// @dev Frees sufficient assets for a withdrawal by closing positions and + /// update Everlong's total assets accounting. /// @param _assets Amount of assets owed to the withdrawer. function _beforeWithdraw( uint256 _assets, @@ -290,9 +284,8 @@ contract Everlong is IEverlong { _closePositions(_assets - balance); } - // Recalculate the assets under Everlong control less the amount being - // withdrawn. - _totalAssets = _calculateTotalAssets() - _assets; + // Decrement the assets under Everlong control by withdrawal amount. + _totalAssets -= _assets; } // ╭─────────────────────────────────────────────────────────╮ @@ -460,8 +453,6 @@ contract Everlong is IEverlong { // │ Hyperdrive │ // ╰─────────────────────────────────────────────────────────╯ - // TODO: Decide if we want to put a slippage guard here. - // /// @notice Closes mature positions in the Everlong portfolio. /// @param _limit Limit the maximum number of positions to close. /// - Zero indicates no limit. @@ -519,6 +510,10 @@ contract Everlong is IEverlong { } /// @dev Close positions until the targeted amount of output is received. + /// @dev It is possible for this function to successfully return without + /// having received the target amount of assets. In practice, this + /// does not occur due to slippage guards and underestimation + /// of Everlong's portfolio value. /// @param _targetOutput Target amount of proceeds to receive. /// @return output Total output received from closed positions. function _closePositions( @@ -560,6 +555,7 @@ contract Everlong is IEverlong { // Close only part of the position if there are sufficient bonds // to reach the target output without leaving a small amount left. + // // For this case, the remaining bonds must be worth at least // Hyperdrive's minimum transaction amount or 1% of the target // output, whichever is greater. @@ -573,7 +569,6 @@ contract Everlong is IEverlong { .getPoolConfig() .minimumTransactionAmount ) - // IHyperdrive(hyperdrive).getPoolConfig().minimumTransactionAmount ) { // Close part of the position and enforce the slippage guard. // Add the amount of assets received to the total output. @@ -636,18 +631,6 @@ contract Everlong is IEverlong { } } - /// @dev Calculates the amount of losses the portfolio has incurred since - /// `_totalAssets` was last calculated. If no losses have been incurred - /// return 0. - /// @return Amount of losses incurred by the portfolio (if any). - function _calculatePortfolioLosses() internal view returns (uint256) { - uint256 newTotalAssets = _calculateTotalAssets(); - if (_totalAssets > newTotalAssets) { - return _totalAssets - newTotalAssets; - } - return 0; - } - // ╭─────────────────────────────────────────────────────────╮ // │ Getters │ // ╰─────────────────────────────────────────────────────────╯ diff --git a/test/integration/CloseImmatureLongs.t.sol b/test/integration/CloseImmatureLongs.t.sol index a6385f5..e3e64f3 100644 --- a/test/integration/CloseImmatureLongs.t.sol +++ b/test/integration/CloseImmatureLongs.t.sol @@ -15,7 +15,7 @@ uint256 constant HYPERDRIVE_LONG_EXPOSURE_LONGS_OUTSTANDING_SLOT = 3; uint256 constant HYPERDRIVE_SHARE_ADJUSTMENT_SHORTS_OUTSTANDING_SLOT = 4; /// @dev Tests pricing functionality for the portfolio and unmatured positions. -contract CloseImmatureLongs is EverlongTest { +contract TestCloseImmatureLongs is EverlongTest { using Packing for bytes32; using FixedPointMath for uint128; using FixedPointMath for uint256; diff --git a/test/integration/PartialClosures.t.sol b/test/integration/PartialClosures.t.sol index fd384c2..1cf3918 100644 --- a/test/integration/PartialClosures.t.sol +++ b/test/integration/PartialClosures.t.sol @@ -9,7 +9,7 @@ import { EverlongTest } from "../harnesses/EverlongTest.sol"; import { IEverlong } from "../../contracts/interfaces/IEverlong.sol"; import { HyperdriveExecutionLibrary } from "../../contracts/libraries/HyperdriveExecution.sol"; -contract PartialClosures is EverlongTest { +contract TestPartialClosures is EverlongTest { using FixedPointMath for uint256; using Lib for *; using HyperdriveUtils for *; diff --git a/test/integration/Sandwich.t.sol b/test/integration/Sandwich.t.sol index f3dfdb9..9c5120c 100644 --- a/test/integration/Sandwich.t.sol +++ b/test/integration/Sandwich.t.sol @@ -6,7 +6,7 @@ import { Lib } from "hyperdrive/test/utils/Lib.sol"; import { HyperdriveUtils } from "hyperdrive/test/utils/HyperdriveUtils.sol"; import { EverlongTest } from "../harnesses/EverlongTest.sol"; -contract Sandwich is EverlongTest { +contract TestSandwich is EverlongTest { using Lib for *; using HyperdriveUtils for *; @@ -75,6 +75,8 @@ contract Sandwich is EverlongTest { } // TODO: Decrease min range to Hyperdrive `MINIMUM_TRANSACTION_AMOUNT`. + // NOTE: Rebalancing is not performed after some interactions with Everlong + // since the attack being evaluated is atomic. // /// @dev Tests the following scenario: /// 1. First an innocent bystander deposits into Everlong. At that time, we @@ -95,6 +97,9 @@ contract Sandwich is EverlongTest { address attacker = alice; address bystander = bob; + // Initialize Everlong with a bond portfolio. + depositEverlong(10_000e18, celine); + // The bystander deposits into Everlong. _bystanderDepositAmount = bound( _bystanderDepositAmount, @@ -103,7 +108,8 @@ contract Sandwich is EverlongTest { ); uint256 bystanderShares = depositEverlong( _bystanderDepositAmount, - bystander + bystander, + false ); // The attacker opens a large short. @@ -138,11 +144,16 @@ contract Sandwich is EverlongTest { // The attacker redeems their Everlong shares. uint256 attackerEverlongProceeds = redeemEverlong( attackerShares, - attacker + attacker, + false ); // The bystander redeems their Everlong shares. - redeemEverlong(bystanderShares, bystander); + // + // While not needed for the assertion below, it's included to ensure + // that the attack does not prevent the bystander from redeeming their + // shares. + redeemEverlong(bystanderShares, bystander, false); // Calculate the amount paid and the proceeds for the attacker. uint256 attackerPaid = _attackerDepositAmount + attackerShortBasePaid; @@ -150,10 +161,12 @@ contract Sandwich is EverlongTest { attackerShortProceeds; // Ensure the attacker does not profit. - assertLt(attackerProceeds, attackerPaid); + assertLe(attackerProceeds, attackerPaid); } // TODO: Decrease min range to Hyperdrive `MINIMUM_TRANSACTION_AMOUNT`. + // NOTE: Rebalancing is not performed after some interactions with Everlong + // since the attack being evaluated is atomic. // /// @dev Tests the following scenario: /// 1. Attacker adds liquidity. @@ -187,7 +200,8 @@ contract Sandwich is EverlongTest { ); uint256 bystanderEverlongShares = depositEverlong( _bystanderDeposit, - bystander + bystander, + false ); // The attacker deposits into Everlong. @@ -207,7 +221,8 @@ contract Sandwich is EverlongTest { // The attacker redeems from Everlong. uint256 attackerEverlongProceeds = redeemEverlong( attackerEverlongShares, - attacker + attacker, + false ); // The bystander redeems from Everlong. @@ -215,9 +230,9 @@ contract Sandwich is EverlongTest { // While not needed for the assertion below, it's included to ensure // that the attack does not prevent the bystander from redeeming their // shares. - redeemEverlong(bystanderEverlongShares, bystander); + redeemEverlong(bystanderEverlongShares, bystander, false); // Ensure that the attacker does not profit from their actions. - assertLt(attackerEverlongProceeds, _attackerDeposit); + assertLe(attackerEverlongProceeds, _attackerDeposit); } } diff --git a/test/playgrounds/VaultSharePrice.t.sol b/test/playgrounds/VaultSharePrice.t.sol new file mode 100644 index 0000000..264c798 --- /dev/null +++ b/test/playgrounds/VaultSharePrice.t.sol @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity 0.8.22; + +import { console2 as console } from "forge-std/console2.sol"; +import { FixedPointMath } from "hyperdrive/contracts/src/libraries/FixedPointMath.sol"; +import { Lib } from "hyperdrive/test/utils/Lib.sol"; +import { HyperdriveUtils } from "hyperdrive/test/utils/HyperdriveUtils.sol"; +import { IERC20 } from "openzeppelin/interfaces/IERC20.sol"; +import { EverlongTest } from "../harnesses/EverlongTest.sol"; +import { IEverlong } from "../../contracts/interfaces/IEverlong.sol"; +import { HyperdriveExecutionLibrary } from "../../contracts/libraries/HyperdriveExecution.sol"; + +contract TestVaultSharePrice is EverlongTest { + using FixedPointMath for uint256; + using Lib for *; + using HyperdriveUtils for *; + using HyperdriveExecutionLibrary for *; + + function test_vault_share_price_deposit_redeem() external { + // Skip this test unless disabled manually. + vm.skip(true); + + deployEverlong(); + + // Alice makes a deposit. + uint256 aliceDeposit = 10_000e18; + uint256 aliceShares = depositEverlong(aliceDeposit, alice); + + console.log( + "Vault Share Price 1: %e", + everlong.totalAssets().divDown(everlong.totalSupply()) + ); + + // Bob makes a deposit. + uint256 bobDeposit = 10_000e18; + uint256 bobShares = depositEverlong(bobDeposit, bob); + console.log( + "Vault Share Price 2: %e", + everlong.totalAssets().divDown(everlong.totalSupply()) + ); + + // Celine makes a deposit. + uint256 celineDeposit = 10_000e18; + uint256 celineShares = depositEverlong(celineDeposit, celine); + console.log( + "Vault Share Price 3: %e", + everlong.totalAssets().divDown(everlong.totalSupply()) + ); + + // Bob redeems. + redeemEverlong(bobShares, bob); + console.log( + "Vault Share Price 4: %e", + everlong.totalAssets().divDown(everlong.totalSupply()) + ); + + // Celine redeems. + redeemEverlong(celineShares, celine); + console.log( + "Vault Share Price 5: %e", + everlong.totalAssets().divDown(everlong.totalSupply()) + ); + + console.log( + "Everlong Balance: %e", + IERC20(everlong.asset()).balanceOf(address(everlong)) + ); + } +} diff --git a/test/integration/VaultSharePriceManipulation.t.sol b/test/playgrounds/VaultSharePriceManipulation.t.sol similarity index 99% rename from test/integration/VaultSharePriceManipulation.t.sol rename to test/playgrounds/VaultSharePriceManipulation.t.sol index d271619..4f33bc9 100644 --- a/test/integration/VaultSharePriceManipulation.t.sol +++ b/test/playgrounds/VaultSharePriceManipulation.t.sol @@ -18,7 +18,7 @@ uint256 constant HYPERDRIVE_LONG_EXPOSURE_LONGS_OUTSTANDING_SLOT = 3; uint256 constant HYPERDRIVE_SHARE_ADJUSTMENT_SHORTS_OUTSTANDING_SLOT = 4; /// @dev Tests vault share price manipulation with the underlying hyperdrive instance. -contract VaultSharePriceManipulation is EverlongTest { +contract TestVaultSharePriceManipulation is EverlongTest { using Packing for bytes32; using FixedPointMath for uint128; using FixedPointMath for uint256; diff --git a/test/units/EverlongERC4626.t.sol b/test/units/EverlongERC4626.t.sol index 6b099c7..1d9031f 100644 --- a/test/units/EverlongERC4626.t.sol +++ b/test/units/EverlongERC4626.t.sol @@ -92,18 +92,4 @@ contract TestEverlongERC4626 is EverlongTest { // and within margins. assertRedemption(shares / 3, alice); } - - /// @dev Tests that the `_beforeWithdraw` hook doesn't underflow when - /// Everlong's balance is greater than the assets being redeemed. - function test_beforeWithdraw_balance_gt_assets() external { - // Deploy Everlong. - deployEverlong(); - - // Mint some assets to everlong - uint256 assets = 100e18; - mintApproveEverlongBaseAsset(address(everlong), assets); - - // Call the `_beforeWithdraw` hook. - everlong.exposed_beforeWithdraw(assets - 10e18, 0); - } }