0x37 - Healthy positions may be liquidated because of inappropriate judgment

[sherlock-admin3]

0x37

Medium

Healthy positions may be liquidated because of inappropriate judgment

Summary

Healthy positions may be liquidated in one special case

Vulnerability Detail

In is_liquidatable(), there are some comments about the position's health. A position becomes liquidatable when its current value is less than a configurable fraction of the initial collateral. The problem is that when pnl.remaining equals required, this position should be healthy and cannot be liquidated. But this edge case will be though as unhealthy and can be liquidated.

@external
@view
def is_liquidatable(position: PositionState, pnl: PnL) -> bool:
    """
    A position becomes liquidatable when its current value is less than
    a configurable fraction of the initial collateral, scaled by
    leverage.
    """
    percent : uint256 = self.PARAMS.LIQUIDATION_THRESHOLD * position.leverage
    required: uint256 = (position.collateral * percent) / 100
    return not (pnl.remaining > required)

Impact

Healthy positions may be liquidated and take some loss.

Code Snippet

https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/params.vy#L117-L138

Tool used

Manual Review

Recommendation

-    return not (pnl.remaining > required)
+    return not (pnl.remaining >= required)

[spacegliderrrr]

Escalate

Issue should be low/info. It only covers an extreme edge case where pnl.remaining == required. It is extremely unlikely for the two values to match exactly and it wouldn't make a difference as the position would be liquidateable in the next block anyway.

[sherlock-admin3]

Escalate

Issue should be low/info. It only covers an extreme edge case where pnl.remaining == required. It is extremely unlikely for the two values to match exactly and it wouldn't make a difference as the position would be liquidateable in the next block anyway.

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.

[oxwhite]

The impact is high even if the likelihood is low(Not extremely low). Therefore it is submitted as medium. You can't say it is going to be liquidated in the next block anyway by simply ignoring other possibilities. How do you certain of that? How about if the user wants to add liquidity and protect his position when it is close to threshold?or if the price goes towards the direction of user's position when exact time equal to pnl.remaining? If the point being questioned here is the low probability, There are accepted reports with different roots which include similar edge case in previous contests. Here is an example:https://github.com/code-423n4/2023-10-nextgen-findings/issues/1323 . Here is another one in recent contest accepted by the judge:https://codehawks.cyfrin.io/c/2024-08-fjord/results?lt=contest&page=1&sc=xp&sj=score&t=report.

[Waydou21]

I agree, the issue is a low at best, even the impact is not a high as when being on such fine edge is synonym for liquidatable, I agree with spacegliderr comment as this means it'll be liquidated in the next block due to fees

[oxwhite]

The code allows to this edge case happen. Even if it happens only once, there is loss for users, hence making the impact high. Beside the tx can be observed by the malicious actors and manipulated based on this case. Therefore it deserves M and a fix

[sabatha7]

Escalate

Issue should be low/info. It only covers an extreme edge case where pnl.remaining == required. It is extremely unlikely for the two values to match exactly and it wouldn't make a difference as the position would be liquidateable in the next block anyway.

Extremely unlikely edge case on the P&L? that isnt true.

I agree, the issue is a low at best, even the impact is not a high as when being on such fine edge is synonym for liquidatable, I agree with spacegliderr comment as this means it'll be liquidated in the next block due to fees

Due to fees? That isnt the same scenario the round down of the healthy position here will be considerable plus the total liquidated amount.

edit: i suspect you have misunderstood the reason being if the position is liquidated wrong they cannot re-liquidate it again or not during the next block even if the position theoretically is still healthy for how long after false liquidation then any given position then if they do liquidated some position 'by fees' during the next block I doubt it will still be the same position. The auditors here have clearly specified pnl.remaining == required there will be wrong liquidation.

https://github.com/code-423n4/2023-07-tapioca-findings/issues/1164 https://github.com/sherlock-audit/2023-03-notional-judging/issues/203

[WangSecurity]

After confirming, this comment is outdated:

A position becomes liquidatable when its current value is less than a configurable fraction of the initial collateral, scaled by leverage.

The code works as intended and the position should be liquidated when PnL equals required and the comment has to be changed. Hence, this issue is invalid as the code works as expected. Planning to accept the escalation and invalidate the issue.

[sabatha7]

@WangSecurity what of issue validity rules; "If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules. In case of contradictions between the README and CODE COMMENTS, the README is the chosen source of truth." Hence this is valid. For example iamnmt

@WangSecurity

Why is the rule "List of Issue categories that are not considered valid" applied but not this rule?

finding was awarded for 2024-06-magicsea-judging and was rewarded and many additional findings only just recently for 2024 findings. The comments that are provided during the contest code will prioritised according to the rules. Thus we will validate this finding regardless.

[oxwhite]

After confirming, this comment is outdated:

A position becomes liquidatable when its current value is less than a configurable fraction of the initial collateral, scaled by leverage.

The code works as intended and the position should be liquidated when PnL equals required and the comment has to be changed. Hence, this issue is invalid as the code works as expected. Planning to accept the escalation and invalidate the issue.

@WangSecurity logically if the remaining value is equal to the required value, the position should not be liquidated. I do not think it is reasonable simply invalidate that a submission based on the comment. The comment, the function names and docs can lie, what is important is that what the code allows. In this case the code allows the user to be liquidated while it shouldn't , because the user has enough collateral. Besides the user may protect their position towards the threshold, but current implementation takes this chance from user. On the other hand, as ı said above it is not logical to invalidate/validate based on the comments provided, however for a second let's focus on "outdated" concept and look at a more critical example. If an outdate concept is enough to invalidate a bug, how about if an outdated function left in the code that leads to a critical exploit? Is it reasonable to invalidate the all reports that are submitted in this case? Based on your reasoning, we should be able to invalidate this case as well. But It is protocol's responsibility to remove the outdated function before the contest in this case. What auditor or judges should pay attention is that does the provided code allows an exploit or not? if yes, then it should be valid.
See the exploit happened due to a deprecated function. The reason of escalation is the edge case here. However that does not decrease the severity. Because it leads to loss of funds. Since there is an edge case it is medium. Otherwise that would be an H. Also there are previous reports with similar edge case accepted by Sherlock. Here is a recent one:https://github.com/sherlock-audit/2024-08-winnables-raffles-judging/issues/26 Now what makes this one different in terms of severity? This and other accepted similar reports also disagree with the reason of escalation.

[WangSecurity]

@sabatha7 The code comments indeed stand above the rules, but don't forget about this rule:

The judge can decide that CODE COMMENTS are outdated based on contextual evidence. In that case, the judging guidelines are the chosen source of truth.

Here, the comment is outdated. Hence, the rules apply. About other findings, firstly, historical decisions are not sources of truth. Secondly, it's hard to understand exactly what you mean by that since there's no reference (i.e. link).

@oxwhite

logically if the remaining value is equal to the required value, the position should not be liquidated. I do not think it is reasonable simply invalidate that a submission based on the comment. The comment, the function names and docs can lie, what is important is that what the code allows. In this case the code allows the user to be liquidated while it shouldn't , because the user has enough collateral. Besides the user may protect their position towards the threshold, but current implementation takes this chance from user.

The key part is that the current behaviour is intended and the user should be liquidatable when remaining == required. Hence, here the protocol works as expected.

On the other hand, as ı said above it is not logical to invalidate/validate based on the comments provided, however for a second let's focus on "outdated" concept and look at a more critical example. If an outdate concept is enough to invalidate a bug, how about if an outdated function left in the code that leads to a critical exploit? Is it reasonable to invalidate the all reports that are submitted in this case? Based on your reasoning, we should be able to invalidate this case as well. But It is protocol's responsibility to remove the outdated function before the contest in this case. What auditor or judges should pay attention is that does the provided code allows an exploit or not? if yes, then it should be valid.

I don't see how this part provides any value to the discussion and it's not relevant here.

See the exploit happened due to a deprecated function

This is a completely different protocol, completely different issue and here we have nothing to do with "deprecated function".

The reason of escalation is the edge case here. However that does not decrease the severity. Because it leads to loss of funds. Since there is an edge case it is medium. Otherwise that would be an H

This is not a loss of funds because the user has to be liquidatable at this point. It's not the mistake in the code and it works as intended here.

Also there are previous reports with similar edge case accepted by Sherlock. Here is a recent one:https://github.com/sherlock-audit/2024-08-winnables-raffles-judging/issues/26 Now what makes this one different in terms of severity? This and other accepted similar reports also disagree with the reason of escalation

The issue you referenced is completely different, it was validate on a completely different rule and is irrelevant here. Also, don't forget that historical decisions are not sources of truth.

Planning to accept the escalation and invalidate the issue.

[oxwhite]

@WangSecurity

"The key part is that the current behaviour is intended and the user should be liquidatable when remaining == required. Hence, here the protocol works as expected."

That can be said if you accept that the comment is outdated and current implementation is intended, which was not known to auditors during the contest. Also, I would appreciate it if you could provide any information on how you determined the comment was outdated. Was this simply communicated by the protocol after submissions?

"I don't see how this part provides any value to the discussion and it's not relevant here."

This is pointing out that it is unreasonable to validate or invalidate an issue while ignoring the code and focusing on the outdated comment. It aligns with what is stated here:

"The judge can decide that CODE COMMENTS are outdated based on contextual evidence. In that case, the judging guidelines are the chosen source of truth. Example: The code comments state that 'a swap can never fail' even though the code is built to support failing swaps."

The example shows that the code is more important than the comments.

"This is not a loss of funds because the user has to be liquidatable at this point. It's not the mistake in the code and it works as intended here."

Again, it is not considered a loss of funds because it is regarded as intended behavior, based on the claim of an "outdated comment," which was likely made by the protocol after the contest. This was neither mentioned in the README nor publicly discussed in the Discord during the contest. Please correct me if I am wrong on this.

In conclusion, if your reasoning for invalidation is still simply based on the 'outdated comment,' I have nothing further to discuss.

[sabatha7]

@WangSecurity Thank you for announcing these valid suggestions.

1. Indeed the computation for Required, and for Pnl are calculated based on the honest participants' actual positions' pot.
2. There is no taggable contextual evidence that suggests that Percent, or Required variable are subtracted a healthy amount, (or Pnl is added a healthy amount) to the extend where Pnl == Required isn't healthy based on point nr. 1 (Indeed the computation for Required, and for Pnl are calculated based on the honest participants' actual positions' pot.)
3. We have to visit the actual contextual evidence.
4. We will assume that it will be the "fees" discussion acting as the contextual evidence being shown?
5. Losses are already factored into the pnl.remaining before invoking is_liquidatable
6. Base fees, and Quote fees are visited after POSITIONS.close has already been visited. This shows that amt_final.fee remaining are manipulatable by, or applied by the liquidator, and apart from that the context of the code suggest that zero fees will be ignored. Thus on the escalation; the position will be liquidated in any case on the next block according to fees, this is not asserted, or constrained to be a constant true (in contextual evidence according to how the code was developed). The kind gentleman will be asked to realise a valid escalation scope.

[WangSecurity]

That can be said if you accept that the comment is outdated and current implementation is intended, which was not known to auditors during the contest. Also, I would appreciate it if you could provide any information on how you determined the comment was outdated. Was this simply communicated by the protocol after submissions?

Yes, I've clarified with the sponsor if the code comment was correct and the code worked incorrectly. It ended up the opposite and the code works correctly. But that's sufficient contextual evidence to apply the rule about outdated code comments that I've shared in the previous comment. Any Watson could do the same during the contest.

The example shows that the code is more important than the comments

It doesn't necessarily mean that the code is more important than code comments. And I don't validate/invalidate the finding based on the code comment being outdated or not. I invalidate this finding based on the fact that the code works as intended and it's only a recommendation to consider changing from pnl.remaining > required to pnl.remaining >= required.

Again, it is not considered a loss of funds because it is regarded as intended behavior, based on the claim of an "outdated comment," which was likely made by the protocol after the contest. This was neither mentioned in the README nor publicly discussed in the Discord during the contest. Please correct me if I am wrong on this.

As said above, it's the contextual evidence that makes the comment outdated.

@sabatha7 Thank you for elaborating on the finding here, but, unfortunately, the code works correctly here and it's only a recommendation, as said above. Planning to accept the escalation and invalidate the issue.

[sabatha7]

@WangSecurity

Thank you.

How could one have audited a Black Box context.

Please clarify on the context.

Clearly an additional amount is being subtracted from the remaining pnl prematurely. To say rounding down a healthy position is the intended behavior context even if that rounding down breaches 1% lost is that what we are justifying i.e are we suggesting that a jump from remain equals requaired to remain < required can not greater than 1% movement? Please observe your response since a movement < 1% can generate a loss of 100% of the remaining position also pnl is allowed to float and tokens are allowed to dump or hike greatly there is absolutely no reason to liquidated the position here regardless of the final bias of the judgement .

[sabatha7]

@WangSecurity

Again kindly show how it is; "working correctly" . The formula is hardcoded to show that the position is still healthy when remaining is equal to required kindly show how this isn't true.

12 was given valid high. The poc refers to negative p&l please accurately define negative p&l. also #12 is invalid and highly unlikely and works as intended there is no guarantee and incentive for anyone to do that . This #17 is the 0x37 finding that is valid tbh.

[sabatha7]

pleading for fair treatment and neutral biases.