NFTVault lending rate compounding is ambiguous

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L190-L192 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L197

Vulnerability details

Impact

Realized rate compounding depends on the frequency and success of off-chain accrue() calls. With TVL growth user will demand strict definition of the lending rate used. 2% with continuous compounding and 2% with annual one differ by 2 basis points (exp(r_cont) = 1 + r_annual), which is big enough to care about when principals are substantial.

Griefing attack is possible when a malicious user calls accrue() frequently, increasing the effective rate for all borrowers. Economic effect of this can be larger than execution cost given big enough total debt.

Placing severity to be high as effective lending rate is the most crucial parameter for any lending protocol and here it can be manipulated to an extent. When an effective rate is greater than one user expected it is a fund loss for a user and a reputational loss for a protocol.

Proof of Concept

Now accrue() can be called by anyone with any frequency:

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L190-L192

The issue is that accrue() not only records current interest, but it updates the nominal (totalDebtAmount), which will be the base for interest computation for the next call:

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

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L197

The frequency of such an update, which is compounding, has a direct impact on the economy of all NFTVault loans and has to be clearly defined and fixed. Now it is determined by random sequence of calls as accrue() is invoked with most of user facing functions.

Recommended Mitigation Steps

Relying on off-chain script doesn't look to be enough in this regard, code level control is advised.

Consider adding a frequency threshold so accrue() can update totalDebtAmount say once a month and then state that the rate have monthly compounding. As accrue() calls are frequent enough the remaining optionality will be low.

This will give users more clarity and calling at least once a month can be additionally ensured via team monitored off-chain script.

[spaghettieth]

Duplicate of #133

[dmvt]

As with #133, this is a feature, not a bug.