Nouns DAO contest - 0xNineDec's results

L 1) Insufficient protection of sensitive data

The hardhat.config.ts uses sensitive information imported from an un-committed environment file. The usage of either .env imported variables or even plain pasted keys make it easier for an attacker to compromise the keys used for monitoring, deployment, testing and even if wallet private keys are used in such way funds can be compromised. The following data could be compromised if a leak happens or if the .gitignore file is mistakenly deleted according to the imports performed on hardhat.config.ts :

INFURA_PROJECT_ID
ETHERSCAN_API_KEY
MNEMONIC
WALLET_PUBLIC_KEY
WALLET_PRIVATE_KEY

L 2) Wrong parameters while emitting the Withdraw event

Currently the mentioned event within NounsDAOLogicV2._withdraw() emits the boolean sent parameter.

function _withdraw() external {
    if (msg.sender != admin) {
        revert AdminOnly();
    }

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

    emit Withdraw(amount, sent);
}

The sent parameter within the mentioned event is not checked after the low-level call, so there are scenarios where Withdraw can be Withdraw(currentBalance, false) which will be deceiving for filtering services and for event tracking. It is advised to check the truth of the sent parameter rather than emitting it on an event (and emit no events if the call fails, e.g. reverting before that line).

QA 1) Inconsistent ways of execution reverting

There are several parts of the codebase where require statements with a revert string is used, and some other parts where if statements with custom errors are used. It is advised to unify the criteria regarding error handling. It is remarked that the boolean check needs to be inverted while going from one method to another (the require revert on false statements whereas true statements trigger custom errors inside if statements). Also, as a reminder, using custom errors from 0.8.x provide a clear feedback to users while saving gas against error strings.

QA 2) Some events lack from indexed parameters

It is advisable to have at least one indexed parameter per event in order to enable off-chain filtering for event tracking services. There are some events that have no indexed parameters:

This issue was found 14 times:

event ProposalCreated
event ProposalCreatedWithRequirements
event ProposalCanceled
event ProposalQueued
event ProposalExecuted
event ProposalVetoed
event VotingDelaySet
event VotingPeriodSet
event NewImplementation
event ProposalThresholdBPSSet
event QuorumVotesBPSSet
event NewPendingAdmin
event NewAdmin
event NewVetoer

QA 3) Public functions that are not called by the contract should be external

If a function is not meant to be called inside a contract, it should be marked as external instead. According to the Solidity Docs an inherited virtual external function can be overridden and its behavior can be changed as public.

This issue was found 8 times:

NounsDAOLogicV1.propose()
NounsDAOLogicV1._burnVetoPower()
NounsDAOLogicV1.proposalThreshold()
NounsDAOLogicV1.quorumVotes()
NounsDAOLogicV2.propose()
NounsDAOLogicV2._burnVetoPower()
NounsDAOLogicV2.proposalThreshold()
NounsDAOLogicV2.maxQuorumVotes()

QA 4) Avoid using in line assembly for available built-in methods

Using inline assembly for methods that are built-in within the current compiler versions avoid the need to use assembly which can make some compilers to interpret the code as complex. It is advised using inline assembly only when there is no other way to perform the intended behavior or when specific goals are meant to be achieved (such as optimizations).

NounsDAOLogicV1.getChainIdInternal()
NounsDAOLogicV2.getChainIdInternal()

Which can be modified by uint256 chainId = block.chainid;

QA 5) Expressions that compute constant values via subsequent calls such as keccak256 could be declared as immutable

The main difference between both variable availability modifiers is that the constant calculates the hash each time it is referenced whereas immutable performs this calculation on deployment. More about this could be read here.

This issue was found 4 times:

NounsDAOLogicV1.DOMAIN_TYPEHASH
NounsDAOLogicV1.BALLOT_TYPEHASH
NounsDAOLogicV2.DOMAIN_TYPEHASH
NounsDAOLogicV2.BALLOT_TYPEHASH

QA 6) Signatures recovered directly with ecrecover() can be malleable

It is advisable to consider using a third party library that avoids any signature malleability scenario that may occur while recovering signatures directly with ecrecover() (such as OpenZeppelin's ECDSA).

This issue was found 3 times:

NounsDAOLogicV1.castVoteBySig()
NounsDAOLogicV2.castVoteBySig()
ERC721Checkpointable.delegateBySig()

G-1 Return values of in-house created safe math ops can be unchecked

The usage of self made functions that check over-underflows as for solidity 0.8.x is pointless because the compiler comes with built in over-underflow checks. If those functions are meant to be implemented besides the fact of having built-in checks, unchecking their returns remove this built-in check and saves gas. Unchecked can be used because over/underflow is checked with require statements before performing the math op.

G-2 Constants that are private instead of public save gas

Because constant variables should be declared an initialized as a one-liner, their value could be easily retrieved by reading directly the source code. Removing the public modifier and using private instead, does not create a function getter for the public constant saving around 3500 gas on deployment.

Found 23 times:

NounsDAOLogicV1.sol 
/// @notice The minimum setable proposal threshold
uint256 public constant MIN_PROPOSAL_THRESHOLD_BPS = 1; // 1 basis point or 0.01%

/// @notice The maximum setable proposal threshold
uint256 public constant MAX_PROPOSAL_THRESHOLD_BPS = 1_000; // 1,000 basis points or 10%

/// @notice The minimum setable voting period
uint256 public constant MIN_VOTING_PERIOD = 5_760; // About 24 hours

/// @notice The max setable voting period
uint256 public constant MAX_VOTING_PERIOD = 80_640; // About 2 weeks

/// @notice The min setable voting delay
uint256 public constant MIN_VOTING_DELAY = 1;

/// @notice The max setable voting delay
uint256 public constant MAX_VOTING_DELAY = 40_320; // About 1 week

/// @notice The minimum setable quorum votes basis points
uint256 public constant MIN_QUORUM_VOTES_BPS = 200; // 200 basis points or 2%

/// @notice The maximum setable quorum votes basis points
uint256 public constant MAX_QUORUM_VOTES_BPS = 2_000; // 2,000 basis points or 20%

/// @notice The maximum number of actions that can be included in a proposal
uint256 public constant proposalMaxOperations = 10; // 10 actions


NounsDAOLogicV2.sol 
/// @notice The name of this contract
string public constant name = 'Nouns DAO';

/// @notice The minimum setable proposal threshold
uint256 public constant MIN_PROPOSAL_THRESHOLD_BPS = 1; // 1 basis point or 0.01%

/// @notice The maximum setable proposal threshold
uint256 public constant MAX_PROPOSAL_THRESHOLD_BPS = 1_000; // 1,000 basis points or 10%

/// @notice The minimum setable voting period
uint256 public constant MIN_VOTING_PERIOD = 5_760; // About 24 hours

/// @notice The max setable voting period
uint256 public constant MAX_VOTING_PERIOD = 80_640; // About 2 weeks

/// @notice The min setable voting delay
uint256 public constant MIN_VOTING_DELAY = 1;

/// @notice The max setable voting delay
uint256 public constant MAX_VOTING_DELAY = 40_320; // About 1 week

/// @notice The lower bound of minimum quorum votes basis points
uint256 public constant MIN_QUORUM_VOTES_BPS_LOWER_BOUND = 200; // 200 basis points or 2%

/// @notice The upper bound of minimum quorum votes basis points
uint256 public constant MIN_QUORUM_VOTES_BPS_UPPER_BOUND = 2_000; // 2,000 basis points or 20%

/// @notice The upper bound of maximum quorum votes basis points
uint256 public constant MAX_QUORUM_VOTES_BPS_UPPER_BOUND = 6_000; // 4,000 basis points or 60%

/// @notice The maximum setable quorum votes basis points
uint256 public constant MAX_QUORUM_VOTES_BPS = 2_000; // 2,000 basis points or 20%

/// @notice The maximum number of actions that can be included in a proposal
uint256 public constant proposalMaxOperations = 10; // 10 actions

/// @notice The maximum priority fee used to cap gas refunds in `castRefundableVote`
uint256 public constant MAX_REFUND_PRIORITY_FEE = 2 gwei;

/// @notice The vote refund gas overhead, including 7K for ETH transfer and 29K for general transaction overhead
uint256 public constant REFUND_BASE_GAS = 36000;

G-3 Use custom errors instead of revert strings

Custom errors available from 0.8.4 provide a clear and cheaper way of reverting saving around 50 gas per reversal. Also they provide a cleaner feedback to the user if defined properly in a descriptive way. They can be triggered inside an if statement.

G-4 Revert strings that exceed 32 bytes use more slots

Each memory slot has 32 bytes. If a revert string size exceeds this limit, more slots will be used incurring in an MSTORE (adding up 3 gas consumption). It is advised also using custom errors as for solidity 0.8.x which provide a clear and cheap way of reverting.

Found 47 times:

NounsDAOLogicV1.sol 
'NounsDAO::propose: proposer votes below proposal threshold'
'NounsDAO::propose: proposal function information arity mismatch'
'NounsDAO::propose: one live proposal per proposer, found an already active proposal'
'NounsDAO::queue: proposal can only be queued if it is succeeded'
'NounsDAO::queueOrRevertInternal: identical proposal action already queued at eta'
'NounsDAO::execute: proposal can only be executed if it is queued'
'NounsDAO::cancel: cannot cancel executed proposal'
'NounsDAO::veto: cannot veto executed proposal'
'NounsDAO::state: invalid proposal id'
'NounsDAO::castVoteInternal: voting is closed'
'NounsDAO::castVoteInternal: invalid vote type'
'NounsDAO::castVoteInternal: voter already voted'
'NounsDAO::_setVotingDelay: admin only'
'NounsDAO::_setVotingPeriod: admin only'
'NounsDAO::_setVotingPeriod: invalid voting period'
'NounsDAO::castVoteBySig: invalid signature'
'NounsDAO::_setProposalThresholdBPS: admin only'
'NounsDAO::_setProposalThreshold: invalid proposal threshold'
'NounsDAO::_setPendingAdmin: admin only'
'NounsDAO::_acceptAdmin: pending admin only'
'NounsDAO::_setVetoer: vetoer only'
'NounsDAO::_burnVetoPower: vetoer only'

NounsDAOLogicV2.sol 
'NounsDAO::propose: proposer votes below proposal threshold'
'NounsDAO::propose: proposal function information arity mismatch'
'NounsDAO::propose: one live proposal per proposer, found an already active proposal'
'NounsDAO::queue: proposal can only be queued if it is succeeded'
'NounsDAO::queueOrRevertInternal: identical proposal action already queued at eta'
'NounsDAO::execute: proposal can only be executed if it is queued'
'NounsDAO::cancel: cannot cancel executed proposal'
'NounsDAO::veto: cannot veto executed proposal'
'NounsDAO::veto: veto power burned'
'NounsDAO::state: invalid proposal id'
'NounsDAO::castVoteInternal: voting is closed'
'NounsDAO::castVoteInternal: invalid vote type'
'NounsDAO::castVoteInternal: voter already voted'
'NounsDAO::_setVotingDelay: admin only'
'NounsDAO::_setVotingPeriod: admin only'
'NounsDAO::_setVotingPeriod: invalid voting period'
'NounsDAO::castVoteBySig: invalid signature'
'NounsDAO::_setProposalThresholdBPS: admin only'
'NounsDAO::_setProposalThreshold: invalid proposal threshold'
'NounsDAO::_setPendingAdmin: admin only'
'NounsDAO::_acceptAdmin: pending admin only'
'NounsDAO::_setVetoer: vetoer only'
'NounsDAO::_burnVetoPower: vetoer only'
'NounsDAO::_setQuorumCoefficient: admin only'
'NounsDAO::getDynamicQuorumParamsAt: block number exceeds 32 bits'