Implemented period based yield analysis on SavingsManager

[alsco77]

Upgrade type: MODULE REPLACEMENT VIA NEXUS
Affected file: SavingsManager.sol
Version: 1.1

Description

Address bug originally submitted through Bug Bounty - See Devan - Bug bounty response.pdf

Removes the blocker on collecting interest more than once in 30 minute period.

Retains existing 'extrapolatedAPY' calculations when it has been longer than 30 minutes since the last interest collection.

If it has been less than 30 minutes, it simply checks that the supply has not inflated by more than 0.1% (or 1e15) during that PERIOD (30 mins). At a 0.1% 'SWAP' fee, this would mean that the total supply of the mAsset would need to be swapped between the two collections, if this were to be hit.

Mechanism:

        //    If it has been 30 mins since last collection, reset period data
        //    Calculate APY from lastCollection, interest, newTotalSupply (like before)
        if(timeSinceLastCollection > THIRTY_MINUTES) {
            lastPeriodStart[_mAsset] = now;
            periodYield[_mAsset] = 0;
        }
        //    Else if period has elapsed, start a new period from the lastCollection time
        //    Its under 30 mins since last collection, so we can calculate the increase
        //    and protect it from going over 10 bps
        else if(timeSincePeriodStart > THIRTY_MINUTES) {
            lastPeriodStart[_mAsset] = previousCollection;
            periodYield[_mAsset] = interestCollected;
        }
        //    Else add yield to period yield
        //    We use the full yield for the period in the 10 bps calculation
        else {
            inflationOperand = periodYield[_mAsset].add(interestCollected);
            periodYield[_mAsset] = inflationOperand;
        }

        //   0        30        60        90        120       
        //   | - - - - | - - - - | - - - - | - - - - |
        //   ^            ^   ^    ^    ^            ^
        //  start        40   50  65    80          120
        //  @time - Description (periodStart, lastCollection, periodYield)
        //  @40 - Should start new period (40, 40, 0)
        //  @50 - Should calc in period 40-70 (40, 50, X)
        //  @65 - Should calc in period 40-70 (40, 65, X+Y)
        //  @80 - Should start new period from last collection, 65-95 (65, 80, Z)
        //  @120 - Should start new period (120, 120, 0)

Deployment plan

• Write and comprehensively test the upgrade
• Get external PR review from specialist
  • 1 approval
  • 2 approval
• Deploy SavingsManager v1.1 to Ropsten/Mainnet
• Use DiffChecker to validate the prev and future contracts once verified
• Call proposeUpgrade on the Nexus with implementation addresses
• Verify proposal is in the Nexus and the keys match up
• Accept on Ropsten and test
• Wait 1 week
• Accept on Mainnet FORK by unlocking the gov address
• Accept on Mainnet

Mainnet Details

Deployed address: 0x7046b0bfc4c5eeb90559c0805dd9c1a6f4815370

Ropsten Details

Deployed address: 0x755629d1ecad92db2b81a3a88207bf6d2b864136

[dpurhar27]

Overall, this looks good besides the one comment I made above. Removing the time barrier before collecting interest and always updating the exchange rate fixes the original issue 👍.

Based on the provided example:

this would mean that the total supply of the mAsset would need to be swapped between the two collections

I think the chosen limit of 0.1% isn't too restrictive as long as the AUM is high relative to trade volume.

Note, copying my original comment over from the private dev. repo