Wormhole: enforce cross-chain executor guardrails#962
Open
mswilkison wants to merge 9 commits into
Open
Conversation
mswilkison
force-pushed
the
codex/wormhole-ntt-guardrails
branch
2 times, most recently
from
e97c4e2 to
8f11b4b
Compare
mswilkison
force-pushed
the
codex/wormhole-ntt-guardrails
branch
from
8f11b4b to
10dccaf
Compare
mswilkison
requested review from
Shadowfiend,
lrsaturnino,
lukasz-zimnoch,
nkuba,
pdyraga and
piotr-roslaniec
as code owners
- L2BTCRedeemerWormhole: pass stored l1BtcRedeemerWormholeChain to gateway call instead of caller-supplied recipientChain (defense-in-depth) - L2BTCRedeemerWormhole: add onlyOwner setter for l1BtcRedeemerWormholeChain to support future upgrades without redeploying the proxy - L2BTCRedeemerWormhole: add __gap storage reserve (OZ upgradeable pattern) - L1BTCDepositorNttWithExecutor: relax exact-match payment to >= with excess ETH refund to prevent price-drift reverts - L1BTCDepositorNttWithExecutor: remove redundant supportedChains require (already checked and reverted inside _getDestinationChainFromReceiver) - L1BTCDepositorNttWithExecutor: remove dead feeArgs.dbps <= MAX_BPS check (defaultExecutorFeeBps is always <= MAX_BPS via setter validation) - L1BTCDepositorNttWithExecutor: remove dead signedQuote.length > 0 check (subsumed by the >= 32 check below it)
- Add solhint-disable for avoid-low-level-calls on ETH refund - Update hardening test: underpayment reverts, overpayment refunds
- L2BTCRedeemerWormhole: test setL1BtcRedeemerWormholeChain (onlyOwner, zero-value revert, storage update, event emission) - L1BTCDepositorNttWithExecutor: verify NTT manager receives exactly requiredPayment (not msg.value) on overpayment; verify depositor retains no ETH after refund
piotr-roslaniec
previously approved these changes
May 6, 2026
- Reorder _transferTbtc so staged parameters are deleted before the external call; cache executorArgs/feeArgs locally so the post-clear call still sees the staged values. Prevents a reentrant caller (via the ETH overpayment refund inside _transferTbtcWithExecutor) from replaying staged params against a second deposit -- finalizeDeposit in AbstractL1BTCDepositor is not nonReentrant. - Move the ETH refund to the very last step of _transferTbtcWithExecutor (after the emit), giving the function checks-effects-interactions ordering instead of refunding before the NTT transfer. - Bind executorArgs.refundAddress == msg.sender at stage time in setExecutorParameters. Fee bps / payee equality remains enforced at finalize time so admins can rotate defaults without invalidating in-flight staged params. - Tighten the underpayment error string from "Incorrect" to "Insufficient" -- the only failure path now is underpayment. Tests: - Add MockRefundRejector, a forwarding contract that selectively refuses ETH only from the depositor, used to exercise the "ETH refund failed" branch. - Add hardening cases for non-zero fee equality (success + payee mismatch), ETH-refund-failure path, and stage-time refund-address binding. - Update existing executor / integration / scenarios / minimal-auto- nonce tests to override refundAddress to the staging caller; the canned EXECUTOR_ARGS_REAL_QUOTE address no longer satisfies the stage-time bind. All 478 wormhole tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
solidity/contracts/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.sol (1)
944-966:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmit the net charged amount instead of raw
msg.value.
executorCostis emitted asmsg.value, but Lines 960-966 refund any excess and onlyrequiredPaymentis actually forwarded tonttManagerWithExecutor. Overpaying callers will therefore produce inflated cost events for indexers and accounting. EmittingrequiredPaymenthere, or renaming the field to reflect “user supplied payment,” would keep the event semantically correct.🤖 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 `@solidity/contracts/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.sol` around lines 944 - 966, The TokensTransferredNttWithExecutor event currently emits msg.value which can be larger than the actual executor fee forwarded (requiredPayment) because excess is refunded; update the emit to report the net charged amount by replacing the msg.value argument with requiredPayment (or alternatively rename the event field to userSuppliedPayment if you want to preserve raw input), ensuring the emitted value matches what is actually forwarded to nttManagerWithExecutor and indexers will see the true executor cost.
🧹 Nitpick comments (1)
typescript/src/lib/sei/extra-data-encoder.ts (1)
14-16: ⚡ Quick winClarify the complete 32-byte encoding format in documentation.
The documentation states "first 2 bytes contain the Wormhole chain ID, followed by the 20-byte EVM-compatible recipient address," but doesn't mention that the address is left-padded with zeros to make the total 32 bytes (2-byte chain ID + 12-byte padding + 20-byte address).
📝 Suggested documentation enhancement
/** * Implementation of the ExtraDataEncoder for Sei chain. - * Encodes SeiEVM addresses as 32-byte values for use in Bitcoin deposit scripts. - * The first 2 bytes contain the Wormhole chain ID, followed by the 20-byte - * EVM-compatible recipient address. + * Encodes SeiEVM addresses as 32-byte receiver data for use in Bitcoin deposit scripts. + * Format: 2-byte Wormhole chain ID + 12-byte zero padding + 20-byte EVM address. */🤖 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 `@typescript/src/lib/sei/extra-data-encoder.ts` around lines 14 - 16, Update the doc comment in extra-data-encoder.ts to fully describe the 32-byte layout: state that the 2-byte Wormhole chain ID is followed by 12 bytes of zero left-padding and then the 20-byte EVM-compatible recipient address (total 32 bytes), e.g., "2-byte chain ID | 12 bytes zero padding | 20-byte address"; modify the existing comment block that starts "Encodes SeiEVM addresses as 32-byte values..." to include this exact padding detail so callers of the encoder (e.g., the function that produces the 32-byte buffer) know the byte ordering and padding convention.
🤖 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.
Inline comments:
In `@solidity/contracts/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.sol`:
- Around line 1029-1047: The staged vs finalized fee checks are inconsistent:
_validateExecutorParameters requires feeArgs.dbps == defaultExecutorFeeBps while
setExecutorParameters currently allows feeArgs.dbps >= defaultPlatformFeeBps,
which can let users stage parameters that will later always revert; fix this by
aligning the checks—update setExecutorParameters to require feeArgs.dbps ==
defaultExecutorFeeBps (and enforce feeArgs.payee == defaultExecutorFeeRecipient
when dbps != 0) so staging uses the same source of truth as
_validateExecutorParameters (alternatively, if you prefer the floor behavior,
change _validateExecutorParameters to require feeArgs.dbps >=
defaultPlatformFeeBps instead, but pick one approach and apply it to both
setExecutorParameters and _validateExecutorParameters).
In
`@solidity/test/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.executor.test.ts`:
- Around line 345-346: The positive test for setExecutorParameters may still
pass falsely because createExecutorArgs() used the default refundAddress; update
the test to set refundAddress equal to the caller (e.g., pin refundAddress in
executorArgs) so refund binding won't trigger a revert, and then assert full
success (not just not.revertedWith) by checking the transaction completes
without revert and emits the expected event or returns success; locate
createExecutorArgs, executorArgs, and the setExecutorParameters call and ensure
refundAddress is explicitly set to the caller and the assertion verifies overall
success.
---
Outside diff comments:
In `@solidity/contracts/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.sol`:
- Around line 944-966: The TokensTransferredNttWithExecutor event currently
emits msg.value which can be larger than the actual executor fee forwarded
(requiredPayment) because excess is refunded; update the emit to report the net
charged amount by replacing the msg.value argument with requiredPayment (or
alternatively rename the event field to userSuppliedPayment if you want to
preserve raw input), ensuring the emitted value matches what is actually
forwarded to nttManagerWithExecutor and indexers will see the true executor
cost.
---
Nitpick comments:
In `@typescript/src/lib/sei/extra-data-encoder.ts`:
- Around line 14-16: Update the doc comment in extra-data-encoder.ts to fully
describe the 32-byte layout: state that the 2-byte Wormhole chain ID is followed
by 12 bytes of zero left-padding and then the 20-byte EVM-compatible recipient
address (total 32 bytes), e.g., "2-byte chain ID | 12 bytes zero padding |
20-byte address"; modify the existing comment block that starts "Encodes SeiEVM
addresses as 32-byte values..." to include this exact padding detail so callers
of the encoder (e.g., the function that produces the 32-byte buffer) know the
byte ordering and padding convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 606c77c0-419a-4a4d-a8c1-1e65d73455cf
📒 Files selected for processing (15)
solidity/README.adocsolidity/contracts/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.solsolidity/contracts/cross-chain/wormhole/L2BTCRedeemerWormhole.solsolidity/contracts/test/MockL2WormholeGateway.solsolidity/contracts/test/MockRefundRejector.solsolidity/contracts/test/TestL1BTCDepositorNttWithExecutor.solsolidity/test/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.executor.test.tssolidity/test/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.hardening.test.tssolidity/test/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.integration.test.tssolidity/test/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.minimal-auto-nonce.test.tssolidity/test/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.scenarios.test.tssolidity/test/cross-chain/wormhole/L2BTCRedeemerWormhole.hardening.test.tssolidity/test/cross-chain/wormhole/L2BTCRedeemerWormhole.test.tstypescript/src/lib/sei/extra-data-encoder.tstypescript/test/lib/sei/extra-data-encoder.test.ts
Comment on lines
+1029
to
+1047
| require( | ||
| executorArgs.refundAddress == msg.sender, | ||
| "Executor refund address must be caller" | ||
| ); | ||
| require( | ||
| feeArgs.dbps == defaultExecutorFeeBps, | ||
| "Fee must match default executor fee" | ||
| ); | ||
|
|
||
| if (feeArgs.dbps == 0) { | ||
| require( | ||
| feeArgs.payee == address(0), | ||
| "Fee payee must be zero when fee is zero" | ||
| ); | ||
| } else { | ||
| require( | ||
| feeArgs.payee == defaultExecutorFeeRecipient, | ||
| "Fee payee must match default executor fee recipient" | ||
| ); |
There was a problem hiding this comment.
Align the staged and finalized fee invariants.
_validateExecutorParameters now requires feeArgs.dbps == defaultExecutorFeeBps, but setExecutorParameters still accepts anything >= defaultPlatformFeeBps at Line 487. If those two owner-configured values diverge, users can successfully stage parameters that are guaranteed to revert at finalize time. Please make both stages enforce the same source of truth, or validate the platform-fee floor here as well so the workflow cannot be bricked by config drift.
🤖 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 `@solidity/contracts/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.sol`
around lines 1029 - 1047, The staged vs finalized fee checks are inconsistent:
_validateExecutorParameters requires feeArgs.dbps == defaultExecutorFeeBps while
setExecutorParameters currently allows feeArgs.dbps >= defaultPlatformFeeBps,
which can let users stage parameters that will later always revert; fix this by
aligning the checks—update setExecutorParameters to require feeArgs.dbps ==
defaultExecutorFeeBps (and enforce feeArgs.payee == defaultExecutorFeeRecipient
when dbps != 0) so staging uses the same source of truth as
_validateExecutorParameters (alternatively, if you prefer the floor behavior,
change _validateExecutorParameters to require feeArgs.dbps >=
defaultPlatformFeeBps instead, but pick one approach and apply it to both
setExecutorParameters and _validateExecutorParameters).
Comment on lines
+345
to
346
| const executorArgs = createExecutorArgs({ refundAddress: owner.address }) | ||
| const invalidFeeArgs = { |
There was a problem hiding this comment.
Strengthen the max-BPS positive test to avoid false passes.
At Line 357, createExecutorArgs() still uses the default refundAddress; if it doesn’t match caller, setExecutorParameters may revert for refund binding while this assertion still passes (not.revertedWith only checks one reason). Please pin refundAddress there too and assert full success.
Suggested fix
- it("should accept maximum valid fee basis points (10000) in setExecutorParameters", async () => {
- const executorArgs = createExecutorArgs()
+ it("should accept maximum valid fee basis points (10000) in setExecutorParameters", async () => {
+ const executorArgs = createExecutorArgs({ refundAddress: owner.address })
const validFeeArgs = {
dbps: 10000, // Exactly 10% (10000/100000)
payee: ethers.Wallet.createRandom().address,
}
- // This should not revert (but will likely revert on quote validation)
- // We're testing that the BPS validation passes
- await expect(
- depositor.setExecutorParameters(executorArgs, validFeeArgs)
- ).to.not.be.revertedWith("Fee cannot exceed 100% (10000 bps)")
+ await expect(
+ depositor.setExecutorParameters(executorArgs, validFeeArgs)
+ ).to.not.be.reverted
})🤖 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
`@solidity/test/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.executor.test.ts`
around lines 345 - 346, The positive test for setExecutorParameters may still
pass falsely because createExecutorArgs() used the default refundAddress; update
the test to set refundAddress equal to the caller (e.g., pin refundAddress in
executorArgs) so refund binding won't trigger a revert, and then assert full
success (not just not.revertedWith) by checking the transaction completes
without revert and emits the expected event or returns success; locate
createExecutorArgs, executorArgs, and the setExecutorParameters call and ensure
refundAddress is explicitly set to the caller and the assertion verifies overall
success.
|
Actionable comments posted: 0 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@typescript/api-reference/classes/SeiBitcoinDepositor.md`:
- Around line 186-189: Update the Throws text in the SeiBitcoinDepositor docs to
reflect receiver-based validation: replace the stale line "Error if the address
is invalid" with a message that mentions the encoded receiver (e.g., "Error if
the encoded receiver is invalid" or "Error if receiver validation fails") so it
accurately describes validation performed by the SeiBitcoinDepositor method that
validates an encoded receiver.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 99a68e28-07cc-4014-9b9c-fca428e972d5
📒 Files selected for processing (11)
typescript/api-reference/README.mdtypescript/api-reference/classes/SeiBitcoinDepositor.mdtypescript/api-reference/classes/TBTC.mdtypescript/api-reference/interfaces/SeiBitcoinDepositorConfig.mdtypescript/src/lib/sei/index.tstypescript/src/lib/sei/sei-depositor.tstypescript/src/services/tbtc.tstypescript/test/lib/sei/index.test.tstypescript/test/lib/sei/sei-depositor.test.tstypescript/test/services/tbtc-sei.test.tstypescript/test/utils/mock-cross-chain-contracts-loader.ts
✅ Files skipped from review due to trivial changes (2)
- typescript/test/utils/mock-cross-chain-contracts-loader.ts
- typescript/api-reference/interfaces/SeiBitcoinDepositorConfig.md
Comment on lines
186
to
189
| **`Throws`** | ||
|
|
||
| Error if the address is invalid | ||
|
|
There was a problem hiding this comment.
Update the throw description to match receiver-based validation.
The method validates an encoded receiver, not a raw address. The current throw text is stale and can mislead users.
Suggested doc fix
-**`Throws`**
-
-Error if the address is invalid
+**`Throws`**
+
+Error if the encoded receiver is invalid🤖 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 `@typescript/api-reference/classes/SeiBitcoinDepositor.md` around lines 186 -
189, Update the Throws text in the SeiBitcoinDepositor docs to reflect
receiver-based validation: replace the stale line "Error if the address is
invalid" with a message that mentions the encoded receiver (e.g., "Error if the
encoded receiver is invalid" or "Error if receiver validation fails") so it
accurately describes validation performed by the SeiBitcoinDepositor method that
validates an encoded receiver.
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
2 participants
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
Validation
npx hardhat compile --no-size-contractspassednpx hardhat test test/cross-chain/wormhole/L2BTCRedeemerWormhole.hardening.test.ts test/cross-chain/wormhole/L1BTCDepositorNttWithExecutor.hardening.test.tspassed with 5 testsgit diff --checkpassed for the safe-fix batchNotes
This is public-safe guardrail hardening. The NTT path is not deployed, and this PR does not include any private deployment mitigation.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests