From a6cb55818ee6869dcb6b09da2b938e2e83af4c4b Mon Sep 17 00:00:00 2001 From: Schlag <89420541+Schlagonia@users.noreply.github.com> Date: Fri, 2 Feb 2024 13:38:16 -0700 Subject: [PATCH] fix: minor changes (#82) * chore: remove min fee * fix: tests * build: cheaper reentrancy * chore: fix spech * feat: remove internal preview * feat: make factory immutable (#87) --- SPECIFICATION.md | 2 +- src/TokenizedStrategy.sol | 156 +++++++++++--------------- src/interfaces/ITokenizedStrategy.sol | 2 - src/test/AccessControl.t.sol | 4 - src/test/e2e.t.sol | 9 +- src/test/utils/Setup.sol | 47 +------- 6 files changed, 78 insertions(+), 142 deletions(-) diff --git a/SPECIFICATION.md b/SPECIFICATION.md index 99d6f2d..e5d8fa9 100644 --- a/SPECIFICATION.md +++ b/SPECIFICATION.md @@ -155,7 +155,7 @@ A strategies management can set another address to serve as an `emergencyAdmin`. #### Setting Performance Fee Setting the percent in terms of basis points for the amount of profit to be charged as a fee. -This has a minimum of 5% and a maximum of 50%. +There is a maximum of 50%. #### Setting performance fee recipient Setting the non-zero address that will receive any shares issued as a result of the performance fee. diff --git a/src/TokenizedStrategy.sol b/src/TokenizedStrategy.sol index 4b0e88e..69c8405 100644 --- a/src/TokenizedStrategy.sol +++ b/src/TokenizedStrategy.sol @@ -243,11 +243,9 @@ contract TokenizedStrategy { address pendingManagement; // Address that is pending to take over `management`. address emergencyAdmin; // Address to act in emergencies as well as `management`. - - // Strategy status checks. - bool entered; // Bool to prevent reentrancy. - bool shutdown; // Bool that can be used to stop deposits into the strategy. - + // Strategy Status + uint8 entered; // To prevent reentrancy. Use uint8 for gas savings. + bool shutdown; // Bool that can be used to stop deposits into the strategy. } /*////////////////////////////////////////////////////////////// @@ -287,15 +285,15 @@ contract TokenizedStrategy { modifier nonReentrant() { StrategyData storage S = _strategyStorage(); // On the first call to nonReentrant, `entered` will be false - require(!S.entered, "ReentrancyGuard: reentrant call"); + require(S.entered != ENTERED, "ReentrancyGuard: reentrant call"); // Any calls to nonReentrant after this point will fail - S.entered = true; + S.entered = ENTERED; _; // Reset to false once call has finished - S.entered = false; + S.entered = NOT_ENTERED; } /** @@ -349,25 +347,25 @@ contract TokenizedStrategy { /// @notice API version this TokenizedStrategy implements. string internal constant API_VERSION = "3.0.2"; + /// @notice Value to set the `entered` flag to during a call. + uint8 internal constant ENTERED = 2; + /// @notice Value to set the `entered` flag to at the end of the call. + uint8 internal constant NOT_ENTERED = 1; + + /// @notice Maximum in Basis Points the Performance Fee can be set to. + uint16 public constant MAX_FEE = 5_000; // 50% + /// @notice Used for fee calculations. uint256 internal constant MAX_BPS = 10_000; /// @notice Used for profit unlocking rate calculations. uint256 internal constant MAX_BPS_EXTENDED = 1_000_000_000_000; - /// @notice Minimum in Basis points the Performance fee can be set to. - // Used to disincentive forking strategies just to lower fees. - uint16 public constant MIN_FEE = 500; // 5% - /// @notice Maximum in Basis Points the Performance Fee can be set to. - uint16 public constant MAX_FEE = 5_000; // 50% - /// @notice Seconds per year for max profit unlocking time. uint256 internal constant SECONDS_PER_YEAR = 31_556_952; // 365.2425 days /// @notice Address of the previously deployed Vault factory that the // protocol fee config is retrieved from. - // NOTE: This will be set to deployed factory. deterministic address for testing is used now - address public constant FACTORY = - 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f; + address public immutable FACTORY; /** * @dev Custom storage slot that will be used to store the @@ -504,7 +502,10 @@ contract TokenizedStrategy { "ERC4626: deposit more than max" ); // Check for rounding error. - require((shares = _previewDeposit(S, assets)) != 0, "ZERO_SHARES"); + require( + (shares = _convertToShares(S, assets, Math.Rounding.Down)) != 0, + "ZERO_SHARES" + ); _deposit(S, receiver, assets, shares); } @@ -525,7 +526,10 @@ contract TokenizedStrategy { // Checking max mint will also check if shutdown. require(shares <= _maxMint(S, receiver), "ERC4626: mint more than max"); // Check for rounding error. - require((assets = _previewMint(S, shares)) != 0, "ZERO_ASSETS"); + require( + (assets = _convertToAssets(S, shares, Math.Rounding.Up)) != 0, + "ZERO_ASSETS" + ); _deposit(S, receiver, assets, shares); } @@ -570,7 +574,10 @@ contract TokenizedStrategy { "ERC4626: withdraw more than max" ); // Check for rounding error or 0 value. - require((shares = _previewWithdraw(S, assets)) != 0, "ZERO_SHARES"); + require( + (shares = _convertToShares(S, assets, Math.Rounding.Up)) != 0, + "ZERO_SHARES" + ); // Withdraw and track the actual amount withdrawn for loss check. _withdraw(S, receiver, owner, assets, shares, maxLoss); @@ -618,7 +625,10 @@ contract TokenizedStrategy { ); uint256 assets; // Check for rounding error or 0 value. - require((assets = _previewRedeem(S, shares)) != 0, "ZERO_ASSETS"); + require( + (assets = _convertToAssets(S, shares, Math.Rounding.Down)) != 0, + "ZERO_ASSETS" + ); // We need to return the actual amount withdrawn in case of a loss. return _withdraw(S, receiver, owner, assets, shares, maxLoss); @@ -664,7 +674,7 @@ contract TokenizedStrategy { * @return . Expected shares that `assets` represents. */ function convertToShares(uint256 assets) external view returns (uint256) { - return _convertToShares(_strategyStorage(), assets); + return _convertToShares(_strategyStorage(), assets, Math.Rounding.Down); } /** @@ -676,7 +686,7 @@ contract TokenizedStrategy { * @return . Expected amount of `asset` the shares represents. */ function convertToAssets(uint256 shares) external view returns (uint256) { - return _convertToAssets(_strategyStorage(), shares); + return _convertToAssets(_strategyStorage(), shares, Math.Rounding.Down); } /** @@ -689,7 +699,7 @@ contract TokenizedStrategy { * @return . Expected shares that would be issued. */ function previewDeposit(uint256 assets) external view returns (uint256) { - return _previewDeposit(_strategyStorage(), assets); + return _convertToShares(_strategyStorage(), assets, Math.Rounding.Down); } /** @@ -703,7 +713,7 @@ contract TokenizedStrategy { * @return . The needed amount of `asset` for the mint. */ function previewMint(uint256 shares) external view returns (uint256) { - return _previewMint(_strategyStorage(), shares); + return _convertToAssets(_strategyStorage(), shares, Math.Rounding.Up); } /** @@ -717,7 +727,7 @@ contract TokenizedStrategy { * @return . The amount of shares that would be burnt. */ function previewWithdraw(uint256 assets) external view returns (uint256) { - return _previewWithdraw(_strategyStorage(), assets); + return _convertToShares(_strategyStorage(), assets, Math.Rounding.Up); } /** @@ -730,7 +740,7 @@ contract TokenizedStrategy { * @return . The amount of `asset` that would be returned. */ function previewRedeem(uint256 shares) external view returns (uint256) { - return _previewRedeem(_strategyStorage(), shares); + return _convertToAssets(_strategyStorage(), shares, Math.Rounding.Down); } /** @@ -802,7 +812,8 @@ contract TokenizedStrategy { /// @dev Internal implementation of {convertToShares}. function _convertToShares( StrategyData storage S, - uint256 assets + uint256 assets, + Math.Rounding _rounding ) internal view returns (uint256) { // Saves an extra SLOAD if totalAssets() is non-zero. uint256 totalAssets_ = _totalAssets(S); @@ -811,35 +822,14 @@ contract TokenizedStrategy { // If assets are 0 but supply is not PPS = 0. if (totalAssets_ == 0) return totalSupply_ == 0 ? assets : 0; - return assets.mulDiv(totalSupply_, totalAssets_, Math.Rounding.Down); + return assets.mulDiv(totalSupply_, totalAssets_, _rounding); } /// @dev Internal implementation of {convertToAssets}. function _convertToAssets( StrategyData storage S, - uint256 shares - ) internal view returns (uint256) { - // Saves an extra SLOAD if totalSupply() is non-zero. - uint256 supply = _totalSupply(S); - - return - supply == 0 - ? shares - : shares.mulDiv(_totalAssets(S), supply, Math.Rounding.Down); - } - - /// @dev Internal implementation of {previewDeposit}. - function _previewDeposit( - StrategyData storage S, - uint256 assets - ) internal view returns (uint256) { - return _convertToShares(S, assets); - } - - /// @dev Internal implementation of {previewMint}. - function _previewMint( - StrategyData storage S, - uint256 shares + uint256 shares, + Math.Rounding _rounding ) internal view returns (uint256) { // Saves an extra SLOAD if totalSupply() is non-zero. uint256 supply = _totalSupply(S); @@ -847,30 +837,7 @@ contract TokenizedStrategy { return supply == 0 ? shares - : shares.mulDiv(_totalAssets(S), supply, Math.Rounding.Up); - } - - /// @dev Internal implementation of {previewWithdraw}. - function _previewWithdraw( - StrategyData storage S, - uint256 assets - ) internal view returns (uint256) { - // Saves an extra SLOAD if totalAssets() is non-zero. - uint256 totalAssets_ = _totalAssets(S); - uint256 totalSupply_ = _totalSupply(S); - - // If assets are 0 but supply is not, then PPS = 0. - if (totalAssets_ == 0) return totalSupply_ == 0 ? assets : 0; - - return assets.mulDiv(totalSupply_, totalAssets_, Math.Rounding.Up); - } - - /// @dev Internal implementation of {previewRedeem}. - function _previewRedeem( - StrategyData storage S, - uint256 shares - ) internal view returns (uint256) { - return _convertToAssets(S, shares); + : shares.mulDiv(_totalAssets(S), supply, _rounding); } /// @dev Internal implementation of {maxDeposit}. @@ -894,7 +861,7 @@ contract TokenizedStrategy { maxMint_ = IBaseStrategy(address(this)).availableDepositLimit(owner); if (maxMint_ != type(uint256).max) { - maxMint_ = _convertToShares(S, maxMint_); + maxMint_ = _convertToShares(S, maxMint_, Math.Rounding.Down); } } @@ -911,10 +878,14 @@ contract TokenizedStrategy { // If there is no limit enforced. if (maxWithdraw_ == type(uint256).max) { // Saves a min check if there is no withdrawal limit. - maxWithdraw_ = _convertToAssets(S, _balanceOf(S, owner)); + maxWithdraw_ = _convertToAssets( + S, + _balanceOf(S, owner), + Math.Rounding.Down + ); } else { maxWithdraw_ = Math.min( - _convertToAssets(S, _balanceOf(S, owner)), + _convertToAssets(S, _balanceOf(S, owner), Math.Rounding.Down), maxWithdraw_ ); } @@ -934,7 +905,7 @@ contract TokenizedStrategy { } else { maxRedeem_ = Math.min( // Can't redeem more than the balance. - _convertToShares(S, maxRedeem_), + _convertToShares(S, maxRedeem_, Math.Rounding.Down), _balanceOf(S, owner) ); } @@ -1143,11 +1114,16 @@ contract TokenizedStrategy { unchecked { performanceFeeShares = _convertToShares( S, - totalFees - protocolFees + totalFees - protocolFees, + Math.Rounding.Down ); } if (protocolFees != 0) { - protocolFeeShares = _convertToShares(S, protocolFees); + protocolFeeShares = _convertToShares( + S, + protocolFees, + Math.Rounding.Down + ); } } @@ -1155,7 +1131,11 @@ contract TokenizedStrategy { if (_profitMaxUnlockTime != 0) { // lock (profit - fees) unchecked { - sharesToLock = _convertToShares(S, profit - totalFees); + sharesToLock = _convertToShares( + S, + profit - totalFees, + Math.Rounding.Down + ); } // Mint the shares to lock the strategy. _mint(S, address(this), sharesToLock); @@ -1181,7 +1161,7 @@ contract TokenizedStrategy { // to offset the loss to prevent any PPS decline post report. uint256 sharesToBurn = Math.min( S.balances[address(this)], - _convertToShares(S, loss) + _convertToShares(S, loss, Math.Rounding.Down) ); // Check if there is anything to burn. @@ -1481,7 +1461,7 @@ contract TokenizedStrategy { */ function pricePerShare() external view returns (uint256) { StrategyData storage S = _strategyStorage(); - return _convertToAssets(S, 10 ** S.decimals); + return _convertToAssets(S, 10 ** S.decimals, Math.Rounding.Down); } /** @@ -1557,13 +1537,11 @@ contract TokenizedStrategy { * @dev Can only be called by the current `management`. * * Denominated in Basis Points. So 100% == 10_000. - * Cannot be set less than the MIN_FEE. * Cannot set greater than to MAX_FEE. * * @param _performanceFee New performance fee. */ function setPerformanceFee(uint16 _performanceFee) external onlyManagement { - require(_performanceFee >= MIN_FEE, "MIN FEE"); require(_performanceFee <= MAX_FEE, "MAX FEE"); _strategyStorage().performanceFee = _performanceFee; @@ -2058,8 +2036,10 @@ contract TokenizedStrategy { /** * @dev On contract creation we set `asset` for this contract to address(1). * This prevents it from ever being initialized in the future. + * @param _factory Address of the factory of the same version for protocol fees. */ - constructor() { + constructor(address _factory) { + FACTORY = _factory; _strategyStorage().asset = ERC20(address(1)); } } diff --git a/src/interfaces/ITokenizedStrategy.sol b/src/interfaces/ITokenizedStrategy.sol index d13b2af..d5877b8 100644 --- a/src/interfaces/ITokenizedStrategy.sol +++ b/src/interfaces/ITokenizedStrategy.sol @@ -94,8 +94,6 @@ interface ITokenizedStrategy is IERC4626, IERC20Permit { CONSTANTS //////////////////////////////////////////////////////////////*/ - function MIN_FEE() external view returns (uint16); - function MAX_FEE() external view returns (uint16); function FACTORY() external view returns (address); diff --git a/src/test/AccessControl.t.sol b/src/test/AccessControl.t.sol index 4c1367c..0644e2c 100644 --- a/src/test/AccessControl.t.sol +++ b/src/test/AccessControl.t.sol @@ -153,10 +153,6 @@ contract AccessControlTest is Setup { assertEq(strategy.performanceFee(), _performanceFee); - vm.prank(management); - vm.expectRevert("MIN FEE"); - strategy.setPerformanceFee(uint16(5)); - vm.prank(management); vm.expectRevert("MAX FEE"); strategy.setPerformanceFee(uint16(_amount + 5_001)); diff --git a/src/test/e2e.t.sol b/src/test/e2e.t.sol index c43b706..1c7a9d0 100644 --- a/src/test/e2e.t.sol +++ b/src/test/e2e.t.sol @@ -48,7 +48,8 @@ contract e2eTest is Setup { _address != address(newStrategy) ); - setPerformanceFeeToZero(address(newStrategy)); + vm.prank(management); + newStrategy.setPerformanceFee(0); // Deposit a unique amount for each one uint256 toDeposit = _amount + i; @@ -143,7 +144,8 @@ contract e2eTest is Setup { _address != address(newStrategy) ); - setPerformanceFeeToZero(address(newStrategy)); + vm.prank(management); + newStrategy.setPerformanceFee(0); // Deposit a unique amount for each one uint256 toDeposit = _amount + i; @@ -262,7 +264,8 @@ contract e2eTest is Setup { _secondAddress != address(newStrategy) ); - setPerformanceFeeToZero(address(newStrategy)); + vm.prank(management); + newStrategy.setPerformanceFee(0); // Deposit a unique amount for each one uint256 toDeposit = _amount + i; diff --git a/src/test/utils/Setup.sol b/src/test/utils/Setup.sol index 33495a5..f63031c 100644 --- a/src/test/utils/Setup.sol +++ b/src/test/utils/Setup.sol @@ -49,7 +49,7 @@ contract Setup is ExtendedTest, IEvents { mockFactory = new MockFactory(0, protocolFeeRecipient); // Deploy the implementation for deterministic location - tokenizedStrategy = new TokenizedStrategy(); + tokenizedStrategy = new TokenizedStrategy(address(mockFactory)); // create asset we will be using as the underlying asset asset = new ERC20Mock(); @@ -262,49 +262,8 @@ contract Setup is ExtendedTest, IEvents { function setFees(uint16 _protocolFee, uint16 _performanceFee) public { mockFactory.setFee(_protocolFee); - // If 0 is passed for testing purposes we manually override - // the minimum set in the TokenizedStrategy. - if (_performanceFee == 0) { - setPerformanceFeeToZero(address(strategy)); - } else { - vm.prank(management); - strategy.setPerformanceFee(_performanceFee); - } - } - - // For easier calculations we may want to set the performance fee - // to 0 in some tests which is underneath the minimum. So we do it manually. - function setPerformanceFeeToZero(address _strategy) public { - bytes32 slot; - TokenizedStrategy.StrategyData storage S = _strategyStorage(); - - assembly { - // Perf fee is stored in the 10th slot of the Struct. - slot := add(S.slot, 10) - } - - // Performance fee is packed in a slot with other variables so we need - // to maintain the same variables packed in the slot - - // profitMaxUnlock time is a uint32 at the most significant spot. - bytes32 data = bytes4( - uint32(IMockStrategy(_strategy).profitMaxUnlockTime()) - ); - // Free up space for the uint16 of performanceFee - data = data >> 16; - // Store 0 in the performance fee spot. - data |= bytes2(0); - // Shit 160 bits for an address - data = data >> 160; - // Store the strategies performance fee recipient - data |= bytes20( - uint160(IMockStrategy(_strategy).performanceFeeRecipient()) - ); - // Shift the remainder of padding. - data = data >> 48; - - // Manually set the storage slot that holds the performance fee to 0 - vm.store(_strategy, slot, data); + vm.prank(management); + strategy.setPerformanceFee(_performanceFee); } function setupWhitelist(address _address) public {