Revolution Protocol - 0xG0P1's results

Lines of code

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

Vulnerability details

Impact

The calculation for quorum votes will be incorrect in this case.

Proof of Concept

Assuming the following scenario:

1. An ongoing auction where bidders are actively bidding for an NFT with 2 days remaining.
2. Simultaneously, an art piece is created. Let's consider ERC20 totalSupply as 1000 tokens and ERC721 totalSupply as 10.

Assume erc721VotingTokenWeight is set to 20. The voting weight is calculated as erc20Balance + (erc721Balance * erc721VotingTokenWeight * 1e18), resulting in a value 'X' (e.g., 1000 + (10 * 20 * 1e18)).

Now, this 'X' is used to calculate quorum votes with the formula (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000. However, this approach is flawed. The ongoing auction holds a VerbToken each time, and these tokens are owned by the auction contract. Since the auction contract cannot execute the vote function in the CultureIndex contract, it disrupts the voting process. In the worst-case scenario, quorum votes might exceed the votes held by the voters. The actual voting weight should be calculated as erc20Balance + ((erc721Balance - 1) * erc721VotingTokenWeight * 1e18),

To address this issue, if an auction is in progress, the VerbTokens held by the Auction contract should be excluded when calculating quorum votes. This adjustment ensures a more accurate representation of voting power, preventing potential issues where quorum votes surpass the actual votes held by voters.

Tools Used

Manual review

Recommended Mitigation Steps

When the auction is ongoing, exclude the VerbTokens held by the Auction contract from the calculation of the voting weight (_calculateVoteWeight( erc20VotingToken.totalSupply(), erc721VotingToken.totalSupply() -1 );).

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

If someone mints tokens after an art piece is created, they can still vote for that piece.

Proof of Concept

Imagine the following scenario:

At block number X:

1. Bob mints 1000 tokens (ERC20 only, no ERC721 for simplicity).

At block number X + 1:

1. Bob mints an additional 1000 tokens. Now, Bob's total votes are 2000, and ERC20's total supply is 2000.
2. An art piece is created. Piece.quorumVotes are calculated as follows: (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000, where totalVotesSupply = _calculateVoteWeight(erc20VotingToken.totalSupply(), erc721VotingToken.totalSupply()). The quorum votes are calculated from the token total supply at the time of piece creation.
3. Bob mints 1000 more tokens in the same block that the piece is created.

In this scenario, although Bob's voting weight should be 2000 (up to piece creation), Bob's votes will be 3000. The vulnerability lies in the _getPastVotes() function called when the vote function is executed. The internal _vote function calculates the voting weight as follows: uint256 weight = _getPastVotes(voter, pieces[pieceId].creationBlock);. The issue is that _getPastVotes() returns the value at the end of the corresponding block (X + 1). Consequently, the quorum votes are calculated corresponding to Bob's 2000 votes (total supply at the time of piece creation), while the actual votes eligible for voting are 3000.

Test:

function testCreatePiece() public {
    address bob = address(5);
    
    vm.stopPrank();
    vm.startPrank(address(erc20TokenEmitter)); 
    erc20Token.mint(bob, 1000); // Minting 1000 tokens at X block
    vm.stopPrank();
    vm.roll(block.number + 1); // X + 1 block
    vm.startPrank(address(erc20TokenEmitter)); 
    erc20Token.mint(bob, 1000); // Minting another 1000 tokens at X + 1 block
    vm.stopPrank();
    uint256 newPieceId = createArtPiece( // Art piece created at X + 1 block
        "Mona Lisa",
        "A masterpiece",
        ICultureIndex.MediaType.IMAGE,
        "ipfs://legends",
        "",
        "",
        address(0x1),
        10000
    );
    vm.startPrank(address(erc20TokenEmitter)); // Minting another 1000 tokens at X + 1 block
    erc20Token.mint(bob, 1000);
    vm.stopPrank();
    vm.roll(block.number + 2); // X + 2 block for voting
    vm.startPrank(bob);
    cultureIndex.vote(newPieceId); // Bob voting for the Piece
    vm.stopPrank();

    // Validate that the piece was created with correct data
    ICultureIndex.ArtPiece memory createdPiece = cultureIndex.getPieceById(newPieceId);

    assertEq(createdPiece.pieceId, newPieceId);
    assertEq(createdPiece.metadata.name, "Mona Lisa");
    assertEq(createdPiece.metadata.description, "A masterpiece");
    assertEq(createdPiece.metadata.image, "ipfs://legends");
    assertEq(createdPiece.creators[0].creator, address(0x1));
    assertEq(createdPiece.creators[0].bps, 10000);
    uint Total = cultureIndex.totalVoteWeights(newPieceId); // Calculating the Total votes weight of the Piece
    assertEq(Total, 3000); // Asserting with Bob's Votes
}

Results:

Running 14 tests for test/culture-index/ArtPiece.t.sol:CultureIndexArtPieceTest
[PASS] testArtPieceCreationAndVoting(uint256,uint256) (runs: 256, μ: 1703438, ~: 1718588)
[PASS] testCreatePiece() (gas: 777916) //Passing Test
[PASS] testCreatePieceWithMultipleCreators() (gas: 420749)
[PASS] testCreatePieceWithMultipleCreatorsInvalidBPS() (gas: 20428)
[PASS] testCreatorArrayLengthConstraint() (gas: 101011)
[PASS] testExcessiveTotalBasisPoints() (gas: 19611)
[PASS] testFirstPieceId() (gas: 347058)
[PASS] testInvalidCreatorAddress() (gas: 18839)
[PASS] testInvalidMediaType() (gas: 19193)
[PASS] testMissingMediaDataAnimation() (gas: 20955)
[PASS] testMissingMediaDataImage() (gas: 20731)
[PASS] testMissingMediaDataText() (gas: 21069)
[PASS] testPieceIDIncrement() (gas: 664986)
[PASS] testTooFewTotalBasisPoints() (gas: 19083)
Test result: ok. 14 passed; 0 failed; 0 skipped; finished in 1.32s

Tools Used

Manual Review and Foundry

Recommended Mitigation Steps

Replace block.number with block.timestamp to record the creation time of an art piece instead of its block number. Then, retrieve past votes that occurred around the timestamp when the art piece was created .

Assessed type

Timing