Make master vault upgradable #127

waelsy123 · 2025-10-01T13:44:06Z

This convert MasterVault to be beacon upgradable

yahgwai · 2025-10-02T14:33:44Z

contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol

        if (token == address(0)) {
            revert ZeroAddress();
        }
+        if (address(beaconProxyFactory) == address(0)) {


Could check the initialized variable?

will add extra condition to cover that

yahgwai · 2025-10-02T14:41:51Z

contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol

+        beacon = new UpgradeableBeacon(address(masterVaultImplementation));
+        beaconProxyFactory = new BeaconProxyFactory();
+        beaconProxyFactory.initialize(address(beacon));
+        beacon.transferOwnership(address(this));


Isnt this contract already the owner? Should this be beacon.transferOwnership(_owner);?

I guess we should also add a test to show the beacon can be upgraded

good catch yes that should be assigned to the owner

yahgwai · 2025-10-02T14:46:53Z

contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol

        _transferOwnership(_owner);
+
+        MasterVault masterVaultImplementation = new MasterVault();
+        beacon = new UpgradeableBeacon(address(masterVaultImplementation));


Do you think we should deploy these with create2? Or is create fine?

I don't have strong opinion on it but my philosophy is to keep simple if there is no need for something more

yahgwai · 2025-10-02T15:01:37Z

contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol

-        );
-
-        vault = Create2.deploy(0, bytes32(0), bytecode);
+        MasterVault(vault).vaultInit(IERC20(token), name, symbol, address(this));


There a bunch of onlyOwner functions on the master vault which won't be callable - we only have setSubVault. Is that expected?

yes that's expected. all of the onlyOwner calls are around:

switching between sub vaults.

managing perf fee (set, withdraw)

if we only want setSubVault to be callable we should remove the othere external owner functions

it depends on whether we want to allow owner to switch or revoke already assigned subvault. if this feature isn't required then we can drop switchSubvault and revokeSubvault methods.

okay, let's table this for now but we might want to just give the upgradeexecutor ownership of the vaults themselves. that way we don't need to forward calls through the factory

maybe we can include that change in our inevitable roles based access control PR

agreed - thanks for flagging that, will keep note of it to handle this in the coming PRs related to ownerships

godzillaba · 2025-10-03T15:06:38Z

contracts/tokenbridge/libraries/vault/MasterVault.sol

        if (address(_subVault) != address(0)) {
-            _subVault.deposit(assets, address(this));
+            uint256 subShares = _subVault.deposit(assets, address(this));
+            if (subShares == 0) revert NoSubvaultShares();


what's the rationale behind this check?

it's just to satisfy slither as there is a rule to not have unused variables / unused returns

ok i think we can ignore slither

waelsy123 · 2025-10-06T16:26:52Z

contracts/tokenbridge/libraries/vault/MasterVault.sol

        if (address(_subVault) != address(0)) {
-            _subVault.withdraw(assets, address(this), address(this));
+            uint256 sharesBurned = _subVault.withdraw(assets, address(this), address(this));
+            if (sharesBurned == 0) revert NoSharesBurned();


@godzillaba same here

godzillaba · 2025-10-13T21:42:11Z

contracts/tokenbridge/libraries/vault/MasterVault.sol

+
+        __ERC20_init(_name, _symbol);
+        __ERC4626_init(IERC20Upgradeable(address(_asset)));
+        __Ownable_init();


_transferOwnership suffices

Suggested change

__Ownable_init();

godzillaba · 2025-10-13T21:52:11Z

contracts/tokenbridge/libraries/vault/MasterVault.sol

+        uint256 _totalAssets = totalAssets();
+        uint256 _totalSupply = totalSupply();
+
+        subVault = _subVault;
+
        IERC20(asset()).safeApprove(address(_subVault), type(uint256).max);
-        uint256 subShares = _subVault.deposit(totalAssets(), address(this));
+        uint256 subShares = _subVault.deposit(_totalAssets, address(this));

-        uint256 _subVaultExchRateWad = subShares.mulDiv(1e18, totalSupply(), Math.Rounding.Down);
+        uint256 _subVaultExchRateWad = subShares.mulDiv(1e18, _totalSupply, MathUpgradeable.Rounding.Down);
        if (_subVaultExchRateWad < minSubVaultExchRateWad) revert SubVaultExchangeRateTooLow();
        subVaultExchRateWad = _subVaultExchRateWad;

-        subVault = _subVault;
-
        emit SubvaultChanged(address(0), address(_subVault));


can we leave this alone except for changing Math to MathUpgradeable? what is the purpose of the changes?

this include a bug for a race condition which cause this line to fail:
uint256 _subVaultExchRateWad = subShares.mulDiv(1e18, totalSupply(), Math.Rounding.Down);

that's not related to the upgradability but the issue is that this line will always fail with error "division by zero" because of totalSupply() returns zero after (master vault deposited all funds into new subvault

I think an easy fix would be to move subVault = _subVault; in between deposit and _subVaultExchRateWad calculation. just pushed a fix for that.

godzillaba · 2025-10-13T22:10:37Z

contracts/tokenbridge/libraries/vault/MasterVault.sol

+        uint256 maxWithdrawAmount = oldSubVault.maxWithdraw(address(this));

-        IERC20(asset()).safeApprove(address(oldSubVault), 0);
-        subVault = ERC4626(address(0));
+        subVault = IERC4626(address(0));
        subVaultExchRateWad = 1e18;

+        uint256 assetReceived = oldSubVault.withdraw(maxWithdrawAmount, address(this), address(this));
+        IERC20(asset()).safeApprove(address(oldSubVault), 0);
+
+        uint256 effectiveAssetExchRateWad = assetReceived.mulDiv(1e18, _totalSupply, MathUpgradeable.Rounding.Down);
+        if (effectiveAssetExchRateWad < minAssetExchRateWad) revert TooFewAssetsReceived();
+


what is the purpose of the changes here?

godzillaba · 2025-10-13T22:12:06Z

contracts/tokenbridge/libraries/vault/MasterVault.sol

+                uint256 sharesRedeemed = _subVault.withdraw(totalProfits, address(this), address(this));
+                if (sharesRedeemed == 0) revert NoSharesRedeemed();


let's leave this as is since it's not relevant to upgradeability

Suggested change

uint256 sharesRedeemed = _subVault.withdraw(totalProfits, address(this), address(this));

if (sharesRedeemed == 0) revert NoSharesRedeemed();

_subVault.withdraw(totalProfits, address(this), address(this));

godzillaba · 2025-10-13T22:26:36Z

contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol

    error ZeroAddress();
+    error BeaconNotDeployed();
+
+    UpgradeableBeacon public beacon;


i'm not sure we need to store the beacon since the factory doesn't use it

godzillaba · 2025-10-13T22:30:51Z

contracts/tokenbridge/libraries/vault/MasterVault.sol

    event BeneficiaryUpdated(address indexed oldBeneficiary, address indexed newBeneficiary);

-    constructor(IERC20 _asset, string memory _name, string memory _symbol) ERC20(_name, _symbol) ERC4626(_asset) Ownable() {}
+    function vaultInit(IERC20 _asset, string memory _name, string memory _symbol, address _owner) external initializer {


nit, for convention

Suggested change

function vaultInit(IERC20 _asset, string memory _name, string memory _symbol, address _owner) external initializer {

function initialize(IERC20 _asset, string memory _name, string memory _symbol, address _owner) external initializer {

godzillaba · 2025-10-13T22:33:21Z

contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol

-        );
-
-        vault = Create2.deploy(0, bytes32(0), bytecode);
+        MasterVault(vault).vaultInit(IERC20(token), name, symbol, address(this));


okay, let's table this for now but we might want to just give the upgradeexecutor ownership of the vaults themselves. that way we don't need to forward calls through the factory

maybe we can include that change in our inevitable roles based access control PR

…tion

sherlock-ai-beta · 2025-10-16T09:11:31Z

Sherlock AI

Thank you for using Sherlock AI!

**Security Review Unavailable**: Your account doesn't have an active subscription.     To enable automated security reviews on your pull requests, please visit     [https://ai.sherlock.xyz](https://ai.sherlock.xyz) to set up your subscription.

Once activated, Sherlock AI will automatically analyze your code changes and provide     detailed security findings directly in your pull requests.

Need help? Reach out to us at [[email protected]](mailto:[email protected]).

cla-bot bot added the cla-signed label Oct 1, 2025

waelsy123 changed the base branch from main to wa/master-vault-isolated October 1, 2025 13:44

feat: make master vault beacon upgradable

7922536

waelsy123 force-pushed the ybb/upgradable-master-vault branch from b9d6f0d to 7922536 Compare October 1, 2025 13:45

waelsy123 requested review from godzillaba and yahgwai October 1, 2025 13:46

yahgwai reviewed Oct 2, 2025

View reviewed changes

waelsy123 added 2 commits October 3, 2025 11:43

fixup! feat: make master vault beacon upgradable

6331e7a

satisify slither

5632d3d

godzillaba reviewed Oct 3, 2025

View reviewed changes

waelsy123 commented Oct 6, 2025

View reviewed changes

godzillaba reviewed Oct 13, 2025

View reviewed changes

waelsy123 added 3 commits October 16, 2025 09:58

remove unnecessary checks

2b8f151

fixup! feat: make master vault beacon upgradable

5348604

fix: set subvault in between depsoit and _subVaultExchRateWad calcula…

3a24b45

…tion

		uint256 sharesRedeemed = _subVault.withdraw(totalProfits, address(this), address(this));
		if (sharesRedeemed == 0) revert NoSharesRedeemed();

	uint256 sharesRedeemed = _subVault.withdraw(totalProfits, address(this), address(this));
	if (sharesRedeemed == 0) revert NoSharesRedeemed();
	_subVault.withdraw(totalProfits, address(this), address(this));

	function vaultInit(IERC20 _asset, string memory _name, string memory _symbol, address _owner) external initializer {
	function initialize(IERC20 _asset, string memory _name, string memory _symbol, address _owner) external initializer {

Make master vault upgradable #127

Are you sure you want to change the base?

Make master vault upgradable #127

Uh oh!

Conversation

waelsy123 commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sherlock-ai-beta bot commented Oct 16, 2025

Sherlock AI

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

waelsy123 commented Oct 1, 2025 •

edited

Loading