Nouns Builder contest - cccz's results

Lines of code

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

Vulnerability details

Impact

In the _writeCheckpoint function of the ERC721Votes contract, votes for the same block time are not recorded in the same Checkpoint status variable, which results in multiple Checkpoint status variables for the same block time.

    function _writeCheckpoint(
        address _account,
        uint256 _id,
        uint256 _prevTotalVotes,
        uint256 _newTotalVotes
    ) private {
        // Get the pointer to store the checkpoint
        Checkpoint storage checkpoint = checkpoints[_account][_id];

        // Record the updated voting weight and current time
        checkpoint.votes = uint192(_newTotalVotes);
        checkpoint.timestamp = uint64(block.timestamp);

        emit DelegateVotesChanged(_account, _prevTotalVotes, _newTotalVotes);
    }

The getPastVotes function returns the votes corresponding to the block time, which means that the result of getPastVotes can be manipulated.

Consider the following scenario: User A currently has the following checkpoints

timestamp 100 100 100 100 100 150
votes     10  20  30  20  50
getPastVotes(A,100) == 30

User B votes on User A

timestamp 100 100 100 100 100 150 150 160
votes     10  20  30  20  50  51
getPastVotes(A,100) == 20

User B manipulates the result of getPastVotes(A,100) by voting for User A

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

    function _writeCheckpoint(
        address _account,
        uint256 _id,
        uint256 _prevTotalVotes,
        uint256 _newTotalVotes
    ) private {
        // Get the pointer to store the checkpoint
        Checkpoint storage checkpoint = checkpoints[_account][_id];

        if (_id > 0 && checkpoints[_account][_id-1].timestamp == block.timestamp) {
             checkpoints[_account][_id-1].votes = uint192(_newTotalVotes);
             numCheckpoints[_account]--;
        } else {
             checkpoint.votes = uint192(_newTotalVotes);
             checkpoint.timestamp = uint64(block.timestamp);
        }
        emit DelegateVotesChanged(_account, _prevTotalVotes, _newTotalVotes);
    }

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L466-L477

Vulnerability details

Impact

In the proposalThreshold() and quorum() of the Governor contract, if the result of the multiplication is less than 10000, the return value will be 0 due to precision loss.

    function proposalThreshold() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.proposalThresholdBps) / 10_000;
        }
    }

    /// @notice The current number of votes required to be in favor of a proposal in order to reach quorum
    function quorum() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;
        }
    }

In propose(), proposalThreshold() returns 0 which allows anyone to create a proposal, and quorum() returns 0 which allows the proposal to be executed without being votes.

        unchecked {
            // Ensure the caller's voting weight is greater than or equal to the threshold
            if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
        }
...
        proposal.quorumVotes = uint32(quorum());
...
        } else if (proposal.forVotes < proposal.againstVotes || proposal.forVotes < proposal.quorumVotes) {
            return ProposalState.Defeated;

Proof of Concept

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L466-L477 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L122-L129 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L441-L442

Tools Used

None

Recommended Mitigation Steps

One suggestion is to use <= in propose() and state() instead of <.

propose():

        unchecked {
            // Ensure the caller's voting weight is greater than or equal to the threshold
-           if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
+           if (getVotes(msg.sender, block.timestamp - 1) <= proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
        }

state():

-       } else if (proposal.forVotes < proposal.againstVotes || proposal.forVotes < proposal.quorumVotes) {
+       } else if (proposal.forVotes < proposal.againstVotes || proposal.forVotes <= proposal.quorumVotes) { // or proposal.forVotes <= proposal.againstVotes || proposal.forVotes < proposal.quorumVotes
            return ProposalState.Defeated;

Lines of code

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

Vulnerability details

Impact

In the _addFounders function of a Token contract, the baseTokenId is limited to 99 by %100, but in the following calculation, the baseTokenId will not be the value after %100, causing the baseTokenId to exceed 99.

   (baseTokenId += schedule) % 100;

If _addFounders() adds two Founders, founderPct is 50, 33, then baseTokenId will exceed 99 and will be 100,103,106 ....

Later, in the _isForFounder function, tokenid greater than 99 will not be vested in the founder.

function _isForFounder(uint256 _tokenId) private returns (bool) {
        // Get the base token id
        uint256 baseTokenId = _tokenId % 100;

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

                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;
+                   baseTokenId = (baseTokenId + schedule) % 100;

                }

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L126-L129

Vulnerability details

Impact

In the propose() of the Governor contract, the user can create a proposal when his votes are equal to proposalThreshold().

        unchecked {
            // Ensure the caller's voting weight is greater than or equal to the threshold
            if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
        }

But in cancel(), the proposal can be cancelled because the user's votes are equal to proposalThreshold.

        unchecked {
            // Ensure the caller is the proposer or the proposer's voting weight has dropped below the proposal threshold
            if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) > proposal.proposalThreshold)
                revert INVALID_CANCEL();
        }

Proof of Concept

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L126-L129 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L361-L365

Tools Used

None

Recommended Mitigation Steps

Use <= in propose() instead of <.

propose():

    unchecked {
        // Ensure the caller's voting weight is greater than or equal to the threshold
-           if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
+           if (getVotes(msg.sender, block.timestamp - 1) <= proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
    }

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L466-L477

Vulnerability details

Impact

In proposalThreshold() and quorum() of the Governor contract, if the totalSupply of token is 0, the return value will be 0.

    function proposalThreshold() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.proposalThresholdBps) / 10_000;
        }
    }

    /// @notice The current number of votes required to be in favor of a proposal in order to reach quorum
    function quorum() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;
        }
    }

In propose(), proposalThreshold() returns 0 which allows anyone to create a proposal, and quorum() returns 0 which allows the proposal to be executed without being votes.

    unchecked {
        // Ensure the caller's voting weight is greater than or equal to the threshold
        if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
    }
...
    proposal.quorumVotes = uint32(quorum());
...
    } else if (proposal.forVotes < proposal.againstVotes || proposal.forVotes < proposal.quorumVotes) {
        return ProposalState.Defeated;

Proof of Concept

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L466-L477

Tools Used

None

Recommended Mitigation Steps

Consider forbidding the creation of proposals when the totalSupply of token is 0

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/storage/AuctionStorageV1.sol#L10-L20

Vulnerability details

Impact

In the *StorageV1 contract, the Settings structure variable is at the top of the storage, which causes a storage collision if the Settings structure needs to be extended later.

contract TokenStorageV1 is TokenTypesV1 {
    /// @notice The token settings
    Settings internal settings;

    /// @notice The vesting details of a founder
    /// @dev Founder id => Founder
    mapping(uint256 => Founder) internal founder;

    /// @notice The recipient of a token
    /// @dev ERC-721 token id => Founder
    mapping(uint256 => Founder) internal tokenRecipient;
}

Also, a storage gap should be added to the *StorageV1 contract to allow new variables to be added when the contract is upgraded.

Proof of Concept

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/storage/AuctionStorageV1.sol#L10-L20 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/storage/GovernorStorageV1.sol#L9-L20 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/storage/TreasuryStorageV1.sol#L9-L16 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/storage/TokenStorageV1.sol#L9-L20

Tools Used

None

Recommended Mitigation Steps

Consider moving the Settings structure to the end of the *StorageV1 contract and adding a storage gap at the end of the *StorageV1 contract.

uint256[50] private __gap;