Feat/celer rfq#10
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTwo new executor contracts are introduced: ChangesRFQVaultExecutor Contract, Deployment, and Tests
CelerExecutor Contract and Deployment
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 100, 200, 0.5)
note right of Solver: RFQVaultExecutor flow
Caller->>RFQVaultExecutor: receiveNative(quoteId) + ETH
Solver->>RFQVaultExecutor: swapAndFulfil(quoteId, nonce, ..., action, sig)
RFQVaultExecutor->>RFQVaultExecutor: _verifyAndMarkQuoteId (sig check + replay guard)
RFQVaultExecutor->>SwapTarget: _performAction (raw low-level call)
RFQVaultExecutor->>RFQVaultExecutor: balance delta >= minOutput check
RFQVaultExecutor->>Receiver: transfer outputToken delta
end
rect rgba(100, 200, 100, 0.5)
note right of Owner: CelerExecutor refund flow
Owner->>CelerExecutor: refundCelerUserAdmin(_request, _sigs, ...)
CelerExecutor->>CelerExecutor: decWithdrawMsg via Pb protobuf decoder
CelerExecutor->>ICelerBridge: withdraws(transferId) view check
CelerExecutor->>ICelerBridge: withdraw(_wdmsg, _sigs, _signers, _powers)
CelerExecutor->>CelerExecutor: balance delta == request.amount check
CelerExecutor->>Receiver: transfer ETH or ERC-20
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 10
🤖 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 @.env.example:
- Line 9: The SOLVER_SIGNER_ADDRESS configuration line has a comment appended
after the equals sign on the same line, which can cause dotenv parsing and
linting issues. Separate the comment from the variable assignment by moving the
comment to a new line above the SOLVER_SIGNER_ADDRESS definition. Remove any
trailing spaces after the equals sign so the line ends with just the equals
sign, keeping the configuration format consistent with dotenv best practices.
In `@scripts/deploy/celerExecutorAddresses.ts`:
- Around line 67-73: The address parameter is assigned directly to
deployments.CelerExecutor without any validation, allowing invalid values to be
persisted to the deployment JSON file. Before the line
`deployments.CelerExecutor = address;`, add validation logic to verify that the
address parameter is in the correct format (such as a valid Ethereum address).
If the address is invalid, throw an error or reject the operation before the
writeFile call executes. This ensures only valid deployment addresses are
persisted to the file.
In `@scripts/deploy/deployCelerExecutor.ts`:
- Around line 228-231: Replace the deployCreate3.staticCall() invocation with
computeCreate3Address() to compute the expected address before checking for
existing bytecode. Use computeCreate3Address() with CELER_EXECUTOR_CREATE3_SALT
and initcode to get the expectedAddress, then proceed with the bytecode
existence check at line 235, and only call deployCreate3() directly if no code
exists at that address. This implements the CreateX-recommended idempotent
deployment pattern that prevents reverts on re-runs when the contract is already
deployed.
In `@scripts/deploy/deployRFQVaultExecutor.ts`:
- Around line 37-59: The functions resolveOwnerAddress and
resolveSolverSignerAddress silently fall back to deployerAddress when
environment variables are present but fail validation against ADDR_HEX_RE, which
can lead to unintended deployments. Modify both functions to throw an error
immediately when process.env.OWNER_ADDRESS or process.env.SOLVER_SIGNER_ADDRESS
are set (non-empty) but do not pass the ADDR_HEX_RE validation, rather than
falling back to deployerAddress. This ensures malformed addresses are caught
early and fail fast during deployment.
In `@src/common/lib/Pb.sol`:
- Around line 35-55: The decVarint function reads up to 10 bytes unconditionally
from memory without validating that those bytes are actually available in the
buffer. To fix this, add a bounds check before the assembly read to ensure the
remaining buffer size is sufficient, and ensure that if the loop completes all
10 iterations without finding a terminating byte (where b & 0x80 == 0), the
function reverts instead of returning a potentially invalid truncated varint.
Specifically, calculate the available bytes as buf.b.length minus buf.idx and
verify that the read operation respects this boundary.
In `@src/executors/CelerExecutor.sol`:
- Around line 34-38: The constructor for CelerExecutor does not validate that
the critical addresses _openRouter and _celerBridgeRouter are non-zero. Add
validation checks in the constructor to ensure both addresses are not zero
before assigning them to OPEN_ROUTER and CELER_BRIDGE respectively. Use require
statements to revert the transaction if either address is zero, preventing
deployment of a misconfigured contract that could route funds incorrectly or
become unusable.
- Around line 61-67: The execute function does not validate msg.value against
the amount parameter, which can lead to spending pre-existing ETH or trapping
excess ETH in the contract. Add a require statement enforcing msg.value ==
amount in the isNative branch before calling CELER_BRIDGE.sendNative, and add a
require statement enforcing msg.value == 0 before the
SafeTransferLib.safeTransferFrom call in the ERC-20 flow to ensure proper
invariants.
In `@src/executors/RFQVaultExecutor.sol`:
- Around line 42-45: Add validation checks in the constructor of
RFQVaultExecutor to reject zero addresses for both _openRouter and _solverSigner
parameters. Use require statements to ensure _openRouter is not address(0) and
_solverSigner is not address(0), with descriptive error messages. Apply the same
validation to the setSolverSigner method (around lines 152-154) to prevent these
critical addresses from being set to zero after deployment, which would break
signed transaction flows and make receiveNative inaccessible.
- Around line 98-100: After the _performAction method is executed within the
approval block (where approvalToken, approvalSpender, and approvalAmount are
used), add a call to revoke the temporary allowance by setting it back to zero
using SafeTransferLib.safeApproveWithRetry with an approvalAmount of 0. This
ensures the spender's allowance is cleared immediately after the swap execution,
preventing any residual allowance that could be exploited for unauthorized token
pulls.
In `@test/unit/RFQVaultExecutor.t.sol`:
- Around line 483-484: Replace the generic vm.expectRevert() calls with specific
error selectors from the Ownable contract. For the vault.setSolverSigner()
calls, determine the exact custom error that Ownable throws (likely
Ownable.Unauthorized() or equivalent) and pass that error selector to
vm.expectRevert() instead of calling it without parameters. This ensures the
test validates the correct revert reason rather than accepting any revert. Apply
this change to both occurrences of the vault.setSolverSigner() test blocks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba006e95-0cb2-49e9-8295-07838ce117f9
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.env.examplepackage.jsonscripts/deploy/celerExecutorAddresses.tsscripts/deploy/create3.tsscripts/deploy/deployCelerExecutor.tsscripts/deploy/deployRFQVaultExecutor.tsscripts/deploy/rfqVaultExecutorAddresses.tssrc/common/lib/Pb.solsrc/executors/CelerExecutor.solsrc/executors/RFQVaultExecutor.solsrc/interfaces/ICelerBridge.soltest/unit/RFQVaultExecutor.t.sol
| # Constructor arguments / deployment checks | ||
| OWNER_ADDRESS= | ||
| SOLVER_SIGNER_ADDRESS= # BungeeReceiver deploy only; defaults to deployer | ||
| SOLVER_SIGNER_ADDRESS= # BungeeReceiver / RFQVaultExecutor deploy; defaults to deployer |
There was a problem hiding this comment.
Fix SOLVER_SIGNER_ADDRESS formatting to avoid dotenv parse/lint ambiguity.
This line should avoid trailing spaces/comment after =. Keep the comment on its own line.
Suggested fix
-SOLVER_SIGNER_ADDRESS= # BungeeReceiver / RFQVaultExecutor deploy; defaults to deployer
+# BungeeReceiver / RFQVaultExecutor deploy; defaults to deployer
+SOLVER_SIGNER_ADDRESS=📝 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.
| SOLVER_SIGNER_ADDRESS= # BungeeReceiver / RFQVaultExecutor deploy; defaults to deployer | |
| # BungeeReceiver / RFQVaultExecutor deploy; defaults to deployer | |
| SOLVER_SIGNER_ADDRESS= |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 9-9: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 9-9: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🤖 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 @.env.example at line 9, The SOLVER_SIGNER_ADDRESS configuration line has a
comment appended after the equals sign on the same line, which can cause dotenv
parsing and linting issues. Separate the comment from the variable assignment by
moving the comment to a new line above the SOLVER_SIGNER_ADDRESS definition.
Remove any trailing spaces after the equals sign so the line ends with just the
equals sign, keeping the configuration format consistent with dotenv best
practices.
Source: Linters/SAST tools
| deployments.CelerExecutor = address; | ||
|
|
||
| await mkdir(dirname(filePath), { recursive: true }); | ||
| await writeFile( | ||
| filePath, | ||
| `${JSON.stringify(deployments, null, 2)}\n`, | ||
| 'utf8', |
There was a problem hiding this comment.
Validate address format before persisting deployment JSON.
This write path accepts any string and persists it as CelerExecutor. Invalid values then look like valid deployment state to downstream scripts.
Proposed fix
import { mkdir, readFile, writeFile } from 'fs/promises';
import { dirname, resolve } from 'path';
+
+const ADDR_HEX_RE = /^0x[a-fA-F0-9]{40}$/;
@@
export async function writeCelerExecutorAddress(
network: string,
address: string,
stage = 'prod',
): Promise<string> {
+ if (!ADDR_HEX_RE.test(address.trim())) {
+ throw new Error(`Invalid CelerExecutor address: ${address}`);
+ }
const filePath = celerExecutorAddressesPath(network, stage);
@@
- deployments.CelerExecutor = address;
+ deployments.CelerExecutor = address.trim();📝 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.
| deployments.CelerExecutor = address; | |
| await mkdir(dirname(filePath), { recursive: true }); | |
| await writeFile( | |
| filePath, | |
| `${JSON.stringify(deployments, null, 2)}\n`, | |
| 'utf8', | |
| if (!ADDR_HEX_RE.test(address.trim())) { | |
| throw new Error(`Invalid CelerExecutor address: ${address}`); | |
| } | |
| deployments.CelerExecutor = address.trim(); | |
| await mkdir(dirname(filePath), { recursive: true }); | |
| await writeFile( | |
| filePath, | |
| `${JSON.stringify(deployments, null, 2)}\n`, | |
| 'utf8', |
🤖 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 `@scripts/deploy/celerExecutorAddresses.ts` around lines 67 - 73, The address
parameter is assigned directly to deployments.CelerExecutor without any
validation, allowing invalid values to be persisted to the deployment JSON file.
Before the line `deployments.CelerExecutor = address;`, add validation logic to
verify that the address parameter is in the correct format (such as a valid
Ethereum address). If the address is invalid, throw an error or reject the
operation before the writeFile call executes. This ensures only valid deployment
addresses are persisted to the file.
Source: Coding guidelines
| const expectedAddress = (await create3Factory.deployCreate3.staticCall( | ||
| CELER_EXECUTOR_CREATE3_SALT, | ||
| initcode, | ||
| )) as string; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For CreateX (https://createx.rocks/), does calling deployCreate3(bytes32,bytes) via eth_call/static simulation revert when the same CREATE3 salt has already been deployed? Please include docs/source references and expected idempotent deployment pattern.
💡 Result:
Yes, calling deployCreate3(bytes32,bytes) via eth_call (or any static simulation) will revert if the address derived from the provided salt has already been deployed [1]. Underlying Mechanism: CreateX uses the CREATE3 pattern, which relies on a two-step deployment: 1. It uses CREATE2 to deploy a proxy contract at an address determined by the CREATE3 salt [1]. 2. It then performs a call to that proxy to deploy the target contract [1]. If the salt has already been used to deploy a contract, the CREATE2 deployment of the proxy (or the subsequent contract initialization call) will fail [1]. The CreateX contract explicitly checks the success of the deployment and reverts if it fails [1]: if (proxy == address(0)) { revert FailedContractCreation({emitter: _SELF}); }... (bool success,) = proxy.call{value: msg.value}(initCode); _requireSuccessfulContractCreation({success: success, newContract: newContract}); When performing an eth_call (static simulation), any operation that triggers a revert will cause the entire call to fail, correctly signaling that the deployment cannot proceed at that address [1]. Idempotent Deployment Pattern: To implement idempotent deployments with CreateX, you should check if the contract already exists at the computed address before attempting to deploy [2]. You can use the computeCreate3Address function provided by the CreateX factory to determine the expected address for a given salt [2][3]: // 1. Compute the expected address address expectedAddress = createX.computeCreate3Address(salt); // 2. Check if code exists at that address uint256 codeSize; assembly { codeSize:= extcodesize(expectedAddress) } if (codeSize == 0) { // 3. Address is empty; safe to deploy createX.deployCreate3{value: msgValue}(salt, initCode); } else { // Contract already exists; handle accordingly } Note: CreateX hashes the provided salt within its _guard function to implement security features like permissioned deployment protection [4]. Therefore, you must use the factory's computeCreate3Address function rather than calculating the address manually to ensure the salt and address match the factory's internal logic [2][3].
Citations:
- 1: https://github.com/pcaversaccio/createx/blob/main/src/CreateX.sol
- 2: https://github.com/pcaversaccio/createx?tab=readme-ov-file
- 3: https://github.com/pcaversaccio/createx/blob/main/src/ICreateX.sol
- 4: 🐛 No Way to Avoid Salt Being Re-Hashed pcaversaccio/createx#98
🏁 Script executed:
cat -n scripts/deploy/deployCelerExecutor.ts | sed -n '220,240p'Repository: SocketDotTech/openrouter
Length of output: 800
🏁 Script executed:
find . -name "Create3ABI*" -o -name "*Create3*" -type f | grep -i abi | head -20Repository: SocketDotTech/openrouter
Length of output: 50
🏁 Script executed:
head -50 scripts/deploy/deployCelerExecutor.ts | grep -A 5 -B 5 "Create3ABI"Repository: SocketDotTech/openrouter
Length of output: 398
🏁 Script executed:
find . -name "create3.ts" -o -name "create3.js" | head -5Repository: SocketDotTech/openrouter
Length of output: 96
🏁 Script executed:
cat -n ./scripts/deploy/create3.ts | head -100Repository: SocketDotTech/openrouter
Length of output: 4438
🏁 Script executed:
rg "Create3ABI" scripts/deploy/create3.ts -A 50 | head -80Repository: SocketDotTech/openrouter
Length of output: 1699
Fix idempotent deployment pattern: compute address before staticCall to prevent revert on re-runs.
Calling deployCreate3.staticCall() before checking if code exists at the address will cause the script to crash if the contract is already deployed. CreateX deployCreate3 reverts when the salt has been used; the call never succeeds to reach the bytecode check at line 235.
Use computeCreate3Address() first to get the expected address, then check if bytecode exists, and only call deployCreate3() if the address is empty. This matches the CreateX-recommended idempotent pattern and is already available in the Create3ABI.
🤖 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 `@scripts/deploy/deployCelerExecutor.ts` around lines 228 - 231, Replace the
deployCreate3.staticCall() invocation with computeCreate3Address() to compute
the expected address before checking for existing bytecode. Use
computeCreate3Address() with CELER_EXECUTOR_CREATE3_SALT and initcode to get the
expectedAddress, then proceed with the bytecode existence check at line 235, and
only call deployCreate3() directly if no code exists at that address. This
implements the CreateX-recommended idempotent deployment pattern that prevents
reverts on re-runs when the contract is already deployed.
| function decVarint(Buffer memory buf) internal pure returns (uint256 v) { | ||
| bytes10 tmp; | ||
| bytes memory bb = buf.b; | ||
| v = buf.idx; | ||
| assembly ("memory-safe") { | ||
| tmp := mload(add(add(bb, 32), v)) | ||
| } | ||
| uint256 b; | ||
| v = 0; | ||
| for (uint256 i = 0; i < 10; i++) { | ||
| assembly ("memory-safe") { | ||
| b := byte(i, tmp) | ||
| } | ||
| v |= (b & 0x7F) << (i * 7); | ||
| if (b & 0x80 == 0) { | ||
| buf.idx += i + 1; | ||
| return v; | ||
| } | ||
| } | ||
| revert(); | ||
| } |
There was a problem hiding this comment.
Bound decVarint reads to remaining bytes.
Line 39/46 reads up to 10 bytes unconditionally from memory. Truncated varints can decode as valid values (with zero-filled tail) and move buf.idx past buf.b.length instead of reverting. This should reject incomplete varints.
Proposed fix
function decVarint(Buffer memory buf) internal pure returns (uint256 v) {
- bytes10 tmp;
bytes memory bb = buf.b;
- v = buf.idx;
- assembly ("memory-safe") {
- tmp := mload(add(add(bb, 32), v))
- }
- uint256 b;
- v = 0;
+ uint256 start = buf.idx;
for (uint256 i = 0; i < 10; i++) {
- assembly ("memory-safe") {
- b := byte(i, tmp)
- }
+ if (start + i >= bb.length) revert();
+ uint256 b = uint8(bb[start + i]);
v |= (b & 0x7F) << (i * 7);
if (b & 0x80 == 0) {
- buf.idx += i + 1;
+ buf.idx = start + i + 1;
return v;
}
}
revert();
}📝 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.
| function decVarint(Buffer memory buf) internal pure returns (uint256 v) { | |
| bytes10 tmp; | |
| bytes memory bb = buf.b; | |
| v = buf.idx; | |
| assembly ("memory-safe") { | |
| tmp := mload(add(add(bb, 32), v)) | |
| } | |
| uint256 b; | |
| v = 0; | |
| for (uint256 i = 0; i < 10; i++) { | |
| assembly ("memory-safe") { | |
| b := byte(i, tmp) | |
| } | |
| v |= (b & 0x7F) << (i * 7); | |
| if (b & 0x80 == 0) { | |
| buf.idx += i + 1; | |
| return v; | |
| } | |
| } | |
| revert(); | |
| } | |
| function decVarint(Buffer memory buf) internal pure returns (uint256 v) { | |
| bytes memory bb = buf.b; | |
| uint256 start = buf.idx; | |
| for (uint256 i = 0; i < 10; i++) { | |
| if (start + i >= bb.length) revert(); | |
| uint256 b = uint8(bb[start + i]); | |
| v |= (b & 0x7F) << (i * 7); | |
| if (b & 0x80 == 0) { | |
| buf.idx = start + i + 1; | |
| return v; | |
| } | |
| } | |
| revert(); | |
| } |
🤖 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 `@src/common/lib/Pb.sol` around lines 35 - 55, The decVarint function reads up
to 10 bytes unconditionally from memory without validating that those bytes are
actually available in the buffer. To fix this, add a bounds check before the
assembly read to ensure the remaining buffer size is sufficient, and ensure that
if the loop completes all 10 iterations without finding a terminating byte
(where b & 0x80 == 0), the function reverts instead of returning a potentially
invalid truncated varint. Specifically, calculate the available bytes as
buf.b.length minus buf.idx and verify that the read operation respects this
boundary.
| constructor(address _owner, address _openRouter, address _celerBridgeRouter) Ownable(_owner) { | ||
| CELER_BRIDGE = ICelerBridge(_celerBridgeRouter); | ||
| OPEN_ROUTER = _openRouter; | ||
| CHAIN_ID = uint64(block.chainid); | ||
| } |
There was a problem hiding this comment.
Reject zero critical addresses in the constructor.
If _openRouter or _celerBridgeRouter is zero, the executor is misconfigured at deploy time and can route funds incorrectly or become unusable. This should fail fast in constructor.
Proposed fix
contract CelerExecutor is Ownable {
using Pb for Pb.Buffer;
+ error ZeroAddress();
error CallerNotOpenRouter();
error InvalidCelerRefund();
error CelerRefundNotReady();
@@
constructor(address _owner, address _openRouter, address _celerBridgeRouter) Ownable(_owner) {
+ if (_openRouter == address(0) || _celerBridgeRouter == address(0)) {
+ revert ZeroAddress();
+ }
CELER_BRIDGE = ICelerBridge(_celerBridgeRouter);
OPEN_ROUTER = _openRouter;
CHAIN_ID = uint64(block.chainid);
}🤖 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 `@src/executors/CelerExecutor.sol` around lines 34 - 38, The constructor for
CelerExecutor does not validate that the critical addresses _openRouter and
_celerBridgeRouter are non-zero. Add validation checks in the constructor to
ensure both addresses are not zero before assigning them to OPEN_ROUTER and
CELER_BRIDGE respectively. Use require statements to revert the transaction if
either address is zero, preventing deployment of a misconfigured contract that
could route funds incorrectly or become unusable.
| if (isNative) { | ||
| CELER_BRIDGE.sendNative{value: amount}(receiver, amount, dstChainId, nonce, maxSlippage); | ||
| return; | ||
| } | ||
|
|
||
| SafeTransferLib.safeTransferFrom(token, OPEN_ROUTER, address(this), amount); | ||
|
|
There was a problem hiding this comment.
Enforce msg.value invariants in execute.
Native flow should require msg.value == amount, and ERC-20 flow should require msg.value == 0. Current logic can spend pre-existing ETH balance or trap extra ETH in this contract.
Proposed fix
error CallerNotOpenRouter();
error InvalidCelerRefund();
error CelerRefundNotReady();
+ error InvalidMsgValue();
@@
if (isNative) {
+ if (msg.value != amount) revert InvalidMsgValue();
CELER_BRIDGE.sendNative{value: amount}(receiver, amount, dstChainId, nonce, maxSlippage);
return;
}
+ if (msg.value != 0) revert InvalidMsgValue();
SafeTransferLib.safeTransferFrom(token, OPEN_ROUTER, address(this), amount);📝 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.
| if (isNative) { | |
| CELER_BRIDGE.sendNative{value: amount}(receiver, amount, dstChainId, nonce, maxSlippage); | |
| return; | |
| } | |
| SafeTransferLib.safeTransferFrom(token, OPEN_ROUTER, address(this), amount); | |
| if (isNative) { | |
| if (msg.value != amount) revert InvalidMsgValue(); | |
| CELER_BRIDGE.sendNative{value: amount}(receiver, amount, dstChainId, nonce, maxSlippage); | |
| return; | |
| } | |
| if (msg.value != 0) revert InvalidMsgValue(); | |
| SafeTransferLib.safeTransferFrom(token, OPEN_ROUTER, address(this), amount); |
🤖 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 `@src/executors/CelerExecutor.sol` around lines 61 - 67, The execute function
does not validate msg.value against the amount parameter, which can lead to
spending pre-existing ETH or trapping excess ETH in the contract. Add a require
statement enforcing msg.value == amount in the isNative branch before calling
CELER_BRIDGE.sendNative, and add a require statement enforcing msg.value == 0
before the SafeTransferLib.safeTransferFrom call in the ERC-20 flow to ensure
proper invariants.
| constructor(address _owner, address _openRouter, address _solverSigner) Ownable(_owner) { | ||
| OPEN_ROUTER = _openRouter; | ||
| SOLVER_SIGNER = _solverSigner; | ||
| } |
There was a problem hiding this comment.
Reject zero addresses for signer/router configuration.
This should enforce non-zero addresses in constructor and setSolverSigner. A zero signer bricks signed flows, and a zero OPEN_ROUTER makes receiveNative unreachable.
Suggested fix
contract RFQVaultExecutor is Ownable {
+ error ZeroAddress();
@@
constructor(address _owner, address _openRouter, address _solverSigner) Ownable(_owner) {
+ if (_openRouter == address(0) || _solverSigner == address(0)) {
+ revert ZeroAddress();
+ }
OPEN_ROUTER = _openRouter;
SOLVER_SIGNER = _solverSigner;
}
@@
function setSolverSigner(address _solverSigner) external onlyOwner {
+ if (_solverSigner == address(0)) {
+ revert ZeroAddress();
+ }
SOLVER_SIGNER = _solverSigner;
}Also applies to: 152-154
🤖 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 `@src/executors/RFQVaultExecutor.sol` around lines 42 - 45, Add validation
checks in the constructor of RFQVaultExecutor to reject zero addresses for both
_openRouter and _solverSigner parameters. Use require statements to ensure
_openRouter is not address(0) and _solverSigner is not address(0), with
descriptive error messages. Apply the same validation to the setSolverSigner
method (around lines 152-154) to prevent these critical addresses from being set
to zero after deployment, which would break signed transaction flows and make
receiveNative inaccessible.
| if (approvalToken != address(0)) { | ||
| SafeTransferLib.safeApproveWithRetry(approvalToken, approvalSpender, approvalAmount); | ||
| } |
There was a problem hiding this comment.
Clear temporary allowance after swap execution.
This should revoke the spender allowance after _performAction. Leaving residual allowance allows later token pulls without a new signed intent.
Suggested fix
if (approvalToken != address(0)) {
SafeTransferLib.safeApproveWithRetry(approvalToken, approvalSpender, approvalAmount);
}
@@
if (!_performAction(swapAction)) {
revert ActionFailed(0);
}
+ if (approvalToken != address(0)) {
+ SafeTransferLib.safeApproveWithRetry(approvalToken, approvalSpender, 0);
+ }Also applies to: 102-117
🤖 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 `@src/executors/RFQVaultExecutor.sol` around lines 98 - 100, After the
_performAction method is executed within the approval block (where
approvalToken, approvalSpender, and approvalAmount are used), add a call to
revoke the temporary allowance by setting it back to zero using
SafeTransferLib.safeApproveWithRetry with an approvalAmount of 0. This ensures
the spender's allowance is cleared immediately after the swap execution,
preventing any residual allowance that could be exploited for unauthorized token
pulls.
| vm.expectRevert(); | ||
| vault.setSolverSigner(newSigner); |
There was a problem hiding this comment.
Use specific revert selectors in owner-only tests.
These checks should assert the exact custom error selector from Ownable; generic vm.expectRevert() can pass on the wrong revert path.
As per coding guidelines, "Assertions are specific - use assertEq, assertGt, etc. instead of generic assert."
Also applies to: 500-501
🤖 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 `@test/unit/RFQVaultExecutor.t.sol` around lines 483 - 484, Replace the generic
vm.expectRevert() calls with specific error selectors from the Ownable contract.
For the vault.setSolverSigner() calls, determine the exact custom error that
Ownable throws (likely Ownable.Unauthorized() or equivalent) and pass that error
selector to vm.expectRevert() instead of calling it without parameters. This
ensures the test validates the correct revert reason rather than accepting any
revert. Apply this change to both occurrences of the vault.setSolverSigner()
test blocks.
Source: Coding guidelines
| } | ||
|
|
||
| /// @notice Accept native deposits from OpenRouter and emit a correlating event. | ||
| function receiveNative(bytes32 quoteId) external payable { |
There was a problem hiding this comment.
I think you should add a receiveERC20 that we can use if we want to
| if (msg.sender != OPEN_ROUTER) { | ||
| revert CallerNotOpenRouter(); | ||
| } |
There was a problem hiding this comment.
curious: why is this check needed if you are listening to the event on OR to know if quoteID is a tx and then doing "If native tokens we check the native token emission"
| bytes32 messageHash = _hashFulfilOrRefund(REFUND_DISCRIMINATOR, quoteId, nonce, token, amount, receiver); | ||
| _verifyAndMarkQuoteId(messageHash, signature, quoteId); |
There was a problem hiding this comment.
im confused, why is this needed?
- cant we just do quoteId, authData{hash, sig), amountToFill
- the data being used to validate is coming from backend, there is no source of truth onchain, so not sure why we are computing messageHash onchain
|
|
||
| mapping(bytes32 quoteId => uint256 isUsed) public quoteIdUsed; | ||
| mapping(uint256 nonce => uint256 isUsed) public nonceUsed; | ||
| mapping(bytes32 quoteId => uint256 isMarked) public isMarkedForRefund; |
| uint256 nonce, | ||
| address token, | ||
| uint256 amount, | ||
| address receiver, |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/unit/RFQVaultExecutor.t.sol (2)
652-675: 💤 Low valueDead code:
_hashSwapAndFulfilis never called.This function is defined but never used.
_signSwapAndFulfilWithKeycalls_hashSwapAndFulfilAtdirectly (line 590-601). Either remove this wrapper or use it in the signing function for consistency.Option 1: Remove dead code
- function _hashSwapAndFulfil( - bytes32 quoteId, - uint256 nonce, - address approvalToken, - address approvalSpender, - uint256 approvalAmount, - address outputToken_, - uint256 minOutput, - RFQVaultExecutor.Action memory swapAction, - address receiver - ) internal view returns (bytes32 hash) { - return _hashSwapAndFulfilAt( - address(vault), - quoteId, - nonce, - approvalToken, - approvalSpender, - approvalAmount, - outputToken_, - minOutput, - swapAction, - receiver - ); - } -🤖 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 `@test/unit/RFQVaultExecutor.t.sol` around lines 652 - 675, The function _hashSwapAndFulfil is never invoked and serves as dead code. Either remove the _hashSwapAndFulfil function entirely, or refactor _signSwapAndFulfilWithKey (which currently calls _hashSwapAndFulfilAt directly at lines 590-601) to use the _hashSwapAndFulfil wrapper function instead for consistency and to eliminate the unused code.
73-86: ⚖️ Poor tradeoffAdd fuzz tests for amount and boundary validation.
Per coding guidelines, fuzz tests should be used for input validation. Key candidates:
fulfil/refundwith fuzzed amounts (including zero and type(uint256).max)swapAndFulfilwith fuzzed minOutput values- Boundary testing around approval amounts
This strengthens confidence that the signature verification and transfer logic handle arbitrary inputs correctly.
Example fuzz test structure
function testFuzz_fulfil_erc20(uint256 amount) public { vm.assume(amount > 0 && amount < type(uint128).max); token.mint(address(vault), amount); bytes32 quoteId = keccak256(abi.encode("fuzz", amount)); bytes memory signature = _signFulfil(quoteId, NONCE, address(token), amount, RECEIVER); vault.fulfil(quoteId, NONCE, address(token), amount, RECEIVER, signature); assertEq(token.balanceOf(RECEIVER), amount); }As per coding guidelines, "Fuzz tests used appropriately for input validation."
🤖 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 `@test/unit/RFQVaultExecutor.t.sol` around lines 73 - 86, The current test_fulfil_erc20 function is a deterministic test with hardcoded values. Convert this and related tests (fulfil, refund, swapAndFulfil) into fuzz tests by adding fuzzed uint256 parameters (e.g., amount, minOutput) to the function signatures. Use vm.assume() to set reasonable bounds on the fuzzed values (such as amount being greater than 0 and less than type(uint128).max) to avoid memory issues while still testing a wide range of inputs. Add additional fuzz tests to cover boundary cases like zero amounts, maximum values, and approval edge cases to ensure signature verification and transfer logic handle arbitrary inputs correctly.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@test/unit/RFQVaultExecutor.t.sol`:
- Around line 652-675: The function _hashSwapAndFulfil is never invoked and
serves as dead code. Either remove the _hashSwapAndFulfil function entirely, or
refactor _signSwapAndFulfilWithKey (which currently calls _hashSwapAndFulfilAt
directly at lines 590-601) to use the _hashSwapAndFulfil wrapper function
instead for consistency and to eliminate the unused code.
- Around line 73-86: The current test_fulfil_erc20 function is a deterministic
test with hardcoded values. Convert this and related tests (fulfil, refund,
swapAndFulfil) into fuzz tests by adding fuzzed uint256 parameters (e.g.,
amount, minOutput) to the function signatures. Use vm.assume() to set reasonable
bounds on the fuzzed values (such as amount being greater than 0 and less than
type(uint128).max) to avoid memory issues while still testing a wide range of
inputs. Add additional fuzz tests to cover boundary cases like zero amounts,
maximum values, and approval edge cases to ensure signature verification and
transfer logic handle arbitrary inputs correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da19b866-e6c4-4bb1-ad82-f0cc997a08df
📒 Files selected for processing (3)
scripts/deploy/deployRFQVaultExecutor.tssrc/executors/RFQVaultExecutor.soltest/unit/RFQVaultExecutor.t.sol
3 participants
Summary by CodeRabbit
Release Notes
New Features
Deployment & Configuration
deploy:rfq-vault-executornpm script..env.exampleto clarifySOLVER_SIGNER_ADDRESSapplies to both related deployments (still defaults to deployer).Tests