Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: safeguard LastInterestTime against hard forks #1288

Merged
merged 7 commits into from
Aug 30, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,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

Expand Down
24 changes: 23 additions & 1 deletion x/leverage/keeper/interest.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -80,9 +84,19 @@ func (k Keeper) AccrueAllInterest(ctx sdk.Context) error {
"prev", prevInterestTime,
)

// if LastInterestTime appears to tbe in the future, do nothing (besides logging) and leave
toteki marked this conversation as resolved.
Show resolved Hide resolved
// 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.
toteki marked this conversation as resolved.
Show resolved Hide resolved
return types.ErrExcessiveTimeElapsed.Wrapf("BlockTime: %d, LastInterestTime: %d",
currentTime, prevInterestTime)
}

// fetch required parameters
tokens := k.GetAllRegisteredTokens(ctx)
Expand Down Expand Up @@ -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 {
toteki marked this conversation as resolved.
Show resolved Hide resolved
// 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
Expand Down
30 changes: 30 additions & 0 deletions x/leverage/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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().Error(err)
toteki marked this conversation as resolved.
Show resolved Hide resolved

// 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().Error(err)

// 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.
Expand Down
5 changes: 3 additions & 2 deletions x/leverage/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)