CollateralTracker maxWithdraw/maxRedeem functions may revert in some cases.

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-09-panoptic/blob/main/contracts/CollateralTracker.sol#L489 https://github.com/code-423n4/2024-09-panoptic/blob/main/contracts/CollateralTracker.sol#L593

Vulnerability details

Impact

CollateralTracker is a ERC4626 compliant vault. However, in some cases, the maxWithdraw/maxRedeem functions may revert.

Bug Description

According to the README: Panoptic’s CollateralTracker supports the full ERC4626 interface. In the scope of this audit contest, a new change was introduced when calculating maxWithdraw/maxRedeem that it uses s_poolAssets - 1 as available asset balance since there was 1 wei of virtual balance in the beginning.

    function maxWithdraw(address owner) public view returns (uint256 maxAssets) {
        // We can only use the standard 4626 withdraw function if the user has no open positions
        // For the sake of simplicity assets can only be withdrawn through the redeem function
@>      uint256 available = s_poolAssets - 1;
        uint256 balance = convertToAssets(balanceOf[owner]);
        return s_panopticPool.numberOfPositions(owner) == 0 ? Math.min(available, balance) : 0;
    }

    function maxRedeem(address owner) public view returns (uint256 maxShares) {
@>      uint256 available = convertToShares(s_poolAssets - 1);
        uint256 balance = balanceOf[owner];
        return s_panopticPool.numberOfPositions(owner) == 0 ? Math.min(available, balance) : 0;
    }

However, it is possible that s_poolAssets is zero. When users are minting short positions (equivalent to minting UniswapV3 LP), tokens are sent to UniswapV3. If the amount of tokens happens to be exactly the remainder asset of a CollateralTracker, the updated s_poolAssets would turn to zero. `

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

            ...

@>          s_poolAssets = uint256(updatedAssets).toUint128();
        }
    }

Proof of Concept

Presented above.

Tools Used

Manual Review

Recommended Mitigation Steps

For both functions, if s_poolAssets == 0, simply return 0.

Assessed type

ERC4626

[dyedm1]

However, it is possible that s_poolAssets is zero. When users are minting short positions (equivalent to minting UniswapV3 LP), tokens are sent to UniswapV3. If the amount of tokens happens to be exactly the remainder asset of a CollateralTracker, the updated s_poolAssets would turn to zero.

Good point. This only happens when you can't withdraw anyway though, so this is more of a quirk/incorrect as to spec Low severity issue I think.

[Picodes]

At first sight I think @dyedm1 is right !

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-a

[c4-judge]

Picodes marked the issue as selected for report

[thebrittfactor]

For awarding purposes, C4 staff have marked as 1st place.