Withdrawal/redemptions employ a non-user provided hardcoded slippage in their executions

[c4-bot-5]

Lines of code

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

Vulnerability details

Proof of Concept

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

    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) external returns (uint256 shares) {
        if (assets > maxWithdraw(owner)) revert Errors.ExceedsMaximumRedemption();

        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);
        }

        // 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;
    }

This function is used to redeem an amount of shares required to withdraw the specified amount of assets the first check in the function's implementation is to check if the attempted withdrawal is greater than user's current maximum amount that can be withdrawn by therm, this check is being done by querying the maxWithdraw() function whose implementation can be seen here https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L507-L514

    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;
        uint256 balance = convertToAssets(balanceOf[owner]);
        return s_panopticPool.numberOfPositions(owner) == 0 ? Math.min(available, balance) : 0;
    }

Evidently, we can see that unlike protocol's assumption in withdraw() that the value retuned by maxWithdraw() is user's maximum redemption, it's only valid at the time and not really the amount of assets they have if the owner's current balance is converted, that's to say in a case where a user's current balance is greater than the available assets, the available assets get returned via Math.min(available, balance) this then causes the user's attempt at withdrawal to revert if, say in this case they pass their whole balance or even any balance that's above the current available asset balance.

Now keep in mind that we can't really rely on the user first calling maxWithdraw() to first see how much they are eligible to withdraw and then pass this data to withdraw, cause even if they do this, there is nothing stopping another user from griefing their attempt at withdrawal by front running their calls with minute amounts that ends up causing the value they passed to be more than the available asset from uint256 available = s_poolAssets;.

Impact

• A user non-provided hardcoded slippage is attached to attempts at withdrawing, this leads to user's not being able to remove their assets on their specified time, note that this could lead to asset loss in their $USD moneterial value as users might be attempting to withdraw all their assets cause they feel the crypto market is taking a heavy dump and want to sell off these assets (which is not uncommon in the crypto world, considering the current volatility/in flow of users with the bull run) but this is not possible cause an attempt to withdraw a value that's higher than the available assets currently reverts instead of sending users the currently available assets.

• One could argue that the first case pertains to a user who's not tech savvy enough to call maxWithdraw() to see how much they can withdraw, however even a user who queries this and sees how much they can withdraw and then requesting this, another user (intentional or not) could just front run thier attempt at withdrawing with minute values of assets just to cause the available assets be less than the amount attempted at withdrawal so as to ensure their transactions revert thereby griefing them, and this is possible since there is no minimum withdrawal amount.

Recommended Mitigation Steps

Generally, slippage shouldn't be calculated on chain but rather provided by the user, so the function could be reimplemented to allow user pass in their slippaged value of the least amount they are willing to receive, or consider sending a user the currently available asset balance if their requested withdrawal is more than this value and then update their balance.

Alternatively at least to mitigate the window for the second case in Impact, consider having a min withdrawal amount so as not to allow users intentionally or unintentionally grief one another.

Assessed type

Context

[c4-judge]

Picodes marked the issue as duplicate of #365

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[Bauchibred]

Hi @Picodes, thanks for judging, based on your comment on the current primary finding here, I can see you've downgraded the issue due to not having an attack path, however I believe this issue should be deduplicated from 365 and re-assessed as a medium, as it includes an attack path and also indicates the impacts that would arise from the protocol's current implementation of the hardcoded on-chain slippage.

[Picodes]

I don't see which credible scenario you are referring to.