code-423n4 / 2023-10-nextgen-findings

5 stars 3 forks source link

`RandomizerNXT` allows randomness re-rolling and also front-running. #2010

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/RandomizerNXT.sol#L55-L59

Vulnerability details

Description

When a collection uses RandomizerNXT as the randomizer, the process of minting and setting the token hash happens in the same transaction and block, which allows two attacks. First, a user can see the randomness outcome in mempool and front-run his own transaction to revert in _safeMint callback if he doesn't like the token hash result. This means that who has access to the generative art algorithm can test the hashs and re-roll it via reverts if he wants, only paying gas each re-roll, until a hash that generates a art he enjoyed or that looks highly speculative rolls. Secondly, front-running to predict and steal a Token Hash is possible, because randomness is calculated using only _mintIndex as unique value, allowing attacker to get a _mintIndex that leads to same token hash before the front-runned user.

Proof of Concept

  1. User calls mint in a collection that uses RandomizerNXT.
  2. The subcall _mintProcessing executes:
    
        function _mintProcessing(uint256 _mintIndex, address _recipient, string memory _tokenData, uint256 _collectionID, uint256 _saltfun_o) internal {

        tokenData[_mintIndex] = _tokenData;

        collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);

        tokenIdsToCollectionIds[_mintIndex] = _collectionID;

        _safeMint(_recipient, _mintIndex);

    }

3. The first important line in this subcall is a call to `RandomizerNXT.calculateTokenHash`: 
```solidity
    function calculateTokenHash(uint256 _collectionID, uint256 _mintIndex, uint256 _saltfun_o) public {
        require(msg.sender == gencore);
        bytes32 hash = keccak256(abi.encodePacked(_mintIndex, blockhash(block.number - 1), randoms.randomNumber(), randoms.randomWord()));
        gencoreContract.setTokenHash(_collectionID, _mintIndex, hash);
    }
  1. After the token hash is calculated and set, the last line of _mintProcessing is _safeMint(_recipient, _mintIndex);, which triggers a callback in the ERC-721 Receiver:
    
        function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual {

        _mint(to, tokenId);

        require(

            _checkOnERC721Received(address(0), to, tokenId, data),

            "ERC721: transfer to non ERC721Receiver implementer"

        );

    }


5. The callback allows the re-roll exploit because:
- User can see the randomness outcome in the mempool and see which Token Hash the `RandomizerNXT` generated. 
- He tests the token hash in the generative art algorithm and see if he liked or if looks trendy and speculative.
- If he didn't like the token hash, he can front-run his own transaction to make `onERC721Received` revert it:
```solidity
    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4){
        if (gasgrief) {
            revert(); 
        }
    }
  1. So, user reverted the transaction paying only the gas cost, and now can call mint again to test another Token Hash. He will do this until a token hash that generates the art he wants rolls. In the other side, a honest user needs to pay the NFT's full price for each roll.

  2. Additionally to this exploit, user can check in the mempool the token hash that another user generated. If he did the same process of testing and liked the token hash outcome, a front-run to steal that token hash is possible:

    • As you can see in the process of randomness generation, the only unique variable to generate it is _mintIndex:
      
      function randomWord() public view returns (string memory) {
      uint256 randomNum = uint(keccak256(abi.encodePacked(block.prevrandao, blockhash(block.number - 1), block.timestamp))) % 100;
      return getWord(randomNum);
      }

    function randomNumber() public view returns (uint256){ uint256 randomNum = uint(keccak256(abi.encodePacked(block.prevrandao, blockhash(block.number - 1), block.timestamp))) % 1000; return randomNum; }

    function calculateTokenHash(uint256 _collectionID, uint256 _mintIndex, uint256 _saltfun_o) public { require(msg.sender == gencore); bytes32 hash = keccak256(abi.encodePacked(_mintIndex, blockhash(block.number - 1), randoms.randomNumber(), randoms.randomWord())); gencoreContract.setTokenHash(_collectionID, _mintIndex, hash); }

    
    - So, attacker can front-run a transaction to mint a NFT of a specific `_mintIndex`  first than the front-runned transaction. As the other variables to calculate the randomness are `block.number`, `block.timestamp`, `blockhash`, `block.prevrandao`, effectivelly having the same `_mintIndex` in the same block leads to the same randomness and also makes the targeted user to generate a different token hash.
    ## Impact

If collection has a website or app to test the token hashs, user can re-roll paying only the gas cost until he rolls an art that looks highly speculative. This is a completely unfair use of randomness, specially for art which has highly speculative values. Even Chainlink's VRF is against re-rolling (https://docs.chain.link/vrf/v2/security#do-not-re-request-randomness). It's a problem because while a user that is unaware of the attack only has one roll for each NFT, an attacker has infinite chances, which also can be used for profit. If only a set of restricted people can test the token hashs in the generative art algorithm, it's also unfair and the vulnerability will make people don't trust the collection. Addionality, the second exploit presented allows attackers to predict and steal other people hashs via frontruns.

  1. Likelihood: High.
  2. Severity: High. Risk: High

Tools Used

Manual Review

Recommended Mitigation Steps

  1. Make the mint/randomness process a 2-step mechanism. In other words, don't let minting and setting the token hash to happen in the same transaction. This will completely solve the re-roll exploit. Also, the second exploit of front-running _mintIndex to predict and steal a token hash of the mempool will be also solved, because when randomness comes the _mintIndex will be already minted.
  2. The randomness step of the 2-step mechanism must have a block check, because if it happens in the same block as the minting step the exploits will still be possible, just harder.

Assessed type

MEV

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #1255

c4-pre-sort commented 1 year ago

141345 marked the issue as not a duplicate

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #1534

c4-judge commented 11 months ago

alex-ppg marked the issue as unsatisfactory: Invalid