A previously timelocked NFT token becomes permanently stuck in vault if it’s ever moved back into the vault

Handle 0xRajeev Vulnerability details Impact Let’s consider a scenario where a particular NFT token was timelocked for a certain duration by the owner using timeLockERC721 with a delegate as the recipient and then transferred out of the vault by the delegate via transferERC721 but without unlocki...

Approval for NFT transfers is not removed after transfer

Handle cmichel Vulnerability details Vulnerability Details The Visor.transferERC721 does not reset the approval for the NFT. Impact An approved delegatee can move the NFT out of the contract once. It could be moved to a market and bought by someone else who then deposits it again to the same vaul...

NFT transfer approvals are not removed and cannot be revoked thus leading to loss of NFT tokens

Handle 0xRajeev Vulnerability details Impact NFT transfer approvals that are set to true in approveTransferERC721 are never set to false and there is no way to remove such an nft approval. Impact-1: The approval is not removed set to false after a transfer in transferERC721. So if the NFT is ever...

Denial of service for NFT's

Handle gpersoon Vulnerability details Impact The function removeNft uses more gas as more NFT's are added. An attacker can send random NFT's to the contract, which are received via onERC721Received. This functions adds the NFT's to the array nfts, using the function addNft. The longer the nfts...

function transferERC721 does not delete timelockERC721s if the token was among locked tokens

Handle paulius.eth Vulnerability details Impact function timeUnlockERC721 deletes timelockERC721s after removing NFT, so I expect a similar behavior with function transferERC721. It iterates over timelockERC721Keys and if it finds the token among locked tokens, it does some extra checks and later...

anyone can call onERC721Received

Handle paulius.eth Vulnerability details Impact function onERC721Received does not authorize a caller thus anyone can add a new NFT passing arbitrary values to nftContract and tokenId even if they do not send the actual NFT. Recommended Mitigation Steps Authorize the caller depending on the...

nftApprovals not reset in transferERC721

Handle gpersoon Vulnerability details Impact The function transferERC721 checks for nftApprovals, however it never reset the value of nftApprovals. This means if the NFT would end up in the contract again in the future, it could be transferred again without an explicit approval. Proof of Concept...

Randomization of NFTs returned in redeem/swap operations can be brute-forced

Handle 0xRajeev Vulnerability details Impact If we assume that certain NFTs in a vault over time will have different market demand/price then the users will try to redeem those specific NFTs. Even if direct redeems are disabled to prevent such a scenario to default to returning randomized NFTs, a...

The direct redeem fee can be circumvented

Handle janbro Vulnerability details Summary The direct redeem fee can be circumvented Risk Rating Medium Vulnerability Details Since the random NFT is determined in the same transaction a payment or swap is being executed, a malicious actor can revert a transaction if they did not get the NFT the...

NFTs initially of similar price may change significantly in value leading to unfair ownership changes

Handle 0xRajeev Vulnerability details Impact Vaults might start off as holding NFTs of similar price but over time some of them might be revealed as having unique/hidden features or older ones with lower IDs leading to increased demand e.g. certain Hashmasks or Cryptopunks which might significant...

function receiveNFTs does not check if amount > 0

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...

getRandomTokenIdFromFund not really random

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 blockhashblock.number - 1 is fully determined for al the transactions in the same block. The...

Missing zero-address check for the beneficiary address

Handle 0xRajeev Vulnerability details Impact The beneficiary address specified in constructor receives all the proceeds from NFT sales which could be of significant value. However, there is no zero-address validation of this beneficiary address parameter during initialization in the constructor...

ERC-721 Enumerable Spec mismatch for index of tokenByIndex() function

Handle 0xRajeev Vulnerability details Impact Index starts at 0 for token array but the implementation here requires index to be greater than 0. This will prevent querying of token at index 0. See reference implementation This will impact compatibility with NFT platforms that expect full conformit...

Missing zero/threshold check for NFT sale price

Handle 0xRajeev Vulnerability details Impact A zero or some minimum threshold check is missing for price parameter of startSale function which sets the mint price for NFTs. If accidentally set to 0 then all sales happen at this incorrect price leading to missed revenue. This cannot be corrected...

ERC-721 Enumerable Spec mismatch for return value of tokenByIndex() function

Handle 0xRajeev Vulnerability details Impact tokenByIndex is required to return the token at queried index but the implementation here returns the parameter index itself. This will prevent all querying of tokens. See reference implementation This will impact compatibility with NFT platforms that...

Missing zero/threshold check for NFT sale duration

Handle 0xRajeev Vulnerability details Impact A zero or some minimum threshold check is missing for saleDuration parameter of startSale function which sets the duration of the public sale of NFTs. If accidentally set to 0 then sales happen at zero price according to the logic in getPrice leading t...

NFT can be minted for free after sale ended

Handle s1m0 Vulnerability details Impact The getPrice return 0 after the sale ended and SALELIMIT - numSales nft can be minted for free. Proof of Concept Tools Used Manual analysis Recommended Mitigation Steps Without documentation i'm not sure if it's the expected behaviour or not. If it's not y...

function tokenByIndex treats last index as invalid

Handle paulius.eth Vulnerability details Impact NFT indexes start from 0: // Don't allow a zero index, start counting at 1 return value.add1; so if there are 30 tokens, indexes would be 1-30. However, function tokenByIndex sets such boundaries: requireindex 0 && index TOKENLIMIT; which means that...

Potential reentrancy in safeTransferFrom functions

Handle 0xRajeev Vulnerability details Impact Reentrancy possible from onERC721Received implementation of a malicious contract at to address. But it doesn’t look like this cannot be exploited here because there are no state effects after the external interaction i.e. CEI pattern holds. Consider...