NFTs are not stored in the appropriate contracts

Summary

NFTs are not stored in the appropriate contracts, which results in DoS when performing borrowing / repaying / liquidating actions.

Vulnerability Detail

When putting an NFT as collateral and taking out a loan, the borrower calls BorrowFacet#borrow.

    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);
        }
    }

The NFT is transferred into address(this), which is the BorrowFacet contract.

When the borrower wants to repay and reclaim back his NFT, he calls RepayFacet#repay.

            loan.assetLent.checkedTransferFrom(msg.sender, address(this), toRepay);
            loan.collateral.implem.safeTransferFrom(address(this), loan.borrower, loan.collateral.id);
            emit Repay(loanIds[i]);

The collateral is then transferred from address(this) (RepayFacet contract) to the borrower.

In AuctionFacet.sol, the buyer of the NFT gets his NFT from the AuctionFacet contract.

    function useLoan(BuyArg memory arg) internal {
        Loan storage loan = protocolStorage().loan[arg.loanId];

        checkLoanStatus(arg.loanId);
        uint256 toPay = price(arg.loanId);

        /* store as liquidated and paid before transfers to avoid malicious reentrency, following
        checks-effects-interaction pattern */
        loan.payment.liquidated = true;
        loan.payment.paid = toPay;
        loan.assetLent.checkedTransferFrom(msg.sender, address(this), toPay);
        loan.collateral.implem.safeTransferFrom(address(this), arg.to, loan.collateral.id);

During liquidation period, the borrower can still call repay to get back his NFT if the NFT is not liquidated or claimed by the borrower yet.

            if (loan.payment.paid > 0 || loan.payment.borrowerClaimed || loan.payment.liquidated) {

If the NFT is in RepayFacet, then only the borrower can retrieve his NFT. If the NFT is in the liquidation contract, then the borrower cannot retrieve his NFT. If the NFT is still in the BorrowFacet contract, then neither the borrower nor the liquidator can get hold of the NFT.

Impact

Users will get DoS because the NFT is stuck in one contract.

Code Snippet

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

https://github.com/sherlock-audit/2023-02-kairos/blob/main/kairos-contracts/src/RepayFacet.sol#L58

https://github.com/sherlock-audit/2023-02-kairos/blob/main/kairos-contracts/src/AuctionFacet.sol#L59-L71

Tool used

Manual Review

Recommendation

Consider try / check for all transfers and calling different contracts too see which contract stores the NFT, or storing all the NFTs in one contract and calling that particular contract.

sherlock-audit / 2023-02-kairos-judging

peanuts - NFTs are not stored in the appropriate contracts #160