Skip to content
This repository has been archived by the owner on Nov 5, 2023. It is now read-only.

Ch_301 - M-03 wrong token address on ShortLongSpell.sol #114

Open
sherlock-admin opened this issue Apr 30, 2023 · 6 comments
Open

Ch_301 - M-03 wrong token address on ShortLongSpell.sol #114

sherlock-admin opened this issue Apr 30, 2023 · 6 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Medium A valid Medium severity issue Reward A payout will be made for this issue Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin
Copy link
Contributor

Ch_301

medium

M-03 wrong token address on ShortLongSpell.sol

Summary

Vulnerability Detail

ShortLongSpell.openPosition() send uToken to SoftVault then deposit it into the Compound protocol to earn a passive yield. In return, SPELL receives share tokes of SoftVault address(strategy.vault)

WERC20.sol should receive address(strategy.vault) token, but the logic of ShortLongSpell.sol subcall (WERC20.sol) wrapper.burn() and pass the uToken address (please check the Code Snippet part) instead of strategy.vault address

Impact

Short/Long Spell will never work

Code Snippet

1- https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/ShortLongSpell.sol#L128-L141

            address burnToken = address(ISoftVault(strategy.vault).uToken());
            if (collSize > 0) {
                if (posCollToken != address(wrapper))
                    revert Errors.INCORRECT_COLTOKEN(posCollToken);
                bank.takeCollateral(collSize);
                wrapper.burn(burnToken, collSize);
                _doRefund(burnToken);
            }

2- https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/ShortLongSpell.sol#L229-L234

        // 1. Take out collateral
        bank.takeCollateral(param.amountPosRemove);
        werc20.burn(
            address(ISoftVault(strategy.vault).uToken()),
            param.amountPosRemove
        );

Tool used

Manual Review

Recommendation

1-

-            address burnToken = address(ISoftVault(strategy.vault).uToken());
+            address burnToken = strategy.vault;
            if (collSize > 0) {
                if (posCollToken != address(wrapper))
                    revert Errors.INCORRECT_COLTOKEN(posCollToken);
                bank.takeCollateral(collSize);
                wrapper.burn(burnToken, collSize);
                _doRefund(burnToken);
            }

2-

        // 1. Take out collateral
        bank.takeCollateral(param.amountPosRemove);
        werc20.burn(
-            address(ISoftVault(strategy.vault).uToken()),
+            strategy.vault,
            param.amountPosRemove
        );
@github-actions github-actions bot closed this as completed May 3, 2023
@github-actions github-actions bot added the Excluded Excluded by the judge without consulting the protocol or the senior label May 3, 2023
@sherlock-admin sherlock-admin added the Non-Reward This issue will not receive a payout label May 20, 2023
@Ch-301
Copy link

Ch-301 commented May 21, 2023

Escalate for 10 USDC

The same reason as #116 but in a different implementation and it needs another solution

@sherlock-admin
Copy link
Contributor Author

Escalate for 10 USDC

The same reason as #116 but in a different implementation and it needs another solution

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.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label May 21, 2023
@hrishibhat hrishibhat added the Medium A valid Medium severity issue label Jun 1, 2023
@hrishibhat
Copy link

hrishibhat commented Jun 1, 2023

Escalation accepted

Valid medium
This is a valid issue

@sherlock-admin
Copy link
Contributor Author

sherlock-admin commented Jun 1, 2023

Escalation accepted

Valid medium
This is a valid issue

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

@hrishibhat hrishibhat reopened this Jun 1, 2023
@sherlock-admin sherlock-admin added Reward A payout will be made for this issue Escalation Resolved This issue's escalations have been approved/rejected and removed Non-Reward This issue will not receive a payout Escalated This issue contains a pending escalation Excluded Excluded by the judge without consulting the protocol or the senior labels Jun 1, 2023
@Gornutz Gornutz added the Will Fix The sponsor confirmed this issue will be fixed label Jun 6, 2023
@IAm0x52
Copy link
Collaborator

IAm0x52 commented Jun 16, 2023

Fix looks good. Contract now correctly burns vault rather than vault.uToken

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Escalation Resolved This issue's escalations have been approved/rejected Medium A valid Medium severity issue Reward A payout will be made for this issue Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

5 participants