fix(contracts-rfq): add bridge/relay revert tests [SLT-322] #3251
Conversation
WalkthroughThe changes in this pull request primarily focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
Deploying sanguine-fe with
|
| Latest commit: |
e5a852f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://19c9ad8d.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-fbv2-user-bridge-revert.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3251 +/- ##
===================================================
+ Coverage 91.00237% 91.08833% +0.08596%
===================================================
Files 60 60
Lines 1267 1268 +1
Branches 154 155 +1
===================================================
+ Hits 1153 1155 +2
+ Misses 111 110 -1
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (2)
187-192: LGTM: Good test for insufficient token approval.This test function verifies that the bridge operation reverts when the token approval is less than the required amount. It's an important check to ensure that the contract handles insufficient approvals correctly.
Consider adding an assertion to verify that the approval amount is actually set to the intended value before attempting the bridge operation. This would make the test more robust:
function test_bridge_token_revert_approvedNotEnough() public { vm.prank(userA); - srcToken.approve(address(fastBridge), tokenParams.originAmount - 1); + uint256 approvalAmount = tokenParams.originAmount - 1; + srcToken.approve(address(fastBridge), approvalAmount); + assertEq(srcToken.allowance(userA, address(fastBridge)), approvalAmount, "Approval not set correctly"); vm.expectRevert(); bridge({caller: userA, msgValue: 0, params: tokenParams}); }
194-197: LGTM: Good test for non-zero msg.value in token bridging.This test function verifies that the bridge operation reverts when attempting to bridge tokens with a non-zero msg.value. It's a critical check to ensure that the contract prevents accidental ETH transfers during token bridging.
To improve clarity, consider adding a comment explaining why this scenario should revert and asserting that the contract's ETH balance remains unchanged:
function test_bridge_token_revert_nonZeroMsgValue() public { + // Bridging tokens should not accept ETH, so this should revert + uint256 initialBalance = address(fastBridge).balance; vm.expectRevert(); bridge({caller: userA, msgValue: tokenParams.originAmount, params: tokenParams}); + assertEq(address(fastBridge).balance, initialBalance, "Contract balance should not change"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
345-347: Excellent security improvement and alignment with PR objectives.The changes in the
_pullTokenfunction significantly enhance the contract's security and align perfectly with the PR objectives:
- The new check
if (msg.value != 0) revert MsgValueIncorrect();prevents unintended behavior when bridging ERC20 tokens, addressing the PR's second key condition.- Moving the balance recording after this check improves efficiency by ensuring the balance is only recorded when necessary.
These modifications effectively prevent potential fund loss scenarios and improve the overall robustness of the bridging process.
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (1)
180-185: LGTM: Good test case for zero token approval.This test function correctly verifies that the bridge operation reverts when the token approval is set to zero. It's a crucial check to ensure that the contract handles insufficient approvals properly.
ChiTimesChi
force-pushed
the
fix/FbV2-user-bridge-revert-tests
branch
from
8fd76b1
to
e40195d
Compare
ChiTimesChi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (3)
364-369: Consider specifying the expected revert reason.The test correctly checks for a revert when the relayer has approved zero tokens. However, to make the test more robust and informative, consider specifying the exact revert reason you expect.
You could modify the test as follows:
- vm.expectRevert(); + vm.expectRevert("ERC20: insufficient allowance"); relay({caller: relayerA, msgValue: 0, bridgeTx: tokenTx});This change will make the test more precise and easier to debug if it fails.
371-376: Enhance test precision by specifying the expected revert reason.The test correctly checks for a revert when the relayer has approved insufficient tokens. To improve the test's robustness and clarity, consider specifying the exact revert reason you expect.
You could modify the test as follows:
- vm.expectRevert(); + vm.expectRevert("ERC20: insufficient allowance"); relay({caller: relayerA, msgValue: 0, bridgeTx: tokenTx});This change will make the test more precise and easier to debug if it fails.
364-396: Overall assessment of new test functionsThe new test functions significantly enhance the test coverage for the
FastBridgeV2contract, particularly for edge cases and error conditions in token approvals and ETH transfers. These tests are crucial for ensuring the contract's robustness and security.Key observations:
- All tests correctly use
vm.expectRevert()to check for reverts in various scenarios.- The tests cover important cases such as zero/insufficient token approvals and incorrect ETH values.
Recommendations for improvement:
- Specify exact revert reasons in all
vm.expectRevert()calls for more precise testing.- Verify that the contract actually implements the checks being tested, especially for ETH transfer edge cases.
- Consider adding assertions to check the state after failed transactions, ensuring no unintended side effects.
These tests significantly contribute to the contract's security. To further enhance the testing strategy, consider:
- Implementing property-based testing for a wider range of input values.
- Adding fuzzing tests to discover edge cases that might have been overlooked.
- Conducting integration tests that simulate real-world scenarios involving multiple contract interactions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol (4)
388-391: Enhance test precision and verify contract behavior for excess ETH.The test checks for a revert when the message value is higher than required for an ETH relay. To improve the test and ensure it aligns with the contract's behavior:
- Specify the exact revert reason you expect, if applicable.
- Verify that the contract actually checks for and reverts on excess message value for ETH transfers.
Consider modifying the test as follows:
- vm.expectRevert(); + vm.expectRevert("FastBridgeV2: excess msg.value"); relay({caller: relayerA, msgValue: ethParams.destAmount + 1, bridgeTx: ethTx});To verify the contract's behavior, run the following script:
#!/bin/bash # Description: Verify that FastBridgeV2 checks for excess msg.value in ETH transfers ast-grep --lang solidity --pattern $'function _pullToken(address token, address from, uint256 amount) internal { $$$ if (msg.value > amount) { revert("FastBridgeV2: excess msg.value"); } $$$ }'If the contract doesn't implement this check, consider whether this test is necessary or if the contract should be updated to include this check for better security.
383-386: Improve test precision and verify contract behavior for ETH transfers.The test correctly checks for a revert when the message value is less than required for an ETH relay. To enhance the test's robustness and clarity:
- Consider specifying the exact revert reason you expect.
- Verify that the contract actually checks for and reverts on insufficient message value for ETH transfers.
You could modify the test as follows:
- vm.expectRevert(); + vm.expectRevert("FastBridgeV2: insufficient msg.value"); relay({caller: relayerA, msgValue: ethParams.destAmount - 1, bridgeTx: ethTx});To verify the contract's behavior, run the following script:
#!/bin/bash # Description: Verify that FastBridgeV2 checks for insufficient msg.value in ETH transfers ast-grep --lang solidity --pattern $'function _pullToken(address token, address from, uint256 amount) internal { $$$ if (msg.value < amount) { revert("FastBridgeV2: insufficient msg.value"); } $$$ }'This change and verification will ensure that the test accurately reflects the contract's intended behavior for ETH transfers.
393-396: Improve test precision and verify contract behavior for zero ETH value.The test checks for a revert when zero message value is provided for an ETH relay. To enhance the test's effectiveness:
- Specify the exact revert reason you expect.
- Verify that the contract actually checks for and reverts on zero message value for ETH transfers.
Consider modifying the test as follows:
- vm.expectRevert(); + vm.expectRevert("FastBridgeV2: zero msg.value for ETH transfer"); relay({caller: relayerA, msgValue: 0, bridgeTx: ethTx});To verify the contract's behavior, run the following script:
#!/bin/bash # Description: Verify that FastBridgeV2 checks for zero msg.value in ETH transfers ast-grep --lang solidity --pattern $'function _pullToken(address token, address from, uint256 amount) internal { $$$ if (msg.value == 0) { revert("FastBridgeV2: zero msg.value for ETH transfer"); } $$$ }'This change and verification will ensure that the test accurately reflects the contract's intended behavior for ETH transfers with zero value.
378-381: Enhance test precision and verify contract behavior.The test correctly checks for a revert when a non-zero message value is provided for an ERC20 token relay. To improve the test's robustness and clarity:
- Consider specifying the exact revert reason you expect.
- Verify that the contract actually checks for and reverts on non-zero message value for ERC20 tokens.
You could modify the test as follows:
- vm.expectRevert(); + vm.expectRevert("FastBridgeV2: non-zero msg.value for ERC20"); relay({caller: relayerA, msgValue: tokenParams.destAmount, bridgeTx: tokenTx});To verify the contract's behavior, run the following script:
This change and verification will ensure that the test accurately reflects the contract's intended behavior.
Changes to gas cost
🧾 Summary (50% most significant diffs)
Full diff report 👇
|
Description
All of the following must end up in a revert to avoid stuck funds in the contract:
msg.valuefor bridging/relaying an ERC20 (this wasn't passing in e40195d and got fixed in e5a852f)Summary by CodeRabbit
New Features
Bug Fixes
bridgefunction behaves correctly under specific conditions, preventing errors related to token approvals and message values.