Nouns Builder contest - MiloTruck's results

Lines of code

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

Vulnerability details

Impact

For projects with a 0% founder percentage ownership, it is possible for the highest bidder of the first auction to not receive the minted token, and funds from the first auction to be stuck in the contract.

Proof of Concept

In Auction.sol, the contract determines if the first auction has been created using the statement auction.tokenId == 0, as shown below:

244:    function unpause() external onlyOwner {
245:        _unpause();
246:
247:        // If this is the first auction:
248:        if (auction.tokenId == 0) {
249:            // Transfer ownership of the contract to the DAO
250:            transferOwnership(settings.treasury);
251:
252:            // Start the first auction
253:            _createAuction();
254:        }
255:        // Else if the contract was paused and the previous auction was settled:
256:        else if (auction.settled) {
257:            // Start the next auction
258:            _createAuction();
259:        }
260:    }

If the check at line 248 passes, the unpause() function will directly call _createAuction() without checking that the current auction has been settled.

Thus, in a situation where the tokenId of the first auction is 0, the owner could potentially call unpause() again without settling the first auction, causing the current auction to be overwritten due to the call to _createAuction().

This would cause the following:

• Funds deposited by the highest bidder from the first auction would become stuck in the contract
• The highest bidder for the first auction will never receive the minted token

For example:

• A project is deployed with a total percentage ownership of 0% from founders
• The founder calls unpause() to start the first auction
  • At line 250, ownership is transferred to the treasury
  • The first auction is started via _createAuction() at line 253
• As founders have no ownership percentage of the minted tokens, the first token minted (tokenId = 0), will be used as the token for the first auction
• The DAO pauses the auction for some reason via pause()
• If the DAO calls unpause() without first calling settleAuction()
  • A new auction with tokenId = 1 would be created, overwriting the first auction

In this scenario, funds from the first auction would be unretrievable as the Auction contract does not have a function to send any amount of ETH to an address. Furthermore, as settleAuction() was not called, the highest bidder would not receive the token of the first auction.

The test in this gist demonstrates the above.

Recommended Mitigation Steps

At line 248, instead of checking auction.tokenId == 0 to determine if the first auction has been created, check if auction.startTime == 0 instead.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L80-L120 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L152-L157

Vulnerability details

Impact

If the Token contract is deployed with a founder's ownership percentage larger than 255, the minting of tokens will break.

Proof of Concept

In Token.sol:80-120, the function _addFounders() handles the addition of each founder as shown below:

 80:    for (uint256 i; i < numFounders; ++i) {
 81:        // Cache the percent ownership
 82:        uint256 founderPct = _founders[i].ownershipPct;
 83:
 84:        // Continue if no ownership is specified
 85:        if (founderPct == 0) continue;
 86:
 87:        // Update the total ownership and ensure it's valid
 88:        if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP(); 
 89:                                                                                                 
 90:        // Compute the founder's id
 91:        uint256 founderId = settings.numFounders++;
 92:
 93:        // Get the pointer to store the founder
 94:        Founder storage newFounder = founder[founderId];
 95:
 96:        // Store the founder's vesting details
 97:        newFounder.wallet = _founders[i].wallet;
 98:        newFounder.vestExpiry = uint32(_founders[i].vestExpiry);
 99:        newFounder.ownershipPct = uint8(founderPct);
100:
101:        // Compute the vesting schedule
102:        uint256 schedule = 100 / founderPct;
103:
104:        // Used to store the base token id the founder will recieve
105:        uint256 baseTokenId;
106:
107:        // For each token to vest:
108:        for (uint256 j; j < founderPct; ++j) {
109:            // Get the available token id
110:            baseTokenId = _getNextTokenId(baseTokenId);
111:
112:            // Store the founder as the recipient
113:            tokenRecipient[baseTokenId] = newFounder;
114:
115:            emit MintScheduled(baseTokenId, founderId, newFounder);
116:
117:            // Update the base token id
118:            (baseTokenId += schedule) % 100; 
119:        }
120:    }

At lines 88 and 99, founderPct is downcasted to uint8 before its value is checked or stored. However, at lines 102 and 108, the founderPct is used directly to handle the logic of allocating tokens to founders.

As seen from here, downcasting to uint8 takes the last 2 characters of the uint256 hexadecimal:

uint256 a = 0x100; 
uint8 b = uint8(a); // b equals to 0x00

Thus, if a founder is provided with ownershipPct set to larger than 0xff, such as 0x100, the following occurs:

• At line 88, the check passes:
  • uint8(founderPct) = uint8(0x100) = 0, which is less than 100
  • totalOwnership is set to 0
• At line 99, newFounder.ownershipPct is also set to 0
• At line 102, schedule is set to 0 as 100 / founderPct = 100 / 0x100, which rounds down to 0
• At line 108, the for statement loops 0x100 instead of 0 times
  • Since schedule is 0, tokenRecipient[] from 0 to 255 is set to the founder

In Token.sol:152-157, the mint() function uses a do-while loop to find the next tokenId not allocated to a founder:

152:    do {
153:        // Get the next token to mint
154:        tokenId = settings.totalSupply++;
155:
156:        // Lookup whether the token is for a founder, and mint accordingly if so
157:    } while (_isForFounder(tokenId));

Since _isForFounder() will always return true for any tokenId, the do-while loop will continue on forever, thus breaking the minting of tokens. However, this is not obvious to users as calling totalFounderOwnership() would return 0.

Additionally, note that setting ownershipPct to 100 will also break minting, but totalFounderOwnership() would return 100 as downcasting to uint8 would not change the value.

This test demonstrates the above.

Recommended Mitigation Steps

Store the value of uint8(founderPct) in a local variable, and use that variable throughout the entire _addFounders() function. Alternatively, use SafeCast.toUint8() instead of uint8().

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L101-L119 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L130-L136 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L177-L199

Vulnerability details

Impact

Due to how the allocation of tokens to founders is handled in Token.sol, projects with a certain sequence of founder percentage ownership may cause founders to receive less tokens than intended.

Proof of Concept

In the Token contract, before any token is minted, its _tokenId is passed into _isForFounder() to determine if the token belongs to a founder, as shown below:

177:    function _isForFounder(uint256 _tokenId) private returns (bool) {
178:        // Get the base token id
179:        uint256 baseTokenId = _tokenId % 100;
180:
181:        // If there is no scheduled recipient:
182:        if (tokenRecipient[baseTokenId].wallet == address(0)) {
183:            return false;
184:
185:            // Else if the founder is still vesting:
186:        } else if (block.timestamp < tokenRecipient[baseTokenId].vestExpiry) {
187:            // Mint the token to the founder
188:            _mint(tokenRecipient[baseTokenId].wallet, _tokenId);
189:
190:            return true;
191:
192:            // Else the founder has finished vesting:
193:        } else {
194:            // Remove them from future lookups
195:            delete tokenRecipient[baseTokenId];
196:
197:            return false;
198:        }
199:    }

At line 179, baseTokenId is derived by using % 100. baseTokenId is then used to check if tokenRecipient[baseTokenId] corresponds to a founder, and if so, the token is minted directly to the founder's wallet.

Since baseTokenId will always be smaller than 100 in _isForFounder(), the valid index range for tokenRecipient[] is from 0 to 99 inclusive. This means that assigning a founder to anything above 99, such as tokenRecipient[100], would not give tokens to the founder.

The logic that assigns founders to tokenRecipient[] based on their percentage ownership is in _addFounders():

101:    // Compute the vesting schedule
102:    uint256 schedule = 100 / founderPct;
103:
104:    // Used to store the base token id the founder will recieve
105:    uint256 baseTokenId;
106:
107:    // For each token to vest:
108:    for (uint256 j; j < founderPct; ++j) {
109:        // Get the available token id
110:        baseTokenId = _getNextTokenId(baseTokenId);
111:
112:        // Store the founder as the recipient
113:        tokenRecipient[baseTokenId] = newFounder;
114:
115:        emit MintScheduled(baseTokenId, founderId, newFounder);
116:
117:        // Update the base token id
118:        (baseTokenId += schedule) % 100; 
119:    }

For each founder, the code above is executed. It can be seen that baseTokenId is either incremented by schedule, or set using _getNextTokenId(), as shown below:

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

From the logic presented above, it is possible for a founder to be assigned to an index larger than 99 in tokenRecipient[], due to the following two issues:

1. At line 118, the % 100 statement used to ensure baseTokenId is smaller than 100 is never applied
2. _getNextTokenId() will always return the next _tokenId available

Issue 1

Due to the syntax error at line 118, baseTokenId is never reset to below 100 with the % operator. Thus, if enough founders with a percentage ownership are added, baseTokenId would eventually exceed 99.

For example, if founders are added in sequence with the following percentages:

1, 1, 1, 1, 1, 20

The last founder would receive less tokens than intended, as he is assigned to tokenRecipient[100].

test_Issue1() in this gist demonstrates this.

Issue 2

_getNextTokenId() does not contain any checks to ensure that the _tokenId returned is less than 100. Thus, if baseTokenId equals to 99, but tokenRecipient[99] already corresponds to another founder, _getNextTokenId() would automatically return 100, which is invalid.

The following sequence of percentages demonstrates this:

1, 1, 1, 1, 1, 1, 1, 1, 1, 10, 11

The last founder would receive less tokens than intended, as he is assigned to tokenRecipient[100].

test_Issue2() in this gist demonstrates this.

Recommended Mitigation Steps

For issue 1, rewrite line 118 in Token.sol to the following:

baseTokenId = (baseTokenId + schedule) % 100;

For issue 2, consider adding a check in either _getNextTokenId() or _addFounders() to ensure baseTokenid is always smaller than 100.

Low Report

Total: 9 instances over 3 issues

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

As seen from the Solidity Docs:

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.

Compared to abi.encodePacked(), abi.encode() pads all items to 32 bytes, which helps to prevent hash collisions. Additionally, if there is only one argument to abi.encodePacked(), it can be cast to bytes() or bytes32() instead.

There are 6 instances of this issue:

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

src/governance/governor/Governor.sol:
 226:        digest = keccak256(
 227:            abi.encodePacked(
 228:                "\x19\x01",
 229:                DOMAIN_SEPARATOR(),
 230:                keccak256(abi.encode(VOTE_TYPEHASH, _voter, _proposalId, _support, nonces[_voter]++, _deadline))
 231:            )
 232:        );

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

Unused/empty receive()/fallback() function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert.

There is 1 instance of this issue:

src/governance/treasury/Treasury.sol:
 269:        receive() external payable {}

Consider addings checks for signature malleability

Use OpenZeppelin's ECDSA contract, which checks for signature malleability, rather than calling ecrecover() directly.

There are 2 instances of this issue:

src/governance/governor/Governor.sol:
 236:        address recoveredAddress = ecrecover(digest, _v, _r, _s);

src/lib/token/ERC721Votes.sol:
 167:        address recoveredAddress = ecrecover(digest, _v, _r, _s);

Non-Critical Report

Total: 29 instances over 3 issues

Unused override function arguments

For functions declared as override, unused arguments should have the variable name removed or commented out to avoid compiler warnings.

There is 1 instance of this issue:

src/manager/Manager.sol:
 209:        function _authorizeUpgrade(address _newImpl) internal override onlyOwner {}

constants should be defined rather than using magic numbers

Even assembly code can benefit from using readable constants instead of hex/numeric literals.

There are 18 instances of this issue:
96:

src/manager/Manager.sol:
 123:        bytes32 salt = bytes32(uint256(uint160(token)) << 96); 

96:

src/manager/Manager.sol:
 165:        bytes32 salt = bytes32(uint256(uint160(_token)) << 96);

0xff:

src/manager/Manager.sol:
 167:        metadata = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, metadataHash)))));

0xff:

src/manager/Manager.sol:
 168:        auction = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, auctionHash)))));

0xff:

src/manager/Manager.sol:
 169:        treasury = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, treasuryHash)))));

0xff:

src/manager/Manager.sol:
 170:        governor = address(uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, governorHash)))));

10_000:

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

10_000:

src/governance/governor/Governor.sol:
 475:        return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;

2 weeks:

src/governance/treasury/Treasury.sol:
  57:        settings.gracePeriod = 2 weeks;

16:

src/token/metadata/MetadataRenderer.sol:
 179:        uint16[16] storage tokenAttributes = attributes[_tokenId];

16:

src/token/metadata/MetadataRenderer.sol:
 197:        seed >>= 16;

20:

src/token/metadata/MetadataRenderer.sol:
 210:        Strings.toHexString(uint256(uint160(address(this))), 20),

16:

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

0x01ffc9a7:

src/lib/token/ERC721.sol:
  63:        _interfaceId == 0x01ffc9a7 || // ERC165 Interface ID

0x80ac58cd:

src/lib/token/ERC721.sol:
  64:        _interfaceId == 0x80ac58cd || // ERC721 Interface ID

0x5b5e139f:

src/lib/token/ERC721.sol:
  65:        _interfaceId == 0x5b5e139f; // ERC721Metadata Interface ID

96:

src/lib/utils/Address.sol:
  26:        return bytes32(uint256(uint160(_account)) << 96);

5 minutes:

src/auction/Auction.sol:
  80:        settings.timeBuffer = 5 minutes;

event is missing indexed fields

Each event should use three indexed fields if there are three or more fields.

There are 10 instances of this issue:

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

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

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

src/governance/treasury/ITreasury.sol:
  22:        event TransactionExecuted(bytes32 proposalId, address[] targets, uint256[] values, bytes[] payloads);

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

src/lib/interfaces/IERC721Votes.sol:
  19:        event DelegateVotesChanged(address indexed delegate, uint256 prevTotalVotes, uint256 newTotalVotes);

src/lib/interfaces/IERC721.sol:
  28:        event ApprovalForAll(address indexed owner, address indexed operator, bool approved);

src/auction/IAuction.sol:
  22:        event AuctionBid(uint256 tokenId, address bidder, uint256 amount, bool extended, uint256 endTime);
  28:        event AuctionSettled(uint256 tokenId, address winner, uint256 amount);
  34:        event AuctionCreated(uint256 tokenId, uint256 startTime, uint256 endTime);

Gas Report

Total: 70 instances over 9 issues

For-loops: Index initialized with default value

Uninitialized uint variables are assigned with a default value of 0.

Thus, in for-loops, explicitly initializing an index with 0 costs unnecesary gas. For example, the following code:

for (uint256 i = 0; i < length; ++i) {

can be written without explicitly setting the index to 0:

for (uint256 i; i < length; ++i) {

There are 5 instances of this issue:

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

src/token/metadata/MetadataRenderer.sol:
 119:        for (uint256 i = 0; i < numNewProperties; ++i) {
 133:        for (uint256 i = 0; i < numNewItems; ++i) {
 189:        for (uint256 i = 0; i < numProperties; ++i) {
 229:        for (uint256 i = 0; i < numProperties; ++i) {

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

There are 2 instances of this issue:

src/token/Token.sol:
  91:        uint256 founderId = settings.numFounders++;
 154:        tokenId = settings.totalSupply++;

Arithmetics: Use Shift Right/Left instead of Division/Multiplication if possible

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 MUL and DIV opcodes use 5 gas, the SHL and SHR opcodes only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

For example, the following code:

uint256 b = a / 2;
uint256 c = a / 4;
uint256 d = a * 8;

can be changed to:

uint256 b = a >> 1;
uint256 c = a >> 2;
uint256 d = a << 3;

There is 1 instance of this issue:

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

Using bool for storage incurs overhead

Declaring storage variables as bool is more expensive compared to uint256, as explained here:

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.

Use uint256(1) and uint256(2) rather than 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.

There are 5 instances of this issue:

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

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

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

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

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

Use calldata instead of memory for read-only arguments in external functions

When an external function with a memory array is called, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length).

Using calldata directly instead of memory helps to save gas as values are read directly from calldata using calldataload, thus removing the need for such a loop in the contract code during runtime execution.

Also, structs have the same overhead as an array of length one.

There are 9 instances of this issue:

src/governance/governor/Governor.sol:
 117:        address[] memory _targets,
 118:        uint256[] memory _values,
 119:        bytes[] memory _calldatas,
 120:        string memory _description
 195:        string memory _reason

src/token/metadata/MetadataRenderer.sol:
 347:        function updateContractImage(string memory _newContractImage) external onlyOwner {
 355:        function updateRendererBase(string memory _newRendererBase) external onlyOwner {
 363:        function updateDescription(string memory _newDescription) external onlyOwner {

src/lib/proxy/UUPS.sol:
  55:        function upgradeToAndCall(address _newImpl, bytes memory _data) external payable onlyProxy {

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

As seen from here:

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

However, this does not apply to storage values as using reduced-size types might be beneficial to pack multiple elements into a single storage slot. Thus, where appropriate, use uint256/int256 and downcast when needed.

There are 39 instances of this issue:

src/governance/governor/Governor.sol:
 213:        uint8 _v,

src/governance/governor/IGovernor.sol:
 181:        uint8 v,

src/governance/governor/types/GovernorTypesV1.sol:
  21:        uint16 proposalThresholdBps;
  22:        uint16 quorumThresholdBps;
  24:        uint48 votingDelay;
  25:        uint48 votingPeriod;
  44:        uint32 timeCreated;
  45:        uint32 againstVotes;
  46:        uint32 forVotes;
  47:        uint32 abstainVotes;
  48:        uint32 voteStart;
  49:        uint32 voteEnd;
  50:        uint32 proposalThreshold;
  51:        uint32 quorumVotes;

src/governance/treasury/types/TreasuryTypesV1.sol:
  12:        uint128 gracePeriod;
  13:        uint128 delay;

src/token/types/TokenTypesV1.sol:
  18:        uint96 totalSupply;
  20:        uint8 numFounders;
  21:        uint8 totalOwnership;
  30:        uint8 ownershipPct;
  31:        uint32 vestExpiry;

src/token/metadata/types/MetadataRendererTypesV1.sol:
  20:        uint16 referenceSlot;

src/lib/token/ERC721Votes.sol:
 148:        uint8 _v,

src/lib/utils/SafeCast.sol:
   9:        function toUint128(uint256 x) internal pure returns (uint128) {
  15:        function toUint64(uint256 x) internal pure returns (uint64) {
  21:        function toUint48(uint256 x) internal pure returns (uint48) {
  27:        function toUint40(uint256 x) internal pure returns (uint40) {
  33:        function toUint32(uint256 x) internal pure returns (uint32) {
  39:        function toUint16(uint256 x) internal pure returns (uint16) {
  45:        function toUint8(uint256 x) internal pure returns (uint8) {

src/lib/utils/Initializable.sol:
  55:        modifier reinitializer(uint8 _version) {

src/lib/interfaces/IERC721Votes.sol:
  36:        uint64 timestamp;
  37:        uint192 votes;
  72:        uint8 v,

src/auction/types/AuctionTypesV1.sol:
  16:        uint40 duration;
  17:        uint40 timeBuffer;
  18:        uint8 minBidIncrement;
  33:        uint40 startTime;
  34:        uint40 endTime;

abi.encode() is less efficient than abi.encodePacked()

abi.encode() pads its parameters to 32 bytes, regardless of their type.

In comparison, abi.encodePacked() encodes its parameters using the minimal space required by their types. For example, encoding a uint8 it will use only 1 byte. Thus, abi.encodePacked() should be used instead of abi.encode() when possible as it saves space, thus reducing gas costs.

There are 4 instances of this issue:

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

internal functions only called once can be inlined to save gas

As compared to internal functions, a non-inlined function costs 20 to 40 gas extra because of two extra JUMP instructions and additional stack operations needed for function calls. Thus, consider inlining internal functions that are only called once in the function that calls them.

There are 2 instances of this issue:

src/token/Token.sol:
  71:        function _addFounders(IManager.FounderParams[] calldata _founders) internal {
 130:        function _getNextTokenId(uint256 _tokenId) internal view returns (uint256) {

Multiple accesses of a mapping/array should use a local variable cache

If a mapping/array is read with the same key/index multiple times in a function, it should be cached in a stack variable.

Caching a mapping's value in a local storage variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory.

There are 3 instances of this issue:
timestamps[_proposalId] in isReady():

src/governance/treasury/Treasury.sol:
  89:        return timestamps[_proposalId] != 0 && block.timestamp >= timestamps[_proposalId];

tokenRecipient[baseTokenId] in _isForFounder():

src/token/Token.sol:
 182:        if (tokenRecipient[baseTokenId].wallet == address(0)) {
 186:        } else if (block.timestamp < tokenRecipient[baseTokenId].vestExpiry) {
 188:        _mint(tokenRecipient[baseTokenId].wallet, _tokenId);

ipfsData[_item.referenceSlot] in _getItemImage():

src/token/metadata/MetadataRenderer.sol:
 259:        abi.encodePacked(ipfsData[_item.referenceSlot].baseUri, _propertyName, "/", _item.name, ipfsData[_item.referenceSlot].extension)