Repaying the debt twice is allowed, which cause total_borrowed to underflow

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

A malicious actor can open a position with a low amount, and repay it twice. This will cause ctx.accounts.node_wallet.total_borrowed to underflow, making it a big number.

  ctx.accounts.node_wallet.total_borrowed -= borrowed_amount;

swapback.rs#L197

This is possible because the protocol doesn't prevent repaying the already paid position. The malicious actor can do this immediately after the trading pool is created by the lender and funded.

As a result, the accounting in the protocol is messed up as total_borroweddoesn't represent the actual borrowed value.

Proof of Concept

Included within the impact description above for easiness

Tools Used

Manual analysis

Recommended Mitigation Steps

Disallow repaying the same position twice. This also protects the borrower from making a mistake by repaying the position twice.

Assessed type

Invalid Validation

[c4-sponsor]

piske-alex (sponsor) confirmed

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[DadeKuma]

This seems more on the QA side for me.

First of all, this is a self-rekt as the user gets nothing in return (why would they pay their debt twice?).

Second, even if total_borrowed underflows, it isn't used anywhere. The protocol is not at risk and there isn't a leak of value:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

[koolexcrypto]

@DadeKuma

Thank you for your feedback

First of all, this is a self-rekt as the user gets nothing in return (why would they pay their debt twice?).

A malicious user could do that, the action is very cheap. you just borrow the minimum amount and repay it twice. I can produce actual values in case the judge requires that.

Second, even if total_borrowed underflows, it isn't used anywhere. The protocol is not at risk and there isn't a leak of value

total_borrowed is a core invariant and should hold a correct value that represents the actual borrowed amounts, the lender can't track how much was borrowed and could be easily deceived by this. Moreover, borrowers will also not take the protocol (or the lenders) seriously. Imagine a scenario where someone caused underflow for many node wallets that are on the landing page of the protocol. As a borrower, if I see approx. 18446744073.709551615 SOL borrowed (max u64), would I even try to borrow from the protocol? This amount 18446744073 SOL would equal 2.5 trillion USD (at the price of 1 SOL = 139 USD) Thus, the lack of correct values prevents the users from making informed decisions which harm lenders.

Furthermore, lenders who use programs (e.g bots) to automate lending, would have wrong assumptions since total_borrowed is incorrect.

CC: @c4-judge

[alcueca]

As for #11, the protocol keeps working normally, annoying as it is to lose a metric on it.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as not selected for report

[c4-judge]

alcueca marked the issue as grade-a