Nouns Builder contest - 0xSmartContract's results

Lines of code

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

Vulnerability details

Impact

With the deploy function of the manager contract, the ERC-721 DAO token contract is started. During deployment the list of DAO founders is set as the dynamic call argument.However, if the number of DAO founders is 101, the operation will be reverted.

This check happens with the following check in the _addFounders function in the token.sol file ;

        unchecked {
            // For each founder:
            for (uint256 i; i < numFounders; ++i) {
                // Cache the percent ownership
                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();

During the first deploy, DAO owners may have serious doubts that the 100 limit is desirable (it is obvious that there should not be a 100 limit in communities like DAO)

Regarding the initial 100 DAO Founders limit; 1 - No starting limit is specified in the documents. 2- There is no border control in the deploy function in the manager contract. 3- In case of transaction rever due to 100 limit, no custom Error is issued for the limit 4- The comment in the line of code where the limit is applied does not explain the subject clearly; "Update the total ownership and ensure it's valid" 5- There is no information about the border for the investor

Within the scope of the above reasons, this user may create grievances,

Proof of Concept

If the detailed code is updated to 101 for local test only in the 100 founders checking part, it will revert,

function _addFounders() public {
   

        // Used to store the total percent ownership among the founders
        uint256 totalOwnership;
        unchecked {
                uint256 founderPct = 101;
                if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();
                // Compute the vesting schedule
                uint256 schedule = 100 / founderPct;
       }
}

Tools Used

Manuel Code review

Recommended Mitigation Steps

1 - Specify starting limit in documents 2- Add a 100 DAO founders limit check to the deploy function of the manager contract 3- In case of transaction rever due to limit of 100, issue a custom Error specific to the limit 4- Explain the subject clearly with the comment on the line of code where the limit is applied.

function deploy(
        FounderParams[] calldata _founderParams,
        TokenParams calldata _tokenParams,
        AuctionParams calldata _auctionParams,
        GovParams calldata _govParams
    )
        external
        returns (
            address token,
            address metadata,
            address auction,
            address treasury,
            address governor
        )
    {
        // 100 DAO founders limit check
         if (_founderParams.length > 101) revert DAO_LIMIT_EXCEEDED();
        // Used to store the address of the first (or only) founder
        // This founder is responsible for adding token artwork and launching the first auction -- they're also free to transfer this responsiblity
        address founder;

        // Ensure at least one founder is provided
        if ((founder = _founderParams[0].wallet) == address(0)) revert FOUNDER_REQUIRED();

        // Deploy the DAO's ERC-721 governance token
        token = address(new ERC1967Proxy(tokenImpl, ""));

        // Use the token address to precompute the DAO's remaining addresses
        bytes32 salt = bytes32(uint256(uint160(token)) << 96);

        // Deploy the remaining DAO contracts
        metadata = address(new ERC1967Proxy{ salt: salt }(metadataImpl, ""));
        auction = address(new ERC1967Proxy{ salt: salt }(auctionImpl, ""));
        treasury = address(new ERC1967Proxy{ salt: salt }(treasuryImpl, ""));
        governor = address(new ERC1967Proxy{ salt: salt }(governorImpl, ""));

        // Initialize each instance with the provided settings
        IToken(token).initialize(_founderParams, _tokenParams.initStrings, metadata, auction);
        IBaseMetadata(metadata).initialize(_tokenParams.initStrings, token, founder, treasury);
        IAuction(auction).initialize(token, founder, treasury, _auctionParams.duration, _auctionParams.reservePrice);
        ITreasury(treasury).initialize(governor, _govParams.timelockDelay);
        IGovernor(governor).initialize(
            treasury,
            token,
            founder,
            _govParams.votingDelay,
            _govParams.votingPeriod,
            _govParams.proposalThresholdBps,
            _govParams.quorumThresholdBps
        );

        emit DAODeployed(token, metadata, auction, treasury, governor);
    }

Lines of code

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

Vulnerability details

Impact

In the token contract, the mint function increases totalSupply but the burn function does not decrease totalSupply.

Proof of Concept

ERC721.sol :

 function _burn(uint256 _tokenId) internal virtual {
        address owner = owners[_tokenId];
        if (owner == address(0)) revert NOT_MINTED();
        _beforeTokenTransfer(owner, address(0), _tokenId);

        unchecked {
            --balances[owner];
        }

        delete owners[_tokenId];
        delete tokenApprovals[_tokenId];
        emit Transfer(owner, address(0), _tokenId);
        _afterTokenTransfer(owner, address(0), _tokenId);
    }

As we can see, this implementation already provides an internal _burn function. The function accepts the token id as the input and does the following: -gets the NFT's owner -check (0) address control -calls the _beforeTokenTransfer hook function (what a hook is will be clearer later) -clears all approvals for the token -decreases the owner balance -emits the Transfer event -calls the _afterTokenTransfer hook function

TotalSupply is not decreased by the function, we also need to do this in the burn function. But this is not done in the burn function either.

function burn(uint256 _tokenId) external {
        // Ensure the caller is the auction house
        if (msg.sender != settings.auction) revert ONLY_AUCTION();

        // Burn the token
        _burn(_tokenId);
    }

One reason for not doing this is conceivable because "Burns a token that did not see any bids" However, this is an upgradeable project and entitlements may change in the future.

Tools Used

Manual code review

Recommended Mitigation Steps

Change the function as below;

function burn(uint256 _tokenId) external {
         if (msg.sender != settings.auction) revert ONLY_AUCTION();
         tokenId = settings.totalSupply--;
        _burn(_tokenId);
    }

Low Risk Issues List

Total 12 issues

Non-Critical Issues List

Total 18 issues

[L-01] Floating Pragma

Description: Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. https://swcregistry.io/docs/SWC-103

Floating Pragma List: pragma ^0.8.4. (src/lib/ all files) pragma ^0.8.0. (src/lib/utils/TokenReceiver.sol)

Recommendation: Lock the pragma version and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

[L-02] _owner declaration shadows an existing declaration

Context: Treasury.sol#L141-L172

Description: Solidity docs: The function initialize in the file manager.sol#L82 has a local variable named _owner. However, there is also a state variable in the Ownable.sol file with the same name, from which it inherited. Therefore, there is shadowing here. This shadowing does not pose a security threat because the owner in Ownable.sol is also updated with this function. However, shadowing is undesirable, state variable and local variable names should not be the same, this negatively audit control and code readability.

Recommendation: State variable and local variable should be named differently.

following codes

Ownable.sol
abstract contract Ownable;

    address internal _owner;


Manager.sol
contract Manager is Ownable; 

    function initialize(address _owner) external initializer {
	if (_owner == address(0)) revert ADDRESS_ZERO();
	__Ownable_init(_owner);
}

[L-03] Non-essential inheritance list

Context: Auction.sol#L22

Description: If a contract inherited different contracts, the last inherited contract can be used directly. But there are files in the codes that are not implemented this way.

Recommendation: IAuction; A, B, C contracts are inherited. Auction can inherit entire hierarchy by simply inheriting IAuction

following codes

lib
proxy─ ERC1967Upgrade - "Contract A"
proxy─ UUPS - "Contract B"
token─ Ownable - "Contract C"
auction
IAuction — "Contract D is A,B,C"
Auction — "Contract E is A,B,C,D"

[L-04] Need Fuzzing test

Context: 35 results - 9 files Project uncheckeds list:

src/auction/Auction.sol:
  116              // Cannot realistically overflow
  117:             unchecked {
  118                  // Compute the minimum bid

  138          // Cannot underflow as `_auction.endTime` is ensured to be greater than the current time above
  139:         unchecked {
  140              // Compute whether the time remaining is less than the buffer

  146              // Cannot realistically overflow
  147:             unchecked {
  148                  // Extend the auction by the time buffer

  216              // Cannot realistically overflow
  217:             unchecked {
  218                  // Compute the auction end time

src/governance/governor/Governor.sol:
  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

  157          // Cannot realistically overflow
  158:         unchecked {
  159              // Compute the snapshot and deadline

  223          // Cannot realistically overflow
  224:         unchecked {
  225              // Compute the message

  272          // Cannot realistically underflow and `getVotes` would revert
  273:         unchecked {
  274              // Get the voter's weight at the time the proposal was created

  360          // Cannot realistically underflow and `getVotes` would revert
  361:         unchecked {
  362              // Ensure the caller is the proposer or the proposer's voting weight has dropped below the proposal threshold

  466      function proposalThreshold() public view returns (uint256) {
  467:         unchecked {
  468              return (settings.token.totalSupply() * settings.proposalThresholdBps) / 10_000;

  473      function quorum() public view returns (uint256) {
  474:         unchecked {
  475              return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;

src/governance/treasury/Treasury.sol:
   74      function isExpired(bytes32 _proposalId) external view returns (bool) {
   75:         unchecked {
   76              return block.timestamp > (timestamps[_proposalId] + settings.gracePeriod);

  120          // Cannot realistically overflow
  121:         unchecked {
  122              // Compute the timestamp that the proposal will be valid to execute

  159          // Cannot realistically overflow
  160:         unchecked {
  161              // For each target:

src/lib/token/ERC721.sol:
  137  
  138:         unchecked {
  139              --balances[_from];

  197  
  198:         unchecked {
  199              ++balances[_to];

  217  
  218:         unchecked {
  219              --balances[owner];

src/lib/token/ERC721Votes.sol:
   49          // Cannot underflow as `nCheckpoints` is ensured to be greater than 0 if reached
   50:         unchecked {
   51              // Return the number of votes at the latest checkpoint if applicable

   71  
   72:         unchecked {
   73              // Get the latest checkpoint id

  158          // Cannot realistically overflow
  159:         unchecked {
  160              // Compute the hash of the domain seperator with the typed delegation data

  200      ) internal {
  201:         unchecked {
  202              // If voting weight is being transferred:

src/token/Token.sol:
   77  
   78:         unchecked {
   79              // For each founder:

  130      function _getNextTokenId(uint256 _tokenId) internal view returns (uint256) {
  131:         unchecked {
  132              while (tokenRecipient[_tokenId].wallet != address(0)) ++_tokenId;

  150          // Cannot realistically overflow
  151:         unchecked {
  152              do {

  259          // Cannot realistically overflow
  260:         unchecked {
  261              // Add each founder to the array

src/token/metadata/MetadataRenderer.sol:
  116  
  117:         unchecked {
  118              // For each new property:

  186  
  187:         unchecked {
  188              // For each property:

  223  
  224:         unchecked {
  225              // Cache the index of the last property

test/Token.t.sol:
   97  
   98:         unchecked {
   99              for (uint256 i; i < 100; ++i) {

  129  
  130:         unchecked {
  131              for (uint256 i; i < 50; ++i) {

  165  
  166:         unchecked {
  167              for (uint256 i; i < 2; ++i) {

  180  
  181:         unchecked {
  182              for (uint256 i; i < 500; ++i) {

test/utils/NounsBuilderTest.sol:
  109  
  110:         unchecked {
  111              for (uint256 i; i < numFounders; ++i) {

  225  
  226:         unchecked {
  227              for (uint256 i; i < _numUsers; ++i) {

  240  
  241:         unchecked {
  242              for (uint256 i; i < _numTokens; ++i) {

Description: In total 9 contracts, 35 unchecked are used, the functions used are critical. For this reason, there must be fuzzing tests in the tests of the project. Not seen in tests.

Recommendation: Use should fuzzing test like Echidna.

As Alberto Cuesta Canada said: Fuzzing is not easy, the tools are rough, and the math is hard, but it is worth it. Fuzzing gives me a level of confidence in my smart contracts that I didn’t have before. Relying just on unit testing anymore and poking around in a testnet seems reckless now.

https://medium.com/coinmonks/smart-contract-fuzzing-d9b88e0b0a05

[L-05] Missing Event for Critical Parameters Change

Context: Auction.sol#L263 Auction.sol#L268 Governor.sol#L208 Treasury.sol#L210 Treasury.sol#L269

Description: Events help non-contract tools to track changes, and events prevent users from being surprised by changes.

Recommendation: Add event-emit

[L-06] Add to Blacklist function

Description: Cryptocurrency mixing service, Tornado Cash, has been blacklisted in the OFAC. A lot of blockchain companies, token projects, NFT Projects have blacklisted all Ethereum addresses owned by Tornado Cash listed in the US Treasury Department's sanction against the protocol. https://home.treasury.gov/policy-issues/financial-sanctions/recent-actions/20220808 In addition, these platforms even ban accounts that have received ETH on their account with Tornadocash.

Some of these Projects;

• USDC (https://www.circle.com/en/usdc)
• Flashbots (https://www.paradigm.xyz/portfolio/flashbots )
• Aave (https://aave.com/)
• Uniswap
• Balancer
• Infura
• Alchemy
• Opensea
• dYdX

Details on the subject; https://twitter.com/bantg/status/1556712790894706688?s=20&t=HUTDTeLikUr6Dv9JdMF7AA

https://cryptopotato.com/defi-protocol-aave-bans-justin-sun-after-he-randomly-received-0-1-eth-from-tornado-cash/

For this reason, every project in the Ethereum network must have a blacklist function, this is a good method to avoid legal problems in the future, apart from the current need.

Transactions from the project by an account funded by Tonadocash or banned by OFAC can lead to legal problems.Especially American citizens may want to get addresses to the blacklist legally, this is not an obligation

If you think that such legal prohibitions should be made directly by validators, this may not be possible: https://www.paradigm.xyz/2022/09/base-layer-neutrality

The ban on Tornado Cash makes little sense, because in the end, no one can prevent people from using other mixer smart contracts, or forking the existing ones. It neither hinders cybercrime, nor privacy.

Here is the most beautiful and close to the project example; Manifold

Manifold Contract https://etherscan.io/address/0xe4e4003afe3765aca8149a82fc064c0b125b9e5a#code

     modifier nonBlacklistRequired(address extension) {
         require(!_blacklistedExtensions.contains(extension), "Extension blacklisted");
         _;
     }

Recommended Mitigation Steps add to Blacklist function and modifier

[L-07] No check if OnERC721Received is implemented

Context: Auction.sol#L206

Description: When minting a NFT, the function does not check if a receiving contract implements onERC721Received(). Check if OnERC721Received is implemented. If this is intended, safemint should be used instead of mint

 function _createAuction() private {
        // Get the next token available for bidding
        try token.mint() returns (uint256 tokenId) {
            // Store the token id
            auction.tokenId = tokenId;

            // Cache the current timestamp
            uint256 startTime = block.timestamp;

            // Used to store the auction end time
            uint256 endTime;

            // Cannot realistically overflow
            unchecked {
                // Compute the auction end time
                endTime = startTime + settings.duration;
            }

            // Store the auction start and end time
            auction.startTime = uint40(startTime);
            auction.endTime = uint40(endTime);

            // Reset data from the previous auction
            auction.highestBid = 0;
            auction.highestBidder = address(0);
            auction.settled = false;

            emit AuctionCreated(tokenId, startTime, endTime);

            // Pause the contract if token minting failed
        } catch Error(string memory) {
            _pause();
        }
    }

Recommendation: Consider using _safeMint instead of _mint

[L-08] Missing supportsInterface

Context: ERC721.sol#L61-L66

Description: ERC721TokenReceiver interface ID is missing in supportInterface

 function supportsInterface(bytes4 _interfaceId) external pure returns (bool) {
        return
            _interfaceId == 0x01ffc9a7 || // ERC165 Interface ID
            _interfaceId == 0x80ac58cd || // ERC721 Interface ID
            _interfaceId == 0x5b5e139f; // ERC721Metadata Interface ID
    }

Recommendation:

 function supportsInterface(bytes4 _interfaceId) external pure returns (bool) {
        return
            _interfaceId == 0x01ffc9a7 || // ERC165 Interface ID
            _interfaceId == 0x80ac58cd || // ERC721 Interface ID
            _interfaceId == 0x150b7a02 || // ERC721TokenReceiver Interface ID
            _interfaceId == 0x5b5e139f; // ERC721Metadata Interface ID
    }

[L-09] Use SafeTransferOwnership instead of transferOwnership function

Context: Ownable.sol#L63-L67

Description: transferOwnership function is used to change Ownership from Ownable.sol.

transferOwnership is used in Ownable.sol and MetadataRenderer.sol files to transfer ownership of the contract to the DAO.

There is another function, SafeTransferOwnership, use it is more secure due to 2-stage ownership transfer.

 function transferOwnership(address _newOwner) public onlyOwner {
        emit OwnerUpdated(_owner, _newOwner);

        _owner = _newOwner;
    }

Recommendation:

Remove to transferOwnership


 function safeTransferOwnership(address _newOwner) public onlyOwner {
        _pendingOwner = _newOwner;

        emit OwnerPending(_owner, _newOwner);
    }

[L-10] Insufficient Tests

Context: MetadataRenderer.sol

Description: It is crucial to write tests with possibly 100% coverage for smart contract systems. Contract of top in Context were found to be not included in the test cases.

Recommendation: It is recommended to write proper test for all possible code flows and specially edge cases.

[L-11] Use abi.encode() to avoid collision

Context: MetadataRenderer.sol#L243 MetadataRenderer.sol#L243 MetadataRenderer.sol#L259 MetadataRenderer.sol#L290 MetadataRenderer.sol#L208 MetadataRenderer.sol#L309

Description: If you use keccak256(abi.encodePacked(a, b)) and both a and b are dynamic types, it is easy to craft collisions in the hash value by moving parts of a into b and vice-versa. More specifically, abi.encodePacked("a", "bc") == abi.encodePacked("ab", "c"). If you use abi.encodePacked for signatures, authentication or data integrity, make sure to always use the same types and check that at most one of them is dynamic. Unless there is a compelling reason, abi.encode should be preferred.

When are these used?

abi.encode: abi.encode encode its parameters using the ABI specs. The ABI was designed to make calls to contracts. Parameters are padded to 32 bytes. If you are making calls to a contract you likely have to use abi.encode. If you are dealing with more than one dynamic data type, you may prefer to use abi.encode as it avoids collision. keccak256(abi.encode(data, ...)) is the safe way to generate a unique ID for a given set of inputs.

abi.encodePacked: abi.encodePacked encode its parameters using the minimal space required by the type. Encoding an uint8 it will use 1 byte. It is used when you want to save some space, and not calling a contract.Takes all types of data and any amount of input.

Proof of Concept

contract Contract0 {

    // abi.encode
    // (AAA,BBB) keccak = 0xd6da8b03238087e6936e7b951b58424ff2783accb08af423b2b9a71cb02bd18b
    // (AA,ABBB) keccak = 0x54bc5894818c61a0ab427d51905bc936ae11a8111a8b373e53c8a34720957abb
    function collision(string memory _text, string memory _anotherText)
        public pure returns (bytes32)
    {
        return keccak256(abi.encode(_text, _anotherText));
    }
}


contract Contract1 {

    // abi.encodePacked
    // (AAA,BBB) keccak = 0xf6568e65381c5fc6a447ddf0dcb848248282b798b2121d9944a87599b7921358
    // (AA,ABBB) keccak = 0xf6568e65381c5fc6a447ddf0dcb848248282b798b2121d9944a87599b7921358

    function hard(string memory _text, string memory _anotherText)
        public pure returns (bytes32)
    {
        return keccak256(abi.encodePacked(_text, _anotherText));
    }
}

[L-12] Return value of transfer not checked

Context: Auction.sol#L363

Description: When there’s a failure in transfer()/transferFrom() the function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment.

Recommendation: Add return value checks.

[N-01] Inconsistent solidity versions

Description: Different Solidity compiler versions are used throughout src repositories. The following contracts mix versions:

pragma ^0.8.4 (src/lib/ all files) pragma ^0.8.15 pragma ^0.8.0 (src/lib/utils/TokenReceiver.sol) pragma 0.8.15

Recommendation: Versions must be consistent with each other

Full List:

src/auction/Auction.sol:
pragma solidity 0.8.15;
 
src/auction/IAuction.sol:
pragma solidity 0.8.15;
 
src/auction/storage/AuctionStorageV1.sol:
pragma solidity 0.8.15;
 
src/auction/types/AuctionTypesV1.sol:
pragma solidity 0.8.15;
 
src/governance/governor/Governor.sol:
pragma solidity 0.8.15;
 
src/governance/governor/IGovernor.sol:
pragma solidity 0.8.15;
 
src/governance/governor/storage/GovernorStorageV1.sol:
pragma solidity 0.8.15;
 
src/governance/governor/types/GovernorTypesV1.sol:
pragma solidity 0.8.15;
 
src/governance/treasury/ITreasury.sol:
pragma solidity 0.8.15;
 
src/governance/treasury/Treasury.sol:
pragma solidity 0.8.15;
 
src/governance/treasury/storage/TreasuryStorageV1.sol:
pragma solidity 0.8.15;
 
src/governance/treasury/types/TreasuryTypesV1.sol:
pragma solidity 0.8.15;
 
src/lib/interfaces/IEIP712.sol:
pragma solidity ^0.8.4;
 
src/lib/interfaces/IERC721.sol:
pragma solidity ^0.8.4;
 
src/lib/interfaces/IERC721Votes.sol:
pragma solidity ^0.8.4;
 
src/lib/interfaces/IERC1967Upgrade.sol:
pragma solidity ^0.8.4;
 
src/lib/interfaces/IInitializable.sol:
pragma solidity ^0.8.4;
 
src/lib/interfaces/IOwnable.sol:
pragma solidity ^0.8.4;
 
src/lib/interfaces/IPausable.sol:
pragma solidity ^0.8.4;
 
src/lib/interfaces/IUUPS.sol:
pragma solidity ^0.8.15;
 
src/lib/interfaces/IWETH.sol:
pragma solidity ^0.8.15;
 
src/lib/proxy/ERC1967Proxy.sol:
pragma solidity ^0.8.4;
 
src/lib/proxy/ERC1967Upgrade.sol:
pragma solidity ^0.8.4;
 
src/lib/proxy/UUPS.sol:
pragma solidity ^0.8.4;
 
src/lib/token/ERC721.sol:
pragma solidity ^0.8.4;
 
src/lib/token/ERC721Votes.sol:
pragma solidity ^0.8.4;
 
src/lib/utils/Address.sol:
pragma solidity ^0.8.4;
 
src/lib/utils/EIP712.sol:
pragma solidity ^0.8.4;
 
src/lib/utils/Initializable.sol:
pragma solidity ^0.8.4;
 
src/lib/utils/Ownable.sol:
pragma solidity ^0.8.4;
 
src/lib/utils/Pausable.sol:
pragma solidity ^0.8.4;
 
src/lib/utils/ReentrancyGuard.sol:
pragma solidity ^0.8.4;
 
src/lib/utils/SafeCast.sol:
pragma solidity ^0.8.4;
 
src/lib/utils/TokenReceiver.sol:
pragma solidity ^0.8.0;
 
src/manager/IManager.sol:
pragma solidity 0.8.15;
 
src/manager/Manager.sol:
pragma solidity 0.8.15;
 
src/manager/storage/ManagerStorageV1.sol:
pragma solidity 0.8.15;
 
src/token/IToken.sol:
pragma solidity 0.8.15;
 
src/token/Token.sol:
pragma solidity 0.8.15;
 
src/token/metadata/MetadataRenderer.sol:
pragma solidity 0.8.15;
 
src/token/metadata/interfaces/IBaseMetadata.sol:
pragma solidity 0.8.15;
 
src/token/metadata/interfaces/IPropertyIPFSMetadataRenderer.sol:
pragma solidity 0.8.15;
 
src/token/metadata/storage/MetadataRendererStorageV1.sol:
pragma solidity 0.8.15;
 
src/token/metadata/types/MetadataRendererTypesV1.sol:
pragma solidity 0.8.15;
 
src/token/storage/TokenStorageV1.sol:
pragma solidity 0.8.15;
 
src/token/types/TokenTypesV1.sol:
pragma solidity 0.8.15;

[N-02] Use a more recent version of solidity

Context: TokenReceiver.sol

Description: Old version of Solidity is used (^0.8.0), newer version should be used

[N-03] Unused/Empty receive() / fallback() function

Context: Treasury.sol#L269

Description: If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth)))

[N-04] Function writing that does not comply with the 'Solidity Style Guide’

Context: Treasury.sol

Description: Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

https://docs.soliditylang.org/en/v0.8.15/style-guide.html

Functions should be grouped according to their visibility and ordered:

• constructor
• receive function (if exists)
• fallback function (if exists)
• external
• public
• internal
• private
• within a grouping, place the view and pure functions last

[N-05] WETH address definition can be use directly

Context: Auction.sol#L41

Description: WETH is a wrap Ether contract with a specific address in the Etherum network, giving the option to define it may cause false recognition, it is healthier to define it directly.

Advantages of defining a specific contract directly:

• It saves gas
• Prevents incorrect argument definition
• Prevents execution on a different chain and re-signature issues

WETH Address : 0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2

    /// @param _manager The contract upgrade manager address
    /// @param _weth The address of WETH
    constructor(address _manager, address _weth) payable initializer {
        manager = IManager(_manager);
        WETH = _weth;
    }

Recommendation:

address private constant WETH = 0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2;

    constructor(address _manager) payable initializer {
        manager = IManager(_manager);
}

[N-06] 0 address check

Context: Token.sol#L30-L32 Auction.sol#L39-L42 Governor.sol#L41-L43 Treasury.sol#L33

Description: Also check of the address to protect the code from 0x0 address problem just in case. This is best practice or instead of suggesting that they verify address != 0x0, you could add some good natspec comments explaining what is valid and what is invalid and what are the implications of accidentally using an invalid address.

Recommendation: like this ; if (_treasury == address(0)) revert ADDRESS_ZERO();

[N-07] Missing NatSpec mapping comment

Context: ManagerStorageV1.sol#L9

Description: Last value in Nested Mapping is missing from comments

Recommendation: ManagerStorageV1.sol#L9

/// @dev Base impl => Upgrade impl
    mapping(address => mapping(address => bool)) internal isUpgrade;

Recomendation code:

/// @dev Base impl => Upgrade impl => Approved
    mapping(address => mapping(address => bool)) internal isUpgrade;

[N-08] There is no need to cast a variable that is already an address, such as address(x)

Context: Governor.sol#L550 Governor.sol#L555

Description: There is no need to cast a variable that is already an address, such as address(auctioneer_), auctioneer also address.

Recommendation: Use directly variable.

contract Contract0 {
    address tokenaddr;

    function token() external returns (address) {
        return address(tokenaddr);
    }
}

//recommendation

contract Contract1 { 
    address tokenaddr;

    function token() public returns(address) {
        tokenaddr;
    }
}

[N-09] Add to indexed parameter for countable Events

Context: IAuction.sol#L22 IAuction.sol#L28 IGovernor.sol#L42 IGovernor.sol#L20 ITreasury.sol#L22

Recommendation: Add to indexed parameter for countable Events.

[N-10] Include return parameters in NatSpec comments

Context: All Contracts

Description: It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

https://docs.soliditylang.org/en/v0.8.15/natspec-format.html

Recommendation: Include return parameters in NatSpec comments

Recommendation Code Style: (from Uniswap3)

    /// @notice Adds liquidity for the given recipient/tickLower/tickUpper position
    /// @dev The caller of this method receives a callback in the form of IUniswapV3MintCallback#uniswapV3MintCallback
    /// in which they must pay any token0 or token1 owed for the liquidity. The amount of token0/token1 due depends
    /// on tickLower, tickUpper, the amount of liquidity, and the current price.
    /// @param recipient The address for which the liquidity will be created
    /// @param tickLower The lower tick of the position in which to add liquidity
    /// @param tickUpper The upper tick of the position in which to add liquidity
    /// @param amount The amount of liquidity to mint
    /// @param data Any data that should be passed through to the callback
    /// @return amount0 The amount of token0 that was paid to mint the given amount of liquidity. Matches the value in the callback
    /// @return amount1 The amount of token1 that was paid to mint the given amount of liquidity. Matches the value in the callback
    function mint(
        address recipient,
        int24 tickLower,
        int24 tickUpper,
        uint128 amount,
        bytes calldata data
    ) external returns (uint256 amount0, uint256 amount1);

[N-11] Avoid to same local variable names declarated for code readability

Context: Token.sol#L179 Token.sol#L105

Description: uint256 baseTokenId local variable name is used in two different functions, this causes confusion and negatively affects code readability / controllability.

Token.sol#L179
    function _addFounders(IManager.FounderParams[] calldata _founders) internal {
	//code..
	uint256 baseTokenId;


Token.sol#L105
    function _isForFounder(uint256 _tokenId) private returns (bool) {
	//code..
	uint256 baseTokenId;

Recommendation: Avoid to same local variable names declarated for code readability.

[N-12] Do not import more than one same contract

Context: Token.sol#L6-L7

Description: In Token.sol file, ERC721Votes and ERC721 contracts are imported, but ERC721Votes has already imported ERC721 contract. Therefore, there is no need to import ERC721.

Token.sol:
import { ERC721Votes } from "../lib/token/ERC721Votes.sol";
import { ERC721 } from "../lib/token/ERC721.sol";

ERC721Votes.sol:
import { ERC721 } from "../token/ERC721.sol";

ERC721.sol:
import { IERC721 } from "../interfaces/IERC721.sol";

Recommendation:

Token.sol:
import { ERC721Votes } from "../lib/token/ERC721Votes.sol";

ERC721Votes.sol:
import { ERC721 } from "../token/ERC721.sol";

ERC721.sol:
import { IERC721 } from "../interfaces/IERC721.sol";

[N-13] Incorrect information in the document

Context: Manager.sol#L120-L129

Description: Protocol document states that contracts are created with create2, but contracts are created with new instance, these are two different architectures, must be updated.

Recommendation: Update to document.

[N-14] Highest risk must be specified in NatSpec comments and documentation

Description: Although the UUPS model has many advantages and the proposed proxy model is currently the UUPS model, there are some caveats we should be aware of before applying it to a real-world project.

One of the main attention: Since upgrades are done through the implementation contract with the help of the upgradeTo method, there is a high risk of new implementations such as excluding the upgradeTo method, which can permanently kill the smart contract's ability to upgrade. Also, this pattern is somewhat complicated to implement compared to other proxy patterns.

Recommendation: Therefore, this highest risk must be found in NatSpec comments and documents.

[N-15] Implement some type of version counter that will be incremented automatically for contract upgrades

Description: As part of the upgradeability of UUPS Proxies, the contract can be upgraded multiple times, where it is a systematic approach to record the version of each upgrade.

I suggest implementing some kind of version counter that will be incremented automatically when you upgrade the contract.

Recommendation:

uint256 public authorizeUpgradeCounter;

function _authorizeUpgrade(address _newImpl) internal view override onlyOwner {
        // Ensure the new implementation is registered by the Builder DAO
        if (!manager.isRegisteredUpgrade(_getImplementation(), _newImpl)) revert INVALID_UPGRADE(_newImpl);
	
        authorizeUpgradeCounter+=1;
    }

[N-16] Add castVote  and castVotebySig in ERC721Votes

Description: There are two methods by which a user can delegate their voting rights or cast votes on proposals: either calling the relevant functions (delegate, castVote) directly; or using by-signature functionality (delegateBySig, castVotebySig).

Recommendation: The ERC721Votes contract is just delegate and delegateBySig has the function. Consider adding castVote and castVotebySig.

https://medium.com/compound-finance/delegation-and-voting-with-eip-712-signatures-a636c9dfec5e

[N-17] Empty blocks should be removed or Emit something

Context: Manager.sol#L209-L210

Description: Code contains empty block.

Recommendation: 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.

Current code:

    function _authorizeUpgrade(address _newImpl) internal override onlyOwner {}

Suggested code:

    function _authorizeUpgrade(address _newImpl) internal override onlyOwner {
        emit _authorizeUpgrade(address _newImpl);
}

[N-18] Long lines are not suitable for the ‘Solidity Style Guide’

Context: Token.sol#L59 Token.sol#L221 Token.sol#L310 Auction.sol#L376 Governor.sol#L27 Governor.sol#L363 Governor.sol#L620 Manager.sol#L167-L170 Token.sol#L310

Description: It is generally recommended that lines in the source code should not exceed 80-120 characters. Today's screens are much larger, so in some cases it makes sense to expand that. The lines above should be split when they reach that length, as the files will most likely be on GitHub and GitHub always uses a scrollbar when the length is more than 164 characters.

(why-is-80-characters-the-standard-limit-for-code-width)[https://softwareengineering.stackexchange.com/questions/148677/why-is-80-characters-the-standard-limit-for-code-width]

Recommendation: Multiline output parameters and return statements should follow the same style recommended for wrapping long lines found in the Maximum Line Length section.

https://docs.soliditylang.org/en/v0.8.17/style-guide.html#introduction

thisFunctionCallIsReallyLong(
    longArgument1,
    longArgument2,
    longArgument3
);

Gas Optimizations List

Total 20 issues

Suggestions

Total 2 suggestions

[G-01] Sort solidity operations using short-circuit mode [9 gas saved]

Context: ERC1967Upgrade.sol#L61 Initializable.sol#L36

Description: Short-circuiting is a solidity contract development model that uses OR/AND logic to sequence different cost operations. It puts low gas cost operations in the front and high gas cost operations in the back, so that if the front is low If the cost operation is feasible, you can skip (short-circuit) the subsequent high-cost Ethereum Virtual Machine operation.

//f(x) is a low gas cost operation 
//g(y) is a high gas cost operation 

//Sort operations with different gas costs as follows 
f(x) || g(y) 
f(x) && g(y)

Proof Of Concept: The optimizer was turned on and set to 10000 runs

ERC1967Upgrade.sol#L61: [9 Gas Saved (PoC is given below)]

contract GasTest is DSTest {
    
    Contract0 c0;
    Contract1 c1;
    
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    
    function testGas() public {
        c0._upgradeToAndCall("0x1", true);
        c1._upgradeToAndCall2("0x1", true);
    }
}

contract Contract0 {
    uint256 marketCapacity_ = 0;
    
    function _upgradeToAndCall(bytes memory _data, bool _forceCall) public {
        if (_data.length > 0 || _forceCall){
            marketCapacity_ = 10;
        }
    }
}

contract Contract1 {
    uint256 marketCapacity_ = 0;
    
    function _upgradeToAndCall2(bytes memory _data, bool _forceCall) public {
        if (_forceCall || _data.length > 0 ){
            marketCapacity_ = 10;
        }
    }
}

Gas Report:

╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract0 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 90941                                     ┆ 479             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ _upgradeToAndCall                         ┆ 22766           ┆ 22766 ┆ 22766  ┆ 22766 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 90941                                     ┆ 479             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ _upgradeToAndCall2                        ┆ 22757           ┆ 22757 ┆ 22757  ┆ 22757 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯

[G-02] No need return keyword for gas efficient [~10 / ~2000 gas saved]

Context: MetadataRenderer.sol#L77-L78 MetadataRenderer.sol#L83-L84 MetadataRenderer.sol#L317-L319 MetadataRenderer.sol#L322-L324 MetadataRenderer.sol#L327-L329 MetadataRenderer.sol#L332-L334 MetadataRenderer.sol#L337-L339 EIP712.sol#L58-L60 EIP712.sol#L63-L65 EIP712.sol#L68-L70 Treasury.sol#L68-L70

Recommendation: You must remove the return keyword from the specified contexts.

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

MetadataRenderer.sol#L317-L319: [~2000 Gas saved (PoC is given below)]

contract GasTest is DSTest {
    
    Contract0 c0;
    Contract1 c1;
    
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    
    function testGas() public {
        c0.token();
        c1.token();
    }
}

contract Contract0 {
     address addr = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4;
     function token() external view returns (address) {
        return addr;
    }
}

contract Contract1 {
    address addr = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4;
     function token() external view returns (address) {
        addr;
    }
}

Gas Report:

╭───────────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/GasTest.t.sol:Contract0 contract ┆                 ┆      ┆        ┆      ┆         │
╞═══════════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 50817                                     ┆ 211             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ token                                     ┆ 2258            ┆ 2258 ┆ 2258   ┆ 2258 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆     ┆        ┆     ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 46011                                     ┆ 187             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg ┆ median ┆ max ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ token                                     ┆ 149             ┆ 149 ┆ 149    ┆ 149 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴─────┴────────┴─────┴─────────╯

[G-03] Shifting cheaper than division [146 gas per instance]

Context: ERC721Votes.sol#L95

Description: A division by 2 can be calculated by shifting one to the right. While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

Recommendation: Instead of dividing by 2 use >> 1. like this:

medium = high - (high - low) >> 1;

Proof Of Concept: The optimizer was turned on and set to 10000 runs

contract GasTest is DSTest {
    
    Contract1 c1;
    Contract2 c2;
    
    function setUp() public {
        c1 = new Contract1();
        c2 = new Contract2();
    }
    
    function testGas() public {
        c1.checkPoint();
        c2.checkPoint();
    }
}

contract Contract1 {
    uint32 a = 10;
    uint32 b = 2;
    
    function checkPoint() public view returns (uint32) {
        uint32 center = a/b;
        return(center);
    }
}

contract Contract2 { 
    uint32 a = 10;
    uint32 b = 2;
    
    function checkPoint() public view returns (uint32) {
        uint32 center = a >> 1;
        return(center);
    }
}

Gas Report

╭───────────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆      ┆        ┆      ┆         │
╞═══════════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 70035                                     ┆ 293             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ checkPoint                                ┆ 2413            ┆ 2413 ┆ 2413   ┆ 2413 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/GasTest.t.sol:Contract2 contract ┆                 ┆      ┆        ┆      ┆         │
╞═══════════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 49417                                     ┆ 189             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ checkPoint                                ┆ 2267            ┆ 2267 ┆ 2267   ┆ 2267 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯

[G-04] Using uint256 instead of bool in mappings is more gas efficient [146 gas per instance]

Context:  GovernorStorageV1.sol#L19 ERC721.sol#L38

Description: OpenZeppelin uint256 and bool comparison : 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.The values being non-zero value makes deployment a bit more expensive.

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

Recommendation: Use uint256(1) and uint256(2) instead of bool.

Proof Of Concept: The optimizer was turned on and set to 10000 runs

contract GasTest is DSTest {
    Contract0 c0;
    Contract1 c1;
    
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    
    function testGas() public {
     
        c0._setPolicyPermissions(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4, 79, true);
        c1._setPolicyPermissions(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4, 79, 2);
    }
}

contract Contract0 {
    mapping(address => mapping(uint256  => bool)) public modulePermissions;
    error boolcheck();

    function _setPolicyPermissions(address addr_, uint256 id, bool log) external {
       modulePermissions[addr_][id] = log; 
       if(modulePermissions[addr_][id] = false) revert boolcheck();
    }
}
contract Contract1 {
    mapping(address => mapping(uint256  => uint256)) public modulePermissions;
    error boolcheck();

    // Use uint256 instead of bool    (1 = false , 2 = true) 
    function _setPolicyPermissions(address addr_, uint256 id, uint256 log) external {
       modulePermissions[addr_][id] == log; 
       if(modulePermissions[addr_][id] == 1) revert boolcheck();
    }
}

Gas Report

╭───────────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/GasTest.t.sol:Contract0 contract ┆                 ┆      ┆        ┆      ┆         │
╞═══════════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 88335                                     ┆ 473             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ _setPolicyPermissions                     ┆ 2750            ┆ 2750 ┆ 2750   ┆ 2750 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆      ┆        ┆      ┆         │
╞═══════════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 87135                                     ┆ 467             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ _setPolicyPermissions                     ┆ 2604            ┆ 2604 ┆ 2604   ┆ 2604 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯

[G-05] Use assembly to write address storage values [33 gas per instance]

Context: Auction.sol#L41 Manager.sol#L62-L66

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

contract GasTestFoundry is DSTest {
    Contract1 c1;
    Contract2 c2;
    
    function setUp() public {
        c1 = new Contract1();
        c2 = new Contract2();
    }
    
    function testGas() public {
        c1.setRewardTokenAndAmount(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4, 356);
        c2.setRewardTokenAndAmount(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4, 356);
    }
}

contract Contract1  {
    address rewardToken ;
    uint256 reward;

    function setRewardTokenAndAmount(address token_, uint256 reward_) external {
        rewardToken = token_;
        reward = reward_;
    }
}

contract Contract2  {
    address rewardToken ;
    uint256 reward;

    function setRewardTokenAndAmount(address token_, uint256 reward_) external {
        assembly {
            sstore(rewardToken.slot, token_)
            sstore(reward.slot, reward_)           

        }
    }
}

Gas Report

╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 50899                                     ┆ 285             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ setRewardTokenAndAmount                   ┆ 44490           ┆ 44490 ┆ 44490  ┆ 44490 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract2 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 38087                                     ┆ 221             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ setRewardTokenAndAmount                   ┆ 44457           ┆ 44457 ┆ 44457  ┆ 44457 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯

[G-06] Functions guaranteed to revert when callled by normal users can be marked payable [24 gas per instance]

Context:  Treasury.sol#L180 Treasury.sol#L116 Governor.sol#L605 Governor.sol#L596 Governor.sol#L588 Governor.sol#L580 Governor.sol#L572 Governor.sol#L564 Auction.sol#L331 Auction.sol#L323 Auction.sol#L315 Auction.sol#L307 Auction.sol#L263 Auction.sol#L244

Description: If a function modifier or require such as onlyOwner-admin 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 24 gas per call to the function, in addition to the extra deployment cost.

Recommendation: Functions guaranteed to revert when called by normal users can be marked payable  (for only onlyowner or admin functions)

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

contract GasTest is DSTest {
    Contract0 c0;
    Contract1 c1;
    
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    
    function testGas() public {
        c0.resetBeat();
        c1.resetBeat();
    }
}

contract Contract0 {
    uint256 versionNFTDropCollection;
    
    function resetBeat() external {
        versionNFTDropCollection++;
    }
}

contract Contract1 {
    uint256 versionNFTDropCollection;
    
    function resetBeat() external payable {
        versionNFTDropCollection++;
    }
}

Gas Report

╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract0 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 44293                                     ┆ 252             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ resetBeat                                 ┆ 22308           ┆ 22308 ┆ 22308  ┆ 22308 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬───────┬────────┬───────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆       ┆        ┆       ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═══════╪════════╪═══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 41893                                     ┆ 240             ┆       ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg   ┆ median ┆ max   ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ resetBeat                                 ┆ 22284           ┆ 22284 ┆ 22284  ┆ 22284 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴───────┴────────┴───────┴─────────╯

[G-07] Function ordering via method ID [22 gas per instance]

Context:  All Contracts

Description:  Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions. 

Recommendation:  Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas For example, the function IDs in the Governor.sol contract will be the most used; A lower method ID may be given.

Proof of Consept: https://coinsbench.com/advanced-gas-optimizations-tips-for-solidity-85c47f413dc5

Governor.sol function names can be named and sorted according to METHOD ID

Sighash | Function Signature
========================
2b4656c8 => initialize(address,address,address,uint256,uint256,uint256,uint256)
c59057e4 => hashProposal(address[],uint256[],bytes[],bytes32)
7d5e81e2 => propose(address[],uint256[],bytes[],string)
75a12d72 => castVote(bytes32,uint256)
41f9b62c => castVoteWithReason(bytes32,uint256,string)
c3e51789 => castVoteBySig(address,bytes32,uint256,uint256,uint8,bytes32,bytes32)
ac836d73 => _castVote(bytes32,address,uint256,string)
7c10dea6 => queue(bytes32)
2656227d => execute(address[],uint256[],bytes[],bytes32)
c4d252f5 => cancel(bytes32)
fb6f93f9 => veto(bytes32)
61d585da => state(bytes32)
eb9019d4 => getVotes(address,uint256)
b58131b0 => proposalThreshold()
1703a018 => quorum()
430694cf => getProposal(bytes32)
cbcda201 => proposalSnapshot(bytes32)
3d24375f => proposalDeadline(bytes32)
d14b85b9 => proposalVotes(bytes32)
177e5a9f => proposalEta(bytes32)
48b1aeb4 => proposalThresholdBps()
3bec7f5b => quorumThresholdBps()
3932abb1 => votingDelay()
02a251a3 => votingPeriod()
d8bff440 => vetoer()
fc0c546a => token()
61d027b3 => treasury()
63d61a19 => updateVotingDelay(uint256)
ef00ef43 => updateVotingPeriod(uint256)
e42eb4f6 => updateProposalThresholdBps(uint256)
ebfdeacf => updateQuorumThresholdBps(uint256)
9dcbfd7d => updateVetoer(address)
1e5cc3bd => burnVetoer()
5ec29272 => _authorizeUpgrade(address)

[G-08] x += y costs more gas than x = x + y for state variables [16 gas per instance]

Context: Governor.sol#L280 Governor.sol#L285 Governor.sol#L290 Token.sol#L88 Token.sol#L118 MetadataRenderer.sol#L140

Description: x += y costs more gas than x = x + y for state variables.

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

contract GasTest is DSTest {
    Contract0 c0;
    Contract1 c1;
    
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    
    function testGas() public {
        c0.foo(33);
        c1.foo(33);
    }
}

contract Contract0 {
    uint256 _totalBorrow = 60;
    
    function foo(uint256 _interestEarned) public {
        _totalBorrow += interestEarned;
    }
}

contract Contract1 {
    uint256 _totalBorrow = 60;
    function foo(uint256 _interestEarned) public {
        _totalBorrow = _totalBorrow + interestEarned;
    }
}

Gas Report

╭───────────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/GasTest.t.sol:Contract0 contract ┆                 ┆      ┆        ┆      ┆         │
╞═══════════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 70805                                     ┆ 279             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ foo                                       ┆ 5302            ┆ 5302 ┆ 5302   ┆ 5302 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬──────┬────────┬──────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆      ┆        ┆      ┆         │
╞═══════════════════════════════════════════╪═════════════════╪══════╪════════╪══════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 69805                                     ┆ 274             ┆      ┆        ┆      ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg  ┆ median ┆ max  ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ foo                                       ┆ 5286            ┆ 5286 ┆ 5286   ┆ 5286 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴──────┴────────┴──────┴─────────╯

[G-09] Using private rather than public for constans, saves gas [7 gas per instance]

Context: Governor.sol#L27

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

contract GasTest is DSTest {
    
    Contract0 c0;
    Contract1 c1;
    
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    
    function testGas() public {
        c0._getPriceDecimals();
        c1._getPriceDecimals();
    }
}

contract Contract0 {
    uint32 public constant FACTOR_SCALE = 1e4;
        function _getPriceDecimals() public returns(uint32) {
        return FACTOR_SCALE;
    }
}
contract Contract1 {
    uint32 private constant FACTOR_SCALE = 1e4;
     function _getPriceDecimals() public returns(uint32) {
        return FACTOR_SCALE;
    }
}

Gas Report:

╭───────────────────────────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ src/test/GasTest.t.sol:Contract0 contract ┆                 ┆     ┆        ┆     ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 29281                                     ┆ 176             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg ┆ median ┆ max ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ _getPriceDecimals                         ┆ 156             ┆ 156 ┆ 156    ┆ 156 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴─────┴────────┴─────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆     ┆        ┆     ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 24075                                     ┆ 149             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg ┆ median ┆ max ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ _getPriceDecimals                         ┆ 149             ┆ 149 ┆ 149    ┆ 149 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴─────┴────────┴─────┴─────────╯

[G-10] Use assembly to check for address(0) [6 gas per instance]

Context:  Auction.sol#L104 Governor.sol#L70-L71 Governor.sol#L597 Treasury.sol#L48

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

contract GasTest is DSTest {
    Contract0 c0;
    Contract1 c1;
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    function testGas() public view {
        c0.setOperator(address(this));
        c1.setOperator(address(this));
    }
}
contract Contract0 {
    error Callback_InvalidParams();
    function setOperator(address operator_) public pure {
        if (address(operator_) == address(0)) revert Callback_InvalidParams();
    }
}
contract Contract1 {
    function setOperator(address operator_) public pure {
        assembly {
            if iszero(operator_) {
                mstore(0x00, "Callback_InvalidParams")
                revert(0x00, 0x20)
            }
        }
    }
}

Gas Report:

╭───────────────────────────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ src/test/GasTest.t.sol:Contract0 contract ┆                 ┆     ┆        ┆     ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 50899                                     ┆ 285             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg ┆ median ┆ max ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ setOperator                               ┆ 258             ┆ 258 ┆ 258    ┆ 258 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴─────┴────────┴─────┴─────────╯
╭───────────────────────────────────────────┬─────────────────┬─────┬────────┬─────┬─────────╮
│ src/test/GasTest.t.sol:Contract1 contract ┆                 ┆     ┆        ┆     ┆         │
╞═══════════════════════════════════════════╪═════════════════╪═════╪════════╪═════╪═════════╡
│ Deployment Cost                           ┆ Deployment Size ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 44893                                     ┆ 255             ┆     ┆        ┆     ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                             ┆ min             ┆ avg ┆ median ┆ max ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ setOperator                               ┆ 252             ┆ 252 ┆ 252    ┆ 252 ┆ 1       │
╰───────────────────────────────────────────┴─────────────────┴─────┴────────┴─────┴─────────╯

[G-11] Using > instead of >= is more gas efficient [3 gas per instance]

Context: Governor.sol#L261 Token.sol#L88

Proof Of Concept: The optimizer was turned on and set to 10000 runs.

use for GasTest: Governor.sol#L261

contract GasTest is DSTest {
    
    Contract0 c0;
    Contract1 c1;
    
    function setUp() public {
        c0 = new Contract0();
        c1 = new Contract1();
    }
    
    function testGas() public {
        c0._castVote0(2);
        c1._castVote1(2);
    }
}

contract Contract0 {
error INVALID_VOTE0();

    function _castVote0(uint256 _support) public returns (uint256) {
        if (_support >= 3) revert INVALID_VOTE0();
        return _support;
    }
}

contract Contract1 {
error INVALID_VOTE1();

    function _castVote1(uint256 _support) public returns (uint256) {
        if (_support > 2) revert INVALID_VOTE1();
        return _support;
    }
}