xiaoming90 - Revert when adjusting the position

[sherlock-admin2]

xiaoming90

high

Revert when adjusting the position

Summary

The margin adjustment will revert unexpectedly when executing. Margin adjustment is time-sensitive as long traders often rely on it to increase their position's margin when their positions are on the verge of being liquidated to avoid liquidation.

If the margin adjustment fails to execute due to an inherent bug within its implementation, the user's position might be liquidated as the transaction to increase its margin fails to execute, leading to a loss of assets for the traders.

Vulnerability Detail

When announcing an adjustment order, if the marginAdjustment is larger than 0, marginAdjustment + totalFee number of collateral will be transferred from the user account to the DelayedOrder contract. The totalFee comprises the keeper fee + trade fee.

Let's denote marginAdjustment as $M$, keeper fee as $K$, and trade fee as $T$. Thus, the balance of DelayedOrder is increased by $(M + K + T)$ rETH.

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L303

File: DelayedOrder.sol
217:     function announceLeverageAdjust(
..SNIP..
300:         // If user increases margin, fees are charged from their account.
301:         if (marginAdjustment > 0) {
302:             // Sending positive margin adjustment and both fees from the user to the delayed order contract.
303:             vault.collateral().safeTransferFrom(msg.sender, address(this), uint256(marginAdjustment) + totalFee);
304:         }

When the keeper executes the adjustment order, the following function calls will happen:

DelayedOrder.executeOrder() => DelayedOrder._executeLeverageAdjust() => LeverageModule.executeAdjust()

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L137

File: LeverageModule.sol
147:     function executeAdjust(
..SNIP..
234:         // Sending keeper fee from order contract to the executor.
235:         vault.sendCollateral({to: _keeper, amount: _order.keeperFee});

When the LeverageModule.executeAdjust function is executed, it will instruct the FlatCoin vault to send the $K$ rETH (keeper fee) to the keeper address. Thus, the amount of rETH held by the vault is reduced by $K$. However, in an edge case where the amount of rETH held by the vault is $V$ and $V < K$, the transaction will revert.

This is because, at this point, the DelayedOrder contract has not forwarded the keeper fee it collected from the users to the FlatCoin vault yet. The keeper fee is only forwarded to the vault after the executeAdjust function is executed at Line 608 below, which is too late in the above-described edge case.

This edge case might occur if there is low liquidity in the vault, a high keeper fee in the market, or a combination of both. Thus, the implementation should not assume that there is always sufficient liquidity in the vault to pay the keeper in advance and collect the keeper fee from the DelayedOrder contract later.

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L303

File: DelayedOrder.sol
586:     /// @notice Execution of user delayed leverage adjust order.
587:     /// @dev Uses the Pyth network price to execute.
588:     /// @param account The user account which has a pending order.
589:     function _executeLeverageAdjust(address account) internal {
590:         FlatcoinStructs.Order memory order = _announcedOrder[account];
591:         FlatcoinStructs.AnnouncedLeverageAdjust memory leverageAdjust = abi.decode(
592:             order.orderData,
593:             (FlatcoinStructs.AnnouncedLeverageAdjust)
594:         );
595: 
596:         _prepareExecutionOrder(account, order.executableAtTime);
597: 
598:         ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).executeAdjust({
599:             account: account,
600:             keeper: msg.sender,
601:             order: order
602:         });
603: 
604:         if (leverageAdjust.marginAdjustment > 0) {
605:             // Sending positive margin adjustment and fees from delayed order contract to the vault
606:             vault.collateral().safeTransfer({
607:                 to: address(vault),
608:                 value: uint256(leverageAdjust.marginAdjustment) + leverageAdjust.tradeFee + order.keeperFee
609:             });
610:         }

Impact

The margin adjustment will revert unexpectedly when executing, as shown in the scenario above. Margin adjustment is time-sensitive as long traders often rely on it to increase their position's margin when their positions are on the verge of being liquidated to avoid liquidation.

If the margin adjustment fails to execute due to an inherent bug within its implementation, the user's position might be liquidated as the transaction to increase its margin fails to execute, leading to a loss of assets for the traders.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L303

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L137

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L303

Tool used

Manual Review

Recommendation

Consider transferring the keeper fee to the vault first before sending it to the keeper's address to ensure that the transfer of the keeper fee will work under all circumstances.

function _executeLeverageAdjust(address account) internal {
    FlatcoinStructs.Order memory order = _announcedOrder[account];
    FlatcoinStructs.AnnouncedLeverageAdjust memory leverageAdjust = abi.decode(
        order.orderData,
        (FlatcoinStructs.AnnouncedLeverageAdjust)
    );

    _prepareExecutionOrder(account, order.executableAtTime);

+    if (leverageAdjust.marginAdjustment > 0) {
+        // Sending positive margin adjustment and fees from delayed order contract to the vault
+        vault.collateral().safeTransfer({
+            to: address(vault),
+            value: uint256(leverageAdjust.marginAdjustment) + leverageAdjust.tradeFee + order.keeperFee
+        });
+    }

    ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).executeAdjust({
        account: account,
        keeper: msg.sender,
        order: order
    });

-    if (leverageAdjust.marginAdjustment > 0) {
-        // Sending positive margin adjustment and fees from delayed order contract to the vault
-        vault.collateral().safeTransfer({
-            to: address(vault),
-            value: uint256(leverageAdjust.marginAdjustment) + leverageAdjust.tradeFee + order.keeperFee
-        });
-    }

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: medium(9)

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/dhedge/flatcoin-v1/pull/272.

[santipu03]

Escalate

This issue should be invalid.

In what possible scenario the vault won't have enough funds to cover for the keeper fee while adjusting a leveraged position?

The vault will always have the funds from the LPs and the margins of the traders. Even in the extreme case that the vault only has one position open, the funds from LPs and the margin deposited will be enough to cover for the keeper fee while adjusting the position.

The scenario where the vault doesn't have enough funds to cover the keeper fee is unreal, therefore, the issue should be invalidated.

[sherlock-admin2]

Escalate

This issue should be invalid.

In what possible scenario the vault won't have enough funds to cover for the keeper fee while adjusting a leveraged position?

The vault will always have the funds from the LPs and the margins of the traders. Even in the extreme case that the vault only has one position open, the funds from LPs and the margin deposited will be enough to cover for the keeper fee while adjusting the position.

The scenario where the vault doesn't have enough funds to cover the keeper fee is unreal, therefore, the issue should be invalidated.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[securitygrid]

I totally agree with santipu03's POV, which is why I didn't submit this issue.

[xiaoming9090]

Disagree with the above escalations. This is a valid issue as it highlights genuine accounting and logic flaws within the system. As mentioned in the report, this is an edge case, and this scenario might occur if there is low liquidity in the vault, a high keeper fee in the market, or a combination of both. One should not assume that there is always sufficient liquidity in the vault to pay the keeper in advance and collect the keeper fee back later at all times.

[0xjuaan]

The protocol will be running their own keepers. Thus, there won't be a shortage of keeper activity which is the only thing that can inflate keeperFee other than gas price. So keeperFee will not exceed gas fee estimates by much. Since this will be on the base L2, low gas fees -> low keeperFee.

Combine that with the fact that for this bug to occur, the user needs to try to executeAdjust. This means that the vault already holds margin + tradeFee + keeperFee from the leverage position creation. This means that the vault holds plenty of rETH which it can send.

Likelihood of this issue occuring is extremely low. Impact of this issue is medium/low. Hence, the severity is low at best.

[Evert0x]

Can someone provide more context on the exact scenario where the liquidity is insufficient to pay for the fee?

Combine that with the fact that for this bug to occur, the user needs to try to executeAdjust. This means that the vault already holds margin + tradeFee + keeperFee from the leverage position creation. This means that the vault holds plenty of rETH which it can send.

It seems like this logic will prevent this bug from being triggered.

[xiaoming9090]

The keeper fee $K$ is computed based on a percentage of the adjustment made to the position's margin (marginAdjustment). If a user intends to make a huge adjustment (= huge marginAdjustment), the $K$ will be high. The size of $K$ also depends on the keeper fee being configured at any point. Higher fee would lead to higher $K$.

The amount of rETH held by the vault ($V$) might be low in liquidity during the following scenario:

• When the vault is still in its early stage where there are few users
• A large number of users have withdrawn from the vault during a black swan event leaving little to no liquidity left on the vault

Thus, it is technically possible a state where $K > V$ would occur.

[0xjuaan]

In the absolute worst case, K would have to exceed marginMin + 1.5 * marginMin + tradeFee + keeperFee for the revert to occur.

keeperFee and tradeFee are the fees paid to the vault when this user initially deposited marginMin to create the leverage position.

In order to deposit marginMin in the first place, there must have been 1.5*marginMin deposited from LPs (this is the minimum additional size) since the minimum leverage is 1.5x.

If my math is wrong, please correct me but due to the sheer impossibility of this scenario (above scenario is most optimistic for this issue), the issue should be low/informational.

[Czar102]

It also seems that this transaction failure could be easily mitigated, apart from the off-chance of the off-chance of this issue happening. Would you agree that this is a low severity issue @xiaoming9090?

[xiaoming9090]

It also seems that this transaction failure could be easily mitigated, apart from the off-chance of the off-chance of this issue happening. Would you agree that this is a low severity issue @xiaoming9090?

If a user wants to adjust their position by a specific amount and this issue occurs, the user could:

• Reduce the adjustment size so that the fee will be reduced accordingly to ensure the fee is smaller than the available liquidity to avoid the revert. However, the user might not achieve its intended goal (e.g., increase their margin to avoid being liquidated - in this case, the insufficient margin being top-up)
• Wait for other users to deposit into the protocol to replenish the available liquidity, which might or might not happen (e.g., due to a lack of users or an event where everyone is exiting). Once there is sufficient liquidity, execute the adjustment order, but it might be too late.

It does not seem that the above approach is "easy". I would consider it easy if one could resubmit their TX immediately without adjusting the intended adjustment size after the first failed execution, and the TX goes through the second time.

I do not think that a trading system, which is time-sensitive, should suggest its users reduce their adjustment size when they intend to increase their margin under any circumstance or have the users wait for the right condition before they can adjust their position size without valid reasons (lack of liquidity is not valid). In the first place, the accounting within the system should be robust to handle such scenario.

[Czar102]

@xiaoming9090 Couldn't the user deposit themselves? In the end, a only small fraction of the position needs to be deposited, so they should be able to do that easily.

[xiaoming9090]

@xiaoming9090 Couldn't the user deposit themselves? In the end, a only small fraction of the position needs to be deposited, so they should be able to do that easily.

Yes, users could deposit themselves to bump up the available liquidity to work around the issue.

[Czar102]

Planning to consider this issue a Low severity one.

[Czar102]

Result: Low Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• santipu03: accepted