code-423n4 / 2023-05-venus-findings

2 stars 1 forks source link

`borrowRatePerBlock()` is not fresh and will not include interests accrued #556

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/VToken.sol#L587-L590

Vulnerability details

borrowRatePerBlock() returns the borrowing rate, but the interests accrued are not included.

VToken.sol
587: function borrowRatePerBlock() external view override returns (uint256) {
588:         return interestRateModel.getBorrowRate(_getCashPrior(), totalBorrows, totalReserves);
589:     }
590: 

Impact

Any smart contract relying on the value of borrowRatePerBlock() to perform some logic - such as decide whether to borrow from VToken based on the returned value - will be impacted.

For instance, imagine a protocol ABC having a function in their contract that calls borrowRatePerBlock() and have a conditional call to VToken.borrow() if the borrowing rate is less than N. The call to borrowRatePerBlock() may return a value M < N, but the actual borrowing rate processed in borrow() can be P > N due to interest accrual.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Include some interest accrual logic in borrowRatePerBlock() - note that it cannot be a simple call to accrueInterest as borrowRatePerBlock() is a view function.

Assessed type

Timing

c4-sponsor commented 1 year ago

chechu marked the issue as disagree with severity

chechu commented 1 year ago

Smart contracts integrating borrowRatePerBlock() can call accrueInterest() first to get updated borrow rate

0xean commented 1 year ago

I think QA is appropriate here, documentation could be improved to make the integration pattern more clear OR a view function with the logic needed for this to work could be created to avoid additional gas costs... but hard to declare this as M severity as is.

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)