JPEG'd contest - Dravee's results

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/package.json#L18-L19

Vulnerability details

Impact

The solution uses:

    "@openzeppelin/contracts": "^4.0.0",
    "@openzeppelin/contracts-upgradeable": "^4.3.2",

These dependencies have a known high severity vulnerability:

• https://security.snyk.io/vuln/SNYK-JS-OPENZEPPELINCONTRACTSUPGRADEABLE-2320177
• https://snyk.io/test/npm/@openzeppelin/contracts-upgradeable/4.3.2#SNYK-JS-OPENZEPPELINCONTRACTSUPGRADEABLE-2320177
• https://snyk.io/test/npm/@openzeppelin/contracts/4.0.0#SNYK-JS-OPENZEPPELINCONTRACTS-2320176

Which makes these contracts vulnerable:

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:
  71:     ) external initializer {

contracts/vaults/NFTVault.sol:
  149:     ) external initializer {

Recommended Mitigation Steps

Upgrade @openzeppelin/contracts and @openzeppelin/contracts-upgradeable to version 4.4.1 or higher.

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:     }

Table of Contents:

• NFTEscrow._executeTransfer(): Cheap Contract Deployment Through Clones
• LPFarming.newEpoch(): L128 and L133 should be unchecked due to parent if/else condition
• LPFarming.withdraw(): L248 should be unchecked due to L243
• LPFarming._withdrawReward(): poolInfo[_pid].accRewardPerShare should get cached
• yVaultLPFarming.withdraw(): L124 should be unchecked due to L119
• yVaultLPFarming._withdrawReward(): accRewardPerShare should get cached
• JPEGLock.unlock(): use storage instead of copying struct in memory L69
• FungibleAssetVaultForDAO._collateralPriceUsd(): oracle should get cached
• FungibleAssetVaultForDAO._collateralPriceUsd(): return statement should be unchecked
• FungibleAssetVaultForDAO.deposit(): collateralAsset should get cached
• FungibleAssetVaultForDAO.repay): L184 should be unchecked due to L182
• FungibleAssetVaultForDAO.withdraw(): L196 should be unchecked due to L194
• FungibleAssetVaultForDAO.withdraw(): collateralAmount should get cached
• NFTVault._normalizeAggregatorAnswer(): return statement should be unchecked
• NFTVault._calculateAdditionalInterest(): totalDebtAmount should get cached
• NFTVault.sol: struct PositionPreview can be tightly packed to save 1 storage slot
• NFTVault.showPosition(): L659 should be unchecked due to L649
• NFTVault.showPosition(): positions[_nftIndex].liquidatedAt should get cached
• NFTVault.showPosition(): Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it (positions[_nftIndex])
• NFTVault.borrow(): totalDebtPortion should get cached
• NFTVault.repay(): L781 should be unchecked due to ternary operator
• NFTVault.repay(): totalDebtPortion and totalDebtAmount should get cached
• Controller.setStrategy(): boolean comparison L87
• StrategyPUSDConvex.balanceOfJPEG(): jpeg should get cached
• StrategyPUSDConvex.balanceOfJPEG(): use a return statement instead of break
• StrategyPUSDConvex.withdraw(): L281 should be unchecked due to L279
• StrategyPUSDConvex.harvest(): L362 should be unchecked due to L359-L360 and how performanceFee is set L183
• yVault.earn(): token and controller should get cached
• yVault.withdraw(): L178 should be unchecked due to L177
• yVault.withdraw(): token should get cached
• yVault.withdrawJPEG(): farm should get cached
• Upgrade pragma to at least 0.8.4
• No need to explicitly initialize variables with default values
• > 0 is less efficient than != 0 for unsigned integers (with proof)
• >= is cheaper than >
• An array's length should be cached to save gas in for-loops
• ++i costs less gas compared to i++ or i += 1
• Increments can be unchecked
• Use calldata instead of memory
• Consider making some constants as non-public to save gas
• Public functions to external
• Reduce the size of error messages (Long revert Strings)
• Use Custom Errors instead of Revert Strings to save Gas

NFTEscrow._executeTransfer(): Cheap Contract Deployment Through Clones

See @audit tag:

67:     function _executeTransfer(address _owner, uint256 _idx) internal {
68:         (bytes32 salt, ) = precompute(_owner, _idx);
69:         new FlashEscrow{salt: salt}( //@audit gas: deployment can cost less through clones 
70:             nftAddress,
71:             _encodeFlashEscrowPayload(_idx)
72:         );
73:     }

There's a way to save a significant amount of gas on deployment using Clones: https://www.youtube.com/watch?v=3Mw-pMmJ7TA .

This is a solution that was adopted, as an example, by Porter Finance. They realized that deploying using clones was 10x cheaper:

• https://github.com/porter-finance/v1-core/issues/15#issuecomment-1035639516
• https://github.com/porter-finance/v1-core/pull/34

I suggest applying a similar pattern.

LPFarming.newEpoch(): L128 and L133 should be unchecked due to parent if/else condition

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

I suggest wrapping with an unchecked block here (see @audit tag):

107:     function newEpoch(
...
111:     ) external onlyOwner {
127:         if (remainingRewards > newRewards) {
128:             jpeg.safeTransfer(msg.sender, remainingRewards - newRewards);  //@audit gas: should be unchecked (can't underflow due to L127)
129:         } else if (remainingRewards < newRewards) {
130:             jpeg.safeTransferFrom(
131:                 msg.sender,
132:                 address(this),
133:                 newRewards - remainingRewards  //@audit gas: should be unchecked (can't underflow due to L129)
134:             );
135:         }
136:     }

LPFarming.withdraw(): L248 should be unchecked due to L243

See @audit tag:

235:     function withdraw(uint256 _pid, uint256 _amount)
236:         external
237:         noContract(msg.sender)
238:     {
239:         require(_amount > 0, "invalid_amount");
240: 
241:         PoolInfo storage pool = poolInfo[_pid];
242:         UserInfo storage user = userInfo[_pid][msg.sender];
243:         require(user.amount >= _amount, "insufficient_amount");
244: 
245:         _updatePool(_pid);
246:         _withdrawReward(_pid);
247: 
248:         user.amount -= _amount;  //@audit gas: should be unchecked (can't underflow due to L243)

LPFarming._withdrawReward(): poolInfo[_pid].accRewardPerShare should get cached

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). Here, the storage value should get cached in memory (see the @audit tags for further details):

315:     function _withdrawReward(uint256 _pid) internal returns (uint256) {
316:         UserInfo storage user = userInfo[_pid][msg.sender];
317:         uint256 pending = (user.amount *
318:             (poolInfo[_pid].accRewardPerShare - user.lastAccRewardPerShare)) / //@audit gas: SLOAD 1 poolInfo[_pid].accRewardPerShare
319:             1e36;
320:         if (pending > 0) {
321:             userRewards[msg.sender] += pending;
322:         }
323: 
324:         user.lastAccRewardPerShare = poolInfo[_pid].accRewardPerShare; //@audit gas: SLOAD 2 poolInfo[_pid].accRewardPerShare
325: 
326:         return pending;
327:     }

yVaultLPFarming.withdraw(): L124 should be unchecked due to L119

See @audit tag:

117:     function withdraw(uint256 _amount) external noContract(msg.sender) {
118:         require(_amount > 0, "invalid_amount");
119:         require(balanceOf[msg.sender] >= _amount, "insufficient_amount");
120: 
121:         _update();
122:         _withdrawReward(msg.sender);
123: 
124:         balanceOf[msg.sender] -= _amount;  //@audit gas: should be unchecked (can't underflow due to L119)

yVaultLPFarming._withdrawReward(): accRewardPerShare should get cached

See @audit tags:

177:     function _withdrawReward(address account) internal returns (uint256) {
178:         uint256 pending = (balanceOf[account] *
179:             (accRewardPerShare - userLastAccRewardPerShare[account])) / 1e36; //@audit gas: SLOAD 1 accRewardPerShare
180: 
181:         if (pending > 0) userPendingRewards[account] += pending;
182: 
183:         userLastAccRewardPerShare[account] = accRewardPerShare; //@audit gas: SLOAD 2 accRewardPerShare
184: 
185:         return pending;
186:     }

JPEGLock.unlock(): use storage instead of copying struct in memory L69

See @audit tag:

68:     function unlock(uint256 _nftIndex) external nonReentrant {
69:         LockPosition memory position = positions[_nftIndex]; //@audit gas: costing 3 SLOADs while only lockAmount is needed twice. Replace "memory" with "storage" and cache only position.lockAmount 
70:         require(position.owner == msg.sender, "unauthorized");
71:         require(position.unlockAt <= block.timestamp, "locked");
72: 
73:         delete positions[_nftIndex];
74: 
75:         jpeg.safeTransfer(msg.sender, position.lockAmount);
76: 
77:         emit Unlock(msg.sender, _nftIndex, position.lockAmount);
78:     }

Here, a copy in memory is costing 3 SLOADs and 3 MSTORES. The, 2 variables are only read once through MLOAD (position.owner and position.unlockAt) and one is read twice (position.lockAmount). I suggest replacing the memory keyword with storage at L69 and only copying position.lockAmount in memory.

FungibleAssetVaultForDAO._collateralPriceUsd(): oracle should get cached

See @audit tags:

104:     function _collateralPriceUsd() internal view returns (uint256) {
105:         int256 answer = oracle.latestAnswer();  //@audit gas: SLOAD 1 oracle
106:         uint8 decimals = oracle.decimals();  //@audit gas: SLOAD 2 oracle
107: 

FungibleAssetVaultForDAO._collateralPriceUsd(): return statement should be unchecked

See @audit tag:

104:     function _collateralPriceUsd() internal view returns (uint256) {
...
111:         return //@audit gas: whole return statement should be unchecked (obviously can't underflow/overflow here)
112:             decimals > 18
113:                 ? uint256(answer) / 10**(decimals - 18)  
114:                 : uint256(answer) * 10**(18 - decimals);  
115:     }

Due to the ternary condition and the fact that int256 answer = oracle.latestAnswer();, the return statement can't underflow and should be unchecked.

FungibleAssetVaultForDAO.deposit(): collateralAsset should get cached

See @audit tags:

141:     function deposit(uint256 amount) external payable onlyRole(WHITELISTED_ROLE) {
142:         require(amount > 0, "invalid_amount");
143: 
144:         if (collateralAsset == ETH) {  //@audit gas: SLOAD 1 collateralAsset
145:             require(msg.value == amount, "invalid_msg_value");
146:         } else {
147:             require(msg.value == 0, "non_zero_eth_value");
148:             IERC20Upgradeable(collateralAsset).safeTransferFrom(  //@audit gas: SLOAD 2 collateralAsset
149:                 msg.sender,
150:                 address(this),
151:                 amount
152:             );
153:         }

FungibleAssetVaultForDAO.repay): L184 should be unchecked due to L182

See @audit tag:

179:     function repay(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {
180:         require(amount > 0, "invalid_amount");
181: 
182:         amount = amount > debtAmount ? debtAmount : amount;
183: 
184:         debtAmount -= amount; //@audit gas: should be unchecked (can't underflow due to L182)

FungibleAssetVaultForDAO.withdraw(): L196 should be unchecked due to L194

See @audit tag:

193:     function withdraw(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {
194:         require(amount > 0 && amount <= collateralAmount, "invalid_amount");
195: 
196:         uint256 creditLimit = getCreditLimit(collateralAmount - amount); //@audit gas: should be unchecked (can't underflow due to L194)

FungibleAssetVaultForDAO.withdraw(): collateralAmount should get cached

See @audit tags:

193:     function withdraw(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {
194:         require(amount > 0 && amount <= collateralAmount, "invalid_amount");  //@audit gas: SLOAD 1 collateralAmount
195: 
196:         uint256 creditLimit = getCreditLimit(collateralAmount - amount); //@audit gas: SLOAD 2 collateralAmount
197:         require(creditLimit >= debtAmount, "insufficient_credit");
198: 
199:         collateralAmount -= amount; //@audit gas: SLOAD 3 collateralAmount (could've used the a cached value for calculation)

NFTVault._normalizeAggregatorAnswer(): return statement should be unchecked

See @audit tag:

454:     function _normalizeAggregatorAnswer(IAggregatorV3Interface aggregator)
455:         internal
456:         view
457:         returns (uint256)
458:     {
...
464:         return //@audit gas: whole return statement should be unchecked (obviously can't underflow/overflow)
465:             decimals > 18
466:                 ? uint256(answer) / 10**(decimals - 18)
467:                 : uint256(answer) * 10**(18 - decimals);
468:     }

NFTVault._calculateAdditionalInterest(): totalDebtAmount should get cached

See @audit tags:

578:     function _calculateAdditionalInterest() internal view returns (uint256) {
...
585:         if (totalDebtAmount == 0) {  //@audit gas: SLOAD 1 totalDebtAmount
586:             return 0;
587:         }
588: 
589:         // Accrue interest
590:         uint256 interestPerYear = (totalDebtAmount *  //@audit gas: SLOAD 2 totalDebtAmount

NFTVault.sol: struct PositionPreview can be tightly packed to save 1 storage slot

From (see @audit tags):

610:     struct PositionPreview { // @audit gas: can be tightly packed by moving borrowType and liquidatable at the end
611:         address owner;
612:         uint256 nftIndex;
613:         bytes32 nftType;
614:         uint256 nftValueUSD;
615:         VaultSettings vaultSettings;
616:         uint256 creditLimit;
617:         uint256 debtPrincipal;
618:         uint256 debtInterest; // @audit gas: 32 bytes
619:         BorrowType borrowType; // @audit gas: 1 byte (this enum is equivalent to uint8 as it has less than 256 options)
620:         bool liquidatable; // @audit gas: 1 byte
621:         uint256 liquidatedAt; // @audit gas: 32 bytes
622:         address liquidator; // @audit gas: 20 bytes
623:     }

To:

    struct PositionPreview {
        address owner;
        uint256 nftIndex;
        bytes32 nftType;
        uint256 nftValueUSD;
        VaultSettings vaultSettings;
        uint256 creditLimit;
        uint256 debtPrincipal;
        uint256 debtInterest; // @audit gas: 32 bytes
        uint256 liquidatedAt; // @audit gas: 32 bytes
        BorrowType borrowType; // @audit gas: 1 byte (this enum is equivalent to uint8 as it has less than 256 options)
        bool liquidatable; // @audit gas: 1 byte
        address liquidator; // @audit gas: 20 bytes
    }

NFTVault.showPosition(): L659 should be unchecked due to L649

See @audit tag:

File: NFTVault.sol
628:     function showPosition(uint256 _nftIndex)
...
649:         if (debtPrincipal > debtAmount) debtAmount = debtPrincipal;
...
659:             debtInterest: debtAmount - debtPrincipal, //@audit gas: should be unchecked (can't underflow due to L649)

NFTVault.showPosition(): positions[_nftIndex].liquidatedAt should get cached

See @audit tags:

File: NFTVault.sol
628:     function showPosition(uint256 _nftIndex)
...
661:             liquidatable: positions[_nftIndex].liquidatedAt == 0 &&  //@audit gas: SLOAD 1 positions[_nftIndex].liquidatedAt
662:                 debtAmount >= _getLiquidationLimit(_nftIndex),
663:             liquidatedAt: positions[_nftIndex].liquidatedAt,  //@audit gas: SLOAD 2 positions[_nftIndex].liquidatedAt

NFTVault.showPosition(): Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it (positions[_nftIndex])

To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.

The effect can be quite significant.

Here, instead of repeatedly calling positions[_nftIndex], save its reference like this: Position storage _position = positions[_nftIndex] and use it.

Impacted lines (see @audit tags):

  636:         uint256 debtPrincipal = positions[_nftIndex].debtPrincipal; //@audit gas: use the suggested storage variable "Position storage _position"
  637:         uint256 debtAmount = positions[_nftIndex].liquidatedAt > 0 //@audit gas: use the suggested storage variable "Position storage _position"
  638:             ? positions[_nftIndex].debtAmountForRepurchase //@audit gas: use the suggested storage variable "Position storage _position"
  641:                 positions[_nftIndex].debtPortion, //@audit gas: use the suggested storage variable "Position storage _position"
  660:             borrowType: positions[_nftIndex].borrowType, //@audit gas: use the suggested storage variable "Position storage _position"
  661:             liquidatable: positions[_nftIndex].liquidatedAt == 0 && //@audit gas: use the suggested storage variable "Position storage _position"
  663:             liquidatedAt: positions[_nftIndex].liquidatedAt, //@audit gas: use the suggested storage variable "Position storage _position"
  664:             liquidator: positions[_nftIndex].liquidator //@audit gas: use the suggested storage variable "Position storage _position"

This practice already exists in the solution, as seen in NFTVault.borrow():

675:     function borrow(
...
697:         Position storage position = positions[_nftIndex];

NFTVault.borrow(): totalDebtPortion should get cached

See @audit tags:

675:     function borrow(
...
735:         if (totalDebtPortion == 0) {  //@audit gas: SLOAD 1 totalDebtPortion
...
738:         } else {
739:             uint256 plusPortion = (totalDebtPortion * _amount) / //@audit gas: SLOAD 2 totalDebtPortion
740:                 totalDebtAmount;
741:             totalDebtPortion += plusPortion; //@audit gas: SLOAD 3 totalDebtPortion (could've used cached value for calculation)

NFTVault.repay(): L781 should be unchecked due to ternary operator

See @audit tag:

756:     function repay(uint256 _nftIndex, uint256 _amount)
...
780:         uint256 paidPrincipal = _amount > debtInterest
781:             ? _amount - debtInterest //@audit gas: should be unchecked (obviously)
782:             : 0;

NFTVault.repay(): totalDebtPortion and totalDebtAmount should get cached

See @audit tags:

756:     function repay(uint256 _nftIndex, uint256 _amount)
...
784:         uint256 minusPortion = paidPrincipal == debtPrincipal
785:             ? position.debtPortion
786:             : (totalDebtPortion * _amount) / totalDebtAmount; //@audit gas: SLOADs 1 totalDebtPortion & totalDebtAmount
787: 
788:         totalDebtPortion -= minusPortion; //@audit gas: SLOAD 2 totalDebtPortion (could've used cached value for calculation)
...
791:         totalDebtAmount -= _amount; //@audit gas: SLOAD 2 totalDebtAmount (could've used cached value for calculation)

Controller.setStrategy(): boolean comparison L87

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false) here (see @audit tag):

82:     function setStrategy(IERC20 _token, IStrategy _strategy)
83:         external
84:         onlyRole(STRATEGIST_ROLE)
85:     {
86:         require(
87:             approvedStrategies[_token][_strategy] == true, //@audit gas: instead of comparing to a constant, just use "approvedStrategies[_token][_strategy]"

StrategyPUSDConvex.balanceOfJPEG(): jpeg should get cached

See @audit tags:

226:     function balanceOfJPEG() external view returns (uint256) {
227:         uint256 availableBalance = jpeg.balanceOf(address(this)); //@audit gas: SLOAD 1 jpeg
...
231:         for (uint256 i = 0; i < length; i++) {
...
233:             if (address(jpeg) == extraReward.rewardToken()) { //@audit gas: SLOADs in Loop for jpeg. Cache it at L227

StrategyPUSDConvex.balanceOfJPEG(): use a return statement instead of break

See @audit tag:

226:     function balanceOfJPEG() external view returns (uint256) {
...
231:         for (uint256 i = 0; i < length; i++) {
...
233:             if (address(jpeg) == extraReward.rewardToken()) {
234:                 availableBalance += extraReward.earned();
235:                 //we found jpeg, no need to continue the loop
236:                 break; //@audit gas: instead of adding to availableBalance & breaking, just return here "availableBalance + extraReward.earned()"
237:             }
238:         }
239: 
240:         return availableBalance;
241:     }

Here, instead of making a memory operation with availableBalance += extraReward.earned(); and then using break; before returning the memory variable availableBalance, it would've been more optimized to just return availableBalance + extraReward.earned():

    function balanceOfJPEG() external view returns (uint256) {
...
        for (uint256 i = 0; i < length; i++) {
...
            if (address(jpeg) == extraReward.rewardToken()) {
              return availableBalance + extraReward.earned();
            }
        }
    }

StrategyPUSDConvex.withdraw(): L281 should be unchecked due to L279

See @audit tag:

273:     function withdraw(uint256 _amount) external onlyController {
...
279:         if (balance < _amount)
280:             convexConfig.baseRewardPool.withdrawAndUnwrap(
281:                 _amount - balance, //@audit gas: should be unchecked (can't underflow due to L279)
282:                 false
283:             );

StrategyPUSDConvex.harvest(): L362 should be unchecked due to L359-L360 and how performanceFee is set L183

See @audit tags:

177:     function setPerformanceFee(Rate memory _performanceFee)
...
181:         require(
182:             _performanceFee.denominator > 0 &&
183:                 _performanceFee.denominator >= _performanceFee.numerator, //@audit gas: can uncheck L362 thanks to this
184:             "INVALID_RATE"
185:         );
...
311:     function harvest(uint256 minOutCurve) external onlyRole(STRATEGIST_ROLE) {
...
359:         uint256 fee = (usdcBalance * performanceFee.numerator) /
360:             performanceFee.denominator;
361:         usdc.safeTransfer(strategy.controller.feeAddress(), fee);
362:         usdcBalance -= fee; //@audit gas: should be unchecked (can't underflow due to L359-L360 & how performanceFee is set L183)

yVault.earn(): token and controller should get cached

See @audit tags:

129:     function earn() external {
130:         uint256 _bal = available();
131:         token.safeTransfer(address(controller), _bal); //@audit gas: SLOADs 1 token & controller
132:         controller.earn(address(token), _bal); //@audit gas: SLOADs 2 token & controller
133:     }

yVault.withdraw(): L178 should be unchecked due to L177

See @audit tag:

166:     function withdraw(uint256 _shares) public noContract(msg.sender) {
...
177:         if (vaultBalance < backingTokens) {
178:             uint256 toWithdraw = backingTokens - vaultBalance; //@audit gas: should be unchecked (can't underflow due to L177)

yVault.withdraw(): token should get cached

See @audit tags:

166:     function withdraw(uint256 _shares) public noContract(msg.sender) {
...
176:         uint256 vaultBalance = token.balanceOf(address(this)); //@audit gas: SLOAD 1 token
177:         if (vaultBalance < backingTokens) {
178:             uint256 toWithdraw = backingTokens - vaultBalance;
179:             controller.withdraw(address(token), toWithdraw);  //@audit gas: SLOAD 2 token
180:         }
181: 
182:         token.safeTransfer(msg.sender, backingTokens);  //@audit gas: SLOAD 3 token

yVault.withdrawJPEG(): farm should get cached

See @audit tags:

187:     function withdrawJPEG() external {
188:         require(farm != address(0), "NO_FARM");  //@audit gas: SLOAD 1 farm
189:         controller.withdrawJPEG(address(token), farm);  //@audit gas: SLOAD 2 farm
190:     }

Upgrade pragma to at least 0.8.4

Across the whole solution, the declared pragma is ^0.8.0.

Using newer compiler versions and the optimizer give gas optimizations. Also, additional safety checks are available for free.

The advantages here are:

• Low level inliner (>= 0.8.2): Cheaper runtime gas (especially relevant when the contract has small functions).
• Optimizer improvements in packed structs (>= 0.8.3)
• Custom errors (>= 0.8.4): cheaper deployment cost and runtime cost. Note: the runtime cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Consider upgrading pragma to at least 0.8.4:

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

farming/LPFarming.sol:281:        for (uint256 pid = 0; pid < length; ++pid) {
farming/LPFarming.sol:348:        for (uint256 i = 0; i < poolInfo.length; i++) {
vaults/yVault/strategies/StrategyPUSDConvex.sol:145:        for (uint256 i = 0; i < _strategyConfig.rewardTokens.length; i++) {
vaults/yVault/strategies/StrategyPUSDConvex.sol:231:        for (uint256 i = 0; i < length; i++) {
vaults/yVault/strategies/StrategyPUSDConvex.sol:319:            for (uint256 i = 0; i < rewardTokens.length; i++) {
vaults/FungibleAssetVaultForDAO.sol:45:    address internal constant ETH = address(0);
vaults/NFTVault.sol:181:        for (uint256 i = 0; i < _typeInitializers.length; i++) {
vaults/NFTVault.sol:184:            for (uint256 j = 0; j < initializer.nfts.length; j++) {

I suggest removing explicit initializations for default values.

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

farming/LPFarming.sol:114:        require(_rewardPerBlock > 0, "Invalid reward per block");
farming/LPFarming.sol:218:        require(_amount > 0, "invalid_amount");
farming/LPFarming.sol:239:        require(_amount > 0, "invalid_amount");
farming/LPFarming.sol:337:        require(rewards > 0, "no_reward");
farming/LPFarming.sol:354:        require(rewards > 0, "no_reward");
farming/yVaultLPFarming.sol:101:        require(_amount > 0, "invalid_amount");
farming/yVaultLPFarming.sol:118:        require(_amount > 0, "invalid_amount");
farming/yVaultLPFarming.sol:139:        require(rewards > 0, "no_reward");
lock/JPEGLock.sol:40:        require(_newTime > 0, "Invalid lock time");
staking/JPEGStaking.sol:32:        require(_amount > 0, "invalid_amount");
vaults/yVault/strategies/StrategyPUSDConvex.sol:182:            _performanceFee.denominator > 0 &&
vaults/yVault/strategies/StrategyPUSDConvex.sol:334:            require(wethBalance > 0, "NOOP");
vaults/yVault/yVault.sol:100:            _rate.numerator > 0 && _rate.denominator >= _rate.numerator,
vaults/yVault/yVault.sol:143:        require(_amount > 0, "INVALID_AMOUNT");
vaults/yVault/yVault.sol:167:        require(_shares > 0, "INVALID_AMOUNT");
vaults/yVault/yVault.sol:170:        require(supply > 0, "NO_TOKENS_DEPOSITED");
vaults/FungibleAssetVaultForDAO.sol:94:            _creditLimitRate.denominator > 0 &&
vaults/FungibleAssetVaultForDAO.sol:108:        require(answer > 0, "invalid_oracle_answer");
vaults/FungibleAssetVaultForDAO.sol:142:        require(amount > 0, "invalid_amount");
vaults/FungibleAssetVaultForDAO.sol:164:        require(amount > 0, "invalid_amount");
vaults/FungibleAssetVaultForDAO.sol:180:        require(amount > 0, "invalid_amount");
vaults/FungibleAssetVaultForDAO.sol:194:        require(amount > 0 && amount <= collateralAmount, "invalid_amount");
vaults/NFTVault.sol:278:        require(_newFloor > 0, "Invalid floor");
vaults/NFTVault.sol:327:            _type == bytes32(0) || nftTypeValueETH[_type] > 0,
vaults/NFTVault.sol:365:        require(pendingValue > 0, "no_pending_value");
vaults/NFTVault.sol:402:            rate.denominator > 0 && rate.denominator >= rate.numerator,
vaults/NFTVault.sol:462:        require(answer > 0, "invalid_oracle_answer");
vaults/NFTVault.sol:687:        require(_amount > 0, "invalid_amount");
vaults/NFTVault.sol:764:        require(_amount > 0, "invalid_amount");
vaults/NFTVault.sol:770:        require(debtAmount > 0, "position_not_borrowed");
vaults/NFTVault.sol:882:        require(position.liquidatedAt > 0, "not_liquidated");
vaults/NFTVault.sol:926:        require(position.liquidatedAt > 0, "not_liquidated");

Also, please enable the Optimizer.

>= is cheaper than >

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas)

I suggest using >= instead of > to avoid some opcodes here:

vaults/NFTVault.sol:539:        return principal > calculatedDebt ? principal : calculatedDebt;
vaults/NFTVault.sol:775:        _amount = _amount > debtAmount ? debtAmount : _amount;

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

farming/LPFarming.sol:348:        for (uint256 i = 0; i < poolInfo.length; i++) {
vaults/yVault/strategies/StrategyPUSDConvex.sol:145:        for (uint256 i = 0; i < _strategyConfig.rewardTokens.length; i++) {
vaults/yVault/strategies/StrategyPUSDConvex.sol:319:            for (uint256 i = 0; i < rewardTokens.length; i++) {
vaults/NFTVault.sol:181:        for (uint256 i = 0; i < _typeInitializers.length; i++) {
vaults/NFTVault.sol:184:            for (uint256 j = 0; j < initializer.nfts.length; j++) {

This is already done here:

farming/LPFarming.sol:281:        for (uint256 pid = 0; pid < length; ++pid) {
vaults/yVault/strategies/StrategyPUSDConvex.sol:231:        for (uint256 i = 0; i < length; i++) {

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

farming/LPFarming.sol:348:        for (uint256 i = 0; i < poolInfo.length; i++) {
vaults/yVault/strategies/StrategyPUSDConvex.sol:145:        for (uint256 i = 0; i < _strategyConfig.rewardTokens.length; i++) {
vaults/yVault/strategies/StrategyPUSDConvex.sol:231:        for (uint256 i = 0; i < length; i++) {
vaults/yVault/strategies/StrategyPUSDConvex.sol:319:            for (uint256 i = 0; i < rewardTokens.length; i++) {
vaults/NFTVault.sol:181:        for (uint256 i = 0; i < _typeInitializers.length; i++) {
vaults/NFTVault.sol:184:            for (uint256 j = 0; j < initializer.nfts.length; j++) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

This is already done here:

farming/LPFarming.sol:281:        for (uint256 pid = 0; pid < length; ++pid) {

Increments can be unchecked

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Instances include:

farming/LPFarming.sol:281:        for (uint256 pid = 0; pid < length; ++pid) {
farming/LPFarming.sol:348:        for (uint256 i = 0; i < poolInfo.length; i++) {
vaults/yVault/strategies/StrategyPUSDConvex.sol:145:        for (uint256 i = 0; i < _strategyConfig.rewardTokens.length; i++) {
vaults/yVault/strategies/StrategyPUSDConvex.sol:231:        for (uint256 i = 0; i < length; i++) {
vaults/yVault/strategies/StrategyPUSDConvex.sol:319:            for (uint256 i = 0; i < rewardTokens.length; i++) {
vaults/NFTVault.sol:181:        for (uint256 i = 0; i < _typeInitializers.length; i++) {
vaults/NFTVault.sol:184:            for (uint256 j = 0; j < initializer.nfts.length; j++) {

The code would go from:

for (uint256 i; i < numIterations; i++) {  
 // ...  
}  

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

The risk of overflow is inexistant for a uint256 here.

Use calldata instead of memory

When arguments are read-only on external functions, the data location should be calldata:

contracts/vaults/NFTVault.sol:
  212:     function setDebtInterestApr(Rate memory _debtInterestApr) //@audit gas: should be calldata
  222:     function setValueIncreaseLockRate(Rate memory _valueIncreaseLockRate)  //@audit gas: should be calldata
  232:     function setCreditLimitRate(Rate memory _creditLimitRate)  //@audit gas: should be calldata
  247:     function setLiquidationLimitRate(Rate memory _liquidationLimitRate)  //@audit gas: should be calldata
  290:     function setOrganizationFeeRate(Rate memory _organizationFeeRate)  //@audit gas: should be calldata
  300:     function setInsurancePurchaseRate(Rate memory _insurancePurchaseRate)  //@audit gas: should be calldata
  311:         Rate memory _insuranceLiquidationPenaltyRate  //@audit gas: should be calldata

Consider making some constants as non-public to save gas

Reducing from public to private or internal can save gas when a constant isn't used outside of its contract. I suggest changing the visibility from public to internal or private here:

tokens/JPEG.sol:10:    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
tokens/StableCoin.sol:22:    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
tokens/StableCoin.sol:23:    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
vaults/yVault/strategies/StrategyPUSDConvex.sol:66:    bytes32 public constant STRATEGIST_ROLE = keccak256("STRATEGIST_ROLE");
vaults/yVault/Controller.sol:15:    bytes32 public constant STRATEGIST_ROLE = keccak256("STRATEGIST_ROLE");
vaults/FungibleAssetVaultForDAO.sol:41:    bytes32 public constant WHITELISTED_ROLE = keccak256("WHITELISTED_ROLE");
vaults/NFTVault.sol:71:    bytes32 public constant DAO_ROLE = keccak256("DAO_ROLE");
vaults/NFTVault.sol:72:    bytes32 public constant LIQUIDATOR_ROLE = keccak256("LIQUIDATOR_ROLE");
vaults/NFTVault.sol:74:    bytes32 public constant CUSTOM_NFT_HASH = keccak256("CUSTOM");

Public functions to external

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

withdraw(IERC20,uint256) should be declared external:
 - Controller.withdraw(IERC20,uint256) (contracts/vaults/yVault/Controller.sol#151-154)
setFarmingPool(address) should be declared external:
 - YVault.setFarmingPool(address) (contracts/vaults/yVault/yVault.sol#115-118)

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

tokens/JPEG.sol:23:            "JPEG: must have minter role to mint"
tokens/StableCoin.sol:41:            "StableCoin: must have minter role to mint"
tokens/StableCoin.sol:55:            "StableCoin: must have pauser role to pause"
tokens/StableCoin.sol:69:            "StableCoin: must have pauser role to unpause"
vaults/NFTVault.sol:394:            "credit_rate_exceeds_or_equals_liquidation_rate" 

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors as described next.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

escrow/NFTEscrow.sol:18:        require(success, "FlashEscrow: call_failed");
escrow/NFTEscrow.sol:86:        require(
farming/LPFarming.sol:86:        require(
farming/LPFarming.sol:112:        require(_startBlock >= block.number, "Invalid start block");
farming/LPFarming.sol:113:        require(_endBlock > _startBlock, "Invalid end block");
farming/LPFarming.sol:114:        require(_rewardPerBlock > 0, "Invalid reward per block");
farming/LPFarming.sol:218:        require(_amount > 0, "invalid_amount");
farming/LPFarming.sol:239:        require(_amount > 0, "invalid_amount");
farming/LPFarming.sol:243:        require(user.amount >= _amount, "insufficient_amount");
farming/LPFarming.sol:337:        require(rewards > 0, "no_reward");
farming/LPFarming.sol:354:        require(rewards > 0, "no_reward");
farming/yVaultLPFarming.sol:42:        require(_vault != address(0), "INVALID_VAULT");
farming/yVaultLPFarming.sol:43:        require(_jpeg != address(0), "INVALID_JPEG");
farming/yVaultLPFarming.sol:55:        require(
farming/yVaultLPFarming.sol:101:        require(_amount > 0, "invalid_amount");
farming/yVaultLPFarming.sol:118:        require(_amount > 0, "invalid_amount");
farming/yVaultLPFarming.sol:119:        require(balanceOf[msg.sender] >= _amount, "insufficient_amount");
farming/yVaultLPFarming.sol:139:        require(rewards > 0, "no_reward");
helpers/CryptoPunksHelper.sol:79:        require(
helpers/EtherRocksHelper.sol:81:        require(
lock/JPEGLock.sol:40:        require(_newTime > 0, "Invalid lock time");
lock/JPEGLock.sol:70:        require(position.owner == msg.sender, "unauthorized");
lock/JPEGLock.sol:71:        require(position.unlockAt <= block.timestamp, "locked");
staking/JPEGStaking.sol:32:        require(_amount > 0, "invalid_amount");
staking/JPEGStaking.sol:45:        require(
tokens/JPEG.sol:21:        require(
tokens/StableCoin.sol:39:        require(
tokens/StableCoin.sol:53:        require(
tokens/StableCoin.sol:67:        require(
vaults/yVault/strategies/StrategyPUSDConvex.sol:108:        require(_want != address(0), "INVALID_WANT");
vaults/yVault/strategies/StrategyPUSDConvex.sol:109:        require(_jpeg != address(0), "INVALID_JPEG");
vaults/yVault/strategies/StrategyPUSDConvex.sol:110:        require(_pusd != address(0), "INVALID_PUSD");
vaults/yVault/strategies/StrategyPUSDConvex.sol:111:        require(_weth != address(0), "INVALID_WETH");
vaults/yVault/strategies/StrategyPUSDConvex.sol:112:        require(_usdc != address(0), "INVALID_USDC");
vaults/yVault/strategies/StrategyPUSDConvex.sol:113:        require(
vaults/yVault/strategies/StrategyPUSDConvex.sol:117:        require(
vaults/yVault/strategies/StrategyPUSDConvex.sol:121:        require(address(_curveConfig.curve) != address(0), "INVALID_CURVE");
vaults/yVault/strategies/StrategyPUSDConvex.sol:122:        require(
vaults/yVault/strategies/StrategyPUSDConvex.sol:126:        require(_curveConfig.pusdIndex < 4, "INVALID_PUSD_CURVE_INDEX");
vaults/yVault/strategies/StrategyPUSDConvex.sol:127:        require(_curveConfig.usdcIndex < 4, "INVALID_USDC_CURVE_INDEX");
vaults/yVault/strategies/StrategyPUSDConvex.sol:128:        require(
vaults/yVault/strategies/StrategyPUSDConvex.sol:132:        require(
vaults/yVault/strategies/StrategyPUSDConvex.sol:136:        require(
vaults/yVault/strategies/StrategyPUSDConvex.sol:140:        require(
vaults/yVault/strategies/StrategyPUSDConvex.sol:146:            require(
vaults/yVault/strategies/StrategyPUSDConvex.sol:168:        require(
vaults/yVault/strategies/StrategyPUSDConvex.sol:181:        require(
vaults/yVault/strategies/StrategyPUSDConvex.sol:195:        require(_controller != address(0), "INVALID_CONTROLLER");
vaults/yVault/strategies/StrategyPUSDConvex.sol:205:        require(_vault != address(0), "INVALID_USDC_VAULT");
vaults/yVault/strategies/StrategyPUSDConvex.sol:262:        require(want != _asset, "want");
vaults/yVault/strategies/StrategyPUSDConvex.sol:263:        require(pusd != _asset, "pusd");
vaults/yVault/strategies/StrategyPUSDConvex.sol:264:        require(usdc != _asset, "usdc");
vaults/yVault/strategies/StrategyPUSDConvex.sol:265:        require(weth != _asset, "weth");
vaults/yVault/strategies/StrategyPUSDConvex.sol:266:        require(jpeg != _asset, "jpeg");
vaults/yVault/strategies/StrategyPUSDConvex.sol:275:        require(vault != address(0), "ZERO_VAULT"); // additional protection so we don't burn the funds
vaults/yVault/strategies/StrategyPUSDConvex.sol:292:        require(vault != address(0), "ZERO_VAULT"); // additional protection so we don't burn the funds
vaults/yVault/strategies/StrategyPUSDConvex.sol:334:            require(wethBalance > 0, "NOOP");
vaults/yVault/Controller.sol:37:        require(_feeAddress != address(0), "INVALID_FEE_ADDRESS");
vaults/yVault/Controller.sol:48:        require(vaults[_token] == address(0), "ALREADY_HAS_VAULT");
vaults/yVault/Controller.sol:49:        require(_vault != address(0), "INVALID_VAULT");
vaults/yVault/Controller.sol:60:        require(address(_token) != address(0), "INVALID_TOKEN");
vaults/yVault/Controller.sol:61:        require(address(_strategy) != address(0), "INVALID_STRATEGY");
vaults/yVault/Controller.sol:73:        require(address(_token) != address(0), "INVALID_TOKEN");
vaults/yVault/Controller.sol:74:        require(address(_strategy) != address(0), "INVALID_STRATEGY");
vaults/yVault/Controller.sol:86:        require(
vaults/yVault/Controller.sol:152:        require(msg.sender == vaults[_token], "NOT_VAULT");
vaults/yVault/Controller.sol:164:        require(msg.sender == vaults[_token], "NOT_VAULT");
vaults/yVault/yVault.sol:62:        require(
vaults/yVault/yVault.sol:99:        require(
vaults/yVault/yVault.sol:109:        require(_controller != address(0), "INVALID_CONTROLLER");
vaults/yVault/yVault.sol:116:        require(_farm != address(0), "INVALID_FARMING_POOL");
vaults/yVault/yVault.sol:143:        require(_amount > 0, "INVALID_AMOUNT");
vaults/yVault/yVault.sol:167:        require(_shares > 0, "INVALID_AMOUNT");
vaults/yVault/yVault.sol:170:        require(supply > 0, "NO_TOKENS_DEPOSITED");
vaults/yVault/yVault.sol:188:        require(farm != address(0), "NO_FARM");
vaults/FungibleAssetVaultForDAO.sol:93:        require(
vaults/FungibleAssetVaultForDAO.sol:108:        require(answer > 0, "invalid_oracle_answer");
vaults/FungibleAssetVaultForDAO.sol:142:        require(amount > 0, "invalid_amount");
vaults/FungibleAssetVaultForDAO.sol:145:            require(msg.value == amount, "invalid_msg_value");
vaults/FungibleAssetVaultForDAO.sol:147:            require(msg.value == 0, "non_zero_eth_value");
vaults/FungibleAssetVaultForDAO.sol:164:        require(amount > 0, "invalid_amount");
vaults/FungibleAssetVaultForDAO.sol:168:        require(newDebtAmount <= creditLimit, "insufficient_credit");
vaults/FungibleAssetVaultForDAO.sol:180:        require(amount > 0, "invalid_amount");
vaults/FungibleAssetVaultForDAO.sol:194:        require(amount > 0 && amount <= collateralAmount, "invalid_amount");
vaults/FungibleAssetVaultForDAO.sol:197:        require(creditLimit >= debtAmount, "insufficient_credit");
vaults/NFTVault.sol:118:        require(nftContract.ownerOf(nftIndex) != address(0), "invalid_nft");
vaults/NFTVault.sol:278:        require(_newFloor > 0, "Invalid floor");
vaults/NFTVault.sol:326:        require(
vaults/NFTVault.sol:365:        require(pendingValue > 0, "no_pending_value");
vaults/NFTVault.sol:391:        require(
vaults/NFTVault.sol:401:        require(
vaults/NFTVault.sol:462:        require(answer > 0, "invalid_oracle_answer");
vaults/NFTVault.sol:682:        require(
vaults/NFTVault.sol:687:        require(_amount > 0, "invalid_amount");
vaults/NFTVault.sol:688:        require(
vaults/NFTVault.sol:698:        require(position.liquidatedAt == 0, "liquidated");
vaults/NFTVault.sol:699:        require(
vaults/NFTVault.sol:710:        require(debtAmount + _amount <= creditLimit, "insufficient_credit");
vaults/NFTVault.sol:763:        require(msg.sender == positionOwner[_nftIndex], "unauthorized");
vaults/NFTVault.sol:764:        require(_amount > 0, "invalid_amount");
vaults/NFTVault.sol:767:        require(position.liquidatedAt == 0, "liquidated");
vaults/NFTVault.sol:770:        require(debtAmount > 0, "position_not_borrowed");
vaults/NFTVault.sol:805:        require(msg.sender == positionOwner[_nftIndex], "unauthorized");
vaults/NFTVault.sol:806:        require(_getDebtAmount(_nftIndex) == 0, "position_not_repaid");
vaults/NFTVault.sol:839:        require(posOwner != address(0), "position_not_exist");
vaults/NFTVault.sol:842:        require(position.liquidatedAt == 0, "liquidated");
vaults/NFTVault.sol:845:        require(
vaults/NFTVault.sol:881:        require(msg.sender == positionOwner[_nftIndex], "unauthorized");
vaults/NFTVault.sol:882:        require(position.liquidatedAt > 0, "not_liquidated");
vaults/NFTVault.sol:883:        require(
vaults/NFTVault.sol:887:        require(
vaults/NFTVault.sol:925:        require(address(0) != owner, "no_position");
vaults/NFTVault.sol:926:        require(position.liquidatedAt > 0, "not_liquidated");
vaults/NFTVault.sol:927:        require(
vaults/NFTVault.sol:932:        require(position.liquidator == msg.sender, "unauthorized");

I suggest replacing revert strings with custom errors.