Forgotten Runiverse - Versus contest - hansfriese's results

Judge has assessed an item in Issue #22 as M risk. The relevant finding follows:

[L-02] RuniverseLandMinter.ownerMintUsingTokenId() doesn't check if tokenId and plotSize are matched. https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/dcad1802bf258bf294900a08a03ca0d26d2304f4/contracts/RuniverseLandMinter.sol#L387-L393 function ownerMintUsingTokenId( IRuniverseLand.PlotSize plotSize, uint256 tokenId, address recipient ) public onlyOwner { _mintTokensUsingTokenId(plotSize, tokenId, recipient); } When admin adds a custom tokenId, it doesn't check if the id and plotSize are matched. If they aren't matched, plotsMinted will be counted wrongly.

Even if it's an admin function, it would be good to validate for safety.

Lines of code

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

Vulnerability details

Impact

The function mint() of RuniverseLand will not work and seemingly unnecessary

Proof of Concept

RuniverseLand.sol has a public function mint() that can be used to mint a new plot. Note that this function uses numMinted as a new token ID while numMinted denotes the number of tokens minted so far.

RuniverseLand.sol
72:     function mint(address recipient, PlotSize size)
73:         public
74:         override
75:         returns (uint256)
76:     {
77:         uint256 tokenId = numMinted;
78:         mintTokenId(recipient, tokenId, size);
79:         return tokenId;
80:     }

On the other hand, RuniverseLandMinter.sol uses a function mintTokenId() where it specifies the new token ID. Note that the new token ID is from ownerGetNextTokenId() and it starts from 1 because plotGlobalOffset is initialized by 1. So once private tokens are minted, the number of tokens are equal to the LAST minted token's global ID. As a result, the function mint() of RuniverseLand will always revert because there already exists a token.

It is difficult to analyze the impact because I could not find the usage of RuniverseLand.mint() but I believe this is a wrong implementation and future usecases will be affected.

RuniverseLandMinter.sol
323:     function _mintTokens(
324:         IRuniverseLand.PlotSize plotSize,
325:         uint256 numPlots,
326:         address recipient
327:     ) private {
328:         require(
329:             plotsMinted[uint256(plotSize)] <
330:                 plotsAvailablePerSize[uint256(plotSize)],
331:             "All plots of that size minted"
332:         );
333:         require(
334:             plotsMinted[uint256(plotSize)] + numPlots <=
335:                 plotsAvailablePerSize[uint256(plotSize)],
336:             "Trying to mint too many plots"
337:         );
338:         for (uint256 i = 0; i < numPlots; i++) {
339:
340:             uint256 tokenId = ownerGetNextTokenId(plotSize);
341:             plotsMinted[uint256(plotSize)] += 1;
342:
343:             runiverseLand.mintTokenId(recipient, tokenId, plotSize);
344:         }
345:     }
...
RuniverseLandMinter.sol
399:     function ownerGetNextTokenId(IRuniverseLand.PlotSize plotSize) private view returns (uint256) {
400:         uint256 globalCounter = plotsMinted[0] + plotsMinted[1] + plotsMinted[2] + plotsMinted[3] + plotsMinted[4] + plotGlobalOffset;
401:         uint256 localCounter  = plotsMinted[uint256(plotSize)] + plotSizeLocalOffset[uint256(plotSize)];
402:         require( localCounter <= 4294967295, "Local index overflow" );//1<<32
403:         require( uint256(plotSize) <= 255, "Plot index overflow" );
404:
405:         return (globalCounter<<40) + (localCounter<<8) + uint256(plotSize);//@audit-info always greater than 0
406:     }
407:

Tools Used

Manual Review

Recommended Mitigation Steps

If the function mint() is supposed to be used by any means, change it to use the numMinted+1 instead of numMinted. Keep in mind that with this change the token ID will start from 1.

Judge has assessed an item in Issue #22 as M risk. The relevant finding follows:

[L-01] ERC721Vestable._setVestingStart() should check _newVestingStart<vestingEnd. https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/dcad1802bf258bf294900a08a03ca0d26d2304f4/contracts/ERC721Vestable.sol#L100 function _setVestingStart(uint256 _newVestingStart) internal virtual { //@audit require(vestingEnd > _newVestingStart, "End must be greater than start"); vestingStart = _newVestingStart; }

[L-01] ERC721Vestable._setVestingStart() should check _newVestingStart<vestingEnd.

• https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/dcad1802bf258bf294900a08a03ca0d26d2304f4/contracts/ERC721Vestable.sol#L100

    function _setVestingStart(uint256 _newVestingStart) internal virtual {
        //@audit
        require(vestingEnd > _newVestingStart, "End must be greater than start");
        vestingStart = _newVestingStart;
    }

[L-02] RuniverseLandMinter.ownerMintUsingTokenId() doesn't check if tokenId and plotSize are matched.

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

    function ownerMintUsingTokenId(
        IRuniverseLand.PlotSize plotSize,
        uint256 tokenId,
        address recipient
    ) public onlyOwner {
        _mintTokensUsingTokenId(plotSize, tokenId, recipient);
    }

When admin adds a custom tokenId, it doesn't check if the id and plotSize are matched. If they aren't matched, plotsMinted will be counted wrongly.

Even if it's an admin function, it would be good to validate for safety.

[N-01] Typo

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

"@return getPlotPrices uint256 plot type" => "@return getTokenIdPlotType uint256 plot type"

• https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/dcad1802bf258bf294900a08a03ca0d26d2304f4/contracts/ERC721Vestable.sol#L54

"besting property" => "vesting property"

[N-02] Missing checks for address(0)

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

The new values should be checked to prevent zero address.

[N-03] Time values are not checked

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

    uint256 public publicMintStartTime = type(uint256).max;
    uint256 public mintlistStartTime = type(uint256).max;
    uint256 public claimsStartTime = type(uint256).max;

The above time values are used to decide the current minting phase. Obviously these are supposed to be in specific orders, e.g. claimsStartTime<mintlistStartTime<publicMintStartTime. But it is not checked on the setter functions.

    function setPublicMintStartTime(uint256 _newPublicMintStartTime)
        public
        onlyOwner
    {
        publicMintStartTime = _newPublicMintStartTime;
    }

    /**
     * @dev Assigns a new mintlist start minting time.
     * @param _newAllowlistMintStartTime uint256 echo time in seconds.
     */
    function setMintlistStartTime(uint256 _newAllowlistMintStartTime)
        public
        onlyOwner
    {
        mintlistStartTime = _newAllowlistMintStartTime;
    }

    /**
     * @dev Assigns a new claimlist start minting time.
     * @param _newClaimsStartTime uint256 echo time in seconds.
     */
    function setClaimsStartTime(uint256 _newClaimsStartTime) public onlyOwner {
        claimsStartTime = _newClaimsStartTime;
    }