From 868b331fc1e69181f586b3bfe82d5468a4a2e0c2 Mon Sep 17 00:00:00 2001 From: Ivan Zhelyazkov Date: Thu, 10 Oct 2024 16:41:53 +0300 Subject: [PATCH] add vault deployment script --- components/Contracts.ts | 4 +- contracts/helpers/TestReentrancyVault.sol | 28 +++ contracts/vault/Vault.sol | 81 +++++++ deploy/scripts/mainnet/0017-Vault.ts | 31 +++ ...pgrade.ts => 0018-CarbonVortex-upgrade.ts} | 16 +- test/forge/Vault.t.sol | 199 ++++++++++++++++++ utils/Deploy.ts | 16 +- 7 files changed, 368 insertions(+), 7 deletions(-) create mode 100644 contracts/helpers/TestReentrancyVault.sol create mode 100644 contracts/vault/Vault.sol create mode 100644 deploy/scripts/mainnet/0017-Vault.ts rename deploy/scripts/mainnet/{0017-CarbonVortex-upgrade.ts => 0018-CarbonVortex-upgrade.ts} (64%) create mode 100644 test/forge/Vault.t.sol diff --git a/components/Contracts.ts b/components/Contracts.ts index ad31fcb3..f93a5313 100644 --- a/components/Contracts.ts +++ b/components/Contracts.ts @@ -20,7 +20,8 @@ import { TestTokenType__factory, TestUpgradeable__factory, TestVoucher__factory, - Voucher__factory + Voucher__factory, + Vault__factory } from '../typechain-types'; import { deployOrAttach } from './ContractBuilder'; import { Signer } from 'ethers'; @@ -37,6 +38,7 @@ const getContracts = (signer?: Signer) => ({ MockBancorNetworkV3: deployOrAttach('MockBancorNetworkV3', MockBancorNetworkV3__factory, signer), ProxyAdmin: deployOrAttach('ProxyAdmin', ProxyAdmin__factory, signer), Voucher: deployOrAttach('Voucher', Voucher__factory, signer), + Vault: deployOrAttach('Vault', Vault__factory, signer), TestERC20FeeOnTransfer: deployOrAttach('TestERC20FeeOnTransfer', TestERC20FeeOnTransfer__factory, signer), TestERC20Burnable: deployOrAttach('TestERC20Burnable', TestERC20Burnable__factory, signer), TestERC20Token: deployOrAttach('TestERC20Token', TestERC20Token__factory, signer), diff --git a/contracts/helpers/TestReentrancyVault.sol b/contracts/helpers/TestReentrancyVault.sol new file mode 100644 index 00000000..a8751e31 --- /dev/null +++ b/contracts/helpers/TestReentrancyVault.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +import { Vault } from "../vault/Vault.sol"; +import { Token } from "../token/Token.sol"; + +/** + * @dev test re-entrancy protection for Vault + */ +contract TestReentrancyVault { + Vault private immutable _vault; + Token private immutable _token; + + constructor(Vault vaultInit, Token tokenInit) { + _vault = vaultInit; + _token = tokenInit; + } + + receive() external payable { + // re-enter withdrawFunds, reverting the tx + _vault.withdrawFunds(_token, payable(address(this)), 1000); + } + + /// @dev try to reenter withdraw funds + function tryReenterWithdrawFunds(Token token, address payable target, uint256 amount) external { + _vault.withdrawFunds(token, target, amount); + } +} diff --git a/contracts/vault/Vault.sol b/contracts/vault/Vault.sol new file mode 100644 index 00000000..c4ba1079 --- /dev/null +++ b/contracts/vault/Vault.sol @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +import { Address } from "@openzeppelin/contracts/utils/Address.sol"; +import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; +import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; +import { AccessControlEnumerable } from "@openzeppelin/contracts/access/AccessControlEnumerable.sol"; + +import { Token } from "../token/Token.sol"; + +import { Utils, AccessDenied } from "../utility/Utils.sol"; + +contract Vault is ReentrancyGuard, AccessControlEnumerable, Utils { + using Address for address payable; + using SafeERC20 for IERC20; + + // the admin role is used to grant and revoke the asset manager role + bytes32 internal constant ROLE_ADMIN = keccak256("ROLE_ADMIN"); + // the asset manager role is required to access all the funds + bytes32 private constant ROLE_ASSET_MANAGER = keccak256("ROLE_ASSET_MANAGER"); + + /** + * @dev triggered when tokens have been withdrawn from the vault + */ + event FundsWithdrawn(Token indexed token, address indexed caller, address indexed target, uint256 amount); + + /** + * @dev used to initialize the implementation + */ + constructor() { + // set up administrative roles + _setRoleAdmin(ROLE_ADMIN, ROLE_ADMIN); + _setRoleAdmin(ROLE_ASSET_MANAGER, ROLE_ADMIN); + // allow the deployer to initially be the admin of the contract + _setupRole(ROLE_ADMIN, msg.sender); + } + + // solhint-enable func-name-mixedcase + + /** + * @dev authorize the contract to receive the native token + */ + receive() external payable {} + + /** + * @dev returns the admin role + */ + function roleAdmin() external pure returns (bytes32) { + return ROLE_ADMIN; + } + + /** + * @dev returns the asset manager role + */ + function roleAssetManager() external pure returns (bytes32) { + return ROLE_ASSET_MANAGER; + } + + /** + * @dev withdraws funds held by the contract and sends them to an account + */ + function withdrawFunds( + Token token, + address payable target, + uint256 amount + ) external validAddress(target) nonReentrant { + if (!hasRole(ROLE_ASSET_MANAGER, msg.sender)) { + revert AccessDenied(); + } + + if (amount == 0) { + return; + } + + // safe due to nonReentrant modifier (forwards all available gas in case of ETH) + token.unsafeTransfer(target, amount); + + emit FundsWithdrawn({ token: token, caller: msg.sender, target: target, amount: amount }); + } +} diff --git a/deploy/scripts/mainnet/0017-Vault.ts b/deploy/scripts/mainnet/0017-Vault.ts new file mode 100644 index 00000000..09d2ec4f --- /dev/null +++ b/deploy/scripts/mainnet/0017-Vault.ts @@ -0,0 +1,31 @@ +import { deploy, DeployedContracts, grantRole, InstanceName, setDeploymentMetadata } from '../../../utils/Deploy'; +import { DeployFunction } from 'hardhat-deploy/types'; +import { HardhatRuntimeEnvironment } from 'hardhat/types'; +import { Roles } from '../../../utils/Roles'; + +/** + * @dev deploy vault and grant asset manager role to the vortex + */ +const func: DeployFunction = async ({ getNamedAccounts }: HardhatRuntimeEnvironment) => { + const { deployer } = await getNamedAccounts(); + + await deploy({ + name: InstanceName.Vault, + from: deployer, + args: [] + }); + + const carbonVortex = await DeployedContracts.CarbonVortex.deployed(); + + // grant asset manager role to carbon vortex + await grantRole({ + name: InstanceName.Vault, + id: Roles.Vault.ROLE_ASSET_MANAGER, + member: carbonVortex.address, + from: deployer + }); + + return true; +}; + +export default setDeploymentMetadata(__filename, func); diff --git a/deploy/scripts/mainnet/0017-CarbonVortex-upgrade.ts b/deploy/scripts/mainnet/0018-CarbonVortex-upgrade.ts similarity index 64% rename from deploy/scripts/mainnet/0017-CarbonVortex-upgrade.ts rename to deploy/scripts/mainnet/0018-CarbonVortex-upgrade.ts index 13aeae25..cd28a008 100644 --- a/deploy/scripts/mainnet/0017-CarbonVortex-upgrade.ts +++ b/deploy/scripts/mainnet/0018-CarbonVortex-upgrade.ts @@ -1,6 +1,6 @@ import { DeployFunction } from 'hardhat-deploy/types'; import { HardhatRuntimeEnvironment } from 'hardhat/types'; -import { DeployedContracts, upgradeProxy, InstanceName, setDeploymentMetadata } from '../../../utils/Deploy'; +import { DeployedContracts, upgradeProxy, InstanceName, setDeploymentMetadata, execute } from '../../../utils/Deploy'; import { NATIVE_TOKEN_ADDRESS } from '../../../utils/Constants'; /** @@ -10,19 +10,29 @@ import { NATIVE_TOKEN_ADDRESS } from '../../../utils/Constants'; * make transfer address a settable variable */ const func: DeployFunction = async ({ getNamedAccounts }: HardhatRuntimeEnvironment) => { - const { deployer, bnt, vault } = await getNamedAccounts(); + const { deployer, bnt } = await getNamedAccounts(); const carbonController = await DeployedContracts.CarbonController.deployed(); + const vault = await DeployedContracts.Vault.deployed(); + await upgradeProxy({ name: InstanceName.CarbonVortex, from: deployer, - args: [carbonController.address, vault, NATIVE_TOKEN_ADDRESS, bnt], + args: [carbonController.address, vault.address, NATIVE_TOKEN_ADDRESS, bnt], checkVersion: false, proxy: { args: [bnt] } }); + // Set the transfer address to BNT in the vortex contract + await execute({ + name: InstanceName.CarbonVortex, + methodName: 'setTransferAddress', + args: [bnt], + from: deployer + }); + return true; }; diff --git a/test/forge/Vault.t.sol b/test/forge/Vault.t.sol new file mode 100644 index 00000000..a6a1909e --- /dev/null +++ b/test/forge/Vault.t.sol @@ -0,0 +1,199 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +import { Test } from "forge-std/Test.sol"; +import { Utilities } from "./Utilities.t.sol"; + +import { Address } from "@openzeppelin/contracts/utils/Address.sol"; +import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; + +import { Vault } from "../../contracts/vault/Vault.sol"; +import { Token, NATIVE_TOKEN } from "../../contracts/token/Token.sol"; +import { AccessDenied, InvalidAddress } from "../../contracts/utility/Utils.sol"; +import { TestERC20Token } from "../../contracts/helpers/TestERC20Token.sol"; +import { TestReentrancyVault } from "../../contracts/helpers/TestReentrancyVault.sol"; + +contract VaultTest is Test { + using Address for address payable; + + bytes32 private constant ROLE_ADMIN = keccak256("ROLE_ADMIN"); + bytes32 private constant ROLE_ASSET_MANAGER = keccak256("ROLE_ASSET_MANAGER"); + uint256 private constant MAX_WITHDRAW_AMOUNT = 100_000_000 ether; + + Utilities private utils; + Vault private vault; + ProxyAdmin private proxyAdmin; + TestERC20Token private token; + + address payable[] private users; + address payable private admin; + address payable private user1; + address payable private user2; + + /** + * @dev triggered when tokens have been withdrawn from the vault + */ + event FundsWithdrawn(Token indexed token, address indexed caller, address indexed target, uint256 amount); + + /// @dev function to set up state before tests + function setUp() public virtual { + utils = new Utilities(); + // create 4 users + users = utils.createUsers(3); + admin = users[0]; + user1 = users[1]; + user2 = users[2]; + + // deploy contracts from admin + vm.startPrank(admin); + vault = new Vault(); + + // deploy proxy admin + proxyAdmin = new ProxyAdmin(); + + // deploy test token + token = new TestERC20Token("TKN1", "TKN1", 1_000_000_000 ether); + + // transfer some tokens to the vault + token.transfer(address(vault), MAX_WITHDRAW_AMOUNT); + // transfer eth to the vault + vm.deal(address(vault), MAX_WITHDRAW_AMOUNT); + + // set user1 to be the asset manager + vault.grantRole(ROLE_ASSET_MANAGER, user1); + + vm.stopPrank(); + } + + /** + * @dev construction tests + */ + + /// @dev test should be initialized properly + function testShouldBeInitializedProperly() public view { + assertEq(ROLE_ADMIN, vault.roleAdmin()); + assertEq(ROLE_ASSET_MANAGER, vault.roleAssetManager()); + + assertEq(admin, vault.getRoleMember(ROLE_ADMIN, 0)); + assertEq(user1, vault.getRoleMember(ROLE_ASSET_MANAGER, 0)); + + assertEq(1, vault.getRoleMemberCount(ROLE_ADMIN)); + assertEq(1, vault.getRoleMemberCount(ROLE_ASSET_MANAGER)); + } + + /// @dev test should revert when attempting to withdraw funds without the asset manager role + function testShouldRevertWhenAttemptingToWithdrawFundsWithoutTheAssetManagerRole() public { + uint256 withdrawAmount = 1000; + vm.prank(user2); + vm.expectRevert(AccessDenied.selector); + vault.withdrawFunds(Token.wrap(address(token)), user2, withdrawAmount); + } + + /// @dev test should revert when attempting to withdraw funds to an invalid address + function testShouldRevertWhenAttemptingToWithdrawFundsToAnInvalidAddress() public { + uint256 withdrawAmount = 1000; + vm.prank(user1); + vm.expectRevert(InvalidAddress.selector); + vault.withdrawFunds(Token.wrap(address(token)), payable(address(0)), withdrawAmount); + } + + /// @dev test admin should be able to grant the asset manager role + function testAdminShouldBeAbleToGrandTheAssetManagerRole() public { + vm.prank(admin); + // grant asset manager role to user2 + vault.grantRole(ROLE_ASSET_MANAGER, user2); + // test user2 has asset manager role + assertEq(user2, vault.getRoleMember(ROLE_ASSET_MANAGER, 1)); + } + + /// @dev test admin should be able to grant the asset manager role + function testAdminShouldBeAbleToRevokeTheAssetManagerRole() public { + // assert there is only one asset manager role and that is user1 + assertEq(user1, vault.getRoleMember(ROLE_ASSET_MANAGER, 0)); + assertEq(1, vault.getRoleMemberCount(ROLE_ASSET_MANAGER)); + + vm.startPrank(admin); + // revoke asset manager role from user1 + vault.revokeRole(ROLE_ASSET_MANAGER, user1); + // test user1 doesn't have asset manager role + assertEq(0, vault.getRoleMemberCount(ROLE_ASSET_MANAGER)); + } + + /// @dev test addresses with the asset manager role should be able to withdraw tokens + function testAssetManagerShouldBeAbleToWithdrawTokens(uint256 withdrawAmount) public { + withdrawAmount = bound(withdrawAmount, 0, MAX_WITHDRAW_AMOUNT); + + uint256 balanceBeforeVault = token.balanceOf(address(vault)); + uint256 balanceBeforeUser2 = token.balanceOf(user2); + + vm.prank(user1); + // withdraw token to user2 + vault.withdrawFunds(Token.wrap(address(token)), user2, withdrawAmount); + + uint256 balanceAfterVault = token.balanceOf(address(vault)); + uint256 balanceAfterUser2 = token.balanceOf(user2); + + uint256 balanceWithdrawn = balanceBeforeVault - balanceAfterVault; + uint256 balanceGainUser2 = balanceAfterUser2 - balanceBeforeUser2; + + assertEq(balanceWithdrawn, withdrawAmount); + assertEq(balanceGainUser2, withdrawAmount); + } + + /// @dev test addresses with the asset manager role should be able to withdraw the native token + function testAssetManagerShouldBeAbleToWithdrawNativeToken(uint256 withdrawAmount) public { + withdrawAmount = bound(withdrawAmount, 0, MAX_WITHDRAW_AMOUNT); + + uint256 balanceBeforeVault = address(vault).balance; + uint256 balanceBeforeUser2 = user2.balance; + + vm.prank(user1); + // withdraw native token to user2 + vault.withdrawFunds(NATIVE_TOKEN, user2, withdrawAmount); + + uint256 balanceAfterVault = address(vault).balance; + uint256 balanceAfterUser2 = user2.balance; + + uint256 balanceWithdrawn = balanceBeforeVault - balanceAfterVault; + uint256 balanceGainUser2 = balanceAfterUser2 - balanceBeforeUser2; + + assertEq(balanceWithdrawn, withdrawAmount); + assertEq(balanceGainUser2, withdrawAmount); + } + + /// @dev test withdrawing funds should emit event + function testWithdrawingFundsShouldEmitEvent() public { + uint256 withdrawAmount = 1000; + vm.prank(user1); + vm.expectEmit(); + emit FundsWithdrawn(Token.wrap(address(token)), user1, user2, withdrawAmount); + // withdraw token to user2 + vault.withdrawFunds(Token.wrap(address(token)), user2, withdrawAmount); + } + + /// @dev test withdrawing funds shouldn't emit event if withdrawing zero amount + function testFailWithdrawingFundsShouldnEmitEventIfWithdrawingZeroAmount() public { + uint256 withdrawAmount = 0; + vm.prank(user1); + vm.expectEmit(); + emit FundsWithdrawn(Token.wrap(address(token)), user1, user2, withdrawAmount); + // withdraw token to user2 + vault.withdrawFunds(Token.wrap(address(token)), user2, withdrawAmount); + } + + /** + * @dev test that fund withdrawal should revert if reentrancy is attempted + */ + function testShouldRevertFundWithdrawalIfReentrancyIsAttempted() public { + vm.startPrank(admin); + TestReentrancyVault testReentrancy = new TestReentrancyVault(vault, Token.wrap(address(token))); + // grant withdrawal role to testReentrancy contract + vault.grantRole(ROLE_ASSET_MANAGER, address(testReentrancy)); + + uint256 withdrawAmount = 1000; + // reverts in "unsafeTransfer" in withdrawFunds - which uses OZ's Address "sendValue" function + vm.expectRevert("Address: unable to send value, recipient may have reverted"); + testReentrancy.tryReenterWithdrawFunds(NATIVE_TOKEN, payable(address(testReentrancy)), withdrawAmount); + vm.stopPrank(); + } +} diff --git a/utils/Deploy.ts b/utils/Deploy.ts index c0a097b6..fc617619 100644 --- a/utils/Deploy.ts +++ b/utils/Deploy.ts @@ -1,5 +1,13 @@ import { ArtifactData } from '../components/ContractBuilder'; -import { CarbonController, CarbonPOL, CarbonVortex, IVersioned, ProxyAdmin, Voucher } from '../components/Contracts'; +import { + CarbonController, + CarbonPOL, + CarbonVortex, + IVersioned, + ProxyAdmin, + Vault, + Voucher +} from '../components/Contracts'; import Logger from '../utils/Logger'; import { DeploymentNetwork, ZERO_BYTES } from './Constants'; import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'; @@ -50,7 +58,8 @@ enum NewInstanceName { ProxyAdmin = 'ProxyAdmin', Voucher = 'Voucher', CarbonVortex = 'CarbonVortex', - CarbonPOL = 'CarbonPOL' + CarbonPOL = 'CarbonPOL', + Vault = 'Vault' } export const LegacyInstanceName = {}; @@ -71,7 +80,8 @@ const DeployedNewContracts = { ProxyAdmin: deployed(InstanceName.ProxyAdmin), Voucher: deployed(InstanceName.Voucher), CarbonVortex: deployed(InstanceName.CarbonVortex), - CarbonPOL: deployed(InstanceName.CarbonPOL) + CarbonPOL: deployed(InstanceName.CarbonPOL), + Vault: deployed(InstanceName.Vault) }; export const DeployedContracts = {