Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Owner Can Instantly Update Critical GMX Integration Addresses Without Timelock, Endangering Active Orders and User Funds

codertjay

Summary

Critical vulnerability in updateGmxAddresses allows owner to instantly update all GMX integration addresses without
timelock, potentially disrupting protocol operations and active user positions while an order is in progress.

Currently we know the owner has the right to make lot of modification but what if there is a queue.requestKey which the bytes is not empty that could affect the order. So i feel the update still have to follow some certain procedures

According to the rules owner is able to update gmxAddress but that doesnt mean he should make modification any way he feels like when an order is still on.

For example an order is opened and the owner can set router address to zero which doesnt sit well.

I agree the owner have full permission but if the owner makes any mistake it affect users so there could be a minimal verification the owner is providing the right information at the very least.

Like it doesnt stop anything to even check if address(0) is provided by the owner when there is an active order .

Vulnerability Details

The vulnerable function:

function updateGmxAddresses(
address _orderHandler,
address _liquidationHandler,
address _adlHandler,
address _gExchangeRouter,
address _gmxRouter,
address _dataStore,
address _orderVault,
address _reader,
address _referralStorage
) external onlyOwner {
// Instant update of all addresses
orderHandler = _orderHandler;
liquidationHandler = _liquidationHandler;
adlHandler = _adlHandler;
gExchangeRouter = IExchangeRouter(_gExchangeRouter);
gmxRouter = _gmxRouter;
dataStore = IDataStore(_dataStore);
orderVault = _orderVault;
gmxReader = IGmxReader(_reader);
referralStorage = _referralStorage;
}

Key issues:

  1. No timelock for address changes

  2. No check for active orders in queue

OrderQueue public queue; // Not checked before address updates

  1. Can affect ongoing order processing:

function afterOrderExecution(...) {
// Uses addresses that could be changed mid-order
MarketProps memory marketInfo = gmxReader.getMarket(address(dataStore), market);
// ... more operations using these addresses
}

Impact

HIGH severity because:

  1. Active orders could be disrupted by address changes

  2. Users could lose funds if malicious addresses are set

  3. No time for users to react to changes

  4. Protocol integration with GMX could break mid-operation

Tools Used

  • Manual code review

Proof Of Concept

This POC demonstrates:

  • Creates an active order that enters the queue

  • Updates GMX addresses while order is pending

  • Shows failure on cancellation of order

  • Order becomes stuck in queue

​
​
function test_UpdateGmxAddressesDuringActiveOrder() public {
address gmxProxyAddress = address(PerpetualVault(vault).gmxProxy());
GmxProxy gmxProxy = GmxProxy(payable(gmxProxyAddress));
​
// Setup: Create a deposit first
address alice = makeAddr("alice");
payable(alice).transfer(1 ether);
​
// Get USDC token and setup deposit
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
address whale = 0x489ee077994B6658eAfA855C308275EAd8097C4A;
vm.startPrank(whale);
uint256 amount = 1e10;
collateralToken.transfer(alice, amount);
vm.stopPrank();
​
// Create deposit and order
vm.startPrank(alice);
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(true);
collateralToken.approve(vault, amount);
PerpetualVault(vault).deposit{value: executionFee * tx.gasprice}(amount);
vm.stopPrank();
​
// Start order creation
MarketPrices memory prices = mockData.getMarketPrices();
bytes[] memory data = new bytes[](2);
data[0] = abi.encode(3380000000000000);
​
address keeperOwner = KeeperProxy(keeper).owner();
vm.prank(keeperOwner);
KeeperProxy(keeper).run(vault, true, false, prices, data);
​
// Verify order exists in queue
(bytes32 requestKey,) = gmxProxy.queue();
assertTrue(requestKey != bytes32(0), "Order should exist in queue");
​
// Store old addresses
address oldOrderHandler = gmxProxy.orderHandler();
address oldGmxRouter = gmxProxy.gmxRouter();
​
// Update GMX addresses while order is active
address owner = gmxProxy.owner();
vm.startPrank(owner);
​
// Use actual GMX contract addresses but wrong ones
gmxProxy.updateGmxAddresses(
address(0xe68CAAACdf6439628DFD2fe624847602991A31eB), // Different orderHandler
address(0xdAb9bA9e3a301CCb353f18B4C8542BA2149E4010), // Different liquidationHandler
address(0x9242FbED25700e82aE26ae319BCf68E9C508451c), // Different adlHandler
address(0), // Different gExchangeRouter
address(0), // Different gmxRouter
address(0xFD70de6b91282D8017aA4E741e9Ae325CAb992d8), // Different dataStore
address(0x31eF83a530Fde1B38EE9A18093A333D8Bbbc40D5), // Different orderVault
address(0x5Ca84c34a381434786738735265b9f3FD814b824), // Different reader
address(0xe6fab3F0c7199b0d34d7FbE83394fc0e0D06e99d) // Different referralStorage
);
vm.stopPrank();
​
// Verify addresses changed during active order
assertNotEq(gmxProxy.gmxRouter(), oldGmxRouter, "GmxRouter should be different");
​
// Verify order is still in queue but with changed dependencies
(bytes32 queuedKey,) = gmxProxy.queue();
assertEq(queuedKey, requestKey, "Order key should remain unchanged");
​
// Try to process order with new addresses
vm.warp(block.timestamp + 1000);
vm.expectRevert();
KeeperProxy(keeper).cancelOrder(vault); // Should fail since the gmxRouter has been updated
​
}
​

The foundry Response

➜ 2025-02-gamma git:(main) ✗ forge test --mt test_UpdateGmxAddressesDuringActiveOrder -vvvv --via-ir --rpc-url arbitrum
​
Ran 1 test for test/KeeperProxy.t.sol:KeeperProxyTest
[PASS] test_UpdateGmxAddressesDuringActiveOrder() (gas: 1705472)
Traces:
[1888272] KeeperProxyTest::test_UpdateGmxAddressesDuringActiveOrder()
├─ [11090] TransparentUpgradeableProxy::fallback() [staticcall]
│ ├─ [3170] PerpetualVault::gmxProxy() [delegatecall]
│ │ ....
├─ [17395] TransparentUpgradeableProxy::fallback(TransparentUpgradeableProxy: [0x03A6a84cD762D9707A21605b548aaaB891562aAb])
│ ├─ [15974] KeeperProxy::cancelOrder(TransparentUpgradeableProxy: [0x03A6a84cD762D9707A21605b548aaaB891562aAb]) [delegatecall]
│ │ ├─ [13899] TransparentUpgradeableProxy::fallback()
│ │ │ ├─ [12481] PerpetualVault::cancelOrder() [delegatecall]
│ │ │ │ ├─ [6191] TransparentUpgradeableProxy::fallback()
│ │ │ │ │ ├─ [4773] GmxProxy::cancelOrder() [delegatecall]
│ │ │ │ │ │ └─ ← [Revert] EvmError: Revert
│ │ │ │ │ └─ ← [Revert] EvmError: Revert
│ │ │ │ └─ ← [Revert] EvmError: Revert
│ │ │ └─ ← [Revert] EvmError: Revert
│ │ └─ ← [Revert] EvmError: Revert
│ └─ ← [Revert] EvmError: Revert
└─ ← [Return]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 18.48ms (11.29ms CPU time)
​
Ran 1 test suite in 2.89s (18.48ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommendations

Implement a timelock system for address updates:

This modification we make sure there isn't any active order or i feel we check for address zero also to just prevent active order from breaking

Fix 1:

function updateGmxAddresses(
address _orderHandler,
address _liquidationHandler,
address _adlHandler,
address _gExchangeRouter,
address _gmxRouter,
address _dataStore,
address _orderVault,
address _reader,
address _referralStorage
) external onlyOwner {
require(queue.requestKey == bytes32(0), "Active order exists");
require(_orderHandler != address(0), "No zero address");
require(_liquidationHandler != address(0), "No zero address");
require(_adlHandler != address(0), "No zero address");
require(_gExchangeRouter != address(0), "No zero address");
require(_gmxRouter != address(0), "No zero address");
require(_dataStore != address(0), "No zero address");
require(_orderVault != address(0), "No zero address");
orderHandler = _orderHandler;
liquidationHandler = _liquidationHandler;
adlHandler = _adlHandler;
gExchangeRouter = IExchangeRouter(_gExchangeRouter);
gmxRouter = _gmxRouter;
dataStore = IDataStore(_dataStore);
orderVault = _orderVault;
gmxReader = IGmxReader(_reader);
referralStorage = _referralStorage;
}

Fix 2:

struct PendingAddresses {
address orderHandler;
address liquidationHandler;
address adlHandler;
address gExchangeRouter;
address gmxRouter;
address dataStore;
address orderVault;
address reader;
address referralStorage;
uint256 effectiveTime;
}
​
PendingAddresses public pendingAddresses;
uint256 public constant UPDATE_DELAY = 2 days;
​
function proposeGmxAddresses(...) external onlyOwner {
require(queue.requestKey == bytes32(0), "Active order exists");
pendingAddresses = PendingAddresses({
...
effectiveTime: block.timestamp + UPDATE_DELAY
});
}
​
function executeGmxAddressesUpdate() external onlyOwner {
require(block.timestamp >= pendingAddresses.effectiveTime, "Delay not met");
require(queue. == bytes32(0), "Active order exists");
// Update addresses
}

This ensures:

  1. Changes have mandatory delay

  2. Cannot update during active orders

  3. Users have time to exit positions

  4. Protocol stability maintained

Updates

Lead Judging Commences

n0kto Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

n0kto Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Admin is trusted / Malicious keepers

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point. Keepers are added by the admin, there is no "malicious keeper" and if there is a problem in those keepers, that's out of scope. ReadMe and known issues states: " * System relies heavily on keeper for executing trades * Single keeper point of failure if not properly distributed * Malicious keeper could potentially front-run or delay transactions * Assume that Keeper will always have enough gas to execute transactions. There is a pay execution fee function, but the assumption should be that there's more than enough gas to cover transaction failures, retries, etc * There are two spot swap functionalies: (1) using GMX swap and (2) using Paraswap. We can assume that any swap failure will be retried until success. " " * Heavy dependency on GMX protocol functioning correctly * Owner can update GMX-related addresses * Changes in GMX protocol could impact system operations * We can assume that the GMX keeper won't misbehave, delay, or go offline. " "Issues related to GMX Keepers being DOS'd or losing functionality would be considered invalid."

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.