Updating node_wallet.total_funds based on user input is problematic and could mess up with accounting

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

A malicious actor can send SOL directly to a node_wallet (lender's funding account). When the lender withdraws all available SOL amount, ctx.accounts.node_wallet.total_funds will underflow and becomes a big number. This is because ctx.accounts.node_wallet.total_funds is updated based on user input while not considering external direct transfer.

	let from = &ctx.accounts.node_wallet.to_account_info();
	let to = &ctx.accounts.funder;
	**from.try_borrow_mut_lamports()? -= amount;
	**to.try_borrow_mut_lamports()? += amount;
	ctx.accounts.node_wallet.total_funds -= amount;

lending.rs#L87

As a result, the accounting in the protocol for the node and relevant trading pool is messed up as total_funds is a core invariant in the protocol.

Another way of manipulating total_funds:

if the borrower set the fee_receipient to the node account, the ctx.accounts.node_wallet.total_funds will have an incorrect value, because it deducts the interest_share from repay_amount even if fee_receipient was the node account.

  ctx.accounts.node_wallet.total_funds += repay_amount - interest_share;

swapback.rs#L196

Proof of Concept

Included within the impact description above for easiness

Tools Used

Manual analysis

Recommended Mitigation Steps

Always update ctx.accounts.node_wallet.total_funds based on the actual balance (lamports) of node_wallet. This way, we get consistency naturally by design.

Assessed type

Under/Overflow

[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]

Similar to #30, this seems QA to me. The attacker has zero incentives in doing this and the protocol is not at risk. They are donating money to get nothing in return, which is a self-rekt. total_funds isn't used anywhere, and there isn't a leak of funds.

[koolexcrypto]

To not repeat the same comment again. Please check this #30 (comment)

Thank you

[alcueca]

I agree with @DadeKuma. While it might be annoying to not be able to trust total_funds, the protocol will function normally. No funds will be lost, and the team will have to find a different way to display the correct data in the frontend. That's about 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