Skip to content

Conversation

@anajuliabit
Copy link
Collaborator

@anajuliabit anajuliabit commented Oct 23, 2025

This pull request introduces a new validation hook for bridge actions in proposals and integrates it into the HybridProposal contract. The main goal is to ensure that any bridge operation in a proposal sends an appropriate amount of native tokens relative to the actual bridge cost, improving security and consistency. Additionally, the changes update inheritance and override logic to support this new validation alongside existing market creation checks.

New Bridge Validation Logic and Integration:

  • Added the new BridgeValidationHook contract, which validates bridgeToRecipient calls in proposals to ensure the native value sent is between 5x and 10x the router's bridge cost for the target chain. This includes utility functions for calldata parsing and error messaging. (proposals/hooks/BridgeValidationHook.sol)
  • Updated the HybridProposal contract to inherit from both MarketCreationHook and the new BridgeValidationHook, and to override _verifyActionsPreRun so both validations are performed for each proposal. (proposals/proposalTypes/HybridProposal.sol) [1] [2]
  • Added the import for BridgeValidationHook in HybridProposal.sol to enable its usage. (proposals/proposalTypes/HybridProposal.sol)

Inheritance and Override Adjustments:

  • Made the _verifyActionsPreRun and bytesToBytes4 functions in MarketCreationHook virtual, allowing them to be properly overridden in derived contracts such as HybridProposal. (proposals/hooks/MarketCreationHook.sol) [1] [2]
  • Overrode bytesToBytes4 in HybridProposal to resolve diamond inheritance, delegating to the implementation in MarketCreationHook. (proposals/proposalTypes/HybridProposal.sol)

These changes collectively improve proposal validation by ensuring bridge actions are properly checked for value correctness and maintain compatibility with existing market creation checks.

@openzeppelin-code
Copy link

openzeppelin-code bot commented Oct 23, 2025

add bridge validation hook

Generated at commit: c792c087e9506d2b114429c1e1184f70d8fda158

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
4
4
0
12
46
66
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@anajuliabit anajuliabit requested a review from Copilot October 24, 2025 15:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request introduces a new bridge validation mechanism to ensure that bridge operations in governance proposals send an appropriate amount of native tokens (between 5x and 10x the actual bridge cost) to prevent underfunding or excessive overpayment.

Key changes:

  • Added BridgeValidationHook contract to validate bridgeToRecipient calls in proposals
  • Integrated the new validation hook into HybridProposal alongside existing market creation checks
  • Modified MarketCreationHook to support inheritance with virtual function declarations

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
proposals/hooks/BridgeValidationHook.sol New validation hook that checks bridge operation values against router costs
proposals/proposalTypes/HybridProposal.sol Integrated both MarketCreationHook and BridgeValidationHook with proper override handling
proposals/hooks/MarketCreationHook.sol Made functions virtual to enable proper inheritance
test/integration/BridgeValidationHookIntegration.t.sol Integration tests covering valid/invalid bridge values and boundary conditions

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@@ -0,0 +1,168 @@
pragma solidity 0.8.19;
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing SPDX license identifier. Add // SPDX-License-Identifier: GPL-3.0-or-later at the top of the file to match the project's license convention.

Copilot uses AI. Check for mistakes.
Comment on lines +114 to +116
// Skip 32 bytes (array length) + 4 bytes (selector) + 64 bytes (first two params)
// = 100 bytes total, so we load from position 68 after the length prefix
let dataPointer := add(add(input, 0x20), 0x44) // 0x20 (32) + 0x44 (68) = 100
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect offset calculation. The comment states the total is 100 bytes, but 0x20 + 0x44 = 0x64 which equals 100 in decimal. However, the code adds these offsets to input, which already points to the data. The correct offset from the start of the data should be 0x44 (68 bytes) to skip selector (4 bytes) + first two params (64 bytes). The addition of 0x20 accounts for the length prefix, so the final pointer is at position 100 from the start of the memory location, but only 68 bytes into the actual calldata. This works correctly but the inline comment is confusing as it suggests 0x20 + 0x44 = 100 in hex.

Suggested change
// Skip 32 bytes (array length) + 4 bytes (selector) + 64 bytes (first two params)
// = 100 bytes total, so we load from position 68 after the length prefix
let dataPointer := add(add(input, 0x20), 0x44) // 0x20 (32) + 0x44 (68) = 100
// To reach the third parameter (uint16 wormholeChainId), skip:
// - 0x20 (32 bytes) for the length prefix
// - 0x04 (4 bytes) for the function selector
// - 0x40 (64 bytes) for the first two parameters
// Total offset: 0x20 + 0x04 + 0x40 = 0x64 (100 decimal) bytes from the start of the bytes array in memory.
// Since input points to the start of the bytes array, add 0x20 for the length prefix, then 0x44 (68 decimal) to reach the third parameter.
let dataPointer := add(add(input, 0x20), 0x44) // 0x20 (32) + 0x44 (68) = 100 (decimal)

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,543 @@
//SPDX-License-Identifier: GPL-3.0-or-later
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing SPDX license identifier. Add // SPDX-License-Identifier: GPL-3.0-or-later at the top of the file to match the project's license convention.

Suggested change
//SPDX-License-Identifier: GPL-3.0-or-later
// SPDX-License-Identifier: GPL-3.0-or-later

Copilot uses AI. Check for mistakes.
- Remove intermediate array allocations for gas savings
- Add router contract validation using address.code.length
- Add zero bridge cost validation
- Add tests for router and zero cost validation
- Simplify code by removing _verifyBridgeCalls function

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@anajuliabit anajuliabit marked this pull request as ready for review October 24, 2025 17:06
- Mark BridgeValidationHook and MarketCreationHook as abstract
- Make bytesToBytes4 virtual with no implementation in hooks
- Provide single implementation in HybridProposal
- Update TestHook to implement bytesToBytes4
- Eliminates code duplication across hooks

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
imthatcarlos
imthatcarlos previously approved these changes Oct 24, 2025
Copy link
Contributor

@imthatcarlos imthatcarlos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk if copilot is right, but you have integration tests and if the values are extracted correctly then all good 👍

LGTM

- Rename MarketCreationHook._verifyActionsPreRun to _verifyMarketCreationActions
- Remove virtual keyword (no longer needed)
- Update HybridProposal to call renamed function directly
- Remove override annotation from HybridProposal._verifyActionsPreRun
- Update comments for clarity
- Simplifies inheritance by eliminating virtual/override pattern

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
anajuliabit and others added 2 commits October 24, 2025 14:25
- Update moonbeam-integration.yml to match IntegrationTest contracts
- Keep BridgeValidationHookIntegrationTest suffix (not MoonbeamTest)
- Unit test already covered by UnitTest pattern
- Integration test now covered by MoonbeamTest|IntegrationTest pattern

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
imthatcarlos
imthatcarlos previously approved these changes Oct 24, 2025
@anajuliabit anajuliabit merged commit 347c6ce into main Oct 30, 2025
24 of 25 checks passed
@anajuliabit anajuliabit deleted the feat/bridge-validation branch October 30, 2025 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants