sherlock-audit / 2023-07-blueberry-judging

2 stars 1 forks source link

nobody2018 - ShortLongSpell.openPosition cannot increase position on the existing position #65

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

nobody2018

medium

ShortLongSpell.openPosition cannot increase position on the existing position

Summary

AuraSpell/ConvexSpell/CurveSpell/IchiSpell can all increase positions via openPositionFarm. However, users cannot increase the position via [ShortLongSpell.openPosition](https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/spell/ShortLongSpell.sol#L140-L143). The reason is that openPosition lacks logic like the other 4 Spells: Take out existing collateral and burn. Therefore, when the user wants to increase the position on the existing position via ShortLongSpell.openPosition, tx will revert in [BlueBerryBank.putCollateral](https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/BlueBerryBank.sol#L855-L856).

Vulnerability Detail

[_doPutCollateral](https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/spell/ShortLongSpell.sol#L158-L161) is used to add collateral to the bank, which internally calls bank.putCollateral.

File: blueberry-core\contracts\BlueBerryBank.sol
846:     function putCollateral(
847:         address collToken,
848:         uint256 collId,
849:         uint256 amountCall
850:     ) external override inExec onlyWhitelistedERC1155(collToken) {
851:         Position storage pos = positions[POSITION_ID];
852:         if (pos.collToken != collToken || pos.collId != collId) {
853:             if (!oracle.isWrappedTokenSupported(collToken, collId))
854:                 revert Errors.ORACLE_NOT_SUPPORT_WTOKEN(collToken);
855:->           if (pos.collateralSize > 0)
856:->               revert Errors.DIFF_COL_EXIST(pos.collToken);
857:             pos.collToken = collToken;
858:             pos.collId = collId;
859:         }
......
870:     }

From L855-856, we can see that if pos.collateralSize is bigger than 0, tx will revert. So, if the user only opens new position, pos.collateralSize is equal to 0, no problem. However, if the user wants to increase the position on the existing position, pos.collateralSize must be greater than 0. In this case, such an operation cannot be performed.

Impact

User can't increase position on the existing position via openPosition. This is different from AuraSpell/ConvexSpell/CurveSpell/IchiSpell and breaks protocol assumption.

Code Snippet

https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/spell/ShortLongSpell.sol#L140-L162

Tool used

Manual Review

Recommendation

--- a/blueberry-core/contracts/spell/ShortLongSpell.sol
+++ b/blueberry-core/contracts/spell/ShortLongSpell.sol
@@ -154,7 +154,17 @@ contract ShortLongSpell is BasicSpell {

         /// 4. Put collateral - strategy token
         address vault = strategies[param.strategyId].vault;
-
+        {
+            IBank.Position memory pos = bank.getCurrentPositionInfo();
+            address posCollToken = pos.collToken;
+            uint256 collSize = pos.collateralSize;
+            if (collSize > 0) {
+                if (posCollToken != address(werc20))
+                    revert Errors.INCORRECT_COLTOKEN(posCollToken);
+                uint burnAmount = bank.takeCollateral(collSize);
+                werc20.burn(vault), burnAmount);
+            }
+        }
         _doPutCollateral(
             vault,
             IERC20Upgradeable(ISoftVault(vault)).balanceOf(address(this))
sherlock-admin2 commented 1 year ago

2 comment(s) were left on this issue during the judging contest.

shogoki commented:

This revert is only triggered if the collTOken or collId does not match the stored position. (In case it is a new position)

0xyPhilic commented:

invalid because the if statement can be triggered only in the case when the passed colToken is different from the actual pos.colToken or the id is different from pos.colId