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 #57

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

Gas Optimizations #57

code423n4 opened this issue Feb 12, 2022 · 3 comments
Assignees
Labels
bug Something isn't working G (Gas Optimization) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

GAS :
1.
Title: Unnecessary owner function call

Impact:
In the https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L134 it uses owner function call, instead of _owner, using _owner directly can save some gas

POC :
https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L134

Title: It cheaper to remove the && operator and make the 2 different require

POC :
https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedAsset.sol#L78

Mitigation :

require(_exists(_replicatedTokenId), "NA: INVALID_REPLICATED_TOKEN_ID");
require(tokenId != _replicatedTokenId, "NA: INVALID_REPLICATED_TOKEN_ID");

Title : It Cheaper to use 0 instead of empty string

Impact : change bytes32("") to bytes32(0) can save +- 3 gas, both check for zero bytes32

POC :
https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L101

Mitigation :

require(operator != bytes32(0), "NF: INVALID_OPERATOR_NAME");

Title : It cheaper to cached the operators to a memory instead use storage multiple times

Impact : In the removeOperator function it do the check in the loop by calling the operators storage, instead checking it with memory, multiple storage read is more expansive than doing multiple read from memory therefore saving the operator value to a memory first before checking inside a loop can make this call cheaper, just like addOperator function do.

POC :
https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L111

Mitigation :

function removeOperator(bytes32 operator) external override onlyOwner {
        bytes32[] memory operatorsCache = operators;
        uint256 operatorsLength = operatorsCache.length;
        for (uint256 i = 0; i < operatorsLength; i++) {
            if (operatorsCache[i] == operator) {
                operators[i] = operatorsCache[operatorsLength - 1];
                operators.pop();
                emit OperatorRemoved(operator);
                return;
            }
        }
        revert("NF: NON_EXISTENT_OPERATOR");
    }
@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
@maximebrugel
Copy link
Collaborator

"Unnecessary owner function call" (Duplicated)

#67

"It cheaper to remove the && operator and make the 2 different require" (Confirmed)

20 unit saved

"It Cheaper to use 0 instead of empty string" (Disputed)

No impact.

"It cheaper to cached the operators to a memory instead use storage multiple times" (Confirmed)

@maximebrugel maximebrugel added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 17, 2022
@maximebrugel maximebrugel self-assigned this Feb 18, 2022
@harleythedogC4
Copy link
Collaborator

My personal judgments:

  1. "Unnecessary owner function call". Valid and small-optimization.
  2. "remove the && operator". Valid and small-optimization.
  3. "It Cheaper to use 0 instead of empty string". Report claims it can save +-3 gas, which is confusing and implies it might spend more gas? Sponsor notes no difference so I will agree with sponsor. Invalid.
  4. "cached the operators to a memory". Valid and small-optimization.

@harleythedogC4
Copy link
Collaborator

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 3 points.
Thus the final score of this gas report is (3/10)*100 = 30.

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) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

3 participants