feat: add RocketPool Shield validator with STAKE and APPROVAL validation#13
feat: add RocketPool Shield validator with STAKE and APPROVAL validation#13
Conversation
📝 WalkthroughWalkthroughAdds a new RocketPoolValidator for EVM that validates Rocket Pool STAKE and APPROVAL transactions, re-exports and registers it in the validator registry, includes ABI/interface decoding, and adds comprehensive tests and a package version bump. Changes
Sequence DiagramsequenceDiagram
participant Client
participant RocketPoolValidator
participant BaseEVMValidator
participant ethersInterface as ethers.Interface
participant ValidationResult
Client->>RocketPoolValidator: validate(unsignedTx, txType, userAddress)
RocketPoolValidator->>BaseEVMValidator: decode unsignedTx
BaseEVMValidator-->>RocketPoolValidator: EVMTransaction
RocketPoolValidator->>RocketPoolValidator: check tx.from == userAddress
RocketPoolValidator->>RocketPoolValidator: check chainId supported
alt txType == STAKE
RocketPoolValidator->>ethersInterface: decodeFunction(calldata)
ethersInterface-->>RocketPoolValidator: swapTo(params)
RocketPoolValidator->>RocketPoolValidator: validateStake (to address, value > 0, min/ideal checks)
else txType == APPROVAL
RocketPoolValidator->>ethersInterface: decodeFunction(calldata)
ethersInterface-->>RocketPoolValidator: approve(spender, amount)
RocketPoolValidator->>RocketPoolValidator: validateApproval (to address, value == 0, amount > 0)
end
RocketPoolValidator->>ValidationResult: return safe() or failure(reason, details)
ValidationResult-->>Client: ValidationResult
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/validators/evm/rocketpool/rocketpool.validator.test.ts (1)
39-847: Extract a shared tx fixture builder to reduce duplication.Most tests rebuild near-identical transaction objects; a small factory would make cases shorter and less brittle.
♻️ Example refactor direction
+ const baseLegacyTx = { + nonce: 0, + gasLimit: '0x30d40', + gasPrice: '0x4a817c800', + chainId: 1, + type: 0 as const, + }; + + const makeTx = (overrides: Record<string, unknown>) => ({ + ...baseLegacyTx, + ...overrides, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/validators/evm/rocketpool/rocketpool.validator.test.ts` around lines 39 - 847, Introduce a small tx fixture builder in this test file (e.g., function buildTx(overrides?: Partial<any>) => ({ to: rocketSwapRouterAddress, from: userAddress, value: '0xde0b6b3a7640000', data: stakeCalldata, nonce: 0, gasLimit: '0x30d40', gasPrice: '0x4a817c800', chainId: 1, type: 0, ...overrides })) and use it everywhere you currently inline transaction objects (the tests that call shield.validate with JSON.stringify(tx)), passing overrides for to, from, value, data, chainId, gas fields, etc.; update tests that require EIP-1559 fields to override maxFeePerGas/maxPriorityFeePerGas and remove gasPrice, and for approval tests override to: rETHAddress and data: approveCalldata (or zero/modified calldata), and ensure JSON.stringify(buildTx({...})) is used so all tests (stake, approval, auto-detection, general validation) reuse the single builder and reduce duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/validators/evm/index.ts`:
- Line 4: The file export line "export { RocketPoolValidator } from
'./rocketpool/rocketpool.validator';" is missing a trailing EOF newline which
breaks Prettier CI; simply add a single newline character at the end of the file
(after the export statement) so the file ends with a newline and rerun CI.
In `@src/validators/evm/rocketpool/rocketpool.validator.test.ts`:
- Around line 10-11: Prettier formatting errors in rocketpool.validator.test.ts:
fix the formatting around the rocketSwapRouterAddress constant (and the other
offending blocks around the regions flagged at 504-506) by running Prettier or
reformatting so string literals/lines match the project's Prettier rules, and
add a newline at EOF; specifically update the declaration for
rocketSwapRouterAddress and any nearby long literals to be single-line or
appropriately wrapped per Prettier, re-run the formatter (or apply the project's
Prettier config) to the whole file, and ensure the file ends with a trailing
newline.
In `@src/validators/evm/rocketpool/rocketpool.validator.ts`:
- Around line 154-157: The approval flow currently skips spender validation
(result.parsed.args[0]) which allows arbitrary approve(spender, amount) to rETH;
update the rocketpool validator to enforce spender checks by validating
result.parsed.args[0] against an allowlist of expected spender addresses (e.g.,
LI.FI aggregator and any RocketPool-specific contracts) and reject approvals for
any other addresses. Implement this by adding or using a helper like
isAllowedSpender(spender) and integrate it into the existing validation path
that examines the approve call to rETH (ensure the code path that currently
comments out spender validation performs a strict equality/containment check
against the ALLOWED_SPENDERS set and returns a failure if not matched). Ensure
error messages/logging identify the offending spender for debugging.
- Line 54: Prettier is failing on several argument/spacing commas in the
validator; reformat the file with Prettier (or fix manually) so argument lists
conform to project style—for example the call containing "tx, 1," should be
formatted to match Prettier output (proper spacing and trailing-comma usage),
and apply the same fix to the other failing spots on lines with similar argument
lists; run the project's prettier config or npm/yarn format command and commit
the resulting changes so rocketpool.validator.ts passes CI.
---
Nitpick comments:
In `@src/validators/evm/rocketpool/rocketpool.validator.test.ts`:
- Around line 39-847: Introduce a small tx fixture builder in this test file
(e.g., function buildTx(overrides?: Partial<any>) => ({ to:
rocketSwapRouterAddress, from: userAddress, value: '0xde0b6b3a7640000', data:
stakeCalldata, nonce: 0, gasLimit: '0x30d40', gasPrice: '0x4a817c800', chainId:
1, type: 0, ...overrides })) and use it everywhere you currently inline
transaction objects (the tests that call shield.validate with
JSON.stringify(tx)), passing overrides for to, from, value, data, chainId, gas
fields, etc.; update tests that require EIP-1559 fields to override
maxFeePerGas/maxPriorityFeePerGas and remove gasPrice, and for approval tests
override to: rETHAddress and data: approveCalldata (or zero/modified calldata),
and ensure JSON.stringify(buildTx({...})) is used so all tests (stake, approval,
auto-detection, general validation) reuse the single builder and reduce
duplication.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
Run ID: ee65d74c-869b-47a0-b8f8-6df64b21e51b
📒 Files selected for processing (4)
src/validators/evm/index.tssrc/validators/evm/rocketpool/rocketpool.validator.test.tssrc/validators/evm/rocketpool/rocketpool.validator.tssrc/validators/index.ts
| // Note: spender (result.parsed.args[0]) is NOT validated — it's dynamic | ||
| // from LI.FI and changes per quote. The spender validation is intentionally | ||
| // omitted because the exit path routes through LI.FI aggregator. | ||
|
|
There was a problem hiding this comment.
Approval flow currently allows arbitrary spender addresses.
Line [154]-Line [157] explicitly skip spender validation, so any approve(spender, amount) to rETH passes if other checks pass. That weakens the guardrail and allows attacker-controlled allowance targets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/validators/evm/rocketpool/rocketpool.validator.ts` around lines 154 -
157, The approval flow currently skips spender validation
(result.parsed.args[0]) which allows arbitrary approve(spender, amount) to rETH;
update the rocketpool validator to enforce spender checks by validating
result.parsed.args[0] against an allowlist of expected spender addresses (e.g.,
LI.FI aggregator and any RocketPool-specific contracts) and reject approvals for
any other addresses. Implement this by adding or using a helper like
isAllowedSpender(spender) and integrate it into the existing validation path
that examines the approve call to rETH (ensure the code path that currently
comments out spender validation performs a strict equality/containment check
against the ALLOWED_SPENDERS set and returns a failure if not matched). Ensure
error messages/logging identify the offending spender for debugging.
There was a problem hiding this comment.
When I initially built this PR, the spender was left intentionally unvalidated because the RocketPool exit path in the monorepo routes through the LI.FI aggregator — the approve spender comes from quote.estimate.approvalAddress in the LI.FI /v1/quote API response, which is dynamic per quote. Unlike Lido (which uses its own WithdrawalQueue at a fixed address), RocketPool's exit has no single deterministic spender we could hardcode. So the initial approach was: validate everything we can statically (target contract = rETH, method = approve, amount > 0, calldata integrity) and accept the spender as a known limitation.
During review, investigated whether the spender really is unbounded. Using Etherscan's ERC20 transfer data for the rETH contract, traced actual rETH exit flows and found there are exactly two LI.FI contracts that serve as approval targets:
- LI.FI Diamond (
0x1231DEB6f5749EF6cE6943a275A1D3E7486F4EaE) — primary swap router, handles most rETH→ETH exits - Permit2 Proxy (
0x89c6340B1a1f4b25D36cd8B063D49045caF3f818) — secondary path using Uniswap's Permit2 pattern, always forwards to the Diamond
Both use the Diamond Proxy pattern (EIP-2535), so the addresses are stable across upgrades — only internal facets change, not the entry point. We also cross-referenced this with how our backend constructs the approval (quote.estimate.approvalAddress from the LI.FI API), confirming these are the only two addresses returned.
We further analyzed the security implications for the SWAP transaction that follows this approval. Even with the spender whitelist, the SWAP calldata contains a _receiver parameter that an attacker could modify to redirect funds through the legitimate Diamond contract without needing to compromise LI.FI. This means validating the receiver inside the SWAP calldata is critical — not just the contract address.
The follow-up PR will:
- Add a spender whitelist for APPROVAL (these two addresses, block all others)
- Validate SWAP
tx.toagainst the same whitelist - Investigate parsing
_receiverfrom LI.FI Diamond calldata to prevent fund redirection attacks
There was a problem hiding this comment.
@ajag408, that's excellent security research — tracing actual rETH exit flows on-chain to bound the spender set is exactly the right approach. The two-address allowlist (0x1231DEB6f5749EF6cE6943a275A1D3E7486F4EaE for the LI.FI Diamond and 0x89c6340B1a1f4b25D36cd8B063D49045caF3f818 for the Permit2 Proxy) is well-motivated, and the EIP-2535 stability argument is sound.
The _receiver observation is the more important finding here — a spender allowlist alone would be incomplete if an attacker can redirect proceeds through a legitimate Diamond entry point. Prioritizing _receiver parsing in the follow-up is the right call.
For the follow-up PR, a few things to keep in mind:
- LI.FI Diamond ABI decoding — the Diamond forwards calls to internal facets, so the outer calldata selector won't be a well-known ABI method. You'll likely need to parse the inner payload (e.g.,
bridgeTokensViaLiFior similar) to extract_receiver. The LI.FI calldata format may be helpful here for identifying the receiver field position. - Allowlist maintenance — consider placing the two spender addresses in a named constant block (e.g.,
LIFI_CONTRACTS) alongsideROCKETPOOL_CONTRACTSso future additions are visible in one place. - Test coverage — make sure the follow-up adds cases for: (a) each of the two allowed spenders, (b) a rejected arbitrary spender, and (c) the
_receiverredirection scenario.
I'll note the key facts from this investigation for future reviews.
✏️ Learnings added
Learnt from: ajag408
Repo: stakekit/shield PR: 13
File: src/validators/evm/rocketpool/rocketpool.validator.ts:158-161
Timestamp: 2026-03-04T03:27:27.799Z
Learning: In the RocketPool Shield validator (`src/validators/evm/rocketpool/rocketpool.validator.ts`), the `approve()` spender for the rETH exit path is dynamically sourced from the LI.FI `/v1/quote` API (`quote.estimate.approvalAddress`). The only two valid spender addresses observed on-chain are: LI.FI Diamond `0x1231DEB6f5749EF6cE6943a275A1D3E7486F4EaE` (primary swap router) and Permit2 Proxy `0x89c6340B1a1f4b25D36cd8B063D49045caF3f818` (secondary, always forwards to the Diamond). Both use EIP-2535 Diamond Proxy and their addresses are stable across upgrades. A follow-up PR will add an allowlist for these spenders.
Learnt from: ajag408
Repo: stakekit/shield PR: 13
File: src/validators/evm/rocketpool/rocketpool.validator.ts:158-161
Timestamp: 2026-03-04T03:27:27.799Z
Learning: For the RocketPool Shield validator, even with a spender allowlist on the APPROVAL transaction, the subsequent SWAP transaction's `_receiver` parameter inside LI.FI Diamond calldata can be manipulated by an attacker to redirect funds through the legitimate Diamond contract entry point. Validating SWAP `tx.to` against the LI.FI contract allowlist is necessary but not sufficient — parsing `_receiver` from the inner Diamond calldata is also required to prevent fund redirection attacks.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/validators/evm/rocketpool/rocketpool.validator.test.ts (1)
642-668: Consider adding a comment noting this test documents temporary behavior.This test confirms that arbitrary spenders are currently accepted. Given that a follow-up PR will add a spender whitelist, consider adding a brief comment (e.g.,
// Temporary: accepts any spender until spender whitelist is implemented) to clarify this is intentional but not permanent behavior. This helps future readers understand the test should be updated when the whitelist is added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/validators/evm/rocketpool/rocketpool.validator.test.ts` around lines 642 - 668, Add a one-line inline comment in the test case "should accept approval with any spender address" (around the use of randomSpender/dynamicApproveCalldata/tx) indicating this behavior is temporary and a spender whitelist will be enforced later (e.g., "// Temporary: accepts any spender until spender whitelist is implemented") so future readers know the test should be updated when the whitelist is added.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/validators/evm/rocketpool/rocketpool.validator.test.ts`:
- Around line 642-668: Add a one-line inline comment in the test case "should
accept approval with any spender address" (around the use of
randomSpender/dynamicApproveCalldata/tx) indicating this behavior is temporary
and a spender whitelist will be enforced later (e.g., "// Temporary: accepts any
spender until spender whitelist is implemented") so future readers know the test
should be updated when the whitelist is added.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
Run ID: a012af8d-325e-4ff5-bd45-38d9d8ba1381
📒 Files selected for processing (4)
package.jsonsrc/validators/evm/index.tssrc/validators/evm/rocketpool/rocketpool.validator.test.tssrc/validators/evm/rocketpool/rocketpool.validator.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/validators/evm/index.ts
1 participant
RocketPool Shield Validator
Summary by CodeRabbit
Release Notes
New Features
Tests
Adds a Shield transaction validator for the RocketPool protocol (
ethereum-eth-reth-stakingyield ID), covering the STAKE and APPROVAL transaction types.Follows the established validator pattern (Lido, ERC4626) — extends
BaseEVMValidator, registers in the validator registry, and validates unsigned EVM transactions before they are signed and broadcast.Yield ID
ethereum-eth-reth-stakingContracts
0xae78736Cd615f374D3085123A210448E74Fc63930x16D5A408e807db8eF7c578279BEeEe6b228f1c1CTransaction Types Validated
STAKE (
RocketSwapRouter.swapTo)Validates the enter/deposit path — user sends ETH, receives rETH.
tx.to == RocketSwapRouterTransaction not to RocketPool SwapRouter contracttx.from == userAddressTransaction not from user addresschainId == 1RocketPool only supported on Ethereum mainnetvalue > 0(payable — must send ETH)Stake must send ETH valueswapTo(uint256,uint256,uint256,uint256)Failed to parse transaction data/Invalid method for stakingTransaction calldata has been tampered with_minTokensOut > 0Minimum tokens out must be greater than zero_idealTokensOut > 0Ideal tokens out must be greater than zero_minTokensOut <= _idealTokensOutMinimum tokens out exceeds ideal tokens outThe
_uniswapPortionand_balancerPortionparameters are not validated — they are dynamically computed byRocketSwapRouter.optimiseSwapTo()on-chain and vary with pool state. One portion being zero (100% through a single pool) is a valid scenario.APPROVAL (
rETH.approve)Validates the exit step 0 — user approves rETH spending before the LI.FI swap.
tx.to == rETHTransaction not to RocketPool rETH contracttx.from == userAddressTransaction not from user addresschainId == 1RocketPool only supported on Ethereum mainnetvalue == 0(no ETH sent)Approval should not send ETH valueapprove(address,uint256)Failed to parse transaction data/Invalid method for approvalTransaction calldata has been tampered withamount > 0Approval amount must be greater than zeroSpender is intentionally not validated. The
approvespender address is dynamic — it comes from the LI.FI aggregator API and changes per quote. This is an architectural constraint of the RocketPool exit path in the monorepo, which routes unstaking through LI.FI rather than a native rETH burn.Files Changed
validators/evm/rocketpool/rocketpool.validator.tsvalidators/evm/rocketpool/rocketpool.validator.test.tsvalidators/evm/index.tsRocketPoolValidatorexportvalidators/index.tsethereum-eth-reth-stakingTest Coverage (33 tests)
Design Notes
rewardClaiming: Auto).0xae78736C...(rETH) and0x16D5A408...(RocketSwapRouter) are the same ones hardcoded in the monorepo. If RocketPool upgrades their router, both the monorepo and Shield need updating.Follow-up PR: SWAP Validation + Exhaustive Tests
A second stacked PR will add:
APPROVAL Spender Whitelist
In this PR, the
approvespender is not validated because it comes dynamically from the LI.FI API. During review, on-chain investigation (via Etherscan ERC20 transfer data for the rETH contract) confirmed there are exactly two LI.FI contracts used as approval targets for RocketPool exits on Ethereum mainnet:0x1231DEB6f5749EF6cE6943a275A1D3E7486F4EaE0x89c6340B1a1f4b25D36cd8B063D49045caF3f818Both use the Diamond Proxy pattern (EIP-2535), meaning their addresses are stable across upgrades — only internal facets change, not the entry point. This was cross-referenced with how the monorepo constructs the approval (
quote.estimate.approvalAddressfrom the LI.FI/v1/quoteAPI).The follow-up PR will add a whitelist of these two addresses and block approvals to any other spender.
SWAP Transaction Validation (exit step 1)
The unstake SWAP transaction calls one of the same two LI.FI contracts identified above. While
tx.tois constrained to these addresses,tx.datais dynamic — the LI.FI API selects from dozens of Diamond facet functions (swapTokensSingleV3ERC20ToNative,swapTokensGeneric,bridge, etc.) depending on the optimal route.The follow-up PR will add
TransactionType.SWAPtogetSupportedTransactionTypes()with:Envelope validation:
tx.from == userAddresschainId == 1tx.tois one of the two whitelisted LI.FI addressestx.datais not emptyCalldata receiver validation (critical):
Most LI.FI Diamond swap functions include a
_receiverparameter specifying who gets the swap output. Without validating this, an attacker who intercepts the API response could modify_receiverto their own address — the call would execute successfully on the legitimate Diamond contract and redirect the user's funds. The attacker would not need to compromise LI.FI itself.The follow-up PR will investigate the LI.FI Diamond ABI (
github.com/lifinance/lifi-contract-types) to determine:_receiverhas a consistent offset across facet functionsDocumentation
Inline documentation in the validator file covering the LI.FI limitation, contract roles, and why APPROVAL validation serves as the primary exit-path protection.