feat: add master vault #126
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}
Comment on lines
+87
to
+102
| function _setSubVault(ERC4626 _subVault, uint256 minSubVaultExchRateWad) internal { | ||
| if (address(_subVault) == address(0)) revert SubVaultCannotBeZeroAddress(); | ||
| if (totalSupply() == 0) revert MustHaveSupplyBeforeSettingSubVault(); | ||
| if (address(_subVault.asset()) != address(asset())) revert SubVaultAssetMismatch(); | ||
|
|
||
| IERC20(asset()).safeApprove(address(_subVault), type(uint256).max); | ||
| uint256 subShares = _subVault.deposit(totalAssets(), address(this)); | ||
|
|
||
| uint256 _subVaultExchRateWad = subShares.mulDiv(1e18, totalSupply(), Math.Rounding.Down); | ||
| if (_subVaultExchRateWad < minSubVaultExchRateWad) revert SubVaultExchangeRateTooLow(); | ||
| subVaultExchRateWad = _subVaultExchRateWad; | ||
|
|
||
| subVault = _subVault; | ||
|
|
||
| emit SubvaultChanged(address(0), address(_subVault)); | ||
| } |
Check warning
Code scanning / Slither
Dangerous strict equalities Medium
Comment on lines
+87
to
+102
| function _setSubVault(ERC4626 _subVault, uint256 minSubVaultExchRateWad) internal { | ||
| if (address(_subVault) == address(0)) revert SubVaultCannotBeZeroAddress(); | ||
| if (totalSupply() == 0) revert MustHaveSupplyBeforeSettingSubVault(); | ||
| if (address(_subVault.asset()) != address(asset())) revert SubVaultAssetMismatch(); | ||
|
|
||
| IERC20(asset()).safeApprove(address(_subVault), type(uint256).max); | ||
| uint256 subShares = _subVault.deposit(totalAssets(), address(this)); | ||
|
|
||
| uint256 _subVaultExchRateWad = subShares.mulDiv(1e18, totalSupply(), Math.Rounding.Down); | ||
| if (_subVaultExchRateWad < minSubVaultExchRateWad) revert SubVaultExchangeRateTooLow(); | ||
| subVaultExchRateWad = _subVaultExchRateWad; | ||
|
|
||
| subVault = _subVault; | ||
|
|
||
| emit SubvaultChanged(address(0), address(_subVault)); | ||
| } |
Check warning
Code scanning / Slither
Reentrancy vulnerabilities Medium
Comment on lines
+104
to
+118
| function _revokeSubVault(uint256 minAssetExchRateWad) internal { | ||
| ERC4626 oldSubVault = subVault; | ||
| if (address(oldSubVault) == address(0)) revert NoExistingSubVault(); | ||
|
|
||
| uint256 _totalSupply = totalSupply(); | ||
| uint256 assetReceived = oldSubVault.withdraw(oldSubVault.maxWithdraw(address(this)), address(this), address(this)); | ||
| uint256 effectiveAssetExchRateWad = assetReceived.mulDiv(1e18, _totalSupply, Math.Rounding.Down); | ||
| if (effectiveAssetExchRateWad < minAssetExchRateWad) revert TooFewAssetsReceived(); | ||
|
|
||
| IERC20(asset()).safeApprove(address(oldSubVault), 0); | ||
| subVault = ERC4626(address(0)); | ||
| subVaultExchRateWad = 1e18; | ||
|
|
||
| emit SubvaultChanged(address(oldSubVault), address(0)); | ||
| } |
Check warning
Code scanning / Slither
Reentrancy vulnerabilities Medium
Comment on lines
+201
to
+213
| 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
218
to
254
| 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 (todo should be a separate beneficiary addr set by 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
Comment on lines
+162
to
+174
| function withdrawPerformanceFees() external onlyOwner { | ||
| if (!enablePerformanceFee) revert PerformanceFeeDisabled(); | ||
| if (beneficiary == address(0)) revert BeneficiaryNotSet(); | ||
|
|
||
| uint256 totalProfits = totalProfit(); | ||
| if (totalProfits > 0) { | ||
| ERC4626 _subVault = subVault; | ||
| if (address(_subVault) != address(0)) { | ||
| _subVault.withdraw(totalProfits, address(this), address(this)); | ||
| } | ||
| IERC20(asset()).safeTransfer(beneficiary, totalProfits); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
waelsy123
force-pushed
the
wa/master-vault-isolated
branch
from
590a713 to
fc89964
Compare
godzillaba
reviewed
Oct 1, 2025
| if (address(oldSubVault) == address(0)) revert NoExistingSubVault(); | ||
|
|
||
| uint256 _totalSupply = totalSupply(); | ||
| uint256 assetReceived = oldSubVault.withdraw(oldSubVault.maxWithdraw(address(this)), address(this), address(this)); |
There was a problem hiding this comment.
there is an edge case here - the subvault may not have enough liquidity to serve this big withdrawal all at once.
we probably need to make switching vaults more robust to those liquidity constaints.
the same could be said about depositing to the new vault, it could be such a large deposit that slippage starts to become a serious issue
There was a problem hiding this comment.
we could do check whether maxWithdraw will return same amount of what master vault actually own
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 isolate the implementation for the master vault and its related factory and subvault.