Nouns Builder contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/Token.sol#L88 https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/Token.sol#L157

Vulnerability details

Impact

Deployments with totalOwnership = 100 are supported (the value is not allowed to be greater than 100) and this can also be a valid scenario (e.g., founders get 100% of the allocation for the first few weeks). However, in such situations, tokenRecipient[0] ... tokenRecipient[99] will be set. Because of that, minting will not be possible, i.e. the deployment is non-functional

Proof Of Concept

Imagine we are in a situation with totalOwnership = 100 (it does not matter if there is one or multiple founders) and mint is called. In the while loop, the tokenId will be increased until tokenRecipient[tokenId % 100].wallet != address(0). However, because tokenRecipient[0] ... tokenRecipient[99] is set, this will never be the case, meaning the mint function will run out of gas at some point.

Recommended Mitigation Steps

Either disallow 100% total ownership (probably a bad idea) or handle this case explicitly. For instance, 100 tokens for the founders could be minted, and then one for a user would be minted.

Lines of code

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

Vulnerability details

Impact

Because of the "greedy" minting scheme for founders (tokens to founders are minted until _isForFounder returns false, i.e. until there is an unset tokenRecipient[tokenId % 100]), it can happen that the actual percentages of tokens that the founders receive deviate significantly from the desired percentages:

Proof Of Concept

Imagine we are in a situation where one founder has a 51% share and the other a 48% share. Because schedule is set to 1 for the first founder, tokenRecipient[0] ... tokenRecipient[50] will be set to his address. tokenRecipient[51], tokenRecipient[53], ... is set to the address of the second founder. Now let's say a mint happens just before the vestExpiry and when tokenId % 100 == 0. In such a situation, founder 1 will get 51 tokens (because of the consecutive entries in tokenRecipients) and founder 2 will get 1 token (because of the entry in tokenRecipient[51], which is also consecutive. Let's say that the next mint happens after the vest expiration, which means that no founders get additional tokens.

In such a situation, founder 1 got 51 of the "last 100" token IDs, whereas founder 2 only got 1. Therefore, the overall percentage of tokens that those founders got will not be 51% and 40%. When the vest expiration was set to a time far in the future, it will be close to it, but when the vest timespan was only short, it can be very bad. In the extreme case where the expiration is set such that only 1 mint call causes mints for founders, founder 1 will have 51 tokens and founder 2 only 1, meaning the percentages are 51% / 1% instead of 51% / 48%!

Recommended Mitigation Steps

Consider using another distribution scheme. Instead of the current "greedy" scheme (minting until a slot is free), it would make sense to mint the tokens for the founders every 100 tokens, i.e. everytime when tokenId % 100 == 0. Like that, it is ensured that the actual percentages are equal to the desired percentages.

• In Token, when a founder has wallet set to address(0), he will not receive any tokens (which is good), but he is still included in all of the calculations such as numFounders or totalOwnership, meaning that they will be wrong in such situations.
• In Token, totalFounderOwnership() does not incorparate the vesting period. It is just the total percent ownership that the founders received at one point in time, but this metric is pretty useless without incorporating the timespan (as 100% for 1 week is very different to 10% for 10 years).
• In MetadataRenderer._getItemImage, a dot is missing between the item name and extension (https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/metadata/MetadataRenderer.sol#L259).
• MetadataRenderer.addProperties seems to be very error-prone to me. When an item is added for a new property, isNewProperty needs to be set and you need to keep in mind that the property ID refers to the ID of the newly added property, not the global ID that the property will get.
• In Auction._handleOutgoingTransfer, it is not checked if the WETH transfer succeeds (when WETH is used). The commonly used WETH implementation will revert on failure, but this is no requirement of EIP-20, so using another WETH implementation could lead to a problem there.
• Treasury.isExpired returns true for non-existing proposals, which could lead to wrong states for third-party applications that use this function. Consider reverting for non-existing proposals.
• If the Governor contract would ever contain ETH (e.g, after a selfdestruct of some other contract), it could not be retrieved, as the execute function can only forward the received ETH, but not use its own.
• Even after a vetoer is burned, the owner can still update it. This can reduce the confidence in the system, as the owner always has complete veto control. Consider making the burning operation final.
• Auction can only be paused / unpaused after a vote with the corresponding delays. This can be too slow in certain scenarios (e.g., emergency response after a security incident). Consider introducing an optional emergency pause feature (e.g., by the founder).
• It is still possible to bid when an Auction is paused. This can have negative consequences, for instance when the bidding process contains a security vulnerability and the system is temporarily paused for mitigation.

• In Governer.propose, the value of proposalThreshold() is first cached, but the function is then executed again (https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/governance/governor/Governor.sol#L128). Consider using the cached value.