search

sherlock-audit / 2024-02-tapioca-judging

2 stars 2 forks source link

duc - Using wrong token for the approve action in the `buyCollateral` function #56

Closed sherlock-admin2 closed 5 months ago

sherlock-admin2 commented 5 months ago

duc

medium

Using wrong token for the approve action in the buyCollateral function

Summary

The buyCollateral() function in BBLeverage and SGLLeverage is used to borrow assets and swap to add collateral into the market. After collecting collateral tokens, it attempts to deposit collateral tokens into YieldBox to obtain YieldBox's shares of collateral, but incorrectly approves the asset token.

Vulnerability Detail

BBLeverage.buyCollateral() approves the asset token for YieldBox but deposits the collateralId token for YieldBox.

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

The same vulnerability exists in SGLLeverage.buyCollateral().

Impact

buyCollateral won't work because it will revert in YieldBox due to lacking allowance (broke core functionality).

Code Snippet

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/bigBang/BBLeverage.sol#L103-L105 https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLLeverage.sol#L87-L89

Tool used

Manual Review

Recommendation

Should approve the collateral token.

Duplicate of #100

sherlock-admin3 commented 5 months ago

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

takarez commented:

thats the asset Id

huuducsc commented 5 months ago

Escalate I believe this issue is not a duplicate of #42 since this issue describes a different vulnerability in a different function. Regarding this issue, in the buyCollateral function, depositing collateral tokens into YieldBox is correct, but the approval is wrong since it approves asset tokens instead of collateral tokens for YieldBox. However, issue #42 represents a different situation where the function sellCollateral uses the wrong tokens for depositing. Additionally, the fix for #42 didn't address this issue, and this one hasn't been fixed yet.

sherlock-admin2 commented 5 months ago

Escalate I believe this issue is not a duplicate of #42 since this issue describes a different vulnerability in a different function. Regarding this issue, in the buyCollateral function, depositing collateral tokens into YieldBox is correct, but the approval is wrong since it approves asset tokens instead of collateral tokens for YieldBox. However, issue #42 represents a different situation where the function sellCollateral uses the wrong tokens for depositing. Additionally, the fix for #42 didn't address this issue, and this one hasn't been fixed yet.

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 commented 5 months ago

This issue is not a duplicate of #42 but is a duplicate of #100.

100 contains several problems of buyCollateral() function, and we can duplicate them.

cvetanovv commented 5 months ago

Planning to accept the escalation as not dup of #42 and duplicate with #100.

Evert0x commented 4 months ago

Result: Medium Duplicate of #100

sherlock-admin3 commented 4 months ago

Escalations have been resolved successfully!

Escalation status: