Merge pull request #133 from OffchainLabs/ha/mv-remove-stored-exch-rate

MasterVault: remove stored subvault exchange rate

Author: godzillaba

contracts/tokenbridge/libraries/vault/MasterVault.sol

@@ -26,12 +26,9 @@ contract MasterVault is Initializable, ERC4626Upgradeable, AccessControlUpgradea
     error TooManyAssetsDeposited();
     error TooFewAssetsReceived();
     error SubVaultAlreadySet();
-    error SubVaultCannotBeZeroAddress();
-    error MustHaveSupplyBeforeSettingSubVault();
     error SubVaultAssetMismatch();
     error SubVaultExchangeRateTooLow();
     error NoExistingSubVault();
-    error MustHaveSupplyBeforeSwitchingSubVault();
     error NewSubVaultExchangeRateTooLow();
     error BeneficiaryNotSet();
     error PerformanceFeeDisabled();
@@ -42,12 +39,6 @@ contract MasterVault is Initializable, ERC4626Upgradeable, AccessControlUpgradea
     // we may need a minimum asset or master share amount when setting subvaults (bc of exchange rate calc)
     IERC4626 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;
-
     // 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
@@ -77,8 +68,6 @@ contract MasterVault is Initializable, ERC4626Upgradeable, AccessControlUpgradea
         _grantRole(VAULT_MANAGER_ROLE, _owner);
         _grantRole(FEE_MANAGER_ROLE, _owner); // todo: consider permissionless by default
         _grantRole(PAUSER_ROLE, _owner);
-
-        subVaultExchRateWad = 1e18;
     }
 
 
@@ -110,7 +99,6 @@ contract MasterVault is Initializable, ERC4626Upgradeable, AccessControlUpgradea
     /// @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) {
-        if (address(subVault) != address(0)) revert SubVaultAlreadySet();
         _setSubVault(_subVault, minSubVaultExchRateWad);
     }
 
@@ -121,18 +109,17 @@ contract MasterVault is Initializable, ERC4626Upgradeable, AccessControlUpgradea
     }
 
     function _setSubVault(IERC4626 _subVault, uint256 minSubVaultExchRateWad) internal {
-        if (address(_subVault) == address(0)) revert SubVaultCannotBeZeroAddress();
-        if (totalSupply() == 0) revert MustHaveSupplyBeforeSettingSubVault();
-        if (address(_subVault.asset()) != address(asset())) revert SubVaultAssetMismatch();
-
-        IERC20(asset()).safeApprove(address(_subVault), type(uint256).max);
-        uint256 subShares = _subVault.deposit(totalAssets(), address(this));
+        IERC20 underlyingAsset = IERC20(asset());
+        if (address(subVault) != address(0)) revert SubVaultAlreadySet();
+        if (address(_subVault.asset()) != address(underlyingAsset)) revert SubVaultAssetMismatch();
 
         subVault = _subVault;
 
-        uint256 _subVaultExchRateWad = subShares.mulDiv(1e18, totalAssets(), MathUpgradeable.Rounding.Down);
-        if (_subVaultExchRateWad < minSubVaultExchRateWad) revert SubVaultExchangeRateTooLow();
-        subVaultExchRateWad = _subVaultExchRateWad;
+        IERC20(asset()).safeApprove(address(_subVault), type(uint256).max);
+        _subVault.deposit(underlyingAsset.balanceOf(address(this)), address(this));
+
+        uint256 subVaultExchRateWad = _subVault.balanceOf(address(this)).mulDiv(1e18, totalSupply(), MathUpgradeable.Rounding.Down);
+        if (subVaultExchRateWad < minSubVaultExchRateWad) revert NewSubVaultExchangeRateTooLow();
 
         emit SubvaultChanged(address(0), address(_subVault));
     }
@@ -141,14 +128,13 @@ contract MasterVault is Initializable, ERC4626Upgradeable, AccessControlUpgradea
         IERC4626 oldSubVault = subVault;
         if (address(oldSubVault) == address(0)) revert NoExistingSubVault();
 
-        uint256 _totalSupply = totalSupply();
-        uint256 assetReceived = oldSubVault.withdraw(oldSubVault.maxWithdraw(address(this)), address(this), address(this));
-        uint256 effectiveAssetExchRateWad = assetReceived.mulDiv(1e18, _totalSupply, MathUpgradeable.Rounding.Down);
-        if (effectiveAssetExchRateWad < minAssetExchRateWad) revert TooFewAssetsReceived();
+        subVault = IERC4626(address(0));
 
+        oldSubVault.redeem(oldSubVault.balanceOf(address(this)), address(this), address(this));
         IERC20(asset()).safeApprove(address(oldSubVault), 0);
-        subVault = IERC4626(address(0));
-        subVaultExchRateWad = 1e18;
+
+        uint256 assetExchRateWad = IERC20(asset()).balanceOf(address(this)).mulDiv(1e18, totalSupply(), MathUpgradeable.Rounding.Down);
+        if (assetExchRateWad < minAssetExchRateWad) revert SubVaultExchangeRateTooLow();
 
         emit SubvaultChanged(address(oldSubVault), address(0));
     }
@@ -166,11 +152,13 @@ contract MasterVault is Initializable, ERC4626Upgradeable, AccessControlUpgradea
     }
 
     function masterSharesToSubShares(uint256 masterShares, MathUpgradeable.Rounding rounding) public view returns (uint256) {
-        return masterShares.mulDiv(subVaultExchRateWad, 1e18, rounding);
+        // masterShares * totalSubVaultShares / totalMasterShares
+        return masterShares.mulDiv(subVault.balanceOf(address(this)), totalSupply(), rounding);
     }
 
     function subSharesToMasterShares(uint256 subShares, MathUpgradeable.Rounding rounding) public view returns (uint256) {
-        return subShares.mulDiv(1e18, subVaultExchRateWad, rounding);
+        // subShares * totalMasterShares / totalSubVaultShares
+        return subShares.mulDiv(totalSupply(), subVault.balanceOf(address(this)), rounding);
     }
 
     /// @notice Toggle performance fee collection on/off

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

@@ -122,7 +122,6 @@ contract MasterVaultTest is Test {
         vault.setSubVault(subVault, minSubVaultExchRateWad);
 
         assertEq(address(vault.subVault()), address(subVault), "SubVault should be set");
-        assertEq(vault.subVaultExchRateWad(), 1e18, "Exchange rate should be 1:1 initially");
         assertEq(vault.totalAssets(), depositAmount, "Total assets should remain the same");
         assertEq(subVault.balanceOf(address(vault)), depositAmount, "SubVault should have received assets");
     }
@@ -164,7 +163,6 @@ contract MasterVaultTest is Test {
         vault.switchSubVault(newSubVault, minAssetExchRateWad, minNewSubVaultExchRateWad);
 
         assertEq(address(vault.subVault()), address(newSubVault), "New subvault should be set");
-        assertEq(vault.subVaultExchRateWad(), 1e18, "Exchange rate should remain 1:1");
         assertEq(vault.totalAssets(), depositAmount, "Total assets should remain the same");
         assertEq(oldSubVault.balanceOf(address(vault)), 0, "Old subvault should have no assets");
         assertEq(newSubVault.balanceOf(address(vault)), depositAmount, "New subvault should have received assets");
@@ -188,7 +186,6 @@ contract MasterVaultTest is Test {
 
         assertEq(address(vault.subVault()), address(subVault), "SubVault should be set");
         assertEq(subVault.balanceOf(address(vault)), depositAmount, "SubVault should have assets");
-        assertEq(vault.subVaultExchRateWad(), 1e18, "Exchange rate should be 1:1");
 
         uint256 minAssetExchRateWad = 1e18;
 
@@ -198,7 +195,6 @@ contract MasterVaultTest is Test {
         vault.revokeSubVault(minAssetExchRateWad);
 
         assertEq(address(vault.subVault()), address(0), "SubVault should be revoked");
-        assertEq(vault.subVaultExchRateWad(), 1e18, "Exchange rate should reset to 1:1");
         assertEq(vault.totalAssets(), depositAmount, "Total assets should remain the same");
         assertEq(subVault.balanceOf(address(vault)), 0, "SubVault should have no assets");
         assertEq(token.balanceOf(address(vault)), depositAmount, "MasterVault should have assets directly");