Nouns Builder contest - R2's results

Lines of code

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

Vulnerability details

Impact

Malicious user can get unlimited count of votes

Proof of Concept

1. Alice bought 1 NFT on auction
2. Alice delegates votes to Bob
3. Alice bought second NFT on auction
4. Alice delegates votes to someone else
5. Bob's last checkpoint will be unchecked(1 - 2) == MAX_UIN256

Tools Used

Writed test in hardhat:

...
    let aliceAddress = accounts[0].address;
    let bobAddress = accounts[1].address;

    // 1. Alice bought 1 NFT on auction
    await token.mint();

    // 2. Alice delegates votes to Bob
    await token.delegate(bobAddress, {from: aliceAddress});

    // 3. Alice bought second NFT on auction
    await token.mint();
    
    // 4. Alice delegates voted to someone else
    await token.delegate(someoneElse, {from: aliceAddress});

    // 5. Bob has 6277101735386680763835789423207666416102355444464034512895 votes
    console.log("bobVotes = %s", await token.getVotes(bobAddress) + '');
...

Recommended Mitigation Steps

Save how many tokens were delegated. And when delegation revoked, use that value to subtraction

Lines of code

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

Vulnerability details

Impact

If settings.token.totalSupply() < 10_000 / settings.quorumThresholdBps , function will return 0 So all created proposition will automatically be in status ProposalState.Succeeded after voting delay

Recommended Mitigation Steps

Review your calculations, maybe add second statement for cases while totalSupply() is low

Lines of code

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

Vulnerability details

Impact

baseTokenId can be more then 100 Your expression is (baseTokenId += schedule) % 100 But it should be baseTokenId = (baseTokenId + schedule) % 100 Now in some cases baseTokenId can be more then 100 In such cases founder will not receive his NFT, for which baseTokenId were >= 100

Recommended Mitigation Steps

Fix statement

Lines of code

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

Vulnerability details

Impact

You have no checks that votingDelay more then 0 If anyone will deploy DAO with votingDelay=0, anyone will be able to manipulate votes:

1. Attacker bought 1 NFT
2. Attacker creates a proposal sending him funds
3. In the same transaction attacker votes for his own proposal. He will be able to do so because you are checking token balance at current block getVotes(_voter, proposal.timeCreated) (proposal.timeCreated - current block. because ``votingDelay=0`)
4. In the same trx attacker send NFT to his second wallet and votes from that wallet too
5. Attacker can repeat this many times

So malicious user can create proposal and vote it with many tokens (even when he have just 1)

Proof of Concept

See above

Recommended Mitigation Steps

Add checks for governance params. Check then votingDelay more then allowed minimum

Judge has assessed an item in Issue #139 as Medium risk. The relevant finding follows:

Lines of code

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

Vulnerability details

Impact

If settings.token.totalSupply() < 10_000/settings.proposalThresholdBps), then function returns 0 So anyone can send proposal

Recommended Mitigation Steps

Review math, maybe add other statement while there are low totalSupply

1. No check in Auction.constructor() and Auction.initialize()

Details

manager, _token, _founder, _duration, _reservePrice,treasury may be 0

Recommendations

Add required checks

2. Auction may be prolonged forever if your buyers have enough money

Details

Prolongation logic: auction.endTime = uint40(block.timestamp + settings.timeBuffer) And may be prolonging forever

Recommendations

Add hard limit

3. No limit for proposal operations

Details

No checks for _targets.length. When executing this proposal, OutOfGas may happen and it proposal will not be executed ever

Recommendations

Add limit for _targets.length

4. No balance checks in Governor.propose()

Details

No checks that sum of _values exists on balance It will fail on executing

Recommendations

Add balance checks and do not accept proposal if no balance for it's execution

5. Saving votes in uint32 may become a problem in future

Details

Balance of token is uint256. But you stores votes in uint32

6. Why Governor.execute() is payable?

Details

The function should not be payable. All funds should be stored in treasury

7. There can be only 1 vetoer

Details

It would be better to make at as role, not hardcoded variable And to be able to give that role to more than 1 address

8. You have copy-pasted standard contract (openzeppelin) instead of just importing them

Details

There may be mistakes and typos on copying

9. No check in Manager.constructor()

Details

_tokenImpl, _metadataImpl, _auctionImpl, _treasuryImpl, _governorImpl may be 0

Recommendations

Add required checks

10. No check in Token.constructor() and Token.initialize()

Details

manager, metadataRenderer, auction may be 0

Recommendations

Add required checks

11. No check if _founders[i].wallet == 0 (i > 0)

Details

There is a check only for first founder, not for other

Recommendations

Add required checks

12. No check in MetadataRenderer.constructor() and MetadataRenderer.initialize()

Details

_manager, _token, _founder, treasury may be 0

Recommendations

Add required checks

13. MetadataRenderer.numProperties is not limited

Details

It may lead to OutOfGas in MetadataRenderer.onMinted()

Recommendations

Add required checks

14. blockhash(block.number) is useless

Details

blockhash(block.number) is always 0

15. It makes no sense to add more then 16 properties

Details

Other properties will be ignored in MetadataRenderer.onMinted() But there are no checks in MetadataRenderer.addProperties()

Recommendations

Add required checks

1. No need to cache startTime

Details

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L211 block.timestamp costs only 2 gas, it so cheep So you do not need to create new variable

2. Redundant function call

Details

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L128 You already saved function result in currentProposalThreshold, use it

3. Copy mapping to memory

Details

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L70 You have many calls to this object, so it would be better to save it to memory

4. Useless EIP712._computeDomainSeparator()

Details

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/utils/EIP712.sol#L68 There is no overrides for this function, so you can just hardcode this value and remove function