Skip to content

Commit

Permalink
[Certora Audit] G-09. Cache array length outside of loop (#896)
Browse files Browse the repository at this point in the history
This pull request includes several optimizations to the `OwnerManager`
and `ERC165Handler` contracts by reducing the number of times array
lengths are accessed within loops. These changes aim to improve the
efficiency of the code by storing the array lengths in variables before
the loops.

If not cached, the solidity compiler will always read the length of the
array during each iteration. That is, if it is a storage array, this is
an extra sload operation (100 additional extra gas for each iteration
except for the first) and if it is a memory array, this is an extra
mload operation (3 additional gas for each iteration except for the
first).

Optimizations in `OwnerManager` contract:

*
[`contracts/base/OwnerManager.sol`](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L38-R39):
Stored `_owners.length` in a variable `ownersLength` before the loop to
avoid repeatedly accessing the array length.
*
[`contracts/base/OwnerManager.sol`](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L49-R50):
Updated `ownerCount` to use the `ownersLength` variable instead of
accessing `_owners.length` again.

Optimizations in `ERC165Handler` contract:

*
[`contracts/handler/extensible/ERC165Handler.sol`](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L56-R57):
Stored `handlerWithSelectors.length` in a variable `len` before the loop
in `addSupportedInterfaceBatch` to avoid repeatedly accessing the array
length.
*
[`contracts/handler/extensible/ERC165Handler.sol`](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L78-R80):
Stored `selectors.length` in a variable `len` before the loop in
`removeSupportedInterfaceBatch` to avoid repeatedly accessing the array
length.
  • Loading branch information
remedcu authored Jan 9, 2025
1 parent e35793d commit b2c6087
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 4 deletions.
5 changes: 3 additions & 2 deletions contracts/base/OwnerManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ abstract contract OwnerManager is SelfAuthorized, IOwnerManager {
if (_threshold == 0) revertWithError("GS202");
// Initializing Safe owners.
address currentOwner = SENTINEL_OWNERS;
for (uint256 i = 0; i < _owners.length; i++) {
uint256 ownersLength = _owners.length;
for (uint256 i = 0; i < ownersLength; i++) {
// Owner address cannot be null.
address owner = _owners[i];
if (owner == address(0) || owner == SENTINEL_OWNERS || owner == address(this) || currentOwner == owner)
Expand All @@ -46,7 +47,7 @@ abstract contract OwnerManager is SelfAuthorized, IOwnerManager {
currentOwner = owner;
}
owners[currentOwner] = SENTINEL_OWNERS;
ownerCount = _owners.length;
ownerCount = ownersLength;
threshold = _threshold;
}

Expand Down
6 changes: 4 additions & 2 deletions contracts/handler/extensible/ERC165Handler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ abstract contract ERC165Handler is ExtensibleBase, IERC165Handler {
function addSupportedInterfaceBatch(bytes4 _interfaceId, bytes32[] calldata handlerWithSelectors) external override onlySelf {
ISafe safe = ISafe(payable(_msgSender()));
bytes4 interfaceId;
for (uint256 i = 0; i < handlerWithSelectors.length; i++) {
uint256 len = handlerWithSelectors.length;
for (uint256 i = 0; i < len; i++) {
(bool isStatic, bytes4 selector, address handlerAddress) = MarshalLib.decodeWithSelector(handlerWithSelectors[i]);
_setSafeMethod(safe, selector, MarshalLib.encode(isStatic, handlerAddress));
if (i > 0) {
Expand All @@ -77,7 +78,8 @@ abstract contract ERC165Handler is ExtensibleBase, IERC165Handler {
function removeSupportedInterfaceBatch(bytes4 _interfaceId, bytes4[] calldata selectors) external override onlySelf {
ISafe safe = ISafe(payable(_msgSender()));
bytes4 interfaceId;
for (uint256 i = 0; i < selectors.length; i++) {
uint256 len = selectors.length;
for (uint256 i = 0; i < len; i++) {
_setSafeMethod(safe, selectors[i], bytes32(0));
if (i > 0) {
interfaceId ^= selectors[i];
Expand Down

0 comments on commit b2c6087

Please sign in to comment.