The amount of debt removed during `liquidation` may be worth more than the account's collateral

criticalCode4rena

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L822

Vulnerability details

Impact

The contract decreases user's debts but may not take the full worth in collateral from the user, leading to the contract losing potential funds from the missing collateral.

Proof of concept

During the liquidate() function call, the function _updateBorrowAndCollateralShare() is eventually invoked. This function liquidates a user's debt and collateral based on the value of the collateral they own.

In particular, the equivalent amount of debt, availableBorrowPart is calculated from the user's collateral on line 225 through the computeClosingFactor() function call.

Then, an additional fee through the liquidationBonusAmount is applied to the debt, which is then compared to the user's debt on line 240. The minimum of the two is assigned borrowPart, which intuitively means the maximum amount of debt that can be removed from the user's debt.

borrowPart is then increased by a bonus through liquidationMultiplier, and then converted to generate collateralShare, which represents the amount of collateral equivalent in value to borrowPart (plus some fees and bonus).

This new collateralShare may be more than the collateral that the user owns. In that case, the collateralShare is simply decreased to the user's collateral.

collateralShare is then removed from the user's collateral.

The problem lies in that although the collateralShare is equivalent to the borrowPart, or the debt removed from the user's account, it could be worth more than the collateral that the user owns in the first place. Hence, the contract loses out on funds, as debt is removed for less than it is actually worth.

To demonstrate, we provide a runnable POC.

Preconditions

solidity
... 
if (collateralShare > userCollateralShare[user]) {
        require(false, "collateralShare and borrowPart not worth the same"); //@audit add this line
        collateralShare = userCollateralShare[user];
    }
    userCollateralShare[user] -= collateralShare;
...

Add the require statement to line 261. This require statement essentially reverts the contract when the if condition satisfies. The if condition holds true when the collateralShare is greater that the user's collateral, which is the target bug.

Once the changes have been made, add the following test into the singularity.test.ts test in tapioca-bar-audit/test

Code

typescript
it('POC', async () => {
            const {
                usdc,
                wbtc,
                yieldBox,
                wbtcDepositAndAddAsset,
                usdcDepositAndAddCollateralWbtcSingularity,
                eoa1,
                approveTokensAndSetBarApproval,
                deployer,
                wbtcUsdcSingularity,
                multiSwapper,
                wbtcUsdcOracle,
                __wbtcUsdcPrice,
            } = await loadFixture(register);

            const assetId = await wbtcUsdcSingularity.assetId();
            const collateralId = await wbtcUsdcSingularity.collateralId();
            const wbtcMintVal = ethers.BigNumber.from((1e8).toString()).mul(1);
            const usdcMintVal = wbtcMintVal
                .mul(1e10)
                .mul(__wbtcUsdcPrice.div((1e18).toString()));

            // We get asset
            await wbtc.freeMint(wbtcMintVal);
            await usdc.connect(eoa1).freeMint(usdcMintVal);

            // We approve external operators
            await approveTokensAndSetBarApproval();
            await approveTokensAndSetBarApproval(eoa1);

            // We lend WBTC as deployer
            await wbtcDepositAndAddAsset(wbtcMintVal);
            expect(
                await wbtcUsdcSingularity.balanceOf(deployer.address),
            ).to.be.equal(await yieldBox.toShare(assetId, wbtcMintVal, false));

            // We deposit USDC collateral
            await usdcDepositAndAddCollateralWbtcSingularity(usdcMintVal, eoa1);
            expect(
                await wbtcUsdcSingularity.userCollateralShare(eoa1.address),
            ).equal(await yieldBox.toShare(collateralId, usdcMintVal, false));

            // We borrow 74% collateral, max is 75%

            console.log("Collateral amt: ", usdcMintVal);
            const wbtcBorrowVal = usdcMintVal
                .mul(74)
                .div(100)
                .div(__wbtcUsdcPrice.div((1e18).toString()))
                .div(1e10);
            console.log("WBTC borrow val: ", wbtcBorrowVal);
            console.log("[$] Original price: ", __wbtcUsdcPrice.div((1e18).toString()));
            await wbtcUsdcSingularity
                .connect(eoa1)
                .borrow(eoa1.address, eoa1.address, wbtcBorrowVal.toString());
            await yieldBox
                .connect(eoa1)
                .withdraw(
                    assetId,
                    eoa1.address,
                    eoa1.address,
                    wbtcBorrowVal,
                    0,
                );

            const data = new ethers.utils.AbiCoder().encode(['uint256'], [1]);
            // Can't liquidate
            await expect(
                wbtcUsdcSingularity.liquidate(
                    [eoa1.address],
                    [wbtcBorrowVal],
                    multiSwapper.address,
                    data,
                    data,
                ),
            ).to.be.reverted;
            console.log("Price Drop: 120%");
            const priceDrop = __wbtcUsdcPrice.mul(40).div(100);
            await wbtcUsdcOracle.set(__wbtcUsdcPrice.add(priceDrop));
            await wbtcUsdcSingularity.updateExchangeRate()
            console.log("Running liquidation... ");
            await expect(
                wbtcUsdcSingularity.liquidate(
                    [eoa1.address],
                    [wbtcBorrowVal],
                    multiSwapper.address,
                    data,
                    data,
                ),
            ).to.be.revertedWith("collateralShare and borrowPart not worth the same");
            console.log("[*] Reverted with reason: collateralShare and borrowPart not worth the same [Bug]");
            //console.log("Collateral Share after liquidation: ", (await wbtcUsdcSingularity.userCollateralShare(eoa1.address)));
            //console.log("Borrow part after liquidation: ", (await wbtcUsdcSingularity.userBorrowPart(eoa1.address)));
        });

Expected Result

bash
Collateral amt:  BigNumber { value: "10000000000000000000000" }
WBTC borrow val:  BigNumber { value: "74000000" }
[$] Original price:  BigNumber { value: "10000" }
Price Drop: 120%
Running liquidation... 
[*] Reverted with reason: collateralShare and borrowPart not worth the same [Bug]
      ✔ POC (2289ms)

As demonstrated, the function call reverts due to the require statement added in the preconditions.

Recommended Mitigation

One potential mitigation for this issue would be to calculate the borrowPart depending on the existing users' collateral factoring in the fees and bonuses. The collateralShare with the fees and bonuses should not exceed the user's collateral.