018.md

Bnke0x0

medium

Using TransferFrom on ERC721 tokens

Summary

Vulnerability Detail

Impact

the transferFrom keyword is used instead of safeTransferFrom. The sent tokens could be locked if any winner is in a contract and is unaware of incoming ERC721 tokens.

Code Snippet

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L343-L347

            IERC721Upgradeable(collateralInfo._collateralAddress).transferFrom(
                borrower,
                address(this),
                collateralInfo._tokenId
            );

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/escrow/CollateralEscrowV1.sol#L129-L133

            IERC721Upgradeable(_collateralAddress).transferFrom(
                _msgSender(),
                address(this),
                _tokenId
            );

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/escrow/CollateralEscrowV1.sol#L174-L178

            IERC721Upgradeable(_collateralAddress).transferFrom(
                address(this),
                _recipient,
                _collateral._tokenId
            );

Tool used

Manual Review

Recommendation

Consider changing transferFrom to safeTransferFrom. However, it could introduce a DoS attack vector if any user maliciously rejects the received ERC721 tokens to make the others unable to get their awards. Possible mitigations are to use a try/catch statement to handle error cases separately or provide a function for the pool owner to remove malicious winners manually if this happens.