Forgotten Runiverse - Versus contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/00a247e70de35d7a96d0ce03d66c0206b62e2f65/contracts/RuniverseLandMinter.sol#L358 https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/ea5fce62baeabf3f9d067ad747cee521d7be3a8a/contracts/RuniverseLand.sol#L195 https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/00a247e70de35d7a96d0ce03d66c0206b62e2f65/contracts/RuniverseLandMinter.sol#L513

Vulnerability details

Impact & Proof Of Concepts / Implications

Some privileged functions are often unavoidable in smart contracts. However, in these contracts, the privileges are (unnecessarily) very extensive and without checks on the smart contract side:

1. He can use ownerMint (or ownerMintUsingTokenId) to mint an arbitrary number of tokens. While these functions should be used for private minting, there is nothing restricting the owner from minting more than 10,924 plots and using the function later on to mint additional plots.
2. He can use setVestingStart / setVestingEnd / setLastVestingGlobalId to change the vesting configuration at any point. This can cause already vested tokens to suddenly become unvested (and therefore untransferable). Furthermore, there is no restriction on the parameters, so an owner could for instance set a vesting end that is 100 years in the future.
3. He can change the plots that are available per size at any time with setPlotsAvailablePerSize. Therefore, a user might think that he buys a rare plot size, but the plot size becomes very common afterwards, destroying the value of his NFT.

Therefore, the user currently has to trust the owner that he does not perform any of the previously described actions.

Recommended Mitigation Steps

1. Only allow the owner to mint up to 10,924 plots and only allow it before the other phases have started (mintlist / claimlist / public sale).
2. Remove these functions, these should be immutable parameters such that a user can be sure that his vesting date never changes.
3. Remove this function, this should be immutable such that the rarities of NFTs cannot be changed arbitarily.

Lines of code

https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/00a247e70de35d7a96d0ce03d66c0206b62e2f65/contracts/RuniverseLandMinter.sol#L392

Vulnerability details

Impact

With the function ownerMintUsingTokenId, it is possible for the owner to mint a token with an arbitrary token ID. However, this can brick the whole contract and cause a situation where no more mints / buys are possible. This happens when a token ID is minted with that function that is later on also generated with ownerGetNextTokenId. In that case, the call to runiverseLand.mintTokenId will fail because the function calls _mint internally, which reverts when the token ID was already minted:

function _mint(address to, uint256 tokenId) internal virtual {
        require(to != address(0), "ERC721: mint to the zero address");
        require(!_exists(tokenId), "ERC721: token already minted");
				...
}

Another problem with this function is that the owner can encode an arbitrary plotSize in this tokenId (e.g., also the ones with ID > 4 that are defined in IRuniverseLand, but are not for sale).

Proof Of Concept

Let's say we have plotsMinted[0] = 101 and plotsMinted[1] = plotsMinted[2] = plotsMinted[3] = plotsMinted[4] = plotGlobalOffset = 0. The owner uses the function to mint the token ID which corresponds to the token encoding [102][102][0] (112150186059264). He might not even be aware of that because it is not immediately visible in the decimal representation that 112150186059264 corresponds to [102][102][0]. This mint increases plotsMinted[0] to 102. When a user now tries to mint for plot size ID 0, the function _mintTokens calls ownerGetNextTokenId(0), which will return [102][102][0] = 112150186059264. This will cause the mint to fail because this ID was already minted.

Recommended Mitigation Steps

Remove the function ownerMintUsingTokenId or implement checks that the provided token ID wil not collide with a future token ID (by decoding it and checking that the provided globalCounter / localCounter are impossible).

Lines of code

https://github.com/code-423n4/2022-12-forgotten-runiverse/blob/00a247e70de35d7a96d0ce03d66c0206b62e2f65/contracts/RuniverseLandMinter.sol#L513

Vulnerability details

Impact

The function setPlotsAvailablePerSize can be used for two things:

1. Decreasing the number of plots that is available for a certain size
2. Increase the number of plots that is available for a certain size

However, in both cases it can introduce errors that can brick parts of the contract. In case 1 where the number of plots is decreased, it is possible that the new number is smaller than plotsMinted for a specific ID. This will cause an underflow in getAvailableLands, which subtracts these values:

        plotsAvailableBySize[0] = plotsAvailablePerSize[0] - plotsMinted[0];
        plotsAvailableBySize[1] = plotsAvailablePerSize[1] - plotsMinted[1];
        plotsAvailableBySize[2] = plotsAvailablePerSize[2] - plotsMinted[2];
        plotsAvailableBySize[3] = plotsAvailablePerSize[3] - plotsMinted[3];
        plotsAvailableBySize[4] = plotsAvailablePerSize[4] - plotsMinted[4];

On the other hand, when the number of available plots per size is increased, the minting can fail. This can happen because the MAX_SUPPLY in RuniverseLand is set to 70000, which is also the sum of all plotsAvailablePerSize entries. When the sum of these entries is increased beyond 70000, some tokens cannot be minted, because the MAX_SUPPLY is already reached.

Proof Of Concept

plotsAvailablePerSize[0] is changed by the owner to 54500, all other entries are kept the same. Because more users prefer cheap plots, they buy all 54500 plots of size 8x8 and 15500 plots of size 16x16. However, this means that 70000 tokens are minted and no one can buy the 32x32 or 64x64, leading to a substantial financial loss for the protocol (because larger investors might have bought them later, but can no longer do so).

Recommended Mitigation Steps

Enforce that

1. All entries are larger than plotsMinted
2. The new entries sum up to 70000 (or to a number that is <= 70000 if decreasing the max supply should be allowed)

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

MAX_SUPPLY of RuniverseLand could be reached before RuniverseLandMinter mints all tokens because of secondary minter RuniverseLand has a MAX_SUPPLY of 70000, which is also the sum of all plotsAvailablePerSize within RuniverseLandMinter. However, because there is also an (arbitrary) secondaryMinter in RuniverseLand, it can happen that this secondary minter mints some tokens. Then, RuniverseLandMinter will not be able to mint / sell some tokens because the limit was already reached.

mintTokenId uses _mint instead of _safeMint

RuniverseLand.mintTokenId uses _mint instead of _safeMint to mint NFTs. Therefore, no callback will be performed for smart contracts and it is not checked if a smart contract can handle NFTs. This should not be a huge security problem, because this function is usually called when a user / contract explicitly requests that a token is minted (and pays for it). It would be very stupid (and a user error) to do this from a contract where the NFT is afterwards not retrievable. However, the private sale minting is a bit different because this is done by the owner to a list of given addresses. I do not know how this list is created, but there it could potentially happen that a user accidentally provided a smart contract (e.g., a smart contract wallet) and did not think about this. So you might want to consider using _safeMint instead.

MAX_SUPPLY of RuniverseLand could be reached before RuniverseLandMinter mints all tokens because of secondary minter

RuniverseLand has a MAX_SUPPLY of 70000, which is also the sum of all plotsAvailablePerSize within RuniverseLandMinter. However, because there is also an (arbitrary) secondaryMinter in RuniverseLand, it can happen that this secondary minter mints some tokens. Then, RuniverseLandMinter will not be able to mint / sell some tokens because the limit was already reached.

Unnecessary check in RuniverseLand.forwardERC20s

The check address(msg.sender) != address(0) in forwardERC20s is not necessary because this function is only callble by the owner and msg.sender cannot be address(0) (or it could be in theory, but no one has the private key for address(0), or we would have huge problems).

Runiverse.withdrawAll uses send instead of call to send ETH

The function withdrawAll uses send for sending ETH. This function only provides a 2300 gas stipend, which might not be sufficient. However, in this context this is not very severe in my opinion because the function is only used for recovery purposes and if the owner is currently a recipient that uses more than 2300 gas in its receive function, it could be temporarily changed to an EOA, so no funds are lost.

Runiverse.forwardERC20s does not check the transfer return value

Because Runiverse.forwardERC20s does not check the return value of transfer, some ERC20 transfers that fail (when the token itself does not revert) will not cause the whole transaction to fail. Because this is only a recovery function, this is not very bad, but for usability / to recognize failed transfers (e.g., in a blockchain explorer), reverting in these cases might still be sensible.

Unused constant in RuniverseLand

The constant string R in RuniverseLand is used nowhere and can be removed.

Rounding Error in ERC721Vestable._beforeTokenTransfer

Because of the calculation in _beforeTokenTransfer which first divides and then multiplies, it can happen that all tokens are fully vested 10924 seconds (~3 hours) before the configured end, which may be undesirable. Consider rounding up in the first division.

setGlobalIdOffset / setLocalIdOffsets callable during claimlist phase

The functions setGlobalIdOffset & setLocalIdOffsets include the check

require(!mintlistStarted(), "Can't change during mint");

However, this means that they are potentially callable during the claimlist phase (namely when the claimlist begins before the mintlist, which is quite probable in my opinion). To ensure consecutive counters (and no accidental collisions), this should not be possible, so consider requiring that both the mintlist and the claimlist has not started.

IRuniverseLand.PlotSize contains non-mintable sizes

The enum PlotSize als contains entries for 256x256, ..., 4096x4096 plots, although they are not mintable using these contracts. Consider removing them in order to not confuse developers that built on top of the project and use this interface for the integration.

RuniverseLandMinter.mintlisted has unnecessary argument _leaf

The argument _leaf is only used to compare it with the hashed _who argument. This check does not add any benefit and requires all clients (which will often be some off-chain client for this view function) to also calculate this hash unnecessarily.

Unnecessary check in RuniverseLand._mintTokens

The first check in _mintTokens (plotsMinted[uint256(plotSize)] < plotsAvailablePerSize[uint256(plotSize)]) is unnecessary and could be removed. The second check (plotsMinted[uint256(plotSize)] + numPlots <= plotsAvailablePerSize[uint256(plotSize)]) implies the first one, so there is no need to perform the first one (except for the different error message, but I do not think that this is worth the gas).