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

Some variables are deleted before used this bring problems for the execution of some parts of the code necesary for the keepers.

mikebello

Summary

the flow variable and other variables are deleted before they are used to emit events that are important for off-chain scripts. this can cause different problems for the keepers and the off-chain scrips.

Vulnerability Details

Asking the sponsors about events in the contracts, they confirmed that it is important that these events are emitted correctly every time because the keepers in the off-chain scrips use these events to make decisions about how to handle the user's positions.

In the perpetual vault contract, there is an edge case where the flow variable is deleted before it is used to emit the GmxPositionUpdated event in the swap case causing this event is not emitted, this can bring problems for the keeper and off-chain scripts, and depending on the way these events are used in the off-chain scripts, this can cause losses for the protocol or the users of the vault.

For example, when a GMX 1x long leverage position is being opened, closed, or changed from long to short or vice versa the keeper executes the run function, and the flow variable is set to SIGNAL_CHANGE to indicate this.

function run(/* code omitted */) external nonReentrant {
// code omitted
flow = FLOW.SIGNAL_CHANGE;
if (isOpen) {
if (positionIsClosed) {
if (_isLongOneLeverage(isLong)) {
_runSwap(metadata, true, prices);
}
// code omitted
}

Then the _runSwap function is called to swap the tokens, in the case the swap is done using Paraswap and Gmx the orderType is set OrderType.MarketSwap in the _doGmxSwap function and an order is created in the GMX contracts; at this point, the variables flow and nextAction are set to these values: flow = SIGNAL_CHANGE and nextAction = NO_ACTION .

function _runSwap(/* Code Omitted */) internal returns (bool completed)
{
/* Code Omitted */
if (metadata.length == 2) {
(PROTOCOL _protocol, bytes memory data) = abi.decode(metadata[0], (PROTOCOL, bytes));
if (_protocol != PROTOCOL.DEX) {
revert Error.InvalidData();
}
swapProgressData.swapped = swapProgressData.swapped + _doDexSwap(data, isCollateralToIndex);
​
(_protocol, data) = abi.decode(metadata[1], (PROTOCOL, bytes));
if (_protocol != PROTOCOL.GMX) {
revert Error.InvalidData();
}
​
_doGmxSwap(data, isCollateralToIndex);
return false;
}
/* Code Omitted */
}

After this, the GMX keeper will execute the executeOrder function and return a callback to the GMXProxy, the proxy will do some validations and enter the last else case for the swap scenario which calls the PerpetualVault.afterOrderExecution.

function afterOrderExecution(/* Code Omitted */) external override validCallback(requestKey, order)
{
/* Code Omitted */
else {
// Determine output token and amount for swap or decrease orders
/* Code Omitted */
IPerpetualVault(perpVault).afterOrderExecution(requestKey, positionKey, orderResultData, prices);
delete queue;
}
}

The PerpetualVault.afterOrderExecution will enter the else case where the orderType is MarketSwap and as the flow variable is set to SIGNAL_CHANGE the last else case will be executed, this case call the _updateState function which will delete the flow variable.

function afterOrderExecution(/* Code Omitted */ ) external nonReentrant
{
/* Code Omitted */
} else if (orderResultData.orderType == Order.OrderType.MarketSwap) {
/* Code Omitted */
else {
// Same as if (flow == FLOW.SIGNAL_CHANGE || FLOW.COMPOUND)
if (orderResultData.outputToken == indexToken) {
//@note delete swapProgressData y si selector es NO_Action delete flow y flowData.
_updateState(false, true);
} else {
_updateState(true, false);
}
}
}
}
​

The _updateState function deletes the flow variable and some other variables, which set these variables to their default values.

function _updateState(bool _positionIsClosed, bool _isLong) internal {
if (flow == FLOW.SIGNAL_CHANGE) {
positionIsClosed = _positionIsClosed;
if (_positionIsClosed == false) {
beenLong = _isLong;
}
}
//@follow-up cuando se llama desde runNextAction, se elimina el valor de nextAction, por lo que entra a este if ya que el default es "NO_ACTION"
if (nextAction.selector == NextActionSelector.NO_ACTION) {
if (_isLongOneLeverage(_isLong)) {
delete flowData;
delete flow;
} else {
nextAction.selector = NextActionSelector.FINALIZE;
}
}
delete swapProgressData;
}

After these variables are deleted the afterOrderExecution function will try to emit the GmxPositionUpdated if the flow variable is set to SIGNAL_CHANGE, but the flow variable was reset to its default value in the _updateState function, so this event will not be emitted, this can cause different problems in the off-chain scripts depending how they use this event.

if (flow == FLOW.SIGNAL_CHANGE) {
emit GmxPositionUpdated(
positionKey,
market,
orderResultData.orderType == Order.OrderType.MarketIncrease,
orderResultData.isLong,
orderResultData.sizeDeltaUsd,
prices.indexTokenPrice.min
);
}

Impact

some events will not be emitted correctly, which can bring problems for off-chain scripts.

Tools Used

Manual review.

Recommendations

make sure you delete/reset the variables that are used for other processes like emitting events are deleted at the end of the functions that use these variables to complete some process.

Updates

Lead Judging Commences

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

Appeal created

mikebello Submitter
9 months ago
0xl33 Auditor
9 months ago
mikebello Submitter
9 months ago
n0kto Lead Judge
8 months ago
n0kto Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!