Revolution Protocol - King_'s results

Lines of code

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

Vulnerability details

Impact

Art piece will have inflated quorum which might be impossible to reach

Proof of Concept

Consider the following POC

function testArtPieceCreatedWithExcessQuorumVotesRequirement() public {
        vm.stopPrank();
        //create art piece
        uint256 firstPieceId = voter1Test.createDefaultArtPiece();

        //Auction the created art piece (piece passes quorum)
        vm.startPrank(address(dao));
        auction.unpause();

        //verb token is minted for auction
        console2.log(erc721Token.totalSupply()); // 1


        //create new art piece
        uint256 secondPiece = voter1Test.createDefaultArtPiece();
        //quorum is 200000000000000000 as the newly minted verb token is included in 
        token total supply
        //used for calculating quorum
        console2.log(cultureIndex.getPieceById(secondPiece).quorumVotes); 
        //200000000000000000

        //auction ends with no bids and token is burned
        vm.warp(block.timestamp + auction.duration() + 1);
        auction.pause();
        auction.settleAuction();
        vm.stopPrank();

        //token supply is back to 0 since minted token is burned
        //but the previously created piece's quorum is still 200000000000000000 which is 
        impossible to reach currently
        console2.log(erc721Token.totalSupply()); // 0 

        console2.log(cultureIndex.getPieceById(secondPiece).quorumVotes); 
        //200000000000000000

        //subsequent pieces are created with correct quorum
        uint256 fourth = voter1Test.createDefaultArtPiece();
        console2.log(cultureIndex.getPieceById(fourth).quorumVotes); //0
    }

The above tests demonstrates that the second created piece has an inflated quorum which might be impossible to reach.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider subtracting the verb token balanceOf for the auction contract from the erc721Token total supply when a new art piece is being created (i.e taking a snapshot of all distributed tokens at art piece creation).

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

For all tokens created before the first successful auction the quorum votes required to be dropped and put for auction is 0 and can therefore be auctioned off without needing to pass any vote quorun requirement

Proof of Concept

function testFirstCreatedPiecesPassQuorumCheck() public {
        uint256 firstPieceId = voter1Test.createDefaultArtPiece();
        uint256 secondPieceId = voter2Test.createDefaultArtPiece();
        uint256 ThirdPieceId = voter1Test.createDefaultArtPiece();


        vm.startPrank(address(dao));
        auction.unpause();

        vm.warp(block.timestamp + auction.duration() + 1);
        auction.settleCurrentAndCreateNewAuction();
        vm.deal(address(11), 1 ether);
        vm.startPrank(address(11));
        auction.createBid{ value: 1 ether }(secondPieceId, address(11)); // Assuming first auction's verbId is 0
        vm.stopPrank();
        vm.warp(block.timestamp + auction.duration() + 1);
        auction.settleCurrentAndCreateNewAuction();
        vm.warp(block.timestamp + auction.duration() + 1);
        auction.settleCurrentAndCreateNewAuction();

        console2.log(erc721Token.totalSupply());
        

        uint256 pieceId = voter2Test.createDefaultArtPiece();
        console2.log(cultureIndex.getPieceById(pieceId).quorumVotes); //202166659585650220
    }

The above POC demonstrates that all the tokens minted before the first successful action can be dropped and auctioned off as verb tokens even though they have not been voted on by any users. It is also possible in extreme cases that the first "n" number of auctions is unsuccessful hence, no erc721 or erc20 tokens are disperced causing the total supply of tokens needed to calculate quorum votes required to evaluate to zero, therefore allowing all tokens created during that period to be dropped without quorum requirements.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider airdropping some erc20 voting tokens to some verified/trusted addresses before creating any art pieces in the culture index.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The current max heap implementation has some irregularities:

1. Size Limit not defined
2. Delete operation not implemented
3. Heap begins at index 0

Proof of Concept

1. The heap size is incremented on every insert, however there is no bound or limit put on the heap size which can cause it to grow indefinitely and therefore make insertions or heapify functions use up a large amount of gas or even the entire contract balance in extreme cases

2. The culture index adds an art piece to our max heap on every creation, these art pieces have to gain a number of votes before they can be dropped from the heap and minted, some art pieces might never gain enough votes and end up stuck in the heap for an indefinite amount of time, therefore taking up space in storage and making heap operations more expensive

3. According to traditional max heap implementations max heaps beginning at index 0 are more expensive to operate on because they include an extra find calculation for each side. As demonstrated by the table below

Recommended Mitigation Steps

1. Consider defining a size limit for max heaps/ creating more than 1 implementation
2. Consider defining a deadline for created art pieces to pass quorum and delete them from the max heap after expiration
3. increment the heap size variable before insertion into the max heap to ensure index starts at 1

Assessed type

Context