fix(rewards): ValidateGenesis checks denom consistency for inactive schedules - #356
fix(rewards): ValidateGenesis checks denom consistency for inactive schedules#356amathxbt wants to merge 1 commit into
Conversation
…chedules Previously, denom and amount comparisons between TotalAmount and ReleasedAmount were skipped entirely for inactive schedules. An inactive schedule stored with mismatched denoms would corrupt state the moment it was activated via governance. Fix: validate denom consistency and the released <= total invariant regardless of the active flag when both fields are non-zero.
WalkthroughThe change modifies Estimated code review effort: 2 (Simple) | ~10 minutes Changes
Sequence Diagram(s)Not applicable — the change is a validation logic reordering within a single function. Related issues: No related issues linked. Related PRs: No related PRs identified. Suggested labels: bug, rewards, validation Suggested reviewers: Not enough information to determine appropriate reviewers. Poem: 🐰 A rabbit checked the schedule twice, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@x/rewards/types/release_schedule.go`:
- Line 26: The denom-mismatch validation in the release schedule check is being
skipped whenever TotalAmount is zero, so a non-zero ReleasedAmount can still
bypass the inactive-schedule validation. Update the conditional in the release
schedule logic to validate denom whenever ReleasedAmount is non-zero, regardless
of TotalAmount, and keep the check localized to the existing release schedule
validation path in the relevant release_schedule helper.
- Around line 23-36: The validation in the release schedule checks currently
calls IsZero on TotalAmount and ReleasedAmount without first guarding against
nil coins, which can panic in this SDK version. Update the validation in the
release schedule type to check rr.TotalAmount.IsNil() and
rr.ReleasedAmount.IsNil() before any IsZero or denom/amount comparisons, and
skip or handle uninitialized coins accordingly in this block so the invalid
total amount and active with zero end time paths remain safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82a5d07f-bb1a-40c0-9a89-913eb1681646
📒 Files selected for processing (1)
x/rewards/types/release_schedule.go
| // Validate denom consistency between TotalAmount and ReleasedAmount regardless | ||
| // of active state. An inactive schedule with mismatched denoms can corrupt | ||
| // state the moment it is activated. | ||
| if !rr.TotalAmount.IsZero() && !rr.ReleasedAmount.IsZero() { | ||
| if rr.ReleasedAmount.Denom != rr.TotalAmount.Denom { | ||
| return fmt.Errorf("released amount denom %s doesn't match total amount denom %s", | ||
| rr.ReleasedAmount.Denom, rr.TotalAmount.Denom) | ||
| } | ||
| if rr.ReleasedAmount.Amount.GT(rr.TotalAmount.Amount) { | ||
| return fmt.Errorf("released amount %s cannot be greater than total amount %s", | ||
| rr.ReleasedAmount.String(), rr.TotalAmount.String()) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod excerpt =="
sed -n '1,220p' go.mod | sed -n '/github.com\/cosmos\/cosmos-sdk/p;/github.com\/cosmos\/cosmossdk.io/p'
echo
echo "== release_schedule.go =="
cat -n x/rewards/types/release_schedule.go | sed -n '1,220p'
echo
echo "== release_schedule_test.go mentions =="
rg -n 'invalid total amount|active with zero end time|ReleasedAmount: sdk\.Coin\{\}|ValidateGenesis|IsZero\(' x/rewards/types/release_schedule_test.go x/rewards/types -n -A3 -B3
echo
echo "== module cache sdk coin/int source candidates =="
go env GOMODCACHE 2>/dev/null || true
fd -a 'coin.go|int.go' "$(go env GOMODCACHE 2>/dev/null)/github.com" 2>/dev/null | rg 'cosmos-sdk/.*/math/(coin\.go|int\.go)$|cosmossdk\.io/.*/math/(coin\.go|int\.go)$' || trueRepository: KiiChain/kiichain
Length of output: 12972
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Resolve the exact SDK version used by the module and inspect the source files if present.
SDK_VER=$(awk '/github.com\/cosmos\/cosmos-sdk/ {print $2; exit} /cosmossdk\.io\/core/ {print $2; exit}' go.mod || true)
echo "SDK version token: ${SDK_VER:-<none>}"
GOMODCACHE=$(go env GOMODCACHE)
echo "GOMODCACHE=$GOMODCACHE"
echo
echo "== find cosmos-sdk math sources =="
find "$GOMODCACHE" -path '*github.com/cosmos/cosmos-sdk*' -path '*/math/int.go' -o -path '*/types/coin.go' 2>/dev/null | sed -n '1,40p'
echo
echo "== inspect any matching int.go / coin.go files =="
for f in $(find "$GOMODCACHE" -path '*github.com/cosmos/cosmos-sdk*' \( -path '*/math/int.go' -o -path '*/types/coin.go' \) 2>/dev/null | sed -n '1,4p'); do
echo "--- $f ---"
cat -n "$f" | sed -n '1,220p' | rg -n 'IsZero|Sign|nil|type Int|type Coin|Amount'
doneRepository: KiiChain/kiichain
Length of output: 255
🌐 Web query:
github.com/cosmos/cosmos-sdk v0.53.6 math.Int IsZero Sign nil pointer Coin.IsZero source
💡 Result:
In Cosmos SDK v0.53.6, a math.Int with a nil internal big.Int can cause a nil pointer panic when methods such as IsZero, IsNegative, or others are called, because these methods attempt to dereference the internal value without checking if it is nil [1]. This issue arises primarily when an unmarshaling process (such as decoding malformed protobuf data) creates a math.Int where the internal big.Int is not properly initialized [1]. While standard SDK operations typically ensure that Coin amounts are initialized, malformed or crafted inputs that bypass these checks can result in a Coin struct with a nil Amount. Key technical details: 1. Panic Mechanism: Methods like IsZero and IsNegative dereference the internal big.Int [1]. If that pointer is nil, the Go runtime triggers a panic [1]. 2. Coin.IsZero: The implementation of Coin.IsZero in v0.53.6 calls Amount.IsZero directly [2]. If Amount is nil, this leads to the aforementioned panic. 3. Mitigation: The Cosmos SDK has introduced several guards over time, such as adding Coin.IsNil checks [2][3] and implementing validation logic in Coin.Validate to ensure the Amount is not nil before it is processed [3][4]. 4. Recent Fixes: Ongoing efforts, such as PR #26536, aim to prevent nil internal pointers during unmarshaling by ensuring empty inputs are initialized to a zero-valued big.Int rather than a nil pointer [1]. Developers should use the IsNil method provided on the Coin struct to safely verify if an amount is initialized before performing operations [2][3].
Citations:
- 1: fix(math): avoid nil internal big.Int when Unmarshal receives empty i… cosmos/cosmos-sdk#26536
- 2: https://github.com/cosmos/cosmos-sdk/blob/v0.53.6/types/coin.go
- 3: https://github.com/cosmos/cosmos-sdk/blob/main/types/coin.go
- 4: fix: types: ensure .Amount is non-nil in Coin.Validate() cosmos/cosmos-sdk#15691
🌐 Web query:
github.com/cosmos/cosmos-sdk v0.53.6 sdk.Coin IsNil method source
💡 Result:
In the Cosmos SDK version v0.53.6, the IsNil method for the sdk.Coin type is defined in the types/coin.go file [1]. The method implementation is as follows: func (coin Coin) IsNil bool { return coin.Amount.BigInt == nil } This method returns true if the coin amount is nil, and false otherwise [1][2]. It functions by checking if the underlying BigInt representation of the Coin's amount is nil [1].
Citations:
- 1: https://github.com/cosmos/cosmos-sdk/blob/v0.53.6/types/coin.go
- 2: https://pkg.go.dev/github.com/cosmos/cosmos-sdk/types
Guard IsZero() behind IsNil() checks — sdk.Coin{} can make IsZero() panic in v0.53.6, so this block needs to skip uninitialized coins first. Use rr.TotalAmount.IsNil() / rr.ReleasedAmount.IsNil() here; the invalid total amount and active with zero end time cases hit this path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@x/rewards/types/release_schedule.go` around lines 23 - 36, The validation in
the release schedule checks currently calls IsZero on TotalAmount and
ReleasedAmount without first guarding against nil coins, which can panic in this
SDK version. Update the validation in the release schedule type to check
rr.TotalAmount.IsNil() and rr.ReleasedAmount.IsNil() before any IsZero or
denom/amount comparisons, and skip or handle uninitialized coins accordingly in
this block so the invalid total amount and active with zero end time paths
remain safe.
| // Validate denom consistency between TotalAmount and ReleasedAmount regardless | ||
| // of active state. An inactive schedule with mismatched denoms can corrupt | ||
| // state the moment it is activated. | ||
| if !rr.TotalAmount.IsZero() && !rr.ReleasedAmount.IsZero() { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Denom-mismatch check is skipped entirely when TotalAmount is zero.
The gate !rr.TotalAmount.IsZero() && !rr.ReleasedAmount.IsZero() means a schedule with a zero TotalAmount but a non-zero ReleasedAmount (mismatched denom or exceeding total) bypasses this validation entirely, since the && requires both sides non-zero. Given the PR's stated goal — catching mismatched denoms on inactive schedules before governance activation — this is a residual gap: an inactive schedule could have TotalAmount = 0uatom and ReleasedAmount = 100akii, which would still pass genesis validation.
Consider validating denom whenever ReleasedAmount is non-zero (regardless of TotalAmount's zero-ness), since a non-zero released amount with no matching total is inherently invalid.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@x/rewards/types/release_schedule.go` at line 26, The denom-mismatch
validation in the release schedule check is being skipped whenever TotalAmount
is zero, so a non-zero ReleasedAmount can still bypass the inactive-schedule
validation. Update the conditional in the release schedule logic to validate
denom whenever ReleasedAmount is non-zero, regardless of TotalAmount, and keep
the check localized to the existing release schedule validation path in the
relevant release_schedule helper.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Summary
Bug:
ReleaseSchedule.ValidateGenesis()only validated denom consistency when the schedule was active. An inactive schedule with mismatched denoms (e.g.TotalAmount.Denom = "akii",ReleasedAmount.Denom = "uatom") would pass validation but corrupt state the moment governance activated it.Fix: Validate denom consistency and the
ReleasedAmount <= TotalAmountinvariant regardless of theActiveflag whenever both fields are non-zero.Files changed
x/rewards/types/release_schedule.go