code-423n4 / 2022-02-nested-findings

0 stars 0 forks source link

Gas Optimizations #64

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

NestedFinanceGasFindings

1-- -using multiple require() is gas saving https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L54-L62 instead of using &&, using multiple require is gas saving.

require(address(_nestedAsset) != address(0));
require(address(_nestedRecords) != address(0));
...

2-- -better for() implementation https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L113 replace i++ to ++i and dont set the i value because the default is already 0. Its cost less gas usage

for (uint256 i; i < operatorsLength; ++i)

3-- -Better way of using SafeERC20 lib https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L19 By calling SafeERC20.function directly and removing line 19 can save 15 gas per call:

SafeERC20.safeTransfer(_token, owner(), amount);

SafeErc20.function was called 8 times in this contract. Also very good to implemented at NestedReserve.sol 4-- -using storage instead of caching struct/array data can save gas https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L205 using storage can save gas

address[] storage tokens = nestedRecords.getAssetTokens(_nftId);

tokens is called once at destroy() before it chaced to tokensLength as tokens.lenght so reading from storage is cheaper than using memory 5-- -use calldata to store _weights & _account https://github.com/code-423n4/2022-02-nested/blob/main/contracts/FeeSplitter.sol#L89-L90 change memory to calldata

maximebrugel commented 2 years ago

1 (Disputed)

Not the case. From 4966507 to 4970427

2 (Disputed)

Already in first audit : https://github.com/code-423n4/2021-11-nested-findings/issues/25

3 (Duplicated)

https://github.com/code-423n4/2022-02-nested-findings/issues/55

4 (Disputed)

Compilation error address[] storage tokens = nestedRecords.getAssetTokens(_nftId); you can't convert to storage like this.

5 (Disputed)

Data location must be "storage" or "memory" for constructor parameter.

harleythedogC4 commented 2 years ago

My personal judgements:

  1. "using multiple require() is gas saving". This was confirmed by the sponsor in #55, so it should be valid here too (although it is a very small difference in gas). Valid and small optimization.
  2. "better for() implementation". Agree with sponsor. Invalid.
  3. "Better way of using SafeERC20 lib". Valid and small optimization.
  4. "using storage instead of caching struct/array data can save gas". Example gives compilation error, Invalid.
  5. "use calldata to store _weights & _account". As sponsor says. Invalid.
harleythedogC4 commented 2 years ago

Now, here is the methodology I used for calculating a score for each gas report. I first assigned each submission to be either small-optimization (1 point), medium-optimization (5 points) or large-optimization (10 points), depending on how useful the optimization is. The score of a gas report is the sum of these points, divided by the maximum number of points achieved by a gas report. This maximum number was 10 points, achieved by #67.

The number of points achieved by this report is 2 points. Thus the final score of this gas report is (2/10)*100 = 20.