Revolution Protocol - ast3ros's results

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/08ff070da420e95d7c7ddf9d068cbf54433101c4/packages/revolution/src/ERC20TokenEmitter.sol#L164-L177

Vulnerability details

Impact

Due to a flaw in the ERC20TokenEmitter contract, a portion of ETH used to buy governance tokens becomes irretrievably stuck in the contract. This represents a financial loss to the protocol and its participants.

Proof of Concept

When purchasing tokens from the ERC20TokenEmitter contract, the incoming ETH is allocated to protocol rewards, treasury, and creators

    // Get value left after protocol rewards
    uint256 msgValueRemaining = _handleRewardsAndGetValueToSend(
        msg.value,
        protocolRewardsRecipients.builder,
        protocolRewardsRecipients.purchaseReferral,
        protocolRewardsRecipients.deployer
    );

    //Share of purchase amount to send to treasury
    uint256 toPayTreasury = (msgValueRemaining * (10_000 - creatorRateBps)) / 10_000;

    //Share of purchase amount to reserve for creators
    //Ether directly sent to creators
    uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000;

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/08ff070da420e95d7c7ddf9d068cbf54433101c4/packages/revolution/src/ERC20TokenEmitter.sol#L164-L177

However, when entropyRateBps is greater than zero, not all of the remaining ETH is transferred to creators. Only a fraction is sent according to the following formula, leaving the remainder trapped in the contract.

    uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000;

POC:

• Navigate to : cd packages/revolution
• Create a test file test/EthStuck.t.sol
• Execute forge test -vvvvv --match-path test/EthStuck.t.sol --match-test testEthStuckInContract

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

import { Test } from "forge-std/Test.sol";
import {console} from "forge-std/console.sol";
import { AuctionHouseTest } from "./auction/AuctionHouse.t.sol";
import { IERC20TokenEmitter } from "../../src/interfaces/IERC20TokenEmitter.sol";

contract POCTest is AuctionHouseTest {
    function testEthStuckInContract() public {
        erc20TokenEmitter.setCreatorRateBps(1000);
        erc20TokenEmitter.setEntropyRateBps(3000);

        uint256 buyAmount = 10 ether;
        vm.deal(address(21), buyAmount);

        address[] memory recipients = new address[](1);
        recipients[0] = address(1);

        uint256[] memory bps = new uint256[](1);
        bps[0] = 10_000;
        vm.stopPrank();

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

        console.log("Contract balance: ", address(erc20TokenEmitter).balance); // 682500000000000000
        // Total input amount 10 ETH
        // Reward split: 2.5% total => 10 * 2.5% = 0.25 ETH (1)
        // Remainning: 10 - 0.25 = 9.75 ETH
        // toPayTreasury = 9.75 * (1 - creatorRateBps) = 9.75 * (1 - 10%) = 8.775 ETH (2)
        // creatorDirectPayment = (9.75 - 8.775) * entropyRateBps = (9.75 - 8.775) * 30% = 0.2925 ETH (3)
        // ETH left in the ERC20TokenEmitter contract: total - (1) - (2) - (3) =  10 - 0.25 - 8.775 - 0.2925 = 0.6825 ETH 
        assertEq(address(erc20TokenEmitter).balance, 682500000000000000);
    }
}

Tools Used

Manual

Recommended Mitigation Steps

Adjust the contract logic to handle cases where entropyRateBps is greater than zero. Specifically, the remaining ETH after creator payments should be transferred to the treasury account. This ensures that no ETH is left stranded in the contract.

        // Get value left after protocol rewards
        uint256 msgValueRemaining = _handleRewardsAndGetValueToSend(
            msg.value,
            protocolRewardsRecipients.builder,
            protocolRewardsRecipients.purchaseReferral,
            protocolRewardsRecipients.deployer
        );

        //Share of purchase amount to send to treasury
        uint256 toPayTreasury = (msgValueRemaining * (10_000 - creatorRateBps)) / 10_000;

        //Share of purchase amount to reserve for creators
        //Ether directly sent to creators
        uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000;
+       if (entropyRateBps > 0) {
+            toPayTreasury += (msgValueRemaining - toPayTreasury - creatorDirectPayment)
+       }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

There's a systemic bias against art pieces created earlier in the protocol's lifecycle. Due to the voting mechanism's design, these pieces may never be selected for auction, representing an inherent unfairness to early creators.

Proof of Concept

The voting power for an art piece is fixed at its creation time, based on the total voting power available in the system at that moment. As the protocol evolves and more voting tokens are minted or bought, later-created pieces inherently have a larger pool of potential votes.

    uint256 weight = _getPastVotes(voter, pieces[pieceId].creationBlock);
    require(weight > minVoteWeight, "Weight must be greater than minVoteWeight");

    votes[pieceId][voter] = Vote(voter, weight);
    totalVoteWeights[pieceId] += weight;

    uint256 totalWeight = totalVoteWeights[pieceId];

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

However, the maxHeap uses the absolute amount of vote weight to consider which pieceId has the highest vote.

    maxHeap.updateValue(pieceId, totalWeight);

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

The maxHeap structure uses the absolute amount of vote weight to determine the art piece with the highest votes. However, the total voting power in the system increases over time, allowing newer pieces to amass more votes than earlier ones.

A voter can easily forever block an old art piece from selected by voting for a new piece with the amount more than total potential received voting power of the old one.

Consider Scenario:

• Art piece A is created when the total possible voting weight is 10.
• Later, art piece B is created when the total possible voting weight is 20.
• A voter can cast 11 votes for art piece B, permanently blocking art piece A from ever being selected.

POC: In this POC, we have art piece id 1 blocked by art piece id 3. Art piece 1 is created at start then the total possible vote weight is 0. Art piece id 3 has 10e18 vote and block art piece 1 forever from being selected.

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

import { Test } from "forge-std/Test.sol";
import {console} from "forge-std/console.sol";
import { AuctionHouseTest } from "./auction/AuctionHouse.t.sol";
import { ICultureIndex, ICultureIndexEvents } from "../../src/interfaces/ICultureIndex.sol";

contract POCTest is AuctionHouseTest {
    function testBlockArtPiece() public {

        // Setup for first auction with 2 art piece ID 0 and 1 and 2
        uint256 verbId0 = createDefaultArtPiece();
        uint256 verbId1 = createArtPiece(
            "Mona Lisa 1",
            "A real masterpiece",
            ICultureIndex.MediaType.IMAGE,
            "ipfs://legends",
            "",
            "",
            address(0x2),
            10000
        );

        uint256 verbId2 = createArtPiece(
            "Mona Lisa 2",
            "A fake masterpiece",
            ICultureIndex.MediaType.IMAGE,
            "ipfs://legends",
            "",
            "",
            address(0x3),
            10000
        );
        

        (,,,,,,,uint256 totalVotesSupply)= cultureIndex.pieces(0);
        console.log("total vote supply of piece 1: ", totalVotesSupply);
        (,,,,,,,uint256 totalVotesSupply2)= cultureIndex.pieces(2);
        console.log("total vote supply of piece 2: ", totalVotesSupply2);
        vm.roll(block.number + 1);
        auction.unpause();

        // First auction start
        uint256 bidAmount = 100 ether;
        vm.deal(address(21), bidAmount + 2 ether);
        vm.stopPrank();

        vm.startPrank(address(21));
        auction.createBid{ value: bidAmount }(0, address(21));
        vm.stopPrank();
        (, , , uint256 endTime, , ) = auction.auction();
        vm.warp(endTime);
        auction.settleCurrentAndCreateNewAuction();
        
        vm.warp(endTime + 20);
        vm.roll(block.number + 20);

        // First auction end with settlement for id 0. ID 2 is selected to the next auction. ID 1 move to the root of the maxHeap as max value

        uint256 verbId3 = createArtPiece(
            "Mona Lisa 2",
            "A fake masterpiece",
            ICultureIndex.MediaType.IMAGE,
            "ipfs://legends",
            "",
            "",
            address(0x3),
            10000
        );

        vm.roll(block.number + 1);

        vm.startPrank(address(21));

        // Cannot vote for art piece 1 because the vote at block number 1 is 0
        console.log("Past vote at block number 1 for address 21: ", cultureIndex.getPastVotes(address(21), 1)); // 0
        vm.expectRevert("Weight must be greater than minVoteWeight");
        cultureIndex.vote(1);

        // However, voter can vote for art piece 3 because art piece 3 is created at block 22, and the vote at block 22 of user is more than 0
        cultureIndex.vote(3);
        vm.stopPrank();

        uint256 totalVoteWeights1= cultureIndex.totalVoteWeights(1);
        console.log("totalVoteWeights1: ", totalVoteWeights1); // 0 Vote
        uint256 totalVoteWeights3= cultureIndex.totalVoteWeights(3);
        console.log("totalVoteWeights3: ", totalVoteWeights3); // 10e18 vote

        // Art piece id 3 is the max item max Heap.
        (uint256 maxItemId, uint256 maxValue) = maxHeap.getMax();
        console.log("maxItemId: ", maxItemId); // maxItemId = 3
        console.log("maxValue: ", maxValue); // maxValue = 10e18
    }
}

Tools Used

Manual

Recommended Mitigation Steps

Consider allowing creators to "refresh" their art pieces in the maxHeap. This would enable them to reinsert their pieces into the voting pool, giving them access to the increased voting power available later in the protocol's lifecycle.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

This vulnerability allows for the minting and auctioning of an art piece that has not met the required quorum. It enables malicious voters to influence outcomes with fewer votes than what is stipulated by the protocol. This undermines a key invariant of the protocol:

    An art piece that has not met quorum cannot be dropped.

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/08ff070da420e95d7c7ddf9d068cbf54433101c4/README.md?plain=1#L291

Proof of Concept

The quorumVotes for an art piece are calculated at its creation as a fraction of the totalVotesSupply, which depends on the total supply of erc20VotingToken and erc721VotingToken:

    (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000.

File: src/CultureIndex.sol
209:     function createPiece(
210:         ArtPieceMetadata calldata metadata,
211:         CreatorBps[] calldata creatorArray
212:     ) public returns (uint256) {
213:         uint256 creatorArrayLength = validateCreatorsArray(creatorArray);
214: 
215:         // Validate the media type and associated data
216:         validateMediaType(metadata);
217: 
218:         uint256 pieceId = _currentPieceId++;
219: 
220:         /// @dev Insert the new piece into the max heap
221:         maxHeap.insert(pieceId, 0);
222: 
223:         ArtPiece storage newPiece = pieces[pieceId];
224: 
225:         newPiece.pieceId = pieceId;
226:         newPiece.totalVotesSupply = _calculateVoteWeight(
227:             erc20VotingToken.totalSupply(),
228:             erc721VotingToken.totalSupply()
229:         );
230:         newPiece.totalERC20Supply = erc20VotingToken.totalSupply();
231:         newPiece.metadata = metadata;
232:         newPiece.sponsor = msg.sender;
233:         newPiece.creationBlock = block.number;
234:         newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000;

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

The totalVotesSupply is calculated using total supply of erc20VotingToken and erc721VotingToken at the time the piece is created. It intends to calculate all the voting power that can vote for this art piece.

File: src/CultureIndex.sol
284:     function _calculateVoteWeight(uint256 erc20Balance, uint256 erc721Balance) internal view returns (uint256) {
285:         return erc20Balance + (erc721Balance * erc721VotingTokenWeight * 1e18);
286:     }

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

The vulnerability arises because the totalVotesSupply is computed based on the token supplies at the time of art piece creation. However, due to the block-based clock mode in the vote checkpoint, the total supplies of erc20VotingToken and erc721VotingToken can increase within the same block, resulting in an underestimation of totalVotesSupply and consequently, quorumVotes.

File: src/base/VotesUpgradeable.sol
85:     /**
86:      * @dev Clock used for flagging checkpoints. Can be overridden to implement timestamp based
87:      * checkpoints (and voting), in which case {CLOCK_MODE} should be overridden as well to match.
88:      */
89:     function clock() public view virtual returns (uint48) {
90:         return Time.blockNumber();
91:     }

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/08ff070da420e95d7c7ddf9d068cbf54433101c4/packages/revolution/src/base/VotesUpgradeable.sol#L85-L91

Possible attack scenarios include:

• A voter back-running the createPiece transaction and purchasing governance tokens in the same block, thereby artificially lowering the quorumVotes.
• A creator front-running a significant token purchase or auction settlement, leading to a similar underestimation of quorumVotes.

POC: This POC demonstrates the first case when a voter back-run the createPiece transaction to understate the totalVotesSupply and quorumVotes.

• Navigate to : cd packages/revolution
• Create a test file test/BypassQuorum.t.sol
• Execute forge test -vvvvv --match-path test/BypassQuorum.t.sol --match-test testBypassquorumVotes

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

import { Test } from "forge-std/Test.sol";
import {console} from "forge-std/console.sol";
import { AuctionHouseTest } from "./auction/AuctionHouse.t.sol";
import { IERC20TokenEmitter } from "../../src/interfaces/IERC20TokenEmitter.sol";

contract POCTest is AuctionHouseTest {

    function testBypassquorumVotes() public {

        uint256 verbId0 = createDefaultArtPiece();

        (,,,,uint256 creationBlock ,uint256 quorumVotes,,uint256 totalVotesSupply)= cultureIndex.pieces(0);
        console.log("creationBlock: ", creationBlock); // creationBlock:  1
        console.log("quorumVotes: ", quorumVotes); // quorumVotes:  0
        console.log("totalVotesSupply: ", totalVotesSupply); // totalVotesSupply:  0

        // Voter back-run the createPiece transaction and buy vote tokens in the same block, the supply is not reflected in the piece info and the quorum is understated at 0
        uint256 buyAmount = 100 ether;
        vm.deal(address(21), buyAmount);

        address[] memory recipients = new address[](1);
        recipients[0] = address(1);

        uint256[] memory bps = new uint256[](1);
        bps[0] = 10_000;
        vm.stopPrank();

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

        console.log("Should be quorum: ", cultureIndex.quorumVotes()); // Should be quorum:  1940052234587701020

    }
}

Tools Used

Manual

Recommended Mitigation Steps

When the piece is created, only store the creationBlock. The quorum should not be stored. It should be calculated directly using VotesUpgradeable.getPastTotalSupply, this will return the value at the end of the corresponding block.

It ensures that quorumVotes accurately reflects the voting power at the end of the block in which the art piece was created, thereby mitigating the risk of quorum bypass through token supply manipulation within the same block.

    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.");
+       uint256 totalVotesSupply = _calculateVoteWeight(
+               erc20VotingToken.getPastTotalSupply(piece.creationBlock),
+               erc721VotingToken.getPastTotalSupply(piece.creationBlock)
+            );
+       uint256 quorumVotes = (quorumVotesBPS * totalVotesSupply) / 10_000;
+       require(totalVoteWeights[piece.pieceId] >= quorumVotes, "Does not meet quorum votes to be dropped.");

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/08ff070da420e95d7c7ddf9d068cbf54433101c4/packages/revolution/src/AuctionHouse.sol#L248-L258

Vulnerability details

Impact

When entropyRateBps is set to 0 in the AuctionHouse contract, the _settleAuction function will always revert. This results in the inability of the auction house to settle auctions, effectively causing a Denial of Service (DoS) for this critical functionality. Consequently, the bid winner is unable to receive the NFT.

Proof of Concept

In the auctionHouse contract, entropyRateBps determines the proportion of (auction proceeds * creatorRate) that is allocated to the creator in ether, expressed in basis points.

File: src/AuctionHouse.sol
248:     /**
249:      * @notice Set the split of (auction proceeds * creatorRate) that is sent to the creator as ether in basis points.
250:      * @dev Only callable by the owner.
251:      * @param _entropyRateBps New entropy rate in basis points.
252:      */
253:     function setEntropyRateBps(uint256 _entropyRateBps) external onlyOwner {
254:         require(_entropyRateBps <= 10_000, "Entropy rate must be less than or equal to 10_000");
255: 
256:         entropyRateBps = _entropyRateBps;
257:         emit EntropyRateBpsUpdated(_entropyRateBps);
258:     }

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/08ff070da420e95d7c7ddf9d068cbf54433101c4/packages/revolution/src/AuctionHouse.sol#L248-L258

The contract allows for entropyRateBps to be zero, resulting in the entire auction proceeds being used to purchase governance tokens, with no direct ether transfer to the creators.

    uint256 ethPaidToCreators = 0;

    //Transfer creator's share to the creator, for each creator, and build arrays for erc20TokenEmitter.buyToken
    if (creatorsShare > 0 && entropyRateBps > 0) {
        for (uint256 i = 0; i < numCreators; i++) {
            ICultureIndex.CreatorBps memory creator = verbs.getArtPieceById(_auction.verbId).creators[i];
            vrgdaReceivers[i] = creator.creator;
            vrgdaSplits[i] = creator.bps;

            //Calculate paymentAmount for specific creator based on BPS splits - same as multiplying by creatorDirectPayment
            uint256 paymentAmount = (creatorsShare * entropyRateBps * creator.bps) / (10_000 * 10_000);
            ethPaidToCreators += paymentAmount;

            //Transfer creator's share to the creator
            _safeTransferETHWithFallback(creator.creator, paymentAmount);
        }
    }

    //Buy token from ERC20TokenEmitter for all the creators
    if (creatorsShare > ethPaidToCreators) {
        creatorTokensEmitted = erc20TokenEmitter.buyToken{ value: creatorsShare - ethPaidToCreators }(
            vrgdaReceivers,
            vrgdaSplits,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: deployer
            })
        );
    }

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/08ff070da420e95d7c7ddf9d068cbf54433101c4/packages/revolution/src/AuctionHouse.sol#L380-L409

With entropyRateBps == 0, the first conditional block (if (creatorsShare > 0 && entropyRateBps > 0)) is bypassed, leading to the potential issue in the subsequent block (if (creatorsShare > ethPaidToCreators)). Here, the vrgdaReceivers and vrgdaSplits arrays are not constructed, causing the erc20TokenEmitter.buyToken external call to revert.

POC:

• Navigate to : cd packages/revolution
• Create a test file test/EntropyRateZero.t.sol
• Execute forge test -vvvvv --match-path test/EntropyRateZero.t.sol --match-test testAuctionHouseRevertWhenEntropyRateBpsZero

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

import { Test } from "forge-std/Test.sol";
import { AuctionHouseTest } from "./auction/AuctionHouse.t.sol";

contract POCTest is AuctionHouseTest {
    function testAuctionHouseRevertWhenEntropyRateBpsZero() public {
        // Set entropy to 0
        auction.setEntropyRateBps(0);
        uint256 verbId = createDefaultArtPiece();
        auction.unpause();

        uint256 bidAmount = 100 ether;
        vm.deal(address(1), bidAmount + 2 ether);
        vm.stopPrank();

        vm.startPrank(address(1));
        auction.createBid{ value: bidAmount }(0, address(21));

        (, , , uint256 endTime, , ) = auction.auction();
        vm.warp(endTime);

        // Function is reverted because receiver address is 0
        vm.expectRevert(); // Error: ERC20InvalidReceiver(0x0000000000000000000000000000000000000000)
        auction.settleCurrentAndCreateNewAuction();
    }
}

Tools Used

Manual

Recommended Mitigation Steps

Handle the case when EntropyRateBps is 0 explicitly in the contract logic to ensures that even when entropyRateBps is 0, the vrgdaReceivers and vrgdaSplits arrays are properly populated:

        //Transfer creator's share to the creator, for each creator, and build arrays for erc20TokenEmitter.buyToken
        if (creatorsShare > 0 && entropyRateBps > 0) {
            for (uint256 i = 0; i < numCreators; i++) {
                ICultureIndex.CreatorBps memory creator = verbs.getArtPieceById(_auction.verbId).creators[i];
                vrgdaReceivers[i] = creator.creator;
                vrgdaSplits[i] = creator.bps;

                //Calculate paymentAmount for specific creator based on BPS splits - same as multiplying by creatorDirectPayment
                uint256 paymentAmount = (creatorsShare * entropyRateBps * creator.bps) / (10_000 * 10_000);
                ethPaidToCreators += paymentAmount;

                //Transfer creator's share to the creator
                _safeTransferETHWithFallback(creator.creator, paymentAmount);
            }
        }
+        
+       if (entropyRateBps == 0) {
+           for (uint256 i = 0; i < numCreators; i++) {
+               ICultureIndex.CreatorBps memory creator = verbs.getArtPieceById(_auction.verbId).creators[i];
+               vrgdaReceivers[i] = creator.creator;
+               vrgdaSplits[i] = creator.bps;
+           }
+       }
+
        //Buy token from ERC20TokenEmitter for all the creators
        if (creatorsShare > ethPaidToCreators) {
            creatorTokensEmitted = erc20TokenEmitter.buyToken{ value: creatorsShare - ethPaidToCreators }(
                vrgdaReceivers,
                vrgdaSplits,
                IERC20TokenEmitter.ProtocolRewardAddresses({
                    builder: address(0),
                    purchaseReferral: address(0),
                    deployer: deployer
                })
            );
        }

Assessed type

Error

[L-1] Using the maxHeap data structure leads to unfairness in art piece selection.

The current implementation of maxHeap introduces a bias in art piece selection, as elements in the left branch are favored when two nodes have identical values. This issue arises during the maxHeapify process, where the left child is preferred over the right, regardless of their insertion order.

It means that the art piece in the left branch will always be priortize in selection if two art pieces have the same voting amount even though the left one is inserted later than the right one.

    function maxHeapify(uint256 pos) internal {
        uint256 left = 2 * pos + 1;
        uint256 right = 2 * pos + 2;

        uint256 posValue = valueMapping[heap[pos]];
        uint256 leftValue = valueMapping[heap[left]];
        uint256 rightValue = valueMapping[heap[right]];

        if (pos >= (size / 2) && pos <= size) return;

        if (posValue < leftValue || posValue < rightValue) {
            if (leftValue > rightValue) {
                swap(pos, left);
                maxHeapify(left);
            } else {
                swap(pos, right);
                maxHeapify(right);
            }
        }
    }

POC: Put the test in the MaxHeap test: packages/revolution/test/max-heap/MaxHeap.t.sol

    function testLeftPriorityOverRight() public {
        maxHeap.insert(0, 17);
        maxHeap.insert(1, 11);
        maxHeap.insert(2, 16);
        maxHeap.insert(3, 12);
        maxHeap.insert(4, 13);
        maxHeap.insert(5, 16);
        (uint256 maxItemId, uint256 maxValue) = maxHeap.extractMax();
        console.log("maxItemId: ", maxItemId);
        console.log("maxValue: ", maxValue);
        // the item id 5 is the maximum over item id 2 even though item id 2 is inserted much earlier
        (maxItemId, maxValue) = maxHeap.getMax();
        console.log("maxItemId: ", maxItemId);
        console.log("maxValue: ", maxValue);
    }

Recommended Mitigation Steps

Document this unexpected behavior for users so they can aware and does not feel unfair.

[L-2] The hard-coded gas limit may not applicable in the future

The fixed gas limit of 50,000 in the _safeTransferETHWithFallback function lacks flexibility and may not align with future Ethereum network changes. A rigid gas limit can lead to failed transactions if the required gas exceeds this threshold due to change in Opcodes gas.

    assembly {
        // Transfer ETH to the recipient
        // Limit the call to 50,000 gas
        success := call(50000, _to, _amount, 0, 0, 0, 0)
    }

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/08ff070da420e95d7c7ddf9d068cbf54433101c4/packages/revolution/src/AuctionHouse.sol#L426-L430

Recommended Mitigation Steps

Allow admin to update the gas limit.

[L-3] The metadata input value is not sufficiently validated

When creating a new piece, the requirements for metadata are they must include name, description, and image. However, there is no input validation to make sure that the name, description and image are provided.

This oversight could result in incomplete or incorrect art piece registrations, as it currently doesn't enforce the presence of essential fields.

    function createPiece(
        ArtPieceMetadata calldata metadata,
        CreatorBps[] calldata creatorArray
    ) public returns (uint256) {
        uint256 creatorArrayLength = validateCreatorsArray(creatorArray);

        // Validate the media type and associated data
        validateMediaType(metadata);

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

Recommended Mitigation Steps

Add name, description and image validation in createPiece function.

[L-4] Rereview voting clockMode when deploying to L2 such as Optimism

The clockmode in voting calculation is defaulted to block.number. However, there can be potential issue when the block.number is determined differently in Optimism.

    function CLOCK_MODE() public view virtual returns (string memory) {
        // Check that the clock was not modified
        if (clock() != Time.blockNumber()) {
            revert ERC6372InconsistentClock();
        }
        return "mode=blocknumber&from=default";
    }

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/08ff070da420e95d7c7ddf9d068cbf54433101c4/packages/revolution/src/base/VotesUpgradeable.sol#L97-L103

Similar issue is reported: https://github.com/code-423n4/2023-06-lybra-findings/issues/114

[NC-1] Incorrect Natspec Documentation

The Natspec documentation for the getPastVotes function is misleading and incorrect. It currently describes the functionality of a different function (getVotes), which can cause confusion for developers and users of the code.

    /**
    * @notice Returns the voting power of a voter at the current block.
    * @param account The address of the voter.
    * @return The voting power of the voter.
    */
    function getPastVotes(address account, uint256 blockNumber) external view override returns (uint256) {
        return _getPastVotes(account, blockNumber);
    }

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/08ff070da420e95d7c7ddf9d068cbf54433101c4/packages/revolution/src/CultureIndex.sol#L269-L276

Recommended Mitigation Steps

Update the Natspec comments to accurately reflect the purpose and behavior of the getPastVotes function.