Submitted by shw, also found by cmichel, gpersoon, pauliax, Sherlock, and toastedsteaksandwhich
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.
The function _removeNft uses an unbounded loop, which iterates the array nfts until a specific one is found. If the NFT to be removed is at the very end of the nfts array, this function could consume a large amount of gas.
The function onERC721Received is permission-less. The vault accepts any NFTs from any NFT contract and pushes the received NFT into the array nfts.
A malicious user could write an NFT contract, which calls onERC721Received of the vault many times to make the array nfts grow to a large size. Besides, the malicious NFT contract reverts when anyone tries to transfer (e.g., safeTransferFrom) its NFT.
The vault then has no way to remove the transferred NFT from the malicious NFT contract. The two only functions to remove NFTs, transferERC721 and timeUnlockERC721, fail since the malicious NFT contract reverts all safeTransferFrom calls.
As a result, benign users who unlock or transfer NFTs would suffer from large and unnecessary gas consumption. The consumed gas could even exceed the block gas limit and cause the transaction to fail every time.
Recommend using a mapping (e.g., mapping(address=>Nft[]) nfts) to store the received NFTs into separate arrays according to nftContract instead of putting them into the same one. Or, add a method specifically for the owner to remove NFTs from the nfts array directly.
xyz-ctrl (Visor) confirmed:
ghoul-sol (Judge) commented:
xyz-ctrl (Visor) commented:
ghoul-sol (Judge) commented:
ztcrypto (Visor) patched:
