MasterVault reserve (#141)

* wip

* wip: conversion and deposit/withdraw functions look okay

* settargetAllocationWad

* deploy with initial vault

* use factory in core test setup

* slippage tolerance on setTargetAllocationWad

* big simplify

* WIP: mastervault donation attack mitigation (#142)

* wip: dead shares and fix div by zero

* doc

* add one

* initial approval and small refactor

* tests

* fix PerformanceFeesWithdrawn event

* move event

* rebalancing does not count profit

* distribute fees before disabling

* fix maxMint

* fix outdated comment

* remove unused errors

* nonReentrant

* handle max in maxMint

* sanity check we have no sub shares when switching

* init reentrancy guard

* rebalance after setting target

* update docs

* update outdated comment

* fix nonReentrant modifier placement

* remove testFoo

* fix tests

Author: godzillaba

contracts/tokenbridge/libraries/vault/MasterVault.sol

@@ -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();
@@ -27,23 +32,13 @@ contract MasterVaultFactory is IMasterVaultFactory, Initializable {
     }
 
     function deployVault(address token) public returns (address vault) {
-        if (token == address(0)) {
-            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);
     }
@@ -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 "";
+        }
+    }
 }

contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol

@@ -5,145 +5,176 @@ import { MasterVaultCoreTest } from "./MasterVaultCore.t.sol";
 import { MockSubVault } from "../../../contracts/tokenbridge/test/MockSubVault.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import { IERC4626 } from "@openzeppelin/contracts/interfaces/IERC4626.sol";
+import { Math } from "@openzeppelin/contracts/utils/math/Math.sol";
+import {console2} from "forge-std/console2.sol";
+
+contract MasterVaultFirstDepositTest is MasterVaultCoreTest {
+    using Math for uint256;
+
+    uint256 constant FRESH_STATE_PLACEHOLDER = uint256(keccak256("FRESH_STATE_PLACEHOLDER"));
+    uint256 constant DEAD_SHARES = 10**18;
+
+    struct State {
+        uint256 userShares;
+        uint256 masterVaultTotalAssets;
+        uint256 masterVaultTotalSupply;
+        uint256 masterVaultTokenBalance;
+        uint256 masterVaultSubVaultShareBalance;
+        uint256 subVaultTotalAssets;
+        uint256 subVaultTotalSupply;
+        uint256 subVaultTokenBalance;
+    }
 
-contract MasterVaultTest is MasterVaultCoreTest {
     // first deposit
-    function test_deposit() public {
-        address _assetsHoldingVault = address(vault.subVault()) == address(0)
-            ? address(vault)
-            : address(vault.subVault());
-        uint256 _assetsHoldingVaultBalanceBefore = token.balanceOf(_assetsHoldingVault);
-
+    function test_deposit(uint96 _depositAmount) public {
+        uint256 depositAmount = _depositAmount;
         vm.startPrank(user);
-        token.mint();
-        uint256 depositAmount = 100;
-
+        token.mint(depositAmount);
         token.approve(address(vault), depositAmount);
-
         uint256 shares = vault.deposit(depositAmount, user);
-
-        uint256 _assetsHoldingVaultBalanceAfter = token.balanceOf(_assetsHoldingVault);
-        uint256 diff = _assetsHoldingVaultBalanceAfter - _assetsHoldingVaultBalanceBefore;
-
-        assertEq(vault.balanceOf(user), shares, "User should receive shares");
-        assertEq(vault.totalAssets(), depositAmount, "Vault should hold deposited assets");
-        assertEq(vault.totalSupply(), shares, "Total supply should equal shares minted");
-
-        assertEq(diff, depositAmount, "Vault should increase holding of assets");
-        assertGt(token.balanceOf(_assetsHoldingVault), 0, "Vault should hold the tokens");
-
-        assertEq(vault.totalSupply(), diff, "First deposit should be at a rate of 1");
-
         vm.stopPrank();
+        _checkState(State({
+            userShares: depositAmount * DEAD_SHARES,
+            masterVaultTotalAssets: depositAmount,
+            masterVaultTotalSupply: (1 + depositAmount) * DEAD_SHARES,
+            masterVaultTokenBalance: depositAmount,
+            masterVaultSubVaultShareBalance: 0,
+            subVaultTotalAssets: 0,
+            subVaultTotalSupply: 0,
+            subVaultTokenBalance: 0
+        }));
+        assertEq(shares, depositAmount * DEAD_SHARES, "shares mismatch deposit return value");
     }
 
-    // first mint
-    function test_mint() public {
-        address _assetsHoldingVault = address(vault.subVault()) == address(0)
-            ? address(vault)
-            : address(vault.subVault());
-
-        uint256 _assetsHoldingVaultBalanceBefore = token.balanceOf(_assetsHoldingVault);
-
+    function test_mint(uint96 _mintAmount) public {
+        uint256 mintAmount = _mintAmount;
         vm.startPrank(user);
-        token.mint();
-        uint256 sharesToMint = 100;
-
-        token.approve(address(vault), type(uint256).max);
-
-        //   assertEq(1, vault.totalAssets(), "First mint should be at a rate of 1"); // 0
-        // assertEq(1, vault.totalSupply(), "First mint should be at a rate of 1");   // 0
-
-
-        uint256 assetsCost = vault.mint(sharesToMint, user);
-
-        uint256 _assetsHoldingVaultBalanceAfter = token.balanceOf(_assetsHoldingVault);
-
-        assertEq(vault.balanceOf(user), sharesToMint, "User should receive requested shares");
-        assertEq(vault.totalSupply(), sharesToMint, "Total supply should equal shares minted");
-        assertEq(vault.totalAssets(), assetsCost, "Vault should hold the assets deposited");
-        assertEq(
-            _assetsHoldingVaultBalanceAfter - _assetsHoldingVaultBalanceBefore,
-            assetsCost,
-            "Vault should hold the tokens"
-        );
-
-        assertEq(vault.totalSupply(), vault.totalAssets(), "First mint should be at a rate of 1");
+        token.mint(mintAmount);
+        token.approve(address(vault), mintAmount);
+        uint256 assets = vault.mint(mintAmount, user);
         vm.stopPrank();
+        _checkState(State({
+            userShares: mintAmount,
+            masterVaultTotalAssets: mintAmount.ceilDiv(1e18),
+            masterVaultTotalSupply: mintAmount + DEAD_SHARES,
+            masterVaultTokenBalance: mintAmount.ceilDiv(1e18),
+            masterVaultSubVaultShareBalance: 0,
+            subVaultTotalAssets: 0,
+            subVaultTotalSupply: 0,
+            subVaultTokenBalance: 0
+        }));
+        assertEq(assets, mintAmount.ceilDiv(1e18), "assets mismatch mint return value");
     }
 
-    function test_withdraw() public {
+    function test_withdraw(uint96 _firstDeposit, uint96 _withdrawAmount) public {
+        uint256 firstDeposit = _firstDeposit;
+        uint256 withdrawAmount = _withdrawAmount;
+        vm.assume(withdrawAmount <= firstDeposit);
+        test_deposit(_firstDeposit);
         vm.startPrank(user);
-        token.mint();
-        uint256 depositAmount = token.balanceOf(user);
-        token.approve(address(vault), depositAmount);
-        vault.deposit(depositAmount, user);
-
-        uint256 userSharesBefore = vault.balanceOf(user);
-        uint256 withdrawAmount = depositAmount; // withdraw all assets
-
         uint256 sharesRedeemed = vault.withdraw(withdrawAmount, user, user);
-
-        assertEq(vault.balanceOf(user), 0, "User should have no shares left");
-        assertEq(token.balanceOf(user), depositAmount, "User should receive all withdrawn tokens");
-        assertEq(vault.totalAssets(), 0, "Vault should have no assets left");
-        assertEq(vault.totalSupply(), 0, "Total supply should be zero");
-        assertEq(token.balanceOf(address(vault)), 0, "Vault should have no tokens left");
-        assertEq(sharesRedeemed, userSharesBefore, "All shares should be redeemed");
-
         vm.stopPrank();
+        _checkState(State({
+            userShares: (firstDeposit - withdrawAmount) * DEAD_SHARES,
+            masterVaultTotalAssets: firstDeposit - withdrawAmount,
+            masterVaultTotalSupply: (1 + firstDeposit - withdrawAmount) * DEAD_SHARES,
+            masterVaultTokenBalance: firstDeposit - withdrawAmount,
+            masterVaultSubVaultShareBalance: 0,
+            subVaultTotalAssets: 0,
+            subVaultTotalSupply: 0,
+            subVaultTokenBalance: 0
+        }));
+        assertEq(sharesRedeemed, withdrawAmount * DEAD_SHARES, "sharesRedeemed mismatch withdraw return value");
     }
 
-    function test_redeem() public {
+    function test_redeem(uint96 _firstMint, uint96 _redeemAmount) public {
+        uint256 firstMint = _firstMint;
+        uint256 redeemAmount = _redeemAmount;
+        vm.assume(redeemAmount <= firstMint);
+        test_mint(_firstMint);
+        State memory beforeState = _getState();
         vm.startPrank(user);
-        token.mint();
-        uint256 depositAmount = token.balanceOf(user);
-        token.approve(address(vault), depositAmount);
-        uint256 shares = vault.deposit(depositAmount, user);
-
-        uint256 sharesToRedeem = shares; // redeem all shares
-
-        uint256 assetsReceived = vault.redeem(sharesToRedeem, user, user);
-
-        assertEq(vault.balanceOf(user), 0, "User should have no shares left");
-        assertEq(token.balanceOf(user), depositAmount, "User should receive all assets back");
-        assertEq(vault.totalAssets(), 0, "Vault should have no assets left");
-        assertEq(vault.totalSupply(), 0, "Total supply should be zero");
-        assertEq(token.balanceOf(address(vault)), 0, "Vault should have no tokens left");
-        assertEq(assetsReceived, depositAmount, "All assets should be received");
-
+        uint256 assets = vault.redeem(redeemAmount, user, user);
+        uint256 expectedAssets = (1 + beforeState.masterVaultTotalAssets) * redeemAmount / (beforeState.masterVaultTotalSupply);
         vm.stopPrank();
+        _checkState(State({
+            userShares: beforeState.userShares - redeemAmount,
+            masterVaultTotalAssets: beforeState.masterVaultTotalAssets - expectedAssets,
+            masterVaultTotalSupply: beforeState.masterVaultTotalSupply - redeemAmount,
+            masterVaultTokenBalance: beforeState.masterVaultTokenBalance - expectedAssets,
+            masterVaultSubVaultShareBalance: 0,
+            subVaultTotalAssets: 0,
+            subVaultTotalSupply: 0,
+            subVaultTokenBalance: 0
+        }));
+        assertEq(assets, expectedAssets, "assets mismatch redeem return value");
     }
-}
 
-contract MasterVaultTestWithSubvaultFresh is MasterVaultTest {
-    function setUp() public override {
-        super.setUp();
-        MockSubVault _subvault = new MockSubVault(IERC20(address(token)), "TestSubvault", "TSV");
-        vault.setSubVault(IERC4626(address(_subvault)), 0);
+    function _checkState(State memory expectedState) internal {
+        assertEq(expectedState.userShares, vault.balanceOf(user), "userShares mismatch");
+        assertEq(expectedState.masterVaultTotalAssets, vault.totalAssets(), "masterVaultTotalAssets mismatch");
+        assertEq(expectedState.masterVaultTotalSupply, vault.totalSupply(), "masterVaultTotalSupply mismatch");
+        assertEq(expectedState.masterVaultTokenBalance, token.balanceOf(address(vault)), "masterVaultTokenBalance mismatch");
+        assertEq(expectedState.masterVaultSubVaultShareBalance, vault.subVault().balanceOf(address(vault)), "masterVaultSubVaultShareBalance mismatch");
+        assertEq(expectedState.subVaultTotalAssets, vault.subVault().totalAssets(), "subVaultTotalAssets mismatch");
+        assertEq(expectedState.subVaultTotalSupply, vault.subVault().totalSupply(), "subVaultTotalSupply mismatch");
+        assertEq(expectedState.subVaultTokenBalance, token.balanceOf(address(vault.subVault())), "subVaultTokenBalance mismatch");
     }
-}
-
-contract MasterVaultTestWithSubvaultHoldingAssets is MasterVaultTest {
-    function setUp() public override {
-        super.setUp();
 
-        MockSubVault _subvault = new MockSubVault(IERC20(address(token)), "TestSubvault", "TSV");
-        uint256 _initAmount = 97659743;
-        token.mint(_initAmount);
-        token.approve(address(_subvault), _initAmount);
-        _subvault.deposit(_initAmount, address(this));
-        assertEq(
-            _initAmount,
-            _subvault.totalAssets(),
-            "subvault should be initiated with assets = _initAmount"
-        );
-        assertEq(
-            _initAmount,
-            _subvault.totalSupply(),
-            "subvault should be initiated with shares = _initAmount"
-        );
+    function _getState() internal view returns (State memory) {
+        return State({
+            userShares: vault.balanceOf(user),
+            masterVaultTotalAssets: vault.totalAssets(),
+            masterVaultTotalSupply: vault.totalSupply(),
+            masterVaultTokenBalance: token.balanceOf(address(vault)),
+            masterVaultSubVaultShareBalance: vault.subVault().balanceOf(address(vault)),
+            subVaultTotalAssets: vault.subVault().totalAssets(),
+            subVaultTotalSupply: vault.subVault().totalSupply(),
+            subVaultTokenBalance: token.balanceOf(address(vault.subVault()))
+        });
+    }
 
-        vault.setSubVault(IERC4626(address(_subvault)), 0);
+    function _logState(string memory label, State memory state) internal view {
+        console2.log(label);
+        console2.log(" userShares:", state.userShares);
+        console2.log(" masterVaultTotalAssets:", state.masterVaultTotalAssets);
+        console2.log(" masterVaultTotalSupply:", state.masterVaultTotalSupply);
+        console2.log(" masterVaultTokenBalance:", state.masterVaultTokenBalance);
+        console2.log(" masterVaultSubVaultShareBalance:", state.masterVaultSubVaultShareBalance);
+        console2.log(" subVaultTotalAssets:", state.subVaultTotalAssets);
+        console2.log(" subVaultTotalSupply:", state.subVaultTotalSupply);
+        console2.log(" subVaultTokenBalance:", state.subVaultTokenBalance);
     }
 }
+
+// contract MasterVaultTestWithSubvaultFresh is MasterVaultTest {
+//     function setUp() public override {
+//         super.setUp();
+//         MockSubVault _subvault = new MockSubVault(IERC20(address(token)), "TestSubvault", "TSV");
+//         vault.setSubVault(IERC4626(address(_subvault)));
+//     }
+// }
+
+// contract MasterVaultTestWithSubvaultHoldingAssets is MasterVaultTest {
+//     function setUp() public override {
+//         super.setUp();
+
+//         MockSubVault _subvault = new MockSubVault(IERC20(address(token)), "TestSubvault", "TSV");
+//         uint256 _initAmount = 97659743;
+//         token.mint(_initAmount);
+//         token.approve(address(_subvault), _initAmount);
+//         _subvault.deposit(_initAmount, address(this));
+//         assertEq(
+//             _initAmount,
+//             _subvault.totalAssets(),
+//             "subvault should be initiated with assets = _initAmount"
+//         );
+//         assertEq(
+//             _initAmount,
+//             _subvault.totalSupply(),
+//             "subvault should be initiated with shares = _initAmount"
+//         );
+
+//         vault.setSubVault(IERC4626(address(_subvault)));
+//     }
+// }

test-foundry/libraries/vault/MasterVault.t.sol

@@ -2,16 +2,16 @@
 pragma solidity ^0.8.0;
 
 import "forge-std/console2.sol";
-import { MasterVaultTest } from "./MasterVault.t.sol";
+import { MasterVaultCoreTest } from "./MasterVaultCore.t.sol";
 import { MockSubVault } from "../../../contracts/tokenbridge/test/MockSubVault.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import { IERC4626 } from "@openzeppelin/contracts/interfaces/IERC4626.sol";
 
-contract MasterVaultTestWithSubvaultFresh is MasterVaultTest {
+contract MasterVaultTestWithSubvaultFresh is MasterVaultCoreTest {
     function setUp() public override {
         super.setUp();
         MockSubVault _subvault = new MockSubVault(IERC20(address(token)), "TestSubvault", "TSV");
-        vault.setSubVault(IERC4626(address(_subvault)), 0);
+        vault.setSubVault(IERC4626(address(_subvault)));
     }
 }