User can lose all governance power

[code423n4]

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 ;

[davidbrai]

Duplicate of #157