Gas Optimizations

[code423n4]

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

[c4-judge]

gzeon-c4 marked the issue as grade-b

[0xJCN]

Hello @gzeon-c4 ,

I would like to encourage you to revisit this issue. I believe it should receive a grade A and I will explain why down below. If you still decide to maintain the current grade after reading this, I would very much appreciate some advice on how I can improve my submisions and provide more value in the future.

• As per my Analysis, I prioritized high-impact findings and exlcuded bot-like findings in an attempt to provide more value, and out of respect for the sponor's time.
• A majority of the instances are benchmarked. The instances that are not benchmarked contain explanations via opcodes.
• All instances contain diffs to illustrate the necessary refactoring needed for the findings.
• All instances contain explanations.
• All of the instances (with the exception of Issue 7) are high impact, meaning they save >= 100 gas.
• This report only contains one finding Issue 7 that can be considered not high impact (This is debateable as it can only be high impact on an unhappy path).
• This report contains no false findings.
• The total amount of gas saved across all instances is displayed in the report.

In the spirit of fairness (as well as maintaining and encouraging high quality reports) I have provided a detailed comparison of the other reports which received a grade A. Some of these reports contain false findings/false positives, but a majority of the reports contain low impact findings akin to the kind in bot races. I believe reports such as these should be discouraged from being submitted as a regular gas report and instead be encouraged to participate in the bot races. Regular gas reports, in my opinion, should be held to a higher standard compared to bot races.

In addition, seeing as we are receiving compensation for our time and effort, I wanted to be fair to myself as it is difficult for me to observe the reports below and understand what I am 'missing' that resulted in my report being graded lower.

Issue #285

• Note that none of the instances contain diffs to illustrate the neccessary refactoring needed. The warden takes the time to write their own test for each optimization, but since the code in each instance is unique I believe it would be more beneficial to the sponsor if the protocol's code was refactored and benchmarked.
• G-01 low impact and would benefit from diffs.
• G-02 low impact and would benefit from diffs.
• G-03 This optimization only works for specific code and does not relate to every for loop. This optimization assumes a specific scenario where every for loop is writing to storage. In this case the optimization could work, but unfortunately this is not the case for the majority of the instances listed, if not all instances listed.
• G-04 low impact.
• G-05 bot-like. In addition, the link provided is from a repo which uses an outdated compiler version.
• G-07 A large number of these instances were included in the bot race report.
• G-08 low impact
• G-09 low impact

Issue #230

• Note that the instances do not include diffs to illustrate the necessary refactoring needed to make for the gas savings to take effect and this report includes no benchmarking.
• G-01 low impact
• G-02 no refactoring is given. I believe reafactorings are essential especially for optimizations that use assembly.
• G-03 This finding lacks any explanation and refactoring.
• G-04 This finding lacks refactoring and an explanation for how much gas would be saved.
• G-05 low impact
• G-06 This finding does not explain how much gas is saved.
• G-07 This finding lacks refactoring
• G-08 bot-like/low impact
• G-10 No refactoring or explanation for gas saved is given.
• G-11 low impact
• G-12 This is false as this instance does not point to a storage array being read into memory. In this instance a new array is being created in memory. Using the storage keyword would result in a compiler error.
• G-13 This is false as the two values mentioned are different and are being assigned to different variables.
• G-14 This finding is context specific and the amount of gas savings could only be known via refactoring.
• G-15 bot-like/low impact
• G-16 Lacks refactoring and the amount of gas saved. Instantiating a stack variable inside of a for loop will result in the variable being discarded after the loop, but if it is instantiated outside of the loop then it will be left on the stack and this may result in unwanted stack manipulation for the remainder of the code.
• G-17 This is false. The refactoring suggested results in the same number of storage slots as the original code. In the original code snippet 3 slots are used and 3 slots are also being used in the refactored code.
• G-18 Low impact and this finding does not include refactoring or an explanation for the amount of gas saved.
• G-19 Lacks refactoring or the amount of gas saved
• G-20 Does not specify the amount of gas saved
• G-21 Lacks refactoring or the amount of gas saved
• G-23 This is the same as the finding above
• G-24 Requires more explanation + proof that ERC721A can be used, how it can be used, and where it can specifically be used.
• G-27 More QA.

Issue #227

• Note that no findings contain diffs to illustrate the refactoring needed for the optimizations. In addition, no optimizations include benchmarking.
• G-01 Lacks refactoring or the amount of gas saved
• G-02 Lacks refactoring and the amount of gas saved. Instantiating a stack variable inside of a for loop will result in the variable being discarded after the loop, but if it is instantiated outside of the loop then it will be left on the stack and this may result in unwanted stack manipulation for the remainder of the code.
• G-04 This is false. Public functions cost the same as external functions.
• G-06 lacks refactoring
• G-07 This is false as this instance does not point to a storage array being read into memory. In this instance a new array is being created in memory. Using the storage keyword would result in a compiler error.
• G-08 Lacks refactoring or the amount of gas saved
• G-09 Lacks refactoring or the amount of gas saved
• G-10 bot-like/low impact
• G-11 low impact
• G-12 Lacks refactoring or the amount of gas saved. The link given refers to an outdated compiler version.
• G-13 low impact
• G-14 Lacks refactoring or the amount of gas saved
• G-15 This is false as the two values mentioned are different and are being assigned to different variables.
• G-16 low impact
• G-17 This is false. The refactoring suggested results in the same number of storage slots as the original code. In the original code snippet 3 slots are used and 3 slots are also being used in the refactored code.
• G-18 bot-like/low impact
• G-19 Does not specify the amount of gas saved
• G-20 This finding does not point to any specific instances.
• G-21 Lacks refactoring and benchmarking for proof of optimization
• G-22 This finding lacks any explanation and refactoring.
• G-23 Low impact/bot-like
• G-24 This finding is context specific and the amount of gas savings could only be known via refactoring.
• G-25 More QA

Issue #117

• Note that this report does not contain diffs to illustrate the neccessary changes needed for the optimizations nor does it contain benchmarks against the protocol's code. This report is also extremely similar to bot-race reports. Therefore, I will only comment on the high impact findings presented in this report.
• G-11 This is false as the code snippet is taken out of context. There is an address state variable above the isFixedLengthApprovalPeriod state variable and therefore the refactoring suggested will not save 1 slot. The refactoring will result in 3 slots, similarly the original code also results in 3 slots
• G-13 This finding does not point to any specific code and therefore we can not confirm the validity of the optimization.
• G-15 This is false as the Config struct is not used in storage. It is used to instantiate a memory struct.

Issue #84

• Note that this report contains multiple false findings and no benchmarks to illustrate the specific amount of gas saved for any instances.
• G-01 This is false as the argument parameter in the instance given is already set to calldata
• G-02 This is false as public functions cost the same as external functions.
• G-03 This is false as the refactoring shown still does the same storage writes as the original code.
• G-04 This is false as the contract given inherits from LlamaAbsoluteStrategyBase, which is upgradeable and thus the policy state variable is set in a separate initialize function instead of the constructor. Therefore, none of the state variables in LlamaAbsoluteStrategyBase, policy included, can be set as immutable.
• G-05 This is false as the external function calls in question take different input arguments and are therefore unique and would have to be called separately. The refactorinng given would not mitigate any external calls.
• G-06 This is false as LlamaAbsoluteStrategyBase is upgradeable and thus the policy state variable is set in a separate initialize function instead of the constructor. Therefore, none of the state variables in LlamaAbsoluteStrategyBase, policy included, can be set as immutable.
• G-07 This is false as the external function calls in question take different input arguments and are therefore unique and would have to be called separately. The refactorinng given would not mitigate any external calls.
• G-08 No benchmarks included as proof for the validity of the optimization.
• G-09 No benchmarks included as proof for the validity of the optimization.
• G-10 This finding does not explain how much gas would be saved. No benchmarks included.
• G-11 This is false as grabbing block.timestamp uses the NUMBER opcode, which costs 2 Gas. Caching the timestamp in a variable will be more expensive because you have the perform stack manipulation (at least DUP (3 gas)) to use the variable.
• G-12 This requires more information. Mappings are generally more efficient than arrays. Benchmarks should be included as proof for the validity of the optimization.
• G-13 This is false as mappings can not be set to immutable. The suggested refactoring will result in a compiler error.
• G-15 This finding is misleading as a majority of the loops in the contract in question already cache the length of the calldata array. In addition, the array is stored in calldata, not storage, as suggested in the explanation of the optimization.
• G-16 This is false as public functions cost the same as external functions.
• G-17 This is false as safeTransfer and safeTransferFrom does not explicitly check for a zero address. Checking for the zero address early and reverting early before any further calls or changes to state are made will save gas.
• G-18 This would only have a small effect on deployment gas, not runtime. Gas savings are negligible.
• G-19 This would only have a small effect on deployment gas, not runtime. Gas savings are negligible.
• G-20 This is false as public functions cost the same as external functions.
• G-21 This finding lacks any refactoring to illustrate the optimization.
• G-22 low impact
• G-23 This finding lacks refactoring and benchmarks as proof for the validity of the optimization
• G-27 low impact
• G-28 No links to specific code or refactoring is given

Issue #70

• Note that none of the instances included benchmarks to illustrate the specific amount of gas that would be saved.
• G-01 This finding does not include any explanation nor does it specify how much gas would be saved.
• G-02 This finding does not include any benchmarks to specify how much gas is saved. In addition, the instance in question is a pure function, which do not cost gas.
• G-03 This finding does not include any benchmarks to specify how much gas is saved. In addition, the instance in question is a pure function, which do not cost gas.
• G-04 This finding only affects deployment gas, not runtime gas. This is also low impact.

[gzeon-c4]

Thanks for flagging, will do another pass of gas report review.

[c4-judge]

gzeon-c4 marked the issue as grade-a

[0xJCN]

Hi @gzeon-c4 ,

Thank you for taking the time to review my comment. I have one more clarifying question I'd like to ask you. I was wondering if you could offer some insight as to how this report could've been improved so that it qualified for "selected for report"? I'd appreciate any feedback so I can do better in the future and provide more value to the sponsor.

Thank you.

[gzeon-c4]

Hi @gzeon-c4 ,

Thank you for taking the time to review my comment. I have one more clarifying question I'd like to ask you. I was wondering if you could offer some insight as to how this report could've been improved so that it qualified for "selected for report"? I'd appreciate any feedback so I can do better in the future and provide more value to the sponsor.

Thank you.

I actually got interrupted yesterday and will finish the review in the next few hours I think.

[gzeon-c4]

Apologize again for the delay, I am going to mark this report as best because out of all the gas reports, this provided the most unique insight and the least inaccuracy. Judging qa and gas reports are tricky, since they are assigned a very small % of the overall prize pool it does not make a lot of sense to spend more time on it than HM issues. I usually quickly sort them into A B C bucket and reviewing only a few of them in more detail. That said, there are still ways to improve my process here as in retrospect I have been tricked by some warden to believe certain gas optimization is legit, when they are providing invalid or out-of-context examples.

Regarding this report, I think it looks good and it is impressive that everything is with actual coded poc and gas metering. It would be nicer if you can show the function you metered in the summary table (for example, a gas saving in a hot path is much better than in say init function which is only called once). Nit-picking but e.g. this finding:

Use already cached actionId to save 1 SLOAD

This seems to have this risk of making actionCount off due to possible reentrancy, it seems to be possible that an action is created during the external call to the strategy or the guard so if you use the cached action count you will only increment it by 1 even if many action is created. I am unsure this is an actual issue tho because the mapping will be overwritten anyway.

[c4-judge]

gzeon-c4 marked the issue as selected for report

[0xJCN]

Apologize again for the delay, I am going to mark this report as best because out of all the gas reports, this provided the most unique insight and the least inaccuracy. Judging qa and gas reports are tricky, since they are assigned a very small % of the overall prize pool it does not make a lot of sense to spend more time on it than HM issues. I usually quickly sort them into A B C bucket and reviewing only a few of them in more detail. That said, there are still ways to improve my process here as in retrospect I have been tricked by some warden to believe certain gas optimization is legit, when they are providing invalid or out-of-context examples.

Regarding this report, I think it looks good and it is impressive that everything is with actual coded poc and gas metering. It would be nicer if you can show the function you metered in the summary table (for example, a gas saving in a hot path is much better than in say init function which is only called once). Nit-picking but e.g. this finding:

Use already cached actionId to save 1 SLOAD

This seems to have this risk of making actionCount off due to possible reentrancy, it seems to be possible that an action is created during the external call to the strategy or the guard so if you use the cached action count you will only increment it by 1 even if many action is created. I am unsure this is an actual issue tho because the mapping will be overwritten anyway.

Thank you for your detailed response. In future contests I will be sure to exclude init functions as I agree that they hold less value compared to functions that are expected to be called again and again. I will also be much more specific with the exact functions that I am metering. The action information seems to be stored into the mapping using the correct actionId. Th tests passed with this optimization, as I always make sure no obvious issues with the test suite occur, but I understand the observation.

I also understand that as a judge your time is rightfully spent evaluating HM for the majority of the judging process. As someone who primarily participates in Gas Optimization, I recognize this and therefore believe it is beneficial for me to share additional insight regarding some reports that seem to have inaccuracies, so we can provide more value to the sponsors. That being said I would like to provide some additional remarks regarding Issue 120 that I believe is important for the assessment of the report:

• G-01 This finding is false. Firstly it is important to note that all 3 of the values in the struct are packed into one slot. Therefore only one Gcoldsload (2100 gas) will incur when the fields are accessed. The first instance (lines 106) will not save any gas when using the storage keyword. This is due to the fact that on line 107 all of the struct fields are read regardless. This means the same amount of SLOADs will take place regardless whether or not the struct is copied into memory. The second instance will not save any gas due to the fact that the timestamp field is read twice (once on line 135 and once on line 138). This means that the Gwarmaccess (100 gas) that would be saved by using the storage keyword (this comes from not reading the quantity field) is is ultimately spent when the timestamp field is read for a second time.
• G-02 This finding is false as the ERC1155Data struct is not used in storage. It is used in calldata.
• G-03 The 4th instance in this finding is false as the second occurance is a storage write and therefore the mapping's value can not be cached and used here.