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

Replace OwnableUpgradeable with AccessControlUpgradeable in Bridge Contracts #849

Open
Primata opened this issue Nov 13, 2024 · 4 comments · May be fixed by #772 or movementlabsxyz/aptos-core#98
Open

Comments

@Primata
Copy link
Contributor

Primata commented Nov 13, 2024

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. movementlabsxyz/aptos-core#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.

@Primata Primata changed the title Change Bridge Inheritance to AccessControlUpgradeable Replace OwnableUpgradeable with AccessControlUpgradeable in Bridge Contracts Nov 13, 2024
@Primata Primata linked a pull request Nov 13, 2024 that will close this issue
@0xmovses
Copy link
Contributor

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

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.

@Primata
Copy link
Contributor Author

Primata commented Nov 13, 2024

@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
Copy link
Contributor

andygolay commented Nov 13, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants