Omniscia 0xPhase Audit

AccessControl Code Style Findings

AccessControl Code Style Findings

ACL-01C: Inefficient mapping Lookups

TypeSeverityLocation
Gas OptimizationAccessControl.sol:L225, L226, L244, L245

Description:

The linked statements perform key-based lookup operations on mapping declarations from storage multiple times for the same key redundantly.

Example:

core/AccessControl.sol
224function _grantRoleAccount(bytes32 role, address account) internal virtual {
225  if (!_acs().roles[role].members[account]) {
226    _acs().roles[role].members[account] = true;
227    emit RoleAccountGranted(role, account, msg.sender);
228  }
229}

Recommendation:

As the lookups internally perform an expensive keccak256 operation, we advise the lookups to be cached wherever possible to a single local declaration that either holds the value of the mapping in case of primitive types or holds a storage pointer to the struct contained.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

All referenced mapping lookups have been optimized as advised, utilizing interim variables for iterative mapping declarations and caching values to local variables where applicable.

ACL-02C: Loop Iterator Optimization

TypeSeverityLocation
Gas OptimizationAccessControl.sol:L191

Description:

The linked for loop increments / decrements the iterator "safely" due to Solidity's built-in safe arithmetics (post-0.8.X).

Example:

core/AccessControl.sol
191for (uint256 i = 0; i < length; i++) {

Recommendation:

We advise the increment / decrement operation to be performed in an unchecked code block as the last statement within the for loop to optimize its execution cost.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

All referenced loop iterators have been optimized as advised, removing their for declaration increment statement and instead performing it in an unchecked code block wherever needed (i.e. before a continue statement or at the end of the for loop's body).

ACL-03C: Multiple Top-Level Declarations

TypeSeverityLocation
Code StyleAccessControl.sol:L21, L109

Description:

The referenced file contains multiple top-level declarations that decrease the legibility of the codebase.

Example:

core/AccessControl.sol
21interface IAccessControl {
22  /// @notice Event emitted when the admin of a role changes
23  /// @param role The role that changed
24  /// @param previousAdminRole The previous admin role
25  /// @param newAdminRole The new admin role
26  event RoleAdminChanged(
27    bytes32 indexed role,
28    bytes32 indexed previousAdminRole,
29    bytes32 indexed newAdminRole
30  );
31

32  /// @notice Event emitted when the role is granted to an account
33  /// @param role The role that was granted
34  /// @param account The account the role was granted to
35  /// @param sender The message sender
36  event RoleAccountGranted(
37    bytes32 indexed role,
38    address indexed account,
39    address indexed sender
40  );
41

42  /// @notice Event emitted when the role is revoked from an account
43  /// @param role The role that was revoked
44  /// @param account The account the role was revoked from
45  /// @param sender The message sender
46  event RoleAccountRevoked(
47    bytes32 indexed role,
48    address indexed account,
49    address indexed sender
50  );
51

52  /// @notice Event emitted when the role is granted to a DB key
53  /// @param role The role that was granted
54  /// @param key The DB key the role was granted to
55  /// @param sender The message sender
56  event RoleKeyGranted(
57    bytes32 indexed role,
58    bytes32 indexed key,
59    address indexed sender
60  );
61

62  /// @notice Event emitted when the role is revoked from a DB key
63  /// @param role The role that was revoked
64  /// @param key The DB key the role was revoked from
65  /// @param sender The message sender
66  event RoleKeyRevoked(
67    bytes32 indexed role,
68    bytes32 indexed key,
69    address indexed sender
70  );
71

72  /// @notice Grants the role to the account
73  /// @param role The granted role
74  /// @param account The account the role is granted to
75  function grantRoleAccount(bytes32 role, address account) external;
76

77  /// @notice Grants the role to the DB key
78  /// @param role The granted role
79  /// @param key The DB key the role is granted to
80  function grantRoleKey(bytes32 role, bytes32 key) external;
81

82  /// @notice Revokes the role from the account
83  /// @param role The revoked role
84  /// @param account The account the role is revoked from
85  function revokeRoleAccount(bytes32 role, address account) external;
86

87  /// @notice Revokes the role from the DB key
88  /// @param role The revoked role
89  /// @param key The DB key the role is revoked from
90  function revokeRoleKey(bytes32 role, bytes32 key) external;
91

92  /// @notice Removes the role from the message sender
93  /// @param role The revoked role
94  /// @param account The message sender
95  function renounceRole(bytes32 role, address account) external;
96

97  /// @notice Checks if the account has the role
98  /// @param role The role the account is checked against
99  /// @param account The
100  /// @return If the account has the role
101  function hasRole(bytes32 role, address account) external view returns (bool);
102

103  /// @notice Gets the admin of the role
104  /// @param role The role to get the admin for
105  /// @return The admin of the role
106  function getRoleAdmin(bytes32 role) external view returns (bytes32);
107}
108

109contract AccessControl is IAccessControl, ERC165, Element {

Recommendation:

We advise all highlighted top-level declarations to be split into their respective code files, avoiding unnecessary imports as well as increasing the legibility of the codebase.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The IAccessControl interface was properly split to its dedicated file and is now imported by the AccessControl codebase, rendering this exhibit addressed.

ACL-04C: Redundant Repetitive Invocations of Getter Function

TypeSeverityLocation
Gas OptimizationAccessControl.sol:L192

Description:

The Element::db getter function is redundantly invoked multiple times within AccessControl::hasRole on each iteration.

Example:

core/AccessControl.sol
179/// @inheritdoc IAccessControl
180function hasRole(
181  bytes32 role,
182  address account
183) public view virtual override returns (bool) {
184  RoleData storage roleData = _acs().roles[role];
185

186  if (roleData.members[account]) return true;
187

188  bytes32 addr = bytes32(bytes20(account));
189  uint256 length = roleData.keys.length();
190

191  for (uint256 i = 0; i < length; i++) {
192    if (db().hasPair(roleData.keys.at(i), addr)) return true;
193  }
194

195  return false;
196}

Recommendation:

We advise it to be invoked once and stored to a local variable that is consequently utilized within the for loop, optimizing the code's gas cost.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The result of the _db call is properly cached outside the for loop in the updated code, optimizing each iteration's gas cost.

Prev

VaultSettersFacet.sol (VSF-M)

Next

AccessControlBase.sol (ACB-C)

On this page