to/loan.borrower is unchecked in removeCollateral()/repay(), which can cause user's collateral NFT to be frozen

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L266-L266

Vulnerability details

Impact

The to/loan.borrower will receive the collateral NFT when removeCollateral()/repay() is called. However, if to/loan.borrower is a contract address that does not support ERC721, the collateral NFT can be frozen in the contract.

As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

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

Proof of Concept

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L266-L266
https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L540-L540
https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L295-L295
https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L573-L573

Tools Used

None

Recommended Mitigation Steps

Use safeTransferFrom instead of transferFrom

[cryptolyndon]

Not an issue. safeTransferFrom protects against some, but not all, avenues of losing tokens by misuse. The price for this is that an external call can get made, incurring a gas cost and introducing a potential reentrancy attack vector. With all this in mind, we have intentionally elected not to use it.

[0xean]

downgrading to QA, this is a design decision / low severity as the lender and borrower should understand how this contract will interact with theirs.

[JeeberC4]

Preserving original title: to/loan.borrower is unchecked in removeCollateral()/repay(), which can cause user's collateral NFT to be frozen