Lack of overflow validation allows manipulation of s_poolAssets leading to incorrect totalAssets calculation

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

The lack of overflow validation allows s_poolAssets to be manipulated.
Once overflow occurs, totalAssets can be set higher than the actual collaterals, preventing other users from withdrawing their own collateral due to the incorrect totalAssets.

Proof of Concept

totalAssets is calculated as the sum of s_poolAssets and s_inAMM.

shares = assets * totalSupply / totalAssets()
assets = shares / totalSupply * (s_poolAssets + s_inAMM)

If a user owns 50% of the totalShares, their withdrawal assets are calculated as:

assets = 0.5 * (s_poolAssets + s_inAMM)

If s_inAMM is significantly larger than s_poolAssets, the calculated assets can exceed s_poolAssets, leading to an overflow of s_poolAssets.

assets = 0.5 * (s_poolAssets + s_poolAssets + x)

s_poolAssets and s_inAMM are calculated in the takeCommissionAddData function.

File: https://github.com/code-423n4/2024-06-panoptic/blob/main/contracts/CollateralTracker.sol#L578
    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); // @audit assets can be larger than s_poolAssets?
        }
        ...
    }

Tools Used

Manual review

Recommended Mitigation Steps

Add overflow validation or remove the unchecked to prevent manipulation of s_poolAssets.

    function withdraw(
        uint256 assets,
        address receiver,
        address owner,
        TokenId[] calldata positionIdList
    ) external returns (uint256 shares) {
+        if (assets > s_poolAssets) revert Errors.ExceedsMaximumRedemption();
        shares = previewWithdraw(assets);
        ...
    }

Assessed type

Under/Overflow

[c4-judge]

Picodes marked the issue as duplicate of #38

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b

[liveactionllama]

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