Malicious pausing the contract

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L204
https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L206
https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L235

Vulnerability details

Vulnerability details

Description

There is a function _createAuction in Auction contract.

It consist the following logic:

/// @dev Creates an auction for the next token
function _createAuction() private {
    // Get the next token available for bidding
    try token.mint() returns (uint256 tokenId) {
        **creating of the auction for token with id equal to tokenId**

        // Pause the contract if token minting failed
    } catch Error(string memory) {
        _pause();
    }
}

According to the EIP-150 call opcode can consume as most 63/64 of parrent calls' gas. That means token.mint() can fail since there will be no gas.

All in all, if token.mint() fail on gas and the rest gas is enough for pausing the contract by calling _pause in catch statement the contract will be paused.

Please note, that a bug can be exploitable if the token.mint() consume more than 1.500.000 of gas, because 1.500.000 / 64 > 20.000 that need to pause the contract. Also, the logic of token.mint() includes traversing the array up to 100 times, that's heavy enough to reach 1.500.000 gas limit.

Impact

Contract can be paused by any user by passing special amount of gas for the call of settleCurrentAndCreateNewAuction (which consists of two internal calls of _settleAuction and _createAuction functions).

Recommended Mitigation Steps

Add a special check for upper bound of gasLeft at start of _createAuction function.

[GalloDaSballo]

Honestly I'm really impressed by the submission, however I think the quote:

Please note, that a bug can be exploitable if the token.mint() consume more than 1.500.000 of gas, because 1.500.000 / 64 > 20.000 that need to pause the contract. Also, the logic of token.mint() includes traversing the array up to 100 times, that's heavy enough to reach 1.500.000 gas limit.

shows that the likelihood of this happening is extremely small, the base mint will cost between 20k and 40k gas, and each instance of the loop should roughly cost 5k, with the minting instance costing around 5k + up to 50k

From basic napkin math this is actually surprisingly possible

However I believe the odds of this happening are extremely low as you'd need to have at least 20 tokens being minted to founders

I think Med is more appropriate for now, I really like the catch of the OOG exploit, however I may have to downgrade further as the worst case scenario being pausing is not impactful

[GalloDaSballo]

I believe the finding to be valid and Medium Severity as the conditions are non-trivial, but the impact is Denial of Service which can be triggered predictably given the circumnstances

[davidbrai]

@GalloDaSballo @iainnash catch Error(string memory) doesn't catch out-of-gas errors
to catch out-of-gas errors one would need to use catch (bytes memory lowLevelData)
Therefore, IIUC, this issue is invalid

This was intentionally not catching out-of-gas errors in the Nouns contract this code is based on. (credit to @solimander)

[GalloDaSballo]

@davidbrai Thank you for the clarification, I have ran my own sim and must agree with you.

Errors of the type:

• Underflow
• Oog

Will not be caught by the Catch Statement.

I have to agree with you that I should have disputed this report for lacking a specific POC and the POC I wrote indicates that it is invalid.

I'd like to flag that the POC I wrote seems to suggest that the function will not catch custom errors as well, meaning that as far as I can tell, the custom errors will not be caught.

POC follows (can be quickly setup using the excellent: https://github.com/0xKitsune/gas-lab)

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.13;

import "../../lib/test.sol";
import "../../lib/Console.sol";

enum Values {
    First,
    Second
}

contract Exploit is DSTest {
    Reverter testC;

    function testExploit() public {
        testC = new Reverter();

        try testC.doTheRevertingThing{gas: 123}() returns (uint256) {

        } catch Error(string memory) {
            return;
        }

        require(false, "Did not catch");
    }
}

contract Reverter {
    error INVALID_APPROVAL();

    function doTheRevertingThing() external returns (uint256){
        revert INVALID_APPROVAL();
        return 123;
    }
}

I apologize for the mistake and hope it didn't cause needless refactorings. The site is looking great, wish you the best with the product!