QA Report

[code423n4]

1. Low - Deprecated _setupRole function used

Impact

The _setupRole function is deprecated according to the Open Zeppelin comment NOTE: This function is deprecated in favor of {_grantRole}

Use the recommended _grantRole function instead.

Proof of concept

The _setupRole function, which is deprecated, is found in several places https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L153 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L75 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/JPEG.sol#L17 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L26 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L26 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L152

This Open Zeppelin comment indicates it is deprecated https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L195

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace the _setupRole function with _grantRole from the same Open Zeppelin library

2. Low - Incompatible with popular NFTs

Impact

The JPEG'd contract show awareness that CryptoPunks and EtherRocks are early NFTs that don't follow the standard ERC721 format. Cryptokitties is another popular project that does not follow the standard ERC721. Although CryptoKitties are not as valuable individually as CryptoPunks and EtherRocks, there is a large population so they have a large market cap. Incompatibility with Cryptokitties could cause NFTs to get locked/lost in JPEG'd or prevent many users with Cryptokitties from using the platform. Projects including NFTX have special code to handle cryptokitties, for example https://github.com/NFTX-project/nftx-protocol-v2/blob/master/contracts/solidity/NFTXMarketplaceZap.sol#L562-L564

Proof of concept

The helpers directory contains contracts for CryptoPunks and EtherRocks, but not CryptoKitties https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/helpers

Tools Used

Manual analysis

Recommended Mitigation Steps

Add custom support for Cryptokitties to enable support for a popular large market cap NFT

3. Low - Rate change functions doesn't consider current positions

Impact

The NFTVault and FungibleAssetVaultForDAO contracts have functions with the names setCreditLimitRate and setLiquidationLimitRate to change the credit limit and liquidation limit. These functions are called manually by an admin, but besides the access control check, there is no other check to verify that the change does not place some users positions outside the new limits. This can have unexpected consequences with how the protocol operates because user positions will exist outside expected bounds.

Proof of concept

Several limit change functions exist, but there are no checks whether users maintain positions beyond the new limit that is set. For example, if an 90% loan-to-value ratio is permitted but the limit is changed to only allow a 80% loan-to-value ratio, the users who took out a 90% LTV loan before the setting changed are operating outside the assumed limits of the new limit values. Because no on-chain checks exist, the limit will be changed and the user may be unexpectedly blocked from certain actions or placed into more unusual edge cases.

setCreditLimitRate and setLiquidationLimitRate functions in NFTVault 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

setCreditLimitRate function in FungibleAssetVaultForDAO https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L92

Tools Used

Manual analysis

Recommended Mitigation Steps

Add some basic checks into the setCreditLimitRate and setLiquidationLimitRate functions to prevent situations where a user is place into a "disallowed" position. The best approach is a for loop to check all current positions, or a state variable that maintains the maximum LTV of any position which is more easily queried but harder to keep updated. On-chain checks are particularly important because user positions may change earlier in the same block that the limit is changed, either intentional frontrunning or coincidental, and off-chain checks would not be able to easily catch such a scenario.

4. Low - transfer return value is ignored

Impact

The transfer return value should be checked for edge case scenarios where some tokens return false instead reverting on failure. safeTransfer and safeTransferFrom should be used instead of transfer or transferFrom.

Proof of Concept

safeTransferFrom is already used in many places, but it is not always used. safeTransferFrom should be used instead of transferFrom in many places https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L899 https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/staking/JPEGStaking.sol#L34

safeTransfer and safeTransferFrom are applied to the jpeg token elsewhere https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/lock/JPEGLock.sol#L54 https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/lock/JPEGLock.sol#L75

Tools Used

Manual analysis

Recommended Mitigation Steps

Use safeTransferFrom instead of transferFrom or check the return value of transferFrom

5. Low - Incorrect Comment

Impact

There is an error in a natspec comment in StableCoin. There are two references to a contract named AssetVaultForDAO, but there is no contract with this name. Instead, the natspec comment should reference FungibleAssetVaultForDAO. This problem might be an artifact from when the contract was renamed. This is considered low risk because it is "issues with comments" (source: code4rena judging criteria)

Proof of concept

This error exists in two places https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L11 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L14

Tools Used

Manual analysis

Recommended Mitigation Steps

Reference the FungibleAssetVaultForDAO contract instead of the AssetVaultForDAO contract

6. Low - Math precision optimization in finalizePendingNFTValueETH

Impact

The math in finalizePendingNFTValueETH contains many division operations that will result in a loss of precision. This impacts the amount of tokens that need to be locked, which impacts user funds. To maintain as much precision as possible, multiplication operations should be performed first and division operations after.

Proof of concept

The math in finalizePendingNFTValueETH is currently https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L366-L372

uint256 toLockJpeg = (((pendingValue *
    _ethPriceUSD() *
    settings.creditLimitRate.numerator) /
    settings.creditLimitRate.denominator) *
    settings.valueIncreaseLockRate.numerator) /
    settings.valueIncreaseLockRate.denominator /
    _jpegPriceUSD();

By performing multiplications first, we can reduce the number of division operations and increase precision

uint256 toLockJpeg = (((pendingValue *
    _ethPriceUSD() *
    settings.creditLimitRate.numerator *
    settings.valueIncreaseLockRate.numerator) /
    (settings.creditLimitRate.denominator *
    settings.valueIncreaseLockRate.denominator) /
    _jpegPriceUSD();

Tools Used

Manual analysis

Recommended Mitigation Steps

Perform all multiplication operations prior to division operations to increase math precision

7. Low - Math precision optimization in finalizePendingNFTValueETH

Impact

The math in _calculateAdditionalInterest contains two division operations instead of reducing it to one, which will result in a loss of precision. This impacts the interest rate, which impacts user funds. To maintain as much precision as possible, multiplication operations should be performed first and division operations after.

Proof of concept

The math in _calculateAdditionalInterest is currently https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L590-L593

uint256 interestPerYear = (totalDebtAmount *
    settings.debtInterestApr.numerator) /
    settings.debtInterestApr.denominator;
uint256 interestPerSec = interestPerYear / 365 days;

By performing multiplications first, we can reduce the number of division operations and increase precision

uint256 interestPerSec = (totalDebtAmount *
    settings.debtInterestApr.numerator) /
    (settings.debtInterestApr.denominator * 365 days);

Tools Used

Manual analysis

Recommended Mitigation Steps

Perform all multiplication operations prior to division operations to increase math precision