Redacted Cartel contest - 0x52's results

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.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L210-L296

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

    uint256 preClaimTotalAssets = asset.balanceOf(address(this));
    uint256 preClaimPxGmxAmount = pxGmx.balanceOf(address(this));

    PirexRewards(rewardsModule).claim(asset, address(this));
    PirexRewards(rewardsModule).claim(pxGmx, address(this));
    ...

    // Track the amount of pxGMX received
    pxGmxAmountOut = pxGmx.balanceOf(address(this)) - preClaimPxGmxAmount;

    if (pxGmxAmountOut != 0) {
        // Calculate and distribute pxGMX fees if the amount of pxGMX increased
        totalPxGmxFee = (pxGmxAmountOut * platformFee) / FEE_DENOMINATOR;
        pxGmxIncentive = optOutIncentive
            ? 0
            : (totalPxGmxFee * compoundIncentive) / FEE_DENOMINATOR;

        if (pxGmxIncentive != 0)
            pxGmx.safeTransfer(msg.sender, pxGmxIncentive);

        pxGmx.safeTransfer(owner, totalPxGmxFee - pxGmxIncentive);

        // Update the pxGmx reward accrual
        _harvest(pxGmxAmountOut - totalPxGmxFee);
    } else {
        // Required to keep the globalState up-to-date
        _globalAccrue();
    }

function _harvest(uint256 rewardAmount) internal {
    // Update global reward accrual state and associate with the update of reward state
    _globalAccrue();

    if (rewardAmount != 0) {
        rewardState += rewardAmount;

        emit Harvest(rewardAmount);
    }
}

AutoPxGlp internally accounts for pxGmx with the _harvest subcall in compound. The amount of pxGmx received by claiming from the PirexRewards is passed in updating the rewardState, which is used to calculate pxGmx distribution. This accounting is only accurate if claim is only ever called by compound. The issue is that PirexRewards#claim is external and can be called by anyone for any account. An adversary can use this to lock the rewards so they can never be distributed.

Example: AutoPxGlp has a balance of 10 pxGmx and rewardState has been accurately updated so it also has a balance of 10. Now there is 1 pxGmx available to be claimed for the vault. An adversary calls PirexRewards#claim for the vault sending it 1 pxGmx. Later the there is 1 pxGmx available to claim and the vault calls compound with a current balance of 11 pxGmx. It claims, increasing the balance of the vault to 12 pxGmx. Harvest is called with the difference (12 - 11 = 1) increasing rewardState to 11 (10 + 1). The balances are now forever out of sync and all rewards collected by the adversary cannot be distributed.

Tools Used

Manual Review

Recommended Mitigation Steps

Change the harvest subcall in compound to:

-   _harvest(pxGmxAmountOut - totalPxGmxFee);
+   _harvest(pxGmx.balanceOf(address(this)) - rewardState);

Lines of code

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

Vulnerability details

Impact

PirexGMX migration can always be DOS'd if owner is multisig

Proof of Concept

GMX Reward Router

function _validateReceiver(address _receiver) private view {
    require(IRewardTracker(stakedGmxTracker).averageStakedAmounts(_receiver) == 0, "RewardRouter: stakedGmxTracker.averageStakedAmounts > 0");
    require(IRewardTracker(stakedGmxTracker).cumulativeRewards(_receiver) == 0, "RewardRouter: stakedGmxTracker.cumulativeRewards > 0");

RewardRouterV2 requires that the new user accepting a migration has never had any esGMX. This can be exploited to block any migration attempt made by PirexGMX. esGMX is currently non-transferable but esGMX can be sent to any account by claiming fsGLP with that account as the claim receiver.

fsGLP

function claim(address _receiver) external override nonReentrant returns (uint256) {
    //@audit inPrivateClaimingMode = false currently
    if (inPrivateClaimingMode) { revert("RewardTracker: action not enabled"); } 
    return _claim(msg.sender, _receiver);
}

To block a migration an adversary could claim to the migration target, giving it an esGMX balance and permanently blocking the target from receiving the account transfer. As a mutlisig there would be no way to avoid this DOS. Even in the migration was done as a batch call (initiate migration, create new contract, complete migration) and adversary could precompute the migration target after the transaction has been initiated on the multisig and is waiting for approval by multisig members.

Tools Used

Manual Review

Recommended Mitigation Steps

Migration should be a separate permission from owner, allowing migration to be handled by an external smart contract.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L242-L313 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L210-L296

Vulnerability details

Impact

Rewards can be stolen by sandwich attacking compound calls

Proof of Concept

function compound(
    uint24 fee,
    uint256 amountOutMinimum,
    uint160 sqrtPriceLimitX96,
    bool optOutIncentive
)
    public
    returns (
        uint256 gmxBaseRewardAmountIn,
        uint256 gmxAmountOut,
        uint256 pxGmxMintAmount,
        uint256 totalFee,
        uint256 incentive
    )
{

Compound is a public function allowing the caller to specify the minOut of the swap. An adversary can call this with a very small amountOutMinimum value (removing all slippage protection) then sandwich attack the swap to steal all the rewards.

Tools Used

Manual Review

Recommended Mitigation Steps

Use chainlink oracles to create a slippage bounds for compound

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L130-L136

Vulnerability details

Impact

The old platform still has an allowance for gmxBaseReward after being changed

Proof of Concept

function setPlatform(address _platform) external onlyOwner {
    if (_platform == address(0)) revert ZeroAddress();

    platform = _platform;

    emit PlatformUpdated(_platform);
}

In the constructor platform is given a max allowance for gmxBaseReward. When the platform is changed by setPlatform the allowance for the old platform isn't removed, allowing it to continue to pull assets from AutoPxGmx.

Tools Used

Manual Review

Recommended Mitigation Steps

Remove approval from old platform when setting new platform:

    function setPlatform(address _platform) external onlyOwner {
        if (_platform == address(0)) revert ZeroAddress();

+       gmxBaseReward.safeApprove(platform, 0);

        platform = _platform;

        emit PlatformUpdated(_platform);
    }