JPEG'd contest - AuditsAreUS's results

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L360-L381 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L347-L353 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/lock/JPEGLock.sol#L49-L63

Vulnerability details

Impact

If the same NFT index is locked more than once then the initial locker will lose their funds, they will not be recoverable.

The function lockFor() does not account for the case where the nftIndex is already in use. If this index is already in use positions[_nftIndex] is overwritten with the new value and the old users funds a lost.

Proof of Concept

The function finalizePendingNFTValueETH() may be called multiple times for the same nftIndex so long as their is pendingValue set by the DAO.

    function finalizePendingNFTValueETH(uint256 _nftIndex)
        external
        validNFTIndex(_nftIndex)
    {
        uint256 pendingValue = pendingNFTValueETH[_nftIndex];
        require(pendingValue > 0, "no_pending_value");
        uint256 toLockJpeg = (((pendingValue *
            _ethPriceUSD() *
            settings.creditLimitRate.numerator) /
            settings.creditLimitRate.denominator) *
            settings.valueIncreaseLockRate.numerator) /
            settings.valueIncreaseLockRate.denominator /
            _jpegPriceUSD();

        //lock JPEG using JPEGLock
        jpegLocker.lockFor(msg.sender, _nftIndex, toLockJpeg);

        nftTypes[_nftIndex] = CUSTOM_NFT_HASH;
        nftValueETH[_nftIndex] = pendingValue;
        //clear pending value
        pendingNFTValueETH[_nftIndex] = 0;
    }

There are no restrictions in setPendingNFTValueETH() on whether the token has previously been set.

    function setPendingNFTValueETH(uint256 _nftIndex, uint256 _amountETH)
        external
        validNFTIndex(_nftIndex)
        onlyRole(DAO_ROLE)
    {
        pendingNFTValueETH[_nftIndex] = _amountETH;
    }

The function lockFor() will overwrite positions[_nftIndex] irrelevant of the current state.

    function lockFor(
        address _account,
        uint256 _nftIndex,
        uint256 _lockAmount
    ) external onlyOwner nonReentrant {
        jpeg.safeTransferFrom(_account, address(this), _lockAmount);

        positions[_nftIndex] = LockPosition({
            owner: _account,
            unlockAt: block.timestamp + lockTime,
            lockAmount: _lockAmount
        });

        emit Lock(_account, _nftIndex, _lockAmount);
    }

Recommended Mitigation Steps

Consider updating the function lockFor() to handle the case where some value is already locked for this nftIndex.

One option is to unlock the previous amount and transfer those tokens to the previous owner before updating the position.

An alternate option is to revert if there is already a position with lockAmount > 0.

Lines of code

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

Vulnerability details

Impact

There is a division before multiplication bug in NFTVault._calculateAdditionalInterest() which may result in no interesting being accrued and will have significant rounding issues for tokens with small decimal places.

This issue occurs since an intermediate calculation of interestPerSec may round to zero and therefore the multiplication by elapsedTime may remain zero.

Furthermore, even if interestPerSec > 0 there will still be rounding errors as a result of doing division before multiplication and _calculatedInterest() will be understated.

This issue is significant as one divisor is 365 days = 30,758,400 (excluding the rate). Since many ERC20 tokens such as USDC and USDT only have 6 decimal places a numerator of less 30 * 10^6 will round to zero.

The rate also multiplies into the denominator. e.g. If the rate is 1% then the denominator will be equivalent to 1 / rate * 30 * 10^6 = 3,000 * 10^6.

Proof of Concept

The order of operations for the interest calculations

• totalDebtAmount
• MUL settings.debtInterestApr.numerator
• DIV settings.debtInterestApr.denominator
• DIV 365 days
• MUL elapsedTime

If the intermediate value of interestPerSec = 0 then the multiplication by elapsedTime will still be zero and no interested will be accrued.

Excerpt from NFTVault._calculateAdditionalInterest().

        uint256 interestPerYear = (totalDebtAmount *
            settings.debtInterestApr.numerator) /
            settings.debtInterestApr.denominator;
        uint256 interestPerSec = interestPerYear / 365 days;

        return elapsedTime * interestPerSec;

Recommended Mitigation Steps

This issue may be resolved by performing the multiplication by elapsedTime before the division by the denominator or 365 days.

        uint256 interestAccrued = (elapsedTime * 
            totalDebtAmount *
            settings.debtInterestApr.numerator) /
            settings.debtInterestApr.denominator /
            365 days;

        return  interestAccrued;

Lines of code

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

Vulnerability details

Impact

NFTVault.closePosition() does not enforce that a position has not been liquidated.

As a result it is possible for the owner to close a position with closePosition() after a debt has been liquidated so long as it is insured since the position will only be deleted if it is not insured or has been repurchased.

Proof of Concept

The function closePosition() does not enforce that a position is not liquidated.

    function closePosition(uint256 _nftIndex)
        external
        validNFTIndex(_nftIndex)
    {
        accrue();

        require(msg.sender == positionOwner[_nftIndex], "unauthorized");
        require(_getDebtAmount(_nftIndex) == 0, "position_not_repaid");

        positionOwner[_nftIndex] = address(0);
        delete positions[_nftIndex];
        positionIndexes.remove(_nftIndex);

        // transfer nft back to owner if nft was deposited
        if (nftContract.ownerOf(_nftIndex) == address(this)) {
            nftContract.safeTransferFrom(address(this), msg.sender, _nftIndex);
        }

        emit PositionClosed(msg.sender, _nftIndex);
    }

Recommended Mitigation Steps

Consider adding the check to see if the position is liquidated in closePosition(). e.g.

        require(positions[_nftIndex].liquidatedAt == 0, "liquidated");