ShortLongSpell would never work

Summary

The ShortLongSpell contract would never work well, due to bugs in the deposit functionality.

Vulnerability Detail

The existing bugs in the deposit functionality shows like this:

strTokenAmt calculation would always revert due to underflow
```
    uint256 strTokenAmt = _doBorrow(param.borrowToken, param.borrowAmount);

    IERC20Upgradeable swapToken = ISoftVault(strategy.vault).uToken();

    PSwapLib.megaSwap(augustusSwapper, tokenTransferProxy, swapData);
    strTokenAmt = swapToken.balanceOf(address(this)) - strTokenAmt;
```
The initial strTokenAmt is the balance of swapToken. Since swapToken is the fromToken for megaSwap, the remaining balance should be always less than the initial one. So it would always underflow if you try swapToken.balanceOf(address(this)) - strTokenAmt.
Wrong invariant for strTokenAmt and expectedAmount
```
    if (strTokenAmt < swapData.expectedAmount)
```
Even if the strTokenAmt is calculated correctly, strTokenAmt refers to the balance of swapToken, but expectedAmount refers to the balance of the finally swapped result token (e.g. USDT when swapping WETH > DAI > USDT). Direct comparison between different units.

Inconsistent collateral

    address vault = strategies[param.strategyId].vault;
    _doPutCollateral(
        vault,
        IERC20Upgradeable(ISoftVault(vault).uToken()).balanceOf(
            address(this)
        )
    );

Here vault refers to the SoftVault token, but the second parameter of _doPutCollateral refers to the balance of the underlying token of vault.

/* BasicSpell.sol */
    function _doPutCollateral(address token, uint256 amount) internal {
        if (amount > 0) {
            _ensureApprove(token, address(werc20), amount);
            werc20.mint(token, amount);
            bank.putCollateral(
                address(werc20),
                uint256(uint160(token)),
                amount
            );
        }
    }

/* WERC20.sol */
    function mint(
        address token,
        uint256 amount
    ) external override nonReentrant returns (uint256 id) {
        uint256 balanceBefore = IERC20Upgradeable(token).balanceOf(
            address(this)
        );
        IERC20Upgradeable(token).safeTransferFrom(
            msg.sender,
            address(this),
            amount
        );
        uint256 balanceAfter = IERC20Upgradeable(token).balanceOf(
            address(this)
        );
        id = _encodeTokenId(token);
        _mint(msg.sender, id, balanceAfter - balanceBefore, "");
    }

As you can see in these code, the amount should refer to the balance of token itself, not the underlying token. I know, SoftVault basically shares the unit with its underlying token, but before that you need to deposit those underlying tokens.

Impact

The contract would never work as expected

Code Snippet

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

Tool used

Manual Review

Recommendation

Do not ever deploy untested, untidy code Do not include it in the audit scope neither

Duplicate of #133

sinarette commented 1 year ago

Escalate for 10 USDC

This issue is not a duplicate of #133 or #138 The report essentially contains of three separate bugs: first one referring to the underflow in strTokenAmt calculation, and the second one referring to the inconsistency of strTokenAmt and swapData.expectedAmount. These two are stated in #133 and #138. However the third one refers to the miscalculation of vault amount in _doPutCollateral, which is not stated in other issues.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

This issue is not a duplicate of #133 or #138 The report essentially contains of three separate bugs: first one referring to the underflow in strTokenAmt calculation, and the second one referring to the inconsistency of strTokenAmt and swapData.expectedAmount. These two are stated in #133 and #138. However the third one refers to the miscalculation of vault amount in _doPutCollateral, which is not stated in other issues.

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.

ctf-sec commented 1 year ago

Senior watson's comment:

However the third one refers to the miscalculation of vault amount in _doPutCollateral, which is not stated in other issues.

Watson is pointing out that the issue above isn't present in either of the issues it was duped with. That issue is the same as #116 which has also been escalated.

Judge comment:

it is easier to judge when the issue are separately submitted

issue https://github.com/sherlock-audit/2023-04-blueberry-judging/issues/116 has escalated resolved and being classified as a separate medium

so looks like reject this escalation is more favorable to auditor because the duplicate of high earn auditor better reward

hrishibhat commented 1 year ago

Escalation rejected

Valid duplicate of #133 Maintaining the duplication state as it is.

Multiple issues must be submitted separately