Revolution Protocol - pep7siup's results

Lines of code

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

Vulnerability details

Impact

The VerbsToken contract deviates from the ERC-721 standard, specifically in the tokenURI implementation. According to the standard, the tokenURI method must revert if a non-existent tokenId is passed. In the VerbsToken contract, this requirement was overlooked, leading to a violation of the EIP-721 specification and breaking the invariants declared in the protocol's README.

Proof of Concept

The responsibility for checking whether a token exists may be argued to be placed on the descriptor. However, the core VerbsToken contract, which is expected to adhere to the invariant stated in the Protocol's README, does not follow the specification.

// File: README.md
414:## EIP conformity
415:
416:- [VerbsToken](https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/VerbsToken.sol): Should comply with `ERC721`

Note: the original NounsToken contract, which VerbsToken was forked from, did implement the tokenURI function properly.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to strictly adopt the implementation from the original NounsToken contract to ensure compliance with the ERC-721 standard.

    function tokenURI(uint256 tokenId) public view override returns (string memory) {
+        require(_exists(tokenId));
        return descriptor.tokenURI(tokenId, artPieces[tokenId].metadata);
    }

References

1. EIP-721 Standard
2. Code 423n4 Finding - Caviar
3. Code 423n4 Finding - OpenDollar
4. NounsToken Contract Implementation

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

The vulnerability introduces a limitation on early minted art pieces, capping their influence to the snapshot weight of voters. In scenarios where a substantial number of votes are cast, and new tokens are minted before early art pieces reach quorum, these initial pieces become unable to compete effectively against newly minted ones. This reduces the competitive viability of early minted art pieces.

Proof of Concept

The root cause of the vulnerability is that early minted art pieces are restricted by the snapshot weight of voters, leading to their diminished influence compared to newly minted pieces. This results in a scenario where, despite potential artistic merit, early art pieces struggle to garner enough influence in the face of increased competition from newer submissions.

// File: packages/revolution/test/culture-index/TopArtPiece.t.sol
    function testEarlyMintedArtPieceCannotCompete() public {// @audit-issue M-earlyMintedArtPieceCannotCompete
        // Create first 2 Art pieces when erc20Token total supply is low (100 for voter1, 200 for voter2)
        uint256 firstPieceId = voter1Test.createDefaultArtPiece();
        uint256 secondPieceId = voter2Test.createDefaultArtPiece();

        // Mint tokens to the test contracts (acting as voters)
        vm.stopPrank();
        vm.startPrank(address(erc20TokenEmitter));

        erc20Token.mint(address(voter1Test), 100);
        erc20Token.mint(address(voter2Test), 200);

        vm.roll(block.number + 1); // roll block number to enable voting


        // Create 3rd Art piece when erc20Token total supply increases (100 for voter1, 200 + 21_000 for voter2)
        uint256 thirdPieceId = voter2Test.createDefaultArtPiece();

        erc20Token.mint(address(voter2Test), 21_000);
        vm.roll(block.number + 1); // roll block number to enable voting snapshot

        // Votes for all Art Pieces, max totalVoteWeights for each piece should be filled
        voter1Test.voteForPiece(firstPieceId);
        voter1Test.voteForPiece(secondPieceId);
        voter1Test.voteForPiece(thirdPieceId);

        voter2Test.voteForPiece(firstPieceId);
        voter2Test.voteForPiece(secondPieceId);
        voter2Test.voteForPiece(thirdPieceId);

        assertEq(cultureIndex.topVotedPieceId(), thirdPieceId, "Third piece should now be top-voted");

        // Assess max totalVoteWeights capacity for each Art Piece
        emit log_string("max votes for firstPieceId");
        emit log_uint(cultureIndex.totalVoteWeights(firstPieceId));
        emit log_string("max votes for secondPieceId");
        emit log_uint(cultureIndex.totalVoteWeights(secondPieceId));
        emit log_string("max votes for thirdPieceId");
        emit log_uint(cultureIndex.totalVoteWeights(thirdPieceId));
    }

The following test results show that those early created Art Piece can ONLY reach upto 300 votes due to the limitation of snapshot weight from voters. That forces their Art Pieces out of the contest for any incoming auctions, potentially compromises the AuctionHouse fairness.

FOUNDRY_PROFILE=dev forge test -vv --mt testEarlyMintedArtPieceCannotCompete

Running 1 test for test/culture-index/TopArtPiece.t.sol:CultureIndexArtPieceTest [PASS] testEarlyMintedArtPieceCannotCompete() (gas: 1900697) Logs: max votes for firstPieceId 300 max votes for secondPieceId 300 max votes for thirdPieceId 21300

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.38ms

Tools Used

Foundry test

Recommended Mitigation Steps

It is recommended to reevaluate the snapshot weight mechanism for early minted art pieces. Adjustments should be made to ensure that these pieces can maintain competitive viability, even in the presence of a large number of votes and newly minted tokens. Implementing a dynamic snapshot weight calculation that considers both the historical and current voting landscape could address this vulnerability.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

The vulnerability introduces a risk of item removal and resurrection at the same time in the heap due to the last element not being properly reset after extractMax. In the event of a future update involving downvoting or unvoting, this unreset last element could impact the property of the MaxHeap, effectively compromising the AuctionHouse core functionality.

According to the Severity Standardization - Speculation on Future Code, this issue validity could be reasonably considered.

Proof of Concept

The root cause of the vulnerability is that the heap's last element is not cleared after extractMax was called, allowing the rightValue variable in subsequent calls to maxHeapify to read the value the the uncleared position.

// File: packages/revolution/src/MaxHeap.sol
94:    function maxHeapify(uint256 pos) internal {
95:        uint256 left = 2 * pos + 1;
96:        uint256 right = 2 * pos + 2; // <= FOUND
97:
98:        uint256 posValue = valueMapping[heap[pos]];
99:        uint256 leftValue = valueMapping[heap[left]];
100:        uint256 rightValue = valueMapping[heap[right]]; // <= FOUND
101:
102:        if (pos >= (size / 2) && pos <= size) return; // <= FOUND: is size is even, the value of the right leaf can still be read despite it being out of range
...
113:    }

If an updateValue was called to an item at position parent(size) occurs, reducing its value to lower than the out-of-range valueMapping[heap[size]], the updated item will be downward heapified to the invalid size position instead of the expected size - 1 position. This can lead to the resurrection of the item into the heap, potentially causing two versions of the same itemID to exist in the heap.

POC

Run the POC with cd packages/revolution && FOUNDRY_PROFILE=dev forge test -vvv --mc MaxHeapUpdateTestSuite --mt testResurectHeap. The PASS result will confirm that if the future udpateValue to a lower votes is realized, the resurrection issue will occur causing undesired impacts such as (1) a targeted Art Piece was forced to be removed from the heap or (2) a duplication of the Art Piece, which was previously located at the end of the Heap, occurs.

// File: packages/revolution/test/max-heap/Updates.t.sol
    function testResurectHeap() public {// @audit-issue M-unresetLastElementId
        maxHeap.insert(0, 80);
        maxHeap.insert(1, 50);
        maxHeap.insert(2, 40);
        maxHeap.insert(3, 35);
        maxHeap.insert(4, 20);
        maxHeap.insert(5, 10);
        maxHeap.insert(6, 11);

        (uint256 maxItemId, uint256 maxValue) = maxHeap.extractMax();
        assertEq(maxItemId, 0, "Item ID should be 0 after heapify");
        assertEq(maxValue, 80, "Value should be 80 after heapify");
        (maxItemId, maxValue) = maxHeap.getMax();
        assertEq(maxItemId, 1, "New max piece ID should be 1");
        assertEq(maxValue, 50, "New max value should be 50");

        // Verify the last item before extractMax is not cleared, itemId 6 can be read
        assertEq(maxHeap.heap(6), 6, "Heap item 6 cleared");

        // Lower the value of itemId 2 (parent of 6) to 10 (less than 11), effectively down-heapify it to position 6 which is out of the heap's valid range
        maxHeap.updateValue(2, 10);

        // Impact 1: itemId 2 effectively removed from the heap, since it was swapped to position 6
        assertEq(maxHeap.heap(6), 2, "Heap itemId 2 not moved down to pos 6");
        assertEq(maxHeap.valueMapping(maxHeap.heap(6)), 10, "Heap item 2 not moved down to pos 6");

        // Impact 2: Heap itemId 6 swapped to pos 2. More severely, itemId 6 exists at 2 positions 2 and 3
        assertEq(maxHeap.heap(2), 6, "Heap itemId 6 not resurect to pos 2");
        assertEq(maxHeap.valueMapping(maxHeap.heap(2)), 11, "Heap item 6 not moved up to pos 2");
        assertEq(maxHeap.heap(2), maxHeap.heap(3), "Heap itemId 6 not duplicated at pos[2, 3]");
    }

Illustration

Before extractMax:

        80
      /    \
    50      40
   /  \    /  \
  35   20 10   11

This is the initial state of the max heap represented as a tree. The root has the value 80, and the children are arranged such that each node is greater than or equal to its children.

Now, let's go through the steps of the extractMax function:

1. Pop the root (80).
2. Replace the root with the last element (11).

After extractMax:

        50
      /    \
    35      40
   /  \    /  \
  11   20 10   11 (uncleared)

After updateValue(2, 10):

        50
      /    \
    35      11 (itemId 6 resurrected here, duplication occured)
   /  \    /  \
  11   20 10   10 (itemId 2 was swapped to here)

Tools Used

Foundry test

Recommended Mitigation Steps

It is recommended to add a line to reset the heap[size] to an out-of-range ID after the extraction. This ensures that the last element is properly reset, mitigating the risk of item resurrection in the heap in the future.

    function extractMax() external onlyAdmin returns (uint256, uint256) {
        require(size > 0, "Heap is empty");

        uint256 popped = heap[0];
        heap[0] = heap[--size];
+        heap[size] = type(uint256).max;
        maxHeapify(0);

        return (popped, valueMapping[popped]);
    }

Assessed type

Other