Nouns Builder contest - eierina's results

Lines of code

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

Vulnerability details

Impact

If a property is added to the metadata renderer that causes the total properties to reach more than 15 properties, the auctions/DAO will halt and will require an upgrade of the contracts to resolve the issue.

Proof of Concept

Initially MetadataRenderer is owned by a founder address who is responsible for adding the *initial* properties. During the first execution of the addProperties function by the founder address, the ownership is transferred to the treasury. The MetadataRenderer's storage properties, ipfsData, and Property.items are dynamically sized arrays, hence unbounded.

During the onMint function, for each property, one random property item is selected as a token attribute. The token attributes are stored into a statically sized array of 16 elements via a mapping where the first element of the array is reserved to store the number of properties will be used to generate the token attributes. Because of the +1 offset, the maximum number of properties that can be used for the attributes generation is 15. If by any chance, either during founder initial call, or more easily later via proposal, a new property is added via addProperty function upgrading the new total properties to 16 or more, the onMinted function will start to revert due to array access out of bounds during token attributes assignment.

Because of the chain of calls Auction.createAuction Auction._createAuction -> Token.mint -> Token._mint -> MetadataRenderer.onMinted will revert, the DAO will pause and settling and unpausing will not resolve the problem. Only an upgrade of the contracts with a workaround will resolve the problem.

Tools Used

n/a

Recommended Mitigation Steps

There are two possible fixes, see below.

Simple preventative fix, add count check to addProperties function after line 117 and line 140 as follows (see + for insertions)

function addProperties(
    string[] calldata _names,
    ItemParam[] calldata _items,
    IPFSGroup calldata _ipfsGroup
) external onlyOwner {
    // Cache the existing amount of IPFS data stored
    uint256 dataLength = ipfsData.length;

    // If this is the first time adding properties and/or items:
    if (dataLength == 0) {
        // Transfer ownership to the DAO treasury
        transferOwnership(settings.treasury);
    }

    // Add the IPFS group information
    ipfsData.push(_ipfsGroup);

    // Cache the number of existing properties
    uint256 numStoredProperties = properties.length;

    // Cache the number of new properties
    uint256 numNewProperties = _names.length;

    // Cache the number of new items
    uint256 numNewItems = _items.length;

    unchecked {

+
+    if(numStoredProperties + numNewProperties > 15) revert TOOMANY_PROPERTIES();
+

        // For each new property:
        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]);
        }

        // For each new item:
        for (uint256 i = 0; i < numNewItems; ++i) {
            // Cache the id of the associated property
            uint256 _propertyId = _items[i].propertyId;

            // Offset the id if the item is for a new property
            // Note: Property ids under the hood are offset by 1
            if (_items[i].isNewProperty) {
                _propertyId += numStoredProperties;
+
+               if(_propertyId > 15) revert TOOMANY_PROPERTIES();
+
            }

            // Get the pointer to the other items for the property
            Item[] storage items = properties[_propertyId].items;

            // Append storage space
            items.push();

            // Get the index of the new item
            // Cannot underflow as the items array length is ensured to be at least 1
            uint256 newItemIndex = items.length - 1;

            // Store the new item
            Item storage newItem = items[newItemIndex];

            // Store the new item's name and reference slot
            newItem.name = _items[i].name;
            newItem.referenceSlot = uint16(dataLength);

            emit ItemAdded(_propertyId, newItemIndex);
        }
    }
}

An alternative option would be to make the tokeAttributes a dynamic array in the onMinted function of MetadataRenderer by replacing the fixed size array in mapping(uint256 => uint16[16]) attributes to a dynamic array, and have the tokenAttributes[i + 1] = uint16(seed % numItems); replaced by [tokenAttributes.push(uint16(seed % numItems)); but this would require other changes in few other places, a new seed every 16 properties, and it would be consuming more gas.

Impact

Verified use case with @kulk from the team and "a user can place a bid without attaching any ETH. If that is the only bid then they can win the auction". The current implementation, when reservePrice is zero, allows the next zero bid always win over the previous zero bid.

Proof of Concept

In createBid, the minBid for the next bid is calculated as follows:

minBid = highestBid + ((highestBid * settings.minBidIncrement) / 100);

When a bidder started with a 0 ETH bid, then the next bidder can win the bid with another 0 ETH bid due to the (highestBid * settings.minBidIncrement) resulting in minBid being still zero.

Tools Used

n/a

Recommended Mitigation Steps

A possible solution would be to change from: if (msg.value < minBid) revert MINIMUM_BID_NOT_MET(); to: if (msg.value < minBid || minBid == highestBid) revert MINIMUM_BID_NOT_MET();