Nouns Builder contest - rvierdiiev's results

Lines of code

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

Vulnerability details

Impact

Governor::propose function checks if sender has more than proposalThreshold() votes to create proposal. It checks it in such a way if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();. It checks the votes in the checkpoint that was before block.timestamp.

This allows user to transfer all his nft(checkpoint with block.timestamp is created where proposer doesn't have votes), then create a proposal in the same block(votes from checkpoint that was before are used).

Proof of Concept

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

Recommended Mitigation Steps

Use if (getVotes(msg.sender, block.timestamp) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD(); instead to check checkpoint with current block.timestamp as well.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L564-L592

Vulnerability details

Impact

Governor has functions to update settings.votingDelay, settings.votingPeriod, settings.proposalThresholdBps, settings.quorumThresholdBps. They are Governor::updateVotingDelay, Governor::updateVotingPeriod, Governor::updateProposalThresholdBps, Governor::updateQuorumThresholdBps. These functions don't do any checkings of input values. Providing incorrect values may break the logic of governing.

Governor::updateVotingDelay function should check the value using some upper bound value. Governor::updateVotingPeriod function should at least check that the value is not 0, because then voting will finish instantly. Governor::updateProposalThresholdBps function should check at least that the value is not bigger then 100%(however smarter will be to use some upper and lower bound values). Governor::updateQuorumThresholdBps function should check at least that the value is not bigger then 100% and is not 0(however smarter will be to use some upper and lower bound values).

Proof of Concept

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L564-L592

Recommended Mitigation Steps

Add input checking to the setters.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L385-L405

Vulnerability details

Impact

Governor::veto function gives ability for vetoer to block any proposal. And there is no ability to somehow beat veto. That means that vetoer can control all system using veto in case when he doesn't like smth.

Some mechanism like 50% of total votes can unblock veto should resolve such problem. Dao should be able to control vetoer.

Proof of Concept

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L385-L405

Tools Used

Recommended Mitigation Steps

Add ability to beat veto using some mechanism. For example 50% of total votes.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L331 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L315 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L323 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L106 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L119 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L141

Vulnerability details

Impact

In Auction contract there is ability for the owner to use setTimeBuffer, setMinimumBidIncrement, setReservePrice functions to change settings.timeBuffer, settings.minBidIncrement and settings.reservePrice params. All this settings have impact on createBid function as they are used to determine time to extend auction, minimum bid increment and start price for the nft. So if you change any of this values during the auction it will impact current auction.

Proof of Concept

These are setters. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L331 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L315 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L323

And here you can find how these settings are used in current auction in createBid function. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L106 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L119 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L141

Tools Used

Recommended Mitigation Steps

settings.timeBuffer, settings.minBidIncrement and settings.reservePrice should be set for each auction at creation time.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L71-L126 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L154 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L207-L213

Vulnerability details

Impact

According to nouns builder, founder can have percentage of created nft. This is set in Token::_addFounders function. When new nft is minted by mint function then total supply of tokens is incremented and assigned to tokenId using tokenId = settings.totalSupply++. Then this token is checked if it should be mint to founder(then again increment total supply of tokens) or should be mint to auction using while (_isForFounder(tokenId)).

If token wasn't sold during the auction then auction burns it using burn function. And this function doesn't decrement settings.totalSupply value. But total supply has changed now, it has decreased by one.

So suppose that we have 1 founder of dao that should receive 2% of nft, that means that if 100 nft are available(for example), then 2 of them belongs to that founder. If we have minted 100 nft and 10 of them were not sold(they were then burned), then there are 90 nft available now. And in current implementation founder has ownership of 2 of them, however 2 is not 2% of 90. So in case when nft are not sold on auction the percentage of founder's tokens is increasing and the increasing speed depends on how many tokens were not sold. Also founder gets more power in the community(as he has more percentage now).

Proof of Concept

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L71-L126 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L154 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L207-L213

Tools Used

Recommended Mitigation Steps

When burn function is called then do settings.totalSupply--.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L91-L163 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L185-L201 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L222 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L172

Vulnerability details

Impact

When new token is minted then metadata should be created for it. After the minting MetadataRenderer::onMinted function is called that returns boolean. If metadata was not created then Token::_mint function will revert.

MetadataRenderer::onMinted function is getting properties.length(count of metadata properties) and then tries to fill each property in loop. In the end of function it always returns true. If noone provided properties before then empty metadata will be created(but still function will return true and token will be minted). However if you then call MetadataRenderer::getAttributes or MetadataRenderer::tokenURI the function will revert with TOKEN_NOT_MINTED error. This is because the metadata for token is empty.

Why that can happen that properties.length may be empty? That can happen if the owner didn't call MetadataRenderer::addProperties function that is responsible for storing metadata params.

I believe that MetadataRenderer::onMinted should not be called before MetadataRenderer::addProperties.

Proof of Concept

After token is minted then metadata is rendered. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L172 On minted is called and if properties length is 0 it's still returns true. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L185-L201 When you try to get metadata of token then function reverts. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L222

This function should provide metadata configuration and may not forgot to be called. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L91-L163

Recommended Mitigation Steps

Make sure that MetadataRenderer::onMinted could not be called before MetadataRenderer::addProperties. You can check for the properties.length != 0 for example to allow to use MetadataRenderer::onMinted.

1. Check address param for 0 address. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L40-L41 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L42 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L33 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L62-L66 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L31 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L65-L66 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L65-L69
2. Check if _proposalId exists. Revert when no _proposalId instead of returning 0 values. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L482 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L488 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L516 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L508 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L516 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L69 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L76
3. Check _founders[i].wallet is not 0 address as this address is very important for the dao. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L97
4. Check if _founderId exists. Revert when no _ founderId instead of returning 0 values. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L247