AccessControlFacetStorage Code Style Findings

ACS-01C: Non-Standard Storage Slot Definition

Type	Severity	Location
Standard Conformity		AccessControlFacetStorage.sol:L60

Description:

The referenced declaration will define a storage slot for use by a facet of the system's main EIP-2535 Diamond, however, the way it is declared does not adhere to the latest standards.

Example:

Copy
59/// @dev Unique identifier for the storage slot where the Layout struct is stored.
60bytes32 internal constant STORAGE_SLOT = keccak256("evergonlabs.Staking.storage.AccessControlFacetStorage");

Recommendation:

We advise the EIP-7201 name-spaced layout approach to be adhered to similarly to OpenZeppelin and other relevant standard libraries, ensuring consistency among the ecosystem's widely utilized libraries and conforming to the latest standards.

Alleviation (b64b659786cf3c84bea52feb3a69f546ba3601f0):

The referenced slot definition has been updated to its standardized EIP-7201 representation, addressing this exhibit.

ACS-02C: Non-Streamlined Code

Type	Severity	Location
Standard Conformity		AccessControlFacetStorage.sol:L110, L160, L162

Description:

The AccessControlFacetStorage::hasRole and AccessControlFacetStorage::onlyRole functions do not contain streamlined code as both repeat the same statements with the former containing two return statements inefficiently.

Example:

Copy
159function hasRole(Layout storage l, bytes32 role, address account) internal view returns (bool) {
160    if (role == OPEN_ROLE) return true;
161
162    return l.hasRole_[account][role];
163}

Recommendation:

We advise the AccessControlFacetStorage::hasRole function to join the two return statements into one using the OR conditional clause (||), and the AccessControlFacetStorage::onlyRole function to invoke the AccessControlFacetStorage::hasRole function greatly increasing the code's legibility and standardization.

Alleviation (b64b659786cf3c84bea52feb3a69f546ba3601f0):

The AccessControlFacetStorage::onlyRole function properly invokes the AccessControlFacetStorage::hasRole function to evaluate whether the caller has the relevant role, standardizing the codebase.

ACS-03C: Potentially Inefficient Mapping Layouts

Type	Severity	Location
Gas Optimization		AccessControlFacetStorage.sol: • I-1: L238 • I-2: L267 • I-3: L323 • I-4: L352

Description:

The referenced mapping declarations are inefficiently ordered as they do not follow the optimal widening approach.

Specifically, mapping relations should be declared from the lowest subset to the greatest subset to permit caching of keccak256 offsets and thus optimizations.

Example:

Copy
199/**
200 * @notice Grants `role` to `account` within the context of a specified staking campaign.
201 * @dev Only callable by the Admin or a handler of `role`.
202 * @param l A reference to the Layout struct in storage.
203 * @param campaignId The unique identifier of the targeted staking campaign.
204 * @param role The identifier of the role to grant.
205 * @param account The address to grant `role` to.
206 */
207function grantRoleForId(
208    Layout storage l,
209    uint256 campaignId,
210    bytes32 role,
211    address account
212) internal onlyRoleHandler(role) {
213    if (campaignId == 0) {
214        revert InvalidCampaignIdZero();
215    }
216    l.hasRoleForId_[campaignId][account][role] = true;
217}
218
219/**
220 * @notice Grants `role` to multiple `accounts` within the context of the staking platform.
221 * @dev Only callable by the Admin or a handler of `role`.
222 * @param l A reference to the Layout struct in storage.
223 * @param role The identifier of the role to grant.
224 * @param accounts An array of addresses to grant `role` to.
225 */
226function grantRoleMultiple(
227    Layout storage l,
228    bytes32 role,
229    address[] calldata accounts
230) internal onlyRoleHandler(role) {
231    uint256 length = accounts.length;
232
233    if (length == 0) {
234        revert ZeroAccountsToGrant();
235    }
236
237    for (uint256 i = 0; i < length; i++) {
238        l.hasRole_[accounts[i]][role] = true;
239    }
240}

Recommendation:

The hasRole_ mapping should be point a role to the accounts that possess it permitting the hasRole_[role] mapping to be cached in the referenced instances, and the hasRoleForId_ should be similarly restructured to point a campaign ID to a role to the accounts that possess it allowing the same optimization to be applied.

Alleviation (b64b659786):

While the mapping declarations themselves were updated as advised, the mapping offset caching optimizations have not been applied.

Alleviation (5312126fb2):

The mapping optimizations have been applied as advised, greatly reducing the gas cost involved in each for loop that repetitively accesses subsets of the mapping.

ACS-04C: Reduced Legibility of Conditionals

Type	Severity	Location
Code Style		AccessControlFacetStorage.sol:L95, L110

Description:

The referenced conditionals will perform a negation on a multi-component logical joint statement which we believe hurts the code's readability.

Example:

Copy
95if (!(layout().canHandleRole[msg.sender][role] || layout().hasRole_[msg.sender][ADMIN_ROLE])) {

Recommendation:

We advise the negation to be incorporated into the condition itself, increasing the logical operations by one but significantly increasing the code's legibility.

Alleviation (b64b659786cf3c84bea52feb3a69f546ba3601f0):

The code was updated to utilize an interim bool variable instead that is negated, properly depicting its value via its name and thus addressing the exhibits legibility concerns.

ACS-05C: Repetitive Invocations of Layout

Type	Severity	Location
Gas Optimization		AccessControlFacetStorage.sol:L95

Description:

The referenced statements will repetitively invoke the facet's layout storage pointer inefficiently.

Example:

Copy
95if (!(layout().canHandleRole[msg.sender][role] || layout().hasRole_[msg.sender][ADMIN_ROLE])) {

Recommendation:

We advise it to be invoked once, stored to a local variable, and consequently re-used as many times as needed.

Alleviation (b64b659786cf3c84bea52feb3a69f546ba3601f0):

The layout pointer is properly cached optimizing the code's gas cost.