Nouns Builder contest - lucacez's results

MISSING ZERO-ADDRESS CHECK IN THE SETTER FUNCTIONS AND INITILIAZERS

Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly.

Proof of Concept Navigate to the following contracts.

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L39 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L54-L59 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L41 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L57-L64 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L116-L117 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L324-L325 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L32 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L43 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L141-L142 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/manager/Manager.sol#L55-L60 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L30 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L43-L47 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L32 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L45-L49

Recommended Mitigation Steps Consider adding zero-address checks in the discussed constructors: require(newAddr != address(0));.

UPGRADEABLE CONTRACT IS MISSING A __GAP[50] STORAGE VARIABLE TO ALLOW FOR NEW STORAGE VARIABLES IN LATER VERSIONS

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

All upgradeables contracts

Don't Initialize Variables with Default Value

Uninitialized variables are assigned with the types default value. Explicitly initializing a variable with it's default value costs unnecesary gas.

There are 5 instances of this issue:

File: src/governance/treasury/Treasury.sol

167:            for (uint256 i = 0; i < numTargets; ++i) {

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/treasury/Treasury.sol#L162

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

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

COMPARISONS: != IS MORE EFFICIENT THAN > IN REQUIRE (6 GAS LESS)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

For uints the minimum value would be 0 and never a negative value. Since it cannot be a negative value, then the check > 0 is essentially checking that the value is not equal to 0 therefore >0 can be replaced with !=0 which saves gas.

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you’re in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

File: ERC1967Upgrade.sol Line 61

        if (_data.length > 0 || _forceCall) {

File: ERC721Votes.sol Line 203

            if (_from != _to && _amount > 0) {

++X IS MORE EFFICIENT THAN X++(SAVES ~6 GAS)

There are 6 instances of this issue:

File: Governor.sol Line 230

          nonces[_voter]++

File: ERC721Votes.sol Line 162

          nonces[_from]++

File: ERC721Votes.sol Line 207

          uint256 nCheckpoints = numCheckpoints[_from]++;

File: ERC721Votes.sol Line 222

          uint256 nCheckpoints = numCheckpoints[_to]++;

File: Token.sol Line 91

          uint256 founderId = settings.numFounders++;

File: Token.sol Line 154

          tokenId = settings.totalSupply++;

X += Y COSTS MORE GAS THAN X = X + Y FOR STATE VARIABLES

File: Governor.sol Line 280

                proposal.againstVotes += uint32(weight);

File: Governor.sol Line 285

                proposal.forVotes += uint32(weight);

File: Governor.sol Line 290

                proposal.abstainVotes += uint32(weight);

File: Token.sol Line 88

                if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

File: Token.sol Line 118

                    (baseTokenId += schedule) % 100;

File: MetadataRender.sol Line 140

                    _propertyId += numStoredProperties;

EMITTING STORAGE VALUES INSTEAD OF THE MEMORY ONE

Here, the values emitted shouldn’t be read from storage. The existing memory values should be used instead: File: Auction.sol Line 172

        Auction memory _auction = auction;


        // Ensure the auction wasn't already settled
        if (auction.settled) revert AUCTION_SETTLED();

It must be read from the variable created in memory and not from storage

CACHE STORAGE VALUES IN MEMORY TO MINIMIZE SLOADS

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive 100 gas compared to MLOADs/MSTOREs(3gas) Storage value should get cached in memory

File: Auction.sol Lines 244-260

Create a variable in memory to read auction, and not read it twice from storage:

    function unpause() external onlyOwner {
        _unpause();

        Auction memory _auction = auction;

        // If this is the first auction:
        if (_auction.tokenId == 0) {
            // Transfer ownership of the contract to the DAO
            transferOwnership(settings.treasury);


            // Start the first auction
            _createAuction();
        }
        // Else if the contract was paused and the previous auction was settled:
        else if (_auction.settled) {
            // Start the next auction
            _createAuction();
        }
    }

File: Token.sol Lines 177-199

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

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

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

            return true;

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

            return false;
        }
    }