-
Notifications
You must be signed in to change notification settings - Fork 156
another master vault design #125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: wa/ybb-master-vault
Are you sure you want to change the base?
Changes from all commits
e80db5d
4d8711f
17d9f15
dc313de
2413f09
6510c86
7571587
f8f9274
e50c272
15c6401
17ae81f
1151c8c
e385df2
afc5490
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,247 @@ | ||
// SPDX-License-Identifier: Apache-2.0 | ||
pragma solidity ^0.8.0; | ||
|
||
import {ERC4626} from "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol"; | ||
import { IERC20, ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; | ||
import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; | ||
import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; | ||
import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; | ||
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; | ||
|
||
contract MasterVault2 is ERC4626, Ownable { | ||
using SafeERC20 for IERC20; | ||
using Math for uint256; | ||
|
||
// todo: avoid inflation, rounding, other common 4626 vulns | ||
// we may need a minimum asset or master share amount when setting subvaults (bc of exchange rate calc) | ||
|
||
ERC4626 public subVault; | ||
|
||
// how many subVault shares one MV2 share can be redeemed for | ||
// initially 1 to 1 | ||
// constant per subvault | ||
// changes when subvault is set | ||
uint256 public subVaultExchRateWad = 1e18; | ||
|
||
// note: the performance fee can be avoided if the underlying strategy can be sandwiched (eg ETH to wstETH dex swap) | ||
// 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 | ||
uint256 totalPrincipal; // total assets deposited, used to calculate profit | ||
|
||
event SubvaultChanged(address indexed oldSubvault, address indexed newSubvault); | ||
|
||
constructor(IERC20 _asset, string memory _name, string memory _symbol) ERC20(_name, _symbol) ERC4626(_asset) Ownable() {} | ||
|
||
function deposit(uint256 assets, address receiver, uint256 minSharesMinted) public returns (uint256) { | ||
uint256 shares = super.deposit(assets, receiver); | ||
require(shares >= minSharesMinted, "too few shares received"); | ||
return shares; | ||
} | ||
|
||
function withdraw(uint256 assets, address receiver, address _owner, uint256 maxSharesBurned) public returns (uint256) { | ||
uint256 shares = super.withdraw(assets, receiver, _owner); | ||
require(shares <= maxSharesBurned, "too many shares burned"); | ||
return shares; | ||
} | ||
|
||
function mint(uint256 shares, address receiver, uint256 maxAssetsDeposited) public returns (uint256) { | ||
uint256 assets = super.mint(shares, receiver); | ||
require(assets <= maxAssetsDeposited, "too many assets deposited"); | ||
return assets; | ||
} | ||
|
||
function redeem(uint256 shares, address receiver, address _owner, uint256 minAssetsReceived) public returns (uint256) { | ||
uint256 assets = super.redeem(shares, receiver, _owner); | ||
require(assets >= minAssetsReceived, "too few assets received"); | ||
return assets; | ||
} | ||
|
||
/// @notice Set a subvault. Can only be called if there is not already a subvault set. | ||
/// @param _subVault The subvault to set. Must be an ERC4626 vault with the same asset as this MasterVault. | ||
/// @param minSubVaultExchRateWad Minimum acceptable ratio (times 1e18) of new subvault shares to outstanding MasterVault shares after deposit. | ||
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"); | ||
waelsy123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
require(address(_subVault.asset()) == address(asset()), "subvault asset mismatch"); | ||
|
||
// deposit to subvault | ||
IERC20(asset()).safeApprove(address(_subVault), type(uint256).max); | ||
waelsy123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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)); | ||
} | ||
Comment on lines
+64
to
+83
Check warningCode scanning / Slither Reentrancy vulnerabilities Medium
Reentrancy in MasterVault2.setSubVault(ERC4626,uint256):
External calls: - IERC20(asset()).safeApprove(address(_subVault),type()(uint256).max) - subShares = _subVault.deposit(totalAssets(),address(this)) State variables written after the call(s): - subVault = _subVault MasterVault2.subVault can be used in cross function reentrancies: - MasterVault2._convertToAssets(uint256,Math.Rounding) - MasterVault2._convertToShares(uint256,Math.Rounding) - MasterVault2._deposit(address,address,uint256,uint256) - MasterVault2._withdraw(address,address,address,uint256,uint256) - MasterVault2.maxDeposit(address) - MasterVault2.maxMint(address) - MasterVault2.setSubVault(ERC4626,uint256) - MasterVault2.subVault - MasterVault2.switchSubVault(ERC4626,uint256,uint256) - MasterVault2.totalAssets() |
||
|
||
/// @notice Switch to a new subvault. Withdraws all assets from the old subvault and deposits them into the new subvault. | ||
/// Can only be called if there is already a subvault set. | ||
/// @param newSubVault The new subvault to switch to. If address(0), the MasterVault will no longer use a subvault. | ||
/// @param minAssetExchRateWad Minimum acceptable ratio (times 1e18) of assets received from subvault to outstanding MasterVault shares. | ||
/// @param minNewSubVaultExchRateWad Minimum acceptable ratio (times 1e18) of new subvault shares to outstanding MasterVault shares after deposit. | ||
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"); | ||
waelsy123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// 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)); | ||
} | ||
Comment on lines
+90
to
+125
Check warningCode scanning / Slither Reentrancy vulnerabilities Medium
Reentrancy in MasterVault2.switchSubVault(ERC4626,uint256,uint256):
External calls: - assetReceived = oldSubVault.withdraw(oldSubVault.maxWithdraw(address(this)),address(this),address(this)) - IERC20(asset()).safeApprove(address(oldSubVault),0) - IERC20(asset()).safeApprove(address(newSubVault),type()(uint256).max) - newSubShares = newSubVault.deposit(totalAssets(),address(this)) State variables written after the call(s): - subVault = newSubVault MasterVault2.subVault can be used in cross function reentrancies: - MasterVault2._convertToAssets(uint256,Math.Rounding) - MasterVault2._convertToShares(uint256,Math.Rounding) - MasterVault2._deposit(address,address,uint256,uint256) - MasterVault2._withdraw(address,address,address,uint256,uint256) - MasterVault2.maxDeposit(address) - MasterVault2.maxMint(address) - MasterVault2.setSubVault(ERC4626,uint256) - MasterVault2.subVault - MasterVault2.switchSubVault(ERC4626,uint256,uint256) - MasterVault2.totalAssets() |
||
|
||
function masterSharesToSubShares(uint256 masterShares, Math.Rounding rounding) public view returns (uint256) { | ||
return masterShares.mulDiv(subVaultExchRateWad, 1e18, rounding); | ||
} | ||
|
||
function subSharesToMasterShares(uint256 subShares, Math.Rounding rounding) public view returns (uint256) { | ||
return subShares.mulDiv(1e18, subVaultExchRateWad, rounding); | ||
} | ||
|
||
/** @dev See {IERC4626-totalAssets}. */ | ||
function totalAssets() public view virtual override returns (uint256) { | ||
ERC4626 _subVault = subVault; | ||
if (address(_subVault) == address(0)) { | ||
return super.totalAssets(); | ||
} | ||
return _subVault.convertToAssets(_subVault.balanceOf(address(this))); | ||
} | ||
|
||
/** @dev See {IERC4626-maxDeposit}. */ | ||
function maxDeposit(address) public view virtual override returns (uint256) { | ||
return subVault.maxDeposit(address(this)); | ||
} | ||
|
||
/** @dev See {IERC4626-maxMint}. */ | ||
function maxMint(address) public view virtual override returns (uint256) { | ||
uint256 subShares = subVault.maxMint(address(this)); | ||
if (subShares == type(uint256).max) { | ||
return type(uint256).max; | ||
} | ||
return subSharesToMasterShares(subShares, Math.Rounding.Down); | ||
} | ||
|
||
/** | ||
* @dev Internal conversion function (from assets to shares) with support for rounding direction. | ||
* | ||
* Will revert if assets > 0, totalSupply > 0 and totalAssets = 0. That corresponds to a case where any asset | ||
* would represent an infinite amount of shares. | ||
*/ | ||
function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual override returns (uint256 shares) { | ||
ERC4626 _subVault = subVault; | ||
if (address(_subVault) == address(0)) { | ||
return super._convertToShares(assets, rounding); | ||
} | ||
uint256 subShares = rounding == Math.Rounding.Up ? _subVault.previewWithdraw(assets) : _subVault.previewDeposit(assets); | ||
return subSharesToMasterShares(subShares, rounding); | ||
} | ||
|
||
/** | ||
* @dev Internal conversion function (from shares to assets) with support for rounding direction. | ||
*/ | ||
function _convertToAssets(uint256 shares, Math.Rounding rounding) internal view virtual override returns (uint256 assets) { | ||
ERC4626 _subVault = subVault; | ||
if (address(_subVault) == address(0)) { | ||
return super._convertToAssets(shares, rounding); | ||
} | ||
uint256 subShares = masterSharesToSubShares(shares, rounding); | ||
return rounding == Math.Rounding.Up ? _subVault.previewMint(subShares) : _subVault.previewRedeem(subShares); | ||
} | ||
|
||
function totalProfit() public view returns (uint256) { | ||
uint256 _totalAssets = totalAssets(); | ||
return _totalAssets > totalPrincipal ? _totalAssets - totalPrincipal : 0; | ||
} | ||
|
||
/** | ||
* @dev Deposit/mint common workflow. | ||
*/ | ||
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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice one! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks! |
||
} | ||
} | ||
Comment on lines
+193
to
+205
Check warningCode scanning / Slither Unused return Medium
MasterVault2._deposit(address,address,uint256,uint256) ignores return value by _subVault.deposit(assets,address(this))
|
||
|
||
/** | ||
* @dev Withdraw/redeem common workflow. | ||
*/ | ||
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 failure
Code scanning / Slither
Uninitialized state variables High