`s_poolAssets` underflow in `CollateralTracker.sol` will lead to protocol failure

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/CollateralTracker.sol#L578

Vulnerability details

s_poolAssets can underflow in CollateralTracker.sol. This is because, in the withdraw() function, the assets that the user withdraws are deducted from s_poolAssets; however, there is no check to ensure s_poolAssets >= assets. Moreover, the updation of s_poolAssets is handled in an unchecked block, which makes the underflow possible.

    function withdraw(
        uint256 assets,
        address receiver,
        address owner,
        TokenId[] calldata positionIdList
    ) external returns (uint256 shares) {
        shares = previewWithdraw(assets);

        // check/update allowance for approved withdraw
        if (msg.sender != owner) {
            uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.

            if (allowed != type(uint256).max) allowance[owner][msg.sender] = allowed - shares;
        }

        // burn collateral shares of the Panoptic Pool funds (this ERC20 token)
        _burn(owner, shares);

        // update tracked asset balance
        unchecked {
            s_poolAssets -= uint128(assets);
        }

        // reverts if account is not solvent/eligible to withdraw
        s_panopticPool.validateCollateralWithdrawable(owner, positionIdList);

        // transfer assets (underlying token funds) from the PanopticPool to the LP
        SafeTransferLib.safeTransferFrom(
            s_underlyingToken,
            address(s_panopticPool),
            receiver,
            assets
        );

        emit Withdraw(msg.sender, receiver, owner, assets, shares);

        return shares;
    }

s_poolAssets can be less than assets, this is because when a short option is minted, assets are moved from the Panoptic pool to the Uniswap pool. i.e, assets are deducted from s_poolAssets and incremented in s_inAMM. So, the underflow is possible when a large share of the deposited liquidity is in the Uniswap pool.

Impact

This breaks the functionality and accounting of the entire protocol. A number of attacks can be performed to drain the pool due to this vulnerability. An example would be :

1. Attacker mints a large number of short options

2. Attacker withdraws and causes underflow

3. Attacker can drain the pool by calling withdraw() again as assets are now highly undervalued relative to shares.

   Proof of Concept

   The following test demonstrates the underflow scenario :

   function test_POC_Underflow() public {
       // initalize world state
       uint256 x = 4532 ; uint104 assets = 1000;
       _initWorld(x);
   
       // Invoke all interactions with the Collateral Tracker from user Bob
       vm.startPrank(Bob);
   
       // give Bob the max amount of tokens
       _grantTokens(Bob);
   
       // approve collateral tracker to move tokens on the msg.senders behalf
       IERC20Partial(token0).approve(address(collateralToken0), assets);
   
       // deposit a number of assets determined via fuzzing
       // equal deposits for both collateral token pairs for testing purposes
       uint256 returnedShares0 = collateralToken0.deposit(assets, Bob);
   
       // total amount of shares before withdrawal
   
       uint256 assetsToken0 = convertToAssets(returnedShares0, collateralToken0);
   
       // user mints options and liquidity is moved to the Uniswap pool
       // for simpicity, we manually set the values of `s_poolAssets` and `s_inAMM`
       collateralToken0.setPoolAssets(1);
       collateralToken0.setInAMM(int128(uint128(assets)-1));
   
       // withdraw tokens
       collateralToken0.withdraw(assetsToken0, Bob, Bob, new TokenId[](0));
   
       // confirm the underflow
       assertEq(collateralToken0._availableAssets(), type(uint128).max - assetsToken0 + 2);
   }

   To run the test:

4. Copy the code above into CollateralTracker.t.sol

5. Run forge test --match-test test_POC_Underflow

Tools Used

Foundry

Recommended Mitigation Steps

Remove the unchecked block. Alternatively, add this check in withdraw():

        if (assets > s_poolAssets) revert Errors.ExceedsMaximumRedemption();

Assessed type

Under/Overflow