Closed sherlock-admin2 closed 4 months ago
1 comment(s) were left on this issue during the judging contest.
santipu_ commented:
High
While it's true that not all LPs can exit or redeem their UNIT due to leverage losses being unrealized, I don't believe the stableCollateralTotal
can become -ve or capped at 0 due to LP redemptions. This is because the checkSkewMax
function will fail in such cases. I believe the protocol can handle redemptions which make the skew become around 15-20% max.
Unfortunately, I don't see any easy solution to this. The market incentives should come into effect in this case since the majority of leverage traders are negative (in PnL terms) then it doesn't make sense for them to keep their positions sooner or later they will be liquidated if the price further goes down. CC @itsermin
Yes step 3 is not possible because of the checkSkewMax
. Ie. It sets a cap on available liquidity. Withdrawing a smaller amount is possible.
If withdrawing a smaller amount, the market is then skewed towards leverage traders. The funding would start to pay LPs, incentivising leverage traders to close their positions, or until liquidation.
This mechanism is similar to available liquidity for lenders in a lending market.
Escalate
I think this issue is valid.
Even if step 3 described in my report fails because of MaxSkewReached
instead of PriceImpactDuringWithdraw
, the LPs still cannot withdraw their funds due to the unrealized losses of the traders.
Here is a brief POC to show an example:
Paste this test at StableCollateralTotal.t.sol
and run it with forge test --match-test test_unrealized_losses -vvv
.
function test_unrealized_losses() public {
vm.startPrank(alice);
// LP deposits 100 ETH
announceAndExecuteDeposit({
traderAccount: alice,
keeperAccount: keeper,
depositAmount: 100e18,
oraclePrice: 1_000e8,
keeperFeeAmount: 0
});
// Trader opens position with 100 ETH additional size (market is delta neutral)
announceAndExecuteLeverageOpen({
traderAccount: alice,
keeperAccount: keeper,
margin: 100e18,
additionalSize: 100e18,
oraclePrice: 1_000e8,
keeperFeeAmount: 0
});
// The price of ETH decreases in half.
skip(2 days);
uint256 newCollateralPrice = 500e8;
setWethPrice(newCollateralPrice);
// Assets owned by the LPs --> 200 ETH
assertEq(stableModProxy.stableCollateralTotalAfterSettlement(), 200e18);
// LP tries to withdraw 10 shares --> 20 ETH
// Transaction fails with error `MaxSkewReached`
announceAndExecuteWithdraw({
traderAccount: alice,
keeperAccount: keeper,
withdrawAmount: 10e18,
oraclePrice: newCollateralPrice,
keeperFeeAmount: 0
});
}
In the final stage of the test, the total collateral owned by the LPs is 200 ETH and the total size of the market is 100 ETH. Despite these numbers, when the LP tries to withdraw 10 shares (20 ETH), the transaction will fail because of the unrealized loss of the trader.
The root cause of this issue is that the checkSkewMax
function is not taking into account the unrealized PnL of the traders, that is what I've described in another issue (#12). However, even if we fix the checkSkewMax
function, we could run into the error I described first in the report (PriceImpactDuringWithdraw
).
As the sponsor remarked, there may not be an easy fix for this issue, but in my opinion, that shouldn't make the issue invalid.
Escalate
I think this issue is valid.
Even if step 3 described in my report fails because of
MaxSkewReached
instead ofPriceImpactDuringWithdraw
, the LPs still cannot withdraw their funds due to the unrealized losses of the traders.Here is a brief POC to show an example: Paste this test at
StableCollateralTotal.t.sol
and run it withforge test --match-test test_unrealized_losses -vvv
.function test_unrealized_losses() public { vm.startPrank(alice); // LP deposits 100 ETH announceAndExecuteDeposit({ traderAccount: alice, keeperAccount: keeper, depositAmount: 100e18, oraclePrice: 1_000e8, keeperFeeAmount: 0 }); // Trader opens position with 100 ETH additional size (market is delta neutral) announceAndExecuteLeverageOpen({ traderAccount: alice, keeperAccount: keeper, margin: 100e18, additionalSize: 100e18, oraclePrice: 1_000e8, keeperFeeAmount: 0 }); // The price of ETH decreases in half. skip(2 days); uint256 newCollateralPrice = 500e8; setWethPrice(newCollateralPrice); // Assets owned by the LPs --> 200 ETH assertEq(stableModProxy.stableCollateralTotalAfterSettlement(), 200e18); // LP tries to withdraw 10 shares --> 20 ETH // Transaction fails with error `MaxSkewReached` announceAndExecuteWithdraw({ traderAccount: alice, keeperAccount: keeper, withdrawAmount: 10e18, oraclePrice: newCollateralPrice, keeperFeeAmount: 0 }); }
In the final stage of the test, the total collateral owned by the LPs is 200 ETH and the total size of the market is 100 ETH. Despite these numbers, when the LP tries to withdraw 10 shares (20 ETH), the transaction will fail because of the unrealized loss of the trader.
The root cause of this issue is that the
checkSkewMax
function is not taking into account the unrealized PnL of the traders, that is what I've described in another issue (#12). However, even if we fix thecheckSkewMax
function, we could run into the error I described first in the report (PriceImpactDuringWithdraw
).As the sponsor remarked, there may not be an easy fix for this issue, but in my opinion, that shouldn't make the issue invalid.
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.
The checkSkewMax function is only supposed to take into account the total size of the leverage positions and the stableCollateralTotal and this is by design to keep the market delta neutral and within safe leverage parameters. If we incorporated PnL into the calculation, the amount that can be used by leverage traders would increase as the price of the collateral falls which means the overall protocol can be leveraged long to a very high factor (more than the configured 120%).
The solution to the "problem" you are describing is to settle the PnLs periodically. The protocol was doing so when we gave the contracts for the first audit. However, that created some other issues. See this and this. Fundamentally, we cannot settle the PnLs in the collateral terms.
The other way to look at this is how it's similar to a lending protocol when all the supplied assets have been borrowed (pool utilisation is close to 100%), in such cases the depositors will not be able to withdraw their assets without the borrowers having returned their borrowed shares.
For these reasons, we considered this issue as invalid. It's the way the protocol is designed to work.
As the sponsor has written this issue is invalid and thus the protocol works.
I originally submitted this issue because its impact wasn't in the previous version of the code and its implications weren't described anywhere so I had no way of knowing that this new behavior was the latest "intended design".
In the previous protocol version, the PnL of all positions was settled globally regularly, updating the stableCollateralTotal
and the marginDepositedTotal
with the new price changes. Because of this, when the ETH price fell, even if the positions weren't closed, the LPs could withdraw their owned funds, including the unrealized losses from the open positions on the market.
With these new changes, when the ETH price falls and some positions on the market are not closed, the LPs cannot withdraw their owned funds (stableCollateralTotalAfterSettlement
) until the positions with unrealized losses are closed.
The problem is that this new weird behavior was introduced with the new code updates and its implications and impact weren't described anywhere, making it impossible for me to know that this was the new "intended design". Because of this, I still think this issue falls into the MEDIUM severity, even if the protocol team won't fix it.
Although LPs cannot withdraw their funds if its used by the traders in leveraged longs, the funds are not lost or locked forever and the LPs can withdraw them when positions are closed/liquidated. LPs continue to accumulate fees and the total stable collateral continues to increase with longs being closed/liquidated. Moreover, the only loss here is that the LPs continue to hold ETH when its price goes down. Hence, I believe this report is a design recommendation.
Planning to reject the escalation and leave the issue as it is.
I don't agree that this issue won't cause a loss of funds for LPs.
When the market is perfectly skewed (leverage = collateral
), the price movements don't matter because the LPs' assets are delta-neutral. However, in a long-skewed market (leverage > collateral
), when the price goes up, the LPs are going to experience losses on their assets.
In the scenario where the LPs cannot withdraw their funds due to the unrealized losses of the traders, the ETH price may start going up again. When this happens in a long-skewed market, the LPs will experience a loss of funds on their collateral because they haven't been able to withdraw their liquidity on time due to this issue.
@santipu03 can you please elaborate on why would this happen? For example, ETH price is X, the market is long-skewed. ETH price drops to Y (which is less than X), but then goes back up to X, the LPs will suffer a loss from it?
If we have the following scenario:
The LPs that stay for steps 1-3 will not experience losses. However, if an LP wants to withdraw liquidity at step 2 to realize the profits, it won't be possible due to this issue. Then, at step 3 those profits will be gone, that's the losses I'm referring to. If you have some profits and you lose them because of a bug, isn't that a loss of funds?
And if the price goes up but stays between Y and X, the trader closes their position, the LP will receive less profits then if they withdrew at price Y, correct?
Another question, if we allow the LP to withdraw at price Y, wouldn't that lead to the trader becoming liquidatable? If both are correct, then I would ask you to look at your PoC again and see if you want to change/improve something in it. I would like to try different scenarios to understand your report and comments on a much deeper level and from other angles.
And if the price goes up but stays between Y and X, the trader closes their position, the LP will receive less profits then if they withdrew at price Y, correct?
That's correct
Another question, if we allow the LP to withdraw at price Y, wouldn't that lead to the trader becoming liquidatable?
No, the trader can only become liquidatable if his margin is less than the required margin. The required margin is calculated as follows (here):
$$ liquidationMargin := positionSize \times liquidationBufferRatio + liquidationFee $$
As you can see, the liquidation doesn't depend in any way on the total stable collateral. Letting the LPs withdraw or not isn't related in any way to the liquidations.
@santipu03 thanks for your response, is there anything you'd like to change to your PoC, so I can test it in different scenarios? If not, just put a thumbs up reaction to this comment. If yes, please provide an updated PoC.
What kind of changes do you want to check?
In your escalation comment, you've said that it's a "brief" PoC, therefore, wanted to clarify there is nothing you'd like to add. Want to check how the profits by LPs change with price going higher/lower, traders closing their positions at these prices or being liquidated.
Here is an improved version of the POC. This new PoC shows how the assets owned by the LPs evolve when the price changes in a long-skewed market. And it also shows how the LPs cannot realize their profits until the traders realize their losses. Feel free to ask if you have any doubts.
You'll have to paste this line on the file to assert the error later:
error MaxSkewReached(uint256 skewFraction);
function test_unrealized_losses() public {
vm.startPrank(alice);
// LP deposits 100 ETH
announceAndExecuteDeposit({
traderAccount: alice,
keeperAccount: keeper,
depositAmount: 100e18,
oraclePrice: 1_000e8,
keeperFeeAmount: 0
});
// Trader opens position with 120 ETH additional size (market is long skewed)
uint256 tokenId = announceAndExecuteLeverageOpen({
traderAccount: alice,
keeperAccount: keeper,
margin: 150e18,
additionalSize: 120e18,
oraclePrice: 1_000e8,
keeperFeeAmount: 0
});
// The price of ETH decreases to 750
skip(2 days);
setWethPrice(750e8);
// Assets owned by the LPs --> 140 ETH
assertEq(stableModProxy.stableCollateralTotalAfterSettlement(), 140e18);
// Each share is worth 1.4 ETH
assertEq(stableModProxy.stableCollateralPerShare(), 1.4e18);
// LP tries to withdraw 10 shares --> 14 ETH
uint256 keeperFee = mockKeeperFee.getKeeperFee();
vm.startPrank(alice);
// It's not possible due to the unrealized losses
vm.expectRevert(abi.encodeWithSelector(MaxSkewReached.selector, 1395348837209302325));
delayedOrderProxy.announceStableWithdraw({withdrawAmount: 10e18, minAmountOut: 0, keeperFee: keeperFee});
// Price almost goes back to 900
skip(2 days);
setWethPrice(900e8);
// Assets owned by the LPs --> 113.3 ETH
assertEq(stableModProxy.stableCollateralTotalAfterSettlement(), 113333333333333333333);
// Each share is worth 1.33 ETH
assertEq(stableModProxy.stableCollateralPerShare(), 1133333333333333333);
// LP tries to withdraw 1 share --> 1.33 ETH
// It's not possible due to the unrealized losses
vm.startPrank(alice);
vm.expectRevert(abi.encodeWithSelector(MaxSkewReached.selector, 1213755900202292650));
delayedOrderProxy.announceStableWithdraw({withdrawAmount: 1e18, minAmountOut: 0, keeperFee: keeperFee});
// Finally the trader's position is closed
announceAndExecuteLeverageClose({
tokenId: tokenId,
traderAccount: alice,
keeperAccount: keeper,
oraclePrice: 900e8,
keeperFeeAmount: 0
});
// Now, the LPs can withdraw their funds but will have less profits due to the bug
assertEq(stableModProxy.stableCollateralTotalAfterSettlement(), 113333333333333333334); // Assets owned by the LPs are still 113.3 ETH
assertEq(stableModProxy.stableCollateralPerShare(), 1133333333333333333); // Each share is still worth 1.33 ETH
announceAndExecuteWithdraw({
traderAccount: alice,
keeperAccount: keeper,
withdrawAmount: 10e18,
oraclePrice: 900e8,
keeperFeeAmount: 0
});
}
@santipu03, firstly, thank you for such PoC with all the explanations. Another question that came to my mind. If the market is long-skewed (i.e. leverage > collateral), then the long traders have to borrow from LP. Then the LPs cannot withwdraw only borrowed part or all the liquidity.
Giving a small numerical example so it's easier to understand:
LPs have provided 100 ETH. Traders have borrowed 50 ETH of it to open levereged longs. Traders canot withdraw the 50 ETH (that are borrowed) or cannot withdraw at all?
The variable skewFractionMax
measures exactly that, the maximum limit of leverage vs collateral the market can have:
/// @notice The maximum limit of total leverage long size vs stable LP.
/// @dev This prevents excessive short skew of stable LPs by capping long trader total open interest.
/// Care needs to be taken when increasing this value as it can lead to the stable LPs being excessively short.
uint256 public skewFractionMax;
In all the protocol tests this variable is set to 1.2e18
, meaning for each unit of collateral in the market, there can only be a maximum of 1.2 units of leverage. In your example, the LPs could withdraw a maximum of 58 ETH, leaving 42 ETH so that the market skew would be 1.19 (50 leverage / 42 collateral
). If the LPs were to withdraw 1 more ETH of collateral, the transaction would revert because the skew would be 1.22 (50 / 41
).
Thank you for these responses, but in that case, I believe it's in fact an intended design to not allow LPs to withdraw funds if it's borrowed by legeraged longs, and not a bug. The protocol is working as expected. As you show, the design is suboptimal, but still I see this report as a design improvement, rather than a security issue. Hence, planning to reject the escalation and leave the issue as it is.
It's expected to not allow LPs to withdraw funds that are borrowed by traders up to the maximum skew allowed. If we assume that the maximum skew is 1.2, then LPs shouldn't be able to withdraw when the skew reaches that value. However, my issue is pointing out that, because of the unrealized losses of the traders, the LPs won't be able to withdraw EVEN if the skew is not reaching the maximum.
I will clarify the scenario from before:
120 / 140
). Because the skew is lower than the max skew (0.86 < 1.2
), the LPs should be able to withdraw some assets, realizing their profits. Because of this bug, the LPs cannot withdraw even 1 wei of assets.The point of this report is that when the skew has not reached its maximum value, the LPs should be able to withdraw some assets. However, because of this bug, they won't be able to, losing some funds as unrealized profits.
For clarification, at step 2, the LPs are unable to withdraw any funds even though it's below skewFrartionMax, because initially they borrowed all the funds from LPs and unrealized losses are not taken into account, correct?
That's the thing, the unrealized losses of the traders will prevent LPs from withdrawing funds and realizing their profits even though the skew is below skewFractionMax
.
That wasn't the intended design as per the protocol docs and code comments at the start of the audit. Also, it will cause a loss fo funds for LPs so that's why I think it should be a medium.
@santipu03 Can you refer me to where in docs/code comments it is not an intended design? Or you mean it wasn't not mentioned in the docs/code comments that it is intended design?
The intended design is inferred from the definition of skew
and skewFractionMax
:
/// @notice The maximum limit of total leverage long size vs stable LP.
/// @dev This prevents excessive short skew of stable LPs by capping long trader total open interest.
/// Care needs to be taken when increasing this value as it can lead to the stable LPs being excessively short.
uint256 public skewFractionMax;
If the current market skew is 0.86 and the maximum skew allowed is 1.2, then by definition the LPs should be able to withdraw funds. When the skew is lower than the maximum skew and the LPs cannot withdraw liquidity, isn't that a bug?
I believe it's not a bug but a design decision. As said before, I believe this behaviour is intended to not allow withdrawal of liquidity if it's used in leveraged longs, even though they're in a loss, but not closed. In the explanation it is said that long leveraged trades cannot go over skewFractionMax
but it's not related to the borrowed liquidity or ability to withdraw it.
If the current market skew is 0.86 and the maximum skew allowed is 1.2, then by definition the LPs should be able to withdraw funds
It's correct, but in this example, all the liquidity from LPs was used by traders and since the losses are not realised, LPs cannot withdraw their liquidity, even though the skew
would allow it if we take unrealized losses into account.
I agree that this design can be suboptimal, but still see it as a design decision after this discussion.
Decision remains the same, reject the escalation and leave the issue as it.
I believe it's not a bug but a design decision. As said before, I believe this behaviour is intended to not allow withdrawal of liquidity if it's used in leveraged longs, even though they're in a loss, but not closed.
In the previous protocol audit, LPs could withdraw liquidity even with unrealized losses from the traders, that's always been the intended design. However, the team implemented a fix for a PnL issue in the previous contest and as a result, this bug was created. No documentation or code comments have been updated to declare that this is the new intended design, that's why I believe it's unexpected and a bug.
In the explanation it is said that long leveraged trades cannot go over skewFractionMax but it's not related to the borrowed liquidity or ability to withdraw it.
Yes, it's absolutely related sir. The skew is, by definition, the difference between the total long leverage (borrowed liquidity) and the stable collateral owned by the LPs. When an LP tries to withdraw liquidity and the error message "MaxSkewReached" appears, it should be because the max skew has been reached right? Then, why is this error getting triggered if the max skew has not been reached? And what is your base for thinking that this is the intended design?
I get that the protocol team acknowledged the issue and treated it as the new "intended design". However, this counterintuitive behavior cannot be inferred by reading the available sources at the time of the audit. I think it's unfair to base a judgment on the protocol's comment AFTER the audit is finished and try to support it with just opinions. All my arguments in this discussion have been based on the source of truth when it comes to judging (protocol docs and code comments).
No documentation or code comments have been updated to declare that this is the new intended design, that's why I believe it's unexpected and a bug.
That's why I asked what is the proof or evidence that this new design is intended or another design is claimed to be intended. I understand that it was different in the previous version of the audit, but it doesn't neccessarily means it should be the same in this audit just with fixed issues.
Regarding the code comments, if this is the only evidence for your words, then I'm afraid it's not sufficient. Sorry for poor phrasing in the previous comment, I meant that it's not said the LPs should be able to withdraw at any time regardless of traders' losses/profits. And in that case, "MaxSkewReached" error is correct, cause without taking unrealized losses of traders and profits of LPs in account is correct. Not taking these two factors into consideration for that case is not a bug but a design choice, and Flat Money team decided to make this choice.
Hence, my decision remains the same, it's an intended design and the protocol works as expected. It's based not on the fact that the sponsor said it, but on your description of the bug and the code, the two points above and our discussion.
Sir, I respect your judgment but still think you're looking at it from the wrong perspective.
Given that the code comments define the skew as the difference between the total long leverage and the stable collateral, it's expected that this definition isn't violated when checking for the max skew. That's why I sent this issue in the first place because there was a contradiction between the specification (code comments) and the implementation (code behavior) that caused a loss of funds to LPs.
Your argument that the skew shouldn't consider the unrealized losses from the traders directly contradicts the definition of skew. You're claiming that the behavior implicit from the docs should have been explicitly stated in order to make this issue valid when in reality it should be the other way around. The behavior that contradicts the specification should have been explicitly stated if intended.
Result: Invalid Unique
santipu_
high
When the ETH price goes down, LPs won't be able to withdraw liquidity
Summary
When the ETH price goes down and there are still some leverage positions open that have not realized their losses, LPs won't be able to withdraw liquidity.
Vulnerability Detail
Consider the following scenario, we're going to ignore the funding fee for the sake of simplicity:
stableCollateralPerShare
is1e18
(total shares is10e18
).stableCollateralPerShare
increases to2e18
5.5e18
shares) --> transaction will fail with an errorPriceImpactDuringWithdraw
.This has happened because no leverage positions have been closed, so their loss hasn't been realized yet. This will cause the variable
stableCollateralTotal
to be10e18
(for the initial liquidity of 10 rETH) instead of20e18
. So, when the LP tries to withdraw,stableCollateralTotal
will be capped at zero, and that will increase the value ofstableCollateralPerShare
too much, reverting the whole transaction.Here is the function to withdraw liquidity:
https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/StableModule.sol#L93-L138
When the function
updateStableCollateralTotal
is called,stableCollateralTotal
will be capped at zero:https://github.com/sherlock-audit/2024-03-flat-money-fix-review-contest/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L442-L448
The value of
stableCollateralTotal
will be capped at zero because_stableCollateralAdjustment
is-11e18
andstableCollateralTotal
is10e18
. After this, whenstableCollateralPerShareAfter
is calculated, the result will be significantly higher thanstableCollateralPerShareBefore
because of this behavior.Because the variables
stableCollateralPerShareBefore
andstableCollateralPerShareAfter
have such a difference, the whole transaction will revert with the errorPriceImpactDuringWithdraw
.Impact
When the price of ETH goes down, LPs won't be able to withdraw liquidity as they should
Code Snippet
See above
Tool used
Manual Review
Recommendation
Allow the value of
stableCollateralTotal
to go below zero to deal with these edge cases.