Audit Fix

[Kukoomomo]

Summary by CodeRabbit

• New Features

  • Gas estimation now incorporates FeeLimit information for improved accuracy.

• Refactor

  • Optimized balance retrieval logic and cost cap update mechanisms.
  • Streamlined token information retrieval structure.
  • Standardized variable naming in token rate calculations.

• Bug Fixes

  • Corrected potential nil-value handling in cost calculations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR refactors token-related fee logic and gas estimation across multiple components. It removes explicit contract address parameters from token information functions (now using a hardcoded global), simplifies balance slot checking via a dedicated flag, forwards FeeLimit to gas estimation, and removes a nil-check in transaction cost cap updates. Mostly parameter signature and logic streamlining changes.

Changes

Cohort / File(s)	Summary
Token balance and gas handling core/token_gas.go	Removed unused import "bytes"; replaced BalanceSlot empty-hash comparison with info.HasSlot flag check to determine balance retrieval path
Transaction cost management core/tx_list.go	Removed nil-check before updating Eth cost cap; now directly compares and updates without nil-path handling
Token information storage rollup/fees/token_info.go	Removed contractAddr parameter from GetUint256MappingValue, GetTokenPriceByIDWithState, and GetTokenInfoFromStorage; now uses hardcoded TokenRegistryAddress global variable
Fee rate calculation rollup/fees/rate.go	Updated TokenRate to call GetTokenInfoFromStorage without contractAddr; renamed tokenScale variable to scale throughout EthToAlt and AltToETH functions
Gas estimation internal/ethapi/transaction_args.go	Added FeeLimit field forwarding to gas estimation call arguments in TransactionArgs.setDefaults
Documentation updates core/types/receipt.go, rollup/fees/token_transfer.go	Comment-only updates clarifying feature introduction context and balance change operation

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Areas requiring extra attention:

• core/tx_list.go: Removal of nil-check assumes Eth() always returns non-nil; verify this invariant holds across all code paths
• rollup/fees/token_info.go: Centralization of contract address to hardcoded TokenRegistryAddress global variable—confirm no use cases require alternate registry addresses
• core/token_gas.go: Logic change from empty-hash comparison to HasSlot flag requires verification that the flag is consistently set/maintained across token metadata

Possibly related PRs

• [R-4] Standardized fix #264: Modifies identical files (core/types/receipt.go, rollup/fees/rate.go, rollup/fees/token_transfer.go) with same variable renaming (tokenScale → scale) and comment updates
• Update balance slot decode #244: Modifies core/token_gas.go with the same HasSlot flag logic for balance retrieval path selection
• Trace Storage Proof #254: Updates rollup/fees/rate.go TokenRate and scale handling with overlapping parameter/return type changes

Suggested reviewers

• Web3Jumb0

Poem

🐰 Hops through code with nimble paws,
Contract address, removed with clause,
Scale and slots now lean and clean,
GlobalRegistry reigns supreme!
Refactored paths, the code runs fleet. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	❓ Inconclusive	The title 'Audit Fix' is too vague and generic to convey meaningful information about the specific changes in the changeset, which include refactoring function signatures, updating balance checking logic, and removing unused imports.	Replace with a more descriptive title that highlights the main change, such as 'Refactor token info functions to use implicit TokenRegistryAddress' or 'Update balance checking logic and function signatures for audit fixes'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ 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 feature/audit_fix

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 589b2c2 and 435ac3d.

📒 Files selected for processing (7)

• core/token_gas.go (1 hunks)
• core/tx_list.go (1 hunks)
• core/types/receipt.go (1 hunks)
• internal/ethapi/transaction_args.go (1 hunks)
• rollup/fees/rate.go (2 hunks)
• rollup/fees/token_info.go (2 hunks)
• rollup/fees/token_transfer.go (1 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: Kukoomomo
Repo: morph-l2/go-ethereum PR: 205
File: core/token_gas.go:140-155
Timestamp: 2025-11-10T06:56:24.937Z
Learning: In the ERC20 fee implementation (core/token_gas.go), ERC20 transfer and balance check calls use math.MaxUint64 gas limit. This is intentional given that fee tokens are restricted to an allowlist (via IsTokenActive checks and AllowListSlot/AllowListEnabledSlot in rollup/rcfg/config.go), so only vetted contracts can be invoked. The team has chosen this approach understanding the tradeoffs versus adding fixed gas stipends.

📚 Learning: 2025-11-10T06:56:24.937Z

Learnt from: Kukoomomo
Repo: morph-l2/go-ethereum PR: 205
File: core/token_gas.go:140-155
Timestamp: 2025-11-10T06:56:24.937Z
Learning: In the ERC20 fee implementation (core/token_gas.go), ERC20 transfer and balance check calls use math.MaxUint64 gas limit. This is intentional given that fee tokens are restricted to an allowlist (via IsTokenActive checks and AllowListSlot/AllowListEnabledSlot in rollup/rcfg/config.go), so only vetted contracts can be invoked. The team has chosen this approach understanding the tradeoffs versus adding fixed gas stipends.

Applied to files:

• core/token_gas.go
• rollup/fees/token_transfer.go
• core/tx_list.go
• internal/ethapi/transaction_args.go
• rollup/fees/token_info.go

🧬 Code graph analysis (2)

rollup/fees/rate.go (2)

rollup/fees/token_info.go (1)

• GetTokenInfoFromStorage (171-185)

core/types/token_fee.go (2)

• EthToAlt (52-66)
• AltToEth (69-83)

rollup/fees/token_info.go (2)

rollup/fees/rollup_fee.go (1)

• StateDB (44-49)

rollup/rcfg/config.go (1)

• PriceRatioSlot (55-55)

⏰ 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: Analyze (go)

🔇 Additional comments (10)

rollup/fees/token_transfer.go (1)

62-63: LGTM!

The updated comment more accurately reflects the action being performed—modifying the user's balance rather than calculating the storage slot.

core/types/receipt.go (1)

127-128: LGTM!

Minor documentation cleanup removing the specific date reference.

internal/ethapi/transaction_args.go (1)

166-177: Good fix: FeeLimit now forwarded to gas estimation.

This ensures that FeeLimit is properly considered when estimating gas for AltFeeTx transactions. Previously, FeeTokenID was passed but FeeLimit was missing, which could lead to inaccurate gas estimates.

core/token_gas.go (1)

64-80: LGTM!

Refactoring to use info.HasSlot instead of comparing BalanceSlot to an empty hash is cleaner and more explicit. The branching logic is now consistent with GetAltTokenBalanceHybrid (line 30) and TransferAltTokenHybrid (line 50).

rollup/fees/rate.go (2)

13-27: LGTM!

The function now uses the simplified GetTokenInfoFromStorage(state, tokenID) signature, aligning with the centralized TokenRegistryAddress approach in token_info.go. The variable rename from tokenScale to scale is a cosmetic improvement.

---

30-43: LGTM!

Both EthToAlt and AltToETH correctly use the scale value from TokenRate and forward it to types.EthToAlt and types.AltToEth respectively.

rollup/fees/token_info.go (3)

164-167: LGTM!

GetTokenPriceByIDWithState now uses the simplified GetUint256MappingValue signature, consistent with the centralized contract address approach.

---

170-184: LGTM!

GetTokenInfoFromStorage signature simplified to remove the redundant contractAddr parameter. The function correctly delegates to GetTokenInfo and GetTokenPriceByIDWithState.

---

70-82: API simplification is correct. Removing the contractAddr parameter from GetUint256MappingValue and using hardcoded TokenRegistryAddress simplifies the public API. The codebase analysis confirms all callers use TokenRegistryAddress consistently, and the private getTokenInfo function retains contractAddr for internal flexibility.

core/tx_list.go (1)

359-363: No issue found. The Eth() method is guaranteed to return a non-nil value.

The SuperAccount.Eth() method in core/types/token_fee.go includes a nil-check that ensures a non-nil *big.Int is always returned—even when the internal ethAmount is nil, it returns new(big.Int). The code at lines 359–363 (and similarly at lines 388 and 441) safely calls Eth().Cmp() without additional nil-checks because the method's design provides this guarantee.

Likely an incorrect or invalid review comment.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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