QA Report

[code423n4]

Low Risk Issues

Missing checks for address(0x0) when assigning values to address state variables

1. File: contracts/escrow/NFTEscrow.sol (line 38)

       nftAddress = _nftAddress;

2. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 79)

       collateralAsset = _collateralAsset;

Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

1. File: contracts/helpers/CryptoPunksHelper.sol (line 16)

   contract CryptoPunksHelper is NFTEscrow, OwnableUpgradeable {

2. File: contracts/helpers/EtherRocksHelper.sol (line 16)

   contract EtherRocksHelper is NFTEscrow, OwnableUpgradeable {

Non-critical Issues

public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

1. File: contracts/vaults/yVault/Controller.sol (line 151)

   function withdraw(IERC20 _token, uint256 _amount) public {

2. File: contracts/vaults/yVault/yVault.sol (line 115)

   function setFarmingPool(address _farm) public onlyOwner {

constants should be defined rather than using magic numbers

1. File: contracts/escrow/NFTEscrow.sol (line 98)

               bytes1(0xff),

2. File: contracts/vaults/NFTVault.sol (line 465)

           decimals > 18

3. File: contracts/vaults/NFTVault.sol (line 466)

               ? uint256(answer) / 10**(decimals - 18)

4. File: contracts/vaults/NFTVault.sol (line 467)

               : uint256(answer) * 10**(18 - decimals);

5. File: contracts/vaults/NFTVault.sol (line 593)

       uint256 interestPerSec = interestPerYear / 365 days;

6. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 112)

           decimals > 18

7. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 113)

               ? uint256(answer) / 10**(decimals - 18)

8. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 114)

               : uint256(answer) * 10**(18 - decimals);

9. File: contracts/vaults/yVault/yVault.sol (line 196)

       return (balance() * 1e18) / supply;

10. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 126)

        require(_curveConfig.pusdIndex < 4, "INVALID_PUSD_CURVE_INDEX");

11. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 127)

        require(_curveConfig.usdcIndex < 4, "INVALID_USDC_CURVE_INDEX");

12. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 343)

                    abi.encodePacked(weth, uint24(500), usdc),

13. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 368)

            10**12;

14. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 371)

        uint256[4] memory liquidityAmounts = [uint256(0), 0, 0, 0];

15. File: contracts/farming/yVaultLPFarming.sol (line 94)

            1e36;

16. File: contracts/farming/yVaultLPFarming.sol (line 172)

        newAccRewardsPerShare = accRewardPerShare + newRewards * 1e36 / totalStaked;

17. File: contracts/farming/yVaultLPFarming.sol (line 179)

            (accRewardPerShare - userLastAccRewardPerShare[account])) / 1e36;

18. File: contracts/farming/LPFarming.sol (line 196)

                1e36 *

19. File: contracts/farming/LPFarming.sol (line 207)

            1e36;

20. File: contracts/farming/LPFarming.sol (line 307)

            1e36 *

21. File: contracts/farming/LPFarming.sol (line 319)

            1e36;

22. File: contracts/lock/JPEGLock.sol (line 33)

        lockTime = 365 days;

Use a more recent version of solidity

Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>) Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)

1. File: contracts/escrow/NFTEscrow.sol (line 2)

   pragma solidity ^0.8.0;

2. File: contracts/vaults/yVault/yVault.sol (line 2)

   pragma solidity ^0.8.0;

3. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 2)

   pragma solidity ^0.8.0;

Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

1. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 368)

           10**12;

Non-library/interface files should use fixed compiler versions, not floating ones

1. File: contracts/escrow/NFTEscrow.sol (line 2)

   pragma solidity ^0.8.0;

2. File: contracts/vaults/NFTVault.sol (line 2)

   pragma solidity ^0.8.0;

3. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 2)

   pragma solidity ^0.8.0;

4. File: contracts/vaults/yVault/Controller.sol (line 2)

   pragma solidity ^0.8.0;

5. File: contracts/vaults/yVault/yVault.sol (line 2)

   pragma solidity ^0.8.0;

6. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 2)

   pragma solidity ^0.8.0;

7. File: contracts/tokens/StableCoin.sol (line 2)

   pragma solidity ^0.8.0;

8. File: contracts/tokens/JPEG.sol (line 2)

   pragma solidity ^0.8.0;

9. File: contracts/staking/JPEGStaking.sol (line 2)

   pragma solidity ^0.8.0;

10. File: contracts/farming/yVaultLPFarming.sol (line 2)

    pragma solidity ^0.8.0;

11. File: contracts/farming/LPFarming.sol (line 2)

    pragma solidity ^0.8.0;

12. File: contracts/lock/JPEGLock.sol (line 2)

    pragma solidity ^0.8.0;

13. File: contracts/helpers/CryptoPunksHelper.sol (line 2)

    pragma solidity ^0.8.0;

14. File: contracts/helpers/EtherRocksHelper.sol (line 2)

    pragma solidity ^0.8.0;

File is missing NatSpec

1. File: contracts/tokens/JPEG.sol (line 0)

   // SPDX-License-Identifier: GPL-3.0

NatSpec is incomplete

1. File: contracts/escrow/NFTEscrow.sol (lines 56-62)

   /// @dev Virtual function, should return the `payload` to use in {FlashEscrow}'s constructor
   /// @param _idx The index of the NFT that's going to be sent to the {FlashEscrow} instance
   function _encodeFlashEscrowPayload(uint256 _idx)
       internal
       view
       virtual
       returns (bytes memory);

   Missing: @return

2. File: contracts/vaults/yVault/Controller.sol (lines 24-25)

   /// @param _feeAddress The address to send fees to
   constructor(address _jpeg, address _feeAddress) {

   Missing: @param _jpeg

3. File: contracts/vaults/yVault/yVault.sol (lines 37-49)

   /// @param _token The token managed by this vault
   /// @param _controller The JPEG'd strategies controller
   constructor(
       address _token,
       address _controller,
       Rate memory _availableTokensRate
   )
       ERC20(
           string(
               abi.encodePacked("JPEG\xE2\x80\x99d ", ERC20(_token).name())
           ),
           string(abi.encodePacked("JPEGD", ERC20(_token).symbol()))
       )

   Missing: @param _availableTokensRate

4. File: contracts/farming/yVaultLPFarming.sol (lines 175-177)

   /// @dev Updates `account`'s claimable rewards by adding pending rewards
   /// @param account The account to update
   function _withdrawReward(address account) internal returns (uint256) {

   Missing: @return

5. File: contracts/farming/LPFarming.sol (lines 260-269)

   /// @dev Normalizes `blockNumber` to fit within the bounds of an epoch.
   /// This is done to ensure that no rewards are distributed for staking outside of an epoch without modifying the reward logic.
   /// For example:
   /// `blockNumber` is 1100, the epoch's `endBlock` is 1000. In this case the function would return 1000. If this value were to be used
   /// in the {_updatePool} function, where the pool's `lastRewardBlock` is 990, only the rewards from block 990 to block 1000 would be distributed
   /// @return Normalized `blockNumber`
   function _normalizeBlockNumber(uint256 blockNumber)
       internal
       view
       returns (uint256)

   Missing: @param blockNumber

6. File: contracts/farming/LPFarming.sol (lines 313-315)

   /// @dev Updates `msg.sender`'s claimable rewards by adding pending rewards from `_pid`
   /// @param _pid The pool to withdraw rewards from
   function _withdrawReward(uint256 _pid) internal returns (uint256) {

   Missing: @return

7. File: contracts/helpers/CryptoPunksHelper.sol (lines 94-100)

   /// @dev The {transferPunk} function is used as the escrow's payload.
   /// @param _idx The index of the punk that's going to be transferred using {NFTEscrow}
   function _encodeFlashEscrowPayload(uint256 _idx)
       internal
       view
       override
       returns (bytes memory)

   Missing: @return

8. File: contracts/helpers/EtherRocksHelper.sol (lines 99-105)

   /// @dev The {giftRock} function is used as the escrow's payload.
   /// @param _idx The index of the rock that's going to be transferred using {NFTEscrow}
   function _encodeFlashEscrowPayload(uint256 _idx)
       internal
       view
       override
       returns (bytes memory)

   Missing: @return

Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

1. File: contracts/vaults/NFTVault.sol (lines 23-27)

   event Borrowed(
       address indexed owner,
       uint256 indexed index,
       uint256 amount
   );

2. File: contracts/vaults/NFTVault.sol (line 28)

   event Repaid(address indexed owner, uint256 indexed index, uint256 amount);

3. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 31)

   event Deposit(address indexed user, uint256 depositAmount);

4. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 32)

   event Borrow(address indexed user, uint256 borrowAmount);

5. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 33)

   event Repay(address indexed user, uint256 repayAmount);

6. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 34)

   event Withdraw(address indexed user, uint256 withdrawAmount);

7. File: contracts/vaults/yVault/yVault.sol (line 20)

   event Deposit(address indexed depositor, uint256 wantAmount);

8. File: contracts/vaults/yVault/yVault.sol (line 21)

   event Withdrawal(address indexed withdrawer, uint256 wantAmount);

9. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 25)

   event Harvested(uint256 wantEarned);

10. File: contracts/staking/JPEGStaking.sol (line 14)

    event Stake(address indexed user, uint256 amount);

11. File: contracts/staking/JPEGStaking.sol (line 15)

    event Unstake(address indexed user, uint256 amount);

12. File: contracts/farming/yVaultLPFarming.sol (line 18)

    event Deposit(address indexed user, uint256 amount);

13. File: contracts/farming/yVaultLPFarming.sol (line 19)

    event Withdraw(address indexed user, uint256 amount);

14. File: contracts/farming/yVaultLPFarming.sol (line 20)

    event Claim(address indexed user, uint256 rewards);

15. File: contracts/farming/LPFarming.sol (line 20)

    event Deposit(address indexed user, uint256 indexed pid, uint256 amount);

16. File: contracts/farming/LPFarming.sol (line 21)

    event Withdraw(address indexed user, uint256 indexed pid, uint256 amount);

17. File: contracts/farming/LPFarming.sol (line 22)

    event Claim(address indexed user, uint256 indexed pid, uint256 amount);

18. File: contracts/farming/LPFarming.sol (line 23)

    event ClaimAll(address indexed user, uint256 amount);

19. File: contracts/lock/JPEGLock.sol (line 14)

    event Lock(address indexed user, uint256 indexed nftIndex, uint256 amount);

20. File: contracts/lock/JPEGLock.sol (line 15)

    event Unlock(address indexed user, uint256 indexed nftIndex, uint256 amount);

The nonReentrant modifier should occur before all other modifiers

This is a best-practice to protect against reentrancy in other modifiers

1. File: contracts/vaults/NFTVault.sol (line 679)

   ) external validNFTIndex(_nftIndex) nonReentrant {

2. File: contracts/vaults/NFTVault.sol (line 759)

       nonReentrant

3. File: contracts/vaults/NFTVault.sol (line 834)

       nonReentrant

4. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 163)

   function borrow(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {

5. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 179)

   function repay(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {

6. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 193)

   function withdraw(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {

7. File: contracts/lock/JPEGLock.sol (line 53)

   ) external onlyOwner nonReentrant {

Typos

1. File: contracts/escrow/NFTEscrow.sol (line 30)

   /// This is an alternative to the classic "reservation" method, which requires users to call 3 functions in a specifc order (making the process non atomic)

   specifc

2. File: contracts/vaults/NFTVault.sol (line 117)

       //The standard OZ ERC721 implementation of ownerOf reverts on a non existing nft isntead of returning address(0)

   isntead

3. File: contracts/vaults/NFTVault.sol (line 625)

   /// @notice Returns data relative to a postition, existing or not

   postition

4. File: contracts/vaults/NFTVault.sol (line 673)

   /// @param _useInsurance Whereter to open an insured position. In case the position has already been opened previously,

   Whereter

5. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 50)

   /// instead of fetching it everytime to save gas

   everytime

6. File: contracts/vaults/yVault/Controller.sol (line 128)

   /// @notice Allows members of the `STRATEGIST_ROLE` to withdraw tokens stuck in this constract

   constract