sherlock-audit / 2023-04-blueberry-judging

8 stars 5 forks source link

Ch_301 - No one can open a Short/Long position on `ShortLongSpell.sol` #113

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Ch_301

medium

No one can open a Short/Long position on ShortLongSpell.sol

Summary

The SoftVault receive uToken and deposits them into Compound protocol to earn a passive yield In order to open a position in the Short/Long SPELL it will send an amount of uToken from Short/Long SPELL to SoftVault

Vulnerability Detail

the user invokes openPosition() to open Short/Long Position. openPosition() has this check

    function openPosition(
        OpenPosParam calldata param,
        Utils.MegaSwapSellData calldata swapData
    )
        external
        existingStrategy(param.strategyId)
        existingCollateral(param.strategyId, param.collToken)
    {
        Strategy memory strategy = strategies[param.strategyId];
        if (
            address(ISoftVault(strategy.vault).uToken()) != param.borrowToken ||
            swapData.fromToken != param.borrowToken
        ) revert Errors.INCORRECT_LP(param.borrowToken);

        // 1-3 Swap to strategy underlying token, deposit to softvault
        _deposit(param, swapData);

so it makes sure the uToken == borrowToken and swapData.fromToken == param.borrowToken otherways it will revert in the first place Go further to deposit() to perform a swap has no sense

PSwapLib.megaSwap(augustusSwapper, tokenTransferProxy, swapData);

it is trying to swap the param.borrowToken to uToken, we know uToken == borrowToken (two sides of the same coin) so we don't need to do any swap here

Impact

Code Snippet

Tool used

Manual Review

Recommendation

no need to perform a swap just transfer the borrowToken to the SoftVault

Ch-301 commented 1 year ago

Escalate for 10 USDC

There is a clear inconsistency between this check

        if (
            address(ISoftVault(strategy.vault).uToken()) != param.borrowToken ||
            swapData.fromToken != param.borrowToken
        ) revert Errors.INCORRECT_LP(param.borrowToken);

and this swap

PSwapLib.megaSwap(augustusSwapper, tokenTransferProxy, swapData);

e.g.this flow from the Sponsor

You have isolated collateral asset of ETH (1x)
You have a borrow asset of USDC (3x)
You swap the USDC for ETH

Net exposure is 
ETH x1 yielding some interest
USDC x3 paying the interest 
ETH (looking for price appreciation)

in this example the uToken == ETH and param.borrowToke == USDC so the first check will revert this one

        if (
            address(ISoftVault(strategy.vault).uToken()) != param.borrowToken ||
            swapData.fromToken != param.borrowToken
        ) revert Errors.INCORRECT_LP(param.borrowToken);
sherlock-admin commented 1 year ago

Escalate for 10 USDC

There is a clear inconsistency between this check

        if (
            address(ISoftVault(strategy.vault).uToken()) != param.borrowToken ||
            swapData.fromToken != param.borrowToken
        ) revert Errors.INCORRECT_LP(param.borrowToken);

and this swap

PSwapLib.megaSwap(augustusSwapper, tokenTransferProxy, swapData);

e.g.this flow from the Sponsor

You have isolated collateral asset of ETH (1x)
You have a borrow asset of USDC (3x)
You swap the USDC for ETH

Net exposure is 
ETH x1 yielding some interest
USDC x3 paying the interest 
ETH (looking for price appreciation)

in this example the uToken == ETH and param.borrowToke == USDC so the first check will revert this one

        if (
            address(ISoftVault(strategy.vault).uToken()) != param.borrowToken ||
            swapData.fromToken != param.borrowToken
        ) revert Errors.INCORRECT_LP(param.borrowToken);

You've created a valid escalation for 10 USDC!

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.

hrishibhat commented 1 year ago

Escalation rejected

The underlying issue of uToken == borrowToken not requiring a swap is already a part of a larger issue whose impact is already mentioned in issues #138 and #133. Hence this issue cannot be considered as a separate issue. Keeping this issue closed.

sherlock-admin commented 1 year ago

Escalation rejected

The underlying issue of uToken == borrowToken not requiring a swap is already a part of a larger issue whose impact is already mentioned in issues #138 and #133. Hence this issue cannot be considered as a separate issue. Keeping this issue closed.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.