jasonxiale - assets will be stuck in `PositionManager.sol` if `params.strike` is not equal to `DVP.currentStrike()`

[sherlock-admin2]

jasonxiale

high

assets will be stuck in PositionManager.sol if params.strike is not equal to DVP.currentStrike()

Summary

PositionManager.sol acts as a middleman between the user and the DVP. While calling PositionManager.mint, the function needs user to provide IPositionManager.MintParams calldata params, within params, there is a member called params.strike, if its vaule is not equal to IG.currentStrike(), PositionManager.sell/sellAll will revert, which means user's asset will be stuck.

Vulnerability Detail

While PositionManager.mint is called, the user need to provide IPositionManager.MintParams calldata params, and then the function calls dvp.mint, the code flow will arrive at IG.mint. In IG.mint, the user supplied params.strike is ignored, instead the function calls _mint with financeParameters.currentStrike Within DVP._mint, the strike will be financeParameters.currentStrike. And in DVP.sol#L187-L192, a position is created for PositionManager.

120     function _mint(
121         address recipient,
122         uint256 strike,
123         Amount memory amount,
124         uint256 expectedPremium,
125         uint256 maxSlippage
126     ) internal returns (uint256 premium_) {
    ...
186         // Create or update position:
187         Position.Info storage position = _getPosition(epoch.current, recipient, strike); <<<--- Here `strike` is `financeParameters.currentStrike`
188         position.premium += premium_;
189         position.epoch = epoch.current;
190         position.strike = strike;
191         position.amountUp += amount.up;
192         position.amountDown += amount.down;
193
194         emit Mint(msg.sender, recipient);
195     }

After IG.mint returns, a position will be created for the user, and in the position, user supplied params.strike will be stored in PositionManager.sol#L153

 91     function mint(
 92         IPositionManager.MintParams calldata params
 93     ) external override returns (uint256 tokenId, uint256 premium) {
     ...
142 
143         if (params.tokenId == 0) {
    ...
149 
150             // Save position:
151             _positions[tokenId] = ManagedPosition({
152                 dvpAddr: params.dvpAddr,
153                 strike: params.strike, <<<---- here user supplied params.strike is stored
154                 expiry: epoch.current,
155                 premium: premium,
156                 leverage: (params.notionalUp + params.notionalDown) / premium,
157                 notionalUp: params.notionalUp,
158                 notionalDown: params.notionalDown,
159                 cumulatedPayoff: 0
160             });
161         } else {
    ...
178     }

After PositionManager.mint, when the user calls PositionManager.sell to withdraw his position, the PositionManager._sell will the user's position to call DVP.burn, while calling DVP.burn, position.strike(which is set in PositionManager.sol#L153) is used in PositionManager.sol#L238

Then in IG.burn, strike is passed to DVP._burn. And at the begin of the function, position is fetched in DVP.sol#L240, when calling _getPosition, strike is the user supplied value in PositionManager.sol#L153.

And if the position doesn't exist, the function will revert.

231     function _burn(
232         uint256 expiry,
233         address recipient,
234         uint256 strike,
235         Amount memory amount,
236         uint256 expectedMarketValue,
237         uint256 maxSlippage
238     ) internal returns (uint256 paidPayoff) {
239         _requireNotPaused();
240         Position.Info storage position = _getPosition(expiry, msg.sender, strike); <<<--- here strike is user supplied
241         if (!position.exists()) { <<<--- here if the position doesn't exists, the function will revert
242             revert PositionNotFound();
243         }
...

So if the params.strike is not equal to financeParameters.currentStrike, the PositionNotFound.sell/sellAll will revert.

For PoC, please add the following code to test/PositionManager.t.sol and run forge test --mc PositionManagerTest --mt testMintAndBurnStrike -vv

    function testMintAndBurnStrike() public {
        uint256 tokenId;
        IG ig ;
        vm.startPrank(admin);
        ig = new IG(address(vault), address(ap));
        ig.grantRole(ig.ROLE_ADMIN(), admin);
        ig.grantRole(ig.ROLE_EPOCH_ROLLER(), admin);
        vault.grantRole(vault.ROLE_ADMIN(), admin);
        vault.setAllowedDVP(address(ig));

        MarketOracle mo = MarketOracle(ap.marketOracle());

        mo.setDelay(ig.baseToken(), ig.sideToken(), ig.getEpoch().frequency, 0, true);

        Utils.skipDay(true, vm);
        ig.rollEpoch();
        vm.stopPrank();

        uint256 strike = ig.currentStrike() - 1;

        (uint256 expectedMarketValue, ) = ig.premium(0, 10 ether, 0);
        TokenUtils.provideApprovedTokens(admin, baseToken, DEFAULT_SENDER, address(pm), expectedMarketValue, vm);
        // NOTE: somehow, the sender is something else without this prank...
        vm.prank(DEFAULT_SENDER);
        (tokenId, ) = pm.mint(
            IPositionManager.MintParams({
                dvpAddr: address(ig),
                notionalUp: 10 ether,
                notionalDown: 0,
                strike: strike,
                recipient: alice,
                tokenId: 0,
                expectedPremium: expectedMarketValue,
                maxSlippage: 0.1e18,
                nftAccessTokenId: 0
            })
        );

        vm.prank(alice);
        pm.sell(
            IPositionManager.SellParams({
                tokenId: tokenId,
                notionalUp: 1 ether,
                notionalDown: 0,
                expectedMarketValue: 0,
                maxSlippage: 0.1e18
            })
        );
    }

Impact

If user calls PositionManager.mint with params.strike isn't equal to IG.currentStrike(), PositionManager.sell/sellAll will revert

Code Snippet

https://github.com/sherlock-audit/2024-02-smilee-finance/blob/3241f1bf0c8e951a41dd2e51997f64ef3ec017bd/smilee-v2-contracts/src/periphery/PositionManager.sol#L153

Tool used

VS

Recommendation

diff --git a/smilee-v2-contracts/src/periphery/PositionManager.sol b/smilee-v2-contracts/src/periphery/PositionManager.sol
index 24096c3..6940490 100644
--- a/smilee-v2-contracts/src/periphery/PositionManager.sol
+++ b/smilee-v2-contracts/src/periphery/PositionManager.sol
@@ -11,6 +11,11 @@ import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
 import {ERC721Enumerable} from "@openzeppelin/contracts/token/ERC721/extensions/ERC721Enumerable.sol";
 import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";

+
+interface IIG{
+    function currentStrike() external view returns (uint256);
+}
+
 contract PositionManager is ERC721Enumerable, Ownable, IPositionManager {
     using SafeERC20 for IERC20;

@@ -150,7 +155,7 @@ contract PositionManager is ERC721Enumerable, Ownable, IPositionManager {
             // Save position:
             _positions[tokenId] = ManagedPosition({
                 dvpAddr: params.dvpAddr,
-                strike: params.strike,
+                strike: IIG(address(dvp)).currentStrike(),
                 expiry: epoch.current,
                 premium: premium,
                 leverage: (params.notionalUp + params.notionalDown) / premium,

Duplicate of #65

[sherlock-admin3]

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

panprog commented:

low, because it is user error to provide incorrect strike. dup of #23

takarez commented:

valid; high(2)

[metadato-eth]

SAME AS 65