function onMinted(uint256 _tokenId) external returns (bool) {
// Ensure the caller is the token contract
if (msg.sender != settings.token) revert ONLY_TOKEN();
// Compute some randomness for the token id
uint256 seed = _generateSeed(_tokenId);
// Get the pointer to store generated attributes
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);
So, an NFT can have up to 15 properties set.
However, the number of properties addProperties() introduces isn't controlled anyhow, it is possible to add an arbitrary large numNewProperties:
// 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 {
// 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]);
}
Once numStoredProperties + numNewProperties > 15, all the token mints will be failing:
function onMinted(uint256 _tokenId) external returns (bool) {
// Ensure the caller is the token contract
if (msg.sender != settings.token) revert ONLY_TOKEN();
// Compute some randomness for the token id
uint256 seed = _generateSeed(_tokenId);
// Get the pointer to store generated attributes
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);
function _mint(address _to, uint256 _tokenId) internal override {
// Mint the token
super._mint(_to, _tokenId);
// Generate the token attributes
if (!settings.metadataRenderer.onMinted(_tokenId)) revert NO_METADATA_GENERATED();
function _createAuction() private {
// Get the next token available for bidding
try token.mint() returns (uint256 tokenId) {
...
} catch Error(string memory) {
_pause();
}
But there will be no fix as there is no way to remove a property, MetadataRenderer have only property addition functionality. I.e. the auctioning system will become permanently frozen.
Recommended Mitigation Steps
To align property management functionality with the storage consider introducing the same limit to addProperties():
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;
+ if (numStoredProperties + numNewProperties > 15) revert PROPERTY_LIMIT_BREACHED();
Lines of code
https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L108-L130
Vulnerability details
The total number of properties is now limited to be 15 or less with hard code on the storage structures level.
In the same time it is possible to add unlimited number of properties with MetadataRenderer's addProperties().
If this happens, with a malicious intent or as the result of an operational mistake, the whole auctioning will become permanently frozen.
Proof of Concept
The
attributes
mapping has fixed length of16
for each token's array of attributes:https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/storage/MetadataRendererStorageV1.sol#L19-L20
First element,
tokenAttributes[0]
, is dedicated to the currently existing properties count:https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L171-L185
So, an NFT can have up to 15 properties set.
However, the number of properties addProperties() introduces isn't controlled anyhow, it is possible to add an arbitrary large
numNewProperties
:https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L108-L130
Once
numStoredProperties + numNewProperties > 15
, all the token mints will be failing:https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L171-L194
https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L167-L172
The Auction pauses as a result:
https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L204-L236
But there will be no fix as there is no way to remove a property, MetadataRenderer have only property addition functionality. I.e. the auctioning system will become permanently frozen.
Recommended Mitigation Steps
To align property management functionality with the storage consider introducing the same limit to addProperties():
https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/metadata/MetadataRenderer.sol#L91-L112