Nouns Builder contest - asutorufos's results

L-1 MISSING ZERO-ADDRESS CHECK IN THE SETTER FUNCTIONS AND INITILIAZERS Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L307

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L315

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L331

L-2 _SAFEMINT() SHOULD BE USED RATHER THAN _MINT() WHEREVER POSSIBLE _mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver

L-3 SETTERS SHOULD CHECK THE INPUT VALUE https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L316

N-1 NATSPEC IS INCOMPLETE Missing @returns token, metadata, auction, treasury, governor https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L104

N-2 PUBLIC FUNCTIONS CAN BE EXTERNAL It is good practice to mark functions as external instead of public if they are not called by the contract where they are defined. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L98 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L461

G-1 Using calldata instead of memory for read-only arguments in external functions saves gas When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L116-L121 This one below is public but since it not being called by the contract itself it could be put as external https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L98-L103

G-2 Using storage instead of memory for structs/arrays saves gas When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L358 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L415

G-3 ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L80 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L108

G-4 Using private rather than public for constants, saves gas https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L27

G-5 Empty blocks should be removed or emit something The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}}). Empty receive()/fallback() payable functions that are not used, can be removed to save deployment gas. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L269