movementlabsxyz / movement

The Movement Network is a Move-based L2 on Ethereum.
Apache License 2.0
77 stars 63 forks source link

Replace `OwnableUpgradeable` with `AccessControlUpgradeable` in Bridge Contracts #849

Open 0xPrimata opened 3 days ago

0xPrimata commented 3 days ago

Is your feature request related to a problem? Please describe. Currently, the bridge contract uses OwnableUpgradeable, which limits its ability to split roles within the bridge and makes it difficult to support future upgrades. The goal is to enable role-based access management, allowing for greater flexibility and security in managing various roles (such as admin, relayer, and refunder). However, switching from Ownable to AccessControl could leave residual behaviors, creating potential security risks or leading to complex upgrade paths, and these could even introduce exploits if not carefully managed. The importance of separating Admin, Relayer and Refunder it's so that we can have an Admin that sets configs, and/or changes roles, a relayer that its sole purpose is relaying bridges which we can lock behind a protected environment to secure that nobody, not even MovementLabs can relay an arbitrary bridge, and the refunder to be a multisig that authorized personnel (MovementLabs potentially), can refund users.

Describe the solution you'd like Implement AccessControlUpgradeable instead of OwnableUpgradeable in the bridge contract. This change will enable distinct roles to be assigned and managed, making it easier to upgrade the bridge without needing to redeploy for simple role adjustments. Initially, all roles (admin, relayer, refunder) will be assigned to a single public address, ensuring that no changes occur in the bridge's current behavior within the existing Relayer stack.

By introducing AccessControlUpgradeable, the DEFAULT_ADMIN_ROLE will have the authority to delegate specific roles as needed. Over time, this will allow us to add a refunder role that governance alone can alter. These enhancements will be implemented in the movementlabsxyz/aptos-core repository. https://github.com/movementlabsxyz/aptos-core/pull/98

Describe alternatives you've considered

  1. Keeping OwnableUpgradeable: This approach lacks flexibility and will likely require multiple contract upgrades as we introduce new roles and permissions.
  2. Manually implementing a role-based system: Instead of using AccessControlUpgradeable, in the future manually implement role management functions. However, this would increase code complexity and the potential for bugs or exploits, compared to the robust role management provided by AccessControl.

Additional context The current implementation assigns all roles to the same address, ensuring no behavioral changes. AccessControlUpgradeable also provides a streamlined way to add or remove roles without requiring contract upgrades solely to modify roles on the Ethereum side. By allowing the DEFAULT_ADMIN_ROLE to manage role assignments, we can flexibly evolve the role structure in line with governance requirements, increasing contract resilience and adaptability.

0xmovses commented 3 days ago

Have you started working on this Primata? If so it should be on the project board, with status set to in-progress. Otherwise assuming its still an issue for debate.

andygolay commented 3 days ago

If this is mainly on the ETH side then perhaps we don't need to add a separate refunder role on the Move framework side at all? The bridge operator is already access controlled properly on the Move side.

0xPrimata commented 3 days ago

@andygolay this is for both eth and move move already has 2 roles, admin (governance) and bridge_operator (relayer). We need to separate relayer from refunder so that we can manually refund users and not share keys for the relayer itself. This is meant to guard the relayer key.

andygolay commented 3 days ago

Thank you, yes the relayer keys must be different than the refunder keys. Got it. Makes total sense the way you put it.