Redacted Cartel contest - cccz's results

Lines of code

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

Vulnerability details

Impact

When calling AutoPx*.previewWithdraw in AutoPx*.withdraw, the comment says "No need to check for rounding error, previewWithdraw rounds up".

    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public override returns (uint256 shares) {
        // Compound rewards and ensure they are properly accounted for prior to withdrawal calculation
        compound(poolFee, 1, 0, true);

        shares = previewWithdraw(assets); // No need to check for rounding error, previewWithdraw rounds up.

But only PirexERC4626.previewWithdraw rounds up. AutoPx*.previewWithdraw overrides PirexERC4626.previewWithdraw, but does 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);
    }
...
    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());
    }

This breaks the developer's intention and does not comply with the EIP-4626 specification

Proof of Concept

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L199-L217 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L177-L195 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L99-L105 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L315-L323

Tools Used

None

Recommended Mitigation Steps

Call PirexERC4626.previewWithdraw instead of convertToShares in AutoPx*.previewWithdraw

    function previewWithdraw(uint256 assets)
        public
        view
        override
        returns (uint256)
    {
        // Calculate shares based on the specified assets' proportion of the pool
-        uint256 shares = convertToShares(assets);
+       uint256 shares = PirexERC4626.previewWithdraw(assets);

Lines of code

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

Vulnerability details

Impact

A well known attack vector for almost all shares based liquidity pool contracts, where an early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share.

A malicious early user can deposit() with 1 wei of pxGMX as the first depositor of the apxGMX, and get 1 wei of shares.

Then the attacker can send 10000e18 - 1 of pxGMX 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().

Proof of Concept

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L60-L78 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L164-L166 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L339-L362 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L167-L176

Tools Used

None

Recommended Mitigation Steps

Consider requiring a minimal amount of share tokens to be minted for the first minter, and send a part 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#L242-L278

Vulnerability details

Impact

AutoPxGmx.compound allows anyone to call to compound the reward and get the incentive. However, AutoPxGmx.compound calls SWAP_ROUTER.exactInputSingle with some of the parameters provided by the caller, which allows the user to perform a sandwich attack for profit. For example, a malicious user could provide the fee parameter to make the token swap occur in a small liquid pool, and could make the amountOutMinimum parameter 1 to make the token swap accept a large slippage, thus making it easier to perform a sandwich attack.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider using poolFee as the fee and using an onchain price oracle to calculate the amountOutMinimum

Lines of code

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

Vulnerability details

Impact

amountOutMinimum is 1 when compound() is called in some functions of the AutoPxGmx contract, and will be used as the amountOutMinimum parameter when calling SWAP_ROUTER.exactInputSingle

        compound(poolFee, 1, 0, true);

According to the https://docs.uniswap.org/contracts/v3/guides/swaps/single-swaps description, the amountOutMinimum should be calculated using the Uniswap SDK or the onchain price oracle. Simply setting it to 1 may result in losses due to improper slippage control.

Proof of Concept

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L242-L278 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L321-L322 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L345-L346 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L227-L228

Tools Used

None

Recommended Mitigation Steps

Consider using the Uniswap SDK or the onchain price oracle to calculate the amountOutMinimum

Lines of code

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

Vulnerability details

Impact

In the constructor of AutoPx*, when setting the platform, the platform is approved to use the contract's gmx or gmxBaseReward tokens in order to perform the token swap in the compound function.

        platform = _platform;
        rewardsModule = _rewardsModule;

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

However, when the platform is updated in the setPlatform function, the new platform is not approved to use the tokens in the contract. This causes the compound function to fail when the platform is updated because the platform is not approved to use the tokens.

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

        platform = _platform;

        emit PlatformUpdated(_platform);
    }

Proof of Concept

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L152-L158 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L92-L97 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L130-L136 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L83-L87

Tools Used

None

Recommended Mitigation Steps

Change to

    function setPlatform(address _platform) external onlyOwner {
        if (_platform == address(0)) revert ZeroAddress();
+     gmx.safeApprove(platform, 0);
        platform = _platform;
+     gmx.safeApprove(_platform, type(uint256).max);
        emit PlatformUpdated(_platform);
    }
...
    function setPlatform(address _platform) external onlyOwner {
        if (_platform == address(0)) revert ZeroAddress();
+     gmxBaseReward.safeApprove(platform, 0);
        platform = _platform;
+     gmxBaseReward.safeApprove(_platform, type(uint256).max);
        emit PlatformUpdated(_platform);
    }