Test twap underflow possibility by Thaleszh · Pull Request #169 · KiiChain/kiichain

Thaleszh · 2025-11-12T21:40:41Z

Description

Adds audit test for twap underflow and adds missing block time setup.

Type of change

test (For updates on tests)

coderabbitai · 2025-11-12T21:41:28Z

Walkthrough

A new test function TestVulnerabilityTWAPNumericUnderflow was added to x/oracle/keeper/keeper_test.go. The test creates out-of-order price snapshots with decreasing timestamps to simulate a negative-duration scenario in TWAP calculation, calls CalculateTwaps to observe underflow or incorrect results and error handling, and includes a parallel subtest using correctly ordered snapshots to validate normal behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Single test function added to an existing test file; no production code changes.
Homogeneous change with limited scope.

Focus areas for review:

Verify the test constructs the out-of-order timestamps and snapshots exactly as intended to reproduce WB-OR-001.
Confirm assertions check for underflow/incorrect TWAP results and appropriate error handling.
Review the normal-operation subtest for correct timestamp ordering and valid comparisons.
Check logging and test clarity for reproducibility and diagnostics.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Test twap underflow possibility' directly describes the main change - adding a test for TWAP underflow vulnerability, which matches the core objective of the pull request.
Description check	✅ Passed	The description is related to the changeset, explicitly mentioning the addition of an audit test for TWAP underflow and block time setup, which aligns with the added test function.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch test/twap-numeric-underflow

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

x/oracle/keeper/keeper_test.go (1)
724-724: Remove redundant block time setting.

The block time was already set to time.Unix(200, 0) at line 615, and the same context is used throughout the test. This line is redundant.

Apply this diff:
-		ctx = ctx.WithBlockTime(time.Unix(200, 0))
 		twaps, err := oracleKeeper.CalculateTwaps(ctx, 200)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b3d7fdf and 6d843b0.

📒 Files selected for processing (1)

x/oracle/keeper/keeper_test.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

x/oracle/keeper/keeper_test.go (5)

x/oracle/keeper/test_utils.go (1)

CreateTestInput (130-294)

x/feeabstraction/types/expected_keepers.go (1)

OracleKeeper (39-43)

x/oracle/utils/assets.go (1)

KiiDenom (5-5)

x/oracle/types/params.pb.go (9)

Denom (113-116)

Denom (119-119)

Denom (120-122)

OracleExchangeRate (229-233)

OracleExchangeRate (236-236)

OracleExchangeRate (237-239)

PriceSnapshot (322-325)

PriceSnapshot (329-329)

PriceSnapshot (330-332)

x/oracle/types/snapshots.go (3)

NewPriceSnapshotItem (22-27)

NewPriceSnapshot (14-19)

PriceSnapshotItems (4-4)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

GitHub Check: tests
GitHub Check: golangci-lint
GitHub Check: Analyze
GitHub Check: test-e2e

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (1)

x/oracle/keeper/keeper_test.go (1)
654-658: Fix contradictory error handling logic.

The code uses t.Errorf to fail the test when an error occurs, but the comments suggest this is meant to "demonstrate the vulnerability." This creates contradictory behavior:

If the vulnerability exists and causes an error, the test fails (line 657)

If the vulnerability is fixed with validation, the test also fails (line 657)

The test can only pass if the function succeeds despite invalid input

Additionally, this issue was already flagged in the previous review comment (lines 623-664) but remains unaddressed. The test still lacks assertions to validate the expected behavior.

Apply this diff to remove the unconditional failure:
 		if err != nil {
 			t.Logf("  RESULT: CalculateTwaps returned error: %v", err)
 			t.Logf("  This demonstrates the function can fail due to timing issues")
-			t.Errorf("Failed with: %s", err)
 		} else {
Then follow one of the approaches suggested in the previous review to add proper assertions (assert error for fixed vulnerability, assert success for unfixed, or skip test).

🧹 Nitpick comments (1)

x/oracle/keeper/keeper_test.go (1)
692-692: Remove redundant block time setting.

The block time is already set to time.Unix(200, 0) at line 615 and doesn't change between subtests. This line is redundant.

Apply this diff to remove the redundancy:
-		ctx = ctx.WithBlockTime(time.Unix(200, 0))
 		twaps2, err := oracleKeeper.CalculateTwaps(ctx, 200)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6d843b0 and eb2e899.

📒 Files selected for processing (1)

x/oracle/keeper/keeper_test.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

x/oracle/keeper/keeper_test.go (5)

x/oracle/keeper/test_utils.go (1)

CreateTestInput (130-294)

x/feeabstraction/types/expected_keepers.go (1)

OracleKeeper (39-43)

x/oracle/utils/assets.go (1)

KiiDenom (5-5)

x/oracle/types/params.pb.go (9)

Denom (113-116)

Denom (119-119)

Denom (120-122)

OracleExchangeRate (229-233)

OracleExchangeRate (236-236)

OracleExchangeRate (237-239)

PriceSnapshot (322-325)

PriceSnapshot (329-329)

PriceSnapshot (330-332)

x/oracle/types/snapshots.go (4)

OracleTwaps (10-10)

NewPriceSnapshotItem (22-27)

NewPriceSnapshot (14-19)

PriceSnapshotItems (4-4)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: golangci-lint
GitHub Check: tests
GitHub Check: liveness-test
GitHub Check: test-e2e
GitHub Check: Analyze

🔇 Additional comments (1)

x/oracle/keeper/keeper_test.go (1)

606-622: Test setup is correct, but be aware of cross-subtest dependency.

The setup properly initializes the test environment and sets block time. The twaps variable is declared at line 622 to enable comparison between subtests, but this creates a dependency where the second subtest assumes the first succeeded.

Thales Zirbel added 2 commits November 12, 2025 18:37

test: add audit test

3c46d32

fix: set block time for twap test

6d843b0

Thaleszh requested a review from jhelison as a code owner November 12, 2025 21:40

coderabbitai Bot reviewed Nov 12, 2025

View reviewed changes

Comment thread x/oracle/keeper/keeper_test.go

fix: simplify test and add failing checks

eb2e899

coderabbitai Bot reviewed Nov 13, 2025

View reviewed changes

Comment thread x/oracle/keeper/keeper_test.go

jhelison approved these changes Dec 10, 2025

View reviewed changes

Thaleszh merged commit ff10ef2 into main Dec 10, 2025
8 of 9 checks passed

Thaleszh deleted the test/twap-numeric-underflow branch December 10, 2025 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test twap underflow possibility#169

Test twap underflow possibility#169
Thaleszh merged 3 commits into
mainfrom
test/twap-numeric-underflow

Thaleszh commented Nov 12, 2025

Uh oh!

coderabbitai Bot commented Nov 12, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Thaleszh commented Nov 12, 2025

Description

Type of change

Uh oh!

coderabbitai Bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Nov 12, 2025 •

edited

Loading