Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIP-120] DAO Vote: PCVGuardian v3 #952

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions contracts/libs/CoreRefPauseableLib.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
14 changes: 12 additions & 2 deletions contracts/mock/MockPCVDepositV2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
50 changes: 47 additions & 3 deletions contracts/pcv/IPCVGuardian.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
eswak marked this conversation as resolved.
Show resolved Hide resolved
/// @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
Expand All @@ -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
eswak marked this conversation as resolved.
Show resolved Hide resolved
/// @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
Expand All @@ -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;
}
171 changes: 119 additions & 52 deletions contracts/pcv/PCVGuardian.sol
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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)
eswak marked this conversation as resolved.
Show resolved Hide resolved
{
_setSafeAddress(pcvDeposit);
}
Expand All @@ -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]);
}
Expand All @@ -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);
}
Expand All @@ -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(
eswak marked this conversation as resolved.
Show resolved Hide resolved
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
eswak marked this conversation as resolved.
Show resolved Hide resolved
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use ensureUnpaused here?

Copy link
Contributor Author

@eswak eswak Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensureUnpaused reads the pcvDeposit.paused() but does not return it. We need to read the paused state before withdrawal, because after withdrawal we want to re-pause if the deposit was paused in the first place. But yeah essentially it's the same code than what's in the ensureUnpaused function

pcvDeposit._unpause();
}

IPCVDeposit(pcvDeposit).withdraw(safeAddress, amount);
_;

if (pauseAfter) {
if (paused || pauseAfter) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much prefer this also. What we had before had this horrible side effect of changing pause state unintentionally

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
Expand All @@ -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
Expand All @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not need this to withdraw from paused deposits?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do, that's why before withdrawals the modifier reads the paused state & unpauses if the deposit is paused
image


) 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we agreed to leave depositAfter in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we kept depositAfter, the diff is displaying very weird 🤔 this logic is now in the modifier

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);
}
}
Loading