Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No grace time for traders when L2 sequencers come online after being down, making them prone to liquidations #56

Closed
howlbot-integration bot opened this issue Jun 17, 2024 · 7 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-c primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_152_group AI based duplicate group recommendation sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/PriceFeed.sol#L45-L58
https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/logic/LiquidationLogic.sol#L39

Vulnerability details

Impact

Traders are prone to liquidation when an L2 sequencer becomes online as there is no grace period for traders to adjust their positions.

Proof of Concept

Please read this section of the blog -

However one more issue remains; if repayments are paused, during that pause market fluctuations can cause a Borrower to become subject to liquidation as the Borrower is unable to repay(). As soon as repayments are resumed, such a Borrower will be immediately liquidated by liquidation bots, with the only possibility to save their position being if the Borrower themselves runs a repayment bot & can successfully front-run the liquidation bots.

This situation unfairly disadvantages Borrowers as such Borrowers became subject to liquidation through no fault of their own. Upon repayments resuming a Borrower will be immediately liquidated, unfairly disadvantaging the Borrower and giving a huge advantage to the Liquidator.

To fix the game theory such that neither Borrowers nor Liquidators are unfairly favored, after repayments are resumed there should be a grace period during which Borrowers can't be liquidated.

Although the protocol has not directly implemented a pausing functionality. Think of L2 sequencer downtime scenarios.

When an L2 sequencer comes online again after it went offline, and if the market conditions continue to remain unfavourable for traders, they can get immediately liquidated by bots, as they don't have enough time to adjust their positions.

The protocol should give grace time to traders to avoid liquidations. Put another way, liquidations should only be executed after the grace time has passed.

AAVE has implemented a grace time for its users to avoid liquidations -
https://docs.aave.com/developers#price-oracle-sentinel

Since the impact of the issue is high and the likelihood can be low, this issue is of medium severity.

Tools Used

Manual review

Recommended Mitigation Steps

Add a grace time after the L2 sequencer comes back online, such that liquidations can only be executed after the grace time has passed, giving enough time for traders to adjust their margins.

Assessed type

Other

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_152_group AI based duplicate group recommendation bug Something isn't working primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Jun 17, 2024
howlbot-integration bot added a commit that referenced this issue Jun 17, 2024
@syuhei176 syuhei176 added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Jun 19, 2024
@alex-ppg
Copy link

The submission and its duplicates describe how the oracles in the Predy system do not validate that the sequencer is live via an on-chain call.

This is generally considered a best practice recommendation and has historically been graded as a QA-level flaw which I would consider as a fair evaluation (specifically, QA (L)). As such, I do not believe this submission and its duplicates are eligible for an HM reward.

@c4-judge
Copy link
Contributor

alex-ppg changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jun 28, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked the issue as grade-c

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Jun 28, 2024
@NishithPat
Copy link

@alex-ppg The issue is not just about checking whether the sequencer is online. It is not like the Chainlink issue about stale prices due to sequencer downtime (which has been historically judged as QA).

The issue is about providing grace time for users to adjust their positions to avoid liquidation after sequencer downtime. The impact is quite high because users are prone to liquidations in this case. As the likelihood is low, the issue is of medium severity. Also, note that the sponsor has acknowledged this issue. They recognise the seriousness of the issue.

@c4-judge c4-judge reopened this Jul 4, 2024
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jul 4, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Jul 4, 2024

This previously downgraded issue has been upgraded by alex-ppg

@c4-judge
Copy link
Contributor

c4-judge commented Jul 4, 2024

alex-ppg changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jul 4, 2024
@alex-ppg
Copy link

alex-ppg commented Jul 4, 2024

Hey @NishithPat, thank you for the additional feedback! I understand what the submission entails, and did not mention Chainlink in my response.

It is impossible to reliably detect if there was any sequencer downtime recently, meaning the issue could still manifest if the sequencer went down at timestamp A, liquidations became eligible at timestamp B, and the sequencer came up at timestamp C. There is no way to reliably check if a loan became liquidatable in between sequencer downtimes rendering the recommendation to remain a best practice and thus QA.

@C4-Staff C4-Staff closed this as completed Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-c primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_152_group AI based duplicate group recommendation sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

5 participants