Conversation
|
@coderabbitia review |
|
You are on the CodeRabbit Free Plan. In order to use the Chat feature, please upgrade to CodeRabbit Pro. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update restructures the project from a basic Foundry template to a specialized proof-of-concept for a swap-and-bridge helper contract using CoW Protocol and Omnibridge. It introduces new Solidity contracts, interfaces, tests, and configuration, while removing the original Counter example and its related scripts and tests. Documentation and licensing are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Proxy (COWShed)
participant OmnibridgeApproveAndBridge
participant Omnibridge
User->>Proxy: Transfer tokens (e.g., WETH)
User->>Proxy: Delegatecall approveAndBridge(token, minAmount, receiver)
Proxy->>OmnibridgeApproveAndBridge: approveAndBridge(token, minAmount, receiver)
OmnibridgeApproveAndBridge->>token: forceApprove(Omnibridge, amount)
OmnibridgeApproveAndBridge->>Omnibridge: relayTokens(token, receiver, amount)
Omnibridge-->>User: TokensBridgingInitiated event
Poem
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (15)
LICENSE (1)
8-675: Assess copyleft implications.
The full GPLv3 text is properly included, but GPLv3’s strong copyleft may impose downstream distribution obligations on all recipients of this proof-of-concept. For a helper/library contract intended for broader adoption, consider whether a more permissive license (e.g., LGPL or MIT) would better align with your usage and integration goals.test/e2e/lib/ForkedRpc.sol (1)
8-15: Surface the fallback RPC choice or make it configurableSilently defaulting to
https://eth.merkle.iomay:
- Hide mis-configuration (a typo in
MAINNET_ARCHIVE_NODE_URLstill falls back).- Push load to a third-party endpoint that may rate-limit tests.
Consider reverting when the env var is unset or, at minimum, emitting a Forge log so it’s obvious which RPC is in use.
- } catch { - forkUrl = "https://eth.merkle.io"; + } catch { + // Revert early to make mis-configuration explicit. + revert("MAINNET_ARCHIVE_NODE_URL not set"); }src/interface/IOmnibridge.sol (1)
2-2: Same pragma concern as abovePin a minor compiler version for determinism.
-pragma solidity ^0.8; +pragma solidity ^0.8.24;src/interface/IApproveAndBridge.sol (1)
2-2: Pin compiler minor versionSee comment in
ForkedRpc.sol.-pragma solidity ^0.8; +pragma solidity ^0.8.24;script/Deploy.s.sol (2)
4-4: Remove unused import
consoleisn’t referenced. Dropping it avoids an unnecessary byte-code inclusion once solidity inlines libraries.-import {Script, console} from "forge-std/Script.sol"; +import {Script} from "forge-std/Script.sol";
14-18: Broadcast only when required & handle non-mainnet runsIf this script is executed against a fork (chain id ≠ 1) nothing happens, which can be confusing in CI.
Two lightweight improvements:
- Add an
elsebranch with a Forge log so users know the deployment was skipped.- Wrap deployment between
vm.startBroadcast()/vm.stopBroadcast()for multi-tx future extensions.- if (block.chainid == 1) { - vm.broadcast(); - omnibridgeApproveAndBridge = new OmnibridgeApproveAndBridge(MAINNET_OMNIBRIDGE); - } + if (block.chainid == 1) { + vm.startBroadcast(); + omnibridgeApproveAndBridge = new OmnibridgeApproveAndBridge(MAINNET_OMNIBRIDGE); + vm.stopBroadcast(); + } else { + console2.log("DeployScript: skipped – not on mainnet"); + }foundry.toml (1)
11-12: 1 000 000 optimizer runs → extremely slow buildsRuns in the seven-digit range barely change gas compared with 200k but explode compile time (and CI minutes).
Unless you have measured gas savings that justify it, consider reducing to e.g.200_000.README.md (1)
40-44: Deployment command lacks broadcast / verify flagsMost teams eventually turn the PoC into prod and will ask “why didn’t
forge script …verify / broadcast?”.
Adding--broadcastand--verifyexamples (commented-out is fine) will save future friction.src/mixin/ApproveAndBridge.sol (1)
14-16: Error message could expose internal logic
"Bridging less than min amount"gives an attacker a hint that the contract keeps balance > min.
For production consider a generic message like"Insufficient balance".test/OmnibridgeApproveAndBridge.t.sol (2)
8-15: Variable namecounteris a leftover from old exampleOmnibridgeApproveAndBridge public counter;The helper is not a counter. Rename to something descriptive (e.g.
bridgeHelper) to avoid confusion in future assertions.- OmnibridgeApproveAndBridge public counter; + OmnibridgeApproveAndBridge public bridgeHelper;
12-15: No actual tests yet – CI will silently succeed
setUp()is fine, but withoutfunction testSomething() external { … }forge marks the suite as passed even if nothing is asserted.
Add at least one assertion (e.g. verifyingbridgeApprovalTarget()returns the mocked address) to prevent false confidence.test/e2e/OmnibridgeApproveAndBridge.t.sol (2)
86-96: Event check misses the success path in thenotEnoughFundstestIn the revert-then-top-up flow you call
trustedExecuteHooksa second
time (line 132) but do not assert that the expected
TokensBridgingInitiatedevent is eventually emitted. Adding a second
expectEmitmakes the negative & positive cases symmetric and prevents
false positives where the hook silently does nothing after the top-up.
115-133: Duplicate setup code – extract a helper
notEnoughFunds()repeats ~20 lines already present intest_happyPath
(set-up fork, deploy shed, allocate calls). Extracting a private
_prepareShed()function keeps both tests short and focused on the
behaviour they’re validating.src/vendored/SafeERC20.sol (2)
39-46:forceApproveswallows a failing first call but not a second – consider bubbling
_callOptionalReturnBoolconverts any revert intofalse, so
forceApprovegracefully retries withapprove(0).
However, the second & third low-level calls use_callOptionalReturn
which reverts on failure, bubbling the original reason. That’s fine for
most tokens but breaks the “always succeed” contract the wrapper tries
to provide. Consider wrapping the second call in the same
_callOptionalReturnBoolpattern or documenting that an unexpected
second failure will bubble.
59-69: Memory clobbering risk when readingreturnValueBoth low-level helpers write call return data directly to memory slot 0
and immediately read it withmload(0).
While this mirrors OZ’s implementation, it also overwrites whatever was
stored at address 0, which may break contracts relying on that memory
(for example, inline assembly utilities that reuse slot 0 as scratch).A minimal cost improvement is to switch to the free-memory pointer:
- let success := call(gas(), token, 0, add(data, 0x20), mload(data), 0, 0x20) + let free := mload(0x40) + let success := call(gas(), token, 0, add(data, 0x20), mload(data), free, 0x20) ... - returnValue := mload(0) + returnValue := mload(free)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
LICENSE(1 hunks)README.md(1 hunks)foundry.toml(1 hunks)script/Counter.s.sol(0 hunks)script/Deploy.s.sol(1 hunks)src/Counter.sol(0 hunks)src/OmnibridgeApproveAndBridge.sol(1 hunks)src/interface/IApproveAndBridge.sol(1 hunks)src/interface/IOmnibridge.sol(1 hunks)src/mixin/ApproveAndBridge.sol(1 hunks)src/vendored/IERC20.sol(1 hunks)src/vendored/SafeERC20.sol(1 hunks)test/Counter.t.sol(0 hunks)test/OmnibridgeApproveAndBridge.t.sol(1 hunks)test/e2e/OmnibridgeApproveAndBridge.t.sol(1 hunks)test/e2e/lib/ForkedRpc.sol(1 hunks)
💤 Files with no reviewable changes (3)
- src/Counter.sol
- script/Counter.s.sol
- test/Counter.t.sol
🔇 Additional comments (5)
LICENSE (1)
1-7: Approve GPLv3 license header.
The header correctly declares “GNU GENERAL PUBLIC LICENSE Version 3, 29 June 2007” and includes the necessary copyright notice.src/vendored/IERC20.sol (1)
1-85: Vendored code looks untouched – no issues from this difffoundry.toml (1)
21-22:isolate = truemight be ignored in profile contextThe
isolatetest flag belongs under the[profile.*]table infoundry.toml.
Keeping it at root level may be silently ignored by forge ≥ 0.2.0. Make sure it is read by moving it inside[profile.default]or[test].README.md (1)
1-14: Great to see “proof-of-concept” caveats up-frontClear disclaimers reduce the chance that someone ships the PoC to production inadvertently.
src/vendored/SafeERC20.sol (1)
71-73:⚠️ Potential issue
address(token).code.length == 0silently allows EOAsIf the token is accidentally an EOA,
_callOptionalReturnsilently
accepts a zero-length return with the EOA check passing (== 0). This
masks an obvious bug. The original OZ code treatscode.length == 0
call as failure. The condition should be!= 0:- if (returnSize == 0 ? address(token).code.length == 0 : returnValue != 1) + if (returnSize == 0 ? address(token).code.length != 0 : returnValue != 1)Otherwise, a typo in the token address could go unnoticed.
Likely an incorrect or invalid review comment.
| @@ -0,0 +1,16 @@ | |||
| // SPDX-License-Identifier: GPL-3.0-or-later | |||
| pragma solidity ^0.8; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Lock-in a concrete compiler minor version
pragma solidity ^0.8; floats across every 0.8.x release. A new minor release could introduce subtle behaviour changes (e.g. new defaults for overflow checks on bytes.concat, updated Yul IR) that alter byte-code or runtime semantics and break reproducible builds.
Pin to the latest audited minor used in the repo (e.g. ^0.8.24) or even an exact version if determinism is paramount.
-pragma solidity ^0.8;
+pragma solidity ^0.8.24;📝 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.
| pragma solidity ^0.8; | |
| -pragma solidity ^0.8; | |
| +pragma solidity ^0.8.24; |
🤖 Prompt for AI Agents
In test/e2e/lib/ForkedRpc.sol at line 2, the pragma solidity version is set to a
floating minor version (^0.8), which can lead to inconsistent builds due to
changes in minor releases. Update the pragma statement to pin to a specific
minor version, such as ^0.8.24 or the exact version used and audited in the
repo, to ensure deterministic and reproducible builds.
| # Compiler options | ||
| solc = "0.8.30" | ||
| evm_version = "prague" # https://docs.soliditylang.org/en/latest/using-the-compiler.html#target-options | ||
| bytecode_hash = "none" | ||
| cbor_metadata = false | ||
| optimizer = true | ||
| optimizer_runs = 1000000 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify evm_version = "prague" is actually supported by solc 0.8.30
prague is still an upcoming hard-fork. solc 0.8.30 may not recognise it, causing compilation warnings or silently falling back to the default EVM version, which defeats the purpose of pinning.
Please double-check that the compiler in your CI image supports this tag and upgrade/pin accordingly.
🤖 Prompt for AI Agents
In foundry.toml around lines 6 to 13, the evm_version is set to "prague" which
may not be supported by solc 0.8.30, potentially causing compilation warnings or
fallback behavior. Verify if solc 0.8.30 in your CI environment supports
"prague"; if not, update the solc version to one that does or change evm_version
to a supported value to ensure consistent compilation behavior.
| constructor(IOmnibridge omnibridge_) { | ||
| omnibridge = omnibridge_; | ||
| } |
There was a problem hiding this comment.
Guard against zero-address Omnibridge in constructor
If someone accidentally passes IOmnibridge(address(0)), approvals will later fail silently. Add an explicit check for clarity.
constructor(IOmnibridge omnibridge_) {
- omnibridge = omnibridge_;
+ require(address(omnibridge_) != address(0), "Zero omnibridge");
+ omnibridge = omnibridge_;
}📝 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.
| constructor(IOmnibridge omnibridge_) { | |
| omnibridge = omnibridge_; | |
| } | |
| constructor(IOmnibridge omnibridge_) { | |
| require(address(omnibridge_) != address(0), "Zero omnibridge"); | |
| omnibridge = omnibridge_; | |
| } |
🤖 Prompt for AI Agents
In src/OmnibridgeApproveAndBridge.sol around lines 12 to 14, the constructor
assigns the omnibridge address without validation, which can lead to silent
failures if a zero address is passed. Add a require statement to check that the
omnibridge_ parameter is not the zero address before assignment, reverting the
transaction with a clear error message if it is zero.
| pragma solidity ^0.8; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin pragma to the same minor as foundry.toml
pragma solidity ^0.8; picks up future compiler versions with breaking semantic-checker changes.
Since the repo pins solc = "0.8.30" it’s safer to align the pragma to ^0.8.30 (or =0.8.30) to guarantee identical byte-code across machines.
-pragma solidity ^0.8;
+pragma solidity ^0.8.30;📝 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.
| pragma solidity ^0.8; | |
| pragma solidity ^0.8.30; |
🤖 Prompt for AI Agents
In src/OmnibridgeApproveAndBridge.sol at lines 2 to 3, the Solidity pragma is
set to ^0.8, which allows any 0.8.x compiler version and may cause
inconsistencies. Update the pragma to ^0.8.30 or =0.8.30 to match the solc
version specified in foundry.toml, ensuring consistent compilation and bytecode
across environments.
| /// @dev This function isn't intended to be called directly, it should be | ||
| /// delegatecalled instead. | ||
| function approveAndBridge(IERC20 token, uint256 minAmount, address receiver) external { | ||
| uint256 balance = token.balanceOf(address(this)); | ||
| require(balance >= minAmount, "Bridging less than min amount"); | ||
|
|
||
| token.forceApprove(bridgeApprovalTarget(), balance); | ||
|
|
||
| bridge(token, balance, receiver); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential re-entrancy between forceApprove and bridge
forceApprove invokes an external call on any ERC-20; a malicious token could re-enter approveAndBridge before the bridge call executes.
Even though no mutable state is written, child contracts overriding bridge() might be vulnerable.
Mitigation: add a simple ReentrancyGuard modifier or an internal status flag.
🤖 Prompt for AI Agents
In src/mixin/ApproveAndBridge.sol around lines 11 to 20, the approveAndBridge
function calls forceApprove on an external token before calling bridge, which
can lead to re-entrancy if the token is malicious and bridge is overridden in
child contracts. To fix this, add a ReentrancyGuard modifier or implement an
internal status flag to prevent re-entrant calls during the execution of
approveAndBridge, ensuring the function cannot be re-entered before it
completes.
| uint256 orderProceeds = 1_337 ether; | ||
| uint256 minProceeds = 42 ether; | ||
| assertGt(orderProceeds, minProceeds); | ||
|
|
||
| // For simplicity we take the funds from the user, but they should come | ||
| // from an order. | ||
| vm.prank(user); | ||
| WETH.transfer(address(shed), orderProceeds); | ||
|
|
There was a problem hiding this comment.
Test depends on live mainnet balances – make it deterministic
The fork assumes that user still holds ≥ 1 337 WETH at block
22689062. Should that balance ever drop (e.g. a future airdrop,
migration, or account sweep), the test will revert.
Foundry provides vm.deal / deal helpers; use them to mint the exact
amount needed and keep the test hermetic:
- vm.prank(user);
- WETH.transfer(address(shed), orderProceeds);
+ deal(address(WETH), user, orderProceeds);
+ vm.prank(user);
+ WETH.transfer(address(shed), orderProceeds);📝 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.
| uint256 orderProceeds = 1_337 ether; | |
| uint256 minProceeds = 42 ether; | |
| assertGt(orderProceeds, minProceeds); | |
| // For simplicity we take the funds from the user, but they should come | |
| // from an order. | |
| vm.prank(user); | |
| WETH.transfer(address(shed), orderProceeds); | |
| uint256 orderProceeds = 1_337 ether; | |
| uint256 minProceeds = 42 ether; | |
| assertGt(orderProceeds, minProceeds); | |
| // For simplicity we take the funds from the user, but they should come | |
| // from an order. | |
| - vm.prank(user); | |
| - WETH.transfer(address(shed), orderProceeds); | |
| + deal(address(WETH), user, orderProceeds); | |
| + vm.prank(user); | |
| + WETH.transfer(address(shed), orderProceeds); |
🤖 Prompt for AI Agents
In test/e2e/OmnibridgeApproveAndBridge.t.sol around lines 68 to 76, the test
relies on the user having a live mainnet WETH balance of at least 1,337 ether,
which can cause flakiness if the balance changes. To fix this, use Foundry's
vm.deal or deal helper to set the user's WETH balance explicitly to the required
amount before the transfer, ensuring the test is deterministic and does not
depend on external state.
| return address(omnibridge); | ||
| } | ||
|
|
||
| function bridge(IERC20 token, uint256 amount, address receiver) internal override { |
There was a problem hiding this comment.
nit: I would add minReceiveAmount, because the amount to bridge will be likely bigger. This should be used just for the premature-execution assertion
There was a problem hiding this comment.
This function is a helper used by the contract that inherits ApproveAndBridge to explain how bridging works in the specific implementation (and so get the approveAndBridge function for free).
We can change the parameters if needed (to make it more generic, we could add a bytes array) but a minReceiveAmount isn't part of the bridging operation per se, it only needs to appear in approveAndBridge (where it does).
|
|
||
| vm.forkEthereumMainnetAtBlock(MAINNET_FORK_BLOCK); | ||
| approveAndBridge = new OmnibridgeApproveAndBridge(OMNIBRIDGE); | ||
| receiver = makeAddr("E2EOmnibridgeApproveAndBridgeTest: receiver"); |
There was a problem hiding this comment.
nit: if this is an alias, feels a bit long :)
There was a problem hiding this comment.
The reason it's so long is that I use the format "$indentifier: $short name" unless the function is in the body of a test. This is to avoid that a generic name for an address is reused in both the setup and the body of the test for different purposes (and so add some hidden assumptions).
Note that calling makeAddr("label") twice returns the same address.
| shed.trustedExecuteHooks(calls); | ||
|
|
||
| // The same hook can still be used if more funds become available | ||
| vm.prank(user); |
There was a problem hiding this comment.
ubernit: we could assert the nonce has not been consumed. I think you do that already by executing the calls
There was a problem hiding this comment.
Mmm, actually, you don't deal with the nonce cause you execute the call directly. Ideally we should do a test of pre-mature execution using signed hooks, but you don't need to do that, you've done more than enough! 🙏
There was a problem hiding this comment.
You're absolutely right, I just wanted to have the test ready earlier. 🙂
|
Results of testing the AI code reviewer. I'll split the comments in three groups: (1) good, addressed, where I actually made a change (2) good, unaddressed, where I think it's ok to point them out but for one reason or another I still think it's better to not make a change, and (3) not good. Obviously imho.
|
…er-native feat: native token
|
Closing, although I find this PR useful both as a reference, and also as a more efficient way to bridge using native bridges. Instead of using an intermediate contract (like socket), uses the native bridge. I would love we could reconsider cleaning it up and merging if one day the FE is willing to also do their part in the UI. But I think is not worth it yet, because we need to also keep the status of the execution in the SDK, which should not be hard, but probably is not something to work now for them |
3 participants
A first draft for a helper contract for the swap and bridge feature.
It was written so that it requires minimum effort to migrate the Omnibridge code from the SDK away from Weiroll in favor of
OmnibridgeApproveAndBridge.It's a proof of concept and not intended for production use at this point.
Notably, it solves two issues we discussed in the smart-contract team:
How to test/understand
See the forked e2e test on mainnet.
Summary by CodeRabbit