Implement AccessController and ModuleRegistry for IPAccount Execution Model #4

kingster-will · 2024-01-20T01:27:35Z

This PR introduces the implementation of AccessController and ModuleRegistry to enhance the execution model of IPAccount. The new model supports multiple call paths with robust access control mechanisms.

The execution models supported include:

Direct interaction from IPAccount to Module.
Interaction from Module to IPAccount and then to another Module.
Interaction from Orchestrator Module to IPAccount and then interact with multiple modules.

These changes ensure that the system is flexible and secure, allowing for complex interactions while maintaining control over access permissions.

In addition, this PR includes comprehensive test coverage to ensure the reliability and stability of the new features. The tests cover all new functionality, providing confidence in the robustness of the implementation.

jdubpark · 2024-01-20T01:44:34Z

contracts/lib/AccessPermission.sol

+library AccessPermission {
+    // ABSTAIN means having not enough information to make decision at current level,
+    // deferred decision to up level permission.
+    uint8 public constant ABSTAIN = 0;


Wondering what case would require ABSTAIN?

Oh I see in AccessController.sol

// If specific function permission is ABSTAIN, check module level permission if (permissions[ipAccount_][signer_][to_][func_] == AccessPermission.ABSTAIN) { // Return true if allow to call all functions of the module if (permissions[ipAccount_][signer_][to_][bytes4(0)] == AccessPermission.ALLOW) { return true; } // If module level permission is ABSTAIN, check transaction signer level permission if (permissions[ipAccount_][signer_][to_][bytes4(0)] == AccessPermission.ABSTAIN) { // Return true if the ipAccount allow the signer can call all functions of all modules // Otherwise, return false return permissions[ipAccount_][signer_][address(0)][bytes4(0)] == AccessPermission.ALLOW; } return false; }

jdubpark · 2024-01-20T01:50:25Z

test/foundry/mocks/MockOrchestratorModule.sol

Would be nice to standardize errors for mocks as well

Good point, might handle in seperated PR.

jdubpark

LGTM, nice testing

leeren

LGTM

leeren · 2024-01-20T01:57:22Z

contracts/AccessController.sol

+    address public IP_ACCOUNT_REGISTRY;
+    address public MODULE_REGISTRY;
+
+    mapping(address => mapping(address => mapping(address => mapping(bytes4 => uint8)))) public permissions;


Probably helpful to document and use named mapping parameters
https://ethereum.stackexchange.com/questions/51629/how-to-name-the-arguments-in-mapping

Good point! will change in the next PR.

leeren · 2024-01-20T02:21:30Z

contracts/AccessController.sol

+///
+/// Each policy is represented as a mapping from an IP account address to a signer address to a recipient
+/// address to a function selector to a permission level.
+/// The permission level can be 0 (ABSTAIN), 1 (ALLOW), or 2 (DENY).


Can we make an enum for this used through a library?

Currently these values are defined in a library, but as constant. I was thinking about using enum instead. but feel constants would more clear than enum, since it reflect actual value in storage, for example 0 for abstain is default value.

leeren · 2024-01-20T02:21:55Z

contracts/AccessController.sol

+    /// @param to_ The recipient of the transaction (support wildcard permission)
+    /// @param func_ The function selector (support wildcard permission)
+    /// @param permission_ The permission level (0 => ABSTAIN, 1 => ALLOW, 3 => DENY)
+    function setPermission(address ipAccount_, address signer_, address to_, bytes4 func_, uint8 permission_) external {


We should use caller_ instead of signer_, signer is pretty confusing

yeah, caller is good.

leeren · 2024-01-20T02:22:19Z

contracts/AccessController.sol

+    /// @param func_ The function selector.
+    /// @return The permission level for the specific function call.
+    function getPermission(
+        address ipAccount_,


I think we agreed to stop using underscore suffixes

this code was actual writen before, will have another PR handle all code style.

leeren · 2024-01-20T02:22:43Z

contracts/IPAccountImpl.sol

@@ -34,7 +34,6 @@ contract IPAccountImpl is IERC165, IIPAccount {
    // TODO: can only be called by IPAccountRegistry
    function initialize(address accessController_) external {
        require(accessController_ != address(0), "Invalid access controller");


Can we go back to using custom errors? Not sure why we're reverting. I think we agreed on that from alpha

yes, will use custom error instead in code style pr.

leeren · 2024-01-20T02:24:20Z

contracts/lib/registries/IPAccountChecker.sol

+        if (ipAccountAddress_ == address(0)) return false;
+        if (ipAccountAddress_.code.length == 0) return false;
+        if (!ERC165Checker.supportsERC165(ipAccountAddress_)) return false;
+        if (!ERC165Checker.supportsInterface(ipAccountAddress_, type(IERC6551Account).interfaceId)) return false;


Isn't this redundant

yeah, this check should be able to be removed it.

Ramarti

I think this contract should inherit from OZ's AccessManager (later on), which shares some the core logic. That way we have less code to audit
https://docs.openzeppelin.com/contracts/5.x/api/access#AccessManager

Ramarti · 2024-01-20T01:54:00Z

contracts/AccessController.sol

+///
+/// Each policy is represented as a mapping from an IP account address to a signer address to a recipient
+/// address to a function selector to a permission level.
+/// The permission level can be 0 (ABSTAIN), 1 (ALLOW), or 2 (DENY).


ABSTAIN is confusing as a term

In access control systems, the term 'ABSTAIN' is commonly used to represent a specific decision state. Unlike 'ALLOW' or 'DENY', which are clear actions, 'ABSTAIN' indicates that the system does not make a definitive decision. This could be due to a lack of sufficient information, rules, or policies applicable to a particular request. It's a neutral state where the system neither grants nor denies access but leaves the decision to be made by a higher level in the decision-making hierarchy.

Ramarti · 2024-01-20T02:05:33Z

contracts/AccessController.sol

+    address public IP_ACCOUNT_REGISTRY;
+    address public MODULE_REGISTRY;
+
+    mapping(address => mapping(address => mapping(address => mapping(bytes4 => uint8)))) public permissions;


This 5 times nested mapping feels like a lot. Opportunities for optimization

Yes, we might consider if there is a room for optimization later.

kingster-will · 2024-01-20T03:41:14Z

I think this contract should inherit from OZ's AccessManager (later on), which shares some the core logic. That way we have less code to audit https://docs.openzeppelin.com/contracts/5.x/api/access#AccessManager

Based on my in-depth analysis of OpenZeppelin's AccessManager, I've identified certain limitations that make it less suitable for our project's specific requirements. Primarily, the AccessManager implements Role-Based Access Control (RBAC), which lacks support for wildcards. This limitation necessitates setting permissions on a per-function basis, which can be cumbersome and less efficient for our use case. Additionally, the AccessManager framework requires that all functions include the 'AccessManaged.restricted' modifier. Implementing this across community-developed modules would impose a significant overhead, both in terms of development effort and runtime costs. Given these considerations, although AccessManager offers robust access control, its structure and operational requirements do not align optimally with our project's needs and constraints.

kingster-will requested review from Ramarti, leeren, jdubpark and Spablob January 20, 2024 01:27

jdubpark reviewed Jan 20, 2024

View reviewed changes

kingster-will added 2 commits January 19, 2024 17:48

initial impl

ad508b4

Add AccessController and ModuleRegistry

22deeca

kingster-will force-pushed the access branch from 3209a88 to 22deeca Compare January 20, 2024 01:49

jdubpark reviewed Jan 20, 2024

View reviewed changes

jdubpark approved these changes Jan 20, 2024

View reviewed changes

leeren approved these changes Jan 20, 2024

View reviewed changes

Ramarti approved these changes Jan 20, 2024

View reviewed changes

kingster-will merged commit 25827dd into storyprotocol:main Jan 20, 2024
1 check passed

kingster-will deleted the access branch January 20, 2024 03:13

kingster-will self-assigned this Jan 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement AccessController and ModuleRegistry for IPAccount Execution Model #4

Implement AccessController and ModuleRegistry for IPAccount Execution Model #4

kingster-will commented Jan 20, 2024

jdubpark Jan 20, 2024

jdubpark Jan 20, 2024

jdubpark Jan 20, 2024

kingster-will Jan 20, 2024

jdubpark left a comment

leeren left a comment

leeren Jan 20, 2024

kingster-will Jan 20, 2024

leeren Jan 20, 2024

kingster-will Jan 20, 2024

leeren Jan 20, 2024

kingster-will Jan 20, 2024

leeren Jan 20, 2024

kingster-will Jan 20, 2024

leeren Jan 20, 2024

kingster-will Jan 20, 2024

leeren Jan 20, 2024

kingster-will Jan 20, 2024

Ramarti left a comment

Ramarti Jan 20, 2024

kingster-will Jan 20, 2024

Ramarti Jan 20, 2024

kingster-will Jan 20, 2024

kingster-will commented Jan 20, 2024

Implement AccessController and ModuleRegistry for IPAccount Execution Model #4

Implement AccessController and ModuleRegistry for IPAccount Execution Model #4

Conversation

kingster-will commented Jan 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdubpark left a comment

Choose a reason for hiding this comment

leeren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ramarti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingster-will commented Jan 20, 2024