JPEG'd contest - horsefacts's results

Lines of code

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

Vulnerability details

Use of deprecated oracle API in _normalizeAggregatorAnswer

Likelihood low, impact high.

The Chainlink latestAnswer function included in IAggregatorV3Interface and called in NFTVault#_normalizeAggregatorAnswer() is considered deprecated and no longer included in the Chainlink API documentation.

It's considered best practice to use the latestRoundData function instead. (API docs). latestAnswer returns only the value of the latest price, whereas latestRoundData returns additional information that can be used to validate whether a price is stale:

NFTVault#459:

        int256 answer = oracle.latestAnswer();
        uint8 decimals = oracle.decimals();

Example using latestRoundData:

        (uint80 roundId,
        int256 answer,
        uint256 startedAt,
        uint256 updatedAt,
        uint80 answeredInRound) = oracle.latestRoundData();
        require(answeredInRound >= roundId, "Stale data");
        require(timestamp != 0, "Zero timestamp");
        uint8 decimals = oracle.decimals();

Stale prices may impact collateral value and credit limit calculations, incorrectly reporting a position as under- or overcollateralized.

See also OpenZeppelin's guidelines for safely using latestRoundData and latestAnswer, and consider the impact of a stale or reverting price feed.

Low

Missing events for parameter changes

Throughout the codebase, most state changing functions that update protocol parameters do not emit events.

For example:

• NFTVault#setCreditLimitRate
• NFTVault#setLiquidationLimitRate
• NFTVault#toggleFallbackOracle
• NFTVault#setBorrowAmountCap
• NFTVault#setDebtInterestApr
• NFTVault#setValueIncreaseLockRate
• NFTVault#setJPEGLockTime
• NFTVault#disableFloorOverride
• NFTVault#setOrganizationFeeRate
• NFTVault#setInsurancePurchaseRate
• NFTVault#setInsuranceLiquidationPenaltyRate

This limits the ability for off-chain monitoring tools to observe and react to changes to protocol parameters. It's considered a best practice to emit events for state changing operations, especially changes to key protocol parameters.

Avoid payable.transfer

ETH transfers in FungibleAssetVaultForDAO#withdraw are performed using payable(msg.sender).transfer.

It's considered a best practice to avoid this pattern for ETH transfers, since it forwards a fixed amount of gas and may revert if future gas costs change. (See the Consensys Diligence article here.)

Since the withdraw function is already nonReentrant, consider using OpenZeppelin's Address.sendValue instead.

Unbounded iteration over LP farming pools in _massUpdatePools

The _massUpdatePools function in LPFarming.sol iterates over all active and inactive LP pools. The contract owner may add, but not update or remove pools.

If the poolInfo array grows too large, add, set, and newEpoch may revert, impacting the ability to create and manage pools and preventing creation of new rewards epochs.

Additionally, the _updatePool function contains an external call to pool.lpToken.balanceOf. If a malicious or malfunctioning LP token is added to a configured pool, this call may revert.

Likelihood is mitigated since adding pools is a permissioned function and there will likely be a limited number of LP pools. Owner should monitor gas usage when adding a new pool, and take care to validate approved LP token addresses. Consider adding the ability to disable, remove, or reconfigure a pool.

Code

LPFarming.sol#281

    function _massUpdatePools() internal {
        uint256 length = poolInfo.length;
        for (uint256 pid = 0; pid < length; ++pid) {
            _updatePool(pid);
        }
    }

LPFraming.sol#300

    function _updatePool(uint256 _pid) internal {
        PoolInfo storage pool = poolInfo[_pid];
        if (pool.allocPoint == 0) {
            return;
        }

        uint256 blockNumber = _blockNumber();
        //normalizing the pool's `lastRewardBlock` ensures that no rewards are distributed by staking outside of an epoch
        uint256 lastRewardBlock = _normalizeBlockNumber(pool.lastRewardBlock);
        if (blockNumber <= lastRewardBlock) {
            return;
        }
        uint256 lpSupply = pool.lpToken.balanceOf(address(this));
        if (lpSupply == 0) {
            pool.lastRewardBlock = blockNumber;
            return;
        }
        uint256 reward = ((blockNumber - lastRewardBlock) *
            epoch.rewardPerBlock *
            1e36 *
            pool.allocPoint) / totalAllocPoint;
        pool.accRewardPerShare = pool.accRewardPerShare + reward / lpSupply;
        pool.lastRewardBlock = blockNumber;
    }

Unbounded iteration over LP farming pools in claimAll()

The claimAll function in LPFarming.sol iterates over all active and inactive LP pools in the poolInfo array to claim rewards. The contract owner may add, but not remove pools.

If the poolInfo array grows too large, claimAll, may revert, preventing users from claiming accrued rewards. Additionally, the _updatePool function contains an external call to pool.lpToken.balanceOf that could revert if a malicious or malfunctioning LP token is added to pool configuration.

Severity is significantly mitigated since the user may still claim rewards from a single pool using claim(uint256). Likelihood is mitigated since adding pools is a permissioned function and there will likely be a limited number of LP pools. Owner should monitor gas usage when adding new pools.

Code

LPFarming.sol#348

    function claimAll() external nonReentrant noContract(msg.sender) {
        for (uint256 i = 0; i < poolInfo.length; i++) {
            _updatePool(i);
            _withdrawReward(i);
        }

        uint256 rewards = userRewards[msg.sender];
        require(rewards > 0, "no_reward");

        jpeg.safeTransfer(msg.sender, rewards);
        userRewards[msg.sender] = 0;

        emit ClaimAll(msg.sender, rewards);
    }

QA/Informational

Duplicated noContract modifier

The noContract modifier, whitelistedContracts mapping, and setContractWhitelisted functions are duplicated across multiple contracts. Consider extracting this functionality to a shared abstract contract.

Usages:

• yVaultLPFarming.sol#54
• LPFarming.sol#85
• yVault.sol#61

Constructor call without args can be omitted

The Ownable() and ReentrancyGuard() calls included inline in the JPEGLock constructor may be omitted since these base contracts require no constructor arguments.

JPEGLock#31

    constructor(IERC20 _jpeg) Ownable() ReentrancyGuard() {
        jpeg = _jpeg;
        lockTime = 365 days;
    }

Suggested:

    constructor(IERC20 _jpeg) {
        jpeg = _jpeg;
        lockTime = 365 days;
    }

Local variable shadows initializer

The local variable initializer inside the category initialization loop in NFTVault#constructor shadows the initializer modifier. Consider renaming this local variable.

NFTVault.sol#182

        for (uint256 i = 0; i < _typeInitializers.length; i++) {
            // QA: variable name shadows `initializer` modifier
            NFTCategoryInitializer memory initializer = _typeInitializers[i];
            nftTypeValueETH[initializer.hash] = initializer.valueETH;
            for (uint256 j = 0; j < initializer.nfts.length; j++) {
                nftTypes[initializer.nfts[j]] = initializer.hash;
            }
        }

Missing parameter documentation

Natspec documentation is missing for the _jpeg parameter in Controller#constructor.

Omit boolean check

Since approvedStrategies[_token][_strategy] returns a boolean value, you can safely omit the == true check on line 87 of Controller#setStrategy.