Nouns DAO contest - GalloDaSballo's results

Executive Summary

Below are findings, sorted by severity, along with notes on how to remediate them.

Legend:

• U -> Impact is unclear and potentially higher than Low Severity
• L -> Low Severity -> Could cause issues however impact is limited
• R -> Refactoring -> Suggested Code Change to improve readability and maintainability or to offer better User Experience
• NC -> Non-Critical / Informational -> No risk of loss, pertains to events or has no impact

U - Change of Admin can happen without a delay

While the purpose of the Governor is to ensure governance votes on proposals, certain operations can be executed by the admin without delay nor voting, one of them is changing admin.

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L799-L833

In spirit of the contract I recommend adding a delay, or forcing admin changes to be performed by the Timelock

U - Lack of a Sweep function means tokens can be stuck in the Governor

A sweep, similar to the withdraw function, ideally transferring the tokens to the timelock, for a proposal to then release them, will allow recouping back tokens sent back by mistake.

A lack of sweep on the Governor means that tokens sent to it by mistake will not be retrievable, a catch-all sweep, that sends tokens to the Timelock will allow to use governance to claw them back.

L - Unchecked Call in Withdraw

payable.call is a low-level call, it will not cause a revert on failure.

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L789-L790

        (bool sent, ) = msg.sender.call{ value: amount }('');

This is not particularly risky as it can always be called again until it works

L - Lack of Zero check

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L839-L840

    function _setVetoer(address newVetoer) public {
        require(msg.sender == vetoer, 'NounsDAO::_setVetoer: vetoer only');

• See also: NounsDAOExecutor.constructor

L - DelegateTo will not fail if implementation is not a contract

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOProxyV2.sol#L98

Is a low level call, if implementation = random address it will still return true.

That's because the low level call delegatecall will not check for the contract existence

Adding a check for contract existence on the setter will avoid this low likelihood risk: https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOProxyV2.sol#L86

L - Signature verification scheme is subject to signature malleability

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV1.sol#L482-L485

Due to the structure of ECDSA, each signature has a corresponding also valid signature, for that reason Open Zeppelin's implementation of tryRecover ensures the value of the signature is in the lower bits to avoid receiving a "reflected key"

You can see how OZ solved this problem here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/93bc3b657b69e4886dcfef7a2502e165f2cd90d2/contracts/utils/cryptography/ECDSA.sol#L147

Because of the fact that you use receipts for voting, malleability won't be exploitable effectively, however the way you verify signatures is open to that attack

R - DelegateTo is Redundant

delegateTo is only used in constructor, would recommend reconciling code or using it in both the constructor and the fallback.

R - Unusual naming convention

_setVotingDelay is prefixed by an _ which is used to signify internal functions

This nomenclature is inconsistent, I'd recommend swapping it

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L621-L622

    function _setVotingDelay(uint256 newVotingDelay) external {

Change to:

    function setVotingDelay(uint256 newVotingDelay) external {

R - Hardcoded Values - Recommend using Enums

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV1.sol#L510-L516

if (support == 0) {
    proposal.againstVotes = proposal.againstVotes + votes;
} else if (support == 1) {
    proposal.forVotes = proposal.forVotes + votes;
} else if (support == 2) {
    proposal.abstainVotes = proposal.abstainVotes + votes;
}

R - Inconsistent use of camelCase for CONSTANTS

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L92-L93

    uint256 public constant proposalMaxOperations = 10; // 10 actions

Recommend using ALL_CAPS to maintain consistency

R - Falsy statement

In cancel we can cancel if the proposer doesn't meet proposalThreshold

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L352

nouns.getPriorVotes(proposal.proposer, block.number - 1) < proposal.proposalThreshold,

However that is by definition impossible due to the check

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L198-L199

            nouns.getPriorVotes(msg.sender, block.number - 1) > temp.proposalThreshold,

I recommend deleting the check

R - Bps is not checked to be actual BPS or any uint

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L1005-L1008

    function bps2Uint(uint256 bps, uint256 number) internal pure returns (uint256) {
        return (number * bps) / 10000;
    }

Because the only parameter passed as bps is maxQuorumVotesBPS, the code is fine, I'd recommend adding a comment for clarity (also consider changing uint256 to uint16 which is in line with the size of maxQuorumVotesBPS)

NC - Avoid Floating Pragma

Especially to avoid the herd of C4ooooors

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L53-L54

pragma solidity ^0.8.6;

The caret will reduce risk of breaking changes however, due to plenty of bugs for versions between 0.8.1 to 0.8.14, it would be best to settle on a specific version and "pick your poison".

Considering that Hardhat is set to use 0.8.15, you may as well commit to using that one

NC - Incorrect Comment

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L847-L856

    /**
     * @notice Burns veto priviledges
     * @dev Vetoer function destroying veto power forever
     */
    function _burnVetoPower() public {
        // Check caller is pendingAdmin and pendingAdmin ≠ address(0)
        require(msg.sender == vetoer, 'NounsDAO::_burnVetoPower: vetoer only');

        _setVetoer(address(0));
    }

There is no pendingAdmin in this function

Also minor typo priviledges -> privileges

Executive Summary

The codebase can benefit by a more aggressive use of the unchecked keyword, additionally a packing opportunity will save plenty of gas.

Below findings are sorted by effectiveness as well as uniqueness, the bottom of the report will contain findings most other wardens will have submitted.

Each finding lists estimated gas savings. These are estimates based on EVM Opcodes.

Pack booleans with address to save storage slots - 15k write, 2k read

Address uses 20 bytes, each bool 1, by moving the bool declaration for Proposal to be below address proposer you can pack them all in one Storage Slot

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOInterfaces.sol#L203-L208

        /// @notice Creator of the proposal
        /// @audit Move bools below proposer for packing
        address proposer;
        /// @notice Flag marking whether the proposal has been canceled
        bool canceled;
        /// @notice Flag marking whether the proposal has been vetoed
        bool vetoed;
        /// @notice Flag marking whether the proposal has been executed
        bool executed;

Will save one storage slot, meaning each proposal creation will cost 15k less gas, and reading that data will cost 2000 less gas each time

Also applies to:

• ProposalCondensed

Avoid emitting variables from Storage, use Memory Cache instead - 100 gas per instance

The code below is emitting an event using the Storage variable admin

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOExecutor.sol#L94-L95

        emit NewAdmin(admin);

This will cost 100 gas (SLOAD), it would be cheaper to use msg.sender (the accepting admin) to save 98 gas (100 - 2 gas for CALLER).

This pattern is used pretty widely, so this change will bring hundreds of gas savings throughout the codebase

Also see:

• NounsDAOExecutor.setDelay

• All "Compound" setters in NounsaDAOLogicV2

Unchecked Addition - 40 gas per operation

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L601-L608

The below additions can be made unchecked, saving around 40 gas per operation (between 20 and 80)

        if (support == 0) {
            proposal.againstVotes = proposal.againstVotes + votes;
        } else if (support == 1) {
            proposal.forVotes = proposal.forVotes + votes;
        } else if (support == 2) {
            proposal.abstainVotes = proposal.abstainVotes + votes;
        }

Cache length - 3 gas per instance

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV1.sol#L192-L193

            targets.length == values.length &&

Instead of recomputing targets.length each time (which costs an extra 3 gas), you can store the value in a local variable

Usual Gas Savings

Unchecked Addition and Increment - 180 gas

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L222-L226

        temp.startBlock = block.number + votingDelay;
        temp.endBlock = temp.startBlock + votingPeriod;

        proposalCount++;

Also save the return value and read from memory

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L228

```solidity
// Delete
// proposalCount++;

// Use
unchecked {
        newProposal.id = ++proposalCount;
}

Saves an SLOAD - 100 gas, and the unchecked math

Unchecked for Binary Search - 100+

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV2.sol#L948-L960

        uint256 lower = 0;
        uint256 upper = len - 1;
        while (upper > lower) {
            uint256 center = upper - (upper - lower) / 2;
            DynamicQuorumParamsCheckpoint memory cp = quorumParamsCheckpoints[center];
            if (cp.fromBlock == blockNumber) {
                return cp.params;
            } else if (cp.fromBlock < blockNumber) {
                lower = center;
            } else {
                upper = center - 1;
            }
        }

Usual Loop Optimizations - 25 per iteration

• You don't need to assign the default value to i - Saves 3 gas uint256 i = 0; => uint256 i;

• Cache the Array Length - 100 gas per iteration (SLOAD) proposal.targets.length => uint256 length = proposal.targets.length;

• Change the increment to unchecked pre-increment i++ => unchecked { ++i; }

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV1.sol#L281-L282

        for (uint256 i = 0; i < proposal.targets.length; i++) {

Use < 3 to save 3 gas as <= costs twice as much - 3 gas

https://github.com/code-423n4/2022-08-nounsdao/blob/452695d4764ba9d5e1d3eef0d5ecca3d004f215a/contracts/governance/NounsDAOLogicV1.sol#L502-L503

        require(support <= 2, 'NounsDAO::castVoteInternal: invalid vote type');

Can be changed to

        require(support < 3, 'NounsDAO::castVoteInternal: invalid vote type');

Saving 3 gas