Open sherlock-admin4 opened 4 months ago
Not an issue, the yield that accumulated and is for distribution was generated over the last 30 days and thus should match the initial fix-point of the prior 30 day EMA calculation. There's dilution that occurs with this methodology that we don't want to introduce into our overall accounting system
1 comment(s) were left on this issue during the judging contest.
panprog commented:
medium, distributeYield uses outdated ema values, updating them after all calculations. However, the impact is limited, because the expected EMA averaging period is 2 months and it's unlikely that distributeYield is called so rarely that the impact of outdated ema values causes any real issues.
After considering developer's response and intentions, changing this to invalid.
According to developers response, this is intentional, because using current EMA (which includes current totalSupply at the time of distribution, which might be manipulated) can introduce some attack vectors from the current totalSupply, so current distribution is done over EMA from the previous period.
Sponsor response:
we did a run-through on accounting scenarios It is indeed underpaying the depositors By what we think is fair within the system And manipulation isn't possible because they'd have to deposit and lock up tokens so there's no flash-loan etc.
Agree with sponsor, changing back to Medium
The protocol team fixed this issue in the following PRs/commits: https://github.com/Zivoe/zivoe-core-foundry/pull/263
The Lead Senior Watson signed off on the fix.
t0x1c
high
distributeYield() calls earningsTrancheuse() with outdated emaSTT & emaJTT while calculating senior & junior tranche yield distributions
Summary
The distributeYield() function internally calls
earningsTrancheuse()
on L232 which calculates & returns the_seniorTranche
&_juniorTranche
yield distribution. However, this call results inearningsTrancheuse()
using outdated values ofemaSTT
&emaJTT
on L461-L462 as these variables are updated only on L238-L239 after the call toearningsTrancheuse()
is concluded.Code Snippet
and
Vulnerability Detail
As the docs explain, the EMA plays a significant role in yield distribution calculations.
The EMA is used to calculate the:
yT, eSTT & eJTT
.Both the above mentioned calls happen inside
earningsTrancheuse()
here. The issue is that these global variablesemaSTT and emaJTT
are still outdated and correspond to the ones belonging to thelastDistribution
timestamp instead of current updated ones. These values are updated only after the call toearningsTrancheuse()
has concluded here.Impact
Imagine that in the last 30 days, JTT supply has reduced due to defaulted loans. As a result, the target yield $yT{real}$ would come down too (i.e. $yT{real}$ < $yT$ where $yT$ is the protocol calculated incorrect target yield) and the junior tranche ditributable yield will be smaller than before. However if there is excess yield, then the junior tranche would receive more than their fair share since last 30 days have not been considered by the protocol calculations. If the quantum of defaulted loans is high (and hence $yT_{real}$ has dipped significantly), the impact is not only limited to the junior tranche, but then also effects the senior tranche.
On the flip side an increase in the adjusted supplies of STT and JTT in the last 30 days will have a impact on the smoothened emaSTT and emaJTT, making the values go higher. As a result, target yield $yT{real}$ would go up too. Thus, it could happen that
yD
is less than $yT{real}$ but the protocol is oblivious to that due to usage of outdated values. This would result in the protcol using seniorProportionBase() to calculate the senior's yield instead of seniorProportionShortfall().Finally, since at each of these function calls an outdated eSTT is passed as a param, the value returned would be more/less than what it should be, thus causing gain/loss of yield for both the tranches (juniorProportion() calculation has the same problem).
Note: For the very first distribution i.e. when
distributionCounter = 1
, the values used are from 60 days ago instead of 30 days as can be seen insideunlock()
, further increasing the margin of error. The 60 day gap has initially been provided by the protocol to allow for more time to bring in initial yield.Tool used
Manual Review
Recommendation
Update the ema values before calling
earningsTrancheuse()
: