Nouns Builder contest - __141345__'s results

Lines of code

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

Vulnerability details

Impact

baseTokenId update is wrong, the id generating mechanism might not work as expected. Some NFTs are reserved for founders, but now bashTokenId could be above 100, the reserved id will not be minted for them, equivalent to loss those NFTs.

Proof of Concept

According to the following, % 100 would not take effect src/token/Token.sol

    function _addFounders() internal {
118:    (baseTokenId += schedule) % 100;

The check for the baseTokenId above 100 will never be reached, since in _isForFounder() the % 100 will limit the id below 100.

    function _isForFounder(uint256 _tokenId) private returns (bool) {
        // Get the base token id
        uint256 baseTokenId = _tokenId % 100;

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

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

            return true;

            // Else the founder has finished vesting:
        } else {
            // Remove them from future lookups
            delete tokenRecipient[baseTokenId];

            return false;
        }
    }

Effectively those founders will lose those reserved NFTs.

Tools Used

Manual analysis.

Recommended Mitigation Steps

change to:

        baseTokenId = (baseTokenId + schedule) % 100;

Lines of code

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

Vulnerability details

Impact

totalSupply can not reflect the true number of tokens minted. The governance function quorum() will refer to totalSupply, inaccurate value will influence the governance function.

Proof of Concept

If no bid for an auction, the corresponding tokenId will be burned. src/auction/Auction.sol

194-198:
            // Else no bid was placed:
        } else {
            // Burn the token
            token.burn(_auction.tokenId);
        }

But totalSupply is not decreased here.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• adjust totalSupply when NFT is burned.
• used a different variable to store the NFT id.

Lines of code

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

Vulnerability details

Impact

The auction parameters can be changed anytime, even during ongoing auctions, and take effect immediately. Users may need time to react to the changes. The impacts maybe followings:

• some sudden changes may cause bidder's transaction fail, such as setReservePrice() and setMinimumBidIncrement()
• some changes may change users expectation about the auction, such as setDuration() and setTimeBuffer(), with different time parameters, bidders will use different strategy

Proof of Concept

src/auction/Auction.sol

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

        emit DurationUpdated(_duration);
    }

    function setReservePrice(uint256 _reservePrice) external onlyOwner {
        settings.reservePrice = _reservePrice;

        emit ReservePriceUpdated(_reservePrice);
    }

    function setTimeBuffer(uint256 _timeBuffer) external onlyOwner {
        settings.timeBuffer = SafeCast.toUint40(_timeBuffer);

        emit TimeBufferUpdated(_timeBuffer);
    }

    function setMinimumBidIncrement(uint256 _percentage) external onlyOwner {
        settings.minBidIncrement = SafeCast.toUint8(_percentage);

        emit MinBidIncrementPercentageUpdated(_percentage);
    }```


## Tools Used
Manual analysis.

## Recommended Mitigation Steps

- do not apply changed parameters on ongoing auctions 
- add a timelock for the changes

Lines of code

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

Vulnerability details

Impact

When NFT totalSupply is low, the quorum() might be more sensitive to rounding errors. Sometimes the error could be large, or even return 0 result. The vote results based on these quorum() will deviate from the intention of governance.

Proof of Concept

Any new bidder can call createBid() to place a bid:

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

In test file, quorumThresholdBps is set to 2_000, then:

• when totalSupply of NFT is less than 5, quorum() will return 0.
• when totalSupply of NFT is 9, quorum() will return 1.

Seems the error is too big for small totalSupply and quorumThresholdBps.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• For low totalSupply, do additional checks for quorum() results, maybe round up.
• Set minimum and maximum values for quorumThresholdBps in initialize() and updateProposalThresholdBps().

EVENT IS MISSING INDEXED FIELDS

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

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);

src/governance/governor/IGovernor.sol
18-26:
    event ProposalCreated(
        bytes32 proposalId,
        address[] targets,
        uint256[] values,
        bytes[] calldatas,
        string description,
        bytes32 descriptionHash,
        Proposal proposal
    );
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/lib/interfaces/IERC721Votes.sol
19:    event DelegateVotesChanged(address indexed delegate, uint256 prevTotalVotes, uint256 newTotalVotes);

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

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

AVOID FLOATING PRAGMAS: THE VERSION SHOULD BE LOCKED

The pragma declared across the contract range from ^0.8.4- ^0.8.15. Locking the pragma (for e.g. by not using ^ in pragma solidity 0.8.15) ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs. (see here)

use string.concat() instead of abi.encodePacked()

src/token/metadata/MetadataRenderer.sol
208-213:
        queryString = abi.encodePacked(
            "?contractAddress=",
            Strings.toHexString(uint256(uint160(address(this))), 20),
            "&tokenId=",
            Strings.toString(_tokenId)
        );
243-244:
        aryAttributes = abi.encodePacked(aryAttributes, '"', property.name, '": "', item.name, '"', isLast ? "" : ",");
        queryString = abi.encodePacked(queryString, "&images=", _getItemImage(item, property.name));
258-260:
        string(
            abi.encodePacked(ipfsData[_item.referenceSlot].baseUri, _propertyName, "/", _item.name, ipfsData[_item.referenceSlot].extension)
        )
272-280:
        abi.encodePacked(
            '{"name": "',
            settings.name,
            '", "description": "',
            settings.description,
            '", "image": "',
            settings.contractImage,
            '"}'
        )
290-303:
        abi.encodePacked(
            '{"name": "',
            settings.name,
            " #",
            LibUintToString.toString(_tokenId),
            '", "description": "',
            settings.description,
            '", "image": "',
            settings.rendererBase,
            queryString,
            '", "properties": {',
            aryAttributes,
            "}}"
        )
309:
        return string(abi.encodePacked("data:application/json;base64,", Base64.encode(_jsonBlob)));

Refer to Solidity Documents.

NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

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) {

The demo of the loop gas comparison can be seen here.

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas.

src/governance/governor/Governor.sol
280:        proposal.againstVotes += uint32(weight);
285:        proposal.forVotes += uint32(weight);
290:        proposal.abstainVotes += uint32(weight);

src/token/Token.sol
88:         if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();
118:        (baseTokenId += schedule) % 100;

src/token/metadata/MetadataRenderer.sol
140:        _propertyId += numStoredProperties;

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

EXPRESSIONS FOR CONSTANT VALUES SUCH AS A CALL TO keccak256(),` SHOULD USE IMMUTABLE RATHER THAN CONSTANT

Constant expressions are left as expressions, not constants.

When a constant declared as:

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

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

It is expected that the value should be converted into a constant value at compile time. But the expression is re-calculated each time the constant is referenced.

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 )

Suggestion: Change these expressions from constant to immutable and implement the calculation in the constructor or hardcode these values in the constants and add a comment to say how the value was calculated.

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

Use bytes32 instead of string

Use bytes32 instead of string to save gas whenever possible. String is a dynamic data structure and therefore is more gas consuming then bytes32.

Bytes32 can be used instead of string in the following:

src/token/metadata/types/MetadataRendererTypesV1.sol
    struct ItemParam {
        uint256 propertyId;
        string name;
        bool isNewProperty;
    }

    struct IPFSGroup {
        string baseUri;
        string extension;
    }

    struct Item {
        uint16 referenceSlot;
        string name;
    }

    struct Property {
        string name;
        Item[] items;
    }

    struct Settings {
        address token;
        address treasury;
        string name;
        string description;
        string contractImage;
        string rendererBase;
    }

USE CALLDATA INSTEAD OF MEMORY

When arguments are read-only on external functions, the data location can be calldata.

The demo of the gas comparison can be seen here.

src/governance/governor/Governor.sol
116-121:
    function propose(
        address[] memory _targets,
        uint256[] memory _values,
        bytes[] memory _calldatas,
        string memory _description
    ) external returns (bytes32) {

192-196:
    function castVoteWithReason(
        bytes32 _proposalId,
        uint256 _support,
        string memory _reason
    ) external returns (uint256) {

FUNCTIONS ONLY CALLED OUTSIDE OF THE CONTRACT CAN BE DECLARED AS EXTERNAL

External call cost is less expensive than of public functions.

The difference is because in public functions, Solidity immediately copies array arguments to memory, while external functions can read directly from calldata. Memory allocation is expensive, whereas reading from calldata is cheap.

src/token/metadata/MetadataRenderer.sol
206:    function getAttributes(uint256 _tokenId) public view returns (bytes memory aryAttributes, bytes memory queryString) {

MULTIPLE ADDRESS MAPPINGS CAN BE COMBINED INTO A SINGLE MAPPING OF AN ADDRESS TO A STRUCT WHERE APPROPRIATE

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot.

src/governance/governor/storage/GovernorStorageV1.sol
14-19:
    /// @dev Proposal Id => Proposal
    mapping(bytes32 => Proposal) internal proposals;

    /// @dev Proposal Id => User => Has Voted
    mapping(bytes32 => mapping(address => bool)) internal hasVoted;

src/lib/token/ERC721.sol
24-34:
    /// @dev ERC-721 token id => Owner
    mapping(uint256 => address) internal owners;

    /// @dev ERC-721 token id => Manager
    mapping(uint256 => address) internal tokenApprovals;

28-38:
    /// @dev Owner => Balance
    mapping(address => uint256) internal balances;

    /// @dev Owner => Operator => Approved
    mapping(address => mapping(address => bool)) internal operatorApprovals;

src/lib/token/ERC721Votes.sol
28-37:
    /// @notice Account => Delegate
    mapping(address => address) internal delegation;

    /// @dev Account => Num Checkpoints
    mapping(address => uint256) internal numCheckpoints;

    /// @dev Account => Checkpoint Id => Checkpoint
    mapping(address => mapping(uint256 => Checkpoint)) internal checkpoints;

Use bit shift instead of power operation or * 2, / 2

`` seem to be frequently called functions.

src/lib/token/ERC721Votes.sol

        middle = high - (high - low) / 2;

Suggestion: Change the above to

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

The demo of the gas comparison can be seen here.