Division before Multiplication May Result In No Interest Being Accrued

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L590-L595

Vulnerability details

Impact

There is a division before multiplication bug in NFTVault._calculateAdditionalInterest() which may result in no interesting being accrued and will have significant rounding issues for tokens with small decimal places.

This issue occurs since an intermediate calculation of interestPerSec may round to zero and therefore the multiplication by elapsedTime may remain zero.

Furthermore, even if interestPerSec > 0 there will still be rounding errors as a result of doing division before multiplication and _calculatedInterest() will be understated.

This issue is significant as one divisor is 365 days = 30,758,400 (excluding the rate). Since many ERC20 tokens such as USDC and USDT only have 6 decimal places a numerator of less 30 * 10^6 will round to zero.

The rate also multiplies into the denominator. e.g. If the rate is 1% then the denominator will be equivalent to 1 / rate * 30 * 10^6 = 3,000 * 10^6.

Proof of Concept

The order of operations for the interest calculations

• totalDebtAmount
• MUL settings.debtInterestApr.numerator
• DIV settings.debtInterestApr.denominator
• DIV 365 days
• MUL elapsedTime

If the intermediate value of interestPerSec = 0 then the multiplication by elapsedTime will still be zero and no interested will be accrued.

Excerpt from NFTVault._calculateAdditionalInterest().

        uint256 interestPerYear = (totalDebtAmount *
            settings.debtInterestApr.numerator) /
            settings.debtInterestApr.denominator;
        uint256 interestPerSec = interestPerYear / 365 days;

        return elapsedTime * interestPerSec;

Recommended Mitigation Steps

This issue may be resolved by performing the multiplication by elapsedTime before the division by the denominator or 365 days.

        uint256 interestAccrued = (elapsedTime * 
            totalDebtAmount *
            settings.debtInterestApr.numerator) /
            settings.debtInterestApr.denominator /
            365 days;

        return  interestAccrued;

[spaghettieth]

Duplicate of #40

[dmvt]

I'm going to leave this as the primary and make #40 a duplicate. This report makes sense as a medium to me because it involves a calculation error that can lead to the protocol functioning incorrectly.