Nouns Builder contest - Ch_301's results

Lines of code

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

Vulnerability details

Impact

Attacker could get type(uint192).max of voting weight So he can create a proposal() to withdraw an amount from the Treasury.sol and he can pass the proposalThreshold with no need to anyone

Proof of Concept

1- Attacker buy one NFT and transfer it to addr_1 2- addr_1 will delegate() to addr_2 3- now Attacker will buy another NFT and transfer it to addr_1. So he has two 2 NFTs on the same address addr_1 4- this time addr_1 will delegate() to addr_3 In this step the addr_2 has type(uint192).max of voting weight. How? 4.1- delegate()==>_delegate()==> _moveDelegateVotes() 4.2- _moveDelegateVotes() it takes 3 param 4.2.1- _from is addr_2. As we know his voting weight is only one 1. 4.2.2- _to is addr_3 4.2.3- _amount is the amount of addr_1 which is two 2 4.3- on _moveDelegateVotes() in the first block if (_from != address(0)) on the last line we have this calculation prevTotalVotes - _amount as we know _amount == 2 and prevTotalVotes == 1. So 1-2 is 2**256 - 1 because we have an unchecked block on this function 5- on _writeCheckpoint() the _newTotalVotes will be only type(uint192).max

Recommended Mitigation Steps

Remove the unchecked block from _moveDelegateVotes()

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L133-L135 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/token/ERC721.sol#L125-L151

Vulnerability details

Impact

With only one NFT the user can keep increasing the voting weight on different addresses

Proof of Concept

1- let’s say Alice has one NFT 2- he delegate() to Bob 3- Alice invoke transferFrom() to Richard 4- Richard invoke delegate() to Bob So now Bob has two voting weight We can keep doing this process until we achieve the voting weight that we need it

Recommended Mitigation Steps

When someone invokes transferFrom() you need to undelegated his voting weight

Lines of code

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

Vulnerability details

Impact

The proposer's voting weight still has not dropped below the proposal threshold but anyone can invoke cancel() successfully

Proof of Concept

On the propose() it just check if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) so you can creat a propose in two cases = or >.

1- Let's say Alice creates a proposal and their getVotes(msg.sender, block.timestamp - 1) == proposalThreshold() 2- Bob will invoke cancel() in this check

if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) > proposal.proposalThreshold)
    revert INVALID_CANCEL();

3- the first part msg.sender != proposal.proposer will be true and the cecond part getVotes(proposal.proposer, block.timestamp - 1) > proposal.proposalThreshold is false. Because when Alice creates their proposal he was have getVotes(msg.sender, block.timestamp - 1) == proposalThreshold() 4- Bob will cancel Alice even he’s voting weight still has not dropped below the proposal threshold

Even when Alice decides to invoke propose() he has getVotes(msg.sender, block.timestamp - 1) > proposalThreshold() but after that he can sell or transfer some tokens. He can be in this situation getVotes(msg.sender, block.timestamp - 1) == proposalThreshold()

Recommended Mitigation Steps

if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) >= proposal.proposalThreshold)
                revert INVALID_CANCEL();

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L184-L189

Vulnerability details

Impact

In case you want to undelegated your voting weight you need to reset the delegation[your_Addr ] == address(0) . But unfortunately this not working you will lose your voting weight

Proof of Concept

1- Alice delegated their voting weight to Bob 2- after that he decides to undelegated them to use all their own voting weight 3- Alice invoke delegate(address(0)) 4- on _moveDelegateVotes() _to param is address(0) so it can’t pass the check if (_to != address(0))

Recommended Mitigation Steps

Add more checks on _delegate() so if _to == address(0) you need to set it to be _to == _from

Lines of code

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

It effects on the number of votes required to be in favor of a proposal in order to reach a quorum We can see on Governor.sol

    function quorum() public view returns (uint256) {
        unchecked {
            return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;
        }
    }

So its effects directly the proposal

Proof of Concept

On Token.sol Every mint() the totalSupply is incrented by one

 do {
                // Get the next token to mint
                tokenId = settings.totalSupply++;

                // Lookup whether the token is for a founder, and mint accordingly if so
            } while (_isForFounder(tokenId));

But the burn() has no decrement ont

Recommended Mitigation Steps

settings.totalSupply- - 

This check if (nCheckpoints != 0) is always true

Finding

File: /lib/token/ERC721Votes.sol

 if (nCheckpoints != 0) prevTotalVotes = checkpoints[_from][nCheckpoints - 1].votes;

if (nCheckpoints != 0) prevTotalVotes = checkpoints[_to][nCheckpoints - 1].votes;

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

Missing check for address(0)

Finding

File: /lib/token/ERC721Votes.sol

 function _delegate(address _from, address _to) internal {
        address prevDelegate = delegation[_from];
        delegation[_from] = _to;

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L179-L184