Missing checkOnERC721Received deviates from ERC721 and could lock/lose NFTs

Handle 0xRajeev Vulnerability details Impact ERC721 specification for safeTransferFrom says: “this function checks if to is a smart contract code size 0. If so, it calls onERC721Received on to and throws if the return value is not bytes4keccak256“onERC721Receivedaddress,address,uint256,bytes”.”...

Locked funds from tokenization are credited twice to user leading to protocol fund loss

Handle 0xRajeev Vulnerability details Impact The tokens optionally locked during tokenization are released twice on acquiring conviction back from a NFT. The incorrect double debit of locked funds during tokenization has been filed as a separate finding because it is not necessarily related and...

Conviction score is not updated during tokenization if funds are locked

Handle 0xRajeev Vulnerability details Impact The updateConvictionScore on Line284 of tokenizeConviction is only called if user specifies zero locked funds. This leads to loss of accounting of user’s conviction score for tokenization since the last update for user if non-zero amount of FSDs are...

ERC20ConvictionScore.acquireConviction implements wrong governance checks

Handle cmichel Vulnerability details Vulnerability Details There are two issues with the governance checks when acquiring them from an NFT: Missing balance check The governance checks in updateConvictionScore are: !isGovernanceuser && userConvictionScore = governanceThreshold && balanceOfuser =...

Unbounded loop in _removeNft could lead to a griefing/DOS attack

Handle shw Vulnerability details Impact Griefing/DOS attack is possible when a malicious NFT contract sends many NFTs to the vault, which could cause excessive gas consumed and even transactions reverted when other users are trying to unlock or transfer NFTs. Proof of Concept 1. The function...

Should reset timelockERC721s after calling transferERC721

Handle shw Vulnerability details Impact The function transferERC721 does not reset timelockERC721s after the NFT is transferred. If the same NFT token is time-locked again but with a different recipient, the recipient could not transfer the time-locked NFT by calling transferERC721 since he...

A user could transfer the approved NFT several times

Handle shw Vulnerability details Impact Once the owner calls approveTransferERC721 to approve a user to transfer a specific NFT from the vault, the user can transfer out the NFT every time as long as the owner of NFT is the vault. Proof of Concept In the function transferERC721, the approvals of...

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

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

Getting NFT could exceed block size limit

Handle Sherlock Vulnerability details Impact On line 239 it loops over all the NFTs stored in the contract. Although a return statement is included it can cause the transaction to run out of gas in case of an extreme amount of NFTs. Proof of Concept N/A Tools Used Hardhat Recommended Mitigation...

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

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

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

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