code-423n4 / 2022-04-jpegd-findings

1 stars 1 forks source link

Gas Optimizations #185

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Gas

Issues found

1. All contracts using unlocked pragma

Impact

All the contracts in scope use pragma solidity ^0.8.0, allowing wide enough range of versions.

Proof of Concept

pragma version^0.8.0 (contracts/escrow/NFTEscrow.sol#2) 
pragma version^0.8.0 (contracts/staking/JPEGStaking.sol#2) 
pragma version^0.8.0 (contracts/vaults/FungibleAssetVaultForDAO.sol#2) 
pragma version^0.8.0 (contracts/vaults/NFTVault.sol#2) 
pragma version^0.8.0 (contracts/farming/LPFarming.sol#2) 
pragma version^0.8.0 (contracts/farming/yVaultLPFarming.sol#2) 
pragma version^0.8.0 (contracts/tokens/JPEG.sol#2) 
pragma version^0.8.0 (contracts/tokens/StableCoin.sol#2) 
pragma version^0.8.0 (contracts/vaults/yVault/Controller.sol#2) 
pragma version^0.8.0 (contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#2) 
pragma version^0.8.0 (contracts/vaults/yVault/yVault.sol#2) 

Recommendation

Consider locking compiler version, for example pragma solidity 0.8.6. This can have additional benefits, for example using custom errors to save gas and so forth.

2. Cache Array Length Outside of Loop.

Impact

Cache length outside of for loop to save gas.

Proof of Concept

LPFarming.sol#L348, NFTVault.sol#L181, NFTVault.sol#L184, StrategyPUSDConvex.sol#L145, StrategyPUSDConvex.sol#L319.

Recommendation

More gas efficient example for LPFarming::claimAll().

function claimAll() external nonReentrant noContract(msg.sender) {
    uint256 len = poolInfo.length;
    for (uint256 i = 0; i < len; ++i) {
        _updatePool(i);
        _withdrawReward(i);
    }

Similar changes apply for the rest of the for-loops listed above.

3. Use != 0 instead of > 0 for Unsigned Integer Comparison.

Impact

!= 0 is cheapear than > 0 when comparing unsigned integers.

Proof of Concept

LPFarming.sol#L114, LPFarming.sol#L218, LPFarming.sol#L239, LPFarming.sol#L320, LPFarming.sol#L337, LPFarming.sol#L354, yVaultLPFarming.sol#L84, yVaultLPFarming.sol#L101, yVaultLPFarming.sol#L118, yVaultLPFarming.sol#L139, yVaultLPFarming.sol#L181, JPEGLock.sol#L40, JPEGStaking.sol#L32, JPEGStaking.sol#L46, FungibleAssetVaultForDAO.sol#L142, FungibleAssetVaultForDAO.sol#L164, FungibleAssetVaultForDAO.sol#L180, FungibleAssetVaultForDAO.sol#L194, NFTVault.sol#L278, NFTVault.sol#L365, NFTVault.sol#L637, NFTVault.sol#L687, NFTVault.sol#L764, NFTVault.sol#L770, NFTVault.sol#L882, NFTVault.sol#L926, StrategyPUSDConvex.sol#L322, StrategyPUSDConvex.sol#L334, yVault.sol#L143, yVault.sol#L167, yVault.sol#L170.

Recommendation

Use != 0 instead of > 0.

4. Revert Strings > 32bytes.

Impact

Revert strings > 32bytes use more gas.

Proof of Concept

JPEG.sol#L23, StableCoin.sol#L41, StableCoin.sol#L55, StableCoin.sol#L69, NFTVault.sol#L394.

Recommendation

Make revert strings are <= 32bytes.

5. Functions can be external

Impact

public functions that are never called by the contract should be declared external to save gas.

Proof of Concept

Controller.sol#L151, yVault.sol#L115.

Recommendation

withdraw(IERC20,uint256) should be declared external:
        - Controller.withdraw(IERC20,uint256) (contracts/vaults/yVault/Controller.sol#151-154)
setFarmingPool(address) should be declared external:
        - YVault.setFarmingPool(address) (contracts/vaults/yVault/yVault.sol#115-118)

6. No need to initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

FungibleAssetVaultForDAO.sol#L45, LPFarming.sol#L348, NFTVault.sol#L181, NFTVault.sol#L184, StrategyPUSDConvex.sol#L145, StrategyPUSDConvex.sol#L319.

Recommendation

Remove explicit zero initialization.

7. Unnecessary variable assignment.

Impact

In NFTVault::borrow(), since a new uint256 variable feeAmount is set to equal organizationFee and organizationFee does not get updated after, there is no reason to use feeAmount when you can just use organizationFee. Waste of gas.

Proof of Concept

NFTVault.sol#L713-L717

Recommendation

Get rid of feeAmount and use organizationFee instead.

Tools used

c4rena, manual, slither.