Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gas Optimizations #51

Closed
code423n4 opened this issue Feb 12, 2022 · 3 comments
Closed

Gas Optimizations #51

code423n4 opened this issue Feb 12, 2022 · 3 comments
Labels
bug Something isn't working G (Gas Optimization) invalid This doesn't seem right sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

  • [S]: Suggested optimation, save a decent amount of gas without compromising readability;
  • [M]: Minor optimation, the amount of gas saved is minor, change when you see fit;
  • [N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[M] Adding unchecked directive can save gas

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/NestedFactory.sol#L337-L339

require(amountSpent <= _inputTokenAmount - feesAmount, "NF: OVERSPENT");

uint256 underSpentAmount = _inputTokenAmount - feesAmount - amountSpent;

_inputTokenAmount - feesAmount - amountSpent; will never underflow.

[M] ++i is more efficient than i++

Using ++i is more gas efficient than i++, especially in a loop.

For example:

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/FeeSplitter.sol#L148-L148

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/FeeSplitter.sol#L165-L165

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/FeeSplitter.sol#L165-L165

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/FeeSplitter.sol#L261-L261

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/FeeSplitter.sol#L280-L280

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/FeeSplitter.sol#L318-L318

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/NestedFactory.sol#L103-L103

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/NestedFactory.sol#L113-L113

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/NestedRecords.sol#L196-L196

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/OperatorResolver.sol#L60-L60

[N] Unnecessary checked arithmetic in for loops

There is no risk of overflow caused by increamenting the iteration index in for loops (the i++ in for for (uint256 i = 0; i < tokensCount; i++)).

Increments perform overflow checks that are not necessary in this case.

Recommendation

Surround the increment expressions with an unchecked { ... } block to avoid the default overflow checks. For example, change the for loop:

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/NestedRecords.sol#L196-L198

for (uint256 i = 0; i < tokensCount; i++) {
        amounts[i] = getAssetHolding(_nftId, tokens[i]);
}

to:

for (uint256 i = 0; i < tokensCount;) {
        amounts[i] = getAssetHolding(_nftId, tokens[i]);
        unchecked { ++i; }
}
@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Feb 12, 2022
code423n4 added a commit that referenced this issue Feb 12, 2022
@adrien-supizet
Copy link
Collaborator

adrien-supizet commented Feb 16, 2022

[M] Adding unchecked directive can save gas

This suggestion requires wrapping all the following statements of the function in the unchecked directive. Looking at solidity docs this "should" be safe, but the small gas optimization is not worth taking a risk and making readers of the code doubtful of what it does.

[M] ++i is more efficient than i++

disputed, already acknowledged during the first audit

[N] Unnecessary checked arithmetic in for loops

disputed. the optimization suggested makes the code hard to read for very very little gas saving

@adrien-supizet adrien-supizet added sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue and removed sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels Feb 16, 2022
@harleythedogC4
Copy link
Collaborator

harleythedogC4 commented Feb 27, 2022

Even though the changes will not be adopted, some of these are technically valid submissions so I am removing the dispute tag and will assign an appropriate score.

EDIT: Nevermind, see below. All have been judged invalid.

@harleythedogC4 harleythedogC4 added sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue and removed sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue labels Feb 27, 2022
@harleythedogC4
Copy link
Collaborator

harleythedogC4 commented Mar 6, 2022

My personal judgements:

  1. "Adding unchecked directive can save gas". This issue (or at least something similar enough to be considered a duplicate) appeared in the first audit Adding unchecked directive can save gas 2021-11-nested-findings#173 Invalid.
  2. "++i is more efficient than i++". Already from last contest. Invalid.
  3. "Unnecessary checked arithmetic in for loops". Already from last contest (unchecked { ++i } is more gas efficient than i++ for loops 2021-11-nested-findings#25). Invalid.

@harleythedogC4 harleythedogC4 added enhancement New feature or request invalid This doesn't seem right and removed enhancement New feature or request labels Mar 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working G (Gas Optimization) invalid This doesn't seem right sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants