Open c4-bot-2 opened 4 months ago
alcueca marked the issue as primary issue
piske-alex (sponsor) confirmed
alcueca marked the issue as satisfactory
alcueca marked the issue as selected for report
This seems QA to me as:
1) total_borrowed
isn't used anywhere and its actual value has zero impact on the protocol
2) This is false:
will be updated/increased in storage to be much higher than actual
The storage used will be the same, no matter the value of total_borrowed
, and there isn't any increase in cost no matter its value.
- total_borrowed isn't used anywhere and its actual value has zero impact on the protocol
Total borrowed is always used by the borrow fcn for every node wallet every time there is a new borrow or a loan repay. the values are increased or decreased respectively.
This is false: will be updated/increased in storage to be much higher than actual. The storage used will be the same, no matter the value of total_borrowed, and there isn't any increase in cost no matter its value.
why do you say this @DadeKuma ? anyway i wrote another extensive test (similar to the one in the issue) for it. Upon second borrow from the lender's node wallet, totalBorrowed will be increased again to become further higher than actual if first borrow is already liquidated. check it here -> https://gist.github.com/adeolu98/fd2ac477e95a0c02fdc2b74523fa3cab
Perhaps you talk about storage cost? this issue is not about that. this is about the value written into storage/global variable ctx.accounts.node_wallet.total_borrowed
.
this is clearly an accounting issue as total_funds and total_borrowed are used to track user accounting in the protocol. if you say it has zero impact then you are wrong. The protocol clearly manipulates the values of total_funds and total_borrowed on subsequent borrows, repays for loans serviced by a node wallet. This (total_borrowed) can stack up to become a ridiculously much higher value than actual.
Hey @adeolu98
Total borrowed is always used by the borrow fcn for every node wallet every time there is a new borrow or a loan repay. the values are increased or decreased respectively.
Yes, but only to keep track of the amount, it's never actually used for anything: https://github.com/search?q=repo%3Acode-423n4%2F2024-04-lavarage%20total_borrowed%20&type=code
why do you say this @DadeKuma ? ... Perhaps you talk about storage cost? this issue is not about that. this is about the value written into storage/global variable
I was saying that total_borrowed
is a u64
and it always allocates 8 bytes, I thought that you were referring about storage cost.
this is clearly an accounting issue as total_funds and total_borrowed are used to track user accounting in the protocol.
What you are saying is true, this variable can have a wrong value. But I don't see how it can be abused in the current codebase. This is why I think it's QA, funds/availability of the protocol are not at risk.
What you are saying is true, this variable can have a wrong value. But I don't see how it can be abused in the current codebase. This is why I think it's QA, funds/availability of the protocol are not at risk.
Quote from c4 docs
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.
but @DadeKuma the protocol doesn't function properly or as intended. It is only tracked/manipulated in borrow, repay and not liquidations. liquidations also affect borrower and lender node_wallet. ctx.accounts.node_wallet.total_borrowed
was created for a particular purpose but isn't currently being utilized in all required parts of the code and this causes subsequent borrow and repay to manipulate wrong values and execute sucessfully. Its a miss in logic.
@DadeKuma is right, total_borrowed
is tracked, but not used by the protocol. The function of the protocol is not impacted (i.e., users can do everything that they wanted to do). There is no statement of how value could leak under reasonable assumptions.
alcueca changed the severity to QA (Quality Assurance)
alcueca marked the issue as grade-a
alcueca marked the issue as not selected for report
Lines of code
https://github.com/code-423n4/2024-04-lavarage/blob/9e8295b542fb71b2ba9b4693e25619585266d19e/libs/smart-contracts/programs/lavarage/src/processor/liquidate.rs#L19
Vulnerability details
Impact
liquidations via
liquidate()
return/add funds to the lender's token account (funds was taken or lent out during the borrow to user) and close borrow positions but dont reduce the total amount borrowed. This means:on the next borrow (via call to swap.rs
borrow()
fcn) from the lender's node wallet, program will add to the already false/inflated/not reducednode_wallet_total_borrow
amount thus causing accouting issues/false data to persist/updated into contract storage.ui will display wrong data as
node_wallet.total_Borrowed
will be higher than it ought to beProof of Concept
the function liquidate() does not and decrease the
ctx.accounts.node_wallet.total_borrowed
after a liquidation is executed.If during borrow() these variables are modified to reflect a borrow action as seen here then they should be modified again to reflect a liquidation event as liquidations return the borrwed funds back to the lender and should reduce the
total_borrowed
amount because funds are not in a borrow position any longer.ctx.accounts.node_wallet.total_borrowed
tracks the total amount of lender funds currently lent/borrowed to users in various positions at any given moment.Since this modifications is not made,this means
ctx.accounts.node_wallet.total_borrowed
will not be decreased on liquidation thus falsly reporting that the lender has higher amount of funds in active borrow position(s). Upon another borrow by the lender to a user thectx.accounts.node_wallet.total_borrowed
will be updated/increased in storage to be much higher than actual.Scenario
ctx.accounts.node_wallet.total_funds
is increased to 1ctx.accounts.node_wallet.total_funds
is decreased to 0.5 sol andctx.accounts.node_wallet.total_borrowed
is increased to 0.5 solctx.accounts.node_wallet.total_borrowed
is still old amount borrowed out by lender before the liquidation (0.5 sol). even though lender actually has no funds borrowed out at the moment. (all collateral has been returned)ctx.accounts.node_wallet.total_borrowed
becomes 1 sol. even though at that moment lender only supplied one loan position worth 0.5 sol.I made a test to further describe this scenario. To run the test please
1.) modify the
PositionOpenEvent
andPositionCloseEvent
instate/position.rs
to be like below. I made the modification to be able to watch the changes to these values (total_funds and total_borrowed) better in the tests.2.) modify the
PositionOpenEvent
emit in add_collateral() fcn inswap.rs
to be2.) modify the
PositionCloseEvent
emit in repay_sol() fcn inswapback.rs
to be3.) modify the
PositionCloseEvent
emit in liquidate() fcn inliquidate.rs
to be4.) modify the AddCollateral context in
add_collateral.rs
like below5.) paste in ./tests folder and run with
ORACLE_PUB_KEY=ATeSYS4MQUs2d6UQbBvs9oSNvrmNPU1ibnS2Dmk21BKZ anchor test
Tools Used
anchor, manual review
Recommended Mitigation Steps
at the end current
liquidate()
fcn logic, add a line to reduce the total_borrwed by the amount liquidated.Assessed type
Context