Feat/accesscontrol temporal facets#197
Conversation
|
| Name | Link |
|---|---|
| 🔨 Latest commit | 9323621 |
Coverage Report
Last updated: Thu, 13 Nov 2025 05:14:10 GMT for commit |
Gas ReportNo gas usage changes detected between All functions maintain the same gas costs. ✅ Last updated: Thu, 13 Nov 2025 05:14:34 GMT for commit |
farismln
left a comment
farismln
left a comment
There was a problem hiding this comment.
hey thanks for the good work. I notice unintended outcome on the revokeTemporalRole function in certain cases. please check it out
| bool _hasRole = acs.hasRole[_account][_role]; | ||
| if (_hasRole) { | ||
| acs.hasRole[_account][_role] = false; | ||
| } | ||
|
|
||
| // Clear expiry timestamp | ||
| s.roleExpiry[_account][_role] = 0; |
There was a problem hiding this comment.
there are security risk here, if _hasRole is already false it would still clear the expiry timestamp, making the said _account became permanent in the roles.
consider to only clear the expiry timestamp if _hasRole is true
| // Revoke the role if it exists | ||
| if (_hasRole) { | ||
| acs.hasRole[_account][_role] = false; | ||
| } | ||
|
|
||
| // Clear expiry timestamp | ||
| s.roleExpiry[_account][_role] = 0; |
There was a problem hiding this comment.
this also have the same issue with the non Lib contract
|
@farismln changes are made please check the new commit |
|
hey @isihin-3 the fix looks good! also, I just realized for the revoke role that clearing the expiry is not necessary. what do you think? do you have any reason to clear the expiry time to 0 when revoking a role? |
|
Hey @farismln , that's a great point on the gas angle. I looked into it, and I think we must keep the expiry clear for two big reasons:
Given the low cost, I think it's safer to clear the slot to 0 to prevent the re-grant bug and keep the state clean for off-chain tools. What do you think? |
|
yeah fair point you mention @isihin-3 edit: maybe we can implement using 0 as expiry for it to be a permanent role. but maybe this would conflict with the normal access control behavior as there would be two way a permanent role can be granted when using the temporal one. but this would not be necessary if we think this temporal access control as extension of the normal facet. |
The user has to check which facet is taken in use. According to that they can decide |
farismln
left a comment
farismln
left a comment
There was a problem hiding this comment.
the changes fixed the issue from before. looks good!!
| if (_hasRole) { | ||
| // Revoke the role from AccessControl storage | ||
| acs.hasRole[_account][_role] = false; | ||
|
|
||
| // Clear expiry timestamp | ||
| s.roleExpiry[_account][_role] = 0; | ||
|
|
||
| emit TemporalRoleRevoked(_role, _account, msg.sender); | ||
| } |
There was a problem hiding this comment.
looks good. thanks for the fix
| bool _hasRole = acs.hasRole[_account][_role]; | ||
| if (!_hasRole) { | ||
| return false; | ||
| } | ||
|
|
||
| acs.hasRole[_account][_role] = false; | ||
|
|
||
| // Clear expiry timestamp only when the role existed | ||
| s.roleExpiry[_account][_role] = 0; |
There was a problem hiding this comment.
the approach different with the non lib ones where this is return early. but the issue are fixed on both cases. thanks
|
hi @mudgen I already reviewed the fixes. |
3 participants
Summary
Introduced temporal access control functionality with supporting facets, library helpers, and comprehensive tests to manage role expirations.
Changes Made
AccessControlTemporalFacetproviding grant/revoke flows with expiry validation.LibAccessControlTemporalexposing reusable storage helpers and events.Checklist
Before submitting this PR, please ensure:
Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions,
using fordirectives, orselfdestructCode follows Design Principles - Readable, uses diamond storage, favors composition over inheritance
Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)
Code is formatted with
forge fmtTests are included - All new functionality has comprehensive tests
All tests pass - Run
forge testand ensure everything worksDocumentation updated - If applicable, update relevant documentation
Additional Notes
No documentation updates were required