there is a 15 min cooldown duration after minting GLP in GMX, user can DOS (block) the PirexGmx.sol#redeemPxGlp call by keep extending the 15 minutes cooldown by calling AutoPxGlp#depositGlp with a small amount of Glp

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L394
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L602
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L510
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L730
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L657

Vulnerability details

Impact

there is a 15 min cooldown duration after minting GLP, user can DOS (block) the PirexGmx.sol#redeemPxGlp call by keep extending the 15 minutes cooldown by calling AutoPxGlp#depositGlp with a small amount of Glp

Proof of Concept

I want to quote a special timelock mechanism in GMX:

https://gmxio.gitbook.io/gmx/contracts#transferring-staked-glp

Since there is a 15 min cooldown duration after minting GLP, this amount of time needs to pass for the user before transferFrom can be called for their account.

How does this cooldown period implemented and how is affected the PirexGmx.sol contract and the Auto
PxGlp.sol contract?

Why do I say that a user can DOS the redeem function can extending the cool down period?

Let us take a top-down approach and look into the contract code:

User can call AutoPxGlp.sol#depositGlp to Deposit GLP (minted with ERC20 tokens) for apxGLP

function depositGlp(
	address token,
	uint256 tokenAmount,
	uint256 minUsdg,
	uint256 minGlp,
	address receiver
) external nonReentrant returns (uint256) {

which calls:

(, uint256 assets, ) = PirexGmx(platform).depositGlp(
	token,
	tokenAmount,
	minUsdg,
	minGlp,
	address(this)
);

which calls PirexGmx.sol#depositGlp

function depositGlp(
	address token,
	uint256 tokenAmount,
	uint256 minUsdg,
	uint256 minGlp,
	address receiver
)

which calls:

return _depositGlp(token, tokenAmount, minUsdg, minGlp, receiver);

which calls:

ERC20 t = ERC20(token);

// Intake user ERC20 tokens and approve GLP Manager contract for amount
t.safeTransferFrom(msg.sender, address(this), tokenAmount);
t.safeApprove(glpManager, tokenAmount);

// Mint and stake GLP using ERC20 tokens
deposited = gmxRewardRouterV2.mintAndStakeGlp(
	token,
	tokenAmount,
	minUsdg,
	minGlp
);

this call is very important:

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

it is calling:

https://arbiscan.io/address/0xA906F338CB21815cBc4Bc87ace9e68c87eF8d8F1#code#F1#L128

function mintAndStakeGlp(address _token, uint256 _amount, uint256 _minUsdg, uint256 _minGlp) external nonReentrant returns (uint256) {
	require(_amount > 0, "RewardRouter: invalid _amount");

	address account = msg.sender;
	uint256 glpAmount = IGlpManager(glpManager).addLiquidityForAccount(account, account, _token, _amount, _minUsdg, _minGlp);
	IRewardTracker(feeGlpTracker).stakeForAccount(account, account, glp, glpAmount);
	IRewardTracker(stakedGlpTracker).stakeForAccount(account, account, feeGlpTracker, glpAmount);

	emit StakeGlp(account, glpAmount);

	return glpAmount;
}

which calls:

uint256 glpAmount = IGlpManager(glpManager).addLiquidityForAccount(account, account, _token, _amount, _minUsdg, _minGlp);

this is also important:

we are calling:

https://arbiscan.io/address/0x321F653eED006AD1C29D174e17d96351BDe22649#code#L841

function addLiquidityForAccount(address _fundingAccount, address _account, address _token, uint256 _amount, uint256 _minUsdg, uint256 _minGlp) external override nonReentrant returns (uint256) {
	_validateHandler();
	return _addLiquidity(_fundingAccount, _account, _token, _amount, _minUsdg, _minGlp);
}

which calls:

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

crucial notes about this section:

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

lastAddedAt[_account] = block.timestamp;

how does the code use this lastAddedAt[_account] to implement the 15 minutes cooldown period?

it is used here:

https://arbiscan.io/address/0x321F653eED006AD1C29D174e17d96351BDe22649#code#L936

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

this means, within the 15 minutes cooldown period, if removeLiqudity is called, the transaction revert.

did we use this function? Yes.

The call stack is:

PirexGmx.sol#redeemPxGlp -> gmxRewardRouterV2.unstakeAndRedeemGlp ->

uint256 amountOut = IGlpManager(glpManager).removeLiquidityForAccount(account, _tokenOut, _glpAmount, _minOut, _receiver);

https://arbiscan.io/address/0xA906F338CB21815cBc4Bc87ace9e68c87eF8d8F1#code#F1#L164

which calls:

 return _removeLiquidity(_account, _tokenOut, _glpAmount, _minOut, _receiver);

https://arbiscan.io/address/0x321F653eED006AD1C29D174e17d96351BDe22649#code#L853

which revert in 15 cooldown period:

https://arbiscan.io/address/0x321F653eED006AD1C29D174e17d96351BDe22649#code#L936

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

the user can keep using 1 amount of GLP token to call AutoPxGlp#depositGlp to keep extending the 15 minutes to another 15 minutes cooldown period, then PirexGmx.sol#redeemPxGlp revert.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project add rate limit to not let user call depositGlp too frequently to not let transaction revert in cooldown period.

[c4-judge]

Picodes marked the issue as duplicate of #110

[c4-judge]

Picodes marked the issue as duplicate of #113

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 3 (High Risk)