AutoPx*.previewWithdraw does not round up

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L199-L217

Vulnerability details

Impact

When calling AutoPx*.previewWithdraw in AutoPx*.withdraw, the comment says "No need to check for rounding error, previewWithdraw rounds up".

    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public override returns (uint256 shares) {
        // Compound rewards and ensure they are properly accounted for prior to withdrawal calculation
        compound(poolFee, 1, 0, true);

        shares = previewWithdraw(assets); // No need to check for rounding error, previewWithdraw rounds up.

But only PirexERC4626.previewWithdraw rounds up.
AutoPx*.previewWithdraw overrides PirexERC4626.previewWithdraw, but does not round up.

    function previewWithdraw(uint256 assets)
        public
        view
        override
        returns (uint256)
    {
        // Calculate shares based on the specified assets' proportion of the pool
        uint256 shares = convertToShares(assets);

        // Save 1 SLOAD
        uint256 _totalSupply = totalSupply;

        // Factor in additional shares to fulfill withdrawal if user is not the last to withdraw
        return
            (_totalSupply == 0 || _totalSupply - shares == 0)
                ? shares
                : (shares * FEE_DENOMINATOR) /
                    (FEE_DENOMINATOR - withdrawalPenalty);
    }
...
    function convertToShares(uint256 assets)
        public
        view
        virtual
        returns (uint256)
    {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

        return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
    }

This breaks the developer's intention and does not comply with the EIP-4626 specification

Proof of Concept

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L199-L217
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L177-L195
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L99-L105
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L315-L323

Tools Used

None

Recommended Mitigation Steps

Call PirexERC4626.previewWithdraw instead of convertToShares in AutoPx*.previewWithdraw

    function previewWithdraw(uint256 assets)
        public
        view
        override
        returns (uint256)
    {
        // Calculate shares based on the specified assets' proportion of the pool
-        uint256 shares = convertToShares(assets);
+       uint256 shares = PirexERC4626.previewWithdraw(assets);

[c4-judge]

Picodes marked the issue as duplicate of #264

[c4-judge]

Picodes marked the issue as partial-25

[Picodes]

No impact described aside from the non compliance with the EIP

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[C4-Staff]

JeeberC4 marked the issue as duplicate of #264

[liveactionllama]

Duplicate of #178