From 04c43b8a055b72b7129e49d57bf3d1d4fc502390 Mon Sep 17 00:00:00 2001 From: Erwan Beauvois Date: Mon, 4 Jul 2022 13:27:43 +0200 Subject: [PATCH 01/12] Update PCVGuardian to v3 and add forge tests --- contracts/libs/CoreRefPauseableLib.sol | 4 + contracts/mock/MockPCVDepositV2.sol | 14 +- contracts/pcv/IPCVGuardian.sol | 50 ++- contracts/pcv/PCVGuardian.sol | 171 ++++++--- contracts/test/unit/PCVGuardian.sol | 491 +++++++++++++++++++++++++ contracts/test/utils/Fixtures.sol | 12 +- test/unit/pcv/PCVGuardian.test.ts | 269 -------------- 7 files changed, 684 insertions(+), 327 deletions(-) create mode 100644 contracts/test/unit/PCVGuardian.sol delete mode 100644 test/unit/pcv/PCVGuardian.test.ts diff --git a/contracts/libs/CoreRefPauseableLib.sol b/contracts/libs/CoreRefPauseableLib.sol index ef207500c..2c09ce4cb 100644 --- a/contracts/libs/CoreRefPauseableLib.sol +++ b/contracts/libs/CoreRefPauseableLib.sol @@ -34,6 +34,10 @@ library CoreRefPauseableLib { } } + function _paused(address _pauseableCoreRefAddress) internal view returns (bool) { + return CoreRef(_pauseableCoreRefAddress).paused(); + } + function _pause(address _pauseableCoreRefAddress) internal { CoreRef(_pauseableCoreRefAddress).pause(); } diff --git a/contracts/mock/MockPCVDepositV2.sol b/contracts/mock/MockPCVDepositV2.sol index ed8855300..80cb090c7 100644 --- a/contracts/mock/MockPCVDepositV2.sol +++ b/contracts/mock/MockPCVDepositV2.sol @@ -33,12 +33,22 @@ contract MockPCVDepositV2 is IPCVDeposit, CoreRef { return (resistantBalance, resistantProtocolOwnedFei); } + // gets the resistant token balance of this deposit + function _resistantBalance() external view returns (uint256) { + return resistantBalance; + } + + // gets the resistant protocol owned fei of this deposit + function _resistantFei() external view returns (uint256) { + return resistantProtocolOwnedFei; + } + // IPCVDeposit V1 - function deposit() external override { + function deposit() external override whenNotPaused { resistantBalance = IERC20(balanceReportedIn).balanceOf(address(this)); } - function withdraw(address to, uint256 amount) external override { + function withdraw(address to, uint256 amount) external override whenNotPaused { IERC20(balanceReportedIn).transfer(to, amount); resistantBalance = IERC20(balanceReportedIn).balanceOf(address(this)); } diff --git a/contracts/pcv/IPCVGuardian.sol b/contracts/pcv/IPCVGuardian.sol index 959e6981a..a3fbc8bef 100644 --- a/contracts/pcv/IPCVGuardian.sol +++ b/contracts/pcv/IPCVGuardian.sol @@ -50,7 +50,7 @@ interface IPCVGuardian { /// @param safeAddresses the addresses to un-set as safe function unsetSafeAddresses(address[] calldata safeAddresses) external; - /// @notice governor-or-guardian-only method to withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param amount the amount to withdraw @@ -64,7 +64,21 @@ interface IPCVGuardian { bool depositAfter ) external; - /// @notice governor-or-guardian-only method to withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @param pcvDeposit the address of the pcv deposit contract + /// @param safeAddress the destination address to withdraw to + /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw + /// @param pauseAfter if true, the pcv contract will be paused after the withdraw + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawRatioToSafeAddress( + address pcvDeposit, + address safeAddress, + uint256 basisPoints, + bool pauseAfter, + bool depositAfter + ) external; + + /// @notice withdraw funds from a pcv deposit, by calling the withdrawETH() method on it /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param amount the amount of tokens to withdraw @@ -78,7 +92,21 @@ interface IPCVGuardian { bool depositAfter ) external; - /// @notice governor-or-guardian-only method to withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdrawETH() method on it + /// @param pcvDeposit the address of the pcv deposit contract + /// @param safeAddress the destination address to withdraw to + /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw + /// @param pauseAfter if true, the pcv contract will be paused after the withdraw + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawETHRatioToSafeAddress( + address pcvDeposit, + address payable safeAddress, + uint256 basisPoints, + bool pauseAfter, + bool depositAfter + ) external; + + /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it /// @param pcvDeposit the deposit to pull funds from /// @param safeAddress the destination address to withdraw to /// @param token the token to withdraw @@ -93,4 +121,20 @@ interface IPCVGuardian { bool pauseAfter, bool depositAfter ) external; + + /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it + /// @param pcvDeposit the deposit to pull funds from + /// @param safeAddress the destination address to withdraw to + /// @param token the token to withdraw + /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw + /// @param pauseAfter whether to pause the pcv after withdrawing + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawERC20RatioToSafeAddress( + address pcvDeposit, + address safeAddress, + address token, + uint256 basisPoints, + bool pauseAfter, + bool depositAfter + ) external; } diff --git a/contracts/pcv/PCVGuardian.sol b/contracts/pcv/PCVGuardian.sol index 497098656..ae46978c9 100644 --- a/contracts/pcv/PCVGuardian.sol +++ b/contracts/pcv/PCVGuardian.sol @@ -1,12 +1,15 @@ // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity ^0.8.4; -import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; -import "../refs/CoreRef.sol"; -import "./IPCVGuardian.sol"; -import "./IPCVDeposit.sol"; -import "../libs/CoreRefPauseableLib.sol"; +import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; +import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import {ICore} from "../core/ICore.sol"; +import {CoreRef} from "../refs/CoreRef.sol"; +import {IPCVGuardian} from "./IPCVGuardian.sol"; +import {IPCVDeposit} from "./IPCVDeposit.sol"; +import {CoreRefPauseableLib} from "../libs/CoreRefPauseableLib.sol"; import {TribeRoles} from "../core/TribeRoles.sol"; +import {Constants} from "../Constants.sol"; contract PCVGuardian is IPCVGuardian, CoreRef { using CoreRefPauseableLib for address; @@ -41,7 +44,7 @@ contract PCVGuardian is IPCVGuardian, CoreRef { function setSafeAddress(address pcvDeposit) external override - hasAnyOfTwoRoles(TribeRoles.GOVERNOR, TribeRoles.PCV_GUARDIAN_ADMIN) + hasAnyOfTwoRoles(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GOVERNOR) { _setSafeAddress(pcvDeposit); } @@ -51,9 +54,9 @@ contract PCVGuardian is IPCVGuardian, CoreRef { function setSafeAddresses(address[] calldata _safeAddresses) external override - hasAnyOfTwoRoles(TribeRoles.GOVERNOR, TribeRoles.PCV_GUARDIAN_ADMIN) + hasAnyOfTwoRoles(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GOVERNOR) { - require(_safeAddresses.length != 0, "empty"); + require(_safeAddresses.length != 0, "PCVGuardian: empty"); for (uint256 i = 0; i < _safeAddresses.length; i++) { _setSafeAddress(_safeAddresses[i]); } @@ -66,7 +69,7 @@ contract PCVGuardian is IPCVGuardian, CoreRef { function unsetSafeAddress(address pcvDeposit) external override - hasAnyOfThreeRoles(TribeRoles.GOVERNOR, TribeRoles.GUARDIAN, TribeRoles.PCV_GUARDIAN_ADMIN) + hasAnyOfThreeRoles(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GUARDIAN, TribeRoles.GOVERNOR) { _unsetSafeAddress(pcvDeposit); } @@ -76,45 +79,92 @@ contract PCVGuardian is IPCVGuardian, CoreRef { function unsetSafeAddresses(address[] calldata _safeAddresses) external override - hasAnyOfThreeRoles(TribeRoles.GOVERNOR, TribeRoles.GUARDIAN, TribeRoles.PCV_GUARDIAN_ADMIN) + hasAnyOfThreeRoles(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GUARDIAN, TribeRoles.GOVERNOR) { - require(_safeAddresses.length != 0, "empty"); + require(_safeAddresses.length != 0, "PCVGuardian: empty"); for (uint256 i = 0; i < _safeAddresses.length; i++) { _unsetSafeAddress(_safeAddresses[i]); } } - /// @notice governor-or-guardian-only method to withdraw funds from a pcv deposit, by calling the withdraw() method on it - /// @param pcvDeposit the address of the pcv deposit contract - /// @param safeAddress the destination address to withdraw to - /// @param amount the amount to withdraw - /// @param pauseAfter if true, the pcv contract will be paused after the withdraw - /// @param depositAfter if true, attempts to deposit to the target PCV deposit - function withdrawToSafeAddress( + /// @notice modifier to factorize the logic in all withdrawals : + /// - first, ensure the deposit to withdraw from is unpaused + /// - second, perform withdrawal + /// - third, re-pause deposit if it was paused or if pauseAfter = true + /// - finally, call pcvDeposit.deposit() if depositAfter = true + modifier beforeAndAfterWithdrawal( address pcvDeposit, address safeAddress, - uint256 amount, bool pauseAfter, bool depositAfter - ) external override hasAnyOfThreeRoles(TribeRoles.GOVERNOR, TribeRoles.PCV_SAFE_MOVER_ROLE, TribeRoles.GUARDIAN) { - require(isSafeAddress(safeAddress), "Provided address is not a safe address!"); + ) { + { + // scoped in this modifier to prevent stack to deep errors & enforce consitent acl + ICore _core = core(); + require( + _core.hasRole(TribeRoles.GUARDIAN, msg.sender) || + _core.hasRole(TribeRoles.PCV_SAFE_MOVER_ROLE, msg.sender) || + _core.hasRole(TribeRoles.GOVERNOR, msg.sender), + "UNAUTHORIZED" + ); + require(isSafeAddress(safeAddress), "PCVGuardian: address not whitelisted"); + } - pcvDeposit._ensureUnpaused(); + bool paused = pcvDeposit._paused(); + if (paused) { + pcvDeposit._unpause(); + } - IPCVDeposit(pcvDeposit).withdraw(safeAddress, amount); + _; - if (pauseAfter) { + if (paused || pauseAfter) { pcvDeposit._pause(); } if (depositAfter) { IPCVDeposit(safeAddress).deposit(); } + } + + /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @param pcvDeposit the address of the pcv deposit contract + /// @param safeAddress the destination address to withdraw to + /// @param amount the amount to withdraw + /// @param pauseAfter if true, the pcv contract will be paused after the withdraw + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawToSafeAddress( + address pcvDeposit, + address safeAddress, + uint256 amount, + bool pauseAfter, + bool depositAfter + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + IPCVDeposit(pcvDeposit).withdraw(safeAddress, amount); + emit PCVGuardianWithdrawal(pcvDeposit, safeAddress, amount); + } + /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @param pcvDeposit the address of the pcv deposit contract + /// @param safeAddress the destination address to withdraw to + /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw + /// @param pauseAfter if true, the pcv contract will be paused after the withdraw + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawRatioToSafeAddress( + address pcvDeposit, + address safeAddress, + uint256 basisPoints, + bool pauseAfter, + bool depositAfter + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + require(basisPoints <= Constants.BASIS_POINTS_GRANULARITY, "PCVGuardian: basisPoints too high"); + uint256 amount = (IPCVDeposit(pcvDeposit).balance() * basisPoints) / Constants.BASIS_POINTS_GRANULARITY; + require(amount != 0, "PCVGuardian: no value to withdraw"); + + IPCVDeposit(pcvDeposit).withdraw(safeAddress, amount); emit PCVGuardianWithdrawal(pcvDeposit, safeAddress, amount); } - /// @notice governor-or-guardian-only method to withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdrawETH() method on it /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param amount the amount of tokens to withdraw @@ -126,25 +176,33 @@ contract PCVGuardian is IPCVGuardian, CoreRef { uint256 amount, bool pauseAfter, bool depositAfter - ) external override hasAnyOfThreeRoles(TribeRoles.GOVERNOR, TribeRoles.PCV_SAFE_MOVER_ROLE, TribeRoles.GUARDIAN) { - require(isSafeAddress(safeAddress), "Provided address is not a safe address!"); - - pcvDeposit._ensureUnpaused(); - + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { IPCVDeposit(pcvDeposit).withdrawETH(safeAddress, amount); + emit PCVGuardianETHWithdrawal(pcvDeposit, safeAddress, amount); + } - if (pauseAfter) { - pcvDeposit._pause(); - } - - if (depositAfter) { - IPCVDeposit(safeAddress).deposit(); - } + /// @notice withdraw funds from a pcv deposit, by calling the withdrawETH() method on it + /// @param pcvDeposit the address of the pcv deposit contract + /// @param safeAddress the destination address to withdraw to + /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw + /// @param pauseAfter if true, the pcv contract will be paused after the withdraw + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawETHRatioToSafeAddress( + address pcvDeposit, + address payable safeAddress, + uint256 basisPoints, + bool pauseAfter, + bool depositAfter + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + require(basisPoints <= Constants.BASIS_POINTS_GRANULARITY, "PCVGuardian: basisPoints too high"); + uint256 amount = (pcvDeposit.balance * basisPoints) / Constants.BASIS_POINTS_GRANULARITY; + require(amount != 0, "PCVGuardian: no value to withdraw"); + IPCVDeposit(pcvDeposit).withdrawETH(safeAddress, amount); emit PCVGuardianETHWithdrawal(pcvDeposit, safeAddress, amount); } - /// @notice governor-or-guardian-only method to withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it /// @param pcvDeposit the deposit to pull funds from /// @param safeAddress the destination address to withdraw to /// @param amount the amount of funds to withdraw @@ -157,33 +215,42 @@ contract PCVGuardian is IPCVGuardian, CoreRef { uint256 amount, bool pauseAfter, bool depositAfter - ) external override hasAnyOfThreeRoles(TribeRoles.GOVERNOR, TribeRoles.PCV_SAFE_MOVER_ROLE, TribeRoles.GUARDIAN) { - require(isSafeAddress(safeAddress), "Provided address is not a safe address!"); - - pcvDeposit._ensureUnpaused(); - + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { IPCVDeposit(pcvDeposit).withdrawERC20(token, safeAddress, amount); + emit PCVGuardianERC20Withdrawal(pcvDeposit, safeAddress, token, amount); + } - if (pauseAfter) { - pcvDeposit._pause(); - } - - if (depositAfter) { - IPCVDeposit(safeAddress).deposit(); - } + /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it + /// @param pcvDeposit the deposit to pull funds from + /// @param safeAddress the destination address to withdraw to + /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw + /// @param pauseAfter whether to pause the pcv after withdrawing + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawERC20RatioToSafeAddress( + address pcvDeposit, + address safeAddress, + address token, + uint256 basisPoints, + bool pauseAfter, + bool depositAfter + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + require(basisPoints <= Constants.BASIS_POINTS_GRANULARITY, "PCVGuardian: basisPoints too high"); + uint256 amount = (IERC20(token).balanceOf(pcvDeposit) * basisPoints) / Constants.BASIS_POINTS_GRANULARITY; + require(amount != 0, "PCVGuardian: no value to withdraw"); + IPCVDeposit(pcvDeposit).withdrawERC20(token, safeAddress, amount); emit PCVGuardianERC20Withdrawal(pcvDeposit, safeAddress, token, amount); } // ---------- Internal Functions ---------- function _setSafeAddress(address anAddress) internal { - require(safeAddresses.add(anAddress), "set"); + require(safeAddresses.add(anAddress), "PCVGuardian: already a safe address"); emit SafeAddressAdded(anAddress); } function _unsetSafeAddress(address anAddress) internal { - require(safeAddresses.remove(anAddress), "unset"); + require(safeAddresses.remove(anAddress), "PCVGuardian: not a safe address"); emit SafeAddressRemoved(anAddress); } } diff --git a/contracts/test/unit/PCVGuardian.sol b/contracts/test/unit/PCVGuardian.sol new file mode 100644 index 000000000..93c43546e --- /dev/null +++ b/contracts/test/unit/PCVGuardian.sol @@ -0,0 +1,491 @@ +pragma solidity ^0.8.4; + +import {Vm} from "../utils/Vm.sol"; +import {DSTest} from "../utils/DSTest.sol"; +import {getCore, getAddresses, FeiTestAddresses} from "../utils/Fixtures.sol"; + +import {ICore} from "../../core/ICore.sol"; +import {MockERC20} from "../../mock/MockERC20.sol"; +import {MockPCVDepositV2} from "../../mock/MockPCVDepositV2.sol"; +import {PCVGuardian} from "../../pcv/PCVGuardian.sol"; + +contract PCVGuardianTest is DSTest { + ICore private core; + PCVGuardian private pcvGuardian; + MockERC20 private token; + MockPCVDepositV2 private pcvDeposit1; + MockPCVDepositV2 private pcvDeposit2; + MockPCVDepositV2 private pcvDeposit3; + + Vm public constant vm = Vm(HEVM_ADDRESS); + FeiTestAddresses public addresses = getAddresses(); + + function setUp() public { + // get core + core = getCore(); + + // initialize mock tokens & deposits + token = new MockERC20(); + pcvDeposit1 = new MockPCVDepositV2(address(core), address(token), 0, 0); + pcvDeposit2 = new MockPCVDepositV2(address(core), address(token), 0, 0); + pcvDeposit3 = new MockPCVDepositV2(address(core), address(token), 0, 0); + token.mint(address(pcvDeposit1), 100e18); + pcvDeposit1.deposit(); + payable(address(pcvDeposit1)).transfer(1 ether); + + // initialize pcv guardian + address[] memory safeAddresses = new address[](2); + safeAddresses[0] = address(pcvDeposit1); + safeAddresses[1] = address(pcvDeposit2); + pcvGuardian = new PCVGuardian(address(core), safeAddresses); + // give roles to pcv guardian + vm.startPrank(addresses.governorAddress); + core.grantPCVController(address(pcvGuardian)); + core.grantGuardian(address(pcvGuardian)); + vm.stopPrank(); + + // labels + vm.label(address(core), "core"); + vm.label(address(token), "token"); + vm.label(address(pcvDeposit1), "pcvDeposit1"); + vm.label(address(pcvDeposit2), "pcvDeposit2"); + vm.label(address(pcvDeposit3), "pcvDeposit3"); + vm.label(address(pcvGuardian), "pcvGuardian"); + } + + // should have no safe addresses upon deployment when deployed with no safe addresses + function testDeploy1() public { + address[] memory safeAddresses = new address[](0); + PCVGuardian pcvGuardianWithoutAddresses = new PCVGuardian(address(core), safeAddresses); + address[] memory getSafeAddresses = pcvGuardianWithoutAddresses.getSafeAddresses(); + assertEq(getSafeAddresses.length, 0); + } + + // should have safe addresses upon deployment when deployed with safe addresses + function testDeploy2() public { + address[] memory safeAddresses = new address[](1); + safeAddresses[0] = address(0x42); + PCVGuardian pcvGuardianWithoutAddresses = new PCVGuardian(address(core), safeAddresses); + address[] memory getSafeAddresses = pcvGuardianWithoutAddresses.getSafeAddresses(); + assertEq(getSafeAddresses.length, 1); + assertEq(getSafeAddresses[0], address(0x42)); + } + + // access control and state changes + + // should revert when calling setSafeAddress & setSafeAddresses from a non-privileged address + // should revert when calling unsetSafeAddress & unsetSafeAddresses from a non-privileged address + function testAccessControl1() public { + address safeAddress = address(0x42); + address[] memory safeAddresses = new address[](1); + safeAddresses[0] = safeAddress; + + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.setSafeAddress(safeAddress); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.setSafeAddresses(safeAddresses); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.unsetSafeAddress(safeAddress); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.unsetSafeAddresses(safeAddresses); + } + + // ADD safe address(es) : + // should allow PCV_GUARDIAN_ADMIN + // should allow GOVERNOR + // REMOVE safe adress(es) : + // should allow GUARDIAN + // should allow PCV_GUARDIAN_ADMIN + // should allow GOVERNOR + function testAccessControl2() public { + address safeAddress = address(0x42); + address[] memory safeAddresses = new address[](1); + safeAddresses[0] = safeAddress; + + // none of the following calls shall revert if access + // control is properly checked for the various callers + vm.startPrank(addresses.pcvGuardianAdminAddress); + pcvGuardian.setSafeAddress(safeAddress); + pcvGuardian.unsetSafeAddress(safeAddress); + pcvGuardian.setSafeAddresses(safeAddresses); + pcvGuardian.unsetSafeAddresses(safeAddresses); + vm.stopPrank(); + + vm.startPrank(addresses.governorAddress); + pcvGuardian.setSafeAddress(safeAddress); + pcvGuardian.unsetSafeAddress(safeAddress); + pcvGuardian.setSafeAddresses(safeAddresses); + pcvGuardian.unsetSafeAddresses(safeAddresses); + vm.stopPrank(); + + vm.prank(addresses.pcvGuardianAdminAddress); + pcvGuardian.setSafeAddress(safeAddress); + vm.prank(addresses.guardianAddress); + pcvGuardian.unsetSafeAddress(safeAddress); + vm.prank(addresses.pcvGuardianAdminAddress); + pcvGuardian.setSafeAddresses(safeAddresses); + vm.prank(addresses.guardianAddress); + pcvGuardian.unsetSafeAddresses(safeAddresses); + } + + // should revert when calling withdrawToSafeAddress from a non-privileged address + // should revert when calling withdrawETHToSafeAddress from a non-privileged address + // should revert when calling withdrawERC20ToSafeAddress from a non-privileged address + // should revert when calling withdrawRatioToSafeAddress from a non-privileged address + // should revert when calling withdrawETHRatioToSafeAddress from a non-privileged address + // should revert when calling withdrawERC20RatioToSafeAddress from a non-privileged address + function testAccessControl3() public { + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, false); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1e18, false, false); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.withdrawERC20ToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1e18, + false, + false + ); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.withdrawERC20RatioToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + } + + // should allow GOVERNOR to perform withdrawals + // should allow PCV_SAFE_MOVER_ROLE to perform withdrawals + // should allow GUARDIAN to perform withdrawals + function testAccessControl4() public { + // none of the following calls shall revert if access + // control is properly checked for the various callers + vm.startPrank(addresses.governorAddress); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawERC20ToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawERC20RatioToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + vm.stopPrank(); + + vm.startPrank(addresses.pcvSafeMoverAddress); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawERC20ToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawERC20RatioToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + vm.stopPrank(); + + vm.startPrank(addresses.guardianAddress); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawERC20ToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawERC20RatioToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + vm.stopPrank(); + + // move back all tokens & eth + vm.startPrank(addresses.governorAddress); + pcvGuardian.withdrawETHRatioToSafeAddress( + address(pcvDeposit2), + payable(address(pcvDeposit1)), + 10000, + false, + false + ); + pcvGuardian.withdrawERC20RatioToSafeAddress( + address(pcvDeposit2), + address(pcvDeposit1), + address(token), + 10000, + false, + false + ); + pcvDeposit1.deposit(); + pcvDeposit2.deposit(); + vm.stopPrank(); + } + + // check state changes & getters after set & unset + function testStateConsistency1() public { + address[] memory getSafeAddresses = pcvGuardian.getSafeAddresses(); + assertEq(getSafeAddresses.length, 2); + assertEq(getSafeAddresses[0], address(pcvDeposit1)); + assertEq(getSafeAddresses[1], address(pcvDeposit2)); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); + + vm.prank(addresses.governorAddress); + pcvGuardian.setSafeAddress(address(pcvDeposit3)); + + getSafeAddresses = pcvGuardian.getSafeAddresses(); + assertEq(getSafeAddresses.length, 3); + assertEq(getSafeAddresses[0], address(pcvDeposit1)); + assertEq(getSafeAddresses[1], address(pcvDeposit2)); + assertEq(getSafeAddresses[2], address(pcvDeposit3)); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), true); + + vm.prank(addresses.governorAddress); + pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); + + getSafeAddresses = pcvGuardian.getSafeAddresses(); + assertEq(getSafeAddresses.length, 2); + assertEq(getSafeAddresses[0], address(pcvDeposit1)); + assertEq(getSafeAddresses[1], address(pcvDeposit2)); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); + } + + // can't set an already safe address + // can't unset an already unsafe address + function testStateConsistency2() public { + address safeAddress = address(0x42); + vm.startPrank(addresses.governorAddress); + + vm.expectRevert("PCVGuardian: not a safe address"); + pcvGuardian.unsetSafeAddress(safeAddress); + + pcvGuardian.setSafeAddress(safeAddress); + + vm.expectRevert("PCVGuardian: already a safe address"); + pcvGuardian.setSafeAddress(safeAddress); + + vm.stopPrank(); + } + + // withdrawals + + // should not be able to withdraw to a non-safe address + function testOnlyWithdrawableToSafeAdress() public { + vm.prank(addresses.guardianAddress); + vm.expectRevert("PCVGuardian: address not whitelisted"); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); + + vm.prank(addresses.pcvGuardianAdminAddress); + pcvGuardian.setSafeAddress(address(pcvDeposit3)); + + vm.startPrank(addresses.guardianAddress); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit3), address(pcvDeposit1), 1, false, true); + pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); + vm.expectRevert("PCVGuardian: address not whitelisted"); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); + vm.stopPrank(); + } + + // should withdraw from a pcv deposit + // should withdrawETH from a pcv deposit + // should withdrawERC20 from a pcv deposit + // should withdrawRatio from a pcv deposit + // should withdrawETHRatio from a pcv deposit + // should withdrawERC20Ratio from a pcv deposit + function testWithdrawals() public { + // initial state + vm.startPrank(addresses.pcvSafeMoverAddress); + assertEq(pcvDeposit1.balance(), 100e18); + assertEq(pcvDeposit2.balance(), 0); + assertEq(address(pcvDeposit1).balance, 1 ether); + assertEq(address(pcvDeposit2).balance, 0); + + // withdrawals + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); + assertEq(pcvDeposit1.balance(), 99e18); + assertEq(pcvDeposit2.balance(), 1e18); + + pcvGuardian.withdrawETHToSafeAddress( + address(pcvDeposit1), + payable(address(pcvDeposit2)), + 0.1 ether, + false, + false + ); + assertEq(address(pcvDeposit1).balance, 0.9 ether); + assertEq(address(pcvDeposit2).balance, 0.1 ether); + + pcvGuardian.withdrawERC20ToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1e18, + false, + true + ); + assertEq(pcvDeposit1.balance(), 98e18); + assertEq(pcvDeposit2.balance(), 2e18); + + // ratio withdrawals + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 5000, false, true); + assertEq(pcvDeposit1.balance(), 49e18); + assertEq(pcvDeposit2.balance(), 51e18); + + pcvGuardian.withdrawETHRatioToSafeAddress( + address(pcvDeposit1), + payable(address(pcvDeposit2)), + 5000, + false, + false + ); + assertEq(address(pcvDeposit1).balance, 0.45 ether); + assertEq(address(pcvDeposit2).balance, 0.55 ether); + + pcvGuardian.withdrawERC20RatioToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 5000, + false, + true + ); + assertEq(pcvDeposit1.balance(), 24.5e18); + assertEq(pcvDeposit2.balance(), 75.5e18); + + // transfer back all tokens & eth + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 10000, false, true); + pcvGuardian.withdrawETHRatioToSafeAddress( + address(pcvDeposit2), + payable(address(pcvDeposit1)), + 10000, + false, + false + ); + vm.stopPrank(); + + // end state = initial state + assertEq(pcvDeposit1.balance(), 100e18); + assertEq(pcvDeposit2.balance(), 0); + assertEq(address(pcvDeposit1).balance, 1 ether); + assertEq(address(pcvDeposit2).balance, 0); + } + + // should withdraw and deposit after + // should withdraw and not deposit after + function testDepositAfter() public { + vm.startPrank(addresses.guardianAddress); + + assertEq(pcvDeposit1._resistantBalance(), 100e18); + assertEq(pcvDeposit2._resistantBalance(), 0); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, false); + assertEq(pcvDeposit1._resistantBalance(), 99e18); + assertEq(pcvDeposit2._resistantBalance(), 0); + pcvDeposit2.deposit(); + assertEq(pcvDeposit2._resistantBalance(), 1e18); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 1e18, false, true); + assertEq(pcvDeposit1._resistantBalance(), 100e18); + assertEq(pcvDeposit2._resistantBalance(), 0); + + vm.stopPrank(); + } + + // should withdraw and pause after + // should withdraw and not pause after + function testPauseAfter() public { + vm.startPrank(addresses.guardianAddress); + + assertEq(pcvDeposit1.paused(), false); + assertEq(pcvDeposit2.paused(), false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); + assertEq(pcvDeposit1.paused(), false); + assertEq(pcvDeposit2.paused(), false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, true, true); + assertEq(pcvDeposit1.paused(), true); + assertEq(pcvDeposit2.paused(), false); + + pcvDeposit1.unpause(); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 2e18, false, true); + + vm.stopPrank(); + } + + // should withdraw and unpause before + // should withdraw, and pause after + // should keep paused state if deposit was paused before withdraw + function testUnpauseBefore() public { + vm.startPrank(addresses.guardianAddress); + + pcvDeposit1.pause(); + assertEq(pcvDeposit1.paused(), true); + assertEq(pcvDeposit2.paused(), false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); + assertEq(pcvDeposit1.paused(), true); + assertEq(pcvDeposit2.paused(), false); + pcvDeposit1.unpause(); + assertEq(pcvDeposit1.paused(), false); + assertEq(pcvDeposit2.paused(), false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, true, true); + assertEq(pcvDeposit1.paused(), true); + assertEq(pcvDeposit2.paused(), false); + + pcvDeposit1.unpause(); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 2e18, false, true); + + vm.stopPrank(); + } + + // should revert if trying to depositAfter on a paused safe address + function testRevertPausedAfter() public { + vm.startPrank(addresses.guardianAddress); + pcvDeposit2.pause(); + vm.expectRevert("Pausable: paused"); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); + pcvDeposit2.unpause(); + vm.stopPrank(); + } +} diff --git a/contracts/test/utils/Fixtures.sol b/contracts/test/utils/Fixtures.sol index 8c76563ca..b44d60501 100644 --- a/contracts/test/utils/Fixtures.sol +++ b/contracts/test/utils/Fixtures.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.4; import {Core} from "../../core/Core.sol"; +import {TribeRoles} from "../../core/TribeRoles.sol"; import {Vm} from "./Vm.sol"; struct FeiTestAddresses { @@ -16,6 +17,8 @@ struct FeiTestAddresses { address minterAddress; address burnerAddress; address guardianAddress; + address pcvGuardianAdminAddress; + address pcvSafeMoverAddress; } /// @dev Get a list of addresses @@ -31,7 +34,9 @@ function getAddresses() pure returns (FeiTestAddresses memory) { pcvControllerAddress: address(0x8), minterAddress: address(0x9), burnerAddress: address(0x10), - guardianAddress: address(0x11) + guardianAddress: address(0x11), + pcvGuardianAdminAddress: address(0x12), + pcvSafeMoverAddress: address(0x13) }); return addresses; @@ -53,6 +58,11 @@ function getCore() returns (Core) { core.grantPCVController(addresses.pcvControllerAddress); core.grantGuardian(addresses.guardianAddress); + core.createRole(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GOVERNOR); + core.grantRole(TribeRoles.PCV_GUARDIAN_ADMIN, addresses.pcvGuardianAdminAddress); + core.createRole(TribeRoles.PCV_SAFE_MOVER_ROLE, TribeRoles.GOVERNOR); + core.grantRole(TribeRoles.PCV_SAFE_MOVER_ROLE, addresses.pcvSafeMoverAddress); + vm.stopPrank(); return core; } diff --git a/test/unit/pcv/PCVGuardian.test.ts b/test/unit/pcv/PCVGuardian.test.ts deleted file mode 100644 index e626ddf71..000000000 --- a/test/unit/pcv/PCVGuardian.test.ts +++ /dev/null @@ -1,269 +0,0 @@ -import { - Core, - MockERC20, - MockERC20__factory, - MockPCVDepositV2__factory, - PCVDeposit, - PCVGuardian -} from '@custom-types/contracts'; -import { expectRevert, getAddresses, getCore, getImpersonatedSigner } from '@test/helpers'; -import { forceEth } from '@test/integration/setup/utils'; -import chai, { expect } from 'chai'; -import { Signer } from 'ethers'; -import { ethers } from 'hardhat'; - -// This will theoretically make the error stack actually print! -chai.config.includeStack = true; - -// Import if needed, just a helper. -// const toBN = ethers.BigNumber.from; - -describe('PCV Guardian', function () { - // variable decs for vars that you want to use in multiple tests - // typeing contracts specifically to what kind they are will catch before you run them! - let core: Core; - let pcvGuardianWithoutStartingAddresses: PCVGuardian; - let pcvGuardianWithStartingAddresses: PCVGuardian; - - let userAddress: string; - let userAddress2: string; - let pcvControllerAddress: string; - let governorAddress: string; - let guardianAddress: string; - - const impersonatedSigners: { [key: string]: Signer } = {}; - - before(async () => { - // add any addresses that you want to get here - const addresses = await getAddresses(); - - userAddress = addresses.userAddress; - userAddress2 = addresses.secondUserAddress; - pcvControllerAddress = addresses.pcvControllerAddress; - governorAddress = addresses.governorAddress; - guardianAddress = addresses.guardianAddress; - - // add any addresses you want to impersonate here - const impersonatedAddresses = [userAddress, pcvControllerAddress, governorAddress, guardianAddress]; - - for (const address of impersonatedAddresses) { - impersonatedSigners[address] = await getImpersonatedSigner(address); - } - }); - - beforeEach(async () => { - // If the forked-network state needs to be reset between each test, run this - // await network.provider.request({method: 'hardhat_reset', params: []}); - - // Do any pre-test setup here - core = await getCore(); - - const pcvGuardianFactory = await ethers.getContractFactory('PCVGuardian'); - - pcvGuardianWithoutStartingAddresses = await pcvGuardianFactory.deploy(core.address, []); - pcvGuardianWithStartingAddresses = await pcvGuardianFactory.deploy(core.address, [userAddress, userAddress2]); - - await pcvGuardianWithoutStartingAddresses.deployTransaction.wait(); - await pcvGuardianWithStartingAddresses.deployTransaction.wait(); - - // To deploy a contract, import and use the contract factory specific to that contract - // note that the signer supplied is optional - }); - - // Try and do as much deployment in beforeEach, and as much testing in the actual functions - - describe('initial conditions', async () => { - it('should have no safe addresses upon deployment when deployed with no safe addresses', async () => { - expect(await pcvGuardianWithoutStartingAddresses.getSafeAddresses()).to.be.empty; - }); - - it('should have safe addresses upon deployment when deployed with safe addresses', async () => { - expect(await pcvGuardianWithStartingAddresses.getSafeAddresses()).not.to.be.empty; - }); - }); - - describe('access control', async () => { - it('should revert when calling setSafeAddress & setSafeAddresses from a non-governor address', async () => { - await expect(pcvGuardianWithoutStartingAddresses.setSafeAddress(userAddress)).to.be.revertedWith('UNAUTHORIZED'); - await expect( - pcvGuardianWithoutStartingAddresses.setSafeAddresses([userAddress, userAddress2]) - ).to.be.revertedWith('UNAUTHORIZED'); - }); - - it('should revert when calling unsetSafeAddress & unsetSafeAddresses from a non-guardian-or-governor-or-admin address', async () => { - await expect(pcvGuardianWithoutStartingAddresses.unsetSafeAddress(userAddress)).to.be.revertedWith( - 'UNAUTHORIZED' - ); - - await expect( - pcvGuardianWithoutStartingAddresses.unsetSafeAddresses([userAddress, userAddress2]) - ).to.be.revertedWith('UNAUTHORIZED'); - }); - - it('should revert when calling withdrawToSafeAddress from a non-guardian-or-governor-or-admin address', async () => { - await expect( - pcvGuardianWithoutStartingAddresses.withdrawToSafeAddress(userAddress, userAddress, 1, false, false) - ).to.be.revertedWith('UNAUTHORIZED'); - }); - - it('should revert when calling withdrawETHToSafeAddress from a non-guardian-or-governor-or-admin address', async () => { - await expect( - pcvGuardianWithoutStartingAddresses.withdrawETHToSafeAddress(userAddress, userAddress, 1, false, false) - ).to.be.revertedWith('UNAUTHORIZED'); - }); - - it('should revert when calling withdrawERC20ToSafeAddress from a non-guardian-or-governor-or-admin address', async () => { - await expect( - pcvGuardianWithoutStartingAddresses.withdrawERC20ToSafeAddress( - userAddress, - userAddress, - userAddress, - 1, - false, - false - ) - ).to.be.revertedWith('UNAUTHORIZED'); - }); - - it('should allow the governor to add a safe address', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[governorAddress]) - .setSafeAddress(userAddress); - expect(await pcvGuardianWithoutStartingAddresses.isSafeAddress(userAddress)).to.be.true; - }); - - it("can't set an already safe address", async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[governorAddress]) - .setSafeAddress(userAddress); - expect(await pcvGuardianWithoutStartingAddresses.isSafeAddress(userAddress)).to.be.true; - - await expectRevert( - pcvGuardianWithoutStartingAddresses.connect(impersonatedSigners[governorAddress]).setSafeAddress(userAddress), - 'set' - ); - }); - - it("can't unset an already unsafe address", async () => { - await expectRevert( - pcvGuardianWithoutStartingAddresses.connect(impersonatedSigners[governorAddress]).unsetSafeAddress(userAddress), - 'unset' - ); - }); - - it('should allow the guardian to remove a safe address', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[governorAddress]) - .setSafeAddress(userAddress); - expect(await pcvGuardianWithoutStartingAddresses.isSafeAddress(userAddress)).to.be.true; - - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .unsetSafeAddress(userAddress); - expect(await pcvGuardianWithoutStartingAddresses.isSafeAddress(userAddress)).to.be.false; - }); - }); - - describe('withdrawals', async () => { - let token: MockERC20; - let tokenPCVDeposit: PCVDeposit; - let tokenPCVDeposit2: PCVDeposit; - - beforeEach(async () => { - const tokenFactory = new MockERC20__factory(impersonatedSigners[userAddress]); - const pcvDepositFactory = new MockPCVDepositV2__factory(impersonatedSigners[userAddress]); - - token = await tokenFactory.deploy(); - tokenPCVDeposit = await pcvDepositFactory.deploy(core.address, token.address, 1, 0); - tokenPCVDeposit2 = await pcvDepositFactory.deploy(core.address, token.address, 1, 0); - - await token.mint(tokenPCVDeposit.address, 100); - await forceEth(tokenPCVDeposit.address); - - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[governorAddress]) - .setSafeAddress(userAddress); - await core - .connect(impersonatedSigners[governorAddress]) - .grantPCVController(pcvGuardianWithoutStartingAddresses.address); - await core - .connect(impersonatedSigners[governorAddress]) - .grantGuardian(pcvGuardianWithoutStartingAddresses.address); - }); - - it('should not be able to withdraw to a non-safe address', async () => { - await expect( - pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawToSafeAddress(token.address, token.address, 1, false, false) - ).to.be.revertedWith('Provided address is not a safe address!'); - }); - - it('should withdraw from a token-pcv deposit when called by the guardian', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawToSafeAddress(tokenPCVDeposit.address, userAddress, 1, false, false); - expect(await token.balanceOf(userAddress)).to.eq(1); - }); - - it('should withdraw from a token-pcv deposit when called by the governor', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[governorAddress]) - .withdrawToSafeAddress(tokenPCVDeposit.address, userAddress, 1, false, false); - expect(await token.balanceOf(userAddress)).to.eq(1); - }); - - it('should withdraw from a token-pcv deposit and deposit after', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[governorAddress]) - .setSafeAddress(tokenPCVDeposit2.address); - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawToSafeAddress(tokenPCVDeposit.address, tokenPCVDeposit2.address, 1, false, true); - expect(await token.balanceOf(tokenPCVDeposit2.address)).to.eq(1); - }); - - it('should withdrawETH from a pcv deposit when called by the guardian', async () => { - const balanceBefore = await ethers.provider.getBalance(userAddress); - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawETHToSafeAddress(tokenPCVDeposit.address, userAddress, 1, false, false); - const balanceAfter = await ethers.provider.getBalance(userAddress); - - expect(balanceAfter.sub(balanceBefore)).to.eq(1); - }); - - it('should withdrawERC20 from a pcv deposit when called by the guardian', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawERC20ToSafeAddress(tokenPCVDeposit.address, userAddress, token.address, 1, false, false); - expect(await token.balanceOf(userAddress)).to.eq(1); - }); - - it('should withdraw and unpause beforehand', async () => { - await tokenPCVDeposit.connect(impersonatedSigners[guardianAddress]).pause(); - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawToSafeAddress(tokenPCVDeposit.address, userAddress, 1, false, false); - expect(await token.balanceOf(userAddress)).to.eq(1); - expect(await tokenPCVDeposit.paused()).to.be.false; - }); - - it('should withdraw and pause after', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawToSafeAddress(tokenPCVDeposit.address, userAddress, 1, true, false); - expect(await token.balanceOf(userAddress)).to.eq(1); - expect(await tokenPCVDeposit.paused()).to.be.true; - }); - - it('should withdraw, unpause before, and pause after', async () => { - await tokenPCVDeposit.connect(impersonatedSigners[guardianAddress]).pause(); - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawToSafeAddress(tokenPCVDeposit.address, userAddress, 1, true, false); - expect(await token.balanceOf(userAddress)).to.eq(1); - expect(await tokenPCVDeposit.paused()).to.be.true; - }); - }); -}); From 5d37dbd9678070632da7ad9c986ae15cd46b4b82 Mon Sep 17 00:00:00 2001 From: Erwan Beauvois Date: Tue, 5 Jul 2022 17:26:13 +0200 Subject: [PATCH 02/12] tom's review --- contracts/pcv/IPCVGuardian.sol | 9 +- contracts/pcv/PCVGuardian.sol | 2 +- contracts/test/unit/PCVGuardian.sol | 159 +++++++++++++++------------- contracts/test/utils/Fixtures.sol | 12 +-- 4 files changed, 98 insertions(+), 84 deletions(-) diff --git a/contracts/pcv/IPCVGuardian.sol b/contracts/pcv/IPCVGuardian.sol index a3fbc8bef..3e1c838f9 100644 --- a/contracts/pcv/IPCVGuardian.sol +++ b/contracts/pcv/IPCVGuardian.sol @@ -64,7 +64,8 @@ interface IPCVGuardian { bool depositAfter ) external; - /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it. + /// The amount withdrawn is expressed as a ratio, basis points are given & not absolute value. /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw @@ -92,7 +93,8 @@ interface IPCVGuardian { bool depositAfter ) external; - /// @notice withdraw funds from a pcv deposit, by calling the withdrawETH() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdrawETH() method on it. + /// The amount withdrawn is expressed as a ratio, basis points are given & not absolute value. /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw @@ -122,7 +124,8 @@ interface IPCVGuardian { bool depositAfter ) external; - /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it. + /// The amount withdrawn is expressed as a ratio, basis points are given & not absolute value. /// @param pcvDeposit the deposit to pull funds from /// @param safeAddress the destination address to withdraw to /// @param token the token to withdraw diff --git a/contracts/pcv/PCVGuardian.sol b/contracts/pcv/PCVGuardian.sol index ae46978c9..051bd6b21 100644 --- a/contracts/pcv/PCVGuardian.sol +++ b/contracts/pcv/PCVGuardian.sol @@ -99,7 +99,7 @@ contract PCVGuardian is IPCVGuardian, CoreRef { bool depositAfter ) { { - // scoped in this modifier to prevent stack to deep errors & enforce consitent acl + // scoped in this modifier to prevent stack to deep errors & enforce consistent acl ICore _core = core(); require( _core.hasRole(TribeRoles.GUARDIAN, msg.sender) || diff --git a/contracts/test/unit/PCVGuardian.sol b/contracts/test/unit/PCVGuardian.sol index 93c43546e..602fa0e8d 100644 --- a/contracts/test/unit/PCVGuardian.sol +++ b/contracts/test/unit/PCVGuardian.sol @@ -54,7 +54,7 @@ contract PCVGuardianTest is DSTest { } // should have no safe addresses upon deployment when deployed with no safe addresses - function testDeploy1() public { + function testDeployWithoutSafeAddresses() public { address[] memory safeAddresses = new address[](0); PCVGuardian pcvGuardianWithoutAddresses = new PCVGuardian(address(core), safeAddresses); address[] memory getSafeAddresses = pcvGuardianWithoutAddresses.getSafeAddresses(); @@ -62,7 +62,7 @@ contract PCVGuardianTest is DSTest { } // should have safe addresses upon deployment when deployed with safe addresses - function testDeploy2() public { + function testDeployWithSafeAddresses() public { address[] memory safeAddresses = new address[](1); safeAddresses[0] = address(0x42); PCVGuardian pcvGuardianWithoutAddresses = new PCVGuardian(address(core), safeAddresses); @@ -71,11 +71,11 @@ contract PCVGuardianTest is DSTest { assertEq(getSafeAddresses[0], address(0x42)); } - // access control and state changes + ///////////// ACCESS CONTROL AND STATE CHANGES ////////// // should revert when calling setSafeAddress & setSafeAddresses from a non-privileged address // should revert when calling unsetSafeAddress & unsetSafeAddresses from a non-privileged address - function testAccessControl1() public { + function testAccessControlSetUnsetSafeAddressesUnprivilegedCaller() public { address safeAddress = address(0x42); address[] memory safeAddresses = new address[](1); safeAddresses[0] = safeAddress; @@ -97,14 +97,14 @@ contract PCVGuardianTest is DSTest { // should allow GUARDIAN // should allow PCV_GUARDIAN_ADMIN // should allow GOVERNOR - function testAccessControl2() public { + function testAccessControlSetUnsetSafeAddressesPrivilegedCallers() public { address safeAddress = address(0x42); address[] memory safeAddresses = new address[](1); safeAddresses[0] = safeAddress; // none of the following calls shall revert if access // control is properly checked for the various callers - vm.startPrank(addresses.pcvGuardianAdminAddress); + vm.startPrank(addresses.pcvGuardianAdmin); pcvGuardian.setSafeAddress(safeAddress); pcvGuardian.unsetSafeAddress(safeAddress); pcvGuardian.setSafeAddresses(safeAddresses); @@ -118,11 +118,11 @@ contract PCVGuardianTest is DSTest { pcvGuardian.unsetSafeAddresses(safeAddresses); vm.stopPrank(); - vm.prank(addresses.pcvGuardianAdminAddress); + vm.prank(addresses.pcvGuardianAdmin); pcvGuardian.setSafeAddress(safeAddress); vm.prank(addresses.guardianAddress); pcvGuardian.unsetSafeAddress(safeAddress); - vm.prank(addresses.pcvGuardianAdminAddress); + vm.prank(addresses.pcvGuardianAdmin); pcvGuardian.setSafeAddresses(safeAddresses); vm.prank(addresses.guardianAddress); pcvGuardian.unsetSafeAddresses(safeAddresses); @@ -134,7 +134,7 @@ contract PCVGuardianTest is DSTest { // should revert when calling withdrawRatioToSafeAddress from a non-privileged address // should revert when calling withdrawETHRatioToSafeAddress from a non-privileged address // should revert when calling withdrawERC20RatioToSafeAddress from a non-privileged address - function testAccessControl3() public { + function testAccessControlWithdrawalsUnprivilegedCaller() public { vm.expectRevert("UNAUTHORIZED"); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, false); vm.expectRevert("UNAUTHORIZED"); @@ -166,7 +166,7 @@ contract PCVGuardianTest is DSTest { // should allow GOVERNOR to perform withdrawals // should allow PCV_SAFE_MOVER_ROLE to perform withdrawals // should allow GUARDIAN to perform withdrawals - function testAccessControl4() public { + function testAccessControlWithdrawalsPrivilegedCallers() public { // none of the following calls shall revert if access // control is properly checked for the various callers vm.startPrank(addresses.governorAddress); @@ -192,7 +192,7 @@ contract PCVGuardianTest is DSTest { ); vm.stopPrank(); - vm.startPrank(addresses.pcvSafeMoverAddress); + vm.startPrank(addresses.pcvSafeMover); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); pcvGuardian.withdrawERC20ToSafeAddress( @@ -261,14 +261,32 @@ contract PCVGuardianTest is DSTest { } // check state changes & getters after set & unset - function testStateConsistency1() public { + function testIsSafeAddress() public { + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); + + vm.prank(addresses.governorAddress); + pcvGuardian.setSafeAddress(address(pcvDeposit3)); + + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), true); + + vm.prank(addresses.governorAddress); + pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); + + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); + } + + // check state changes & getters after set & unset + function testGetSafeAddresses() public { address[] memory getSafeAddresses = pcvGuardian.getSafeAddresses(); assertEq(getSafeAddresses.length, 2); assertEq(getSafeAddresses[0], address(pcvDeposit1)); assertEq(getSafeAddresses[1], address(pcvDeposit2)); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); vm.prank(addresses.governorAddress); pcvGuardian.setSafeAddress(address(pcvDeposit3)); @@ -278,9 +296,6 @@ contract PCVGuardianTest is DSTest { assertEq(getSafeAddresses[0], address(pcvDeposit1)); assertEq(getSafeAddresses[1], address(pcvDeposit2)); assertEq(getSafeAddresses[2], address(pcvDeposit3)); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), true); vm.prank(addresses.governorAddress); pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); @@ -289,29 +304,23 @@ contract PCVGuardianTest is DSTest { assertEq(getSafeAddresses.length, 2); assertEq(getSafeAddresses[0], address(pcvDeposit1)); assertEq(getSafeAddresses[1], address(pcvDeposit2)); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); } // can't set an already safe address - // can't unset an already unsafe address - function testStateConsistency2() public { - address safeAddress = address(0x42); - vm.startPrank(addresses.governorAddress); - - vm.expectRevert("PCVGuardian: not a safe address"); - pcvGuardian.unsetSafeAddress(safeAddress); - - pcvGuardian.setSafeAddress(safeAddress); - + function testSetAlreadySafeAddress() public { + vm.prank(addresses.governorAddress); vm.expectRevert("PCVGuardian: already a safe address"); - pcvGuardian.setSafeAddress(safeAddress); + pcvGuardian.setSafeAddress(address(pcvDeposit1)); + } - vm.stopPrank(); + // can't unset an already unsafe address + function testUnsetUnsafeAddress() public { + vm.prank(addresses.governorAddress); + vm.expectRevert("PCVGuardian: not a safe address"); + pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); } - // withdrawals + ///////////// WITHDRAWALS ////////// // should not be able to withdraw to a non-safe address function testOnlyWithdrawableToSafeAdress() public { @@ -319,7 +328,7 @@ contract PCVGuardianTest is DSTest { vm.expectRevert("PCVGuardian: address not whitelisted"); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); - vm.prank(addresses.pcvGuardianAdminAddress); + vm.prank(addresses.pcvGuardianAdmin); pcvGuardian.setSafeAddress(address(pcvDeposit3)); vm.startPrank(addresses.guardianAddress); @@ -332,24 +341,20 @@ contract PCVGuardianTest is DSTest { } // should withdraw from a pcv deposit - // should withdrawETH from a pcv deposit - // should withdrawERC20 from a pcv deposit - // should withdrawRatio from a pcv deposit - // should withdrawETHRatio from a pcv deposit - // should withdrawERC20Ratio from a pcv deposit - function testWithdrawals() public { - // initial state - vm.startPrank(addresses.pcvSafeMoverAddress); + function testWithdrawToSafeAddress() public { assertEq(pcvDeposit1.balance(), 100e18); assertEq(pcvDeposit2.balance(), 0); - assertEq(address(pcvDeposit1).balance, 1 ether); - assertEq(address(pcvDeposit2).balance, 0); - - // withdrawals + vm.prank(addresses.pcvSafeMover); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); assertEq(pcvDeposit1.balance(), 99e18); assertEq(pcvDeposit2.balance(), 1e18); + } + // should withdrawETH from a pcv deposit + function testWithdrawETHToSafeAddress() public { + assertEq(address(pcvDeposit1).balance, 1 ether); + assertEq(address(pcvDeposit2).balance, 0); + vm.prank(addresses.pcvSafeMover); pcvGuardian.withdrawETHToSafeAddress( address(pcvDeposit1), payable(address(pcvDeposit2)), @@ -359,7 +364,13 @@ contract PCVGuardianTest is DSTest { ); assertEq(address(pcvDeposit1).balance, 0.9 ether); assertEq(address(pcvDeposit2).balance, 0.1 ether); + } + // should withdrawERC20 from a pcv deposit + function testWithdrawERC20ToSafeAddress() public { + assertEq(pcvDeposit1.balance(), 100e18); + assertEq(pcvDeposit2.balance(), 0); + vm.prank(addresses.pcvSafeMover); pcvGuardian.withdrawERC20ToSafeAddress( address(pcvDeposit1), address(pcvDeposit2), @@ -368,51 +379,51 @@ contract PCVGuardianTest is DSTest { false, true ); - assertEq(pcvDeposit1.balance(), 98e18); - assertEq(pcvDeposit2.balance(), 2e18); + assertEq(pcvDeposit1.balance(), 99e18); + assertEq(pcvDeposit2.balance(), 1e18); + } - // ratio withdrawals - pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 5000, false, true); - assertEq(pcvDeposit1.balance(), 49e18); - assertEq(pcvDeposit2.balance(), 51e18); + // should withdrawRatio from a pcv deposit + function testWithdrawRatioToSafeAddress() public { + assertEq(pcvDeposit1.balance(), 100e18); + assertEq(pcvDeposit2.balance(), 0); + vm.prank(addresses.pcvSafeMover); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 5500, false, true); + assertEq(pcvDeposit1.balance(), 45e18); + assertEq(pcvDeposit2.balance(), 55e18); + } + // should withdrawETHRatio from a pcv deposit + function testWithdrawETHRatioToSafeAddress() public { + assertEq(address(pcvDeposit1).balance, 1 ether); + assertEq(address(pcvDeposit2).balance, 0); + vm.prank(addresses.pcvSafeMover); pcvGuardian.withdrawETHRatioToSafeAddress( address(pcvDeposit1), payable(address(pcvDeposit2)), - 5000, + 5500, false, false ); assertEq(address(pcvDeposit1).balance, 0.45 ether); assertEq(address(pcvDeposit2).balance, 0.55 ether); + } + // should withdrawERC20Ratio from a pcv deposit + function testWithdrawERC20RatioToSafeAddress() public { + assertEq(pcvDeposit1.balance(), 100e18); + assertEq(pcvDeposit2.balance(), 0); + vm.prank(addresses.pcvSafeMover); pcvGuardian.withdrawERC20RatioToSafeAddress( address(pcvDeposit1), address(pcvDeposit2), address(token), - 5000, + 6700, false, true ); - assertEq(pcvDeposit1.balance(), 24.5e18); - assertEq(pcvDeposit2.balance(), 75.5e18); - - // transfer back all tokens & eth - pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 10000, false, true); - pcvGuardian.withdrawETHRatioToSafeAddress( - address(pcvDeposit2), - payable(address(pcvDeposit1)), - 10000, - false, - false - ); - vm.stopPrank(); - - // end state = initial state - assertEq(pcvDeposit1.balance(), 100e18); - assertEq(pcvDeposit2.balance(), 0); - assertEq(address(pcvDeposit1).balance, 1 ether); - assertEq(address(pcvDeposit2).balance, 0); + assertEq(pcvDeposit1.balance(), 33e18); + assertEq(pcvDeposit2.balance(), 67e18); } // should withdraw and deposit after diff --git a/contracts/test/utils/Fixtures.sol b/contracts/test/utils/Fixtures.sol index b44d60501..3f28502cd 100644 --- a/contracts/test/utils/Fixtures.sol +++ b/contracts/test/utils/Fixtures.sol @@ -17,8 +17,8 @@ struct FeiTestAddresses { address minterAddress; address burnerAddress; address guardianAddress; - address pcvGuardianAdminAddress; - address pcvSafeMoverAddress; + address pcvGuardianAdmin; + address pcvSafeMover; } /// @dev Get a list of addresses @@ -35,8 +35,8 @@ function getAddresses() pure returns (FeiTestAddresses memory) { minterAddress: address(0x9), burnerAddress: address(0x10), guardianAddress: address(0x11), - pcvGuardianAdminAddress: address(0x12), - pcvSafeMoverAddress: address(0x13) + pcvGuardianAdmin: address(0x12), + pcvSafeMover: address(0x13) }); return addresses; @@ -59,9 +59,9 @@ function getCore() returns (Core) { core.grantGuardian(addresses.guardianAddress); core.createRole(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GOVERNOR); - core.grantRole(TribeRoles.PCV_GUARDIAN_ADMIN, addresses.pcvGuardianAdminAddress); + core.grantRole(TribeRoles.PCV_GUARDIAN_ADMIN, addresses.pcvGuardianAdmin); core.createRole(TribeRoles.PCV_SAFE_MOVER_ROLE, TribeRoles.GOVERNOR); - core.grantRole(TribeRoles.PCV_SAFE_MOVER_ROLE, addresses.pcvSafeMoverAddress); + core.grantRole(TribeRoles.PCV_SAFE_MOVER_ROLE, addresses.pcvSafeMover); vm.stopPrank(); return core; From 915d275cac8eb9e104d50a8c721789acafa6154e Mon Sep 17 00:00:00 2001 From: Erwan Beauvois Date: Tue, 5 Jul 2022 18:03:25 +0200 Subject: [PATCH 03/12] rename fixture addresses & fix podexecutor unit tests --- .../integration/governance/RoleBastion.t.sol | 6 +-- contracts/test/unit/Fei.t.sol | 8 +-- contracts/test/unit/NonCustodialPSM.t.sol | 30 +++++------ contracts/test/unit/PCVGuardian.sol | 38 +++++++------- contracts/test/unit/governance/NopeDAO.t.sol | 2 +- .../test/unit/governance/PodExecutor.t.sol | 11 ++-- contracts/test/utils/Fixtures.sol | 50 +++++++++---------- 7 files changed, 72 insertions(+), 73 deletions(-) diff --git a/contracts/test/integration/governance/RoleBastion.t.sol b/contracts/test/integration/governance/RoleBastion.t.sol index afb006326..df6dad602 100644 --- a/contracts/test/integration/governance/RoleBastion.t.sol +++ b/contracts/test/integration/governance/RoleBastion.t.sol @@ -24,18 +24,18 @@ contract RoleBastionIntegrationTest is DSTest { roleBastion = new RoleBastion(address(core)); // 1. Grant tribalCouncil ROLE_ADMIN role - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); core.createRole(TribeRoles.ROLE_ADMIN, TribeRoles.GOVERNOR); core.grantRole(TribeRoles.ROLE_ADMIN, tribalCouncil); vm.stopPrank(); // 2. Grant roleBastion GOVERNOR - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); core.grantRole(TribeRoles.GOVERNOR, address(roleBastion)); vm.stopPrank(); // 3. Create a mock Guardian to pause the contract - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); core.createRole(TribeRoles.GUARDIAN, TribeRoles.GOVERNOR); core.grantRole(TribeRoles.GUARDIAN, guardian); vm.stopPrank(); diff --git a/contracts/test/unit/Fei.t.sol b/contracts/test/unit/Fei.t.sol index ecb2e436c..92994f585 100644 --- a/contracts/test/unit/Fei.t.sol +++ b/contracts/test/unit/Fei.t.sol @@ -25,15 +25,15 @@ contract FeiTest is DSTest { function testDeployedMetaData() public { assertEq(fei.totalSupply(), 0); - assertTrue(core.isGovernor(addresses.governorAddress)); + assertTrue(core.isGovernor(addresses.governor)); } function testMintsFei() public { uint256 mintAmount = 100; - vm.prank(addresses.minterAddress); - fei.mint(addresses.userAddress, mintAmount); + vm.prank(addresses.minter); + fei.mint(addresses.user, mintAmount); - assertEq(fei.balanceOf(addresses.userAddress), mintAmount); + assertEq(fei.balanceOf(addresses.user), mintAmount); } } diff --git a/contracts/test/unit/NonCustodialPSM.t.sol b/contracts/test/unit/NonCustodialPSM.t.sol index ca4869580..7102539cd 100644 --- a/contracts/test/unit/NonCustodialPSM.t.sol +++ b/contracts/test/unit/NonCustodialPSM.t.sol @@ -87,13 +87,13 @@ contract NonCustodialPSMTest is DSTest { /// create PSM psm = new NonCustodialPSM(oracleParams, multiRateLimitedParams, PSMParams); - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); /// grant the PSM the PCV Controller role - core.grantMinter(addresses.governorAddress); + core.grantMinter(addresses.governor); core.grantMinter(address(rateLimitedMinter)); core.grantPCVController(address(psm)); - core.grantPCVController(addresses.governorAddress); + core.grantPCVController(addresses.governor); rateLimitedMinter.addAddress(address(psm), uint112(rps), uint112(bufferCap)); /// mint fei to the user @@ -131,7 +131,7 @@ contract NonCustodialPSMTest is DSTest { function testGetMaxMintAmountOut() public { assertEq(psm.getMaxMintAmountOut(), bufferCap); - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); fei.mint(address(psm), mintAmount); vm.stopPrank(); @@ -233,7 +233,7 @@ contract NonCustodialPSMTest is DSTest { /// @notice replenishable rate limited minter buffer on the PSM gets increased on mint function testBufferReplenishmentDoesNotCompound() public { - vm.prank(addresses.governorAddress); + vm.prank(addresses.governor); fei.mint(address(this), mintAmount * 10); fei.approve(address(psm), mintAmount * 10); /// drain buffer @@ -287,7 +287,7 @@ contract NonCustodialPSMTest is DSTest { /// @notice withdraw erc20 succeeds with correct permissions function testERC20WithdrawSuccess() public { - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); core.grantPCVController(address(this)); underlyingToken.mint(address(psm), mintAmount); @@ -310,7 +310,7 @@ contract NonCustodialPSMTest is DSTest { /// @notice set global rate limited minter fails when caller is governor and new address is 0 function testSetGlobalRateLimitedMinterFailureZeroAddress() public { - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); vm.expectRevert(bytes("PegStabilityModule: Invalid new GlobalRateLimitedMinter")); psm.setGlobalRateLimitedMinter(GlobalRateLimitedMinter(address(0))); @@ -320,7 +320,7 @@ contract NonCustodialPSMTest is DSTest { /// @notice set global rate limited minter succeeds when caller is governor function testSetGlobalRateLimitedMinterSuccess() public { - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); psm.setGlobalRateLimitedMinter(GlobalRateLimitedMinter(address(this))); @@ -331,7 +331,7 @@ contract NonCustodialPSMTest is DSTest { /// @notice set global rate limited minter fails when caller is governor and new address is 0 function testSetPCVDepositFailureZeroAddress() public { - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); vm.expectRevert(bytes("PegStabilityModule: Invalid new PCVDeposit")); psm.setPCVDeposit(IPCVDeposit(address(0))); @@ -348,7 +348,7 @@ contract NonCustodialPSMTest is DSTest { /* Disabled - we deploy to a pcv deposit v1 /// @notice set PCV deposit fails when caller is governor and new address is 0 function testSetPCVDepositFailureUnderlyingTokenMismatch() public { - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); MockPCVDepositV2 newPCVDeposit = new MockPCVDepositV2( address(core), @@ -367,7 +367,7 @@ contract NonCustodialPSMTest is DSTest { /// @notice set PCV Deposit succeeds when caller is governor and underlying tokens match function testSetPCVDepositSuccess() public { - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); MockPCVDepositV2 newPCVDeposit = new MockPCVDepositV2(address(core), address(underlyingToken), 0, 0); @@ -380,7 +380,7 @@ contract NonCustodialPSMTest is DSTest { /// @notice set mint fee succeeds function testSetMintFeeSuccess() public { - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); psm.setMintFee(100); vm.stopPrank(); @@ -396,7 +396,7 @@ contract NonCustodialPSMTest is DSTest { /// @notice set redeem fee succeeds function testSetRedeemFeeSuccess() public { - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); psm.setRedeemFee(100); vm.stopPrank(); @@ -412,7 +412,7 @@ contract NonCustodialPSMTest is DSTest { /// @notice redeem fails when paused function testRedeemFailsWhenPaused() public { - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); psm.pauseRedeem(); vm.stopPrank(); @@ -422,7 +422,7 @@ contract NonCustodialPSMTest is DSTest { /// @notice mint fails when paused function testMintFailsWhenPaused() public { - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); psm.pauseMint(); vm.stopPrank(); diff --git a/contracts/test/unit/PCVGuardian.sol b/contracts/test/unit/PCVGuardian.sol index 602fa0e8d..8df04ecf3 100644 --- a/contracts/test/unit/PCVGuardian.sol +++ b/contracts/test/unit/PCVGuardian.sol @@ -39,7 +39,7 @@ contract PCVGuardianTest is DSTest { safeAddresses[1] = address(pcvDeposit2); pcvGuardian = new PCVGuardian(address(core), safeAddresses); // give roles to pcv guardian - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); core.grantPCVController(address(pcvGuardian)); core.grantGuardian(address(pcvGuardian)); vm.stopPrank(); @@ -111,7 +111,7 @@ contract PCVGuardianTest is DSTest { pcvGuardian.unsetSafeAddresses(safeAddresses); vm.stopPrank(); - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); pcvGuardian.setSafeAddress(safeAddress); pcvGuardian.unsetSafeAddress(safeAddress); pcvGuardian.setSafeAddresses(safeAddresses); @@ -120,11 +120,11 @@ contract PCVGuardianTest is DSTest { vm.prank(addresses.pcvGuardianAdmin); pcvGuardian.setSafeAddress(safeAddress); - vm.prank(addresses.guardianAddress); + vm.prank(addresses.guardian); pcvGuardian.unsetSafeAddress(safeAddress); vm.prank(addresses.pcvGuardianAdmin); pcvGuardian.setSafeAddresses(safeAddresses); - vm.prank(addresses.guardianAddress); + vm.prank(addresses.guardian); pcvGuardian.unsetSafeAddresses(safeAddresses); } @@ -169,7 +169,7 @@ contract PCVGuardianTest is DSTest { function testAccessControlWithdrawalsPrivilegedCallers() public { // none of the following calls shall revert if access // control is properly checked for the various callers - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); pcvGuardian.withdrawERC20ToSafeAddress( @@ -215,7 +215,7 @@ contract PCVGuardianTest is DSTest { ); vm.stopPrank(); - vm.startPrank(addresses.guardianAddress); + vm.startPrank(addresses.guardian); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); pcvGuardian.withdrawERC20ToSafeAddress( @@ -239,7 +239,7 @@ contract PCVGuardianTest is DSTest { vm.stopPrank(); // move back all tokens & eth - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); pcvGuardian.withdrawETHRatioToSafeAddress( address(pcvDeposit2), payable(address(pcvDeposit1)), @@ -266,14 +266,14 @@ contract PCVGuardianTest is DSTest { assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); - vm.prank(addresses.governorAddress); + vm.prank(addresses.governor); pcvGuardian.setSafeAddress(address(pcvDeposit3)); assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), true); - vm.prank(addresses.governorAddress); + vm.prank(addresses.governor); pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); @@ -288,7 +288,7 @@ contract PCVGuardianTest is DSTest { assertEq(getSafeAddresses[0], address(pcvDeposit1)); assertEq(getSafeAddresses[1], address(pcvDeposit2)); - vm.prank(addresses.governorAddress); + vm.prank(addresses.governor); pcvGuardian.setSafeAddress(address(pcvDeposit3)); getSafeAddresses = pcvGuardian.getSafeAddresses(); @@ -297,7 +297,7 @@ contract PCVGuardianTest is DSTest { assertEq(getSafeAddresses[1], address(pcvDeposit2)); assertEq(getSafeAddresses[2], address(pcvDeposit3)); - vm.prank(addresses.governorAddress); + vm.prank(addresses.governor); pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); getSafeAddresses = pcvGuardian.getSafeAddresses(); @@ -308,14 +308,14 @@ contract PCVGuardianTest is DSTest { // can't set an already safe address function testSetAlreadySafeAddress() public { - vm.prank(addresses.governorAddress); + vm.prank(addresses.governor); vm.expectRevert("PCVGuardian: already a safe address"); pcvGuardian.setSafeAddress(address(pcvDeposit1)); } // can't unset an already unsafe address function testUnsetUnsafeAddress() public { - vm.prank(addresses.governorAddress); + vm.prank(addresses.governor); vm.expectRevert("PCVGuardian: not a safe address"); pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); } @@ -324,14 +324,14 @@ contract PCVGuardianTest is DSTest { // should not be able to withdraw to a non-safe address function testOnlyWithdrawableToSafeAdress() public { - vm.prank(addresses.guardianAddress); + vm.prank(addresses.guardian); vm.expectRevert("PCVGuardian: address not whitelisted"); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); vm.prank(addresses.pcvGuardianAdmin); pcvGuardian.setSafeAddress(address(pcvDeposit3)); - vm.startPrank(addresses.guardianAddress); + vm.startPrank(addresses.guardian); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit3), address(pcvDeposit1), 1, false, true); pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); @@ -429,7 +429,7 @@ contract PCVGuardianTest is DSTest { // should withdraw and deposit after // should withdraw and not deposit after function testDepositAfter() public { - vm.startPrank(addresses.guardianAddress); + vm.startPrank(addresses.guardian); assertEq(pcvDeposit1._resistantBalance(), 100e18); assertEq(pcvDeposit2._resistantBalance(), 0); @@ -448,7 +448,7 @@ contract PCVGuardianTest is DSTest { // should withdraw and pause after // should withdraw and not pause after function testPauseAfter() public { - vm.startPrank(addresses.guardianAddress); + vm.startPrank(addresses.guardian); assertEq(pcvDeposit1.paused(), false); assertEq(pcvDeposit2.paused(), false); @@ -469,7 +469,7 @@ contract PCVGuardianTest is DSTest { // should withdraw, and pause after // should keep paused state if deposit was paused before withdraw function testUnpauseBefore() public { - vm.startPrank(addresses.guardianAddress); + vm.startPrank(addresses.guardian); pcvDeposit1.pause(); assertEq(pcvDeposit1.paused(), true); @@ -492,7 +492,7 @@ contract PCVGuardianTest is DSTest { // should revert if trying to depositAfter on a paused safe address function testRevertPausedAfter() public { - vm.startPrank(addresses.guardianAddress); + vm.startPrank(addresses.guardian); pcvDeposit2.pause(); vm.expectRevert("Pausable: paused"); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); diff --git a/contracts/test/unit/governance/NopeDAO.t.sol b/contracts/test/unit/governance/NopeDAO.t.sol index e35d5e30c..2482dabaf 100644 --- a/contracts/test/unit/governance/NopeDAO.t.sol +++ b/contracts/test/unit/governance/NopeDAO.t.sol @@ -56,7 +56,7 @@ contract NopeDAOTest is DSTest { // 2. Setup Tribe and transfer some to a test address tribeAddress = address(core.tribe()); - vm.prank(addresses.governorAddress); + vm.prank(addresses.governor); core.allocateTribe(user, excessQuorumTribe); tribe = ERC20VotesComp(tribeAddress); diff --git a/contracts/test/unit/governance/PodExecutor.t.sol b/contracts/test/unit/governance/PodExecutor.t.sol index e04d54773..c949a8ce4 100644 --- a/contracts/test/unit/governance/PodExecutor.t.sol +++ b/contracts/test/unit/governance/PodExecutor.t.sol @@ -72,11 +72,6 @@ contract PodExecutorTest is DSTest { // 1. Deploy PodExecutor podExecutor = new PodExecutor(address(core)); - vm.startPrank(addresses.governorAddress); - core.createRole(TribeRoles.GOVERNOR, TribeRoles.GOVERNOR); - core.grantRole(TribeRoles.GOVERNOR, addresses.governorAddress); - vm.stopPrank(); - // 2. Deploy TimelockController. Set podExecutor as EXECUTOR uint256 minDelay = 0; address[] memory proposers = new address[](1); @@ -89,11 +84,15 @@ contract PodExecutorTest is DSTest { // 3. Give timelock some ETH to transfer in proposal vm.deal(address(timelock), 10 ether); + + // 4. Initialize time to be >1 + // TimelockController relies on timestamps being at least _DONE_TIMESTAMP (>1) + vm.warp(1000); } /// @notice Validate can pause execution function testCanPause() public { - vm.prank(addresses.governorAddress); + vm.prank(addresses.governor); podExecutor.pause(); vm.expectRevert(bytes("Pausable: paused")); diff --git a/contracts/test/utils/Fixtures.sol b/contracts/test/utils/Fixtures.sol index 3f28502cd..957b15c47 100644 --- a/contracts/test/utils/Fixtures.sol +++ b/contracts/test/utils/Fixtures.sol @@ -6,17 +6,17 @@ import {TribeRoles} from "../../core/TribeRoles.sol"; import {Vm} from "./Vm.sol"; struct FeiTestAddresses { - address userAddress; - address secondUserAddress; - address beneficiaryAddress1; - address beneficiaryAddress2; - address governorAddress; + address user; + address secondUser; + address beneficiary1; + address beneficiary2; + address governor; address genesisGroup; - address keeperAddress; - address pcvControllerAddress; - address minterAddress; - address burnerAddress; - address guardianAddress; + address keeper; + address pcvController; + address minter; + address burner; + address guardian; address pcvGuardianAdmin; address pcvSafeMover; } @@ -24,17 +24,17 @@ struct FeiTestAddresses { /// @dev Get a list of addresses function getAddresses() pure returns (FeiTestAddresses memory) { FeiTestAddresses memory addresses = FeiTestAddresses({ - userAddress: address(0x1), - secondUserAddress: address(0x2), - beneficiaryAddress1: address(0x3), - beneficiaryAddress2: address(0x4), - governorAddress: address(0x5), + user: address(0x1), + secondUser: address(0x2), + beneficiary1: address(0x3), + beneficiary2: address(0x4), + governor: address(0x5), genesisGroup: address(0x6), - keeperAddress: address(0x7), - pcvControllerAddress: address(0x8), - minterAddress: address(0x9), - burnerAddress: address(0x10), - guardianAddress: address(0x11), + keeper: address(0x7), + pcvController: address(0x8), + minter: address(0x9), + burner: address(0x10), + guardian: address(0x11), pcvGuardianAdmin: address(0x12), pcvSafeMover: address(0x13) }); @@ -49,14 +49,14 @@ function getCore() returns (Core) { FeiTestAddresses memory addresses = getAddresses(); // Deploy Core from Governor address - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); Core core = new Core(); core.init(); - core.grantMinter(addresses.minterAddress); - core.grantBurner(addresses.burnerAddress); - core.grantPCVController(addresses.pcvControllerAddress); - core.grantGuardian(addresses.guardianAddress); + core.grantMinter(addresses.minter); + core.grantBurner(addresses.burner); + core.grantPCVController(addresses.pcvController); + core.grantGuardian(addresses.guardian); core.createRole(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GOVERNOR); core.grantRole(TribeRoles.PCV_GUARDIAN_ADMIN, addresses.pcvGuardianAdmin); From 1dbaf4b35c5cfa0de875c85a3e06272ed4c924c1 Mon Sep 17 00:00:00 2001 From: Erwan Beauvois Date: Mon, 4 Jul 2022 13:27:43 +0200 Subject: [PATCH 04/12] Update PCVGuardian to v3 and add forge tests --- contracts/libs/CoreRefPauseableLib.sol | 4 + contracts/mock/MockPCVDepositV2.sol | 14 +- contracts/pcv/IPCVGuardian.sol | 50 ++- contracts/pcv/PCVGuardian.sol | 171 ++++++--- contracts/test/unit/PCVGuardian.sol | 491 +++++++++++++++++++++++++ contracts/test/utils/Fixtures.sol | 12 +- test/unit/pcv/PCVGuardian.test.ts | 269 -------------- 7 files changed, 684 insertions(+), 327 deletions(-) create mode 100644 contracts/test/unit/PCVGuardian.sol delete mode 100644 test/unit/pcv/PCVGuardian.test.ts diff --git a/contracts/libs/CoreRefPauseableLib.sol b/contracts/libs/CoreRefPauseableLib.sol index ef207500c..2c09ce4cb 100644 --- a/contracts/libs/CoreRefPauseableLib.sol +++ b/contracts/libs/CoreRefPauseableLib.sol @@ -34,6 +34,10 @@ library CoreRefPauseableLib { } } + function _paused(address _pauseableCoreRefAddress) internal view returns (bool) { + return CoreRef(_pauseableCoreRefAddress).paused(); + } + function _pause(address _pauseableCoreRefAddress) internal { CoreRef(_pauseableCoreRefAddress).pause(); } diff --git a/contracts/mock/MockPCVDepositV2.sol b/contracts/mock/MockPCVDepositV2.sol index ed8855300..80cb090c7 100644 --- a/contracts/mock/MockPCVDepositV2.sol +++ b/contracts/mock/MockPCVDepositV2.sol @@ -33,12 +33,22 @@ contract MockPCVDepositV2 is IPCVDeposit, CoreRef { return (resistantBalance, resistantProtocolOwnedFei); } + // gets the resistant token balance of this deposit + function _resistantBalance() external view returns (uint256) { + return resistantBalance; + } + + // gets the resistant protocol owned fei of this deposit + function _resistantFei() external view returns (uint256) { + return resistantProtocolOwnedFei; + } + // IPCVDeposit V1 - function deposit() external override { + function deposit() external override whenNotPaused { resistantBalance = IERC20(balanceReportedIn).balanceOf(address(this)); } - function withdraw(address to, uint256 amount) external override { + function withdraw(address to, uint256 amount) external override whenNotPaused { IERC20(balanceReportedIn).transfer(to, amount); resistantBalance = IERC20(balanceReportedIn).balanceOf(address(this)); } diff --git a/contracts/pcv/IPCVGuardian.sol b/contracts/pcv/IPCVGuardian.sol index 959e6981a..a3fbc8bef 100644 --- a/contracts/pcv/IPCVGuardian.sol +++ b/contracts/pcv/IPCVGuardian.sol @@ -50,7 +50,7 @@ interface IPCVGuardian { /// @param safeAddresses the addresses to un-set as safe function unsetSafeAddresses(address[] calldata safeAddresses) external; - /// @notice governor-or-guardian-only method to withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param amount the amount to withdraw @@ -64,7 +64,21 @@ interface IPCVGuardian { bool depositAfter ) external; - /// @notice governor-or-guardian-only method to withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @param pcvDeposit the address of the pcv deposit contract + /// @param safeAddress the destination address to withdraw to + /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw + /// @param pauseAfter if true, the pcv contract will be paused after the withdraw + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawRatioToSafeAddress( + address pcvDeposit, + address safeAddress, + uint256 basisPoints, + bool pauseAfter, + bool depositAfter + ) external; + + /// @notice withdraw funds from a pcv deposit, by calling the withdrawETH() method on it /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param amount the amount of tokens to withdraw @@ -78,7 +92,21 @@ interface IPCVGuardian { bool depositAfter ) external; - /// @notice governor-or-guardian-only method to withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdrawETH() method on it + /// @param pcvDeposit the address of the pcv deposit contract + /// @param safeAddress the destination address to withdraw to + /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw + /// @param pauseAfter if true, the pcv contract will be paused after the withdraw + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawETHRatioToSafeAddress( + address pcvDeposit, + address payable safeAddress, + uint256 basisPoints, + bool pauseAfter, + bool depositAfter + ) external; + + /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it /// @param pcvDeposit the deposit to pull funds from /// @param safeAddress the destination address to withdraw to /// @param token the token to withdraw @@ -93,4 +121,20 @@ interface IPCVGuardian { bool pauseAfter, bool depositAfter ) external; + + /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it + /// @param pcvDeposit the deposit to pull funds from + /// @param safeAddress the destination address to withdraw to + /// @param token the token to withdraw + /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw + /// @param pauseAfter whether to pause the pcv after withdrawing + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawERC20RatioToSafeAddress( + address pcvDeposit, + address safeAddress, + address token, + uint256 basisPoints, + bool pauseAfter, + bool depositAfter + ) external; } diff --git a/contracts/pcv/PCVGuardian.sol b/contracts/pcv/PCVGuardian.sol index 497098656..ae46978c9 100644 --- a/contracts/pcv/PCVGuardian.sol +++ b/contracts/pcv/PCVGuardian.sol @@ -1,12 +1,15 @@ // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity ^0.8.4; -import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; -import "../refs/CoreRef.sol"; -import "./IPCVGuardian.sol"; -import "./IPCVDeposit.sol"; -import "../libs/CoreRefPauseableLib.sol"; +import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; +import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import {ICore} from "../core/ICore.sol"; +import {CoreRef} from "../refs/CoreRef.sol"; +import {IPCVGuardian} from "./IPCVGuardian.sol"; +import {IPCVDeposit} from "./IPCVDeposit.sol"; +import {CoreRefPauseableLib} from "../libs/CoreRefPauseableLib.sol"; import {TribeRoles} from "../core/TribeRoles.sol"; +import {Constants} from "../Constants.sol"; contract PCVGuardian is IPCVGuardian, CoreRef { using CoreRefPauseableLib for address; @@ -41,7 +44,7 @@ contract PCVGuardian is IPCVGuardian, CoreRef { function setSafeAddress(address pcvDeposit) external override - hasAnyOfTwoRoles(TribeRoles.GOVERNOR, TribeRoles.PCV_GUARDIAN_ADMIN) + hasAnyOfTwoRoles(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GOVERNOR) { _setSafeAddress(pcvDeposit); } @@ -51,9 +54,9 @@ contract PCVGuardian is IPCVGuardian, CoreRef { function setSafeAddresses(address[] calldata _safeAddresses) external override - hasAnyOfTwoRoles(TribeRoles.GOVERNOR, TribeRoles.PCV_GUARDIAN_ADMIN) + hasAnyOfTwoRoles(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GOVERNOR) { - require(_safeAddresses.length != 0, "empty"); + require(_safeAddresses.length != 0, "PCVGuardian: empty"); for (uint256 i = 0; i < _safeAddresses.length; i++) { _setSafeAddress(_safeAddresses[i]); } @@ -66,7 +69,7 @@ contract PCVGuardian is IPCVGuardian, CoreRef { function unsetSafeAddress(address pcvDeposit) external override - hasAnyOfThreeRoles(TribeRoles.GOVERNOR, TribeRoles.GUARDIAN, TribeRoles.PCV_GUARDIAN_ADMIN) + hasAnyOfThreeRoles(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GUARDIAN, TribeRoles.GOVERNOR) { _unsetSafeAddress(pcvDeposit); } @@ -76,45 +79,92 @@ contract PCVGuardian is IPCVGuardian, CoreRef { function unsetSafeAddresses(address[] calldata _safeAddresses) external override - hasAnyOfThreeRoles(TribeRoles.GOVERNOR, TribeRoles.GUARDIAN, TribeRoles.PCV_GUARDIAN_ADMIN) + hasAnyOfThreeRoles(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GUARDIAN, TribeRoles.GOVERNOR) { - require(_safeAddresses.length != 0, "empty"); + require(_safeAddresses.length != 0, "PCVGuardian: empty"); for (uint256 i = 0; i < _safeAddresses.length; i++) { _unsetSafeAddress(_safeAddresses[i]); } } - /// @notice governor-or-guardian-only method to withdraw funds from a pcv deposit, by calling the withdraw() method on it - /// @param pcvDeposit the address of the pcv deposit contract - /// @param safeAddress the destination address to withdraw to - /// @param amount the amount to withdraw - /// @param pauseAfter if true, the pcv contract will be paused after the withdraw - /// @param depositAfter if true, attempts to deposit to the target PCV deposit - function withdrawToSafeAddress( + /// @notice modifier to factorize the logic in all withdrawals : + /// - first, ensure the deposit to withdraw from is unpaused + /// - second, perform withdrawal + /// - third, re-pause deposit if it was paused or if pauseAfter = true + /// - finally, call pcvDeposit.deposit() if depositAfter = true + modifier beforeAndAfterWithdrawal( address pcvDeposit, address safeAddress, - uint256 amount, bool pauseAfter, bool depositAfter - ) external override hasAnyOfThreeRoles(TribeRoles.GOVERNOR, TribeRoles.PCV_SAFE_MOVER_ROLE, TribeRoles.GUARDIAN) { - require(isSafeAddress(safeAddress), "Provided address is not a safe address!"); + ) { + { + // scoped in this modifier to prevent stack to deep errors & enforce consitent acl + ICore _core = core(); + require( + _core.hasRole(TribeRoles.GUARDIAN, msg.sender) || + _core.hasRole(TribeRoles.PCV_SAFE_MOVER_ROLE, msg.sender) || + _core.hasRole(TribeRoles.GOVERNOR, msg.sender), + "UNAUTHORIZED" + ); + require(isSafeAddress(safeAddress), "PCVGuardian: address not whitelisted"); + } - pcvDeposit._ensureUnpaused(); + bool paused = pcvDeposit._paused(); + if (paused) { + pcvDeposit._unpause(); + } - IPCVDeposit(pcvDeposit).withdraw(safeAddress, amount); + _; - if (pauseAfter) { + if (paused || pauseAfter) { pcvDeposit._pause(); } if (depositAfter) { IPCVDeposit(safeAddress).deposit(); } + } + + /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @param pcvDeposit the address of the pcv deposit contract + /// @param safeAddress the destination address to withdraw to + /// @param amount the amount to withdraw + /// @param pauseAfter if true, the pcv contract will be paused after the withdraw + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawToSafeAddress( + address pcvDeposit, + address safeAddress, + uint256 amount, + bool pauseAfter, + bool depositAfter + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + IPCVDeposit(pcvDeposit).withdraw(safeAddress, amount); + emit PCVGuardianWithdrawal(pcvDeposit, safeAddress, amount); + } + /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @param pcvDeposit the address of the pcv deposit contract + /// @param safeAddress the destination address to withdraw to + /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw + /// @param pauseAfter if true, the pcv contract will be paused after the withdraw + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawRatioToSafeAddress( + address pcvDeposit, + address safeAddress, + uint256 basisPoints, + bool pauseAfter, + bool depositAfter + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + require(basisPoints <= Constants.BASIS_POINTS_GRANULARITY, "PCVGuardian: basisPoints too high"); + uint256 amount = (IPCVDeposit(pcvDeposit).balance() * basisPoints) / Constants.BASIS_POINTS_GRANULARITY; + require(amount != 0, "PCVGuardian: no value to withdraw"); + + IPCVDeposit(pcvDeposit).withdraw(safeAddress, amount); emit PCVGuardianWithdrawal(pcvDeposit, safeAddress, amount); } - /// @notice governor-or-guardian-only method to withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdrawETH() method on it /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param amount the amount of tokens to withdraw @@ -126,25 +176,33 @@ contract PCVGuardian is IPCVGuardian, CoreRef { uint256 amount, bool pauseAfter, bool depositAfter - ) external override hasAnyOfThreeRoles(TribeRoles.GOVERNOR, TribeRoles.PCV_SAFE_MOVER_ROLE, TribeRoles.GUARDIAN) { - require(isSafeAddress(safeAddress), "Provided address is not a safe address!"); - - pcvDeposit._ensureUnpaused(); - + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { IPCVDeposit(pcvDeposit).withdrawETH(safeAddress, amount); + emit PCVGuardianETHWithdrawal(pcvDeposit, safeAddress, amount); + } - if (pauseAfter) { - pcvDeposit._pause(); - } - - if (depositAfter) { - IPCVDeposit(safeAddress).deposit(); - } + /// @notice withdraw funds from a pcv deposit, by calling the withdrawETH() method on it + /// @param pcvDeposit the address of the pcv deposit contract + /// @param safeAddress the destination address to withdraw to + /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw + /// @param pauseAfter if true, the pcv contract will be paused after the withdraw + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawETHRatioToSafeAddress( + address pcvDeposit, + address payable safeAddress, + uint256 basisPoints, + bool pauseAfter, + bool depositAfter + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + require(basisPoints <= Constants.BASIS_POINTS_GRANULARITY, "PCVGuardian: basisPoints too high"); + uint256 amount = (pcvDeposit.balance * basisPoints) / Constants.BASIS_POINTS_GRANULARITY; + require(amount != 0, "PCVGuardian: no value to withdraw"); + IPCVDeposit(pcvDeposit).withdrawETH(safeAddress, amount); emit PCVGuardianETHWithdrawal(pcvDeposit, safeAddress, amount); } - /// @notice governor-or-guardian-only method to withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it /// @param pcvDeposit the deposit to pull funds from /// @param safeAddress the destination address to withdraw to /// @param amount the amount of funds to withdraw @@ -157,33 +215,42 @@ contract PCVGuardian is IPCVGuardian, CoreRef { uint256 amount, bool pauseAfter, bool depositAfter - ) external override hasAnyOfThreeRoles(TribeRoles.GOVERNOR, TribeRoles.PCV_SAFE_MOVER_ROLE, TribeRoles.GUARDIAN) { - require(isSafeAddress(safeAddress), "Provided address is not a safe address!"); - - pcvDeposit._ensureUnpaused(); - + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { IPCVDeposit(pcvDeposit).withdrawERC20(token, safeAddress, amount); + emit PCVGuardianERC20Withdrawal(pcvDeposit, safeAddress, token, amount); + } - if (pauseAfter) { - pcvDeposit._pause(); - } - - if (depositAfter) { - IPCVDeposit(safeAddress).deposit(); - } + /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it + /// @param pcvDeposit the deposit to pull funds from + /// @param safeAddress the destination address to withdraw to + /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw + /// @param pauseAfter whether to pause the pcv after withdrawing + /// @param depositAfter if true, attempts to deposit to the target PCV deposit + function withdrawERC20RatioToSafeAddress( + address pcvDeposit, + address safeAddress, + address token, + uint256 basisPoints, + bool pauseAfter, + bool depositAfter + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + require(basisPoints <= Constants.BASIS_POINTS_GRANULARITY, "PCVGuardian: basisPoints too high"); + uint256 amount = (IERC20(token).balanceOf(pcvDeposit) * basisPoints) / Constants.BASIS_POINTS_GRANULARITY; + require(amount != 0, "PCVGuardian: no value to withdraw"); + IPCVDeposit(pcvDeposit).withdrawERC20(token, safeAddress, amount); emit PCVGuardianERC20Withdrawal(pcvDeposit, safeAddress, token, amount); } // ---------- Internal Functions ---------- function _setSafeAddress(address anAddress) internal { - require(safeAddresses.add(anAddress), "set"); + require(safeAddresses.add(anAddress), "PCVGuardian: already a safe address"); emit SafeAddressAdded(anAddress); } function _unsetSafeAddress(address anAddress) internal { - require(safeAddresses.remove(anAddress), "unset"); + require(safeAddresses.remove(anAddress), "PCVGuardian: not a safe address"); emit SafeAddressRemoved(anAddress); } } diff --git a/contracts/test/unit/PCVGuardian.sol b/contracts/test/unit/PCVGuardian.sol new file mode 100644 index 000000000..93c43546e --- /dev/null +++ b/contracts/test/unit/PCVGuardian.sol @@ -0,0 +1,491 @@ +pragma solidity ^0.8.4; + +import {Vm} from "../utils/Vm.sol"; +import {DSTest} from "../utils/DSTest.sol"; +import {getCore, getAddresses, FeiTestAddresses} from "../utils/Fixtures.sol"; + +import {ICore} from "../../core/ICore.sol"; +import {MockERC20} from "../../mock/MockERC20.sol"; +import {MockPCVDepositV2} from "../../mock/MockPCVDepositV2.sol"; +import {PCVGuardian} from "../../pcv/PCVGuardian.sol"; + +contract PCVGuardianTest is DSTest { + ICore private core; + PCVGuardian private pcvGuardian; + MockERC20 private token; + MockPCVDepositV2 private pcvDeposit1; + MockPCVDepositV2 private pcvDeposit2; + MockPCVDepositV2 private pcvDeposit3; + + Vm public constant vm = Vm(HEVM_ADDRESS); + FeiTestAddresses public addresses = getAddresses(); + + function setUp() public { + // get core + core = getCore(); + + // initialize mock tokens & deposits + token = new MockERC20(); + pcvDeposit1 = new MockPCVDepositV2(address(core), address(token), 0, 0); + pcvDeposit2 = new MockPCVDepositV2(address(core), address(token), 0, 0); + pcvDeposit3 = new MockPCVDepositV2(address(core), address(token), 0, 0); + token.mint(address(pcvDeposit1), 100e18); + pcvDeposit1.deposit(); + payable(address(pcvDeposit1)).transfer(1 ether); + + // initialize pcv guardian + address[] memory safeAddresses = new address[](2); + safeAddresses[0] = address(pcvDeposit1); + safeAddresses[1] = address(pcvDeposit2); + pcvGuardian = new PCVGuardian(address(core), safeAddresses); + // give roles to pcv guardian + vm.startPrank(addresses.governorAddress); + core.grantPCVController(address(pcvGuardian)); + core.grantGuardian(address(pcvGuardian)); + vm.stopPrank(); + + // labels + vm.label(address(core), "core"); + vm.label(address(token), "token"); + vm.label(address(pcvDeposit1), "pcvDeposit1"); + vm.label(address(pcvDeposit2), "pcvDeposit2"); + vm.label(address(pcvDeposit3), "pcvDeposit3"); + vm.label(address(pcvGuardian), "pcvGuardian"); + } + + // should have no safe addresses upon deployment when deployed with no safe addresses + function testDeploy1() public { + address[] memory safeAddresses = new address[](0); + PCVGuardian pcvGuardianWithoutAddresses = new PCVGuardian(address(core), safeAddresses); + address[] memory getSafeAddresses = pcvGuardianWithoutAddresses.getSafeAddresses(); + assertEq(getSafeAddresses.length, 0); + } + + // should have safe addresses upon deployment when deployed with safe addresses + function testDeploy2() public { + address[] memory safeAddresses = new address[](1); + safeAddresses[0] = address(0x42); + PCVGuardian pcvGuardianWithoutAddresses = new PCVGuardian(address(core), safeAddresses); + address[] memory getSafeAddresses = pcvGuardianWithoutAddresses.getSafeAddresses(); + assertEq(getSafeAddresses.length, 1); + assertEq(getSafeAddresses[0], address(0x42)); + } + + // access control and state changes + + // should revert when calling setSafeAddress & setSafeAddresses from a non-privileged address + // should revert when calling unsetSafeAddress & unsetSafeAddresses from a non-privileged address + function testAccessControl1() public { + address safeAddress = address(0x42); + address[] memory safeAddresses = new address[](1); + safeAddresses[0] = safeAddress; + + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.setSafeAddress(safeAddress); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.setSafeAddresses(safeAddresses); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.unsetSafeAddress(safeAddress); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.unsetSafeAddresses(safeAddresses); + } + + // ADD safe address(es) : + // should allow PCV_GUARDIAN_ADMIN + // should allow GOVERNOR + // REMOVE safe adress(es) : + // should allow GUARDIAN + // should allow PCV_GUARDIAN_ADMIN + // should allow GOVERNOR + function testAccessControl2() public { + address safeAddress = address(0x42); + address[] memory safeAddresses = new address[](1); + safeAddresses[0] = safeAddress; + + // none of the following calls shall revert if access + // control is properly checked for the various callers + vm.startPrank(addresses.pcvGuardianAdminAddress); + pcvGuardian.setSafeAddress(safeAddress); + pcvGuardian.unsetSafeAddress(safeAddress); + pcvGuardian.setSafeAddresses(safeAddresses); + pcvGuardian.unsetSafeAddresses(safeAddresses); + vm.stopPrank(); + + vm.startPrank(addresses.governorAddress); + pcvGuardian.setSafeAddress(safeAddress); + pcvGuardian.unsetSafeAddress(safeAddress); + pcvGuardian.setSafeAddresses(safeAddresses); + pcvGuardian.unsetSafeAddresses(safeAddresses); + vm.stopPrank(); + + vm.prank(addresses.pcvGuardianAdminAddress); + pcvGuardian.setSafeAddress(safeAddress); + vm.prank(addresses.guardianAddress); + pcvGuardian.unsetSafeAddress(safeAddress); + vm.prank(addresses.pcvGuardianAdminAddress); + pcvGuardian.setSafeAddresses(safeAddresses); + vm.prank(addresses.guardianAddress); + pcvGuardian.unsetSafeAddresses(safeAddresses); + } + + // should revert when calling withdrawToSafeAddress from a non-privileged address + // should revert when calling withdrawETHToSafeAddress from a non-privileged address + // should revert when calling withdrawERC20ToSafeAddress from a non-privileged address + // should revert when calling withdrawRatioToSafeAddress from a non-privileged address + // should revert when calling withdrawETHRatioToSafeAddress from a non-privileged address + // should revert when calling withdrawERC20RatioToSafeAddress from a non-privileged address + function testAccessControl3() public { + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, false); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1e18, false, false); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.withdrawERC20ToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1e18, + false, + false + ); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + vm.expectRevert("UNAUTHORIZED"); + pcvGuardian.withdrawERC20RatioToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + } + + // should allow GOVERNOR to perform withdrawals + // should allow PCV_SAFE_MOVER_ROLE to perform withdrawals + // should allow GUARDIAN to perform withdrawals + function testAccessControl4() public { + // none of the following calls shall revert if access + // control is properly checked for the various callers + vm.startPrank(addresses.governorAddress); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawERC20ToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawERC20RatioToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + vm.stopPrank(); + + vm.startPrank(addresses.pcvSafeMoverAddress); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawERC20ToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawERC20RatioToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + vm.stopPrank(); + + vm.startPrank(addresses.guardianAddress); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawERC20ToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawERC20RatioToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1, + false, + false + ); + vm.stopPrank(); + + // move back all tokens & eth + vm.startPrank(addresses.governorAddress); + pcvGuardian.withdrawETHRatioToSafeAddress( + address(pcvDeposit2), + payable(address(pcvDeposit1)), + 10000, + false, + false + ); + pcvGuardian.withdrawERC20RatioToSafeAddress( + address(pcvDeposit2), + address(pcvDeposit1), + address(token), + 10000, + false, + false + ); + pcvDeposit1.deposit(); + pcvDeposit2.deposit(); + vm.stopPrank(); + } + + // check state changes & getters after set & unset + function testStateConsistency1() public { + address[] memory getSafeAddresses = pcvGuardian.getSafeAddresses(); + assertEq(getSafeAddresses.length, 2); + assertEq(getSafeAddresses[0], address(pcvDeposit1)); + assertEq(getSafeAddresses[1], address(pcvDeposit2)); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); + + vm.prank(addresses.governorAddress); + pcvGuardian.setSafeAddress(address(pcvDeposit3)); + + getSafeAddresses = pcvGuardian.getSafeAddresses(); + assertEq(getSafeAddresses.length, 3); + assertEq(getSafeAddresses[0], address(pcvDeposit1)); + assertEq(getSafeAddresses[1], address(pcvDeposit2)); + assertEq(getSafeAddresses[2], address(pcvDeposit3)); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), true); + + vm.prank(addresses.governorAddress); + pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); + + getSafeAddresses = pcvGuardian.getSafeAddresses(); + assertEq(getSafeAddresses.length, 2); + assertEq(getSafeAddresses[0], address(pcvDeposit1)); + assertEq(getSafeAddresses[1], address(pcvDeposit2)); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); + } + + // can't set an already safe address + // can't unset an already unsafe address + function testStateConsistency2() public { + address safeAddress = address(0x42); + vm.startPrank(addresses.governorAddress); + + vm.expectRevert("PCVGuardian: not a safe address"); + pcvGuardian.unsetSafeAddress(safeAddress); + + pcvGuardian.setSafeAddress(safeAddress); + + vm.expectRevert("PCVGuardian: already a safe address"); + pcvGuardian.setSafeAddress(safeAddress); + + vm.stopPrank(); + } + + // withdrawals + + // should not be able to withdraw to a non-safe address + function testOnlyWithdrawableToSafeAdress() public { + vm.prank(addresses.guardianAddress); + vm.expectRevert("PCVGuardian: address not whitelisted"); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); + + vm.prank(addresses.pcvGuardianAdminAddress); + pcvGuardian.setSafeAddress(address(pcvDeposit3)); + + vm.startPrank(addresses.guardianAddress); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit3), address(pcvDeposit1), 1, false, true); + pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); + vm.expectRevert("PCVGuardian: address not whitelisted"); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); + vm.stopPrank(); + } + + // should withdraw from a pcv deposit + // should withdrawETH from a pcv deposit + // should withdrawERC20 from a pcv deposit + // should withdrawRatio from a pcv deposit + // should withdrawETHRatio from a pcv deposit + // should withdrawERC20Ratio from a pcv deposit + function testWithdrawals() public { + // initial state + vm.startPrank(addresses.pcvSafeMoverAddress); + assertEq(pcvDeposit1.balance(), 100e18); + assertEq(pcvDeposit2.balance(), 0); + assertEq(address(pcvDeposit1).balance, 1 ether); + assertEq(address(pcvDeposit2).balance, 0); + + // withdrawals + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); + assertEq(pcvDeposit1.balance(), 99e18); + assertEq(pcvDeposit2.balance(), 1e18); + + pcvGuardian.withdrawETHToSafeAddress( + address(pcvDeposit1), + payable(address(pcvDeposit2)), + 0.1 ether, + false, + false + ); + assertEq(address(pcvDeposit1).balance, 0.9 ether); + assertEq(address(pcvDeposit2).balance, 0.1 ether); + + pcvGuardian.withdrawERC20ToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 1e18, + false, + true + ); + assertEq(pcvDeposit1.balance(), 98e18); + assertEq(pcvDeposit2.balance(), 2e18); + + // ratio withdrawals + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 5000, false, true); + assertEq(pcvDeposit1.balance(), 49e18); + assertEq(pcvDeposit2.balance(), 51e18); + + pcvGuardian.withdrawETHRatioToSafeAddress( + address(pcvDeposit1), + payable(address(pcvDeposit2)), + 5000, + false, + false + ); + assertEq(address(pcvDeposit1).balance, 0.45 ether); + assertEq(address(pcvDeposit2).balance, 0.55 ether); + + pcvGuardian.withdrawERC20RatioToSafeAddress( + address(pcvDeposit1), + address(pcvDeposit2), + address(token), + 5000, + false, + true + ); + assertEq(pcvDeposit1.balance(), 24.5e18); + assertEq(pcvDeposit2.balance(), 75.5e18); + + // transfer back all tokens & eth + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 10000, false, true); + pcvGuardian.withdrawETHRatioToSafeAddress( + address(pcvDeposit2), + payable(address(pcvDeposit1)), + 10000, + false, + false + ); + vm.stopPrank(); + + // end state = initial state + assertEq(pcvDeposit1.balance(), 100e18); + assertEq(pcvDeposit2.balance(), 0); + assertEq(address(pcvDeposit1).balance, 1 ether); + assertEq(address(pcvDeposit2).balance, 0); + } + + // should withdraw and deposit after + // should withdraw and not deposit after + function testDepositAfter() public { + vm.startPrank(addresses.guardianAddress); + + assertEq(pcvDeposit1._resistantBalance(), 100e18); + assertEq(pcvDeposit2._resistantBalance(), 0); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, false); + assertEq(pcvDeposit1._resistantBalance(), 99e18); + assertEq(pcvDeposit2._resistantBalance(), 0); + pcvDeposit2.deposit(); + assertEq(pcvDeposit2._resistantBalance(), 1e18); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 1e18, false, true); + assertEq(pcvDeposit1._resistantBalance(), 100e18); + assertEq(pcvDeposit2._resistantBalance(), 0); + + vm.stopPrank(); + } + + // should withdraw and pause after + // should withdraw and not pause after + function testPauseAfter() public { + vm.startPrank(addresses.guardianAddress); + + assertEq(pcvDeposit1.paused(), false); + assertEq(pcvDeposit2.paused(), false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); + assertEq(pcvDeposit1.paused(), false); + assertEq(pcvDeposit2.paused(), false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, true, true); + assertEq(pcvDeposit1.paused(), true); + assertEq(pcvDeposit2.paused(), false); + + pcvDeposit1.unpause(); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 2e18, false, true); + + vm.stopPrank(); + } + + // should withdraw and unpause before + // should withdraw, and pause after + // should keep paused state if deposit was paused before withdraw + function testUnpauseBefore() public { + vm.startPrank(addresses.guardianAddress); + + pcvDeposit1.pause(); + assertEq(pcvDeposit1.paused(), true); + assertEq(pcvDeposit2.paused(), false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); + assertEq(pcvDeposit1.paused(), true); + assertEq(pcvDeposit2.paused(), false); + pcvDeposit1.unpause(); + assertEq(pcvDeposit1.paused(), false); + assertEq(pcvDeposit2.paused(), false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, true, true); + assertEq(pcvDeposit1.paused(), true); + assertEq(pcvDeposit2.paused(), false); + + pcvDeposit1.unpause(); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 2e18, false, true); + + vm.stopPrank(); + } + + // should revert if trying to depositAfter on a paused safe address + function testRevertPausedAfter() public { + vm.startPrank(addresses.guardianAddress); + pcvDeposit2.pause(); + vm.expectRevert("Pausable: paused"); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); + pcvDeposit2.unpause(); + vm.stopPrank(); + } +} diff --git a/contracts/test/utils/Fixtures.sol b/contracts/test/utils/Fixtures.sol index 8c76563ca..b44d60501 100644 --- a/contracts/test/utils/Fixtures.sol +++ b/contracts/test/utils/Fixtures.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.4; import {Core} from "../../core/Core.sol"; +import {TribeRoles} from "../../core/TribeRoles.sol"; import {Vm} from "./Vm.sol"; struct FeiTestAddresses { @@ -16,6 +17,8 @@ struct FeiTestAddresses { address minterAddress; address burnerAddress; address guardianAddress; + address pcvGuardianAdminAddress; + address pcvSafeMoverAddress; } /// @dev Get a list of addresses @@ -31,7 +34,9 @@ function getAddresses() pure returns (FeiTestAddresses memory) { pcvControllerAddress: address(0x8), minterAddress: address(0x9), burnerAddress: address(0x10), - guardianAddress: address(0x11) + guardianAddress: address(0x11), + pcvGuardianAdminAddress: address(0x12), + pcvSafeMoverAddress: address(0x13) }); return addresses; @@ -53,6 +58,11 @@ function getCore() returns (Core) { core.grantPCVController(addresses.pcvControllerAddress); core.grantGuardian(addresses.guardianAddress); + core.createRole(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GOVERNOR); + core.grantRole(TribeRoles.PCV_GUARDIAN_ADMIN, addresses.pcvGuardianAdminAddress); + core.createRole(TribeRoles.PCV_SAFE_MOVER_ROLE, TribeRoles.GOVERNOR); + core.grantRole(TribeRoles.PCV_SAFE_MOVER_ROLE, addresses.pcvSafeMoverAddress); + vm.stopPrank(); return core; } diff --git a/test/unit/pcv/PCVGuardian.test.ts b/test/unit/pcv/PCVGuardian.test.ts deleted file mode 100644 index e626ddf71..000000000 --- a/test/unit/pcv/PCVGuardian.test.ts +++ /dev/null @@ -1,269 +0,0 @@ -import { - Core, - MockERC20, - MockERC20__factory, - MockPCVDepositV2__factory, - PCVDeposit, - PCVGuardian -} from '@custom-types/contracts'; -import { expectRevert, getAddresses, getCore, getImpersonatedSigner } from '@test/helpers'; -import { forceEth } from '@test/integration/setup/utils'; -import chai, { expect } from 'chai'; -import { Signer } from 'ethers'; -import { ethers } from 'hardhat'; - -// This will theoretically make the error stack actually print! -chai.config.includeStack = true; - -// Import if needed, just a helper. -// const toBN = ethers.BigNumber.from; - -describe('PCV Guardian', function () { - // variable decs for vars that you want to use in multiple tests - // typeing contracts specifically to what kind they are will catch before you run them! - let core: Core; - let pcvGuardianWithoutStartingAddresses: PCVGuardian; - let pcvGuardianWithStartingAddresses: PCVGuardian; - - let userAddress: string; - let userAddress2: string; - let pcvControllerAddress: string; - let governorAddress: string; - let guardianAddress: string; - - const impersonatedSigners: { [key: string]: Signer } = {}; - - before(async () => { - // add any addresses that you want to get here - const addresses = await getAddresses(); - - userAddress = addresses.userAddress; - userAddress2 = addresses.secondUserAddress; - pcvControllerAddress = addresses.pcvControllerAddress; - governorAddress = addresses.governorAddress; - guardianAddress = addresses.guardianAddress; - - // add any addresses you want to impersonate here - const impersonatedAddresses = [userAddress, pcvControllerAddress, governorAddress, guardianAddress]; - - for (const address of impersonatedAddresses) { - impersonatedSigners[address] = await getImpersonatedSigner(address); - } - }); - - beforeEach(async () => { - // If the forked-network state needs to be reset between each test, run this - // await network.provider.request({method: 'hardhat_reset', params: []}); - - // Do any pre-test setup here - core = await getCore(); - - const pcvGuardianFactory = await ethers.getContractFactory('PCVGuardian'); - - pcvGuardianWithoutStartingAddresses = await pcvGuardianFactory.deploy(core.address, []); - pcvGuardianWithStartingAddresses = await pcvGuardianFactory.deploy(core.address, [userAddress, userAddress2]); - - await pcvGuardianWithoutStartingAddresses.deployTransaction.wait(); - await pcvGuardianWithStartingAddresses.deployTransaction.wait(); - - // To deploy a contract, import and use the contract factory specific to that contract - // note that the signer supplied is optional - }); - - // Try and do as much deployment in beforeEach, and as much testing in the actual functions - - describe('initial conditions', async () => { - it('should have no safe addresses upon deployment when deployed with no safe addresses', async () => { - expect(await pcvGuardianWithoutStartingAddresses.getSafeAddresses()).to.be.empty; - }); - - it('should have safe addresses upon deployment when deployed with safe addresses', async () => { - expect(await pcvGuardianWithStartingAddresses.getSafeAddresses()).not.to.be.empty; - }); - }); - - describe('access control', async () => { - it('should revert when calling setSafeAddress & setSafeAddresses from a non-governor address', async () => { - await expect(pcvGuardianWithoutStartingAddresses.setSafeAddress(userAddress)).to.be.revertedWith('UNAUTHORIZED'); - await expect( - pcvGuardianWithoutStartingAddresses.setSafeAddresses([userAddress, userAddress2]) - ).to.be.revertedWith('UNAUTHORIZED'); - }); - - it('should revert when calling unsetSafeAddress & unsetSafeAddresses from a non-guardian-or-governor-or-admin address', async () => { - await expect(pcvGuardianWithoutStartingAddresses.unsetSafeAddress(userAddress)).to.be.revertedWith( - 'UNAUTHORIZED' - ); - - await expect( - pcvGuardianWithoutStartingAddresses.unsetSafeAddresses([userAddress, userAddress2]) - ).to.be.revertedWith('UNAUTHORIZED'); - }); - - it('should revert when calling withdrawToSafeAddress from a non-guardian-or-governor-or-admin address', async () => { - await expect( - pcvGuardianWithoutStartingAddresses.withdrawToSafeAddress(userAddress, userAddress, 1, false, false) - ).to.be.revertedWith('UNAUTHORIZED'); - }); - - it('should revert when calling withdrawETHToSafeAddress from a non-guardian-or-governor-or-admin address', async () => { - await expect( - pcvGuardianWithoutStartingAddresses.withdrawETHToSafeAddress(userAddress, userAddress, 1, false, false) - ).to.be.revertedWith('UNAUTHORIZED'); - }); - - it('should revert when calling withdrawERC20ToSafeAddress from a non-guardian-or-governor-or-admin address', async () => { - await expect( - pcvGuardianWithoutStartingAddresses.withdrawERC20ToSafeAddress( - userAddress, - userAddress, - userAddress, - 1, - false, - false - ) - ).to.be.revertedWith('UNAUTHORIZED'); - }); - - it('should allow the governor to add a safe address', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[governorAddress]) - .setSafeAddress(userAddress); - expect(await pcvGuardianWithoutStartingAddresses.isSafeAddress(userAddress)).to.be.true; - }); - - it("can't set an already safe address", async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[governorAddress]) - .setSafeAddress(userAddress); - expect(await pcvGuardianWithoutStartingAddresses.isSafeAddress(userAddress)).to.be.true; - - await expectRevert( - pcvGuardianWithoutStartingAddresses.connect(impersonatedSigners[governorAddress]).setSafeAddress(userAddress), - 'set' - ); - }); - - it("can't unset an already unsafe address", async () => { - await expectRevert( - pcvGuardianWithoutStartingAddresses.connect(impersonatedSigners[governorAddress]).unsetSafeAddress(userAddress), - 'unset' - ); - }); - - it('should allow the guardian to remove a safe address', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[governorAddress]) - .setSafeAddress(userAddress); - expect(await pcvGuardianWithoutStartingAddresses.isSafeAddress(userAddress)).to.be.true; - - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .unsetSafeAddress(userAddress); - expect(await pcvGuardianWithoutStartingAddresses.isSafeAddress(userAddress)).to.be.false; - }); - }); - - describe('withdrawals', async () => { - let token: MockERC20; - let tokenPCVDeposit: PCVDeposit; - let tokenPCVDeposit2: PCVDeposit; - - beforeEach(async () => { - const tokenFactory = new MockERC20__factory(impersonatedSigners[userAddress]); - const pcvDepositFactory = new MockPCVDepositV2__factory(impersonatedSigners[userAddress]); - - token = await tokenFactory.deploy(); - tokenPCVDeposit = await pcvDepositFactory.deploy(core.address, token.address, 1, 0); - tokenPCVDeposit2 = await pcvDepositFactory.deploy(core.address, token.address, 1, 0); - - await token.mint(tokenPCVDeposit.address, 100); - await forceEth(tokenPCVDeposit.address); - - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[governorAddress]) - .setSafeAddress(userAddress); - await core - .connect(impersonatedSigners[governorAddress]) - .grantPCVController(pcvGuardianWithoutStartingAddresses.address); - await core - .connect(impersonatedSigners[governorAddress]) - .grantGuardian(pcvGuardianWithoutStartingAddresses.address); - }); - - it('should not be able to withdraw to a non-safe address', async () => { - await expect( - pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawToSafeAddress(token.address, token.address, 1, false, false) - ).to.be.revertedWith('Provided address is not a safe address!'); - }); - - it('should withdraw from a token-pcv deposit when called by the guardian', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawToSafeAddress(tokenPCVDeposit.address, userAddress, 1, false, false); - expect(await token.balanceOf(userAddress)).to.eq(1); - }); - - it('should withdraw from a token-pcv deposit when called by the governor', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[governorAddress]) - .withdrawToSafeAddress(tokenPCVDeposit.address, userAddress, 1, false, false); - expect(await token.balanceOf(userAddress)).to.eq(1); - }); - - it('should withdraw from a token-pcv deposit and deposit after', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[governorAddress]) - .setSafeAddress(tokenPCVDeposit2.address); - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawToSafeAddress(tokenPCVDeposit.address, tokenPCVDeposit2.address, 1, false, true); - expect(await token.balanceOf(tokenPCVDeposit2.address)).to.eq(1); - }); - - it('should withdrawETH from a pcv deposit when called by the guardian', async () => { - const balanceBefore = await ethers.provider.getBalance(userAddress); - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawETHToSafeAddress(tokenPCVDeposit.address, userAddress, 1, false, false); - const balanceAfter = await ethers.provider.getBalance(userAddress); - - expect(balanceAfter.sub(balanceBefore)).to.eq(1); - }); - - it('should withdrawERC20 from a pcv deposit when called by the guardian', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawERC20ToSafeAddress(tokenPCVDeposit.address, userAddress, token.address, 1, false, false); - expect(await token.balanceOf(userAddress)).to.eq(1); - }); - - it('should withdraw and unpause beforehand', async () => { - await tokenPCVDeposit.connect(impersonatedSigners[guardianAddress]).pause(); - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawToSafeAddress(tokenPCVDeposit.address, userAddress, 1, false, false); - expect(await token.balanceOf(userAddress)).to.eq(1); - expect(await tokenPCVDeposit.paused()).to.be.false; - }); - - it('should withdraw and pause after', async () => { - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawToSafeAddress(tokenPCVDeposit.address, userAddress, 1, true, false); - expect(await token.balanceOf(userAddress)).to.eq(1); - expect(await tokenPCVDeposit.paused()).to.be.true; - }); - - it('should withdraw, unpause before, and pause after', async () => { - await tokenPCVDeposit.connect(impersonatedSigners[guardianAddress]).pause(); - await pcvGuardianWithoutStartingAddresses - .connect(impersonatedSigners[guardianAddress]) - .withdrawToSafeAddress(tokenPCVDeposit.address, userAddress, 1, true, false); - expect(await token.balanceOf(userAddress)).to.eq(1); - expect(await tokenPCVDeposit.paused()).to.be.true; - }); - }); -}); From 531415d6289e48e0b1e6f7150e154d3eeddd503c Mon Sep 17 00:00:00 2001 From: Erwan Beauvois Date: Tue, 5 Jul 2022 17:26:13 +0200 Subject: [PATCH 05/12] tom's review --- contracts/pcv/IPCVGuardian.sol | 9 +- contracts/pcv/PCVGuardian.sol | 2 +- contracts/test/unit/PCVGuardian.sol | 159 +++++++++++++++------------- contracts/test/utils/Fixtures.sol | 12 +-- 4 files changed, 98 insertions(+), 84 deletions(-) diff --git a/contracts/pcv/IPCVGuardian.sol b/contracts/pcv/IPCVGuardian.sol index a3fbc8bef..3e1c838f9 100644 --- a/contracts/pcv/IPCVGuardian.sol +++ b/contracts/pcv/IPCVGuardian.sol @@ -64,7 +64,8 @@ interface IPCVGuardian { bool depositAfter ) external; - /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it. + /// The amount withdrawn is expressed as a ratio, basis points are given & not absolute value. /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw @@ -92,7 +93,8 @@ interface IPCVGuardian { bool depositAfter ) external; - /// @notice withdraw funds from a pcv deposit, by calling the withdrawETH() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdrawETH() method on it. + /// The amount withdrawn is expressed as a ratio, basis points are given & not absolute value. /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw @@ -122,7 +124,8 @@ interface IPCVGuardian { bool depositAfter ) external; - /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it + /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it. + /// The amount withdrawn is expressed as a ratio, basis points are given & not absolute value. /// @param pcvDeposit the deposit to pull funds from /// @param safeAddress the destination address to withdraw to /// @param token the token to withdraw diff --git a/contracts/pcv/PCVGuardian.sol b/contracts/pcv/PCVGuardian.sol index ae46978c9..051bd6b21 100644 --- a/contracts/pcv/PCVGuardian.sol +++ b/contracts/pcv/PCVGuardian.sol @@ -99,7 +99,7 @@ contract PCVGuardian is IPCVGuardian, CoreRef { bool depositAfter ) { { - // scoped in this modifier to prevent stack to deep errors & enforce consitent acl + // scoped in this modifier to prevent stack to deep errors & enforce consistent acl ICore _core = core(); require( _core.hasRole(TribeRoles.GUARDIAN, msg.sender) || diff --git a/contracts/test/unit/PCVGuardian.sol b/contracts/test/unit/PCVGuardian.sol index 93c43546e..602fa0e8d 100644 --- a/contracts/test/unit/PCVGuardian.sol +++ b/contracts/test/unit/PCVGuardian.sol @@ -54,7 +54,7 @@ contract PCVGuardianTest is DSTest { } // should have no safe addresses upon deployment when deployed with no safe addresses - function testDeploy1() public { + function testDeployWithoutSafeAddresses() public { address[] memory safeAddresses = new address[](0); PCVGuardian pcvGuardianWithoutAddresses = new PCVGuardian(address(core), safeAddresses); address[] memory getSafeAddresses = pcvGuardianWithoutAddresses.getSafeAddresses(); @@ -62,7 +62,7 @@ contract PCVGuardianTest is DSTest { } // should have safe addresses upon deployment when deployed with safe addresses - function testDeploy2() public { + function testDeployWithSafeAddresses() public { address[] memory safeAddresses = new address[](1); safeAddresses[0] = address(0x42); PCVGuardian pcvGuardianWithoutAddresses = new PCVGuardian(address(core), safeAddresses); @@ -71,11 +71,11 @@ contract PCVGuardianTest is DSTest { assertEq(getSafeAddresses[0], address(0x42)); } - // access control and state changes + ///////////// ACCESS CONTROL AND STATE CHANGES ////////// // should revert when calling setSafeAddress & setSafeAddresses from a non-privileged address // should revert when calling unsetSafeAddress & unsetSafeAddresses from a non-privileged address - function testAccessControl1() public { + function testAccessControlSetUnsetSafeAddressesUnprivilegedCaller() public { address safeAddress = address(0x42); address[] memory safeAddresses = new address[](1); safeAddresses[0] = safeAddress; @@ -97,14 +97,14 @@ contract PCVGuardianTest is DSTest { // should allow GUARDIAN // should allow PCV_GUARDIAN_ADMIN // should allow GOVERNOR - function testAccessControl2() public { + function testAccessControlSetUnsetSafeAddressesPrivilegedCallers() public { address safeAddress = address(0x42); address[] memory safeAddresses = new address[](1); safeAddresses[0] = safeAddress; // none of the following calls shall revert if access // control is properly checked for the various callers - vm.startPrank(addresses.pcvGuardianAdminAddress); + vm.startPrank(addresses.pcvGuardianAdmin); pcvGuardian.setSafeAddress(safeAddress); pcvGuardian.unsetSafeAddress(safeAddress); pcvGuardian.setSafeAddresses(safeAddresses); @@ -118,11 +118,11 @@ contract PCVGuardianTest is DSTest { pcvGuardian.unsetSafeAddresses(safeAddresses); vm.stopPrank(); - vm.prank(addresses.pcvGuardianAdminAddress); + vm.prank(addresses.pcvGuardianAdmin); pcvGuardian.setSafeAddress(safeAddress); vm.prank(addresses.guardianAddress); pcvGuardian.unsetSafeAddress(safeAddress); - vm.prank(addresses.pcvGuardianAdminAddress); + vm.prank(addresses.pcvGuardianAdmin); pcvGuardian.setSafeAddresses(safeAddresses); vm.prank(addresses.guardianAddress); pcvGuardian.unsetSafeAddresses(safeAddresses); @@ -134,7 +134,7 @@ contract PCVGuardianTest is DSTest { // should revert when calling withdrawRatioToSafeAddress from a non-privileged address // should revert when calling withdrawETHRatioToSafeAddress from a non-privileged address // should revert when calling withdrawERC20RatioToSafeAddress from a non-privileged address - function testAccessControl3() public { + function testAccessControlWithdrawalsUnprivilegedCaller() public { vm.expectRevert("UNAUTHORIZED"); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, false); vm.expectRevert("UNAUTHORIZED"); @@ -166,7 +166,7 @@ contract PCVGuardianTest is DSTest { // should allow GOVERNOR to perform withdrawals // should allow PCV_SAFE_MOVER_ROLE to perform withdrawals // should allow GUARDIAN to perform withdrawals - function testAccessControl4() public { + function testAccessControlWithdrawalsPrivilegedCallers() public { // none of the following calls shall revert if access // control is properly checked for the various callers vm.startPrank(addresses.governorAddress); @@ -192,7 +192,7 @@ contract PCVGuardianTest is DSTest { ); vm.stopPrank(); - vm.startPrank(addresses.pcvSafeMoverAddress); + vm.startPrank(addresses.pcvSafeMover); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); pcvGuardian.withdrawERC20ToSafeAddress( @@ -261,14 +261,32 @@ contract PCVGuardianTest is DSTest { } // check state changes & getters after set & unset - function testStateConsistency1() public { + function testIsSafeAddress() public { + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); + + vm.prank(addresses.governorAddress); + pcvGuardian.setSafeAddress(address(pcvDeposit3)); + + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), true); + + vm.prank(addresses.governorAddress); + pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); + + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); + assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); + } + + // check state changes & getters after set & unset + function testGetSafeAddresses() public { address[] memory getSafeAddresses = pcvGuardian.getSafeAddresses(); assertEq(getSafeAddresses.length, 2); assertEq(getSafeAddresses[0], address(pcvDeposit1)); assertEq(getSafeAddresses[1], address(pcvDeposit2)); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); vm.prank(addresses.governorAddress); pcvGuardian.setSafeAddress(address(pcvDeposit3)); @@ -278,9 +296,6 @@ contract PCVGuardianTest is DSTest { assertEq(getSafeAddresses[0], address(pcvDeposit1)); assertEq(getSafeAddresses[1], address(pcvDeposit2)); assertEq(getSafeAddresses[2], address(pcvDeposit3)); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), true); vm.prank(addresses.governorAddress); pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); @@ -289,29 +304,23 @@ contract PCVGuardianTest is DSTest { assertEq(getSafeAddresses.length, 2); assertEq(getSafeAddresses[0], address(pcvDeposit1)); assertEq(getSafeAddresses[1], address(pcvDeposit2)); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit1)), true); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit2)), true); - assertEq(pcvGuardian.isSafeAddress(address(pcvDeposit3)), false); } // can't set an already safe address - // can't unset an already unsafe address - function testStateConsistency2() public { - address safeAddress = address(0x42); - vm.startPrank(addresses.governorAddress); - - vm.expectRevert("PCVGuardian: not a safe address"); - pcvGuardian.unsetSafeAddress(safeAddress); - - pcvGuardian.setSafeAddress(safeAddress); - + function testSetAlreadySafeAddress() public { + vm.prank(addresses.governorAddress); vm.expectRevert("PCVGuardian: already a safe address"); - pcvGuardian.setSafeAddress(safeAddress); + pcvGuardian.setSafeAddress(address(pcvDeposit1)); + } - vm.stopPrank(); + // can't unset an already unsafe address + function testUnsetUnsafeAddress() public { + vm.prank(addresses.governorAddress); + vm.expectRevert("PCVGuardian: not a safe address"); + pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); } - // withdrawals + ///////////// WITHDRAWALS ////////// // should not be able to withdraw to a non-safe address function testOnlyWithdrawableToSafeAdress() public { @@ -319,7 +328,7 @@ contract PCVGuardianTest is DSTest { vm.expectRevert("PCVGuardian: address not whitelisted"); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); - vm.prank(addresses.pcvGuardianAdminAddress); + vm.prank(addresses.pcvGuardianAdmin); pcvGuardian.setSafeAddress(address(pcvDeposit3)); vm.startPrank(addresses.guardianAddress); @@ -332,24 +341,20 @@ contract PCVGuardianTest is DSTest { } // should withdraw from a pcv deposit - // should withdrawETH from a pcv deposit - // should withdrawERC20 from a pcv deposit - // should withdrawRatio from a pcv deposit - // should withdrawETHRatio from a pcv deposit - // should withdrawERC20Ratio from a pcv deposit - function testWithdrawals() public { - // initial state - vm.startPrank(addresses.pcvSafeMoverAddress); + function testWithdrawToSafeAddress() public { assertEq(pcvDeposit1.balance(), 100e18); assertEq(pcvDeposit2.balance(), 0); - assertEq(address(pcvDeposit1).balance, 1 ether); - assertEq(address(pcvDeposit2).balance, 0); - - // withdrawals + vm.prank(addresses.pcvSafeMover); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); assertEq(pcvDeposit1.balance(), 99e18); assertEq(pcvDeposit2.balance(), 1e18); + } + // should withdrawETH from a pcv deposit + function testWithdrawETHToSafeAddress() public { + assertEq(address(pcvDeposit1).balance, 1 ether); + assertEq(address(pcvDeposit2).balance, 0); + vm.prank(addresses.pcvSafeMover); pcvGuardian.withdrawETHToSafeAddress( address(pcvDeposit1), payable(address(pcvDeposit2)), @@ -359,7 +364,13 @@ contract PCVGuardianTest is DSTest { ); assertEq(address(pcvDeposit1).balance, 0.9 ether); assertEq(address(pcvDeposit2).balance, 0.1 ether); + } + // should withdrawERC20 from a pcv deposit + function testWithdrawERC20ToSafeAddress() public { + assertEq(pcvDeposit1.balance(), 100e18); + assertEq(pcvDeposit2.balance(), 0); + vm.prank(addresses.pcvSafeMover); pcvGuardian.withdrawERC20ToSafeAddress( address(pcvDeposit1), address(pcvDeposit2), @@ -368,51 +379,51 @@ contract PCVGuardianTest is DSTest { false, true ); - assertEq(pcvDeposit1.balance(), 98e18); - assertEq(pcvDeposit2.balance(), 2e18); + assertEq(pcvDeposit1.balance(), 99e18); + assertEq(pcvDeposit2.balance(), 1e18); + } - // ratio withdrawals - pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 5000, false, true); - assertEq(pcvDeposit1.balance(), 49e18); - assertEq(pcvDeposit2.balance(), 51e18); + // should withdrawRatio from a pcv deposit + function testWithdrawRatioToSafeAddress() public { + assertEq(pcvDeposit1.balance(), 100e18); + assertEq(pcvDeposit2.balance(), 0); + vm.prank(addresses.pcvSafeMover); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 5500, false, true); + assertEq(pcvDeposit1.balance(), 45e18); + assertEq(pcvDeposit2.balance(), 55e18); + } + // should withdrawETHRatio from a pcv deposit + function testWithdrawETHRatioToSafeAddress() public { + assertEq(address(pcvDeposit1).balance, 1 ether); + assertEq(address(pcvDeposit2).balance, 0); + vm.prank(addresses.pcvSafeMover); pcvGuardian.withdrawETHRatioToSafeAddress( address(pcvDeposit1), payable(address(pcvDeposit2)), - 5000, + 5500, false, false ); assertEq(address(pcvDeposit1).balance, 0.45 ether); assertEq(address(pcvDeposit2).balance, 0.55 ether); + } + // should withdrawERC20Ratio from a pcv deposit + function testWithdrawERC20RatioToSafeAddress() public { + assertEq(pcvDeposit1.balance(), 100e18); + assertEq(pcvDeposit2.balance(), 0); + vm.prank(addresses.pcvSafeMover); pcvGuardian.withdrawERC20RatioToSafeAddress( address(pcvDeposit1), address(pcvDeposit2), address(token), - 5000, + 6700, false, true ); - assertEq(pcvDeposit1.balance(), 24.5e18); - assertEq(pcvDeposit2.balance(), 75.5e18); - - // transfer back all tokens & eth - pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 10000, false, true); - pcvGuardian.withdrawETHRatioToSafeAddress( - address(pcvDeposit2), - payable(address(pcvDeposit1)), - 10000, - false, - false - ); - vm.stopPrank(); - - // end state = initial state - assertEq(pcvDeposit1.balance(), 100e18); - assertEq(pcvDeposit2.balance(), 0); - assertEq(address(pcvDeposit1).balance, 1 ether); - assertEq(address(pcvDeposit2).balance, 0); + assertEq(pcvDeposit1.balance(), 33e18); + assertEq(pcvDeposit2.balance(), 67e18); } // should withdraw and deposit after diff --git a/contracts/test/utils/Fixtures.sol b/contracts/test/utils/Fixtures.sol index b44d60501..3f28502cd 100644 --- a/contracts/test/utils/Fixtures.sol +++ b/contracts/test/utils/Fixtures.sol @@ -17,8 +17,8 @@ struct FeiTestAddresses { address minterAddress; address burnerAddress; address guardianAddress; - address pcvGuardianAdminAddress; - address pcvSafeMoverAddress; + address pcvGuardianAdmin; + address pcvSafeMover; } /// @dev Get a list of addresses @@ -35,8 +35,8 @@ function getAddresses() pure returns (FeiTestAddresses memory) { minterAddress: address(0x9), burnerAddress: address(0x10), guardianAddress: address(0x11), - pcvGuardianAdminAddress: address(0x12), - pcvSafeMoverAddress: address(0x13) + pcvGuardianAdmin: address(0x12), + pcvSafeMover: address(0x13) }); return addresses; @@ -59,9 +59,9 @@ function getCore() returns (Core) { core.grantGuardian(addresses.guardianAddress); core.createRole(TribeRoles.PCV_GUARDIAN_ADMIN, TribeRoles.GOVERNOR); - core.grantRole(TribeRoles.PCV_GUARDIAN_ADMIN, addresses.pcvGuardianAdminAddress); + core.grantRole(TribeRoles.PCV_GUARDIAN_ADMIN, addresses.pcvGuardianAdmin); core.createRole(TribeRoles.PCV_SAFE_MOVER_ROLE, TribeRoles.GOVERNOR); - core.grantRole(TribeRoles.PCV_SAFE_MOVER_ROLE, addresses.pcvSafeMoverAddress); + core.grantRole(TribeRoles.PCV_SAFE_MOVER_ROLE, addresses.pcvSafeMover); vm.stopPrank(); return core; From 9428149ca6d8cf2c5cd8e76983ce1267f2606fe5 Mon Sep 17 00:00:00 2001 From: Erwan Beauvois Date: Thu, 7 Jul 2022 18:02:25 +0200 Subject: [PATCH 06/12] PCVGuardian v3: remove pauseAfter param --- contracts/pcv/IPCVGuardian.sol | 12 -- contracts/pcv/PCVGuardian.sol | 29 +-- .../sentinel/guards/FuseWithdrawalGuard.sol | 2 +- .../integration/fixtures/MainnetAddresses.sol | 1 - .../sentinel/FuseWithdrawalGuard.t.sol | 6 +- contracts/test/unit/PCVGuardian.sol | 171 +++++------------- proposals/dao/pcv_guardian_v3.ts | 132 ++++++++++++++ proposals/description/pcv_guardian_v3.ts | 53 ++++++ protocol-configuration/mainnetAddresses.ts | 28 ++- protocol-configuration/proposalsConfig.ts | 12 +- test/integration/tests/pcv.ts | 22 +-- 11 files changed, 273 insertions(+), 195 deletions(-) create mode 100644 proposals/dao/pcv_guardian_v3.ts create mode 100644 proposals/description/pcv_guardian_v3.ts diff --git a/contracts/pcv/IPCVGuardian.sol b/contracts/pcv/IPCVGuardian.sol index 3e1c838f9..ee2ba6403 100644 --- a/contracts/pcv/IPCVGuardian.sol +++ b/contracts/pcv/IPCVGuardian.sol @@ -54,13 +54,11 @@ interface IPCVGuardian { /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param amount the amount to withdraw - /// @param pauseAfter if true, the pcv contract will be paused after the withdraw /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawToSafeAddress( address pcvDeposit, address safeAddress, uint256 amount, - bool pauseAfter, bool depositAfter ) external; @@ -69,13 +67,11 @@ interface IPCVGuardian { /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw - /// @param pauseAfter if true, the pcv contract will be paused after the withdraw /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawRatioToSafeAddress( address pcvDeposit, address safeAddress, uint256 basisPoints, - bool pauseAfter, bool depositAfter ) external; @@ -83,13 +79,11 @@ interface IPCVGuardian { /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param amount the amount of tokens to withdraw - /// @param pauseAfter if true, the pcv contract will be paused after the withdraw /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawETHToSafeAddress( address pcvDeposit, address payable safeAddress, uint256 amount, - bool pauseAfter, bool depositAfter ) external; @@ -98,13 +92,11 @@ interface IPCVGuardian { /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw - /// @param pauseAfter if true, the pcv contract will be paused after the withdraw /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawETHRatioToSafeAddress( address pcvDeposit, address payable safeAddress, uint256 basisPoints, - bool pauseAfter, bool depositAfter ) external; @@ -113,14 +105,12 @@ interface IPCVGuardian { /// @param safeAddress the destination address to withdraw to /// @param token the token to withdraw /// @param amount the amount of funds to withdraw - /// @param pauseAfter whether to pause the pcv after withdrawing /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawERC20ToSafeAddress( address pcvDeposit, address safeAddress, address token, uint256 amount, - bool pauseAfter, bool depositAfter ) external; @@ -130,14 +120,12 @@ interface IPCVGuardian { /// @param safeAddress the destination address to withdraw to /// @param token the token to withdraw /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw - /// @param pauseAfter whether to pause the pcv after withdrawing /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawERC20RatioToSafeAddress( address pcvDeposit, address safeAddress, address token, uint256 basisPoints, - bool pauseAfter, bool depositAfter ) external; } diff --git a/contracts/pcv/PCVGuardian.sol b/contracts/pcv/PCVGuardian.sol index 051bd6b21..69e6d20d4 100644 --- a/contracts/pcv/PCVGuardian.sol +++ b/contracts/pcv/PCVGuardian.sol @@ -90,12 +90,11 @@ contract PCVGuardian is IPCVGuardian, CoreRef { /// @notice modifier to factorize the logic in all withdrawals : /// - first, ensure the deposit to withdraw from is unpaused /// - second, perform withdrawal - /// - third, re-pause deposit if it was paused or if pauseAfter = true + /// - third, re-pause deposit if it was paused /// - finally, call pcvDeposit.deposit() if depositAfter = true modifier beforeAndAfterWithdrawal( address pcvDeposit, address safeAddress, - bool pauseAfter, bool depositAfter ) { { @@ -117,7 +116,7 @@ contract PCVGuardian is IPCVGuardian, CoreRef { _; - if (paused || pauseAfter) { + if (paused) { pcvDeposit._pause(); } @@ -130,15 +129,13 @@ contract PCVGuardian is IPCVGuardian, CoreRef { /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param amount the amount to withdraw - /// @param pauseAfter if true, the pcv contract will be paused after the withdraw /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawToSafeAddress( address pcvDeposit, address safeAddress, uint256 amount, - bool pauseAfter, bool depositAfter - ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, depositAfter) { IPCVDeposit(pcvDeposit).withdraw(safeAddress, amount); emit PCVGuardianWithdrawal(pcvDeposit, safeAddress, amount); } @@ -147,15 +144,13 @@ contract PCVGuardian is IPCVGuardian, CoreRef { /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw - /// @param pauseAfter if true, the pcv contract will be paused after the withdraw /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawRatioToSafeAddress( address pcvDeposit, address safeAddress, uint256 basisPoints, - bool pauseAfter, bool depositAfter - ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, depositAfter) { require(basisPoints <= Constants.BASIS_POINTS_GRANULARITY, "PCVGuardian: basisPoints too high"); uint256 amount = (IPCVDeposit(pcvDeposit).balance() * basisPoints) / Constants.BASIS_POINTS_GRANULARITY; require(amount != 0, "PCVGuardian: no value to withdraw"); @@ -168,15 +163,13 @@ contract PCVGuardian is IPCVGuardian, CoreRef { /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param amount the amount of tokens to withdraw - /// @param pauseAfter if true, the pcv contract will be paused after the withdraw /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawETHToSafeAddress( address pcvDeposit, address payable safeAddress, uint256 amount, - bool pauseAfter, bool depositAfter - ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, depositAfter) { IPCVDeposit(pcvDeposit).withdrawETH(safeAddress, amount); emit PCVGuardianETHWithdrawal(pcvDeposit, safeAddress, amount); } @@ -185,15 +178,13 @@ contract PCVGuardian is IPCVGuardian, CoreRef { /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw - /// @param pauseAfter if true, the pcv contract will be paused after the withdraw /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawETHRatioToSafeAddress( address pcvDeposit, address payable safeAddress, uint256 basisPoints, - bool pauseAfter, bool depositAfter - ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, depositAfter) { require(basisPoints <= Constants.BASIS_POINTS_GRANULARITY, "PCVGuardian: basisPoints too high"); uint256 amount = (pcvDeposit.balance * basisPoints) / Constants.BASIS_POINTS_GRANULARITY; require(amount != 0, "PCVGuardian: no value to withdraw"); @@ -206,16 +197,14 @@ contract PCVGuardian is IPCVGuardian, CoreRef { /// @param pcvDeposit the deposit to pull funds from /// @param safeAddress the destination address to withdraw to /// @param amount the amount of funds to withdraw - /// @param pauseAfter whether to pause the pcv after withdrawing /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawERC20ToSafeAddress( address pcvDeposit, address safeAddress, address token, uint256 amount, - bool pauseAfter, bool depositAfter - ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, depositAfter) { IPCVDeposit(pcvDeposit).withdrawERC20(token, safeAddress, amount); emit PCVGuardianERC20Withdrawal(pcvDeposit, safeAddress, token, amount); } @@ -224,16 +213,14 @@ contract PCVGuardian is IPCVGuardian, CoreRef { /// @param pcvDeposit the deposit to pull funds from /// @param safeAddress the destination address to withdraw to /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw - /// @param pauseAfter whether to pause the pcv after withdrawing /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawERC20RatioToSafeAddress( address pcvDeposit, address safeAddress, address token, uint256 basisPoints, - bool pauseAfter, bool depositAfter - ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, pauseAfter, depositAfter) { + ) external override beforeAndAfterWithdrawal(pcvDeposit, safeAddress, depositAfter) { require(basisPoints <= Constants.BASIS_POINTS_GRANULARITY, "PCVGuardian: basisPoints too high"); uint256 amount = (IERC20(token).balanceOf(pcvDeposit) * basisPoints) / Constants.BASIS_POINTS_GRANULARITY; require(amount != 0, "PCVGuardian: no value to withdraw"); diff --git a/contracts/sentinel/guards/FuseWithdrawalGuard.sol b/contracts/sentinel/guards/FuseWithdrawalGuard.sol index 29d93847f..7c4b4a480 100644 --- a/contracts/sentinel/guards/FuseWithdrawalGuard.sol +++ b/contracts/sentinel/guards/FuseWithdrawalGuard.sol @@ -22,7 +22,7 @@ contract FuseWithdrawalGuard is IGuard, CoreRef { EnumerableSet.AddressSet private fuseDeposits; /// @notice the PCV mover contract exposed to guardian role - PCVGuardian public constant pcvGuardian = PCVGuardian(0x02435948F84d7465FB71dE45ABa6098Fc6eC2993); + PCVGuardian public constant pcvGuardian = PCVGuardian(0x0000000000000000000000000000000000000000); // TODO update /// @notice the minimum amount of underlying which can be withdrawn from a cToken that registers in the guard. /// i.e. if the min is 100 FEI but the amount in the contract is 1 FEI, the amountToWithdraw will return 0 and the check will fail diff --git a/contracts/test/integration/fixtures/MainnetAddresses.sol b/contracts/test/integration/fixtures/MainnetAddresses.sol index a936ca5d7..091098dbf 100644 --- a/contracts/test/integration/fixtures/MainnetAddresses.sol +++ b/contracts/test/integration/fixtures/MainnetAddresses.sol @@ -28,7 +28,6 @@ library MainnetAddresses { address public constant LUSD_PSM = 0xb0e731F036AdfDeC12da77c15aaB0F90E8e45A0e; address public constant DAI_PSM = 0x2A188F9EB761F70ECEa083bA6c2A40145078dfc2; - address public constant PCV_GUARDIAN_NEW = 0x02435948F84d7465FB71dE45ABa6098Fc6eC2993; address payable public constant PCV_SENTINEL = payable(0xC297705Acf50134d256187c754B92FA37826C019); address public constant LUSD = 0x5f98805A4E8be255a32880FDeC7F6728C6568bA0; diff --git a/contracts/test/integration/sentinel/FuseWithdrawalGuard.t.sol b/contracts/test/integration/sentinel/FuseWithdrawalGuard.t.sol index 514f0286f..63a24ee94 100644 --- a/contracts/test/integration/sentinel/FuseWithdrawalGuard.t.sol +++ b/contracts/test/integration/sentinel/FuseWithdrawalGuard.t.sol @@ -54,7 +54,9 @@ contract FuseWithdrawalGuardIntegrationTest is DSTest, StdLib { sentinel.knight(address(guard)); } - function testGuardCanProtec() public { + // TODO: update Fuse withdrawal guard PCV_GUARDIAN address, + // and then uncomment this integration test file + /*function testGuardCanProtec() public { // 1. Check preconditions of FEI pool 8 withdraw assertTrue(guard.check()); uint256 feiCTokenBalanceBefore = fei.balanceOf(MainnetAddresses.RARI_POOL_8_FEI); @@ -97,5 +99,5 @@ contract FuseWithdrawalGuardIntegrationTest is DSTest, StdLib { // 6. Check no more withdrawals assertFalse(guard.check()); - } + }*/ } diff --git a/contracts/test/unit/PCVGuardian.sol b/contracts/test/unit/PCVGuardian.sol index 602fa0e8d..231e0ffbb 100644 --- a/contracts/test/unit/PCVGuardian.sol +++ b/contracts/test/unit/PCVGuardian.sol @@ -136,29 +136,21 @@ contract PCVGuardianTest is DSTest { // should revert when calling withdrawERC20RatioToSafeAddress from a non-privileged address function testAccessControlWithdrawalsUnprivilegedCaller() public { vm.expectRevert("UNAUTHORIZED"); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false); vm.expectRevert("UNAUTHORIZED"); - pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1e18, false, false); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1e18, false); vm.expectRevert("UNAUTHORIZED"); - pcvGuardian.withdrawERC20ToSafeAddress( - address(pcvDeposit1), - address(pcvDeposit2), - address(token), - 1e18, - false, - false - ); + pcvGuardian.withdrawERC20ToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), address(token), 1e18, false); vm.expectRevert("UNAUTHORIZED"); - pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false); vm.expectRevert("UNAUTHORIZED"); - pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false); vm.expectRevert("UNAUTHORIZED"); pcvGuardian.withdrawERC20RatioToSafeAddress( address(pcvDeposit1), address(pcvDeposit2), address(token), 1, - false, false ); } @@ -170,89 +162,58 @@ contract PCVGuardianTest is DSTest { // none of the following calls shall revert if access // control is properly checked for the various callers vm.startPrank(addresses.governorAddress); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); - pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); - pcvGuardian.withdrawERC20ToSafeAddress( - address(pcvDeposit1), - address(pcvDeposit2), - address(token), - 1, - false, - false - ); - pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); - pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false); + pcvGuardian.withdrawERC20ToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), address(token), 1, false); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false); pcvGuardian.withdrawERC20RatioToSafeAddress( address(pcvDeposit1), address(pcvDeposit2), address(token), 1, - false, false ); vm.stopPrank(); vm.startPrank(addresses.pcvSafeMover); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); - pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); - pcvGuardian.withdrawERC20ToSafeAddress( - address(pcvDeposit1), - address(pcvDeposit2), - address(token), - 1, - false, - false - ); - pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); - pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false); + pcvGuardian.withdrawERC20ToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), address(token), 1, false); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false); pcvGuardian.withdrawERC20RatioToSafeAddress( address(pcvDeposit1), address(pcvDeposit2), address(token), 1, - false, false ); vm.stopPrank(); vm.startPrank(addresses.guardianAddress); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); - pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); - pcvGuardian.withdrawERC20ToSafeAddress( - address(pcvDeposit1), - address(pcvDeposit2), - address(token), - 1, - false, - false - ); - pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false, false); - pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false, false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false); + pcvGuardian.withdrawERC20ToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), address(token), 1, false); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false); pcvGuardian.withdrawERC20RatioToSafeAddress( address(pcvDeposit1), address(pcvDeposit2), address(token), 1, - false, false ); vm.stopPrank(); // move back all tokens & eth vm.startPrank(addresses.governorAddress); - pcvGuardian.withdrawETHRatioToSafeAddress( - address(pcvDeposit2), - payable(address(pcvDeposit1)), - 10000, - false, - false - ); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit2), payable(address(pcvDeposit1)), 10000, false); pcvGuardian.withdrawERC20RatioToSafeAddress( address(pcvDeposit2), address(pcvDeposit1), address(token), 10000, - false, false ); pcvDeposit1.deposit(); @@ -326,17 +287,17 @@ contract PCVGuardianTest is DSTest { function testOnlyWithdrawableToSafeAdress() public { vm.prank(addresses.guardianAddress); vm.expectRevert("PCVGuardian: address not whitelisted"); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, true); vm.prank(addresses.pcvGuardianAdmin); pcvGuardian.setSafeAddress(address(pcvDeposit3)); vm.startPrank(addresses.guardianAddress); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit3), address(pcvDeposit1), 1, false, true); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, true); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit3), address(pcvDeposit1), 1, true); pcvGuardian.unsetSafeAddress(address(pcvDeposit3)); vm.expectRevert("PCVGuardian: address not whitelisted"); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, false, true); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit3), 1, true); vm.stopPrank(); } @@ -345,7 +306,7 @@ contract PCVGuardianTest is DSTest { assertEq(pcvDeposit1.balance(), 100e18); assertEq(pcvDeposit2.balance(), 0); vm.prank(addresses.pcvSafeMover); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, true); assertEq(pcvDeposit1.balance(), 99e18); assertEq(pcvDeposit2.balance(), 1e18); } @@ -355,13 +316,7 @@ contract PCVGuardianTest is DSTest { assertEq(address(pcvDeposit1).balance, 1 ether); assertEq(address(pcvDeposit2).balance, 0); vm.prank(addresses.pcvSafeMover); - pcvGuardian.withdrawETHToSafeAddress( - address(pcvDeposit1), - payable(address(pcvDeposit2)), - 0.1 ether, - false, - false - ); + pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 0.1 ether, false); assertEq(address(pcvDeposit1).balance, 0.9 ether); assertEq(address(pcvDeposit2).balance, 0.1 ether); } @@ -371,14 +326,7 @@ contract PCVGuardianTest is DSTest { assertEq(pcvDeposit1.balance(), 100e18); assertEq(pcvDeposit2.balance(), 0); vm.prank(addresses.pcvSafeMover); - pcvGuardian.withdrawERC20ToSafeAddress( - address(pcvDeposit1), - address(pcvDeposit2), - address(token), - 1e18, - false, - true - ); + pcvGuardian.withdrawERC20ToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), address(token), 1e18, true); assertEq(pcvDeposit1.balance(), 99e18); assertEq(pcvDeposit2.balance(), 1e18); } @@ -388,7 +336,7 @@ contract PCVGuardianTest is DSTest { assertEq(pcvDeposit1.balance(), 100e18); assertEq(pcvDeposit2.balance(), 0); vm.prank(addresses.pcvSafeMover); - pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 5500, false, true); + pcvGuardian.withdrawRatioToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 5500, true); assertEq(pcvDeposit1.balance(), 45e18); assertEq(pcvDeposit2.balance(), 55e18); } @@ -398,13 +346,7 @@ contract PCVGuardianTest is DSTest { assertEq(address(pcvDeposit1).balance, 1 ether); assertEq(address(pcvDeposit2).balance, 0); vm.prank(addresses.pcvSafeMover); - pcvGuardian.withdrawETHRatioToSafeAddress( - address(pcvDeposit1), - payable(address(pcvDeposit2)), - 5500, - false, - false - ); + pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 5500, false); assertEq(address(pcvDeposit1).balance, 0.45 ether); assertEq(address(pcvDeposit2).balance, 0.55 ether); } @@ -419,7 +361,6 @@ contract PCVGuardianTest is DSTest { address(pcvDeposit2), address(token), 6700, - false, true ); assertEq(pcvDeposit1.balance(), 33e18); @@ -433,70 +374,48 @@ contract PCVGuardianTest is DSTest { assertEq(pcvDeposit1._resistantBalance(), 100e18); assertEq(pcvDeposit2._resistantBalance(), 0); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, false); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false); assertEq(pcvDeposit1._resistantBalance(), 99e18); assertEq(pcvDeposit2._resistantBalance(), 0); pcvDeposit2.deposit(); assertEq(pcvDeposit2._resistantBalance(), 1e18); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 1e18, false, true); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 1e18, true); assertEq(pcvDeposit1._resistantBalance(), 100e18); assertEq(pcvDeposit2._resistantBalance(), 0); vm.stopPrank(); } - // should withdraw and pause after - // should withdraw and not pause after - function testPauseAfter() public { + // should revert if trying to depositAfter on a paused safe address + function testRevertPausedDepositAfter() public { vm.startPrank(addresses.guardianAddress); - - assertEq(pcvDeposit1.paused(), false); - assertEq(pcvDeposit2.paused(), false); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); - assertEq(pcvDeposit1.paused(), false); - assertEq(pcvDeposit2.paused(), false); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, true, true); - assertEq(pcvDeposit1.paused(), true); - assertEq(pcvDeposit2.paused(), false); - - pcvDeposit1.unpause(); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 2e18, false, true); - + pcvDeposit2.pause(); + vm.expectRevert("Pausable: paused"); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, true); + pcvDeposit2.unpause(); vm.stopPrank(); } - // should withdraw and unpause before - // should withdraw, and pause after - // should keep paused state if deposit was paused before withdraw - function testUnpauseBefore() public { + // should withdraw and unpause before, if the source deposit is paused, + // then re-pause it after withdrawal if it was paused + function testKeepPausedState() public { vm.startPrank(addresses.guardianAddress); pcvDeposit1.pause(); assertEq(pcvDeposit1.paused(), true); assertEq(pcvDeposit2.paused(), false); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, true); assertEq(pcvDeposit1.paused(), true); assertEq(pcvDeposit2.paused(), false); pcvDeposit1.unpause(); assertEq(pcvDeposit1.paused(), false); assertEq(pcvDeposit2.paused(), false); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, true, true); - assertEq(pcvDeposit1.paused(), true); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, true); + assertEq(pcvDeposit1.paused(), false); assertEq(pcvDeposit2.paused(), false); - pcvDeposit1.unpause(); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 2e18, false, true); + pcvGuardian.withdrawToSafeAddress(address(pcvDeposit2), address(pcvDeposit1), 2e18, true); vm.stopPrank(); } - - // should revert if trying to depositAfter on a paused safe address - function testRevertPausedAfter() public { - vm.startPrank(addresses.guardianAddress); - pcvDeposit2.pause(); - vm.expectRevert("Pausable: paused"); - pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1e18, false, true); - pcvDeposit2.unpause(); - vm.stopPrank(); - } } diff --git a/proposals/dao/pcv_guardian_v3.ts b/proposals/dao/pcv_guardian_v3.ts new file mode 100644 index 000000000..5bdfb4665 --- /dev/null +++ b/proposals/dao/pcv_guardian_v3.ts @@ -0,0 +1,132 @@ +import { ethers } from 'hardhat'; +import { expect } from 'chai'; +import { + DeployUpgradeFunc, + NamedAddresses, + SetupUpgradeFunc, + TeardownUpgradeFunc, + ValidateUpgradeFunc, + PcvStats +} from '@custom-types/types'; +import { getImpersonatedSigner, overwriteChainlinkAggregator, time, expectRevert } from '@test/helpers'; +import { forceEth } from '@test/integration/setup/utils'; +import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'; +import { BigNumber, Contract } from 'ethers'; + +let pcvStatsBefore: PcvStats; +let ethPrice8Decimals: string; +const e18 = ethers.constants.WeiPerEther; + +const fipNumber = 'pcv_guardian_v3'; + +// Do any deployments +// This should exclusively include new contract deployments +const deploy: DeployUpgradeFunc = async (deployAddress: string, addresses: NamedAddresses, logging: boolean) => { + // fetch currently safe addresses on the PCV Guardian v2 + const pcvGuardianV2 = new ethers.Contract( + addresses.pcvGuardianV2, + ['function getSafeAddresses() public view returns (address[])'], + ethers.provider.getSigner(addresses.pcvGuardianV2) + ); + const safeAddresses = await pcvGuardianV2.getSafeAddresses(); + + console.log('safeAddresses', safeAddresses); + // Deploy new PCV Guardian v3 + const pcvGuardianFactory = await ethers.getContractFactory('PCVGuardian'); + const pcvGuardian = await pcvGuardianFactory.deploy(addresses.core, []); + await pcvGuardian.deployed(); + logging && console.log(`pcvGuardian: ${pcvGuardian.address}`); + + const fuseWithdrawalGuardFactory = await ethers.getContractFactory('FuseWithdrawalGuard'); + const fuseWithdrawalGuard = await fuseWithdrawalGuardFactory.deploy( + addresses.core, + [ + addresses.rariPool8FeiPCVDeposit, + addresses.rariPool8DaiPCVDeposit, + addresses.rariPool8LusdPCVDeposit, + addresses.rariPool79FeiPCVDeposit, + addresses.rariPool128FeiPCVDeposit, + addresses.rariPool22FeiPCVDeposit, + addresses.rariPool24FeiPCVDeposit, + addresses.rariPool18FeiPCVDeposit, + addresses.rariPool6FeiPCVDeposit, + addresses.turboFusePCVDeposit + ], + [ + addresses.daiFixedPricePSM, + addresses.daiFixedPricePSM, + addresses.lusdHoldingPCVDeposit, + addresses.daiFixedPricePSM, + addresses.daiFixedPricePSM, + addresses.daiFixedPricePSM, + addresses.daiFixedPricePSM, + addresses.daiFixedPricePSM, + addresses.daiFixedPricePSM, + addresses.daiFixedPricePSM + ], + [ + addresses.fei, + addresses.dai, + addresses.lusd, + addresses.fei, + addresses.fei, + addresses.fei, + addresses.fei, + addresses.fei, + addresses.fei, + addresses.fei + ], + [e18.mul(191_000), e18.mul(64_000), e18.mul(4_000), e18.mul(100_000), 0, 0, 0, 0, 0, 0] + ); + await fuseWithdrawalGuard.deployed(); + logging && console.log(`fuseWithdrawalGuard: ${fuseWithdrawalGuard.address}`); + + return { + pcvGuardian, + fuseWithdrawalGuard + }; +}; + +// Do any setup necessary for running the test. +// This could include setting up Hardhat to impersonate accounts, +// ensuring contracts have a specific state, etc. +const setup: SetupUpgradeFunc = async (addresses, oldContracts, contracts, logging) => { + // make sure ETH oracle is fresh (for B.AMM not to revert, etc) + // Read Chainlink ETHUSD price & override chainlink storage to make it a fresh value + ethPrice8Decimals = Math.round((await contracts.chainlinkEthUsdOracleWrapper.read())[0].toString() / 1e10).toString(); + await overwriteChainlinkAggregator(addresses.chainlinkEthUsdOracle, ethPrice8Decimals, '8'); + + // read pcvStats before proposal execution + pcvStatsBefore = await contracts.collateralizationOracle.pcvStats(); +}; + +// Tears down any changes made in setup() that need to be +// cleaned up before doing any validation checks. +const teardown: TeardownUpgradeFunc = async (addresses, oldContracts, contracts, logging) => { + console.log(`No actions to complete in teardown for fip${fipNumber}`); +}; + +// Run any validations required on the fip using mocha or console logging +// IE check balances, check state of contracts, etc. +const validate: ValidateUpgradeFunc = async (addresses, oldContracts, contracts, logging) => { + // display pcvStats + console.log('----------------------------------------------------'); + console.log(' pcvStatsBefore.protocolControlledValue [M]e18 ', Number(pcvStatsBefore.protocolControlledValue) / 1e24); + console.log(' pcvStatsBefore.userCirculatingFei [M]e18 ', Number(pcvStatsBefore.userCirculatingFei) / 1e24); + console.log(' pcvStatsBefore.protocolEquity [M]e18 ', Number(pcvStatsBefore.protocolEquity) / 1e24); + const pcvStatsAfter: PcvStats = await contracts.collateralizationOracle.pcvStats(); + console.log('----------------------------------------------------'); + console.log(' pcvStatsAfter.protocolControlledValue [M]e18 ', Number(pcvStatsAfter.protocolControlledValue) / 1e24); + console.log(' pcvStatsAfter.userCirculatingFei [M]e18 ', Number(pcvStatsAfter.userCirculatingFei) / 1e24); + console.log(' pcvStatsAfter.protocolEquity [M]e18 ', Number(pcvStatsAfter.protocolEquity) / 1e24); + console.log('----------------------------------------------------'); + const pcvDiff: BigNumber = pcvStatsAfter.protocolControlledValue.sub(pcvStatsBefore.protocolControlledValue); + const cFeiDiff: BigNumber = pcvStatsAfter.userCirculatingFei.sub(pcvStatsBefore.userCirculatingFei); + const eqDiff: BigNumber = pcvStatsAfter.protocolEquity.sub(pcvStatsBefore.protocolEquity); + console.log(' PCV diff [M]e18 ', Number(pcvDiff) / 1e24); + console.log(' Circ FEI diff [M]e18 ', Number(cFeiDiff) / 1e24); + console.log(' Equity diff [M]e18 ', Number(eqDiff) / 1e24); + console.log('----------------------------------------------------'); +}; + +export { deploy, setup, teardown, validate }; diff --git a/proposals/description/pcv_guardian_v3.ts b/proposals/description/pcv_guardian_v3.ts new file mode 100644 index 000000000..7041a7083 --- /dev/null +++ b/proposals/description/pcv_guardian_v3.ts @@ -0,0 +1,53 @@ +import { ethers } from 'hardhat'; +import { TemplatedProposalDescription } from '@custom-types/types'; + +const proposal: TemplatedProposalDescription = { + title: 'TIP-XYZ: PCV Guardian v3', + commands: [ + { + target: 'core', + values: '0', + method: 'revokeRole(bytes32,address)', + arguments: (addresses) => [ethers.utils.id('PCV_CONTROLLER_ROLE'), addresses.pcvGuardianV2], + description: 'Revoke PCV_CONTROLLER_ROLE from pcvGuardianV2' + }, + { + target: 'core', + values: '0', + method: 'revokeRole(bytes32,address)', + arguments: (addresses) => [ethers.utils.id('GUARDIAN_ROLE'), addresses.pcvGuardianV2], + description: 'Revoke GUARDIAN_ROLE from pcvGuardianV2' + }, + { + target: 'core', + values: '0', + method: 'grantRole(bytes32,address)', + arguments: (addresses) => [ethers.utils.id('PCV_CONTROLLER_ROLE'), addresses.pcvGuardian], + description: 'Grant PCV_CONTROLLER_ROLE from pcvGuardianV3' + }, + { + target: 'core', + values: '0', + method: 'grantRole(bytes32,address)', + arguments: (addresses) => [ethers.utils.id('GUARDIAN_ROLE'), addresses.pcvGuardian], + description: 'Grant GUARDIAN_ROLE from pcvGuardianV3' + }, + { + target: 'pcvSentinel', + values: '0', + method: 'slay(address)', + arguments: (addresses) => [addresses.fuseWithdrawalGuardV1], + description: 'Slay old fuseWithdrawalGuardV1 guard' + }, + { + target: 'pcvSentinel', + values: '0', + method: 'knight(address)', + arguments: (addresses) => [addresses.fuseWithdrawalGuard], + description: 'Knight new fuseWithdrawalGuard guard' + } + ], + description: `TODO` +}; + +export default proposal; diff --git a/protocol-configuration/mainnetAddresses.ts b/protocol-configuration/mainnetAddresses.ts index 5dca65301..31b9a0458 100644 --- a/protocol-configuration/mainnetAddresses.ts +++ b/protocol-configuration/mainnetAddresses.ts @@ -36,21 +36,36 @@ const MainnetContractsConfig: MainnetContractsConfig = { address: '0xC297705Acf50134d256187c754B92FA37826C019', category: AddressCategory.Security }, - pcvGuardian: { + pcvGuardianV1: { + artifactName: 'PCVGuardian', + address: '0x2D1b1b509B6432A73e3d798572f0648f6453a5D9', + category: AddressCategory.Deprecated + }, + pcvGuardianV2: { artifactName: 'PCVGuardian', address: '0x02435948F84d7465FB71dE45ABa6098Fc6eC2993', - category: AddressCategory.Security + category: AddressCategory.Deprecated }, + //pcvGuardian: { // V3 + // artifactName: 'PCVGuardian', + // address: '...', + // category: AddressCategory.Security + //}, guardianMultisig: { artifactName: 'unknown', address: '0xB8f482539F2d3Ae2C9ea6076894df36D1f632775', category: AddressCategory.Security }, - fuseWithdrawalGuard: { + fuseWithdrawalGuardV1: { artifactName: 'FuseWithdrawalGuard', address: '0xd079ec4b442600a381eAc7E95662eB1b313cd113', - category: AddressCategory.Security + category: AddressCategory.Deprecated }, + //fuseWithdrawalGuard: { + // artifactName: 'FuseWithdrawalGuard', + // address: '...', + // category: AddressCategory.Security + //}, ratioPCVControllerV2: { artifactName: 'RatioPCVControllerV2', address: '0x221fff24FB66dA3c722c7C5B856956a6a30C0179', @@ -1851,11 +1866,6 @@ const MainnetContractsConfig: MainnetContractsConfig = { address: '0x1fa69a416bcf8572577d3949b742fbb0a9cd98c7', category: AddressCategory.Deprecated }, - pcvGuardianV1: { - artifactName: 'PCVGuardian', - address: '0x2D1b1b509B6432A73e3d798572f0648f6453a5D9', - category: AddressCategory.Deprecated - }, daiPSM: { artifactName: 'PriceBoundPSM', address: '0x210300C158f95E1342fD008aE417ef68311c49C2', diff --git a/protocol-configuration/proposalsConfig.ts b/protocol-configuration/proposalsConfig.ts index 327c5f5cd..63d279039 100644 --- a/protocol-configuration/proposalsConfig.ts +++ b/protocol-configuration/proposalsConfig.ts @@ -1,8 +1,7 @@ import { ProposalCategory, TemplatedProposalDescription, TemplatedProposalsConfigMap } from '@custom-types/types'; -import fip_x from '@proposals/description/fip_x'; - import tip_118 from '@proposals/description/tip_118'; +import pcv_guardian_v3 from '@proposals/description/pcv_guardian_v3'; const proposals: TemplatedProposalsConfigMap = { tip_118: { @@ -13,6 +12,15 @@ const proposals: TemplatedProposalsConfigMap = { affectedContractSignoff: [], deprecatedContractSignoff: [], category: ProposalCategory.DAO + }, + pcv_guardian_v3: { + deploy: true, + totalValue: 0, + proposal: pcv_guardian_v3, + proposalId: '', + affectedContractSignoff: [], + deprecatedContractSignoff: [], + category: ProposalCategory.DEBUG } }; diff --git a/test/integration/tests/pcv.ts b/test/integration/tests/pcv.ts index 0e7319322..8fbc83b00 100644 --- a/test/integration/tests/pcv.ts +++ b/test/integration/tests/pcv.ts @@ -80,7 +80,7 @@ describe('e2e-pcv', function () { }); describe('PCV Guardian', async () => { - it('can withdraw PCV and pause', async () => { + it('can withdraw PCV', async () => { const pcvGuardian = contracts.pcvGuardian; const amount = await contracts.compoundEthPCVDeposit.balance(); @@ -88,31 +88,11 @@ describe('e2e-pcv', function () { contractAddresses.compoundEthPCVDeposit, contractAddresses.wethHoldingPCVDeposit, amount, - false, true ); expect(await ethers.provider.getBalance(contractAddresses.aaveEthPCVDeposit)).to.be.bignumber.equal(toBN(0)); }); - - it('can withdraw PCV and pause', async () => { - const pcvGuardian = contracts.pcvGuardian; - - const feiBalanceBefore = await contracts.fei.balanceOf(contractAddresses.feiDAOTimelock); - await pcvGuardian.withdrawToSafeAddress( - contractAddresses.rariPool8FeiPCVDeposit, - contractAddresses.feiDAOTimelock, - ethers.constants.WeiPerEther, - true, - false - ); - - const feiBalanceAfter = await contracts.fei.balanceOf(contractAddresses.feiDAOTimelock); - - expect(await contracts.rariPool8FeiPCVDeposit.paused()).to.be.true; - expect(feiBalanceAfter.sub(feiBalanceBefore)).to.be.bignumber.equal(ethers.constants.WeiPerEther); - await contracts.rariPool8FeiPCVDeposit.unpause(); - }); }); describe('Drip Controller', async () => { From 5508ba79be482794a99759787c90486f19824239 Mon Sep 17 00:00:00 2001 From: Erwan Beauvois Date: Fri, 8 Jul 2022 12:16:07 +0200 Subject: [PATCH 07/12] reorder proposals --- proposals/dao/pcv_guardian_v3.ts | 3 +-- protocol-configuration/proposalsConfig.ts | 18 +++++++++--------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/proposals/dao/pcv_guardian_v3.ts b/proposals/dao/pcv_guardian_v3.ts index 5bdfb4665..879aac322 100644 --- a/proposals/dao/pcv_guardian_v3.ts +++ b/proposals/dao/pcv_guardian_v3.ts @@ -30,10 +30,9 @@ const deploy: DeployUpgradeFunc = async (deployAddress: string, addresses: Named ); const safeAddresses = await pcvGuardianV2.getSafeAddresses(); - console.log('safeAddresses', safeAddresses); // Deploy new PCV Guardian v3 const pcvGuardianFactory = await ethers.getContractFactory('PCVGuardian'); - const pcvGuardian = await pcvGuardianFactory.deploy(addresses.core, []); + const pcvGuardian = await pcvGuardianFactory.deploy(addresses.core, safeAddresses); await pcvGuardian.deployed(); logging && console.log(`pcvGuardian: ${pcvGuardian.address}`); diff --git a/protocol-configuration/proposalsConfig.ts b/protocol-configuration/proposalsConfig.ts index 63d279039..f4cae3d34 100644 --- a/protocol-configuration/proposalsConfig.ts +++ b/protocol-configuration/proposalsConfig.ts @@ -4,15 +4,6 @@ import tip_118 from '@proposals/description/tip_118'; import pcv_guardian_v3 from '@proposals/description/pcv_guardian_v3'; const proposals: TemplatedProposalsConfigMap = { - tip_118: { - deploy: false, // deploy flag for whether to run deploy action during e2e tests or use mainnet state - totalValue: 0, // amount of ETH to send to DAO execution - proposal: tip_118, // full proposal file, imported from '@proposals/description/fip_xx.ts' - proposalId: '', - affectedContractSignoff: [], - deprecatedContractSignoff: [], - category: ProposalCategory.DAO - }, pcv_guardian_v3: { deploy: true, totalValue: 0, @@ -21,6 +12,15 @@ const proposals: TemplatedProposalsConfigMap = { affectedContractSignoff: [], deprecatedContractSignoff: [], category: ProposalCategory.DEBUG + }, + tip_118: { + deploy: false, // deploy flag for whether to run deploy action during e2e tests or use mainnet state + totalValue: 0, // amount of ETH to send to DAO execution + proposal: tip_118, // full proposal file, imported from '@proposals/description/fip_xx.ts' + proposalId: '', + affectedContractSignoff: [], + deprecatedContractSignoff: [], + category: ProposalCategory.DAO } }; From fef615cf2bd9dd88fbaa57b95d690c721730da8e Mon Sep 17 00:00:00 2001 From: Erwan Beauvois Date: Mon, 11 Jul 2022 15:07:22 +0200 Subject: [PATCH 08/12] finish resolving merge conflicts --- contracts/pcv/IPCVGuardian.sol | 17 ----------------- contracts/pcv/PCVGuardian.sol | 2 -- .../test/unit/ERC20HoldingPCVDeposit.t.sol | 8 ++++---- contracts/test/unit/PCVGuardian.sol | 6 +++--- 4 files changed, 7 insertions(+), 26 deletions(-) diff --git a/contracts/pcv/IPCVGuardian.sol b/contracts/pcv/IPCVGuardian.sol index 51edf5b0e..ee2ba6403 100644 --- a/contracts/pcv/IPCVGuardian.sol +++ b/contracts/pcv/IPCVGuardian.sol @@ -128,21 +128,4 @@ interface IPCVGuardian { uint256 basisPoints, bool depositAfter ) external; - - /// @notice withdraw funds from a pcv deposit, by calling the withdrawERC20() method on it. - /// The amount withdrawn is expressed as a ratio, basis points are given & not absolute value. - /// @param pcvDeposit the deposit to pull funds from - /// @param safeAddress the destination address to withdraw to - /// @param token the token to withdraw - /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw - /// @param pauseAfter whether to pause the pcv after withdrawing - /// @param depositAfter if true, attempts to deposit to the target PCV deposit - function withdrawERC20RatioToSafeAddress( - address pcvDeposit, - address safeAddress, - address token, - uint256 basisPoints, - bool pauseAfter, - bool depositAfter - ) external; } diff --git a/contracts/pcv/PCVGuardian.sol b/contracts/pcv/PCVGuardian.sol index a5c0c7887..69e6d20d4 100644 --- a/contracts/pcv/PCVGuardian.sol +++ b/contracts/pcv/PCVGuardian.sol @@ -178,7 +178,6 @@ contract PCVGuardian is IPCVGuardian, CoreRef { /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw - /// @param pauseAfter if true, the pcv contract will be paused after the withdraw /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawETHRatioToSafeAddress( address pcvDeposit, @@ -214,7 +213,6 @@ contract PCVGuardian is IPCVGuardian, CoreRef { /// @param pcvDeposit the deposit to pull funds from /// @param safeAddress the destination address to withdraw to /// @param basisPoints the percent in basis points [1-10000] if the deposit's balance to withdraw - /// @param pauseAfter whether to pause the pcv after withdrawing /// @param depositAfter if true, attempts to deposit to the target PCV deposit function withdrawERC20RatioToSafeAddress( address pcvDeposit, diff --git a/contracts/test/unit/ERC20HoldingPCVDeposit.t.sol b/contracts/test/unit/ERC20HoldingPCVDeposit.t.sol index 308831f2b..bf64007d3 100644 --- a/contracts/test/unit/ERC20HoldingPCVDeposit.t.sol +++ b/contracts/test/unit/ERC20HoldingPCVDeposit.t.sol @@ -66,7 +66,7 @@ contract ERC20HoldingPCVDepositTest is DSTest { function testWithdraw() public { erc20.transfer(address(emptyDeposit), 10); - vm.prank(addresses.pcvControllerAddress); + vm.prank(addresses.pcvController); emptyDeposit.withdraw(receiver, 5); assertEq(erc20.balanceOf(address(emptyDeposit)), 5); @@ -77,7 +77,7 @@ contract ERC20HoldingPCVDepositTest is DSTest { function testCanWithdrawERC20() public { erc20.transfer(address(emptyDeposit), 10); - vm.prank(addresses.pcvControllerAddress); + vm.prank(addresses.pcvController); emptyDeposit.withdrawERC20(address(erc20), receiver, 5); assertEq(erc20.balanceOf(address(emptyDeposit)), 5); @@ -88,7 +88,7 @@ contract ERC20HoldingPCVDepositTest is DSTest { function testCanWithdraw() public { erc20.transfer(address(emptyDeposit), 10); - vm.prank(addresses.pcvControllerAddress); + vm.prank(addresses.pcvController); emptyDeposit.withdraw(receiver, 10); assertEq(erc20.balanceOf(address(emptyDeposit)), 0); @@ -100,7 +100,7 @@ contract ERC20HoldingPCVDepositTest is DSTest { function testCanWithdrawEth() public { payable(address(emptyDeposit)).transfer(1 ether); - vm.prank(addresses.pcvControllerAddress); + vm.prank(addresses.pcvController); emptyDeposit.withdrawETH(receiver, 1 ether); assertEq(address(receiver).balance, 1 ether); diff --git a/contracts/test/unit/PCVGuardian.sol b/contracts/test/unit/PCVGuardian.sol index edde99734..02e49f915 100644 --- a/contracts/test/unit/PCVGuardian.sol +++ b/contracts/test/unit/PCVGuardian.sol @@ -161,7 +161,7 @@ contract PCVGuardianTest is DSTest { function testAccessControlWithdrawalsPrivilegedCallers() public { // none of the following calls shall revert if access // control is properly checked for the various callers - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false); pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false); pcvGuardian.withdrawERC20ToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), address(token), 1, false); @@ -191,7 +191,7 @@ contract PCVGuardianTest is DSTest { ); vm.stopPrank(); - vm.startPrank(addresses.guardianAddress); + vm.startPrank(addresses.guardian); pcvGuardian.withdrawToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), 1, false); pcvGuardian.withdrawETHToSafeAddress(address(pcvDeposit1), payable(address(pcvDeposit2)), 1, false); pcvGuardian.withdrawERC20ToSafeAddress(address(pcvDeposit1), address(pcvDeposit2), address(token), 1, false); @@ -207,7 +207,7 @@ contract PCVGuardianTest is DSTest { vm.stopPrank(); // move back all tokens & eth - vm.startPrank(addresses.governorAddress); + vm.startPrank(addresses.governor); pcvGuardian.withdrawETHRatioToSafeAddress(address(pcvDeposit2), payable(address(pcvDeposit1)), 10000, false); pcvGuardian.withdrawERC20RatioToSafeAddress( address(pcvDeposit2), From f59d04b7a80b639582657343544bfa99f63a7b16 Mon Sep 17 00:00:00 2001 From: Erwan Beauvois Date: Wed, 13 Jul 2022 11:34:56 +0200 Subject: [PATCH 09/12] klob's review --- proposals/description/pcv_guardian_v3.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/proposals/description/pcv_guardian_v3.ts b/proposals/description/pcv_guardian_v3.ts index 7041a7083..866d2b152 100644 --- a/proposals/description/pcv_guardian_v3.ts +++ b/proposals/description/pcv_guardian_v3.ts @@ -23,14 +23,14 @@ const proposal: TemplatedProposalDescription = { values: '0', method: 'grantRole(bytes32,address)', arguments: (addresses) => [ethers.utils.id('PCV_CONTROLLER_ROLE'), addresses.pcvGuardian], - description: 'Grant PCV_CONTROLLER_ROLE from pcvGuardianV3' + description: 'Grant PCV_CONTROLLER_ROLE to pcvGuardianV3' }, { target: 'core', values: '0', method: 'grantRole(bytes32,address)', arguments: (addresses) => [ethers.utils.id('GUARDIAN_ROLE'), addresses.pcvGuardian], - description: 'Grant GUARDIAN_ROLE from pcvGuardianV3' + description: 'Grant GUARDIAN_ROLE to pcvGuardianV3' }, { target: 'pcvSentinel', @@ -47,7 +47,10 @@ const proposal: TemplatedProposalDescription = { description: 'Knight new fuseWithdrawalGuard guard' } ], - description: `TODO` + description: `TIP-120: PCV Guardian v3 + +PCV Guardian v3 adds features that allow moving a percentage of the funds held by the PCV Deposits. This is useful for the Tribal Council when executing proposals if the amounts of tokens are not known in advance. This capability was existing for the DAO Timelock, but not for the Tribal Council. This proposal does not allow movements of funds to new addresses and does not increase the responsibilities of any role. This proposal also upgrades the Fuse Withdrawal Guard to use the new PCV Guardian v3 to keep this feature active. +` }; export default proposal; From 8752fb3d679afa2ffdabafab61acc458b436f547 Mon Sep 17 00:00:00 2001 From: Erwan Beauvois Date: Wed, 13 Jul 2022 11:37:33 +0200 Subject: [PATCH 10/12] attempt to make diff more friendly --- contracts/pcv/PCVGuardian.sol | 78 ++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 38 deletions(-) diff --git a/contracts/pcv/PCVGuardian.sol b/contracts/pcv/PCVGuardian.sol index 69e6d20d4..5bb7df52c 100644 --- a/contracts/pcv/PCVGuardian.sol +++ b/contracts/pcv/PCVGuardian.sol @@ -87,44 +87,6 @@ contract PCVGuardian is IPCVGuardian, CoreRef { } } - /// @notice modifier to factorize the logic in all withdrawals : - /// - first, ensure the deposit to withdraw from is unpaused - /// - second, perform withdrawal - /// - third, re-pause deposit if it was paused - /// - finally, call pcvDeposit.deposit() if depositAfter = true - modifier beforeAndAfterWithdrawal( - address pcvDeposit, - address safeAddress, - bool depositAfter - ) { - { - // scoped in this modifier to prevent stack to deep errors & enforce consistent acl - ICore _core = core(); - require( - _core.hasRole(TribeRoles.GUARDIAN, msg.sender) || - _core.hasRole(TribeRoles.PCV_SAFE_MOVER_ROLE, msg.sender) || - _core.hasRole(TribeRoles.GOVERNOR, msg.sender), - "UNAUTHORIZED" - ); - require(isSafeAddress(safeAddress), "PCVGuardian: address not whitelisted"); - } - - bool paused = pcvDeposit._paused(); - if (paused) { - pcvDeposit._unpause(); - } - - _; - - if (paused) { - pcvDeposit._pause(); - } - - if (depositAfter) { - IPCVDeposit(safeAddress).deposit(); - } - } - /// @notice withdraw funds from a pcv deposit, by calling the withdraw() method on it /// @param pcvDeposit the address of the pcv deposit contract /// @param safeAddress the destination address to withdraw to @@ -229,6 +191,46 @@ contract PCVGuardian is IPCVGuardian, CoreRef { emit PCVGuardianERC20Withdrawal(pcvDeposit, safeAddress, token, amount); } + // --------------- Modifiers ------------- + + /// @notice modifier to factorize the logic in all withdrawals : + /// - first, ensure the deposit to withdraw from is unpaused + /// - second, perform withdrawal + /// - third, re-pause deposit if it was paused + /// - finally, call pcvDeposit.deposit() if depositAfter = true + modifier beforeAndAfterWithdrawal( + address pcvDeposit, + address safeAddress, + bool depositAfter + ) { + { + // scoped in this modifier to prevent stack to deep errors & enforce consistent acl + ICore _core = core(); + require( + _core.hasRole(TribeRoles.GUARDIAN, msg.sender) || + _core.hasRole(TribeRoles.PCV_SAFE_MOVER_ROLE, msg.sender) || + _core.hasRole(TribeRoles.GOVERNOR, msg.sender), + "UNAUTHORIZED" + ); + require(isSafeAddress(safeAddress), "PCVGuardian: address not whitelisted"); + } + + bool paused = pcvDeposit._paused(); + if (paused) { + pcvDeposit._unpause(); + } + + _; + + if (paused) { + pcvDeposit._pause(); + } + + if (depositAfter) { + IPCVDeposit(safeAddress).deposit(); + } + } + // ---------- Internal Functions ---------- function _setSafeAddress(address anAddress) internal { From 4787aeb115b125dd3a4e37eab1ab69b0b1661d3d Mon Sep 17 00:00:00 2001 From: Erwan Beauvois Date: Wed, 13 Jul 2022 16:20:10 +0200 Subject: [PATCH 11/12] tom's review --- .../sentinel/guards/FuseWithdrawalGuard.sol | 4 +++- ...sol.disabled => FuseWithdrawalGuard.t.sol} | 19 +++++++++++++++---- proposals/dao/pcv_guardian_v3.ts | 1 + proposals/description/pcv_guardian_v3.ts | 2 +- protocol-configuration/proposalsConfig.ts | 2 +- 5 files changed, 21 insertions(+), 7 deletions(-) rename contracts/test/integration/sentinel/{FuseWithdrawalGuard.t.sol.disabled => FuseWithdrawalGuard.t.sol} (84%) diff --git a/contracts/sentinel/guards/FuseWithdrawalGuard.sol b/contracts/sentinel/guards/FuseWithdrawalGuard.sol index 7c4b4a480..1c3f79ca3 100644 --- a/contracts/sentinel/guards/FuseWithdrawalGuard.sol +++ b/contracts/sentinel/guards/FuseWithdrawalGuard.sol @@ -22,7 +22,7 @@ contract FuseWithdrawalGuard is IGuard, CoreRef { EnumerableSet.AddressSet private fuseDeposits; /// @notice the PCV mover contract exposed to guardian role - PCVGuardian public constant pcvGuardian = PCVGuardian(0x0000000000000000000000000000000000000000); // TODO update + PCVGuardian public immutable pcvGuardian; /// @notice the minimum amount of underlying which can be withdrawn from a cToken that registers in the guard. /// i.e. if the min is 100 FEI but the amount in the contract is 1 FEI, the amountToWithdraw will return 0 and the check will fail @@ -31,6 +31,7 @@ contract FuseWithdrawalGuard is IGuard, CoreRef { constructor( address core, + address pcvGuardianAddress, address[] memory deposits, address[] memory destinations, address[] memory underlyings, @@ -49,6 +50,7 @@ contract FuseWithdrawalGuard is IGuard, CoreRef { ++i; } } + pcvGuardian = PCVGuardian(pcvGuardianAddress); } /// @notice setter for the Fuse deposit destination and minimum liquidity diff --git a/contracts/test/integration/sentinel/FuseWithdrawalGuard.t.sol.disabled b/contracts/test/integration/sentinel/FuseWithdrawalGuard.t.sol similarity index 84% rename from contracts/test/integration/sentinel/FuseWithdrawalGuard.t.sol.disabled rename to contracts/test/integration/sentinel/FuseWithdrawalGuard.t.sol index ce290bd89..06649d2ac 100644 --- a/contracts/test/integration/sentinel/FuseWithdrawalGuard.t.sol.disabled +++ b/contracts/test/integration/sentinel/FuseWithdrawalGuard.t.sol @@ -6,6 +6,8 @@ import {StdLib} from "../../utils/StdLib.sol"; import {Vm} from "../../utils/Vm.sol"; import {MainnetAddresses} from "../fixtures/MainnetAddresses.sol"; +import {Core} from "../../../core/Core.sol"; +import "../../../pcv/PCVGuardian.sol"; import "../../../sentinel/PCVSentinel.sol"; import "../../../sentinel/guards/FuseWithdrawalGuard.sol"; @@ -13,6 +15,7 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; contract FuseWithdrawalGuardIntegrationTest is DSTest, StdLib { PCVSentinel sentinel; + PCVGuardian guardian; FuseWithdrawalGuard guard; IERC20 fei = IERC20(MainnetAddresses.FEI); @@ -21,6 +24,15 @@ contract FuseWithdrawalGuardIntegrationTest is DSTest, StdLib { Vm public constant vm = Vm(HEVM_ADDRESS); function setUp() public { + address[] memory safeAddresses = new address[](2); + safeAddresses[0] = MainnetAddresses.DAI_PSM; + safeAddresses[1] = MainnetAddresses.LUSD_HOLDING_PCV_DEPOSIT; + guardian = new PCVGuardian(MainnetAddresses.CORE, safeAddresses); + vm.prank(MainnetAddresses.FEI_DAO_TIMELOCK); + Core(MainnetAddresses.CORE).grantGuardian(address(guardian)); + vm.prank(MainnetAddresses.FEI_DAO_TIMELOCK); + Core(MainnetAddresses.CORE).grantPCVController(address(guardian)); + sentinel = PCVSentinel(MainnetAddresses.PCV_SENTINEL); uint256 len = 2; @@ -44,6 +56,7 @@ contract FuseWithdrawalGuardIntegrationTest is DSTest, StdLib { guard = new FuseWithdrawalGuard( MainnetAddresses.CORE, + address(guardian), deposits, destinations, underlyings, @@ -54,9 +67,7 @@ contract FuseWithdrawalGuardIntegrationTest is DSTest, StdLib { sentinel.knight(address(guard)); } - // TODO: update Fuse withdrawal guard PCV_GUARDIAN address, - // and then uncomment this integration test file - /*function testGuardCanProtec() public { + function testGuardCanProtec() public { // 1. Check preconditions of FEI pool 8 withdraw assertTrue(guard.check()); uint256 feiCTokenBalanceBefore = fei.balanceOf(MainnetAddresses.RARI_POOL_8_FEI); @@ -102,5 +113,5 @@ contract FuseWithdrawalGuardIntegrationTest is DSTest, StdLib { // 6. Check no more withdrawals assertFalse(guard.check()); - }*/ + } } diff --git a/proposals/dao/pcv_guardian_v3.ts b/proposals/dao/pcv_guardian_v3.ts index 879aac322..c42c5c171 100644 --- a/proposals/dao/pcv_guardian_v3.ts +++ b/proposals/dao/pcv_guardian_v3.ts @@ -39,6 +39,7 @@ const deploy: DeployUpgradeFunc = async (deployAddress: string, addresses: Named const fuseWithdrawalGuardFactory = await ethers.getContractFactory('FuseWithdrawalGuard'); const fuseWithdrawalGuard = await fuseWithdrawalGuardFactory.deploy( addresses.core, + pcvGuardian.address, [ addresses.rariPool8FeiPCVDeposit, addresses.rariPool8DaiPCVDeposit, diff --git a/proposals/description/pcv_guardian_v3.ts b/proposals/description/pcv_guardian_v3.ts index 866d2b152..2dfa1656a 100644 --- a/proposals/description/pcv_guardian_v3.ts +++ b/proposals/description/pcv_guardian_v3.ts @@ -2,7 +2,7 @@ import { ethers } from 'hardhat'; import { TemplatedProposalDescription } from '@custom-types/types'; const proposal: TemplatedProposalDescription = { - title: 'TIP-XYZ: PCV Guardian v3', + title: 'TIP-120: PCV Guardian v3', commands: [ { target: 'core', diff --git a/protocol-configuration/proposalsConfig.ts b/protocol-configuration/proposalsConfig.ts index 8d507efeb..d9a0d40da 100644 --- a/protocol-configuration/proposalsConfig.ts +++ b/protocol-configuration/proposalsConfig.ts @@ -14,7 +14,7 @@ export const ProposalsConfig: TemplatedProposalsConfigMap = { proposalId: '', affectedContractSignoff: [], deprecatedContractSignoff: [], - category: ProposalCategory.DEBUG + category: ProposalCategory.TC }, tip_118: { deploy: false, // deploy flag for whether to run deploy action during e2e tests or use mainnet state From a8ee388afaf1508839757057665900368215723b Mon Sep 17 00:00:00 2001 From: Erwan Beauvois Date: Wed, 13 Jul 2022 17:25:09 +0200 Subject: [PATCH 12/12] Update pcv_guardian_v3 to be executed by DAO --- proposals/dao/pcv_guardian_v3.ts | 16 ++++++++++++++++ proposals/description/pcv_guardian_v3.ts | 10 +++++++++- protocol-configuration/proposalsConfig.ts | 2 +- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/proposals/dao/pcv_guardian_v3.ts b/proposals/dao/pcv_guardian_v3.ts index c42c5c171..ef4993a7a 100644 --- a/proposals/dao/pcv_guardian_v3.ts +++ b/proposals/dao/pcv_guardian_v3.ts @@ -109,6 +109,22 @@ const teardown: TeardownUpgradeFunc = async (addresses, oldContracts, contracts, // Run any validations required on the fip using mocha or console logging // IE check balances, check state of contracts, etc. const validate: ValidateUpgradeFunc = async (addresses, oldContracts, contracts, logging) => { + // validate that the guardian can use the new PCVGuardian to perform a + // ratio PCV movement to a safe address + const guardianSigner = await getImpersonatedSigner(addresses.guardianMultisig); + const balanceBefore = await contracts.dai.balanceOf(addresses.daiHoldingPCVDeposit); + await contracts.pcvGuardian.connect(guardianSigner).withdrawERC20RatioToSafeAddress( + addresses.daiFixedPricePSM, + addresses.daiHoldingPCVDeposit, + addresses.dai, + '5000', // 50% + true // depositAfter + ); + const balanceAfter = await contracts.dai.balanceOf(addresses.daiHoldingPCVDeposit); + expect(balanceBefore).to.be.equal('0'); + // round to unit because there can be 1 wei of error + expect(balanceAfter / 1e18).to.be.equal((await contracts.daiFixedPricePSM.balance()) / 1e18); + // display pcvStats console.log('----------------------------------------------------'); console.log(' pcvStatsBefore.protocolControlledValue [M]e18 ', Number(pcvStatsBefore.protocolControlledValue) / 1e24); diff --git a/proposals/description/pcv_guardian_v3.ts b/proposals/description/pcv_guardian_v3.ts index 2dfa1656a..3c088aaa3 100644 --- a/proposals/description/pcv_guardian_v3.ts +++ b/proposals/description/pcv_guardian_v3.ts @@ -49,7 +49,15 @@ const proposal: TemplatedProposalDescription = { ], description: `TIP-120: PCV Guardian v3 -PCV Guardian v3 adds features that allow moving a percentage of the funds held by the PCV Deposits. This is useful for the Tribal Council when executing proposals if the amounts of tokens are not known in advance. This capability was existing for the DAO Timelock, but not for the Tribal Council. This proposal does not allow movements of funds to new addresses and does not increase the responsibilities of any role. This proposal also upgrades the Fuse Withdrawal Guard to use the new PCV Guardian v3 to keep this feature active. +PCV Guardian v3 adds features that allow moving a percentage of the funds held by the PCV Deposits. + +This is useful for the Tribal Council when executing proposals if the amounts of tokens are not known in advance. + +This capability was existing for the DAO Timelock, but not for the Tribal Council. + +This proposal does not allow movements of funds to new addresses and does not increase the responsibilities of any role. + +This proposal also upgrades the Fuse Withdrawal Guard to use the new PCV Guardian v3. ` }; diff --git a/protocol-configuration/proposalsConfig.ts b/protocol-configuration/proposalsConfig.ts index ed4d59381..8e2f9cf60 100644 --- a/protocol-configuration/proposalsConfig.ts +++ b/protocol-configuration/proposalsConfig.ts @@ -12,7 +12,7 @@ export const ProposalsConfig: TemplatedProposalsConfigMap = { proposalId: '', affectedContractSignoff: [], deprecatedContractSignoff: [], - category: ProposalCategory.TC + category: ProposalCategory.DAO }, tip_119: { deploy: false, // deploy flag for whether to run deploy action during e2e tests or use mainnet state