Ybb/simplified #137
Ybb/simplified #137
Conversation
Make master vault upgradable
* feat: access control roles * remove OwnableUpgradeable * add note about permissionless fee manager --------- Co-authored-by: Henry <[email protected]>
MasterVault: remove stored subvault exchange rate
MasterVault: remove switchSubVault
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function sharePrice() public view returns (uint256) { | ||
| uint256 _totalAssets = totalAssets(); | ||
| uint256 _totalSupply = totalSupply(); | ||
|
|
||
| // todo: should we also consider _totalAssets == 0 case? | ||
| if (_totalSupply == 0 || _totalAssets == 0) { | ||
| return 1 * MULTIPLIER; | ||
| } | ||
|
|
||
| uint256 _sharePrice = MathUpgradeable.mulDiv(_totalAssets, MULTIPLIER, _totalSupply); | ||
|
|
||
| if (enablePerformanceFee) { | ||
| _sharePrice = MathUpgradeable.min(_sharePrice, 1e18); | ||
| } | ||
|
|
||
| return _sharePrice; | ||
| } |
Check warning
Code scanning / Slither
Dangerous strict equalities Medium
| function _convertToAssets( | ||
| uint256 shares, | ||
| MathUpgradeable.Rounding rounding | ||
| ) internal view virtual override returns (uint256) { | ||
| uint256 _totalAssets = totalAssets(); | ||
| uint256 _totalSupply = totalSupply(); | ||
| uint256 _effectiveAssets = enablePerformanceFee | ||
| ? MathUpgradeable.min(_totalAssets, uint256(totalPrincipal)) | ||
| : _totalAssets; | ||
|
|
||
| if (_totalSupply == 0) { | ||
| return 1; | ||
| } | ||
|
|
||
| uint256 _assets = MathUpgradeable.mulDiv(shares, _effectiveAssets, _totalSupply, rounding); | ||
| return _assets; | ||
| } |
Check warning
Code scanning / Slither
Dangerous strict equalities Medium
| function revokeSubVault(uint256 minAssetExchRateWad) external onlyRole(VAULT_MANAGER_ROLE) { | ||
| IERC4626 oldSubVault = subVault; | ||
| if (address(oldSubVault) == address(0)) revert NoExistingSubVault(); | ||
|
|
||
| subVault = IERC4626(address(0)); | ||
|
|
||
| oldSubVault.redeem(oldSubVault.balanceOf(address(this)), address(this), address(this)); | ||
| IERC20(asset()).safeApprove(address(oldSubVault), 0); | ||
|
|
||
| uint256 assetExchRateWad = MathUpgradeable.mulDiv(IERC20(asset()).balanceOf(address(this)), 1e18, totalSupply(), MathUpgradeable.Rounding.Down); | ||
| if (assetExchRateWad < minAssetExchRateWad) revert SubVaultExchangeRateTooLow(); | ||
|
|
||
| emit SubvaultChanged(address(oldSubVault), address(0)); | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
| function withdrawPerformanceFees() external onlyRole(FEE_MANAGER_ROLE) { | ||
| if (!enablePerformanceFee) revert PerformanceFeeDisabled(); | ||
| if (beneficiary == address(0)) revert BeneficiaryNotSet(); | ||
|
|
||
| int256 _totalProfits = totalProfit(); | ||
| if (_totalProfits > 0) { | ||
| if (address(subVault) == address(0)) { | ||
| SafeERC20.safeTransfer(IERC20(asset()), beneficiary, uint256(_totalProfits)); | ||
| } else { | ||
| subVault.withdraw(uint256(_totalProfits), beneficiary, address(this)); | ||
| } | ||
|
|
||
| emit PerformanceFeesWithdrawn(beneficiary, uint256(_totalProfits)); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
| function _deposit( | ||
| address caller, | ||
| address receiver, | ||
| uint256 assets, | ||
| uint256 shares | ||
| ) internal virtual override whenNotPaused { | ||
| super._deposit(caller, receiver, assets, shares); | ||
|
|
||
| if (address(subVault) != address(0)) { | ||
| IERC20 underlyingAsset = IERC20(asset()); | ||
| // todo: should we deposit only users assets and account for trasnfer fee or keep depositing _idleAssets? | ||
| uint256 _idleAssets = underlyingAsset.balanceOf(address(this)); | ||
| subVault.deposit(_idleAssets, address(this)); | ||
| } | ||
|
|
||
| totalPrincipal += int256(assets); | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
| function _withdraw( | ||
| address caller, | ||
| address receiver, | ||
| address owner, | ||
| uint256 assets, | ||
| uint256 shares | ||
| ) internal virtual override whenNotPaused { | ||
| if (address(subVault) != address(0)) { | ||
| subVault.withdraw(assets, address(this), address(this)); | ||
| } | ||
|
|
||
| // todo: account trasnfer fee? should we withdraw all? should we validate against users assets if transfer fee accure? | ||
| super._withdraw(caller, receiver, owner, assets, shares); | ||
| totalPrincipal -= int256(assets); | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
| function setSubVault( | ||
| IERC4626 _subVault, | ||
| uint256 minSubVaultExchRateWad | ||
| ) external onlyRole(VAULT_MANAGER_ROLE) { | ||
| IERC20 underlyingAsset = IERC20(asset()); | ||
| if (address(subVault) != address(0)) revert SubVaultAlreadySet(); | ||
| if (address(_subVault.asset()) != address(underlyingAsset)) revert SubVaultAssetMismatch(); | ||
|
|
||
| subVault = _subVault; | ||
|
|
||
| IERC20(asset()).safeApprove(address(_subVault), type(uint256).max); | ||
| _subVault.deposit(underlyingAsset.balanceOf(address(this)), address(this)); | ||
|
|
||
| uint256 _totalSupply = totalSupply(); | ||
| if (_totalSupply > 0) { | ||
| uint256 subVaultExchRateWad = MathUpgradeable.mulDiv( | ||
| _subVault.balanceOf(address(this)), | ||
| 1e18, | ||
| totalSupply(), | ||
| MathUpgradeable.Rounding.Down | ||
| ); | ||
| if (subVaultExchRateWad < minSubVaultExchRateWad) | ||
| revert NewSubVaultExchangeRateTooLow(); | ||
| } | ||
|
|
||
| emit SubvaultChanged(address(0), address(_subVault)); | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
waelsy123
force-pushed
the
ybb/simplified
branch
from
0bdc6b3 to
b96feae
Compare
| if (enabled) { | ||
| totalPrincipal = 0; | ||
| } |
There was a problem hiding this comment.
if we're turning it on, and we set principal to 0, then wouldn't that make the vault entirely profit?
There was a problem hiding this comment.
we should set totalprincipal to totalAssets rounded up
| uint256 _totalAssets = totalAssets(); | ||
| uint256 _totalSupply = totalSupply(); |
There was a problem hiding this comment.
i think when we use totalAssets to calculate the mastervault share price we could run into rounding trouble.
the subvault will have different rounding error than the calculation here. maybe this could lead to us issuing more mastervault shares than we should
There was a problem hiding this comment.
here's a POC that confirms that this rounding error can be exploited for profit. i believe we need to use the preview* functions on the subvault to implement the convertTo* functions on the mastervault.
here is the POC: 0f55ee5
#136 implements the convertTo* functions using the subvault's preview* functions so it shouldn't have this issue
https://docs.google.com/spreadsheets/d/1w5vPwu6MD8P8j7TQ0PooVxPvN3X93CKX0ClWr-M9E7Y/edit?gid=740893849#gid=740893849