Gas Optimizations

[c4-submissions]

See the markdown file with the details of this report here.

[141345]

1 L 4 r 7 nc

[G-1] Improving storage efficiency through Struct Packing
r

[G-2] Replacing booleans with uint256(1)/uint256(2) for gas optimization
L

[G-3] Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate
i

[G-4] totalDepositedAmountPerUser[_l1Token][_depositor] should be cached
r

[G-5] Efficient event consolidation for Glogtopic Gas Savings
r

[G-6] Optimize gas usage by avoiding variable declarations inside loops
nc

[G-7] Use dot notation method for struct assignment
r

[G-8] !_checkBit(processedLogs, uint8(logKey) this check is redundant when processedLogs is 0
nc

[G-9] The claimFailedDeposit() function can be refactored for greater gas efficiency
nc

[G-10] Optimize the fallback() function for better gas efficiency
d

[G-11] Optimize the commitBatches() function for better gas efficiency
d

[G-12] Optimize the requestL2Transaction() function for better gas efficiency
d

[G-13] Optimize code to avoid to spend unnecessary GAS
r

[G-14] Not initializing variables to their default values incurs extra gas, So leaving them with their default values is more gas-efficient
x

[G-15] Don't cache immutable variable incurs extra Gas
nc

[G-16] Bytes constants are more efficient than string constants
nc

[G-17] Don't cache variable only used once
nc

[G-18] First perform less expensive checks and then proceed to more costly ones
nc

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-judge]

GalloDaSballo marked the issue as grade-b

[sathishpic22]

Hi @GalloDaSballo

Struct packing traditionally marked as LOW for Previous contests . I also confirmed this issue with sponsor he accepted this. The protocol versions can be uint96 . This findings alone saves 4000 GAS . Even no other wardens reported this issue

[G-1] Improving storage efficiency through Struct Packing

[G-1.1] protocolVersion and l2SystemContractsUpgradeBatchNumber can be uint96 instead of uint256 : Saves 4000 GAS , 2 SLOT
[G-1.2] newProtocolVersion can be uint96 instead of uint256 : Saves 2000 GAS , 1 SLOT

Also saving mapping cache traditionally comes under LOW category

totalDepositedAmountPerUser[_l1Token][_depositor] should be cached : Saves 100 GAS , 1 SLOD

The claimFailedDeposit() function can be refactored for greater gas efficiency

This saves 1 external call if any failures in amount > 0 check

Regarding issue #996 there is OOS findings are marked as LOW . These instances are already found in bot reports. 4 Findings marked as LOW . Please check this .

[G-8] totalBatchesCommitted is read twice from storage in Executor::commitBatches()
[G-9] totalBatchesVerified is read twice from storage in Executor::proveBatches()
[G-10] totalBatchesCommitted, totalBatchesVerified and totalBatchesExecuted are re-read from storage in Executor::revertBatches()
[G-13] User deposited amount in Mailbox::_verifyDepositLimit() is read twice from storage

Thank you

[lsaudit]

[G-18]

This is invalid, in both cases we're reading from struct.
Moreover, it's more likely that faces is freezable, than diamond is frozen. The order is correct, because the more likely scenario is being checked first. If first condition is true, the second one won't be evaluated (because of OR operation). Thus the current implementation saves more gas.

[G-15] and [G-17] are dups,

Both issues are referring to caching local variable and should be counted once

[G-16]

This is invalid getName is override, because it's related to facet's name. Diamond needs to ensure that some facets can be named with more than 32 characters. This cannot be bytes32, it has to be string. This issue is invalid!

[G-13]

This is not refactoring issue, it's just changing the order of require statements (moving require statement up), thus it should be considered as NC.
Moreover, since this is just moving require statement up, this should be considered as duplicate, the same type/class of findings is here: G-12, G-11, G-10, G-9

[G-08]

This seems to be invalid, this is required to make sure that log hasn't been processed multiple of times

[G-07]

This is not refactoring issue, it should be NC

[G-05]

Events NewPendingGovernor(), NewGovernor(), NewPendingAdmin(), NewAdmin() guarantee the offchain security of the protocol. They let the users to monitor critical operations (such as admin is being changed, governor is being changed), so user will be able to quickly react in case of emergency.

There are two emergency cases - someone is trying to change the governor (then NewPendingGovernor() is being emitted), the governor is being changed (NewGovernor() is being emitted).
Users who monitors for these events can properly react on some critical changes. Adding another event - PendingAndNewGovernor() provides some unnecessary mess, which impacts the overall security of the user.

In case of contract's critical changes, each event should be linked to the action. Current implementation provides that. NewPendingGovernor() is emitted whenever pendingGovernor is being changed.
NewGovernor() is being emitted whenever s.governor is being updated. Since acceptGovernor() performs two of these actions (updating s.governor and pendingGoernor), both NewGovernor() and NewPendingGovernor() should be emitted.

[G-1.2]
"The newprotocolVersion value is increased in a sequential way" - this is not true. Function _setNewProtocolVersion() can set it to anything.

[sathishpic22]

Hi @lsaudit

Thank you for your suggestions.

I think all your understanding and assumptions are wrong.

[G-18] regarding this

require(!diamondStorage.isFrozen || !facet.isFreezable, "q1");

!diamondStorage.isFrozen is a state variable, while !facet.isFreezable is a memory variable. According to best practices for gas optimization, it's more efficient to first check the memory variable !facet.isFreezable. If this check returns true, then it's not necessary to access the state variable !diamondStorage.isFrozen, as memory operations are less costly in terms of gas than state variable accesses. This approach helps in reducing gas consumption and optimizing smart contract performance

[G-15] Its about immutable cache [G-17] its about state variables cache. This is the standard way followed for all findings.

[G-16] I reported about constants not normal variables. Constant values in a Solidity contract are fixed and do not change anywhere in the contract once they are assigned. These values always remain the same, as all instances are constants. Once a constant value is assigned, it cannot be changed

[G-9] regarding this issue if any failures this will save 1 external calls. Yes ,refactoring because the structure of the code is aligned . Any changes in existing code always comes refactoring category

[G-13],[G-12], [G-11], [G-10], [G-9] yes still the code structure is changed. All this suggestions saves gas in particular scenarios.

[G-8] !_checkBit(processedLogs, uint8(logKey) this check is redundant when processedLogs is 0.

Suggestion indicates that the check becomes redundant during the first iteration, as the processedLogs value is initially 0. In this scenario, the condition will always be true for the first iteration, rendering the check unnecessary at that stage

[G-05] Suggestion about combining events in Solidity smart contracts is indeed a recognized gas optimization technique. In scenarios where functions like acceptGovernor() and acceptAdmin() are called, and both trigger separate events, combining these two events into a single event can result in significant gas savings.

By emitting a single event instead of two separate ones, you save on the gas costs associated with each event. Specifically, you conserve 375 gas for each Glogtopic (the cost for a topic in an event log) and an additional 700 gas for the two instances combined. This optimization is beneficial because it reduces the amount of data stored on the blockchain and the computational work needed to process and log these events.

This approach has been accepted in many previous contests, demonstrating its effectiveness in optimizing smart contracts for better gas efficiency

[G-1.2]
"The newprotocolVersion value is increased in a sequential way" - this is not true. Function _setNewProtocolVersion() can set it to anything.

This struct optimization alone saves 6000 GAS , 3 SLOTS

Regarding your suggestion, the sponsor has confirmed that the increment within the contract is always sequential, as per their response.

I don't know disclosing sponsor name is right or wrong. so i just hide his name

[sathishpic22]

Hi @GalloDaSballo

I want to add some additional points here

[G-10] Optimize the fallback() function for better gas efficiency

I don't understand why this is marked as disputed. Any failures in require(msg.data.length >= 4 || msg.data.length == 0, "Ut") mean that creating the storage variable Diamond.DiamondStorage storage diamondStorage = Diamond.getDiamondStorage() and making an external call is a waste of gas. This finding has already been proven and accepted in many old contests.

[G-11] Optimize the commitBatches() function for better gas efficiency

According to Solidity coding standards and for better gas efficiency, parameters should be checked first, followed by function calls and state variable checks

[G-12] Optimize the requestL2Transaction() function for better gas efficiency

If there are any failures in this check _l2GasPerPubdataByteLimit == REQUIRED_L2_GAS_PRICE_PER_PUBDATA, then checking and assigning sender values becomes a waste of gas.

I also want to mention that all my findings include the mitigation steps and gas saved. No other researchers wrote about the mitigation steps and gas saved

I also sent 'many unique findings' and high value findings more than any other wardens.

Struct packing is a prime example of this. This technique alone saves 6000 gas and uses only 3 slots, which no other wardens have reported. This mitigation accepted by sponsor.

"My report is currently the second best. It has 1L, 4R,7NC findings. Usually, the grades are decided based on the L count. Two of my Low findings have been marked as R.

Issue 996 4 of his LOW findings are out of scope that are marked as LOW. If those are marked invalid then my report is best compare to others, Many wardens just spamming with more counts than quality findings. This approach will degrade the wardens who are trying genuine high value findings and wrote with good quality reports.

If the grades based on score then there is no point for good quality findings. many wardens just simply sending bunch of low value NC findings and can get good scores.

Example issue 1070 and 338 sent so many NC critical findings and ignoring and got more score than some of the good reports.

Thank you for this chance to express my thoughts.

[GalloDaSballo]

(https://gist.github.com/g-1-improving-storage-efficiency-through-struct-packing)
I
(https://gist.github.com/g-11-protocolversion-and-l2systemcontractsupgradebatchnumber-can-be-uint96-instead-of-uint256--saves-4000-gas--2-slot)
L
(https://gist.github.com/g-12--newprotocolversion-can-be-uint96-instead-of-uint256--saves-2000-gas--1-slot)
L
(https://gist.github.com/g-13--txtype-and-nonce-can-be-uint128-instead-of-uint256--saves-2000-gas--1-slot)
I
(https://gist.github.com/g-2-replacing-booleans-with-uint2561uint2562-for-gas-optimization)
I
(https://gist.github.com/g-3-multiple-addressid-mappings-can-be-combined-into-a-single-mapping-of-an-addressid-to-a-struct-where-appropriate)
I
(https://gist.github.com/g-4-totaldepositedamountperuser_l1token_depositor-should-be-cached)
I
(https://gist.github.com/g-5-efficient-event-consolidation-for-glogtopic-gas-savings)
I
(https://gist.github.com/g-6-optimize-gas-usage-by-avoiding-variable-declarations-inside-loops)
I
(https://gist.github.com/g-7-use-dot-notation-method-for-struct-assignment)
I
(https://gist.github.com/g-8-_checkbitprocessedlogs-uint8logkey-this-check-is-redundant-when-processedlogs-is-0)
NC
(https://gist.github.com/g-9-the-claimfaileddeposit-function-can-be-refactored-for-greater-gas-efficiency)
I
(https://gist.github.com/g-10-optimize-the-fallback-function-for-better-gas-efficiency)
I
(https://gist.github.com/g-11-optimize-the-commitbatches-function-for-better-gas-efficiency)
I
(https://gist.github.com/g-12--optimize-the-requestl2transaction-function-for-better-gas-efficiency)
I
(https://gist.github.com/g-13-optimize-code-to-avoid-to-spend-unnecessary-gas)
I
(https://gist.github.com/g-14-not-initializing-variables-to-their-default-values-incurs-extra-gas-so-leaving-them-with-their-default-values-is-more-gas-efficient)
I
(https://gist.github.com/g-15-dont-cache-immutable-variable-incurs-extra-gas)
I
(https://gist.github.com/g-16-bytes-constants-are-more-efficient-than-string-constants)
I
(https://gist.github.com/g-17-dont-cache-variable-only-used-once)
I
()
I

I - 18
L - 2
NC - 1

[sathishpic22]

Hi @GalloDaSballo

There is an inconsistency in the severity assessment. Different opinions exist between the judge and the presorter. One report marks the same findings as ignored, while another report marks the same findings as NC and L.

Based on this, the grades assigned are affected. It seems that my reports have mistakenly marked some of my findings as 'Ignored', I believe.

From issue #624 these findings are accepted in other reports as valid but in my reports those are marked as ignoring findings.

My findings

From Issue #924

My findings

I request to reassess my findings.

Regards,

[GalloDaSballo]

Have reviewed and believe B is acceptable, ultimately I believe the report to be sufficient, but not A, would recommend less findings but higher impact