`SettleLongPremium` is incorrectly implemented: premium should be deducted instead of added

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticPool.sol#L1621-L1640 https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/CollateralTracker.sol#L1043-L1089

Vulnerability details

Impact

SettleLongPremium is the function intended to settle premiums for long option holders. When called, it should deduct the premium from the option owner's account, but the current implementation adds the premium instead.

Bug Description

Lets see the code for premium calculation. We can see that accumulatedPremium and s_options[owner][tokenId][legIndex] are premium accumulators for calculating the owed amount of premium, and that accumulatedPremium is a LeftRightUnsigned type, which means it must be positive.

The realizedPremia is also positive, because it is calculated by accumulatedPremium * liquidity.

The issue occurs when calling s_collateralToken.exercise(). The realizedPremia that is passed inside should be negative instead of positive, because negative means user pays premia, and positive means user receives premia. The current implementation is incorrect.

PanopticPool.sol

            accumulatedPremium = LeftRightUnsigned
                .wrap(0)
                .toRightSlot(premiumAccumulator0)
                .toLeftSlot(premiumAccumulator1);

            // update the premium accumulator for the long position to the latest value
            // (the entire premia delta will be settled)
            LeftRightUnsigned premiumAccumulatorsLast = s_options[owner][tokenId][legIndex];
            s_options[owner][tokenId][legIndex] = accumulatedPremium;

>           accumulatedPremium = accumulatedPremium.sub(premiumAccumulatorsLast);
        }

        uint256 liquidity = PanopticMath
            .getLiquidityChunk(tokenId, legIndex, s_positionBalance[owner][tokenId].rightSlot())
            .liquidity();

        unchecked {
            // update the realized premia
>           LeftRightSigned realizedPremia = LeftRightSigned
>               .wrap(0)
>               .toRightSlot(int128(int256((accumulatedPremium.rightSlot() * liquidity) / 2 ** 64)))
>               .toLeftSlot(int128(int256((accumulatedPremium.leftSlot() * liquidity) / 2 ** 64)));

            // deduct the paid premium tokens from the owner's balance and add them to the cumulative settled token delta
            s_collateralToken0.exercise(owner, 0, 0, 0, realizedPremia.rightSlot());
            s_collateralToken1.exercise(owner, 0, 0, 0, realizedPremia.leftSlot());

CollateralTracker.sol

   function exercise(
        address optionOwner,
        int128 longAmount,
        int128 shortAmount,
        int128 swappedAmount,
        int128 realizedPremium
    ) external onlyPanopticPool returns (int128) {
        unchecked {
            // current available assets belonging to PLPs (updated after settlement) excluding any premium paid
            int256 updatedAssets = int256(uint256(s_poolAssets)) - swappedAmount;

            // add premium to be paid/collected on position close
>           int256 tokenToPay = -realizedPremium;

            // if burning ITM and swap occurred, compute tokens to be paid through exercise and add swap fees
            int256 intrinsicValue = swappedAmount - (longAmount - shortAmount);

            if ((intrinsicValue != 0) && ((shortAmount != 0) || (longAmount != 0))) {
                // intrinsic value is the amount that need to be exchanged due to burning in-the-money

                // add the intrinsic value to the tokenToPay
                tokenToPay += intrinsicValue;
            }

>           if (tokenToPay > 0) {
                // if user must pay tokens, burn them from user balance (revert if balance too small)
                uint256 sharesToBurn = Math.mulDivRoundingUp(
                    uint256(tokenToPay),
                    totalSupply,
                    totalAssets()
                );
                _burn(optionOwner, sharesToBurn);
>           } else if (tokenToPay < 0) {
                // if user must receive tokens, mint them
                uint256 sharesToMint = convertToShares(uint256(-tokenToPay));
                _mint(optionOwner, sharesToMint);
            }

Proof of Concept

We can also see from unit test test_success_settleLongPremium: The tests checks that after calling settleLongPremium, the assets of Buyer[0] actually increases instead of decreases, which is obviously incorrect.

        assetsBefore0 = ct0.convertToAssets(ct0.balanceOf(Buyers[0]));
        assetsBefore1 = ct1.convertToAssets(ct1.balanceOf(Buyers[0]));

        // collect buyer 1's three relevant chunks
        for (uint256 i = 0; i < 3; ++i) {
            pp.settleLongPremium(collateralIdLists[i], Buyers[0], 0);
        }

        assertEq(
            ct0.convertToAssets(ct0.balanceOf(Buyers[0])) - assetsBefore0,
            33_342,
            "Incorrect Buyer 1 1st Collect 0"
        );

Tools Used

Manual review.

Recommended Mitigation Steps

Take the negative of realizedPremia before calling s_collateralToken.exercise().

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #376

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[Picodes]

Keeping High severity as funds are stake