s_poolAssets underflow in CollateralTracker.sol will lead to protocol failure

[howlbot-integration]

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:

1. Copy the code above into CollateralTracker.t.sol
2. 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

[Picodes]

This is correct but what would be the impact? It seems that totalAssets would behave correctly so funds wouldn't be affected. However for example getPoolData and maxWithdraw would be affected so we could argue that functionality is broken but it isn't described by these reports.

I'll downgrade to QA

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b

[dyedm1]

Can confirm the PoC is valid, but we are also unable to find significant impact on our end (besides some potential confusion if frontends use the values returned by getPoolData to display stats)

[sammy-tm]

Agreed that the impact isn't correctly described in the report. Kudos to the judge @Picodes for pointing it out. However, the following problems still occur :

1. maxWithdraw() will return the incorrect amount, which will lead to reverts for the withdraw() function temporarily. This is a violation of ERC-4626

2. maxRedeem() will also cause 'redeem' to revert, if the user's shares balance in assets is greater than the pool's token balance. This is, again, a violation of ERC-4626.

withdraw() and redeem() will hence be temporarily affected, and some users will also lose access to their funds temporarily. After the pool recovers its balance again through deposits, these functions will operate normally.

Again, as the judge pointed out, there are no funds directly at risk, which is why my initial conviction that this is a High severity issue is incorrect.

However, given broken functionality, temporary loss of access to funds, and violations of ERC-4626, I would like to urge the judge to re-assess and see if this should be a Medium rather than a QA

I trust the judge to make the correct decision here.

Thanks!

[Picodes]

@sammy-tm my view is that this issue could have been a Medium but when judging a report we're supposed to take into account only the impacts described by this report, and in this case there is none

[liveactionllama]

Per direction from the judge, staff have marked as 1st place. Also noting there was a tie for 1st/2nd place.