Skip to content

MasterVault reserve #141

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.

Sign up for GitHub

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

Merged
godzillaba merged 27 commits into from
Dec 23, 2025
Merged

MasterVault reserve #141

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
dc2c6ab
wip
godzillaba Dec 11, 2025
a337686
wip: conversion and deposit/withdraw functions look okay
godzillaba Dec 12, 2025
545d96d
settargetAllocationWad
godzillaba Dec 12, 2025
ec19522
deploy with initial vault
godzillaba Dec 12, 2025
69345c1
use factory in core test setup
godzillaba Dec 12, 2025
05fe9e4
slippage tolerance on setTargetAllocationWad
godzillaba Dec 15, 2025
3fa9992
big simplify
godzillaba Dec 15, 2025
9480360
WIP: mastervault donation attack mitigation (#142)
godzillaba Dec 15, 2025
cf3d27b
initial approval and small refactor
godzillaba Dec 16, 2025
e2bdd27
tests
godzillaba Dec 16, 2025
6b5e740
fix PerformanceFeesWithdrawn event
godzillaba Dec 16, 2025
71237b7
move event
godzillaba Dec 16, 2025
ba01452
rebalancing does not count profit
godzillaba Dec 17, 2025
79819e8
distribute fees before disabling
godzillaba Dec 17, 2025
83a5966
fix maxMint
godzillaba Dec 17, 2025
281bf38
fix outdated comment
godzillaba Dec 17, 2025
b49255e
remove unused errors
godzillaba Dec 17, 2025
aba0f92
nonReentrant
godzillaba Dec 17, 2025
234efe7
handle max in maxMint
godzillaba Dec 17, 2025
45e1d00
sanity check we have no sub shares when switching
godzillaba Dec 17, 2025
a023fa2
init reentrancy guard
godzillaba Dec 17, 2025
b0cf3e0
rebalance after setting target
godzillaba Dec 17, 2025
519198b
update docs
godzillaba Dec 17, 2025
101268a
update outdated comment
godzillaba Dec 17, 2025
b8f2c2f
fix nonReentrant modifier placement
godzillaba Dec 17, 2025
b601580
remove testFoo
godzillaba Dec 17, 2025
15a1121
fix tests
godzillaba Dec 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
208 changes: 87 additions & 121 deletions contracts/tokenbridge/libraries/vault/MasterVault.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,15 @@
/// @notice Pauser role can pause/unpause deposits and withdrawals (todo: pause should pause EVERYTHING)
bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

/// @notice Extra decimals added to the ERC20 decimals of the underlying asset to determine the decimals of the MasterVault
/// @dev This is done to mitigate the "first depositor" problem described in the OpenZeppelin ERC4626 documentation.
/// See https://docs.openzeppelin.com/contracts/5.x/erc4626 for more details on the mitigation.
uint8 public constant EXTRA_DECIMALS = 18;
Copy link
Contributor

@waelsy123 waelsy123 Dec 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inspired by morpho:

DECIMALS_OFFSET = uint8(uint256(18).zeroFloorSub(IERC20Metadata(_asset).decimals()));

where zeroFloorSub is max(0, x-y) = max(0, 18-asset.decimals)

Copy link
Contributor Author

@godzillaba godzillaba Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't really want an offset of 0, because then the mitigation no longer works


error SubVaultAlreadySet();
error SubVaultAssetMismatch();
error SubVaultExchangeRateTooLow();
error NoExistingSubVault();
error NewSubVaultExchangeRateTooLow();
error SubVaultExchangeRateTooLow(int256 required, int256 actual);
error PerformanceFeeDisabled();
error BeneficiaryNotSet();
error InvalidAsset();
Expand All @@ -50,6 +54,8 @@
// we may need a minimum asset or master share amount when setting subvaults (bc of exchange rate calc)
IERC4626 public subVault;

uint256 public targetAllocationWad;

/// @notice Flag indicating if performance fee is enabled
bool public enablePerformanceFee;

Expand All @@ -65,12 +71,13 @@
event BeneficiaryUpdated(address indexed oldBeneficiary, address indexed newBeneficiary);
event PerformanceFeesWithdrawn(address indexed beneficiary, uint256 amount);

function initialize(IERC20 _asset, string memory _name, string memory _symbol, address _owner) external initializer {
if (address(_asset) == address(0)) revert InvalidAsset();
if (_owner == address(0)) revert InvalidOwner();

function initialize(IERC4626 _subVault, string memory _name, string memory _symbol, address _owner) external initializer {
__ERC20_init(_name, _symbol);
__ERC4626_init(IERC20Upgradeable(address(_asset)));
__ERC4626_init(IERC20Upgradeable(_subVault.asset()));

// call decimals() to ensure underlying has reasonable decimals and we won't have overflow
decimals();

__AccessControl_init();
__Pausable_init();

Expand All @@ -81,7 +88,17 @@
_grantRole(VAULT_MANAGER_ROLE, _owner);
_grantRole(PAUSER_ROLE, _owner);

_pause();
// mint some dead shares to avoid first depositor issues
// for more information on the mitigation:
// https://web.archive.org/web/20250609034056/https://docs.openzeppelin.com/contracts/4.x/erc4626#fees
_mint(address(1), 10 ** EXTRA_DECIMALS);

subVault = _subVault;
}

/// @dev Overridden to add EXTRA_DECIMALS to the underlying asset decimals
function decimals() public view override returns (uint8) {
return super.decimals() + EXTRA_DECIMALS;
}

function distributePerformanceFee() external whenNotPaused {
Expand All @@ -93,55 +110,68 @@
uint256 profit = totalProfit(MathUpgradeable.Rounding.Down);
if (profit == 0) return;

if (address(subVault) != address(0)) {
subVault.redeem(totalProfitInSubVaultShares(MathUpgradeable.Rounding.Down), beneficiary, address(this));
} else {
IERC20(asset()).safeTransfer(beneficiary, profit);
uint256 totalIdle = IERC20(asset()).balanceOf(address(this));
if (totalIdle > 0) {
uint256 amountToTransfer = profit <= totalIdle ? profit : totalIdle;
IERC20(asset()).safeTransfer(beneficiary, amountToTransfer);
profit -= amountToTransfer;
}

if (profit > 0) {
subVault.withdraw(profit, beneficiary, address(this));
}

rebalance();

emit PerformanceFeesWithdrawn(beneficiary, profit);
}

error NonZeroTargetAllocation(uint256 targetAllocationWad);

/// @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(IERC4626 _subVault, uint256 minSubVaultExchRateWad) external onlyRole(VAULT_MANAGER_ROLE) {
function setSubVault(IERC4626 _subVault) external onlyRole(VAULT_MANAGER_ROLE) {
IERC20 underlyingAsset = IERC20(asset());
if (address(subVault) != address(0)) revert SubVaultAlreadySet();
if (address(_subVault.asset()) != address(underlyingAsset)) revert SubVaultAssetMismatch();
if (targetAllocationWad != 0) revert NonZeroTargetAllocation(targetAllocationWad);

address oldSubVault = address(subVault);
subVault = _subVault;

if (oldSubVault != address(0)) IERC20(asset()).safeApprove(address(oldSubVault), 0);
IERC20(asset()).safeApprove(address(_subVault), type(uint256).max);
_subVault.deposit(underlyingAsset.balanceOf(address(this)), address(this));

uint256 supply = totalSupply();
if (supply > 0) {
uint256 subVaultExchRateWad = _subVault.balanceOf(address(this)).mulDiv(1e18, supply, MathUpgradeable.Rounding.Down);
if (subVaultExchRateWad < minSubVaultExchRateWad) revert NewSubVaultExchangeRateTooLow();
}

emit SubvaultChanged(address(0), address(_subVault));
emit SubvaultChanged(oldSubVault, address(_subVault));
}

/// @notice Revokes the current subvault, moving all assets back to MasterVault
/// @param minAssetExchRateWad Minimum acceptable ratio (times 1e18) of assets received from subvault to outstanding MasterVault shares
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);
function rebalance() public {
Copy link
Contributor

@waelsy123 waelsy123 Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since that this method is public, could be exploited by slippage by manipulating sub assets to shares ratio?

Copy link
Contributor Author

@godzillaba godzillaba Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, maybe? what would be the exploit?

// todo: handle 0 and 100 special cases if needed
uint256 totalAssetsUp = _totalAssets(MathUpgradeable.Rounding.Up);
uint256 totalAssetsDown = _totalAssets(MathUpgradeable.Rounding.Down);
uint256 idleTargetUp = totalAssetsUp.mulDiv(1e18 - targetAllocationWad, 1e18, MathUpgradeable.Rounding.Up);
uint256 idleTargetDown = totalAssetsDown.mulDiv(1e18 - targetAllocationWad, 1e18, MathUpgradeable.Rounding.Down);
uint256 idleBalance = IERC20(asset()).balanceOf(address(this));

if (idleTargetDown <= idleBalance && idleBalance <= idleTargetUp) {
return;
}

uint256 supply = totalSupply();
if (supply > 0) {
uint256 assetExchRateWad = IERC20(asset()).balanceOf(address(this)).mulDiv(1e18, supply, MathUpgradeable.Rounding.Down);
if (assetExchRateWad < minAssetExchRateWad) revert SubVaultExchangeRateTooLow();
if (idleBalance < idleTargetDown) {
// we need to withdraw from subvault
uint256 assetsToWithdraw = idleTargetDown - idleBalance;
subVault.withdraw(assetsToWithdraw, address(this), address(this));
}
else {
// we need to deposit into subvault
uint256 assetsToDeposit = idleBalance - idleTargetUp;
subVault.deposit(assetsToDeposit, address(this));
}
}

emit SubvaultChanged(address(oldSubVault), address(0));
function setTargetAllocationWad(uint256 _targetAllocationWad) external onlyRole(VAULT_MANAGER_ROLE) {
require(_targetAllocationWad <= 1e18, "Target allocation must be <= 100%");
require(targetAllocationWad != _targetAllocationWad, "Allocation unchanged");
targetAllocationWad = _targetAllocationWad;
}

/// @notice Toggle performance fee collection on/off
Expand Down Expand Up @@ -207,17 +237,6 @@
return __totalAssets > totalPrincipal ? __totalAssets - totalPrincipal : 0;
}

function totalProfitInSubVaultShares(MathUpgradeable.Rounding rounding) public view returns (uint256) {
if (address(subVault) == address(0)) {
revert("Subvault not set");
}
uint256 profitAssets = totalProfit(rounding);
if (profitAssets == 0) {
return 0;
}
return _assetsToSubVaultShares(profitAssets, rounding);
}

/**
* @dev Deposit/mint common workflow.
*/
Expand All @@ -226,15 +245,10 @@
address receiver,
uint256 assets,
uint256 shares
) internal virtual override whenNotPaused {
) internal override whenNotPaused {
super._deposit(caller, receiver, assets, shares);

if (enablePerformanceFee) totalPrincipal += assets;

IERC4626 _subVault = subVault;
if (address(_subVault) != address(0)) {
_subVault.deposit(assets, address(this));
}
rebalance();
}

/**
Expand All @@ -246,22 +260,19 @@
address _owner,
uint256 assets,
uint256 shares
) internal virtual override whenNotPaused {
) internal override whenNotPaused {
if (enablePerformanceFee) totalPrincipal -= assets;

IERC4626 _subVault = subVault;
if (address(_subVault) != address(0)) {
_subVault.withdraw(assets, address(this), address(this));
uint256 idleAssets = IERC20(asset()).balanceOf(address(this));
if (idleAssets < assets) {
uint256 assetsToWithdraw = assets - idleAssets;
subVault.withdraw(assetsToWithdraw, address(this), address(this));
}

super._withdraw(caller, receiver, _owner, assets, shares);
Copy link
Contributor

@waelsy123 waelsy123 Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this fail silently?

Copy link
Contributor Author

@godzillaba godzillaba Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

    function _withdraw(
        address caller,
        address receiver,
        address owner,
        uint256 assets,
        uint256 shares
    ) internal virtual {
        if (caller != owner) {
            _spendAllowance(owner, caller, shares);
        }

        // If _asset is ERC777, `transfer` can trigger a reentrancy AFTER the transfer happens through the
        // `tokensReceived` hook. On the other hand, the `tokensToSend` hook, that is triggered before the transfer,
        // calls the vault, which is assumed not malicious.
        //
        // Conclusion: we need to do the transfer after the burn so that any reentrancy would happen after the
        // shares are burned and after the assets are transferred, which is a valid state.
        _burn(owner, shares);
        SafeERC20Upgradeable.safeTransfer(_asset, receiver, assets);

        emit Withdraw(caller, receiver, owner, assets, shares);
    }

rebalance();
}

function _totalAssets(MathUpgradeable.Rounding rounding) internal view returns (uint256) {
if (address(subVault) == address(0)) {
return IERC20(asset()).balanceOf(address(this));
}
return _subVaultSharesToAssets(subVault.balanceOf(address(this)), rounding);
return IERC20(asset()).balanceOf(address(this)) + _subVaultSharesToAssets(subVault.balanceOf(address(this)), rounding);
}

/**
Expand All @@ -271,77 +282,32 @@
* would represent an infinite amount of shares.
*/
function _convertToShares(uint256 assets, MathUpgradeable.Rounding rounding) internal view virtual override returns (uint256 shares) {
uint256 supply = totalSupply();

if (address(subVault) == address(0)) {
uint256 effectiveTotalAssets = enablePerformanceFee ? _min(totalAssets(), totalPrincipal) : totalAssets();

if (supply == 0 || effectiveTotalAssets == 0) {
return assets;
}

return supply.mulDiv(assets, effectiveTotalAssets, rounding);
}

uint256 totalSubShares = subVault.balanceOf(address(this));

// we add one as part of the first deposit mitigation
// see for details: https://docs.openzeppelin.com/contracts/5.x/erc4626
uint256 __totalAssets = _totalAssets(_flipRounding(rounding)) + 1;
if (enablePerformanceFee) {
// since we use totalSubShares in the denominator of the final calculation,
// and we are subtracting profit from it, we should use the same rounding direction for profit
totalSubShares -= totalProfitInSubVaultShares(_flipRounding(rounding));
}

uint256 subShares = _assetsToSubVaultShares(assets, rounding);

if (supply == 0 || totalSubShares == 0) {
return subShares;
__totalAssets -= totalProfit(rounding);
}

return supply.mulDiv(subShares, totalSubShares, rounding);
return assets.mulDiv(totalSupply(), __totalAssets, rounding);
}

/**
* @dev Internal conversion function (from shares to assets) with support for rounding direction.
*/
function _convertToAssets(uint256 shares, MathUpgradeable.Rounding rounding) internal view virtual override returns (uint256 assets) {
uint256 _totalSupply = totalSupply();

if(_totalSupply == 0) {
return shares;
}

// if we have no subvault, we just do normal pro-rata calculation
if (address(subVault) == address(0)) {
uint256 effectiveTotalAssets = enablePerformanceFee ? _min(totalAssets(), totalPrincipal) : totalAssets();
return effectiveTotalAssets.mulDiv(shares, _totalSupply, rounding);
}

uint256 totalSubShares = subVault.balanceOf(address(this));

// we add one as part of the first deposit mitigation
// see for details: https://docs.openzeppelin.com/contracts/5.x/erc4626
uint256 __totalAssets = _totalAssets(rounding) + 1;
if (enablePerformanceFee) {
// since we use totalSubShares in the numerator of the final calculation,
// and we are subtracting profit from it, we should use the opposite rounding direction for profit
totalSubShares -= totalProfitInSubVaultShares(_flipRounding(rounding));
__totalAssets -= totalProfit(_flipRounding(rounding));
}

// totalSubShares * shares / totalMasterShares
uint256 subShares = totalSubShares.mulDiv(shares, _totalSupply, rounding);

return _subVaultSharesToAssets(subShares, rounding);
}

function _assetsToSubVaultShares(uint256 assets, MathUpgradeable.Rounding rounding) internal view returns (uint256 subShares) {
return rounding == MathUpgradeable.Rounding.Up ? subVault.previewWithdraw(assets) : subVault.previewDeposit(assets);
return shares.mulDiv(__totalAssets, totalSupply(), rounding);
}

function _subVaultSharesToAssets(uint256 subShares, MathUpgradeable.Rounding rounding) internal view returns (uint256 assets) {
return rounding == MathUpgradeable.Rounding.Up ? subVault.previewMint(subShares) : subVault.previewRedeem(subShares);
}

function _min(uint256 a, uint256 b) internal pure returns (uint256) {
return a <= b ? a : b;
}

function _flipRounding(MathUpgradeable.Rounding rounding) internal pure returns (MathUpgradeable.Rounding) {
return rounding == MathUpgradeable.Rounding.Up ? MathUpgradeable.Rounding.Down : MathUpgradeable.Rounding.Up;
}
Expand Down
39 changes: 25 additions & 14 deletions contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,19 @@

pragma solidity ^0.8.0;

import "@openzeppelin/contracts/utils/Create2.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
import "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol";
import "../ClonableBeaconProxy.sol";
import "./IMasterVault.sol";
import "./IMasterVaultFactory.sol";
import "./MasterVault.sol";

contract DefaultSubVault is ERC4626 {
constructor(address token) ERC4626(IERC20(token)) ERC20("Default SubVault", "DSV") {}
}

// todo: slim down this contract
contract MasterVaultFactory is IMasterVaultFactory, Initializable {
error ZeroAddress();
error BeaconNotDeployed();
Expand All @@ -27,23 +32,13 @@ contract MasterVaultFactory is IMasterVaultFactory, Initializable {
}

function deployVault(address token) public returns (address vault) {
if (token == address(0)) {
Copy link
Contributor

@waelsy123 waelsy123 Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we actually need a vault with asset with zero address?

Copy link
Contributor Author

@godzillaba godzillaba Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, i don't think we need to prevent it though

revert ZeroAddress();
}
if (
address(beaconProxyFactory) == address(0) && beaconProxyFactory.beacon() == address(0)
) {
revert BeaconNotDeployed();
}

bytes32 userSalt = _getUserSalt(token);
vault = beaconProxyFactory.createProxy(userSalt);

IERC20Metadata tokenMetadata = IERC20Metadata(token);
string memory name = string(abi.encodePacked("Master ", tokenMetadata.name()));
string memory symbol = string(abi.encodePacked("m", tokenMetadata.symbol()));
string memory name = string(abi.encodePacked("Master ", _tryGetTokenName(token)));
string memory symbol = string(abi.encodePacked("m", _tryGetTokenSymbol(token)));

MasterVault(vault).initialize(IERC20(token), name, symbol, owner);
MasterVault(vault).initialize(new DefaultSubVault(token), name, symbol, owner);

emit VaultDeployed(token, vault);
}
Expand All @@ -64,4 +59,20 @@ contract MasterVaultFactory is IMasterVaultFactory, Initializable {
}
return vault;
}

function _tryGetTokenName(address token) internal view returns (string memory) {
try IERC20Metadata(token).name() returns (string memory name) {
return name;
} catch {
return "";
}
}

function _tryGetTokenSymbol(address token) internal view returns (string memory) {
try IERC20Metadata(token).symbol() returns (string memory symbol) {
return symbol;
} catch {
return "";
}
}
}
Loading
Loading