Nouns Builder contest - izhuer's results

Lines of code

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

Vulnerability details

Impact

Specifically, ERC721Votes enables users to delegate their voting power to others. Note that when delegating the voting power, the actual token balance will not change.

On the other hand, when transferring tokens, the voting power transfers accordingly: https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L268.

Considering the following situation.

• Alice holds two wallet addresses, A and B
• Wallet A gets 5 tokens, so current the voting power of wallet A is 5.
• Wallet A delegates all its voting power to B, so current the voting power of A is 0 but B is 5. Note that currently, the balance of wallet A remains 5
• Wallet A transfers 1 token to wallet B, where _moveDelegateVotes happens again.
  • Note that it will decrease wallet A's voting power by 1, although it is already 0.
  • The unchecked statement (https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L201) enables an integer overflow
  • As a result, wallet A's voting power becomes type(uint256).max

Recommended Mitigation Steps

Following the code in https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Votes.sol, remove the unchecked statement to prevent such underflow.

Lines of code

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

Vulnerability details

Impact

Specifically, to support a proposal, the voting power is counted at the time of the proposal creation time.

However, there are multiple services that support NFT flashloan, e.g., NFTuloan (https://www.nftuloan.com/).

Since the voting power is counted as the proposal creation time, a malicious proposer can first borrow a large amount of NFTs and then create the proposal, and repay the flashloan at the end of this transaction. As such, his voting power will be as large as possible.

Note that the malicious proposer still needs to hold a few token to make the proposal alive.

The attack is also enabled by another ERC721Vote bugs (multiple voting power at the same timestamp, which I will make a separate report).

If the founder does not notice such a malicious proposal, the malicious proposal can get processed.

The bug can also be degraded as medium, since there is a time lock for a process getting effective.

Recommended Mitigation Steps

Check the weight at a slightly different time compared with the proposal creation time (against flashloan)

int8 underflow in src/token/Token.sol:99

    function _addFounders(IManager.FounderParams[] calldata _founders) internal {
        // Cache the number of founders
        uint256 numFounders = _founders.length;

        // Used to store the total percent ownership among the founders
        uint256 totalOwnership;

        unchecked {
            // For each founder:
            for (uint256 i; i < numFounders; ++i) {
                // Cache the percent ownership
                uint256 founderPct = _founders[i].ownershipPct;

                // Continue if no ownership is specified
                if (founderPct == 0) continue;

                // Update the total ownership and ensure it's valid
                if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

                // Compute the founder's id
                uint256 founderId = settings.numFounders++;

                // Get the pointer to store the founder
                Founder storage newFounder = founder[founderId];

                // Store the founder's vesting details
                newFounder.wallet = _founders[i].wallet;
                newFounder.vestExpiry = uint32(_founders[i].vestExpiry);
                newFounder.ownershipPct = uint8(founderPct);

                // Compute the vesting schedule
                uint256 schedule = 100 / founderPct;

                // 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;
                }
            }

            // Store the founders' details
            settings.totalOwnership = uint8(totalOwnership);
            settings.numFounders = uint8(numFounders);
        }

When a malicious founder supplies a founderPct bigger than 2^8, he can bypass the check against if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

unknown statement in src/token/Token.sol:118

This statement (baseTokenId += schedule) % 100; seems not helpful.

array indexing overflow in src/token/metadata/MetadataRenderer.sol:194

tokenAttributes[i + 1] = uint16(seed % numItems);

When numProperties is larger than 16, any token mint will fail.

ECDSA Malleability in src/governance/governor/Governor.sol:208

It is suggested to use OpenZeppelin ECDSA library.