search

code-423n4 / 2024-04-lavarage-findings

2 stars 2 forks source link

Borrowers can steal lenders funds #4

Closed c4-bot-6 closed 4 months ago

c4-bot-6 commented 4 months ago

Lines of code

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

Vulnerability details

Proof of Concept

When trader borrows sol from trading pool, then position account is created. Pls, note, that random_account_as_id field that is provided by trader is used to create PDA. This is needed, because same trader can have several position with same trading pool.

borrow function checks that there will be add_collateral instruction later in transaction and only after that it borrows sol to the trader. This is because add_collateral should check that trader has provided tokens as collateral. Also borrow function stores random_account_as_id to the position, so we know which random was used to derive position account.

add_collateral function checks that position account is now funded with enough amount of tokens to cover borrowed funds. add_collateral function is provided with context, where position account is provided with random_account_as_id as well.

The problem is that trader can provide another position, which is already collateralized. In that case trader will be able to steal all borrowed funds.

Pls, note that there is same problem inside swapback.rs, which allows to steal tokens from position without repaying sol on it.

Impact

Funds can be stolen.

Tools Used

VsCode

Recommended Mitigation Steps

In the function that is responsible for the check of final instruction you should check that position account provided is same.

Assessed type

Error

c4-judge commented 4 months ago

alcueca marked the issue as primary issue

c4-judge commented 4 months ago

alcueca marked issue #28 as primary and marked this issue as a duplicate of 28

c4-judge commented 4 months ago

alcueca marked the issue as satisfactory

rvierdiiev commented 4 months ago

hello @alcueca i see that you have separated this bug into 2 bugs.

Pls, note this sentence from report: Pls, note that there is same problem inside swapback.rs, which allows to steal tokens from position without repaying sol on it.. I thought that as it is very similar it is better to put into 1 report. As for now, i guess that it should be duplicate of both separate bugs.

Arabadzhiew commented 4 months ago

Given that this report only mentions the other issue in one ambiguous sentence, that does not even pinpoint the root cause of the issue, I don't think that it would be fair to also mark it as a dup of #13 and #26 solely based on that.

alcueca commented 4 months ago

I think that in this case these issues can be separate.

rvierdiiev commented 4 months ago

Given that this report only mentions the other issue in one ambiguous sentence, that does not even pinpoint the root cause of the issue, I don't think that it would be fair to also mark it as a dup of https://github.com/code-423n4/2024-04-lavarage-findings/issues/13 and https://github.com/code-423n4/2024-04-lavarage-findings/issues/26 solely based on that.

In same way it's not fair to me. In this case it would be fair to leave it as 1 report.

@alcueca I have explained exactly how it works in one case and another one is exactly same(same problem). I don't see the need to explain it separately. And the problem is indeed same, position random is not checked.

rvierdiiev commented 4 months ago

hey @alcueca i am not sure, you saw the comment above if you are not going to split this issue into 2 and give full credit, then pls, consider at least make it as duplicate with partial credit.