Incorrect rounding while withdrawing assets from `AutoPxGmx` and `AutoPxGlp` contracts

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L190 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L212

Vulnerability details

The function previewWithdraw is overridden in the AutoPxGmx and AutoPxGlp contracts to account for penalty fees while exiting the vaults. This happens in line 212 of the AutoPxGmx contract and similarly in line 190 of the AutoPxGlp contract:

return
    (_totalSupply == 0 || _totalSupply - shares == 0)
        ? shares
        : (shares * FEE_DENOMINATOR) /
            (FEE_DENOMINATOR - withdrawalPenalty);

Here, the integer division will round down, while the correct behavior should be to round up in favor of the protocol.

Impact

This issue would allow a user to withdraw tokens without paying penalties, which should not represent a big impact in a realistic scenario since it would need to involve handling extremely low quantities of assets/shares or withdrawing assets in batches of a relatively small amount (which should be discouraged by gas costs).

During each call to withdraw, if the division is not exact, the protocol will be losing 1 share of penalties due to rounding.

The correct way should be to round up the division, following the base ERC4626 implementation of the previewWithdraw function which uses mulDivUp.

Proof of Concept

The following test demonstrates the issue. In this case the user mints 10 shares and then withdraws 1 token by burning a single share without any penalty.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;

import "forge-std/Test.sol";

import {ERC20} from "solmate/tokens/ERC20.sol";
import {AutoPxGmx} from "src/vaults/AutoPxGmx.sol";
import {PirexGmx} from "src/PirexGmx.sol";
import {Helper} from "./Helper.sol";

contract AuditTest is Helper {
    function test_AutoPxGmx_WithdrawWrongRounding() public {        
        address user = testAccounts[0];

        uint256 userBalance = 10;
        _mintPx(user, userBalance, true);

        // Approve contract
        vm.prank(user);
        pxGmx.approve(address(autoPxGmx), type(uint256).max);

        // User deposits balance, he gets 10 shares
        vm.prank(user);
        autoPxGmx.deposit(userBalance, user);        
        assertEq(autoPxGmx.balanceOf(user), userBalance);

        vm.prank(user);
        uint256 shares = autoPxGmx.previewWithdraw(1);

        // Only 1 share needed to withdraw 1 token
        assertEq(shares, 1);

        vm.prank(user);
        autoPxGmx.withdraw(1, user, user);

        // User has withdrawn 1 token and still has 9 shares 
        assertEq(pxGmx.balanceOf(user), 1);
        assertEq(autoPxGmx.balanceOf(user), 9);   
    }
}

Recommended Mitigation Steps

Change the penalty calculation in the previewWithdraw function to round up:

return
    (_totalSupply == 0 || _totalSupply - shares == 0)
        ? shares
        : shares.mulDivUp(FEE_DENOMINATOR, FEE_DENOMINATOR - withdrawalPenalty);

[c4-judge]

Picodes marked the issue as duplicate of #264

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[C4-Staff]

JeeberC4 marked the issue as duplicate of #264

[liveactionllama]

Duplicate of #178