Tapioca DAO - dirk_y's results

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/USDO.sol#L81-L104

Vulnerability details

Impact

USDO has a flash mint limit to ensure that only a maximum amount of USDO can be loaned in a single transaction. This is important because if there was no limit on the amount of USDO minted then you would effectively accumulate a huge amount of instantaneous debt. Usually the flash loan limit is defined by the available collateral, but because this is a flash mint (not a loan) governance sets the maximum limit to a sensible value of 100k USDO.

However this flash limit can be easily exceeded to borrow an arbitrarily high amount of USDO (capped by the block gas limit for computation), which results in a potentially unreasonable amount of instantaneous debt.

Proof of Concept

This is a relatively simple vulnerability to show and to resolve. Let's have a look at the method:

    function flashLoan(
        IERC3156FlashBorrower receiver,
        address token,
        uint256 amount,
        bytes calldata data
    ) external override notPaused returns (bool) {
        require(token == address(this), "USDO: token not valid");
        require(maxFlashLoan(token) >= amount, "USDO: amount too big");
        require(amount > 0, "USDO: amount not valid");
        uint256 fee = flashFee(token, amount);
        _mint(address(receiver), amount);

        require(
            receiver.onFlashLoan(msg.sender, token, amount, fee, data) ==
                FLASH_MINT_CALLBACK_SUCCESS,
            "USDO: failed"
        );

        uint256 _allowance = allowance(address(receiver), address(this));
        require(_allowance >= (amount + fee), "USDO: repay not approved");
        _approve(address(receiver), address(this), _allowance - (amount + fee));
        _burn(address(receiver), amount + fee);
        return true;
    }

The interesting part of the above code that I could craft a receiver onFlashLoan method that calls flashLoan again. Since there is no check for reentrancy I could repeatedly call flashLoan via the onFlashLoan callback with the maximum 100_000e18 amount to flash mint 100k USDO at a time. For example I could do this loop 10 times to flash mint 1m USDO. Provided I pay the fees for each of the 10 flash mints then this set of calls will exit successfully.

Tools Used

Manual review

Recommended Mitigation Steps

The OpenZeppelin reentrancy code should be inherited and the nonReentrant modifier should be added to the flashLoan method.

Assessed type

Reentrancy

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L608-L625 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L639-L650 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/Market.sol#L442-L461 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L576-L580 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L148-L149 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/Market.sol#L66-L68 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L500-L537

Vulnerability details

Impact

For BigBang liquidations I believe the intended behaviour is that the protocol takes a 10% cut of the profit and the caller should receive the remaining 90%. However the actual behaviour is that the liquidator takes between 1 and 10%, the protocol gets 10% and the rest is stuck forever. So for example, if the liquidator reward was 10%, the protocol would receive 10% and the remaining 80% would get stuck in the BigBang yield box.

This significantly disincentivises users to liquidate insolvent users in BigBang because the reward is significantly lower than expected and it also results in the unintended "burning" of capital.

Proof of Concept

A liquidator can liquidate a user by calling liquidate, which leads to the following call flow:

liquidate -> _closedLiquidation -> _liquidateUser

The interesting part of _liquidateUser is the following set of lines:

        uint256 balanceBefore = yieldBox.balanceOf(address(this), assetId);

        ISwapper.SwapData memory swapData = swapper.buildSwapData(
            collateralId,
            assetId,
            0,
            collateralShare,
            true,
            true
        );
        swapper.swap(swapData, minAssetMount, address(this), "");
        uint256 balanceAfter = yieldBox.balanceOf(address(this), assetId);

        uint256 returnedShare = balanceAfter - balanceBefore;
        (uint256 feeShare, uint256 callerShare) = _extractLiquidationFees(
            returnedShare,
            borrowShare,
            callerReward
        );

In these lines the seized collateral from the user being liquidated is swapped for USDO and then the liquidation fees are calculated based on the profit from the liquidation:

    function _extractLiquidationFees(
        uint256 returnedShare,
        uint256 borrowShare,
        uint256 callerReward
    ) private returns (uint256 feeShare, uint256 callerShare) {
        uint256 extraShare = returnedShare - borrowShare;
        feeShare = (extraShare * protocolFee) / FEE_PRECISION; // x% of profit goes to fee.
        callerShare = (extraShare * callerReward) / FEE_PRECISION; //  y%  of profit goes to caller.

        yieldBox.transfer(address(this), penrose.feeTo(), assetId, feeShare);
        yieldBox.transfer(address(this), msg.sender, assetId, callerShare);
    }

As you can see the protocol takes its 10% fee (protocolFee = 10000; // 10%) and then the caller fee is calculated. However, rather than using the callerFee variable (callerFee = 90000; // 90%) to calculated the caller fee, the callerReward variable is used which is calculated by an earlier call to _getCallerReward. This method calculates the reward based on the borrowed amount with a value between:

    uint256 public minLiquidatorReward = 1e3; //1%
    /// @notice max % a liquidator can receive in rewards
    uint256 public maxLiquidatorReward = 1e4; //10%

Therefore, the user is getting a maximum reward of 10%. So, what happens to this remaining 80% - 89%? This stays stuck in the yield box of the BigBang contract. The only time the USDO balance of the BigBang yield box is touched is when fees are collected, however the fees are calculated dynamically in the Penrose _depositFeesToYieldBox method, so the total balance of the yield box (accrued over liquidations) won't be able to be transferred to the fee receiver (i.e. the protocol).

    function _depositFeesToYieldBox(
        IMarket market,
        ISwapper swapper,
        IPenrose.SwapData calldata dexData
    ) private {
        require(swappers[swapper], "Penrose: Invalid swapper");
        require(isMarketRegistered[address(market)], "Penrose: Invalid market");

        uint256 feeShares = market.refreshPenroseFees(feeTo);
        if (feeShares == 0) return;

        uint256 assetId = market.assetId();
        uint256 amount = 0;
        if (assetId != wethAssetId) {
            yieldBox.transfer(
                address(this),
                address(swapper),
                assetId,
                feeShares
            );

            ISwapper.SwapData memory swapData = swapper.buildSwapData(
                assetId,
                wethAssetId,
                0,
                feeShares,
                true,
                true
            );
            (amount, ) = swapper.swap(
                swapData,
                dexData.minAssetAmount,
                feeTo,
                ""
            );
        } else {
            yieldBox.transfer(address(this), feeTo, assetId, feeShares);
        }

        emit LogYieldBoxFeesDeposit(feeShares, amount);
    }

As you can see the value transferred is feeShares which is calculated based on the actual USDO balance of the BigBang contract (not its yield box USDO balance):

    function refreshPenroseFees(
        address
    ) external onlyOwner notPaused returns (uint256 feeShares) {
        uint256 balance = asset.balanceOf(address(this));
        totalFees += balance;
        feeShares = yieldBox.toShare(assetId, totalFees, false);

        if (totalFees > 0) {
            asset.approve(address(yieldBox), totalFees);

            yieldBox.depositAsset(
                assetId,
                address(this),
                msg.sender,
                totalFees,
                0
            );

            totalFees = 0;
        }
    }

The funds intended for the liquidator are now stuck forever.

Tools Used

Manual review

Recommended Mitigation Steps

The following changes should be made to the BigBang contract to send 90% of the liquidation profit to the liquidator as intended:

diff --git a/contracts/markets/bigBang/BigBang.sol b/contracts/markets/bigBang/BigBang.sol
index c9b5397..f1c36c0 100644
--- a/contracts/markets/bigBang/BigBang.sol
+++ b/contracts/markets/bigBang/BigBang.sol
@@ -573,11 +573,7 @@ contract BigBang is BoringOwnable, Market {
                 userCollateralShare[user],
                 _exchangeRate
             );
-        uint256 callerReward = _getCallerReward(
-            userBorrowPart[user],
-            startTVLInAsset,
-            maxTVLInAsset
-        );
+
 
         (
             uint256 borrowAmount,
@@ -621,8 +617,7 @@ contract BigBang is BoringOwnable, Market {
         uint256 returnedShare = balanceAfter - balanceBefore;
         (uint256 feeShare, uint256 callerShare) = _extractLiquidationFees(
             returnedShare,
-            borrowShare,
-            callerReward
+            borrowShare
         );
         address[] memory _users = new address[](1);
         _users[0] = user;
@@ -638,12 +633,11 @@ contract BigBang is BoringOwnable, Market {
 
     function _extractLiquidationFees(
         uint256 returnedShare,
-        uint256 borrowShare,
-        uint256 callerReward
+        uint256 borrowShare
     ) private returns (uint256 feeShare, uint256 callerShare) {
         uint256 extraShare = returnedShare - borrowShare;
         feeShare = (extraShare * protocolFee) / FEE_PRECISION; // x% of profit goes to fee.
-        callerShare = (extraShare * callerReward) / FEE_PRECISION; //  y%  of profit goes to caller.
+        callerShare = (extraShare * callerFee) / FEE_PRECISION; //  y%  of profit goes to caller.
 
         yieldBox.transfer(address(this), penrose.feeTo(), assetId, feeShare);
         yieldBox.transfer(address(this), msg.sender, assetId, callerShare);

Assessed type

Math

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L103-L112 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L127-L138

Vulnerability details

Impact

Each of the YieldBox strategy contracts has an emergencyWithdraw method that allows the owner of the strategy to withdraw all the assets that have been deployed in a yield generating position. This emergency withdraw functionality exists to allow the owner to protect deployed funds in the event there is an exploit or issue with the external protocols being interacted with. Withdrawing quickly could be the difference between saving most of the deployed user funds and losing most of the deployed funds.

However, there is currently nothing stopping a malicious user from instantly depositing the funds that were emergency withdrawn again. The malicious user can repeatedly redeploy these funds after the admin tries to withdraw them, completely defeating the point of the emergency withdraw functionality.

Luckily there is a workaround for the admin to set an arbitrarily high deposit threshold that I talk about more in the recommendation section. This is the reason for downgrading from High to Medium severity.

Proof of Concept

Each one of the YieldBox strategy contracts has an emergencyWithdraw method that withdraws all the user funds that have been deposited to generate yield from the respective protocol. These are all slightly different depending on the underlying protocol it interacts with, however they all have the same core functionality. For the sake of argument we'll have a look at the LidoEthStrategy.sol contract:

    /// @notice withdraws everythig from the strategy
    function emergencyWithdraw() external onlyOwner returns (uint256 result) {
        compound("");

        uint256 toWithdraw = stEth.balanceOf(address(this));
        uint256 minAmount = (toWithdraw * 50) / 10_000; //0.5%
        result = curveStEthPool.exchange(1, 0, toWithdraw, minAmount);

        INative(address(wrappedNative)).deposit{value: result}();
    }

As you can see, this method does what it's supposed to do and withdraws all the ETH that was deposited and wraps it to WETH.

In order to deploy these funds again, _deposited has to be called again:

    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
            emit AmountDeposited(queued);
            return;
        }
        emit AmountQueued(amount);
    }

Since we've just withdrawn all the funds in an emergency, queued is guaranteed to be larger that depositThreshold. There is also no check here to prevent a call to _deposited if an emergency withdrawal had just been performed. The YieldBox depositAsset method also has no such checks, so all a malicious user has to do is call depositAsset with a trivial amount of asset to trigger the deposit into the yield generating position again; the assets are now in a vulnerable position again.

Tools Used

Manual review

Recommended Mitigation Steps

As I touched upon at the start of this report, a nice way to prevent a malicious user from instantly redepositing funds that have been withdrawn in an emergency is to set the deposit threshold to a huge number during the emergencyWithdraw call. This can then be set back to a lower value at a later date by the owner if desired. Below is a suggested diff that should be applied to all the strategy contracts:

diff --git a/contracts/lido/LidoEthStrategy.sol b/contracts/lido/LidoEthStrategy.sol
index f7aa0ea..ea4c191 100644
--- a/contracts/lido/LidoEthStrategy.sol
+++ b/contracts/lido/LidoEthStrategy.sol
@@ -109,6 +109,8 @@ contract LidoEthStrategy is BaseERC20Strategy, BoringOwnable, ReentrancyGuard {
         result = curveStEthPool.exchange(1, 0, toWithdraw, minAmount);
 
         INative(address(wrappedNative)).deposit{value: result}();
+
+        depositThreshold = type(uint256).max;
     }
 
     // ************************* //

Assessed type

Other

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/Balancer.sol#L200-L208 https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/Balancer.sol#L280-L295 https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/Balancer.sol#L322-L332

Vulnerability details

Impact

The mTapiocaOFT.sol contract is a special TOFT implementation that can balance its supply. The supply of the token is balanced across chains by using Stargate. When balancing a mTapiocaOFT through Stargate a cross chain swap fee has to be paid in native gas on the source chain as detailed in https://stargateprotocol.gitbook.io/stargate/developers/cross-chain-swap-fee.

However the current calls to Stargate will always fail because none of the native token value is passed along in the call. As a result the mTapiocaOFT representation of a token will never be able to be balanced across chain.

Proof of Concept

The owner of the Balancer.sol contract can perform a rebalance operation by calling rebalance. The part of this method that is interesting for this report is part where different paths are taken depending on the type of token:

        //send
        bool _isNative = ITapiocaOFT(_srcOft).erc20() == address(0);
        if (_isNative) {
            if (msg.value <= _amount) revert FeeAmountNotSet();
            _sendNative(_srcOft, _amount, _dstChainId, _slippage);
        } else {
            if (msg.value == 0) revert FeeAmountNotSet();
            _sendToken(_srcOft, _amount, _dstChainId, _slippage, _ercData);
        }

As you can see the mTapiocaOFTs that wrap a native token take a different path by calling _sendNative. In _sendNative the Stargate RouterETH is called:

    function _sendNative(
        address payable _oft,
        uint256 _amount,
        uint16 _dstChainId,
        uint256 _slippage
    ) private {
        if (address(this).balance < _amount) revert ExceedsBalance();

        routerETH.swapETH(
            _dstChainId,
            _oft, //refund
            abi.encodePacked(connectedOFTs[_oft][_dstChainId].dstOft),
            _amount,
            _computeMinAmount(_amount, _slippage)
        );
    }

whereas for non-native tokens _sendToken is called. We'll consider just the _sendNative case going forwards, but both have the same issue.

The bug in this code is that no native token (for the sake of argument let's say ETH) is forwarded with the external call to swapETH. As you can see from the implementation of this method (also found at https://etherscan.io/address/0x150f94B44927F078737562f0fcF3C95c01Cc2376#code), the call will instantly revert as no ETH is forwarded:

    function swapETH(
        uint16 _dstChainId,                         // destination Stargate chainId
        address payable _refundAddress,             // refund additional messageFee to this address
        bytes calldata _toAddress,                  // the receiver of the destination ETH
        uint256 _amountLD,                          // the amount, in Local Decimals, to be swapped
        uint256 _minAmountLD                        // the minimum amount accepted out on destination
    ) external payable {
        require(msg.value > _amountLD, "Stargate: msg.value must be > _amountLD");

Tools Used

Manual review

Recommended Mitigation Steps

The external calls to swapETH and swap should forward the appropriate amount of native token to pay for the cross chain swap fees:

diff --git a/contracts/Balancer.sol b/contracts/Balancer.sol
index b58058d..84b8e26 100644
--- a/contracts/Balancer.sol
+++ b/contracts/Balancer.sol
@@ -285,7 +284,7 @@ contract Balancer is Owned {
     ) private {
         if (address(this).balance < _amount) revert ExceedsBalance();
 
-        routerETH.swapETH(
+        routerETH.swapETH{value: address(this).balance}(
             _dstChainId,
             _oft, //refund
             abi.encodePacked(connectedOFTs[_oft][_dstChainId].dstOft),
@@ -319,7 +318,7 @@ contract Balancer is Owned {
             });
 
         erc20.approve(address(router), _amount);
-        router.swap(
+        router.swap{value: msg.value}(
             _dstChainId,
             _srcPoolId,
             _dstPoolId,

Assessed type

ETH-Transfer

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/convex/ConvexTricryptoStrategy.sol#L189-L190 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/convex/ConvexTricryptoStrategy.sol#L148-L149 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/convex/ConvexTricryptoStrategy.sol#L332

Vulnerability details

Impact

For the YieldBox strategies, each strategy that has the concept of rewards has the ability to compound these rewards by redeeming, swapping, and then redepositing into the same yield generating strategy. Every strategy compounds during withdrawal and emergency withdrawal.

However, the ConvexTricryptoStrategy never actually compounds the rewards during either of these calls. This is bad for 3 main reasons:

1. In emergency withdrawals we're not actually redeeming these rewards and pulling these funds. As a result, we could lose the accumulated rewards due to an exploit/issue in the underlying protocol
2. This is bad for YieldBox users as the rewards are not being compounded during normal use. As a result the yield obtained from the YieldBox for the relevant asset is lower than it could/should be and probably lower compared to the other strategies that are compounding rewards
3. There is no incentive for anyone to manually call compound to compound the rewards. Yes, the user would be getting a share of the rewards, however this is unlikely to be worth the gas costs to call compound on a relatively frequent cadence. As a result rewards will go un-compounded for a significant period of time (vs other strategies where rewards are compounded on most withdrawals)

Proof of Concept

Like all the other YieldBox strategies, the ConvexTricryptoStrategy strategy has a compound method. The interesting part of this method is the first line:

    function compound(bytes memory data) public {
        if (data.length == 0) return;

As you can see, if 0 bytes are passed with the call, then the compounding action doesn't actually take place. Now, compound is called in both _withdraw and emergencyWithdraw as designed, however for both of these calls, 0 bytes are passed:

    /// @notice withdraws everythig from the strategy
    function emergencyWithdraw() external onlyOwner returns (uint256 result) {
        compound("");

    /// @dev unstakes from Convex and withdraws from Curve
    function _withdraw(
        address to,
        uint256 amount
    ) internal override nonReentrant {
        uint256 available = _currentBalance();
        require(
            available >= amount,
            "ConvexTricryptoStrategy: amount not valid"
        );

        uint256 queued = wrappedNative.balanceOf(address(this));
        if (amount > queued) {
            compound(bytes(""));

As a result compounding of rewards isn't happening when it's supposed to.

Tools Used

Manual review

Recommended Mitigation Steps

The compound calls should pass in the appropriate data to perform the compounding during withdrawals and emergency withdrawals. If this data can change it should be set to a variable in state through another onlyOwner method as the reward tokens/pools change. Even if this pre-set data only covers 80% of the reward tokens/pools, this is a significant improvement. One-off calls can still be made to compound to compound any rewards not captured on the regular cadence.

Assessed type

Other

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOLeverageModule.sol#L62-L68 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOLeverageModule.sol#L190-L214 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L593

Vulnerability details

Impact

USDO and TapiocaOFT tokens can be sent across chains via LayerZero for leverage up and leverage down operations. This process is initiated through the sendForLeverage method in both USDOLeverageModule.sol and BaseTOFTLeverageModule.sol. The caller that is performing the leverage operation is responsible for providing the relevant parameters that are required, including the contract address of the swapper contract on the destination chain.

However the address of the swapper contract isn't validated once the LayerZero message is received on the destination chain, therefore it is possible that the swapper contract isn't a whitelisted address. This means that the swapper contract could be malicious and would therefore be able to steal the funds (either TOFT or USDO) minted on the destination chain.

You might say that this is just user error, however this check is performed in other Tapioca contracts like liquidate in BigBang.sol, where the user is also able to pass in an arbitrary swapper address, but in these cases the swapper address is checked to be a whitelisted address. There is a non-zero chance that a user could be manipulated (either through a UI exploit or phishing) into performing a leverage operation with a malicious swapper contract. It would be extremely difficult to spot this at transaction signing time.

Proof of Concept

Let's consider a leverage up action that is initiated from the USDOLeverageModule.sol:

function sendForLeverage(
        uint256 amount,
        address leverageFor,
        IUSDOBase.ILeverageLZData calldata lzData,
        IUSDOBase.ILeverageSwapData calldata swapData,
        IUSDOBase.ILeverageExternalContractsData calldata externalData
    ) external payable {

As you can see the user is able to provide arbitrary externalData which includes the address for the destination chain swapper contract. The whitelist status of the contract can't be checked here, it needs to be checked on the destination chain.

The LayerZero message that is sent from the above call is received on the destination chain and leads to a call to leverageUp and then leverageUpInternal. The interesting part of this flow is the first part of leverageUpInternal where the call to the swapper contract is made:

    function leverageUpInternal(
        uint256 amount,
        IUSDOBase.ILeverageSwapData memory swapData,
        IUSDOBase.ILeverageExternalContractsData memory externalData,
        IUSDOBase.ILeverageLZData memory lzData,
        address leverageFor
    ) public payable {
        //swap from USDO
        _approve(address(this), externalData.swapper, amount);
        ISwapper.SwapData memory _swapperData = ISwapper(externalData.swapper)
            .buildSwapData(
                address(this),
                swapData.tokenOut,
                amount,
                0,
                false,
                false
            );

        (uint256 amountOut, ) = ISwapper(externalData.swapper).swap(
            _swapperData,
            swapData.amountOutMin,
            address(this),
            swapData.data
        );

As you can see, the swapper contract is approved to spend the balance minted earlier in the call, and there is no check as to whether the swapper contract is a whitelisted contract. As a result, the call to swap could be used to steal the allowance and then return an amountOut value of 0 (or a very small number) that then completes the rest of the execution.

As mentioned above, there are other flows in Tapioca that check the whitelist status of a swapper, including the liquidate flow:

require(penrose.swappers(swapper), "BigBang: Invalid swapper");

Given every supported chain needs to have whitelisted swappers anyway for other functionality, there is no good reason why leverage operations shouldn't also have the same protection applied to them to ensure users can't lose funds.

Tools Used

Manual review

Recommended Mitigation Steps

The swapper contract should be checked on the destination chain during leverage operations. Below I have a suggested diff for USDO, but the same should also be applied to TOFT:

diff --git a/contracts/usd0/modules/USDOLeverageModule.sol b/contracts/usd0/modules/USDOLeverageModule.sol
index 53dcdef..53dc9c8 100644
--- a/contracts/usd0/modules/USDOLeverageModule.sol
+++ b/contracts/usd0/modules/USDOLeverageModule.sol
@@ -195,6 +195,7 @@ contract USDOLeverageModule is BaseUSDOStorage {
         address leverageFor
     ) public payable {
         //swap from USDO
+        require(IPenrose(owner).swappers(externalData.swapper), "USDO: Invalid swapper");
         _approve(address(this), externalData.swapper, amount);
         ISwapper.SwapData memory _swapperData = ISwapper(externalData.swapper)
             .buildSwapData(

Assessed type

Invalid Validation

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/Balancer.sol#L312-L319

Vulnerability details

Impact

The mTapiocaOFT.sol contract is a special TOFT implementation that can balance its supply. The supply of the token is balanced across chains by using Stargate.

The issue with the current implementation is that when calling Stargate swap to rebalance ERC20 tokens across chain there is an additional set of call parameters provided that are unnecessary and don't perform the function that is expected. Specifically the current implementation attempts to airdrop destination gas tokens to the destination OFT address even though this is completely unnecessary.

Proof of Concept

The relevant code can be found in _sendToken in Balancer.sol:

function _sendToken(
        address payable _oft,
        uint256 _amount,
        uint16 _dstChainId,
        uint256 _slippage,
        bytes memory _data
    ) private {
        IERC20Metadata erc20 = IERC20Metadata(ITapiocaOFT(_oft).erc20());
        if (erc20.balanceOf(address(this)) < _amount) revert ExceedsBalance();

        (uint256 _srcPoolId, uint256 _dstPoolId) = abi.decode(
            _data,
            (uint256, uint256)
        );

        IStargateRouter.lzTxObj memory _lzTxParams = IStargateRouterBase
            .lzTxObj({
                dstGasForCall: 0,
                dstNativeAmount: msg.value,
                dstNativeAddr: abi.encode(
                    connectedOFTs[_oft][_dstChainId].dstOft
                )
            });

        erc20.approve(address(router), _amount);
        router.swap(
            _dstChainId,
            _srcPoolId,
            _dstPoolId,
            _oft, //refund,
            _amount,
            _computeMinAmount(_amount, _slippage),
            _lzTxParams,
            _lzTxParams.dstNativeAddr,
            "0x"
        );
    }

Specifically we're looking at the _lzTxParams creation:

        IStargateRouter.lzTxObj memory _lzTxParams = IStargateRouterBase
            .lzTxObj({
                dstGasForCall: 0,
                dstNativeAmount: msg.value,
                dstNativeAddr: abi.encode(
                    connectedOFTs[_oft][_dstChainId].dstOft
                )
            });

This isn't performing the function that I think the author is expecting. Here are some relevant snippets from the Stargate documentation:

// https://stargateprotocol.gitbook.io/stargate/developers/how-to-swap
IStargateRouter.lzTxObj(0, 0, "0x")  // 0 additional gasLimit increase, 0 airdrop, at 0x address

// https://stargateprotocol.gitbook.io/stargate/v/user-docs/stargate-features/transfer#native-gas-delivery-transfer-to-destination-network

// https://stargateprotocol.gitbook.io/stargate/developers/cross-chain-swap-fee
    ({
        dstGasForCall: 0,       // extra gas, if calling smart contract,
        dstNativeAmount: 0,     // amount of dust dropped in destination wallet 
        dstNativeAddr: taskArgs.dstNativeAddr // destination wallet for dust
    })

So what the author is actually doing here is airdropping msg.value of the destination chain native gas token to the destination chain OFT address. There is no purpose to this action, and in fact it could result in the call to Stargate failing because you're trying to use some of the cross chain swap fee to airdrop gas tokens on the destination chain. Whether the call will fail depends on the swap fee provided and the difference in value between the source chain gas token and the destination chain gas token.

Tools Used

Manual review

Recommended Mitigation Steps

The following change should be made to the existing code since there is no need to airdrop tokens on the destination chain:

diff --git a/contracts/Balancer.sol b/contracts/Balancer.sol
index b58058d..d3151d2 100644
--- a/contracts/Balancer.sol
+++ b/contracts/Balancer.sol
@@ -312,10 +312,8 @@ contract Balancer is Owned {
         IStargateRouter.lzTxObj memory _lzTxParams = IStargateRouterBase
             .lzTxObj({
                 dstGasForCall: 0,
-                dstNativeAmount: msg.value,
-                dstNativeAddr: abi.encode(
-                    connectedOFTs[_oft][_dstChainId].dstOft
-                )
+                dstNativeAmount: 0,
+                dstNativeAddr: "0x0"
             });
 
         erc20.approve(address(router), _amount);

Assessed type

Other

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/Balancer.sol#L197-L208 https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/mTapiocaOFT.sol#L141-L146

Vulnerability details

Impact

The mTapiocaOFT.sol contract is a special TOFT implementation that can balance its supply. The supply of the token is balanced across chains by using Stargate. When balancing a mTapiocaOFT the caller needs to provide some native token to pay the Stargate fees.

The issue with the current implementation is that the fee check for mTapiocaOFTs that represent native tokens is incorrect, forcing the admin to pay for the rebalance amount plus the fee.

Proof of Concept

A mTapiocaOFT can be rebalanced if the owner of an approved balancer contract calls rebalance in Balancer.sol. As mentioned above, the rebalance operation uses Stargate to send tokens across chain. In order to pay the Stargate fees, some ETH (i.e. a native token) needs to be passes with any calls to Stargate and thus the rebalance method needs to be payable (which it is). The issues comes where the value passed to the call is validated:

        //extract
        ITapiocaOFT(_srcOft).extractUnderlying(_amount);

        //send
        bool _isNative = ITapiocaOFT(_srcOft).erc20() == address(0);
        if (_isNative) {
            if (msg.value <= _amount) revert FeeAmountNotSet();
            _sendNative(_srcOft, _amount, _dstChainId, _slippage);
        } else {
            if (msg.value == 0) revert FeeAmountNotSet();
            _sendToken(_srcOft, _amount, _dstChainId, _slippage, _ercData);
        }

As you can see, for native tokens the check is (msg.value <= _amount). However, we're already extracting the native token when calling extractUnderlying:

    function extractUnderlying(uint256 _amount) external {
        require(balancers[msg.sender], "TOFT_auth");

        bool _isNative = erc20 == address(0);
        if (_isNative) {
            _safeTransferETH(msg.sender, _amount);
        } else {
            IERC20(erc20).safeTransfer(msg.sender, _amount);
        }

        emit Rebalancing(msg.sender, _amount, _isNative);
    }

So in order to rebalance amount x across chain, the Balancer.sol owner would have to call rebalance with msg.value = x + fee, and the actual amount being rebalanced would be 2 * x.

However, as I've highlighted in another issue, the underlying call will luckily fail, but this is a distinct issue that should also be resolved.

Tools Used

Manual review

Recommended Mitigation Steps

The fee check should be identical for both native and non native mTapiocaOFT representations:

diff --git a/contracts/Balancer.sol b/contracts/Balancer.sol
index b58058d..aa09a07 100644
--- a/contracts/Balancer.sol
+++ b/contracts/Balancer.sol
@@ -182,6 +182,7 @@ contract Balancer is Owned {
         onlyValidDestination(_srcOft, _dstChainId)
         onlyValidSlippage(_slippage)
     {
+        if (msg.value == 0) revert FeeAmountNotSet();
         if (connectedOFTs[_srcOft][_dstChainId].rebalanceable < _amount)
             revert RebalanceAmountNotSet();
 
@@ -200,10 +201,8 @@ contract Balancer is Owned {
         //send
         bool _isNative = ITapiocaOFT(_srcOft).erc20() == address(0);
         if (_isNative) {
-            if (msg.value <= _amount) revert FeeAmountNotSet();
             _sendNative(_srcOft, _amount, _dstChainId, _slippage);
         } else {
-            if (msg.value == 0) revert FeeAmountNotSet();
             _sendToken(_srcOft, _amount, _dstChainId, _slippage, _ercData);
         }

Assessed type

Invalid Validation

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionLiquidityProvision.sol#L171-L201 https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionLiquidityProvision.sol#L351-L354 https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L216-L220 https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L351-L359 https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L413-L417

Vulnerability details

Impact

Once a user has lent capital in a Singularity market, they can lock their tOLR tokens for a tOLP NFT. They can then use this tOLP to participate with the TapiocaOptionBroker contract and mint an oTAP position. The minted oTAP gives the user the option to purchase TAP at a discount every epoch (week) for as long as the tOLR tokens are locked.

However a user can abuse the current system to only lock their tOLR tokens for 1 week, but be able to exercise 3 epochs worth of options to purchase TAP at a discount. The user is limiting their risk whilst maximising their rewards by circumventing the intended function of the option broker. Effectively a user is stealing funds from the protocol without interacting with the protocol as intended.

Proof of Concept

For the sake of argument, let's say that the TapiocaOptionBroker epoch has just finished and is eligible to be incremented by calling newEpoch, but hasn't been called yet.

A user that is trying to game the system could now bundle the following series of transactions:

1. Call lock in TapiocaOptionLiquidityProvision.sol with a lock duration equal to 1 week (1 epoch)
2. Call participate in TapiocaOptionBroker.sol with the _tOLPTokenID that was minted in the previous call
3. Call exerciseOption with the _oTAPTokenID minted in the previous step to purchase TAP at a discount
4. Call newEpoch to increment the epoch
5. Call exerciseOption again to purchase TAP at a discount in this new epoch

At this point the user has been able to exercise their option twice in the same block. Now the user can wait until the epoch is about to finish do the following:

1. Call newEpoch to increment the epoch
2. Call exerciseOption again to purchase TAP at a discount in this new epoch

The user has now managed to exercise 3 weeks worth of options with just a single week lock. This is possible for a number of reasons:

1. The user is able to exercise an option in the same epoch from which the option was minted
2. The underlying lock is valid up to and including the current timestamp and the epoch can be incremented from and including the current timestamp

     function _isPositionActive(uint256 tokenId) internal view returns (bool) {
        LockPosition memory lockPosition = lockPositions[tokenId];

        return
            (lockPosition.lockTime + lockPosition.lockDuration) >=
            block.timestamp;
    }

        require(
            block.timestamp >= lastEpochUpdate + EPOCH_DURATION,
            "tOB: too soon"
        );

3. Even if the above 2 issues were fixed tOLR tokens can be locked for an arbitrary duration that doesn't have to be a multiple of 1 week (1 epoch). This is important because even if there is a 1 block timing issue that I've accidentally missed, a user could lock for 1 epoch + 5 seconds to achieve the same effect.

    function lock(
        address _to,
        IERC20 _singularity,
        uint128 _lockDuration,
        uint128 _amount
    ) external returns (uint256 tokenId) {
        require(_lockDuration > 0, "tOLP: lock duration must be > 0");

4. Anyone can start a new epoch by calling newEpoch so these timing constraints can be achieved

The end result of all of this is that users can purchase a significant quantity of discounted TAP without having to commit to a long lock time.

Apologies for not including a hardhat test case to validate this. I did try for quite some time but Hardhat makes it really difficult to work with the timings of exact transactions compared to foundry in my opinion.

Tools Used

Manual review

Recommended Mitigation Steps

I suggest two main changes to mitigate this issue:

1. Users should not be able to exercise their options in the epoch in which the oTAP was minted
2. Lock durations in TapiocaOptionLiquidityProvision.sol should be a multiple of the epoch duration (1 week). This both enforces a minimum lock time and ensure that users aren't gaming the timing of epochs by locking for just more than the duration of a whole epoch.

Assessed type

Timing

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L430 https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L508-L536 https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L551-L555

Vulnerability details

Impact

At the moment both the TapiocaOptionBroker.sol contract and the AirdropBroker.sol contract assume the number of decimals returned from the TAP oracle and the payment token oracle are identical.

However this is not guaranteed to be true. If the number of decimals returned by the oracles is different then the discounted payment token amount to pay when exercising options is either orders of magnitude too large or orders of magnitude too small.

Proof of Concept

When a new epoch is started by calling newEpoch the current TAP value is fetched from the TAP oracle and remains constant for the epoch:

(, epochTAPValuation) = tapOracle.get(tapOracleData);

Let's assume this oracle uses 8 decimals.

When a user wants to exercise an option they call exerciseOption which calls _processOTCDeal under the hood:

    function _processOTCDeal(
        ERC20 _paymentToken,
        PaymentTokenOracle memory _paymentTokenOracle,
        uint256 tapAmount,
        uint256 discount
    ) internal {
        // Get TAP valuation
        uint256 otcAmountInUSD = tapAmount * epochTAPValuation;

        // Get payment token valuation
        (, uint256 paymentTokenValuation) = _paymentTokenOracle.oracle.get(
            _paymentTokenOracle.oracleData
        );

        // Calculate payment amount and initiate the transfers
        uint256 discountedPaymentAmount = _getDiscountedPaymentAmount(
            otcAmountInUSD,
            paymentTokenValuation,
            discount,
            _paymentToken.decimals()
        );

        _paymentToken.transferFrom(
            msg.sender,
            address(this),
            discountedPaymentAmount
        );
        tapOFT.extractTAP(msg.sender, tapAmount);
    }

Given the TAP token has 18 decimals, if we wanted to purchase 1 TAP where the value of TAP was 1USD then otcAmountInUSD = 1e18 * 1e8 = 1e26.

Next we get the paymentTokenValuation. Let's assume that this oracle uses 6 decimals and that the payment token also had a value of 1 USD and has 18 decimals. The return value would be 1e6.

Now _getDiscountedPaymentAmount is called to calculate the discounted amount:

    function _getDiscountedPaymentAmount(
        uint256 _otcAmountInUSD,
        uint256 _paymentTokenValuation,
        uint256 _discount,
        uint256 _paymentTokenDecimals
    ) internal pure returns (uint256 paymentAmount) {
        // Calculate payment amount
        uint256 rawPaymentAmount = _otcAmountInUSD / _paymentTokenValuation;
        paymentAmount =
            rawPaymentAmount -
            muldiv(rawPaymentAmount, _discount, 100e4); // 1e4 is discount decimals, 100 is discount percentage
        paymentAmount = paymentAmount / (10 ** (18 - _paymentTokenDecimals));
    }

With the assumed values above rawPaymentAmount = 1e26 / 1e6 = 1e20. For ease of explanation let's skip the discount calculation to the final line where paymentAmount = 1e20 / 10 ** (18 - 18) = 1e20.

So the final value of payment tokens we could be sending to purchase 1e18 TAP tokens is (of the order of) 1e20 payment tokens. Given both tokens are of the same underlying USD value, we're overpaying for the TAP tokens by 100x. Of course, this multiplier changes depending on difference in oracle decimals.

Right now most oracles use 8 decimals when pricing against the US Dollar, so it's fair to assume the Seer.sol will use 8 decimals to reflect the underlying oracles, however Tapioca should be built to work gracefully in any decimal environment as market conditions change over the next 5-10 years.

Tools Used

Manual review

Recommended Mitigation Steps

The number of decimals used by the oracles should be included in the payment token amount calculations when exercising options. Below is a diff to the TapiocaOptionBroker.sol contract, where the same changes should also be applied to the AirdropBroker.sol contract:

diff --git a/contracts/options/TapiocaOptionBroker.sol b/contracts/options/TapiocaOptionBroker.sol
index f359ff7..256be64 100644
--- a/contracts/options/TapiocaOptionBroker.sol
+++ b/contracts/options/TapiocaOptionBroker.sol
@@ -524,7 +524,8 @@ contract TapiocaOptionBroker is Pausable, BoringOwnable, TWAML {
             otcAmountInUSD,
             paymentTokenValuation,
             discount,
-            _paymentToken.decimals()
+            _paymentToken.decimals(),
+            _paymentTokenOracle.oracle.decimals()
         );
 
         _paymentToken.transferFrom(
@@ -545,14 +546,15 @@ contract TapiocaOptionBroker is Pausable, BoringOwnable, TWAML {
         uint256 _otcAmountInUSD,
         uint256 _paymentTokenValuation,
         uint256 _discount,
-        uint256 _paymentTokenDecimals
+        uint256 _paymentTokenDecimals,
+        uint256 _paymentTokenOracleDecimals
     ) internal pure returns (uint256 paymentAmount) {
         // Calculate payment amount
         uint256 rawPaymentAmount = _otcAmountInUSD / _paymentTokenValuation;
         paymentAmount =
             rawPaymentAmount -
             muldiv(rawPaymentAmount, _discount, 100e4); // 1e4 is discount decimals, 100 is discount percentage
-        paymentAmount = paymentAmount / (10 ** (18 - _paymentTokenDecimals));
+        paymentAmount = paymentAmount * (10 ** _paymentTokenOracleDecimals) / (10 ** ((18 - _paymentTokenDecimals) + tapOracle.decimals()));
     }
 
     /// @notice Emit TAP to the gauges equitably

Assessed type

Oracle

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L232-L236 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L529-L534

Vulnerability details

Impact

As the owner of all the singularity markets, the Penrose contract has the ability to withdraw all the market fees and send these fees to the fee distributor contract. The entrypoint for this functionality is withdrawAllMarketFees.

Since the method is public a malicious user can call this method with a combination of the worst swapper and a swapData_.minAssetAmount of 0. This is particularly important during periods where there might be a bank run or low liquidity in a trading pool (e.g. UniswapV2) since the exchange rate can be significantly impacted. It is possible that the markets could have accumulated a significant number of fees that a malicious user can then wipe out by withdrawing the fees with unlimited slippage tolerance.

Proof of Concept

As you can see, the withdrawAllMarketFees method is callable by anyone and allows the user to specify swapData_:

function withdrawAllMarketFees(
        IMarket[] calldata markets_,
        ISwapper[] calldata swappers_,
        IPenrose.SwapData[] calldata swapData_
    ) public notPaused {

Where swapData_ is:

    struct SwapData {
        uint256 minAssetAmount;
    }

The withdrawAllMarketFees method calls _depositFeesToYieldBox for every market specified, where the swapper contract is then called with the minAssetAmount value passed in by the caller:

            (amount, ) = swapper.swap(
                swapData,
                dexData.minAssetAmount,
                feeTo,
                ""
            );

You can easily see how any malicious user could specify a minAssetAmount value of 0 to withdraw accumulated fees at a significant loss to the protocol. If the protocol were acting in their best interest they would only withdraw the protocol fees when swapping in DEXs was at a favourable slippage. This can't be guaranteed with the current code.

Tools Used

Manual review

Recommended Mitigation Steps

I can see the appeal for having the withdrawAllMarketFees callable by anyone. Therefore my suggestion would be remove the swapData_.minAssetAmount argument from the function call and instead use a maxSlippageTolerance parameter that is set by the Penrose owner as a state variable in the constructor and is modifiable in a dedicated onlyOwner method. This maxSlippageTolerance parameter can calculate the minAmountOut on the fly based on the number of shares being withdrawn when the swap is performed by the swapper contract.

Usually you want the slippage to be very small, but this way the admin has the option to increase the slippage tolerance in turbulent market scenarios if they are willing to accept a lower amount out from an DEX swap.

Assessed type

Uniswap