Nouns DAO contest - KIntern_NA's results

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L126-L144 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L88-L91

Vulnerability details

Impact

Function delegate(delegatee) will delegates votes from msg.sender to delegatee. In case delegatee = address(0), it mean msg.sender wanna vote to himself.

// url = https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L112-L115
function delegate(address delegatee) public {
    if (delegatee == address(0)) delegatee = msg.sender;
    return _delegate(msg.sender, delegatee);
}

In the same sense, function delegateBySig will delegates votes from signatory to delegatee with given signature (v, r, s). But this function doesn't cover case when delegatee = address(0). It can make some carelessness users, who think function delegateBySig will act same as function delegate, lose their votes when they trynna vote for themself by calling delegateBySig(address(0), nonce, expiry, v, r, s).

// url = https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L126-L144
function delegateBySig(
    address delegatee,
    uint256 nonce,
    uint256 expiry,
    uint8 v,
    bytes32 r,
    bytes32 s
) public {
    bytes32 domainSeparator = keccak256(
        abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name())), getChainId(), address(this))
    );
    bytes32 structHash = keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry));
    bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, structHash));
    address signatory = ecrecover(digest, v, r, s);
    require(signatory != address(0), 'ERC721Checkpointable::delegateBySig: invalid signature');
    require(nonce == nonces[signatory]++, 'ERC721Checkpointable::delegateBySig: invalid nonce');
    require(block.timestamp <= expiry, 'ERC721Checkpointable::delegateBySig: signature expired');
    return _delegate(signatory, delegatee);
}

It will call to internal function _moveDelegates(oldDelegate, address(0), amount), and won't update the checkpoint of neither signatory nor address(0)

// url = https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L223-L228
if (dstRep != address(0)) {
    uint32 dstRepNum = numCheckpoints[dstRep];
    uint96 dstRepOld = dstRepNum > 0 ? checkpoints[dstRep][dstRepNum - 1].votes : 0;
    uint96 dstRepNew = add96(dstRepOld, amount, 'ERC721Checkpointable::_moveDelegates: amount overflows');
    _writeCheckpoint(dstRep, dstRepNum, dstRepOld, dstRepNew);
}

It will make users's votes burnt forever because even users realize their mistake, and trynna call function delegate/delegateBySig(someUser) to recover their votes, the tx will revert (or tx won't change any state in case users call delegate for themself). The reason why the tx revert is it will get the currentDelegate = delegates(delegator) = delegator then it will calculate the srcRepNew = srcRepOld - amount = 0 - amount < 0 --> underflow --> revert

function _delegate(address delegator, address delegatee) internal {
    /// @notice differs from `_delegate()` in `Comp.sol` to use `delegates` override method to simulate auto-delegation
    address currentDelegate = delegates(delegator);
    _delegates[delegator] = delegatee;
    emit DelegateChanged(delegator, currentDelegate, delegatee);
    uint96 amount = votesToDelegate(delegator);
    _moveDelegates(currentDelegate, delegatee, amount);
}


function _moveDelegates(
    address srcRep,
    address dstRep,
    uint96 amount
) internal {
    if (srcRep != dstRep && amount > 0) {
        if (srcRep != address(0)) {
            uint32 srcRepNum = numCheckpoints[srcRep];
            uint96 srcRepOld = srcRepNum > 0 ? checkpoints[srcRep][srcRepNum - 1].votes : 0;
            uint96 srcRepNew = sub96(srcRepOld, amount, 'ERC721Checkpointable::_moveDelegates: amount underflows');
            _writeCheckpoint(srcRep, srcRepNum, srcRepOld, srcRepNew);
        }


        if (dstRep != address(0)) {
            uint32 dstRepNum = numCheckpoints[dstRep];
            uint96 dstRepOld = dstRepNum > 0 ? checkpoints[dstRep][dstRepNum - 1].votes : 0;
            uint96 dstRepNew = add96(dstRepOld, amount, 'ERC721Checkpointable::_moveDelegates: amount overflows');
            _writeCheckpoint(dstRep, dstRepNum, dstRepOld, dstRepNew);
        }
    }
}

Proof of concept

Please use this file for ease to understand the issue https://gist.github.com/huuducst/3358ed16335814622b898340c0ebaa39 (Add this file to test/governance/)

Tools Used

Hardhat

Recommended Mitigation Steps

Add the condition if delegatee == address(0) then delegatee = sinatory

[2022-08-nounsdao] QA report

tags: c4, 2022-08-nounsdao, QA

Shouldn't use _ before name of external/public function

• https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L529
• https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L545
• https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L562
• https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L580
• https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L597
• https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L615
• https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L637
• https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L649

Wrong comment

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L651 Change comment

• From Check caller is pendingAdmin and pendingAdmin ≠ address(0)
• To Check caller is vetoer

[2022-08-nounsdao] Gas optimization

tags: c4, 2022-08-nounsdao, QA

unnecessary check if vetoer != address(0) or not

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L364

unnecessary check msg.sender != address(0)

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L617

typo veteor -> vetoer

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L46

quorumVotesBPS is always equal to zero

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L942-L943

In NounsDaoLogicV2, variable quorumBPS can't be changed since there is no function to update this variable. --> Delete it if not necessary