Subtle mint/deposit limit invariant can easily be sidestepped

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L417-L441

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L417-L441

    function deposit(uint256 assets, address receiver) external returns (uint256 shares) {
        if (assets > type(uint104).max) revert Errors.DepositTooLarge();

        shares = previewDeposit(assets);

        // transfer assets (underlying token funds) from the user/the LP to the PanopticPool
        // in return for the shares to be minted
        SafeTransferLib.safeTransferFrom(
            s_underlyingToken,
            msg.sender,
            address(s_panopticPool),
            assets
        );

        // mint collateral shares of the Panoptic Pool funds (this ERC20 token)
        _mint(receiver, shares);

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

        emit Deposit(msg.sender, receiver, assets, shares);
    }

And https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L477-L501

    function mint(uint256 shares, address receiver) external returns (uint256 assets) {
        assets = previewMint(shares);

        if (assets > type(uint104).max) revert Errors.DepositTooLarge();

        // transfer assets (underlying token funds) from the user/the LP to the PanopticPool
        // in return for the shares to be minted
        SafeTransferLib.safeTransferFrom(
            s_underlyingToken,
            msg.sender,
            address(s_panopticPool),
            assets
        );

        // mint collateral shares of the Panoptic Pool funds (this ERC20 token)
        _mint(receiver, shares);

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

        emit Deposit(msg.sender, receiver, assets, shares);
    }

Evidently, we can see that the CollateralTracker includes functionalities for depositing assets and minting shares, with a maximum limit of (2 ** 104) - 1 for both assets and shares. However, the contract lacks a mechanism to verify if a user already owns a portion of the maximum limit, allowing for the bypassing of this invariant. Specifically, an attacker could circumvent this limit by dividing their intended deposit or mint into multiple transactions (instead of directly attempting to mint (2 ** 104) which is > (2 ** 104) - 1, they divide it into two), thereby accumulating more than the maximum limit.

Impact

The absence of a check for a user's existing assets before allowing a deposit or mint operation allows anyone to bypass the subtle invariant of having a limit attached to deposits/mints.

Recommended Mitigation Steps

Before processing a deposit or mint operation, consider verifying the user's current assets, i.e If the addition of the current transaction would exceed the maximum limit, the operation should be reverted.

Assessed type

ERC4626

[c4-judge]

Picodes marked the issue as duplicate of #553

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes marked the issue as unsatisfactory:
Invalid

[Picodes]

maxMint would only concern a single mint