search

code-423n4 / 2023-12-revolutionprotocol-findings

3 stars 2 forks source link

Anyone can pause AuctionHouse in _createAuction #195

Open c4-bot-9 opened 10 months ago

c4-bot-9 commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L328 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/AuctionHouse.sol#L311-L313 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/VerbsToken.sol#L292-L310

Vulnerability details

Any user can call AuctionHouse::settleCurrentAndCreateNewAuction in order to settle the old auction and create a new one. settleCurrentAndCreateNewAuction will call _createAuction, which code is shown below:

    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) {
            uint256 startTime = block.timestamp;
            uint256 endTime = startTime + duration;

            auction = Auction({
                verbId: verbId,
                amount: 0,
                startTime: startTime,
                endTime: endTime,
                bidder: payable(0),
                settled: false
            });

            emit AuctionCreated(verbId, startTime, endTime);
        } catch {
            _pause();
        }
    }

As we can see, it will try to mint an NFT, but if it fails, it will execute the catch clause. Sponsors are already aware that catch block would also catch deliberate Out Of Gas exceptions and that is why require(gasleft() >= MIN_TOKEN_MINT_GAS_THRESHOLD) is present in the code (MIN_TOKEN_MINT_GAS_THRESHOLD = 750_000). However, it is still possible to consume a lot of gas in verbs.mint and force the catch clause to execute, and, in turn, pause the contract.

In order to see it, let's look at the code of VerbsToken::mint:

    function mint() public override onlyMinter nonReentrant returns (uint256) {
        return _mintTo(minter);
    }

It will call _mintTo:

    function _mintTo(address to) internal returns (uint256) {
       [...]
        // Use try/catch to handle potential failure
        try cultureIndex.dropTopVotedPiece() returns (ICultureIndex.ArtPiece memory _artPiece) {
            artPiece = _artPiece;
            uint256 verbId = _currentVerbId++;

            ICultureIndex.ArtPiece storage newPiece = artPieces[verbId];

            newPiece.pieceId = artPiece.pieceId;
            newPiece.metadata = artPiece.metadata;
            newPiece.isDropped = artPiece.isDropped;
            newPiece.sponsor = artPiece.sponsor;
            newPiece.totalERC20Supply = artPiece.totalERC20Supply;
            newPiece.quorumVotes = artPiece.quorumVotes;
            newPiece.totalVotesSupply = artPiece.totalVotesSupply;

            for (uint i = 0; i < artPiece.creators.length; i++) {
                newPiece.creators.push(artPiece.creators[i]);
            }

            _mint(to, verbId);

            [...]
    }

As can be seen, it performs some costly operations in terms of gas:

Although all above mentioned operations could be used in order to cause OOG exception, I will focus only on the third one - as I will show in the PoC section, if 10 creators are specified (which is a reasonable number) and even a small art piece (containing only a short URL and short description), the attack is possible as pushes will consume over 750 000 gas. It shows that the attack can be performed very often - probably in case of majority of auctions that are ready to be started (as many of art pieces will have several creators or slightly longer descriptions).

Impact

Malicious users will be able to often pause the AuctionHouse contract - they don't even need for their art piece to win the voting as the attack will be likely possible even on random art pieces.

Of course, the owner (DAO) will still be able to unpause the contract, but until it does so (the proposal would have to be first created and voted, which takes time), the contract will be paused and impossible to use. The upgrade of AuctionHouse contract will be necessary in order to recover.

Proof of Concept

The test below demonstrates how an attacker can cause _createAuction to execute catch clause causing AuctionHouse to pause itself when it shouldn't as there is another art piece waiting to be auctioned.

Please put the following test into AuctionSettling.t.sol and run it:

    // create an auction with a piece of art with given number of creators and finish it
    function _createAndFinishAuction() internal
    {
        uint nCreators = 10;
        address[] memory creatorAddresses = new address[](nCreators);
        uint256[] memory creatorBps = new uint256[](nCreators);
        uint256 totalBps = 0;
        address[] memory creators = new address[](nCreators + 1);
        for (uint i = 0; i < nCreators + 1; i++)
        {
            creators[i] = address(uint160(0x1234 + i));
        }

        for (uint256 i = 0; i < nCreators; i++) {
            creatorAddresses[i] = address(creators[i]);
            if (i == nCreators - 1) {
                creatorBps[i] = 10_000 - totalBps;
            } else {
                creatorBps[i] = (10_000) / (nCreators - 1);
            }
            totalBps += creatorBps[i];
        }

        // create the initial art piece
        uint256 verbId = createArtPieceMultiCreator(
            "Multi Creator Art",
            "An art piece with multiple creators", 
            ICultureIndex.MediaType.IMAGE,
            "ipfs://multi-creator-art",
            "",
            "",
            creatorAddresses,
            creatorBps
        );

        vm.startPrank(auction.owner());
        auction.unpause();
        vm.stopPrank();

        uint256 bidAmount = auction.reservePrice();
        vm.deal(address(creators[nCreators]), bidAmount + 1 ether);
        vm.startPrank(address(creators[nCreators]));
        auction.createBid{ value: bidAmount }(verbId, address(creators[nCreators]));
        vm.stopPrank();

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

        // create another art piece so that it's possible to create next auction
        createArtPieceMultiCreator(
            "Multi Creator Art",
            "An art piece with multiple creators", 
            ICultureIndex.MediaType.IMAGE,
            "ipfs://multi-creator-art",
            "",
            "",
            creatorAddresses,
            creatorBps
        );
    }

    function testDOS4() public
    {
        vm.startPrank(cultureIndex.owner());
        cultureIndex._setQuorumVotesBPS(0);
        vm.stopPrank();

        _createAndFinishAuction();

        assert(auction.paused() == false);
        // 2 900 000 will be enough to perform the attack
        auction.settleCurrentAndCreateNewAuction{ gas: 2_900_000 }();
        assert(auction.paused());
    }

Tools Used

VS Code

Recommended Mitigation Steps

Preventing OOG with MIN_TOKEN_MINT_GAS_THRESHOLD will not work as, I have shown before, there are at least 3 ways for attackers to boost the amount of gas used in VerbsToken::mint.

In order to fix the issue, consider changing catch to catch Error(string memory) that will not catch OOG exceptions.

Assessed type

DoS

c4-pre-sort commented 10 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #93

c4-pre-sort commented 10 months ago

raymondfam marked the issue as not a duplicate

c4-pre-sort commented 10 months ago

raymondfam marked the issue as primary issue

c4-sponsor commented 10 months ago

rocketman-21 (sponsor) confirmed

c4-judge commented 10 months ago

MarioPoneder marked the issue as satisfactory

MarioPoneder commented 10 months ago

Warden identified 3 main gas griefing attack vectors:

As can be seen, it performs some costly operations in terms of gas:

  • cultureIndex.dropTopVotedPiece() may be costly if there are many art pieces in the MaxHeap - height of MaxHeap is O(log n), where n is the number of elements, but dropTopVotedPiece may iterate over the entire height and each time it will read and write storage memory, which is expensive
  • newPiece.metadata = artPiece.metadata; can be very costly when artPiece.metadata is big (currently, it can have an arbitrary size, so this operation may be very costly)
  • newPiece.creators.push(artPiece.creators[i]); is writing to storage in a loop, which is expensive as well

Although all above mentioned operations could be used in order to cause OOG exception, ...

With a subsequent impact of pausing the AuctionHouse.

Issues which partially identified the underlying core issues will be awarded partial credit.

Edit: Going through the duplicates, I saw many self-duplications.
Assuming the wardens will be dissatisfied with the duplications and will comment during PJQA, I want to refer to the following part of our C4 SC verdicts in order to avoid unnecessary discussions during PJQA:

https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-similar-exploits-under-a-single-issue

Verdict: Similar exploits under a single issue The findings are duplicates if they share the same root cause. More specifically, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates. Given the above, when similar exploits would demonstrate different impacts, the highest, most irreversible would be the one used for scoring the finding. Duplicates of the finding will be graded based on the achieved impact relative to the Submission Chosen for Report.

c4-judge commented 10 months ago

MarioPoneder marked the issue as selected for report

bart1e commented 9 months ago

@MarioPoneder , first of all, thank you for your time for judging this issue. However, I believe that all duplicated issues (including this one) fall into 3 different categories instead of 1. These categories are:

  1. Pausing AuctionHouse by deliberately causing OOG exception.
  2. DoSing AuctionHouse by specifying many creators (possibly with griefing receive functions).
  3. DoSing AuctionHouse by creating art piece with heavy metadata.

I will try to explain the differences by going through my issues which were duplicated with this one:

Now, I will explain why 1. category is different from 2. and 3. and then I will show why 2. and 3. are different. In order to do that, I will show that fixing any of these issues will not fix the remaining 2 and that even fixing any two of them will not fix the third one.

The root cause of the issue described in this submission is a possibility to exploit the logic related to MIN_TOKEN_MINT_GAS_THRESHOLD as the users can bypass the check: require(gasleft() >= MIN_TOKEN_MINT_GAS_THRESHOLD) and cause OOG. In order to do that, an attacker has to be able to spend considerable amount of gas. However, even if:

then this issue will still exist. In fact, I anticipated that my issues may be judged as duplicates and that is why I wrote the PoC for this issue that uses 10 creators and a very small art piece. It proves that limiting art pieces' size and number of creators doesn't solve this problem.

Now, solving this problem will not fix the remaining two. It's because the problem cannot be fixed by limiting number of creators or art piece size (as I have shown in the PoC), so the fix has to be different. I believe that the only logical fix is the solution I presented in this submission, which is to simply not catch OOG in the catch statement. Even limiting every possible metric (art size, num of creators, max heap size, etc.) and increasing MIN_TOKEN_MINT_GAS_THRESHOLD does not solve the issue as cost of EVM operations can change in the future, so even if a solution works today, it may not work tomorrow. And during each contracts upgrade, this limit (already hard to set as it depends on many different things) would have to be set to a different value. Hence I believe that the only reasonable fix here is to not catch OOG and not catching OOG here clearly doesn't solve problems 1. and 2.

Now, I will show why 1. is different than 2.: Similarly to this submission, in #182, I deliberately used small art piece in the PoC and block gas limit is still surpassed. The same is true for #178 as the PoC uses only 1 creator and even assumes that the "loop" issue from #182 is fixed (this is even more strict than only limiting the number of creators). This shows fixing one of these issues does not influence another, which means that they have different root causes.

In fact all 3 issues are independent of each other and even fixing any two of them will not fix the third one (I think it's obvious now, so I don't have to elaborate more on this).

I would like ask you to review my arguments for this matter.

NicolaMirchev commented 9 months ago

Hey,

mcgrathcoutinho commented 9 months ago

Hi @MarioPoneder, I believe this issue should be QA.

Here's why:

  1. In the README here, the minimum duration after which the team said they would consider a DOS finding to be valid is 20 minutes. In this case, the admin can just unpause the contract when such a situation occurs.
  2. The attacker has no benefit in performing this DOS attack as it is a waste of gas fees and as such non-sensical to even execute due to waste of resources.
bart1e commented 9 months ago

Please consider removing all issues duplicates of "newPiece.creators.push(artPiece.creators[i]); is writing to storage in a loop, which is expensive as well". The root cause of these issues is the external call within a for loop iterating over an unbound array, which was previously reported by bots.

~In fact almost every report that was about too many creators allowed implicitly states this issue. If it was fixed, it wouldn't be possible at the moment to reach block gas limit as I have shown at #182.~ Edit: I was talking about an issue of verbs.getArtPieceById called in loop, not the loop mentioned by the Warden. Sorry for confusion.

Regarding bot submission, I have written this in #182:

I argue that this finding is different from the bot finding ([L-02] For loops in public or external functions should be avoided due to high gas costs and possible DOS) for the following reasons:

  • _settleFunction is neither external nor public
  • bot didn't mention this function as an instance of that issue
  • most importantly, I argue that Sponsors were already aware of a potential DoS when calling external functions in a loop and this is precisely the reason why they limited the number of creators to 100, hence the bot finding wouldn't cause them to fix the issue described in my submission as it was probably already considered as "fixed" by the limit imposed on the number of creators

So, I would like to ask @MarioPoneder to take these arguments into account. It's easy to ignore a low severity finding stating obvious things, but if someone shows that it's of High severity the situation is different.

Concerns about the cultureIndex.dropTopVotedPiece() function being costly if there are many art pieces in the MaxHeap. It's important to note that the height of the MaxHeap is O(log n), where 'n' is the number of elements. While this function may iterate over the entire height, each iteration will read and write storage memory, which can be expensive. However, it's worth mentioning that this cost is mathematically low. For example, with 1,000,000 elements, only 17 iterations would be made due to the nature of the heap, making the impact relatively manageable

While I 100% agree that only because a lot of elements in the MaxHeap block gas limit will not be achieved (and I haven't stated that anywhere), it is still one of the factors why it's possible to exploit "MIN_TOKEN_MINT_GAS_THRESHOLD mechanism" and I wrote this in this particular context. Extracting max will not achieve 30 000 000 gas, but it may achieve MIN_TOKEN_MINT_GAS_THRESHOLD which is 750_000 hence may be used for this particular attack.

Here's why:

  1. In the README here, the minimum duration after which the team said they would consider a DOS finding to be valid is 20 minutes. In this case, the admin can just unpause the contract when such a situation occurs.
  2. The attacker has no benefit in performing this DOS attack as it is a waste of gas fees and as such non-sensical to even execute due to waste of resources.

Regarding 1.: As far as I am concerned this contract is governed by a DAO. So, in order to unpause the contract, the voting must be done and then, the timelock will add another time to the process, so definitely 20 minutes will be exceeded. Even if this wasn't the case and it would have been possible to unpause at any moment, it would still be very annoying for the owner.

Regarding 2.: It is a griefing attack, yes. But it doesn't mean it isn't of Medium severity. And it is important for the Sponsors as they asked us to look for ways to DoS the protocol:

So - focusing on ways in which the CultureIndex -> VerbsToken -> AuctionHouse flow can be attacked, or DOS'd to prevent community intent from manifesting is a good start

Moreover:

Begin by examining the access control and permissions for contracts that make up the art piece to AuctionHouse flow, such as the CultureIndex. It’s essential to ensure that access is tightly constrained and locked down to prevent unauthorized or malicious activities.

Last, but not least:

Ensuring gas passed to the settleAndCreateNewAuction functions or other nefarious interactions with AuctionSettlement cannot brick/pause the auction

MarioPoneder commented 9 months ago

Thanks for all the input so far! Tagging @rocketman-21 since all the additional arguments here provide value for the sponsor.

Edit: After reviewing your comments and the (former) duplicates, I agree with this the most:

  • Pausing AuctionHouse by deliberately causing OOG exception.
  • DoSing AuctionHouse by specifying many creators (possibly with griefing receive functions).
  • DoSing AuctionHouse by creating art piece with heavy metadata.

All those vulnerabilities have different core issues which require different mitigation steps. Therefore de-duplicated accordingly.