Nouns Builder contest - c3phas's results

QA

Missing checks for address(0x0) when assigning values to address state variables

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

File: /src/auction/Auction.sol
41:        WETH = _weth;

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L76

File: /src/governance/governor/Governor.sol

76:        settings.vetoer = _vetoer;

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/manager/Manager.sol#L62-L66

File: /src/manager/Manager.sol
62:        tokenImpl = _tokenImpl;
63:        metadataImpl = _metadataImpl;
64:        auctionImpl = _auctionImpl;
65:        treasuryImpl = _treasuryImpl;
66:        governorImpl = _governorImpl;

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/utils/Ownable.sol#L46

File: /src/lib/utils/Ownable.sol
46:        _owner = _initialOwner;

Non assembly method exists

assembly{ id := chainid() } => uint256 id = block.chainid, assembly { size := extcodesize() } => uint256 size = address().code.length There are some automated tools that will flag a project as having higher complexity if there is inline-assembly, so it's best to avoid using it where it's not necessary

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/utils/Address.sol#L30-L34

File: /src/lib/utils/Address.sol
30:    function isContract(address _account) internal view returns (bool rv) {
31:        assembly {
32:            rv := gt(extcodesize(_account), 0)
33:        }
34:    }

We can modify the above code as follows

     function isContract(address _account) internal view returns (bool) {

          return account.code.length > 0;

see openzeppelin implementation on the same

State variable shadowing:

Shadowing state variables in derived contracts may be dangerous for critical variables such as contract owner (for e.g. where modifiers in base contracts check on base variables but shadowed variables are set mistakenly) and contracts incorrectly use base/shadowed variables. Do not shadow state variables

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/manager/Manager.sol#L80-L86

File: /src/manager/Manager.sol
80:    function initialize(address _owner) external initializer {
81:        // Ensure an owner is specified
82:        if (_owner == address(0)) revert ADDRESS_ZERO();
83:
84:        // Set the contract owner
85:        __Ownable_init(_owner);
86:    }

A similar variable is declared in an inherited contract, see below https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/utils/Ownable.sol#L18

File: /src/lib/utils/Ownable.sol
18:     address internal _owner;

constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name.

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L468

File: /src/governance/governor/Governor.sol
//@audit: 10_000
468:            return (settings.token.totalSupply() * settings.proposalThresholdBps) / 10_000;

//@audit: 10_000
475:            return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L88

File: /src/token/Token.sol
//@audit: 100
88:                if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

//@audit: 100
102:                uint256 schedule = 100 / founderPct;

//@audit: 100
118:                (baseTokenId += schedule) % 100;

//@audit: 100
179:        uint256 baseTokenId = _tokenId % 100;

//@audit: 100
271:        return tokenRecipient[_tokenId % 100];

abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions .Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

If all arguments are strings and or bytes, bytes.concat() should be used instead https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/manager/Manager.sol#L68-L71

File: /src/manager/Manager.sol
68:        metadataHash = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(_metadataImpl, "")));
71:        auctionHash = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(_auctionImpl, "")));
72:        treasuryHash = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(_treasuryImpl, "")));
73:        governorHash = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(_governorImpl, "")));

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/manager/Manager.sol#L167-L170

File: /src/manager/Manager.sol
167:        metadata = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, metadataHash)))));
168:        auction = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, auctionHash)))));
169:        treasury = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, treasuryHash)))));
170:        governor = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, governorHash)))));

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L161-L163

File: /src/lib/token/ERC721Votes.sol
161:            digest = keccak256(
162:                abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR(), keccak256(abi.encode(DELEGATION_TYPEHASH, _from, _to, nonces[_from]++, _deadline)))
163: );

ERC-721 transfers do not use safeTransferFrom

The cases where ERC-721 tokens are transferred are always using transferFrom and not safeTransferFrom. This means users need to be aware their tokens may get locked up in an unprepared contract recipient.

This may be intentional to reduce the potential for reentrancy, but it may strike users unprepared.

If it is by design, then it means users must be educated about this fact and the frontend would need to verify target addresses prior to submitting any transactions and hope that other frontends/integrations do not exists.

Should a ERC-721 compatible token be transferred to an unprepared contract, it would end up being locked up there.

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

File: /src/auction/Auction.sol
192:            token.transferFrom(address(this), _auction.highestBidder, _auction.tokenId);

_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. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren't lost if they're minted to contracts that cannot transfer them back out.

safeMint should be used in place of _mint in the following https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L161

File: /src/token/Token.sol
161:         _mint(minter, tokenId);

188:            _mint(tokenRecipient[baseTokenId].wallet, _tokenId);

Be careful however to respect the CEI pattern or add a re-entrancy guard as _safeMint adds a callback-check (_checkOnERC721Received) and a malicious onERC721Received could be exploited if not careful.

Missing a __gap[50] storage variable to allow for new storage variables in later versions

This is empty reserved space in storage that is put in place in Upgradeable contracts. It allows us to freely add new state variables in the future without compromising the storage compatibility with existing deployments. See Docs

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/proxy/UUPS.sol#L12

File: /src/lib/proxy/UUPS.sol
12: abstract contract UUPS is IUUPS, ERC1967Upgrade {

Looking at the OpenZeppelin UUPSUpgradeable.sol Implementation we can see the __gap variable is present there at Line 107

Hardcoded Values - Recommend using Enums

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L278-L288

File: /src/governance/governor/Governor.sol
278:            if (_support == 0) {
            
283:            } else if (_support == 1) {
              
288:            } else if (_support == 2) {

Upper case variables should be reserved for constants

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/utils/EIP712.sol#L25-L35

File: /src/lib/utils/EIP712.sol
25:    /// @notice The hash of the EIP-712 domain name
26:    bytes32 internal HASHED_NAME;

28:    /// @notice The hash of the EIP-712 domain version
29:    bytes32 internal HASHED_VERSION;

31:    /// @notice The domain separator computed upon initialization
32:    bytes32 internal INITIAL_DOMAIN_SEPARATOR;

34:    /// @notice The chain id upon initialization
35:    uint256 internal INITIAL_CHAIN_ID;

Lack of event emission after critical initialize() functions

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L54-L82

File: /src/auction/Auction.sol
54:    function initialize(
55:        address _token,
56:        address _founder,
57:        address _treasury,
58:        uint256 _duration,
59:        uint256 _reservePrice
60:    ) external initializer {
         ...
82:    }

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L57-L65

File: /src/governance/governor/Governor.sol
57:    function initialize(
58:        address _treasury,
59:        address _token,
60:        address _vetoer,
61:        uint256 _votingDelay,
62:        uint256 _votingPeriod,
63:        uint256 _proposalThresholdBps,
64:        uint256 _quorumThresholdBps
65:    ) external initializer {

Missing indexed values on events

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/IAuction.sol#L22

File: /src/auction/IAuction.sol
22:    event AuctionBid(uint256 tokenId, address bidder, uint256 amount, bool extended, uint256 endTime);

28:    event AuctionSettled(uint256 tokenId, address winner, uint256 amount);

34:    event AuctionCreated(uint256 tokenId, uint256 startTime, uint256 endTime);

38:    event DurationUpdated(uint256 duration);

42:    event ReservePriceUpdated(uint256 reservePrice);

46:    event MinBidIncrementPercentageUpdated(uint256 minBidIncrementPercentage);

50:    event TimeBufferUpdated(uint256 timeBuffer);

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/IGovernor.sol#L18-L26

File: /src/governance/governor/IGovernor.sol
18:    event ProposalCreated(
19:        bytes32 proposalId,
20:        address[] targets,
21:        uint256[] values,
22:        bytes[] calldatas,
23:        string description,
24:        bytes32 descriptionHash,
25:        Proposal proposal
26:    );


29:    event ProposalQueued(bytes32 proposalId, uint256 eta);

33:    event ProposalExecuted(bytes32 proposalId);

36:    event ProposalCanceled(bytes32 proposalId);

39:    event ProposalVetoed(bytes32 proposalId);

42:    event VoteCast(address voter, bytes32 proposalId, uint256 support, uint256 weight, string reason);

45:    event VotingDelayUpdated(uint256 prevVotingDelay, uint256 newVotingDelay);

48:    event VotingPeriodUpdated(uint256 prevVotingPeriod, uint256 newVotingPeriod);

51:    event ProposalThresholdBpsUpdated(uint256 prevBps, uint256 newBps);

54:    event QuorumVotesBpsUpdated(uint256 prevBps, uint256 newBps);

57:    event VetoerUpdated(address prevVetoer, address newVetoer);

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/ITreasury.sol#L16

File: /src/governance/treasury/ITreasury.sol
16:    event TransactionScheduled(bytes32 proposalId, uint256 timestamp);

19:    event TransactionCanceled(bytes32 proposalId);

22:    event TransactionExecuted(bytes32 proposalId, address[] targets, uint256[] values, bytes[] payloads);

25:    event DelayUpdated(uint256 prevDelay, uint256 newDelay);

28:    event GracePeriodUpdated(uint256 prevGracePeriod, uint256 newGracePeriod);

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/manager/IManager.sol#L21

File: /src/manager/IManager.sol
21:    event DAODeployed(address token, address metadata, address auction, address treasury, address governor);

26:    event UpgradeRegistered(address baseImpl, address upgradeImpl);

31:    event UpgradeRemoved(address baseImpl, address upgradeImpl);

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/IToken.sol#L21

File: /src/token/IToken.sol
21:    event MintScheduled(uint256 baseTokenId, uint256 founderId, Founder founder);

NatSpec is incomplete

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L93-L105

File: /src/governance/governor/Governor.sol

//@audit: Missing @return
93:    /// @notice Hashes a proposal's details into a proposal id
94:    /// @param _targets The target addresses to call
95:    /// @param _values The ETH values of each call
96:    /// @param _calldatas The calldata of each call
97:    /// @param _descriptionHash The hash of the description
98:    function hashProposal(
99:        address[] memory _targets,
100:        uint256[] memory _values,
101:        bytes[] memory _calldatas,
102:        bytes32 _descriptionHash
103:    ) public pure returns (bytes32) {
104:        return keccak256(abi.encode(_targets, _values, _calldatas, _descriptionHash));
105:    }

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L111-L121

//@audit: missing @return 
111:    /// @notice Creates a proposal
112:    /// @param _targets The target addresses to call
113:    /// @param _values The ETH values of each call
114:    /// @param _calldatas The calldata of each call
115:    /// @param _description The proposal description
116:    function propose(
117:        address[] memory _targets,
118:        uint256[] memory _values,
119:        bytes[] memory _calldatas,
120:        string memory _description
121:    ) external returns (bytes32) {

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L181-L186

File: /src/governance/governor/Governor.sol

//@audit: Missing @return
181:    /// @notice Casts a vote
182:    /// @param _proposalId The proposal id
183:    /// @param _support The support value (0 = Against, 1 = For, 2 = Abstain)
184:    function castVote(bytes32 _proposalId, uint256 _support) external returns (uint256) {
185:        return _castVote(_proposalId, msg.sender, _support, "");
186:    }

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L303-L305

File: /src/governance/governor/Governor.sol

//@audit: Missing @return eta
303:    /// @notice Queues a proposal
304:    /// @param _proposalId The proposal id
305:    function queue(bytes32 _proposalId) external returns (uint256 eta) {

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L204-L206

File: /src/token/metadata/MetadataRenderer.sol

//@audit: Missing @return aryAttributes,@return queryString
204:    /// @notice The properties and query string for a generated token
205:    /// @param _tokenId The ERC-721 token id
206:    function getAttributes(uint256 _tokenId) public view returns (bytes memory aryAttributes, bytes memory queryString) {

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L268-L269

File: /src/token/metadata/MetadataRenderer.sol

//@audit: Missing @return 
268:    /// @notice The contract URI
269:    function contractURI() external view returns (string memory) {


//@audit: Missing @return
284:    /// @notice The token URI
285:    /// @param _tokenId The ERC-721 token id
286:    function tokenURI(uint256 _tokenId) external view returns (string memory) {


//@audit: Missing @return
316:    /// @notice The associated ERC-721 token
317:    function token() external view returns (address) {

//@audit: Missing @return
321:    /// @notice The DAO treasury
322:    function treasury() external view returns (address) {

//@audit: Missing @return
326:    /// @notice The contract image
327:    function contractImage() external view returns (string memory) {

//@audit: Missing @return
331:    /// @notice The renderer base
332:    function rendererBase() external view returns (string memory) {

//@audit: Missing @return
336:    /// @notice The collection description
337:    function description() external view returns (string memory) {

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L142-L143

File: /src/token/Token.sol

//@audit: Missing @return tokenId
142:    /// @notice Mints tokens to the auction house for bidding and handles founder vesting
143:    function mint() external nonReentrant returns (uint256 tokenId) {


//@audit: Missing @return 
219:    /// @notice The URI for a token
220:    /// @param _tokenId The ERC-721 token id
221:    function tokenURI(uint256 _tokenId) public view override(IToken, ERC721) returns (string memory) {

//@audit: Missing @return
225:    /// @notice The URI for the contract
226:    function contractURI() public view override(IToken, ERC721) returns (string memory) {

//@audit: Missing @return
244:    /// @notice The vesting details of a founder
245:    /// @param _founderId The founder id
246:    function getFounder(uint256 _founderId) external view returns (Founder memory) {


//@audit: Missing @return
250:    /// @notice The vesting details of all founders
251:    function getFounders() external view returns (Founder[] memory) {

//@audit: Missing @return
269:    /// @param _tokenId The ERC-721 token id
270:    function getScheduledRecipient(uint256 _tokenId) external view returns (Founder memory) {

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/proxy/UUPS.sol#L60-L61

File: /src/lib/proxy/UUPS.sol

//@audit: Missing @return
60:  /// @notice The storage slot of the implementation address
61:    function proxiableUUID() external view notDelegated returns (bytes32) {

For loops

There is some inconsistency in how loops are declared across the codebase General structure used:

for (uint256 i = 0; CONDITION; ++i) { ... }

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L162

File: /src/governance/treasury/Treasury.sol
162:            for (uint256 i = 0; i < numTargets; ++i) {

Other parts of code use (note : i is not initilized, assumes the default value)

for (uint256 i; CONDITION; ++i) { ... }

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L80

File: /src/token/Token.sol
80:            for (uint256 i; i < numFounders; ++i) {

Try to stick to one.

FINDINGS

Cache storage values in memory to minimize SLOADs

The code can be optimized by minimizing the number of SLOADs.

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.

NB: Some functions have been truncated where neccessary to just show affected parts of the code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/utils/Initializable.sol#L33-L51

Initializable.sol.initializer(): _initialized should be cached

File: /src/lib/utils/Initializable.sol
33:    modifier initializer() {
34:        bool isTopLevelCall = !_initializing;

36:        if ((!isTopLevelCall || _initialized != 0) && (Address.isContract(address(this)) || _initialized != 1)) revert ALREADY_INITIALIZED();

51:    }

diff --git a/src/lib/utils/Initializable.sol b/src/lib/utils/Initializable.sol
index c0e8c83..2e5693d 100644
--- a/src/lib/utils/Initializable.sol
+++ b/src/lib/utils/Initializable.sol
@@ -32,8 +32,8 @@ abstract contract Initializable is IInitializable {
     /// @dev Enables initializing upgradeable contracts
     modifier initializer() {
         bool isTopLevelCall = !_initializing;
-
-        if ((!isTopLevelCall || _initialized != 0) && (Address.isContract(address(this)) || _initialized != 1)) revert ALREADY_INITIALIZED();
+        uint8 temp = _initialized;
+        if ((!isTopLevelCall || temp != 0) && (Address.isContract(address(this)) || temp != 1)) revert ALREADY_INITIALIZED();

         _initialized = 1;

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

The result of a function call should be cached rather than re-calling the function

External calls are expensive. Consider caching the following:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L116-L129

Governor.sol.propose(): proposalThreshold() being called twice despite having a cached value(saves ~1333 gas)

Average Gas Before: 61309
Average Gas after: 59706

File: /src/governance/governor/Governor.sol
116:    function propose(
117:        address[] memory _targets,
118:        uint256[] memory _values,
119:        bytes[] memory _calldatas,
120:       string memory _description
121:    ) external returns (bytes32) {
122:        // Get the current proposal threshold
123:        uint256 currentProposalThreshold = proposalThreshold();//@audit: First proposalThreshold() call

125:        // Cannot realistically underflow and `getVotes` would revert
126:        unchecked {
127:            // Ensure the caller's voting weight is greater than or equal to the threshold
128:            if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();//@audit: Second proposalThreshold() call
129:        }

diff --git a/src/governance/governor/Governor.sol b/src/governance/governor/Governor.sol
index 0d963c5..ce1ad1a 100644
--- a/src/governance/governor/Governor.sol
+++ b/src/governance/governor/Governor.sol
@@ -125,7 +125,7 @@ contract Governor is IGovernor, UUPS, Ownable, EIP712, GovernorStorageV1 {
         // Cannot realistically underflow and `getVotes` would revert
         unchecked {
             // Ensure the caller's voting weight is greater than or equal to the threshold
-            if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
+            if (getVotes(msg.sender, block.timestamp - 1) < currentProposalThreshold) revert BELOW_PROPOSAL_THRESHOLD();
         }

Internal/Private functions only called once can be inlined to save gas

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

Affected code: https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L250-L252

File: /src/token/metadata/MetadataRenderer.sol
250:    function _generateSeed(uint256 _tokenId) private view returns (uint256) {
251:        return uint256(keccak256(abi.encode(_tokenId, blockhash(block.number), block.coinbase, block.timestamp)));
252:    }

The above is only called once on Line 176

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L255-L262

File: /src/token/metadata/MetadataRenderer.sol
255:    function _getItemImage(Item memory _item, string memory _propertyName) private view returns (string memory) {

The above is only called on Line 244

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L71

File: /src/token/Token.sol
71:     function _addFounders(IManager.FounderParams[] calldata _founders) internal {

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L130

File: /src/token/Token.sol
130:    function _getNextTokenId(uint256 _tokenId) internal view returns (uint256) {

The above is only used on Line 110

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L177

File: /src/token/Token.sol
177:    function _isForFounder(uint256 _tokenId) private returns (bool) {

The above is only used on Line 157

Help the Optimizer by saving a storage variable's reference instead of repeatedly fetching it

Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time. To help the optimizer,declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array. As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L177-L199

Token.sol._isForFounder(): tokenRecipient[baseTokenId] should be cached

File: /src/token/Token.sol
177:    function _isForFounder(uint256 _tokenId) private returns (bool) {

181:        // If there is no scheduled recipient:
182:        if (tokenRecipient[baseTokenId].wallet == address(0)) {
183:            return false;

185:            // Else if the founder is still vesting:
186:        } else if (block.timestamp < tokenRecipient[baseTokenId].vestExpiry) {
187:            // Mint the token to the founder
188:            _mint(tokenRecipient[baseTokenId].wallet, _tokenId);

190:            return true;

199:    }

Using calldata instead of memory for read-only arguments in external functions saves gas(See audit tags in the code)

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.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L116-L121

File: /src/governance/governor/Governor.sol
//@audit: use calldata for the variables _targets,_values,_calldatas, _description
116:    function propose(
117:        address[] memory _targets,
118:        uint256[] memory _values,
119:        bytes[] memory _calldatas,
120:        string memory _description
121:    ) external returns (bytes32) {

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L192-L198

File: /src/governance/governor/Governor.sol

//@audit: use calldata for the variables _reason
192:    function castVoteWithReason(
193:        bytes32 _proposalId,
194:        uint256 _support,
195:        string memory _reason
196:    ) external returns (uint256) {
197:        return _castVote(_proposalId, msg.sender, _support, _reason);
198:    }

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L324-L329

File: /src/governance/governor/Governor.sol

//@audit: use calldata for the variables _targets,_values,_calldatas
324:    function execute(
325:        address[] calldata _targets,
326:        uint256[] calldata _values,
327:        bytes[] calldata _calldatas,
328:        bytes32 _descriptionHash
329:    ) external payable returns (bytes32) {

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L101-L108

File: /src/governance/treasury/Treasury.sol

//@audit: use calldata for the variables _targets,_values,_calldatas
101:    function hashProposal(
102:        address[] calldata _targets,
103:        uint256[] calldata _values,
104:        bytes[] calldata _calldatas,
105:        bytes32 _descriptionHash
106:    ) public pure returns (bytes32) {
107:        return keccak256(abi.encode(_targets, _values, _calldatas, _descriptionHash));
108:    }

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L141-L146

File: /src/governance/treasury/Treasury.sol

//@audit: use calldata for the variables _targets,_values,_calldatas
141:    function execute(
142:        address[] calldata _targets,
143:        uint256[] calldata _values,
144:        bytes[] calldata _calldatas,
145:        bytes32 _descriptionHash
146:    ) external payable onlyOwner {

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L347-L351

File: /src/token/metadata/MetadataRenderer.sol

//@audit: use calldata for the variable _newContractImage
347:    function updateContractImage(string memory _newContractImage) external onlyOwner {
348:        emit ContractImageUpdated(settings.contractImage, _newContractImage);
            ...
350:        settings.contractImage = _newContractImage;
351:    }

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L355-L359

File: /src/token/metadata/MetadataRenderer.sol

//@audit: use calldata for the variable _newRendererBase
355:    function updateRendererBase(string memory _newRendererBase) external onlyOwner {
356:        emit RendererBaseUpdated(settings.rendererBase, _newRendererBase);
            ...
358:        settings.rendererBase = _newRendererBase;
359:    }

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L363-L367

File: /src/token/metadata/MetadataRenderer.sol

//@audit: use calldata for the variable _newDescription
363:    function updateDescription(string memory _newDescription) external onlyOwner {
364:        emit DescriptionUpdated(settings.description, _newDescription);
            ...
366:        settings.description = _newDescription;
367:    }

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/proxy/UUPS.sol#L55-L58

File: /src/lib/proxy/UUPS.sol

//@audit: use calldata for _data
55:    function upgradeToAndCall(address _newImpl, bytes memory _data) external payable onlyProxy {
56:        _authorizeUpgrade(_newImpl);
57:        _upgradeToAndCallUUPS(_newImpl, _data, true);
58:    }

Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/storage/TokenStorageV1.sol#L15-L19

File: /src/token/storage/TokenStorageV1.sol
15:    mapping(uint256 => Founder) internal founder;
   
19:    mapping(uint256 => Founder) internal tokenRecipient;

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/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L92

File: /src/auction/Auction.sol
92:        Auction memory _auction = auction;

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

File: /src/governance/governor/Governor.sol
358:        Proposal memory proposal = proposals[_proposalId];

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

File: /src/governance/governor/Governor.sol
415:        Proposal memory proposal = proposals[_proposalId];

508:        Proposal memory proposal = proposals[_proposalId];

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

File: /src/auction/Auction.sol
169:        Auction memory _auction = auction;

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L216

File: /src/token/metadata/MetadataRenderer.sol
216:        uint16[16] memory tokenAttributes = attributes[_tokenId];

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L234

File: /src/token/metadata/MetadataRenderer.sol
234:                Property memory property = properties[i];

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L240

File: /src/token/metadata/MetadataRenderer.sol
240:                Item memory item = property.items[attribute];

Use Shift Right/Left instead of Division/Multiplication

While the DIV / MUL opcode uses 5 gas, the SHR / SHL opcode only uses 3 gas. Furthermore, beware that Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting. Eventually, overflow checks are never performed for shift operations as they are done for arithmetic operations. Instead, the result is always truncated, so the calculation can be unchecked in Solidity version 0.8+

    Use >> 1 instead of / 2

relevant source

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L95

File: /src/lib/token/ERC721Votes.sol
95:                 middle = high - (high - low) / 2;

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L208-L216

File: /src/governance/governor/Governor.sol

//@audit: uint8 _v
208:    function castVoteBySig(
209:        address _voter,
210:        bytes32 _proposalId,
211:        uint256 _support,
212:        uint256 _deadline,
213:        uint8 _v,
214:        bytes32 _r,
215:        bytes32 _s
216:    ) external returns (uint256) {

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L144-L151

File: /src/lib/token/ERC721Votes.sol

//@audit: uint8 _v
144:    function delegateBySig(
145:        address _from,
146:        address _to,
147:        uint256 _deadline,
148:        uint8 _v,
149:        bytes32 _r,
150:        bytes32 _s
151:    ) external {

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

consequences:

• Each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..)

• Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

See: ethereum/solidity#9232

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L27

File: /src/governance/governor/Governor.sol
27:    bytes32 public constant VOTE_TYPEHASH = keccak256("Vote(address voter,uint256 proposalId,uint256 support,uint256 nonce,uint256 deadline)");

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/utils/EIP712.sol#L19

File: /src/lib/utils/EIP712.sol
19:    bytes32 internal constant DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L21

File: /src/lib/token/ERC721Votes.sol
21:    bytes32 internal constant DELEGATION_TYPEHASH = keccak256("Delegation(address from,address to,uint256 nonce,uint256 deadline)");

Proof: can be tested on remix also, showing results for foundry

pragma solidity 0.8.15;
contract Constant{
    bytes32 constant noteDenom = keccak256(bytes("NOTE"));
    function doConstant() external view returns(bytes32){
        return noteDenom;
    }
}
contract Immutable{
    bytes32 immutable noteDenom = keccak256(bytes("NOTE"));
    function doImmutable() external view returns(bytes32){
        return noteDenom;
    }
}

Results from : forge test --gas-report --optimize

Running 1 test for test/Constant.t.sol:ConstantTest
[PASS] testdoThing() (gas: 5416)
Test result: ok. 1 passed; 0 failed; finished in 420.08µs

Running 1 test for test/Immutable.t.sol:ImmutableTest
[PASS] testdoThing() (gas: 5356)
Test result: ok. 1 passed; 0 failed; finished in 389.91µs

Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

See source Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

Instances affected include

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/storage/GovernorStorageV1.sol#L19

File: /src/governance/governor/storage/GovernorStorageV1.sol
19:    mapping(bytes32 => mapping(address => bool)) internal hasVoted;

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/manager/storage/ManagerStorageV1.sol#L10

File: /src/manager/storage/ManagerStorageV1.sol
10:    mapping(address => mapping(address => bool)) internal isUpgrade;

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/utils/Initializable.sol#L20

File: /src/lib/utils/Initializable.sol
20:    bool internal _initializing;

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/utils/Pausable.sol#L15

File: /src/lib/utils/Pausable.sol
15:    bool internal _paused;

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721.sol#L38

File: /src/lib/token/ERC721.sol
38:    mapping(address => mapping(address => bool)) internal operatorApprovals;

Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L27

File: /src/governance/governor/Governor.sol
27:    bytes32 public constant VOTE_TYPEHASH = keccak256("Vote(address voter,uint256 proposalId,uint256 support,uint256 nonce,uint256 deadline)");