NFTX contest - pauliax's results

Handle

paulius.eth

Vulnerability details

Impact

function flashLoan is vulnerable to overflow/underflow when the fee is not 0. Although currently the fee is set to 0, there is a comment: "By default there is no fee, but this can be changed by overriding {flashFee}" As these contracts are upgradeable, I cannot assume that this fee will always stay 0, thus I want you to be aware of this possible issue. function flashLoan does not use SafeMath when doing the calculations and accepts an arbitrary value for the amount parameter. When the fee is above 0, it is possible to pass such a value for an amount that (amount + fee) will overflow/underflow. For example, if the fee is set to 1 and I invoke a flashLoan with an amount of max_uint, I will be minted max_uint of tokens and will need to return (burn) 0 tokens. Also, there is a function maxFlashLoan which returns the maximum amount of tokens available for a loan, however, this function is never used. I assume the intention was to limit the amount you can borrow but currently it has no effect.

Recommended Mitigation Steps

Either use SafeMath here or make sure to never introduce a fee > 0. Also, make use of maxFlashLoan function.

Handle

paulius.eth

Vulnerability details

Impact

function swap has a nonReentrant modifier but function swapTo doesn't. swapTo is a public function so it can be invoked directly.

Recommended Mitigation Steps

I guess it was meant to be the opposite as swap just invokes swapTo so it could automatically inherit nonReentrant from swapTo. Move nonReentrant modifier from swap to swapTo.

Handle

paulius.eth

Vulnerability details

Impact

function getPseudoRand uses a very poor source of randomness so it is easily replicable on another smart contract. When enableDirectRedeem is turned off, you can't specify specificIds, however, it does not stop advanced users to write a custom smart contract that exploits this randomness or reverts if the final output is not what was intended. I know that you are aware that this random generation is not safe (thus named 'pseudo') but still I think this is worth pointing out as it gives an unfair advantage to those that know smart contracts and can build a service on top of it. Also, it helps to avoid directRedeemFee.

Handle

paulius.eth

Vulnerability details

Impact

When is1155 is true, function receiveNFTs iterates over all the tokens and updates holdings and quantity1155. If the quantity1155 is 0 for that token, it adds this token to the holdings set. However, it does not check that the amount is greater than 0, thus it is possible to push the same token to the hodlings multiple times as quantity1155 still remains 0.

Recommended Mitigation Steps

Solution: check that amount > 0 if is1155 is true. Also, it would be a good practice to check that the amounts array is empty when it is erc721 as amounts are ignored for ERC721 vaults.

Handle

paulius.eth

Vulnerability details

Impact

Contract NFTXMintRequestEligibility function requestMint sets mintRequests to the amount that was minted, however, it does not check that amount[i] > 0, so it is possible that when the token is not erc1155, the amount has a value of 0 but the token is still transferred from the sender to the contract and because mintRequests remains 0, this token cannot later be redeemed as all these claim functions have a condition: require(amount > 0).

Recommended Mitigation Steps

Either always require that amount[i] > 0 or explicitly set mintRequests to 1 when erc721 is deposited.