Interest is not accrued before parameters are updated in SavingsVest

[code423n4]

Lines of code

https://github.com/AngleProtocol/angle-transmuter/blob/9707ee4ed3d221e02dcfcd2ebaa4b4d38d280936/contracts/savings/SavingsVest.sol#L196

Vulnerability details

Impact

Stablecoin holders can receive wrongly calculated yield in the SavingsVest contract. Also, wrong vesting profit can be slashed when the protocol is under-collateralized.

Proof of Concept

The SavingsVest contract lets users deposit their stablecoins and earn vested yield when the stablecoin in the Transmuter protocol is over-collateralized. The interest is accrued via calls to the SavingsVest.accrue function.

There are two parameters that affect the profit of depositors:

1. protocolSafetyFee is the fees paid to the protocol;
2. vestingPeriod is the period when the yield remains locked.

The two parameters can be changed via the setParams function. However, before they're changed, the current interest is not accrued. E.g. this may lead to:

1. If protocolSafetyFee is increased without accruing interest, the next accrual will happen at the increased fees, which will reduce the rewards for the depositors.
2. If vestingPeriod is increased without accruing interest, the yield will be locked for a longer period and the next accrual may slash more vested yield.

Thus, users can lose a portion of the yield that was earned at a lower protocol fee after the fee was increased. Likewise, increasing the vesting period may result in slashing yield that was earned before the period was increased.

Tools Used

Manual review

Recommended Mitigation Steps

In the SavingsVest.setParams function, consider accruing interest with the current parameters before setting new protocolSafetyFee and vestingPeriod.

Assessed type

Other

[c4-judge]

hansfriese marked the issue as primary issue

[c4-sponsor]

Picodes marked the issue as sponsor confirmed

[Picodes]

Confirmed. The worst scenario is that when modifying the vestingPeriod it can lead to a jump in lockedProfit, so to a jump in totalAssets(). So eventually someone could sandwich attack the update for a profit

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as selected for report