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

Unsafe Emergency State Management - Findings Report

kiarash

Summary

Critical vulnerability in PerpetualVault.sol where the emergency state management function setVaultState() lacks proper validation and safety checks, allowing the admin to set inconsistent states that could lead to system corruption and fund loss.

Vulnerability Details

In PerpetualVault.sol, the setVaultState() function allows direct manipulation of critical system states:

function setVaultState(
FLOW _flow,
uint256 _flowData,
bool _beenLong,
bytes32 _curPositionKey,
bool _positionIsClosed,
bool _isLock,
Action memory _nextAction
) external onlyOwner {
flow = _flow;
flowData = _flowData;
beenLong = _beenLong;
curPositionKey = _curPositionKey;
positionIsClosed = _positionIsClosed;
_gmxLock = _isLock;
nextAction = _nextAction;
}

Key vulnerabilities:

  1. No validation of input parameters

  2. No checks for state consistency

  3. Can directly modify GMX lock state

  4. No event emissions for transparency

  5. Can set contradictory states (e.g. positionIsClosed=true with non-zero curPositionKey)

Impact

  1. System state corruption leading to deadlocks

  2. Potential loss of funds due to inconsistent position states

  3. Interference with ongoing GMX operations

  4. No transparency or tracking of emergency changes

  5. Breaking core protocol assumptions and invariants

Proof of Concept

This PoC demonstrates how the setVaultState function's lack of validation can lead to system corruption through two critical attack vectors:

Scenario 1: State Corruption Attack

This scenario shows how an admin (or compromised admin account) can corrupt the vault's state by setting contradictory values:

  1. Initial Setup:

    • User deposits 5000 USDC into the vault

    • System starts in a clean, consistent state

  2. Attack Execution:

    • Admin calls setVaultState with contradictory parameters

    • Sets positionIsClosed = true while also setting a non-zero positionKey

    • This creates an impossible state (closed position should have zero key)

  3. Impact:

    • Withdrawal functionality breaks due to state inconsistency

    • System cannot recover without contract upgrade

    • User funds effectively locked

Scenario 2: GMX Operation Interference

This scenario demonstrates how the function can be used to interfere with ongoing GMX operations:

  1. Initial Setup:

    • Start a legitimate GMX operation (position change)

    • System sets GMX lock as expected

  2. Attack Execution:

    • Admin interferes mid-operation by calling setVaultState

    • Removes GMX lock while operation is ongoing

    • Changes critical state variables during active operation

  3. Impact:

    • GMX operation atomicity broken

    • Transaction can fail in unexpected ways

    • Potential loss of funds due to incomplete operations

The test cases provided in the PoC code systematically demonstrate both scenarios and their impacts on the system's functionality and user funds.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.4;
​
import "forge-std/Test.sol";
import "../../contracts/PerpetualVault.sol";
import "../../contracts/GmxProxy.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
​
contract EmergencyStateTest is Test {
PerpetualVault vault;
GmxProxy gmxProxy;
address owner;
address user1;
IERC20 usdc;
function setUp() public {
owner = address(this);
user1 = address(0x1);
// Setup with real USDC address on Arbitrum
usdc = IERC20(0xFF970A61A04b1cA14834A43f5dE4533eBDDB5CC8);
// Deploy vault & proxy
vault = new PerpetualVault();
gmxProxy = new GmxProxy();
// Initialize vault
vault.initialize(
0x70d95587d40A2caf56bd97485aB3Eec10Bee6336, // ETH/USD market
address(this), // keeper
address(0xTreasury),
address(gmxProxy),
address(vaultReader),
1000e6, // minDeposit 1000 USDC
100000e6, // maxDeposit 100k USDC
10000 // 1x leverage
);
// Fund test accounts
deal(address(usdc), user1, 10000e6);
}
​
function testStateCorruption() public {
// Step 1: Setup initial state with user deposit
vm.startPrank(user1);
usdc.approve(address(vault), 5000e6);
vault.deposit(5000e6);
vm.stopPrank();
​
// Step 2: Start GMX operation
vault.run(
true, // isOpen
true, // isLong
_mockMarketPrices(),
new bytes[](0)
);
// Verify GMX lock is active
assertTrue(vault.isLock());
// Step 3: Demonstrate state corruption
bytes32 fakePositionKey = bytes32(uint256(1));
vm.startPrank(owner);
// Set contradictory states
vault.setVaultState(
FLOW.NONE, // _flow
0, // _flowData
false, // _beenLong
fakePositionKey, // _curPositionKey
true, // _positionIsClosed - Contradicts non-zero positionKey
false, // _isLock - Dangerously unlocks during GMX op
Action({
selector: NextActionSelector.NO_ACTION,
data: ""
})
);
vm.stopPrank();
​
// Step 4: Demonstrate system corruption
vm.startPrank(user1);
// Try withdrawal - should fail due to inconsistent state
vm.expectRevert(); // System can't handle contradictory position state
vault.withdraw(user1, 1);
vm.stopPrank();
​
// Step 5: Show permanent impact
assertEq(vault.curPositionKey(), fakePositionKey);
assertTrue(vault.positionIsClosed());
// System now has inconsistent state - closed position but non-zero key
}
​
function testGMXInterference() public {
// Step 1: Start position change
vault.run(
true,
true,
_mockMarketPrices(),
new bytes[](0)
);
// Step 2: Admin interferes mid-operation
vm.startPrank(owner);
vault.setVaultState(
FLOW.NONE,
0,
false,
bytes32(0),
true,
false, // Removes GMX lock during operation
Action({
selector: NextActionSelector.NO_ACTION,
data: ""
})
);
vm.stopPrank();
​
// Step 3: Show broken GMX operation
// Try to complete original operation
vm.expectRevert(); // Operation fails due to broken lock state
vault.runNextAction(_mockMarketPrices(), new bytes[](0));
}
function _mockMarketPrices() internal pure returns (MarketPrices memory) {
return MarketPrices({
indexTokenPrice: PriceProps({
min: 2000e30,
max: 2000e30
}),
longTokenPrice: PriceProps({
min: 2000e30,
max: 2000e30
}),
shortTokenPrice: PriceProps({
min: 1e30,
max: 1e30
})
});
}
}

Tools Used

  • Manual code review

  • Foundry for testing

  • Static analysis with Aderyn

Recommended Mitigation

Add comprehensive validation and safety checks:

function setVaultState(
FLOW _flow,
uint256 _flowData,
bool _beenLong,
bytes32 _curPositionKey,
bool _positionIsClosed,
bool _isLock,
Action memory _nextAction
) external onlyOwner {
// Check state consistency
if (_positionIsClosed) {
require(_curPositionKey == bytes32(0), "Invalid position state");
}
// Prevent interference with GMX
require(!_gmxLock || _isLock, "Cannot unlock during GMX operation");
// Validate flow state
if (_flow != FLOW.NONE) {
require(_nextAction.selector != NextActionSelector.NO_ACTION,
"Active flow requires next action");
}
// Store previous state for event
FLOW oldFlow = flow;
bytes32 oldKey = curPositionKey;
// Update states
flow = _flow;
flowData = _flowData;
beenLong = _beenLong;
curPositionKey = _curPositionKey;
positionIsClosed = _positionIsClosed;
_gmxLock = _isLock;
nextAction = _nextAction;
emit EmergencyStateChange(
oldFlow,
flow,
oldKey,
curPositionKey,
positionIsClosed,
_gmxLock
);
}
​
// Add validation helper
function validateVaultState() public view returns (bool, string memory) {
if (positionIsClosed && curPositionKey != bytes32(0)) {
return (false, "Inconsistent position state");
}
if (_gmxLock && flow == FLOW.NONE) {
return (false, "Invalid GMX lock state");
}
return (true, "");
}

Additional recommendations:

  1. Add timelock for emergency state changes

  2. Implement step-by-step state transition functions

  3. Add comprehensive event logging

  4. Create automated state consistency checks

  5. Add emergency pause functionality instead of direct state manipulation

Updates

Lead Judging Commences

n0kto Lead Judge 7 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 7 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.