xiaoming90 - Incorrect skew check formula used during withdrawal

[sherlock-admin]

xiaoming90

high

Incorrect skew check formula used during withdrawal

Summary

The purpose of the long max skew (skewFractionMax) is to prevent the FlatCoin holders from being increasingly short. However, the existing controls are not adequate, resulting in the long skew exceeding the long max skew deemed acceptable by the protocol, as shown in the example in this report.

When the FlatCoin holders are overly net short, an increase in the collateral price (rETH) leads to a more pronounced decrease in the price of UNIT, amplifying the risk and loss of the FlatCoin holders and increasing the risk of UNIT's price going to 0.

Vulnerability Detail

When the users withdraw their collateral (rETH) from the system, the skew check (checkSkewMax()) at Line 132 will be executed to ensure that the withdrawal does not cause the system to be too skewed towards longs and the skew is still within the skewFractionMax.

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

File: DelayedOrder.sol
109:     function announceStableWithdraw(
110:         uint256 withdrawAmount,
111:         uint256 minAmountOut,
112:         uint256 keeperFee
113:     ) external whenNotPaused {
114:         uint64 executableAtTime = _prepareAnnouncementOrder(keeperFee);
115: 
116:         IStableModule stableModule = IStableModule(vault.moduleAddress(FlatcoinModuleKeys._STABLE_MODULE_KEY));
117:         uint256 lpBalance = IERC20Upgradeable(stableModule).balanceOf(msg.sender);
118: 
119:         if (lpBalance < withdrawAmount)
120:             revert FlatcoinErrors.NotEnoughBalanceForWithdraw(msg.sender, lpBalance, withdrawAmount);
121: 
122:         // Check that the requested minAmountOut is feasible
123:         {
124:             uint256 expectedAmountOut = stableModule.stableWithdrawQuote(withdrawAmount);
125: 
126:             if (keeperFee > expectedAmountOut) revert FlatcoinErrors.WithdrawalTooSmall(expectedAmountOut, keeperFee);
127: 
128:             expectedAmountOut -= keeperFee;
129: 
130:             if (expectedAmountOut < minAmountOut) revert FlatcoinErrors.HighSlippage(expectedAmountOut, minAmountOut);
131: 
132:             vault.checkSkewMax({additionalSkew: expectedAmountOut});
133:         }

However, using the checkSkewMax function for checking skew when LPs/stakers withdraw collateral from the system is incorrect. The checkSkewMax function is specifically used when there is a change in position size on the long-trader side.

In Line 303 below, the numerator of the formula holds the collateral/margin size of the long traders, while the denominator of the formula holds the collateral size of the LPs. When the LP withdraws collateral from the system, it should be deducted from the denominator. Thus, the formula is incorrect to be used in this scenario.

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

File: FlatcoinVault.sol
294:     /// @notice Asserts that the system will not be too skewed towards longs after additional skew is added (position change).
295:     /// @param _additionalSkew The additional skew added by either opening a long or closing an LP position.
296:     function checkSkewMax(uint256 _additionalSkew) public view {
297:         // check that skew is not essentially disabled
298:         if (skewFractionMax < type(uint256).max) {
299:             uint256 sizeOpenedTotal = _globalPositions.sizeOpenedTotal;
300: 
301:             if (stableCollateralTotal == 0) revert FlatcoinErrors.ZeroValue("stableCollateralTotal");
302: 
303:             uint256 longSkewFraction = ((sizeOpenedTotal + _additionalSkew) * 1e18) / stableCollateralTotal;
304: 
305:             if (longSkewFraction > skewFractionMax) revert FlatcoinErrors.MaxSkewReached(longSkewFraction);
306:         }
307:     }

Let's make a comparison between the current formula and the expected (correct) formula to determine if there is any difference:

Let sizeOpenedTotal be $SO{total}$, stableCollateralTotal be $SC{total}$ and _additionalSkew be $AS$. Assume that the sizeOpenedTotal is 100 ETH and stableCollateralTotal is 100 ETH. Thus, the current longSkewFraction is zero as both the long and short sizes are the same.

Assume that someone intends to withdraw 20 ETH collateral from the system. Thus, the _additionalSkew will be 20 ETH.

Current Formula

$$ \begin{align} skewFrac = \frac{SO{total} + AS}{SC{total}} \ skewFrac = \frac{100 + 20}{100} = 1.2 \end{align} $$

Expected (correct) formula

$$ \begin{align} skewFrac = \frac{SO{total}}{SC{total} - AS} \ skewFrac = \frac{100}{100 - 20} = 1.25 \end{align} $$

Assume the skewFractionMax is 1.20 within the protocol.

The first formula will indicate that the long skew after the withdrawal will not exceed the long max skew, and thus, the withdrawal will proceed to be executed. Immediately after the execution is completed, the system exceeds the skewFractionMax of 1.2 as the current long skew has become 1.25.

Impact

The purpose of the long max skew (skewFractionMax) is to prevent the FlatCoin holders from being increasingly short. However, the existing controls are not adequate, resulting in the long skew exceeding the long max skew deemed acceptable by the protocol, as shown in the example above.

When the FlatCoin holders are overly net short, an increase in the collateral price (rETH) leads to a more pronounced decrease in the price of UNIT, amplifying the risk and loss of the FlatCoin holders and increasing the risk of UNIT's price going to 0.

Code Snippet

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

Tool used

Manual Review

Recommendation

The checkSkewMax function is designed specifically for long trader's operations such as open, adjust, and close positions. They cannot be used interchangeably with the LP's operations, such as depositing and withdrawing stable collateral.

Consider implementing a new function that uses the following for calculating the long skew after withdrawal:

$$ skewFrac = \frac{SO{total}}{SC{total} - AS} $$

Let sizeOpenedTotal be $SO{total}$, stableCollateralTotal be $SC{total}$ and _additionalSkew be $AS$.

[sherlock-admin]

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

takarez commented:

valid: medium(7)

[sherlock-admin]

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

[santipu03]

Escalate

The impact should be QA.

While I agree that when announcing a withdrawal of collateral the skew is not correctly checked, the order won't execute if the skew is too high due to this check here

Therefore, the skew won't be higher than the max allowed skew.

[sherlock-admin2]

Escalate

The impact should be QA.

While I agree that when announcing a withdrawal of collateral the skew is not correctly checked, the order won't execute if the skew is too high due to this check here

Therefore, the skew won't be higher than the max allowed skew.

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.

[NishithPat]

Escalate

Even if the order may not execute due to this check - vault.checkSkewMax({additionalSkew: 0}), the fact that it allows the user to call announceWithdraw in this manner is still an issue.

Consider the same example in the above issue where a user tries to withdraw 20 ETH. Using the current formula announceWithdraw would execute because the longSkewFraction is not greater than 1.2 (skewFractionMax). When it is time for its execution using the executeWithdraw function, it might revert due to this check - vault.checkSkewMax({additionalSkew: 0}).

The user notices that his withdrawal does not get executed. So, he decides to withdraw lower amounts for the execution to take place.

In order to do this he needs to do the additional step of cancelling the previous announceWithdraw first using cancelExistingOrder and then call announceWithdraw with a lower amount. Or, even if he does not call cancelExistingOrder first, and uses announceWithdraw directly, it would call the _prepareAnnouncementOrder function which would anyway call the cancelExistingOrder function. So, this means that the user has to pay additional gas to do this.

function _prepareAnnouncementOrder(uint256 keeperFee) internal returns (uint64 executableAtTime) {
        // Settle funding fees to not encounter the `MaxSkewReached` error.
        // This error could happen if the funding fees are not settled for a long time and the market is skewed long
        // for a long time.
        vault.settleFundingFees();

        if (keeperFee < IKeeperFee(vault.moduleAddress(FlatcoinModuleKeys._KEEPER_FEE_MODULE_KEY)).getKeeperFee())
            revert FlatcoinErrors.InvalidFee(keeperFee);

        // If the user has an existing pending order that expired, then cancel it.
        cancelExistingOrder(msg.sender);

        executableAtTime = uint64(block.timestamp + vault.minExecutabilityAge());
    }

Another important point to note is that a user will be able to call announceWithdraw only after order.executableAtTime + vault.maxExecutabilityAge() amount of time has passed because cancelExistingOrder has this check -

 if (block.timestamp <= order.executableAtTime + vault.maxExecutabilityAge())
            revert FlatcoinErrors.OrderHasNotExpired();

So, this means that not only does a user have to spend additional gas to cancel the original announceWithdraw request, but he also needs to wait until he can send a new withdrawal transaction as he cannot withdraw whenever he wants based on what’s written above. The old order needs to expire first.

This might not seem much for a single user, but many users can face the same issue. They would also have to spend more gas and time to execute withdrawals. This combined effect cannot be ignored. This would make for a terrible UX.

I mean all these extra steps could be easily avoided if the protocol chooses to use the right formula for announceWithdraw as well. The first transaction for announceWithdraw would not have been executed in the first place, had the right formula been used.

checkSkewMax function is a core invariant of the system. Its implementation cannot be inconsistent. It cannot be different in announceWithdraw and executeWithdraw.

The impact may not seem high, but it is not QA.

[sherlock-admin2]

Escalate

Even if the order may not execute due to this check - vault.checkSkewMax({additionalSkew: 0}), the fact that it allows the user to call announceWithdraw in this manner is still an issue.

Consider the same example in the above issue where a user tries to withdraw 20 ETH. Using the current formula announceWithdraw would execute because the longSkewFraction is not greater than 1.2 (skewFractionMax). When it is time for its execution using the executeWithdraw function, it might revert due to this check - vault.checkSkewMax({additionalSkew: 0}).

The user notices that his withdrawal does not get executed. So, he decides to withdraw lower amounts for the execution to take place.

In order to do this he needs to do the additional step of cancelling the previous announceWithdraw first using cancelExistingOrder and then call announceWithdraw with a lower amount. Or, even if he does not call cancelExistingOrder first, and uses announceWithdraw directly, it would call the _prepareAnnouncementOrder function which would anyway call the cancelExistingOrder function. So, this means that the user has to pay additional gas to do this.

function _prepareAnnouncementOrder(uint256 keeperFee) internal returns (uint64 executableAtTime) {
        // Settle funding fees to not encounter the `MaxSkewReached` error.
        // This error could happen if the funding fees are not settled for a long time and the market is skewed long
        // for a long time.
        vault.settleFundingFees();

        if (keeperFee < IKeeperFee(vault.moduleAddress(FlatcoinModuleKeys._KEEPER_FEE_MODULE_KEY)).getKeeperFee())
            revert FlatcoinErrors.InvalidFee(keeperFee);

        // If the user has an existing pending order that expired, then cancel it.
        cancelExistingOrder(msg.sender);

        executableAtTime = uint64(block.timestamp + vault.minExecutabilityAge());
    }

Another important point to note is that a user will be able to call announceWithdraw only after order.executableAtTime + vault.maxExecutabilityAge() amount of time has passed because cancelExistingOrder has this check -

 if (block.timestamp <= order.executableAtTime + vault.maxExecutabilityAge())
            revert FlatcoinErrors.OrderHasNotExpired();

So, this means that not only does a user have to spend additional gas to cancel the original announceWithdraw request, but he also needs to wait until he can send a new withdrawal transaction as he cannot withdraw whenever he wants based on what’s written above. The old order needs to expire first.

This might not seem much for a single user, but many users can face the same issue. They would also have to spend more gas and time to execute withdrawals. This combined effect cannot be ignored. This would make for a terrible UX.

I mean all these extra steps could be easily avoided if the protocol chooses to use the right formula for announceWithdraw as well. The first transaction for announceWithdraw would not have been executed in the first place, had the right formula been used.

checkSkewMax function is a core invariant of the system. Its implementation cannot be inconsistent. It cannot be different in announceWithdraw and executeWithdraw.

The impact may not seem high, but it is not QA.

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.

[ydspa]

Escalate

The impact should be QA.

While I agree that when announcing a withdrawal of collateral the skew is not correctly checked, the order won't execute if the skew is too high due to this check here

Therefore, the skew won't be higher than the max allowed skew.

Good catch, i also find this issue during audit, but i finally decide not to submit it due to this check. As the impact only limits to user's gas waste, and the occurrence possibility is very low. Therefore, it's a valid low issue.

[santipu03]

@NishithPat

The overall probability of this happening could be considered as medium/low. Regarding the impact, in the worst case the user would have to wait one minute more to withdraw the collateral and waste a little bit of gas (cheap on Base chain).

Probability being medium/low and impact being low results on the overall severity being QA.

[NishithPat]

But, you can't deny the fact that a user has to do all these extra steps just to withdraw some amount again. This is a waste of his resources. And as I said, when you look at just 1 user it might not look as much, but multiple users will face the same issue. They will also be wasting their resources. So, when looked at as a whole (keeping multiple users in mind), the resources being used just to execute these transactions again would add up.

All this could have been easily prevented, had the protocol used the right formula in announceWithdraw as well. The initial incorrect transaction for announceWithdraw would have never been executed if the right formula had been used.

The issue's severity is at least a medium. Such a violation of a core invariant of the system cannot be QA.

[0xLogos]

Agree with the first escalation

low, there's correct check in stable withdraw execution. After doing some math, you'll see that the correct check is more strict, so the announce function won't revert falsely.

[NishithPat]

@0xLogos

announceWithdraw function would have rightly reverted if they had used the right formula. LongSkew would have been 1.25, which would be greater than 1.2 (max allowed skew), based on the example in the above issue.

But, since the wrong formula is used, announceWithdraw will not revert as it should have as LongSkew is not greater than 1.2.

[0xLogos]

@NishithPat

I mean there is no situations when correct check in stable withdraw execution allows execution, but announcment is reverting because of this incorrect check.

It's addition to first escalation because to fully invalidate this issue you need to ensure that incorrect check wil not cause dos to legitimate orders.

[NishithPat]

I don't think I understand what you are trying to convey.

All I am trying to say is that announceWithdraw must revert as well and not just executeWithdraw when longSkew becomes greater than max skew. For this, I have provided my reasons above.

Anyway, I have said what I wanted to say. I believe this issue is valid. The sponsors do think the same, as they have fixed the issue.

I am okay with whatever the judges decide.

[xiaoming9090]

Disagree with the escalator that the impact is QA/Low.

NishithPat has made a valid and comprehensive response to the escalations. I would like to add to his response.

The impact of this issue is not just limited to the waste of gas. We must understand that all trading operations (open/adjust/close order, deposit/withdraw) are time-sensitive in the real world.

Let's assume that using the current formula announceWithdraw, the withdrawal trade order will be executed because the longSkewFraction is not greater than 1.2 (skewFractionMax). However, when it is time for its execution using the executeWithdraw function, it reverts due to this check vault.checkSkewMax({additionalSkew: 0}).

This kind of outcome is absolutely unacceptable for a trading system in the real world. The trading system is effectively misleading the traders in the first place, telling them that the system will execute their withdrawal as they have already verified that the withdrawal will not cause the system's skew ratio to exceed the limit.

All orders must wait for a period before they can be executed, and only one order can be queued for each user at any time. This means that while the withdrawal trade order is in the queue, the users cannot perform any other operations, such as open/adjust position, even if they wish.

After the holding period has passed, the keeper takes the withdrawal trade order and executes it. Only then, the system will tell the users that the system has to invalidate the user's withdrawal trade order (because the system has used the wrong checkSkew formula in the first place). The users might keep repeatedly trying to withdraw and only realize that their withdrawal orders get invalidated much later when executed, not knowing what the underlying issue/bug is.

Lastly, I have already shown the math with real numbers where the checkSkew formula does not detect skew while they should be in the report.

[ydspa]

Disagree with the escalator that the impact is QA/Low.

NishithPat has made a valid and comprehensive response to the escalations. I would like to add to his response.

The impact of this issue is not just limited to the waste of gas. We must understand that all trading operations (open/adjust/close order, deposit/withdraw) are time-sensitive in the real world.

Let's assume that using the current formula announceWithdraw, the withdrawal trade order will be executed because the longSkewFraction is not greater than 1.2 (skewFractionMax). However, when it is time for its execution using the executeWithdraw function, it reverts due to this check vault.checkSkewMax({additionalSkew: 0}).

This kind of outcome is absolutely unacceptable for a trading system in the real world. The trading system is effectively misleading the traders in the first place, telling them that the system will execute their withdrawal as they have already verified that the withdrawal will not cause the system's skew ratio to exceed the limit.

All orders must wait for a period before they can be executed, and only one order can be queued for each user at any time. This means that while the withdrawal trade order is in the queue, the users cannot perform any other operations, such as open/adjust position, even if they wish.

After the holding period has passed, the keeper takes the withdrawal trade order and executes it. Only then, the system will tell the users that the system has to invalidate the user's withdrawal trade order (because the system has used the wrong checkSkew formula in the first place). The users might keep repeatedly trying to withdraw and only realize that their withdrawal orders get invalidated much later when executed, not knowing what the underlying issue/bug is.

Lastly, I have already shown the math with real numbers where the checkSkew formula does not detect skew while they should be in the report.

I think the above debate fall into the following sherlock rule:

8. Opportunity Loss is not considered a loss of funds by Sherlock.

And also this issue is not a break of core functionality that make contract useless, therefore LOW is suitable

[NishithPat]

The users might keep repeatedly trying to withdraw and only realize that their withdrawal orders get invalidated much later when executed, not knowing what the underlying issue/bug is.

This comment by the LSW pretty much sums it up. Users will repeatedly try to call the announce withdraw function, not realizing why their withdrawal gets reverted. Then they don't just spend gas for 1 extra transaction, they do it for several transactions. They don't have to wait for 1 minute, but they need to wait for several minutes because of the wrong implementation of checkSkewMax in the announceWithdraw function.

Also, think about the case of emergencies when users want to withdraw the underlying assets from the protocol. Because of this improper implementation in the announceWithdraw function, users who are trying to withdraw and get their orders invalidated will try to withdraw several times, not realizing why their orders get invalidated. And when they do realize after several attempts, it will already be too late by then as several minutes would have passed by. In such cases, there would definitely be a loss of funds for the user.

The issue cannot be QA.

[Czar102]

I am siding with the escalation. https://github.com/sherlock-audit/2023-12-flatmoney-judging/issues/193#issuecomment-1959803953 also explains the reasoning well.

Planning to accept the escalation and invalidate the issue.

[nevillehuang]

@Czar102 As mentioned by LSW, the max skew intended by protocol is 20%, but because of this issue, it will allow potential bypass. The trading opportunity cost impact is definitely not valid, but I believe the impact highlighted warrants medium severity.

The purpose of the long max skew (skewFractionMax) is to prevent the FlatCoin holders from being increasingly short. However, the existing controls are not adequate, resulting in the long skew exceeding the long max skew deemed acceptable by the protocol, as shown in the example above.

When the FlatCoin holders are overly net short, an increase in the collateral price (rETH) leads to a more pronounced decrease in the price of UNIT, amplifying the risk and loss of the FlatCoin holders and increasing the risk of UNIT's price going to 0.

[santipu03]

@nevillehuang The statement that the long skew will exceed the long max skew is wrong.

Refer to my escalation here

[nevillehuang]

@santipu03 Apologies, agree, it will revert here, so this issue can be invalid.

[Czar102]

Result: Low Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• santipu03: accepted
• NishithPat: rejected