Liquidating positions with bounded Kerosen could be unprofitable for liquidators

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L205-L228

Vulnerability details

Description

Bounded Kerosene is not attractive for liquidation. First of all bounded kerosene has twice the value of unbounded kerosene. E.g if 1 Kerosene is worth 10 usd in the unbounded vault then 0.5 Kerosen will be worth the same amount in a bounded vault. Twice the valuation is justified for users and Liquidity Providers. The problem however is if we take the prespective of a liquidator. If he liquidates a position that has a significant percentage of the position value in bounded Kerosene Vault. The liquidator will recieve the amount locked in the bounded kerosene vault, which he can't withdraw so the liquidator actually wouldn't be necessary be able to swap the kerosene against stable coins to make a profit for example. He will also recieve half the amount of kerosene if this was an unbounded kerosene Vault. This might not be clear, so we encourage the reader to see the following example to understand the need to rework liquidations for bounded kerosene vaults.

N.B please note that there is a bug in the liquidate() function and Kerosene Collateral are not sent to the liquidator, but as I have confirmed with the sponsor, The protocol intended to send the Kerosene Tokens to the liquidators along side the seized Eth

Proof of Concept

As an example to show the huge disadvantage a liquidator will face when liquidating bounded kerosene vs unbounded kerosen vs exo collateral (like eth)

let's take similar context but with different composition of collateral

Collateral worth 1.4k and minted dyad is worth 1k usd: -> collRatio =140 % which is elligible for liquidation. percentage of coll to seize is (100%+ 40%*20%)/140% => 77.14% of collateral to seize

For simplicity let's say 1 unbounded kerosene Token is worth 100 usd and 1 bounded kerosene Token is worth 200 usd (twice the asset price) in the vaults as collateral ( however the value of Kerosene outside the vaults in exchanges is gonna be near the 100 usd valuation)

The liquidator needs to pay down 1k dyads (1k usd)		Bounded Kerosene	Unbounded Kerosene	Eth (no Kerosen)
Liquidation cost for liquidators	1k DYAD	1k DYAD	1k DYAD
collateral composition	1k worth of eth + 2 kerosene Tokens	1k worth of eth + 4 kerosene Tokens	1.4k usd worth of eth
seized coll	771 usd worth of eth + 1.54 Kerosene Token (not withdrawable)	771 usd worth of eth + 3.08 Kerosene Tokens(withdrawable)	1.08k usd worth of eth
usd worth of seized collateral	925 usd (154 usd value is locked, not tradable)	1.08k usd	1.08 k usd

Recomendation

Having twice the value for kerosene if locked, might make sense for Protocol user, this however will result in problems for liquidators. We recommend the protocol to implement a solution that addresses the following issues:

• Liquidators of kerosene type collateral, should always be able to withdraw the token (whetether seized from bounded or unbounded Vaults)
• When calculating the amount of tokens to seize (incentive), Kerosene tokens should be valued the same, for unbounded and bounded kerosene Vaults, so it would make economic sense for the liquidator

Assessed type

Context

[c4-pre-sort]

JustDravee marked the issue as duplicate of #128

[c4-pre-sort]

JustDravee marked the issue as sufficient quality report

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto marked the issue as duplicate of #128

[c4-judge]

koolexcrypto marked the issue as satisfactory

[aliX40]

Dear judge @koolexcrypto , Thanks for judging the contest, I appreciate the hard work.

I want to point out that this issue is wrongly duped to the primary #128 (Kerosene collateral is not being moved on liquidation, exposing liquidators to loss), to this issue i have already submitted an issue that is already correctly dupplicated to the primary: liquidate() only seizes non-Kerosene Collateral leading to unprofitable liquidations #342

This issue #343 i submitted, showcases that even if Kerosene collateral is correctly moved (if the implementation was actually correct) Liquidators who will liquidate positions with a significant bounded Kerosene in it, will have unprofitable liquidations, due to the following reasons (please also see issue for full details):

• liquidated bounded kerosene Tokens will be locked and the liquidator wouldn't be able to withdraw them (in order to make a profit)
• Liquidator will pay double the market price (price in exchanges) for kerosene in bounded vaults, which will make the liquidation unprofitable.

Fixing the primary (moving kerosene tokens to liquidators) wouldn't fix this issue and according to C4 documentation this makes the issue I described not a dupplicate but a seperate issue

According to C4 juding criteria described in the official docs (link)[https://docs.code4rena.com/awarding/judging-criteria]:

Findings are duplicates if they share the same root cause.

More specifically, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates.

As i already described the bug is not a dupplicate because it doesn't have the same root cause and if the root cause of the primary is fixed the bug will actually be exploitable and not fixed

Furthermore to explain why the issue is of high severity:

• likeability of this scenario is high (as normal users will also normally use bounded kerosene to mint dyad) and positions with high percentage in bounded kerosen will naturally arise.
• impact is also high as unprofitable liquidations could result in the insolvancy of the protocol. The treasury could force liquidate the positions at a loss, but long term this is not a sustainable strategy which could lead to insolvancy

T.B.D: To summarize:

• primary issue describes the issue of the missing implementation to move kerosene tokens on liquidation in VaultManagerV2.liquidate() from Kerosene Valuts (Bounded and Unbounded Vaults)
• my issue describes that the implementation of Vault.kerosine.bounded.move() is faulty along with the concept of liquidating from Bounded Kerosen Vaults, which will lead to unprofitable liquidations ( Vault.kerosine.unbounded.move() is correctly implemented, this issue only affects Bounded Vaults)

[aliX40]

Please also note That this issue doesn't only target the implementation of VaultManagerV2.liquidate() which lacks the move Kerosene Functionality. But it also targets the move() implementation in /src/core/Vault.kerosine.bounded.sol, which would be called by VaultManagerV2.liquidate() if the implementation was correct. Also please note that to fix the issue #343 the devs need to override the method move() inherited in Vault.kerosine.bounded.sol and add the fixes, in contrast to the primary issue where they need to only update VaultManagerV2.liquidate() implementation.

[shafu0x]

great find and description. making liquidated kerosene unbounded is a good idea.

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto marked the issue as primary issue

[c4-judge]

koolexcrypto marked the issue as selected for report

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)

[aliX40]

Dear judge @koolexcrypto, Thanks for reevaluating this issue, I still think this is a high severity bug and for the following reasons:

• impact: High -> Unprofitable liquidations always have a high impact
• likeability: High -> Bounded Kerosene is a huge part of the new protocol update, and users are incentivized (2x Vaulation) to lock their Kerosene Tokens In bounded Vaults which would lead to a high likeability of positions with bounded Kerosene Vaults in them becoming elligible for liquidation.

Dear judge, i urge u to reconsider the severity of this bug, as in my opinion this is of high severity due to the high likeability and the high impact.

[McCoady]

Hi, sorry for the comment so far outside of PJQA but hopefully due to the status change it's understandable.

I'm unsure how this issue is in scope given it first requires speculating on how the sponsor will mitigate the underlying issue (that neither Kerosene type is handled during liquidations).

In fact KeroseneVault (inherited by bounded & unbounded vaults) has a move function which assumedly was supposed to be used to move Kerosene on liquidation meaning the liquidators would still receive bounded Kerosene.

  function move(
    uint from,
    uint to,
    uint amount
  )
    external
      onlyVaultManager
  {
    id2asset[from] -= amount;
    id2asset[to]   += amount;
    emit Move(from, to, amount);
  }

The profitability of taking on BoundedKerosene is a decision to be made by the liquidator, and given that the liquidator is expected to also own a Note nft rather than a traditional liquidation bot, it's likely they would also value owning bounded Kerosene.

The issue basically boils down to the opinion that "liquidators don't want bounded Kerosene" and I don't think is our job to decide whether that's the case or not.

Additionally the recommended mitigation of unlocking bounded Kerosene completely defeats the purpose of having bounded/unbounded kerosene and users would be able to enjoy the benefits of 2x valued Kerosene as collateral and then liquidate themselves when they wished to withdraw their "bounded" Kerosene.

[aliX40]

Hi @McCoady, thanks for commenting on this issue. I assume u are commenting to assure the fairness of the competition, which i respect. But to keep the conversation short and to respect the judge time, i wish that u would also mention valid points in short form (in bullet proof format) Concerning the points u mentioned:

Point1: Out of scope/ speculations:

• As mentioned in the issue: I have already confirmed beforehand with the sponsor that it is the intended implementation making this not a speculation
• As u have probably encountered in c4 in this contest and other contest, the design decisions and problems with the system design in itself are always a valid concern and in scope.
• As u have mentioned also part of the issue that i mentioned, that bounded kerosen also is locked for liquidators, is already the case and it is part of the codebase, making the implementation faulty and this submission in scope

Point2: It is not a problem if we leave liquidations unprofitable

• Having 30%, 40% (due to the incentive of locking kerosene) of liquidations in the system being unprofitable is a very huge risk. I didn't quite follow ur logic. But as u can see from the issue (please also confirm the math urself) liquidating positions with a significant chunk of Bounded Kerosene in them is inflated and liquidators will end up with a huge loss, exposing the protocol to risk of insolvancy as unhealthy positions would accumulate and no liquidator willing to liquidate them

Point 3: it defeats the purpose of having bounded Kerosene

• The purpose of the bounded kerosene Vaults is to encourage Users to lock their Tokens, and not for liquidators not being profitable
• Concerning mitigation it is up to the sponsor to decide, and he can put in protection mechanisms to try to prevent such scenarios (e.g prevent self liquidations, to make such attack more costly as they would need another dnft at least). U also not putting in mind the competitive market of liquidations bots and assuming that every user is able to liquidate themselves. Putting those two things together u would see that the risk of such thing happening is really low and compared to the risk of having unprofitable liquidations most of the times is not comparable. Also this part of ur example only targets making bounded kerosene withdrawable and doesn't address the inflated price.

[McCoady]

• Further information given by the sponsor in private is not a valid proof as per C4 rules.

• Liquidations are only "unprofitable" under the assumption that the liquidators don't want the locked Kerosene. Locked Kerosene that is moved will still be locked and therefore still be worth 2x value towards the liquidators own collateral (given they must also be a Notes owner to liquidate)

• Liquidations may be competitive but smart users can bundle their transactions (either as a contract or via flashbots)

Will leave it up to the judge from here, just wanted to offer the counter arguments as this went astray otherwise.

[aliX40]

• Firstly, there is nothing of sort in the official doc, secondly the issue of not moving Kerosene has been identified by a huge number of wardens. Assuming that it is not clear that it is the intended behaviour is also questionable
• Liquidations are mainly built on liquidation bots, making flashloan, trading seized collateral, repaying their flash loan and making a profit which is not possible at all with bounded kerosene.
• U are assuming that there would be enough LIquidators, who would agree to take a loss in liquidations, which is not a reliable way to protect the protocol against market risks
• Even if a liquidator will want to have bounded kerosene (which has a low likeablility, as it is never going to be the case for most positions and liquidators (as mentioned liquidation are mostly automated)). It will be more financially viable for the liquidator to use the dyad he is going to use in liquidations to buy kerosine from exchanges, as he will pay half the price for it.
• U are still focusing on part of the recomended mitigation and not addressing the main issue which is unprofitable liquidations and insolvancy risk for the protocol

[koolexcrypto]

"Medium" since it is explicitly known (by docs, code) that bounded can't be withdrawn. One could argue that, liquidators could be aware of this and choose not to liquidate. Furthermore, liquidators could liquidate to accumulate bounded kerosene to utilize in the protocol for enhancing the CR. So, it is not completely unprofitable.

[thebrittfactor]

For transparency, the DYAD team (shafu) acknowledged this finding outside of github. The appropriate sponsor labeling has been added on their behalf.