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 14 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
222 changes: 95 additions & 127 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,20 +36,27 @@
/// @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();
error InvalidOwner();
error NonZeroTargetAllocation(uint256 targetAllocationWad);

// 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)
IERC4626 public subVault;

uint256 public targetAllocationWad;

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

Expand All @@ -63,14 +70,15 @@
event SubvaultChanged(address indexed oldSubvault, address indexed newSubvault);
event PerformanceFeeToggled(bool enabled);
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();
event PerformanceFeesWithdrawn(address indexed beneficiary, uint256 amountTransferred, uint256 amountWithdrawn);

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,83 +89,107 @@
_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);

IERC20(asset()).safeApprove(address(_subVault), type(uint256).max);

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 {
function distributePerformanceFee() public whenNotPaused {
if (!enablePerformanceFee) revert PerformanceFeeDisabled();
if (beneficiary == address(0)) {
revert BeneficiaryNotSet();
}

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));

uint256 amountToTransfer = profit <= totalIdle ? profit : totalIdle;
uint256 amountToWithdraw = profit - amountToTransfer;

if (amountToTransfer > 0) {
IERC20(asset()).safeTransfer(beneficiary, amountToTransfer);
}
if (amountToWithdraw > 0) {
subVault.withdraw(amountToWithdraw, beneficiary, address(this));
}

emit PerformanceFeesWithdrawn(beneficiary, profit);
rebalance();

emit PerformanceFeesWithdrawn(beneficiary, amountToTransfer, amountToWithdraw);
}

Check warning

Code scanning / Slither

Dangerous strict equalities Medium

MasterVault.distributePerformanceFee() uses a dangerous strict equality:
- profit == 0

Check warning

Code scanning / Slither

Unused return Medium

MasterVault.distributePerformanceFee() ignores return value by subVault.withdraw(amountToWithdraw,beneficiary,address(this))

/// @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?

uint256 totalAssetsUp = _totalAssetsLessProfit(MathUpgradeable.Rounding.Up);
uint256 totalAssetsDown = _totalAssetsLessProfit(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
/// @param enabled True to enable performance fees, false to disable
function setPerformanceFee(bool enabled) external onlyRole(VAULT_MANAGER_ROLE) {
enablePerformanceFee = enabled;

// reset totalPrincipal to current totalAssets when enabling performance fee
// this prevents a sudden large profit
if (enabled) {
totalPrincipal = _totalAssets(MathUpgradeable.Rounding.Up);
}
else {
distributePerformanceFee();
totalPrincipal = 0;
}

enablePerformanceFee = enabled;

emit PerformanceFeeToggled(enabled);
}

Expand Down Expand Up @@ -207,17 +239,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 +247,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 +262,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 +284,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));

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;
}

return supply.mulDiv(subShares, totalSubShares, rounding);
// we add one as part of the first deposit mitigation
// see for details: https://docs.openzeppelin.com/contracts/5.x/erc4626
return assets.mulDiv(totalSupply(), _totalAssetsLessProfit(_flipRounding(rounding)) + 1, 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
return shares.mulDiv(_totalAssetsLessProfit(rounding) + 1, totalSupply(), rounding);
}

function _totalAssetsLessProfit(MathUpgradeable.Rounding rounding) internal view returns (uint256) {
uint256 __totalAssets = _totalAssets(rounding);
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 __totalAssets;
}

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
Loading
Loading