[Do Not Merge: CI BLOCKED ON PR #20] feat: add slippage floor validation for RocketPool STAKE and SWAP (XBTO request) #22
Open
ajag408 wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds slippage-floor validation to the RocketPoolValidator for both RocketPool staking transactions and LI.FI swap operations. An 80% floor is enforced on staking (minTokensOut must not fall below 80% of ETH value), and on swaps (minAmountOut must not fall below 80% of fromAmount, with logic to sum fromAmount across Multiple swap entries). Test coverage includes boundary cases, happy paths, receiver validation, and comprehensive slippage-floor scenarios. ChangesSlippage Floor Validation for RocketPool & LI.FI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/validators/evm/rocketpool/rocketpool.validator.ts (1)
312-324: ⚡ Quick winConsider blocking swaps with zero
fromAmount.When
totalFromAmount === 0n, the slippage floor check is skipped entirely. While mathematically this is equivalent to a floor of 0, a legitimate swap should always have a positive input amount. Blocking this case maintains the fail-closed security posture.♻️ Proposed fix to block zero fromAmount
+ if (totalFromAmount <= 0n) { + return this.blocked('fromAmount must be greater than zero'); + } - if (totalFromAmount > 0n) { - const slippageFloor = (totalFromAmount * SLIPPAGE_FLOOR_BPS) / 10000n; - if (minAmountOut < slippageFloor) { - return this.blocked( - '_minAmountOut is too low relative to fromAmount (slippage floor)', - { - minAmountOut: minAmountOut.toString(), - fromAmount: totalFromAmount.toString(), - slippageFloor: slippageFloor.toString(), - }, - ); - } + const slippageFloor = (totalFromAmount * SLIPPAGE_FLOOR_BPS) / 10000n; + if (minAmountOut < slippageFloor) { + return this.blocked( + '_minAmountOut is too low relative to fromAmount (slippage floor)', + { + minAmountOut: minAmountOut.toString(), + fromAmount: totalFromAmount.toString(), + slippageFloor: slippageFloor.toString(), + }, + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validators/evm/rocketpool/rocketpool.validator.ts` around lines 312 - 324, The check currently skips when totalFromAmount === 0n; instead enforce that totalFromAmount must be > 0n by adding a guard before the slippage-floor logic: if totalFromAmount === 0n return this.blocked(...) with a clear message (e.g., "_fromAmount must be > 0") and include totalFromAmount and minAmountOut in the metadata; keep the existing slippage-floor check using SLIPPAGE_FLOOR_BPS and minAmountOut unchanged so the zero-case is explicitly rejected.src/validators/evm/rocketpool/rocketpool.validator.test.ts (5)
1115-1120: 💤 Low valueFix code formatting.
The code has Prettier formatting violations that should be corrected.
🎨 Proposed formatting fix
- const permit2WrappedMultiple = - permit2ProxyIface.encodeFunctionData('callDiamondWithPermit2', [ - multipleSwapCalldata, - dummyPermit, - dummySignature, - ]); + const permit2WrappedMultiple = permit2ProxyIface.encodeFunctionData( + 'callDiamondWithPermit2', + [multipleSwapCalldata, dummyPermit, dummySignature] + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validators/evm/rocketpool/rocketpool.validator.test.ts` around lines 1115 - 1120, The line assigning permit2WrappedMultiple is misformatted; reformat the call to permit2ProxyIface.encodeFunctionData('callDiamondWithPermit2', [multipleSwapCalldata, dummyPermit, dummySignature]) to match project Prettier rules (proper indentation and line breaks) so the array arguments are each on their own properly indented line and the overall statement follows surrounding test formatting conventions; update the statement that defines permit2WrappedMultiple accordingly.
1659-1664: 💤 Low valueFix code formatting.
The code has Prettier formatting violations that should be corrected.
🎨 Proposed formatting fix
- const permit2WrappedManipulated = - permit2ProxyIface.encodeFunctionData('callDiamondWithPermit2', [ - manipulatedDiamondCalldata, - dummyPermit, - dummySignature, - ]); + const permit2WrappedManipulated = permit2ProxyIface.encodeFunctionData( + 'callDiamondWithPermit2', + [manipulatedDiamondCalldata, dummyPermit, dummySignature] + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validators/evm/rocketpool/rocketpool.validator.test.ts` around lines 1659 - 1664, The test contains Prettier formatting violations around the Permit2 call encoding; reformat the expression that assigns permit2WrappedManipulated so it complies with project Prettier rules (proper line breaks and indentation) for the call to permit2ProxyIface.encodeFunctionData('callDiamondWithPermit2', [manipulatedDiamondCalldata, dummyPermit, dummySignature]); locate the permit2WrappedManipulated assignment and reflow the arguments (or inline them) and spacing to match surrounding tests' style so the file passes Prettier.
1688-1688: 💤 Low valueRemove trailing whitespace.
Line has unnecessary trailing whitespace.
🎨 Proposed fix
expect(swapAttempt?.reason).toContain('slippage floor'); }); - + it('should reject a Multiple variant swap with manipulated slippage', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validators/evm/rocketpool/rocketpool.validator.test.ts` at line 1688, Remove the trailing whitespace on the indicated line in src/validators/evm/rocketpool/rocketpool.validator.test.ts (line ~1688) by deleting the extra space at the end of the line; you can also run the repo formatter or ESLint fix to ensure no other trailing whitespace remains and commit the cleaned file (target the test file name rocketpool.validator.test.ts to locate the offending line).
1297-1297: 💤 Low valueRemove trailing whitespace.
Line has unnecessary trailing whitespace.
🎨 Proposed fix
- }); - + }); + it('should reject Multiple variant swap with wrong receiver', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validators/evm/rocketpool/rocketpool.validator.test.ts` at line 1297, Remove the trailing whitespace on the reported empty line in rocketpool.validator.test.ts by deleting the extra space characters at the end of the line (or run your formatter/prettier to trim end-of-line whitespace), then re-run the tests/linter to ensure no trailing-space warnings remain.
1610-1610: 💤 Low valueRemove trailing whitespace.
Line has unnecessary trailing whitespace.
🎨 Proposed fix
expect(result.detectedType).toBe(TransactionType.SWAP); }); - + it('should reject SWAP with _minAmountOut just below 80% floor', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validators/evm/rocketpool/rocketpool.validator.test.ts` at line 1610, Remove the trailing whitespace on the flagged line in rocketpool.validator.test.ts: open the test file (rocketpool.validator.test.ts), find the empty/whitespace-only characters at the end of the reported line and delete them so the line ends with no trailing spaces, then save and run linters/tests to confirm no trailing-whitespace errors remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/validators/evm/rocketpool/rocketpool.validator.test.ts`:
- Around line 1115-1120: The line assigning permit2WrappedMultiple is
misformatted; reformat the call to
permit2ProxyIface.encodeFunctionData('callDiamondWithPermit2',
[multipleSwapCalldata, dummyPermit, dummySignature]) to match project Prettier
rules (proper indentation and line breaks) so the array arguments are each on
their own properly indented line and the overall statement follows surrounding
test formatting conventions; update the statement that defines
permit2WrappedMultiple accordingly.
- Around line 1659-1664: The test contains Prettier formatting violations around
the Permit2 call encoding; reformat the expression that assigns
permit2WrappedManipulated so it complies with project Prettier rules (proper
line breaks and indentation) for the call to
permit2ProxyIface.encodeFunctionData('callDiamondWithPermit2',
[manipulatedDiamondCalldata, dummyPermit, dummySignature]); locate the
permit2WrappedManipulated assignment and reflow the arguments (or inline them)
and spacing to match surrounding tests' style so the file passes Prettier.
- Line 1688: Remove the trailing whitespace on the indicated line in
src/validators/evm/rocketpool/rocketpool.validator.test.ts (line ~1688) by
deleting the extra space at the end of the line; you can also run the repo
formatter or ESLint fix to ensure no other trailing whitespace remains and
commit the cleaned file (target the test file name rocketpool.validator.test.ts
to locate the offending line).
- Line 1297: Remove the trailing whitespace on the reported empty line in
rocketpool.validator.test.ts by deleting the extra space characters at the end
of the line (or run your formatter/prettier to trim end-of-line whitespace),
then re-run the tests/linter to ensure no trailing-space warnings remain.
- Line 1610: Remove the trailing whitespace on the flagged line in
rocketpool.validator.test.ts: open the test file (rocketpool.validator.test.ts),
find the empty/whitespace-only characters at the end of the reported line and
delete them so the line ends with no trailing spaces, then save and run
linters/tests to confirm no trailing-whitespace errors remain.
In `@src/validators/evm/rocketpool/rocketpool.validator.ts`:
- Around line 312-324: The check currently skips when totalFromAmount === 0n;
instead enforce that totalFromAmount must be > 0n by adding a guard before the
slippage-floor logic: if totalFromAmount === 0n return this.blocked(...) with a
clear message (e.g., "_fromAmount must be > 0") and include totalFromAmount and
minAmountOut in the metadata; keep the existing slippage-floor check using
SLIPPAGE_FLOOR_BPS and minAmountOut unchanged so the zero-case is explicitly
rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 270d8592-7b23-4465-a6cb-a52c0dab2333
📒 Files selected for processing (2)
src/validators/evm/rocketpool/rocketpool.validator.test.tssrc/validators/evm/rocketpool/rocketpool.validator.ts
ajag408
changed the title
ajag408
requested review from
Philippoes,
aditya172926,
auralshin,
jdomingos and
raiseerco
Contributor
Author
|
**Blocked on #20 **: This PR requires the CI hardening PR to merge first — needed for CI workflows to run on this branch. |
ajag408
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Security Improvements
Tests
minTokensOuton STAKE and_minAmountOuton SWAP in the RocketPool validatorminTokensOut > 0(STAKE) and validated_receiver(SWAP) — a compromised upstream could setminTokensOut = 1 weion a 1 ETH stake and pass validation, exposing the transaction to MEV sandwich extractionWhat changed
src/validators/evm/rocketpool/rocketpool.validator.tsSLIPPAGE_FLOOR_BPSconstant, slippage check invalidateStake,_minAmountOut > 0+ slippage check invalidateLifiSwapReceiver(with Single/Multiple variant handling)src/validators/evm/rocketpool/rocketpool.validator.test.tsSecurity context
Addresses the slippage parameter manipulation gap identified in the Shield security review. Unlike the
args.amountvalidation (Phase 6), this fix is independently shippable because both the input (tx.value/fromAmount) and output floor (minTokensOut/_minAmountOut) are already present in the transaction calldata.Threat model: requires a compromised upstream API (consistent with Shield's threat model). The validator remains fail-closed — unknown LI.FI function selectors are blocked, not bypassed.
Test plan
_minAmountOutblocked on SWAPfromAmountsumming, slippage floor