Nouns DAO contest - 0xbepresent's results

1 - Consider two-phase vetoer transfer

The vetoer calls _setVetoer in order to tranfers the ownership to the new address directly. There is a risk that the ownership is transferred to an invalid address, thus causing the contract to be without vetoer.

Recommendation

Consider a two step process where the vetoer nominates an prevetoer account and the nominated prevetoer account need to call an acceptVetoOwnership() function for the transfer of vetoer to fully succeed. This ensures the nominated EOA account is a valid and active account

2 - The logic is not according to the comment

The require here should validates pendingAdmin != address(0) instead msg.sender != address(0)

// Check caller is pendingAdmin and pendingAdmin ≠ address(0)
require(msg.sender == pendingAdmin && msg.sender != address(0), 'NounsDAO::_acceptAdmin: pending admin only');

Recommendation

Consider use pendingAdmin != address(0) in the require() logic.

1 - Internal functions only called once can be inlined to save gas

Not inlining costs more gas because of two extra JUMP instructions and additional stack operations needed for function calls.

There are 3 instances of this issue:

contracts/governance/NounsDAOLogicV2.sol#L305    function queueOrRevertInternal(
contracts/governance/NounsDAOLogicV2.sol#L866    function proposalCreationBlock(Proposal storage proposal) internal view returns (uint256) {
contracts/governance/NounsDAOLogicV2.sol#L974    function _refundGas(uint256 startGas) internal {

2 - Process functions only if they have not been processed

In addition to checking that the function is not in the "Executed" state, you can also check that the function has not been processed.

Example:

# cancel function
require(state(proposalId) != ProposalState.Canceled, 'NounsDAO::cancel: proposal already canceled');

# veto function
require(state(proposalId) != ProposalState.Vetoed, 'NounsDAO::veto: proposal already vetoed');

There are 2 instances of this issue:

contracts/governance/NounsDAOLogicV2.sol#L347    require(state(proposalId) != ProposalState.Executed, 'NounsDAO::cancel: cannot cancel executed proposal');
contracts/governance/NounsDAOLogicV2.sol#L377    require(state(proposalId) != ProposalState.Executed, 'NounsDAO::veto: cannot veto executed proposal');

3 - Unnecessary checked arithmetic in for loop

There is no risk that the loop counter can overflow, using solidity's unchecked block saves gas.

There are 4 instances of this issue:

governance/NounsDAOLogicV2.sol#292 => for (uint256 i = 0; i < proposal.targets.length; i++) {
governance/NounsDAOLogicV2.sol#331 => for (uint256 i = 0; i < proposal.targets.length; i++) {
governance/NounsDAOLogicV2.sol#359 => for (uint256 i = 0; i < proposal.targets.length; i++) {
governance/NounsDAOLogicV2.sol#385 => for (uint256 i = 0; i < proposal.targets.length; i++) {

Unchecked implementation example:

for (uint256 i; i < 10;) {
    j++;
    unchecked {
        ++i;
    }
}

Gas Report example using this gist:

╭─────────────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/Unchecked.t.sol:Contract0 contract ┆                 ┆      ┆        ┆      ┆         │
╞═════════════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                             ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 55105                                       ┆ 307             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                               ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ withOutUnChecked                            ┆ 2068            ┆ 2068 ┆ 2068   ┆ 2068 ┆ 1       │
╰─────────────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯
╭─────────────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/Unchecked.t.sol:Contract1 contract ┆                 ┆      ┆        ┆      ┆         │
╞═════════════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                             ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 53705                                       ┆ 300             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                               ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ withUnchecked                               ┆ 1408            ┆ 1408 ┆ 1408   ┆ 1408 ┆ 1       │
╰─────────────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯

4 - Cache Array Length Outside of Loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

There are 4 instances of this issue:

governance/NounsDAOLogicV2.sol#292    for (uint256 i = 0; i < proposal.targets.length; i++) {
governance/NounsDAOLogicV2.sol#331    for (uint256 i = 0; i < proposal.targets.length; i++) {
governance/NounsDAOLogicV2.sol#359    for (uint256 i = 0; i < proposal.targets.length; i++) {
governance/NounsDAOLogicV2.sol#385    for (uint256 i = 0; i < proposal.targets.length; i++) {

5 - Consider using custom errors

The contracts in scope use Solidity >=0.8.4, consider using Custom Errors as they are more gas efficient while allowing developers to describe the error in detail using Natspec