Nouns Builder contest - Tomo's results

Lines of code

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

Vulnerability details

🎨 Category

Governance

💥 Impact

If the benefit to be gained from the outcome of the vote is less than the cost of obtaining the right to vote, the outcome of the vote is influenced

📝 Proof of Concept

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L248-L297 Vampire Attack Explained https://finematics.com/vampire-attack-sushiswap-explained/

🚜 Tools Used

Manual

✅ Recommended Mitigation Steps

• Add blacklist check for _castVote function.

if(isBlacklisted(msg.sender) revert BLACKLISTED();

• Create a function to delete the voting power and add the address to the blacklist.

function addBlackList(address _blackList) external onlyOwner {
		isBlackListed[_blackList] = true;
		emit Blacklisted(_blackList);
}

function deleteVotingPower(address _blackListed,uint _weight, uint _support) external onlyOwner {
    if (_support > 2) revert INVALID_VOTE();
    require(hasVoted[_proposalId][_blackListed],"NO VOTING BY BLACKLISTED");
    if (_support == 0) {
      proposal.againstVotes -= _weight;
  } else if (_support == 1) {
      proposal.forVotes -= _weight;
  } else if (_support == 2) {
      // Update the total number of votes abstaining
      proposal.abstainVotes -= _weight;
  }
}

👬 Similar Issue

https://github.com/code-423n4/2022-05-aura-findings/issues/278

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L88 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L149 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L223-L224

Vulnerability details

🎨 Category

Overflow/Underflow

💥 Impact

Without checking value is smaller than the limit of the variable’s type, it happens silently overflow.

This causes unexpected revert

📝 Proof of Concept

1. The founderPct is declared as uint256. This means the limit of this variable is 2^256.
2. uint8(founderPct)): This code means the type of founderPct changing to uint8. This means the limit of new types is 2^8.
3. If the founderPct is in 2^8+1 ~ 2^256, it happens overflow.

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

uint256 founderPct = _founders[i].ownershipPct;
// Continue if no ownership is specified
if (founderPct == 0) continue;
// Update the total ownership and ensure it's valid
if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

🚜 Tools Used

Manual

✅ Recommended Mitigation Steps

Use SafeCast library to prevent this error.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/utils/SafeCast.sol

// before
if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();
// after
if ((totalOwnership += SafeCast.toUint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

👬 Similar Issue

https://github.com/code-423n4/2022-06-notional-coop-findings/issues/239

QA Report

✅ N-1: Consider addings checks for signature malleability

📝 Description

You should consider addings checks for signature malleability

💡 Recommendation

Use OpenZeppelin's ECDSA contract rather than calling ecrecover() directly

🔍 Findings:

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L236 address recoveredAddress = ecrecover(digest, _v, _r, _s);

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L167 address recoveredAddress = ecrecover(digest, _v, _r, _s);

✅ N-2: Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

📝 Description

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

💡 Recommendation

You should use immutable instead of constant

🔍 Findings:

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L27 bytes32 public constant VOTE_TYPEHASH = keccak256("Vote(address voter,uint256 proposalId,uint256 support,uint256 nonce,uint256 deadline)");

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L21 bytes32 internal constant DELEGATION_TYPEHASH = keccak256("Delegation(address from,address to,uint256 nonce,uint256 deadline)");

2022-09-nouns-builder/blob/main/src/lib/utils/EIP712.sol#L19 bytes32 internal constant DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");

✅ N-3: No use of two-phase ownership transfers

📝 Description

Consider adding a two-phase transfer, where the current owner nominates the next owner, and the next owner has to call accept*() to become the new owner. This prevents passing the ownership to an account that is unable to use it.

💡 Recommendation

Consider implementing a two step process where the admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of admin to fully succeed. This ensures the nominated EOA account is a valid and active account.

🔍 Findings:

2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L5 import { Ownable } from "../lib/utils/Ownable.sol";

2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L22 contract Auction is IAuction, UUPS, Ownable, ReentrancyGuard, Pausable, AuctionStorageV1 {

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L5 import { Ownable } from "../../lib/utils/Ownable.sol";

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L21 contract Governor is IGovernor, UUPS, Ownable, EIP712, GovernorStorageV1 {

2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L19 contract Treasury is ITreasury, UUPS, Ownable, TreasuryStorageV1 {

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L19 contract Manager is IManager, UUPS, Ownable, ManagerStorageV1 {

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L19 contract MetadataRenderer is IPropertyIPFSMetadataRenderer, UUPS, Ownable, MetadataRendererStorageV1 {

✅ N-4: Use a more recent version of solidity

📝 Description

Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked (<bytes>, <bytes>) Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked (<str>, <str>) Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

💡 Recommendation

Use more recent version of solidity.

🔍 Findings:

2022-09-nouns-builder/blob/main/src/governance/treasury/types/TreasuryTypesV1.sol#L2 pragma solidity 0.8.15;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IEIP712.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IERC1967Upgrade.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IERC721.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IERC721Votes.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IInitializable.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IOwnable.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IPausable.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/proxy/ERC1967Proxy.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/proxy/ERC1967Upgrade.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/proxy/UUPS.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/token/ERC721.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/Address.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/EIP712.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/Initializable.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/Ownable.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/Pausable.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/ReentrancyGuard.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/SafeCast.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/TokenReceiver.sol#L2 pragma solidity ^0.8.0;

✅ N-5: Use string.concat() orbytes.concat()

📝 Description

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))Solidity version 0.8.12 introduces string.concat()(vs abi.encodePacked(<str>,<str>))

💡 Recommendation

Use concat instead of abi.encodePacked

🔍 Findings:

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L227 abi.encodePacked(

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L162 abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR(), keccak256(abi.encode(DELEGATION_TYPEHASH, _from, _to, nonces[_from]++, _deadline)))

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L68 metadataHash = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(_metadataImpl, "")));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L69 auctionHash = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(_auctionImpl, "")));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L70 treasuryHash = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(_treasuryImpl, "")));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L71 governorHash = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(_governorImpl, "")));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L167 metadata = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, metadataHash)))));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L168 auction = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, auctionHash)))));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L169 treasury = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, treasuryHash)))));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L170 governor = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, governorHash)))));

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L208 queryString = abi.encodePacked(

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L243 aryAttributes = abi.encodePacked(aryAttributes, '"', property.name, '": "', item.name, '"', isLast ? "" : ",");

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L244 queryString = abi.encodePacked(queryString, "&images=", _getItemImage(item, property.name));

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L259 abi.encodePacked(ipfsData[_item.referenceSlot].baseUri, _propertyName, "/", _item.name, ipfsData[_item.referenceSlot].extension)

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L272 abi.encodePacked(

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L290 abi.encodePacked(

Gas | QA

✅ G-1: Don't Initialize Variables with Default Value

📝 Description

Uninitialized variables are assigned with the types default value. Explicitly initializing a variable with it's default value costs unnecesary gas. Not overwriting the default for stack variables saves 8 gas. Storage and memory variables have larger savings

💡 Recommendation

Delete useless variable declarations to save gas.

🔍 Findings:

2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L162 for (uint256 i = 0; i < numTargets; ++i) {

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L119 for (uint256 i = 0; i < numNewProperties; ++i) {

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L133 for (uint256 i = 0; i < numNewItems; ++i) {

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L189 for (uint256 i = 0; i < numProperties; ++i) {

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L229 for (uint256 i = 0; i < numProperties; ++i) {

2022-09-nouns-builder/test/Gov.t.sol#L79 for (uint256 i = 0; i < _numAgainst; ++i) {

2022-09-nouns-builder/test/Gov.t.sol#L86 for (uint256 i = 0; i < _numFor; ++i) {

2022-09-nouns-builder/test/Gov.t.sol#L93 for (uint256 i = 0; i < _numAbstain; ++i) {

✅ G-2: Use != 0 instead of > 0 for Unsigned Integer Comparison

📝 Description

Use != 0 when comparing uint variables to zero, which cannot hold values below zero

💡 Recommendation

You should change from > 0 to !=0.

🔍 Findings:

2022-09-nouns-builder/blob/main/src/lib/proxy/ERC1967Upgrade.sol#L61 if (_data.length > 0 || _forceCall) {

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L203 if (_from != _to && _amount > 0) {

2022-09-nouns-builder/blob/main/src/lib/utils/Address.sol#L50 if (_returndata.length > 0) {

✅ G-3: Use Shift Right/Left instead of Division/Multiplication if possible

📝 Description

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left. While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. urthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

💡 Recommendation

You should change multiplication and division by powers of two to bit shift.

🔍 Findings:

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L95 middle = high - (high - low) / 2;

✅ G-4: Use assembly balance ETH

📝 Description

You can save 159 gas per instance if using assembly to check for balance of address

💡 Recommendation

Please follow this link to make corrections

🔍 Findings:

2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L346 if (address(this).balance < _amount) revert INSOLVENT();

2022-09-nouns-builder/test/Auction.t.sol#L69 uint256 beforeAuctionBalance = address(auction).balance;

2022-09-nouns-builder/test/Auction.t.sol#L79 uint256 afterBidderBalance = bidder1.balance;

2022-09-nouns-builder/test/Auction.t.sol#L80 uint256 afterAuctionBalance = address(auction).balance;

2022-09-nouns-builder/test/Auction.t.sol#L108 uint256 bidder1BeforeBalance = bidder1.balance;

2022-09-nouns-builder/test/Auction.t.sol#L109 uint256 bidder2BeforeBalance = bidder2.balance;

2022-09-nouns-builder/test/Auction.t.sol#L119 uint256 bidder1AfterBalance = bidder1.balance;

2022-09-nouns-builder/test/Auction.t.sol#L120 uint256 bidder2AfterBalance = bidder2.balance;

2022-09-nouns-builder/test/Auction.t.sol#L124 assertEq(address(auction).balance, 0.5 ether);

2022-09-nouns-builder/test/Auction.t.sol#L191 assertEq(address(treasury).balance, 1 ether);

2022-09-nouns-builder/test/utils/mocks/WETH.sol#L40 return address(this).balance;

✅ G-5: Use assembly to hash

📝 Description

See this issue You can save about 5000 gas per instance in deploying contracrt. You can save about 80 gas per instance if using assembly to execute the function.

💡 Recommendation

Please follow this link to make corrections

🔍 Findings:

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L27 bytes32 public constant VOTE_TYPEHASH = keccak256("Vote(address voter,uint256 proposalId,uint256 support,uint256 nonce,uint256 deadline)");

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L104 return keccak256(abi.encode(_targets, _values, _calldatas, _descriptionHash));

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L142 bytes32 descriptionHash = keccak256(bytes(_description));

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L226 digest = keccak256(

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L230 keccak256(abi.encode(VOTE_TYPEHASH, _voter, _proposalId, _support, nonces[_voter]++, _deadline))

2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L107 return keccak256(abi.encode(_targets, _values, _calldatas, _descriptionHash));

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L21 bytes32 internal constant DELEGATION_TYPEHASH = keccak256("Delegation(address from,address to,uint256 nonce,uint256 deadline)");

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L161 digest = keccak256(

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L162 abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR(), keccak256(abi.encode(DELEGATION_TYPEHASH, _from, _to, nonces[_from]++, _deadline)))

2022-09-nouns-builder/blob/main/src/lib/utils/EIP712.sol#L19 bytes32 internal constant DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");

2022-09-nouns-builder/blob/main/src/lib/utils/EIP712.sol#L49 HASHED_NAME = keccak256(bytes(_name));

2022-09-nouns-builder/blob/main/src/lib/utils/EIP712.sol#L50 HASHED_VERSION = keccak256(bytes(_version));

2022-09-nouns-builder/blob/main/src/lib/utils/EIP712.sol#L69 return keccak256(abi.encode(DOMAIN_TYPEHASH, HASHED_NAME, HASHED_VERSION, block.chainid, address(this)));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L68 metadataHash = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(_metadataImpl, "")));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L69 auctionHash = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(_auctionImpl, "")));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L70 treasuryHash = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(_treasuryImpl, "")));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L71 governorHash = keccak256(abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(_governorImpl, "")));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L167 metadata = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, metadataHash)))));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L168 auction = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, auctionHash)))));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L169 treasury = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, treasuryHash)))));

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L170 governor = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, governorHash)))));

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L251 return uint256(keccak256(abi.encode(_tokenId, blockhash(block.number), block.coinbase, block.timestamp)));

2022-09-nouns-builder/test/Gov.t.sol#L175 bytes32 descriptionHash = keccak256(bytes(""));

2022-09-nouns-builder/test/Gov.t.sol#L251 bytes32 descriptionHash = keccak256(bytes(""));

2022-09-nouns-builder/test/Gov.t.sol#L329 bytes32 digest = keccak256(

2022-09-nouns-builder/test/Gov.t.sol#L330 abi.encodePacked("\x19\x01", domainSeparator, keccak256(abi.encode(voteTypeHash, voter1, proposalId, FOR, voterNonce, deadline)))

2022-09-nouns-builder/test/Gov.t.sol#L395 bytes32 digest = keccak256(

2022-09-nouns-builder/test/Gov.t.sol#L396 abi.encodePacked("\x19\x01", domainSeparator, keccak256(abi.encode(voteTypeHash, voter1, proposalId, FOR, voterNonce, deadline)))

2022-09-nouns-builder/test/Gov.t.sol#L418 bytes32 digest = keccak256(

2022-09-nouns-builder/test/Gov.t.sol#L419 abi.encodePacked("\x19\x01", domainSeparator, keccak256(abi.encode(voteTypeHash, voter1, proposalId, FOR, voterNonce + 1, deadline)))

2022-09-nouns-builder/test/Gov.t.sol#L441 bytes32 digest = keccak256(

2022-09-nouns-builder/test/Gov.t.sol#L442 abi.encodePacked("\x19\x01", domainSeparator, keccak256(abi.encode(voteTypeHash, voter1, proposalId, FOR, voterNonce, deadline)))

2022-09-nouns-builder/test/Gov.t.sol#L614 governor.execute(targets, values, calldatas, keccak256(bytes("")));

2022-09-nouns-builder/test/Gov.t.sol#L668 governor.execute(targets, values, calldatas, keccak256(bytes("")));

2022-09-nouns-builder/test/Gov.t.sol#L680 bytes32 descriptionHash = keccak256(bytes("test"));

2022-09-nouns-builder/test/Gov.t.sol#L736 governor.execute(targets, values, calldatas, keccak256(bytes("")));

2022-09-nouns-builder/test/Gov.t.sol#L771 governor.execute(targets, values, calldatas, keccak256(bytes("")));

✅ G-6: Solidity Version should be the latest

📝 Description

Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

💡 Recommendation

You should consider with your team members whether you should choose the latest version. The latest version has its advantages, but also it has disadvantages, such as the possibility of unknown bugs.

🔍 Findings:

2022-09-nouns-builder/blob/main/src/lib/interfaces/IEIP712.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IERC1967Upgrade.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IERC721.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IERC721Votes.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IInitializable.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IOwnable.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/interfaces/IPausable.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/proxy/ERC1967Proxy.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/proxy/ERC1967Upgrade.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/proxy/UUPS.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/token/ERC721.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/Address.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/EIP712.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/Initializable.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/Ownable.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/Pausable.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/ReentrancyGuard.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/SafeCast.sol#L2 pragma solidity ^0.8.4;

2022-09-nouns-builder/blob/main/src/lib/utils/TokenReceiver.sol#L2 pragma solidity ^0.8.0;

✅ G-7: Empty blocks should be removed or emit something

📝 Description

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.

💡 Recommendation

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.

🔍 Findings:

2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L269 receive() external payable {}

✅ G-8: Functions guaranteed to revert when called by normal users can be marked payable

📝 Description

See this issue If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost Saves 2400 gas per instance in deploying contracrt. Saves about 20 gas per instance if using assembly to executing the function.

💡 Recommendation

You should add the keyword payable to those functions.

🔍 Findings:

2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L244 function unpause() external onlyOwner {

2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L263 function pause() external onlyOwner {

2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L307 function setDuration(uint256 _duration) external onlyOwner {

2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L315 function setReservePrice(uint256 _reservePrice) external onlyOwner {

2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L323 function setTimeBuffer(uint256 _timeBuffer) external onlyOwner {

2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L331 function setMinimumBidIncrement(uint256 _percentage) external onlyOwner {

2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L374 function _authorizeUpgrade(address _newImpl) internal view override onlyOwner {

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L564 function updateVotingDelay(uint256 _newVotingDelay) external onlyOwner {

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L572 function updateVotingPeriod(uint256 _newVotingPeriod) external onlyOwner {

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L580 function updateProposalThresholdBps(uint256 _newProposalThresholdBps) external onlyOwner {

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L588 function updateQuorumThresholdBps(uint256 _newQuorumVotesBps) external onlyOwner {

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L596 function updateVetoer(address _newVetoer) external onlyOwner {

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L605 function burnVetoer() external onlyOwner {

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L618 function _authorizeUpgrade(address _newImpl) internal view override onlyOwner {

2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L116 function queue(bytes32 _proposalId) external onlyOwner returns (uint256 eta) {

2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L146 ) external payable onlyOwner {

2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L180 function cancel(bytes32 _proposalId) external onlyOwner {

2022-09-nouns-builder/blob/main/src/lib/proxy/UUPS.sol#L47 function upgradeTo(address _newImpl) external onlyProxy {

2022-09-nouns-builder/blob/main/src/lib/proxy/UUPS.sol#L55 function upgradeToAndCall(address _newImpl, bytes memory _data) external payable onlyProxy {

2022-09-nouns-builder/blob/main/src/lib/utils/Ownable.sol#L63 function transferOwnership(address _newOwner) public onlyOwner {

2022-09-nouns-builder/blob/main/src/lib/utils/Ownable.sol#L71 function safeTransferOwnership(address _newOwner) public onlyOwner {

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L187 function registerUpgrade(address _baseImpl, address _upgradeImpl) external onlyOwner {

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L196 function removeUpgrade(address _baseImpl, address _upgradeImpl) external onlyOwner {

2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L209 function _authorizeUpgrade(address _newImpl) internal override onlyOwner {}

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L95 ) external onlyOwner {

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L347 function updateContractImage(string memory _newContractImage) external onlyOwner {

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L355 function updateRendererBase(string memory _newRendererBase) external onlyOwner {

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L363 function updateDescription(string memory _newDescription) external onlyOwner {

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L376 function _authorizeUpgrade(address _impl) internal view override onlyOwner {

✅ G-9: Splitting require() statements that use && saves gas

📝 Description

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper

💡 Recommendation

You should change one require which has && to two simple require() statements to save gas

🔍 Findings:

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L363 if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) > proposal.proposalThreshold)

2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L89 return timestamps[_proposalId] != 0 && block.timestamp >= timestamps[_proposalId];

2022-09-nouns-builder/blob/main/src/lib/token/ERC721.sol#L105 if (msg.sender != owner && !operatorApprovals[owner][msg.sender]) revert INVALID_APPROVAL();

2022-09-nouns-builder/blob/main/src/lib/token/ERC721.sol#L134 if (msg.sender != _from && !operatorApprovals[_from][msg.sender] && msg.sender != tokenApprovals[_tokenId]) revert INVALID_APPROVAL();

2022-09-nouns-builder/blob/main/src/lib/token/ERC721.sol#L165 Address.isContract(_to) &&

2022-09-nouns-builder/blob/main/src/lib/token/ERC721.sol#L183 Address.isContract(_to) &&

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L203 if (_from != _to && _amount > 0) {

2022-09-nouns-builder/blob/main/src/lib/utils/Initializable.sol#L36 if ((!isTopLevelCall || _initialized != 0) && (Address.isContract(address(this)) || _initialized != 1)) revert ALREADY_INITIALIZED();

2022-09-nouns-builder/test/Auction.t.sol#L63 vm.assume(_amount >= auction.reservePrice() && _amount <= bidder1.balance);

2022-09-nouns-builder/test/utils/NounsBuilderTest.sol#L108 require(numFounders == _percents.length && numFounders == _vestingEnds.length);

2022-09-nouns-builder/test/utils/mocks/WETH.sol#L60 if (src != msg.sender && allowance[src][msg.sender] != type(uint128).max) {

✅ G-10: Use ++i instead of i++

📝 Description

You can get cheaper for loops (at least 25 gas, however can be up to 80 gas under certain conditions), by rewriting The post-increment operation (i++) boils down to saving the original value of i, incrementing it and saving that to a temporary place in memory, and then returning the original value; only after that value is returned is the value of i actually updated (4 operations).On the other hand, in pre-increment doesn't need to store temporary(2 operations) so, the pre-increment operation uses less opcodes and is thus more gas efficient.

💡 Recommendation

You should change from i++ to ++i.

🔍 Findings:

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L230 keccak256(abi.encode(VOTE_TYPEHASH, _voter, _proposalId, _support, nonces[_voter]++, _deadline))

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L162 abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR(), keccak256(abi.encode(DELEGATION_TYPEHASH, _from, _to, nonces[_from]++, _deadline)))

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L207 uint256 nCheckpoints = numCheckpoints[_from]++;

2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L222 uint256 nCheckpoints = numCheckpoints[_to]++;

2022-09-nouns-builder/blob/main/src/token/Token.sol#L91 uint256 founderId = settings.numFounders++;

2022-09-nouns-builder/blob/main/src/token/Token.sol#L154 tokenId = settings.totalSupply++;

✅ G-11: Use x=x+y instad of x+=y

📝 Description

You can save about 35 gas per instance if you change from x+=y**** to x=x+y This works equally well for subtraction, multiplication and division.

💡 Recommendation

You should change from x+=y to x=x+y.

🔍 Findings:

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L280 proposal.againstVotes += uint32(weight);

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L285 proposal.forVotes += uint32(weight);

2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L290 proposal.abstainVotes += uint32(weight);

2022-09-nouns-builder/blob/main/src/token/Token.sol#L88 if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

2022-09-nouns-builder/blob/main/src/token/Token.sol#L118 (baseTokenId += schedule) %!;(MISSING)

2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L140 _propertyId += numStoredProperties;

2022-09-nouns-builder/test/utils/mocks/WETH.sol#L28 balanceOf[msg.sender] += msg.value;

2022-09-nouns-builder/test/utils/mocks/WETH.sol#L34 balanceOf[msg.sender] -= wad;

2022-09-nouns-builder/test/utils/mocks/WETH.sol#L62 allowance[src][msg.sender] -= wad;

2022-09-nouns-builder/test/utils/mocks/WETH.sol#L65 balanceOf[src] -= wad;

2022-09-nouns-builder/test/utils/mocks/WETH.sol#L66 balanceOf[dst] += wad;

✅ G-12: Use uint256 instead of bool

📝 Description

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.

💡 Recommendation

You should change from bool to uint256 to save gas

🔍 Findings:

2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L136 bool extend;

2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L349 bool success;

2022-09-nouns-builder/blob/main/src/auction/types/AuctionTypesV1.sol#L35 bool settled;

2022-09-nouns-builder/blob/main/src/governance/governor/types/GovernorTypesV1.sol#L52 bool executed;

2022-09-nouns-builder/blob/main/src/governance/governor/types/GovernorTypesV1.sol#L53 bool canceled;

2022-09-nouns-builder/blob/main/src/governance/governor/types/GovernorTypesV1.sol#L54 bool vetoed;

✅ G-13: Use indexed for uint, bool, and address

📝 Description

Using the indexed keyword for value types such as uint, bool, and address saves gas costs, as seen in the example below. However, this is only the case for value types, whereas indexing bytes and strings are more expensive than their unindexed version. Also indexed keyword has more merits. It can be useful to have a way to monitor the contract's activity after it was deployed. One way to accomplish this is to look at all transactions of the contract, however that may be insufficient, as message calls between contracts are not recorded in the blockchain. Moreover, it shows only the input parameters, not the actual changes being made to the state. Also events could be used to trigger functions in the user interface.

💡 Recommendation

Up to three indexed can be used per event and should be added.

🔍 Findings:

2022-09-nouns-builder/blob/main/src/auction/IAuction.sol#L22 event AuctionBid(uint256 tokenId, address bidder, uint256 amount, bool extended, uint256 endTime);

2022-09-nouns-builder/blob/main/src/auction/IAuction.sol#L28 event AuctionSettled(uint256 tokenId, address winner, uint256 amount);

2022-09-nouns-builder/blob/main/src/auction/IAuction.sol#L34 event AuctionCreated(uint256 tokenId, uint256 startTime, uint256 endTime);

2022-09-nouns-builder/blob/main/src/auction/IAuction.sol#L38 event DurationUpdated(uint256 duration);

2022-09-nouns-builder/blob/main/src/auction/IAuction.sol#L42 event ReservePriceUpdated(uint256 reservePrice);

2022-09-nouns-builder/blob/main/src/auction/IAuction.sol#L46 event MinBidIncrementPercentageUpdated(uint256 minBidIncrementPercentage);

2022-09-nouns-builder/blob/main/src/auction/IAuction.sol#L50 event TimeBufferUpdated(uint256 timeBuffer);

2022-09-nouns-builder/blob/main/src/governance/governor/IGovernor.sol#L29 event ProposalQueued(bytes32 proposalId, uint256 eta);

2022-09-nouns-builder/blob/main/src/governance/governor/IGovernor.sol#L42 event VoteCast(address voter, bytes32 proposalId, uint256 support, uint256 weight, string reason);

2022-09-nouns-builder/blob/main/src/governance/governor/IGovernor.sol#L45 event VotingDelayUpdated(uint256 prevVotingDelay, uint256 newVotingDelay);

2022-09-nouns-builder/blob/main/src/governance/governor/IGovernor.sol#L48 event VotingPeriodUpdated(uint256 prevVotingPeriod, uint256 newVotingPeriod);

2022-09-nouns-builder/blob/main/src/governance/governor/IGovernor.sol#L51 event ProposalThresholdBpsUpdated(uint256 prevBps, uint256 newBps);

2022-09-nouns-builder/blob/main/src/governance/governor/IGovernor.sol#L54 event QuorumVotesBpsUpdated(uint256 prevBps, uint256 newBps);

2022-09-nouns-builder/blob/main/src/governance/governor/IGovernor.sol#L57 event VetoerUpdated(address prevVetoer, address newVetoer);

2022-09-nouns-builder/blob/main/src/governance/treasury/ITreasury.sol#L16 event TransactionScheduled(bytes32 proposalId, uint256 timestamp);

2022-09-nouns-builder/blob/main/src/governance/treasury/ITreasury.sol#L25 event DelayUpdated(uint256 prevDelay, uint256 newDelay);

2022-09-nouns-builder/blob/main/src/governance/treasury/ITreasury.sol#L28 event GracePeriodUpdated(uint256 prevGracePeriod, uint256 newGracePeriod);

2022-09-nouns-builder/blob/main/src/lib/interfaces/IERC1967Upgrade.sol#L14 event Upgraded(address impl);

2022-09-nouns-builder/blob/main/src/lib/interfaces/IERC721Votes.sol#L19 event DelegateVotesChanged(address indexed delegate, uint256 prevTotalVotes, uint256 newTotalVotes);

2022-09-nouns-builder/blob/main/src/lib/interfaces/IInitializable.sol#L13 event Initialized(uint256 version);

2022-09-nouns-builder/blob/main/src/lib/interfaces/IPausable.sol#L14 event Paused(address user);

2022-09-nouns-builder/blob/main/src/lib/interfaces/IPausable.sol#L18 event Unpaused(address user);

2022-09-nouns-builder/blob/main/src/manager/IManager.sol#L21 event DAODeployed(address token, address metadata, address auction, address treasury, address governor);

2022-09-nouns-builder/blob/main/src/manager/IManager.sol#L26 event UpgradeRegistered(address baseImpl, address upgradeImpl);

2022-09-nouns-builder/blob/main/src/manager/IManager.sol#L31 event UpgradeRemoved(address baseImpl, address upgradeImpl);

2022-09-nouns-builder/blob/main/src/token/IToken.sol#L21 event MintScheduled(uint256 baseTokenId, uint256 founderId, Founder founder);

2022-09-nouns-builder/blob/main/src/token/metadata/interfaces/IPropertyIPFSMetadataRenderer.sol#L16 event PropertyAdded(uint256 id, string name);

2022-09-nouns-builder/blob/main/src/token/metadata/interfaces/IPropertyIPFSMetadataRenderer.sol#L19 event ItemAdded(uint256 propertyId, uint256 index);

2022-09-nouns-builder/test/utils/mocks/WETH.sol#L13 event Deposit(address indexed dst, uint256 wad);

2022-09-nouns-builder/test/utils/mocks/WETH.sol#L14 event Withdrawal(address indexed src, uint256 wad);

✅ G-14: Do not call the same function within one function

📝 Description

Do not call the same function within one function.

💡 Recommendation

Use currentProposalThreshold variable instead of executing proposalTheshold() again

🔍 Findings:

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

    function propose(
        address[] memory _targets,
        uint256[] memory _values,
        bytes[] memory _calldatas,
        string memory _description
    ) external returns (bytes32) {
        // Get the current proposal threshold
        uint256 currentProposalThreshold = proposalThreshold();

        // Cannot realistically underflow and `getVotes` would revert
        unchecked {
            // Ensure the caller's voting weight is greater than or equal to the threshold
            // Use currentProposalThreshold instead of executing proposalTheshold() again
            if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
        }