fix dispute emit & revamp deposit xfer [SLT-295] [SLT-326] #3293
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FastBridgeV2
participant TokenContract
User->>FastBridgeV2: initiateBridge(params)
FastBridgeV2->>TokenContract: transferTokens()
TokenContract-->>FastBridgeV2: confirmTransfer()
FastBridgeV2->>User: emitBridgeEvent()
User->>FastBridgeV2: disputeTransaction()
FastBridgeV2->>FastBridgeV2: captureDisputedRelayer()
FastBridgeV2->>User: emitDisputeEvent()
User->>FastBridgeV2: requestRefund()
FastBridgeV2->>TokenContract: conditionalTransfer()
TokenContract-->>FastBridgeV2: confirmRefund()
FastBridgeV2->>User: emitRefundEvent()
User->>FastBridgeV2: claimCollateral()
FastBridgeV2->>TokenContract: conditionalTransfer()
TokenContract-->>FastBridgeV2: confirmClaim()
FastBridgeV2->>User: emitClaimEvent()
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
Deploying sanguine-fe with
|
| Latest commit: |
099a4f5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://853d5e13.sanguine-fe.pages.dev |
| Branch Preview URL: | https://ft-fixdisputeemit-revampdepo.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3293 +/- ##
====================================================
+ Coverage 35.14833% 91.52941% +56.38108%
====================================================
Files 77 59 -18
Lines 2663 1275 -1388
Branches 82 160 +78
====================================================
+ Hits 936 1167 +231
+ Misses 1722 104 -1618
+ Partials 5 4 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (4 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (6 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
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/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 (10)
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (6)
77-79: Approved: Improved consistency in dispute handlingThe change from
guardtorelayerin theexpectBridgeProofDisputedfunction signature and event emission aligns with the PR objectives. This modification enhances clarity and consistency in the contract's terminology, accurately reflecting the role of the relayer in the dispute process.
662-662: Approved: Consistent update in test caseThe modification in the
test_dispute_tokenfunction, changing fromguardtorelayer: relayerAin theexpectBridgeProofDisputedcall, is consistent with the earlier function signature update. This change ensures that the test case accurately reflects the new dispute handling process.
674-674: Approved: Consistent update in deadline-related test caseThe change in the
test_dispute_token_justBeforeDeadlinefunction, updating theexpectBridgeProofDisputedcall to userelayer: relayerA, maintains consistency with the previous modifications. This ensures that the test case for disputing just before the deadline accurately reflects the updated dispute handling process.
686-686: Approved: Consistent update in ETH dispute test caseThe modification in the
test_dispute_ethfunction, changing theexpectBridgeProofDisputedcall to userelayer: relayerA, maintains consistency with the previous updates. This change ensures that the test case for disputing ETH transactions accurately reflects the new dispute handling process.
699-699: Approved: Consistent update in ETH dispute deadline test caseThe change in the
test_dispute_eth_justBeforeDeadlinefunction, updating theexpectBridgeProofDisputedcall to userelayer: relayerA, maintains consistency with the previous modifications. This ensures that the test case for disputing ETH transactions just before the deadline accurately reflects the updated dispute handling process.
Line range hint
1-899: Summary: Consistent and focused updates to dispute handlingThe changes in this file are well-focused and consistently update the dispute handling process to use 'relayer' instead of 'guard'. These modifications:
- Improve clarity and consistency in the contract's terminology.
- Accurately reflect the role of the relayer in the dispute process.
- Align with the PR objectives mentioned in the summary.
- Maintain existing functionality in other parts of the file.
These updates enhance the overall quality and readability of the test suite, ensuring it correctly represents the intended behavior of the FastBridgeV2 contract.
packages/contracts-rfq/contracts/FastBridgeV2.sol (4)
85-86: Correct use ofdisputedRelayerindisputefunction.Capturing the
proofRelayerasdisputedRelayerbefore resetting the proof fields ensures that the emitted eventBridgeProofDisputedaccurately reflects the relayer involved in the dispute. This enhances transparency and aligns with the intended event parameters.
93-93: Event emission properly reflects the disputed relayer.The
emit BridgeProofDisputed(transactionId, disputedRelayer);statement now usesdisputedRelayer, ensuring the event accurately indicates the relayer whose proof is disputed.
174-177: Accurate calculation and deduction of protocol fees inbridgefunction.The protocol fee is correctly calculated using the provided
protocolFeeRateand deducted fromoriginAmount. This ensures that the fee is appropriately handled and the relayers receive the correct amount.
316-322: Correct parameters used inBridgeDepositClaimedevent emission.The
BridgeDepositClaimedevent is emitted with the correct parameters, includingtransactionId,proofRelayer,to,originToken, andoriginAmount. This ensures that the event accurately reflects the details of the claimed deposit.
| if (transaction.originToken == UniversalTokenLib.ETH_ADDRESS) { | ||
| Address.sendValue(payable(transaction.originSender), amount); | ||
| } else { | ||
| IERC20(transaction.originToken).safeTransfer(transaction.originSender, amount); | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential reentrancy risk in refund function when transferring funds.
The refund function performs external calls to transfer ETH or ERC20 tokens back to the transaction.originSender without reentrancy protection. This could be exploited if the recipient is a contract with malicious fallback functions.
Consider adding the nonReentrant modifier from OpenZeppelin's ReentrancyGuard to the refund function to prevent reentrancy attacks.
| // transfer origin collateral to specified address (protocol fee was pre-deducted at deposit) | ||
| if (transaction.originToken == UniversalTokenLib.ETH_ADDRESS) { | ||
| Address.sendValue(payable(to), transaction.originAmount); | ||
| } else { | ||
| IERC20(transaction.originToken).safeTransfer(to, transaction.originAmount); | ||
| } |
There was a problem hiding this comment.
Add reentrancy guard to claim function to enhance security.
The claim function updates the state and then performs external calls to transfer ETH or ERC20 tokens to the to address. Although state changes occur before the external calls, adding a nonReentrant modifier will provide additional protection against potential reentrancy attacks.
| address to = transaction.originSender; | ||
| address token = transaction.originToken; |
There was a problem hiding this comment.
Reading from memory just once should be more efficient, so let's get these two lines back?
| address token = transaction.originToken; | ||
| uint256 amount = transaction.originAmount; |
There was a problem hiding this comment.
Same here re: caching the values from memory onto the stack.
There was a problem hiding this comment.
i thought that was already happening efficiently by the transaction struct being in mem (ie: anything accessed within it would be as efficient as repeating them into dedicated vars -- and more concise code)
will defer to your expertise & switch these back to dedicated vars
|
|
||
| emit BridgeDepositClaimed(transactionId, bridgeTxDetails[transactionId].proofRelayer, to, token, amount); | ||
| // solhint-disable-next-line max-line-length |
There was a problem hiding this comment.
This solhint directive is not really needed :)
There was a problem hiding this comment.
ah it was until i linted & it stacked the params for me. tx
ChiTimesChi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
116-126: Improved refund handling for ETH and ERC20 tokensThe refund function has been updated to handle both ETH and ERC20 tokens correctly:
- ETH refunds now use
Address.sendValue, which is a secure method for sending ETH.- ERC20 refunds use the
safeTransferfunction from the SafeERC20 library.- The
BridgeDepositRefundedevent now includes all necessary information about the refund.These changes improve the security and flexibility of the refund process.
Consider adding a check to ensure that the contract has sufficient balance before attempting the transfer, especially for ETH refunds. This can prevent potential issues if the contract's balance is unexpectedly low.
313-326: Improved claim handling for ETH and ERC20 tokensThe
claimfunction has been updated to handle both ETH and ERC20 tokens correctly:
- ETH claims now use
Address.sendValue, which is a secure method for sending ETH.- ERC20 claims use the
safeTransferfunction from the SafeERC20 library.- The
BridgeDepositClaimedevent now includes all necessary information about the claim.These changes improve the security and flexibility of the claim process.
Consider adding a check to ensure that the contract has sufficient balance before attempting the transfer, especially for ETH claims. This can prevent potential issues if the contract's balance is unexpectedly low.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (4 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
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/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 (3)
packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
85-86: Improved clarity and correctness in dispute handlingThe changes in the
disputefunction enhance the clarity and correctness of the dispute process:
- Capturing the
disputedRelayerbefore resetting the transaction status ensures the correct address is emitted in the event.- The
BridgeProofDisputedevent now correctly emits thedisputedRelayerinstead of the sender's address.These modifications improve the transparency and accuracy of the dispute mechanism.
Also applies to: 93-93
176-179: Improved protocol fee handling in bridge functionThe changes in the
bridgefunction enhance the protocol fee calculation and application:
- The protocol fee is now correctly calculated based on the original amount.
- The fee is deducted from the origin amount, ensuring that the amount used in the request is relevant for relayers.
This modification improves the accuracy of the bridging process and ensures proper fee handling.
314-318: Improved token transfer handling in relay functionThe visible changes in the
relayfunction show improved handling of token transfers:
- ETH transfers now use
Address.sendValue, which is a secure method for sending ETH.- ERC20 transfers use the
safeTransferfunction from the SafeERC20 library.These changes enhance the security and correctness of the token transfer process during relay.
As the entire
relayfunction has undergone significant changes, it's important to verify that all modifications are correct and maintain the intended functionality. Please run the following script to retrieve and analyze the fullrelayfunction:
gas optimized claim/refund transfers -- no longer using universalTransfer
dispute (& respective tests) switched to emitting relayer (as was always indicated by the output param label), not guard address.
storage-read optimizations to come in a follow-up PR
Summary by CodeRabbit
New Features
Bug Fixes
Documentation