Nouns Builder contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

Token.mint() finds the unscheduled token and mints for the auction house.

But if the founders have 100% ownership, it can't select the unscheduled token id so it will revert.

As a result, there will be no tokens for the founders and the full system will be broken.

Proof of Concept

There is no requirement totalOwnership should be less than 100 here.

So we can assume there is 1 founder that has 100 founderPct.

Then from this logic, all base tokens from 0 to 99 will be scheduled for this founder.

Then Token.mint() can't find the unscheduled id here and the founder won't have the minted tokens also because this function reverts always.

Even if we wait until the vest is expired here, there will be no founders in this case.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

I think we should check totalOwnership < 100 here like below so that there is at least one token for the auction house.

if ((totalOwnership += uint8(founderPct)) >= 100) revert INVALID_FOUNDER_OWNERSHIP();

Lines of code

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

Vulnerability details

Impact

We assume all baseTokenIds are less than 100 and increase by schedule for equal distribution here.

But % 100 won't be applied after addition so some bastTokenIds will be useless for founders because they are equal or greater than 100.

As a result, founders might have fewer tokens than they expect.

Proof of Concept

With the below code, baseTokenId won't save the final result after % 100.

File: 2022-09-nouns-builder\src\token\Token.sol
118:                     (baseTokenId += schedule) % 100;

Currently it's same as baseTokenId += schedule.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We should modify like below.

baseTokenId = (baseTokenId + schedule) % 100;

Lines of code

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

Vulnerability details

Impact

As we can see from here, all baseTokenIds should be less than 100.

But Token._getNextTokenId() might return some invalid baseTokenId so some founders would have less tokens than they expect.

Proof of Concept

Normally Token._getNextTokenId() returns a valid baseTokenId (less than 100) with small number of founders.

But there are some cases where it returns invalid token Ids.

• Let's assume we have below founders who have such founderPct individually.
• 9 founders from F1 to F9 have 1 founderPct.
• 1 founder F10 has 10 founderPct.
• 21 founders from F11 to F31 have 1 founderPct.
• 1 founder F32 has 3 founderPct.
• Then F1 to F9 will have 0 ~ 8 baseTokenId.
• F10 will have 9, 19, 29, ..., 99 baseTokenId.
• F11 to F31 will have 10 ~ 32 baseTokenId except for 19, 29.
• Then F32 will have 33, 66, 100 as 99 is scheduled already.

So F32 should have 3 scheduled tokens but he has only 2.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

I think we can modify Token._getNextTokenId() like below.

function _getNextTokenId(uint256 _tokenId) internal view returns (uint256) {
    unchecked {
        while (tokenRecipient[_tokenId].wallet != address(0)) _tokenId = (_tokenId + 1) % 100;

        return _tokenId;
    }
}

This while loop won't be infinite because the total number of scheduled tokens <= 100.

Lines of code

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

Vulnerability details

Impact

MetadataRenderer.addProperties() should check if numProperties < 16.

When the number of properties >= 16, onMinted() function will revert here so tokens can't be minted at all.

Proof of Concept

• When we add new properties here, we don't check if the number of properties is less than 16.
• But when we generate tokenAttributes here, this array is declared as uint16[16].
• So onMinted() will revert with 16 or more properties.
• Also, there is no function to remove already assigned properties so we should check carefully when we add new properties.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend adding this require() here.

require(numStoredProperties + numNewProperties < 16, "should be less than 16");

[N-01] Immutable addresses should be 0-checked

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

[N-02] Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields.

• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/IAuction.sol#L22
• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/IAuction.sol#L28
• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/IAuction.sol#L34
• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/IGovernor.sol#L42
• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/ITreasury.sol#L22
• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/manager/IManager.sol#L21
• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/IToken.sol#L21

[N-03] Constants should be defined rather than using magic numbers

• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L468
• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L468
• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L119
• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L88
• 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#L118
• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L179
• https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L271