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

Open
code423n4 opened this issue May 31, 2022 · 2 comments
Open

Gas Optimizations #32

code423n4 opened this issue May 31, 2022 · 2 comments
Labels
bug Something isn't working G (Gas Optimization) resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) 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

RoleManager.sol

  • L27/46/112/113 - The modifier generates a lot of gas cost, it would be less expensive to use an if with a custom error or a private view function.

AddressProvider.sol

  • L64/71/98/102/176/185/199/241/242/260/270/295/296/325/428/434 - The require are too expensive, it would be saved using the Errors custom + if, instead of requires. Another option could be to use a private view function.

  • L101/102/103/118/119/121/122/123 - The get calls to other contracts, being a view method, do not generate any cost, therefore, it would be less expensive not to create variables and call the getter directly.

Controller.sol

  • L49/50/51/66/69/71 - The get calls to other contracts, being a view method, do not generate any cost, therefore, it would be less expensive not to create variables and call the getter directly.

  • L66 - A variable is created and gas is consumed to fill a variable that will only be used if the if in line 68 is true, since it is only used within the if, it should be created within the if.

BkdToken.sol

  • L31 - Gas can be saved if instead of a require an if is used, with the custom error already created.

libraries/UncheckedMath.sol

  • L7 - In the uncheckedInc() function, the gas could be further optimized, if instead of unchecked{a+1;} it was unchecked {++a;}

contracts/RewardHandler.sol

  • L63 - It is less expensive to validate that variable != 0, than to validate variable > 0.

contracts/BkdLocker.sol

  • L91/92/137/139/254/301 - It is less expensive to validate that variable != 0, than to validate variable > 0.

  • L91/92/119/137/208 - Gas can be saved if instead of a require an if is used, with the custom error already created.

  • L140/144 - --i is less expensive than i = i - 1;

  • 150/151 - A newTotal variable is created, to only be used in one place, it is not necessary and gas could be saved by not doing it.

contracts/tokenomics/FeeBurner.sol

  • L117 - It is less expensive to validate that variable != 0, than to validate variable > 0.

contracts/tokenomics/LpGauge.sol

  • L59 - The if that validates: (amount <= 0) does not make much sense, since in uint there is no < 0, therefore the correct validation is:
    (amount == 0).

  • L68/114 - The if that validates param > 0, could become less expensive if instead of >, a != is used.

  • L111/115 - A currentRate variable is created, which is only used in one place, therefore it is not necessary and more gas is spent.

contracts/tokenomics/VestedEscrowRevocable.sol

  • L101 - The claim() function appears as nonReentrant, but there is no reason for reentrant to occur, since only a transferFrom is performed
    on line 146 at VestedEscrow.sol

  • L97/98 - It is not necessary to create a variable that will only be used once, it would save gas if it is used directly.

  • L103/104 - It is possible to create a variable in memory for revokedTime[msg.sender], in this way less gas would be used.

  • L52/53/54 - The modifier generates a lot of gas cost, it would be less expensive to use an if with a custom error or a private view function.

  • L54 - In the revoke() function the storage treasury address is used multiple times, one way to save gas is to create a variable in memory.

contracts/tokenomics/VestedEscrow.sol

  • L83/84 - It is not necessary to create a variable that will only be used once, it would save gas if it is used directly.
    Gas is also saved if instead of > 0 we use != 0.

  • L89 - It is not necessary for the function to be nonReentrant, since the transfers that occur are only in the rewardToken address that is set
    by the owner on deploy, so there shouldn't be a problem with reentry.

  • L134/135/155/156/168/169 - It is not necessary to create a variable that will only be used once, it would save gas if it is used directly.

contracts/utils/Preparable.sol

  • L28/29/86/98/110/111 - The modifier generates a lot of gas cost, it would be less expensive to use an if with a custom error or a private view function.

contracts/tokenomics/KeeperGauge.sol

  • L59/98 - ++epoch uses less gas than epoch++;

  • L39/78/82/126/140 - The requirements are very expensive, it could save gas if instead of a modifier it is an if with a custom error or a private view function.

  • L140 - Gas is saved, if instead of totalClaimable > 0 you can use totalClaimable != 0.

contracts/tokenomics/AmmGauge.sol

  • L63 - It should be amount == 0, it doesn't make sense for amount to be <= 0, since it is uint256.

  • L88/104/125/147 - Gas would be saved if instead of variable > 0 variable != 0 is used.

@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels May 31, 2022
code423n4 added a commit that referenced this issue May 31, 2022
@chase-manning chase-manning added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) labels Jun 6, 2022
@GalloDaSballo
Copy link
Collaborator

L27/46/112/113 - The modifier generates a lot of gas cost, it would be less expensive to use an if with a custom error or a private view function.

Saves deployment cost which is hard to quantify, in lack of details cannot give it any points.

L64/71/98/102/176/185/199/241/242/260/270/295/296/325/428/434 - The require are too expensive, it would be saved using the Errors custom + if, instead of requires. Another option could be to use a private view function.

Same deal, no Code = no points

L101/102/103/118/119/121/122/123 - The get calls to other contracts, being a view method, do not generate any cost, therefore, it would be less expensive not to create variables and call the getter directly.

Doing a call to a view function generates a STATICCALL which costs 100 fixed gas plus the gas cost of the operations, this finding is invalid

L49/50/51/66/69/71 - The get calls to other contracts, being a view method, do not generate any cost, therefore, it would be less expensive not to create variables and call the getter directly.

Invalid per above

L66 - A variable is created and gas is consumed to fill a variable that will only be used if the if in line 68 is true, since it is only used within the if, it should be created within the if.

Agree, would save gas on the "bad path" hence hard to quantify

L31 - Gas can be saved if instead of a require an if is used, with the custom error already created.

Disagree in lack of POC

L7 - In the uncheckedInc() function, the gas could be further optimized, if instead of unchecked{a+1;} it was unchecked {++a;}

Should save 5 gas

L63 - It is less expensive to validate that variable != 0, than to validate variable > 0.

Saves 3 gas

L91/92/137/139/254/301 - It is less expensive to validate that variable != 0, than to validate variable > 0.

Only for requires, L91/92/137/139
4 * 3 = 12

L91/92/119/137/208 - Gas can be saved if instead of a require an if is used, with the custom error already created.

0 per above

L140/144 - --i is less expensive than i = i - 1;

Saves 5 gas
5 * 2 = 10

150/151 - A newTotal variable is created, to only be used in one place, it is not necessary and gas could be saved by not doing it.

3 gas

L117 - It is less expensive to validate that variable != 0, than to validate variable > 0.

Only for requires

L59 - The if that validates: (amount <= 0) does not make much sense, since in uint there is no < 0, therefore the correct validation is:

3 gas

L68/114 - The if that validates param > 0, could become less expensive if instead of >, a != is used.

Only for requires

L111/115 - A currentRate variable is created, which is only used in one place, therefore it is not necessary and more gas is spent.

3 gas

L101 - The claim() function appears as nonReentrant, but there is no reason for reentrant to occur, since only a transferFrom is performed

Arguable as Slither would flag the event for potential reEntrancy, will not award points here

L97/98 - It is not necessary to create a variable that will only be used once, it would save gas if it is used directly.

3

L103/104 - It is possible to create a variable in memory for revokedTime[msg.sender], in this way less gas would be used.

Saves 94 gas (100 - 3 - 3)

L52/53/54 - The modifier generates a lot of gas cost, it would be less expensive to use an if with a custom error or a private view function.

See above

L54 - In the revoke() function the storage treasury address is used multiple times, one way to save gas is to create a variable in memory.

97 + 97 + 94 gas saved

L83/84 - It is not necessary to create a variable that will only be used once, it would save gas if it is used directly.

3

L89 - It is not necessary for the function to be nonReentrant, since the transfers that occur are only in the rewardToken address that is set

I'd keep the guard in lack of CEI pattern (and dodge the wrath of the spam c4 submissions)

L134/135/155/156/168/169 - It is not necessary to create a variable that will only be used once, it would save gas if it is used directly.

Saves 9 gas

L28/29/86/98/110/111 - The modifier generates a lot of gas cost, it would be less expensive to use an if with a custom error or a private view function.

It's an internal function ser

L59/98 - ++epoch uses less gas than epoch++;

5 gas

L39/78/82/126/140 - The requirements are very expensive, it could save gas if instead of a modifier it is an if with a custom error or a private view function.

See above

L140 - Gas is saved, if instead of totalClaimable > 0 you can use totalClaimable != 0.

3 (technically 6 but I already judged tons with 3)

L63 - It should be amount == 0, it doesn't make sense for amount to be <= 0, since it is uint256.

Saves 3 gas

L88/104/125/147 - Gas would be saved if instead of variable > 0 variable != 0 is used.

Only for requires,
2 * 3 = 6

Personally I think this type of report can be drastically improved by:

  • Bulking the fix instead of grouping by file (humans think in problems and ideas, not in files)
  • Add the gas saved
  • Add POC for big statements ("use custom errors lol XD")

Total Gas Saved
453

@GalloDaSballo
Copy link
Collaborator

Adding 24 gas from #33

477

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) resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) 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