-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Override access control functions to have a consistent state in the bridge contract #77
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
Conversation
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.
Pull Request Overview
This PR overrides access control functions in the IexecLayerZeroBridge contract to ensure consistent state management between OwnableUpgradeable and AccessControlDefaultAdminRulesUpgradeable inheritance. The primary purpose is to prevent ownership operations through the Ownable interface and redirect users to use AccessControlDefaultAdminRules instead.
- Disables
renounceOwnershipandtransferOwnershipfunctions to prevent direct ownership changes - Updates
owner()function to return the default admin instead of the ownable owner - Ensures state consistency by updating the ownable owner when default admin transfers occur
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/interfaces/IIexecLayerZeroBridge.sol | Adds OperationNotAllowed error for blocked ownership operations |
| src/bridges/layerZero/IexecLayerZeroBridge.sol | Implements access control overrides and state synchronization logic |
| test/units/bridges/layerZero/IexecLayerZeroBridge.t.sol | Adds comprehensive tests for the new access control behavior |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
==========================================
+ Coverage 83.65% 83.78% +0.12%
==========================================
Files 4 4
Lines 104 111 +7
Branches 7 7
==========================================
+ Hits 87 93 +6
- Misses 16 17 +1
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Le-Caignec
left a comment
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.
LGTM
In the bridge contract
IexecLayerZeroBridgebothOwnable(used by LayerZero) andAccessControlDefaultAdminare used to manager roles, where the owner and the defaultAdmin are supposed to always be the same account, which is the case at deployment time. However, changing one of them later does not change the other. For example, one could change (for any reason) the owner to a new wallet without changing the defaultAdmin which could lead to losing control of the contract if the old private key is destroyed. The suggested fix is to override the followingOwnablefunctions:owner: should return the defaultAdmin address.onlyOwner: nothing to dorenounceOwnershipandtransferOwnership: should always revert