sherlock-audit / 2023-02-kairos-judging

2 stars 0 forks source link

peanuts - Some NFTs like CryptoKitty and CryptoFighter can be paused, which block repaying/liquidation actions #159

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

peanuts

medium

Some NFTs like CryptoKitty and CryptoFighter can be paused, which block repaying/liquidation actions

Summary

Some NFTs like CryptoKitty and CryptoFighter can be paused, which block repaying and liquidating actions. When NFTs are paused, borrowers still need to pay the accumulated interest and might not be able to liquidate on time.

Vulnerability Detail

When a borrower uses an NFT as collateral to borrow money, the NFT is accruing interest every second. In RepayFacet#repay, the function calculates the interest of the collateral through the interestPerSecond value, and adds up the total interests on top of the principal amount.

        for (uint256 i = 0; i < loanIds.length; i++) {
            loan = proto.loan[loanIds[i]];
            // loan.payment.paid may be at 0 and considered repaid in case of an auction sale executed at price 0
            if (loan.payment.paid > 0 || loan.payment.borrowerClaimed || loan.payment.liquidated) {
                revert LoanAlreadyRepaid(loanIds[i]);
            }
            lent = loan.lent;
            /* if the linear interests are very low due to a short time elapsed, the minimal interests amount to repay
            is applied as an anti ddos mechanism */
            interests = RayMath.max(
                /* during the interests calculus, we can consider that (block.timestamp - loan.startDate)
                won't exceed 1e10 (>100 years) and interest per second (unwrapped value) won't exceed
                1e27 (corresponding to an amount to repay doubling after 1 second), we can deduce that
                (loan.interestPerSecond.mul(block.timestamp - loan.startDate)) is capped by 1e10 * 1e27 = 1e37
                we want to avoid the interests calculus to overflow so the result must not exceed 1e77
                as (1e77 < type(uint256).max). So we can allow `lent` to go as high as 1e40, but not above.
                This explains why borrowing throws on loan.lent > 1e40, as this realisticly avoids
                repaying being impossible due to an overflow. */
                /* the interest per second is a share of what has been lent to add to the interests each second. The
                next line accrues linearly */
                lent.mul(loan.interestPerSecond.mul(block.timestamp - loan.startDate)),
                loan.payment.minInterestsToRepay
/// @--audit here the borrower has to pay the lent amount + accumulated interest
            toRepay = lent + interests;
            loan.payment.paid = toRepay;
            );

The NFT collateral also has a loan.endDate which allows for liquidation after loan.endDate is passed.

AuctionFacet.sol

    function checkLoanStatus(uint256 loanId) internal view {
        Loan storage loan = protocolStorage().loan[loanId];

        if (block.timestamp < loan.endDate) {
            revert CollateralIsNotLiquidableYet(loan.endDate, loanId);
        }

In both CryptoKitty and CryptoFighter NFT, the transfer method can be paused.

In crypto-figher NFT:

https://etherscan.io/address/0x87d598064c736dd0C712D329aFCFAA0Ccc1921A1#code#L873

function transferFrom(
    address _from,
    address _to,
    uint256 _tokenId
)
    public
    whenNotPaused
{

In Crypto-kitty NFT:

https://etherscan.io/address/0x06012c8cf97BEaD5deAe237070F9587f8E7A266d#code#L615

function transferFrom(
    address _from,
    address _to,
    uint256 _tokenId
)
    external
    whenNotPaused
{

note the WhenNotPaused modifier.

Impact

If the transfer and transferFrom is paused in CryptoKitty and CryptoFighter NFT, the repaying action will be blocked. The borrower cannot fully clear his debt and has to wait until the transfer is unpaused to pay the unnecessary extra interest. Also, if the NFT is paused for far too long, the NFT will be subjected to liquidation. Both scenarios will be unfair for the borrower.

Code Snippet

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

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

Any ERC721 can be used: 

DEPLOYMENT: any EVM
ERC20: any
ERC721: any
ERC777: none
FEE-ON-TRANSFER: none
REBASING TOKENS: none
ADMIN: owner() - Trusted
EXTERNAL-ADMINS: none

https://github.com/sherlock-audit/2023-02-kairos-cryptostaker2

Tool used

Manual Review

Recommendation

Recommend not charging interest when the external NFT contract is paused, or extending the borrowing time of these NFTs for a similar duration when they are paused. Alternatively, recommend having a blocklist and take note of these types of pausable NFT and not allow these NFTs to be used as collateral.

npasquie commented 1 year ago

I dispute this issue for the following rationale pausing assets is a non-standard practice that can be seen as the token admin intentionally breaking its normal operations I consider it to be the responsibility of the asset admins not to break their usage / compensate problems due to their action, and its the lenders/borrowers responsibility to acknowledge no app can guarantee no loss when its used with assets breaking standards