JPEG'd contest - ellahi's results

QA Report

Low

1. Lack of Zero Address Check

Proof of Concept

StrategyPUSDConvex.sol#L303-L307.

function withdrawJPEG(address _to) external onlyController {
    // claim from convex rewards pool
    convexConfig.baseRewardPool.getReward(address(this), true);
    jpeg.safeTransfer(_to, jpeg.balanceOf(address(this)));
}

Consider adding require(_to != address(0), "INVALID: 0"); to prevent accidental loss of funds.

2. Unsafe ERC20 Operations

Not consitently using safetransfers. Some cases of transfer and transferFrom without checking return value.

Proof of Concept

JPEGStaking.sol#L34, JPEGStaking.sol#L52, NFTVault.sol#L560, NFTVault.sol#L899.

Use safeTransfer and safeTransferFrom from OpenZeppelin's library.

3. Do not use Deprecated Library Functions

_setupRole function is deprecated in favor of _grantRole. https://docs.openzeppelin.com/contracts/4.x/api/access

Proof of Concept

JPEG.sol#L17, StableCoin.sol#L26, FungibleAssetVaultForDAO.sol#L75, NFTVault.sol#L153, Controller.sol#L26, StrategyPUSDConvex.sol#L152.

Non-Critical

1. Typo

NFTVault.sol#L129

/// @param _nftContract The NFT contrat address. It could also be the address of an helper contract

Change contrat to contract.

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.