sherlock-audit / 2023-04-blueberry-judging

8 stars 5 forks source link

Ch_301 - `ShortLongSpell.openPosition()` should not refund token #119

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Ch_301

high

ShortLongSpell.openPosition() should not refund token

Summary

Vulnerability Detail

this part of ShortLongSpell.openPosition()

        // 4. Put collateral -
        {
            IBank.Position memory pos = bank.getCurrentPositionInfo();
            address posCollToken = pos.collToken;
            uint256 collSize = pos.collateralSize;
            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);
            }
        }

is asking werc20 to burn the wrapped token (erc1155) to receive back the burnToken which is the SoftVault token. the received amount should stay on the SPELL to transfer by _doPutCollateral to werc20 again (the old + the new vault token ) in the next step.

But it transfers them to the user by invoking _doRefund(burnToken); with out updating pos.underlyingVaultShare

Impact

Code Snippet

Tool used

Manual Review

Recommendation

        // 4. Put collateral -
        {
            IBank.Position memory pos = bank.getCurrentPositionInfo();
            address posCollToken = pos.collToken;
            uint256 collSize = pos.collateralSize;
            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);
            }
        }

Duplicate of #135