PirexGMX can be permanatly DOS'd due to 15 minute cooldown applied by GMX after staking

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L615-L674

Vulnerability details

##Summary

GMX applies a 15 minute cooldown to users/contracts after they stake an underlying asset for GLP. During this period both transferFrom and unstake are unavailable. PirexGMX only allows the user to redeem their pxGLP for GLP underlying assets and never directly for GLP. Due to this an adversary can DOS withdrawing from the PirexGMX contract by depositing assets every 15 mintutes.

Impact

Users will be unable to withdraw their pxGLP

Proof of Concept

deposited = gmxRewardRouterV2.mintAndStakeGlp(
    token,
    tokenAmount,
    minUsdg,
    minGlp
);

Each time a user calls depositGlp or depositGlpETH the constituent assets are transferred to PirexGMX and stake via the gmxRewardRouterV2#mintAndStakeGlpETH

GMXRewardRouterV2

function mintAndStakeGlpETH(uint256 _minUsdg, uint256 _minGlp) external payable nonReentrant returns (uint256) {
    require(msg.value > 0, "RewardRouter: invalid msg.value");

    IWETH(weth).deposit{value: msg.value}();
    IERC20(weth).approve(glpManager, msg.value);

    address account = msg.sender;
    uint256 glpAmount = IGlpManager(glpManager).addLiquidityForAccount(address(this), account, weth, msg.value, _minUsdg, _minGlp);

    IRewardTracker(feeGlpTracker).stakeForAccount(account, account, glp, glpAmount);
    IRewardTracker(stakedGlpTracker).stakeForAccount(account, account, feeGlpTracker, glpAmount);

    emit StakeGlp(account, glpAmount);

    return glpAmount;
}

Inside mintAndStakeGlpETH the underlying asset is converted to glp via glpManger#addLiquidityForAccount.

(GLPManager)[https://arbiscan.io/address/0x321F653eED006AD1C29D174e17d96351BDe22649#code#L913]

function _addLiquidity(address _fundingAccount, address _account, address _token, uint256 _amount, uint256 _minUsdg, uint256 _minGlp) private returns (uint256) {
    require(_amount > 0, "GlpManager: invalid _amount");

    // calculate aum before buyUSDG
    uint256 aumInUsdg = getAumInUsdg(true);
    uint256 glpSupply = IERC20(glp).totalSupply();

    IERC20(_token).safeTransferFrom(_fundingAccount, address(vault), _amount);
    uint256 usdgAmount = vault.buyUSDG(_token, address(this));
    require(usdgAmount >= _minUsdg, "GlpManager: insufficient USDG output");

    uint256 mintAmount = aumInUsdg == 0 ? usdgAmount : usdgAmount.mul(glpSupply).div(aumInUsdg);
    require(mintAmount >= _minGlp, "GlpManager: insufficient GLP output");

    IMintable(glp).mint(_account, mintAmount);

    lastAddedAt[_account] = block.timestamp;

    emit AddLiquidity(_account, _token, _amount, aumInUsdg, glpSupply, usdgAmount, mintAmount);

    return mintAmount;
}

The nitty gritty of the conversion happens here and lastAddedAt[_account] is update to block.timestamp, which is where the issue lies.

function _removeLiquidity(address _account, address _tokenOut, uint256 _glpAmount, uint256 _minOut, address _receiver) private returns (uint256) {
    require(_glpAmount > 0, "GlpManager: invalid _glpAmount");
    require(lastAddedAt[_account].add(cooldownDuration) <= block.timestamp, "GlpManager: cooldown duration not yet passed");

When a user wishes to redeem their pxGLP they are force to unstake the underlying GLP which requires that the cooldown duration has expired. An adversary can exploit this to make it impossible to withdraw. By repeatedly making small deposits, they can keep refreshing lastAddedAt[_account], causing every withdraw attempt to revert and trapping users.

Tools Used

Manual Review

Recommended Mitigation Steps

My first recommendation would be to allow users to redeem their pxGLP directly for GLP. My second recommendation would be to remove the ability to directly stake constituent assets. Create batching contracts that collects user deposits and batch stakes. Make one for each asset and track balances internally until they can be deposited into the PirexGMX and then users can claim their pxGLP after their batch is staked and deposited.

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Picodes marked the issue as duplicate of #113

[c4-judge]

Picodes marked the issue as satisfactory