Tapioca DAO - ItsNio's results

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L316

Vulnerability details

Impact

Users who participate() and place stakes with large magnitudes may have their weight removed prematurely from pool.cumulative, hence causing the weight logic of participation to be wrong. pool.cumulative will have an incomplete image of the actual pool hence allowing future users to have divergent power when they should not. In particular, this occurs during the exitPosition() function.

Proof of Concept

This vulnerability stems from exitPosition() using the current pool.AverageMagnitude instead of the respective magnitudes of the user's weights to update pool.cumulative on line 316. Hence, when users call exitPosition(), the amount that pool.cumulative is updated but may not be representative of the weight of the user's input.

Imagine if we have three users, Alice, Bob, and Charlie who all decide to call participate(). Alice calls participate() with a smaller amount and a smaller time, hence having a weight of 10. Bob calls participate() with a larger amount and a larger time, hence having a weight of 50. Charlie calls participate() with a weight of 20.

Scenario

• Alice calls participate() first at time 0 with the aforementioned amount and time. The pool.cumulative is now 10 and the pool.AverageMagnitude is 10 as well. Alice's position will expire at time 10.
• Bob calls participate() at time 5. The pool.cumulative is now 10 + 50 = 60 and the pool.AverageMagnitude is 50.
• Alice calls exitPosition() at time 10. pool.cumulative is 60, but pool.AverageMagnitude is still 50. Hence, pool.cumulative will be decreased by 50, even though the weight of Alice's input is 10.
• Charlie calls participate with weight 20. Charlie will have divergent power in the pool with both Bob and Charlie, since 20 > pool.cumulative (10). ...

If Alice does not participate at all, Charlie will not have divergent power in a pool with Bob and Charlie, since the pool.cumulative = Bob's weight = 50 > Charlie's weight (20).

We have provided a test to demonstrate the pool.cumulative inflation. Copy the following code intotap-token-audit/test/oTAP/tOB.test.ts as one of the tests.

it('POC', async () => {
        const {
            signer,
            tOLP,
            tOB,
            tapOFT,
            sglTokenMock,
            sglTokenMockAsset,
            yieldBox,
            oTAP,
        } = await loadFixture(setupFixture);

        // Setup tOB
        await tOB.oTAPBrokerClaim();
        await tapOFT.setMinter(tOB.address);

        // Setup - register a singularity, mint and deposit in YB, lock in tOLP
        const amount = 3e10;
        const lockDurationA = 10;
        const lockDurationB = 100;
        await tOLP.registerSingularity(
            sglTokenMock.address,
            sglTokenMockAsset,
            0,
        );

        await sglTokenMock.freeMint(amount);
        await sglTokenMock.approve(yieldBox.address, amount);
        await yieldBox.depositAsset(
            sglTokenMockAsset,
            signer.address,
            signer.address,
            amount,
            0,
        );

        const ybAmount = await yieldBox.toAmount(
            sglTokenMockAsset,
            await yieldBox.balanceOf(signer.address, sglTokenMockAsset),
            false,
        );
        await yieldBox.setApprovalForAll(tOLP.address, true);
        //A (short less impact)
        console.log(ybAmount);
        await tOLP.lock(
            signer.address,
            sglTokenMock.address,
            lockDurationA,
            ybAmount.div(100),
        );
        //B (long, big impact)
        await tOLP.lock(
            signer.address,
            sglTokenMock.address,
            lockDurationB,
            ybAmount.div(2),
        );
        const tokenID = await tOLP.tokenCounter();
        const snapshot = await takeSnapshot();
        console.log("A Duration: ", lockDurationA, " B Duration: ", lockDurationB);
        // Just A Participate
        console.log("Just A participation");
        await tOLP.approve(tOB.address, tokenID.sub(1));
        await tOB.participate(tokenID.sub(1));
        const participationA = await tOB.participants(tokenID.sub(1));
        const oTAPTknID = await oTAP.mintedOTAP();
        await time.increase(lockDurationA);
        const prevPoolState = await tOB.twAML(sglTokenMockAsset);
        console.log("[B4] Just A Cumulative: ", await prevPoolState.cumulative);
        console.log("[B4] Just A Average: ", participationA.averageMagnitude);
        await oTAP.approve(tOB.address, oTAPTknID);
        await tOB.exitPosition(oTAPTknID);
        console.log("Exit A position");
        const newPoolState = await tOB.twAML(sglTokenMockAsset);
        console.log("[A4] Just A Cumulative: ", await newPoolState.cumulative);
        console.log("[A4] Just A Average: ", await participationA.averageMagnitude);

        //Both Participations
        console.log();
        console.log("Run both participation---");
        const ctime1 = new Date();
        console.log("Time: ", ctime1);
        //A and B Participate
        await snapshot.restore();
        //Before everything
        const initPoolState = await tOB.twAML(sglTokenMockAsset);
        console.log("[IN] Initial Cumulative: ", await initPoolState.cumulative);
        //First participate A
        await tOLP.approve(tOB.address, tokenID.sub(1));
        await tOB.participate(tokenID.sub(1));
        const xparticipationA = await tOB.participants(tokenID.sub(1));
        const ATknID = await oTAP.mintedOTAP();
        console.log("Participate A (smaller weight)");
        console.log("[ID] A Token ID: ", ATknID);
        const xprevPoolState = await tOB.twAML(sglTokenMockAsset);
        console.log("[B4] Both A Cumulative: ", await xprevPoolState.cumulative);
        console.log("[B4] Both A Average: ", await xparticipationA.averageMagnitude);
        console.log();

        //Time skip to half A's duration
        await time.increase(5);
        const ctime2 = new Date();
        console.log("Participate B (larger weight), Time(+5): ", ctime2);

        //Participate B
        await tOLP.approve(tOB.address, tokenID);
        await tOB.participate(tokenID);
        const xparticipationB = await tOB.participants(tokenID);
        const BTknID = await oTAP.mintedOTAP();
        console.log("[ID] B Token ID: ", ATknID);
        const xbothPoolState = await tOB.twAML(sglTokenMockAsset);
        console.log("[B4] Both AB Cumulative: ", await xbothPoolState.cumulative);
        console.log("[B4] Both B Average: ", await xparticipationB.averageMagnitude);
        
        //Time skip end A
        await time.increase(6);
        await oTAP.approve(tOB.address, ATknID);
        await tOB.exitPosition(ATknID);
        const exitAPoolState = await tOB.twAML(sglTokenMockAsset);
        const ctime3 = new Date();
        console.log();
        console.log("Exit A (Dispraportionate Weight, Time(+6 Expire A): ", ctime3);
        console.log("[!X!] Just B Cumulative: ", await exitAPoolState.cumulative);
        console.log("[A4] Just B Average: ", xparticipationB.averageMagnitude);


        //TIme skip end B
        await time.increase(lockDurationB);
        await oTAP.approve(tOB.address, BTknID);
        await tOB.exitPosition(BTknID);
        const exitBPoolState = await tOB.twAML(sglTokenMockAsset);
        const ctime4 = new Date();
        console.log("Exit B, Time(+100 Expire B): ", ctime4);
        console.log("[A4] END Cumulative: ", await exitBPoolState.cumulative);

    });

This test runs the aforementioned scenario.

Expected Output:

BigNumber { value: "30000000000" }
A Duration:  10  B Duration:  100
Just A participation
[B4] Just A Cumulative:  BigNumber { value: "10" }
[B4] Just A Average:  BigNumber { value: "10" }
Exit A position
[A4] Just A Cumulative:  BigNumber { value: "0" }
[A4] Just A Average:  BigNumber { value: "10" }

Run both participation---
Time:  2023-08-03T21:40:52.700Z
[IN] Initial Cumulative:  BigNumber { value: "0" }
Participate A (smaller weight)
[ID] A Token ID:  BigNumber { value: "1" }
[B4] Both A Cumulative:  BigNumber { value: "10" }
[B4] Both A Average:  BigNumber { value: "10" }

Participate B (larger weight), Time(+5):  2023-08-03T21:40:52.801Z
[ID] B Token ID:  BigNumber { value: "1" }
[B4] Both AB Cumulative:  BigNumber { value: "60" }
[B4] Both B Average:  BigNumber { value: "50" }

Exit A (Dispraportionate Weight, Time(+6 Expire A):  2023-08-03T21:40:52.957Z
[!X!] Just B Cumulative:  BigNumber { value: "10" }
[A4] Just B Average:  BigNumber { value: "50" }
Exit B, Time(+100 Expire B):  2023-08-03T21:40:53.029Z
[A4] END Cumulative:  BigNumber { value: "0" }
    ✔ POC (1077ms) 

The POC is split into two parts:

The first part starting with Just A Participation is when just A enters and exits. This is correct, with the pool.cumulative increasing by 10 (the weight of A) and then being decreased by 10 when A exits.

The second part starting with Run both participation--- describes the scenario mentioned by the bullet points. In particular, the pool.cumulative starts as 0 ([IN] Initial Cumulative).

Then, A enters the pool, and the pool.cumulative is increased to 10 ([B4] Both A Cumulative) similar to the first part.

Then, B enters the pool, before A exits. B has a weight of 50, thus the pool.cumulativeincreases to 60 ([B4] Both AB Cumulative).

The bug can be seen after the line beginning with [!X!]. The pool.cumulative labeled by "Just B Cumulative" is decreased by 60 - 10 = 50 when A exits, although the weight of A is only 10.

Recommended Mitigation Steps

There may be a need to store weights at the time of adding a weight instead of subtracting the last computed weight in exitPosition(). For example, when Alice calls participate(), the weight at that time is stored and removed when exitPosition() is called.

Assessed type

Math

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

... 
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

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

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.

Assessed type

Math

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/markets/singularity/SGLCommon.sol#L107

Vulnerability details

Impact

The fee calculation for _getInterestRate() is calculated using the interest amount before accrual. This will result in the contract receiving less funds that originally intended from interest fees.

Proof of Concept

SGLCommon.sol contains a function _getInterestRate() which calculates the current interest rate. In particular, it accrues interest on line 99 and updates the _totalBorrow.elastic on line 104, then calculates a fee based on the interest to be taken on lines 106-109.

However, the fee calculations on the aforementioned lines are not calculated based on the accrued interest, rather the pre-accrual interest. In particular, line 107 makes reference to fullAssetAmount, which is computed on lines 55-59 as a portion of the _totalBorrow.elastic amount. Notice that the accrual to _totalBorrow.elastic occurs on line 104, which is after fullAssetAmount is computed.

Recommended Mitigation Steps

Move the calculation for fullAssetAmount after the interest accrual.

...
        // Accrue interest
        extraAmount =
            (uint256(_totalBorrow.elastic) *
                _accrueInfo.interestPerSecond *
                elapsedTime) /
            1e18;
        _totalBorrow.elastic += uint128(extraAmount);
        
        //@audit fullAssetAmount factors in accrual
    +    uint256 fullAssetAmount = yieldBox.toAmount(     
    +        assetId,
    +        _totalAsset.elastic,
    +        false
    +    ) + _totalBorrow.elastic;

    +    utilization = fullAssetAmount == 0
    +   ? 0
    +        : (uint256(_totalBorrow.elastic) * UTILIZATION_PRECISION) /
    +        fullAssetAmount;
...

Assessed type

Math

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/SGLCommon.sol#L112

Vulnerability details

Impact

The calculation for utilization in _getInterestRate() does not factor in the accrued interest. This leads to _accrueInfo.interestPerSecond being under-represented, and leading to incorrect interest rate calculation and potentially endangering conditions such as utilization > maximumTargetUtilization on line 124.

Proof of Concept

The calculation for utilization in the _getInterestRate() function for SGLCommon.sol occurs on lines 61-64 as a portion of the fullAssetAmount (which is also problematic) and the _totalBorrow.elastic. However, _totalBorrow.elastic is accrued by interest on line 99. This accrued amount is not factored into the calculation for utilization, which will be used to update the new interest rate, as purposed by the comment on line 111.

Recommended Mitigation Steps

Factor in the interest accrual into the utilization calculation:

...
        // Accrue interest
        extraAmount =
            (uint256(_totalBorrow.elastic) *
                _accrueInfo.interestPerSecond *
                elapsedTime) /
            1e18;
        _totalBorrow.elastic += uint128(extraAmount);
        
    +    uint256 fullAssetAmount = yieldBox.toAmount(    
    +        assetId,
    +        _totalAsset.elastic,
    +        false
    +    ) + _totalBorrow.elastic;
        //@audit utilization factors in accrual
    +    utilization = fullAssetAmount == 0
    +   ? 0
    +        : (uint256(_totalBorrow.elastic) * UTILIZATION_PRECISION) /
    +        fullAssetAmount;
...

Assessed type

Math