code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/fced18035107a345c31c9a9497d0da09105df4df/src/core/MToken.sol#L415

Vulnerability details

Impact

The accrual of interests is performed by modifying the borrowIndex, which is updated as: borrowIndexNew = (borrowRate * blockDelta) * borrowIndex + borrowIndex This results in a compounding interest formula based on the multiplication by the prior borrowIndex, and the accumulation of this compounded interest in the borrowIndex.

Based on the formula just described, the interest compounded by calling accrueInterest() once after several elapsed blocks is less than if accrueInterest() was to be called on each block for the same elapsed blocks.

This creates a discrepancy between the computed and the theoretical interests that will depend on the number of blocks elapsed while accrueInterest() wasn't called, which subsequently results in unpredictable interests.

Proof of Concept

Example: We will calculate the difference between the accrued interest at t3 if it was only called once at t3, and if it was accrued at t1, t2, and t3.

borrowRate per block: 0,1
borrowIndex at t0: 1

Formula: newBorrowIndex = borrowRatePerBlock * ElapsedBlocks * borrowIndex + borrowIndex

Executed once at t3:

borrowIndex(t3) = 0,1 * 3 * 1 + 1 = 1,3

Executed per block:

borrowIndex(t3) = 0,1 * 1 * 1 + 1 = 1,1

borrowIndex(t3) = 0,1 * 1 * 1,1 + 1,1 = 1,21

borrowIndex(t3) = 0,1 * 1 * 1,21 + 1,21 = 1,331

We can see there is a clear difference at t3 between accruing once per block and accruing after several elapsed blocks.

Tools Used

Manual Review

Recommended Mitigation Steps

To improve predictability, consider calculating interest with the compound interest formula, rather than simulating it through repeated transactions. Alternatively, consider implementing a keeper that makes sure that accrueInterest() is called at least once per block.

Assessed type

Math

0xSorryNotSorry commented 1 year ago

Invalid assumption since the accrual has the blocktimestamp validation;

        if (accrualBlockTimestampPrior == currentBlockTimestamp) {
            return uint(Error.NO_ERROR);
        }

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as low quality report

alcueca commented 1 year ago

There is no attached proof that simple interest, updated per block, is any worse than compound interest. However, I'll keep this as a grade-b QA in the case that it is of interest to the sponsor.

c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)