Skip to content
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

DO NOT MERGE!!!! - New access control #33

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

0xTranqui
Copy link
Member

@0xTranqui 0xTranqui commented Oct 31, 2022

DO NOT MERGE !!!! - setting this pull request up to track updates that need to happen before it is ready

Scope:

This PR overhauls how access control is handled in Curator.sol. It allows a user to initialize their own access control pattern via an external contract, that can use arbitrary logic as long as it is compatible with this interface. The accessControl pattern that has been set up is generic and can be used in any smart contract, and this curation protocol will be the first to use it

To Dos Before Merging:

  1. Update all of the tests to check that this new access control works as intended. Should be done for at least 2 default access control scenarios, specifically ERC721AccessControl and ERC20MinBalAccessControl.
  2. Related to 1, add tests to determine that upgrade paths for previously existing curation contracts (neosound.xyz + public---assembly.com) will not be broken post upgrade
  3. Related to 2, make sure that listingRecord tokens are still rendering properly in marketplaces like Opensea from curation contracts that are intialized with the now updated SVGMetadataRenderer. Tokens should render like this when working properly

Questions to answer before merging:

  1. Should we add enum or public variables to make the accessControl logic more readable? Currently accessControl is being used in Curator.sol by comparing results of getAccessLevel to hardcoded access level numbers. In the current, its hard/impossible for users to know that 1 = curator, 2 = manager, 3 = admin (could also be helpful to add comments that explicitly state the functions each level has access to). Screen Shot 2022-10-30 at 6 18 12 PM
  2. Should authorizeUpgrade remain as onlyOwner, or be moved over to onlyOwnerOrAdminAccess? I have kept it as onlyOwner for now because until there is a better way of issuing non-transferrable tokens specifically for the Admin role, feels like a major security vulnerability to distribute passes that can be transferred/sold/stolen that enable upgrade access
  3. Should owner be able to access curation functionality like in previous Curator.sol impl? Unlike in point 2, I have removed the ability for the contract owner to access the addListing function, which I think is actually ok because it makes the contract fully reliant on the accessControl functionality to deal with curation functions (not a security vulnerability)
  4. A few storage gaps might needed to be updated due to changes in parameters. @iainnash I am completely unaware of how this works, so could use your eyes on this (see screenshot for the 2 places this shows up)

Screen Shot 2022-10-30 at 6 24 00 PM

5. Think we should push a "DefaultAccessControl.sol" file that can be used without an accessControlInitializer in the factory to provide a generic, onlyOwner based access control strategy. This can then be used as the default access control that is triggered if ppl pass the address(0) value + empty accessControlInitializer into the CuratorFactory 6. *not related to upgrade* - CurationFactory uses the variable “ address curationManger” to initialize the owner of the proxy curation contract. Feels potentially confusing? Do we change it to curationContractOwner maybe? Dont really like that either...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant