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

Chore: created a diff of the zkSync contract changes #593

Open
wants to merge 1 commit into
base: zksync-testing-review
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
18 changes: 15 additions & 3 deletions packages/contracts/src/zksync/GovernanceERC20Upgradeable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,27 @@ import {ERC20VotesUpgradeable} from "@openzeppelin/contracts-upgradeable/token/E
import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol";
import {IVotesUpgradeable} from "@openzeppelin/contracts-upgradeable/governance/utils/IVotesUpgradeable.sol";
import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";

import {DaoAuthorizableUpgradeable} from "../core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol";
import {IDAO} from "../core/dao/IDAO.sol";
import {DaoAuthorizableUpgradeable} from "../core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol";
import {IERC20MintableUpgradeable} from "../token/ERC20/IERC20MintableUpgradeable.sol";

/// @title GovernanceERC20
/// @title GovernanceERC20Upgradeable
/// @author Aragon Association
/// @notice An [OpenZeppelin `Votes`](https://docs.openzeppelin.com/contracts/4.x/api/governance#Votes) compatible [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token that can be used for voting and is managed by a DAO.
contract GovernanceERC20 is
contract GovernanceERC20Upgradeable is
Copy link
Contributor

Choose a reason for hiding this comment

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

Upgradeable tokens are a red flag. External parties could use official Aragon OSx tokens on zkSync to rugpull consumers.
To add to this, they are often not permitted on exchanges - are we 100% sure that this is acceptable in the case of zkSync Era?

Please rethink this and confirm this decision with leadership @Rekard0.

IERC20MintableUpgradeable,
Initializable,
ERC165Upgradeable,
ERC20VotesUpgradeable,
UUPSUpgradeable,
DaoAuthorizableUpgradeable
{
/// @notice The ID of the permission required to call the `_authorizeUpgrade` function.
bytes32 public constant UPGRADE_GOVERNANCE_ERC20_PERMISSION_ID =
keccak256("UPGRADE_GOVERNANCE_ERC20_PERMISSION");

/// @notice The permission identifier to mint new tokens
bytes32 public constant MINT_PERMISSION_ID = keccak256("MINT_PERMISSION");

Expand Down Expand Up @@ -117,4 +123,10 @@ contract GovernanceERC20 is
_delegate(to, to);
}
}

/// @notice Internal method authorizing the upgrade of the contract via the [upgradeability mechanism for UUPS proxies](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)).
/// @dev The caller must have the `UPGRADE_DAO_PERMISSION_ID` permission.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// @dev The caller must have the `UPGRADE_DAO_PERMISSION_ID` permission.
/// @dev The caller must have the `UPGRADE_GOVERNANCE_ERC20_PERMISSION_ID ` permission.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@novaknole good suggestion

function _authorizeUpgrade(
address
) internal virtual override auth(UPGRADE_GOVERNANCE_ERC20_PERMISSION_ID) {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,46 +11,58 @@ import {IERC20MetadataUpgradeable} from "@openzeppelin/contracts-upgradeable/tok
import {ERC20VotesUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20VotesUpgradeable.sol";
import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol";
import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";

import {DaoAuthorizableUpgradeable} from "../core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol";

import {IDAO} from "../core/dao/IDAO.sol";
import {IGovernanceWrappedERC20} from "../token/ERC20/governance/IGovernanceWrappedERC20.sol";

/// @title GovernanceWrappedERC20
/// @title GovernanceWrappedERC20Upgradeable
/// @author Aragon Association
/// @notice Wraps an existing [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token by inheriting from `ERC20WrapperUpgradeable` and allows to use it for voting by inheriting from `ERC20VotesUpgradeable`. The latter is compatible with [OpenZeppelin's `Votes`](https://docs.openzeppelin.com/contracts/4.x/api/governance#Votes) interface.
/// The contract also supports meta transactions. To use an `amount` of underlying tokens for voting, the token owner has to
/// 1. call `approve` for the tokens to be used by this contract
/// 2. call `depositFor` to wrap them, which safely transfers the underlying [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens to the contract and mints wrapped [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens.
/// To get the [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens back, the owner of the wrapped tokens can call `withdrawFor`, which burns the wrapped [ERC-20](https://eips.ethereum.org/EIPS/eip-20) tokens and safely transfers the underlying tokens back to the owner.
/// @dev This contract intentionally has no public mint functionality because this is the responsibility of the underlying [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token contract.
contract GovernanceWrappedERC20 is
contract GovernanceWrappedERC20Upgradeable is
Copy link
Contributor

Choose a reason for hiding this comment

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

See above

IGovernanceWrappedERC20,
Initializable,
ERC165Upgradeable,
ERC20VotesUpgradeable,
ERC20WrapperUpgradeable
ERC20WrapperUpgradeable,
UUPSUpgradeable,
DaoAuthorizableUpgradeable
{
/// @notice The ID of the permission required to call the `_authorizeUpgrade` function.
bytes32 public constant UPGRADE_GOVERNANCE_ERC20_PERMISSION_ID =
keccak256("UPGRADE_GOVERNANCE_ERC20_PERMISSION");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change the permission string so it will have a diffrent hash compared to the GovernanceERC20 permission, or this is done on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These need to be standardized as part of the setup workflow: we grant the same permission regardless of whether we deploy a wrapped token


/// @notice Calls the initialize function.
/// @param _dao The managing DAO.
/// @param _token The underlying [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token.
/// @param _name The name of the wrapped token.
/// @param _symbol The symbol of the wrapped token.
constructor(IERC20Upgradeable _token, string memory _name, string memory _symbol) {
initialize(_token, _name, _symbol);
constructor(IDAO _dao, IERC20Upgradeable _token, string memory _name, string memory _symbol) {
initialize(_dao, _token, _name, _symbol);
}

/// @notice Initializes the contract.
/// @param _dao The managing DAO.
/// @param _token The underlying [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token.
/// @param _name The name of the wrapped token.
/// @param _symbol The symbol of the wrapped token.
function initialize(
IDAO _dao,
IERC20Upgradeable _token,
string memory _name,
string memory _symbol
) public initializer {
__ERC20_init(_name, _symbol);
__ERC20Permit_init(_name);
__ERC20Wrapper_init(_token);
__DaoAuthorizableUpgradeable_init(_dao);
}

/// @notice Checks if this or the parent contract supports an interface by its ID.
Expand Down Expand Up @@ -123,4 +135,10 @@ contract GovernanceWrappedERC20 is
) internal override(ERC20VotesUpgradeable, ERC20Upgradeable) {
super._burn(account, amount);
}

/// @notice Internal method authorizing the upgrade of the contract via the [upgradeability mechanism for UUPS proxies](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)).
/// @dev The caller must have the `UPGRADE_DAO_PERMISSION_ID` permission.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// @dev The caller must have the `UPGRADE_DAO_PERMISSION_ID` permission.
/// @dev The caller must have the `UPGRADE_GOVERNANCE_ERC20_PERMISSION_ID ` permission.

Copy link
Contributor

Choose a reason for hiding this comment

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

the same happens on the other contracts' comment on the same func

function _authorizeUpgrade(
address
) internal virtual override auth(UPGRADE_GOVERNANCE_ERC20_PERMISSION_ID) {}
}
22 changes: 19 additions & 3 deletions packages/contracts/src/zksync/PluginSetupProcessorUpgradeable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
pragma solidity 0.8.17;

import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol";
import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";

import {DAO, IDAO} from "../core/dao/DAO.sol";
import {PermissionLib} from "../core/permission/PermissionLib.sol";
Expand All @@ -16,13 +17,18 @@ import {IPluginSetup} from "../framework/plugin/setup/IPluginSetup.sol";
import {PluginSetup} from "../framework/plugin/setup/PluginSetup.sol";
import {PluginSetupRef, hashHelpers, hashPermissions, _getPreparedSetupId, _getAppliedSetupId, _getPluginInstallationId, PreparationType} from "../framework/plugin/setup/PluginSetupProcessorHelpers.sol";

import {DaoAuthorizableUpgradeable} from "../core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol";

/// @title PluginSetupProcessor
/// @author Aragon Association - 2022-2023
/// @notice This contract processes the preparation and application of plugin setups (installation, update, uninstallation) on behalf of a requesting DAO.
/// @dev This contract is temporarily granted the `ROOT_PERMISSION_ID` permission on the applying DAO and therefore is highly security critical.
contract PluginSetupProcessor {
contract PluginSetupProcessorUpgradeable is UUPSUpgradeable, DaoAuthorizableUpgradeable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this acceptable? The PSP gets ROOT_PERMISSION_ID temporarily during each setup application.
What's happening in case the PSP gets compromised? What are the worst-case implications for DAOs? I would like to see an analysis here.

using ERC165Checker for address;

/// @notice The ID of the permission required to call the `_authorizeUpgrade` function.
bytes32 public constant UPGRADE_PSP_PERMISSION_ID = keccak256("UPGRADE_PSP_PERMISSION");

/// @notice The ID of the permission required to call the `applyInstallation` function.
bytes32 public constant APPLY_INSTALLATION_PERMISSION_ID =
keccak256("APPLY_INSTALLATION_PERMISSION");
Expand Down Expand Up @@ -272,9 +278,15 @@ contract PluginSetupProcessor {
_;
}

/// @notice Constructs the plugin setup processor by setting the associated plugin repo registry.
/// @notice Disables the initializers on the implementation contract to prevent it from being left uninitialized.
constructor() {
_disableInitializers();
}

/// @param _dao The managing dao.
/// @param _repoRegistry The plugin repo registry contract.
constructor(PluginRepoRegistry _repoRegistry) {
function initialize(IDAO _dao, PluginRepoRegistry _repoRegistry) external initializer {
__DaoAuthorizableUpgradeable_init(_dao);
repoRegistry = _repoRegistry;
}

Expand Down Expand Up @@ -734,4 +746,8 @@ contract PluginSetupProcessor {
initData: _initData
});
}

/// @notice Internal method authorizing the upgrade of the contract via the [upgradeability mechanism for UUPS proxies](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable) (see [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)).
/// @dev The caller must have the `UPGRADE_DAO_PERMISSION_ID` permission.
function _authorizeUpgrade(address) internal virtual override auth(UPGRADE_PSP_PERMISSION_ID) {}
}
82 changes: 61 additions & 21 deletions packages/contracts/src/zksync/TokenVotingSetupZkSync.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,20 @@ import {IDAO} from "../core/dao/IDAO.sol";
import {DAO} from "../core/dao/DAO.sol";
import {PermissionLib} from "../core/permission/PermissionLib.sol";
import {PluginSetup, IPluginSetup} from "../framework/plugin/setup/PluginSetup.sol";
import {GovernanceERC20Upgradeable} from "./GovernanceERC20Upgradeable.sol";
import {GovernanceWrappedERC20Upgradeable} from "./GovernanceWrappedERC20Upgradeable.sol";

import {GovernanceERC20} from "../token/ERC20/governance/GovernanceERC20.sol";
import {GovernanceWrappedERC20} from "../token/ERC20/governance/GovernanceWrappedERC20.sol";

import {IGovernanceWrappedERC20} from "../token/ERC20/governance/IGovernanceWrappedERC20.sol";
import {MajorityVotingBase} from "../plugins/governance/majority-voting/MajorityVotingBase.sol";
import {TokenVoting} from "../plugins/governance/majority-voting/token/TokenVoting.sol";

/// @title TokenVotingSetup
/// @author Aragon Association - 2022-2023
/// @notice The setup contract of the `TokenVoting` plugin.
contract TokenVotingSetup is PluginSetup {
/// @notice The setup contract of the `TokenVoting` plugin for the ZkSync network.
contract TokenVotingSetupZkSync is PluginSetup {
using Address for address;
using Clones for address;
using ERC165Checker for address;
Expand Down Expand Up @@ -61,8 +65,8 @@ contract TokenVotingSetup is PluginSetup {
/// @param _governanceERC20Base The base `GovernanceERC20` contract to create clones from.
/// @param _governanceWrappedERC20Base The base `GovernanceWrappedERC20` contract to create clones from.
constructor(
GovernanceERC20 _governanceERC20Base,
GovernanceWrappedERC20 _governanceWrappedERC20Base
GovernanceERC20Upgradeable _governanceERC20Base,
GovernanceWrappedERC20Upgradeable _governanceWrappedERC20Base
) {
tokenVotingBase = new TokenVoting();
governanceERC20Base = address(_governanceERC20Base);
Expand All @@ -88,6 +92,13 @@ contract TokenVotingSetup is PluginSetup {

address token = tokenSettings.addr;

// determine if the plugin needs to be granted the upgrade permission
// this will be if:
// - the token is not passed (new token is deployed)
// - the token is passed, but it does not support our token interfaces
// so we need to deploy it behind a proxy and grant upgrade to the DAO
bool setUpgradePermission = false;

// Prepare helpers.
address[] memory helpers = new address[](1);

Expand All @@ -112,25 +123,33 @@ contract TokenVotingSetup is PluginSetup {
// IVotes nor IGovernanceWrappedERC20, it needs wrapping.
(supportedIds[0] && !supportedIds[1] && !supportedIds[2])
) {
token = governanceWrappedERC20Base.clone();
// User already has a token. We need to wrap it in
// GovernanceWrappedERC20 in order to make the token
// include governance functionality.
GovernanceWrappedERC20(token).initialize(
IERC20Upgradeable(tokenSettings.addr),
tokenSettings.name,
tokenSettings.symbol
token = createERC1967Proxy(
governanceWrappedERC20Base,
abi.encodeWithSelector(
GovernanceWrappedERC20Upgradeable.initialize.selector,
IDAO(_dao),
IERC20Upgradeable(tokenSettings.addr),
tokenSettings.name,
tokenSettings.symbol
)
);
setUpgradePermission = true;
}
} else {
// Clone a `GovernanceERC20`.
token = governanceERC20Base.clone();
GovernanceERC20(token).initialize(
IDAO(_dao),
tokenSettings.name,
tokenSettings.symbol,
mintSettings
token = createERC1967Proxy(
governanceERC20Base,
abi.encodeWithSelector(
GovernanceERC20Upgradeable.initialize.selector,
IDAO(_dao),
tokenSettings.name,
tokenSettings.symbol,
mintSettings
)
);
setUpgradePermission = true;
}

helpers[0] = token;
Expand All @@ -141,11 +160,20 @@ contract TokenVotingSetup is PluginSetup {
abi.encodeWithSelector(TokenVoting.initialize.selector, _dao, votingSettings, token)
);

// Prepare permissions
PermissionLib.MultiTargetPermission[]
memory permissions = new PermissionLib.MultiTargetPermission[](
tokenSettings.addr != address(0) ? 3 : 4
);
PermissionLib.MultiTargetPermission[] memory permissions;

// avoid stack too deep.
{
// check for an existing token: we will grant mint on the token to the dao if so
uint256 permissionCount = tokenSettings.addr != address(0) ? 3 : 4;

// If the plugin needs to be granted the upgrade permission, increment the permission count.
if (setUpgradePermission) {
permissionCount = permissionCount + 1;
}

permissions = new PermissionLib.MultiTargetPermission[](permissionCount);
}

// Set plugin permissions to be granted.
// Grant the list of permissions of the plugin to the DAO.
Expand Down Expand Up @@ -186,6 +214,18 @@ contract TokenVotingSetup is PluginSetup {
);
}

if (setUpgradePermission) {
bytes32 tokenUpgradePermission = GovernanceERC20Upgradeable(token)
.UPGRADE_GOVERNANCE_ERC20_PERMISSION_ID();
permissions[permissions.length - 1] = PermissionLib.MultiTargetPermission(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please think carefully about this: I believe the UPGRADE permission will always be the final permission in the array if it is needed, but we won't know if it is at index 4 or 5 at compile time. This depends on the token being address(0) or not.

Can this break? Can this be exploited?

PermissionLib.Operation.Grant,
token,
_dao,
PermissionLib.NO_CONDITION,
tokenUpgradePermission
);
}

preparedSetupData.helpers = helpers;
preparedSetupData.permissions = permissions;
}
Expand Down
Loading