Nouns DAO contest - d3e4's results

No check for invalid quorumCoefficient If the quorumCoefficient is negative then the dynamic quorum is implemented such that casting an against-vote may bring the proposal from being defeated due to not reaching the quorum to reaching the quorum and succeeding. The quorumCoefficient must not be negative and should be so required whenever it is set, i.e. in the functions _setQuorumCoefficient and _setDynamicQuorumParams.

 

Variable is uninitialised but used quorumVotesBPS is never initialised but used in NounsDAOLogicV2.sol#L922-962. Thus it only serves as the default zero value. Everything seems to work out, but it obfuscates the actual logic.

 

Unexplained setting of default values to already initialised variables. These values of the struct Proposal are already initialised to their default values but are here set again to their default values. This would be a waste of gas. However, it is wise to reset them to default here as a precaution against having set values of a future proposal, which is expected to be default valued, but may be accessed through the mapping proposals. It would be more expensive to declare a new struct in memory, with guaranteed default values, set the desired values, and then copy it over to storage, so the current solution should remain. But this does warrant an explanatory comment, to prevent a naive gas optimisation to remove them. Instances: NounsDAOLogicV2.sol: 231 238 239 240 241 242 243 NounsDAOLogicV1.sol: 223 230 231 232 233 234 235

 

Unnecessary code In NounsDAOLogicV2.sol the function castRefundableVoteInternal 533 is only called by castRefundableVote 503 and castRefundableVoteWithReason 518. The body of castRefundableVoteInternal can be directly incorporated in castRefundableVoteWithReason and the reason can similarly be eliminated by letting castRefundableVote instead call castRefundableVoteWithReason with a blank reason. This makes more sense, and also saves gas.

 

Possibly pointless function safe32 and safe16 The function safe32 in NounsDAOLogicV2.sol#L1018-1021 and ERC721Checkpointable.sol#L253-256 is only ever used to require that block.number is not greater than or equal to 2^32. However, this will not happen in more than a thousand years, and when it does the contract will break anyway. So if anything it should be in an assert rather than a require, but may as well be removed all together. The function safe16 in NounsDAOLogicV2.sol#L1023-1028 is only used with quorumVotesBPS, but this value is never set, aside from its default 0, and therefore this check is also pointless. Removing these would also save gas.

 

Unused constant MAX_QUORUM_VOTES_BPS Instances: NounsDAOLogicV2.sol: 89

 

Typos Comment should say “6,000 basis points or 60%”. NounsDAOLogicV2.sol#L86 “caries” should be “carries”. NounsDAOLogicV2.sol#L582, NounsDAOLogicV1.sol#L490 Comment should say “Check caller is vetoer”. NounsDAOLogicV2.sol#L852, NounsDAOLogicV1.sol#L650 Require error message should be ’NounsDAO::initialize: invalid quorum votes’. NounsDAOLogicV1.sol#L140

 

The so-called ‘quorum’ is not actually a quorum In NounsDAOLogicV2.sol and NounsDAOLogicV1.sol: A quorum is the voter turnout required to decide a vote, but the term is here used to decide the number of for-votes required for success, i.e. it is rather a kind of supermajority. This confusion with terminology is connected to the quorum logic, which possibly contains more severe issues (reported separately). But it seems a genuine quorum is (at least partly) what is intended, so this terminology issue is probably best dealt with by instead fixing the flawed logic. Also related to this is the fact that abstainVotes is not used in any meaningful way, except being registered, but for a true quorum they should probably matter more than non-votes.

 

ProposalState.Defeated does not mean the proposal is defeated ProposalState.Defeated, defined in NounsDAOInterfaces.sol L228 and L332, is only used in NounsDAOLogicV2.sol#L444 and NounsDAOLogicV1.sol#L433 as a state that returns before ProposalState.Succeeded returns. It is never used to indicate that the proposal is defeated, but merely as not yet succeeded (it can be tried again for success). It seems the logic would be clearer if it was removed, in which case the conditions for it can be moved to the check for Proposal.Succeeded (NounsDAOLogicV2.sol#L445 and NounsDAOLogicV1.sol#L434).

 

Constants should be in UPPER_CASE_WITH_UNDERSCORES Instances: NounsDAOLogicV2.sol: 59 92 NounsDAOLogicV1: 67 94 ERC721Checkpointable.sol: 41

 

Function name should be in mixedCase Instances: NounsDAOInterfaces.sol#L405

 

Inconsistent naming of functions to indicate visibility Some internal functions have a prefixed underscore in their names, some end with “Internal”, and some do neither. Also, some external or public functions have a prefixed underscore. All private functions have a prefixed underscore in their names.

The following internal functions use a prefixed underscore. NounsDAOLogicV2.sol: 964 974 ERC721Checkpointable.sol: 97 197 210 232 97 97 ERC721Enumerable.sol: 96

The following internal functions use “Internal” in their name. NounsDAOLogicV2.sol: 305 533 588 1010 NounsDAOLogicV1.sol: 294 500 676

The following internal function names do not indicate that they are internal. NounsDAOLogicV2.sol: 866 988 1006 1018 1023 NounsDAOLogicV1.sol: 672 ERC721Checkpointable.sol: 253 258 263 273 282

The following external or public functions have a prefixed underscore. NounsDAOLogicV2.sol: 621 637 654 673 701 726 748 783 799 817 839 851 NounsDAOLogicV1.sol: 529 545 562 580 597 615 637 649 NounsDAOProxy.sol: 78

 

Missing NatSpec Instances: NounsDAOLogicV2.sol: 305 783 866 964 974 988 1006 1010 1018 1023 NounsDAOLogicV1.sol: 294 672 676 NounsDAOInterfaces.sol: 403 405 407 409 411 419 427 437 439 ERC721Checkpointable.sol: 197 210 232 253 258 263 273 282

 

Magic numbers Consider defining constants instead of using magic numbers. Instances: NounsDAOLogicV2.sol: 908 909 1007 NounsDAOLogicV1.sol: 673

Certain preliminary voting results have a guaranteed outcome and the voting may be halted to save on refunds Considering gas is spent to refund the voting, it may be of interest to halt voting when the result cannot be changed by casting further votes. Then gas would not be wasted on inconsequential votes. The check for this can be done first off-chain after each vote, then a contract function to halt voting, which checks for this, may be called only when we know the conditions for halting the voting are met. The easiest way to check if we can halt the vote is probably to call state() (or perform the equivalent logic) with a test version of the proposal, where all remaining votes have been cast as either for-votes or against-votes – depending on the current state of the proposal – trying to change the outcome. If the outcome doesn’t change, we can halt the voting.

 

Calculation steps clean-up The calculation of quorumBPS in NounsDAOLogicV2.sol#L908-911 can be reduced to uint256 quorumAdjustmentBPS = (params.quorumCoefficient * againstVotes) / (1e2 * totalSupply); uint256 adjustedQuorumBPS = params.minQuorumVotesBPS + quorumAdjustmentBPS; uint256 quorumBPS = min(params.maxQuorumVotesBPS, adjustedQuorumBPS); This also improves accuracy by not multiplying on the result of a division. And if the coefficient is represented in 4 decimals instead of 6, we can also get rid of the 1e2. For maximal gas savings the original four lines can be written in one: uint256 quorumBPS = min( params.maxQuorumVotesBPS, params.minQuorumVotesBPS + (params.quorumCoefficient * againstVotes) / totalSupply); Which saves 3 assignments and 2 operations.

 

Unnecessary function In NounsDAOLogicV2.sol the function castRefundableVoteInternal 533 is only called by castRefundableVote 503 and castRefundableVoteWithReason 518. The body of castRefundableVoteInternal can be directly incorporated in castRefundableVoteWithReason and the reason can similarly be eliminated by letting castRefundableVote instead call castRefundableVoteWithReason with a blank reason. This makes more sense and saves gas.

 

abstainVotes is not meaningfully used In NounsDAOLogicV2.sol and NounsDAOLogicV1.sol abstainVotes is only registered but never really used. Given the current quorum logic it should be considered for removal. However, note that this logic may be flawed (an issue reported separately), and if this is fixed abstainVotes may be needed.

 

ProposalState.Defeated is not meaningfully used ProposalState.Defeated, defined in NounsDAOInterfaces.sol L228 and L332, is only used in NounsDAOLogicV2.sol#L444 and NounsDAOLogicV1.sol#L433 as a state that returns before ProposalState.Succeeded returns. It is never used to indicate that the proposal is defeated, but merely as not yet succeeded (it can be tried again for success). Unless this is changed it may be removed to save gas, in which case the conditions for it can be moved to the check for Proposal.Succeeded (NounsDAOLogicV2.sol#L445 and NounsDAOLogicV1.sol#L434).

 

Possibly pointless function safe32 and safe16 The function safe32 in NounsDAOLogicV2.sol#L1018-1021 and ERC721Checkpointable.sol#L253-256 is only ever used to require that block.number is not greater than or equal to 2^32. However, this will not happen in more than a thousand years, and when it does the contract will break anyway. So if anything it should be in an assert rather than a require, but may as well be removed all together. The function safe16 in NounsDAOLogicV2.sol#L1023-1028 is only used with quorumVotesBPS, but this value is never set, aside from its default 0, and therefore this check is also pointless.

 

Repeated code The if-blocks NounsDAOLogicV2.sol#L926-933 and NounsDAOLogicV2.sol#L939-946 both contain the same code. Consider combining them in one if-block with both of their conditions combined.

 

Declaring constants as private rather than public saves deployment gas The compiler will give public constants a getter method, which costs gas during deployment. As they are constants they can be read directly from the verified source code.

Instances: NounsDAOLogicV2.sol: 59 62 65 68 71 74 77 80 83 86 89 92 95 98 101 105 NounsDAOLogicV1.sol: 67 70 73 76 79 82 85 88 91 94 97 101 ERC721Checkpointable.sol: 41 59 63

 

Function not called by the contract can be external instead of public

Instances: NounsDAOLogicV2.sol: 851 NounsDAOLogicV1.sol: 649 668 ERC721Checkpointable.sol: 126 163 ERC721Enumerable.sol: 61 76

 

Custom errors are cheaper than require-statements See this. Consider replacing require-statements with a string message with a custom error in an if-revert.

Instances: NounsDAOLogicV2.sol: 133 134 135 136 137 141 145 197 201 207 208 213 217 286 312 324 347 350 375 376 377 433 577 593 594 597 622 623 638 639 655 656 674 677 682 702 705 709 727 801 819 840 853 1019 NounsDAOLogicV1.sol: 122 123 124 125 126 130 134 138 187 191 197 198 203 207 275 301 313 336 339 364 365 366 422 485 501 502 505 530 531 546 547 563 564 581 582 599 617 638 651 NounsDAOProxy.sol: 79 80 ERC721Checkpointable.sol: 140 141 142 164 254 259 269 278 (254-278: make errorMessage a custom error) ERC721Enumerable.sol: 62 77

 

Pre-incrementing is cheaper than post-incrementing Consider replacing e.g. i++ with ++i.

Instances: NounsDAOLogicV2.sol: 226 292 330 357 382 NounsDAOLogicV1.sol: 216 281 319 346 371 ERC721Checkpointable.sol: 141

 

Pre-compute .length before repeated usage, especially in loops

Instances: NounsDAOLogicV2.sol: 202-208 292 330 357 382 NounsDAOLogicV1.sol: 192-198 281 319 346 371

 

Integer over-/underflow Where integers cannot over-/underflow they may be unchecked{} to save gas. In loops, make the unchecked increment in the end of the loop.

Instances: NounsDAOLogicV2.sol: 223 (votingDelay is required to be less than MAX_VOTING_DELAY) 224 (votingPeriod is required to be less than MAX_VOTIN_PERIOD) 291330357382 NounsDAOLogicV1.sol: 213 (votingDelay is required to be less than MAX_VOTING_DELAY) 214 (votingPeriod is required to be less than MAX_VOTIN_PERIOD) 281 319 346 371

 

Comparing to constant bool Replace foo == false with !foo.

Instances: NounsDAOLogicV2.sol: 597 NounsDAOLogicV1.sol: 505

 

bool in storage is more expensive to write to than uint256 Consider using a uint256 to represent truth values.

Instances: NounsDAOInterfaces.sol: 204 206 208 216 304 306 308 320 390 392 394

 

Unnecessary check of msg.sender != address(0) This cannot happen and any direct, or indirect, check of this is redundant.

Instances: NounsDAOLogicV2.sol: 375 819 NounsDAOLogicV1.sol: 364 617

 

Use calldata instead of memory for function parameters that are read-only

Instances: NounsDAOLogicV2.sol: 185 186 187 188 189 308 309 536 906 964 1018 NounsDAOLogicV1.sol: 175 176 177 178 179 297 298 NounsDAOProxy.sol: 94 ERC721Checkpointable.sol: 253 258 266 276

 

< is cheaper than <= Consider replacing support <= 2 with support < 3 in NounsDAOLogicV2.sol#L594 and NounsDAOLogicV1.sol#L502.