Skip to content

Commit

Permalink
fix: minor changes (#82)
Browse files Browse the repository at this point in the history 
* chore: remove min fee

* fix: tests

* build: cheaper reentrancy

* chore: fix spech

* feat: remove internal preview

* feat: make factory immutable (#87)
  • Loading branch information
@Schlagonia
Schlagonia authored Feb 2, 2024
1 parent a3ace32 commit a6cb558
Show file tree
Hide file tree
Showing 6 changed files with 78 additions and 142 deletions.
2 changes: 1 addition & 1 deletion SPECIFICATION.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
156 changes: 68 additions & 88 deletions src/TokenizedStrategy.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}

/*//////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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);
Expand All @@ -811,66 +822,22 @@ 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);

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}.
Expand All @@ -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);
}
}

Expand All @@ -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_
);
}
Expand All @@ -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)
);
}
Expand Down Expand Up @@ -1143,19 +1114,28 @@ 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
);
}
}

// we have a net profit. Check if we are locking profit.
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);
Expand All @@ -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.
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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));
}
}
2 changes: 0 additions & 2 deletions src/interfaces/ITokenizedStrategy.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 0 additions & 4 deletions src/test/AccessControl.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
Loading

0 comments on commit a6cb558

Please sign in to comment.