Nouns Builder contest - hyh's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L262-L268

Vulnerability details

If Alice the NFT owner first delegates her votes to herself, second delegates to anyone else with delegate() or delegateBySig() then all her NFT ids will become stuck: their transfers and burning will be disabled.

The issue is _afterTokenTransfer() callback running the _moveDelegateVotes() with an owner instead of her delegate. As Alice's votes in the checkpoint is zero after she delegated them, the subtraction _moveDelegateVotes() tries to perform during the move of the votes will be reverted.

As ERC721Votes is parent to Token and delegate is a kind of common and frequent operation, the impact is governance token moves being frozen in a variety of use cases, which interferes with governance voting process and can be critical for the project.

Proof of Concept

Suppose Alice delegated all her votes to herself and then decided to delegate them to someone else with either delegate() or delegateBySig() calling _delegate():

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L179-L190

    function _delegate(address _from, address _to) internal {
        // Get the previous delegate
        address prevDelegate = delegation[_from];

        // Store the new delegate
        delegation[_from] = _to;

        emit DelegateChanged(_from, prevDelegate, _to);

        // Transfer voting weight from the previous delegate to the new delegate
        _moveDelegateVotes(prevDelegate, _to, balanceOf(_from));
    }

_moveDelegateVotes() will set her votes to 0 as _from == Alice and prevTotalVotes = _amount = balanceOf(Alice) (as _afterTokenTransfer() incremented Alice's vote balance on each mint to her):

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L196-L217

    function _moveDelegateVotes(
        address _from,
        address _to,
        uint256 _amount
    ) internal {
        unchecked {
            // If voting weight is being transferred:
            if (_from != _to && _amount > 0) {
                // If this isn't a token mint:
                if (_from != address(0)) {
                    // Get the sender's number of checkpoints
                    uint256 nCheckpoints = numCheckpoints[_from]++;

                    // Used to store the sender's previous voting weight
                    uint256 prevTotalVotes;

                    // If this isn't the sender's first checkpoint: Get their previous voting weight
                    if (nCheckpoints != 0) prevTotalVotes = checkpoints[_from][nCheckpoints - 1].votes;

                    // Update their voting weight
                    _writeCheckpoint(_from, nCheckpoints, prevTotalVotes, prevTotalVotes - _amount);
                }

After that her votes in the checkpoint become zero. She will not be able to transfer the NFT as _afterTokenTransfer will revert on _moveDelegateVotes's attempt to move 1 vote from Alice to _to, while checkpoints[Alice][nCheckpoints - 1].votes is 0:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L262-L268

    function _afterTokenTransfer(
        address _from,
        address _to,
        uint256 _tokenId
    ) internal override {
        // Transfer 1 vote from the sender to the recipient
        _moveDelegateVotes(_from, _to, 1);

Recommended Mitigation Steps

The root issue is _afterTokenTransfer() dealing with Alice instead of Alice's delegate.

Consider including delegates() call as a fix:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L262-L268

    function _afterTokenTransfer(
        address _from,
        address _to,
        uint256 _tokenId
    ) internal override {
        // Transfer 1 vote from the sender to the recipient
-       _moveDelegateVotes(_from, _to, 1);
+       _moveDelegateVotes(delegates(_from), delegates(_to), 1);

As delegates(address(0)) == address(0) the burning/minting flow will persist:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L124-L129

    /// @notice The delegate for an account
    /// @param _account The account address
    function delegates(address _account) external view returns (address) {
        address current = delegation[_account];
        return current == address(0) ? _account : current;
    }

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L179-L190

Vulnerability details

When Mike the NFT owner delegates for the first time and do it to himself, his votes double as _delegate() will reuse minting workflow by obtaining previous delegate from uninitialized delegation array and running _moveDelegateVotes() with _from being zero address, that corresponds to minting and is used for votes creation. As Mike already have votes equal to his balance (gathered on each mint by the same _moveDelegateVotes() invoked by _afterTokenTransfer()), he obtains x2 votes this way.

Placing severity to be high as ERC721Votes is parent to Token and this allows to substantially bias the governance voting process.

Proof of Concept

_delegate() uses delegation array even if it's not initialized:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L179-L190

    function _delegate(address _from, address _to) internal {
        // Get the previous delegate
        address prevDelegate = delegation[_from];

        // Store the new delegate
        delegation[_from] = _to;

        emit DelegateChanged(_from, prevDelegate, _to);

        // Transfer voting weight from the previous delegate to the new delegate
        _moveDelegateVotes(prevDelegate, _to, balanceOf(_from));
    }

_moveDelegateVotes() doesn't track zero address vote balance, treating it as minting all the time. I.e. when _from == address(0) the votes are created from the thin air:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L196-L233

    function _moveDelegateVotes(
        address _from,
        address _to,
        uint256 _amount
    ) internal {
        unchecked {
            // If voting weight is being transferred:
            if (_from != _to && _amount > 0) {
                // If this isn't a token mint:
                if (_from != address(0)) {
                    // Get the sender's number of checkpoints
                    uint256 nCheckpoints = numCheckpoints[_from]++;

                    // Used to store the sender's previous voting weight
                    uint256 prevTotalVotes;

                    // If this isn't the sender's first checkpoint: Get their previous voting weight
                    if (nCheckpoints != 0) prevTotalVotes = checkpoints[_from][nCheckpoints - 1].votes;

                    // Update their voting weight
                    _writeCheckpoint(_from, nCheckpoints, prevTotalVotes, prevTotalVotes - _amount);
                }

                // If this isn't a token burn:
                if (_to != address(0)) {
                    // Get the recipients's number of checkpoints
                    uint256 nCheckpoints = numCheckpoints[_to]++;

                    // Used to store the recipient's previous voting weight
                    uint256 prevTotalVotes;

                    // If this isn't the recipient's first checkpoint: Get their previous voting weight
                    if (nCheckpoints != 0) prevTotalVotes = checkpoints[_to][nCheckpoints - 1].votes;

                    // Update their voting weight
                    _writeCheckpoint(_to, nCheckpoints, prevTotalVotes, prevTotalVotes + _amount);
                }
            }

This way on the first _delegate() call with _to being the owner it will add balanceOf(owner) to the owner's vote balance, not subtracting it from anywhere.

Now suppose Mike knows that and:

1. Obtains 100 NFT. His votes in the latest checkpoint are now 100 as he got 1 move on each mint to him via _afterTokenTransfer() -> _moveDelegateVotes() logic

2. Delegates votes to himself with delegate(Mike)

His votes is now 200 as delegates[Mike] = 0, while _moveDelegateVotes() will add his balance to the votes again as _from = 0, _to = Mike, and _amount = 100 in (2).

Recommended Mitigation Steps

Consider controlling for the uninitialized delegation array case with the delegates() function:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L124-L129

    /// @notice The delegate for an account
    /// @param _account The account address
    function delegates(address _account) external view returns (address) {
        address current = delegation[_account];
        return current == address(0) ? _account : current;
    }

For example use it in _delegate() directly:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L179-L190

    function _delegate(address _from, address _to) internal {
        // Get the previous delegate
-       address prevDelegate = delegation[_from];
+       address prevDelegate = delegates(_from);

        // Store the new delegate
        delegation[_from] = _to;

        emit DelegateChanged(_from, prevDelegate, _to);

        // Transfer voting weight from the previous delegate to the new delegate
        _moveDelegateVotes(prevDelegate, _to, balanceOf(_from));
    }

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L208-L230 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L43-L54

Vulnerability details

Due to the lack of control of the proposal governance configuration the proposal execution process can be sabotaged:

• On the one hand delay can be set to zero, a setting that disables the control execution the delaying provide

• On the another setting delay can be set to be many magnitudes higher than it's normally used, say 10 years, which will effectively forbid proposal execution

• Also, setting gracePeriod to zero will reduce the execution window to exactly one moment, timestamps[_proposalId], which might not even happen as block.timestamp might not coincide with it.

Such manipulations are conditional on the passing of a corresponding vote. However, due to the Treasury blocking impact the lack of the contract level control here is dangerous.

Suppose vote quorum is thin for any reason and a malicious party passes the vote to block any future proposals as described. This cannot be undone as any future proposals are now disabled.

The logic behind key parameter checks on the contract level is that some values shouldn't be set in any circumstances, so if such a vote passed somehow then it have to be malicious, so the checks aim to limit the impact of that.

Proof of Concept

Delay and grace period can be set to any arbitrary values, i.e. zero or enormous:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L208-L230

    /// @notice Updates the transaction delay
    /// @param _newDelay The new time delay
    function updateDelay(uint256 _newDelay) external {
        // Ensure the caller is the treasury itself
        if (msg.sender != address(this)) revert ONLY_TREASURY();

        emit DelayUpdated(settings.delay, _newDelay);

        // Update the delay
        settings.delay = SafeCast.toUint128(_newDelay);
    }

    /// @notice Updates the execution grace period
    /// @param _newGracePeriod The new grace period
    function updateGracePeriod(uint256 _newGracePeriod) external {
        // Ensure the caller is the treasury itself
        if (msg.sender != address(this)) revert ONLY_TREASURY();

        emit GracePeriodUpdated(settings.gracePeriod, _newGracePeriod);

        // Update the grace period
        settings.gracePeriod = SafeCast.toUint128(_newGracePeriod);
    }

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L43-L54

    function initialize(address _governor, uint256 _delay) external initializer {
        // Ensure the caller is the contract manager
        if (msg.sender != address(manager)) revert ONLY_MANAGER();

        // Ensure a governor address was provided
        if (_governor == address(0)) revert ADDRESS_ZERO();

        // Grant ownership to the governor
        __Ownable_init(_governor);

        // Store the time delay
        settings.delay = SafeCast.toUint128(_delay);

Recommended Mitigation Steps

Consider adding the desired range for delay and gracePeriod in initialize(), updateDelay(), updateGracePeriod() functions.

As an example, from 10 minutes to 100 days for delay, from 1 day to 1 year for gracePeriod.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L179-L190

Vulnerability details

ERC721Votes's delegate() and delegateBySig() allow the delegation to zero address, which result in owner's votes elimination in the checkpoint. I.e. the votes are subtracted from the owner, but aren't added anywhere. _moveDelegateVotes() invoked by _delegate() treats the corresponding call as a burning, erasing the votes.

The impact is that the further transfer and burning attempts for the ids of the owner will be reverted because _afterTokenTransfer() callback will try to reduce owner's votes, which are already zero, reverting the calls due to subtraction fail.

As ERC721Votes is parent to Token the overall impact is governance token burning and transfer being disabled whenever the owner delegated to zero address. This can be done deliberately, i.e. any owner can disable burning and transfer of the owned ids at any moment, which can interfere with governance voting process.

Proof of Concept

User facing delegate() and delegateBySig() allow for zero address delegation:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L131-L135

    /// @notice Delegates votes to an account
    /// @param _to The address delegating votes to
    function delegate(address _to) external {
        _delegate(msg.sender, _to);
    }

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L144-L174

    function delegateBySig(
        address _from,
        address _to,
        uint256 _deadline,
        uint8 _v,
        bytes32 _r,
        bytes32 _s
    ) external {
        // Ensure the signature has not expired
        if (block.timestamp > _deadline) revert EXPIRED_SIGNATURE();

        // Used to store the digest
        bytes32 digest;

        // Cannot realistically overflow
        unchecked {
            // Compute the hash of the domain seperator with the typed delegation data
            digest = keccak256(
                abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR(), keccak256(abi.encode(DELEGATION_TYPEHASH, _from, _to, nonces[_from]++, _deadline)))
            );
        }

        // Recover the message signer
        address recoveredAddress = ecrecover(digest, _v, _r, _s);

        // Ensure the recovered signer is the voter
        if (recoveredAddress == address(0) || recoveredAddress != _from) revert INVALID_SIGNATURE();

        // Update the delegate
        _delegate(_from, _to);
    }

And pass zero address to the _delegate() where it is being set:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L179-L190

    function _delegate(address _from, address _to) internal {
        // Get the previous delegate
        address prevDelegate = delegation[_from];

        // Store the new delegate
        delegation[_from] = _to;

        emit DelegateChanged(_from, prevDelegate, _to);

        // Transfer voting weight from the previous delegate to the new delegate
        _moveDelegateVotes(prevDelegate, _to, balanceOf(_from));
    }

In this case _moveDelegateVotes() will reduce the votes from the owner, not adding it to anywhere as _from is the owner, while _to is zero address:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L203-L220

            if (_from != _to && _amount > 0) {
                // If this isn't a token mint:
                if (_from != address(0)) {
                    // Get the sender's number of checkpoints
                    uint256 nCheckpoints = numCheckpoints[_from]++;

                    // Used to store the sender's previous voting weight
                    uint256 prevTotalVotes;

                    // If this isn't the sender's first checkpoint: Get their previous voting weight
                    if (nCheckpoints != 0) prevTotalVotes = checkpoints[_from][nCheckpoints - 1].votes;

                    // Update their voting weight
                    _writeCheckpoint(_from, nCheckpoints, prevTotalVotes, prevTotalVotes - _amount);
                }

                // If this isn't a token burn:
                if (_to != address(0)) { // @ audit here we add the votes to the target, but only if it's not zero address

The owner might know that and can use such a delegation to interfere with the system by prohibiting of transferring/burning of his ids.

This happens via _afterTokenTransfer() reverting as it's becomes impossible to reduce owner's votes balance by 1:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L262-L271

    function _afterTokenTransfer(
        address _from,
        address _to,
        uint256 _tokenId
    ) internal override {
        // Transfer 1 vote from the sender to the recipient
        _moveDelegateVotes(_from, _to, 1);

        super._afterTokenTransfer(_from, _to, _tokenId);
    }

Recommended Mitigation Steps

Consider prohibiting zero address as a delegation destination:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L179-L190

    function _delegate(address _from, address _to) internal {
+		if (_to == address(0)) revert INVALID_SIGNATURE();

        // Get the previous delegate
        address prevDelegate = delegation[_from];

        // Store the new delegate
        delegation[_from] = _to;

        emit DelegateChanged(_from, prevDelegate, _to);

        // Transfer voting weight from the previous delegate to the new delegate
        _moveDelegateVotes(prevDelegate, _to, balanceOf(_from));
    }

When _to isn't zero there always be an addition in _moveDelegateVotes(), so the system votes balance will be sustained.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L108-L130

Vulnerability details

The total number of properties is now limited to be 15 or less with hard code on the storage structures level.

In the same time it is possible to add unlimited number of properties with MetadataRenderer's addProperties().

If this happens, with a malicious intent or as the result of an operational mistake, the whole auctioning will become permanently frozen.

Proof of Concept

The attributes mapping has fixed length of 16 for each token's array of attributes:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/storage/MetadataRendererStorageV1.sol#L19-L20

    /// @notice The attributes generated for a token
    mapping(uint256 => uint16[16]) internal attributes;

First element, tokenAttributes[0], is dedicated to the currently existing properties count:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L171-L185

    function onMinted(uint256 _tokenId) external returns (bool) {
        // Ensure the caller is the token contract
        if (msg.sender != settings.token) revert ONLY_TOKEN();

        // Compute some randomness for the token id
        uint256 seed = _generateSeed(_tokenId);

        // Get the pointer to store generated attributes
        uint16[16] storage tokenAttributes = attributes[_tokenId];

        // Cache the total number of properties available
        uint256 numProperties = properties.length;

        // Store the total as reference in the first slot of the token's array of attributes
        tokenAttributes[0] = uint16(numProperties);

So, an NFT can have up to 15 properties set.

However, the number of properties addProperties() introduces isn't controlled anyhow, it is possible to add an arbitrary large numNewProperties:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L108-L130

        // Cache the number of existing properties
        uint256 numStoredProperties = properties.length;

        // Cache the number of new properties
        uint256 numNewProperties = _names.length;

        // Cache the number of new items
        uint256 numNewItems = _items.length;

        unchecked {
            // For each new property:
            for (uint256 i = 0; i < numNewProperties; ++i) {
                // Append storage space
                properties.push();

                // Get the new property id
                uint256 propertyId = numStoredProperties + i;

                // Store the property name
                properties[propertyId].name = _names[i];

                emit PropertyAdded(propertyId, _names[i]);
            }

Once numStoredProperties + numNewProperties > 15, all the token mints will be failing:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L171-L194

    function onMinted(uint256 _tokenId) external returns (bool) {
        // Ensure the caller is the token contract
        if (msg.sender != settings.token) revert ONLY_TOKEN();

        // Compute some randomness for the token id
        uint256 seed = _generateSeed(_tokenId);

        // Get the pointer to store generated attributes
        uint16[16] storage tokenAttributes = attributes[_tokenId];

        // Cache the total number of properties available
        uint256 numProperties = properties.length;

        // Store the total as reference in the first slot of the token's array of attributes
        tokenAttributes[0] = uint16(numProperties);

        unchecked {
            // For each property:
            for (uint256 i = 0; i < numProperties; ++i) {
                // Get the number of items to choose from
                uint256 numItems = properties[i].items.length;

                // Use the token's seed to select an item
                tokenAttributes[i + 1] = uint16(seed % numItems);

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

    function _mint(address _to, uint256 _tokenId) internal override {
        // Mint the token
        super._mint(_to, _tokenId);

        // Generate the token attributes
        if (!settings.metadataRenderer.onMinted(_tokenId)) revert NO_METADATA_GENERATED();

The Auction pauses as a result:

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

    function _createAuction() private {
        // Get the next token available for bidding
        try token.mint() returns (uint256 tokenId) {
        	...
        } catch Error(string memory) {
            _pause();
        }

But there will be no fix as there is no way to remove a property, MetadataRenderer have only property addition functionality. I.e. the auctioning system will become permanently frozen.

Recommended Mitigation Steps

To align property management functionality with the storage consider introducing the same limit to addProperties():

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L91-L112

    function addProperties(
        string[] calldata _names,
        ItemParam[] calldata _items,
        IPFSGroup calldata _ipfsGroup
    ) external onlyOwner {
        // Cache the existing amount of IPFS data stored
        uint256 dataLength = ipfsData.length;

        // If this is the first time adding properties and/or items:
        if (dataLength == 0) {
            // Transfer ownership to the DAO treasury
            transferOwnership(settings.treasury);
        }

        // Add the IPFS group information
        ipfsData.push(_ipfsGroup);

        // Cache the number of existing properties
        uint256 numStoredProperties = properties.length;

        // Cache the number of new properties
        uint256 numNewProperties = _names.length;

+       if (numStoredProperties + numNewProperties > 15) revert PROPERTY_LIMIT_BREACHED();

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L136-L151

Vulnerability details

Treasury's settings.gracePeriod now is set on construction and can be changed, but has no material effect on the Treasury workflow as execute() logic doesn't depend on it.

I.e. it can be set to any value, the execution will continue regardless. This is substantial deviation from the expected core logic, so setting the severity to be high.

Proof of Concept

execute() now only checks that !isReady(proposalId), i.e. that timestamps[_proposalId] != 0 && block.timestamp >= timestamps[_proposalId]:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L136-L151

    /// @notice Executes a queued proposal
    /// @param _targets The target addresses to call
    /// @param _values The ETH values of each call
    /// @param _calldatas The calldata of each call
    /// @param _descriptionHash The hash of the description
    function execute(
        address[] calldata _targets,
        uint256[] calldata _values,
        bytes[] calldata _calldatas,
        bytes32 _descriptionHash
    ) external payable onlyOwner {
        // Get the proposal id
        bytes32 proposalId = hashProposal(_targets, _values, _calldatas, _descriptionHash);

        // Ensure the proposal is ready to execute
        if (!isReady(proposalId)) revert EXECUTION_NOT_READY(proposalId);

        ... // @audit execution goes on

However, there is also an expiration logic, that should prohibit the execution once grace period has passed:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L72-L78

    /// @notice If a queued proposal can no longer be executed
    /// @param _proposalId The proposal id
    function isExpired(bytes32 _proposalId) external view returns (bool) {
        unchecked {
            return block.timestamp > (timestamps[_proposalId] + settings.gracePeriod);
        }
    }

Notice, there is no test coverage for gracePeriod effect on execution, only the ability to set new gracePeriod is tested:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/test/Gov.t.sol#L769-L773

        assertEq(treasury.gracePeriod(), 2 weeks);

        governor.execute(targets, values, calldatas, keccak256(bytes("")));

        assertEq(treasury.gracePeriod(), _newGracePeriod);

Recommended Mitigation Steps

Consider checking for the expiration as well (the EXECUTION_EXPIRED error is already present in ITreasury):

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L136-L150

    /// @notice Executes a queued proposal
    /// @param _targets The target addresses to call
    /// @param _values The ETH values of each call
    /// @param _calldatas The calldata of each call
    /// @param _descriptionHash The hash of the description
    function execute(
        address[] calldata _targets,
        uint256[] calldata _values,
        bytes[] calldata _calldatas,
        bytes32 _descriptionHash
    ) external payable onlyOwner {
        // Get the proposal id
        bytes32 proposalId = hashProposal(_targets, _values, _calldatas, _descriptionHash);

        // Ensure the proposal is ready to execute
        if (!isReady(proposalId)) revert EXECUTION_NOT_READY(proposalId);
+       if (isExpired(proposalId)) revert EXECUTION_EXPIRED(proposalId);

    function test_ProposalExpirationRevert() public {
        // propose
        mintVoter1();
        (address[] memory targets, uint256[] memory values, bytes[] memory calldatas) = mockProposal();
        bytes32 descriptionHash = keccak256(bytes("test"));
        vm.warp(1 days);
        vm.prank(voter1);
        governor.propose(targets, values, calldatas, "test");
        bytes32 proposalId = governor.hashProposal(targets, values, calldatas, descriptionHash);

        // vote
        vm.warp(block.timestamp + governor.votingDelay());
        vm.prank(voter1);
        governor.castVote(proposalId, 1);

        // queue
        vm.warp(block.timestamp + governor.votingPeriod());
        vm.prank(voter1);
        governor.queue(proposalId);

        // execute // Instead wait too long
        vm.warp(block.timestamp + 50 days);
        assert (treasury.isQueued(proposalId));
        assert (treasury.isExpired(proposalId));
        try governor.execute(targets, values, calldatas, descriptionHash) {
            revert();
        }
        catch {
        }
    }