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

The run and setVaultState functions should implement the gmxLock modifier.

mikebello

Summary

The run and the setVaultState functions should implement the gmxLock modifier to avoid possible calls to it that can mess up the state or the current flow in the Perpetual Vault while the order in the GMX contract hasn't been executed.

Vulnerability Details

The gmxLock modifier is created to avoid a call to a function that can modify the flow or the state of the PerpetualVault contract while it is waiting for the result and the callback from the GMX contracts to finalize the operation.

https://github.com/CodeHawks-Contests/2025-02-gamma/blob/84b9da452fc84762378481fa39b4087b10bab5e0/contracts/PerpetualVault.sol#L113
https://github.com/CodeHawks-Contests/2025-02-gamma/blob/84b9da452fc84762378481fa39b4087b10bab5e0/contracts/PerpetualVault.sol#L150-L156

// Lock the contract from the moment we create position to the moment we get the callback from GMX keeper
modifier gmxLock() {
if (_gmxLock == true) {
revert Error.GmxLock();
}
_;
}

This modifier is correctly implemented in the runNextAction and the cancelFlow so they can't be called again by the keeper while the perpetualVault is waiting for the callback from the GMX contracts to finalize an operation.

https://github.com/CodeHawks-Contests/2025-02-gamma/blob/84b9da452fc84762378481fa39b4087b10bab5e0/contracts/PerpetualVault.sol#L350
https://github.com/CodeHawks-Contests/2025-02-gamma/blob/84b9da452fc84762378481fa39b4087b10bab5e0/contracts/PerpetualVault.sol#L419

function cancelFlow() external nonReentrant gmxLock {
_onlyKeeper();
_cancelFlow();
}
​
function runNextAction(/* Code Omitted */) external nonReentrant gmxLock {
/* Code Omitted */
}

But the run and the setVaultState functions don't use the gmxLock modifier so they can be called by mistake while the PerpetualVault is still waiting for the callback from the GMX contracts to finalize an operation, these functions have cases where they can modify the flow and nextAction variables, this will mess up the state of the perpetualVault contract and avoid the correct finalization of the previous operation that was waiting for the GMX contract callback..

https://github.com/CodeHawks-Contests/2025-02-gamma/blob/84b9da452fc84762378481fa39b4087b10bab5e0/contracts/PerpetualVault.sol#L290-L341
https://github.com/CodeHawks-Contests/2025-02-gamma/blob/84b9da452fc84762378481fa39b4087b10bab5e0/contracts/PerpetualVault.sol#L681-L697

function run(bool isOpen, bool isLong, MarketPrices memory prices, bytes[] memory metadata) external nonReentrant {
_noneFlow();
_onlyKeeper();
// debe tener eth mayor al minimo
if (gmxProxy.lowerThanMinEth()) {
revert Error.LowerThanMinEth();
}
flow = FLOW.SIGNAL_CHANGE;
​
if (isOpen) {
// no se ha abierto una position
if (positionIsClosed) {
// primero transferimos los fondos al contrato, si no revierte
if (_isFundIdle() == false) {
revert Error.InsufficientAmount();
}
if (_isLongOneLeverage(isLong)) {
_runSwap(metadata, true, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createIncreasePosition(isLong, acceptablePrice, prices);
}
} else {
// la posicion no esta cerrada positionIsClosed = false
if (beenLong == isLong) {
revert Error.NoAction();
} else {
// Close current position first and then open the requested position in the next action
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(isLong);
if (_isLongOneLeverage(beenLong)) {
_runSwap(metadata, false, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createDecreasePosition(0, 0, beenLong, acceptablePrice, prices);
}
}
}
} else {
// isOpen = false
// la posicion no esta cerrada. positionIsClosed == false
if (positionIsClosed == false) {
if (_isLongOneLeverage(beenLong)) {
_runSwap(metadata, false, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createDecreasePosition(0, 0, beenLong, acceptablePrice, prices);
}
} else {
revert Error.NoAction();
}
}
}
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;
}

Impact

Calls to run and setVaultState functions, while the previous operation hasn't been finished can mess up the state or the flow in the perpetual vault contract, while the GMX contracts haven't executed the previous order, so this order can be finalized correctly.

Tools Used

Manual Review

Recommendations

Implement the `gmxLock` modifier in the perpetual vault's run and the setVaultState functions.

Updates

Lead Judging Commences

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

Informational or Gas

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.

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 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

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.

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.

Give us feedback!