JPEG'd contest - ilan's results

QA report - Low/Non-critical

NC01: Uneccesary position.borrowType == BorrowType.USE_INSURANCE in if statement

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

if(position.borrowType == BorrowType.USE_INSURANCE || _useInsurance) can be changed to if(_useInsurance)

Because, if the BorrowType is USE_INSURANCE then

    require(
        position.borrowType == BorrowType.NOT_CONFIRMED ||
            (position.borrowType == BorrowType.USE_INSURANCE &&
                _useInsurance) ||
            (position.borrowType == BorrowType.NON_INSURANCE &&
                !_useInsurance),
        "invalid_insurance_mode"
    );

will pass only if _useInsurance = True. Therefore, position.borrowType == BorrowType.USE_INSURANCE -> _useInsurance = True. Hence, it is enough to check if(_useInsurance)

NC02: Missing parameter validity checks in setNFTTypeValueETH, setPendingNFTValueETH

Functions setNFTTypeValueETH, setPendingNFTValueETH in NFTVault Should check that _amountETH > 0 to avoid uneccesary write to nftTypeValueETH[_type] storage variable.

NC03: Missing @return comment

These functions have a return but do not document it:

LPFarming.sol._withdrawReward, yVaultLPFarming.sol._withdrawReward, NFTEscrow._encodeFlashEscrowPayload, EtherRocksHelper._encodeFlashEscrowPayload, CryptoPunksHelper._encodeFlashEscrowPayload, StrategyPUSDConvex.withdraw

NC04: Missing struct comments

Some structs have untrivial names / variables and should be documented.

JPEGLock.LockPosition, NFTVault.Position, NFTVault.VaultSettings, NFTVault.NFTCategoryInitializer, NFTVault.NFTInfo, NFTVault.PositionPreview, Rate (multiple contracts)

NC05: Functions in JPEG.sol missing documentation

Comments should be added to the file JPEG.sol.

NC06: public function should be external

These functions should be external as they are public but not called locally in the contract.

Controller.withdraw, yVault.setFarmingPool

L01: Multiplication after division can cause wrong calculation

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

In NFTVault.sol, the following code is used:

    uint256 interestPerSec = interestPerYear / 365 days;

    return elapsedTime * interestPerSec;

notice that if interestPerYear < 31536000, then interestPerSec would be 0. Therefore, elapsedTime * interestPerSec; would be 0 and the additional interest returned would be 0, even though elapsedTime could be very high.

Recommended Mitigation Steps

change the above code to

    uint256 additionalInterestYearly = elapsedTime * interestPerYear;

    return additionalinterestPerYearly / 365 days;

L02: Possible denial of service when calling external function inside a loop

When making an external call during a loop, if the external call failes it will result in all of the calls for all the loop iterations to fail. This could happen in the contract LPFarming.

Proof of concept

In LPFarming._massUpdatePools(), _updatePool() is called on every pool using a loop. Inside _updatePool() , we can find the external call

    uint256 lpSupply = pool.lpToken.balanceOf(address(this));

Now, if, for any reason, balanceOf for a specific token will revert, it will revert the whole _massUpdatePools() operation. This could happen if an attacker is able to insert a token with a dangerous balanceOf on purpose, or if it reverts by accident. Therefore, after this attack the functions newEpoch, add, set which call _massUpdatePools() will revert

Gas optimizations

Change i++ to ++i

i++ is generally more expensive because it must increment a value and return the old value, so it may require holding two numbers in memory. ++i only uses one number in memory.

Example:

for (uint256 i = 0; i < poolInfo.length; i++) -> for (uint256 i = 0; i < poolInfo.length; i++) (LPFarming 348)

Affected contracts: LPFarming.sol, NFTVault.sol, StrategyPUSDConvex.sol

No need to explicitly initialize variables with default values

If a variable is not initialized it is automatically set to the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Example:

for (uint256 i = 0; i < poolInfo.length; i++) -> for (uint256 i; i < poolInfo.length; i++)

Affected contracts: LPFarming.sol, NFTVault.sol, StrategyPUSDConvex.sol

Loop storage/memory acess cache

Calling storage/memory variable which does not change, every loop iteration, is wrong and wastes gas.

for (uint256 i = 0; i < poolInfo.length; i++) ->

uint256 length = poolInfo.length; for (uint256 i = 0; i < length; i++)

Affected contracts: LPFarming.sol, NFTVault.sol, StrategyPUSDConvex.sol

Use calldata

Use calldata instead of memory for external functions where the function argument is read-only.

function setBorrowAmountCap(uint256 _borrowAmountCap) external -> function setBorrowAmountCap(uint256 calldata _borrowAmountCap) external

Affected contracts: Neary every contract

Variables should be immutable

Some variables could be set immutable to save gas

collateralAsset in FungibleAssetVaultForDAO _collateralUnit in FungibleAssetVaultForDAO

Unused import

unused import wastes gas.

import "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol"; in NFTEscrow