Nouns Builder contest - V_B's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L71 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L118

Vulnerability details

Description

There is _addFounders function in the Token contract. There is the following loop:

// Used to store the base token id the founder will recieve
uint256 baseTokenId;

// For each token to vest:
for (uint256 j; j < founderPct; ++j) {
    // Get the available token id
    baseTokenId = _getNextTokenId(baseTokenId);

    // Store the founder as the recipient
    tokenRecipient[baseTokenId] = newFounder;

    emit MintScheduled(baseTokenId, founderId, newFounder);

    // Update the base token id
    (baseTokenId += schedule) % 100;
}

As we can see line (baseTokenId += schedule) % 100; have a bug, there is no reason to do % 100 operation over the result value of the incrementing baseTokenId by schedule.

Recommended Mitigation Steps

We recommend removal of such modulo operation. If case it is necessary to perform a modulo operation over baseTokenId inside the loop, it will be correct to do the following:

baseTokenId += schedule;
baseTokenId %= 100;

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.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L71 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L102 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L108

Vulnerability details

Description

There is a function _addFounders in Token contract. It accepts array of FounderParams as an input. For each of founders it uses founderPct as an variable to store percent ownership for such founder.

It is unsafe to cast it to from uint256 to uint8 in some of places, but not all:

...
if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();
...
newFounder.ownershipPct = uint8(founderPct);
...
uint256 schedule = 100 / founderPct;
...
// For each token to vest:
for (uint256 j; j < founderPct; ++j) {
    ...
}
...

Impact

Lack of casting founderPct to uint8 type can lead to incorrect calculation of schedule variable and incorrect number of iterations in the loop that goes for each token to vest for such founder.

Recommended Mitigation Steps

Use uint8 type for ownershipPct in FounderParams struct or cast founderPct to uint8 type in all possible places in _addFounders function.

deadline comparison

There is castVoteBySig function in Governor contract. It accepts _deadline as a parameter and checks it is valid comparing to block.timestamp. Specifically, the function performs such checks.

// Ensure the deadline has not passed
if (block.timestamp > _deadline) revert EXPIRED_SIGNATURE();

There is another logic with non-strict comparison in different parts of the code:

Treasury.sol

/// @notice If a proposal is ready to execute (does not consider expiration)
/// @param _proposalId The proposal id
function isReady(bytes32 _proposalId) public view returns (bool) {
    return timestamps[_proposalId] != 0 && block.timestamp >= timestamps[_proposalId];
}

Auction.sol

// Ensure the auction is still active
if (block.timestamp >= _auction.endTime) revert AUCTION_OVER();

It is reasonable to have consistent logic in all places -- we propose to use non-strict comparison in castVoteBySig function.