-
Notifications
You must be signed in to change notification settings - Fork 12.4k
Add AccessManager contracts #4121
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
Changes from 1 commit
1327b8f
b54b922
5a0fd33
ee5de96
6284768
bce3641
223e6af
eafb571
24af159
833f219
0f27739
2ac5aa1
f26c4d3
63f9393
6fecd25
589b5b1
619cc73
1b681a5
48b31c0
b078157
274c04d
082b05c
52f27c8
1285c49
5c7472a
3cc1e73
ae3482d
535ea13
68bb5fa
fa58ec5
6d76058
3447438
1d10639
d875a7b
d517d27
6d33cda
9ed1aef
6adc0d8
0624794
17ec3b6
f9210c5
a54cd10
8af9ca4
2a05bcc
5f22f2d
6a9fbed
0580198
e01e362
cb026bc
76d35da
4069781
80ffd2a
3094d0c
b3a8b1b
8a68322
eeab8cf
ca61e38
8824c31
67e33b6
5f270c7
6f7ac96
7ec5311
82402d2
27807e6
80bc88b
cf4df22
bab4d34
642e279
51aff23
6e3da66
6b56c8f
b7e3b3f
8200986
18e53e2
f94a881
cd8babd
597edc0
ee560d0
33f5ace
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| pragma solidity ^0.8.0; | ||
|
|
||
| import "./IAuthority.sol"; | ||
|
|
||
| contract AccessManaged { | ||
| event AuthorityUpdated(IAuthority indexed oldAuthority, IAuthority indexed newAuthority); | ||
|
|
||
| IAuthority private _authority; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know that is restrictive, but making this immutable would save a non insignificant amount of gas. In the case of proxy/clones created by a factory, would it make sens that all implementation use the same authority? I think yes.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You know I hate multiple versions... 😄 The main reason why I left this as a mutable variable is because I think it can be used to freeze the permissions in a contract by moving it off of an AccessManager onto an immutable Authority. We could also implement freezing in the AccessManager itself, by having frozen contract groups in which permissions can't be altered, and whose contracts can't be moved out of that group. I felt that having mutability of the authority at the managed contract was the simpler option, but it's true that it makes it more expensive. I'm open to this discussion though.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This makes sense, but I also think it violates the goal of consolidation. In this way, now a managed contract may have the right to update itself without the Manager's approval. While that still makes sense to me in some cases, I am not sure if a significant share of My opinion here would be to make it immutable since any freezing capability can be handled by the Manager by making it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think this is true. Only the current manager should be able to "eject" a managed contract from itself.
Note that adding a contract in the Closed or Open group doesn't freeze the permissions, because the manager retains the ability to move it outside of the group.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why not? Any We can keep this private, but I have some bad feelings about letting the user "eject" the contract itself even if done deliberately.
I see what you meant, but a previous idea I remember we discussed is suggesting overrides like: function setContractModeOpern(address target) ... override {
require(_cantReopen[target]);
super.setContractModeCustom(target);
}Not sure exactly how this can play out, but I think there might be a setup we may like to explore (eg. not reopening by default unless explicitly stated).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO it's fine if the target contract deliberately inserts a custom-gated |
||
|
|
||
| modifier restricted { | ||
| require(_authority.canCall(msg.sender, address(this), msg.sig)); | ||
| _; | ||
| } | ||
|
|
||
| constructor(IAuthority initialAuthority) { | ||
| _authority = initialAuthority; | ||
| } | ||
|
|
||
| function authority() public virtual view returns (IAuthority) { | ||
| return _authority; | ||
| } | ||
|
|
||
| function setAuthority(IAuthority newAuthority) public virtual { | ||
| require(msg.sender == address(_authority)); | ||
ernestognw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| IAuthority oldAuthority = _authority; | ||
| _authority = newAuthority; | ||
| emit AuthorityUpdated(oldAuthority, newAuthority); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| pragma solidity ^0.8.0; | ||
|
|
||
| import "./AccessManager.sol"; | ||
| import "../../utils/Address.sol"; | ||
frangio marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| contract AccessManagerAdapter { | ||
| using Address for address; | ||
|
|
||
| AccessManager private _manager; | ||
|
|
||
| bytes32 private _DEFAULT_ADMIN_ROLE = 0; | ||
|
|
||
| function relay(address target, bytes memory data) external payable { | ||
| bytes4 sig = bytes4(data); | ||
| require(_manager.canCall(msg.sender, target, sig) || _manager.hasRole(_DEFAULT_ADMIN_ROLE, msg.sender)); | ||
| (bool ok, bytes memory result) = target.call{value: msg.value}(data); | ||
| assembly { | ||
| let result_pointer := add(32, result) | ||
| let result_size := mload(result) | ||
| switch ok | ||
| case true { | ||
| return(result_pointer, result_size) | ||
| } | ||
| default { | ||
| revert(result_pointer, result_size) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| pragma solidity ^0.8.0; | ||
|
|
||
| interface IAuthority { | ||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| function canCall(address caller, address target, bytes4 selector) external view returns (bool allowed); | ||
| } | ||
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.
This is very rarely triggered, so the gas cost is not really an issue... but I'm still wondering if emitting the old value really brings anything.
Uh oh!
There was an error while loading. Please reload this page.
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.
I wanted to match the event emitted by Solmate:
It looks different but the semantics are the same with the exception of the constructor event.
Uh oh!
There was an error while loading. Please reload this page.
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.
Remember that there is an internal
_setAuthoritythat can called by someone that is not the oldAuthority.The semantics are not the same!
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.
I'd go for exaclty matching Solmate's semantics
Uh oh!
There was an error while loading. Please reload this page.
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.
Solmate's semantics subsume AccessManaged semantics, because in AccessManaged the
oldAuthorityis theuserthat has triggered the transfer (msg.sender).I don't see what we would need to change to match the semantics exactly. Is it just renaming
oldAuthoritytouserand making sure we usemsg.sender?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.
yes
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.
Ok to use
senderinstead ofuser? We have precedent forsenderinAccessControl.RoleGrantedand so on.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.
If
sender == msg.senderfor every case. Yes.