Add hooks disclaimers #529
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughDocumentation clarifies HooksTrampoline as a solver-facing reference helper (not a guaranteed fixed-address component), adds warnings about relying on its address or msg.sender, notes solvers may choose different implementations, and records that the trampoline forwards sufficient gas to execute hooks; minor README spacing fixes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Solver
participant Trampoline as HooksTrampoline (ref impl)
participant Hook as UserHook
participant Settlement
User->>Solver: Submit order with hook intent
Solver->>Settlement: Prepare settlement tx
alt Uses reference trampoline
Solver->>Trampoline: Call with hook data
note right of Trampoline #D6EAF8: Forwards sufficient gas to hooks
Trampoline->>Hook: execute(hookData)
Hook-->>Trampoline: returns
Trampoline-->>Solver: result
else Custom implementation
Solver->>Hook: execute(hookData) (custom)
Hook-->>Solver: returns
end
Solver->>Settlement: Finalize settlement
Settlement-->>User: Settlement outcome
note over Solver,Hook: Do not rely on a fixed trampoline address or on `msg.sender`
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
anxolin
requested review from
alfetopito,
fedgiac and
carlos-cow
and removed request for
alfetopito
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
docs/cow-protocol/reference/contracts/periphery/README.mdx (4)
5-5: Remove unintended extra blank line.Keeps spacing consistent with the rest of the page.
-
10-10: Fix typo: “determinsitic” → “deterministic”.Minor language polish in a prominent tip.
-CoW Protocol contracts are generally deployed to the same addresses on their respective deployment chains. This may be done either via using determinsitic deployments (`CREATE2`), or ensuring that the deployer uses a consistent `nonce` via `CREATE` for deployment. +CoW Protocol contracts are generally deployed to the same addresses on their respective deployment chains. This may be done either via using deterministic deployments (`CREATE2`), or ensuring that the deployer uses a consistent `nonce` via `CREATE` for deployment.
31-38: Sharpen the HooksTrampoline disclaimer and align with the guidance in hooks_trampoline.md.Good addition. Suggest tightening the copy, and elevating severity to “danger” since misuse can lead to broken assumptions in hooks.
-**Address**: `0x60bf78233f48ec42ee3f101b9a05ec7878728006` +**Address**: `0x60bf78233F48eC42eE3f101B9A05eC7878728006` -:::warning -You should not assume that the HooksTrampoline contract address will remain fixed. Each solver may choose to use this implementation or another one, as long as it fulfills the hook intent. -::: +:::danger +Do not assume the `HooksTrampoline` contract address is fixed or unique. Solvers may use this reference implementation or any alternative that fulfills the hook intent. Design hooks so they do not rely on a specific trampoline address or on `msg.sender` being a particular contract. See “Relying on the trampoline contract address” for details. +:::Note: also consider presenting the address everywhere in EIP‑55 checksum format for consistency (see Networks list below).
31-31: Normalize address format to EIP-55 checksumI ran a checksum validation across
docs/cow-protocol/reference/contracts/periphery/README.mdxand found three entries that need updating:
- Replace
0x60bf78233f48ec42ee3f101b9a05ec7878728006
with
0x60Bf78233f48eC42eE3F101b9a05eC7878728006- Replace
0x96dddac514d0799e34e3f642c5006852ad24cd68
with
0x96ddDAC514d0799e34e3F642c5006852aD24CD68- Replace
0xe84dcd8587287b997f51299430a396ad03aaec06
with
0xe84DCd8587287B997F51299430A396AD03aAEC06No other mismatches were detected—every other address in this file already conforms to EIP-55. After applying these updates, please verify that the Networks list remains consistent across all sections.
docs/cow-protocol/reference/contracts/periphery/hooks_trampoline.md (4)
8-8: Tighten opening sentence (grammar and clarity).Remove the comma, and use plural possessive.
-A helper contract used by solvers, to securely execute user's [hooks](/cow-protocol/reference/core/intents/hooks) with the settlement transaction. +A helper contract used by solvers to securely execute users’ [hooks](/cow-protocol/reference/core/intents/hooks) within the settlement transaction.
19-20: Improve parallelism and precision of the role description.Reads cleaner and more accurate about what’s being isolated/protected.
-Since solvers are responsible for any losses resulting from their settlement transactions, they execute hooks through an intermediary contract. The `HooksTrampoline` contract serves as a reference implementation that helps isolate the settlement contract and provide protection. +Since solvers are responsible for any losses resulting from their settlement transactions, they execute hooks through an intermediary contract. The `HooksTrampoline` contract is a reference implementation that helps isolate and protect the settlement contract.
69-73: Clarify solver choice and fix minor grammar.Keeps the message while tightening the language.
-### Relying on the trampoline contract address - -Most solvers will use the `HooksTrampoline` contract to execute hooks, as it offers strong security guarantees. However, the protocol does not mandate any specific implementation. In fact, solvers are not required to use an intermediary contract at all if they can ensure the security of their hooks by other means, they may do so and save gas. +### Relying on the trampoline contract address + +Most solvers use the `HooksTrampoline` contract because it offers strong security properties. However, the protocol does not mandate any specific implementation. Solvers are not required to use an intermediary at all; if they can ensure hook security by other means, they may do so to save gas.
74-79: Expand the warning with positive guidance for safe hook design.The warning is correct; add actionable recommendations and small copy edits.
-:::warning -Do not design hooks that rely on the caller (`msg.sender`) being a specific `HooksTrampoline` contract: - -1. **Not secure**: Such a check does not actually protect against third-party calls. Anyone can create an order that invokes your contract, and that call will still originate from a trampoline contract. -2. **Not reliable**: As noted above, the trampoline contract address can change at any time. -::: +:::warning +Do not design hooks that rely on the caller (`msg.sender`) being a specific `HooksTrampoline` contract: + +1. **Not secure**: Checking for a particular trampoline address does not prevent third‑party calls. Anyone can submit an order that invokes your contract, and that call can still originate from some trampoline contract. +2. **Not reliable**: As noted above, the trampoline contract address may change at any time. +3. **Prefer capability checks**: If a hook must restrict sensitive actions, use explicit authorization (e.g., `Ownable`/`AccessControl` roles), allowlists, or argument‑level authentication (e.g., verifying a signed intent specific to your contract state). +4. **Assume public callability**: Where possible, design hooks to be safe even if called by arbitrary actors (e.g., idempotent state updates, bounded effects). +:::
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
docs/cow-protocol/reference/contracts/periphery/README.mdx(2 hunks)docs/cow-protocol/reference/contracts/periphery/hooks_trampoline.md(4 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/cow-protocol/reference/contracts/periphery/hooks_trampoline.md
[grammar] ~74-~74: There might be a mistake here.
Context: ...hey may do so and save gas. :::warning Do not design hooks that rely on the cal...
(QB_NEW_EN)
| @@ -57,6 +57,7 @@ sequenceDiagram | |||
|
|
|||
| 1. The trampoline contract is not upgradable | |||
| 2. Hooks are only executed during the course of a settlement on CoW Protocol | |||
| 3. Enough gas is forwarded to the hooks to execute the logic | |||
There was a problem hiding this comment.
Guarantee 3 is overstated; solvers choose gas and it may be insufficient.
The trampoline enforces a per-hook gas limit; it cannot guarantee “enough gas.” Suggest reframing to describe behavior rather than a guarantee to avoid misleading integrators.
-3. Enough gas is forwarded to the hooks to execute the logic
+3. A per-hook `gasLimit` is enforced and forwarded to each hook call. Solvers SHOULD provision sufficient gas for the intended logic; hooks MUST handle potential out‑of‑gas or partial execution gracefully.Optionally, annotate the sequence diagram calls as call (gasLimit) to reflect the behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 3. Enough gas is forwarded to the hooks to execute the logic | |
| 3. A per-hook `gasLimit` is enforced and forwarded to each hook call. Solvers SHOULD provision sufficient gas for the intended logic; hooks MUST handle potential out-of-gas or partial execution gracefully. |
🤖 Prompt for AI Agents
In docs/cow-protocol/reference/contracts/periphery/hooks_trampoline.md around
line 60, the statement "Enough gas is forwarded to the hooks to execute the
logic" overstates behavior because the trampoline enforces a per-hook gas limit
set by the solver and cannot guarantee sufficiency; reword the sentence to
describe that a per-hook gas limit is forwarded (so hooks receive up to that
gas) and amend the sequence diagram calls to annotate them like "call
(gasLimit)" or similar to reflect that gas is limited rather than guaranteed.
docs/cow-protocol/reference/contracts/periphery/hooks_trampoline.md
Outdated
Show resolved
Hide resolved
docs/cow-protocol/reference/contracts/periphery/hooks_trampoline.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Federico Giacon <[email protected]>
Co-authored-by: Federico Giacon <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/cow-protocol/reference/contracts/periphery/hooks_trampoline.md (1)
60-60: Do not promise “enough gas”; describe enforced per‑hook gas limits.As previously noted, this overstates behavior: the trampoline forwards a per‑hook gasLimit chosen by the solver; it cannot guarantee sufficiency. Rephrase accordingly and consider annotating the mermaid diagram calls with “(gas: gasLimit)”.
-3. Enough gas is forwarded to the hooks to execute the logic +3. A per‑hook `gasLimit` is enforced and forwarded to each hook call. Solvers should provision sufficient gas for the intended logic, and hooks must tolerate out‑of‑gas or partial execution gracefully.Optional diagram tweak (not shown in the diff above): change “HooksTrampoline->>Hook: call” lines to “call (gas:
gasLimit)”.
🧹 Nitpick comments (8)
docs/cow-protocol/reference/contracts/periphery/hooks_trampoline.md (8)
8-8: Fix plural possessive and tighten phrasing.Use “users’ hooks” (plural possessive) and keep tone neutral.
-A helper contract that may be used by solvers to securely execute user's [hooks](/cow-protocol/reference/core/intents/hooks) within the settlement transaction. +A helper contract that may be used by solvers to securely execute users’ [hooks](/cow-protocol/reference/core/intents/hooks) within the settlement transaction.
19-19: Clarify responsibility statement and improve parallelism.Slightly soften “responsible for any losses” (jurisdictional nuances) and improve sentence cadence.
-Since solvers are responsible for any losses resulting from their settlement transactions, they execute hooks through an intermediary contract. The `HooksTrampoline` contract serves as a reference implementation that helps isolate the settlement contract and provide protection. +Since solvers bear the risk associated with their settlement transactions, they often execute hooks via an intermediary contract. `HooksTrampoline` is a reference implementation that isolates the settlement contract and reduces blast radius.If “bear the risk” is too strong/weak for your legal stance, I can tune the wording (e.g., “assume operational risk”). Want me to align this with your formal risk docs?
69-69: Heading: broaden scope to include msg.sender.The section discusses both address stability and msg.sender reliance; let’s make the heading reflect that.
-### Relying on the trampoline contract address +### Avoid relying on a specific trampoline address or `msg.sender`
71-71: Minor grammar/punctuation fix; optional style polish.Replace the comma splice with a semicolon; keep “may use” language.
-Solvers may use the `HooksTrampoline` contract to execute hooks, as it offers a pragmatic way to meet many of the security guarantees required of hook execution while still keeping the settlement submission logic relatively simple. However, the protocol does not mandate any specific implementation. In fact, solvers are not required to use an intermediary contract at all if they can ensure the security of their hooks by other means, they may do so and save gas. +Solvers may use the `HooksTrampoline` contract to execute hooks, as it offers a pragmatic way to meet many of the security guarantees required of hook execution while keeping settlement submission relatively simple. However, the protocol does not mandate any specific implementation. In fact, solvers are not required to use an intermediary contract at all; if they can ensure the security of hook execution by other means, they may do so and save gas.
75-79: Strengthen the warning with actionable guidance and precise rationale.Tighten bullets and point readers to safer authorization patterns.
-Do not design hooks that rely on the caller (`msg.sender`) being a specific `HooksTrampoline` contract: - -1. **Not secure**: Such a check does not actually protect against third-party calls. Anyone can create an order that invokes your contract, and that call will still originate from a trampoline contract. -2. **Not reliable**: As noted above, the trampoline contract address can change at any time. +Do not design hooks that rely on the caller (`msg.sender`) being a specific `HooksTrampoline` contract: + +1. **Not secure**: Authorization by `msg.sender` does not prevent unwanted invocation. Anyone can craft an order that routes through a trampoline to your contract. Prefer explicit validation (e.g., checking expected calldata/intended recipient amounts, EIP‑712 signatures you verify, or an allowlist) over caller address checks. +2. **Not reliable**: There is no protocol‑level guarantee that a particular trampoline address will be used. Solvers may rotate or deploy different trampoline implementations over time and across networks.If helpful, I can add a short “Tip” block after this with recommended hook design practices (idempotency, explicit value checks, reentrancy guards, gas‑aware logic).
62-67: Consider tightening the warning block language.Explicitly mention native ETH and ERC‑20 allowances to avoid ambiguity.
-* Beware of leaving any funds in the trampoline contract. These are accessible to anyone. -* Do **NOT** grant any permissions to the trampoline contract. These are accessible to anyone. +* Beware of leaving any funds (native ETH or tokens) in the trampoline contract; anyone can withdraw them. +* Do **NOT** grant token allowances or other permissions to the trampoline contract; they are effectively accessible to anyone.
23-54: Optional diagram enhancement for fidelity.Annotate hook calls with gas info and show revert containment to emphasize isolation.
- Add “(gas:
gasLimit)” to the Hook call arrows.- Optionally note “return/revert (contained)” to signal that reverts don’t bubble to Settlement directly.
6-6: Overall: strong clarifications; happy to batch‑apply edits.The disclaimers meaningfully reduce integrator footguns. If you’d like, I can push a commit with the diffs above and propagate the heading/wording consistency to the Periphery README section too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
docs/cow-protocol/reference/contracts/periphery/hooks_trampoline.md(4 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/cow-protocol/reference/contracts/periphery/hooks_trampoline.md
[grammar] ~74-~74: There might be a mistake here.
Context: ...hey may do so and save gas. :::warning Do not design hooks that rely on the cal...
(QB_NEW_EN)
🔇 Additional comments (1)
docs/cow-protocol/reference/contracts/periphery/hooks_trampoline.md (1)
88-94: Nice:gasLimitfield aligns with the clarified invariant.Good to see the per‑hook limit modeled explicitly; this pairs well with the revised invariant suggestion above.
Description
This PR adds some disclaimers and useful information related to the hooks trampoline contract.
I'm not good with texts, please, suggest any changes you see fit.
Periphery
Adds a disclaimer (warning message below)
Do not rely on msg.sender being the trampoline
Adds this section:
Forwarded gas
Added point 3 here:
Changed the HooksTrampoline intro
Before:
After
Test
Summary by CodeRabbit