fix(profile-storage): guard single-fee underflow in active operator-fee getters#1263
Conversation
…ee getters getActiveOperatorFee / getActiveOperatorFeePercentage read operatorFees[length - 2] whenever the latest fee is not yet effective. With a single fee (length == 1) that index underflows (1 - 2) -> panic 0x11 -> DoS on the active operator-fee getters. Reachable right after createProfile (which seeds operatorFees[0] at effectiveDate == block.timestamp, so `block.timestamp > effectiveDate` is false in the same block), and any time a lone not-yet-effective fee exists. Guard the else-branch with `length == 1` -- a single fee is always the active one. The length >= 2 pending-fee path is unchanged, and the `>` boundary is preserved (no reward-split impact: StakingV10 reads fees via getOperatorFeePercentageByTimestampReverse, not these getters). Bumps ProfileStorage 10.0.2 -> 10.0.3. Adds regression tests (proven to fail on the pre-fix code with panic 0x11; full Profile @Unit suite 53 passing). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| // underflow below when exactly one (not-yet-effective) fee exists — | ||
| // e.g. right after profile creation, where the initial fee's | ||
| // effectiveDate == block.timestamp falls into the else branch. | ||
| if (length == 1 || block.timestamp > fees[length - 1].effectiveDate) { |
There was a problem hiding this comment.
🔴 Bug: The fix duplicates the active-fee selection in two getters, but the same length - 2 fallback still exists in getActiveOperatorFeeEffectiveDate(). With exactly one fee whose effectiveDate >= block.timestamp (including the createProfile equality case), that public active-fee getter still panics on operatorFees[length - 2]. This PR should centralize active-fee lookup in one helper and have the fee, percentage, and effective-date getters all use it, with the regression covered for the effective-date getter too.
There was a problem hiding this comment.
Fixed in 8b4d3667a — centralized the active-fee selection in a single _activeOperatorFee(identityId) helper (with the length==1 / length-2 guard), and routed all three getters through it: getActiveOperatorFee, getActiveOperatorFeePercentage, and getActiveOperatorFeeEffectiveDate. The effective-date getter no longer has its own length-2 path, so the single-not-yet-effective-fee panic (incl. the createProfile equality case) is gone. ProfileStorage 10.0.3 -> 10.0.4.
| // used to read operatorFees[length - 2] whenever the latest fee was not yet | ||
| // effective. With a single fee (length == 1) that is `operatorFees[-1]`, a uint | ||
| // underflow → out-of-bounds panic → DoS on the active-fee getters. | ||
| describe('getActiveOperatorFee* — single-fee length-2 underflow regression', () => { |
There was a problem hiding this comment.
🔴 Bug: The regression suite only exercises getActiveOperatorFee and getActiveOperatorFeePercentage, but ProfileStorage.getActiveOperatorFeeEffectiveDate() is another public active-fee getter with the same operatorFees[length - 2] shape when the latest fee is not past. As written, these tests could pass while that API still panics for the same single-fee state right after createProfile or after setting one future fee. Add coverage for the effective-date getter in the single-fee and pending-second-fee cases so the validation evidence covers the full active-fee surface.
There was a problem hiding this comment.
Fixed in 8b4d3667a — added getActiveOperatorFeeEffectiveDate assertions to all three regression cases (sole fee after createProfile, sole not-yet-effective fee, pending second fee). Verified it goes red the right way: restoring the old effective-date getter makes the 'not yet effective' case panic at ProfileStorage.getActiveOperatorFeeEffectiveDate, and it passes once routed through the shared helper.
…date getter underflow (PR #1263 review) otReviewAgent 🔴: getActiveOperatorFeeEffectiveDate still read operatorFees[length - 2] without the length==1 guard, so it panicked on the single-not-yet-effective-fee state (incl. the createProfile equality case) — the same DoS the other two getters were fixed for. Centralize the active-fee selection in one `_activeOperatorFee` helper and route all three public getters (fee / percentage / effective-date) through it, so the guard lives in one place. Extend the regression suite to assert getActiveOperatorFeeEffectiveDate in the single-fee (createProfile + not-yet-effective) and pending-second-fee cases — verified it panics under the pre-fix effective-date getter and passes after. ProfileStorage 10.0.3 -> 10.0.4. Profile.test.ts: 53 passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function _activeOperatorFee(uint72 identityId) internal view returns (ProfileLib.OperatorFee memory) { | ||
| ProfileLib.OperatorFee[] storage fees = profiles[identityId].operatorFees; | ||
| uint256 length = fees.length; | ||
| if (length == 0) { | ||
| return ProfileLib.OperatorFee({feePercentage: 0, effectiveDate: 0}); | ||
| } | ||
|
|
||
| if ( | ||
| block.timestamp > | ||
| profiles[identityId].operatorFees[profiles[identityId].operatorFees.length - 1].effectiveDate | ||
| ) { | ||
| return profiles[identityId].operatorFees[profiles[identityId].operatorFees.length - 1]; | ||
| } else { | ||
| return profiles[identityId].operatorFees[profiles[identityId].operatorFees.length - 2]; | ||
| if (length == 1 || block.timestamp > fees[length - 1].effectiveDate) { | ||
| return fees[length - 1]; | ||
| } | ||
| return fees[length - 2]; | ||
| } |
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Review Agent completed this review and found no issues.
3 participants
Summary
ProfileStorage.getActiveOperatorFeeandgetActiveOperatorFeePercentagereadoperatorFees[length - 2]whenever the latest fee is not yet effective. When there is exactly one fee (length == 1), that isoperatorFees[1 - 2]— auintunderflow → out-of-bounds panic (0x11) → DoS on the active operator-fee getters.Reachability
createProfile— it seedsoperatorFees[0]ateffectiveDate == block.timestamp, soblock.timestamp > effectiveDateisfalsein the same block →elsebranch →operatorFees[-1].Fix
Guard the
elsebranch withlength == 1— a single fee is always the active one. Thelength >= 2pending-fee path is unchanged, and the>boundary is preserved, so there is no reward-split semantics change (liveStakingV10reads operator fees viagetOperatorFeePercentageByTimestampReverse, not these getters).ProfileStorageversion bumped10.0.2 → 10.0.3.Tests
3 regression tests added to
test/unit/Profile.test.ts:createProfile,length >= 2pending fee (no-regression on the genuine else-branch).Verified the 2 trigger tests fail on the pre-fix code with
panic 0x11; fullProfile @unitsuite: 53 passing.Provenance
Surfaced by a multi-model security review (DeepSeek V4 Pro / Grok 4.20 / Gemini 3.1 Pro) and source-verified; two models independently flagged it. Severity: Medium (view-function DoS; no fund loss). Note: ProfileStorage is a plain-constructor storage contract, so this lands with a fresh/redeployed ProfileStorage.
🤖 Generated with Claude Code