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

Oracle is payer of liquidation transaction fee #12

Open
c4-bot-9 opened this issue Apr 29, 2024 · 13 comments
Open

Oracle is payer of liquidation transaction fee #12

c4-bot-9 opened this issue Apr 29, 2024 · 13 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a Q-04 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-9
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#L113

Vulnerability details

Proof of Concept

For Liquidate context oracle address is set as Signer. This means that this account will pay for the transaction. But this should not be like that, as protocol doesn't charge fees(only UI fees that can be omitted and any other UI can be built and used).

Using oracle API users will be able to fetch transactions signed by oracle and then execute them. All of this txs fees should be paid by oracle. After discussing with sponsor i have found out that oracle account should not hold funds, which means that liquidations won't work.

Instead, traders should sign tx as well and pay for tx.

Impact

Oracle pays for tx, but should not and don't have funds.

Tools Used

VsCode

Recommended Mitigation Steps

Make trader pay the fee.

Assessed type

Error

@c4-bot-9 c4-bot-9 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 29, 2024
c4-bot-9 added a commit that referenced this issue Apr 29, 2024
@c4-bot-13 c4-bot-13 added the 🤖_05_group AI based duplicate group recommendation label Apr 29, 2024
@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-sponsor
Copy link

piske-alex (sponsor) confirmed

@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 satisfactory

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

c4-judge commented May 1, 2024

alcueca marked the issue as selected for report

@koolexcrypto
Copy link

Hi @rvierdiiev

It is not possible to let the trader pay the fee unless the transaction is signed by the trader. Why would the trader sign a transaction that will lead to liquidate their position? This will lead simply to DoS liquidation.

I don't see this an issue here. Oracle which is the only signer should hold funds in order to be able to send TXs to the chain. Or additional account (controlled by the protocol only) which holds funds to pay for the fee. Which takes us back to the same point that, a controlled account by the protocol should hold the funds (Oracle or additional signer).

@rvierdiiev
Copy link

When i said trader, i meant owner of trading pool and not position holder.
And Oracle is not going to hold funds, this is stated by protocol team.

@alcueca
Copy link

alcueca commented May 4, 2024

liquidations won't work

Could I get a PoC that if the oracle doesn't hold funds then the liquidations don't work?

Otherwise, I'll mark this as invalid.

@rvierdiiev
Copy link

Sure @alcueca

From solana docs: https://solana.com/docs/core/fees#fee-collection

Transactions are required to have at least one account which has signed the transaction and is writable. Writable signer accounts are serialized first in the list of transaction accounts and the first of these accounts is always used as the "fee payer".

In anchor, transaction signer is fetched by Signer struct. This struct checks, that account has signed transaction(note that many accounts can sign same tx on Solana). In case of liquidate function, which receives Liquidate context as input, we can see that the only Signer is oracle account and no one else. This means that oracle account's balance will be reduced with transaction fees and if oracle doesn't have funds, then tx will fail.

From solana docs: https://solana.com/docs/core/fees#fee-collection

Before any transaction instructions are processed, the fee payer account balance will be deducted to pay for transaction fees. If the fee payer balance is not sufficient to cover transaction fees, the transaction will be dropped by the cluster.

@alcueca
Copy link

alcueca commented May 6, 2024

Gotcha, then I assume that if we want the transaction to still be signed by the oracle, so that the liquidation price can be trusted, then liquidate should receive more than one signer account. The first one should be the liquidator, paying for fees, and the second one the oracle, to ensure trusted liquidation data.

@C4-Staff C4-Staff added M-05 M-04 and removed M-05 labels May 6, 2024
@alcueca
Copy link

alcueca commented May 10, 2024

The net effect of this issue is that Lavarage would need to provide an small amount of funds. Given that the average price per transaction in Solana is way below $1, this is a dust amount and negligible for the protocol. Users are not impacted. The result is no more than a deviation from expected design.

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

alcueca changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

alcueca marked the issue as grade-a

@c4-judge
Copy link
Contributor

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 10, 2024
@rvierdiiev
Copy link

@alcueca
as i already stated, protocol was not going to hold any funds on Oracle account and was not aware of the fact that Oracle will be fee payer

as result first liquidations would not work, before they figure this out and top up Oracle account
because of that users were impacted as positions could not be liquidated on time.

@C4-Staff C4-Staff added Q-04 and removed M-04 labels 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 Q-04 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

8 participants