-
Notifications
You must be signed in to change notification settings - Fork 43
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
base: zksync-testing-review
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
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"); | ||||||
|
||||||
|
@@ -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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||||||
|
@@ -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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) {} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this acceptable? The PSP gets |
||
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"); | ||
|
@@ -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; | ||
} | ||
|
||
|
@@ -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) {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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); | ||
|
@@ -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); | ||
|
||
|
@@ -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; | ||
|
@@ -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. | ||
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please think carefully about this: I believe the Can this break? Can this be exploited? |
||
PermissionLib.Operation.Grant, | ||
token, | ||
_dao, | ||
PermissionLib.NO_CONDITION, | ||
tokenUpgradePermission | ||
); | ||
} | ||
|
||
preparedSetupData.helpers = helpers; | ||
preparedSetupData.permissions = permissions; | ||
} | ||
|
There was a problem hiding this comment.
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.