Redacted Cartel contest - ladboy233's results

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.

Lines of code

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

Vulnerability details

Impact

I want to quote:

Per EIP 4626's Security Considerations (https://eips.ethereum.org/EIPS/eip-4626)

Finally, ERC-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users:

If (1) it's calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it's determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down. If (1) it's calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it's calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.

Thus, the result of the previewWithdraw should be rounded up.

In PirexERC4626.sol, the previewWithdraw is rounded up, which is ERC46262 vault rounding complaint.

function previewWithdraw(uint256 assets)
	public
	view
	virtual
	returns (uint256)
{
	uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

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

However, in AutoPxGlp.sol and AutoPxGlp.sol, the previewWithdraw function is override and the result is not rounding up.

Other protocols that integrate with the RedactedCertal protocol might wrongly assume that the functions handle rounding as per ERC4626 expectation. Thus, it might cause some intergration problem in the future that can lead to wide range of issues for both parties.

Proof of Concept

ERC 4626 expects the result returned from previewWithdraw function to be rounded up but the code in previewWithdraw logic below is 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);
}

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project round up the function PreviewWithdraw in AutoPxGlp.sol and AutoPxGlp.sol

Ensure that the rounding of vault's functions behave as expected. Following are the expected rounding direction for each vault function:

previewMint(uint256 shares) - Round Up ⬆ previewWithdraw(uint256 assets) - Round Up ⬆ previewRedeem(uint256 shares) - Round Down ⬇ previewDeposit(uint256 assets) - Round Down ⬇ convertToAssets(uint256 shares) - Round Down ⬇ convertToShares(uint256 assets) - Round Down ⬇

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L60 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L68 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L184 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L156 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L164

Vulnerability details

Impact

A malicious early user/attacker can manipulate the pricePerShare to take an unfair share of future users' deposits

The attacker can profit from future users' deposits. While the late users will lose part of their funds to the attacker.

Proof of Concept

The user needs to call deposit to receive shares:

    function deposit(uint256 assets, address receiver)
        public
        virtual
        returns (uint256 shares)
    {
        if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);

        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

        // Need to transfer before minting or ERC777s could reenter.
        asset.safeTransferFrom(msg.sender, address(this), assets);

        _mint(receiver, shares);

        emit Deposit(msg.sender, receiver, assets, shares);

        afterDeposit(receiver, assets, shares);
    }

note the logic, which calls previewDeposit:

require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

Which calls convertToShares:

function previewDeposit(uint256 assets)
	public
	view
	virtual
	returns (uint256)
{
	return convertToShares(assets);
}

which calls convert to shares:

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

A malicious early user can deposit() in AutoPxGmx and AutoPxGLP, which inherits from PirexERC4626.sol with 1 wei of asset token as the first depositor of the Token, and get 1 wei of shares.

Then the attacker can send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1) .

As a result, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of shares token.

They will immediately lose 9999e18 or half of their deposits if they redeem() right after the deposit().

Tools Used

Manual Review

Recommended Mitigation Steps

Consider requiring a minimal amount of share tokens to be minted for the first minter, and send a port of the initial mints as a reserve to the DAO so that the pricePerShare can be more resistant to manipulation.

Lines of code

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

Vulnerability details

Impact

I want to quote from the doc:

- Does it use a side-chain? Yes
- If yes, is the sidechain evm-compatible? Yes, Avalanche

this shows that the projects is intended to support Avalanche side-chain.

SWAP_ROUTER in the AutoPxGmx.sol is hardcoded as:

IV3SwapRouter public constant SWAP_ROUTER =
	IV3SwapRouter(0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45);

but this is address is the Uniswap V3 router address in arbitrium, but it is a EOA address in Avalanche,

https://snowtrace.io/address/0x68b3465833fb72a70ecdf485e0e4c7bd8665fc45

then the AutoPxGmx.sol is not working in Avalanche.

gmxAmountOut = SWAP_ROUTER.exactInputSingle(
	IV3SwapRouter.ExactInputSingleParams({
		tokenIn: address(gmxBaseReward),
		tokenOut: address(gmx),
		fee: fee,
		recipient: address(this),
		amountIn: gmxBaseRewardAmountIn,
		amountOutMinimum: amountOutMinimum,
		sqrtPriceLimitX96: sqrtPriceLimitX96
	})
);

Proof of Concept

the code below revert because the EOA address on Avalanche does not have exactInputSingle method in compound method.

gmxAmountOut = SWAP_ROUTER.exactInputSingle(
	IV3SwapRouter.ExactInputSingleParams({
		tokenIn: address(gmxBaseReward),
		tokenOut: address(gmx),
		fee: fee,
		recipient: address(this),
		amountIn: gmxBaseRewardAmountIn,
		amountOutMinimum: amountOutMinimum,
		sqrtPriceLimitX96: sqrtPriceLimitX96
	})
);

/**
	@notice Compound pxGMX rewards
	@param  fee                    uint24   Uniswap pool tier fee
	@param  amountOutMinimum       uint256  Outbound token swap amount
	@param  sqrtPriceLimitX96      uint160  Swap price impact limit (optional)
	@param  optOutIncentive        bool     Whether to opt out of the incentive
	@return gmxBaseRewardAmountIn  uint256  GMX base reward inbound swap amount
	@return gmxAmountOut           uint256  GMX outbound swap amount
	@return pxGmxMintAmount        uint256  pxGMX minted when depositing GMX
	@return totalFee               uint256  Total platform fee
	@return incentive              uint256  Compound incentive
 */
function compound(
	uint24 fee,
	uint256 amountOutMinimum,
	uint160 sqrtPriceLimitX96,
	bool optOutIncentive
)

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project not hardcode the SWAP_ROUTER in AutoPxGmx.sol, can pass this parameter in the constructor.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L210 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L441 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L454 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L467 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L242 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L227 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L321 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L345

Vulnerability details

Impact

User may take fund loss because the slippage amount is hardcoded in AutoPxGlp.sol and AutoPxGmx.sol.

Proof of Concept

In AutoPxGlp.sol, the function compound is called when the user wants to compound the reward:

/**
	@notice Compound pxGLP (and additionally pxGMX) rewards
	@param  minUsdg                uint256  Minimum USDG amount used when minting GLP
	@param  minGlp                 uint256  Minimum GLP amount received from the WETH deposit
	@param  optOutIncentive        bool     Whether to opt out of the incentive
	@return gmxBaseRewardAmountIn  uint256  WETH inbound amount
	@return pxGmxAmountOut         uint256  pxGMX outbound amount
	@return pxGlpAmountOut         uint256  pxGLP outbound amount
	@return totalPxGlpFee          uint256  Total platform fee for pxGLP
	@return totalPxGmxFee          uint256  Total platform fee for pxGMX
	@return pxGlpIncentive         uint256  Compound incentive for pxGLP
	@return pxGmxIncentive         uint256  Compound incentive for pxGMX
 */
function compound(
	uint256 minUsdg,
	uint256 minGlp,
	bool optOutIncentive
)

the two parameter, minUsdg and minGlp are used as slippage protection.

if (gmxBaseRewardAmountIn != 0) {
	// Deposit received rewards for pxGLP
	(, pxGlpAmountOut, ) = PirexGmx(platform).depositGlp(
		address(gmxBaseReward),
		gmxBaseRewardAmountIn,
		minUsdg,
		minGlp,
		address(this)
	);
}

which calls PirexMax.sol#depositGlp

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

which is calling the GMX contract:

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

note the line:

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

which calls:

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

note the section:

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

the minUsdg and minGlp are used to protect user from slippage loss so user can receive proper amount of glpAmount liqudity token when adding liqudity.

but in AutoPxGlp.sol, the slippage amount for minUsdg and minGlp are hardcoded as 1

compound(1, 1, true);

in AutoPxGmx.sol, the compound function is implemented below:

/**
	@notice Compound pxGMX rewards
	@param  fee                    uint24   Uniswap pool tier fee
	@param  amountOutMinimum       uint256  Outbound token swap amount
	@param  sqrtPriceLimitX96      uint160  Swap price impact limit (optional)
	@param  optOutIncentive        bool     Whether to opt out of the incentive
	@return gmxBaseRewardAmountIn  uint256  GMX base reward inbound swap amount
	@return gmxAmountOut           uint256  GMX outbound swap amount
	@return pxGmxMintAmount        uint256  pxGMX minted when depositing GMX
	@return totalFee               uint256  Total platform fee
	@return incentive              uint256  Compound incentive
 */
function compound(
	uint24 fee,
	uint256 amountOutMinimum,
	uint160 sqrtPriceLimitX96,
	bool optOutIncentive
)

which calls:

gmxAmountOut = SWAP_ROUTER.exactInputSingle(
IV3SwapRouter.ExactInputSingleParams({
	tokenIn: address(gmxBaseReward),
	tokenOut: address(gmx),
	fee: fee,
	recipient: address(this),
	amountIn: gmxBaseRewardAmountIn,
	amountOutMinimum: amountOutMinimum,
	sqrtPriceLimitX96: sqrtPriceLimitX96
})
);

the parameter amountOutMinimum is important becacuse it protects user's from slippage.

but the amountOutMinimum is hardcoded as 1 in AutoPxGmx.sol

compound(poolFee, 1, 0, true);

In AutoPxGlp.sol

the slippage amount is hardcoded as 1 wei, but the token as 18 decimals, (count 10 ** 18) as one token, so 1 wei comparing to 10 ** 18 is a very neligible amount.

if we look at the code below:

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

basically less usdgAmount means less GlpToken minted. if we just use 1 wei as liquidity protection, the usdgAmount from vault.buyUSDG can be arbitraily low, then the minGlp minted can be sufficient less than we expected.

In AutoPxGmx.sol

the slippage amount is hardcoded as 1 wei, but the token as 18 decimals, (count 10 ** 18) as one token, so 1 wei comparing to 10 ** 18 is a very neligible amount.

then we convert from address(gmxBaseReward), for example, WETH to GMX, the user's trade is vulnerable to front-running and sandwiching attack.

the front-running bot can buy before user buy to push the price of GMX token up, after user buy, the bot backrun the user's trade and dump the token on user.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project not hard code the min amount as 1 wei and estimate a reasonable amount of min amount to not let user loss fund in slippage.

Lines of code

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

Vulnerability details

Impact

When updating the platform address, the user is not able to deposit GMX and GLP and compounded token because the allowane is not given to the underlying platform contract.

Proof of Concept

In AutoPxGmx.sol and In AutoPxGlp.sol constructor, the _platform address is set:

@param  _platform       address  Platform address (e.g. PirexGmx)

and in the constructor, max allowance is given to the platform address contract so later when deposit related function is called, the contract has the properly allowane to pull token from the AutoCompound contract.

In AutoPxGmx.sol

// Approve the Uniswap V3 router to manage our base reward (inbound swap token)
gmxBaseReward.safeApprove(address(SWAP_ROUTER), type(uint256).max);
gmx.safeApprove(_platform, type(uint256).max);

In AutoPxGlp.sol

// Approve the Uniswap V3 router to manage our base reward (inbound swap token)
gmxBaseReward.safeApprove(address(_platform), type(uint256).max);

Ok. The contract has another priviledge method:

/**
	@notice Set the platform
	@param  _platform  address  Platform
 */
function setPlatform(address _platform) external onlyOwner {
	if (_platform == address(0)) revert ZeroAddress();

	platform = _platform;

	emit PlatformUpdated(_platform);
}

the owner can update the platform address, but here is the issue: the allowance is not given when the address platform is updated.

Then when the token reward is compounded or a normal deposit is called, transation revert in insufficient allowance:

In AutoPxGmx.sol, we are calling deposit when compounding and depositGmx normally.

We need to give the platform address properly allowane, but the allowance is missing after the platform address is updated.

// Deposit entire GMX balance for pxGMX, increasing the asset/share amount
(, pxGmxMintAmount, ) = PirexGmx(platform).depositGmx(
	gmx.balanceOf(address(this)),
	address(this)
);

In AutoPxGlp.sol, we are calling the deposit in function depositGlp

We need to give the platform address properly allowane, but the allowance is missing after the platform address is updated.

// Approve as needed here since it can be a new whitelisted token (unless it's the baseReward)
if (erc20Token != gmxBaseReward) {
	erc20Token.safeApprove(platform, tokenAmount);
}

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

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend give the platform address proper allowance right before the deposit funtion is called (not even the type(uint256).max to reduce the risk)

For example, in AutoPxGmx.sol, we can implementation:

// Intake sender GMX
gmx.safeTransferFrom(msg.sender, address(this), amount);

gmx.safeApprove(platform, amount);
// Convert sender GMX into pxGMX and get the post-fee amount (i.e. assets)
(, uint256 assets, ) = PirexGmx(platform).depositGmx(
	amount,
	address(this)
);