search

code-423n4 / 2022-04-jpegd-findings

1 stars 1 forks source link

NFTVault will freeze any directly sent NFT #193

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L560 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L805

Vulnerability details

Impact

NFTVault does not implement the onERC721Received function, the absence of which leads to the permanent freeze of any ERC721 tokens received via direct non-safe transfers.

There is no way to recover them as all the functions that can send an NFT away require borrow related structures to be properly filled, which will not be the case for a mistakenly sent NFT. Borrow() will fail also on an attempt to move the NFT in _openPosition() as ownership transfer be already done.

Setting severity to medium as it is the permanent fund freeze impact, but its probability isn't high as only mistakenly sent NFT are in question. For the project that keeps NFT as a collateral it's not too low either (DEX contracts filled with directly sent and frozen funds can be the examples).

Proof of Concept

transferFrom() that doesn't revert on the absence of the onERC721Received trigger is a part of EIP-721:

https://eips.ethereum.org/EIPS/eip-721

NFTVault doesn't implement onERC721Received, while the only instance of inbound transfer logic is _openPosition():

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L560

It is invoked only in borrow():

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L694

closePosition() requires borrow structures to be filled with an original owner:

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L805

Business logic wise this is correct, but since there is no NFT rescue function either, the mistakenly sent NFT will be permanently frozen.

Recommended Mitigation Steps

Consider implementing the onERC721Received in NFTVault which reverts whenever invoked not as a part of _openPosition() (i.e. set a flag for the processed _nftIndex in _openPosition() and require it in onERC721Received)

spaghettieth commented 2 years ago

onERC721Received isn't invoked when calling transferFrom, it's only invoked when calling safeTransferFrom, which reverts anyway because onERC721Received isn't implemented. Implementing it wouldn't prevent erroneously sent NFTs from getting stuck in the contract as transferFrom wouldn't revert. It's also not the NFTVault's job to prevent these situations from happening.

dmvt commented 2 years ago

Invalid