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

1 stars 1 forks source link

Gas Optimizations #110

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

1- For loop index increments can be made unchecked. 2- Prefix increments are cheaper than postfix increments. 3- There is no need to set uint256 variable to 0 since default value is already 0.

Lines of code

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/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/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

Vulnerability details

Impact

There is no risk of overflowing the index of the for loops. Therefore, they can be changed to unchecked to save gas. This would save more gas as iterations in the loop increases. Also postfix increments can be changed as prefix as it is cheaper. Setting uint256 index to 0 is redundant since default value is already 0.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the index increments to unchecked and prefix such as: for (uint256 j ; j < initializer.nfts.length; ) { nftTypes[initializer.nfts[j]] = initializer.hash; unchecked { ++j; } }

Constant keccak variables can be changed to immutable.

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L15 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L41 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/JPEG.sol#L10 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L71 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L72 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L74 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L22 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L23

Vulnerability details

Impact

When these variables defined as constant, keccak operation will be performed whenever the variable is used. However, if they are immutable, keccak hashing would be performed just once.

Proof of Concept

https://github.com/code-423n4/2021-10-slingshot-findings/issues/3

Tools Used

Manual analysis

Recommended Mitigation Steps

Change from constant to immutable.

Public variables can be private

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L15 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L17 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L18 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L20 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L21 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L22 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L41 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L47 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L48 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L53 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L55 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L58 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L60 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/JPEG.sol#L10 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L24 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L26 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L28 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/staking/JPEGStaking.sol#L18 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L56 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L60 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L62 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L64 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L67 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L73

Vulnerability details

Impact

Almost all of the public variables have public visibility where they can be private. Private state variables are cheaper.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change from public to private.

Error messages longer than 32 bytes

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L394 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L41 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L55 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L69

Vulnerability details

Impact

Error strings longer than 32 bytes are more expensive.

Proof of Concept

https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6#c17b

Tools Used

Manual analysis

Recommended Mitigation Steps

Limit the error strings to 32 bytes max.

Use custom errors for revert

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/helpers/CryptoPunksHelper.sol#L91 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/helpers/EtherRocksHelper.sol#L96 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L82 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L364 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L201 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L190

Vulnerability details

Impact

It is possible to use custom errors for revert statements starting from solidity 0.8.4. Custom errors are cheaper.

Proof of Concept

https://blog.soliditylang.org/2021/04/21/custom-errors/

Tools Used

Manual analysis

Recommended Mitigation Steps

Use custom errors for revert.

Use calldata for read-only arguments of external functions

Lines of code

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#L212 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L222 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L232 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L247 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L290 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L300 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L311

Vulnerability details

Impact

When the read-only arguments of external functions are defined as memory, they are copied to memory space whereas they could have directly read from calldata.

Proof of Concept

https://github.com/code-423n4/2022-01-livepeer-findings/issues/61

Tools Used

Manual analysis

Recommended Mitigation Steps

Use calldata instead of memory where appropriate

Some internal functions can be made private

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L104 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L120 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L256 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L266 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L279 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L288 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L315 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/escrow/NFTEscrow.sol#L44 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L387 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L400 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L410 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L430 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L441 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L447 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L454 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L498 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L512 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L526 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L547 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L559 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L578 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L155 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L168 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L177

Vulnerability details

Impact

Calling private functions are cheaper than calling internal functions. Therefore, it is better to declare functions private if they are not called from inherited contracts.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the visibility to private where possible.

Strict inequalities are more expensive than non-strict inequalities

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L271 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L273

Vulnerability details

Impact

In the lines above non-strict inequlalities <= and >= can be used without disrupting the logic. Non-strict inequalities are cheaper.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Use non-strict inequalities at these lines and where appropriate.

A couple of local variables can be deleted.

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L589-L595

Vulnerability details

Impact

All the calculation can be done in a single line by getting rid of the interestPerYear and interestPerSec local variables.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

The code can be changed as: // Accrue interest // uint256 interestPerYear = (totalDebtAmount * // settings.debtInterestApr.numerator) / // settings.debtInterestApr.denominator; // uint256 interestPerSec = interestPerYear / 365 days;

    return elapsedTime * totalDebtAmount * settings.debtInterestApr.numerator /
        settings.debtInterestApr.denominator / 365 days;

Caching can be avoided

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L503 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L517

Vulnerability details

Impact

_getNFTValueUSD(_nftIndex) can directly be used in the return statement, rather than being cached first.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

The code can be changed as: //uint256 asset_value = _getNFTValueUSD(_nftIndex); return (_getNFTValueUSD(_nftIndex) * settings.creditLimitRate.numerator) / settings.creditLimitRate.denominator;

organizationFee variable can be eliminated

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L713

Vulnerability details

Impact

organizationFee is calculated and then the value is assigned to feeAmount and after that point organizationFee is not used elsewhere. Thus, organizationFee can be replaced by feeAmount to save a variable and also an assignment.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

The code can be changed as:

    //calculate the borrow fee
    uint256 feeAmount = (_amount *
        settings.organizationFeeRate.numerator) /
        settings.organizationFeeRate.denominator;

    //uint256 feeAmount = organizationFee;
    //if the position is insured, calculate the insurance fee