hyh - BBLeverage's and SGLLeverage's `buyCollateral()` remove the required funds from the target twice

[sherlock-admin4]

hyh

high

BBLeverage's and SGLLeverage's buyCollateral() remove the required funds from the target twice

Summary

The collateral purchase proceedings of leverage buy operation aren't returned to the user, but kept with the contract instead, so the funds are being requested from the user twice, first in a usual borrow and buy workflow (user now had a liability of collateralShare size), then once again via _addCollateral (user has transferred collateralShare directly in addition to that).

Vulnerability Detail

Both buyCollateral() functions are now broken this way, removing twice the value from the user. The accounting is being updated accordingly to reflect only one instance, so the funds are lost for the user.

I.e. after each of the operations user borrowed the collateralShare worth of asset and then additionally to that supplied it directly via _addCollateral. As only one collateral addition is recorded, the loss for them is collateralShare of collateral. Since the accounting is updated it will not be possible to manually fix the situation in production, so the redeployment would be due.

Impact

It's an unconditional user loss each time the operations are used. There are no prerequisites, so the probability of it is high. Funds loss impact has high severity.

Likelihood: High + Impact: High = Severity: Critical/High.

Code Snippet

First calldata_.borrowAmount is being borrowed for calldata_.from:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/bigBang/BBLeverage.sol#L82-L102

        {
            (, uint256 borrowShare) = _borrow(
>>              calldata_.from,
                address(this),
                calldata_.borrowAmount,
                _computeVariableOpeningFee(calldata_.borrowAmount)
            );
            (memoryData.borrowShareToAmount,) =
                yieldBox.withdraw(assetId, address(this), address(leverageExecutor), 0, borrowShare);
        }
        {
            amountOut = leverageExecutor.getCollateral(
                collateralId,
                address(asset),
                address(collateral),
                memoryData.supplyShareToAmount + memoryData.borrowShareToAmount,
                calldata_.from,
                calldata_.data
            );
        }
        uint256 collateralShare = yieldBox.toShare(collateralId, amountOut, false);

And while borrow proceedings are being held in the contract, the same funds are being requested again from calldata_.from in the collateral form:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/bigBang/BBLeverage.sol#L102-L110

        uint256 collateralShare = yieldBox.toShare(collateralId, amountOut, false);
        address(asset).safeApprove(address(yieldBox), type(uint256).max);
        yieldBox.depositAsset(collateralId, address(this), address(this), 0, collateralShare); // TODO Check for rounding attack?
        address(asset).safeApprove(address(yieldBox), 0);

        if (collateralShare == 0) revert CollateralShareNotValid();
        _allowedBorrow(calldata_.from, collateralShare);
>>      _addCollateral(calldata_.from, calldata_.from, false, 0, collateralShare);
    }

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/bigBang/BBLendingCommon.sol#L40-L49

    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, collateralId, share, oldTotalCollateralShare, skim);
        emit LogAddCollateral(skim ? address(yieldBox) : from, to, share);
    }

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/bigBang/BBCommon.sol#L128-L138

    function _addTokens(address from, uint256 _tokenId, uint256 share, uint256 total, bool skim) internal {
        if (skim) {
            require(share <= yieldBox.balanceOf(address(this), _tokenId) - total, "BB: too much");
        } else {
            // yieldBox.transfer(from, address(this), _tokenId, share);
>>          bool isErr = pearlmit.transferFromERC1155(from, address(this), address(yieldBox), _tokenId, share);
            if (isErr) {
                revert TransferFailed();
            }
        }
    }

This last step of requesting collateralShare that is already with BBLeverage/SGLLeverage contract doesn't make sense for buyCollateral(), being a leftover from the legacy logic (when the collateral funds were transferred to a user, while now they aren't).

SGLLeverage situation is the same, first borrow calldata_.borrowAmount:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLLeverage.sol#L73-L85

>>      (, uint256 borrowShare) = _borrow(calldata_.from, address(this), calldata_.borrowAmount);

        (uint256 borrowShareToAmount,) =
            yieldBox.withdraw(assetId, address(this), address(leverageExecutor), 0, borrowShare);
        amountOut = leverageExecutor.getCollateral(
            collateralId,
            address(asset),
            address(collateral),
            supplyShareToAmount + borrowShareToAmount,
            calldata_.from,
            calldata_.data
        );
        uint256 collateralShare = yieldBox.toShare(collateralId, amountOut, false);

Then additionally request collateralShare from calldata_.from:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLLeverage.sol#L85-L93

        uint256 collateralShare = yieldBox.toShare(collateralId, amountOut, false);
        if (collateralShare == 0) revert CollateralShareNotValid();
        address(asset).safeApprove(address(yieldBox), type(uint256).max);
        yieldBox.depositAsset(collateralId, address(this), address(this), 0, collateralShare); // TODO Check for rounding attack?
        address(asset).safeApprove(address(yieldBox), 0);

        _allowedBorrow(calldata_.from, collateralShare);
>>      _addCollateral(calldata_.from, calldata_.from, false, 0, collateralShare);
    }

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLLendingCommon.sol#L39-L50

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

>>      _addTokens(from, to, collateralId, share, oldTotalCollateralShare, skim);

        emit LogAddCollateral(skim ? address(yieldBox) : from, to, share);
    }

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLCommon.sol#L165-L177

    function _addTokens(address from, address, uint256 _assetId, uint256 share, uint256 total, bool skim) internal {
        if (skim) {
            if (share > yieldBox.balanceOf(address(this), _assetId) - total) {
                revert TooMuch();
            }
        } else {
            // yieldBox.transfer(from, address(this), _assetId, share);
>>          bool isErr = pearlmit.transferFromERC1155(from, address(this), address(yieldBox), _assetId, share);
            if (isErr) {
                revert TransferFailed();
            }
        }
    }

The root cause is that leverageExecutor's logic has changed: previously getCollateral() put the funds to from, while now the funds are directly transferred to the BBLeverage/SGLLeverage contract, which is msg.sender for leverageExecutor, e.g.:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/leverage/SimpleLeverageExecutor.sol#L38-L47

    function getCollateral(
        ....
    ) external payable override returns (uint256 collateralAmountOut) {
        // Should be called only by approved SGL/BB markets.
        if (!cluster.isWhitelisted(0, msg.sender)) revert SenderNotValid();
>>      return _swapAndTransferToSender(true, assetAddress, collateralAddress, assetAmountIn, swapperData);
    }

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/leverage/BaseLeverageExecutor.sol#L129-L159

    function _swapAndTransferToSender(
        bool sendBack,
        ...
    ) internal returns (uint256 amountOut) {
        ...

        // If the tokenOut is a tOFT, wrap it. Handles ETH and ERC20.
        // If `sendBack` is true, wrap the `amountOut to` the sender. else, wrap it to this contract.
        if (swapData.toftInfo.isTokenOutToft) {
            _handleToftWrapToSender(sendBack, tokenOut, amountOut);
        } else if (sendBack == true) {
            // If the token wasn't sent by the wrap OP, send it as a transfer.
>>          IERC20(tokenOut).safeTransfer(msg.sender, amountOut);
        }
    }

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/leverage/AssetTotsDaiLeverageExecutor.sol#L38-L65

    function getCollateral(address assetAddress, address collateralAddress, uint256 assetAmountIn, bytes calldata data)
        ...
    {
        ...

        // Wrap into tsDai to sender
        sDaiAddress.safeApprove(collateralAddress, collateralAmountOut);
>>      ITOFT(collateralAddress).wrap(address(this), msg.sender, collateralAmountOut);
        sDaiAddress.safeApprove(collateralAddress, 0);
    }

Tool used

Manual Review

Recommendation

Consider adding a flag to _addCollateral(), indicating that the funds were already transferred to the contract and only accounting update is needed, e.g.:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/bigBang/BBLendingCommon.sol#L40-L49

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

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLLendingCommon.sol#L39-L50

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

-       _addTokens(from, to, collateralId, share, oldTotalCollateralShare, skim);
+       if (addTokens) _addTokens(from, to, collateralId, share, oldTotalCollateralShare, skim);

        emit LogAddCollateral(skim ? address(yieldBox) : from, to, share);
    }

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/bigBang/BBLeverage.sol#L102-L110

        uint256 collateralShare = yieldBox.toShare(collateralId, amountOut, false);
        address(asset).safeApprove(address(yieldBox), type(uint256).max);
        yieldBox.depositAsset(collateralId, address(this), address(this), 0, collateralShare); // TODO Check for rounding attack?
        address(asset).safeApprove(address(yieldBox), 0);

        if (collateralShare == 0) revert CollateralShareNotValid();
        _allowedBorrow(calldata_.from, collateralShare);
-       _addCollateral(calldata_.from, calldata_.from, false, 0, collateralShare);
+       _addCollateral(calldata_.from, calldata_.from, false, 0, collateralShare, false);
    }

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLLeverage.sol#L85-L93

        uint256 collateralShare = yieldBox.toShare(collateralId, amountOut, false);
        if (collateralShare == 0) revert CollateralShareNotValid();
        address(asset).safeApprove(address(yieldBox), type(uint256).max);
        yieldBox.depositAsset(collateralId, address(this), address(this), 0, collateralShare); // TODO Check for rounding attack?
        address(asset).safeApprove(address(yieldBox), 0);

        _allowedBorrow(calldata_.from, collateralShare);
-       _addCollateral(calldata_.from, calldata_.from, false, 0, collateralShare);
+       _addCollateral(calldata_.from, calldata_.from, false, 0, collateralShare, false);
    }

All other _addCollateral() instances should be updated to go with addTokens == true.

[cryptotechmaker]

Duplicate of https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/57

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

WangAudit commented:

tbh; after looking at _borrow function; I didn't see how funds are requested by the user there

[nevillehuang]

@cryptotechmaker Seems very similar to #141 and #60, so could be duplicates, preconditioned that users must have supplied sufficient allowance. Seems borderline high severity but could be medium. What do you think?

[sherlock-admin4]

The protocol team fixed this issue in PR/commit https://github.com/Tapioca-DAO/Tapioca-bar/pull/367.

[huuducsc]

Escalate The severity of this issue should be high. Users having allowances for the BigBang or Singularity markets is a very common situation, not a kind of external condition or specific state. It's similar to an allowance attack, which is critical for any protocol.

[sherlock-admin2]

Escalate The severity of this issue should be high. Users having allowances for the BigBang or Singularity markets is a very common situation, not a kind of external condition or specific state. It's similar to an allowance attack, which is critical for any protocol.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[cvetanovv]

The severity of the potential duplicated issue #60 applies here. In my opinion, it should remain Medium severity because the loss is limited only to the allowance the user has. According to Sherlock's documentation to be High severity: "Definite loss of funds without (extensive) limitations of external conditions."

[huuducsc]

I believe this issue deserves a high severity because approving for the market is a very common behavior among users, which is expected in the workflow of protocol and shouldn't be considered as an external condition. Regarding issue #87, although it requires a difference in decimals of tokens, it is still considered to have a high severity since the sponsor confirmed that they may use different decimals of tokens in the future.

[cvetanovv]

That a user has a maximum allowance is common but not always the case and depends on the user. Losses are limited only to the allowance and funds the user has.

For this reason, I planned to reject the escalation and leave the issue as it is.

[huuducsc]

@cvetanovv The statement that "the loss is limited to the allowance and funds the user has" is not a feasible reason to downgrade this issue, according to the criteria of Sherlock:

Approval for this own protocol (markets) isn't considered external conditions since most users will approve when using this protocol, and it's commonly expected behavior. In history, there have been several approval attacks which caused huge losses for users and protocols, such as the Old Dolomite exploit.

Additionally, #87 is still a high issue even when it requires different decimals tokens, which depends on the admin. Therefore, I believe this issue and other issues, which can cause loss from approval for markets, truly deserve high severity.

[cvetanovv]

Hey @huuducsc, I'm not downgrading this issue as you say. This issue is judged by the Protocol and Lead Judge as Medium with which I agree for now.

@nevillehuang What do you think? I have to admit it is borderline High/Medium. What makes it Medium in my opinion is the limitation to allowance and the funds it has.

On the other hand, unsuspecting users can give a lot of allowances and have a lot of funds and lose a lot of funds because of this bug. Even hundreds of thousands. So I think there is a reason to upgrade it to High.

[huuducsc]

@cvetanovv

What makes it Medium in my opinion is the limitation to allowance and the funds it has.

When users are not aware of this vulnerability, they may approve millions or infinite funds to the protocol after its launch. As I mentioned, most users will routinely approve for markets when using this protocol because it's expected behavior from both protocol and users. The limitation of possible lost funds depends on users, but it will still be very huge on a regular basis. Therefore, there is no reason to consider this issue as medium severity, based on Sherlock's criteria, since it doesn't require any external conditions

[nevillehuang]

@maarcweiss @cryptotechmaker Could you let us know why you believe this issue is medium severity?

[0xRektora]

I also think It's borderline a high/medium because at the end it's gonna depend on the approval. We integrated a new approval system on Tapioca right before the audit start that takes in a permit for each transaction, there's no infinite approvals. The values are equal to the amounts needed and can't be updated by the web3 wallet because it's a permit.

If we take this into context I'd see it as a medium and not a high because the likelihood would be low.

[cvetanovv]

After this additional information from @0xRektora I decided to keep my original decision to reject the escalation and this issue will remain Medium.

[huuducsc]

We integrated a new approval system on Tapioca right before the audit start that takes in a permit for each transaction

This statement wasn't mentioned in the contest's docs, so I believe it is out of scope and shouldn't be taken into context. Additionally, I believe this cannot prevent users from approving for markets, since the protocol didn't have any rules or docs to restrict user approval, so the likelihood should be high.

[cvetanovv]

I agree with the @huuducsc escalation and will upgrade the severity to High.

An unsuspecting user may have a lot of permission and since funds will always be double removed then High severity is appropriate.

[0xadrii]

In response to Duc's comment, I believe that Rektora is referring to the Pearlmit approval system, which could clearly be found in the contracts in-scope, and was also explicitly mentioned by the sponsors as one of the breaking changes for this audit in this comment on Sherlock's official tapioca discord channel. Just want to clarify that this approval system must not be considered out of scope given that it is one of the essential additions for this audit

[cvetanovv]

If we consider the new approval system then things change. So my last decision is to take into consideration sponsor and @0xadrii comment and reject the escalation.

[huuducsc]

@cvetanovv

Just want to clarify that this approval system must not be considered out of scope given that it is one of the essential additions for this audit

I agree with @0xadrii that the permit system is not out of scope, but he just wanted to clarify it and he didn't point out any consideration of this issue as a medium. My demonstration for considering it a high severity is that there is no statement in the docs which restricts the approval from users to protocol. The protocol didn't tell users to only use the permit functionality, so I believe the likelihood of this issue is still high since it doesn't need any external condition.

[0xadrii]

Well, I did not mention it but my comment was actually meant to support the sponsor's comment and make it clear that the permit integration must be considered for this audit. The sponsor's comment shows how likelihood is clearly low and hence medium severity is justified

[huuducsc]

@0xadrii I mean the sponsor only mentioned a functionality of permit and didn't show any evidence to consider the likelihood as low. I still don't know why approval for the market should be considered as low likelihood since there are no restrictions for it in the protocol's docs

[cvetanovv]

My final decision is to reject the escalation and this issue will remain Medium.

[Evert0x]

Result: Medium Has Duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• huuducsc: rejected