fix(p0-11): inline MIN/MAX/delta guards on all three price setters#114
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64cebef572
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (oldRate != 0) { | ||
| uint256 lower = oldRate * (10000 - EXCHANGE_RATE_DELTA_BPS) / 10000; | ||
| uint256 upper = oldRate * (10000 + EXCHANGE_RATE_DELTA_BPS) / 10000; | ||
| if (_newRate < lower || _newRate > upper) revert ExchangeRateCannotBeZero(); |
There was a problem hiding this comment.
Skip delta guard when existing exchangeRate is out of band
initialize still accepts any _exchangeRate, but this new delta check always runs when oldRate != 0; if a token is deployed with a typoed rate far outside [1e14, 1e22] (for example 1e30), no future value can satisfy both the absolute bounds and the ±20% window, so updateExchangeRate becomes permanently unusable. This turns a recoverable misconfiguration into an irrecoverable one for affected communities.
Useful? React with 👍 / 👎.
| /// cachedPrice is uninitialised (first push). | ||
| function setCachedPrice(uint256 price, uint48 timestamp) external onlyOwner { | ||
| if (price == 0) revert Paymaster__InvalidOraclePrice(); | ||
| if (price < CACHED_PRICE_MIN || price > CACHED_PRICE_MAX) revert Paymaster__InvalidOraclePrice(); |
There was a problem hiding this comment.
Restrict cached-price max to the runtime accepted range
setCachedPrice now allows values up to CACHED_PRICE_MAX (1_000_000e8), but the paymaster runtime path later rejects cached ETH prices above MAX_ETH_USD_PRICE (100_000e8) in _calculateTokenCost; setting a value in that gap can therefore make validation fail, and the new ±30% delta guard can prevent stepping back below 100_000e8 in one manual update. This creates a self-inflicted DoS path from a single owner typo.
Useful? React with 👍 / 👎.
代码审查通过,含 MEDIUM 问题 三个 setter 的三层保护(zero → bounds → delta)均正确实现,检查顺序正确(先验证后写入),绝对边界和 delta 值选取合理。 MEDIUM — // xPNTsToken.sol — 硬编码
uint256 lower = oldRate * (10000 - EXCHANGE_RATE_DELTA_BPS) / 10000;
// SuperPaymaster.sol — 使用常量
uint256 lower = oldPrice * (BPS_DENOMINATOR - APNTS_PRICE_DELTA_BPS) / BPS_DENOMINATOR;两处不一致。若将来 MEDIUM — LOW — |
|
已根据 reviewer 建议修复:将 变更:
请 re-review,谢谢 🙏 |
NatSpec 补充:价格路径独立性 + 零价 delta skip 说明修复内容MEDIUM — 两条路径 delta 上限独立:在
LOW — delta skip 零价条件依赖初始化:在
测试结果:38/38 PASS |
fd874fe to
061484c
Compare
4e72ba5 to
413892b
Compare
2f40260 to
8dc6f01
Compare
413892b to
e91860f
Compare
|
All contributors have signed the CLA ✍️ ✅ |
8dc6f01 to
391b1e1
Compare
e91860f to
91b5b33
Compare
|
@fanhousanbu 已 rebase 到最新 main(跳过了 P0-12a/b + P0-10 共 6 个已合并 commits)。只剩 P0-11 的 5 个 commits。请 approve。 |
Waiting on PR #118 + size adjustment neededCI failing with EIP-170 size overflow in After rebasing onto main (post #118 merge), this PR will still be ~42 bytes over EIP-170. To land cleanly:
The P2 @fanhousanbu re-review needed after these adjustments — the previous approval was dismissed by force-push. |
EIP-170 优化已推送 (commit 26eb558)PR #114 在 rebase 到 main(待 #118 合并后)之前,已在本分支预先应用以下字节裁剪:
当前在旧 main 基准 (26,166B) 上的净增量:+81B Rebase 后预期大小
还需 46 bytes — Rebase 时处理
SDK 注意:三个常量已改为
请先 merge #118 再 rebase 此 PR,并重新测量大小后 merge。 |
…d with SDK migration guide Remove from on-chain ABI to gain ~230B headroom for P0 PRs #110/#113/#114: - isChainlinkStale() → SDK reads cachedPrice().updatedAt + priceStalenessThreshold - getAvailableCredit() → SDK computes off-chain from pendingDebts + registry.getCreditLimit - getSlashHistory() → use getSlashCount + getSlashRecord(operator, index) loop - getLatestSlash() → use getSlashRecord(operator, count - 1) Add getSlashRecord(address,uint256) returns (SlashRecord) to work around Solidity mapping-to-struct-array tuple limitation. Net cost: ~30B. Net savings: ~200B. Update test files to use new API: - EmergencyBreakGlass.t.sol: replace isChainlinkStale() with local _chainlinkStale() - SuperPaymasterV3Query.t.sol: full rewrite using getSlashCount + getSlashRecord - SuperPaymasterV3_Admin.t.sol: use getSlashRecord() instead of removed functions Docs: - API_SUPERPAYMASTER.md: mark removed functions with migration paths; update context encoding (6→5 field), add emergencySetPrice/cancel/execute, add updatePriceDVT chainlinkRecovered param; add⚠️ SDK MAINTAINER banner at top - eip170-impact-analysis: append Section 7 with SDK migration code samples, off-chain indexing guidance, and ABI update instructions All 617 tests pass.
Three setter functions previously accepted arbitrary values (P0-11 / B2-N3 / B4-M2): SuperPaymaster.setAPNTSPrice — absolute [1e15, 1e21], ±10% delta xPNTsToken.updateExchangeRate — absolute [1e14, 1e22], ±20% delta PaymasterBase.setCachedPrice — absolute [$100e8, $1Me8], ±30% delta Delta check is skipped on first set (oldPrice == 0) to allow initial deployment. Existing tests that passed out-of-range values updated to stay within the new windows; 26 new PriceSetter_Bounds tests added.
…hangeRate Adds a private BPS_DENOMINATOR = 10_000 constant to xPNTsToken.sol and uses it in the delta-bound calculation instead of the bare literal, so EXCHANGE_RATE_DELTA_BPS and the denominator stay in sync if the constant changes in the future.
…ta-skip invariant Add NatSpec to setAPNTSPrice() addressing two reviewer findings: - MEDIUM: explain that the ±10% delta in setAPNTSPrice and the ±20% cap in emergencySetPrice are independent paths that operate on different storage slots (aPNTsPriceUSD vs cachedPrice) and cannot be used to bypass each other. - LOW: document that the `if (oldPrice != 0)` delta-skip relies on initialize() setting a non-zero initial price, and that storage layout corruption on upgrade could cause the delta guard to be bypassed for the first write post-upgrade.
UUPSUpgrade.t.sol::test_SuperPaymaster_BusinessLogicAfterUpgrade called setAPNTSPrice(0.05 ether) with an initial price of 0.02 ether. That is a +150% move — far outside the ±10% per-tx delta cap added by P0-11. Changed to 0.021 ether (+5%), which is within the allowed window. Tests: 496/496.
Prevents setting staleness threshold to 0 (instant expiry breaks all ops) or an extreme value (disables the check for a day+). Hitchhike on P0-11.
- Make APNTS_PRICE_MIN/MAX/DELTA_BPS internal (removes 3 public getters, ~150B)
- Remove setPriceStalenessThreshold() function (P2 enhancement, not P0 security; ~73B)
- Simplify staleness init to single ternary (vs 3-line bounds check; ~20B)
- Use unchecked{} for delta lower/upper arithmetic (known-safe mul; ~47B)
- Merge redundant `newPrice == 0` check into `< APNTS_PRICE_MIN` (~5B)
Estimated net addition to SuperPaymaster after these changes: +81B over
EIP-170 baseline (24,541B). Expected final size after rebase on #118:
~24,622B (46B over). Size must be re-measured after rebasing on #118.
SDK note: APNTS_PRICE_MIN = 1e15, APNTS_PRICE_MAX = 1e21,
APNTS_PRICE_DELTA_BPS = 1000 — hardcode in SDK constants.
…grace period - SuperPaymaster: TIMESTAMP_GRACE_SECONDS public→internal (saves ~39B getter) - SuperPaymaster: EMERGENCY_EXPIRY public→internal (saves ~15B getter) - PaymasterBase.setCachedPrice: remove 15s grace period (admin path must reject any future timestamp; grace is for keeper/DVT paths only) - Tests: replace paymaster.TIMESTAMP_GRACE_SECONDS() calls with literal 15 - Tests: rewrite test_SetCachedPrice_AcceptsTimestampWithinGrace → test_SetCachedPrice_RejectsAnyFutureTimestamp (semantics changed) - Tests: replace paymaster.EMERGENCY_EXPIRY() calls with literal 7 days Result: SuperPaymaster 24,564B (12B under EIP-170 limit), 631/631 tests pass
5487042 to
acd96a9
Compare
fanhousanbu
left a comment
There was a problem hiding this comment.
✅ Approved
P0-11 三处价格 setter 新增 MIN/MAX/DELTA bounds,验证如下:
SuperPaymaster.setAPNTSPrice
- MIN=1e15 / MAX=1e21 / DELTA=±10%(BPS 1000)
unchecked乘法安全:oldPrice ≤ MAX=1e21,oldPrice * 11000 / 10000 ≤ 1.1e24,远低于 uint256 上限
PaymasterBase(V4)setCachedPrice / updatePriceDVT
- MIN=100e8($100) / MAX=1_000_000e8($1M) / DELTA=±30%(BPS 3000)
TIMESTAMP_GRACE_SECONDS/EMERGENCY_EXPIRY改为internal(EIP-170 优化),测试改用硬编码值
xPNTsToken.updateExchangeRate
- MIN=1e14 / MAX=1e22 / DELTA=±20%(BPS 2000)
- 旧测试中
updateExchangeRate(2e18)(+100%超限)已删除,替换为合规值
测试值更新正确(setAPNTSPrice 0.021 ether ≈ +5%,在±10%窗口内)。
fanhousanbu
left a comment
There was a problem hiding this comment.
✅ Approved
P0-11 三处 setter 新增 MIN/MAX/DELTA bounds,逻辑正确:
- setAPNTSPrice: MIN=1e15 / MAX=1e21 / DELTA=±10%,unchecked 乘法安全(值有上界,不会溢出 uint256)
- setCachedPrice/updatePriceDVT: MIN=$100 / MAX=$1M / DELTA=±30%,TIMESTAMP_GRACE_SECONDS 改 internal(EIP-170)
- updateExchangeRate: MIN=1e14 / MAX=1e22 / DELTA=±20%,旧超限测试值已删除换为合规值
Summary
P0-11 (B2-N3 / B4-M2): Three setter functions previously accepted arbitrary values — a single mis-typed owner call could distort pricing for all operators at once.
Three setters hardened:
SuperPaymaster.setAPNTSPricexPNTsToken.updateExchangeRatePaymasterBase.setCachedPriceDelta check is skipped when
oldPrice == 0(first deployment set). All three setters share the pattern: zero check → absolute bounds → delta bounds.Stack dependency: This PR is stacked on
fix/p0-10-breakglass(P0-10). GitHub will auto-rebase the base when P0-10 merges.Test plan
PriceSetter_Bounds.t.sol(3 suites, one per setter)P2 顺风车追加(B2-N11)
priceStalenessThreshold无边界检查:设为 0 让所有价格立即过期;设为极大值让过期检测失效。追加修复:
setPriceStalenessThreshold():加[60, 86400]bounds,违反时revert Paymaster__InvalidStalenessThreshold()setPriceStalenessThreshold(uint256 val)setter(原来缺失),复用InvalidConfiguration()error;initialize()超出范围时 fallback 到 3600 秒默认值(构造时容错,setter 时严格 revert)496 tests pass.