sherlock-audit / 2023-04-blueberry-judging

8 stars 5 forks source link

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

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

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
        );
Ch-301 commented 1 year ago

Escalate for 10 USDC

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

sherlock-admin commented 1 year ago

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.

hrishibhat commented 1 year ago

Escalation accepted

Valid medium This is a valid issue

sherlock-admin commented 1 year ago

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.
Gornutz commented 1 year ago

https://github.com/Blueberryfi/blueberry-core/commit/f6d1981f8d4670c79dfba176247ba7371999079b#diff-608b47c5c74c768d52e4449c12d1a3a9ce5d8ee01cce2e65df2e64996df651cb

IAm0x52 commented 1 year ago

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