c4-bot-2 commented 8 months ago

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L171-L200

Vulnerability details

Impact

The auction owner does not and the NFT creator(s) benefit from the auction at all. A malicious user obtains NFT at a very low price and grieves other users.

Proof of Concept

When top piece is auctioned off every day as an ERC721 VerbsToken via the AuctionHouse.sol contract, the owner of AuctionHouse.sol contract can call unpause() function to create an auction via _createAuction() function.

    function unpause() external override onlyOwner {
        _unpause();

        if (auction.startTime == 0 || auction.settled) {
            _createAuction();
        }
    }

    function _createAuction() internal {
        // Check if there's enough gas to safely execute token.mint() and subsequent operations
        require(gasleft() >= MIN_TOKEN_MINT_GAS_THRESHOLD, "Insufficient gas for creating auction");

        try verbs.mint() returns (uint256 verbId) {
            uint256 startTime = block.timestamp;
            uint256 endTime = startTime + duration;

            auction = Auction({
                verbId: verbId,
                amount: 0,
                startTime: startTime,
                endTime: endTime,
                bidder: payable(0),
                settled: false
            });

            emit AuctionCreated(verbId, startTime, endTime);
        } catch {
            _pause();
        }
    }

After that everyone the want the certain NFT form the auction can call createBid() function to create a bid for a Verb, with a given amount

    function createBid(uint256 verbId, address bidder) external payable override nonReentrant {
        IAuctionHouse.Auction memory _auction = auction;

        //require bidder is valid address
        require(bidder != address(0), "Bidder cannot be zero address");
        require(_auction.verbId == verbId, "Verb not up for auction");
        //slither-disable-next-line timestamp
        require(block.timestamp < _auction.endTime, "Auction expired");
        require(msg.value >= reservePrice, "Must send at least reservePrice");
        require(
            msg.value >= _auction.amount + ((_auction.amount * minBidIncrementPercentage) / 100),
            "Must send more than last bid by minBidIncrementPercentage amount"
        );

        address payable lastBidder = _auction.bidder;

        auction.amount = msg.value;
        auction.bidder = payable(bidder);

        // Extend the auction if the bid was received within `timeBuffer` of the auction end time
        bool extended = _auction.endTime - block.timestamp < timeBuffer;
        if (extended) auction.endTime = _auction.endTime = block.timestamp + timeBuffer;

        // Refund the last bidder, if applicable
        if (lastBidder != address(0)) _safeTransferETHWithFallback(lastBidder, _auction.amount);

        emit AuctionBid(_auction.verbId, bidder, msg.sender, msg.value, extended);

        if (extended) emit AuctionExtended(_auction.verbId, _auction.endTime);
    }

Every auction has an endTime set in _createAuction() function. Now, there is a serious problem, because stuffing whole duration of the auction to auction#endTime with dummy transactions is very cheap on Optimism L2 According to https://www.cryptoneur.xyz/en/gas-fees-calculator 50M gas (which is several whole blocks) - costs ~11.1192 USD$ on Optimism L2. This makes a malicious user occasion to cheaply prohibit other users to overbid them, winning the auction at the least favorable price for the protocol.

The createBid() function actually try to prevent the block stuffing with the implementation of minBidIncrementPercentage and timeBuffer but this actually doesn't safe the current auction, because as it writes in the following article:

mining the block takes significantly more time than executing the transactions (https://medium.com/hackernoon/the-anatomy-of-a-block-stuffing-attack-a488698732ae)

Tool Used

Manual Review

Recommendation Migration Steps

There are several possible solutions:

make bidding window dependent on the current bid amount of the auction, to disincentivize block stuffing.
always ensure that the reservePrice is enough high in the beginning of the auction.

Assessed type

Context

c4-pre-sort commented 8 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 8 months ago

raymondfam marked the issue as primary issue

raymondfam commented 8 months ago

QA low.

c4-judge commented 8 months ago

MarioPoneder changed the severity to QA (Quality Assurance)