duc - SGLLeverage.sellCollateral calls _repay with the skim parameter set to false.

[sherlock-admin3]

duc

high

SGLLeverage.sellCollateral calls _repay with the skim parameter set to false.

Summary

See vulnerability detail.

Vulnerability Detail

In SGLLeverage.sellCollateral function, after collecting assets and depositing them into YieldBox, this function attempt to repay user's loan by using the obtain asset shares.

amountOut = leverageExecutor.getAsset(
    assetId, address(collateral), address(asset), leverageAmount, calldata_.from, calldata_.data
);
uint256 shareOut = yieldBox.toShare(assetId, amountOut, false);

address(asset).safeApprove(address(yieldBox), type(uint256).max);
yieldBox.depositAsset(assetId, address(this), address(this), 0, shareOut);

...

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

However, it calls _repay with the skim parameter set to false, resulting in the user's funds being pulled during the SGLLendingCommon._repay() function. This means that even though the necessary asset shares were collected by the contract, it still attempts to mistakenly pull from the user for repayment.

function _repay(address from, address to, bool skim, uint256 part) internal returns (uint256 amount) {
    ...
    _addTokens(from, to, assetId, share, uint256(totalShare), skim);
    ...
}

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();
        }
    }
}

Impact

If users have enough allowance and balance for Singularity market, they will experience a loss of funds when using SGLLeverage.sellCollateral.

Code Snippet

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

Tool used

Manual Review

Recommendation

Should call _repay with skim parameter set to true

Duplicate of #139

[huuducsc]

Escalate
I believe this issue is not a duplicate of #141, since they are very different. Issue #141 represents that the allowance is double-spent in the sellCollateral function. However, this issue describes the vulnerability of calling _repay with the skim parameter set to false, which will pull funds from the user again even when the target funds are already held in this contract. Therefore, if users give an allowance for the Singularity market, users will incur unexpected losses when calling sellCollateral. Thus, this issue should be a high.

[sherlock-admin2]

Escalate
I believe this issue is not a duplicate of #141, since they are very different. Issue #141 represents that the allowance is double-spent in the sellCollateral function. However, this issue describes the vulnerability of calling _repay with the skim parameter set to false, which will pull funds from the user again even when the target funds are already held in this contract. Therefore, if users give an allowance for the Singularity market, users will incur unexpected losses when calling sellCollateral. Thus, this issue should be a high.

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]

I tend to agree in part with the escalation. This is not a duplicate of #141, but a duplicate of #139 (and #57, respectively). The root of the problem is the same, the user's funds are mistakenly pulled twice. If we look at issue #57 we can even see that everything is the same, only the functions are different. In one case it is buyCollateral() and in the other sellCollateral().

If I have to make an analogy for why they should be duplicates, imagine that we have buy() and sell() functions. If they don't have slippage protection then we don't write two separate reports but merge them into one issue with a root slippage protection. I think the analogy here is the same as to why #60 is not a duplicate of #141, but a duplicate of #139 and #57.

This is from the Sherlock documentation: "In case the same vulnerability appears across multiple places in different contracts, they can be considered duplicates."

I also think it should remain Medium because the loss is limited only to the allowance the user has.

[cvetanovv]

Planning to accept the escalation and remove the duplication with #141, but duplicate with #139.

[Evert0x]

Result:
Medium
Duplicate of #139

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• huuducsc: accepted