Lone Fossilized Lemur - Incorrect Event Time Update Sequence Leads to Cumulative Rate Calculation Error

[sherlock-admin3]

Lone Fossilized Lemur

High

Incorrect Event Time Update Sequence Leads to Cumulative Rate Calculation Error

Summary

A significant implementation flaw has been discovered in the Borrowing::_withdraw function where premature updating of lastEventTime leads to inaccurate cumulative rate calculations. This timing issue results in the miscalculation of borrower debt, ultimately causing financial disadvantage to the protocol.

Root Cause

In borrowing::_withdraw this line is lastEventTime = uint128(block.timestamp); is updating lastEventTime to the the current time, even before calling calculateCumulativeRate
this will cause incorrect calculations in function calculateCumulativeRate

Here is the flow after that ->

function borrowing::calculateCumulativeRate

    function calculateCumulativeRate() public returns (uint256) {
        // Get the noOfBorrowers
        uint128 noOfBorrowers = treasury.noOfBorrowers();
        // Call calculateCumulativeRate in borrow library
@>        uint256 currentCumulativeRate = BorrowLib.calculateCumulativeRate(
            noOfBorrowers,
            ratePerSec,
// @audit issue block.timestamp
@->>        lastEventTime,
            lastCumulativeRate
        );
        lastCumulativeRate = currentCumulativeRate;
        return currentCumulativeRate;
    }

Now in BorrowLib::calculateCumulativeRate will uint256 timeInterval = uint128(block.timestamp) - lastEventTime;
will result to = 0 , because our lastEventTime was already updated to block.timestamp, as shown above.

    function calculateCumulativeRate(
        uint128 noOfBorrowers,
        uint256 ratePerSec,
        uint128 lastEventTime,
        uint256 lastCumulativeRate
    ) public view returns (uint256) {
        uint256 currentCumulativeRate;

        // If there is no borrowers in the protocol
        if (noOfBorrowers == 0) {
            // current cumulative rate is same as ratePeSec
            currentCumulativeRate = ratePerSec;
        } else {
            // Find time interval between last event and now
    
// @audit this will come 0        
@->>            uint256 timeInterval = uint128(block.timestamp) - lastEventTime;
            //calculate cumulative rate

// incorrect calculation due to above mistake
@->>            currentCumulativeRate = lastCumulativeRate * _rpow(ratePerSec, timeInterval, RATE_PRECISION);
            currentCumulativeRate = currentCumulativeRate / RATE_PRECISION;
        }
        return currentCumulativeRate;
    }

And this will negatively impact this calculation of currentCumulativeRate by giving a smaller value, which then will be updated in here in state variable of borroing.sol over here lastCumulativeRate = currentCumulativeRate; and a wrong value will be used for all other purposes of protocol calculations as well.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

• from next time every borrower will pay less debt than intended because lastCumulativeRate state variable will always be calculated less than it should be, which later used in debt calculation.
• loss to the protocol as they will get less debt fee than intended when the borrower will repay using withdraw.

PoC

No response

Mitigation

The timing of the lastEventTime update should be modified to occur after the calculateCumulativeRate() call, following the pattern established in the borrowing::depositToken function. This ensures accurate time interval calculations and proper rate accumulation.