code-423n4 / 2022-09-nouns-builder-findings

10 stars 6 forks source link

# `MetadataRenderer` `onMinted()` function use of weak prng for token properties can be exploited for profitable NFTs #688

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/Token.sol#L43-L67 https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/Token.sol#L143-L162 builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/metadata/MetadataRenderer.sol#L194 builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/token/metadata/MetadataRenderer.sol#L249-L252

Vulnerability details

MetadataRenderer onMinted() function use of weak prng for token properties can be exploited for profitable NFTs

Impact

block.number, block.coinbase, block.timestamp are known parameters by anybody or even manipulable by miners.

Contract is using: return uint256(keccak256(abi.encode(_tokenId, blockhash(block.number), block.coinbase, block.timestamp)));

This can be exploited by wrongly assigning settings.auction or by malicious founders to create desired tokens.

Proof of concept

Token.sol initializer doesn't check _auction: settings.auction = _auction; //@audit no check, no double step setter, it can be any kind of address by mistake or by a malicious caller

On Token.sol _mint function is called in:

    function mint() external nonReentrant returns (uint256 tokenId) {
        // Cache the auction address
        address minter = settings.auction;//@audit only settings.auction doesn't revert, but this address can be anyone

        // Ensure the caller is the auction
        if (msg.sender != minter) revert ONLY_AUCTION();

        {...not important for the issue}

        // Mint the next available token to the auction house for bidding
        _mint(minter, tokenId); //@audit calls _mint() with desired tokenId, this will be used in `OnMinted(uint256 tokenId)`
    }

_tokenId can be known by reading the contract storage. This means in case of wrongly assigned settings.auction or malicious founder, onMinted function can be called with desired _tokenId by frontrunning other users or waiting until the _tokenId desired value that gets the rare NFT.

All of this is finally applied in onMinted():

function onMinted(uint256 _tokenId) external returns (bool) {

what includes: uint256 seed = _generateSeed(_tokenId); //@audit pseudo-random seed

and finally: tokenAttributes[i + 1] = uint16(seed % numItems)

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

As all of this values are known/manipulable, settings.auction address or founder can exploit this by minting desired rare NFT, which value would be higher than regular ones.

Github Permalink

Recommended Mitigation Steps

1 - Don't use pseudo randomness for NFT properties, as the value of this NFT resides in how rare are the properties, instead use another source of randomness as Oracles (for example: Chainlink VRF (Verifiable Random Function))

Another known proposals for randomness are:

2 - Carefully set the settings.auction or use a double step assignment for safety

eugenioclrc commented 1 year ago

duplicate #151

GalloDaSballo commented 1 year ago

Dup of https://github.com/code-423n4/2022-09-nouns-builder-findings/issues/417

GalloDaSballo commented 1 year ago

L