Interest rates will not compound correctly if seldom called

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L468

Vulnerability details

Impact

While interest rates for second-by-second compounding are calculated correctly they are then not call every second, which leads to incorrect amounts of interest being calculated. This may have implications for the entire stability of the coin as the interest rate that lenders/borrowers think they are receiving/paying is very important to them.

This has been assessed as Medium Risk due the fact that effect is less pronounced at lower interest rates and when _addInterest is called frequently.

Proof of Concept

If you google for daily compound interest formulas on the Internet, most of them are wrong, even though they are approximately correct for small interest rates. So it was with some joy that I noticed you must have used the correct formula for working out what the second-by-second rate must be for a 10,000% annual rate in contract VariableInterest.

In general, if

• $d$ is number of number of divisions of the year (e.g. $d = 52$ for weeks, $d = 365.24$ for days, $d = 365.24 \times 86400$ for seconds)
• $r$ is interest rate for each division
• $R$ is annual interest rate

then the formula is:

$$ r = e^{1 + R \over d} - 1 $$

For $d = 365.24 * 86400$, $R = 10,000% = 100$ we get:

$$ r = 1.462483589786956 \times 10^{-7} $$

which as a uint256 (with 18 decimals of precision) is 146248358979 which is really close to what you calculated.

uint64 private constant MAX_INT = 146248508681; // 10,000% annual rate

Many sites on the Internet will tell you that all you need to do use the formula

$$ r = {R \over d} $$

which works fine when $R$ is small but is increasingly incorrect as $R$ gets large.

For $d = 365.24 \times 86400$

$R$	$e^{1 + R \over d} - 1$	${R \over d}$	% error
0.25%	79123597	79222389	0.12%
1%	315315551	316889554	0.5%
10%	3020280026	3168895541	4.92%
100%	21965110397	31688955410	44.27%
1,000%	75986799297	316889554103	317%
10,000%	146248358979	3168895541034	2067%

So it's really great to see you using the correct formula for working out the second-by-second compound rate.

However, once you have chosen to use a second-by-second compound rate it must be applied every second in order.

Line 468 defines the interest calculation as

_interestEarned = (_deltaTime * _totalBorrow.amount * _currentRateInfo.ratePerSec) / 1e18;

This formula uses the per-second interest rate per and multiplies by the "delta" between the current block timestamp and the last time _currentRateInfo was updated.

However, this will lead to incorrect values for high values of $R$ if the delta is large. Calls to _addInterest can occur an arbitrary amount of time apart.

Let's assume that _deltaTime is 1 week and now see how much interest would be accrued at 10,000% over 1 year.

Let $d = 365.24 \times 86400$ and $n = 86400 * 7$. Then we have

$$ (1 + n \times 1.462483589786956 \times 10^{-7})^{d \over n} - 1 = 8229\% $$

This is well short of the 10,000% it should earn.

The table below shows the equivalent annual interest calcualted for various "call periods" which is the frequency with with _addInterest is called.

Call period	Interest earned (vs 10000%)
1 second	10,000%
1 day	9,712%
1 week	8,229%
30 days	4,905%
90 days	2,081%

Recommended Mitigation Steps

The easiest solution to this problem would be to calculate interest rates for a period larger than 1 second, certainly larger than the smallest block time.

It is then important that the interest calculations occur periodically, not just when people interact with the contract. It maybe be necessary to set up a bot to call the contract with this frequency.

Another solution would be to calculate the correct interest rate based on the delta between the current call and the previous. However, this involves complex maths that would be costly in gas.

[0xA5DF]

Similar to #145

[DrakeEvans]

Intended functionality, compounding every block is too costly in gas to make default behavior, if lenders want, they can call addInterest() to compound every block but unfair to put gas cost of doing so on average users. Additionally, the difference is earned interest is very small unless interest rates are very very high.

[0xA5DF]

if lenders want, they can call addInterest() to compound every block

I think this should at least be known to users (via docs/UI) that not calling it frequently enough can cause some difference in the interest

but unfair to put gas cost of doing so on average users

Check out my submission for this bug (#145), the solution there shouldn't cost much gas for avg tx (just checking if addInterest() wasn't called for a long time, and if so calculate interest a bit differently).

[gititGoro]

Great report and the warden clearly enjoyed the write up. #145 is very similar. However, 145 offers a gas-feasible mitigation. There are a few typos but it serves well enough as pseudo code. Similar to this report, 145 also charts out the problem with illustrative numbers. So for these two reasons, I'm setting 145 as the original and this as the duplicate.

[gititGoro]

duplicate of #145