diff --git a/contracts/tokenbridge/libraries/vault/IMasterVaultFactory.sol b/contracts/tokenbridge/libraries/vault/IMasterVaultFactory.sol index 7ac654fca..2a7617abc 100644 --- a/contracts/tokenbridge/libraries/vault/IMasterVaultFactory.sol +++ b/contracts/tokenbridge/libraries/vault/IMasterVaultFactory.sol @@ -3,11 +3,9 @@ pragma solidity ^0.8.0; interface IMasterVaultFactory { event VaultDeployed(address indexed token, address indexed vault); - event SubVaultSet(address indexed masterVault, address indexed subVault); function initialize(address _owner) external; function deployVault(address token) external returns (address vault); function calculateVaultAddress(address token) external view returns (address); function getVault(address token) external returns (address); - function setSubVault(address masterVault, address subVault, uint256 minSubVaultExchRateWad) external; } \ No newline at end of file diff --git a/contracts/tokenbridge/libraries/vault/MasterVault.sol b/contracts/tokenbridge/libraries/vault/MasterVault.sol index 0c1ab34c4..4f6c39b64 100644 --- a/contracts/tokenbridge/libraries/vault/MasterVault.sol +++ b/contracts/tokenbridge/libraries/vault/MasterVault.sol @@ -6,16 +6,21 @@ import {ERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/ import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {IERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol"; import {IERC4626} from "@openzeppelin/contracts/interfaces/IERC4626.sol"; -import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import {AccessControlUpgradeable} from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; +import {PausableUpgradeable} from "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol"; import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import {MathUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/math/MathUpgradeable.sol"; import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; -contract MasterVault is Initializable, ERC4626Upgradeable, OwnableUpgradeable { +contract MasterVault is Initializable, ERC4626Upgradeable, AccessControlUpgradeable, PausableUpgradeable { using SafeERC20 for IERC20; using MathUpgradeable for uint256; + bytes32 public constant VAULT_MANAGER_ROLE = keccak256("VAULT_MANAGER_ROLE"); + bytes32 public constant FEE_MANAGER_ROLE = keccak256("FEE_MANAGER_ROLE"); + bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); + error TooFewSharesReceived(); error TooManySharesBurned(); error TooManyAssetsDeposited(); @@ -61,7 +66,17 @@ contract MasterVault is Initializable, ERC4626Upgradeable, OwnableUpgradeable { __ERC20_init(_name, _symbol); __ERC4626_init(IERC20Upgradeable(address(_asset))); - _transferOwnership(_owner); + __AccessControl_init(); + __Pausable_init(); + + _setRoleAdmin(VAULT_MANAGER_ROLE, DEFAULT_ADMIN_ROLE); + _setRoleAdmin(FEE_MANAGER_ROLE, DEFAULT_ADMIN_ROLE); + _setRoleAdmin(PAUSER_ROLE, DEFAULT_ADMIN_ROLE); + + _grantRole(DEFAULT_ADMIN_ROLE, _owner); + _grantRole(VAULT_MANAGER_ROLE, _owner); + _grantRole(FEE_MANAGER_ROLE, _owner); // todo: consider permissionless by default + _grantRole(PAUSER_ROLE, _owner); subVaultExchRateWad = 1e18; } @@ -94,14 +109,14 @@ contract MasterVault is Initializable, ERC4626Upgradeable, OwnableUpgradeable { /// @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 onlyOwner { + function setSubVault(IERC4626 _subVault, uint256 minSubVaultExchRateWad) external onlyRole(VAULT_MANAGER_ROLE) { if (address(subVault) != address(0)) revert SubVaultAlreadySet(); _setSubVault(_subVault, minSubVaultExchRateWad); } /// @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 onlyOwner { + function revokeSubVault(uint256 minAssetExchRateWad) external onlyRole(VAULT_MANAGER_ROLE) { _revokeSubVault(minAssetExchRateWad); } @@ -142,7 +157,7 @@ contract MasterVault is Initializable, ERC4626Upgradeable, OwnableUpgradeable { /// @param newSubVault The new subvault to switch to, or zero address to revoke current subvault /// @param minAssetExchRateWad Minimum acceptable ratio (times 1e18) of assets received from old subvault to outstanding MasterVault shares /// @param minNewSubVaultExchRateWad Minimum acceptable ratio (times 1e18) of new subvault shares to outstanding MasterVault shares after deposit - function switchSubVault(IERC4626 newSubVault, uint256 minAssetExchRateWad, uint256 minNewSubVaultExchRateWad) external onlyOwner { + function switchSubVault(IERC4626 newSubVault, uint256 minAssetExchRateWad, uint256 minNewSubVaultExchRateWad) external onlyRole(VAULT_MANAGER_ROLE) { _revokeSubVault(minAssetExchRateWad); if (address(newSubVault) != address(0)) { @@ -160,22 +175,22 @@ contract MasterVault is Initializable, ERC4626Upgradeable, OwnableUpgradeable { /// @notice Toggle performance fee collection on/off /// @param enabled True to enable performance fees, false to disable - function setPerformanceFee(bool enabled) external onlyOwner { + function setPerformanceFee(bool enabled) external onlyRole(VAULT_MANAGER_ROLE) { enablePerformanceFee = enabled; emit PerformanceFeeToggled(enabled); } /// @notice Set the beneficiary address for performance fees /// @param newBeneficiary Address to receive performance fees, zero address defaults to owner - function setBeneficiary(address newBeneficiary) external onlyOwner { + function setBeneficiary(address newBeneficiary) external onlyRole(FEE_MANAGER_ROLE) { address oldBeneficiary = beneficiary; beneficiary = newBeneficiary; emit BeneficiaryUpdated(oldBeneficiary, newBeneficiary); } /// @notice Withdraw all accumulated performance fees to beneficiary - /// @dev Only callable by owner when performance fees are enabled - function withdrawPerformanceFees() external onlyOwner { + /// @dev Only callable by fee manager when performance fees are enabled + function withdrawPerformanceFees() external onlyRole(FEE_MANAGER_ROLE) { if (!enablePerformanceFee) revert PerformanceFeeDisabled(); if (beneficiary == address(0)) revert BeneficiaryNotSet(); @@ -189,6 +204,14 @@ contract MasterVault is Initializable, ERC4626Upgradeable, OwnableUpgradeable { } } + function pause() external onlyRole(PAUSER_ROLE) { + _pause(); + } + + function unpause() external onlyRole(PAUSER_ROLE) { + _unpause(); + } + /** @dev See {IERC4626-totalAssets}. */ function totalAssets() public view virtual override returns (uint256) { IERC4626 _subVault = subVault; @@ -208,6 +231,9 @@ contract MasterVault is Initializable, ERC4626Upgradeable, OwnableUpgradeable { /** @dev See {IERC4626-maxMint}. */ function maxMint(address) public view virtual override returns (uint256) { + if (address(subVault) == address(0)) { + return type(uint256).max; + } uint256 subShares = subVault.maxMint(address(this)); if (subShares == type(uint256).max) { return type(uint256).max; @@ -255,7 +281,7 @@ contract MasterVault is Initializable, ERC4626Upgradeable, OwnableUpgradeable { address receiver, uint256 assets, uint256 shares - ) internal virtual override { + ) internal virtual override whenNotPaused { super._deposit(caller, receiver, assets, shares); totalPrincipal += assets; @@ -274,7 +300,7 @@ contract MasterVault is Initializable, ERC4626Upgradeable, OwnableUpgradeable { address _owner, uint256 assets, uint256 shares - ) internal virtual override { + ) internal virtual override whenNotPaused { totalPrincipal -= assets; IERC4626 _subVault = subVault; diff --git a/contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol b/contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol index 24a19fb81..0259c84d4 100644 --- a/contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol +++ b/contracts/tokenbridge/libraries/vault/MasterVaultFactory.sol @@ -3,7 +3,6 @@ pragma solidity ^0.8.0; import "@openzeppelin/contracts/utils/Create2.sol"; -import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol"; import "../ClonableBeaconProxy.sol"; @@ -11,15 +10,15 @@ import "./IMasterVault.sol"; import "./IMasterVaultFactory.sol"; import "./MasterVault.sol"; -contract MasterVaultFactory is IMasterVaultFactory, OwnableUpgradeable { +contract MasterVaultFactory is IMasterVaultFactory, Initializable { error ZeroAddress(); error BeaconNotDeployed(); BeaconProxyFactory public beaconProxyFactory; + address public owner; function initialize(address _owner) public initializer { - _transferOwnership(_owner); - + owner = _owner; MasterVault masterVaultImplementation = new MasterVault(); UpgradeableBeacon beacon = new UpgradeableBeacon(address(masterVaultImplementation)); beaconProxyFactory = new BeaconProxyFactory(); @@ -44,7 +43,7 @@ contract MasterVaultFactory is IMasterVaultFactory, OwnableUpgradeable { string memory name = string(abi.encodePacked("Master ", tokenMetadata.name())); string memory symbol = string(abi.encodePacked("m", tokenMetadata.symbol())); - MasterVault(vault).initialize(IERC20(token), name, symbol, address(this)); + MasterVault(vault).initialize(IERC20(token), name, symbol, owner); emit VaultDeployed(token, vault); } @@ -65,14 +64,4 @@ contract MasterVaultFactory is IMasterVaultFactory, OwnableUpgradeable { } return vault; } - - // todo: consider a method to enable bridge owner to transfer specific master vault ownership to new address - function setSubVault( - address masterVault, - address subVault, - uint256 minSubVaultExchRateWad - ) external onlyOwner { - IMasterVault(masterVault).setSubVault(subVault, minSubVaultExchRateWad); - emit SubVaultSet(masterVault, subVault); - } } diff --git a/test-foundry/libraries/vault/MasterVault.t.sol b/test-foundry/libraries/vault/MasterVault.t.sol index 84a628e32..f78c4f2c2 100644 --- a/test-foundry/libraries/vault/MasterVault.t.sol +++ b/test-foundry/libraries/vault/MasterVault.t.sol @@ -8,6 +8,7 @@ import { MockSubVault } from "../../../contracts/tokenbridge/test/MockSubVault.s import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import { UpgradeableBeacon } from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol"; import { BeaconProxyFactory, ClonableBeaconProxy } from "../../../contracts/tokenbridge/libraries/ClonableBeaconProxy.sol"; +import { IAccessControl } from "@openzeppelin/contracts/access/IAccessControl.sol"; contract MasterVaultTest is Test { MasterVault public vault; @@ -45,6 +46,10 @@ contract MasterVaultTest is Test { assertEq(vault.totalSupply(), 0, "Invalid initial supply"); assertEq(vault.totalAssets(), 0, "Invalid initial assets"); assertEq(address(vault.subVault()), address(0), "SubVault should be zero initially"); + + assertTrue(vault.hasRole(vault.DEFAULT_ADMIN_ROLE(), address(this)), "Should have DEFAULT_ADMIN_ROLE"); + assertTrue(vault.hasRole(vault.VAULT_MANAGER_ROLE(), address(this)), "Should have VAULT_MANAGER_ROLE"); + assertTrue(vault.hasRole(vault.FEE_MANAGER_ROLE(), address(this)), "Should have FEE_MANAGER_ROLE"); } function test_WithoutSubvault_deposit() public { @@ -297,4 +302,215 @@ contract MasterVaultTest is Test { assertEq(vault.name(), name, "Name should remain after upgrade"); } + function test_setSubVault_revert_NotVaultManager() public { + MockSubVault subVault = new MockSubVault( + IERC20(address(token)), + "Sub Vault Token", + "svTST" + ); + + vm.startPrank(user); + token.mint(); + uint256 depositAmount = token.balanceOf(user); + token.approve(address(vault), depositAmount); + vault.deposit(depositAmount, user, 0); + + vm.expectRevert(); + vault.setSubVault(subVault, 1e18); + + vm.stopPrank(); + } + + function test_setBeneficiary_revert_NotFeeManager() public { + address newBeneficiary = address(0x999); + + vm.prank(user); + vm.expectRevert(); + vault.setBeneficiary(newBeneficiary); + } + + function test_withdrawPerformanceFees_revert_NotFeeManager() public { + vm.prank(user); + vm.expectRevert(); + vault.withdrawPerformanceFees(); + } + + function test_roleAdmin() public { + address vaultManager = address(0x1111); + address feeManager = address(0x2222); + + vault.grantRole(vault.VAULT_MANAGER_ROLE(), vaultManager); + vault.grantRole(vault.FEE_MANAGER_ROLE(), feeManager); + + assertTrue(vault.hasRole(vault.VAULT_MANAGER_ROLE(), vaultManager), "Should have VAULT_MANAGER_ROLE"); + assertTrue(vault.hasRole(vault.FEE_MANAGER_ROLE(), feeManager), "Should have FEE_MANAGER_ROLE"); + + vault.revokeRole(vault.VAULT_MANAGER_ROLE(), vaultManager); + assertFalse(vault.hasRole(vault.VAULT_MANAGER_ROLE(), vaultManager), "Should not have VAULT_MANAGER_ROLE"); + } + + function test_multipleRoleHolders() public { + address vaultManager1 = address(0x1111); + address vaultManager2 = address(0x2222); + + vault.grantRole(vault.VAULT_MANAGER_ROLE(), vaultManager1); + vault.grantRole(vault.VAULT_MANAGER_ROLE(), vaultManager2); + + assertTrue(vault.hasRole(vault.VAULT_MANAGER_ROLE(), vaultManager1), "Manager1 should have VAULT_MANAGER_ROLE"); + assertTrue(vault.hasRole(vault.VAULT_MANAGER_ROLE(), vaultManager2), "Manager2 should have VAULT_MANAGER_ROLE"); + + MockSubVault subVault = new MockSubVault( + IERC20(address(token)), + "Sub Vault Token", + "svTST" + ); + + vm.startPrank(user); + token.mint(); + uint256 depositAmount = token.balanceOf(user); + token.approve(address(vault), depositAmount); + vault.deposit(depositAmount, user, 0); + vm.stopPrank(); + + vm.prank(vaultManager1); + vault.setSubVault(subVault, 1e18); + + assertEq(address(vault.subVault()), address(subVault), "SubVault should be set by manager1"); + } + + function test_initialize_pauserRole() public { + assertTrue(vault.hasRole(vault.PAUSER_ROLE(), address(this)), "Should have PAUSER_ROLE"); + assertFalse(vault.paused(), "Should not be paused initially"); + } + + function test_pause() public { + assertFalse(vault.paused(), "Should not be paused initially"); + + vault.pause(); + + assertTrue(vault.paused(), "Should be paused"); + } + + function test_unpause() public { + vault.pause(); + assertTrue(vault.paused(), "Should be paused"); + + vault.unpause(); + + assertFalse(vault.paused(), "Should not be paused"); + } + + function test_pause_revert_NotPauser() public { + vm.prank(user); + vm.expectRevert(); + vault.pause(); + } + + function test_unpause_revert_NotPauser() public { + vault.pause(); + + vm.prank(user); + vm.expectRevert(); + vault.unpause(); + } + + function test_deposit_revert_WhenPaused() public { + vm.startPrank(user); + token.mint(); + uint256 depositAmount = token.balanceOf(user); + token.approve(address(vault), depositAmount); + vm.stopPrank(); + + vault.pause(); + + vm.prank(user); + vm.expectRevert("Pausable: paused"); + vault.deposit(depositAmount, user, 0); + } + + function test_withdraw_revert_WhenPaused() public { + vm.startPrank(user); + token.mint(); + uint256 depositAmount = token.balanceOf(user); + token.approve(address(vault), depositAmount); + vault.deposit(depositAmount, user, 0); + vm.stopPrank(); + + vault.pause(); + + vm.prank(user); + vm.expectRevert("Pausable: paused"); + vault.withdraw(depositAmount / 2, user, user, type(uint256).max); + } + + function test_mint_revert_WhenPaused() public { + vm.startPrank(user); + token.mint(); + uint256 depositAmount = token.balanceOf(user); + token.approve(address(vault), depositAmount); + vm.stopPrank(); + + vault.pause(); + + vm.prank(user); + vm.expectRevert("Pausable: paused"); + vault.mint(100, user, type(uint256).max); + } + + function test_redeem_revert_WhenPaused() public { + vm.startPrank(user); + token.mint(); + uint256 depositAmount = token.balanceOf(user); + token.approve(address(vault), depositAmount); + uint256 shares = vault.deposit(depositAmount, user, 0); + vm.stopPrank(); + + vault.pause(); + + vm.prank(user); + vm.expectRevert("Pausable: paused"); + vault.redeem(shares / 2, user, user, 0); + } + + function test_pauseUnpauseFlow() public { + vm.startPrank(user); + token.mint(); + uint256 depositAmount = token.balanceOf(user); + token.approve(address(vault), depositAmount); + vault.deposit(depositAmount / 2, user, 0); + vm.stopPrank(); + + vault.pause(); + + vm.prank(user); + vm.expectRevert("Pausable: paused"); + vault.deposit(depositAmount / 2, user, 0); + + vault.unpause(); + + vm.prank(user); + vault.deposit(depositAmount / 2, user, 0); + + assertEq(token.balanceOf(user), 0, "All tokens should be deposited"); + } + + function test_multiplePausers() public { + address pauser1 = address(0x3333); + address pauser2 = address(0x4444); + + vault.grantRole(vault.PAUSER_ROLE(), pauser1); + vault.grantRole(vault.PAUSER_ROLE(), pauser2); + + assertTrue(vault.hasRole(vault.PAUSER_ROLE(), pauser1), "Pauser1 should have PAUSER_ROLE"); + assertTrue(vault.hasRole(vault.PAUSER_ROLE(), pauser2), "Pauser2 should have PAUSER_ROLE"); + + vm.prank(pauser1); + vault.pause(); + assertTrue(vault.paused(), "Should be paused by pauser1"); + + vm.prank(pauser2); + vault.unpause(); + assertFalse(vault.paused(), "Should be unpaused by pauser2"); + } + } diff --git a/test-foundry/libraries/vault/MasterVaultFactory.t.sol b/test-foundry/libraries/vault/MasterVaultFactory.t.sol index 268b593a8..48359c390 100644 --- a/test-foundry/libraries/vault/MasterVaultFactory.t.sol +++ b/test-foundry/libraries/vault/MasterVaultFactory.t.sol @@ -41,7 +41,7 @@ contract MasterVaultFactoryTest is Test { MasterVault vault = MasterVault(deployedVault); assertEq(address(vault.asset()), address(token), "Invalid vault asset"); - assertEq(vault.owner(), address(factory), "Invalid vault owner"); + assertTrue(vault.hasRole(vault.DEFAULT_ADMIN_ROLE(), owner), "Factory owner should have DEFAULT_ADMIN_ROLE"); } function test_deployVault_RevertZeroAddress() public { @@ -108,7 +108,7 @@ contract MasterVaultFactoryTest is Test { assertEq(UpgradeableBeacon(factory.beaconProxyFactory().beacon()).implementation(), address(newImplementation), "Beacon should point to new implementation"); - MasterVault(vault1).owner(); - MasterVault(vault2).owner(); + assertTrue(MasterVault(vault1).hasRole(MasterVault(vault1).DEFAULT_ADMIN_ROLE(), owner), "Vault1 should have owner as admin"); + assertTrue(MasterVault(vault2).hasRole(MasterVault(vault2).DEFAULT_ADMIN_ROLE(), owner), "Vault2 should have owner as admin"); } } \ No newline at end of file