Forgotten Runiverse - Versus contest - cccz's results

Lines of code

https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/dcad1802bf258bf294900a08a03ca0d26d2304f4/contracts/RuniverseLandMinter.sol#L362-L393

Vulnerability details

Impact

The first eight digits of the RuniverseLand TokenID indicate the corresponding plotSize of the NFT owner can call RuniverseLandMinter.ownerMintUsingTokenId directly to mint the NFT for a specific TokenID In RuniverseLandMinter._mintTokensUsingTokenId, there is no verification that the first eight bits of the tokenId match the plotSize parameter, which allows the owner to bypass the plotsAvailablePerSize limit.

    function _mintTokensUsingTokenId(
        IRuniverseLand.PlotSize plotSize,
        uint256 tokenId,
        address recipient
    ) private {
        uint256 numPlots = 1;
        require(
            plotsMinted[uint256(plotSize)] <
                plotsAvailablePerSize[uint256(plotSize)],
            "All plots of that size minted"
        );
        require(
            plotsMinted[uint256(plotSize)] + numPlots <=
                plotsAvailablePerSize[uint256(plotSize)],
            "Trying to mint too many plots"
        );

        plotsMinted[uint256(plotSize)] += 1;


        runiverseLand.mintTokenId(recipient, tokenId, plotSize);
    }

For example, the plotSize parameter provided by the owner when calling ownerMintUsingTokenId is 8 * 8, while the plotSize contained in the tokenId is 128 * 128, thus bypassing the plotsAvailablePerSize limit

Also, once RuniverseLands with mismatched tokenId and plotSize are minted, the supply of RuniverseLands with different plotSize will no longer be correct because the plotsMinted variable is incorrectly calculated

Proof of Concept

https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/dcad1802bf258bf294900a08a03ca0d26d2304f4/contracts/RuniverseLandMinter.sol#L362-L393

Tools Used

None

Recommended Mitigation Steps

Consider verifying in RuniverseLandMinter._mintTokensUsingTokenId that the first eight bits of the tokenId match the plotSize parameter

Lines of code

https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/dcad1802bf258bf294900a08a03ca0d26d2304f4/contracts/RuniverseLand.sol#L88-L102

Vulnerability details

Impact

RuniverseLand allows primaryMinter and secondaryMinter to mint NFT.

    function mintTokenId(
        address recipient,
        uint256 tokenId,
        PlotSize size
    ) public override nonReentrant {
        require(numMinted < MAX_SUPPLY, "All land has been minted");
        require(
            _msgSender() == primaryMinter || _msgSender() == secondaryMinter,
            "Not a minter"
        );
        numMinted += 1;
        emit LandMinted(recipient, tokenId, size);    
        
        _mint(recipient, tokenId);
    }

RuniverseLandMinter, as one of them, will have a limit on the number of NFTs with different PlotSize

    uint256[] public plotsAvailablePerSize = [
        52500, // 8x8
        16828, // 16x16
        560, // 32x32
        105, // 64x64
        7 // 128x128
    ];

This will be checked in _mintTokens

    function _mintTokens(
        IRuniverseLand.PlotSize plotSize,
        uint256 numPlots,
        address recipient
    ) private {        
        require(
            plotsMinted[uint256(plotSize)] <
                plotsAvailablePerSize[uint256(plotSize)],
            "All plots of that size minted"
        );        
        require(
            plotsMinted[uint256(plotSize)] + numPlots <=
                plotsAvailablePerSize[uint256(plotSize)],
            "Trying to mint too many plots"
        );        
        for (uint256 i = 0; i < numPlots; i++) {

            uint256 tokenId = ownerGetNextTokenId(plotSize);            
            plotsMinted[uint256(plotSize)] += 1; 

But the other minter is not limited and can mint RuniverseLand with any tokenID, thus breaking the plotsAvailablePerSize limit

Proof of Concept

https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/dcad1802bf258bf294900a08a03ca0d26d2304f4/contracts/RuniverseLand.sol#L88-L102 https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/dcad1802bf258bf294900a08a03ca0d26d2304f4/contracts/RuniverseLandMinter.sol#L323-L341

Tools Used

None

Recommended Mitigation Steps

Consider making RuniverseLandMinter the only minter for RuniverseLand

Lines of code

https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/dcad1802bf258bf294900a08a03ca0d26d2304f4/contracts/RuniverseLand.sol#L88-L102

Vulnerability details

Impact

In RuniverseLand.mintTokenId, _mint takes in two parameters, recipient and tokenId

_mint(recipient, tokenId);

However, if the recipient is a contract address that does not support ERC721, the NFT can be frozen in the contract.

As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

https://eips.ethereum.org/EIPS/eip-721

As per the documentation of ERC721.sol by OpenZeppelin:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L274-L285

Proof of Concept

https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/dcad1802bf258bf294900a08a03ca0d26d2304f4/contracts/RuniverseLand.sol#L88-L102

Tools Used

None

Recommended Mitigation Steps

use safeMint instead of mint to check received address support for ERC721 implementation