Gas Optimization Full report

[hats-bug-reporter]

Github username: @saidqayoumsadat
Twitter username: S2AQ143
Submission hash (on-chain): 0xa9465f09b2906684a983510bc80a886bbe434fe0e236a2b930984a58e8cf3cf5
Severity: gas saving

Description:

Code changed Link: https://github.com/saidqayoumsadat/Palmera-contest

gas report link: https://github.com/saidqayoumsadat/Palmera-contest-gas-report

Summary

Gas Optimizations

	Issue	Instances	Total Gas Saved
[G001]	Cache Array Length Outside of Loop	14	42
[G002]	Using private rather than public for constants, saves gas	6	-
[G003]	Use assembly to check for address(0)	4	24
[G004]	Use calldata instead of memory for function arguments that do not get mutated	7	2520
[G005]	Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate	2	40084
[G006]	Multiple accesses of a mapping/array should use a local variable cache.	2	84
[G007]	Internal functions only called once can be inlined to save gas	4	80
[G008]	Optimize names to save gas	11	242
[G0009]	Structs should group like types together to save gas	1	-
[G010]	The result of function calls should be cached rather than re-calling the function	1	21
[G011]	Stack variable used as a cheaper cache for a state variable is only used once	1	268
[G012]	Constructors can be marked payable	4	84
[G013]	>= costs less gas than >	1	3
[G014]	Use assembly to emit events	2	-
[G015]	State variables only set in the constructor should be declared immutable	1	20000
[G016]	Don't use _msgSender() if not supporting EIP-2771	3	-
[G017]	Use uint256(1)/uint256(2) instead for true and false boolean states	1	17100
[G018]	++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops	1	60
[G019]	keccak256() should only need to be called on a specific string literal once	1	-
[G020]	Consider activating via-ir for deploying	1	-
[G021]	Consider using bytes32 rather than a string	3	-
[G022]	Inverting the condition of an if-else-statement	1	-
[G023]	unchecked {} can be used on the division of two uints in order to save gas	2	-
[G024]	Private functions used once can be inlined	1	-
[G025]	Use assembly to calculate hashes to save gas	6	480
[G026]	Avoid updating storage when the value hasn't changed	1	2900
[G027]	The use of a logical AND in place of double if is slightly less gas efficient in instances where there isn't a corresponding else statement for the given if statement	2	-
[G028]	Use the inputs/results of assignments rather than re-reading state variables	2	-
[G029]	State variable read in a loop	3	-
[G030]	Storage re-read via storage pointer	1	-
[G031]	State variables only set in the constructor should be declared immutable	2	-
[G032]	Use local variables for emitting	1	-

Total: 88 instances over 36 issues with 89246 gas saved.

Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.

G001 - Cache Array Length Outside of Loop:

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).

File: src/Helpers.sol


164             for (uint256 j; j < owners.length;) {

228                 for (uint256 i = 1; i < modules.length;) {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/Helpers.sol

File: src/PalmeraModule.sol


441             for (uint256 i; i < superSafe.child.length;) {
454             for (uint256 i; i < _safe.child.length;) {
625             for (uint256 i; i < oldSuper.child.length;) {
707             for (uint256 i; i < users.length;) {
966             for (uint256 i; i < orgHash.length;) {
992             for (uint256 i; i < indexSafe[org].length;) {
1015             for (uint256 i; i < orgHash.length;) {
1147            for (uint256 i; i < indexSafe[org].length;) {
1164            for (uint256 i; i < orgHash.length;) {
1229            for (uint256 i; i < indexSafeByOrg.length;) {
1246            for (uint256 i; i < indexSafeByOrg.length;) {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

File: src/ReentrancyAttack.sol


102              for (uint256 i; i < owners.length; ++i) {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol

G002 - Using private rather than public for constants, saves gas:

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

Click to show 6 findings

File: src/PalmeraGuard.sol


20     string public constant NAME = "Palmera Guard";

23     string public constant VERSION = "0.2.0";

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol

File: src/PalmeraModule.sol


27      string public constant NAME = "Palmera Module";

30      string public constant VERSION = "0.2.0";

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

File: src/PalmeraRoles.sol


16    string public constant NAME = "Palmera Roles";

19   string public constant VERSION = "0.2.0";

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraRoles.sol

G003 - Use assembly to check for address(0):

Saves 6 gas per instance

Click to show 4 findings

File: src/DenyHelper.sol


18              if (to == address(0) || to == Constants.SENTINEL_ADDRESS) {

50              if (wallet == address(0) || wallet == Constants.SENTINEL_ADDRESS) {

69                  && listed[org][wallet] != address(0) && wallet != address(0);

85                      && (currentWallet != address(0)) && (listCount[org] > 0)

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/DenyHelper.sol

File: src/Helpers.sol


34                  safe == address(0) || safe == Constants.SENTINEL_ADDRESS

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/Helpers.sol

File: src/PalmeraGuard.sol


25              if (palmeraModuleAddr == address(0)) {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol

File: src/PalmeraModule.sol


57              if (safes[getOrgBySafe(safe)][safe].safe == address(0)) {
67                  (safe == address(0)) || safe == Constants.SENTINEL_ADDRESS
81                  (safe == address(0)) || safe == Constants.SENTINEL_ADDRESS
100              if (authorityAddress == address(0)) {
248                 prevOwner == address(0) || ownerRemoved == address(0)
714                     wallet == address(0) || wallet == Constants.SENTINEL_ADDRESS
718                 if (listed[org][wallet] != address(0)) {
828             if (indexSafe[org].length == 0 || org == bytes32(0)) return false;
842             if (root == address(0) || safeId == 0) return false;
864             if (childSafe.safe == address(0)) return false;
893                 (childSafe.safe == address(0))
914                 (childSafe.safe == address(0))
926             if ((safe == address(0)) || safe == Constants.SENTINEL_ADDRESS) {
929             if (getOrgHashBySafe(safe) == bytes32(0)) return false;
1018                 if (safes[orgHash[i]][safeId].safe != address(0)) {
1025             if (orgSafe == bytes32(0)) revert Errors.SafeIdNotRegistered(safeId);
1039            if (_safe.safe == address(0)) return false;
1106            if (prevModule == address(0)) {
119                (_childSafe.safe == address(0))

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

G004 - Use calldata instead of memory for function arguments that do not get mutated:

Mark data types as calldata instead of memory where possible. This makes it so that the data is not automatically loaded into memory. If the data passed into the function does not need to be changed (like updating values in an array), it can be passed in as calldata. The one exception to this is if the argument must later be passed into another function that takes an argument that specifies memory storage.

Click to show 7 findings

File: src/PalmeraGuard.sol


52              bytes memory,
45              bytes memory,

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol

File: src/PalmeraModule.sol

138             bytes memory signatures

351         function addSafe(uint256 superSafeId, string memory name)

695         function addToList(address[] memory users)

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

File: src/ReentrancyAttack.sol

82              bytes memory signatures


100          function setOwners(address[] memory _owners) public {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol

File: src/SafeInterfaces.sol


55              bytes memory data,

26              bytes memory signatures

57              bytes memory signatures

68          function createProxy(address singleton, bytes memory data)

78              bytes memory initializer,

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/SafeInterfaces.sol

File: src/SigningUtils.sol


85              Transaction memory safeTx

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/SigningUtils.sol

G005 - Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate:

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

File: src/DenyHelper.sol


40          mapping(bytes32 => uint256) public listCount;
41      
42          /// @dev Mapping of Orgs to Wallets Deny or Allowed
43          /// @dev Org ID ---> Mapping of Orgs to Wallets Deny or Allowed
44          mapping(bytes32 => mapping(address => address)) internal listed;

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/DenyHelper.sol

File: src/PalmeraModule.sol


43          mapping(bytes32 => uint256[]) public indexSafe;
          /// @dev Depth Tree Limit
          /// @dev bytes32: Hash (On-chain Organisation) -> uint256: Depth Tree Limit
46          mapping(bytes32 => uint256) public depthTreeLimit;
          /// @dev Control Nonce of the Palmera Module per Org
          /// @dev bytes32: Hash (On-chain Organisation) -> uint256: Nonce by Orgt
49          mapping(bytes32 => uint256) public nonce;
          /// @dev Hash (On-chain Organisation) -> Safes
          /// @dev bytes32: Hash (On-chain Organisation).   uint256:SafeId of Safe Info
52          mapping(bytes32 => mapping(uint256 => DataTypes.Safe)) public safes;

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

G006 - Multiple accesses of a mapping/array should use a local variable cache.:

The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory/calldata

File: src/PalmeraModule.sol


184             nonce[org]++;
427             DataTypes.Safe storage _safe = safes[org][safeId];
462                 DataTypes.Safe storage childrenSafe = safes[org][_safe.child[i]];
445                     superSafe.child[i] = superSafe.child[superSafe.child.length - 1];
825             if (indexSafe[org].length == 0) removeOrg(org);
544                 disableSafeLeadRoles(safes[org][safe].safe);
629                     oldSuper.child[i] = oldSuper.child[oldSuper.child.length - 1];
687             depthTreeLimit[org] = newLimit;
728             listed[org][currentWallet] = Constants.SENTINEL_ADDRESS;
750             listCount[org] = listCount[org] > 1 ? listCount[org].sub(1) : 0;
749             listed[org][user] = address(0);
805                 safes[org][safeId].superSafe
970                     return orgHash[i];
996                     return indexSafe[org][i];
1019                     orgSafe = orgHash[i];
1097            delete safes[org][safeId];
1151                    indexSafe[org][i] = indexSafe[org][indexSafe[org].length - 1];
1152                    indexSafe[org].pop();
1168                    orgHash[i] = orgHash[orgHash.length - 1];
1253                    indexTree[index] = indexSafeByOrg[i];

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

File: src/PalmeraRoles.sol


256                 getUserRoles[user] &= ~bytes32(1 << role);

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraRoles.sol

G007 - Internal functions only called once can be inlined to save gas:

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

Click to show 4 findings

File: src/PalmeraRoles.sol


40          function setupRoles(address palmeraModule)

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraRoles.sol

File: src/ReentrancyAttack.sol


145         function setParamsForAttack(    //@audit this function removed for gas to inline.

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol

File: src/SigningUtils.sol


39          function _hashTypedDataV4(bytes32 domainSeparator, bytes32 structHash)

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/SigningUtils.sol

G008 - Optimize names to save gas:

public/external function names and public member variable names can be optimized to save gas.

Click to show 11 findings

File: src/DenyHelper.sol


14      abstract contract ValidAddress is Context {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/DenyHelper.sol

File: src/Helpers.sol


26      abstract contract Helpers is DenyHelper, SignatureDecoder, ReentrancyGuard {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/Helpers.sol

File: src/PalmeraGuard.sol


17      contract PalmeraGuard is BaseGuard, Context {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol

File: src/PalmeraModule.sol


21      contract PalmeraModule is Auth, Helpers {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

File: src/PalmeraRoles.sol


14      contract PalmeraRoles is RolesAuthority, ValidAddress {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraRoles.sol

File: src/ReentrancyAttack.sol


9       contract Attacker {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol

File: src/SigningUtils.sol


9       abstract contract SigningUtils {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/SigningUtils.sol

G009 - Structs should group like types together to save gas:

Structs can be more gas-efficient by grouping together members of the same type. This ordering can potentially save gas.

File: src/SigningUtils.sol


11          struct Transaction {
12              address to;
13              uint256 value;
14              bytes data;
15              Enum.Operation operation;
16              uint256 safeTxGas;
17              uint256 baseGas;
18              uint256 gasPrice;
19              address gasToken;
20              address refundReceiver;
21              bytes signatures;
22          }

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/SigningUtils.sol

G010 - The result of function calls should be cached rather than re-calling the function:

Caching the result of a function call in a local variable when the function is called multiple times can save gas due to avoiding the need to execute the function code multiple times.

File: src/PalmeraModule.sol


363                 revert Errors.SafeAlreadyRegistered(caller);

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

G011 - Stack variable used as a cheaper cache for a state variable is only used once:

If the variable is only accessed once, it's cheaper to use the state variable directly that one time, and save the 3 gas the extra stack assignment would spend.

File: src/PalmeraModule.sol


344             emit Events.RootSafeCreated(org, newIndex, caller, newRootSafe, name);              

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

G012 - Constructors can be marked payable:

Payable functions cost less gas to execute, since the compiler does not have to add extra checks to ensure that a payment wasn't provided. A constructor can safely be marked as payable, since only the deployer would be able to pass funds, and the project itself would not pass any funds.

Click to show 4 findings

File: src/PalmeraGuard.sol


25          constructor(address payable palmeraModuleAddr) {
              if (palmeraModuleAddr == address(0)) {
                  revert Errors.ZeroAddressProvided();
              }
              palmeraModule = PalmeraModule(palmeraModuleAddr);
          }

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol

File: src/PalmeraModule.sol


96          constructor(address authorityAddress, uint256 maxDepthTreeLimitInitial)
              Auth(address(0), Authority(authorityAddress))
          {
              if (authorityAddress == address(0)) {
                  revert Errors.InvalidAddressProvided();
              }
     
             rolesAuthority = authorityAddress;
             /// Index of Safes starts in 1 Always
             indexId = 1;
             maxDepthTreeLimit = maxDepthTreeLimitInitial;
         }

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

File: src/PalmeraRoles.sol


22          constructor(address palmeraModule)
              RolesAuthority(_msgSender(), Authority(address(0)))
          {
              setupRoles(palmeraModule);
          }

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraRoles.sol

File: src/ReentrancyAttack.sol


25          constructor(address payable _contractToAttackAddress) {
26              palmeraModule = PalmeraModule(_contractToAttackAddress);
27          }

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol

G013 - >= costs less gas than >:

The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas.

File: src/Helpers.sol


180                         (uint8 v1, bytes32 hashData) = v > 30
                             ? (
                                 v - 4,
                                 keccak256(
                                     abi.encodePacked(
                                         "\x19Ethereum Signed Message:\n32", dataHash
                                     )
                                     )
                             )
                             : (v, dataHash);

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/Helpers.sol

G014 - Use assembly to emit events:

Using the scratch space for event arguments (two words or fewer) will save gas over needing Solidity's full abi memory expansion used for emitting normally.

File: src/PalmeraModule.sol


191             emit Events.TxOnBehalfExecuted(
                 org, caller, superSafe, targetSafe, result
             );
323             emit Events.OrganisationCreated(caller, name, orgName);
344             emit Events.RootSafeCreated(org, newIndex, caller, newRootSafe, name);
399             emit Events.SafeCreated(
                 org, safeId, newSafe.lead, caller, superSafeId, name
             );
482             emit Events.SafeRemoved(
                 org, safeId, superSafe.lead, caller, _safe.superSafe, _safe.name
             );
551             emit Events.WholeTreeRemoved(
                 org, rootSafe, caller, safes[org][rootSafe].name
             );
591             emit Events.RootSafePromoted(
                 org, safeId, caller, newRootSafe.safe, newRootSafe.name
             );
660             emit Events.SafeSuperUpdated(
                 org,
                 safeId,
                 _safe.lead,
                 caller,
                 getSafeIdBySafe(org, oldSuper.safe),
                 newSuperId
             );

684             emit Events.NewLimitLevel(
                 org, rootSafe, caller, depthTreeLimit[org], newLimit
             );
730             emit Events.AddedToList(users);
751             emit Events.DroppedFromList(user);
1113            emit Events.SafeDisconnected(org, safeId, address(safeTarget), caller);

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

File: src/PalmeraRoles.sol


236             emit Events.PalmeraModuleSetup(palmeraModule, _msgSender());
259             emit UserRoleUpdated(user, role, enabled);

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraRoles.sol

G015 - State variables only set in the constructor should be declared immutable:

    Avoids a Gsset (**20000 gas**) in the constructor, and replaces the first access in each transaction (Gcoldsload - **2100 gas**) and each access thereafter (Gwarmacces - **100 gas**) with a `PUSH32` (**3 gas**). 

    While `string`s are not value types, and therefore cannot be `immutable`/`constant` if not hard-coded outside of the constructor, the same behavior can be achieved by making the current contract `abstract` with `virtual` functions for the `string` accessors, and having a child contract override the functions with the hard-coded implementation-specific values.

File: src/ReentrancyAttack.sol


27              palmeraModule = PalmeraModule(_contractToAttackAddress);

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol

G016 - Don't use _msgSender() if not supporting EIP-2771:

    Use `msg.sender` if the code does not implement [EIP-2771 trusted forwarder](https://eips.ethereum.org/EIPS/eip-2771) support

File: src/PalmeraGuard.sol


63              address caller = _msgSender();

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol

File: src/PalmeraModule.sol


149             address caller = _msgSender();
214             address caller = _msgSender();
246             address caller = _msgSender();
284             address caller = _msgSender();
317             address caller = _msgSender();
313             IsSafe(_msgSender())
361             address caller = _msgSender();
498             IsRootSafe(_msgSender())
502             address caller = _msgSender();
355             IsSafe(_msgSender())
361             address caller = _msgSender();
409             SafeRegistered(_msgSender())
412             address caller = _msgSender();
498             IsRootSafe(_msgSender())
502             address caller = _msgSender();
530         function removeWholeTree() external IsRootSafe(_msgSender()) requiresAuth {
5031             address caller = _msgSender();
563             IsRootSafe(_msgSender())
736         ) external validAddress(user) IsRootSafe(_msgSender()) requiresAuth {

             address caller = _msgSender();
             IsRootSafe(_msgSender())
             address caller = _msgSender();
             IsRootSafe(_msgSender())
             bytes32 org = getOrgHashBySafe(_msgSender());
             IsRootSafe(_msgSender())
             bytes32 org = getOrgHashBySafe(_msgSender());
             IsRootSafe(_msgSender())
             bytes32 org = getOrgHashBySafe(_msgSender());
         function enableAllowlist() external IsRootSafe(_msgSender()) requiresAuth {
             bytes32 org = getOrgHashBySafe(_msgSender());
         function enableDenylist() external IsRootSafe(_msgSender()) requiresAuth {
             bytes32 org = getOrgHashBySafe(_msgSender());
             IsRootSafe(_msgSender())
            address caller = _msgSender();

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

File: src/PalmeraRoles.sol


24              RolesAuthority(_msgSender(), Authority(address(0)))
236             emit Events.PalmeraModuleSetup(palmeraModule, _msgSender());

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraRoles.sol

G017 - Use uint256(1)/uint256(2) instead for true and false boolean states:

If you don't use boolean for storage you will avoid Gwarmaccess 100 gas. In addition, state changes of boolean from true to false can cost up to ~20000 gas rather than uint256(2) to uint256(1) that would cost significantly less.

File: src/DenyHelper.sol


35          mapping(bytes32 => bool) public allowFeature;
36          mapping(bytes32 => bool) public denyFeature;

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/DenyHelper.sol

G018 - ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops:

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop.

File: src/ReentrancyAttack.sol


102              for (uint256 i; i < owners.length; ++i) {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol

G019 - keccak256() should only need to be called on a specific string literal once:

It should be saved to an immutable variable, and the variable used instead. If the hash is being used as a part of a function selector, the cast to bytes4 should also only be done once.

File: src/SigningUtils.sol


93                          keccak256(
                              "execTransaction(address to,uint256 value,bytes data,Enum.Operation operation,uint256 safeTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,bytes signatures)"
                          ),

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/SigningUtils.sol

G020 - Consider activating via-ir for deploying:

    The IR-based code generator was introduced with an aim to not only allow code generation to be more transparent and auditable but also to enable more powerful optimization passes that span across functions.

    You can enable it on the command line using `--via-ir` or with the option `{"viaIR": true}`.

    This will take longer to compile, but you can just simple test it before deploying and if you got a better benchmark then you can add --via-ir to your deploy command

    More on: https://docs.soliditylang.org/en/v0.8.17/ir-breaking-changes.html

File: Various Files


None

G021 - Consider using bytes32 rather than a string:

Using the bytes types for fixed-length strings is more efficient than having the EVM have to incur the overhead of string processing. Consider whether the value needs to be a string. A good reason to keep it as a string would be if the variable is defined in an interface that this project does not own.

File: src/PalmeraGuard.sol


20          string public constant NAME = "Palmera Guard";
23          string public constant VERSION = "0.2.0";

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol

File: src/PalmeraModule.sol


27          string public constant NAME = "Palmera Module";
30          string public constant VERSION = "0.2.0";

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

File: src/PalmeraRoles.sol


16          string public constant NAME = "Palmera Roles";
19          string public constant VERSION = "0.2.0";

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraRoles.sol

G022 - Inverting the condition of an if-else-statement:

Flipping the true and false blocks instead saves 3 gas.

File: src/PalmeraModule.sol


71              } else if (!isSafeRegistered(safe)) {
83                  revert Errors.SafeNotRegistered(safe);


87              } else if (
85                  safes[getOrgHashBySafe(safe)][getSafeIdBySafe(
86                      getOrgHashBySafe(safe), safe
87                  )].tier != DataTypes.Tier.ROOT
88              ) {
89                  revert Errors.InvalidRootSafe(safe);
90              }

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

G023 - unchecked {} can be used on the division of two uints in order to save gas:

The division cannot overflow, since both the numerator and the denominator are non-negative.

File: src/Helpers.sol


156             uint256 count = signatures.length / 65;

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/Helpers.sol

G024 - Private functions used once can be inlined:

Private functions used once can be inlined to save GAS

File: src/PalmeraModule.sol


1146        function removeIndexSafe(bytes32 org, uint256 safeId) private {
            for (uint256 i; i < indexSafe[org].length;) {
                if (indexSafe[org][i] == safeId) {
                    indexSafe[org][i] = indexSafe[org][indexSafe[org].length - 1];
                    indexSafe[org].pop();
                    break;
                }
                unchecked {
                    ++i;
                }
            }
        }

1123        function getTreeMember(uint256 rootSafeId, uint256[] memory indexSafeByOrg)
            private
            view
            returns (uint256[] memory indexTree)
        {
            uint256 index;
            for (uint256 i; i < indexSafeByOrg.length;) {
                if (
                    (getRootSafe(indexSafeByOrg[i]) == rootSafeId)
                        && (indexSafeByOrg[i] != rootSafeId)
                ) {
                    unchecked {
                        ++index;
                    }
                }
                unchecked {
                    ++i;
                }
            }
            indexTree = new uint256[](index);
            index = 0;
            for (uint256 i; i < indexSafeByOrg.length;) {
                if (
                    (getRootSafe(indexSafeByOrg[i]) == rootSafeId)
                        && (indexSafeByOrg[i] != rootSafeId)
                ) {
                    indexTree[index] = indexSafeByOrg[i];
                    unchecked {
                        ++index;
                    }
                }
                unchecked {
                    ++i;
                }
            }
        }

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

G025 - Use assembly to calculate hashes to save gas:

Using assembly to calculate hashes can save 80 gas per instance

Click to show 6 findings

File: src/Helpers.sol


45              return keccak256(
46                  abi.encode(Constants.DOMAIN_SEPARATOR_TYPEHASH, getChainId(), this)
47              );


81              bytes32 palmeraTxHash = keccak256(
82                  abi.encode(
83                      Constants.PALMERA_TX_TYPEHASH,
84                      org,
85                      superSafe,
86                      targetSafe,
87                      to,
88                      value,
89                      keccak256(data),
90                      operation,
91                      _nonce
92                  )
93              );

119             return keccak256(
120                 encodeTransactionData(
121                     org, superSafe, targetSafe, to, value, data, operation, _nonce
122                 )
123             );


183                                 keccak256(
                                     abi.encodePacked(
                                         "\x19Ethereum Signed Message:\n32", dataHash
                                     )
                                     )

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/Helpers.sol

File: src/PalmeraModule.sol


175                     keccak256(palmeraTxHashData), signatures, leadSafe.getOwners()
178                     keccak256(palmeraTxHashData),
316             bytes32 name = keccak256(abi.encodePacked(orgName));
1060                ? bytes32(keccak256(abi.encodePacked(name)))

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

File: src/SigningUtils.sol


90                  keccak256(
                      abi.encode(
                          keccak256(
                              "execTransaction(address to,uint256 value,bytes data,Enum.Operation operation,uint256 safeTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,bytes signatures)"
                          ),
                          safeTx.to,
                          safeTx.value,
                          safeTx.data,
                          safeTx.operation,
                          safeTx.safeTxGas,
                          safeTx.baseGas,
                          safeTx.gasPrice,
                          safeTx.gasToken,
                          safeTx.refundReceiver,
                          safeTx.signatures
                      )
                  )

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/SigningUtils.sol

File: src/libraries/Constants.sol


26              keccak256(
27                  bytes("addOwnerWithThreshold(address,uint256,address,bytes32)")
28              )

32              keccak256(bytes("removeOwner(address,address,uint256,address,bytes32)"))
36              bytes4(keccak256(bytes("setRole(uint8,address,uint256,bool)")));
39              bytes4(keccak256(bytes("createRootSafe(address,string)")));
42              bytes4(keccak256(bytes("enableAllowlist()")));
45              bytes4(keccak256(bytes("enableDenylist()")));
48              bytes4(keccak256(bytes("disableDenyHelper()")));
51              bytes4(keccak256(bytes("addToList(address[])")));
54              bytes4(keccak256(bytes("dropFromList(address)")));
57              bytes4(keccak256(bytes("updateSuper(uint256,uint256)")));
60              bytes4(keccak256(bytes("promoteRoot(uint256)")));
63              bytes4(keccak256(bytes("updateDepthTreeLimit(uint256)")));
66              keccak256(
67                  bytes(
68                      "execTransactionOnBehalf(bytes32,address,address,address,uint256,bytes,uint8,bytes)"
69                  )
70              )
74              bytes4(keccak256(bytes("removeSafe(uint256)")));
77              bytes4(keccak256(bytes("removeWholeTree()")));
80              bytes4(keccak256(bytes("disconnectSafe(uint256)")));

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/libraries/Constants.sol

G026 - Avoid updating storage when the value hasn't changed:

If the old value is equal to the new value, not re-storing the value will avoid a Gsreset (2900 gas), potentially at the expense of a Gcoldsload (2100 gas) or a Gwarmaccess (100 gas).

File: src/ReentrancyAttack.sol


145         function setParamsForAttack(

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol

G027 - The use of a logical AND in place of double if is slightly less gas efficient in instances where there isn't a corresponding else statement for the given if statement:

Using a double if statement instead of logical AND (&&) can provide similar short-circuiting behavior whereas double if is slightly more efficient.

File: src/Helpers.sol


219             if ((modules.length == 0) && (nextModule == Constants.SENTINEL_ADDRESS))

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/Helpers.sol

File: src/PalmeraModule.sol


387             if (
                 (
                     !_authority.doesUserHaveRole(
                         superSafeOrgSafe.safe, uint8(DataTypes.Role.SUPER_SAFE)
                     )
                 ) && (superSafeOrgSafe.child.length > 0)

421             if (
                 (!isRootSafeOf(caller, safeId))
                     && (!isSuperSafe(callerSafe, safeId))

429             if (
                 ((_safe.tier == DataTypes.Tier.ROOT) || (_safe.superSafe == 0))
                     && (_safe.child.length > 0)
                 (
509                     (!isRootSafeOf(caller, safeId))
                         && (_disconnectSafe.tier != DataTypes.Tier.REMOVED)

703             if (!allowFeature[org] && !denyFeature[org]) {

742             if (!allowFeature[org] && !denyFeature[org]) {

1062            if (isOrgRegistered(org) && caller == newRootSafe) {

1232                if (
                    (getRootSafe(indexSafeByOrg[i]) == rootSafeId)
                        && (indexSafeByOrg[i] != rootSafeId)

1249                if (
                    (getRootSafe(indexSafeByOrg[i]) == rootSafeId)
                        && (indexSafeByOrg[i] != rootSafeId)

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

G028 - Use the inputs/results of assignments rather than re-reading state variables:

When a state variable is assigned, it saves gas to use the value being assigned, later in the function, rather than re-reading the state variable itself. If needed, it can also be stored to a local variable, and be used in that way. Both options avoid a Gwarmaccess (100 gas). Note that if the operation is, say +=, the assignment also results in a value which can be used. The instances below point to the first reference after the assignment, since later references are already covered by issues describing the caching of state variable values.

File: src/PalmeraGuard.sol


67                  if (!ISafe(caller).isModuleEnabled(address(palmeraModule))) {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol

File: src/ReentrancyAttack.sol


32              if (address(targetSafeFromAttacker).balance > 0 gwei) {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol

G029 - State variable read in a loop:

The state variable should be cached in and read from a local variable, or accumulated in a local variable then written to storage once outside of the loop, rather than reading/updating it on every iteration of the loop, which will replace each Gwarmaccess (100 gas) with a much cheaper stack read.

File: src/PalmeraGuard.sol


84                      for (uint256 i = 1; i < palmeraModule.indexId();) {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol

File: src/PalmeraModule.sol


462                 DataTypes.Safe storage childrenSafe = safes[org][_safe.child[i]];

538                 DataTypes.Safe memory _safe = safes[org][safe];

966             for (uint256 i; i < orgHash.length;) {

992             for (uint256 i; i < indexSafe[org].length;) {
1015             for (uint256 i; i < orgHash.length;) {
1147            for (uint256 i; i < indexSafe[org].length;) {
1164            for (uint256 i; i < orgHash.length;) {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

File: src/ReentrancyAttack.sol


102              for (uint256 i; i < owners.length; ++i) {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol

G030 - Storage re-read via storage pointer:

The instances below point to the second+ access of a state variable, via a storage pointer, within a function. Caching the value replaces each Gwarmaccess (100 gas) with a much cheaper stack read.

File: src/PalmeraModule.sol

441             for (uint256 i; i < superSafe.child.length;) {

454             for (uint256 i; i < _safe.child.length;) {

625             for (uint256 i; i < oldSuper.child.length;) {

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

G031 - State variables only set in the constructor should be declared immutable:

Avoids a Gsset (20000 gas) in the constructor, and replaces the first access in each transaction (Gcoldsload - 2100 gas) and each access thereafter (Gwarmacces - 100 gas) with a PUSH32 (3 gas). While strings are not value types, and therefore cannot be immutable/constant if not hard-coded outside of the constructor, the same behavior can be achieved by making the current contract abstract with virtual functions for the string accessors, and having a child contract override the functions with the hard-coded implementation-specific values.

File: src/ReentrancyAttack.sol


23          PalmeraModule public palmeraModule;

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol

G032 - Use local variables for emitting:

Use the function/modifier's local copy of the state variable, rather than incurring an extra Gwarmaccess (100 gas). In the unlikely event that the state variable hasn't already been used by the function/modifier, consider whether it is really necessary to include it in the event, given the fact that it incurs a Gcoldsload (2100 gas), or whether it can be passed in to or back out of the functions that do use it

File: src/PalmeraModule.sol


552                 org, rootSafe, caller, safes[org][rootSafe].name
685                 org, rootSafe, caller, depthTreeLimit[org], newLimit

https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol

[0xRizwan]

Gas optimization changes should not use assembly. Based on contest rules, its prohibited to use assembly for gas saving.

Optimizations should use solidity (no inline assembly)

cc- @alfredolopez80

[saidqayoumsadat]

Gas optimization changes should not use assembly. Based on contest rules, its prohibited to use assembly for gas saving.

Optimizations should use solidity (no inline assembly)

cc- @alfredolopez80

What , could you explain it

[alfredolopez80]

The Unit-Test Fail!!