duc - The repaying action in `BBLeverage.sellCollateral` function pulls YieldBox shares of asset from wrong address

[sherlock-admin2]

duc

high

The repaying action in BBLeverage.sellCollateral function pulls YieldBox shares of asset from wrong address

Summary

The sellCollateral function is used to sell a user's collateral to obtain YieldBox shares of the asset and repay the user's loan. However, in the BBLeverage contract, it calls _repay with the from parameter set to the user, even though the asset shares have already been collected by this contract beforehand.

Vulnerability Detail

In BBLeverage.sellCollateral, the from variable (user) is used as the repayer address.

if (memoryData.shareOwed <= memoryData.shareOut) {
    _repay(from, from, memoryData.partOwed);
} else {
    //repay as much as we can
    uint256 partOut = totalBorrow.toBase(amountOut, false);
    _repay(from, from, partOut);
}

Therefore, asset shares of user will be pulled in BBLendingCommon._repay function.

function _repay(address from, address to, uint256 part) internal returns (uint256 amount) {
    ...
    // @dev amount includes the opening & accrued fees
    yieldBox.withdraw(assetId, from, address(this), amount, 0);
    ...

This is incorrect behavior since the necessary asset shares were already collected by the contract in the BBLeverage.sellCollateral function. The repayer address should be address(this) for _repay.

Impact

Mistakenly pulling user funds while the received asset shares remain stuck in the contract will result in losses for users who have sufficient allowance and balance when using the BBLeverage.sellCollateral functionality.

Code Snippet

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

Tool used

Manual Review

Recommendation

Should fix as following:

if (memoryData.shareOwed <= memoryData.shareOut) {
    _repay(address(this), from, memoryData.partOwed);
} else {
    //repay as much as we can
    uint256 partOut = totalBorrow.toBase(amountOut, false);
    _repay(address(this), from, partOut);
}

[cryptotechmaker]

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

[sherlock-admin4]

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

takarez commented:

the amount seem to be different, the first one is hardcoded zero and the second one is the amount; so i believe is intended behavior

[huuducsc]

Escalate I believe this issue is not a duplicate of #100 since they are very different in everything. This issue should be a valid high issue, and here are the reasons: After issue #42 and its fix, it becomes apparent that the sellCollateral function attempts to deposit asset tokens into the YieldBox and then utilize the received shares to repay users. However, even though the contract receives asset shares, this function still employs the from to pull asset shares when calling _repay. This behavior results in the extraction of additional funds from users, causing unexpected losses, while the unused funds (received asset shares from depositing into the YieldBox) become stuck within this contract. You can review the commit fix of #42: https://github.com/Tapioca-DAO/Tapioca-bar/pull/352. This issue pertains to a different problem involving the incorrect calling of _repay. You should refer to my recommendation to understand.

[sherlock-admin2]

Escalate I believe this issue is not a duplicate of #100 since they are very different in everything. This issue should be a valid high issue, and here are the reasons: After issue #42 and its fix, it becomes apparent that the sellCollateral function attempts to deposit asset tokens into the YieldBox and then utilize the received shares to repay users. However, even though the contract receives asset shares, this function still employs the from to pull asset shares when calling _repay. This behavior results in the extraction of additional funds from users, causing unexpected losses, while the unused funds (received asset shares from depositing into the YieldBox) become stuck within this contract. You can review the commit fix of #42: https://github.com/Tapioca-DAO/Tapioca-bar/pull/352. This issue pertains to a different problem involving the incorrect calling of _repay. You should refer to my recommendation to understand.

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.

[nevillehuang]

I believe this issue is correctly duplicated, in fact #100 is a more comprehensive report compared to this issue.

[huuducsc]

@nevillehuang I believe the 3 problems mentioned in #100 are not related to this issue. #100 represents incorrect uses of getCollateral(), safeApprove(), and depositAsset() functions in buyCollateral() function, which render the buyCollateral() function unable to work. It only has a medium impact. However, this issue represents the incorrect call of _repay() function in the sellCollateral() function when it attempts to pull funds from the user again. This is a different problem in a different function, and it has a high severity since users will be at risk of losing funds whenever they use sellCollateral() function. Could you please recheck it.

[cvetanovv]

I agree with the escalation not being a duplicate of #100, but disagree that it should be High severity. The loss of funds is limited to the allowance and balance the user has. That's why I think it should stay Medium.

[nevillehuang]

@cvetanovv @cryptotechmaker @huuducsc This seems like a duplicate of #141, might want to double check if a separate fix is required. Maybe a PoC can easily confirm this? I think #101 is a variation of this issue too and could also be duplicated

Might also want to consider this comments here by @maarcweiss on duplication status

[cvetanovv]

I decided to accept escalation to be a valid High but will duplicate it with #141. The function is the same and the impact is the same. In this issue, the function pulls double funds, in the other double allowances. However, we need to check if the fix will fix both problems.

[huuducsc]

@cvetanovv I don't think this issue is similar to #141 since #141 only represents the way allowance of the user for sender is spent twice. There is no higher impact in that report, and the actual root cause is different from this issue. This issue describes that _repay will pull funds from the user again because of an incorrect from address, and it doesn't mention allowance spending. The impact is different because issue #141 doesn't result in a direct loss of funds for the user; it only requires more allowance for the sellCollateral function. Could you please recheck?

[nevillehuang]

@huuducsc @hyh Can you guys provide a PoC to prove #141 and #59 are not duplicates? They are way too similar for me to verify, and I just want to double confirm one issue doesn't lead to another.

CC @maarcweiss @cryptotechmaker @0xRektora

[cvetanovv]

My final decision is to accept the escalation and this issue will not have a duplicate and will remain unique, but will also remain Medium because of the new approval system.

[huuducsc]

@cvetanovv The new permit system is an additional functionality, it shouldn't be a reason to consider the likelihood of this issue as low. There are no restrictions regarding approval from users to the market before utilization, so it's expected from users. Therefore, I believe the likelihood should still be high, and this issue deserves a high severity

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits: https://github.com/Tapioca-DAO/Tapioca-bar/pull/386

[cvetanovv]

My final decision is to accept the escalation this issue to be unique but remain Medium.

[nevillehuang]

@huuducsc Just to check is issue #101 a duplicate of your issue? I don't want to misduplicate issues here

[Evert0x]

Result: Medium Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• huuducsc: accepted