code-423n4 / 2022-04-jpegd-findings

1 stars 1 forks source link

Gas Optimizations #207

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Not using the named return variables when a function returns, wastes deployment gas

  1. File: contracts/escrow/NFTEscrow.sol (line 106)
        return (salt, predictedAddress);
  2. File: contracts/escrow/NFTEscrow.sol (line 106)
        return (salt, predictedAddress);

require()/revert() strings longer than 32 bytes cost extra gas

  1. File: contracts/vaults/NFTVault.sol (lines 391-395)
        require(
            _liquidationLimitRate.numerator * _creditLimitRate.denominator >
                _creditLimitRate.numerator * _liquidationLimitRate.denominator,
            "credit_rate_exceeds_or_equals_liquidation_rate"
        );
  2. File: contracts/tokens/StableCoin.sol (lines 39-42)
        require(
            hasRole(MINTER_ROLE, _msgSender()),
            "StableCoin: must have minter role to mint"
        );
  3. File: contracts/tokens/StableCoin.sol (lines 53-56)
        require(
            hasRole(PAUSER_ROLE, _msgSender()),
            "StableCoin: must have pauser role to pause"
        );
  4. File: contracts/tokens/StableCoin.sol (lines 67-70)
        require(
            hasRole(PAUSER_ROLE, _msgSender()),
            "StableCoin: must have pauser role to unpause"
        );
  5. File: contracts/tokens/JPEG.sol (lines 21-24)
        require(
            hasRole(MINTER_ROLE, _msgSender()),
            "JPEG: must have minter role to mint"
        );

Use a more recent version of solidity

Use a solidity version of at least 0.8.2 to get compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

  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;

Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27

  1. File: contracts/vaults/NFTVault.sol (line 91)
    bool public daoFloorOverride;
  2. File: contracts/vaults/NFTVault.sol (line 93)
    bool public useFallbackOracle;
  3. File: contracts/vaults/yVault/Controller.sol (line 22)
    mapping(IERC20 => mapping(IStrategy => bool)) public approvedStrategies;
  4. File: contracts/vaults/yVault/yVault.sol (line 35)
    mapping(address => bool) public whitelistedContracts;
  5. File: contracts/farming/yVaultLPFarming.sol (line 37)
    mapping(address => bool) public whitelistedContracts;
  6. File: contracts/farming/LPFarming.sol (line 73)
    mapping(address => bool) public whitelistedContracts;

Use delete rather than assigning zero to a mapping to get a gas refund

  1. File: contracts/vaults/NFTVault.sol (line 380)
        pendingNFTValueETH[_nftIndex] = 0;
  2. File: contracts/farming/yVaultLPFarming.sol (line 141)
        userPendingRewards[msg.sender] = 0;
  3. File: contracts/farming/LPFarming.sol (line 340)
        userRewards[msg.sender] = 0;
  4. File: contracts/farming/LPFarming.sol (line 357)
        userRewards[msg.sender] = 0;

Using > 0 costs more gas than != 0 when used on a uint in a require() statement

  1. File: contracts/vaults/NFTVault.sol (line 278)
        require(_newFloor > 0, "Invalid floor");
  2. File: contracts/vaults/NFTVault.sol (line 365)
        require(pendingValue > 0, "no_pending_value");
  3. File: contracts/vaults/NFTVault.sol (line 687)
        require(_amount > 0, "invalid_amount");
  4. File: contracts/vaults/NFTVault.sol (line 764)
        require(_amount > 0, "invalid_amount");
  5. File: contracts/vaults/NFTVault.sol (line 770)
        require(debtAmount > 0, "position_not_borrowed");
  6. File: contracts/vaults/NFTVault.sol (line 882)
        require(position.liquidatedAt > 0, "not_liquidated");
  7. File: contracts/vaults/NFTVault.sol (line 926)
        require(position.liquidatedAt > 0, "not_liquidated");
  8. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 142)
        require(amount > 0, "invalid_amount");
  9. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 164)
        require(amount > 0, "invalid_amount");
  10. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 180)
        require(amount > 0, "invalid_amount");
  11. File: contracts/vaults/yVault/yVault.sol (line 143)
        require(_amount > 0, "INVALID_AMOUNT");
  12. File: contracts/vaults/yVault/yVault.sol (line 167)
        require(_shares > 0, "INVALID_AMOUNT");
  13. File: contracts/vaults/yVault/yVault.sol (line 170)
        require(supply > 0, "NO_TOKENS_DEPOSITED");
  14. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 334)
            require(wethBalance > 0, "NOOP");
  15. File: contracts/staking/JPEGStaking.sol (line 32)
        require(_amount > 0, "invalid_amount");
  16. File: contracts/farming/yVaultLPFarming.sol (line 101)
        require(_amount > 0, "invalid_amount");
  17. File: contracts/farming/yVaultLPFarming.sol (line 118)
        require(_amount > 0, "invalid_amount");
  18. File: contracts/farming/yVaultLPFarming.sol (line 139)
        require(rewards > 0, "no_reward");
  19. File: contracts/farming/LPFarming.sol (line 114)
        require(_rewardPerBlock > 0, "Invalid reward per block");
  20. File: contracts/farming/LPFarming.sol (line 218)
        require(_amount > 0, "invalid_amount");
  21. File: contracts/farming/LPFarming.sol (line 239)
        require(_amount > 0, "invalid_amount");
  22. File: contracts/farming/LPFarming.sol (line 337)
        require(rewards > 0, "no_reward");
  23. File: contracts/farming/LPFarming.sol (line 354)
        require(rewards > 0, "no_reward");
  24. File: contracts/lock/JPEGLock.sol (line 40)
        require(_newTime > 0, "Invalid lock time");

<array>.length should not be looked up in every loop of a for-loop

Even memory arrays incur the overhead of bit tests and bit shifts to calculate the array length

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

It costs more gas to initialize variables to zero than to let the default of zero be applied

  1. File: contracts/vaults/NFTVault.sol (line 181)
        for (uint256 i = 0; i < _typeInitializers.length; i++) {
  2. File: contracts/vaults/NFTVault.sol (line 184)
            for (uint256 j = 0; j < initializer.nfts.length; j++) {
  3. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 145)
        for (uint256 i = 0; i < _strategyConfig.rewardTokens.length; i++) {
  4. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 231)
        for (uint256 i = 0; i < length; i++) {
  5. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 319)
            for (uint256 i = 0; i < rewardTokens.length; i++) {
  6. File: contracts/farming/LPFarming.sol (line 281)
        for (uint256 pid = 0; pid < length; ++pid) {
  7. File: contracts/farming/LPFarming.sol (line 348)
        for (uint256 i = 0; i < poolInfo.length; i++) {

State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second access of a state variable within a function. Less obvious optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, or having local caches of state variable contracts/addresses.

  1. File: contracts/vaults/NFTVault.sol (line 814)
            nftContract.safeTransferFrom(address(this), msg.sender, _nftIndex);
  2. File: contracts/vaults/NFTVault.sol (line 590)
        uint256 interestPerYear = (totalDebtAmount *
  3. File: contracts/vaults/NFTVault.sol (line 740)
                totalDebtAmount;
  4. File: contracts/vaults/NFTVault.sol (line 739)
            uint256 plusPortion = (totalDebtPortion * _amount) /
  5. File: contracts/vaults/NFTVault.sol (line 242)
        settings.creditLimitRate = _creditLimitRate;
  6. File: contracts/vaults/NFTVault.sol (line 257)
        settings.liquidationLimitRate = _liquidationLimitRate;
  7. File: contracts/vaults/NFTVault.sol (line 369)
            settings.creditLimitRate.denominator) *
  8. File: contracts/vaults/NFTVault.sol (line 506)
            settings.creditLimitRate.denominator;
  9. File: contracts/vaults/NFTVault.sol (line 520)
            settings.liquidationLimitRate.denominator;
  10. File: contracts/vaults/NFTVault.sol (line 592)
            settings.debtInterestApr.denominator;
  11. File: contracts/vaults/NFTVault.sol (line 714)
            settings.organizationFeeRate.numerator) /
  12. File: contracts/vaults/NFTVault.sol (line 895)
            settings.insuranceLiquidationPenaltyRate.numerator) /
  13. File: contracts/vaults/NFTVault.sol (line 533)
        uint256 principal = positions[_nftIndex].debtPrincipal;
  14. File: contracts/vaults/NFTVault.sol (line 637)
        uint256 debtAmount = positions[_nftIndex].liquidatedAt > 0
  15. File: contracts/vaults/NFTVault.sol (line 684)
                address(0) == positionOwner[_nftIndex],
  16. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 148)
            IERC20Upgradeable(collateralAsset).safeTransferFrom(
  17. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 203)
            IERC20Upgradeable(collateralAsset).safeTransfer(msg.sender, amount);
  18. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 106)
        uint8 decimals = oracle.decimals();
  19. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 135)
            creditLimitRate.denominator;
  20. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 196)
        uint256 creditLimit = getCreditLimit(collateralAmount - amount);
  21. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 182)
        amount = amount > debtAmount ? debtAmount : amount;
  22. File: contracts/vaults/yVault/yVault.sol (line 78)
            controller.balanceOf(address(token));
  23. File: contracts/vaults/yVault/yVault.sol (line 132)
        controller.earn(address(token), _bal);
  24. File: contracts/vaults/yVault/yVault.sol (line 179)
            controller.withdraw(address(token), toWithdraw);
  25. File: contracts/vaults/yVault/yVault.sol (line 132)
        controller.earn(address(token), _bal);
  26. File: contracts/vaults/yVault/yVault.sol (line 189)
        controller.withdrawJPEG(address(token), farm);
  27. File: contracts/vaults/yVault/yVault.sol (line 125)
            availableTokensRate.denominator;
  28. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 252)
        want.safeIncreaseAllowance(address(convex.booster), balance);
  29. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 277)
        uint256 balance = want.balanceOf(address(this));
  30. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 296)
        balance = want.balanceOf(address(this));
  31. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 233)
            if (address(jpeg) == extraReward.rewardToken()) {
  32. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 306)
        jpeg.safeTransfer(_to, jpeg.balanceOf(address(this)));
  33. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 343)
                    abi.encodePacked(weth, uint24(500), usdc),
  34. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 356)
        uint256 usdcBalance = usdc.balanceOf(address(this));
  35. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 353)
        StrategyConfig memory strategy = strategyConfig;
  36. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 360)
            performanceFee.denominator;
  37. File: contracts/farming/yVaultLPFarming.sol (line 149)
        jpeg.safeTransfer(msg.sender, rewards);
  38. File: contracts/farming/yVaultLPFarming.sol (line 183)
        userLastAccRewardPerShare[account] = accRewardPerShare;
  39. File: contracts/farming/LPFarming.sol (line 130)
            jpeg.safeTransferFrom(
  40. File: contracts/farming/LPFarming.sol (line 120)
            (epoch.endBlock - _blockNumber());
  41. File: contracts/farming/LPFarming.sol (line 271)
        if (blockNumber < epoch.startBlock) return epoch.startBlock;
  42. File: contracts/farming/LPFarming.sol (line 163)
        poolInfo[_pid].allocPoint = _allocPoint;
  43. File: contracts/farming/LPFarming.sol (line 324)
        user.lastAccRewardPerShare = poolInfo[_pid].accRewardPerShare;
  44. File: contracts/farming/LPFarming.sol (line 145)
        totalAllocPoint = totalAllocPoint + _allocPoint;
  45. File: contracts/farming/LPFarming.sol (line 165)
            totalAllocPoint = totalAllocPoint - prevAllocPoint + _allocPoint;

Using calldata instead of memory for read-only arguments in external functions saves gas

  1. File: contracts/vaults/NFTVault.sol (line 146)
        NFTCategoryInitializer[] memory _typeInitializers,
  2. File: contracts/vaults/NFTVault.sol (line 148)
        VaultSettings memory _settings
  3. File: contracts/vaults/NFTVault.sol (line 212)
    function setDebtInterestApr(Rate memory _debtInterestApr)
  4. File: contracts/vaults/NFTVault.sol (line 222)
    function setValueIncreaseLockRate(Rate memory _valueIncreaseLockRate)
  5. File: contracts/vaults/NFTVault.sol (line 232)
    function setCreditLimitRate(Rate memory _creditLimitRate)
  6. File: contracts/vaults/NFTVault.sol (line 247)
    function setLiquidationLimitRate(Rate memory _liquidationLimitRate)
  7. File: contracts/vaults/NFTVault.sol (line 290)
    function setOrganizationFeeRate(Rate memory _organizationFeeRate)
  8. File: contracts/vaults/NFTVault.sol (line 300)
    function setInsurancePurchaseRate(Rate memory _insurancePurchaseRate)
  9. File: contracts/vaults/NFTVault.sol (line 311)
        Rate memory _insuranceLiquidationPenaltyRate
  10. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 70)
        Rate memory _creditLimitRate

++i/i++ should be unchecked{++i}/unchecked{++i} when it is not possible for them to overflow, as is the case when used in for- and while-loops

  1. File: contracts/vaults/NFTVault.sol (line 181)
        for (uint256 i = 0; i < _typeInitializers.length; i++) {
  2. File: contracts/vaults/NFTVault.sol (line 184)
            for (uint256 j = 0; j < initializer.nfts.length; j++) {
  3. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 145)
        for (uint256 i = 0; i < _strategyConfig.rewardTokens.length; i++) {
  4. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 231)
        for (uint256 i = 0; i < length; i++) {
  5. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 319)
            for (uint256 i = 0; i < rewardTokens.length; i++) {
  6. File: contracts/farming/LPFarming.sol (line 281)
        for (uint256 pid = 0; pid < length; ++pid) {
  7. File: contracts/farming/LPFarming.sol (line 348)
        for (uint256 i = 0; i < poolInfo.length; i++) {

++i costs less gas than ++i, especially when it's used in for-loops (--i/i-- too)

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

Splitting require() statements that use && saves gas

See this issue for an example

  1. File: contracts/escrow/NFTEscrow.sol (lines 86-89)
        require(
            _owner != address(this) && _owner != address(0),
            "NFTEscrow: invalid_owner"
        );
  2. File: contracts/vaults/NFTVault.sol (lines 401-404)
        require(
            rate.denominator > 0 && rate.denominator >= rate.numerator,
            "invalid_rate"
        );
  3. File: contracts/vaults/FungibleAssetVaultForDAO.sol (lines 93-98)
        require(
            _creditLimitRate.denominator > 0 &&
                //denominator can be equal to the numerator in some cases (stablecoins used as collateral)
                _creditLimitRate.denominator >= _creditLimitRate.numerator,
            "invalid_rate"
        );
  4. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 194)
        require(amount > 0 && amount <= collateralAmount, "invalid_amount");
  5. File: contracts/vaults/yVault/yVault.sol (lines 99-102)
        require(
            _rate.numerator > 0 && _rate.denominator >= _rate.numerator,
            "INVALID_RATE"
        );
  6. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (lines 181-185)
        require(
            _performanceFee.denominator > 0 &&
                _performanceFee.denominator >= _performanceFee.numerator,
            "INVALID_RATE"
        );
  7. File: contracts/staking/JPEGStaking.sol (lines 45-48)
        require(
            _amount > 0 && _amount <= balanceOf(msg.sender),
            "invalid_amount"
        );

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

  1. File: contracts/vaults/NFTVault.sol (line 55)
        uint128 numerator;
  2. File: contracts/vaults/NFTVault.sol (line 56)
        uint128 denominator;
  3. File: contracts/vaults/NFTVault.sol (line 460)
        uint8 decimals = aggregator.decimals();
  4. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 37)
        uint128 numerator;
  5. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 38)
        uint128 denominator;
  6. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 106)
        uint8 decimals = oracle.decimals();
  7. File: contracts/vaults/yVault/yVault.sol (line 24)
        uint128 numerator;
  8. File: contracts/vaults/yVault/yVault.sol (line 25)
        uint128 denominator;
  9. File: contracts/vaults/yVault/yVault.sol (line 70)
    function decimals() public view virtual override returns (uint8) {
  10. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 28)
        uint128 numerator;
  11. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 29)
        uint128 denominator;

abi.encode() is less efficient than abi.encodePacked()

  1. File: contracts/escrow/NFTEscrow.sol (line 52)
                abi.encode(nftAddress, _encodeFlashEscrowPayload(_idx))

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

See this issue for a detail description of the issue

  1. File: contracts/vaults/NFTVault.sol (line 71)
    bytes32 public constant DAO_ROLE = keccak256("DAO_ROLE");
  2. File: contracts/vaults/NFTVault.sol (line 72)
    bytes32 public constant LIQUIDATOR_ROLE = keccak256("LIQUIDATOR_ROLE");
  3. File: contracts/vaults/NFTVault.sol (line 74)
    bytes32 public constant CUSTOM_NFT_HASH = keccak256("CUSTOM");
  4. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 41)
    bytes32 public constant WHITELISTED_ROLE = keccak256("WHITELISTED_ROLE");
  5. File: contracts/vaults/yVault/Controller.sol (line 15)
    bytes32 public constant STRATEGIST_ROLE = keccak256("STRATEGIST_ROLE");
  6. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 66)
    bytes32 public constant STRATEGIST_ROLE = keccak256("STRATEGIST_ROLE");
  7. File: contracts/tokens/StableCoin.sol (line 22)
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
  8. File: contracts/tokens/StableCoin.sol (line 23)
    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
  9. File: contracts/tokens/JPEG.sol (line 10)
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code

  1. File: contracts/vaults/NFTVault.sol (line 71)
    bytes32 public constant DAO_ROLE = keccak256("DAO_ROLE");
  2. File: contracts/vaults/NFTVault.sol (line 72)
    bytes32 public constant LIQUIDATOR_ROLE = keccak256("LIQUIDATOR_ROLE");
  3. File: contracts/vaults/NFTVault.sol (line 74)
    bytes32 public constant CUSTOM_NFT_HASH = keccak256("CUSTOM");
  4. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 41)
    bytes32 public constant WHITELISTED_ROLE = keccak256("WHITELISTED_ROLE");
  5. File: contracts/vaults/yVault/Controller.sol (line 15)
    bytes32 public constant STRATEGIST_ROLE = keccak256("STRATEGIST_ROLE");
  6. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 66)
    bytes32 public constant STRATEGIST_ROLE = keccak256("STRATEGIST_ROLE");
  7. File: contracts/tokens/StableCoin.sol (line 22)
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
  8. File: contracts/tokens/StableCoin.sol (line 23)
    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
  9. File: contracts/tokens/JPEG.sol (line 10)
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

Don't compare boolean expressions to boolean literals

if (<x> == true) => if (<x>), if (<x> == false) => if (!<x>)

  1. File: contracts/vaults/yVault/Controller.sol (line 87)
            approvedStrategies[_token][_strategy] == true,

Duplicated require()/revert() checks should be refactored to a modifier or function

  1. File: contracts/vaults/NFTVault.sol (line 764)
        require(_amount > 0, "invalid_amount");
  2. File: contracts/vaults/NFTVault.sol (line 767)
        require(position.liquidatedAt == 0, "liquidated");
  3. File: contracts/vaults/NFTVault.sol (line 805)
        require(msg.sender == positionOwner[_nftIndex], "unauthorized");
  4. File: contracts/vaults/NFTVault.sol (line 926)
        require(position.liquidatedAt > 0, "not_liquidated");
  5. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 164)
        require(amount > 0, "invalid_amount");
  6. File: contracts/vaults/yVault/Controller.sol (line 73)
        require(address(_token) != address(0), "INVALID_TOKEN");
  7. File: contracts/vaults/yVault/Controller.sol (line 74)
        require(address(_strategy) != address(0), "INVALID_STRATEGY");
  8. File: contracts/vaults/yVault/Controller.sol (line 164)
        require(msg.sender == vaults[_token], "NOT_VAULT");
  9. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 292)
        require(vault != address(0), "ZERO_VAULT"); // additional protection so we don't burn the funds
  10. File: contracts/farming/yVaultLPFarming.sol (line 118)
        require(_amount > 0, "invalid_amount");
  11. File: contracts/farming/LPFarming.sol (line 239)
        require(_amount > 0, "invalid_amount");
  12. File: contracts/farming/LPFarming.sol (line 354)
        require(rewards > 0, "no_reward");

Stack variable used as a cheaper cache for a state variable is only used once

If the variable is only accessed once, it's cheaper to use the state variable directly that one time

  1. File: contracts/farming/yVaultLPFarming.sol (line 81)
        uint256 staked = totalStaked;

State variables only set in the constructor should be declared immutable

  1. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 75)
    DexConfig public dexConfig;
  2. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 76)
    CurveConfig public curveConfig;
  3. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 77)
    ConvexConfig public convexConfig;
  4. File: contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol (line 78)
    StrategyConfig public strategyConfig;

require() or revert() statements that check input arguments should be at the top of the function

Checks that involve constants should come before checks that involve state variables

  1. File: contracts/vaults/NFTVault.sol (line 687)
        require(_amount > 0, "invalid_amount");
  2. File: contracts/vaults/NFTVault.sol (line 764)
        require(_amount > 0, "invalid_amount");
  3. File: contracts/vaults/yVault/Controller.sol (line 49)
        require(_vault != address(0), "INVALID_VAULT");
  4. File: contracts/farming/LPFarming.sol (line 243)
        require(user.amount >= _amount, "insufficient_amount");

Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

  1. File: contracts/vaults/NFTVault.sol (lines 203-205)
    function setBorrowAmountCap(uint256 _borrowAmountCap)
        external
        onlyRole(DAO_ROLE)
  2. File: contracts/vaults/NFTVault.sol (lines 212-214)
    function setDebtInterestApr(Rate memory _debtInterestApr)
        external
        onlyRole(DAO_ROLE)
  3. File: contracts/vaults/NFTVault.sol (lines 222-224)
    function setValueIncreaseLockRate(Rate memory _valueIncreaseLockRate)
        external
        onlyRole(DAO_ROLE)
  4. File: contracts/vaults/NFTVault.sol (lines 232-234)
    function setCreditLimitRate(Rate memory _creditLimitRate)
        external
        onlyRole(DAO_ROLE)
  5. File: contracts/vaults/NFTVault.sol (lines 247-249)
    function setLiquidationLimitRate(Rate memory _liquidationLimitRate)
        external
        onlyRole(DAO_ROLE)
  6. File: contracts/vaults/NFTVault.sol (lines 262-264)
    function toggleFallbackOracle(bool _useFallback)
        external
        onlyRole(DAO_ROLE)
  7. File: contracts/vaults/NFTVault.sol (line 271)
    function setJPEGLockTime(uint256 _newLockTime) external onlyRole(DAO_ROLE) {
  8. File: contracts/vaults/NFTVault.sol (line 277)
    function overrideFloor(uint256 _newFloor) external onlyRole(DAO_ROLE) {
  9. File: contracts/vaults/NFTVault.sol (line 284)
    function disableFloorOverride() external onlyRole(DAO_ROLE) {
  10. File: contracts/vaults/NFTVault.sol (lines 290-292)
    function setOrganizationFeeRate(Rate memory _organizationFeeRate)
        external
        onlyRole(DAO_ROLE)
  11. File: contracts/vaults/NFTVault.sol (lines 300-302)
    function setInsurancePurchaseRate(Rate memory _insurancePurchaseRate)
        external
        onlyRole(DAO_ROLE)
  12. File: contracts/vaults/NFTVault.sol (lines 310-312)
    function setInsuranceLiquidationPenaltyRate(
        Rate memory _insuranceLiquidationPenaltyRate
    ) external onlyRole(DAO_ROLE) {
  13. File: contracts/vaults/NFTVault.sol (lines 321-324)
    function setNFTType(uint256 _nftIndex, bytes32 _type)
        external
        validNFTIndex(_nftIndex)
        onlyRole(DAO_ROLE)
  14. File: contracts/vaults/NFTVault.sol (lines 336-338)
    function setNFTTypeValueETH(bytes32 _type, uint256 _amountETH)
        external
        onlyRole(DAO_ROLE)
  15. File: contracts/vaults/NFTVault.sol (lines 347-350)
    function setPendingNFTValueETH(uint256 _nftIndex, uint256 _amountETH)
        external
        validNFTIndex(_nftIndex)
        onlyRole(DAO_ROLE)
  16. File: contracts/vaults/NFTVault.sol (lines 830-834)
    function liquidate(uint256 _nftIndex)
        external
        onlyRole(LIQUIDATOR_ROLE)
        validNFTIndex(_nftIndex)
        nonReentrant
  17. File: contracts/vaults/NFTVault.sol (line 944)
    function collect() external nonReentrant onlyRole(DAO_ROLE) {
  18. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 92)
    function setCreditLimitRate(Rate memory _creditLimitRate) public onlyRole(DEFAULT_ADMIN_ROLE) {
  19. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 141)
    function deposit(uint256 amount) external payable onlyRole(WHITELISTED_ROLE) {
  20. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 163)
    function borrow(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {
  21. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 179)
    function repay(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {
  22. File: contracts/vaults/FungibleAssetVaultForDAO.sol (line 193)
    function withdraw(uint256 amount) external onlyRole(WHITELISTED_ROLE) nonReentrant {
  23. File: contracts/vaults/yVault/Controller.sol (lines 33-35)
    function setFeeAddress(address _feeAddress)
        public
        onlyRole(DEFAULT_ADMIN_ROLE)
  24. File: contracts/vaults/yVault/Controller.sol (lines 44-46)
    function setVault(IERC20 _token, address _vault)
        external
        onlyRole(STRATEGIST_ROLE)
  25. File: contracts/vaults/yVault/Controller.sol (lines 56-58)
    function approveStrategy(IERC20 _token, IStrategy _strategy)
        external
        onlyRole(DEFAULT_ADMIN_ROLE)