[WP-H19] `NFTVault.sol` collateral NFT can be frozen in the NFTVault contract, when a contract with no `onERC721Received` method called `liquidate()` on a `USE_INSURANCE` position

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L915-L941

Vulnerability details

For a liquidated position with a borrowType of USE_INSURANCE, when insuraceRepurchaseTimeLimit is over (position.liquidatedAt + settings.insuraceRepurchaseTimeLimit) and the positionOwner still not repurchase(), the collateral NFT of the position can and can only be claimed by the liquidator via claimExpiredInsuranceNFT().

In claimExpiredInsuranceNFT(), the Collateral NFT will safeTransferFrom address(this) to the position.liquidator, which is the msg.sender used in liquidate(), must implement the onERC721Received method for the safeTransferFrom to be successfully.

Otherwise, the claimExpiredInsuranceNFT() transaction will always fail with UNSAFE_RECIPIENT in IERC721.safeTransferFrom().

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L915-L941

    /// @notice Allows the liquidator who liquidated the insured position with NFT at index `_nftIndex` to claim the position's collateral
    /// after the time period defined with `insuranceRepurchaseTimeLimit` has expired and the position owner has not repurchased the collateral.
    /// @dev Emits an {InsuranceExpired} event
    /// @param _nftIndex The NFT to claim
    function claimExpiredInsuranceNFT(uint256 _nftIndex)
        external
        validNFTIndex(_nftIndex)
    {
        Position memory position = positions[_nftIndex];
        address owner = positionOwner[_nftIndex];
        require(address(0) != owner, "no_position");
        require(position.liquidatedAt > 0, "not_liquidated");
        require(
            position.liquidatedAt + settings.insuraceRepurchaseTimeLimit <
                block.timestamp,
            "insurance_not_expired"
        );
        require(position.liquidator == msg.sender, "unauthorized");

        positionOwner[_nftIndex] = address(0);
        delete positions[_nftIndex];
        positionIndexes.remove(_nftIndex);

        nftContract.safeTransferFrom(address(this), msg.sender, _nftIndex);

        emit InsuranceExpired(owner, _nftIndex);
    }

In that case, the collateral NFT will be frozen in NFTVault, even though liquidator already repaid for the position with stablecoin.

PoC

Given:

• SAFE is a multisig timelock contract with no onERC721Received.

1. SAFE liquidate(). at L851 repay debtAmount of stablecoin;
2. settings.insuraceRepurchaseTimeLimit seconds later, SAFE claimExpiredInsuranceNFT() to get back the collateral NFT, the tx always reverts with UNSAFE_RECIPIENT.

Recommendation

Change to:

function claimExpiredInsuranceNFT(uint256 _nftIndex, address _sendCollateralTo)
    external
    validNFTIndex(_nftIndex)
{
    Position memory position = positions[_nftIndex];
    address owner = positionOwner[_nftIndex];
    require(address(0) != owner, "no_position");
    require(position.liquidatedAt > 0, "not_liquidated");
    require(
        position.liquidatedAt + settings.insuraceRepurchaseTimeLimit <
            block.timestamp,
        "insurance_not_expired"
    );
    require(position.liquidator == msg.sender, "unauthorized");

    positionOwner[_nftIndex] = address(0);
    delete positions[_nftIndex];
    positionIndexes.remove(_nftIndex);

    nftContract.safeTransferFrom(address(this), _sendCollateralTo, _nftIndex);

    emit InsuranceExpired(owner, _nftIndex);
}

[spaghettieth]

Liquidator contracts should always implement onERC721Received, responsibility of a bad liquidator implementation is not of NFTVault.

[dmvt]

Agree with the sponsor. In this case, the liquidator is responsible for their own contract and also the party harmed if their contract is written incorrectly. Anyone writing one of these contracts surely understands that they will need to be able to receive an ERC721 and should comply with spec.