Revolution Protocol - Pechenite's results

Lines of code

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

Vulnerability details

Impact

The AuctionHouse.sol contract is designed for managing NFT auctions, includes also various features such as auction creation, bidding, settlement, and integration with external tokens. The contract allow several auction parameters such as timeBuffer, reservePrice, and minBidIncrementPercentage to can be changed globally through an active auction. These parameters are not individually cached for each auction, but rather exist as global settings for the contract. This design can lead to inconsistencies and potentially exploitable conditions if these parameters are changed during an ongoing auction.

    /**
     * @notice Set the auction time buffer.
     * @dev Only callable by the owner.
     */
    function setTimeBuffer(uint256 _timeBuffer) external override onlyOwner {
        timeBuffer = _timeBuffer;

        emit AuctionTimeBufferUpdated(_timeBuffer);
    }

    /**
     * @notice Set the auction reserve price.
     * @dev Only callable by the owner.
     */
    function setReservePrice(uint256 _reservePrice) external override onlyOwner {
        reservePrice = _reservePrice;

        emit AuctionReservePriceUpdated(_reservePrice);
    }

    /**
     * @notice Set the auction minimum bid increment percentage.
     * @dev Only callable by the owner.
     */
    function setMinBidIncrementPercentage(uint8 _minBidIncrementPercentage) external override onlyOwner {
        minBidIncrementPercentage = _minBidIncrementPercentage;

        emit AuctionMinBidIncrementPercentageUpdated(_minBidIncrementPercentage);
    }

Proof of Concept

1. Global Parameters: In the current implementation, the contract uses single, global state variables for timeBuffer, reservePrice, and minBidIncrementPercentage. These variables apply to the active auction and any future auctions until they are changed.

2. Lack of Individual Auction Context: Each auction does not have its own set of these parameters. Consequently, if an auction is ongoing and one of these parameters is changed, the new values immediately affect the current auction.

3. Potential Exploitation: If these parameters are changed while an auction is active. This can lead to scenarios where the rules of the auction are altered in real-time, potentially disadvantaging certain bidders or favoring others. For example, increasing the minBidIncrementPercentage could unexpectedly raise the minimum additional bid amount required, while decreasing the timeBuffer could shorten the auction duration unexpectedly.

Consider the following scenario:

1. New auction is created and currently reservePrice for the AuctionHouse.sol contract is 1 ether.
2. Bob really want this NFT and create bid with amount = 3e18 via createBid() function.
3. The reservePrice is changed to 5 ethers. Subsequently the end time for the current auction is over.
4. Now Bob thinks that he has been won the auction but actually in _settleAuction() function the NFT is burned and is not get the NFT.

    function _settleAuction() internal {
        IAuctionHouse.Auction memory _auction = auction;

        // ...code ...

        uint256 creatorTokensEmitted = 0;
        // Check if contract balance is greater than reserve price
        if (address(this).balance < reservePrice) {
            // If contract balance is less than reserve price, refund to the last bidder
            if (_auction.bidder != address(0)) {
                _safeTransferETHWithFallback(_auction.bidder, _auction.amount);
            }

            // And then burn the Noun
            verbs.burn(_auction.verbId);
        } else {
            // ... code ...
        }

        // ...code...
    }

Tools Used

Manual review

Recommendations for Mitigation

Implement a mechanism where each auction caches its own set of parameters at the time of creation. This ensures that the rules of an auction remain consistent throughout its duration. Another possible solution is to add a whenPaused modifier in the setter functions of auction parameters so that the auction parameters cannot be changed while there is an active auction.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L519-L534

Vulnerability details

Impact

When new pieces are created in CultureIndex.sol, they are assigned a required quorumVotes threshold, which must be reached in order for them to be dropped and potentially moved to an auction. The dropper in CultureIndex.sol can drop only the top voted piece, and only if this piece has reached its required quorum votes.

The dropTopVotedPiece() function in CultureIndex.sol is as follows:

    function dropTopVotedPiece() public nonReentrant returns (ArtPiece memory) {
        require(msg.sender == dropperAdmin, "Only dropper can drop pieces");

        ICultureIndex.ArtPiece memory piece = getTopVotedPiece();
        require(totalVoteWeights[piece.pieceId] >= piece.quorumVotes, "Does not meet quorum votes to be dropped.");

        // Set the piece as dropped
        pieces[piece.pieceId].isDropped = true;

        // Extract the piece from the heap
        maxHeap.extractMax();

        emit PieceDropped(piece.pieceId, msg.sender);

        return pieces[piece.pieceId];
    }

Currently, this logic could significantly impact the functionality and fairness of the system. Specifically, it can lead to scenarios where art pieces that have met their quorum requirements for votes are unable to progress to the dropping phase (which could mean moving to an auction). This issue arises from the dynamic nature of the quorumVotesBPS variable, which affects new pieces but does not retroactively adjust the quorum for existing pieces. Consequently, a newer piece with a higher total vote weight but an unmet quorum can prevent older pieces from being dropped, even if they have met their quorum. This flaw can lead to:

1. Operational Inefficiency: Pieces eligible for dropping are stuck, causing a backlog and potentially impacting the platform's dynamics.
2. User Dissatisfaction: Participants whose pieces are unfairly blocked may lose trust in the platform, leading to reduced participation and engagement.

Proof of Concept

Consider the following scenario:

Current State:

• quorumVotesBPS = 3,000
• erc20VotingToken.totalSupply() = 3,000
• erc721VotingToken.totalSupply() = 0

1. A user creates a new piece, and the quorumVotes for this piece is calculated as 900 ((3000 * 3000) / 10,000).
2. The quorumVotesBPS is then changed to 6,000.
3. Subsequently, another user creates a new piece under the current quorumVotesBPS of 6,000, setting the quorumVotes for this piece at 1,800 ((6000 * 3000) / 10,000).
4. After a period of voting, the totalVoteWeight for the first piece becomes 1,000 and for the second piece becomes 1,500. Thus, the first piece has reached its quorum, but the second piece has not yet.
5. Currently, the first piece, having reached its quorum votes, cannot be dropped and moved to auction because the second piece, although not having met its quorum, has a larger totalVotesWeight.

Tools Used

Manual review

Recommended Mitigation Steps

A possible solution is to refactor the method by which the 'top voted piece' is selected in the getTopVotedPiece() function. Instead of selecting the piece with the highest overall votes, the function should choose the top voted piece among those that have already reached their required quorumVotes. This adjustment would also eliminate the need for the check require(totalVoteWeights[piece.pieceId] >= piece.quorumVotes, "Does not meet quorum votes to be dropped."); in the dropTopVotedPiece() function.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L209-L248 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/CultureIndex.sol#L307-L324

Vulnerability details

Impact

The CultureIndex.sol contract logic concerning voting involves two key components: erc20VotingToken and erc721VotingToken. Votes are cast using these tokens, and their total supply at the time of piece creation is critical in determining totalVotesSupply and quorumVotes for each piece. The issue arises when these tokens are minted after a piece's creation but within the same block. As the contract uses the current block's timestamp to calculate the vote weight and quorum, any tokens minted in the same block.timestamp unexpectedly alter these calculations. This leads to a scenario where totalVotesSupply and quorumVotes become "outdated" or incorrect.

• createPiece() function

    function createPiece(
        ArtPieceMetadata calldata metadata,
        CreatorBps[] calldata creatorArray
    ) public returns (uint256) {
        uint256 creatorArrayLength = validateCreatorsArray(creatorArray);

        // Validate the media type and associated data
        validateMediaType(metadata);

        uint256 pieceId = _currentPieceId++;

        /// @dev Insert the new piece into the max heap
        maxHeap.insert(pieceId, 0);

        ArtPiece storage newPiece = pieces[pieceId];

        newPiece.pieceId = pieceId;
        newPiece.totalVotesSupply = _calculateVoteWeight(
            erc20VotingToken.totalSupply(),
            erc721VotingToken.totalSupply()
        );
        newPiece.totalERC20Supply = erc20VotingToken.totalSupply();
        newPiece.metadata = metadata;
        newPiece.sponsor = msg.sender;
        newPiece.creationBlock = block.number;
        newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000;

        for (uint i; i < creatorArrayLength; i++) {
            newPiece.creators.push(creatorArray[i]);
        }

        emit PieceCreated(pieceId, msg.sender, metadata, newPiece.quorumVotes, newPiece.totalVotesSupply);

        // Emit an event for each creator
        for (uint i; i < creatorArrayLength; i++) {
            emit PieceCreatorAdded(pieceId, creatorArray[i].creator, msg.sender, creatorArray[i].bps);
        }

        return newPiece.pieceId;
    }

• _vote() function

    function _vote(uint256 pieceId, address voter) internal {
        require(pieceId < _currentPieceId, "Invalid piece ID");
        require(voter != address(0), "Invalid voter address");
        require(!pieces[pieceId].isDropped, "Piece has already been dropped");
        require(!(votes[pieceId][voter].voterAddress != address(0)), "Already voted");

        uint256 weight = _getPastVotes(voter, pieces[pieceId].creationBlock);
        require(weight > minVoteWeight, "Weight must be greater than minVoteWeight");

        votes[pieceId][voter] = Vote(voter, weight);
        totalVoteWeights[pieceId] += weight;

        uint256 totalWeight = totalVoteWeights[pieceId];

        // TODO add security consideration here based on block created to prevent flash attacks on drops?
        maxHeap.updateValue(pieceId, totalWeight);
        emit VoteCast(pieceId, voter, weight, totalWeight);
    }

This vulnerability impacts the integrity of the voting mechanism in the CultureIndex.sol contract. The ability for newly minted ERC20 and ERC721 tokens to immediately influence the total vote weight of a piece within the same block undermines the accuracy and reliability of voting outcomes.

Proof of Concept

1. A new piece is created, and its voting parameters are set based on the current total supply of erc20VotingToken and erc721VotingToken.
2. Within the same block.timestamp, additional tokens of erc20VotingToken and erc721VotingToken are minted.
3. When a user votes for this piece, the newly minted tokens are included in the totalVoteWeights, even though they were not part of the original calculation for totalVotesSupply and quorumVotes.

Tools Used

Manual Inspection

Recommended Mitigation Steps

Implement a mechanism to lock in the total supply of voting tokens at the time of piece creation for the entire block. This ensures that tokens minted after the piece's creation within the same block do not affect the vote weight.

Assessed type

Context

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