fix(store): Add safe multiplication to prevent integer overflow in gas calculations #25195
Conversation
📝 WalkthroughWalkthroughIntroduces overflow-safe gas calculations via a new SafeMul helper and replaces direct multiplications in Get, Set, and iterator gas consumption. On overflow or invalid length, it consumes max gas for the specific descriptor. Adds tests for SafeMul edge cases and integration with gaskv.Store. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Store
participant SafeMul
participant GasMeter
Client->>Store: Get/Set(key, value)
Store->>SafeMul: costPerKeyByte × len(key)
alt overflow or invalid
SafeMul-->>Store: error
Store->>GasMeter: Consume(maxGasForKeyBytes)
else ok
SafeMul-->>Store: gasForKeyBytes
Store->>GasMeter: Consume(gasForKeyBytes)
end
Store->>SafeMul: costPerValueByte × len(value)
alt overflow or invalid
SafeMul-->>Store: error
Store->>GasMeter: Consume(maxGasForValueBytes)
else ok
SafeMul-->>Store: gasForValueBytes
Store->>GasMeter: Consume(gasForValueBytes)
end
Store-->>Client: result
sequenceDiagram
participant Iterator
participant SafeMul
participant GasMeter
Iterator->>SafeMul: costPerKeyByte × len(key)
alt overflow/invalid
SafeMul-->>Iterator: error
Iterator->>GasMeter: Consume(maxGasForKeyBytes)
else
SafeMul-->>Iterator: gasForKeyBytes
Iterator->>GasMeter: Consume(gasForKeyBytes)
end
Iterator->>SafeMul: costPerValueByte × len(value)
alt overflow/invalid
SafeMul-->>Iterator: error
Iterator->>GasMeter: Consume(maxGasForValueBytes)
else
SafeMul-->>Iterator: gasForValueBytes
Iterator->>GasMeter: Consume(gasForValueBytes)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (6)
store/gaskv/store.go (3)
200-207: Iterator: charging per-key bytes deviates from documented behavior and likely alters gas accountingThe method comment (Lines 193–195) says the variable gas is based on the current value's length. This block now also charges for key length and uses GasValuePerByteDesc for it. This is a behavior change that can break existing gas invariants/tests and mixes descriptors.
- If the intent is to keep existing semantics, remove the key charge.
- If you really want to start charging per-key bytes, update the docs, consider a distinct descriptor, and revisit all tests relying on exact gas tallies.
Suggested revert (remove key-length charge):
- // Safe gas calculation for key length - if gasCost, err := SafeMul(gi.gasConfig.ReadCostPerByte, len(key)); err == nil { - gi.gasMeter.ConsumeGas(gasCost, types.GasValuePerByteDesc) - } else { - // If overflow occurs, consume maximum gas as a safety measure - gi.gasMeter.ConsumeGas(types.Gas(^uint64(0)), types.GasValuePerByteDesc) - }
216-217: Nit: consider charging the flat seek cost before the per-byte cost (consistency)Minor, but historically many flows charge the flat cost first, then per-byte. Not functionally significant.
219-234: SafeMul is correct; consider minor API and readability refinements
- Using int for length aligns with len(), but exposes 32-bit arch pitfalls in callers/tests. Consider using uint64 for length and casting once at call sites.
- Prefer math.MaxUint64 over ^uint64(0) for readability (or hoist a package-level const var maxGas = types.Gas(math.MaxUint64)).
- Early-return for length == 0 (in addition to cost == 0) improves readability.
Alternative signature and implementation for consideration:
// SafeMul performs overflow-safe multiplication of gas cost by length. // Note: len(...) is always non-negative, so a uint64 length avoids negative checks. func SafeMul(cost types.Gas, length uint64) (types.Gas, error) { if cost == 0 || length == 0 { return 0, nil } max := types.Gas(^uint64(0)) // Overflow if cost * length > max if cost > max/types.Gas(length) { return 0, fmt.Errorf("gas calculation overflow: cost=%d, length=%d", cost, length) } return cost * types.Gas(length), nil }store/gaskv/store_test.go (3)
20-21: Fix comment capitalization to match exported function nameUse SafeMul (not safeMul) in the test description.
-// TestSafeMul tests the safeMul function for various scenarios +// TestSafeMul tests the SafeMul function for various scenarios
156-160: Strengthen the integration assertionAsserting gasConsumed < MaxUint64 is tautological. Assert it does not exceed the meter limit.
- require.Greater(t, gasConsumed, types.Gas(0)) - require.Less(t, gasConsumed, types.Gas(^uint64(0))) // Should not be max uint64 + require.Greater(t, gasConsumed, types.Gas(0)) + require.LessOrEqual(t, gasConsumed, meter.Limit())
131-160: Consider adding an overflow-path integration test to prove the OOG fallbackRight now we don't cover the overflow path that leads to OOG consumption. You could craft a custom gasConfig with extremely large per-byte costs to force SafeMul overflow and assert that Set/Get/iterator operations panic with out-of-gas due to the precise OOG consumption.
I can draft a minimal test that overrides gasConfig.ReadCostPerByte/WriteCostPerByte to near-MaxUint64 and verifies panic on Set/Get for modest key/value sizes. Want me to push that snippet?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
store/gaskv/store.go(4 hunks)store/gaskv/store_test.go(1 hunks)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
store/gaskv/store.go (4)
208-214: Iterator (value length): use dedicated OOG helper on overflowTo ensure consistent out-of-gas errors instead of saturating at
MaxUint64, replace the raw max-gas consumption with a call toconsumeOutOfGas. If this helper isn’t defined yet, please add it (for example, wrappingMeter.ConsumeGas(meter.Limit(), desc)or invoking a proper OOG signal) so that all overflow branches trigger a proper OOG error.– In
store/gaskv/store.goaround line 212:- // If overflow occurs, consume maximum gas as a safety measure - gi.gasMeter.ConsumeGas(types.Gas(^uint64(0)), types.GasValuePerByteDesc) + // On overflow, trigger out-of-gas via helper for precise OOG semantics + consumeOutOfGas(gi.gasMeter, types.GasValuePerByteDesc)
66-73: Incorrect suggestion: helperconsumeOutOfGasdoesn’t exist
The proposed replacement callsconsumeOutOfGas(...), but no such function is defined in the codebase. To trigger a precise out-of-gas instead of using^uint64(0), you should either:• Inline the logic:
remaining := gs.gasMeter.GasRemaining() gs.gasMeter.ConsumeGas(remaining+1, types.GasWritePerByteDesc)• Or define the helper before using it:
func consumeOutOfGas(meter types.GasMeter, desc string) { remaining := meter.GasRemaining() meter.ConsumeGas(remaining+1, desc) }And then replace the overflow branch with:
- gs.gasMeter.ConsumeGas(types.Gas(^uint64(0)), types.GasWritePerByteDesc) + consumeOutOfGas(gs.gasMeter, types.GasWritePerByteDesc)Likely an incorrect or invalid review comment.
74-80: NoconsumeOutOfGashelper exists—suggestion invalidI couldn’t find any
consumeOutOfGasfunction in the repo. You have two options:
- Keep the current explicit overflow branch call:
gs.gasMeter.ConsumeGas(types.Gas(^uint64(0)), types.GasWritePerByteDesc)- Or first extract a helper, for example:
and then call that in each overflow branch.func consumeOutOfGas(m storetypes.GasMeter, desc string) { m.ConsumeGas(types.Gas(^uint64(0)), desc) }[suggest_essential_refactor removed]
Likely an incorrect or invalid review comment.
49-55: Undefined helperconsumeOutOfGasreferenced
The suggested refactor calls a helper that doesn’t exist in the codebase. The existing pattern—explicitly consumingtypes.Gas(^uint64(0))on overflow—is consistently applied across all gas calculations and has corresponding tests. You can either ignore this refactor or introduce and implement a newconsumeOutOfGashelper before applying it.Affected locations for overflow handling (no helper currently defined):
- store/gaskv/store.go lines 45–47 and 53–55 (Get key/value)
- store/gaskv/store.go lines 69–71 and 77–79 (Write key/value)
- store/gaskv/store.go lines 203–206 and 209–214 (Iterator)
Likely an incorrect or invalid review comment.
|
lint failing and needing a changelog |
2 participants
Description
This PR fixes a critical integer overflow vulnerability in the gas store (store/gaskv/store.go) by implementing safe multiplication for gas calculations. The fix prevents potential gas manipulation attacks and ensures the system remains secure when handling large data operations.
Problem
The original code had unsafe integer multiplication in gas calculations:
This could lead to:
Summary by CodeRabbit
Bug Fixes
Tests