diff --git a/CHANGELOG.md b/CHANGELOG.md index b85f0d7a8c..a19659a58d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -103,6 +103,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ - [1018](https://github.com/umee-network/umee/pull/1018) Return nil if negative time elapsed from the last block happens. - [1156](https://github.com/umee-network/umee/pull/1156) Propagate context correctly. +- [1288](https://github.com/umee-network/umee/pull/1288) Safeguards LastInterestTime against time reversals and unintended interest from hard forks. ## [v2.0.2](https://github.com/umee-network/umee/releases/tag/v2.0.2) - 2022-05-13 diff --git a/x/leverage/keeper/interest.go b/x/leverage/keeper/interest.go index 911a91d1b9..2f775fd0db 100644 --- a/x/leverage/keeper/interest.go +++ b/x/leverage/keeper/interest.go @@ -67,7 +67,11 @@ func (k Keeper) DeriveSupplyAPY(ctx sdk.Context, denom string) sdk.Dec { func (k Keeper) AccrueAllInterest(ctx sdk.Context) error { currentTime := ctx.BlockTime().Unix() prevInterestTime := k.GetLastInterestTime(ctx) - if prevInterestTime == 0 { + if prevInterestTime <= 0 { + // if stored LastInterestTime is zero (or negative), either the chain has just started + // or the genesis file has been modified intentionally. In either case, proceed as if + // 0 seconds have passed since the last block, thus accruing no interest and setting + // the current BlockTime as the new starting point. prevInterestTime = currentTime } @@ -80,9 +84,19 @@ func (k Keeper) AccrueAllInterest(ctx sdk.Context) error { "prev", prevInterestTime, ) + // if LastInterestTime appears to be in the future, do nothing (besides logging) and leave + // LastInterestTime at its stored value. This will repeat every block until BlockTime exceeds + // LastInterestTime. return nil } + yearsElapsed := sdk.NewDec(currentTime - prevInterestTime).QuoInt64(types.SecondsPerYear) + if yearsElapsed.GTE(sdk.OneDec()) { + // this safeguards primarily against misbehaving block time or incorrectly modified genesis states + // which would accrue significant interest on borrows instantly. Chain will halt. + return types.ErrExcessiveTimeElapsed.Wrapf("BlockTime: %d, LastInterestTime: %d", + currentTime, prevInterestTime) + } // fetch required parameters tokens := k.GetAllRegisteredTokens(ctx) @@ -178,6 +192,14 @@ func (k *Keeper) SetLastInterestTime(ctx sdk.Context, interestTime int64) error store := ctx.KVStore(k.storeKey) timeKey := types.CreateLastInterestTimeKey() + prevTime := k.GetLastInterestTime(ctx) + + if interestTime < prevTime { + // prevent time from moving backwards + return types.ErrNegativeTimeElapsed.Wrapf("cannot set LastInterestTime from %d to %d", + prevTime, interestTime) + } + bz, err := k.cdc.Marshal(&gogotypes.Int64Value{Value: interestTime}) if err != nil { return err diff --git a/x/leverage/keeper/keeper_test.go b/x/leverage/keeper/keeper_test.go index 045e157f64..d5dfe310c4 100644 --- a/x/leverage/keeper/keeper_test.go +++ b/x/leverage/keeper/keeper_test.go @@ -661,6 +661,36 @@ func (s *IntegrationTestSuite) TestAccrueZeroInterest() { s.Require().Equal(sdk.MustNewDecFromStr("0.000948"), supplyAPY) } +func (s *IntegrationTestSuite) TestAccrueInterest_Invalid() { + // The "supplier" user from the init scenario is being used because it + // already has 1k u/umee for collateral. + addr, _ := s.initBorrowScenario() + + // user borrows 40 umee + err := s.app.LeverageKeeper.Borrow(s.ctx, addr, sdk.NewInt64Coin(umeeapp.BondDenom, 40000000)) + s.Require().NoError(err) + + // Setting last interest time to a negative value is not allowed + err = s.tk.SetLastInterestTime(s.ctx, -1) + s.Require().ErrorIs(err, types.ErrNegativeTimeElapsed) + + // Setting last interest time ahead greatly succeeds + err = s.tk.SetLastInterestTime(s.ctx, 100_000_000) // 3 years + s.Require().NoError(err) + + // Interest will not accrue (suite BlockTime = 0) but will not error either + err = s.app.LeverageKeeper.AccrueAllInterest(s.ctx) + s.Require().NoError(err) + + // Setting last interest time back to an earlier time is not allowed + err = s.tk.SetLastInterestTime(s.ctx, 1000) + s.Require().ErrorIs(err, types.ErrNegativeTimeElapsed) + + // verify user's loan amount is unchanged (40 umee) + loanBalance := s.app.LeverageKeeper.GetBorrow(s.ctx, addr, umeeapp.BondDenom) + s.Require().Equal(loanBalance, sdk.NewInt64Coin(umeeapp.BondDenom, 40000000)) +} + func (s *IntegrationTestSuite) TestDynamicInterest() { // Init scenario is being used because the module account (lending pool) // already has 1000 umee. diff --git a/x/leverage/types/errors.go b/x/leverage/types/errors.go index 357fb61401..861cd0cb3b 100644 --- a/x/leverage/types/errors.go +++ b/x/leverage/types/errors.go @@ -38,6 +38,7 @@ var ( 1126, "market total collateral would exceed MaxCollateralShare", ) - ErrNotUToken = sdkerrors.Register(ModuleName, 1127, "denom should be a uToken") - ErrUToken = sdkerrors.Register(ModuleName, 1128, "denom should not be a uToken") + ErrNotUToken = sdkerrors.Register(ModuleName, 1127, "denom should be a uToken") + ErrUToken = sdkerrors.Register(ModuleName, 1128, "denom should not be a uToken") + ErrExcessiveTimeElapsed = sdkerrors.Register(ModuleName, 1130, "excessive time elapsed since last interest time") )