search

code-423n4 / 2022-05-cally-findings

2 stars 0 forks source link

If users don’t handle ERC721 received, the ERC721 token will be frozen #246

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L295 https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L344

Vulnerability details

Impact

It doesn't check whether users will handle ERC721 received. If a user is a contract but doesn’t handle ERC721 received, the ERC721 token will be frozen when receiving ERC721 tokens.

Proof of Concept

        vault.tokenType == TokenType.ERC721
            ? ERC721(vault.token).transferFrom(address(this), msg.sender, vault.tokenIdOrAmount)
            : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);

It doesn’t check whether the receiver has implemented the onERC721Received function. If a user buys an option but the user doesn't handle ERC721 received, the ERC721(vault.token) will be frozen after the user exercises the option.

Tools Used

vim

Recommended Mitigation Steps

Use safeTransferFrom rather than transferFrom when transferring ERC721 tokens to users.

outdoteth commented 2 years ago

use safeTransferFrom to prevent stuck NFTs: https://github.com/code-423n4/2022-05-cally-findings/issues/38