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

Liquidation doesn't have expiration #6

Open
c4-bot-8 opened this issue Apr 28, 2024 · 14 comments
Open

Liquidation doesn't have expiration #6

c4-bot-8 opened this issue Apr 28, 2024 · 14 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_05_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@c4-bot-8
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-04-lavarage/blob/main/libs/smart-contracts/programs/lavarage/src/processor/liquidate.rs#L24

Vulnerability details

Proof of Concept

When liquidation happens, then user should query oracle API and provide address of account that should be liquidated. Then oracle signs transaction and anyone can execute it later.

There is a check inside liquidation function that LTV is more than 90%. This is the condition to execute liquidation. When oracle signs transaction it also provides position_size, which is current amount of sol that position holds.

As you know prices can change quickly, which means that position_size can be not same in 10 minutes. But the problem is in case if transaction was signed when position was unhealthy, but now used and after that position became healthy, then still anyone can execute old signed transaction with not up to date position_size and as result account will be liquidated and borrower will loose funds.

Impact

Position can be liquidated, when it became healthy again.

Tools Used

VsCode

Recommended Mitigation Steps

I believe that oracle signatures should work for some short time period, so users need to query new one to get up to date info about account.

Assessed type

Error

@c4-bot-8 c4-bot-8 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Apr 28, 2024
c4-bot-7 added a commit that referenced this issue Apr 28, 2024
@c4-bot-13 c4-bot-13 added the 🤖_05_group AI based duplicate group recommendation label Apr 29, 2024
@c4-sponsor
Copy link

piske-alex (sponsor) confirmed

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label May 1, 2024
@c4-judge
Copy link
Contributor

c4-judge commented May 1, 2024

alcueca marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label May 1, 2024
@c4-judge
Copy link
Contributor

c4-judge commented May 1, 2024

alcueca marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label May 1, 2024
@koolexcrypto
Copy link

I believe this is invalid due to the fact that, a Solana TX has an expiration time. So, if it is not processed within a certain time, it will never be. So old signed transaction will be ignored by validators. This is different from EVM where the TX can still be valid in future if no time check is done on smart contract level.

During transaction processing, Solana Validators will check if each transaction's recent blockhash is recorded within the most recent 151 stored hashes (aka "max processing age"). If the transaction's recent blockhash is older than this max processing age, the transaction is not processed.

Check this for more info on this:
https://solana.com/docs/advanced/confirmation#how-does-transaction-expiration-work

@rvierdiiev
Copy link

thank you for teaching me Solana internals, i agree with you.

@koolexcrypto
Copy link

I appreciate it @rvierdiiev , Thank you for your patience and understanding.

@piske-alex
Copy link

I agree @koolexcrypto seems this doesn't need a fix

@alcueca
Copy link

alcueca commented May 4, 2024

Even if transactions expire, the can be liquidated on stale data within the expiration time. That would be quite unfair (unless stated by the sponsor).

@koolexcrypto
Copy link

@alcueca

Some points to consider here:

  • We're talking here about 60 to 90 seconds, the TX will expire after that.
  • The position_size is not a user input, it's fetched by Oracle (kinda of a bot as I understood from @piske-alex, please correct me if I'm wrong). And as you know, Oracle is a trusted role.
  • In general, not a good idea to rely on time in Solana. Timestamps are estimates and they don't reflect real time.
    Furthermore, when there is a restart (i.e. validators vote to restart the chain and start from older slot), the liquidation will be done on stale data (time check will pass). A necessary check should be for Last Restart Slot. I didn't submit that due to the fact that, it is in Oracle scope which is unknown and a trusted role anyway.

cc: @piske-alex

@alcueca
Copy link

alcueca commented May 5, 2024

@koolexcrypto, it is a very strange design where the health of the position is not checked during liquidation execution, but at a previous time (however short). Even if we are talking about 60 to 90 seconds, it is possible that positions will be liquidated when they were unhealthy, but not anymore.

However, I do realize that that could happen only in very rare cases, so I'll downgrade this to QA, hoping that a more robust design can be found.

@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 May 5, 2024
@c4-judge
Copy link
Contributor

c4-judge commented May 5, 2024

alcueca changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

c4-judge commented May 5, 2024

alcueca marked the issue as grade-a

@koolexcrypto
Copy link

@alcueca

Totally agree with what you have stated. Thank you for allowing this conversation to happen.

@c4-judge
Copy link
Contributor

c4-judge commented May 6, 2024

alcueca marked the issue as not selected for report

@c4-judge c4-judge removed the selected for report This submission will be included/highlighted in the audit report label May 6, 2024
@C4-Staff C4-Staff added the Q-04 label May 6, 2024
@C4-Staff C4-Staff removed the Q-04 label May 20, 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-a QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_05_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

9 participants