Add AccessControlDefaultAdminRules

[ernestognw]

Fixes #3623
Fixes LIB-618

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: b164929

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Amxx]

It feels to me that this contract is safe, but that this safety relies on the functions not being virtual, and on the user not doing "tricks".

As a dev, I just need to call AccessControl._grantRole(...) instead of _grantRole(...) to add a second admin.

[ernestognw]

It feels to me that this contract is safe, but that this safety relies on the functions not being virtual, and on the user not doing "tricks".

Sorry, it was not intentional to avoid virtual, the contract should still be safe. However, do you have any concerns with some override?

As a dev, I just need to call AccessControl._grantRole(...) instead of _grantRole(...) to add a second admin.

Hmm, that's correct, but I wonder why that might happen.

[frangio]

As a dev, I just need to call AccessControl._grantRole(...) instead of _grantRole(...) to add a second admin.

This same argument applies to all of our extensions. You can probably mess up ERC20Votes or something by calling ERC20._transfer somewhere and end up with nearly infinite votes (not saying this exact thing is possible). I don't think it's something that is within our threat model at all.

Our guideline is to design contracts that are difficult to misuse, not impossible to misuse or to abuse.

[ernestognw]

Reviewing the docs made me think that the delay should be configurable... That said I want us to merge this PR and we can discuss it as a potential follow up. But let me know your thoughts.

We can discuss it further. The initial reason for not doing it was avoiding the management it comes with.
That said, we can lock the delay if there's no started transfer if that makes sense to you.

If we'll do it. I'd like to do it in a follow-up PR.

[frangio]

This is good to merge, but we should probably follow up with modifiable delay in another PR.

We could also review the naming for "delayed until".

[frangio]

LGTM. Also @Amxx agreed to merge. We will follow up with configurable delay in another PR, and a final review round.

[frangio]

There was a flaky test because of the use of time.increaseTo, which mines a block with the argument timestamp, but does not guarantee the timestamp of the following block. Also due to how time.latest() was being used to compute the "delayed until" timestamp, it needs to be called after the transaction that begins the transfer because latest returns the timestamp of the last block.