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

Open
code423n4 opened this issue Apr 13, 2022 · 0 comments
Open

Gas Optimizations #195

code423n4 opened this issue Apr 13, 2022 · 0 comments
Labels
bug Something isn't working G (Gas Optimization) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

1. Use prefix not postfix in loops

Impact

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

Proof of Concept

Prefix is used in some for loops, but there are still many examples of postfix in loops in the code
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L348
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L145
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L231
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L319
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L181
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L185

Tools Used

Manual analysis

Recommended Mitigation Steps

Use prefix not postfix to increment in a loop

2. Split up require statements instead of &&

Impact

Combining require statement conditions with && logic uses unnecessary gas. It is better to split up each part of the logical statement into a separate require statements

Proof of Concept

Line 194 of FungibleAssetVaultForDAO

require(amount > 0 && amount <= collateralAmount, "invalid_amount");

Instead, split into two to save gas

require(amount > 0, "invalid_amount");
require(amount <= collateralAmount, "invalid_amount");

Tools Used

Manual analysis

Recommended Mitigation Steps

Use separate require statements instead of concatenating with &&

3. Use != 0 instead of > 0

Impact

Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero

Proof of Concept

Locations where this was found include
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L94
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L108
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L142
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L164
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L180
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L194

Tools Used

grep

Recommended Mitigation Steps

Replace > 0 with != 0 to save gas

4. Add payable to functions that won't receive ETH

Impact

Marking a function as payable saves gas. Functions that have the onlyOwner modifier cannot be called by normal users and will not mistakenly receive ETH. These functions can be payable to save gas.

Proof of Concept

There are many functions that have the onlyOwner modifier and can be payable. Some examples are
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L91
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L98
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L108
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L115
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L200

Tools Used

Manual analysis

Recommended Mitigation Steps

Add payable to these functions for gas savings

5. Use calldata instead of memory for function parameters

Impact

Use calldata instead of memory for function parameters. Having function arguments use calldata instead of memory can save gas.

Proof of Concept

There are many cases of function arguments using memory instead of calldata. Some examples are
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L42
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L98
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L146
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L148
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L484

Tools Used

Manual analysis

Recommended Mitigation Steps

Change function arguments from memory to calldata, for example from NFTCategoryInitializer[] memory _typeInitializers to NFTCategoryInitializer[] calldata _typeInitializers

6. Use newer solidity version

Impact

Solidity 0.7.6 is used in JPEG'd contracts. Never versions of solidity include gas optimizations.

Proof of Concept

One example from solidity 0.8.10 is quoted from https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/#full-changelog

Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist

The request function is one example of where this gas savings would apply
https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/PooledCreditLine.sol#L638

Tools Used

Manual analysis

Recommended Mitigation Steps

Use a newer solidity release

7. Replace _setupRole call with _grantRole

Impact

The _setupRole function calls _grantRole, so it would save gas to call _grantRole directly

Proof of concept

The _setupRole function, which is deprecated, is found in several places
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L153
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L75
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/JPEG.sol#L17
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L26
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L26
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L152

This Open Zeppelin _setupRole code shows it is deprecated and only calls _grantRole
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L197-L199

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace the _setupRole function with _grantRole from the same Open Zeppelin library

8. Skip safe checks on trusted tokens

Impact

The JPEG token is created and managed by the JPEG'd project. Because of this, it does not need to use safeTransfer or safeTransferFrom checks. Using "unsafe" checks will save gas.

Proof of Concept

safeTransfer and safeTransferFrom are applied to the jpeg token in several places
https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/lock/JPEGLock.sol#L54
https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/lock/JPEGLock.sol#L75
https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/farming/LPFarming.sol#L128
https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/farming/LPFarming.sol#L130
https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/farming/LPFarming.sol#L339
https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/farming/LPFarming.sol#L356

Tools Used

Manual analysis

Recommended Mitigation Steps

Use transfer instead of safeTransfer and transferFrom instead of safeTransferFrom for the JPEG token calls

9. Short require strings save gas

Impact

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

Proof of Concept

Several cases of this gas optimization were found. These are a few examples, but more may exist

  1. https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L394
  2. https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L41
  3. https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L55
  4. https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L69
  5. https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/JPEG.sol#L23

Tools Used

Manual analysis

Recommended Mitigation Steps

Shorten require strings

10. selfdestruct call unnecessary in constructor

Impact

The selfdestruct call in NFTEscrow is unnecessary and wastes gas. The contract can be left as is, or a separate public selfdestruct function can be made to allow anyone to destroy the contract at a later time.

Proof of Concept

Moving the selfdestruct call to a public function that anyone can call saves saves on deployment but leaves the option open to destroy the contract as needed
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/escrow/NFTEscrow.sol#L20

Tools Used

Manual analysis

Recommended Mitigation Steps

Move the selfdestruct call outside the constructor to a public function that anyone can call to save gas on this selfdestruct call.

11. Redundant zero initialization

Impact

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

Proof of Concept

Several places have this issue, mostly in for loops
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L181
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L184
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L281
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L348
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L145
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L231
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L319

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the redundant zero initializations so they look like this uint256 i;

12. Cache array length before loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop. This saves gas.

Proof of Concept

This optimization is already used in some places, but several places do not use this optimization
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L181
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L184
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L348
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L145
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L319

Tools Used

Manual analysis

Recommended Mitigation Steps

Cache the array length before the for loop

@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Apr 13, 2022
code423n4 added a commit that referenced this issue Apr 13, 2022
@spaghettieth spaghettieth added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Apr 14, 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) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

2 participants