Revolution Protocol - osmanozdemir1's results

Lines of code

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

Vulnerability details

While users buying governance tokens with ERC20TokenEmitter::buyToken function, some portion of the provided ETH is reserved for creators according to the creatorRateBps.

A part of this creator's reserved ETH is directly sent to the creators according to entropyRateBps, and the remaining part is used to buy governance tokens for creators.

That remaining part, which is used to buy governance tokens, is never sent to the DAO treasury. It is locked in the ERC20Emitter contract, causing value leaks for treasury in every buyToken function call.

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

    function buyToken(
        address[] calldata addresses,
        uint[] calldata basisPointSplits,
        ProtocolRewardAddresses calldata protocolRewardsRecipients
    ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
    // ...

        // 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;
        //Tokens to emit to creators
        int totalTokensForCreators = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0
            ? getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment)
            : int(0);

        // Tokens to emit to buyers
        int totalTokensForBuyers = toPayTreasury > 0 ? getTokenQuoteForEther(toPayTreasury) : int(0);

        //Transfer ETH to treasury and update emitted
        emittedTokenWad += totalTokensForBuyers;
        if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;

        //Deposit funds to treasury
-->     (bool success, ) = treasury.call{ value: toPayTreasury }(new bytes(0)); //@audit-issue Treasury is not paid correctly. Only the buyers share is sent. Creators share to buy governance tokens are not sent to treasury
        require(success, "Transfer failed.");                                   //@audit `creators total share` - `creatorDirectPayment` should also be sent to treasury. ==> Which is "((msgValueRemaining - toPayTreasury) - creatorDirectPayment)"

        //Transfer ETH to creators
        if (creatorDirectPayment > 0) {
            (success, ) = creatorsAddress.call{ value: creatorDirectPayment }(new bytes(0));
            require(success, "Transfer failed.");
        }

        // ... rest of the code
    }

In the code above:

toPayTreasury is the buyer's portion of the sent ether.
(msgValueRemaining - toPayTreasury) is the creator's portion of the sent ether.
((msgValueRemaining - toPayTreasury) - creatorDirectPayment) is the remaining part of the creator's share after direct payment (which is used to buy the governance token).

As we can see above, the part that is used to buy governance tokens is not sent to the treasury. Only the buyer's portion is sent.

Impact

• DAO treasury is not properly paid even though the corresponding governance tokens are minted.

• Every buyToken transaction will cause a value leak to the DAO treasury. The leaked ETH amounts are stuck in the ERC20TokenEmitter contract.

Proof of Concept

Coded PoC

You can use the protocol's own test suite to run this PoC.

-Copy and paste the snippet below into the ERC20TokenEmitter.t.sol test file.
-Run it with forge test --match-test testBuyToken_ValueLeak -vvv

function testBuyToken_ValueLeak() public {
        
        // Set creator and entropy rates.
        // Creator rate will be 10% and entropy rate will be 40%
        uint256 creatorRate = 1000;
        uint256 entropyRate = 5000;
        vm.startPrank(address(dao));
        erc20TokenEmitter.setCreatorRateBps(creatorRate);
        erc20TokenEmitter.setEntropyRateBps(entropyRate);

        // Check dao treasury and erc20TokenEmitter balances. Balance of both of them should be 0.
        uint256 treasuryETHBalance_BeforePurchase = address(erc20TokenEmitter.treasury()).balance;
        uint256 emitterContractETHBalance_BeforePurchase = address(erc20TokenEmitter).balance;
        
        assertEq(treasuryETHBalance_BeforePurchase, 0);
        assertEq(emitterContractETHBalance_BeforePurchase, 0);

        // Create token purchase parameters
        address[] memory recipients = new address[](1);
        recipients[0] = address(1);
        uint256[] memory bps = new uint256[](1);
        bps[0] = 10_000;

        // Give some ETH to user and buy governance token.
        vm.startPrank(address(0));
        vm.deal(address(0), 100000 ether);

        erc20TokenEmitter.buyToken{ value: 100 ether }(
            recipients,
            bps,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: address(0)
            })
        );

        // User bought 100 ether worth of tokens.
        // Normally with 2.5% fixed protocol rewards, 10% creator share and 50% entropy share: 
        //  ->  2.5 ether is protocol rewards.
        //  ->  87.75 ether is buyer share (90% of the 97.5)
        //  ->  9.75 of the ether is creators share
        //          - 4.875 ether directly sent to creators
        //          - 4.875 ether should be used to buy governance token and should be sent to the treasury.
        // However, the 4.875 ether is never sent to the treasury even though it is used to buy governance tokens. It is stuck in the Emitter contract. 

        // Check balances after purchase.
        uint256 treasuryETHBalance_AfterPurchase = address(erc20TokenEmitter.treasury()).balance;
        uint256 emitterContractETHBalance_AfterPurchase = address(erc20TokenEmitter).balance;
        uint256 creatorETHBalance_AfterPurchase = address(erc20TokenEmitter.creatorsAddress()).balance;

        // Creator direct payment amount is 4.875 as expected
        assertEq(creatorETHBalance_AfterPurchase, 4.875 ether);
        
        // Dao treasury has 87.75 ether instead of 92.625 ether. 
        // 4.875 ether that is used to buy governance tokens for creators is never sent to treasury and still in the emitter contract.
        assertEq(treasuryETHBalance_AfterPurchase, 87.75 ether);
        assertEq(emitterContractETHBalance_AfterPurchase, 4.875 ether);
    }

Results after running the test:

Running 1 test for test/token-emitter/ERC20TokenEmitter.t.sol:ERC20TokenEmitterTest
[PASS] testBuyToken_ValueLeak() (gas: 459490)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 11.25ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review, foundry

Recommended Mitigation Steps

I would recommend transferring the remaining ETH used to buy governance tokens to the treasury.

+       uint256 creatorsEthAfterDirectPayment = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment);

         //Deposit funds to treasury
-       (bool success, ) = treasury.call{ value: toPayTreasury }(new bytes(0));
+       (bool success, ) = treasury.call{ value: toPayTreasury + creatorsEthAfterDirectPayment }(new bytes(0));
        require(success, "Transfer failed.");

Assessed type

Math

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/CultureIndex.sol#L234

Vulnerability details

In this protocol, art pieces are uploaded, voted on by the community and auctioned. Being the highest-voted art piece is not enough to go to auction, and that art piece also must reach the quorum.

The quorum for the art piece is determined according to the total vote supply when the art piece is created. This total vote supply is calculated according to the current supply of the erc20VotingToken and erc721VotingToken. erc721VotingTokens have weight compared to regular erc20VotingTokens and ERC721 tokens give users much more voting power.

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

file: CultureIndex.sol
    function createPiece ...{
        // ...
        newPiece.totalVotesSupply = _calculateVoteWeight(
            erc20VotingToken.totalSupply(),
-->         erc721VotingToken.totalSupply() //@audit-issue This includes the erc721 token which is currently on auction. No one can use that token to vote on this piece.
        );
        // ...
-->     newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000; //@audit quorum votes will also be higher than it should be.
        // ...
    }
    

_calculateVoteWeight function:

    function _calculateVoteWeight(uint256 erc20Balance, uint256 erc721Balance) internal view returns (uint256) {
        return erc20Balance + (erc721Balance * erc721VotingTokenWeight * 1e18);
    }

As I mentioned above, totalVotesSupply and quorumVotes of an art piece are calculated when the art piece is created based on the total supplies of the erc20 and erc721 tokens.

However, there is an important logic/context issue here.
This calculation includes the erc721 verbs token which is currently on auction and sitting in the AuctionHouse contract. The voting power of this token can never be used for that art piece because:

1. AuctionHouse contract obviously can not vote.

2. The future buyer of this NFT also can not vote since users' right to vote is determined based on the creation block of the art piece.

In the end, totally inaccessible voting powers are included when calculating ArtPiece.totalVotesSupply and ArtPiece.quorumVotes, which results in incorrect quorum requirements and makes it harder to reach the quorum.

Impact

• Quorum vote requirements for created art pieces will be incorrect if there is an ongoing auction at the time the art piece is created.

• This will make it harder to reach the quorum.

• Unfair situations can occur between two art pieces (different totalVotesSuppy, different quorum requirements, but the same accessible/actual vote supply)

I also would like to that add the impact of this issue is not linear. It will decrease over time with the erc721VotingToken supply starts to increase day by day.

The impact is much higher in the early phase of the protocol, especially in the first days/weeks after the protocol launch where the verbsToken supply is only a handful.

Proof of Concept

Let's assume that:
-The current erc20VotingToken supply is 1000 and it won't change for this scenario.
-The weight of erc721VotingToken is 100.
-quorumVotesBPS is 5000 (50% quorum required)

Day 0: Protocol Launched

1. Users started to upload their art pieces.

2. There is no NFT minted yet

3. The total votes supply for all of these art pieces is 1000.

Day 1: First Mint

1. One of the art pieces is chosen.

2. The art piece is minted in VerbsToken contract and transferred to AuctionHouse contract.

3. The auction has started

4. erc721VotingToken supply is 1 at the moment.

5. Users keep uploading art pieces for the next day's auction.

6. For these art pieces uploaded on day 1:
   totalVotesSupply is 1100
   quorumVotes is 550
   Accessible vote supply is still 1000.

7. According to accessible votes, the quorum rate is 55% not 50.

Day 2: Next Day

1. The auction on the first day is concluded and transferred to the buyer.

2. The next verbsToken is minted and the auction is started.

3. erc721VotingToken supply is 2

4. Users keep uploading art pieces for the next day's auction.

5. For these art pieces uploaded on day 2:
   totalVotesSupply is 1200
   quorumVotes is 600
   Accessible vote supply is 1100. (1000 + 100 from the buyer of the first NFT)

6. The actual quorum rate for these art pieces is ~54.5% (600 / 1100).

*NOTE: The numbers used here are just for demonstration purposes. The impact will be much much higher if the erc721VotingToken weight is a bigger value like 1000. *

Tools Used

Manual review

Recommended Mitigation Steps

I strongly recommend subtracting the voting power of the NFT currently on auction when calculating the vote supply of the art piece and the quorum requirements.

// Note: You will also need to store auctionHouse contract address
in this contract.
+   address auctionHouse;

    function createPiece () {
    ...

        newPiece.totalVotesSupply = _calculateVoteWeight(
            erc20VotingToken.totalSupply(),
-           erc721VotingToken.totalSupply()
+           // Note: We don't subtract 1 as fixed amount in case of auction house being paused and not having an NFT at that moment. We only subtract if there is an ongoing auction. 
+           erc721VotingToken.totalSupply() - erc721VotingToken.balanceOf(auctionHouse)
        );

    ...
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L226-L229 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#L523

Vulnerability details

Creators create art pieces in this protocol, and users who have voting power can vote for these art pieces. The art piece with the highest vote goes to auction. Being the highest-voted art piece is not enough to go to auction, and that art piece also must reach the quorum.

The quorum for the art piece is determined according to the total vote supply when the art piece is created. Users' right to vote and voting power is also based on the creation block of the art piece.

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

file. CultureIndex.sol
    function createPiece(
        ArtPieceMetadata calldata metadata,
        CreatorBps[] calldata creatorArray
    ) public returns (uint256) {
        // ...
        
        newPiece.pieceId = pieceId;
-->     newPiece.totalVotesSupply = _calculateVoteWeight( //@audit-issue Total vote supply is fetched when it is created. What if next transactions in the same block buy voteToken?
            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; //@audit quorum votes is based on vote supply at the moment.

        // ...

        return newPiece.pieceId;
    }

As we can see above, totalVotesSupply for the art piece and the required quroumVotes are calculated based on the current supply of the erc20VotingToken and erc721VotingToken.

However, users' right to vote is tied to the block.number. This means that purchasing erc20VotingToken in the same block after the art piece is created gives the right to vote for that art piece, but doesn't increase totalVoteSupply and required quorumVotes.

Transaction orders in the same block affect the protocol in the current implementation, and creators can use it to gain an advantage.

Impact

• There will be a mismatch between ArtPiece.totalVotesSupply and the actual vote supply for that art piece.

• It will be easier to reach a quorum. Creators can easily use this for their own art pieces

Proof of Concept

Let's assume the current totalVoteSupply is 1000 and quorumVotesBPS is 5000 (50% votes required to reach quorum), and Alice is the creator.

Scenario 1:

1. Alice buys 200 votes tokens.

2. Alice creates an art piece.

3. totalVotesSupply for that art piece is 1200.

4. quroumVotes for that art piece is 600.

5. Alice votes 200 for her own art piece.

6. The remaining vote count to reach quorum is 400.

Scenario 2:

1. Alice creates an art piece and buys 200 votes tokens in the next transaction in the same block.

2. totalVotesSupply for that art piece is still 1000.

3. quroumVotes for that art piece is 500.

4. Alice can vote 200 for her own art piece as she has the right to vote due to buying in the same block.

5. The remaining vote count to reach quorum is 300.

The remaining vote count decreased by 25% and it is much easier to reach quorum.

Coded PoC

Down below you can find a basic PoC that demonstrates how transaction order in the same block affects the vote supply of that specific art piece.

-Copy and paste the snippet into the TopArtPiece.t.sol test file.
-Run it with forge test --match-test test_CheckArtPieceVoteSupplies_AccordingTo_TransactionOrder -vvv

function test_CheckArtPieceVoteSupplies_AccordingTo_TransactionOrder() public {
        // All actions below happens in the same block.
        // One of the art pieces is created before ERC20Votes token mint and the other art piece is created after token mint.
        
        // Create the first art piece
        uint256 firstPieceId = voter1Test.createDefaultArtPiece();

        // Mint vote tokens
        vm.stopPrank();
        vm.startPrank(address(erc20TokenEmitter));
        erc20Token.mint(address(voter1Test), 1000);

        // Create the second art piece
        uint256 secondPieceId = voter2Test.createDefaultArtPiece();

        // Check votes supply of these two art pieces
        ICultureIndex.ArtPiece memory firstPiece = cultureIndex.getPieceById(firstPieceId);
        ICultureIndex.ArtPiece memory secondPiece = cultureIndex.getPieceById(secondPieceId);
        
        assertEq(firstPiece.totalVotesSupply, 0);
        assertEq(secondPiece.totalVotesSupply, 1000);
    }

The test result of the test after running:

Running 1 test for test/culture-index/TopArtPiece.t.sol:CultureIndexArtPieceTest
[PASS] test_CheckArtPieceVoteSupplies_AccordingTo_TransactionOrder() (gas: 899207)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 12.53ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review, foundry

Recommended Mitigation Steps

Purchasing vote tokens in the same block right after creating the art piece should not be in favour of the creator. Therefore I would recommend using voting powers in the previous block instead of art piece creation block.

-        uint256 weight = _getPastVotes(voter, pieces[pieceId].creationBlock);
+        uint256 weight = _getPastVotes(voter, pieces[pieceId].creationBlock - 1);

However, this change alone is not enough because it opens up a new vulnerability (direct opposite of this one). This time an adversary can buy voting tokens just before art piece creation in the same block, increase the quorum vote requirement without getting voting power, and make it harder to reach quorum.

Therefore, total vote supply for an art piece should also be determined according to the previous block. However, the problem with that is we can't directly get the totalSupply of a token in the previous block. To overcome this issue, the protocol may need to use OpenZeppelin voting contracts' getPastTotalSupply method. The protocol can get total supplies of both ERC20 and ERC721 voting tokens in the previous block, and then calculate the total voting supply for an art piece considering the ERC721 voting weight.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L180 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L184 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L200-L215

Vulnerability details

Users can buy governance tokens with the ERC20TokenEmitter::buyToken() function. In this protocol, governance token prices are determined by a linear VRGDA calculation. The token price increases if the token supply is ahead of the schedule and decreases if it is behind the schedule.

There are also some other parameters I need to mention, which are creatorRateBps and entropyRateBps.

• creatorRateBps: The portion that goes to the creator anytime a token is bought.

• entropyRateBps: The portion of the creator's cut that is paid directly as ETH. The remaining part of the creator's cut is used to buy governance tokens for the creator.

For example, if the creator rate is 10%, the entropy rate is 50%, and a user wants to buy 100 ETH worth of tokens:
- Creator cut is 10 ETH
- 5 ETH will be sent directly to the creator
- The other 5 ETH will be used to buy governance tokens for the creator.
- The user will get 90 ETH worth of governance tokens.

This is the main logic in the ERC20TokenEmitter contract.

Now, let's examine the buyToken function:
https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L152C3-L230C6

    function buyToken(
        address[] calldata addresses,
        uint[] calldata basisPointSplits,
        ProtocolRewardAddresses calldata protocolRewardsRecipients
    ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
        // ... some code

        // 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
173.    uint256 toPayTreasury = (msgValueRemaining * (10_000 - creatorRateBps)) / 10_000;

        //Share of purchase amount to reserve for creators
        //Ether directly sent to creators
177.    uint256 creatorDirectPayment = ((msgValueRemaining - toPayTreasury) * entropyRateBps) / 10_000;
        //Tokens to emit to creators
179.      int totalTokensForCreators = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0
180.-->     ? getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment)
            : int(0);

        // Tokens to emit to buyers
184.--> int totalTokensForBuyers = toPayTreasury > 0 ? getTokenQuoteForEther(toPayTreasury) : int(0); //@audit-issue tokensForCreators and tokensForBuyers are calculated separately based on their proportional ether payments. This breaks VRGDA logic because these separate calculation are both made according to the current token supply. These two calculations should not be independent of each other

        //Transfer ETH to treasury and update emitted
        emittedTokenWad += totalTokensForBuyers;
        if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;

        //... rest of the code
        // funds are transferred and these amounts are minted.
    }

This function:

1. Calculates buyers' ETH share in line 173.

2. Calculates direct ETH payment to creators in line 177.

3. Calculates token amount to mint for creators with the remaining ETH after direct payment in lines 179-180 using getTokenQuoteForEther function.

4. Calculates token amount to mint for buyers in line 184 using getTokenQuoteForEther function

5. After all of these calculations, it updates emittedTokenWad parameter.

Now let's check the getTokenQuoteForEther function:

    function getTokenQuoteForEther(uint256 etherAmount) public view returns (int gainedX) {
        //...
        return
            vrgdac.yToX({
                timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime),
-->             sold: emittedTokenWad, //@audit it uses current state variable
                amount: int(etherAmount)
            });
    }

As we can see in this function, how many governance tokens will be minted is calculated according to the current supply, which is emittedTokenWad parameter.

Alright, what is the issue?

The issue is that both of the totalTokensForCreators and totalTokensForBuyers parameters are separately calculated like they are independent of each other, and the emittedTokenWad is updated after that. However, this situation breaks the VRGDA logic and more tokens are minted for the same amount of ETH.

These two calculations should not be independent. Total governance tokens to mint for the total ETH payment should be calculated first, and then the governance token amounts should be proportionally distributed.

Calculating proportional ETH amounts for the buyer and the creator first, and then determining the corresponding governance tokens to mint separately, leads to a higher total number of governance tokens to be minted compared to calculating the total governance tokens required based on the overall ETH amounts to be paid.

Impact

• Protocol mints more governance tokens are minted than it should be.

Proof of Concept

Coded PoC

Down below you can find a basic PoC that proves calculating tokens to mint separately results in many more tokens than calculating it only once with total payment.

You can use the protocol's own setup to run this PoC
-Copy and paste the snippet into the ERC20TokenEmitter.t.sol test file
-Run it with forge test --match-test testTokenQuoteForEther_is1plus1equal2 -vvv

    function testTokenQuoteForEther_is1plus1equal2() public {
        int256 tokenToGetFor100ether = erc20TokenEmitter.getTokenQuoteForEther(100e18);
        int256 tokenToGetFor500ether = erc20TokenEmitter.getTokenQuoteForEther(500e18);
        int256 tokenToGetFor600ether = erc20TokenEmitter.getTokenQuoteForEther(600e18);

        // Calculating mint amounts separately will result in minting much more tokens.
        assertGt(tokenToGetFor100ether + tokenToGetFor500ether, tokenToGetFor600ether);
        console2.log("current total: ", tokenToGetFor100ether + tokenToGetFor500ether);
        console2.log("supposed total: ", tokenToGetFor600ether);
    }

Results after running:

Running 1 test for test/token-emitter/ERC20TokenEmitter.t.sol:ERC20TokenEmitterTest
[PASS] testTokenQuoteForEther_is1plus1equal2() (gas: 41635)
Logs:
  current total:  586751802158813828000
  supposed total:  581798293495083372000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 41.60ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review, foundry

Recommended Mitigation Steps

There are two ways to resolve this issue. An unfair but much easier one, and the fair but not that easy one

Unfair One:
The protocol team should decide which side (creators or buyers) will suffer from the unfairness, and update the emittedTokenWad in the middle of two calculations. The latter calculation will buy tokens at a higher price.

In the example below, the change is in favour of the creators. Buyers get tokens with updated prices to keep the VRGDA logic intact.

        //Tokens to emit to creators
        int totalTokensForCreators = ((msgValueRemaining - toPayTreasury) - creatorDirectPayment) > 0
            ? getTokenQuoteForEther((msgValueRemaining - toPayTreasury) - creatorDirectPayment)
            : int(0);

+       // Moving this here will keep the VRGDA logic intact, and the calculation for buyers will be made with updated token supply.
+       if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;
        
        // Tokens to emit to buyers
        int totalTokensForBuyers = toPayTreasury > 0 ? getTokenQuoteForEther(toPayTreasury) : int(0);

        //Transfer ETH to treasury and update emitted
        emittedTokenWad += totalTokensForBuyers;
-       if (totalTokensForCreators > 0) emittedTokenWad += totalTokensForCreators;

Fair One:
This one requires a little more work but it is more fair. The function should:

1. Calculate the total ETH payment (creators + buyers) for the governance token purchase.

2. Calculate how many total governance tokens will be minted with this total ETH payment

3. Then mint proportional amounts to buyers and creators.

Example:

// Note: This is not diff. It is just an example

// Creators ETH payment for purchase
uint256 creatorsPayment = (msgValueRemaining - toPayTreasury) - creatorDirectPayment)

// Total ETH payment for purchase (toPayTreasury is already buyers payment)
uint256 totalPayment = toPayTreasury + creatorsPayment;

// Get total governance tokens to mint
int totalGovernanceTokenAmount = getTokenQuoteForEther(totalPayment);

// Calculate proportional governance token amounts.
// Note: I didn't check this for rounding. Care should be taken if this method will be implemented
int totalTokensForCreators = (totalGovernanceTokenAmount * creatorsPayment) / totalPayment;
int totalTokensForBuyers = (totalGovernanceTokenAmount * toPayTreasury) / totalPayment;

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The protocol is incompatible with EIP712 due to various reasons. Incorrect signatures might be created or some signatures may not be validated.

Proof of Concept

The protocol uses EIP-712 signatures when voting with signatures and it gets help from OpenZeppelin's EIP712Upgradeable contract. OpenZeppelin contract handles the domain separator and digest creation, but the CultureIndex contract itself should perform some of the hashing procedures before using the OpenZeppelin contract.

There are two separate things resulting in incorrect EIP712 signatures in this codebase.

The first one is that the struct definition and the struct typehash do not match. This is the VOTE_TYPEHASH in the CultureIndex.sol file:

    /// @notice The EIP-712 typehash for gasless votes
    bytes32 public constant VOTE_TYPEHASH =
        keccak256("Vote(address from,uint256[] pieceIds,uint256 nonce,uint256 deadline)"); //@audit-issue Vote structure is different.

And here is the Vote structure in the ICultureIndex.sol file:

    // Struct representing a voter and their weight for a specific art piece.
    struct Vote {
        address voterAddress;
        uint256 weight;
    }

The Vote struct itself and the Vote struct used while creating the VOTE_TYPEHASH are significantly different, which causes non-compliance with the EIP specification.

---

The second problem is inside of the _verifyVoteSignature function while creating voteHash (which is the hashStruct in EIP712)
https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L431

    function _verifyVoteSignature(
        address from,
        uint256[] calldata pieceIds,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) internal returns (bool success) {
        require(deadline >= block.timestamp, "Signature expired");

        bytes32 voteHash;
-->     voteHash = keccak256(abi.encode(VOTE_TYPEHASH, from, pieceIds, nonces[from]++, deadline)); //@audit "pieceIds" is an array. It should also be hashed separately.

        bytes32 digest = _hashTypedDataV4(voteHash);

        address recoveredAddress = ecrecover(digest, v, r, s);

Note: VOTE_TYPEHASH was already wrong as I mentioned above, but I will explain the second issue assuming it is correct. There is another implementation error in this line abi.encode(VOTE_TYPEHASH, from, pieceIds, nonces[from]++, deadline) based on the current VOTE_TYPEHASH.

According to EIP-712 hashStruct is defined as:

hashStruct(s : 𝕊) = keccak256(typeHash ‖ encodeData(s))

Data encoding while creating the hashStruct depends on which value type is used. There are atomic types, dynamic types, reference types etc.
https://eips.ethereum.org/EIPS/eip-712#definition-of-encodedata

Let's check the encoding in the codebase again:

voteHash = keccak256(abi.encode(VOTE_TYPEHASH, from, pieceIds, nonces[from]++, deadline)); //@audit "pieceIds" is an array. It should also be hashed separately.

In the codebase above from, nonces, and deadline is atomic types but the pieceIds is an array. Using it directly in the encoding is incorrect and breaks the EIP-712 specification. According to the EIP-712 specification, dynamic arrays need to be encoded as the keccak256 hash of the concatenation of the length of the array and the packed encoding of each element.

The array values are encoded as the keccak256 hash of the concatenated encodeData of their contents (i.e. the encoding of SomeType[5] is identical to that of a struct containing five members of type SomeType).

Due to these two reasons, this protocol incorrectly implements EIP712 signatures, and this may cause incorrect/invalid signatures and interoperability issues.

Tools Used

Manual review

Recommended Mitigation Steps

The solution to the first problem is either changing the Vote struct in the ICultureIndex, or creating another struct called VoteStructForSignatures which has the same structure used in the typehash.

// They have to exactly match.

-  keccak256("Vote(address from,uint256[] pieceIds,uint256 nonce,uint256 deadline)");
+  keccak256("VoteStructForSignatures(address from,uint256[] pieceIds,uint256 nonce,uint256 deadline)");

+    struct VoteStructForSignatures {
+        address from;
+        uint256[] pieceIds;
+        uint256 nonce;
+        uint256 deadline;
+    }

---

The solution for the second problem is:

    function _verifyVoteSignature ... {
        // ...

-        voteHash = keccak256(abi.encode(VOTE_TYPEHASH, from, pieceIds, nonces[from]++, deadline));
+        voteHash = keccak256(abi.encode(
                         VOTE_TYPEHASH, 
                         from, 
+                        keccak256(abi.encodePacked(pieceIds)), 
                         nonces[from]++, 
                         deadline
                     ));
       
    }

Assessed type

Error

Summary

• [L-01] Missing duplicate address checks in the CultureIndex::validateCreatorsArray() function can lead untrusted creators to steal from their collaborators

• [L-02] CultureIndex::validateMediaType doesn't validate the "audio" media type

• [L-03] A change in erc721VotingTokenWeight may create unfair situations for voting

• [L-04] The AuctionHouse contract should implement IERC721Receiver interface

• [N-01] Developer comments in the NontransferableERC20Votes contract are misleading

• [N-02] TokenEmitterRewards::_handleRewardsAndGetValueToSend doesn't have any form of NatSpec explanation

• [N-03] It would be better to document what happens if the top two art pieces have the same vote count in the MaxHeap

[L-01] Missing duplicate address checks in the CultureIndex::validateCreatorsArray() function can lead untrusted creators to steal from their collaborators

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

User-inputted creator arrays are validated via validateCreatorsArray function during the art piece creation. This function validates:

• Creators array doesn't have a zero address

• totalBps is 10_000

However, this function lacks a duplicate address check. The same addresses can be provided multiple times in that array.

This issue will probably not cause an issue if there are only a few creators.

But the maximum creator count is 100 in this protocol. In the case of a huge art collaboration with tens of creators, it is much easier to sneak the same address twice into the array.

Let's assume there is an art collaboration with more than 60 creators. One creator, who calls the createPiece() function, is malicious and others are not aware of it. This creator can put his address twice in the array and still match the exact 10000 bps by clipping minimal bps amounts from other creators.

Recommendation: Consider adding a check for duplicate address in the validateCreatorsArray function.

---

[L-02] CultureIndex::validateMediaType doesn't validate the "audio" media type

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

MediaType enum includes "image", "animation", "audio", "text" and "other"

CultureIndex::validateMediaType function only validates whether "image", "animation" or "text" media data are not empty but it never validates the "audio" type.

     /* Requirements:
     * - The media type must be one of the defined types in the MediaType enum.
-->  * - The corresponding media data must not be empty.
     */
    function validateMediaType(ArtPieceMetadata calldata metadata) internal pure {
        require(uint8(metadata.mediaType) > 0 && uint8(metadata.mediaType) <= 5, "Invalid media type");

        if (metadata.mediaType == MediaType.IMAGE)
            require(bytes(metadata.image).length > 0, "Image URL must be provided");
        else if (metadata.mediaType == MediaType.ANIMATION)
            require(bytes(metadata.animationUrl).length > 0, "Animation URL must be provided");
        else if (metadata.mediaType == MediaType.TEXT)
            require(bytes(metadata.text).length > 0, "Text must be provided");
    }

Recommendation:

struct ArtPieceMetadata {
        string name;
        string description;
        MediaType mediaType;
        string image;
        string text;
        string animationUrl;
+       string audioUrl;
    }
--------
+        else if (metadata.mediaType == MediaType.AUDIO)
+            require(bytes(metadata.audioUrl).length > 0, "Audio URL must be provided");

---

[L-03] A change in erc721VotingTokenWeight may create unfair situations for voting

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

Users vote on art pieces based on their voting powers when the art piece is created. Users' voting power in that block is fetched with _getPastVotes function.

This protocol has both ERC20 and ERC721 voting tokens and ERC721 voting token has a weight compared to ERC20. Users' voting power is calculated based on their token balances and this ERC721 weight.

//@audit this function checks votes in the past but according to today's current ERC721 weight. It should use the ERC721 weight in that past block's time
    function _getPastVotes(address account, uint256 blockNumber) internal view returns (uint256) {
        return
            _calculateVoteWeight(
                erc20VotingToken.getPastVotes(account, blockNumber),
                erc721VotingToken.getPastVotes(account, blockNumber)
            );
    }

----------------
    function _calculateVoteWeight(uint256 erc20Balance, uint256 erc721Balance) internal view returns (uint256) {
        return erc20Balance + (erc721Balance * erc721VotingTokenWeight * 1e18);
    }

As we can see above, user votes are calculated with the current erc721VotingTokenWeight. Even the votes in the past are also calculated according to today's erc721VotingTokenWeight.

This may create unfair situations if the erc721VotingTokenWeight is changed by the owner in the future since users can only vote once for the same art piece.

• Let's assume Alice and Bob have the same amount of erc20 and erc721 tokens at the time of block X and an art piece is created in that block.

• Alice voted for the art piece with her voting power based on erc721VotingTokenWeight.

• erc721VotingTokenWeight is updated.

• Bob voted for the art piece.

Normally they both should have the same voting power in the exact same block for the same art piece. However, the vote weights are not calculated according to the erc721VotingTokenWeight at the art piece creation block, they are calculated according to the current erc721VotingTokenWeight.

Recommendation: The protocol should track erc721VotingTokenWeight like checkpoints and use the erc721VotingTokenWeight at that block when fetching past votes in a specific block.

Note: This is only valid if the protocol plans to update erc721VotingTokenWeight in the future depending on the ERC20 vote token emission rate. The scenario above is not valid if this parameter is never going to be changed, but the parameter should be immutable if that's the case.

---

[L-04] The AuctionHouse contract should implement IERC721Receiver interface

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

contract AuctionHouse is
    IAuctionHouse,
    VersionedContract,
    UUPS,
    PausableUpgradeable,
    ReentrancyGuardUpgradeable,
    Ownable2StepUpgradeable
{

Every day a verbsToken is minted and transferred to the AuctionHouse contract. The art piece NFT is transferred to the buyer after the auction is concluded.

In the current implementation, verbsTokens are minted with the regular _mint method in the VerbsToken::_mintTo() function, not with the _safeMint method. That's why not implementing IERC721Receiver is not a problem in this case.

file: VerbsToken.sol
       function _mintTo(address to) internal returns(uint256) {
            //...
310.-->           _mint(to, verbId);
            // ...
       }

However, it is better to use _safeMint when minting NFTs to AuctionHouse, and in that case the AuctionHouse contract must implement IERC721Receiver interface.

---

[N-01] Developer comments in the NontransferableERC20Votes contract are misleading

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

     /*... 
-->    * By default, token balance does not account for voting power. This makes transfers cheaper. The downside is that it
       * requires users to delegate to themselves in order to activate checkpoints and have their voting power tracked.
     */

Developer comments on the NontransferableERC20Votes contract is directly copied from OpenZeppelin ERC20VotesUpgradeable.

However, tokens in this contract are non-transferable. So the comment "...This makes transfers cheaper..." is not correct for this contract and should be removed.

---

[N-02] TokenEmitterRewards::_handleRewardsAndGetValueToSend doesn't have any form of NatSpec explanation

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/protocol-rewards/src/abstract/TokenEmitter/TokenEmitterRewards.sol#L12C5-L21C6

    function _handleRewardsAndGetValueToSend(
        uint256 msgValue,
        address builderReferral,
        address purchaseReferral,
        address deployer
    ) internal returns (uint256) {
        if (msgValue < computeTotalReward(msgValue)) revert INVALID_ETH_AMOUNT();

        return msgValue - _depositPurchaseRewards(msgValue, builderReferral, purchaseReferral, deployer);
    }

Consider adding NatSpec comments for this function.

---

[N-03] It would be better to document what happens if the top two art pieces have the same vote count in the MaxHeap

MaxHeap contract keeps track of the vote counts of the art pieces in the form of a binary tree and is updated whenever an art piece is voted. The highest-voted art piece is dropped from the maxHeap contract and auctioned every day.

Currently, there is no documentation or explanation of what happens if there is an edge-case situation where the top two pieces have the same vote counts.

The behavior of the maxHeap contract in the current implementation is to keep the current top-voted art piece in its place in case the second one reaches the same vote count. The latter art piece must pass the current top-voted art piece to get the first place and having the same vote count is not enough.

The protocol should explain this behavior to its users beforehand and document it.

Note: I submitted it this way with the assumption that this is the intended behavior.
If the intention was to switch the top-voted piece when an art piece comes behind and reaches the same vote count, this means that the maxHeapify function doesn't work as intended.