code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L830 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L68

Vulnerability details

Impact

The NFTVault allows value proposals to be finalized and warranted as long as users are willing to lock and stake their JPEG tokens in JPEGLock.

However, since the tokens JPEG tokens are never liquidated even if NFT is severely overvalued and liquidated due to expiry of borrow positions, users are incentivised to finalize whatever proposal that is higher than market price. Then proceed to dump NFT as collateral for "borrowing" stablecoin without ever intending to redeem NFT.

In cases when NFT value rises, the malicious actor can always decide on the spot to redeem it and won't suffer from any loss of value.

This is a classic nothing at stake scenario, where finalizing unreasonably high NFT value proposals can only result in gain of malicious actors and loss of current stablecoin holders.

Proof of Concept

During liquidation of a borrow position, no punishment on NFT value warrantor exists, and JPEG are always returned to lockers upon expiry.

This incentivizes malicious users to finalize high NFT value proposals and enjoy the profits without any risk.

It also puts current owners of stablecoin at an inferior position, since excessive distribution of stablecoin decreases its value.

    function liquidate(uint256 _nftIndex)
        external
        onlyRole(LIQUIDATOR_ROLE)
        validNFTIndex(_nftIndex)
        nonReentrant
    {
        accrue();

        address posOwner = positionOwner[_nftIndex];
        require(posOwner != address(0), "position_not_exist");

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

        uint256 debtAmount = _getDebtAmount(_nftIndex);
        require(
            debtAmount >= _getLiquidationLimit(_nftIndex),
            "position_not_liquidatable"
        );

        // burn all payment
        stablecoin.burnFrom(msg.sender, debtAmount);

        // update debt portion
        totalDebtPortion -= position.debtPortion;
        totalDebtAmount -= debtAmount;
        position.debtPortion = 0;

        bool insured = position.borrowType == BorrowType.USE_INSURANCE;
        if (insured) {
            position.debtAmountForRepurchase = debtAmount;
            position.liquidatedAt = block.timestamp;
            position.liquidator = msg.sender;
        } else {
            // transfer nft to liquidator
            positionOwner[_nftIndex] = address(0);
            delete positions[_nftIndex];
            positionIndexes.remove(_nftIndex);
            nftContract.safeTransferFrom(address(this), msg.sender, _nftIndex);
        }

        emit Liquidated(msg.sender, posOwner, _nftIndex, insured);
    }

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Seize warrantor's JPEG tokens when a liquidation happens. If a global NFT value estimation exists, liquidate only the difference between the individual value and global value. This will force proposal finalizers to take responsibility for their decisions.

spaghettieth commented 2 years ago

The new value of an NFT is decided with a governance proposal, if someone were to try and set the value to something too high (higher than the actual value of the NFT) the proposal would be rejected.

dmvt commented 2 years ago

Again, we have to assume that the DAO is going to act with the best intentions and in favor of the protocol continuing to work well. This is invalid.

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