salvorio / salvor-contracts

0 stars 0 forks source link

[SLG-04M] Inexistent Prevention of Repays / Extensions for Expired Loans #19

Closed HKskn closed 2 months ago

HKskn commented 7 months ago

SLG-04M: Inexistent Prevention of Repays / Extensions for Expired Loans

Type Severity Location
Logical Fault SalvorLending.sol:L325, L392

Description:

The SalvorLending::extend and SalvorLending::repay functions do not validate whether a loan has expired while the SalvorLending::clearDebt function permits the NFT of a loan to be claimed (i.e. the loan defaulting) solely if the loan duration has elapsed.

This discrepancy permits a borrower to solely repay / extend their loan if the lender claims the NFT, doing so using a MEV bot to guarantee that they extend / repay it first.

Impact:

The present system is unfair towards lenders who may solely acquire the funds they are owed only if they attempt to claim the NFT collateral and solely at that time.

Example:

/**
* @notice Repays the loan for a specific NFT and returns the NFT to the borrower. This function is internal.
* @param nftContractAddress The address of the NFT contract.
* @param _tokenId The ID of the token (NFT) for which the loan is being repaid.
*/
function repay(address nftContractAddress, uint256 _tokenId) internal {
    address borrower = items[nftContractAddress][_tokenId].borrower;
    address lender = items[nftContractAddress][_tokenId].lender;
    require(borrower == msg.sender, "msg.sender is not borrower");

    uint256 payment = _calculateRepayment(items[nftContractAddress][_tokenId].amount, items[nftContractAddress][_tokenId].rate);

    emit Repay(nftContractAddress, _tokenId, payment);

    IAssetManager(assetManager).transferFrom(borrower, lender, payment);

    IAssetManager(assetManager).nftTransferFrom(address(this), borrower, nftContractAddress, _tokenId);

    delete items[nftContractAddress][_tokenId];
}

/**
* @notice Clears the debt associated with a specific NFT after the loan period has finished. This function is internal.
* @param nftContractAddress The address of the NFT contract.
* @param _tokenId The ID of the token (NFT) for which the debt is being cleared.
*/
function clearDebt(address nftContractAddress, uint256 _tokenId) internal {
    address lender = items[nftContractAddress][_tokenId].lender;
    require(lender == msg.sender, "msg.sender is not lender");
    require((block.timestamp - items[nftContractAddress][_tokenId].startedAt) > items[nftContractAddress][_tokenId].duration, "loan period is not finished");

    emit ClearDebt(nftContractAddress, _tokenId);

    IAssetManager(assetManager).nftTransferFrom(address(this), lender, nftContractAddress, _tokenId);

    delete items[nftContractAddress][_tokenId];
}

Recommendation:

We advise the SalvorLending::repay and SalvorLending::extend functions to prevent invocation if the loan period has finished, ensuring that an expired loan can be claimed at any time by the lender and cannot be repaid at an arbitrary time.

HKskn commented 7 months ago

In response to this issue, it should be noted that this behavior is intentional and aligned with our business plan. Therefore, we do not consider this a medium-level issue.