Fix AccessManager._checkAuthorized in execute context
#4612
Conversation
|
|
Can we make this PR focused exclusively on the refactor of It will be better to review it on its own, and we don't need to include the other changes in the first release candidate. |
| * In addition to the access rules defined by each target's functions being assigned to roles, then entire target can | ||
| * be "closed". This "closed" mode is set/unset by the admin using {setTargetClosed} and can be used to lock a contract | ||
| * while permissions are being (re-)configured. | ||
| * By default every address is member of the `PUBLIC_ROLE` and every contract allows public access until configured otherwise. |
There was a problem hiding this comment.
I'm really not sure that "every contract allows public access". If that is the case, it sounds like a huge risk.
There was a problem hiding this comment.
Let's keep the docs discussion in #4613
| * Emits a {RoleAdminChanged} event | ||
| * Emits a {RoleAdminChanged} event. | ||
| * | ||
| * NOTE: Setting the admin role as the `PUBLIC_ROLE` is allowed, but it will effectively allow |
There was a problem hiding this comment.
Do we want to prevent that ? (same question for _setRoleGuardian)
There was a problem hiding this comment.
I'd personally prevent both because I don't see why anyone would do that but we need to analyze a bit better.
We can do it in the tests PR #4613
ernestognw
force-pushed
the
access-manager/coverage-review
branch
from
62ff89b to
b62e39b
Compare
Sure thing, just updated and also submitted #4613 with the tests WIP |
ernestognw
changed the title
_canCallSelf and _canCallExecuting
frangio
changed the title
_canCallSelf and _canCallExecutingonlyAuthorized functions when called via execute
frangio
changed the title
onlyAuthorized functions when called via executeAccessManager._checkAuthorized in execute context
Co-authored-by: Francisco <fg@frang.io>
Co-authored-by: Francisco <fg@frang.io>
Admin functions in the AccessManager restricted with the
onlyAuthorizedmodifier were not callable viaexecute. This was because_canCallExtendedignored the case wherecaller == this. This is particularly important forGovernorTimelockAccess, which assumes that anything with restricted access can be invoked throughexecuteand always does so._canCallExtendedis also refactored slightly to separate the logic fortarget == this.This PR does not include tests for the fix because we're cherry picking fixes done in the middle of a test refactor, but they are included in #4613.