NFTX contest - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

In NFTXVaultUpgradeable.sol the received ERC1155 NFT's are stored in the array holdings. The number of NFT's of a specific type are kept in quantity1155. Suppose there is one NFT (#1) with 10 instances and 9 NFT's with 1 instance. (#2..#10) When you randomly retrieve a NFT via getRandomTokenIdFromFund it takes a random number from the holdings array, without taking in account the number of instances. So in the example the probability of getting a NFT of type #1 is about 10% While if you take in account all the NFT's + instances then the change should be more than 50%.

Proof of Concept

function getRandomTokenIdFromFund() internal virtual returns (uint256) {
    uint256 randomIndex = getPseudoRand(holdings.length());
    return holdings.at(randomIndex);
}

Tools Used

Editor

Recommended Mitigation Steps

If the unfairness is considered problematic then the code of getRandomTokenIdFromFund should be enhanced.

Handle

gpersoon

Vulnerability details

Impact

The function swapTo of NFTXVaultUpgradeable.sol is kind of a combination of mintTo and redeemTo (the code looks very similar to a combination of mintTo and redeemTo). Before receiveNFTs I would expect a call to allValidNFTs, like in mintTo. This is to make sure only eligible NFTs are transferred. Without this check any NFT could be transferred.

After withdrawNFTsTo I would expect a call to afterRedeemHook, like in redeemTo. The afterRedeemHook fixes the administration for the eligability. Without this the eligibility administrations would be flawed.

This way swapTo would circumvent the checks of mintTo and redeemTo and would allow any NFT to be transferred (even the one's that are not eligible)

Proof of Concept

NFTXVaultUpgradeable.sol

function mintTo(..) ... require(allValidNFTs(tokenIds), "NFTXVault: not eligible"); uint256 count = receiveNFTs(tokenIds, amounts);

function redeemTo(uint256 amount, uint256[] memory specificIds, address to) .... uint256[] memory redeemedIds = withdrawNFTsTo(amount, specificIds, to); afterRedeemHook(redeemedIds);

function swapTo(..) ... uint256 count = receiveNFTs(tokenIds, amounts); ... uint256[] memory ids = withdrawNFTsTo(count, specificIds, to);

Tools Used

Editor

Recommended Mitigation Steps

Check if it necessary to also add the following statements in swapTo:

• require(allValidNFTs(tokenIds), "NFTXVault: not eligible");
• afterRedeemHook(redeemedIds);

Handle

gpersoon

Vulnerability details

Impact

The function getRandomTokenIdFromFund of NFTXVaultUpgradeable.sol is not really random, as noted in the name of the function getPseudoRand. The value of the blockhash(block.number - 1) is fully determined for al the transactions in the same block. The result is that the retrieval of NFS's (via redeemTo, swapTo, withdrawNFTsTo, while enableDirectRedeem==false) can be manipulated. You can make a smart contract that tries to call redeemTo or swapTo, checks the resulting NFTs and reverts if it has NFTs that are unwanted.

Proof of Concept

function getPseudoRand(uint256 modulus) internal virtual returns (uint256) {
    randNonce += 1;
    return
        uint256(
            keccak256(
                abi.encodePacked(blockhash(block.number - 1), randNonce)
            )
        ) %
        modulus;
}

Tools Used

Editor

Recommended Mitigation Steps

If the value or desirability of the NFT's are different then it's important to use other ways to randomize, for example via a random oracle or a commit/reveal schema.