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.

Sign up for GitHub

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

Jump to bottom

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

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

wants to merge 1 commit into from

Conversation

jordaniza
Copy link
Contributor

Description

Task ID: OS-?

Type of change

See the framework lifecycle in packages/contracts/docs/framework-lifecycle to decide what kind of change this pull request is.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran all tests with success and extended them if necessary.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have updated the DEPLOYMENT_CHECKLIST file in the root folder.
  • I have updated the UPDATE_CHECKLIST file in the root folder.
  • I have updated the Subgraph and added a QA URL to the description of this PR.

jordaniza
jordaniza commented May 10, 2024
View reviewed changes
packages/contracts/src/zksync/TokenVotingSetupZkSync.sol
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?

heueristik
heueristik reviewed May 10, 2024
View reviewed changes
packages/contracts/src/zksync/GovernanceERC20Upgradeable.sol
/// @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.

heueristik
heueristik reviewed May 10, 2024
View reviewed changes
packages/contracts/src/zksync/GovernanceWrappedERC20Upgradeable.sol
/// @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

heueristik
heueristik reviewed May 10, 2024
View reviewed changes
packages/contracts/src/zksync/PluginSetupProcessorUpgradeable.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.

heueristik
heueristik suggested changes May 10, 2024
View reviewed changes
Copy link
Contributor

@heueristik heueristik left a comment

Choose a reason for hiding this comment

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

I would like to be part of a discussion on the implications of these changes before approving this.

Rekard0
Rekard0 reviewed May 10, 2024
View reviewed changes
packages/contracts/src/zksync/GovernanceWrappedERC20Upgradeable.sol
{
/// @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

clauBv23
clauBv23 reviewed May 13, 2024
View reviewed changes
packages/contracts/src/zksync/GovernanceERC20Upgradeable.sol
@@ -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

clauBv23
clauBv23 reviewed May 13, 2024
View reviewed changes
packages/contracts/src/zksync/GovernanceWrappedERC20Upgradeable.sol
@@ -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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clauBv23 clauBv23 clauBv23 left review comments

@Rekard0 Rekard0 Rekard0 left review comments

@heueristik heueristik heueristik requested changes

Assignees
No one assigned
Labels
contracts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jordaniza @Rekard0 @heueristik @clauBv23