feat: add gradual vault migration with partial allocation control #129
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function currentAllocationBps() public view returns (uint256) { | ||
| uint256 _totalAssets = totalAssets(); | ||
| if (_totalAssets == 0) return 0; | ||
|
|
||
| ERC4626 _subVault = subVault; | ||
| if (address(_subVault) == address(0)) return 0; | ||
|
|
||
| uint256 subVaultAssets = _subVault.convertToAssets(_subVault.balanceOf(address(this))); | ||
| return subVaultAssets.mulDiv(10000, _totalAssets, Math.Rounding.Down); | ||
| } |
Check warning
Code scanning / Slither
Dangerous strict equalities Medium
| function _rebalance(uint256 maxSlippageBps) internal { | ||
| ERC4626 _subVault = subVault; | ||
| if (address(_subVault) == address(0)) revert NoSubVaultToRebalance(); | ||
|
|
||
| uint256 _totalAssets = totalAssets(); | ||
| if (_totalAssets == 0) revert NoAssetsToRebalance(); | ||
|
|
||
| uint256 currentBps = currentAllocationBps(); | ||
| uint256 targetBps = targetSubVaultAllocationBps; | ||
|
|
||
| uint256 movedToSubVault = 0; | ||
| uint256 withdrawnFromSubVault = 0; | ||
|
|
||
| if (currentBps < targetBps) { | ||
| uint256 targetSubVaultAssets = _totalAssets.mulDiv(targetBps, 10000, Math.Rounding.Down); | ||
| uint256 currentSubVaultAssets = _subVault.convertToAssets(_subVault.balanceOf(address(this))); | ||
| uint256 assetsToDeposit = targetSubVaultAssets > currentSubVaultAssets | ||
| ? targetSubVaultAssets - currentSubVaultAssets | ||
| : 0; | ||
|
|
||
| if (assetsToDeposit > 0) { | ||
| uint256 liquidAssets = IERC20(asset()).balanceOf(address(this)); | ||
| assetsToDeposit = assetsToDeposit > liquidAssets ? liquidAssets : assetsToDeposit; | ||
|
|
||
| if (assetsToDeposit > 0) { | ||
| uint256 minShares = assetsToDeposit.mulDiv(10000 - maxSlippageBps, 10000, Math.Rounding.Down); | ||
| uint256 sharesReceived = _subVault.deposit(assetsToDeposit, address(this)); | ||
| if (sharesReceived < minShares) revert SubVaultExchangeRateTooLow(); | ||
| movedToSubVault = assetsToDeposit; | ||
| } | ||
| } | ||
| } else if (currentBps > targetBps) { | ||
| uint256 targetSubVaultAssets = _totalAssets.mulDiv(targetBps, 10000, Math.Rounding.Down); | ||
| uint256 currentSubVaultAssets = _subVault.convertToAssets(_subVault.balanceOf(address(this))); | ||
| uint256 assetsToWithdraw = currentSubVaultAssets > targetSubVaultAssets | ||
| ? currentSubVaultAssets - targetSubVaultAssets | ||
| : 0; | ||
|
|
||
| if (assetsToWithdraw > 0) { | ||
| uint256 maxWithdrawable = _subVault.maxWithdraw(address(this)); | ||
| assetsToWithdraw = assetsToWithdraw > maxWithdrawable ? maxWithdrawable : assetsToWithdraw; | ||
|
|
||
| if (assetsToWithdraw > 0) { | ||
| uint256 minAssets = assetsToWithdraw.mulDiv(10000 - maxSlippageBps, 10000, Math.Rounding.Down); | ||
| uint256 assetsReceived = _subVault.withdraw(assetsToWithdraw, address(this), address(this)); | ||
| if (assetsReceived < minAssets) revert TooFewAssetsReceived(); | ||
| withdrawnFromSubVault = assetsReceived; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| emit Rebalanced(movedToSubVault, withdrawnFromSubVault); | ||
| } |
Check warning
Code scanning / Slither
Dangerous strict equalities Medium
There was a problem hiding this comment.
will the appropriate mastervault <> subvault exchange rate drift over time? i think we need to account for this somehow because i think as it is currently, _convertToAssets and _convertToShares will return bad values, leading to incorrect amounts passed to _deposit and _withdraw
| uint256 liquidAssets = IERC20(asset()).balanceOf(address(this)); | ||
| uint256 assetsFromSubVault = 0; | ||
|
|
||
| if (liquidAssets < assets) { |
There was a problem hiding this comment.
i wonder if withdrawals should always redeem the user's share of subvault shares instead of just taking as much liquid assets as possible. could there be pricing issues as a result of the proposed behavior?
| _rebalance(maxSlippageBps); | ||
| } | ||
|
|
||
| function _rebalance(uint256 maxSlippageBps) internal { |
There was a problem hiding this comment.
i don't think we can use bps for slippage, probably need to use an exchange rate the same way we do setSubVault
There was a problem hiding this comment.
#130 is an alt rebalance flow that uses an exch rate slippage
| function currentAllocationBps() public view returns (uint256) { | ||
| uint256 _totalAssets = totalAssets(); | ||
| if (_totalAssets == 0) return 0; | ||
|
|
||
| ERC4626 _subVault = subVault; | ||
| if (address(_subVault) == address(0)) return 0; | ||
|
|
||
| uint256 subVaultAssets = _subVault.convertToAssets(_subVault.balanceOf(address(this))); | ||
| return subVaultAssets.mulDiv(10000, _totalAssets, Math.Rounding.Down); | ||
| } |
There was a problem hiding this comment.
we can optimize this a bit
| function currentAllocationBps() public view returns (uint256) { | |
| uint256 _totalAssets = totalAssets(); | |
| if (_totalAssets == 0) return 0; | |
| ERC4626 _subVault = subVault; | |
| if (address(_subVault) == address(0)) return 0; | |
| uint256 subVaultAssets = _subVault.convertToAssets(_subVault.balanceOf(address(this))); | |
| return subVaultAssets.mulDiv(10000, _totalAssets, Math.Rounding.Down); | |
| } | |
| function currentAllocationBps() public view returns (uint256) { | |
| ERC4626 _subVault = subVault; | |
| if (address(_subVault) == address(0)) return 0; | |
| uint256 liquidAssets = IERC20(asset()).balanceOf(address(this)); | |
| uint256 subVaultAssets = _subVault.convertToAssets(_subVault.balanceOf(address(this))); | |
| uint256 _totalAssets = liquidAssets + subVaultAssets; | |
| if (_totalAssets == 0) return 0; | |
| return subVaultAssets.mulDiv(10000, _totalAssets, Math.Rounding.Down); | |
| } |
| if (maxWithdrawable > 0) { | ||
| assetReceived = oldSubVault.withdraw(maxWithdrawable, address(this), address(this)); | ||
| uint256 effectiveAssetExchRateWad = assetReceived.mulDiv(1e18, _totalSupply, Math.Rounding.Down); | ||
| if (effectiveAssetExchRateWad < minAssetExchRateWad) revert TooFewAssetsReceived(); | ||
| } |
There was a problem hiding this comment.
if we don't guarantee that allocation is 0 before entering _revokeSubVault then this can cause loss of funds if liquidity is constrained at the subvault.
we should probably do oldSubVault.redeem(balanceOf(this)) to make sure we definitely get rid of all the subvault shares (or revert)
2 participants
proposing a change to enable owner to set percentage of allocation commitment into sub vault: