BorrowFacet.sol: Using transferFrom on ERC721 tokens could lead users fund to be locked

Summary

The contract BorrowFacet.sol use the call function transferFrom in the function borrow(), which is not adequate for ERC721 tokens.

Vulnerability Detail

In the function borrow() of contract BorrowFacet.sol, when sending borrowed tokens to caller, the transferFrom keyword is used instead of safeTransferFrom. If any caller is a contract and is not aware of incoming ERC721 tokens, the sent tokens could be locked.

Impact

Lock of users fund.

Code Snippet

https://github.com/sherlock-audit/2023-02-kairos/blob/main/kairos-contracts/src/BorrowFacet.sol#L36-L43

    /// @notice take loans, take ownership of NFTs specified as collateral, sends borrowed money to caller
    /// @param args list of arguments specifying at which terms each collateral should be used
    function borrow(BorrowArg[] calldata args) external {
        for (uint256 i = 0; i < args.length; i++) {
            args[i].nft.implem.transferFrom(msg.sender, address(this), args[i].nft.id);
            useCollateral(args[i].args, msg.sender, args[i].nft);
        }
    }

Tool used

Manual Review

Recommendation

Consider changing transferFrom to safeTransferFrom in line 40.

However, it could introduce a DoS attack vector if any caller maliciously rejects the received ERC721 tokens to make the others unable to get their tokens.

Alternatively we recommend to use a try/catch statement to handle error cases separately.

sherlock-audit / 2023-02-kairos-judging

w42d3n - BorrowFacet.sol: Using transferFrom on ERC721 tokens could lead users fund to be locked #177