code-423n4 / 2024-04-panoptic-findings

9 stars 4 forks source link

The `PanopticPool.settleLongPremium()` function does not deduct the premium from the owner of the long position, but rather rewards the premium. #98

Closed c4-bot-6 closed 7 months ago

c4-bot-6 commented 7 months ago

Lines of code

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

Vulnerability details

Impact

Every PanopticPool.settleLongPremium() call causes the CollateralTracker's share value to be lower than the actual value, and an error occurs in the accounting of poolAssets. As a result, the long position owner receives unfair profits equal to two times the premium, and other users suffer losses equivalent to that amount.

  1. If a malicious user intentionally exploits this error, he or she may unfairly steal the collateral from the pool.
  2. Errors in the accounting of poolAssets can cause more serious problems to the entire system.

Proof of Concept

The PanopticPool.settleLongPremium() function is as follows.

File: PanopticPool.sol
1587:     function settleLongPremium(
1588:         TokenId[] calldata positionIdList,
1589:         address owner,
1590:         uint256 legIndex
1591:     ) external {
1592:         _validatePositionList(owner, positionIdList, 0);
...
1600:         LeftRightUnsigned accumulatedPremium;
...
1627:         uint256 liquidity = PanopticMath
1628:             .getLiquidityChunk(tokenId, legIndex, s_positionBalance[owner][tokenId].rightSlot())
1629:             .liquidity();
1630: 
1631:         unchecked {
1632:             // update the realized premia
1633:             LeftRightSigned realizedPremia = LeftRightSigned
1634:                 .wrap(0)
1635:                 .toRightSlot(int128(int256((accumulatedPremium.rightSlot() * liquidity) / 2 ** 64)))
1636:                 .toLeftSlot(int128(int256((accumulatedPremium.leftSlot() * liquidity) / 2 ** 64)));
1637: 
1638:             // deduct the paid premium tokens from the owner's balance and add them to the cumulative settled token delta
1639:             s_collateralToken0.exercise(owner, 0, 0, 0, realizedPremia.rightSlot());  // @audit <-- realizedPremia is an unsigned value greater than 0. However, in order for exercise() to subtract premia, we must pass a value less than 0.
1640:             s_collateralToken1.exercise(owner, 0, 0, 0, realizedPremia.leftSlot());  // @audit <-- realizedPremia is an unsigned value greater than 0. However, in order for exercise() to subtract premia, we must pass a value less than 0.
...
1649:             // commit the delta in settled tokens (all of the premium paid by long chunks in the tokenIds list) to storage
1650:             s_settledTokens[chunkKey] = s_settledTokens[chunkKey].add(
1651:                 LeftRightUnsigned.wrap(uint256(LeftRightSigned.unwrap(realizedPremia)))
1652:             );
...
1659:     }

In L1639 and L1640, the premium value is attempted to be deducted from the long position owner through the s_collateralToken.exercise() function. In L1635 and L1636, the data type of accumulatedPremium is LeftRightUnsigned and the data type of liquidity is uint256, which are both > 0, and therefore, the rightSlot and leftSlot of realizedPremium are both > 0.

Meanwhile, the CollateralTracker.sol#exercise() function is as follows.

File: CollateralTracker.sol
1043:     function exercise(
1044:         address optionOwner,
1045:         int128 longAmount,
1046:         int128 shortAmount,
1047:         int128 swappedAmount,
1048:         int128 realizedPremium
1049:     ) external onlyPanopticPool returns (int128) {
...
1054:             // add premium to be paid/collected on position close
1055:             int256 tokenToPay = -realizedPremium;
...
1067:             if (tokenToPay > 0) {
1068:                 // if user must pay tokens, burn them from user balance (revert if balance too small)
1069:                 uint256 sharesToBurn = Math.mulDivRoundingUp(
1070:                     uint256(tokenToPay),
1071:                     totalSupply,
1072:                     totalAssets()
1073:                 );
1074:                 _burn(optionOwner, sharesToBurn);
1075:             } else if (tokenToPay < 0) {
1076:                 // if user must receive tokens, mint them
1077:                 uint256 sharesToMint = convertToShares(uint256(-tokenToPay));
1078:                 _mint(optionOwner, sharesToMint);
1079:             }
...
1084:             s_poolAssets = uint128(uint256(updatedAssets + realizedPremium));
...
1089:     }

Since the delivered realizedPremium parameter is > 0, tokenToPay < 0 by L1055 and the result is

  1. According to L1078, a share equal to realizedPremium is paid to the owner.
  2. s_poolAssets is increased by realizedPremium by L1084.

This is the exact opposite of what was intended and is a very dangerous result.

To be accurate, the CollateralTracker.sol#exercise() function must burn as much share as realizedPremium from the owner and s_poolAssets must be reduced by realizedPremium. To achieve this, when calling s_collateralToken.exercise() in PanopticPool.sol#L1639, L1640, realizedPremia multiplied by -1 must be passed.

Tools Used

Manual Review

Recommended Mitigation Steps

When calling s_collateralToken.exercise() in PanopticPool.sol#L1639, L1640, the realizedPreemia parameter multiplied by -1 must be passed.

File: PanopticPool.sol
1587:     function settleLongPremium(
1588:         TokenId[] calldata positionIdList,
1589:         address owner,
1590:         uint256 legIndex
1591:     ) external {
...
1637: 
1638:             // deduct the paid premium tokens from the owner's balance and add them to the cumulative settled token delta
--- 1639:             s_collateralToken0.exercise(owner, 0, 0, 0, realizedPremia.rightSlot());
--- 1640:             s_collateralToken1.exercise(owner, 0, 0, 0, realizedPremia.leftSlot());
+++ 1639:             s_collateralToken0.exercise(owner, 0, 0, 0, -1 * realizedPremia.rightSlot());
+++ 1640:             s_collateralToken1.exercise(owner, 0, 0, 0, -1 * realizedPremia.leftSlot());
...
1659:     }

Assessed type

Math

c4-judge commented 7 months ago

Picodes marked the issue as duplicate of #376

c4-judge commented 7 months ago

Picodes marked the issue as satisfactory