feat(contracts-rfq): relay/prove/claim with different address [SLT-130] #3138
Conversation
feat: fast bridge v2 tests [SLT-130]
WalkthroughThe pull request introduces the Changes
Poem
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Outside diff range and nitpick comments (15)
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2)
19-21: Ensure consistent parameter usage across functionsIn the
claimfunction, therelayeraddress parameter is omitted, unlike in therelayandprovefunctions. If therelayeris required for claiming the originally deposited capital, consider including it as a parameter for consistency and clarity. Alternatively, if therelayeris intended to be identified viamsg.senderduring implementation, evaluate whether the same approach should be applied to the other functions.
8-21: Enhance parameter descriptions for clarityThe current Natspec comments provide basic descriptions. To improve clarity for developers, consider enhancing the parameter descriptions by specifying data types and expected formats where applicable.
Apply the following changes to the comments:
/// @notice Relays destination side of bridge transaction by off-chain relayer - /// @param request The encoded bridge transaction to relay on destination chain + /// @param request The ABI-encoded bridge transaction data to relay on the destination chain /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed function relay(bytes memory request, address relayer) external payable; /// @notice Provides proof on origin side that relayer provided funds on destination side of bridge transaction - /// @param request The encoded bridge transaction to prove on origin chain + /// @param request The ABI-encoded bridge transaction data that was relayed /// @param destTxHash The destination transaction hash proving the bridge transaction was relayed /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed function prove(bytes memory request, bytes32 destTxHash, address relayer) external; /// @notice Completes bridge transaction on origin chain by claiming originally deposited capital. Can only send funds to the relayer address on the proof. - /// @param request The encoded bridge transaction to claim on origin chain + /// @param request The ABI-encoded bridge transaction data to claim on the origin chain function claim(bytes memory request) external;packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol (1)
6-6: Re-evaluate the need to disable Solhint rulesThe directive
// solhint-disable func-name-mixedcase, orderingdisables thefunc-name-mixedcaseandorderingrules. If possible, consider adhering to the standard naming conventions and function ordering to maintain code consistency and readability, rather than disabling these linting rules.packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
50-53: Add comments to clarify the purpose of empty virtual functionsThe functions
configureFastBridge()andmintTokens()are empty but marked asvirtual. To improve code readability, consider adding comments explaining that these functions are intended to be overridden in derived test contracts to provide specific configurations or setups necessary for testing.Apply this diff to add explanatory comments:
function configureFastBridge() public virtual {} + // Override to configure the FastBridge instance with specific settings for tests function mintTokens() public virtual {} + // Override to mint tokens required for test scenariospackages/contracts-rfq/test/FastBridgeV2.Management.t.sol (2)
95-102: Consider Edge Cases in Fee Sweeping TestsIn
test_sweepProtocolFees_erc20andtest_sweepProtocolFees_eth, the tests assume that there are protocol fees collected. Ensure that cases where no fees are collected are also tested to confirm the functions handle zero balances gracefully.Consider adding tests for zero protocol fees:
+function test_sweepProtocolFees_zeroFees() public { + // Ensure protocol fees are zero + assertEq(fastBridge.protocolFees(address(srcToken)), 0); + vm.expectRevert("No fees to sweep"); + sweepProtocolFees(governor, address(srcToken), governorA); +}Also applies to: 104-111
53-57: Missing Assertion Message in Role Assignment TestIn
test_grantGovernorRole, the assertions lack custom messages. Adding messages can aid in debugging if the test fails.Enhance assertions with messages:
-assertFalse(fastBridge.hasRole(GOVERNOR_ROLE, governorA)); +assertFalse(fastBridge.hasRole(GOVERNOR_ROLE, governorA), "GovernorA should not have the role before assignment");packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (1)
40-53: ParameterizechainGasAmountinexpectBridgeRelayed()for flexibilityCurrently,
chainGasAmountis hardcoded to0in theexpectBridgeRelayedfunction. To enhance flexibility and allow testing of scenarios with differentchainGasAmountvalues, consider parameterizing this field.Apply the following diff to modify the function:
function expectBridgeRelayed(IFastBridge.BridgeTransaction memory bridgeTx, bytes32 txId, address relayer) public { vm.expectEmit(address(fastBridge)); emit BridgeRelayed({ transactionId: txId, relayer: relayer, to: bridgeTx.destRecipient, originChainId: bridgeTx.originChainId, originToken: bridgeTx.originToken, destToken: bridgeTx.destToken, originAmount: bridgeTx.originAmount, destAmount: bridgeTx.destAmount, - chainGasAmount: 0 + chainGasAmount: bridgeTx.chainGasAmount }); }Ensure that
bridgeTxincludes thechainGasAmountfield and update any calls toexpectBridgeRelayedaccordingly.packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
183-191: Improve Event Logging inproveFunctionThe
BridgeProofProvidedevent does not emit thedestTxHash, which could be valuable for tracking purposes.Recommendation:
Include
destTxHashin the emitted event.event BridgeProofProvided( bytes32 indexed transactionId, address indexed relayer, + bytes32 destTxHash ); - emit BridgeProofProvided(transactionId, relayer, destTxHash); + emit BridgeProofProvided(transactionId, relayer, destTxHash);
251-260: Clarify Event Parameters indisputeFunctionThe
BridgeProofDisputedevent could include additional information to aid in debugging and tracking.Recommendation:
Include the
relayerandreasonin the event.event BridgeProofDisputed( bytes32 indexed transactionId, address indexed disputer, + address indexed relayer ); emit BridgeProofDisputed(transactionId, msg.sender, bridgeProofs[transactionId].relayer);packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (6)
226-227: Implement thetest_bridge_userSpecificNoncetestThe test
test_bridge_userSpecificNonceis currently skipped with a TODO comment. Implement this test to verify that user-specific nonces are handled correctly in bridging operations.Do you want me to help implement this test or open a GitHub issue to track this task?
286-290: Resolve the skipped test intest_bridge_revert_zeroSenderThe test
test_bridge_revert_zeroSenderis skipped with a TODO comment. Address this to ensure the contract correctly handles cases where the sender address is zero.Do you need assistance in fixing this test or should I open a GitHub issue to track it?
293-297: Address the skipped test intest_bridge_revert_zeroRecipientThe test
test_bridge_revert_zeroRecipientis skipped with a TODO comment. Implement this test to confirm that the contract properly reverts when the recipient address is zero.Would you like me to assist in implementing this test or create a GitHub issue for it?
803-806: Unskip and fix thetest_refund_revert_statusNulltestThe test
test_refund_revert_statusNullis skipped with a TODO comment. Resolve the underlying issue to enable this test and ensure proper refund behavior when the status isNULL.Can I assist in fixing this test or should we open a GitHub issue to track this task?
809-814: Unskip and fix thetest_refund_revert_statusProventestThis test is currently skipped. Implementing it will help verify that refunds properly revert when the status is
PROVEN.Would you like assistance in addressing this test or should a GitHub issue be created?
817-824: Unskip and fix thetest_refund_revert_statusClaimedtestThe test is skipped pending a fix. Ensure that refunds correctly revert when the status is
CLAIMEDby resolving this test.Let me know if you need help implementing this test or if a GitHub issue needs to be opened.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (19)
- .vscode/settings.json (1 hunks)
- packages/contracts-rfq/contracts/Admin.sol (1 hunks)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IAdmin.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IMulticallTarget.sol (1 hunks)
- packages/contracts-rfq/contracts/libs/Errors.sol (1 hunks)
- packages/contracts-rfq/contracts/libs/UniversalToken.sol (1 hunks)
- packages/contracts-rfq/contracts/utils/MulticallTarget.sol (1 hunks)
- packages/contracts-rfq/script/ConfigureFastBridge.s.sol (1 hunks)
- packages/contracts-rfq/script/DeployFastBridge.CREATE2.s.sol (1 hunks)
- packages/contracts-rfq/test/FastBridge.t.sol (4 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Management.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.ProtocolFees.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (1 hunks)
Files skipped from review due to trivial changes (9)
- packages/contracts-rfq/contracts/Admin.sol
- packages/contracts-rfq/contracts/interfaces/IAdmin.sol
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol
- packages/contracts-rfq/contracts/interfaces/IMulticallTarget.sol
- packages/contracts-rfq/contracts/libs/Errors.sol
- packages/contracts-rfq/contracts/libs/UniversalToken.sol
- packages/contracts-rfq/contracts/utils/MulticallTarget.sol
- packages/contracts-rfq/script/ConfigureFastBridge.s.sol
- packages/contracts-rfq/script/DeployFastBridge.CREATE2.s.sol
Additional comments not posted (26)
.vscode/settings.json (1)
21-21: Verify compatibility with Solidity v0.8.24The Solidity compiler version has been updated to
v0.8.24. Please ensure that all contracts compile successfully with this new version and review the Solidity release notes for any potential breaking changes or deprecations that might affect the codebase.packages/contracts-rfq/test/FastBridgeV2.Src.ProtocolFees.t.sol (2)
16-24: Ensure transaction amounts align with the 1% protocol fee rateIn
createFixtures(), the transaction amounts and fees are set manually. Please verify that these amounts correctly reflect the 1% protocol fee rate configured earlier. Consistency between the fee rate and the fixture amounts is crucial for accurate testing.For example, for the token transaction:
- Total amount:
tokenTx.originFeeAmount + tokenTx.originAmount = 0.01e6 + 0.99e6 = 1e6- Expected fee (1% of total amount):
1e6 * 1% = 0.01e6Similarly, for the ETH transaction:
- Total amount:
ethTx.originFeeAmount + ethTx.originAmount = 0.01 ether + 0.99 ether = 1 ether- Expected fee (1% of total amount):
1 ether * 1% = 0.01 etherPlease confirm that the
destAmountvalues (tokenTx.destAmount,ethTx.destAmount) are consistent with the fee deductions as per the protocol logic.
10-11: Verify thatsetProtocolFeeRate(1e4)correctly sets the fee rate to 1%The call to
fastBridge.setProtocolFeeRate(1e4);aims to set the protocol fee rate to 1%, as indicated by the comment// 1%. Please verify that1e4corresponds to a 1% fee rate in thefastBridgecontract's context. This ensures that fee calculations in the tests align with the intended rate.To confirm how the fee rate is interpreted, run the following script to examine the
setProtocolFeeRatefunction and its usage in fee calculations:Verification successful
Verification confirms
setProtocolFeeRate(1e4)correctly sets the fee rate to 1%The codebase analysis verifies that
setProtocolFeeRate(1e4)indeed sets the protocol fee rate to 1% as intended. Here's a brief explanation:
- The fee calculations use the formula:
(amount * protocolFeeRate) / FEE_BPSor(amount * protocolFeeRate) / 1e6.FEE_BPSis equivalent to 1e6 (1 million), representing 100% in basis points.- Setting
protocolFeeRateto 1e4 (10,000) results in a 1% fee rate: 10,000 / 1,000,000 = 0.01 = 1%.The test in
FastBridgeV2.Src.ProtocolFees.t.solcorrectly sets the fee rate to 1% usingsetProtocolFeeRate(1e4).Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the unit of `protocolFeeRate` in `setProtocolFeeRate` and its application. # Search for the `setProtocolFeeRate` function definition. ast-grep --lang solidity --pattern $'function setProtocolFeeRate(uint256 feeRate) public { $$$ }' # Search for where `protocolFeeRate` is used in calculations to understand the unit scaling. ast-grep --lang solidity --pattern $'$_amount * protocolFeeRate / $_' # Expectation: The fee rate is a fraction of 1e6 (parts per million), so 1e4 should represent 1%.Length of output: 403
Script:
#!/bin/bash # Description: Verify the unit of `protocolFeeRate` in `setProtocolFeeRate` and its application. # Search for the `setProtocolFeeRate` function definition. rg "function setProtocolFeeRate" -t sol # Search for where `protocolFeeRate` is used in calculations to understand the unit scaling. rg "protocolFeeRate.*/" -t sol # Expectation: The fee rate is a fraction of 1e6 (parts per million), so 1e4 should represent 1%.Length of output: 130
Script:
#!/bin/bash # Description: Verify the unit of `protocolFeeRate` in `setProtocolFeeRate` and its application. # Search for the `setProtocolFeeRate` function definition in .sol files fd -e sol | xargs rg "function setProtocolFeeRate" -n # Search for where `protocolFeeRate` is used in calculations to understand the unit scaling. fd -e sol | xargs rg "protocolFeeRate.*/" -n # Expectation: The fee rate is a fraction of 1e6 (parts per million), so 1e4 should represent 1%.Length of output: 3416
packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
110-112: Ensure consistent hashing by verifying struct encodingIn the
getTxIdfunction, the bridge transaction is hashed usingabi.encode(bridgeTx). To ensure consistent and reliable hashing, verify that all fields withinIFastBridge.BridgeTransactionare of statically-sized types. If any fields are dynamically-sized (such asbytesorstring), the hashing may produce inconsistent results across different executions. If necessary, consider encoding each field explicitly or ensuring that only statically-sized types are used.packages/contracts-rfq/test/FastBridgeV2.Management.t.sol (3)
83-84: Incorrect Revert Message ExpectationIn
test_setProtocolFeeRate_revert_tooHigh, you're expecting the revert message"newFeeRate > max". Ensure that the actual revert message from thesetProtocolFeeRatefunction matches this string exactly. Any mismatch will cause the test to fail unexpectedly.Run this script to confirm the revert message in the
FastBridgeV2contract:Verification successful
Revert Message Expectation is Correct
The review comment's concern about the revert message expectation is unnecessary. The expected message "newFeeRate > max" in the test file matches exactly with the actual implementation in the
Admin.solcontract, which is likely the base contract forFastBridgeV2.
- The
setProtocolFeeRatefunction inpackages/contracts-rfq/contracts/Admin.soluses the exact revert message:"newFeeRate > max".- This message is consistently used across multiple contract versions and test contracts.
The test
test_setProtocolFeeRate_revert_tooHighis correctly expecting the revert message, and no changes are needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the revert message in `setProtocolFeeRate`. # Test: Search for the revert message. Expect: Only one occurrence matching the expected string. rg --type solidity -A2 $'function setProtocolFeeRate' | rg 'require.*newFeeRate.*'Length of output: 116
Script:
#!/bin/bash # Description: Verify the revert message in `setProtocolFeeRate` for Solidity files # Find Solidity files and search for the setProtocolFeeRate function and its revert condition fd -e sol | xargs rg -A5 'function setProtocolFeeRate' | rg 'require.*newFeeRate.*'Length of output: 703
39-40: Ensure Proper Access Control TestingThe functions
setProtocolFeeRateandtest_setProtocolFeeRate_revert_notGovernorrely on access control. Verify that thefastBridge.GOVERNOR_ROLE()call correctly references the governor role. Additionally, confirm that unauthorized callers are properly restricted in both the implementation and tests.Run this script to check the access control implementation:
Also applies to: 88-89
Verification successful
Access Control for setProtocolFeeRate and GOVERNOR_ROLE Verified
The verification process confirms that proper access control is implemented for the
setProtocolFeeRatefunction and theGOVERNOR_ROLE:
- The
GOVERNOR_ROLEconstant is correctly defined and consistently used across the codebase.- The
setProtocolFeeRatefunction inAdmin.soluses theonlyRole(GOVERNOR_ROLE)modifier, ensuring only governors can call it.- The
test_setProtocolFeeRate_revert_notGovernorfunction inFastBridgeV2.Management.t.solproperly tests for unauthorized access.These implementations ensure that only authorized callers (governors) can modify the protocol fee rate, while unauthorized callers are correctly restricted.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `GOVERNOR_ROLE` is correctly set and used. # Test: Search for `GOVERNOR_ROLE` usage in `FastBridgeV2` contract. rg --type solidity 'bytes32 public constant GOVERNOR_ROLE' -A5 # Test: Ensure `grantRole` and `hasRole` are used with `GOVERNOR_ROLE`. rg --type solidity 'grantRole\(GOVERNOR_ROLE' -A2 rg --type solidity 'hasRole\(GOVERNOR_ROLE' -A2Length of output: 265
Script:
#!/bin/bash # Description: Verify access control implementation in Solidity files # Search for GOVERNOR_ROLE constant definition echo "GOVERNOR_ROLE constant definition:" rg "bytes32 public constant GOVERNOR_ROLE" --glob "*.sol" # Check usage of GOVERNOR_ROLE in grantRole and hasRole functions echo "\nGOVERNOR_ROLE usage in grantRole:" rg "grantRole\(GOVERNOR_ROLE" --glob "*.sol" echo "\nGOVERNOR_ROLE usage in hasRole:" rg "hasRole\(GOVERNOR_ROLE" --glob "*.sol" # Verify implementation of setProtocolFeeRate function echo "\nsetProtocolFeeRate function implementation:" rg "function setProtocolFeeRate" -A 10 --glob "*.sol" # Look for test_setProtocolFeeRate_revert_notGovernor test function echo "\ntest_setProtocolFeeRate_revert_notGovernor test function:" rg "function test_setProtocolFeeRate_revert_notGovernor" -A 10 --glob "*.sol"Length of output: 16188
18-20: Ensure Proper Initialization of FastBridgeV2When deploying
FastBridgeV2, confirm that the constructor parameters and initial state are correctly set, especially theadminaddress. Any misconfiguration could lead to governance issues.Run this script to verify the constructor of
FastBridgeV2:Verification successful
Proper Initialization of FastBridgeV2 Confirmed
The constructor of
FastBridgeV2correctly passes the_adminaddress to its parentAdmincontract, which likely sets up the initial admin role. All state variables are appropriately initialized or don't require explicit initialization. The simplicity of the constructor aligns with best practices for contract deployment.For completeness, please manually verify the role setup in the
Admincontract to ensure all necessary roles are properly assigned during initialization.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the constructor of `FastBridgeV2` for proper initialization. # Test: Locate the constructor and verify the admin assignment. rg --type solidity 'constructor\(address _admin\)' -A5Length of output: 89
Script:
#!/bin/bash # Description: Locate FastBridgeV2 constructor and verify initialization # Find Solidity files and search for FastBridgeV2 constructor fd -e sol | xargs grep -n -A 5 "contract FastBridgeV2" | grep -A 10 "constructor" # Broader search for any constructor in FastBridgeV2 file fd -e sol | xargs grep -n -A 5 "contract FastBridgeV2" | grep -A 10 -i "constructor.*("Length of output: 171
Script:
#!/bin/bash # Description: Broad search for Solidity contracts and constructors # Find all Solidity contract definitions echo "Searching for Solidity contracts:" fd -e sol | xargs grep -n "contract.*{" # Find all constructor definitions in Solidity files echo "Searching for constructors in Solidity files:" fd -e sol | xargs grep -n -i "constructor.*(" # Search for files containing "FastBridge" in their name echo "Searching for FastBridge files:" fd -e sol FastBridgeLength of output: 68809
Script:
#!/bin/bash # Description: Examine the Admin contract and FastBridgeV2 contract # Check the Admin contract constructor and relevant functions echo "Admin contract implementation:" rg -A 10 "contract Admin" packages/contracts-rfq/contracts/Admin.sol # Check FastBridgeV2 for any important state variables echo "FastBridgeV2 state variables:" rg "^\s*(uint|int|address|bool|bytes|string)" packages/contracts-rfq/contracts/FastBridgeV2.solLength of output: 1940
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (10)
24-27: Initialization insetUp()is correctly implementedThe
setUpfunction properly sets the chain ID toDST_CHAIN_IDand initializes the test environment by callingsuper.setUp(). This ensures that the destination chain context is accurately simulated for the tests.
29-31: Correct deployment ofFastBridgeV2contractThe
deployFastBridgefunction deploys a new instance of theFastBridgeV2contract usingaddress(this)as the constructor argument. This is appropriate since the contract may require a reference to the deploying contract for testing purposes.
33-38: Proper token minting and approvals inmintTokens()The
mintTokensfunction correctly mints the necessary token balances forrelayerAand sets up ETH forrelayerB. The use ofvm.prank(relayerA)ensures that theapprovecall is made from therelayerAaddress, allowing thefastBridgecontract to spend the tokens on behalf ofrelayerA.
74-83:test_relay_token()accurately tests ERC20 token relay functionalityThe
test_relay_tokenfunction effectively verifies that an ERC20 token bridge request is correctly processed byrelayerA. It checks the event emission, updates to thebridgeRelaysmapping, and the token balances ofuserB,relayerA, and thefastBridgecontract.
85-94:test_relay_token_withRelayerAddress()validates relaying with a specified relayer addressThis test ensures that
relayerAcan relay a transaction while specifyingrelayerBas the relayer address. It confirms that theBridgeRelayedevent reflects the correct relayer and that token balances are updated appropriately.
96-105:test_relay_eth()correctly tests ETH bridge relayThe test function successfully validates that
relayerBcan relay an ETH bridge request. It checks for correct event emission, updates to thebridgeRelaysmapping, and verifies the ETH balances ofuserB,relayerB, and thefastBridgecontract.
107-116:test_relay_eth_withRelayerAddress()tests relaying ETH with a specified relayer addressThis test confirms that
relayerBcan relay an ETH transaction while specifyingrelayerAas the relayer. It ensures the event logs and balance changes reflect the intended behavior.
120-124:test_relay_revert_chainIncorrect()correctly checks for chain ID mismatchesThe test effectively verifies that attempting to relay a transaction on the wrong chain (
SRC_CHAIN_IDinstead ofDST_CHAIN_ID) results in a revert with theChainIncorrecterror, ensuring the contract's chain validation logic is functioning.
126-130:test_relay_revert_transactionRelayed()ensures duplicate transactions are preventedThis test appropriately checks that attempting to relay the same transaction twice results in a revert with the
TransactionRelayederror, preventing duplicate processing of bridge transactions.
132-136:test_relay_revert_deadlineExceeded()validates deadline enforcementThe test confirms that if a relay attempt is made after the
DEADLINEhas passed, the contract correctly reverts with theDeadlineExceedederror, enforcing the timing constraints of the relay process.packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
244-245: Ensure Safe Token TransfersUsing
universalTransfermay not handle all ERC20 tokens safely, especially those that do not return a boolean.Recommendation:
Confirm that
universalTransfercorrectly handles tokens that do not adhere to the ERC20 standard fully. Consider usingSafeERC20.safeTransferfor improved safety.
82-83: Validate Token Addresses Against Known StandardsThe checks for zero addresses in
originTokenanddestTokencould be extended to validate that the tokens conform to ERC20 standards.Recommendation:
Implement additional validation to ensure tokens comply with expected interfaces.
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1)
742-752: Verify the refund recipient intest_refund_eth_diffSenderIn this test, the refund is sent to
userA, even thoughuserBinitiated the bridge. Confirm thatethParams.senderis correctly set touserAand that this behavior is intended.Please double-check that
ethParams.senderis set touserAas expected, and that the refund logic aligns with the intended functionality.packages/contracts-rfq/test/FastBridge.t.sol (6)
31-33: Enhancing test setup extensibility by making 'setUp' virtualThe
setUp()method is now marked asvirtual, allowing derived test contracts to override it. This increases test flexibility and modularity. InitializingfastBridgeusing thedeployFastBridge()method enhances customization.
39-41: Introducing 'deployFastBridge' method for customizable deploymentThe new
deployFastBridge()method is marked asinternal virtual, enabling derived contracts to override and customize the deployment logic of theFastBridge. This improves modularity and supports different deployment scenarios in tests.
43-43: Making '_mintTokensToActors' virtual for flexible token distributionBy marking
_mintTokensToActors()asvirtual, you allow derived test contracts to override the token minting process. This enhances flexibility for testing with different token distributions or amounts.
1304-1304: Allowing 'test_failedRelayNotRelayer' to be overridden in derived testsMarking
test_failedRelayNotRelayer()asvirtualenables derived test contracts to customize or extend the test behavior. This provides greater flexibility in testing different failure scenarios related to relayers.
1649-1649: Enabling override of 'test_failedClaimNotOldRelayer' for extended testingBy making
test_failedClaimNotOldRelayer()virtual, you allow derived test contracts to override this function. This facilitates more comprehensive testing of claim failures involving relayer roles.
1686-1686: Making 'test_failedClaimNotRelayer' virtual for customizable test behaviorMarking
test_failedClaimNotRelayer()asvirtualprovides the ability for derived test contracts to override and tailor the test logic. This enhances the extensibility of tests related to relayer authorization.
| /// @notice Relays destination side of bridge transaction by off-chain relayer | ||
| /// @param request The encoded bridge transaction to relay on destination chain | ||
| /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed | ||
| function relay(bytes memory request, address relayer) external payable; |
There was a problem hiding this comment.
Validate the necessity of the relayer parameter in relay function
The relay function includes a relayer address parameter. Since interfaces cannot implement logic, it's important to ensure that the implementation will correctly handle this parameter. Consider whether the relayer address should be obtained from msg.sender within the implementation to prevent potential misuse where a caller could specify a different relayer address, which might introduce security risks.
| /// @notice Provides proof on origin side that relayer provided funds on destination side of bridge transaction | ||
| /// @param request The encoded bridge transaction to prove on origin chain | ||
| /// @param destTxHash The destination tx hash proving bridge transaction was relayed | ||
| /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed | ||
| function prove(bytes memory request, bytes32 destTxHash, address relayer) external; |
There was a problem hiding this comment.
Assess the use of the relayer parameter in prove function
Similar to the relay function, the prove function takes a relayer address as a parameter. Verify whether it's necessary to pass the relayer explicitly, or if it should be derived from msg.sender during implementation. This could prevent scenarios where a malicious actor proves a transaction on behalf of another relayer without authorization.
| @@ -0,0 +1,23 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.20; | |||
There was a problem hiding this comment.
Update Solidity pragma version for broader compatibility
Consider using a caret (^) pragma to allow for future compatible compiler versions. Changing pragma solidity ^0.8.20; to pragma solidity ^0.8.0; can increase the range of compiler versions that can be used, providing more flexibility for developers.
Apply this change:
-pragma solidity ^0.8.20;
+pragma solidity ^0.8.0;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.20; | |
| pragma solidity ^0.8.0; |
| // SPDX-License-Identifier: MIT | ||
| pragma solidity ^0.8.20; | ||
|
|
||
| import {IFastBridge} from "./IFastBridge.sol"; | ||
|
|
||
| interface IFastBridgeV2 is IFastBridge { | ||
|
|
||
| /// @notice Relays destination side of bridge transaction by off-chain relayer | ||
| /// @param request The encoded bridge transaction to relay on destination chain | ||
| /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed | ||
| function relay(bytes memory request, address relayer) external payable; | ||
|
|
||
| /// @notice Provides proof on origin side that relayer provided funds on destination side of bridge transaction | ||
| /// @param request The encoded bridge transaction to prove on origin chain | ||
| /// @param destTxHash The destination tx hash proving bridge transaction was relayed | ||
| /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed | ||
| function prove(bytes memory request, bytes32 destTxHash, address relayer) external; | ||
|
|
||
| /// @notice Completes bridge transaction on origin chain by claiming originally deposited capital. Can only send funds to the relayer address on the proof. | ||
| /// @param request The encoded bridge transaction to claim on origin chain | ||
| function claim(bytes memory request) external; | ||
|
|
||
| } |
There was a problem hiding this comment.
Add events for important state-changing functions
Interfaces often include event declarations for significant actions to enhance transparency and enable off-chain monitoring. Consider adding events for the relay, prove, and claim functions to allow tracking when these actions occur in the implementing contracts.
Apply the following changes to declare events:
interface IFastBridgeV2 is IFastBridge {
+ /// @notice Emitted when a relay is initiated
+ /// @param request The encoded bridge transaction relayed
+ /// @param relayer The address of the relayer
+ event RelayInitiated(bytes indexed request, address indexed relayer);
/// @notice Relays destination side of bridge transaction by off-chain relayer
/// @param request The encoded bridge transaction to relay on destination chain
/// @param relayer The address of the relaying entity which should have control of the origin funds when claimed
function relay(bytes memory request, address relayer) external payable;
+ /// @notice Emitted when a proof is submitted
+ /// @param request The encoded bridge transaction proved
+ /// @param destTxHash The destination transaction hash
+ /// @param relayer The address of the relayer
+ event ProofSubmitted(bytes indexed request, bytes32 indexed destTxHash, address indexed relayer);
/// @notice Provides proof on origin side that relayer provided funds on destination side of bridge transaction
/// @param request The encoded bridge transaction to prove on origin chain
/// @param destTxHash The destination tx hash proving bridge transaction was relayed
/// @param relayer The address of the relaying entity which should have control of the origin funds when claimed
function prove(bytes memory request, bytes32 destTxHash, address relayer) external;
+ /// @notice Emitted when a claim is made
+ /// @param request The encoded bridge transaction claimed
+ /// @param claimant The address of the claimant
+ event ClaimMade(bytes indexed request, address indexed claimant);
/// @notice Completes bridge transaction on origin chain by claiming originally deposited capital. Can only send funds to the relayer address on the proof.
/// @param request The encoded bridge transaction to claim on origin chain
function claim(bytes memory request) external;
}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.
| // SPDX-License-Identifier: MIT | |
| pragma solidity ^0.8.20; | |
| import {IFastBridge} from "./IFastBridge.sol"; | |
| interface IFastBridgeV2 is IFastBridge { | |
| /// @notice Relays destination side of bridge transaction by off-chain relayer | |
| /// @param request The encoded bridge transaction to relay on destination chain | |
| /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed | |
| function relay(bytes memory request, address relayer) external payable; | |
| /// @notice Provides proof on origin side that relayer provided funds on destination side of bridge transaction | |
| /// @param request The encoded bridge transaction to prove on origin chain | |
| /// @param destTxHash The destination tx hash proving bridge transaction was relayed | |
| /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed | |
| function prove(bytes memory request, bytes32 destTxHash, address relayer) external; | |
| /// @notice Completes bridge transaction on origin chain by claiming originally deposited capital. Can only send funds to the relayer address on the proof. | |
| /// @param request The encoded bridge transaction to claim on origin chain | |
| function claim(bytes memory request) external; | |
| } | |
| // SPDX-License-Identifier: MIT | |
| pragma solidity ^0.8.20; | |
| import {IFastBridge} from "./IFastBridge.sol"; | |
| interface IFastBridgeV2 is IFastBridge { | |
| /// @notice Emitted when a relay is initiated | |
| /// @param request The encoded bridge transaction relayed | |
| /// @param relayer The address of the relayer | |
| event RelayInitiated(bytes indexed request, address indexed relayer); | |
| /// @notice Relays destination side of bridge transaction by off-chain relayer | |
| /// @param request The encoded bridge transaction to relay on destination chain | |
| /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed | |
| function relay(bytes memory request, address relayer) external payable; | |
| /// @notice Emitted when a proof is submitted | |
| /// @param request The encoded bridge transaction proved | |
| /// @param destTxHash The destination transaction hash | |
| /// @param relayer The address of the relayer | |
| event ProofSubmitted(bytes indexed request, bytes32 indexed destTxHash, address indexed relayer); | |
| /// @notice Provides proof on origin side that relayer provided funds on destination side of bridge transaction | |
| /// @param request The encoded bridge transaction to prove on origin chain | |
| /// @param destTxHash The destination tx hash proving bridge transaction was relayed | |
| /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed | |
| function prove(bytes memory request, bytes32 destTxHash, address relayer) external; | |
| /// @notice Emitted when a claim is made | |
| /// @param request The encoded bridge transaction claimed | |
| /// @param claimant The address of the claimant | |
| event ClaimMade(bytes indexed request, address indexed claimant); | |
| /// @notice Completes bridge transaction on origin chain by claiming originally deposited capital. Can only send funds to the relayer address on the proof. | |
| /// @param request The encoded bridge transaction to claim on origin chain | |
| function claim(bytes memory request) external; | |
| } |
| /// @notice Relay function is no longer permissioned, so we skip this test | ||
| function test_failedRelayNotRelayer() public virtual override { | ||
| vm.skip(true); | ||
| } |
There was a problem hiding this comment.
Consider removing obsolete test instead of skipping it
The test test_failedRelayNotRelayer is skipped because the relay function is no longer permissioned. To keep the test suite clean and maintainable, consider removing this test entirely if it's no longer relevant.
| function claim(bytes memory request) external { | ||
| claim(request, address(0)); | ||
| } | ||
|
|
||
| /// @inheritdoc IFastBridge | ||
| function claim(bytes memory request, address to) public { | ||
| bytes32 transactionId = keccak256(request); | ||
| BridgeTransaction memory transaction = getBridgeTransaction(request); | ||
|
|
||
| // update bridge tx status if able to claim origin collateral | ||
| if (bridgeStatuses[transactionId] != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); | ||
|
|
||
| BridgeProof memory proof = bridgeProofs[transactionId]; | ||
|
|
||
| // if "to" is zero addr, permissionlessly send funds to proven relayer | ||
| if (to == address(0)) { | ||
| to = proof.relayer; | ||
| } else if (proof.relayer != msg.sender) { | ||
| revert SenderIncorrect(); | ||
| } | ||
|
|
||
| if (_timeSince(proof) <= DISPUTE_PERIOD) revert DisputePeriodNotPassed(); | ||
|
|
||
| bridgeStatuses[transactionId] = BridgeStatus.RELAYER_CLAIMED; | ||
|
|
||
| // update protocol fees if origin fee amount exists | ||
| if (transaction.originFeeAmount > 0) protocolFees[transaction.originToken] += transaction.originFeeAmount; | ||
|
|
||
| // transfer origin collateral less fee to specified address | ||
| address token = transaction.originToken; | ||
| uint256 amount = transaction.originAmount; | ||
| token.universalTransfer(to, amount); | ||
|
|
||
| emit BridgeDepositClaimed(transactionId, proof.relayer, to, token, amount); | ||
| } |
There was a problem hiding this comment.
Simplify Recipient Logic in claim Function
The recipient logic in the claim function adds complexity and could be streamlined.
Recommendation:
Standardize the recipient to be the proof.relayer to reduce conditional checks.
function claim(bytes memory request) external {
- claim(request, address(0));
+ claim(request);
}
- function claim(bytes memory request, address to) public {
+ function claim(bytes memory request) public {
bytes32 transactionId = keccak256(request);
BridgeTransaction memory transaction = getBridgeTransaction(request);
// existing code
- if (to == address(0)) {
- to = proof.relayer;
- } else if (proof.relayer != msg.sender) {
- revert SenderIncorrect();
- }
+ address to = proof.relayer;
// existing code
}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 claim(bytes memory request) external { | |
| claim(request, address(0)); | |
| } | |
| /// @inheritdoc IFastBridge | |
| function claim(bytes memory request, address to) public { | |
| bytes32 transactionId = keccak256(request); | |
| BridgeTransaction memory transaction = getBridgeTransaction(request); | |
| // update bridge tx status if able to claim origin collateral | |
| if (bridgeStatuses[transactionId] != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); | |
| BridgeProof memory proof = bridgeProofs[transactionId]; | |
| // if "to" is zero addr, permissionlessly send funds to proven relayer | |
| if (to == address(0)) { | |
| to = proof.relayer; | |
| } else if (proof.relayer != msg.sender) { | |
| revert SenderIncorrect(); | |
| } | |
| if (_timeSince(proof) <= DISPUTE_PERIOD) revert DisputePeriodNotPassed(); | |
| bridgeStatuses[transactionId] = BridgeStatus.RELAYER_CLAIMED; | |
| // update protocol fees if origin fee amount exists | |
| if (transaction.originFeeAmount > 0) protocolFees[transaction.originToken] += transaction.originFeeAmount; | |
| // transfer origin collateral less fee to specified address | |
| address token = transaction.originToken; | |
| uint256 amount = transaction.originAmount; | |
| token.universalTransfer(to, amount); | |
| emit BridgeDepositClaimed(transactionId, proof.relayer, to, token, amount); | |
| } | |
| function claim(bytes memory request) external { | |
| claim(request); | |
| } | |
| /// @inheritdoc IFastBridge | |
| function claim(bytes memory request) public { | |
| bytes32 transactionId = keccak256(request); | |
| BridgeTransaction memory transaction = getBridgeTransaction(request); | |
| // update bridge tx status if able to claim origin collateral | |
| if (bridgeStatuses[transactionId] != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); | |
| BridgeProof memory proof = bridgeProofs[transactionId]; | |
| address to = proof.relayer; | |
| if (_timeSince(proof) <= DISPUTE_PERIOD) revert DisputePeriodNotPassed(); | |
| bridgeStatuses[transactionId] = BridgeStatus.RELAYER_CLAIMED; | |
| // update protocol fees if origin fee amount exists | |
| if (transaction.originFeeAmount > 0) protocolFees[transaction.originToken] += transaction.originFeeAmount; | |
| // transfer origin collateral less fee to specified address | |
| address token = transaction.originToken; | |
| uint256 amount = transaction.originAmount; | |
| token.universalTransfer(to, amount); | |
| emit BridgeDepositClaimed(transactionId, proof.relayer, to, token, amount); | |
| } |
| function relay(bytes memory request, address relayer) public payable { | ||
| bytes32 transactionId = keccak256(request); | ||
| BridgeTransaction memory transaction = getBridgeTransaction(request); | ||
| if (transaction.destChainId != uint32(block.chainid)) revert ChainIncorrect(); | ||
|
|
||
| // check haven't exceeded deadline for relay to happen | ||
| if (block.timestamp > transaction.deadline) revert DeadlineExceeded(); | ||
|
|
||
| // mark bridge transaction as relayed | ||
| if (bridgeRelays[transactionId]) revert TransactionRelayed(); | ||
| bridgeRelays[transactionId] = true; | ||
|
|
||
| // transfer tokens to recipient on destination chain and gas rebate if requested | ||
| address to = transaction.destRecipient; | ||
| address token = transaction.destToken; | ||
| uint256 amount = transaction.destAmount; | ||
|
|
||
| uint256 rebate = chainGasAmount; | ||
| if (!transaction.sendChainGas) { | ||
| // forward erc20 | ||
| rebate = 0; | ||
| _pullToken(to, token, amount); | ||
| } else if (token == UniversalTokenLib.ETH_ADDRESS) { | ||
| // lump in gas rebate into amount in native gas token | ||
| _pullToken(to, token, amount + rebate); | ||
| } else { | ||
| // forward erc20 then forward gas rebate in native gas token | ||
| _pullToken(to, token, amount); | ||
| _pullToken(to, UniversalTokenLib.ETH_ADDRESS, rebate); | ||
| } | ||
|
|
||
| emit BridgeRelayed( | ||
| transactionId, | ||
| relayer, | ||
| to, | ||
| transaction.originChainId, | ||
| transaction.originToken, | ||
| transaction.destToken, | ||
| transaction.originAmount, | ||
| transaction.destAmount, | ||
| rebate | ||
| ); | ||
| } |
There was a problem hiding this comment.
Restrict relayer Parameter in relay Function
The relay function allows callers to specify any relayer address, which could lead to misrepresentation or misuse.
Recommendation:
Restrict the relayer to msg.sender to ensure the correct relayer is recorded.
- function relay(bytes memory request, address relayer) public payable {
+ function relay(bytes memory request) public payable {
+ address relayer = msg.sender;
// existing code
}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 relay(bytes memory request, address relayer) public payable { | |
| bytes32 transactionId = keccak256(request); | |
| BridgeTransaction memory transaction = getBridgeTransaction(request); | |
| if (transaction.destChainId != uint32(block.chainid)) revert ChainIncorrect(); | |
| // check haven't exceeded deadline for relay to happen | |
| if (block.timestamp > transaction.deadline) revert DeadlineExceeded(); | |
| // mark bridge transaction as relayed | |
| if (bridgeRelays[transactionId]) revert TransactionRelayed(); | |
| bridgeRelays[transactionId] = true; | |
| // transfer tokens to recipient on destination chain and gas rebate if requested | |
| address to = transaction.destRecipient; | |
| address token = transaction.destToken; | |
| uint256 amount = transaction.destAmount; | |
| uint256 rebate = chainGasAmount; | |
| if (!transaction.sendChainGas) { | |
| // forward erc20 | |
| rebate = 0; | |
| _pullToken(to, token, amount); | |
| } else if (token == UniversalTokenLib.ETH_ADDRESS) { | |
| // lump in gas rebate into amount in native gas token | |
| _pullToken(to, token, amount + rebate); | |
| } else { | |
| // forward erc20 then forward gas rebate in native gas token | |
| _pullToken(to, token, amount); | |
| _pullToken(to, UniversalTokenLib.ETH_ADDRESS, rebate); | |
| } | |
| emit BridgeRelayed( | |
| transactionId, | |
| relayer, | |
| to, | |
| transaction.originChainId, | |
| transaction.originToken, | |
| transaction.destToken, | |
| transaction.originAmount, | |
| transaction.destAmount, | |
| rebate | |
| ); | |
| } | |
| function relay(bytes memory request) public payable { | |
| address relayer = msg.sender; | |
| bytes32 transactionId = keccak256(request); | |
| BridgeTransaction memory transaction = getBridgeTransaction(request); | |
| if (transaction.destChainId != uint32(block.chainid)) revert ChainIncorrect(); | |
| // check haven't exceeded deadline for relay to happen | |
| if (block.timestamp > transaction.deadline) revert DeadlineExceeded(); | |
| // mark bridge transaction as relayed | |
| if (bridgeRelays[transactionId]) revert TransactionRelayed(); | |
| bridgeRelays[transactionId] = true; | |
| // transfer tokens to recipient on destination chain and gas rebate if requested | |
| address to = transaction.destRecipient; | |
| address token = transaction.destToken; | |
| uint256 amount = transaction.destAmount; | |
| uint256 rebate = chainGasAmount; | |
| if (!transaction.sendChainGas) { | |
| // forward erc20 | |
| rebate = 0; | |
| _pullToken(to, token, amount); | |
| } else if (token == UniversalTokenLib.ETH_ADDRESS) { | |
| // lump in gas rebate into amount in native gas token | |
| _pullToken(to, token, amount + rebate); | |
| } else { | |
| // forward erc20 then forward gas rebate in native gas token | |
| _pullToken(to, token, amount); | |
| _pullToken(to, UniversalTokenLib.ETH_ADDRESS, rebate); | |
| } | |
| emit BridgeRelayed( | |
| transactionId, | |
| relayer, | |
| to, | |
| transaction.originChainId, | |
| transaction.originToken, | |
| transaction.destToken, | |
| transaction.originAmount, | |
| transaction.destAmount, | |
| rebate | |
| ); | |
| } |
| expectUnauthorized(caller, fastBridge.RELAYER_ROLE()); | ||
| prove({caller: caller, bridgeTx: tokenTx, destTxHash: hex"01"}); |
There was a problem hiding this comment.
Ensure expectUnauthorized is defined or replace it
At line 365, expectUnauthorized is used but may not be defined in the provided context. This could lead to test failures.
If expectUnauthorized is not defined, consider adding its implementation or replacing it with an appropriate assertion from the testing framework.
| function assertEq(FastBridgeV2.BridgeStatus a, FastBridgeV2.BridgeStatus b) public pure { | ||
| assertEq(uint8(a), uint8(b)); | ||
| } |
There was a problem hiding this comment.
Avoid potential recursive call in assertEq function
In the assertEq function, calling assertEq within itself may lead to unintended recursion. Ensure that you are invoking the correct assertion method from the testing framework.
Specify the contract or library when calling assertEq to avoid confusion:
-function assertEq(FastBridgeV2.BridgeStatus a, FastBridgeV2.BridgeStatus b) public pure {
+function assertEq(FastBridgeV2.BridgeStatus a, FastBridgeV2.BridgeStatus b) public {
+ super.assertEq(uint8(a), uint8(b));
}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 assertEq(FastBridgeV2.BridgeStatus a, FastBridgeV2.BridgeStatus b) public pure { | |
| assertEq(uint8(a), uint8(b)); | |
| } | |
| function assertEq(FastBridgeV2.BridgeStatus a, FastBridgeV2.BridgeStatus b) public { | |
| super.assertEq(uint8(a), uint8(b)); | |
| } |
| vm.expectEmit(address(fastBridge)); | ||
| // Note: BridgeProofDisputed event has a mislabeled address parameter, this is actually the guard | ||
| emit BridgeProofDisputed({transactionId: txId, relayer: guard}); | ||
| } |
There was a problem hiding this comment.
Correct the mislabeled parameter in the BridgeProofDisputed event
At line 157, the BridgeProofDisputed event parameter relayer actually represents the guard. For clarity and maintainability, consider renaming the parameter to accurately reflect its purpose.
Apply this diff to fix the parameter name:
-event BridgeProofDisputed(bytes32 indexed transactionId, address indexed relayer);
+event BridgeProofDisputed(bytes32 indexed transactionId, address indexed guard);Committable suggestion was skipped due to low confidence.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3138 +/- ##
===================================================
+ Coverage 37.95009% 38.40049% +0.45039%
===================================================
Files 418 424 +6
Lines 24245 24445 +200
Branches 82 146 +64
===================================================
+ Hits 9201 9387 +186
- Misses 14306 14314 +8
- Partials 738 744 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range and nitpick comments (8)
packages/contracts-rfq/test/FastBridgeV2.Src.ProtocolFees.t.sol (1)
6-6: Consider Removing Unnecessary Solhint Rule SuppressionThe comment
// solhint-disable func-name-mixedcase, orderingdisables linting rules that help maintain code consistency and style conventions. If the functions in this contract adhere to standard naming and ordering practices, consider removing this suppression to enforce linting and improve code quality.packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol (3)
15-18: Consider removing or updating the skipped testSince the
relayfunction is no longer permissioned, the testtest_failedRelayNotRelayeris no longer applicable. Consider removing this test or updating it to reflect the new behavior to keep the test suite clean and maintainable.
20-24: Consider removing or updating the skipped testThe
test_failedClaimNotRelayeris skipped because theclaimfunction is no longer permissioned by role. To maintain a clean test suite, consider removing this test or updating it to test relevant scenarios under the new permissioning logic.
29-29: Review the necessity ofsetUpRoles()in the updated testSince the
claimfunction is no longer permissioned by roles, the call tosetUpRoles();may be unnecessary. Consider removing it if it's no longer needed to streamline the test and avoid confusion.packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
14-121: Add concrete test functions toFastBridgeV2TestThe
FastBridgeV2Testcontract currently lacks concrete test cases. While it sets up the testing environment, adding test functions (prefixed withtest) is essential to validate theFastBridgeV2contract's functionality. This will improve test coverage and ensure that all features work as intended.Would you like assistance in creating initial test cases to validate core functionalities such as bridging tokens, handling ETH transfers, and enforcing access controls?
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
26-32: Consider Documenting theBridgeStatusEnum ValuesWhile the
BridgeStatusenum is self-explanatory, adding more detailed comments can improve code readability and maintainability.Consider adding inline comments for each enum value to describe the state transitions and their implications.
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1)
227-228: Address the skipped tests marked with TODO commentsThe following tests are currently skipped with TODO comments:
test_bridge_userSpecificNonce(lines 227-228)test_bridge_revert_zeroSender(lines 286-287)test_bridge_revert_zeroRecipient(lines 293-294)test_refund_revert_statusNull(lines 803-804)test_refund_revert_statusProven(lines 809-810)test_refund_revert_statusClaimed(lines 817-818)test_refund_revert_statusRefunded(lines 826-827)Completing these tests will enhance code coverage and ensure that all scenarios are properly validated.
Would you like assistance in implementing these tests or opening GitHub issues to track these tasks?
Also applies to: 286-287, 293-294, 803-804, 809-810, 817-818, 826-827
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
21-21: Maintain consistency in parameter usage across functionsThe
claimfunction does not accept arelayerparameter, unlikerelayandprove. For consistency and clarity, consider including therelayerparameter inclaim, or update the documentation to explain why it is unnecessary.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (19)
- .vscode/settings.json (1 hunks)
- packages/contracts-rfq/contracts/Admin.sol (1 hunks)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IAdmin.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1 hunks)
- packages/contracts-rfq/contracts/interfaces/IMulticallTarget.sol (1 hunks)
- packages/contracts-rfq/contracts/libs/Errors.sol (1 hunks)
- packages/contracts-rfq/contracts/libs/UniversalToken.sol (1 hunks)
- packages/contracts-rfq/contracts/utils/MulticallTarget.sol (1 hunks)
- packages/contracts-rfq/script/ConfigureFastBridge.s.sol (1 hunks)
- packages/contracts-rfq/script/DeployFastBridge.CREATE2.s.sol (1 hunks)
- packages/contracts-rfq/test/FastBridge.t.sol (4 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Management.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.ProtocolFees.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.t.sol (1 hunks)
Files skipped from review due to trivial changes (9)
- packages/contracts-rfq/contracts/Admin.sol
- packages/contracts-rfq/contracts/interfaces/IAdmin.sol
- packages/contracts-rfq/contracts/interfaces/IFastBridge.sol
- packages/contracts-rfq/contracts/interfaces/IMulticallTarget.sol
- packages/contracts-rfq/contracts/libs/Errors.sol
- packages/contracts-rfq/contracts/libs/UniversalToken.sol
- packages/contracts-rfq/contracts/utils/MulticallTarget.sol
- packages/contracts-rfq/script/ConfigureFastBridge.s.sol
- packages/contracts-rfq/script/DeployFastBridge.CREATE2.s.sol
Additional comments not posted (14)
.vscode/settings.json (1)
21-21: Ensure Solidity codebase compatibility with compiler version 0.8.24Upgrading the compiler to Solidity
v0.8.24is beneficial, but please verify that all Solidity files in the codebase are compatible with this version. Specifically, check that the pragma statements do not restrict the compiler version below0.8.24.Run the following script to identify any Solidity files with pragma statements that may not be compatible:
Verification successful
Compiler upgrade to v0.8.24 is compatible with the codebase
The change to use Solidity compiler version 0.8.24 in .vscode/settings.json is safe and compatible with the existing codebase. No files explicitly restrict the compiler version to below 0.8.24, allowing for this upgrade.
However, please note:
- Many files use lower versions (e.g., 0.8.17, 0.8.20) but don't have upper bounds preventing 0.8.24 usage.
- Some files use older versions (0.6.x, 0.7.x). While they don't conflict with the 0.8.24 setting, they may need careful review for compatibility in the future.
Recommendation: Proceed with the compiler upgrade, but consider gradually updating older files to newer Solidity versions in future maintenance tasks.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find Solidity files with pragma statements restricting the compiler version below 0.8.24. # Expected: No files should have an upper version limit below 0.8.24. # Search for pragma statements with a maximum version less than 0.8.24 rg --type solidity --no-filename --no-heading --line-number "pragma solidity [\^<>= ]+0\.8\.[0-9]+" # Confirm that none of the pragma statements impose an upper limit below 0.8.24 rg --type solidity --no-filename --no-heading --line-number "pragma solidity .*< *0\.8\.24"Length of output: 261
Script:
#!/bin/bash # Find Solidity files solidity_files=$(fd -e sol) if [ -z "$solidity_files" ]; then echo "No Solidity files found in the repository." else echo "Solidity files found. Checking pragma statements:" echo "$solidity_files" | while read -r file; do echo "File: $file" grep -n "pragma solidity" "$file" || echo "No pragma statement found" done fi # Check for pragma statements with version restrictions echo -e "\nChecking for version restrictions:" if [ -n "$solidity_files" ]; then grep -n "pragma solidity" $(echo "$solidity_files") | grep -E "< *0\.8\.24" fiLength of output: 156771
packages/contracts-rfq/test/FastBridgeV2.Src.ProtocolFees.t.sol (2)
8-12: FastBridge Configuration is Correctly Set Up [Approved]The
configureFastBridgefunction appropriately calls the parent implementation and correctly sets up the testing environment by:
- Granting the
GOVERNOR_ROLEto the contract itself, which is necessary for governance actions within the test.- Setting the protocol fee rate to
1e4, which represents 1% if the fee rate is measured in parts per million (PPM). This is appropriate for testing the protocol fee functionality.
14-24: Fixture Creation Correctly Initializes Test Parameters [Approved]The
createFixturesfunction successfully initializes the transaction parameters for both token and ETH transactions. The values assigned are:
- Origin fee amounts (
0.01e6for tokens and0.01 etherfor ETH), representing 1% fees.- Origin amounts (
0.99e6for tokens and0.99 etherfor ETH), which, combined with the fee amounts, total the full transaction amounts.- Destination amounts (
0.98e6for tokens and0.98 etherfor ETH), reflecting the deduction of fees.This setup is logical and ensures comprehensive testing of the protocol's fee handling and transaction processing under defined conditions.
packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol (1)
10-13: Deployment ofFastBridgeV2contract is correctly handledThe
deployFastBridge()function correctly deploys theFastBridgeV2contract using the cheatcode, ensuring compatibility between Solidity versions during testing.packages/contracts-rfq/test/FastBridgeV2.t.sol (2)
114-116: Validate the correctness ofexpectUnauthorizedrevert expectationThe
expectUnauthorizedfunction anticipates a revert with theAccessControlUnauthorizedAccountselector. Ensure that this selector matches the actual revert reason emitted by theAccessControlcontract when unauthorized access is attempted. This guarantees that the test accurately captures unauthorized access scenarios.Run the following script to verify the revert reason:
Verification successful
Verification successful:
expectUnauthorizedcorrectly anticipates AccessControlUnauthorizedAccount revertThe
expectUnauthorizedfunction correctly anticipates theAccessControlUnauthorizedAccountrevert. This revert is consistently implemented across the AccessControl contracts in the codebase, matching the selector used in the test function.Key findings:
- The exact revert statement
revert AccessControlUnauthorizedAccount(account, role);is present in the AccessControl implementations.- The test correctly imports IAccessControl from OpenZeppelin:
import {IAccessControl} from "@openzeppelin/contracts/access/IAccessControl.sol";- The selector usage
IAccessControl.AccessControlUnauthorizedAccount.selectorin the test matches the actual implementation.This verification confirms that the test accurately captures unauthorized access scenarios as intended.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Confirm that AccessControl emits AccessControlUnauthorizedAccount on unauthorized access. # Test: Search for the revert statement in AccessControl contract. Expect: Matches the selector used. rg --type solidity 'revert.+AccessControlUnauthorizedAccount' --ignore-caseLength of output: 108
Script:
#!/bin/bash # Description: Find AccessControlUnauthorizedAccount revert in Solidity files # Search for .sol files and grep for AccessControlUnauthorizedAccount fd -e sol -x grep -n -i 'revert.*AccessControlUnauthorizedAccount' {} # Search for import statements related to AccessControl fd -e sol -x grep -n 'import.*AccessControl' {} # Look for AccessControl implementation fd -e sol -x grep -n 'contract AccessControl' {}Length of output: 2455
110-112: Ensure consistent hashing ingetTxIdfor transaction identificationThe
getTxIdfunction useskeccak256(abi.encode(bridgeTx))to compute the transaction ID. If any of the fields inBridgeTransactionare of dynamic types (likebytesorstring), the hashing might produce inconsistent results due to how dynamic types are encoded. Verify that all fields inBridgeTransactionare of fixed-size types to ensure consistent and collision-resistant hashing.To confirm the types within
BridgeTransaction, run the following script:packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
52-70: Ensure Safe Handling of Token Transfers in_pullTokenFunctionThe
_pullTokenfunction handles token transfers and includes logic to account for tokens with transfer fees or deflationary tokens. Ensure that the calculation ofamountPulledaccurately reflects the actual tokens received, especially after thesafeTransferFromcall.Please verify that the balance difference calculation correctly accounts for tokens with transfer fees:
amountPulled = IERC20(token).balanceOf(recipient) - amountPulled;This ensures the contract is aware of the exact amount received.
226-233: Reassess Access Control Logic inclaimFunctionThe
claimfunction allows any caller to claim funds iftois the zero address or ifmsg.sendermatches theproof.relayer. This might unintentionally permit unauthorized users to claim funds.Please verify that the access control meets the intended design and that only authorized entities can claim the funds. Consider restricting the function to only allow the relayer to call it or adding additional checks.
183-190: Ensure Role Management for Relayers inproveFunctionThe
provefunction usesonlyRole(RELAYER_ROLE)to restrict access. Ensure that the role management forRELAYER_ROLEis properly set up and that only trusted relayers are assigned this role.Please confirm that roles are correctly assigned and managed, possibly by providing documentation or comments on how roles are handled in the
Admincontract.packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1)
401-411: Verify the handling ofaddress(0)inclaimfunctionsIn
test_claim_token_permissionless_toZeroAddress(lines 401-411) andtest_claim_eth_permissionless_toZeroAddress(lines 468-479), thetoparameter in theclaimfunction is passed asaddress(0). However, theexpectBridgeDepositClaimedfunction expects thetoaddress to berelayerA. Ensure that passingaddress(0)behaves as intended and that thetoaddress defaults torelayerAor is handled appropriately.Run the following script to find all instances where
claimis called withtoasaddress(0)and verify the behavior:Also applies to: 468-479
Verification successful
Handling of
address(0)inclaimfunctions is correctly testedThe test cases
test_claim_token_permissionless_toZeroAddressandtest_claim_eth_permissionless_toZeroAddressare correctly designed to verify the handling ofaddress(0)in theclaimfunction. The apparent discrepancy between theexpectBridgeDepositClaimedandclaimfunction parameters is intentional and part of the test design.The tests suggest that the
claimfunction likely has internal logic to handle cases wheretoisaddress(0), defaulting to the relayer's address. This is evidenced by:
expectBridgeDepositClaimedis called withto: relayerAclaimis then called withto: address(0)- Subsequent assertions check for successful claim and correct balance updates
To fully verify this behavior:
- Review the implementation of the
claimfunction to confirm the handling ofaddress(0)cases.- Ensure that the
FastBridgeV2contract documentation clearly explains this behavior.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find instances of `claim` function calls with `to: address(0)` rg --type solidity -A 5 $'claim({caller: $_, bridgeTx: $_, to: address(0)})'Length of output: 208
Script:
#!/bin/bash # Description: Find instances of `claim` function calls with `to: address(0)` rg --type solidity -A 5 'claim\s*\(\s*\{.*to:\s*address\(0\).*\}\s*\)'Length of output: 105
Script:
#!/bin/bash # Description: Find instances of `claim` function calls with `to: address(0)` in .sol files rg -g '*.sol' -A 5 'claim\s*\(.*to:\s*address\(0\).*\)'Length of output: 1210
Script:
#!/bin/bash # Description: Find the full context of the test functions including the expectBridgeDepositClaimed calls rg -g '*.sol' -A 15 'function test_claim_(token|eth)_permissionless_toZeroAddress'Length of output: 3502
packages/contracts-rfq/test/FastBridge.t.sol (3)
1304-1304: Approving addition ofvirtualkeyword for test functionsAdding
virtualtotest_failedRelayNotRelayer()allows derived test contracts to override this test, enhancing flexibility in the test suite.
1649-1649: Approving addition ofvirtualkeyword for test functionsBy making
test_failedClaimNotOldRelayer()virtual, you enable derived test contracts to override and customize this test as needed.
1686-1686: Approving addition ofvirtualkeyword for test functionsThe
virtualkeyword ontest_failedClaimNotRelayer()permits overriding in derived contracts, enhancing extensibility of your tests.packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
6-6: InterfaceIFastBridgeV2correctly extendsIFastBridgeExtending
IFastBridgeensures compatibility with existing bridge functionalities and promotes code reuse.
|
|
||
| // solhint-disable func-name-mixedcase, ordering | ||
| contract FastBridgeV2ManagementTest is FastBridgeV2Test { | ||
| uint256 public constant FEE_RATE_MAX = 1e4; // 1% |
There was a problem hiding this comment.
Possible Misalignment of FEE_RATE_MAX Value and Comment
The FEE_RATE_MAX is set to 1e4, which equals 10000. The comment indicates this represents 1%. Typically, a value of 10000 represents 100% when using basis points. If the intended maximum fee rate is 1%, consider adjusting the value to 100.
Apply this diff to correct the value or the comment:
- uint256 public constant FEE_RATE_MAX = 1e4; // 1%
+ uint256 public constant FEE_RATE_MAX = 100; // 1%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 public constant FEE_RATE_MAX = 1e4; // 1% | |
| uint256 public constant FEE_RATE_MAX = 100; // 1% |
| uint256 originFeeAmount; | ||
| if (protocolFeeRate > 0) originFeeAmount = (originAmount * protocolFeeRate) / FEE_BPS; | ||
| originAmount -= originFeeAmount; // remove from amount used in request as not relevant for relayers |
There was a problem hiding this comment.
Declare Missing Variables and Constants Used in Calculations
The variables protocolFeeRate, FEE_BPS, protocolFees, and chainGasAmount are used but not declared in the contract. This will result in compilation errors.
Please declare these variables and constants. For example:
+uint256 public protocolFeeRate; // Fee rate set by the protocol
+uint256 public constant FEE_BPS = 10_000; // Basis points for fee calculation
+mapping(address => uint256) public protocolFees; // Accumulated protocol fees per token
+uint256 public chainGasAmount; // Gas amount for cross-chain transactions
Also applies to: 150-151, 240-240
| /// @notice Relays destination side of bridge transaction by off-chain relayer | ||
| /// @param request The encoded bridge transaction to relay on destination chain | ||
| /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed | ||
| function relay(bytes memory request, address relayer) external payable; |
There was a problem hiding this comment.
Review the necessity of passing relayer as a parameter
Passing the relayer address explicitly in the relay function may introduce security risks. Since any caller could specify an arbitrary relayer address, consider using msg.sender as the relayer to ensure that the entity initiating the relay is correctly associated.
| /// @param request The encoded bridge transaction to prove on origin chain | ||
| /// @param destTxHash The destination tx hash proving bridge transaction was relayed | ||
| /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed | ||
| function prove(bytes memory request, bytes32 destTxHash, address relayer) external; |
There was a problem hiding this comment.
Validate the relayer parameter in prove function
Similar to the relay function, passing relayer as a parameter in prove could allow malicious actors to misrepresent the relayer's identity. Ensure that the implementation validates the authenticity of the relayer address or considers using msg.sender instead.
| /// @notice Relays destination side of bridge transaction by off-chain relayer | ||
| /// @param request The encoded bridge transaction to relay on destination chain | ||
| /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed | ||
| function relay(bytes memory request, address relayer) external payable; | ||
|
|
||
| /// @notice Provides proof on origin side that relayer provided funds on destination side of bridge transaction | ||
| /// @param request The encoded bridge transaction to prove on origin chain | ||
| /// @param destTxHash The destination tx hash proving bridge transaction was relayed | ||
| /// @param relayer The address of the relaying entity which should have control of the origin funds when claimed | ||
| function prove(bytes memory request, bytes32 destTxHash, address relayer) external; | ||
|
|
||
| /// @notice Completes bridge transaction on origin chain by claiming originally deposited capital. Can only send funds to the relayer address on the proof. | ||
| /// @param request The encoded bridge transaction to claim on origin chain | ||
| function claim(bytes memory request) external; |
There was a problem hiding this comment.
Enhance function documentation for clarity
The function comments can be improved to provide clearer guidance:
- Specify the expected behavior and any assumptions.
- Clarify the roles and responsibilities of the caller and the
relayer. - Highlight any security considerations or requirements.
Consider revising the documentation as follows:
For relay:
/// @notice Relays the destination side of a bridge transaction by an off-chain relayer.
/// @param request The encoded bridge transaction to relay on the destination chain.
-/// @param relayer The address of the relaying entity which should have control of the origin funds when claimed.
+/// @param relayer The address of the relayer. This address should control the origin funds when claimed to ensure secure transactions.Apply similar enhancements to the prove and claim function comments.
Committable suggestion was skipped due to low confidence.
ChiTimesChi
changed the title
|
@parodime let's also merge latest master here to get the CI workflows to work |
Description
FastBridgeV2 - extend from FastBridge
Overloads of relay/prove/claim relax permissions and allow single relaying entity to operate with multiple addresses.
Accompanying tests included
Summary by CodeRabbit
New Features
FastBridgeV2contract for cross-chain token transfers, enhancing transaction tracking and user protections.Bug Fixes
Tests
FastBridgeV2functionality, including governance, fee management, and transaction validation scenarios.