Skip to content

Commit 8886161

Browse files
committed
fix(payment): bound settle timestamp on the underflow side too
The previous commit guarded only the high side of the settle-timestamp arithmetic. Int.Int64() panics on either bound, and UpdateStreamRecord has a route to a hugely negative value: when payDuration <= ForcedSettleTime the code calls ForceSettle and then falls through to compute the settle timestamp from the same (stale, negative) payDuration. A deeply indebted account whose netflow rate collapses while its balance is negative makes that value underflow int64, and the else-branch Int64() panics. The route is reachable only when forced (a non-forced update returns early once payDuration drops below ForcedSettleTime), i.e. inside the no-recover EndBlocker -> chain halt. Replace the high-only check with a 3-way bound: reject a user deposit on overflow (unchanged), saturate to MaxInt64 on any other overflow, and saturate to MinInt64 on underflow (never a deposit; the account is already force-settled, so "overdue" is the correct hint). TryResumeStreamRecord is unaffected: after its resume-floor check the pay duration is provably positive. Relates to MOCA-385
1 parent 07f94a8 commit 8886161

3 files changed

Lines changed: 73 additions & 19 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
6666
- (app/upgrades) [#289](https://github.com/mocachain/moca/pull/289) Pin v2 feemarket `min_gas_price` to 20 gwei (moca's intended floor) so upgraded chains match genesis.
6767
- (evm) [#290](https://github.com/mocachain/moca/pull/290) Restore `CallEVM` error-context wrap (method + contract in error message), fix copy-pasted "evil token" comment in `erc721NonTransferable.go`, update stale geth v1.15→v1.16 comments, remove unreachable `AddBalance` blocks in distribution precompile, fix grammar in precompile Run() default cases, and drop dead test-helper expressions.
6868
- (evm) [#291](https://github.com/mocachain/moca/pull/291) Reject native value sent to precompiles to prevent a balance-reconciliation mint.
69-
- (x/payment) [MOCA-385] Prevent Int64 overflow panic in `UpdateStreamRecord` and `TryResumeStreamRecord`: settle-timestamp arithmetic is now performed in arbitrary-precision `sdkmath.Int` instead of native `int64`. Only a user-initiated deposit that would push the pay duration past `MaxInt64` is rejected (`ErrSettleTimestampOverflow`) — the depositor can simply deposit less. Every other path (rate decreases from object deletion/discontinue, lazy re-pricing when the SP price falls, and all forced EndBlocker updates) saturates the settle timestamp to `MaxInt64` instead, since it is only a scheduling hint that is recomputed on the next balance change or bucket touch. This keeps a legitimate object deletion from ever being rejected and prevents the EndBlocker from panicking (which would halt the chain).
69+
- (x/payment) [MOCA-385] Prevent Int64 overflow panic in `UpdateStreamRecord` and `TryResumeStreamRecord`: settle-timestamp arithmetic is now performed in arbitrary-precision `sdkmath.Int` instead of native `int64`, and the result is bounded to the int64 range in both directions before being stored. Only a user-initiated deposit that would push the pay duration past `MaxInt64` is rejected (`ErrSettleTimestampOverflow`) — the depositor can simply deposit less. Every other out-of-range result saturates (to `MaxInt64` on overflow — rate decreases from object deletion/discontinue, lazy re-pricing when the SP price falls, and forced EndBlocker updates; to `MinInt64` on the underflow that a deeply-indebted account can hit in a forced update), since the settle timestamp is only a scheduling hint that is recomputed on the next balance change or bucket touch. This keeps a legitimate object deletion from ever being rejected and prevents the EndBlocker from panicking (which would halt the chain).
7070
- (x/challenge) [#286](https://github.com/mocachain/moca/pull/286) Retire a challenge from the active set once it is attested, making attestation idempotent so duplicate submissions (e.g. redundant relayers or resubmissions by the in-turn submitter) are rejected instead of re-running heartbeat rewards and re-emitting attestation events.
7171
- (ci) [#65](https://github.com/mocachain/moca/pull/65) Resolve goreleaser CI failures for arm64 docker builds
7272
- (audit) [#63](https://github.com/mocachain/moca/pull/63) Apply audit fixes

x/payment/keeper/stream_record.go

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -215,38 +215,49 @@ func (k Keeper) UpdateStreamRecord(ctx sdk.Context, streamRecord *types.StreamRe
215215
return fmt.Errorf("check and force settle failed, err: %w", err)
216216
}
217217
}
218+
// The settle timestamp is stored as int64; the full expression
219+
// (currentTimestamp - forcedSettleTime + payDuration) can land outside the
220+
// int64 range in either direction. Calling Int64() on an out-of-range value
221+
// panics ("Int64() out of bound"); inside the no-recover EndBlocker that
222+
// halts the chain. Saturate instead — the settle timestamp is only a
223+
// scheduling hint for the auto-settle queue and is recomputed on the next
224+
// balance change or bucket touch, so capping it is loss-free.
218225
settleTimestampFull := sdkmath.NewInt(currentTimestamp).
219226
Sub(sdkmath.NewIntFromUint64(params.ForcedSettleTime)).
220227
Add(payDuration)
221-
if settleTimestampFull.GT(sdkmath.NewInt(math.MaxInt64)) {
222-
// The settle timestamp is stored as int64; a pay duration beyond
223-
// MaxInt64 seconds (~2.9e11 years) cannot be represented.
228+
switch {
229+
case settleTimestampFull.GT(sdkmath.NewInt(math.MaxInt64)):
230+
// Over-funded: pay duration beyond MaxInt64 seconds (~2.9e11 years),
231+
// from a huge balance or a collapsed netflow rate.
224232
//
225233
// Reject only a user-initiated deposit — a positive static-balance
226234
// change with no rate change. That depositor is over-funding past what
227-
// we can represent and can simply deposit less. Every other path must
228-
// NOT error here:
229-
// - forced EndBlocker updates: returning an error would panic the
230-
// chain in abci.go ("should not happen");
231-
// - rate decreases from object deletion/discontinue, and lazy
232-
// re-pricing when the SP price falls: rejecting a legitimate
233-
// delete/re-price would be wrong.
234-
// Those saturate the settle timestamp to MaxInt64. It is only a
235-
// scheduling hint for the auto-settle queue and is recomputed on the
236-
// next balance change or bucket touch, so capping it is loss-free.
235+
// we can represent and can simply deposit less. Forced EndBlocker
236+
// updates (an error would panic the chain in abci.go "should not
237+
// happen") and legitimate rate decreases (object deletion/discontinue,
238+
// lazy SP re-pricing) must not error, so they saturate to MaxInt64.
237239
isUserDeposit := !forced && change.StaticBalanceChange.IsPositive() && change.RateChange.IsZero()
238240
if isUserDeposit {
239241
return types.ErrSettleTimestampOverflow.Wrapf(
240242
"account %s pay duration %s exceeds int64 range; reduce deposit",
241243
streamRecord.Account, payDuration.String())
242244
}
243245
ctx.Logger().Error("settle timestamp overflow, capping at MaxInt64",
244-
"account", streamRecord.Account,
245-
"payDuration", payDuration.String(),
246-
"forced", forced,
247-
"height", ctx.BlockHeight())
246+
"account", streamRecord.Account, "payDuration", payDuration.String(),
247+
"forced", forced, "height", ctx.BlockHeight())
248248
settleTimestamp = math.MaxInt64
249-
} else {
249+
case settleTimestampFull.LT(sdkmath.NewInt(math.MinInt64)):
250+
// Deeply indebted: a large netflow rate collapsed to a tiny one while
251+
// the balance was negative makes payDuration hugely negative. Only
252+
// reachable in a forced update (a non-forced one returns above once
253+
// payDuration is below ForcedSettleTime). Never a deposit, so always
254+
// saturate — to MinInt64, so the auto-settle queue treats it as overdue
255+
// (the account is already force-settled above).
256+
ctx.Logger().Error("settle timestamp underflow, capping at MinInt64",
257+
"account", streamRecord.Account, "payDuration", payDuration.String(),
258+
"forced", forced, "height", ctx.BlockHeight())
259+
settleTimestamp = math.MinInt64
260+
default:
250261
settleTimestamp = settleTimestampFull.Int64()
251262
}
252263
}

x/payment/keeper/stream_record_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -865,6 +865,49 @@ func TestUpdateStreamRecord_SettleTimestampOverflow_ForcedSaturates(t *testing.T
865865
"forced overflow must saturate to MaxInt64, not panic")
866866
}
867867

868+
// TestUpdateStreamRecord_SettleTimestampUnderflow_ForcedSaturates covers the low
869+
// side: a forced update on a deeply indebted active account (a large rate that
870+
// collapsed to a tiny one while the balance was negative) makes payDuration
871+
// hugely negative, so currentTimestamp - forcedSettleTime + payDuration underflows
872+
// int64. Int64() would panic inside the EndBlocker and halt the chain; instead the
873+
// settle timestamp must saturate to MinInt64. This is reachable only when forced —
874+
// a non-forced update returns early once payDuration drops below ForcedSettleTime.
875+
func TestUpdateStreamRecord_SettleTimestampUnderflow_ForcedSaturates(t *testing.T) {
876+
k, ctx, dep := makePaymentKeeper(t)
877+
ctx = ctx.WithBlockTime(time.Unix(1_000_000_000, 0))
878+
ctx = ctx.WithValue(types.ForceUpdateStreamRecordKey, true)
879+
880+
// No bank account, so the negative-balance auto-transfer (for the user and for
881+
// the governance account inside ForceSettle) is skipped.
882+
dep.AccountKeeper.EXPECT().HasAccount(gomock.Any(), gomock.Any()).Return(false).AnyTimes()
883+
884+
params := k.GetParams(ctx)
885+
// Buffer matches |rate|=1 so the buffer recompute does not adjust staticBalance.
886+
bufferBalance := sdkmath.NewIntFromUint64(params.VersionedParams.ReserveTime)
887+
// staticBalance far below MinInt64 → payDuration (÷1) underflows int64.
888+
staticBalance := sdkmath.NewInt(math.MinInt64).MulRaw(2)
889+
890+
user := sample.RandAccAddress()
891+
sr := &types.StreamRecord{
892+
Account: user.String(),
893+
Status: types.STREAM_ACCOUNT_STATUS_ACTIVE,
894+
StaticBalance: staticBalance,
895+
BufferBalance: bufferBalance,
896+
LockBalance: sdkmath.ZeroInt(),
897+
NetflowRate: sdkmath.NewInt(-1),
898+
FrozenNetflowRate: sdkmath.ZeroInt(),
899+
CrudTimestamp: ctx.BlockTime().Unix(),
900+
}
901+
902+
change := types.NewDefaultStreamRecordChangeWithAddr(user)
903+
require.NotPanics(t, func() {
904+
err := k.UpdateStreamRecord(ctx, sr, change)
905+
require.NoError(t, err, "forced path must not return an error (would panic EndBlocker)")
906+
})
907+
require.Equal(t, int64(math.MinInt64), sr.SettleTimestamp,
908+
"forced underflow must saturate to MinInt64, not panic")
909+
}
910+
868911
// TestUpdateStreamRecord_SettleTimestampOverflow_UserDepositRejected covers the
869912
// user-initiated deposit path: a positive static-balance change with no rate
870913
// change that pushes payDuration past MaxInt64 is rejected with

0 commit comments

Comments
 (0)