QA Report

[code423n4]

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