Redacted Cartel contest - deliriusz's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L210 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol#L497-L516

Vulnerability details

Impact

Compounded assets may be lost because AutoPxGlp::compound can be called by anyone and minimum amount of Glp and USDG are under caller's control. The only check concerning minValues is that they are not zero (1 will work, however from the perspective of real tokens e.g. 1e6, or 1e18 it's virtually zero). Additionally, internal smart contract functions use it as well with minimal possible value (e.g. beforeDeposit function)

Proof of Concept

compound function calls PirexGmx::depositGlp, that uses external GMX reward router to mint and stake GLP. https://snowtrace.io/address/0x82147c5a7e850ea4e28155df107f2590fd4ba327#code

141:     function mintAndStakeGlpETH(uint256 _minUsdg, uint256 _minGlp) external payable nonReentrant returns (uint256) {
    ...
148: uint256 glpAmount = IGlpManager(glpManager).addLiquidityForAccount(address(this), account, weth, msg.value, _minUsdg, _minGlp);

Next GlpManager::addLiquidityForAccount is called https://github.com/gmx-io/gmx-contracts/blob/master/contracts/core/GlpManager.sol#L103

    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 in turn uses vault to swap token for specific amount of USDG before adding liquidity: https://github.com/gmx-io/gmx-contracts/blob/master/contracts/core/GlpManager.sol#L217

The amount of USGD to mint is calcualted by GMX own price feed: https://github.com/gmx-io/gmx-contracts/blob/master/contracts/core/Vault.sol#L765-L767

In times of market turbulence, or price oracle manipulation, all compound value may be lost

Tools Used

VS Code, arbiscan.io

Recommended Mitigation Steps

Don't depend on user passing minimum amounts of usdg and glp tokens. Use GMX oracle to get current price, and additionally check it against some other price feed (e.g. ChainLink)

1. There is double SafeTransferLib import in AutoPxGlp https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L6-L8

2. AutoPxGlp::setPlatform should be checked for being a contract and ideally if supports specific interface (EIP165). This contract is crucial to AutoPxGlp, so additional care should be put into making sure taht changing it won't break anything. https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L130

3. PxGmxReward refers to a function that it does not implement https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PxGmxReward.sol#L72 PxGmxReward contract is not an ERC20, nor does it implement balanceOf function. Because of that lastBalance always will be 0, hence userRewardStates[user].rewards will always be zero, which leads to user loosing all the funds deposited to this contract. Of course at the moment only AutoPxGlp uses this contract, and it does implement ERC20, but it can lead to serious problems when used by itself/inherited from some other contract.

72: uint256 balance = ERC20(address(this)).balanceOf(user);

Mitigation: It should be at least clearly stated in the documentation that this contract is not meant to be used by itself, or possibly checked if class that inherits from it supportsInterface ERC20 (ERC165), or PxGmxReward should implement ERC20 itself