feat(contracts-rfq): rework permisionless cancellation [SLT-489] #3382
Conversation
WalkthroughThe changes introduced in this pull request involve the addition of a new smart contract Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3382 +/- ##
===================================================
+ Coverage 33.20876% 34.82890% +1.62013%
===================================================
Files 544 531 -13
Lines 34786 33811 -975
Branches 82 82
===================================================
+ Hits 11552 11776 +224
+ Misses 22216 21020 -1196
+ Partials 1018 1015 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Deploying sanguine-fe with
|
| Latest commit: |
922b4a5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e930ca28.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-fbv2-rework-cancellatio.sanguine-fe.pages.dev |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (16)
packages/contracts-rfq/contracts/interfaces/IAdminV2Errors.sol (1)
4-7: Consider adding parameters to custom errors for better debugging.While the current error definitions are functional, adding parameters would provide more context during error scenarios:
interface IAdminV2Errors { - error CancelDelayBelowMin(); - error FeeRateAboveMax(); + error CancelDelayBelowMin(uint256 providedDelay, uint256 minimumDelay); + error FeeRateAboveMax(uint256 providedRate, uint256 maxRate); }This would help developers and users understand:
- What delay value was attempted vs the minimum allowed
- What fee rate was attempted vs the maximum allowed
packages/contracts-rfq/contracts/interfaces/IAdminV2.sol (1)
9-13: Add NatSpec documentation for interface functionsConsider adding NatSpec documentation (@notice, @dev, @param) to describe:
- Expected parameter ranges (e.g., minimum cancelDelay of 1 hour mentioned in PR)
- Function behaviors
- Access control requirements
Example:
/// @notice Updates the delay period required before permissionless cancellation /// @dev Minimum delay is 1 hour (3600 seconds) /// @param newCancelDelay The new delay period in seconds function setCancelDelay(uint256 newCancelDelay) external;packages/contracts-rfq/test/FastBridgeV2.Src.RefundV1.t.sol (1)
6-7: Add documentation to clarify the compatibility test purposeConsider adding a documentation comment to explain that this contract specifically tests backward compatibility with the V1 refund functionality.
// solhint-disable func-name-mixedcase, ordering +/** + * @title FastBridgeV2SrcRefundV1Test + * @dev Test contract ensuring backward compatibility with V1 refund functionality + */ contract FastBridgeV2SrcRefundV1Test is FastBridgeV2SrcTest {packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (2)
Line range hint
164-171: Add edge case tests for permissionless cancellationThe permissionless cancellation test only covers the happy path. Consider adding tests for:
- Attempting cancellation before DEADLINE
- Attempting cancellation after DEADLINE but before PERMISSIONLESS_CANCEL_DELAY
- Attempting cancellation by the original transaction sender
Example test case to add:
function test_cancelPermissionless_token_beforeDeadline() public { bytes32 txId = getTxId(bridgedTokenTx); skipTimeAtLeast({time: DEADLINE - 1}); vm.expectRevert("FastBridgeV2: not expired"); cancel({caller: userB, bridgeTx: bridgedTokenTx}); }
Line range hint
248-255: Add gas comparison for permissionless vs permissioned cancellationSince this is a gas benchmark test file, it would be valuable to compare gas costs between permissioned and permissionless cancellations.
Add gas snapshot comparisons:
function test_cancelGasComparison_eth() public { bytes32 txId = getTxId(bridgedEthTx); skipTimeAtLeast({time: DEADLINE + PERMISSIONLESS_CANCEL_DELAY}); uint256 permissionedGas = gasleft(); cancel({caller: canceler, bridgeTx: bridgedEthTx}); permissionedGas -= gasleft(); uint256 permissionlessGas = gasleft(); cancel({caller: userB, bridgeTx: bridgedEthTx}); permissionlessGas -= gasleft(); console.log("Permissioned cancellation gas:", permissionedGas); console.log("Permissionless cancellation gas:", permissionlessGas); }packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (2)
820-870: Test coverage for token cancellation is comprehensive.The tests thoroughly cover:
- Basic cancellation flow
- Cancellation by different sender
- Long delay scenarios
- Permissionless cancellation with additional delay
However, there's an opportunity to enhance the test coverage.
Consider adding tests for:
- Edge cases around the
PERMISSIONLESS_CANCEL_DELAYboundary- Cancellation attempts with invalid token contracts
939-944: Fix typo in test name: "permisionless" should be "permissionless".The function name contains a spelling error.
- function test_cancel_revert_justBeforeDeadline_permisionless(address caller) public { + function test_cancel_revert_justBeforeDeadline_permissionless(address caller) public {packages/contracts-rfq/contracts/AdminV2.sol (3)
41-43: Add NatSpec comments to public functionsConsider adding NatSpec comments to the
setCancelDelayfunction to enhance documentation and improve code maintainability. This aids in understanding the function's purpose, parameters, and any requirements for its usage.
45-50: Add NatSpec comments tosetProtocolFeeRatefunctionAdding NatSpec documentation to the
setProtocolFeeRatefunction will improve readability and provide clear guidance on how the function should be used, including any constraints on thenewFeeRateparameter.
52-59: Add NatSpec comments tosweepProtocolFeesfunctionEnhancing the
sweepProtocolFeesfunction with NatSpec documentation will help clarify its purpose, parameters, and any important considerations when calling this function.🧰 Tools
🪛 GitHub Check: Slither
[notice] 52-59: Reentrancy vulnerabilities
Reentrancy in AdminV2.sweepProtocolFees(address,address) (contracts/AdminV2.sol#52-59):
External calls:
- token.universalTransfer(recipient,feeAmount) (contracts/AdminV2.sol#57)
Event emitted after the call(s):
- FeesSwept(token,recipient,feeAmount) (contracts/AdminV2.sol#58)packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (3)
94-94: Inconsistent@paramdescription in thecancelfunctionThe
@paramcomment describes "The encoded bridge transaction to refund", but the function is namedcancel. For consistency and clarity, consider updating the@paramdescription to "The encoded bridge transaction to cancel".Apply this diff to update the
@paramdescription:/// @param request The encoded bridge transaction to refund +/// @param request The encoded bridge transaction to cancel
92-93: Add comma for clarity in the@noticecommentConsider adding a comma after "failed" for better readability:
"Cancels an outstanding bridge transaction in case optimistic bridging failed, and returns the full amount to the original sender."
Apply this diff to improve readability:
/// @notice Cancels an outstanding bridge transaction in case optimistic bridging failed and returns the full amount to the original sender. +/// @notice Cancels an outstanding bridge transaction in case optimistic bridging failed, and returns the full amount to the original sender.
95-95: Consider emitting an event in thecancelfunctionTo facilitate tracking and monitoring of cancellations, consider emitting an event when the
cancelfunction is called. This will help in auditing and supporting off-chain services that rely on events.Suggest adding an event declaration:
event BridgeTransactionCancelled(bytes32 indexed transactionId, address indexed sender);And ensure the implementation emits this event upon successful cancellation.
packages/contracts-rfq/test/FastBridgeV2.Management.t.sol (3)
10-10: Consider removing inheritance fromIAdminV2Errors.Since
IAdminV2Errorscontains custom error definitions, importing it is sufficient to use the custom errors in your contract. Inheriting from the interface isn't necessary and can be omitted to simplify the inheritance hierarchy.
77-95: Add a test case for settingcancelDelayto the minimum allowed value.While tests cover setting
cancelDelayabove and below the minimum, adding a test wherecancelDelayis set exactly toMIN_CANCEL_DELAYensures that the edge case is handled correctly.
163-167: Re-evaluate the necessity of testing removed functions via deprecated interfaces.In
test_setChainGasAmount_revert(), castingfastBridgetoIAdminto callsetChainGasAmountmay not be necessary. SincesetChainGasAmounthas been removed from the V2 interface, testing its absence through deprecated interfaces could be redundant or misleading. Consider removing this test to streamline the test suite.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
packages/contracts-rfq/contracts/AdminV2.sol(1 hunks)packages/contracts-rfq/contracts/FastBridgeV2.sol(6 hunks)packages/contracts-rfq/contracts/interfaces/IAdminV2.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/IAdminV2Errors.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol(1 hunks)packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol(2 hunks)packages/contracts-rfq/test/FastBridgeV2.Management.t.sol(5 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol(3 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.RefundV1.t.sol(1 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.t.sol(6 hunks)packages/contracts-rfq/test/FastBridgeV2.t.sol(1 hunks)
🧰 Additional context used
📓 Learnings (3)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-10-14T14:45:57.214Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-10-14T14:45:57.214Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
packages/contracts-rfq/test/FastBridgeV2.Src.RefundV1.t.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-10-14T14:45:57.214Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/AdminV2.sol
[warning] 2-2: Different pragma directives are used
3 different versions of Solidity are used:
- Version constraint ^0.8.20 is used by:
-^0.8.20 (contracts/AdminV2.sol#2)
-^0.8.20 (contracts/libs/UniversalToken.sol#2)
-^0.8.20 (node_modules/@openzeppelin/contracts/access/AccessControl.sol#4)
-^0.8.20 (node_modules/@openzeppelin/contracts/access/IAccessControl.sol#4)
-^0.8.20 (node_modules/@openzeppelin/contracts/access/extensions/AccessControlEnumerable.sol#4)
-^0.8.20 (node_modules/@openzeppelin/contracts/access/extensions/IAccessControlEnumerable.sol#4)
-^0.8.20 (node_modules/@openzeppelin/contracts/token/ERC20/IERC20.sol#4)
-^0.8.20 (node_modules/@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol#4)
-^0.8.20 (node_modules/@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol#4)
-^0.8.20 (node_modules/@openzeppelin/contracts/utils/Address.sol#4)
-^0.8.20 (node_modules/@openzeppelin/contracts/utils/Context.sol#4)
-^0.8.20 (node_modules/@openzeppelin/contracts/utils/introspection/ERC165.sol#4)
-^0.8.20 (node_modules/@openzeppelin/contracts/utils/introspection/IERC165.sol#4)
-^0.8.20 (node_modules/@openzeppelin/contracts/utils/structs/EnumerableSet.sol#5)
- Version constraint 0.8.24 is used by:
-0.8.24 (contracts/FastBridgeV2.sol#2)
- Version constraint ^0.8.4 is used by:
-^0.8.4 (contracts/interfaces/IAdminV2.sol#2)
-^0.8.4 (contracts/interfaces/IAdminV2Errors.sol#2)
-^0.8.4 (contracts/interfaces/IFastBridge.sol#2)
-^0.8.4 (contracts/interfaces/IFastBridgeV2.sol#2)
-^0.8.4 (contracts/interfaces/IFastBridgeV2Errors.sol#2)
-^0.8.4 (contracts/interfaces/IMulticallTarget.sol#2)
-^0.8.4 (contracts/interfaces/IZapRecipient.sol#2)
-^0.8.4 (contracts/libs/BridgeTransactionV2.sol#2)
-^0.8.4 (contracts/libs/Errors.sol#2)
-^0.8.4 (contracts/utils/MulticallTarget.sol#2)
[warning] 2-2: Incorrect versions of Solidity
Version constraint ^0.8.20 contains known severe issues (https://solidity.readthedocs.io/en/latest/bugs.html)
- VerbatimInvalidDeduplication
- FullInlinerNonExpressionSplitArgumentEvaluationOrder
- MissingSideEffectsOnSelectorAccess.
It is used by:
- ^0.8.20 (contracts/AdminV2.sol#2)
- ^0.8.20 (contracts/libs/UniversalToken.sol#2)
- ^0.8.20 (node_modules/@openzeppelin/contracts/access/AccessControl.sol#4)
- ^0.8.20 (node_modules/@openzeppelin/contracts/access/IAccessControl.sol#4)
- ^0.8.20 (node_modules/@openzeppelin/contracts/access/extensions/AccessControlEnumerable.sol#4)
- ^0.8.20 (node_modules/@openzeppelin/contracts/access/extensions/IAccessControlEnumerable.sol#4)
- ^0.8.20 (node_modules/@openzeppelin/contracts/token/ERC20/IERC20.sol#4)
- ^0.8.20 (node_modules/@openzeppelin/contracts/token/ERC20/extensions/IERC20Permit.sol#4)
- ^0.8.20 (node_modules/@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol#4)
- ^0.8.20 (node_modules/@openzeppelin/contracts/utils/Address.sol#4)
- ^0.8.20 (node_modules/@openzeppelin/contracts/utils/Context.sol#4)
- ^0.8.20 (node_modules/@openzeppelin/contracts/utils/introspection/ERC165.sol#4)
- ^0.8.20 (node_modules/@openzeppelin/contracts/utils/introspection/IERC165.sol#4)
- ^0.8.20 (node_modules/@openzeppelin/contracts/utils/structs/EnumerableSet.sol#5)
[notice] 52-59: Reentrancy vulnerabilities
Reentrancy in AdminV2.sweepProtocolFees(address,address) (contracts/AdminV2.sol#52-59):
External calls:
- token.universalTransfer(recipient,feeAmount) (contracts/AdminV2.sol#57)
Event emitted after the call(s):
- FeesSwept(token,recipient,feeAmount) (contracts/AdminV2.sol#58)
packages/contracts-rfq/contracts/FastBridgeV2.sol
[notice] 341-367: Block timestamp
FastBridgeV2.cancel(bytes) (contracts/FastBridgeV2.sol#341-367) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp <= deadline (contracts/FastBridgeV2.sol#350)
packages/contracts-rfq/contracts/interfaces/IAdminV2.sol
[warning] 2-2: Incorrect versions of Solidity
Version constraint ^0.8.4 contains known severe issues (https://solidity.readthedocs.io/en/latest/bugs.html)
- FullInlinerNonExpressionSplitArgumentEvaluationOrder
- MissingSideEffectsOnSelectorAccess
- AbiReencodingHeadOverflowWithStaticArrayCleanup
- DirtyBytesArrayToStorage
- DataLocationChangeInInternalOverride
- NestedCalldataArrayAbiReencodingSizeValidation
- SignedImmutables.
It is used by:
- ^0.8.4 (contracts/interfaces/IAdminV2.sol#2)
- ^0.8.4 (contracts/interfaces/IAdminV2Errors.sol#2)
- ^0.8.4 (contracts/interfaces/IFastBridge.sol#2)
- ^0.8.4 (contracts/interfaces/IFastBridgeV2.sol#2)
- ^0.8.4 (contracts/interfaces/IFastBridgeV2Errors.sol#2)
- ^0.8.4 (contracts/interfaces/IMulticallTarget.sol#2)
- ^0.8.4 (contracts/interfaces/IZapRecipient.sol#2)
- ^0.8.4 (contracts/libs/BridgeTransactionV2.sol#2)
- ^0.8.4 (contracts/libs/Errors.sol#2)
- ^0.8.4 (contracts/utils/MulticallTarget.sol#2)
🔇 Additional comments (15)
packages/contracts-rfq/contracts/interfaces/IAdminV2Errors.sol (1)
1-2: LGTM! License and compiler version are appropriate.
The MIT license is suitable for open-source contracts, and Solidity version ^0.8.4 provides built-in overflow checks.
packages/contracts-rfq/contracts/interfaces/IAdminV2.sol (2)
4-4: LGTM!
Interface name follows standard naming convention and aligns with V2 versioning.
5-7: LGTM! Well-structured events
Events follow best practices by including old/new values for state changes and providing comprehensive information for off-chain tracking.
packages/contracts-rfq/test/FastBridgeV2.Src.RefundV1.t.sol (1)
8-11: Verify the continued need for V1 refund compatibility testing
The implementation calls the deprecated refund function while the PR is transitioning to the new cancel mechanism. This appears intentional for backward compatibility testing, but we should verify if this test will be needed long-term.
Run this script to check the usage of both methods across the codebase:
✅ Verification successful
V1 refund compatibility test should be maintained
The refund function is explicitly marked as deprecated in FastBridgeV2.sol but is intentionally kept for backward compatibility, with cancel being the new recommended method. The FastBridgeV2SrcRefundV1Test serves as a regression test to ensure this backward compatibility continues to work correctly. This test should be maintained until the refund function is completely removed in a future version.
Key findings:
- The
refundfunction inFastBridgeV2.solis marked with: "Note: this function is deprecated and will be removed in a future version" - It internally calls
cancel, acting as a compatibility wrapper - The test ensures this compatibility layer continues to work as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of refund vs cancel methods to understand the migration status
echo "Checking refund method usage:"
rg -p "\.refund\(" --type solidity
echo -e "\nChecking cancel method usage:"
rg -p "\.cancel\(" --type solidity
echo -e "\nChecking for any remaining V1 compatibility tests:"
fd -e sol | rg -l "V1Test"
Length of output: 427
Script:
#!/bin/bash
# Let's try a different approach using plain grep to search for these patterns
echo "Checking refund method usage:"
find . -type f -name "*.sol" -exec grep -l "\.refund(" {} \;
echo -e "\nChecking cancel method usage:"
find . -type f -name "*.sol" -exec grep -l "\.cancel(" {} \;
echo -e "\nChecking for V1 related files and tests:"
find . -type f -name "*.sol" -exec grep -l "V1" {} \;
Length of output: 1249
Script:
#!/bin/bash
# Let's examine the actual implementation of refund and cancel in FastBridgeV2
echo "Checking FastBridgeV2 contract implementation:"
cat ./packages/contracts-rfq/contracts/FastBridgeV2.sol
echo -e "\nChecking the specific V1 refund test implementation:"
cat ./packages/contracts-rfq/test/FastBridgeV2.Src.RefundV1.t.sol
Length of output: 24461
packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (3)
12-13: LGTM! Well-documented test constant.
The constant value is appropriately set above the minimum required 1 hour and uses a distinct value for testing purposes, which is clearly explained in the comment.
32-35: LGTM! Proper role configuration sequence.
The changes correctly implement the role-based access control for cancellation:
- Grant the canceler role
- Grant governor role
- Set the cancel delay
99-101: LGTM! Consistent test helper implementation.
The cancel function follows the established pattern of other test helpers in the file:
- Uses consistent transaction encoding
- Properly sets up the caller context
- Allows for override in derived test cases through
virtualmodifier
packages/contracts-rfq/test/FastBridgeV2.GasBench.Src.t.sol (2)
155-162:
Status enum inconsistency with function naming
While the function has been renamed from refund to cancel, the status check still uses BridgeStatus.REFUNDED. This creates an inconsistency between the function naming and the status enum.
Consider updating the status enum to maintain consistency:
- assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REFUNDED);
+ assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.CANCELLED);Likely invalid or redundant comment.
239-246: Verify ETH balance checks in permissioned cancellation
The test verifies ETH balances correctly but should also ensure that the canceler's balance remains unchanged since they're performing an administrative action.
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (3)
Line range hint 805-819: LGTM: Cancel functionality helper implementation is thorough.
The checkStatusAndProofAfterCancel helper function properly verifies all state changes after cancellation.
871-925: LGTM: ETH cancellation tests mirror token tests appropriately.
The tests maintain consistency with token cancellation tests while properly handling ETH-specific balance checks.
1035-1052: LGTM: Invalid payload tests ensure proper version handling.
The tests comprehensively verify version compatibility and invalid request handling for the cancel functionality.
packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
9-9: Migration to AdminV2
The contract now imports and inherits from AdminV2 instead of Admin. Ensure that all functionalities, access controls, and initializations from Admin are correctly migrated and compatible with AdminV2.
47-47: Constructor updated to initialize AdminV2
The constructor now initializes AdminV2 with the _owner parameter. This change appears correct and aligns with the migration to AdminV2.
104-107: Deprecation of refund function
The refund function is deprecated and now calls cancel(request). Ensure that any external contracts or interfaces using refund are updated to use cancel to maintain compatibility.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/contracts-rfq/contracts/AdminV2.sol (1)
70-75: Follow CEI pattern in _setCancelDelayConsider reordering the operations to follow the Checks-Effects-Interactions pattern:
- Checks (delay validation)
- Store old value
- Effects (state update)
- Event emission
function _setCancelDelay(uint256 newCancelDelay) private { if (newCancelDelay < MIN_CANCEL_DELAY) revert CancelDelayBelowMin(); uint256 oldCancelDelay = cancelDelay; cancelDelay = newCancelDelay; - emit CancelDelayUpdated(oldCancelDelay, newCancelDelay); + emit CancelDelayUpdated(oldCancelDelay, newCancelDelay); }packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
101-104: LGTM: Clean deprecation handlingGood approach to maintain backward compatibility by redirecting to the new
cancelfunction.Consider adding a deprecation timeline to the comment, e.g., "will be removed in version X.X.X".
357-357: Consider enhancing event emissionThe
BridgeDepositRefundedevent could be more informative for off-chain tracking.Consider adding these parameters to the event:
-emit BridgeDepositRefunded(transactionId, to, token, amount); +emit BridgeDepositRefunded( + transactionId, + to, + token, + amount, + request.originAmount(), + request.originFeeAmount() +);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/contracts-rfq/contracts/AdminV2.sol(1 hunks)packages/contracts-rfq/contracts/FastBridgeV2.sol(5 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-10-14T14:45:57.214Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
🔇 Additional comments (6)
packages/contracts-rfq/contracts/AdminV2.sol (3)
4-10: LGTM: Imports are well-organized and appropriate
The imports are logically grouped with interfaces first, followed by OpenZeppelin contracts. The imported contracts provide essential functionality for access control, safe token transfers, and address utilities.
22-25: LGTM: Well-defined constants with reasonable limits
The constants are well-defined with:
- Fee rate maximum capped at 1% (0.01e6) for user protection
- Minimum cancel delay of 1 hour provides reasonable security
- Default cancel delay of 1 day allows sufficient time for normal operations
49-54: LGTM: Well-implemented fee rate update
The function follows security best practices:
- Proper access control with GOVERNOR_ROLE
- Input validation against maximum fee rate
- Follows check-effects-interaction pattern
- Emits appropriate event with old and new values
packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
18-18: LGTM: Contract inheritance updated correctly
The contract now inherits from AdminV2 and the constructor has been updated accordingly to support the new cancellation features.
Also applies to: 44-44
338-364: LGTM: Cancel function implementation is secure
The function correctly implements permissionless cancellation with appropriate security checks:
- Status verification
- Deadline validation
- Role-based access control
- Proper handling of both native and ERC20 tokens
- Follows checks-effects-interactions pattern
While the function uses block.timestamp, the potential manipulation window (few seconds) is acceptable given the minimum cancelDelay of 1 hour.
386-387: LGTM: Documentation updated accurately
The function documentation has been updated to reflect the new cancellation functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol (1)
Line range hint
19-35: Consider simplifying the BridgeTransactionV2 mappingThe manual field-by-field mapping could be simplified to reduce code duplication and improve maintainability.
Consider using a struct casting or a constructor-based approach:
- IFastBridgeV2.BridgeTransactionV2 memory bridgeTxV2; - bridgeTxV2.originChainId = bridgeTx.originChainId; - bridgeTxV2.destChainId = bridgeTx.destChainId; - bridgeTxV2.originSender = bridgeTx.originSender; - bridgeTxV2.destRecipient = bridgeTx.destRecipient; - bridgeTxV2.originToken = bridgeTx.originToken; - bridgeTxV2.destToken = bridgeTx.destToken; - bridgeTxV2.originAmount = bridgeTx.originAmount; - bridgeTxV2.destAmount = bridgeTx.destAmount; - bridgeTxV2.originFeeAmount = bridgeTx.originFeeAmount; - bridgeTxV2.deadline = bridgeTx.deadline; - bridgeTxV2.nonce = bridgeTx.nonce; + IFastBridgeV2.BridgeTransactionV2 memory bridgeTxV2 = IFastBridgeV2.BridgeTransactionV2({ + originChainId: bridgeTx.originChainId, + destChainId: bridgeTx.destChainId, + originSender: bridgeTx.originSender, + destRecipient: bridgeTx.destRecipient, + originToken: bridgeTx.originToken, + destToken: bridgeTx.destToken, + originAmount: bridgeTx.originAmount, + destAmount: bridgeTx.destAmount, + originFeeAmount: bridgeTx.originFeeAmount, + deadline: bridgeTx.deadline, + nonce: bridgeTx.nonce + });packages/contracts-rfq/contracts/AdminV2.sol (2)
55-60: Emit event before state change in setProtocolFeeRateFor better transaction tracking, emit the event before updating the state variable. This ensures that even if the transaction fails after the state update, the event accurately reflects the last known good state.
function setProtocolFeeRate(uint256 newFeeRate) external onlyRole(GOVERNOR_ROLE) { if (newFeeRate > FEE_RATE_MAX) revert FeeRateAboveMax(); uint256 oldFeeRate = protocolFeeRate; + emit FeeRateUpdated(oldFeeRate, newFeeRate); protocolFeeRate = newFeeRate; - emit FeeRateUpdated(oldFeeRate, newFeeRate); }
78-83: Emit event before state change in _setCancelDelaySimilar to setProtocolFeeRate, emit the event before updating the state variable to ensure accurate transaction tracking.
function _setCancelDelay(uint256 newCancelDelay) private { if (newCancelDelay < MIN_CANCEL_DELAY) revert CancelDelayBelowMin(); uint256 oldCancelDelay = cancelDelay; + emit CancelDelayUpdated(oldCancelDelay, newCancelDelay); cancelDelay = newCancelDelay; - emit CancelDelayUpdated(oldCancelDelay, newCancelDelay); }packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
101-104: Consider adding compiler deprecation warningWhile the function is properly documented as deprecated, consider adding the
@deprecatedtag or the Soliditydeprecatedattribute for better tooling support.- /// Note: this function is deprecated and will be removed in a future version. + /// @deprecated This function is deprecated and will be removed in a future version. + /// Use `cancel` instead. /// @inheritdoc IFastBridge + #[deprecated] function refund(bytes calldata request) external {packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (3)
820-832: Fix typo in function name "permisionless".The test function names contain a typo: "permisionless" should be "permissionless".
Apply this diff to fix the typo:
- function test_cancel_token_permisionless(address caller) public { + function test_cancel_token_permissionless(address caller) public {Otherwise, the token cancellation tests provide comprehensive coverage of various scenarios.
Also applies to: 833-845, 846-857, 858-870
871-884: Fix typo in function name "permisionless".The test function names contain a typo: "permisionless" should be "permissionless".
Apply this diff to fix the typo:
- function test_cancel_eth_permisionless(address caller) public { + function test_cancel_eth_permissionless(address caller) public {Otherwise, the ETH cancellation tests provide comprehensive coverage of various scenarios.
Also applies to: 885-898, 899-911, 912-925
926-974: Fix typo in function name "permisionless".The test function names contain a typo: "permisionless" should be "permissionless".
Apply this diff to fix the typo:
- function test_cancel_revert_justBeforeDeadline_permisionless(address caller) public { + function test_cancel_revert_justBeforeDeadline_permissionless(address caller) public {Otherwise, the revert tests provide comprehensive coverage of error conditions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
packages/contracts-rfq/contracts/AdminV2.sol(1 hunks)packages/contracts-rfq/contracts/FastBridgeV2.sol(6 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol(3 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.t.sol(7 hunks)packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol
🧰 Additional context used
📓 Learnings (1)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
🔇 Additional comments (7)
packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol (1)
12-12: Verify PROVER_ROLE consistency across the codebase
The change from RELAYER_ROLE to PROVER_ROLE is a breaking change that affects access control.
packages/contracts-rfq/contracts/AdminV2.sol (1)
14-15: Verify native token handling across different chains
The hardcoded NATIVE_GAS_TOKEN address is used across different chains. Ensure that all chains where this contract will be deployed properly handle this address convention for their native tokens.
packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
6-6: LGTM: AdminV2 integration looks correct
The migration from Admin to AdminV2 is properly implemented with correct inheritance and constructor initialization.
Also applies to: 18-18, 44-44
337-364: LGTM: Cancel function implementation is secure and efficient
The implementation follows best practices:
- Proper access control with CANCELER_ROLE
- Follows checks-effects-interactions pattern
- Correctly handles both ERC20 and native tokens
- Efficient storage access pattern
Note: While the function uses block.timestamp, the risk is minimal given:
- The long duration of
cancelDelay(minimum 1 hour) - Miner manipulation window is small (few seconds)
386-387: LGTM: Documentation update is accurate
The function documentation has been properly updated to reflect the new cancel functionality.
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (2)
Line range hint 807-819: LGTM! Well-structured helper function.
The checkStatusAndProofAfterCancel function thoroughly verifies all necessary state variables after a cancellation operation.
1035-1052: LGTM! Comprehensive version compatibility tests.
The tests thoroughly verify that the cancel operation correctly handles different request versions and invalid payloads.
| constructor(address _owner) { | ||
| _grantRole(DEFAULT_ADMIN_ROLE, _owner); | ||
| _setCancelDelay(DEFAULT_CANCEL_DELAY); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add zero-address validation for owner
The constructor should validate that _owner is not the zero address to prevent potential access control issues.
constructor(address _owner) {
+ if (_owner == address(0)) revert InvalidOwner();
_grantRole(DEFAULT_ADMIN_ROLE, _owner);
_setCancelDelay(DEFAULT_CANCEL_DELAY);
}Committable suggestion skipped: line range outside the PR's diff.
|
also retaining RELAYER_ROLE so we can continue to use that for offchain permissions such as quoting. It is not used within the contract any longer |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/contracts-rfq/contracts/AdminV2.sol (2)
34-35: Fix typo in documentationThe word "permisionlessly" has a typo and should be "permissionlessly".
- /// @notice Delay for a transaction after which it could be permisionlessly cancelled + /// @notice Delay for a transaction after which it could be permissionlessly cancelled
70-70: Clarify the "last transaction action" commentThe comment "Sweep the fees as the last transaction action" is unclear. Consider explaining why this needs to be the last action or remove the comment if it's not essential.
- /// Sweep the fees as the last transaction action + /// Transfer the fees after updating state to follow checks-effects-interactions pattern
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/contracts-rfq/contracts/AdminV2.sol(1 hunks)
🔇 Additional comments (2)
packages/contracts-rfq/contracts/AdminV2.sol (2)
11-27: LGTM! Well-structured constants and roles definition.
The constants are well-defined with clear purposes. The fee rate maximum of 1% and minimum cancel delay of 1 hour provide good security boundaries while maintaining usability.
78-84: LGTM! Well-implemented private function.
The _setCancelDelay function has proper validation and follows best practices by emitting events after state changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/contracts-rfq/contracts/AdminV2.sol (2)
75-80: Consider emitting event before state changeFor better transaction tracking and event monitoring, consider emitting the event before updating the state variable. This ensures that even if the transaction fails after the state update, the event accurately reflects the last known good state.
function setProtocolFeeRate(uint256 newFeeRate) external onlyRole(GOVERNOR_ROLE) { if (newFeeRate > FEE_RATE_MAX) revert FeeRateAboveMax(); uint256 oldFeeRate = protocolFeeRate; + emit FeeRateUpdated(oldFeeRate, newFeeRate); protocolFeeRate = newFeeRate; - emit FeeRateUpdated(oldFeeRate, newFeeRate); }
98-103: Apply consistent event emission patternFor consistency with the previous suggestion, consider emitting the event before updating the state variable.
function _setCancelDelay(uint256 newCancelDelay) private { if (newCancelDelay < MIN_CANCEL_DELAY) revert CancelDelayBelowMin(); uint256 oldCancelDelay = cancelDelay; + emit CancelDelayUpdated(oldCancelDelay, newCancelDelay); cancelDelay = newCancelDelay; - emit CancelDelayUpdated(oldCancelDelay, newCancelDelay); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/contracts-rfq/contracts/AdminV2.sol(1 hunks)
🔇 Additional comments (2)
packages/contracts-rfq/contracts/AdminV2.sol (2)
4-10: LGTM! Import statements are well-organized.
The imports are logically grouped between custom interfaces and OpenZeppelin contracts.
37-40: Verify if 1% maximum fee rate aligns with business requirements
The maximum fee rate is hardcoded to 1%. While this seems reasonable, it's worth verifying if this limit aligns with the business requirements and market conditions.
✅ Verification successful
Let me gather more information about fee-related configurations in the codebase.
Based on the comprehensive search results, I can now provide a final response:
1% maximum protocol fee rate is consistent with the protocol's design and implementation
The 1% maximum fee rate is well-established across the codebase:
- It's consistently defined as
FEE_RATE_MAX = 0.01e6in both V1 and V2 contracts - The fee structure is well-documented and integrated into the protocol's core functionality
- The rate is used in production contracts and their test counterparts
- The implementation shows careful consideration of fee handling, including proper accumulation and governor-controlled management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing fee rate configurations or discussions
rg -i "fee.*rate|protocol.*fee" --type md
Length of output: 270
Script:
#!/bin/bash
# Search for fee-related constants and configurations in Solidity files
rg -U "fee.*rate|protocol.*fee" --type solidity -A 3 -B 3
# Search specifically for FEE_RATE_MAX in other contracts
ast-grep --pattern 'uint256 public constant FEE_RATE_MAX = $_'
Length of output: 650
Script:
#!/bin/bash
# Search for fee-related constants and configurations with .sol extension
rg -U "fee.*rate|protocol.*fee" -g "*.sol" -A 3 -B 3
# Search for any documentation about fees in the entire codebase
rg -i "protocol fee|maximum fee|fee rate" -g "!*.{json,lock}"
Length of output: 48028
* feat: scaffold `ZapData` library * test: define expected behavior for ZapDataV1 * feat: encoding, validation * feat: decoding * feat: scaffold `TokenZap` * test: add coverage for TokenZap * feat: expose encoding/decoding * feat: implement `zap` * fix: noAmount test, slight refactor * test: scenarios where target contract reverts * test: extra/missing funds scenarios * refactor: TokenZap -> TokenZapV1 * test: FastBridgeV2 + TokenZapV1 integration * fix: should revert when zero target in encoding * chore: docs * added target != addr 0 assumptions * added one more target != addr 0 assumption * refactor: relax ZapData pragma * docs: improve grammar in ZapDataV1 comments * test: adapt to #3382 * docs: NatSpec, fixing errors --------- Co-authored-by: parodime <[email protected]>
Description
cancelDelaypasses.cancelDelayis configurable by the governanceMisc additional deprecations that became possible by dropping old
Admintemplate:chainGasAmountandsetChainGasAmountare deprecated, as they are replaced by the "zap native" V2 feature.setProtocolFeeRateuses a custom error instead of a string error message.Summary by CodeRabbit
New Features
AdminV2smart contract for enhanced access control and fee management.cancelfunction to replace the previous refund mechanism inFastBridgeV2, allowing cancellation of transactions.IFastBridgeV2interface with new transaction management capabilities.Bug Fixes
Documentation
Tests