Nouns Builder contest - Jeiwan's results

Lines of code

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

Vulnerability details

Impact

Minting becomes impossible when the number of properties reaches 16. Since there's no way to remove properties, a new patched MetadataRenderer contract needs to be deployed to continue minting.

Proof of Concept

Consider the following test case:

function testRevert_MintingWhenTooManyParameters() public {
    // Deploying a token and a metadata renderer
    address tknImpl = address(new Token(address(this)));
    Token tkn = Token(address(new ERC1967Proxy(tknImpl, "")));

    address rndrImpl = address(new MetadataRenderer(address(this)));
    MetadataRenderer rndr = MetadataRenderer(address(new ERC1967Proxy(rndrImpl, "")));

    bytes memory initString = abi.encode("Test", "Test", "Test", "Test", "Test");
    IManager.FounderParams[] memory founders = new IManager.FounderParams[](1);
    founders[0] = IManager.FounderParams({ wallet: address(this), ownershipPct: 1, vestExpiry: 4 weeks });

    tkn.initialize(founders, initString, address(rndr), address(this));
    rndr.initialize(initString, address(tkn), address(this), address(this));

    // Exploit starts here:

    // Creating 15 properties with items and adding them to the token.
    string[] memory names = new string[](15);
    MetadataRendererTypesV1.ItemParam[] memory items = new MetadataRendererTypesV1.ItemParam[](15);
    MetadataRendererTypesV1.IPFSGroup memory ipfsGroup = MetadataRendererTypesV1.IPFSGroup({ baseUri: "base", extension: "ext" });

    for (uint256 i; i < 15; i++) {
        names[i] = string.concat("prop_", Strings.toString(i));
        items[i] = MetadataRendererTypesV1.ItemParam({ propertyId: i, name: string.concat("item_", Strings.toString(i)), isNewProperty: true });
    }

    rndr.addProperties(names, items, ipfsGroup);
    assertEq(rndr.propertiesCount(), 15);

    // Minting a token with 15 properties. Success.
    tkn.mint();
    assertEq(tkn.totalSupply(), 2); // 1 for the founder, 1 for the auction

    // Adding 16th property.
    names = new string[](1);
    names[0] = "uh-oh";
    items = new MetadataRendererTypesV1.ItemParam[](1);
    items[0] = MetadataRendererTypesV1.ItemParam({ propertyId: 0, name: "uh-oh", isNewProperty: true });

    rndr.addProperties(names, items, ipfsGroup);
    assertEq(rndr.propertiesCount(), 16);

    // Minting a token with 16 properties and getting an error.
    vm.expectRevert(stdError.indexOOBError);
    tkn.mint();
}

As you can see, adding 16th property causes the mint function to revert with an "Index out of bounds" error. This happens at this line if the number of properties is greater than the number of token attributes. The latter is a fixed array with length 16. Since the index 0 is used to store the current number of properties, the actual number of attributes that a token can have is 15. Thus, the "Index out of bounds" error happens when this number overflows.

Recommended Mitigation Steps

Set an upper bound on the number of properties in the addProperties function of MetadataRenderer or allow an unbounded number of attributes in a token.

Lines of code

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

Vulnerability details

Impact

If a property without items was added, minting becomes impossible. To enable minting again, an item must be added to the property, which is only possible through a new governance proposal.

Proof of Concept

Consider the following test case:

function testRevert_MintingWithPropertyWithoutItems() public {
    // Deploying a token and a metadata renderer
    address tknImpl = address(new Token(address(this)));
    Token tkn = Token(address(new ERC1967Proxy(tknImpl, "")));

    address rndrImpl = address(new MetadataRenderer(address(this)));
    MetadataRenderer rndr = MetadataRenderer(address(new ERC1967Proxy(rndrImpl, "")));

    bytes memory initString = abi.encode("Test", "Test", "Test", "Test", "Test");
    IManager.FounderParams[] memory founders = new IManager.FounderParams[](1);
    founders[0] = IManager.FounderParams({ wallet: address(this), ownershipPct: 1, vestExpiry: 4 weeks });

    tkn.initialize(founders, initString, address(rndr), address(this));
    rndr.initialize(initString, address(tkn), address(this), address(this));

    // Exploit starts here:

    // Creating a property without items and adding it to the token.
    string[] memory names = new string[](1);
    MetadataRendererTypesV1.ItemParam[] memory items = new MetadataRendererTypesV1.ItemParam[](0);
    MetadataRendererTypesV1.IPFSGroup memory ipfsGroup = MetadataRendererTypesV1.IPFSGroup({ baseUri: "base", extension: "ext" });

    names[0] = "uh-oh";
    // Founder forgot to add items.

    rndr.addProperties(names, items, ipfsGroup);
    assertEq(rndr.propertiesCount(), 1);

    // Minting a token with a property without items and getting an error.
    vm.expectRevert(stdError.divisionError);
    tkn.mint();

    // Adding an item to the property. At this point, this can only be done through a governance proposal.
    names = new string[](0);
    items = new MetadataRendererTypesV1.ItemParam[](1);
    items[0] = MetadataRendererTypesV1.ItemParam({ propertyId: 0, name: "fixed", isNewProperty: false });
    rndr.addProperties(names, items, ipfsGroup);

    // Success.
    tkn.mint();
    assertEq(tkn.totalSupply(), 2);
}

Recommended Mitigation Steps

Short term, in the addProperties function, ensure that each newly added property has at least one item.

Long term, after adding properties and items in the addProperties function, ensure that next token can be minted and rendered without errors.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L172-L201 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L172

Vulnerability details

Impact

It's possible to mint tokens when properties haven't yet been set in MetadataRenderer. Such tokens won't be possible to render due to this check in the getAttributes function of MetaRenderer contract. There's no way to fix such tokens after they were minted since the number of properties of each token is stored individually, thus a patched MetadataRenderer and a proxy contract need to be deployed.

Proof of Concept

Consider this test case:

function testRevert_MintedWithoutProperties() public {
    // Deploying a token and a metadata renderer
    address tknImpl = address(new Token(address(this)));
    Token tkn = Token(address(new ERC1967Proxy(tknImpl, "")));

    address rndrImpl = address(new MetadataRenderer(address(this)));
    MetadataRenderer rndr = MetadataRenderer(address(new ERC1967Proxy(rndrImpl, "")));

    bytes memory initString = abi.encode("Test", "Test", "Test", "Test", "Test");
    IManager.FounderParams[] memory founders = new IManager.FounderParams[](1);
    founders[0] = IManager.FounderParams({ wallet: address(this), ownershipPct: 1, vestExpiry: 4 weeks });

    tkn.initialize(founders, initString, address(rndr), address(this));
    rndr.initialize(initString, address(tkn), address(this), address(this));

    // Exploit starts here:

    // Properties haven't been added yet...
    assertEq(rndr.propertiesCount(), 0);

    // but minting is still possible.
    tkn.mint();
    assertEq(tkn.totalSupply(), 2); // 1 to the founder, 1 to the auction

    // When trying to render a token without properties, there's a revert.
    vm.expectRevert(abi.encodeWithSignature("TOKEN_NOT_MINTED(uint256)", 0));
    rndr.getAttributes(0);
}

The call to getAttributes() reverts because the token's attributes were not set since properties hadn't been added to the metadata renderer.

Recommended Mitigation Steps

In the onMinted() function of MetadataRenderer, ensure that properties.length is greater than 0.

[L-01] Missing onlyInitializing modifier

Targets:

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

Description

The _addFounders function is only called during initialization (the initialize function). It's recommended to add the onlyInitializing modifier to the function for consistency with the __ReentrancyGuard_init and __ERC721_init functions called in the initialize function. It'll also protect from future contract updates accidentally calling this function outside of initialize.

[L-02] Duplicate dependency for the same functionality

Targets:

• https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L212
• https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L294

Description

There are two dependency contracts that are used for the same functionality:

• Strings from "@openzeppelin/contracts/utils/Strings.sol";
• LibUintToString from "sol2string/contracts/LibUintToString.sol".

They're both used to convert a uint to a string. To ensure that the exact same operation is made in both of the linked cases, it's recommended to use one library,

[L-03] Missing zero-address check

Targets:

• https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/utils/Ownable.sol#L66
• https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/utils/Ownable.sol#L72
• https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L188
• https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L197

[L-04] Avoid shadowing of state variables

Targets:

• https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L82 (_owner)

[N-01] Unused operation result

Targets:

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

Description

The result of the modulus operator is not used.

[N-02] Dead code

Targets:

• https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/utils/Initializable.sol#L55
• https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/utils/Initializable.sol#L74

Description

It's recommended to remove unused code to reduce the cost of contract deployment.

[G-01] Next available founder token ID can be computed in memory

Targets:

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

Description

When finding next available token ID for a founder, there's no need to read the tokenRecipient state variable. Since founder token IDs are scattered evenly and shifted by 1, baseTokenId is simply the index of a founder. Here's how the Token contract can be patched:

diff --git a/src/token/Token.sol b/src/token/Token.sol
index afad142..7c75505 100644
--- a/src/token/Token.sol
+++ b/src/token/Token.sol
@@ -102,13 +102,10 @@ contract Token is IToken, UUPS, ReentrancyGuard, ERC721Votes, TokenStorageV1 {
                 uint256 schedule = 100 / founderPct;

                 // Used to store the base token id the founder will recieve
-                uint256 baseTokenId;
+                uint256 baseTokenId = i;

                 // For each token to vest:
                 for (uint256 j; j < founderPct; ++j) {
-                    // Get the available token id
-                    baseTokenId = _getNextTokenId(baseTokenId);
-
                     // Store the founder as the recipient
                     tokenRecipient[baseTokenId] = newFounder;

@@ -125,16 +122,6 @@ contract Token is IToken, UUPS, ReentrancyGuard, ERC721Votes, TokenStorageV1 {
         }
     }

-    /// @dev Finds the next available base token id for a founder
-    /// @param _tokenId The ERC-721 token id
-    function _getNextTokenId(uint256 _tokenId) internal view returns (uint256) {
-        unchecked {
-            while (tokenRecipient[_tokenId].wallet != address(0)) ++_tokenId;
-
-            return _tokenId;
-        }
-    }
-
     ///                                                          ///
     ///                             MINT                         ///
     ///                                                          ///

[G-02] Re-use currentProposalThreshold

Targets:

• https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L128

Description

Instead of calculating proposalThreshold() again, re-use the previously calculated value:

diff --git a/src/governance/governor/Governor.sol b/src/governance/governor/Governor.sol
index 0d963c5..b441972 100644
--- a/src/governance/governor/Governor.sol
+++ b/src/governance/governor/Governor.sol
@@ -125,7 +125,7 @@ contract Governor is IGovernor, UUPS, Ownable, EIP712, GovernorStorageV1 {
         // Cannot realistically underflow and `getVotes` would revert
         unchecked {
             // Ensure the caller's voting weight is greater than or equal to the threshold
-            if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
+            if (getVotes(msg.sender, block.timestamp - 1) < currentProposalThreshold) revert BELOW_PROPOSAL_THRESHOLD();
         }

         // Cache the number of targets