feat: add allowlist module #178
Conversation
Summary by OctaneNew Contracts
Updated Contracts
🔗 Commit Hash: 4afc628 |
adamegyed
force-pushed
the
adam/add-allowlist
branch
from
067ff2b
to
215a2fa
Compare
|
Contract sizes: | Contract | Size (B) | Margin (B) |
|------------------------------|----------|------------|
| AccountFactory | 4,169 | 20,407 |
+| AllowlistModule | 5,817 | 18,759 |
| ERC20TokenLimitModule | 4,138 | 20,438 |
| ModularAccount | 25,983 | -1,407 |
| SemiModularAccount | 26,487 | -1,911 |
| SingleSignerValidationModule | 3,444 | 21,132 |
| TokenReceiverModule | 2,189 | 22,387 |Code coverage:
|
This comment was marked as resolved.
This comment was marked as resolved.
adamegyed
force-pushed
the
adam/add-allowlist
branch
from
215a2fa
to
a77f7cb
Compare
| struct AllowlistInit { | ||
| address target; | ||
| bool hasSelectorAllowlist; | ||
| bytes4[] selectors; | ||
| } | ||
|
|
||
| struct AllowlistEntry { | ||
| bool allowed; | ||
| bool hasSelectorAllowlist; | ||
| } |
There was a problem hiding this comment.
I see what you are doing. Address is used as the entry point for both addresses and selectors. It wont support cases where user wants to keep addresses and selectors limits separate.
How about something like this?
mapping(bytes4 selector => mapping(address target => mapping(address account => bool))))
) public selectorAllowList;
mapping(address target => mapping(bytes4 selector => mapping(address account => bool))))
) public contractAllowList;
For wildcard support, we set the wildcard to address(0) or bytes4('').
When checking, we check the following:
selectorAllowList[selector][target] ||
selectorAllowList[selector][address(0)] ||
contractAllowList[target][bytes4('')]
We can avoid contractAllowList[target][selector] by use selectorAllowList as priority.
There was a problem hiding this comment.
That could work, but I think it would increase the checking costs in most cases because it needs to cover 2 cases sequentially.
There was a problem hiding this comment.
I'll implement it for now, and optimize later if/when we combine the checking hooks.
There was a problem hiding this comment.
oh wait, this doesn't work - bytes4(0) is a real value, we can't treat it as a wildcard.
Let's keep it as-is for now, and move the address-wildcard restriction to a different PR.
There was a problem hiding this comment.
It would require a major refactor almost like a rewrite. Probably slightly easier to review as a new file.
Why cannot we treat bytes4(uint32(0)) as a wildcard?
There was a problem hiding this comment.
Why cannot we treat bytes4(uint32(0)) as a wildcard?
That is a valid selector - some contracts mine an all-zero selector, see https://www.4byte.directory/signatures/?bytes4_signature=0x00000000
If someone adds that selector to the allowlist, they would accidentally be adding a wildcard, and it may not be obvious if they're just retrieving things from an ABI.
There was a problem hiding this comment.
TIL.
We can update the second mapping actually. Better anyways.
mapping(bytes4 selector => mapping(address target => mapping(address account => bool))))
) public selectorAllowList;
mapping(address target => mapping(address account => bool))) public contractAllowList;
The second mapping is now basically the wildcard for selector for the contract, since all non-wildcard cases can be handled by the first mapping.
adamegyed
force-pushed
the
adam/add-allowlist
branch
from
5da015e
to
4d83375
Compare
| return "alchemy.allowlist-module.0.0.1"; | ||
| } | ||
|
|
||
| function setAllowlistTarget(uint32 entityId, address target, bool allowed, bool hasSelectorAllowlist) public { |
There was a problem hiding this comment.
For a target with selectors, would two calls required to set it up? Should we add one more input here to accept optional selectors?
There was a problem hiding this comment.
What we have in oninstall is does take that into consideration. We probably can reuse the logic there.
There was a problem hiding this comment.
Yeah, I think right now you would need to batch multiple steps together. Technically, the accounts can now directly call onInstall even after the module is already initialized, so they could use it to batch the call, but that's a bit of a clumsy workflow. Do you think we should add a new external function to do the same workflow as onInstall, just named something else? It'd be more intuitive for devs.
There was a problem hiding this comment.
Do you think we should add a new external function to do the same workflow as onInstall, just named something else? It'd be more intuitive for devs.
Yeah, that'd be good. They can call the same internal func under the hood.
| } | ||
|
|
||
| function setAllowlistSelector(uint32 entityId, address target, bytes4 selector, bool allowed) public { | ||
| selectorAllowlist[entityId][target][selector][msg.sender] = allowed; |
There was a problem hiding this comment.
When this is set for the first time for a target, should we update the hasSelectorAllowlist value intargetAllowlist?
There was a problem hiding this comment.
I think we can't tell whether or not it's the first target, because these mappings are non-enumerable.
There was a problem hiding this comment.
In that case, should we do sth like the following?
If (allowed) {
intargetAllowlist[entityId][target].hasSelectorAllowlist = true;
}
Otherwise, we will have to revert selector direct set for target where the target has hasSelectorAllowlist set to true. Because in this case, this selector wont be checked. So users will have to add selectors through the other set method.
I like the first option better.
There was a problem hiding this comment.
I think we should avoid those extra storage writes - the user can already specify these two things independently. To address this, I kept the individual edit functions (setAllowlistTarget, setAllowlistSelector) in, as well as the new batch init function batchInitAllowlist.
There was a problem hiding this comment.
The case I am concerned is.
- User installs hook to limit a SK to call a contract (
addressA). - User realizes that they can only allow one selector on
addressAfor the SK to achieve whatever they want. So they come to callsetAllowlistSelector, and think that it would work. - It wont work because
addressAhashasSelectorAllowlistto be true.
So I think this storage writes is actually worth it in this case.
The only alternative is to educate users/customers to be aware of this, which seems it will be more work.
There was a problem hiding this comment.
I think if they're on the level of calling individual functions on this contract, rather than using some higher-level abstractions from the SDK, then they need to be comfortable with understanding how the contract works. We shouldn't hamper performance to address a hypothetical user error.
There was a problem hiding this comment.
We shouldn't hamper performance to address a hypothetical user error.
The only performance weakness is If (allowed) { this portion. Which is almost nothing.
- if
intargetAllowlist[entityId][target].hasSelectorAllowlistwas true, the set costs nothing. - if it was originally false, (without setting it to true) the whole method call is wasted as it won't do anything.
There was a problem hiding this comment.
the set costs nothing.
This is not true, solidity loads every storage slot before setting to keep other bits the same, and even a warm load or warm set costs something. (ref)
The users sets the config as they wish, including the ability to set it to something that isn't valid or useful. It's not the contract's job to enforce that, that happens on a different level.
There was a problem hiding this comment.
Oh yeah, missed the loading part. Hmm. Need to weigh more.
Anyways, to no block this PR, can you add this behavior to docs as dev notice to warn this case?
There was a problem hiding this comment.
Added a comment to call this out in the natspec, to warn users that this setting will only matter if the target address has the selector allowlist enabled.
adamegyed
force-pushed
the
adam/add-allowlist
branch
2 times, most recently
from
6273dfe
to
e54e9a1
Compare
adamegyed
force-pushed
the
adam/add-allowlist
branch
from
e54e9a1
to
4afc628
Compare
Motivation
We want the ability to apply an allowlist to specific validations on an account.
Solution
Port over the Allowlist module from the RI. This checks addresses and optionally also checks selectors. It does not check any additional portions of calldata.