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

Flow cannot be cancelled for any ongoing flow that involves GMX (1x long leverage or not) unless owner intervenes

0xjarix

Summary

The use of the gmxLock modifier instead of checking isLongOneLeverage(beenLong) inside the cancelFlow() function could have prevented the keeper from cancelling a flow that is not a 1x long leverage. Luckily, there is an onlyOwner setVaultState(...) function that could reset temporarily the _gmxLock parameter to bypass the modifier. Nonetheless, the cancelFlow() is badly implemented, it is not behaving the way it should.

Vulnerability Details

Let's consider the scenario where run(true, false,...) is called by the keeper when there are no current positions that are opened (positionIsClosed is true) and there is a sufficiant amount of collateral tokens in the contract (_isFundIdle() is true).



These lines of code will be executed:

(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createIncreasePosition(isLong, acceptablePrice, prices);

The subcall to createIncreasePosition(false,...)__ will set _gmxLock to true to lock the contract from the moment a position is opened to the moment of the callback.

_gmxLock = true;

Now the gmxLock modifier will revert untill _gmxLock is set again to false (during the callback).

// 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();
}
_;
}

Now the keeper tries to cancel the flow but will not be able because the call will revert because of the gmxLock modifier:

* In the case of 1x long leverage, we never cancel the ongoing flow.
* In the case of gmx position, we could cancel current ongoing
* flow due to some accidents from our side or gmx side.
*/
function cancelFlow() external nonReentrant gmxLock {
_onlyKeeper();
_cancelFlow();
}

Luckily, there's this onlyOwner function that saves the day for the keeper, owner will call it to change _gmxLock to false temporarily, just for the keeper to be able to call cancelFlow() successfully.

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

It could have prevented the keeper from cancelling a flow that is not a 1x long leverage if there wasn't the onlyOwner setVaultState(...) function to set _gmxLock to false. Regardless, even though no funds are at risk, the function is incorrect, the state is not be handled appropriately.

Tools Used

Manual review

Recommendations

-function cancelFlow() external nonReentrant gmxLock {
+function cancelFlow() external nonReentrant {
+ require(!_isLongOneLeverage(beenLong));
_onlyKeeper();
_cancelFlow();
}
Updates

Lead Judging Commences

n0kto Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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