search

sherlock-audit / 2024-02-leverage-contracts-judging

1 stars 0 forks source link

0xDetermination - `liquidationBonus` may be forever unclaimable if a lender burns their NFT #38

Closed sherlock-admin2 closed 8 months ago

sherlock-admin2 commented 8 months ago

0xDetermination

medium

liquidationBonus may be forever unclaimable if a lender burns their NFT

Summary

In the case that normal liquidation is impossible and emergency liquidation must be used, a lender burning the UniV3 NFT can lock the liquidationBonus forever.

Vulnerability Detail

Notice that the liquidationBonus is only transferred to the last emergency liquidator, i.e. when completeRepayment equals true:

            if (completeRepayment) {
                LoanInfo[] memory empty;
                _removeKeysAndClearStorage(borrowing.borrower, params.borrowingKey, empty);
                feesAmt += liquidationBonus;
            } else {
                ...
            }
            holdTokenOut = removedAmt + feesAmt;
            // Transfer removedAmt + feesAmt to msg.sender and emit EmergencyLoanClosure event
            Vault(VAULT_ADDRESS).transferToken(borrowing.holdToken, msg.sender, holdTokenOut);

And this variable is only set to true when all loans are removed from the borrower's loansInfo in _calculateEmergencyLoanClosure(), but this will never happen if a lender has burned the UniV3 NFT due to the msg.sender check against the NFT owner:

    function _calculateEmergencyLoanClosure(
        bool zeroForSaleToken,
        bytes32 borrowingKey,
        uint256 totalfeesOwed,
        uint256 totalBorrowedAmount
    ) private returns (uint256 removedAmt, uint256 feesAmt, bool completeRepayment) {
        // Create a memory struct to store liquidity cache information.
        NftPositionCache memory cache;
        // Get the array of LoanInfo structs associated with the given borrowing key.
        LoanInfo[] storage loans = loansInfo[borrowingKey];
        // Iterate through each loan in the loans array.
        for (uint256 i; i < loans.length; ) {
            LoanInfo memory loan = loans[i];
            // Get the owner address of the loan's token ID using the underlyingPositionManager contract.
            address creditor = _getOwnerOf(loan.tokenId);
            // Check if the owner of the loan's token ID is equal to the `msg.sender`.
            if (creditor == msg.sender) {
                // If the owner matches the `msg.sender`, replace the current loan with the last loan in the loans array
                // and remove the last element.
                loans[i] = loans[loans.length - 1];
                loans.pop();
                // Remove the borrowing key from the tokenIdToBorrowingKeys mapping.
                tokenIdToBorrowingKeys[loan.tokenId].remove(borrowingKey);
                // Update the liquidity cache based on the loan information.
                _upNftPositionCache(zeroForSaleToken, loan, cache);
                // Add the holdTokenDebt value to the removedAmt.
                removedAmt += cache.holdTokenDebt;
                // Calculate the fees amount based on the total fees owed and holdTokenDebt.
                feesAmt += FullMath.mulDiv(totalfeesOwed, cache.holdTokenDebt, totalBorrowedAmount);
            } else {
                // If the owner of the loan's token ID is not equal to the `msg.sender`,
                // the function increments the loop counter and moves on to the next loan.
                unchecked {
                    ++i;
                }
            }
        }
        // Check if all loans have been removed, indicating complete repayment.
        completeRepayment = loans.length == 0;
    }

Therefore, if a borrower's position is only emergency-liquidatable then the liquidationBonus can be lost forever.

Impact

Lenders won't receive the liquidationBonus and it may be forever unclaimable.

Code Snippet

https://github.com/sherlock-audit/2024-02-leverage-contracts/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L656-L659 https://github.com/sherlock-audit/2024-02-leverage-contracts/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L784-L823

Tool used

Manual Review

Recommendation

Perform a check to see if the only loans left are from burned NFTs and set completeRepayment to true if so.

Also want to mention that there's a similar issue that can be fixed in a similar way

nevillehuang commented 8 months ago

Request PoC

Seems Invalid, burning NFT poses a threat only to its owner since he will lose all collateral. Would be good to have a PoC to show potential loss involved.

sherlock-admin commented 8 months ago

PoC requested from @0xDetermination

Requests remaining: 2

0xDetermination commented 8 months ago

Hi @nevillehuang, providing PoC below:

First, run the test "emergency repay will be successful for PosManNFT owner if the collateral is depleted" like normal and console.log the balance of the last liquidator after emergency liquidating. That's the last loan for that borrowing position, so the last liquidator will get the liquidationBonus. See below:

it("emergency repay will be successful for PosManNFT owner if the collateral is depleted", async () => {
    ....
    /////////////LOG BALANCE///////////////
        console.log('last emergency liquidator token balance before', await WETH.balanceOf(owner.address));
        await time.increase(100);
        deadline = (await time.latest()) + 60;
        await expect(borrowingManager.connect(owner).repay(params, deadline))
            .to.emit(borrowingManager, "EmergencyLoanClosure")
            .withArgs(bob.address, owner.address, borrowingKey);
        ...
        /////////////LOG BALANCE///////////////
        console.log('last emergency liquidator token balance after', await WETH.balanceOf(owner.address));

Then to see the difference if a token is burned, run the modified test below (changes are burning the first liquidator's token and commenting out some of the position checks since the burned token will make those fail).

    it("emergency repay will be successful for PosManNFT owner if the collateral is depleted", async () => {
        let debt: ILiquidityBorrowingManager.BorrowingInfoExtStructOutput[] =
            await borrowingManager.getBorrowerDebtsInfo(bob.address);
        await time.increase(debt[1].estimatedLifeTime.toNumber() + 1);

        let borrowingKey = (await borrowingManager.getBorrowingKeysForBorrower(bob.address))[1];
        let deadline = (await time.latest()) + 60;

        let params: ILiquidityBorrowingManager.RepayParamsStruct = {
            returnOnlyHoldToken: true,
            isEmergency: true, //emergency
            internalSwapPoolfee: 0,
            externalSwap: [],
            borrowingKey: borrowingKey,
            minHoldTokenOut: BigNumber.from(0),
            minSaleTokenOut: BigNumber.from(0)
        };

        //console.log(debt);
        let loans: ILiquidityManager.LoanInfoStructOutput[] = await borrowingManager.getLoansInfo(borrowingKey);
        expect(loans.length).to.equal(3);
        //console.log(loans);
        /////////BURN TOKEN////////////
        await nonfungiblePositionManager.connect(alice).burn(nftpos[0].tokenId);
        /*await expect(borrowingManager.connect(alice).repay(params, deadline))
            .to.emit(borrowingManager, "EmergencyLoanClosure")
            .withArgs(bob.address, alice.address, borrowingKey);

        expect(await borrowingManager.getLenderCreditsCount(nftpos[0].tokenId)).to.be.equal(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[1].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[2].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[3].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[4].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[5].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getBorrowerDebtsCount(bob.address)).to.be.equal(2);*/

        debt = await borrowingManager.getBorrowerDebtsInfo(bob.address);
        //console.log(debt);
        loans = await borrowingManager.getLoansInfo(borrowingKey);
        //expect(loans.length).to.equal(2);

        await time.increase(100);
        deadline = (await time.latest()) + 60;
        await expect(borrowingManager.connect(bob).repay(params, deadline))
            .to.emit(borrowingManager, "EmergencyLoanClosure")
            .withArgs(bob.address, bob.address, borrowingKey);
        /*expect(await borrowingManager.getLenderCreditsCount(nftpos[0].tokenId)).to.be.equal(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[1].tokenId)).to.be.equal(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[2].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[3].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[4].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[5].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getBorrowerDebtsCount(bob.address)).to.be.equal(2);
        debt = await borrowingManager.getBorrowerDebtsInfo(bob.address); */
        //console.log(debt);
        loans = await borrowingManager.getLoansInfo(borrowingKey);
        //expect(loans.length).to.equal(1);

        /////////////LOG BALANCE///////////////
        console.log('last emergency liquidator token balance before', await WETH.balanceOf(owner.address));
        await time.increase(100);
        deadline = (await time.latest()) + 60;
        await expect(borrowingManager.connect(owner).repay(params, deadline))
            .to.emit(borrowingManager, "EmergencyLoanClosure")
            .withArgs(bob.address, owner.address, borrowingKey);
        /*expect(await borrowingManager.getLenderCreditsCount(nftpos[0].tokenId)).to.be.equal(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[1].tokenId)).to.be.equal(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[2].tokenId)).to.be.equal(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[3].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[4].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[5].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getBorrowerDebtsCount(bob.address)).to.be.equal(1);*/
        /////////////LOG BALANCE///////////////
        console.log('last emergency liquidator token balance after', await WETH.balanceOf(owner.address));
    });

In both tests the balance of the last liquidator (owner) before liquidating is identical (98166360688065201240), but the balance after is smaller in the second test (100085253956926185648 vs. 100005725406022127556). The difference is from the liquidationBonus not being transferred since alice burned the token.

I also want to add that this issue is distinct from an issue like "if a lender burns the token their fees will be stuck"- in that case the lender will indeed only be self-griefing.

A malicious borrower could also use this bug to grief lenders. For example:

  1. Borrower's position is decreasing in value and the borrower's collateral is low, he doesn't want to increase collateral to repay.
  2. Borrower takes out a dust loan with his own token in the same position
  3. Borrower burns his token
  4. Borrower becomes liquidatable, since the position decreased in value emergency mode must be used.
  5. Lenders liquidate, but since the borrower burned his token the liquidationBonus will not be disbursed to anyone.

Since the max liquidation bonus is 1%, there could be a significant amount of loss (like if the borrower's position is $100,000 which is very likely due to the protocol enabling high leverage, then $1000 will be stuck in the contract and lenders won't receive that amount).

nevillehuang commented 8 months ago

@fann95 Any thoughts on the above? The only way this seems to be economically viable is if the borrower's collateral has a lower value than a 1% liquidation bonus, which seems abit out of reach given the required significant price drop

0xDetermination commented 8 months ago

@nevillehuang @fann95 This typically won't happen as it's not a direct griefing attack, the scenario is more like "if a borrower's position devalued over time and normal liquidation is not possible because the borrower won't repay the loan, then the borrower can lock/grief the liquidation bonus as he will never receive it anyways".

0xDetermination commented 8 months ago

Another scenario that could happen (while unlikely) is that a borrower has a loan from some lender such that the value is <1% of the overall position, in which case the lender could grief with a damage/cost ratio >1 if normal liquidation is impossible.

fann95 commented 8 months ago 
it("emergency repay will be successful for PosManNFT owner if the collateral is depleted", async () => {
        let debt: ILiquidityBorrowingManager.BorrowingInfoExtStructOutput[] =
            await borrowingManager.getBorrowerDebtsInfo(bob.address);
        await time.increase(debt[1].estimatedLifeTime.toNumber() + 1);

        let borrowingKey = (await borrowingManager.getBorrowingKeysForBorrower(bob.address))[1];
        let deadline = (await time.latest()) + 60;

        let params: ILiquidityBorrowingManager.RepayParamsStruct = {
            returnOnlyHoldToken: true,
            isEmergency: true, //emergency
            internalSwapPoolfee: 0,
            externalSwap: [],
            borrowingKey: borrowingKey,
            minHoldTokenOut: BigNumber.from(0),
            minSaleTokenOut: BigNumber.from(0)
        };

        //console.log(debt);
        let loans: ILiquidityManager.LoanInfoStructOutput[] = await borrowingManager.getLoansInfo(borrowingKey);
        expect(loans.length).to.equal(3);
        //console.log(loans);
        /////////BURN TOKEN////////////
        await nonfungiblePositionManager.connect(alice).burn(nftpos[0].tokenId);
        /*await expect(borrowingManager.connect(alice).repay(params, deadline))
            .to.emit(borrowingManager, "EmergencyLoanClosure")
            .withArgs(bob.address, alice.address, borrowingKey);

        expect(await borrowingManager.getLenderCreditsCount(nftpos[0].tokenId)).to.be.equal(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[1].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[2].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[3].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[4].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[5].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getBorrowerDebtsCount(bob.address)).to.be.equal(2);*/

        debt = await borrowingManager.getBorrowerDebtsInfo(bob.address);
        //console.log(debt);
        loans = await borrowingManager.getLoansInfo(borrowingKey);
        //expect(loans.length).to.equal(2);

        await time.increase(100);
        deadline = (await time.latest()) + 60;
        await expect(borrowingManager.connect(bob).repay(params, deadline))
            .to.emit(borrowingManager, "EmergencyLoanClosure")
            .withArgs(bob.address, bob.address, borrowingKey);
        /*expect(await borrowingManager.getLenderCreditsCount(nftpos[0].tokenId)).to.be.equal(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[1].tokenId)).to.be.equal(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[2].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[3].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[4].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[5].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getBorrowerDebtsCount(bob.address)).to.be.equal(2);
        debt = await borrowingManager.getBorrowerDebtsInfo(bob.address); */
        //console.log(debt);
        loans = await borrowingManager.getLoansInfo(borrowingKey);
        //expect(loans.length).to.equal(1);

        /////////////LOG BALANCE///////////////
        console.log('last emergency liquidator token balance before', await WETH.balanceOf(owner.address));
        await time.increase(100);
        deadline = (await time.latest()) + 60;
        await expect(borrowingManager.connect(owner).repay(params, deadline))
            .to.emit(borrowingManager, "EmergencyLoanClosure")
            .withArgs(bob.address, owner.address, borrowingKey);
        /*expect(await borrowingManager.getLenderCreditsCount(nftpos[0].tokenId)).to.be.equal(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[1].tokenId)).to.be.equal(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[2].tokenId)).to.be.equal(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[3].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[4].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getLenderCreditsCount(nftpos[5].tokenId)).to.be.gt(0);
        expect(await borrowingManager.getBorrowerDebtsCount(bob.address)).to.be.equal(1);*/
        /////////////LOG BALANCE///////////////
        console.log('last emergency liquidator token balance after', await WETH.balanceOf(owner.address));
        params.isEmergency = false;
        await expect(borrowingManager.connect(owner).repay(params, deadline))
            .to.emit(borrowingManager, "Repay")
            .withArgs(bob.address, owner.address, borrowingKey);
        console.log('last liquidator token balance after repay(isEmergency = false)', await WETH.balanceOf(owner.address));
    });
fann95 commented 8 months ago 
last emergency liquidator token balance before BigNumber { value: "98166360688065201240" }
last emergency liquidator token balance after BigNumber { value: "100005725406022127556" }
last liquidator token balance after repay(isEmergency = false) BigNumber { value: "101090782980961904620" }

but the liquidator was not the last)) I added 2 lines at the end of the test for understanding.. even if it is not possible to restore liquidity (low liquidity in the pool or another reason) since there are no longer any living NFTs, the position will still be closed and the one who closes the position will receive a liquidation bonus.

0xDetermination commented 8 months ago

@nevillehuang @fann95 Thanks very much for the correction, I think this is indeed Low/Invalid- I missed the fact that the last liquidator can call repay as emergency, then call it again as non-emergency to collect the liquidation fee and the tokens from the burned loan.

nevillehuang commented 8 months ago

@0xDetermination Got it thanks for the further insights, in that case closing this issue.