another master vault design #125
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // maybe a simpler and more robust implementation would be for the owner to adjust the subVaultExchRateWad directly | ||
| // this would also avoid the need for totalPrincipal tracking | ||
| // however, this would require more trust in the owner | ||
| uint256 public performanceFeeBps; // in basis points, e.g. 200 = 2% | todo a way to set this |
Check failure
Code scanning / Slither
Uninitialized state variables High
Comment on lines
+176
to
+188
| function _deposit( | ||
| address caller, | ||
| address receiver, | ||
| uint256 assets, | ||
| uint256 shares | ||
| ) internal virtual override { | ||
| super._deposit(caller, receiver, assets, shares); | ||
| totalPrincipal += assets; | ||
| ERC4626 _subVault = subVault; | ||
| if (address(_subVault) != address(0)) { | ||
| _subVault.deposit(assets, address(this)); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
Comment on lines
193
to
229
| function _withdraw( | ||
| address caller, | ||
| address receiver, | ||
| address _owner, | ||
| uint256 assets, | ||
| uint256 shares | ||
| ) internal virtual override { | ||
| ERC4626 _subVault = subVault; | ||
| if (address(_subVault) != address(0)) { | ||
| _subVault.withdraw(assets, address(this), address(this)); | ||
| } | ||
|
|
||
| ////// PERF FEE STUFF ////// | ||
| // determine profit portion and principal portion of assets | ||
| uint256 _totalProfit = totalProfit(); | ||
| // use shares because they are rounded up vs assets which are rounded down | ||
| uint256 profitPortion = shares.mulDiv(_totalProfit, totalSupply(), Math.Rounding.Up); | ||
| uint256 principalPortion = assets - profitPortion; | ||
|
|
||
| // subtract principal portion from totalPrincipal | ||
| totalPrincipal -= principalPortion; | ||
|
|
||
| // send fee to owner | ||
| if (performanceFeeBps > 0 && profitPortion > 0) { | ||
| uint256 fee = profitPortion.mulDiv(performanceFeeBps, 10000, Math.Rounding.Up); | ||
| // send fee to owner | ||
| IERC20(asset()).safeTransfer(owner(), fee); | ||
|
|
||
| // note subtraction | ||
| assets -= fee; | ||
| } | ||
|
|
||
| ////// END PERF FEE STUFF ////// | ||
|
|
||
| // call super._withdraw with remaining assets | ||
| super._withdraw(caller, receiver, _owner, assets, shares); | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
waelsy123
reviewed
Sep 25, 2025
waelsy123
reviewed
Sep 25, 2025
waelsy123
reviewed
Sep 25, 2025
waelsy123
reviewed
Sep 25, 2025
| totalPrincipal += assets; | ||
| ERC4626 _subVault = subVault; | ||
| if (address(_subVault) != address(0)) { | ||
| _subVault.deposit(assets, address(this)); |
waelsy123
approved these changes
Sep 25, 2025
thanks! the requirement is that we have a switch for 100% or 0% performance fee, so may as well just make it variable between 0 and 100 🤷 |
Comment on lines
+64
to
+83
| function setSubVault(ERC4626 _subVault, uint256 minSubVaultExchRateWad) external onlyOwner { | ||
| require(address(subVault) == address(0), "subvault already set"); | ||
| require(address(_subVault) != address(0), "subvault cannot be zero address"); | ||
| require(totalSupply() > 0, "must have supply before setting subvault"); | ||
| require(address(_subVault.asset()) == address(asset()), "subvault asset mismatch"); | ||
|
|
||
| // deposit to subvault | ||
| IERC20(asset()).safeApprove(address(_subVault), type(uint256).max); | ||
| uint256 subShares = _subVault.deposit(totalAssets(), address(this)); | ||
|
|
||
| // set new exchange rate | ||
| uint256 _subVaultExchRateWad = subShares.mulDiv(1e18, totalSupply(), Math.Rounding.Down); // todo: confirm rounding direction | ||
| require(_subVaultExchRateWad >= minSubVaultExchRateWad, "subvault exchange rate too low"); | ||
| subVaultExchRateWad = _subVaultExchRateWad; | ||
|
|
||
| // set subvault | ||
| subVault = _subVault; | ||
|
|
||
| emit SubvaultChanged(address(0), address(_subVault)); | ||
| } |
Check warning
Code scanning / Slither
Reentrancy vulnerabilities Medium
Comment on lines
+90
to
+125
| function switchSubVault(ERC4626 newSubVault, uint256 minAssetExchRateWad, uint256 minNewSubVaultExchRateWad) external onlyOwner { | ||
| ERC4626 oldSubVault = subVault; | ||
| require(address(oldSubVault) != address(0), "no existing subvault"); | ||
| if (address(newSubVault) != address(0)) { | ||
| require(totalSupply() > 0, "must have supply before switching subvault"); | ||
| require(address(newSubVault.asset()) == address(asset()), "subvault asset mismatch"); | ||
| } | ||
|
|
||
| // withdraw from old subvault | ||
| uint256 _totalSupply = totalSupply(); | ||
| uint256 assetReceived = oldSubVault.withdraw(oldSubVault.maxWithdraw(address(this)), address(this), address(this)); | ||
| uint256 effectiveAssetExchRateWad = assetReceived.mulDiv(1e18, _totalSupply, Math.Rounding.Down); // todo: confirm rounding direction | ||
| require(effectiveAssetExchRateWad >= minAssetExchRateWad, "too few assets received"); | ||
|
|
||
| // revoke approval from old subvault | ||
| IERC20(asset()).safeApprove(address(oldSubVault), 0); | ||
|
|
||
| // deposit to new subvault | ||
| if (address(newSubVault) != address(0)) { | ||
| IERC20(asset()).safeApprove(address(newSubVault), type(uint256).max); | ||
| uint256 newSubShares = newSubVault.deposit(totalAssets(), address(this)); | ||
|
|
||
| // set new exchange rate | ||
| uint256 _subVaultExchRateWad = newSubShares.mulDiv(1e18, totalSupply(), Math.Rounding.Down); | ||
| require(_subVaultExchRateWad >= minNewSubVaultExchRateWad, "new subvault exchange rate too low"); | ||
| subVaultExchRateWad = _subVaultExchRateWad; | ||
| } | ||
| else { | ||
| subVaultExchRateWad = 1e18; | ||
| } | ||
|
|
||
| // set subvault | ||
| subVault = newSubVault; | ||
|
|
||
| emit SubvaultChanged(address(oldSubVault), address(newSubVault)); | ||
| } |
Check warning
Code scanning / Slither
Reentrancy vulnerabilities Medium
|
added a commit to change the way the owner specifies slippage tolerance when setting the subvault. it's an exchange rate based tolerance now which is safer against frontrunning. idea being that the owner doesn't know how many shares/assets the mastervault will have when their |
waelsy123
reviewed
Sep 26, 2025
waelsy123
approved these changes
Sep 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this version should be robust to changes in the subvault's exchange rate.
it should be more robust to slippage (ie on an LP strategy)
the performance fee, however, is not robust to MEV