JPEG'd contest - 0xkatana's results

Lines of code

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

Vulnerability details

Impact

The Chainlink API latestAnswer function is used in two places but it is deprecated:

This API is deprecated. Please see API Reference for the latest Price Feed API. Chainlink Docs

The latestAnswer function does not revert if no answer has been reached but returns 0. Another confusing feature of latestAnswer is that the data is reported with 18 decimals for crypto quotes but 8 decimals for FX quotes according to Chainlink docs. This requires getting the decimals from the oracles instead of hard-coding them in the contract.

Proof of concept

Places where latestAnswer is used https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L459 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L105

Tools Used

Manual analysis

Recommended Mitigation Steps

Use the latestRoundData Chainlink function to get the price instead. When using the latestRoundData function, the return data much be checked for a stale price or an incomplete round.

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

1. Use prefix not postfix in loops

Impact

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

Proof of Concept

Prefix is used in some for loops, but there are still many examples of postfix in loops in the code 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/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 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#L185

Tools Used

Manual analysis

Recommended Mitigation Steps

Use prefix not postfix to increment in a loop

2. Split up require statements instead of &&

Impact

Combining require statement conditions with && logic uses unnecessary gas. It is better to split up each part of the logical statement into a separate require statements

Proof of Concept

Line 194 of FungibleAssetVaultForDAO

require(amount > 0 && amount <= collateralAmount, "invalid_amount");

Instead, split into two to save gas

require(amount > 0, "invalid_amount");
require(amount <= collateralAmount, "invalid_amount");

Tools Used

Manual analysis

Recommended Mitigation Steps

Use separate require statements instead of concatenating with &&

3. Use != 0 instead of > 0

Impact

Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero

Proof of Concept

Locations where this was found include https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L94 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L108 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L142 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L164 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L180 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L194

Tools Used

grep

Recommended Mitigation Steps

Replace > 0 with != 0 to save gas

4. Add payable to functions that won't receive ETH

Impact

Marking a function as payable saves gas. Functions that have the onlyOwner modifier cannot be called by normal users and will not mistakenly receive ETH. These functions can be payable to save gas.

Proof of Concept

There are many functions that have the onlyOwner modifier and can be payable. Some examples are https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L91 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L98 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L108 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L115 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L200

Tools Used

Manual analysis

Recommended Mitigation Steps

Add payable to these functions for gas savings

5. Use calldata instead of memory for function parameters

Impact

Use calldata instead of memory for function parameters. Having function arguments use calldata instead of memory can save gas.

Proof of Concept

There are many cases of function arguments using memory instead of calldata. Some examples are https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L42 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L98 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#L148 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L484

Tools Used

Manual analysis

Recommended Mitigation Steps

Change function arguments from memory to calldata, for example from NFTCategoryInitializer[] memory _typeInitializers to NFTCategoryInitializer[] calldata _typeInitializers

6. Use newer solidity version

Impact

Solidity 0.7.6 is used in JPEG'd contracts. Never versions of solidity include gas optimizations.

Proof of Concept

One example from solidity 0.8.10 is quoted from https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/#full-changelog

Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist

The request function is one example of where this gas savings would apply https://github.com/sublime-finance/sublime-v1/blob/46536a6d25df4264c1b217bd3232af30355dcb95/contracts/PooledCreditLine/PooledCreditLine.sol#L638

Tools Used

Manual analysis

Recommended Mitigation Steps

Use a newer solidity release

7. Replace _setupRole call with _grantRole

Impact

The _setupRole function calls _grantRole, so it would save gas to call _grantRole directly

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 _setupRole code shows it is deprecated and only calls _grantRole https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L197-L199

Tools Used

Manual analysis

Recommended Mitigation Steps

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

8. Skip safe checks on trusted tokens

Impact

The JPEG token is created and managed by the JPEG'd project. Because of this, it does not need to use safeTransfer or safeTransferFrom checks. Using "unsafe" checks will save gas.

Proof of Concept

safeTransfer and safeTransferFrom are applied to the jpeg token in several places 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 https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/farming/LPFarming.sol#L128 https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/farming/LPFarming.sol#L130 https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/farming/LPFarming.sol#L339 https://github.com/code-423n4/2022-04-jpegd/tree/main/contracts/farming/LPFarming.sol#L356

Tools Used

Manual analysis

Recommended Mitigation Steps

Use transfer instead of safeTransfer and transferFrom instead of safeTransferFrom for the JPEG token calls

9. Short require strings save gas

Impact

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

Proof of Concept

Several cases of this gas optimization were found. These are a few examples, but more may exist

1. https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L394
2. https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L41
3. https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L55
4. https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L69
5. https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/JPEG.sol#L23

Tools Used

Manual analysis

Recommended Mitigation Steps

Shorten require strings

10. selfdestruct call unnecessary in constructor

Impact

The selfdestruct call in NFTEscrow is unnecessary and wastes gas. The contract can be left as is, or a separate public selfdestruct function can be made to allow anyone to destroy the contract at a later time.

Proof of Concept

Moving the selfdestruct call to a public function that anyone can call saves saves on deployment but leaves the option open to destroy the contract as needed https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/escrow/NFTEscrow.sol#L20

Tools Used

Manual analysis

Recommended Mitigation Steps

Move the selfdestruct call outside the constructor to a public function that anyone can call to save gas on this selfdestruct call.

11. Redundant zero initialization

Impact

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

Proof of Concept

Several places have this issue, mostly in for loops 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/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/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

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the redundant zero initializations so they look like this uint256 i;

12. Cache array length before loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop. This saves gas.

Proof of Concept

This optimization is already used in some places, but several places do not use this optimization 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/farming/LPFarming.sol#L348 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#L319

Tools Used

Manual analysis

Recommended Mitigation Steps

Cache the array length before the for loop