[PT1-389] Feature/base fee adjuster normolization

[ifelsedeveloper]

Change Summary

What does this PR change?
Fixes a decimal scaling bug in BaseFeeAdjuster._baseFeeAdjuster1D where extraCostInToken1 was computed in 1e18 scale regardless of token1's actual decimals. For non-18-decimal tokens (e.g., USDC with 6 decimals), the gas compensation ratio was inflated by 10^(18 - token1Decimals), causing priceIncrease to saturate to maxIncrease on every swap even with a 1 gwei delta above base — resulting in maker overpaying up to the configured cap on every fill.

Related Issue/Ticket:
PT1-389 — BaseFeeAdjuster decimal scaling error (High severity)

Root Cause

// BEFORE (bug): result always in 1e18 scale, not token1 native units
uint256 extraCostInToken1 = (extraGasCost * ethToToken1Price) / 1e18;

// For USDC (6 dec): 1.5e15 * 3000e18 / 1e18 = 4.5e18 (should be 4.5e6) — 1e12x too large

Fix

Added uint8 token1Decimals parameter (1 byte) to BaseFeeAdjusterArgsBuilder. Normalization divides by 10^(36 - token1Decimals) instead of 1e18:

// AFTER: normalizes to token1's native precision
uint256 extraCostInToken1 = (extraGasCost * ethToToken1Price) / (10 ** (36 - token1Decimals));

// For USDC (6 dec): 1.5e15 * 3000e18 / 1e30 = 4.5e6 ✓
// For DAI (18 dec): 1.5e15 * 3000e18 / 1e18 = 4.5e18 ✓ (backward compatible)

Files Changed

File	Change
src/instructions/BaseFeeAdjuster.sol	Added token1Decimals to args builder/parser, normalized formula
test/BaseFeeAdjuster.t.sol	Updated all call sites to pass 18, added 6-decimal regression test
test/invariants/BaseFeeAdjusterInvariants.t.sol	Updated call sites to pass 18
test/invariants/BaseFeeAdjusterFeesInvariants.t.sol	Updated call sites to pass 18
.gas-snapshot	Refreshed

Testing & Verification

How was this tested?

• Unit tests
• Integration tests
• Manual testing (describe steps)
• Verified on staging

Test results: 629/629 passed, 0 failed, 0 skipped.

New test — test_BaseFeeAdjusterNormalizationWith6Decimals:

• Creates USDC (6 dec) / WETH (18 dec) pair with token1Decimals = 6
• At base gas (20 gwei): output = exactly 1 WETH (no adjustment)
• At +10 gwei (30 gwei): compensation ≈ 0.15%, does NOT saturate to 1% cap
• At +80 gwei (100 gwei): compensation = 1.2%, correctly capped to 1%

Before the fix, even +1 gwei above base saturated to the 1% cap for 6-decimal tokens.

Risk Assessment

Risk Level:

• Low - Minor changes, no operational impact
• Medium - Moderate changes, limited impact, standard rollback available
• High - Significant changes, potential operational impact, complex rollback

Risks & Impact

• Breaking change: BaseFeeAdjusterArgsBuilder.build() now requires a 5th argument (token1Decimals). Existing bytecode programs using BaseFeeAdjuster will fail to parse (args length 31 → 32 bytes). All orders must be re-created with the new encoding.
• Backward compatible for 18-decimal tokens: passing token1Decimals = 18 produces identical behavior to the old formula (10^(36-18) = 10^18).
• No storage migration needed: BaseFeeAdjuster is stateless (no storage writes).
• Rollback: revert the commit and re-deploy. No persistent state to clean up.

[claude]

Claude finished @ifelsedeveloper's task —— View job

---

Code Review for PR #83: BaseFeeAdjuster Decimal Normalization Fix

I've completed my review of this PR that fixes a critical decimal scaling bug in BaseFeeAdjuster._baseFeeAdjuster1D.

Review Checklist:

• Review implementation changes in BaseFeeAdjuster.sol
• Review test coverage and regression test
• Check for potential bugs or edge cases
• Assess performance considerations
• Evaluate security concerns
• Verify backward compatibility

Summary

Overall Assessment: ✅ The fix correctly addresses the decimal scaling bug with a well-designed solution.

Code Quality & Best Practices

Strengths:

• Clear bug fix: The normalization formula 10 ** (36 - token1Decimals) elegantly handles different token decimals
• Comprehensive documentation: Excellent comments explaining the bug and fix with concrete examples
• Consistent code style: Changes follow the existing codebase patterns
• Proper error handling: New custom error BaseFeeAdjusterMissingToken1DecimalsArg added

Implementation Review (src/instructions/BaseFeeAdjuster.sol):

The fix is mathematically sound:

// AFTER: normalizes to token1's native precision
uint256 extraCostInToken1 = (extraGasCost * ethToToken1Price) / (10 ** (36 - token1Decimals));

• For USDC (6 decimals): extraGasCost * ethToToken1Price / 1e30 = correct 6-decimal result
• For DAI (18 decimals): extraGasCost * ethToToken1Price / 1e18 = maintains current behavior

Test Coverage

Excellent Test Coverage:

• ✅ Regression test: test_BaseFeeAdjusterNormalizationWith6Decimals specifically tests the bug fix
• ✅ Edge cases: Tests at base gas price, moderate increases, and maximum cap
• ✅ Backward compatibility: All existing tests updated with token1Decimals = 18
• ✅ Comprehensive scenarios: Tests with LimitSwap, DutchAuction, exactIn/exactOut modes

Test Quality:

The new test properly validates:

1. No adjustment at base gas price (20 gwei)
2. Proportional adjustment at +10 gwei (~0.15% compensation, not hitting cap)
3. Correct capping at +80 gwei (1.2% → capped to 1%)

Potential Issues

1. Breaking Change (As documented):

• Args size changes from 31 to 32 bytes
• All existing orders using BaseFeeAdjuster must be recreated
• Mitigation: Clearly documented in PR description

2. Edge Case - Zero Decimals:

• 10 ** (36 - 0) = 10^36 could theoretically overflow in extreme cases
• Risk: Low - no standard tokens have 0 decimals
• Recommendation: Consider adding a require statement if concerned

3. Parameter Validation:

• No validation that token1Decimals <= 36
• Risk: Low - standard tokens have 0-18 decimals
• Current behavior: Would underflow and revert if decimals > 36

Performance Considerations

✅ Minimal Impact:

• One additional parameter read (1 byte)
• One exponentiation operation (gas-efficient for small exponents)
• Gas snapshot shows negligible increase

Security Assessment

✅ No Security Concerns:

• No external calls or reentrancy risks
• Pure mathematical adjustments
• Proper bounds checking with maxPriceDecay
• No overflow risks in realistic scenarios

Backward Compatibility

✅ Verified:

• Setting token1Decimals = 18 produces identical behavior to the previous implementation
• All existing tests pass with this parameter
• Clear migration path for existing deployments

Recommendations

1. Consider adding validation (optional):

   require(token1Decimals <= 36, "Invalid decimals");

2. Documentation: The inline comments and examples are excellent - no changes needed

3. Migration guide: Consider documenting the upgrade process for existing orders in the repository

Conclusion

This is a high-quality fix for a critical bug. The implementation is clean, well-tested, and properly documented. The breaking change is unavoidable but handled transparently. I recommend approval after considering the optional validation suggestion.

[alexbabits]

Fix is correct, test_BaseFeeAdjusterNormalizationWith6Decimals test passes showing that the issue is fixed. Also wrote a test for 20 decimal token (over 18) and it passes.

However, one thing you may want to consider, is if we trust Maker with token1Decimals input during his order build?

E.g. He lies about this and inputs 6 or 8 or 24 when the actual token 1 decimal is 18.

As far as I can see, during a Taker order that fills this malicious Maker order, the Taker has a thresholdAmount check in TakerTraits.validate(), so that protects from a malicious maker.

If Taker doesn't set thresholdAmount with unlimited slippage, sure he gets screwed now, but that's his own footgun. I think the Taker will always set a reasonable threshold/slippage?

Also Maker's program bytecode is public and it would be possible to see the malicious token1Decimals amount against the real token decimals.

If you think that's worth changing, the fix would be to force IERC20.decimals() call and not trust maker for token1Decimals amount, but I don't think this is needed.