QA Report

[code423n4]

Codebase Impressions & Summary

Overall, the code quality is high.

The findings here revolve around some commonly suggested practices.

[L-01] Add constructor initializers

As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”

Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”

Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:

contracts/helpers/CryptoPunksHelper.sol:
  19:     function initialize(address punksAddress) external initializer {

contracts/helpers/EtherRocksHelper.sol:
  19:     function initialize(address rocksAddress) external initializer {

contracts/staking/JPEGStaking.sol:
  21:     function initialize(IERC20Upgradeable _jpeg) external initializer {

contracts/vaults/FungibleAssetVaultForDAO.sol:
  66:     function initialize(

contracts/vaults/NFTVault.sol:
  139:     function initialize(

[L-02] Immutable addresses should be 0-checked

Consider adding an address(0) check here (see @audit):

contracts/farming/LPFarming.sol:
  77:         jpeg = IERC20(_jpeg); //@audit low: should be address(0) checked just like in yVaultLPFarming.sol and StrategyPUSDConvex.sol

contracts/vaults/yVault/Controller.sol:
  28:         jpeg = IERC20(_jpeg);  //@audit low: should be address(0) checked just like in yVaultLPFarming.sol and StrategyPUSDConvex.sol

contracts/vaults/yVault/yVault.sol:
  53:         token = ERC20(_token);  //@audit low: should be address(0)

[L-03] Unbounded loop on array can lead to DoS

As this array can grow quite large, the transaction's gas cost could exceed the block gas limit and make it impossible to call this function at all (see @audit):

File: LPFarming.sol
141:     function add(uint256 _allocPoint, IERC20 _lpToken) external onlyOwner { 
...
146:         poolInfo.push( //@audit low: a push exist but there's no pop in the solution.
...
154:     }
...
347:     function claimAll() external nonReentrant noContract(msg.sender) {
348:         for (uint256 i = 0; i < poolInfo.length; i++) { //@audit low: poolInfo is unbounded
349:             _updatePool(i);
350:             _withdrawReward(i);
351:         }
...
360:     }

Consider introducing a reasonable upper limit based on block gas limits and/or adding a remove method to remove elements in the array.

[L-04] Add a timelock and event to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate.

Consider adding a timelock and event to:

vaults/yVault/strategies/StrategyPUSDConvex.sol:177:    function setPerformanceFee(Rate memory _performanceFee)
vaults/NFTVault.sol:290:    function setOrganizationFeeRate(Rate memory _organizationFeeRate)

[L-05] Fee in StrategyPUSDConvex.setPerformanceFee() should be upper-bounded

See @audit:

File: StrategyPUSDConvex.sol
177:     function setPerformanceFee(Rate memory _performanceFee)
178:         public
179:         onlyRole(DEFAULT_ADMIN_ROLE)
180:     {
181:         require(
182:             _performanceFee.denominator > 0 &&
183:                 _performanceFee.denominator >= _performanceFee.numerator,
184:             "INVALID_RATE"
185:         );
186:         performanceFee = _performanceFee; //@audit low: fee can be 100% (_performanceFee.denominator == _performanceFee.numerator)
187:     }

[L-06] Fee in NFTVault.setOrganizationFeeRate()should be upper-bounded

See @audit:

File: NFTVault.sol
290:     function setOrganizationFeeRate(Rate memory _organizationFeeRate)  
291:         external
292:         onlyRole(DAO_ROLE)
293:     {
294:         _validateRate(_organizationFeeRate);
295:         settings.organizationFeeRate = _organizationFeeRate; //@audit low: fee can be 100%
296:     }
...
400:     function _validateRate(Rate memory rate) internal pure {
401:         require(
402:             rate.denominator > 0 && rate.denominator >= rate.numerator,  //@audit low: fee can be 100% (rate.denominator == rate.numerator)
403:             "invalid_rate"
404:         );
405:     }

[L-07] A magical number should be documented and explained: 1e36. Use a constant instead

farming/LPFarming.sol:196:                1e36 *
farming/LPFarming.sol:207:            1e36;
farming/LPFarming.sol:307:            1e36 *
farming/LPFarming.sol:319:            1e36;
farming/yVaultLPFarming.sol:94:            1e36;
farming/yVaultLPFarming.sol:172:        newAccRewardsPerShare = accRewardPerShare + newRewards * 1e36 / totalStaked;
farming/yVaultLPFarming.sol:179:            (accRewardPerShare - userLastAccRewardPerShare[account])) / 1e36;

I suggest using constant variables as this would make the code more maintainable and readable while costing nothing gas-wise.

[N-01] Avoid floating pragmas: the version should be locked (preferably at >= 0.8.4)

The pragma declared across the solution is ^0.8.0. As the compiler introduces a several interesting upgrades in Solidity 0.8.4, consider locking at this version or a more recent one.

[N-02] Related data should be grouped in a struct

The following maps should be grouped in structs.

From:

contracts/farming/yVaultLPFarming.sol:
  31:     mapping(address => uint256) public balanceOf; //@audit NC: related data 1
  32:     mapping(address => uint256) private userLastAccRewardPerShare;//@audit NC: related data 2
  33:     mapping(address => uint256) private userPendingRewards;//@audit NC: related data 3

contracts/vaults/yVault/Controller.sol:
  20:     mapping(IERC20 => address) public vaults; //@audit NC: related data 1
  21:     mapping(IERC20 => IStrategy) public strategies; //@audit NC: related data 2
  22:     mapping(IERC20 => mapping(IStrategy => bool)) public approvedStrategies; //@audit NC: related data 3

To

    struct UserInfo {
        uint256 balance;  
        uint256 lastAccRewardPerShare;
        uint256 pendingReward;
    }

    mapping(address => UserInfo) public userInfo;

And

    struct TokenInfo {
        address vaults;  
        IStrategy approvedStrategies;
        mapping(IStrategy => bool) pendingReward;
    }

    mapping(IERC20 => TokenInfo) public tokenInfo;

It would be less error-prone, more readable, and it would be possible to delete all related fields with a simple delete userInfo[address].

However, the sponsor should notice that pendingReward won't be as easily deleted in tokenInfo, as it's a mapping field. It would still improve code quality nonetheless.

[N-03] Unused named returns

Using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity (see @audit):

File: NFTEscrow.sol
081:     function precompute(address _owner, uint256 _idx)
082:         public
083:         view
084:         returns (bytes32 salt, address predictedAddress) //@audit NC: unused named returns
085:     {
...
091:         salt = sha256(abi.encodePacked(_owner));
...
105:         predictedAddress = address(uint160(uint256(hash)));
106:         return (salt, predictedAddress); //@audit NC: unused named returns
107:     }

[dmvt]

I agree with the warden's security ratings for the issues described.