Nouns DAO contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

Contract is missing self delegation in case of delegateBySig function. This means if delegateBySig is called with zero address delegatee then User votes will be burned instead of setting delegatee to signatory

Proof of Concept

1. User calls delegateBySig function with valid signature and delegatee set as address(0)

2. This makes call to _delegate function

function delegateBySig(
        address delegatee,
        uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public {
        ...
        return _delegate(signatory, delegatee);
    }

3. This updates _delegates for signatory to address(0)

function _delegate(address delegator, address delegatee) internal {
       ...

        _delegates[delegator] = delegatee;   //  _delegates[signatory] =address(0);

        ...
    }

4. Also this function computes voting power of signatory and moves the voting power to delegatee using _moveDelegates function

function _delegate(address delegator, address delegatee) internal {
        ...
address currentDelegate = delegates(delegator);
        uint96 amount = votesToDelegate(delegator);

        _moveDelegates(currentDelegate, delegatee, amount);
    }

5. Since only signatory is non zero address, so _moveDelegates only subtracts the voting power of signatory and does nothing on delegatee

function _moveDelegates(
        address srcRep,
        address dstRep,
        uint96 amount
    ) internal {
        if (srcRep != dstRep && amount > 0) {
            if (srcRep != address(0)) {
               ...
                uint96 srcRepNew = sub96(srcRepOld, amount, 'ERC721Checkpointable::_moveDelegates: amount underflows');
                _writeCheckpoint(srcRep, srcRepNum, srcRepOld, srcRepNew);
            }

            if (dstRep != address(0)) {
                // will not execute as delegatee as address(0)
            }
        }
    }

6. This means after this transaction signatory voting power becomes 0 which is wrong. Ideally signatory should have been assigned as delegatee

Recommended Mitigation Steps

Change the delegateBySig function to include below:

if(delegatee==address(0))
delegatee= signatory ;

Unrestricted receive function

Contract: https://github.com/davidbrai/c4-nouns-gov-v2/blob/master/contracts/governance/NounsDAOLogicV2.sol#L1030

Function: receive

Issue: If user accidentally sends eth to this contract, the eth will get stuck forever due to unrestricted receive function

Recommendation: Remove receive function if contract is not expecting any external amount.

Withdraw can fail without creating any error

Contract: https://github.com/davidbrai/c4-nouns-gov-v2/blob/master/contracts/governance/NounsDAOLogicV2.sol#L783

Function: _withdraw

Issue: It was observed that function is using call to transfer eth but is not checking the return value to verify if call was success or not

Recommendation: Add a check to see if call was success

Wrong DOMAIN_TYPEHASH definition

Contract: https://github.com/davidbrai/c4-nouns-gov-v2/blob/master/contracts/governance/NounsDAOLogicV2.sol#L101

Issue: version is missing while deducing DOMAIN_TYPEHASH which breaks the EIP 712 standard

Recommendation: Add string version, to the EIP712Domain string