Closed sherlock-admin3 closed 5 months ago
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.
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 theskim
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.
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.
Planning to accept the escalation and remove the duplication with #141, but duplicate with #139.
Result: Medium Duplicate of #139
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.However, it calls
_repay
with theskim
parameter set to false, resulting in the user's funds being pulled during theSGLLendingCommon._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.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 trueDuplicate of #139