getRandomTokenIdFromFund not really random

[code423n4]

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.

[cemozerr]

Duplicate of #78