Nouns Builder contest - 0x1f8b's results

Lines of code

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

Vulnerability details

Impact

If the owner adds more than 16 properties, it will cause a denial of service.

Proof of Concept

If the owner, due to a failure to re-prioritize a tx, resubmits the addProperties transaction, or directly send a transaction with more than 16 _names calling MetadataRenderer.addProperties, these properties will remain stored forever without the possibility of being removed from the array properties.

for (uint256 i = 0; i < numNewProperties; ++i) {
    // Append storage space
    properties.push();

    // Get the new property id
    uint256 propertyId = numStoredProperties + i;

    // Store the property name
    properties[propertyId].name = _names[i];

    emit PropertyAdded(propertyId, _names[i]);
}

Later, when onMinted is called on the token, service will be denied since the array of tokenAttributes is a maximum of 16 entries.

uint16[16] storage tokenAttributes = attributes[_tokenId];

// Cache the total number of properties available
uint256 numProperties = properties.length;

// Store the total as reference in the first slot of the token's array of attributes
tokenAttributes[0] = uint16(numProperties);

unchecked {
    // For each property:
    for (uint256 i = 0; i < numProperties; ++i) {
        // Get the number of items to choose from
        uint256 numItems = properties[i].items.length;

        // Use the token's seed to select an item
        tokenAttributes[i + 1] = uint16(seed % numItems);

        // Adjust the randomness
        seed >>= 16;
    }
}

Recommended Mitigation Steps

• Remove the max length in attributes or limit the properties in MetadataRenderer.addProperties.

• Low
  • 1. Outdated compiler
  • 2. Method marked as payable unnecessarily
  • 3. Integer overflow by unsafe casting
  • 4. Lack of checks
  • 5. Not fully decentralized
  • 6. Unsecure state order
  • 7. Timestamp dependence
• Non critical
  • 8. Use abstract for base contracts
  • 9. Avoid duplicate logic
  • 10. Avoid hardcoded values
  • 11. Unpredictable change of owner
  • 12. Improve user experience

Low

1. Outdated compiler

The pragma version used are:

pragma solidity ^0.8.15;
pragma solidity 0.8.15;
pragma solidity ^0.8.4;
pragma solidity ^0.8.0;

Note that mixing pragma are not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.

The minimum required version must be 0.8.16; otherwise, contracts will be affected by the following important bug fixes:

0.8.3:

• Optimizer: Fix bug on incorrect caching of Keccak-256 hashes.

0.8.4:

• ABI Decoder V2: For two-dimensional arrays and specially crafted data in memory, the result of abi.decode can depend on data elsewhere in memory. Calldata decoding is not affected.

0.8.9:

• Immutables: Properly perform sign extension on signed immutables.
• User Defined Value Type: Fix storage layout of user defined value types for underlying types shorter than 32 bytes.

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

Apart from these, there are several minor bug fixes and improvements.

2. Method marked as payable unnecessarily

The following methods have been marked as payable, this may cause both administrators and users calling them to accidentally deposit ether with no recovery possible.

This may be done to save gas, but may result in more loss than benefit in case of one human error.

Affected source code:

• Manager.sol:61
• ERC1967Proxy.sol:21
• MetadataRenderer.sol:32
• Token.sol:30
• Auction.sol:39
• Governor.sol:41

3. Integer overflow by unsafe casting

Keep in mind that the version of solidity used, despite being greater than 0.8, does not prevent integer overflows during casting, it only does so in mathematical operations.

It is necessary to safely convert between the different numeric types.

Example:

A dangerous example is with the quorum method.

        proposal.quorumVotes = uint32(quorum());

This method depends the token's totalSupply, so it could overflow with certains tokens.

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

Recommendation:

Use a safeCast from Open Zeppelin.

Affected source code:

• ERC721Votes.sol:252
• Token.sol:88
• Token.sol:98
• Token.sol:99
• Token.sol:123
• Token.sol:124
• Auction.sol:149
• Auction.sol:223
• Auction.sol:224
• Governor.sol:168
• Governor.sol:280
• Governor.sol:285
• Governor.sol:290

4. Lack of checks

The following methods have a lack of checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code for address(0):

• Manager.sol:62-66
• MetadataRenderer.sol:33
• Treasury.sol:33
• Token.sol:31
• Token.sol:65
• Token.sol:66
• Auction.sol:40
• Auction.sol:41
• Auction.sol:79
• Governor.sol:42

It wasn't checked _delay max value (E.g. more than 1 month...):

• Treasury.sol:43

5. Not fully decentralized

The updateVotingDelay, updateVotingPeriod, updateProposalThresholdBps, updateQuorumThresholdBps, updateVetoer, burnVetoer methods in the Governor contract have the onlyOwner modifier, but to be fully decentralized, these methods must verify that the address is the same as the contract himself to force a proposal to call these methods.

Affected source code:

• Governor.sol:564-609

6. Unsecure state order

Negative states must be processed before positive states, otherwise, a proposal that is Expired or Executed, but is Active or Pending will be returned as Active or Pending, this makes it necessary to check that the state is correct from outside this method. I mean, changing outside the variables that alter this state in the correct way.

    function state(bytes32 _proposalId) public view returns (ProposalState) {
        // Get a copy of the proposal
        Proposal memory proposal = proposals[_proposalId];

        // Ensure the proposal exists
        if (proposal.voteStart == 0) revert PROPOSAL_DOES_NOT_EXIST();

        // If the proposal was executed:
        if (proposal.executed) {
            return ProposalState.Executed;

            // Else if the proposal was canceled:
        } else if (proposal.canceled) {
            return ProposalState.Canceled;

            // Else if the proposal was vetoed:
        } else if (proposal.vetoed) {
            return ProposalState.Vetoed;

            // Else if voting has not started:
        } else if (block.timestamp < proposal.voteStart) {
            return ProposalState.Pending;

+           // Else if the proposal can no longer be executed:
+       } else if (settings.treasury.isExpired(_proposalId)) {
+           return ProposalState.Expired;
-           // Else if voting has not ended:
-       } else if (block.timestamp < proposal.voteEnd) {
-           return ProposalState.Active;

            // Else if the proposal failed (outvoted OR didn't reach quorum):
        } else if (proposal.forVotes < proposal.againstVotes || proposal.forVotes < proposal.quorumVotes) {
            return ProposalState.Defeated;

+           // Else if voting has not ended:
+       } else if (block.timestamp < proposal.voteEnd) {
+           return ProposalState.Active;
            // Else if the proposal has not been queued:
        } else if (settings.treasury.timestamp(_proposalId) == 0) {
            return ProposalState.Succeeded;

-           // Else if the proposal can no longer be executed:
-       } else if (settings.treasury.isExpired(_proposalId)) {
-           return ProposalState.Expired;

            // Else the proposal is queued
        } else {
            return ProposalState.Queued;
        }
    }

Affected source code:

• Governor.sol:413-456

7. Timestamp dependence

There are three main considerations when using a timestamp to execute a critical function in a contract, especially when actions involve fund transfer.

• Timestamp manipulation
  • Be aware that the timestamp of the block can be manipulated by a miner
• The 15-second Rule
  • The Yellow Paper (Ethereum’s reference specification) does not specify a constraint on how much blocks can drift in time, but it does specify that each timestamp should be bigger than the timestamp of its parent. Popular Ethereum protocol implementations Geth and Parity both reject blocks with timestamp more than 15 seconds in future. Therefore, a good rule of thumb in evaluating timestamp usage is: if the scale of your time-dependent event can vary by 15 seconds and maintain integrity, it is safe to use a block.timestamp.
• Avoid using block.number as a timestamp
  • It is possible to estimate a time delta using the block.number property and average block time, however this is not future proof as block times may change (such as fork reorganisations and the difficulty bomb). In a sale spanning days, the 15-second rule allows one to achieve a more reliable estimate of time. See SWC-116

Reference:

• https://consensys.net/blog/developers/solidity-best-practices-for-smart-contract-security/

Affected source code:

• Treasury.sol:89

---

Non critical

8. Use abstract for base contracts

abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• TokenStorageV1.sol:9
• MetadataRendererStorageV1.sol:9
• ManagerStorageV1.sol:7
• TreasuryTypesV1.sol:7
• TreasuryStorageV1.sol:9
• GovernorStorageV1.sol:9
• AuctionTypesV1.sol:7
• AuctionStorageV1.sol:10

9. Avoid duplicate logic

Use Address.toBytes32 instead of replicate the logic.

Affected source code:

• Manager.sol:123
• Manager.sol:165

10. Avoid hardcoded values

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these addresses from the source code.

use selectors:

• ERC721.sol:63-L65

Use immutable with the formula, instead of the result:

• ERC1967Upgrade.sol:21
• ERC1967Upgrade.sol:24

11. Unpredictable change of owner

The MetadataRenderer.addProperties method takes advantage when ipfsData is not defined to transfer the owner to the treasury, however this logic should be done in an initialization method and not in a configuration one, since the user does not expect this type of change when calling to this type of methods, it is convenient to adapt the logic or mention this behavior in a comment.

Affected source code:

• MetadataRenderer.sol:102

12. Improve user experience

In the Auction._handleOutgoingTransfer method, before throwing the INSOLVENT error, check if the contract has WETH, because maybe it's possible to send WETH instead of an throwing an error.

    function _handleOutgoingTransfer(address _to, uint256 _amount) private {
        // Ensure the contract has enough ETH to transfer
        if (address(this).balance < _amount) revert INSOLVENT();

        // Used to store if the transfer succeeded
        bool success;

        assembly {
            // Transfer ETH to the recipient
            // Limit the call to 50,000 gas
            success := call(50000, _to, _amount, 0, 0, 0, 0)
        }

        // If the transfer failed:
        if (!success) {
            // Wrap as WETH
            IWETH(WETH).deposit{ value: _amount }();

            // Transfer WETH instead
            IWETH(WETH).transfer(_to, _amount);
        }
    }

Affected source code:

• Auction.sol:346

• Gas
  • 1. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 3 = 39
  • 2. Shift right instead of dividing by 2
    • Total gas saved: 172 * 1 = 172
  • 3. Remove empty blocks
  • 4. Use calldata instead of memory
  • 5. Change bool to uint256 can save gas
  • 6. constants expressions are expressions, not constants
  • 7. Optimize ERC721Votes.delegateBySig logic
  • 8. Optimize MetadataRenderer.initialize logic
  • 9. There's no need to set default values for variables
    • Total gas saved: 8 * 5 = 40
  • 10. Optimize MetadataRenderer.onMinted logic
  • 11. Avoid unused returns
  • 12. Optimize MetadataRenderer.getAttributes
  • 13. Change arguments type in MetadataRenderer
  • 14. Optimize Token.initialize
  • 15. Remove unused math
  • 16. Use array instead of mapping
  • 17. Change arguments type in Auction
  • 18. Change arguments type in Governor

Gas

1. Avoid compound assignment operator in state variables

Using compound assignment operators for state variables (like State += X or State -= X ...) it's more expensive than using operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
uint private _a;
function testShort() public {
_a += 1;
}
}

contract TesterB {
uint private _a;
function testLong() public {
_a = _a + 1;
}
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• Governor.sol:280
• Governor.sol:285
• Governor.sol:290

Total gas saved: 13 * 3 = 39

2. Shift right instead of dividing by 2

Shifting one to the right will calculate a division by two.

he SHR opcode only requires 3 gas, compared to the DIV opcode's consumption of 5. Additionally, shifting is used to get around Solidity's division operation's division-by-0 prohibition.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testDiv(uint a) public returns (uint) { return a / 2; }
}

contract TesterB {
function testShift(uint a) public returns (uint) { return a >> 1; }
}

Gas saving executing: 172 per entry

TesterA.testDiv:    21965
TesterB.testShift:  21793   

Affected source code:

• ERC721Votes.sol:95

Total gas saved: 172 * 1 = 172

3. Remove empty blocks

An empty block or an empty method generally indicates a lack of logic that it’s unnecessary and should be eliminated, call a method that literally does nothing only consumes gas unnecessarily, if it is a virtual method which is expects it to be filled by the class that implements it, it is better to use abstract contracts with just the definition.

Sample code:

pragma solidity >=0.4.0 <0.7.0;

abstract contract Feline {
    function utterance() public virtual returns (bytes32);
}

Reference:

• https://docs.soliditylang.org/en/v0.6.0/contracts.html?highlight=abstract#abstract-contracts

Affected source code:

• Treasury.sol:269
• ERC721.sol:54
• ERC721.sol:57
• ERC721.sol:235-239
• ERC721.sol:245-249
• Manager.sol:209

4. Use calldata instead of memory

Some methods are declared as external but the arguments are defined as memory instead of as calldata.

By marking the function as external it is possible to use calldata in the arguments shown below and save significant gas.

Recommended change:

-   function updateContractImage(string memory _newContractImage) external onlyOwner {
+   function updateContractImage(string calldata _newContractImage) external onlyOwner {
        emit ContractImageUpdated(settings.contractImage, _newContractImage);
        settings.contractImage = _newContractImage;
    }

Affected source code:

• MetadataRenderer.sol:347-351
• MetadataRenderer.sol:355-359
• MetadataRenderer.sol:363-367

5. Change bool to uint256 can save gas

Because each write operation requires an additional SLOAD to read the slot's contents, replace the bits occupied by the boolean, and then write back, booleans are more expensive than uint256 or any other type that uses a complete word. This cannot be turned off because it is the compiler's defense against pointer aliasing and contract upgrades.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27

Also, this is applicable to integer types different from uint256 or int256.

Affected source code for booleans:

• ManagerStorageV1.sol:10
• MetadataRendererTypesV1.sol:11
• GovernorStorageV1.sol:19
• Pausable.sol:15
• Initializable.sol:20

6. constants expressions are expressions, not constants

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

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

Reference:

• https://github.com/ethereum/solidity/issues/9232

Consequences: each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..). since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library).

Affected source code:

• EIP712.sol:19
• ERC721Votes.sol:21
• Governor.sol:27

7. Optimize ERC721Votes.delegateBySig logic

It's possible to optimize the method ERC721Votes.delegateBySig as follows:

Recommended changes:

    function delegateBySig(
        address _from,
        address _to,
        uint256 _deadline,
        uint8 _v,
        bytes32 _r,
        bytes32 _s
    ) external {
        // Ensure the signature has not expired
        if (block.timestamp > _deadline) revert EXPIRED_SIGNATURE();
+       if (_from == address(0)) revert INVALID_SIGNATURE();

        // Used to store the digest
        bytes32 digest;

        // Cannot realistically overflow
        unchecked {
            // Compute the hash of the domain seperator with the typed delegation data
            digest = keccak256(
-               abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR(), keccak256(abi.encode(DELEGATION_TYPEHASH, _from, _to, nonces[_from]++, _deadline)))
+               abi.encodePacked("\x19\x01", DOMAIN_SEPARATOR(), keccak256(abi.encode(DELEGATION_TYPEHASH, _from, _to, ++nonces[_from], _deadline)))
            );
        }

        // Recover the message signer
        address recoveredAddress = ecrecover(digest, _v, _r, _s);

        // Ensure the recovered signer is the voter
-       if (recoveredAddress == address(0) || recoveredAddress != _from) revert INVALID_SIGNATURE();
+       if (recoveredAddress != _from) revert INVALID_SIGNATURE();

        // Update the delegate
        _delegate(_from, _to);
    }

Affected source code:

• ERC721Votes.sol:170

8. Optimize MetadataRenderer.initialize logic

It's possible to optimize the method MetadataRenderer.initialize as follows:

Recommended changes:

    function initialize(
        bytes calldata _initStrings,
        address _token,
        address _founder,
        address _treasury
    ) external initializer {
        // Ensure the caller is the contract manager
        if (msg.sender != address(manager)) revert ONLY_MANAGER();

        // Decode the token initialization strings
+       (settings.name, , settings.description, settings.contractImage, settings.rendererBase) = abi.decode(
-       (string memory _name, , string memory _description, string memory _contractImage, string memory _rendererBase) = abi.decode(
            _initStrings,
            (string, string, string, string, string)
        );

        // Store the renderer settings
-       settings.name = _name;
-       settings.description = _description;
-       settings.contractImage = _contractImage;
-       settings.rendererBase = _rendererBase;
        settings.token = _token;
        settings.treasury = _treasury;

        // Grant initial ownership to a founder
        __Ownable_init(_founder);
    }

Affected source code:

• MetadataRenderer.sol:45-70

9. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384   

Affected source code:

• MetadataRenderer.sol:119
• MetadataRenderer.sol:133
• MetadataRenderer.sol:189
• MetadataRenderer.sol:229
• Treasury.sol:162

Total gas saved: 8 * 5 = 40

10. Optimize MetadataRenderer.onMinted logic

It's possible to optimize the method MetadataRenderer.onMinted as follows:

Recommended changes:

    function onMinted(uint256 _tokenId) external returns (bool) {
        ...
        unchecked {
            // For each property:
-           for (uint256 i = 0; i < numProperties; ++i) {
+           for (uint256 i = 0; i < numProperties;) {
                // Get the number of items to choose from
                uint256 numItems = properties[i].items.length;

                // Use the token's seed to select an item
+               ++i;
+               tokenAttributes[i] = uint16(seed % numItems);
-               tokenAttributes[i + 1] = uint16(seed % numItems);

                // Adjust the randomness
                seed >>= 16;
            }
        }
        return true;
    }

Affected source code:

• MetadataRenderer.sol:189-194

11. Avoid unused returns

Having a method that always returns the same value is not correct in terms of consumption, if you want to modify a value, and the method will perform a revert in case of failure, it is not necessary to return a true, since it will never be false. It is less expensive not to return anything, and it also eliminates the need to double-check the returned value by the caller.

Affected source code:

• MetadataRenderer.sol:201

12. Optimize MetadataRenderer.getAttributes

It's possible to optimize the method MetadataRenderer.getAttributes as follows:

Recommended changes:

+   string private immutable _queryStringPath = abi.encodePacked("?contractAddress=",Strings.toHexString(uint256(uint160(address(this))), 20),"&tokenId=");
+
    function getAttributes(uint256 _tokenId) public view returns (bytes memory aryAttributes, bytes memory queryString) {
        // Get the token's query string
        queryString = abi.encodePacked(
-           "?contractAddress=",
-           Strings.toHexString(uint256(uint160(address(this))), 20),
-           "&tokenId=",
+           _queryStringPath,
            Strings.toString(_tokenId)
        );
        ...

Affected source code:

• MetadataRenderer.sol:206-213

13. Change arguments type in MetadataRenderer

It's possible to optimize some methods in MetadataRenderer contract changing the argument type as follows:

Recommended changes:

-   function initialize(address _governor, uint256 _delay) external initializer {
+   function initialize(address _governor, uint128 _delay) external initializer {
        // Ensure the caller is the contract manager
        if (msg.sender != address(manager)) revert ONLY_MANAGER();

        // Ensure a governor address was provided
        if (_governor == address(0)) revert ADDRESS_ZERO();

        // Grant ownership to the governor
        __Ownable_init(_governor);

        // Store the time delay
-       settings.delay = SafeCast.toUint128(_delay);
+       settings.delay = _delay;

        // Set the default grace period
        settings.gracePeriod = 2 weeks;

        emit DelayUpdated(0, _delay);
    }
    ...
-   function updateDelay(uint256 _newDelay) external {
+   function updateDelay(uint128 _newDelay) external {
        // Ensure the caller is the treasury itself
        if (msg.sender != address(this)) revert ONLY_TREASURY();

        emit DelayUpdated(settings.delay, _newDelay);

        // Update the delay
-       settings.delay = SafeCast.toUint128(_newDelay);
+       settings.delay = _newDelay;
    }
    ...
-   function updateGracePeriod(uint256 _newGracePeriod) external {
+   function updateGracePeriod(uint128 _newGracePeriod) external {
        // Ensure the caller is the treasury itself
        if (msg.sender != address(this)) revert ONLY_TREASURY();

        emit GracePeriodUpdated(settings.gracePeriod, _newGracePeriod);

        // Update the grace period
-       settings.gracePeriod = SafeCast.toUint128(_newGracePeriod);
+       settings.gracePeriod = _newGracePeriod;
    }

Affected source code:

• Treasury.sol:43
• Treasury.sol:210
• Treasury.sol:222

14. Optimize Token.initialize

It is not necessary to send more information than necessary to the initialize method. It's possible to optimize the method Token.initialize as follows:

Recommended changes:

    function initialize(
        IManager.FounderParams[] calldata _founders,
        bytes calldata _initStrings,
        address _metadataRenderer,
        address _auction
    ) external initializer {
        // Ensure the caller is the contract manager
        if (msg.sender != address(manager)) revert ONLY_MANAGER();

        // Initialize the reentrancy guard
        __ReentrancyGuard_init();

        // Store the founders and compute their allocations
        _addFounders(_founders);

        // Decode the token name and symbol
-       (string memory _name, string memory _symbol, , , ) = abi.decode(_initStrings, (string, string, string, string, string));
+       (string memory _name, string memory _symbol) = abi.decode(_initStrings, (string, string));

        // Initialize the ERC-721 token
        __ERC721_init(_name, _symbol);

        // Store the metadata renderer and auction house
        settings.metadataRenderer = IBaseMetadata(_metadataRenderer);
        settings.auction = _auction;
    }

Affected source code:

• Token.sol:59

15. Remove unused math

There are mathematical operations that can be eliminated and not affect the logic of the contract.

Recommended changes:

    function _addFounders(IManager.FounderParams[] calldata _founders) internal {
        ...

        unchecked {
            // For each founder:
            for (uint256 i; i < numFounders; ++i) {
                ...

                // For each token to vest:
                for (uint256 j; j < founderPct; ++j) {
                    ...
                    // Update the base token id
-                   (baseTokenId += schedule) % 100;
+                   baseTokenId += schedule;
                }
            }
            ...
        }
    }

Affected source code:

• Token.sol:118

16. Use array instead of mapping

The Token contract uses a map to store the founders, but the key is the index, it is better to use an array instead of a mapping because there is no need to store the length and it will also be cheaper to return the array without iterating through all the entries.

    function getFounders() external view returns (Founder[] memory) {
        // Cache the number of founders
        uint256 numFounders = settings.numFounders;

        // Get a temporary array to hold all founders
        Founder[] memory founders = new Founder[](numFounders);

        // Cannot realistically overflow
        unchecked {
            // Add each founder to the array
            for (uint256 i; i < numFounders; ++i) founders[i] = founder[i];
        }

        return founders;
    }

Affected source code:

• Token.sol:251-265

17. Change arguments type in Auction

It's possible to optimize some methods in Auction contract changing the argument type as follows:

Recommended changes:

-   function setDuration(uint256 _duration) external onlyOwner {
-       settings.duration = SafeCast.toUint40(_duration);
+   function setDuration(uint40 _duration) external onlyOwner {
+       settings.duration = _duration;

        emit DurationUpdated(_duration);
    }

    /// @notice Updates the time buffer of each auction
    /// @param _timeBuffer The new time buffer
-   function setTimeBuffer(uint256 _timeBuffer) external onlyOwner {
-       settings.timeBuffer = SafeCast.toUint40(_timeBuffer);
+   function setTimeBuffer(uint40 _timeBuffer) external onlyOwner {
+       settings.timeBuffer = _timeBuffer;

        emit TimeBufferUpdated(_timeBuffer);
    }

    /// @notice Updates the minimum bid increment of each subsequent bid
    /// @param _percentage The new percentage
-   function setMinimumBidIncrement(uint256 _percentage) external onlyOwner {
-       settings.minBidIncrement = SafeCast.toUint8(_percentage);
+   function setMinimumBidIncrement(uint40 _percentage) external onlyOwner {
+       settings.minBidIncrement = _percentage;

        emit MinBidIncrementPercentageUpdated(_percentage);
    }

Affected source code:

• Auction.sol:77
• Auction.sol:307-335

18. Change arguments type in Governor

It's possible to optimize some methods in Governor contract changing the argument type as follows:

Recommended changes:

    function initialize(
        address _treasury,
        address _token,
        address _vetoer,
-       uint256 _votingDelay,
-       uint256 _votingPeriod,
-       uint256 _proposalThresholdBps,
-       uint256 _quorumThresholdBps
+       uint48 _votingDelay,
+       uint48 _votingPeriod,
+       uint16 _proposalThresholdBps,
+       uint16 _quorumThresholdBps
    ) external initializer {
        // Ensure the caller is the contract manager
        if (msg.sender != address(manager)) revert ONLY_MANAGER();

        // Ensure non-zero addresses are provided
        if (_treasury == address(0)) revert ADDRESS_ZERO();
        if (_token == address(0)) revert ADDRESS_ZERO();

        // Store the governor settings
        settings.treasury = Treasury(payable(_treasury));
        settings.token = Token(_token);
        settings.vetoer = _vetoer;
-       settings.votingDelay = SafeCast.toUint48(_votingDelay);
-       settings.votingPeriod = SafeCast.toUint48(_votingPeriod);
-       settings.proposalThresholdBps = SafeCast.toUint16(_proposalThresholdBps);
-       settings.quorumThresholdBps = SafeCast.toUint16(_quorumThresholdBps);
+       settings.votingDelay = _votingDelay;
+       settings.votingPeriod = _votingPeriod;
+       settings.proposalThresholdBps = _proposalThresholdBps;
+       settings.quorumThresholdBps = _quorumThresholdBps;

        // Initialize EIP-712 support
        __EIP712_init(string.concat(settings.token.symbol(), " GOV"), "1");

        // Grant ownership to the treasury
        __Ownable_init(_treasury);
    }
    ...
-   function updateVotingDelay(uint256 _newVotingDelay) external onlyOwner {
+   function updateVotingDelay(uint48 _newVotingDelay) external onlyOwner {
        emit VotingDelayUpdated(settings.votingDelay, _newVotingDelay);

-       settings.votingDelay = SafeCast.toUint48(_newVotingDelay);
+       settings.votingDelay = _newVotingDelay;
    }

    /// @notice Updates the voting period
    /// @param _newVotingPeriod The new voting period
-   function updateVotingPeriod(uint256 _newVotingPeriod) external onlyOwner {
+   function updateVotingPeriod(uint48 _newVotingPeriod) external onlyOwner {
        emit VotingPeriodUpdated(settings.votingPeriod, _newVotingPeriod);

-       settings.votingPeriod = SafeCast.toUint48(_newVotingPeriod);
+       settings.votingPeriod = _newVotingPeriod;
    }

    /// @notice Updates the minimum proposal threshold
    /// @param _newProposalThresholdBps The new proposal threshold basis points
-   function updateProposalThresholdBps(uint256 _newProposalThresholdBps) external onlyOwner {
+   function updateProposalThresholdBps(uint16 _newProposalThresholdBps) external onlyOwner {
        emit ProposalThresholdBpsUpdated(settings.proposalThresholdBps, _newProposalThresholdBps);

-       settings.proposalThresholdBps = SafeCast.toUint16(_newProposalThresholdBps);
+       settings.proposalThresholdBps = _newProposalThresholdBps;
    }

    /// @notice Updates the minimum quorum threshold
    /// @param _newQuorumVotesBps The new quorum votes basis points
-   function updateQuorumThresholdBps(uint256 _newQuorumVotesBps) external onlyOwner {
+   function updateQuorumThresholdBps(uint16 _newQuorumVotesBps) external onlyOwner {
        emit QuorumVotesBpsUpdated(settings.quorumThresholdBps, _newQuorumVotesBps);

-       settings.quorumThresholdBps = SafeCast.toUint16(_newQuorumVotesBps);
+       settings.quorumThresholdBps = _newQuorumVotesBps;
    }

Affected source code:

• Governor.sol:61-64
• Governor.sol:564-592