Open hats-bug-reporter[bot] opened 4 months ago
0xRizwan
commented
4 months ago 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
commented
4 months ago 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
commented
4 months ago The Unit-Test Fail!!
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
privaterather thanpublicfor constants, saves gasaddress(0)>=costs less gas than>immutable_msgSender()if not supporting EIP-2771uint256(1)/uint256(2)instead fortrueandfalseboolean states++i/i++should beunchecked{++i}/unchecked{i++}when it is not possible for them to overflow, as is the case when used infor- andwhile-loopskeccak256()should only need to be called on a specific string literal oncevia-irfor deployingunchecked {}can be used on the division of twouintsin order to save gasTotal: 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).
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/Helpers.sol
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol
G002 - Using
privaterather thanpublicfor 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
```solidity 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 ```solidity 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 ```solidity 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.solG003 - Use assembly to check for
address(0):Saves 6 gas per instance
Click to show 4 findings
```solidity 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 ```solidity File: src/Helpers.sol 34 safe == address(0) || safe == Constants.SENTINEL_ADDRESS ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/Helpers.sol ```solidity File: src/PalmeraGuard.sol 25 if (palmeraModuleAddr == address(0)) { ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol ```solidity 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.solG004 - Use
calldatainstead ofmemoryfor function arguments that do not get mutated:Mark data types as
calldatainstead ofmemorywhere 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 ascalldata. The one exception to this is if the argument must later be passed into another function that takes an argument that specifiesmemorystorage.Click to show 7 findings
```solidity File: src/PalmeraGuard.sol 52 bytes memory, 45 bytes memory, ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol ```solidity 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 ```solidity 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 ```solidity 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 ```solidity File: src/SigningUtils.sol 85 Transaction memory safeTx ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/SigningUtils.solG005 - 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.
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/DenyHelper.sol
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
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol
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
```solidity File: src/PalmeraRoles.sol 40 function setupRoles(address palmeraModule) ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraRoles.sol ```solidity 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 ```solidity File: src/SigningUtils.sol 39 function _hashTypedDataV4(bytes32 domainSeparator, bytes32 structHash) ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/SigningUtils.solG008 - 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
```solidity File: src/DenyHelper.sol 14 abstract contract ValidAddress is Context { ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/DenyHelper.sol ```solidity File: src/Helpers.sol 26 abstract contract Helpers is DenyHelper, SignatureDecoder, ReentrancyGuard { ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/Helpers.sol ```solidity File: src/PalmeraGuard.sol 17 contract PalmeraGuard is BaseGuard, Context { ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol ```solidity File: src/PalmeraModule.sol 21 contract PalmeraModule is Auth, Helpers { ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol ```solidity File: src/PalmeraRoles.sol 14 contract PalmeraRoles is RolesAuthority, ValidAddress { ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraRoles.sol ```solidity File: src/ReentrancyAttack.sol 9 contract Attacker { ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol ```solidity File: src/SigningUtils.sol 9 abstract contract SigningUtils { ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/SigningUtils.solG009 - 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.
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.
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.
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
```solidity 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 ```solidity 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 ```solidity 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 ```solidity File: src/ReentrancyAttack.sol 25 constructor(address payable _contractToAttackAddress) { 26 palmeraModule = PalmeraModule(_contractToAttackAddress); 27 } ``` https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.solG013 -
>=costs less gas than>:The compiler uses opcodes
GTandISZEROfor solidity code that uses>, but only requiresLTfor>=, which saves 3 gas.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.
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraRoles.sol
G015 - State variables only set in the constructor should be declared
immutable:https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol
G016 - Don't use
_msgSender()if not supporting EIP-2771:https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraRoles.sol
G017 - Use
uint256(1)/uint256(2)instead fortrueandfalseboolean states:If you don't use boolean for storage you will avoid Gwarmaccess 100 gas. In addition, state changes of boolean from
truetofalsecan cost up to ~20000 gas rather thanuint256(2)touint256(1)that would cost significantly less.https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/DenyHelper.sol
G018 -
++i/i++should beunchecked{++i}/unchecked{i++}when it is not possible for them to overflow, as is the case when used infor- andwhile-loops:The
uncheckedkeyword 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.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
bytes4should also only be done once.https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/SigningUtils.sol
G020 - Consider activating
via-irfor deploying: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.
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol
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.
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol
G023 -
unchecked {}can be used on the division of twouintsin order to save gas:The division cannot overflow, since both the numerator and the denominator are non-negative.
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
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
```solidity 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 ```solidity 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 ```solidity 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 ```solidity 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.solG026 - 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).
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/ReentrancyAttack.sol
G027 - The use of a logical
ANDin place of doubleifis 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.
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/Helpers.sol
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.
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol
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.
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraGuard.sol
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol
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.
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.
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
https://github.com/saidqayoumsadat/Palmera-contest/blob/master/src/PalmeraModule.sol