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

Unsafe Admin State Combinations in setVaultState Allow Protocol Deadlock via Invalid Invariants

codertjay

Summary

The setVaultState function allows setting multiple critical state variables without validating their relationships,
potentially creating invalid states that could block protocol operations.

The setVaultState function in PerpetualVault allows the admin to set critical state variables without validating their
logical relationships, potentially leading to protocol deadlock. An admin can set contradictory states such as having a
DEPOSIT flow with an active position (positionIsClosed = false), a NONE flow with active GMX lock (_gmxLock = true), or
a withdrawal flow with incompatible next actions (FLOW.WITHDRAW with INCREASE_ACTION). These invalid state combinations
violate core protocol invariants and can permanently block critical functions like deposits, withdrawals, and position
management. This is especially dangerous as these states cannot be fixed
through normal protocol operations once set.

Vulnerability Details

Here are the dangerous combinations that should never exist together:

  1. Flow and Position State Mismatch:

// DANGEROUS: DEPOSIT flow with positionIsClosed = false
setVaultState(
FLOW.DEPOSIT, // New deposits
_flowData,
_beenLong,
_curPositionKey, // Non-zero position key
false, // Position not closed
_isLock,
_nextAction
);

  1. Lock and Flow State Conflict:

// DANGEROUS: NONE flow with _gmxLock = true
setVaultState(
FLOW.NONE, // No active flow
_flowData,
_beenLong,
_curPositionKey,
_positionIsClosed,
true, // GMX locked
_nextAction
);

  1. Position Key and Closed State Inconsistency:

// DANGEROUS: Non-zero position key with positionIsClosed = true
setVaultState(
_flow,
_flowData,
_beenLong,
bytes32(123), // Non-zero position key
true, // But position marked as closed
_isLock,
_nextAction
);

  1. NextAction and Flow Mismatch:

// DANGEROUS: WITHDRAW flow with INCREASE_ACTION
setVaultState(
FLOW.WITHDRAW,
_flowData,
_beenLong,
_curPositionKey,
_positionIsClosed,
_isLock,
Action({ // Incompatible next action
selector: NextActionSelector.INCREASE_ACTION,
data: ""
})
);

Proof Of Concept

Here's a test demonstrating these invalid states:

​
function test_InvalidStatesCombinations() public {
address owner = PerpetualVault(vault).owner();
vm.startPrank(owner);
​
// Test 1: DEPOSIT flow with active position
bytes32 fakePositionKey = bytes32(uint256(1));
PerpetualVault.Action memory action = PerpetualVault.Action({
selector: PerpetualVault.NextActionSelector.NO_ACTION,
data: ""
});
​
PerpetualVault(vault).setVaultState(
PerpetualVault.FLOW.DEPOSIT,
0, // flowData
true, // beenLong
fakePositionKey,
false, // positionIsClosed = false (active position)
false, // _isLock
action
);
​
// Now deposits should fail
vm.stopPrank();
address alice = makeAddr("alice");
uint256 amount = 1e10;
vm.startPrank(alice);
vm.expectRevert(Error.FlowInProgress.selector);
PerpetualVault(vault).deposit(amount);
vm.stopPrank();
}
​

The Foundry Response

➜ 2025-02-gamma git:(main) ✗ forge test --mt test_InvalidStatesCombinations -vvvv --via-ir --rpc-url arbitrum
[⠒] Compiling...
[⠘] Compiling 1 files with Solc 0.8.28
[⠃] Solc 0.8.28 finished in 44.75s
Compiler run successful with warnings:
Warning (2072): Unused local variable.
Ran 1 test for test/PerpetualVault.t.sol:PerpetualVaultTest
[PASS] test_InvalidStatesCombinations() (gas: 111078)
Traces:......
│ └─ ← [Return]
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6]
├─ [0] VM::label(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], "alice")
│ └─ ← [Return]
├─ [0] VM::startPrank(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6])
│ └─ ← [Return]
├─ [0] VM::expectRevert(custom error 0xc31eb0e0: 08cf371700000000000000000000000000000000000000000000000000000000)
│ └─ ← [Return]
├─ [9099] TransparentUpgradeableProxy::fallback(10000000000 [1e10])
│ ├─ [7675] PerpetualVault::deposit(10000000000 [1e10]) [delegatecall]
│ │ └─ ← [Revert] FlowInProgress()
│ └─ ← [Revert] FlowInProgress()
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Return]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 12.97ms (733.35µs CPU time)
​
Ran 2 test suites in 3.18s (25.88ms CPU time): 2 tests passed, 0 failed, 0 skipped (2 total tests)
➜ 2025-02-gamma git:(main) ✗
​

Impact

  • Protocol operations can become permanently blocked

  • Inconsistent states can prevent deposits, withdrawals, or position management

  • Recovery may require contract upgrade

Recommendations

  1. Add state validation checks:

function setVaultState(
FLOW _flow,
uint256 _flowData,
bool _beenLong,
bytes32 _curPositionKey,
bool _positionIsClosed,
bool _isLock,
Action memory _nextAction
) external onlyOwner {
// Validate flow and position state
if (_flow == FLOW.DEPOSIT) {
require(_positionIsClosed == true, "Cannot set DEPOSIT flow with active position");
}
​
// Validate lock and flow state
if (_flow == FLOW.NONE) {
require(_isLock == false, "Cannot set lock with NONE flow");
}
​
// Validate position key and closed state
if (_curPositionKey != bytes32(0)) {
require(_positionIsClosed == false, "Position key incompatible with closed state");
}
​
// Validate next action and flow compatibility
if (_flow == FLOW.WITHDRAW) {
require(_nextAction.selector != NextActionSelector.INCREASE_ACTION,
"Invalid next action for withdrawal");
}
​
// Set states after validation
flow = _flow;
flowData = _flowData;
beenLong = _beenLong;
curPositionKey = _curPositionKey;
positionIsClosed = _positionIsClosed;
_gmxLock = _isLock;
nextAction = _nextAction;
}

  1. Consider breaking this into smaller, more focused state update functions that maintain invariants.

  2. Add events to track state changes for monitoring.

The key is to prevent combinations that violate the protocol's core assumptions about state relationships.

Updates

Lead Judging Commences

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