Nouns DAO contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/c1c7c6201d0247f92472419ff657b570f9104565/contracts/base/ERC721Checkpointable.sol#L143

Vulnerability details

Impact

In delegateBySig, it is possible to delegate to address(0). In such cases, the votes are burned in _moveDelegates (the srcRep is updated, but the dstRep is not). The account of the delegator becomes useless. Delegating to other addresses is no longer possible and transferring the NFTs also becomes impossible.

Proof Of Concept

Alice has 2 tokens that are delegated to Bob. She calls delegateBySig with address(0) as delegatee. Bob's balance is decreased by two as a result. If Alice now calls delegate or delegateBySig again, there is always an underflow, as delegates(address(Alice)) = Alice and _moveDelegates therefore tries to subtract from Alice's balance. The same happens when she tries to transfer the NFTs because of the _moveDelegates(delegates(from), delegates(to), 1); within _beforeTokenTransfer.

Recommended Mitigation Steps

Do not allow delegation to the zero address.

• It would make more sense to me if the votes need to be equal to or higher than the proposalThreshold (https://github.com/code-423n4/2022-08-nounsdao/blob/c1c7c6201d0247f92472419ff657b570f9104565/contracts/governance/NounsDAOLogicV2.sol#L198). This would also be in line with the error message. Because it currently states "proposer votes below proposal threshold", but also reverts when the votes are equal to the threshold.
• There is no way to revoke a signature that was given for delegation. Many projects have such a function, e.g. when the signature was provided by accident or to the wrong person.
• In _delegate, the event DelegateChanged is emitted, even when the delegatee was not changed. This could be confusing for applications that consume these events.
• add96 (https://github.com/code-423n4/2022-08-nounsdao/blob/c1c7c6201d0247f92472419ff657b570f9104565/contracts/base/ERC721Checkpointable.sol#L269) will never show the errorMessage, because the calculation already reverts with Solidity 0.8. Consider using an unchecked such that the correct message is shown when an overflow occurs.
• Since Solidity 0.8, block.chainid could be used, there is no need to use inline assembly to retrieve the chain ID.
• The comment for MAX_QUORUM_VOTES_BPS_UPPER_BOUND(https://github.com/code-423n4/2022-08-nounsdao/blob/c1c7c6201d0247f92472419ff657b570f9104565/contracts/governance/NounsDAOLogicV2.sol#L86) is wrong, as it states 4,000 basis points, but the value is 6,000 basis points.
• In _burnVetoPower, the comment // Check caller is pendingAdmin and pendingAdmin ≠ address(0) is wrong (probably a copy paste error).
• state returns ProposalState.Defeated for the (non-existing) proposal with ID 0. While this is not extremely problematic, it is wrong, could lead to problems in the future, and it is not in line with the function otherwise (as it reverts for non-exiting proposals). Consider reverting for ID 0.
• Unlike the transfer of the admin, the transfer of the vetoer is not a two-step process with confirmation. Consider also implementing a two-step process to avoid accidentally losing the vetoer possibility when it is not intended.

• The check vetoer != address(0) is unnecessary, as it is required that vetoer is equal to msg.sender. Because msg.sender can never be address(0) (or the probability is extremely low, almost all smart contracts would break if someone would find the private key), the second check is sufficient.
• Similarly to the previous issue, checking that msg.sender is not equal to address(0) can be skipped in _acceptAdmin() (https://github.com/code-423n4/2022-08-nounsdao/blob/c1c7c6201d0247f92472419ff657b570f9104565/contracts/governance/NounsDAOLogicV2.sol#L819).
• _setVetoer already checks that msg.sender == vetoer, so repeating this check in _burnVetoPower is not necessary (https://github.com/code-423n4/2022-08-nounsdao/blob/c1c7c6201d0247f92472419ff657b570f9104565/contracts/governance/NounsDAOLogicV2.sol#L853).
• In multiple for loops, the loop iteration can be marked as unchecked because an overflow is not possible (as the iterator is bounded) and i++ can be replaced with ++i:

./governance/NounsDAOLogicV2.sol:292:        for (uint256 i = 0; i < proposal.targets.length; i++) {
./governance/NounsDAOLogicV2.sol:330:        for (uint256 i = 0; i < proposal.targets.length; i++) {
./governance/NounsDAOLogicV2.sol:357:        for (uint256 i = 0; i < proposal.targets.length; i++) {
./governance/NounsDAOLogicV2.sol:382:        for (uint256 i = 0; i < proposal.targets.length; i++) {
./governance/NounsDAOLogicV1.sol:281:        for (uint256 i = 0; i < proposal.targets.length; i++) {
./governance/NounsDAOLogicV1.sol:319:        for (uint256 i = 0; i < proposal.targets.length; i++) {
./governance/NounsDAOLogicV1.sol:346:        for (uint256 i = 0; i < proposal.targets.length; i++) {
./governance/NounsDAOLogicV1.sol:371:        for (uint256 i = 0; i < proposal.targets.length; i++) {