FbV2 sender nonces [SLT-183] #3214
Conversation
WalkthroughThe Changes
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: |
0cb26c4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c0ba66dc.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-fbv2-sendernonce2.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3214 +/- ##
===================================================
+ Coverage 90.56974% 90.74960% +0.17986%
===================================================
Files 54 60 +6
Lines 1018 1254 +236
Branches 82 150 +68
===================================================
+ Hits 922 1138 +216
- Misses 93 112 +19
- Partials 3 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: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (3 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (1 hunks)
- packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (4 hunks)
🔇 Additional comments (7)
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (6)
106-107: Excellent addition of nonce checks!These assertions are crucial for verifying the correct implementation of sender-specific nonces. They ensure that:
- The nonce for
userAis incremented after the bridge operation.- The nonce for
userBremains unchanged, as expected.This helps prevent potential replay attacks and ensures proper transaction ordering per sender.
117-118: Great addition of nonce checks for different sender scenario!These assertions are vital for verifying the correct behavior of sender-specific nonces when the caller is different from the sender. They confirm that:
- The nonce for
userA(the sender specified in the parameters) is incremented after the bridge operation.- The nonce for
userB(the actual caller) remains unchanged.This ensures that the nonce is correctly associated with the specified sender, not the transaction caller, maintaining proper transaction ordering and security.
133-134: Valuable nonce checks before ETH bridge operation!These assertions are important for establishing the correct initial state before the ETH bridge operation. They verify that:
- The nonce for
userAis 1, reflecting the previous token bridge operation.- The nonce for
userBis still 0, as expected.This ensures that the test starts from a known, correct state, which is crucial for the validity of subsequent operations and assertions.
139-140: Crucial nonce checks after ETH bridge operation!These assertions are essential for verifying the correct nonce behavior after multiple operations. They confirm that:
- The nonce for
userAis incremented to 2, reflecting both the token and ETH bridge operations.- The nonce for
userBremains 0, as it hasn't performed any operations.This ensures that the nonce is correctly incremented for each operation, maintaining proper transaction ordering and preventing potential replay attacks across different asset types.
148-149: Excellent nonce checks for ETH bridge with different sender!These assertions and the modified bridge call are crucial for testing the correct behavior of sender-specific nonces in a complex scenario. They verify that:
- Initial state:
userA's nonce is 1 (from token bridge),userB's nonce is 0.- The ETH bridge is called by
userBbut withuserAas the sender.- Final state:
userA's nonce is incremented to 2,userB's nonce remains 0.This test ensures that the nonce is correctly associated with the specified sender (userA) rather than the transaction caller (userB), maintaining proper transaction ordering and security across different asset types and caller scenarios.
Also applies to: 153-156
164-165: Vital nonce checks for user-specific nonce initialization!These assertions and the explicit nonce setting are crucial for testing the correct initialization and increment of user-specific nonces. They verify that:
- Initial state:
userA's nonce is 1 (from previous token bridge),userB's nonce is 0.- The ETH bridge transaction for
userBis explicitly set with nonce 0.- Final state:
userA's nonce remains 1,userB's nonce is incremented to 1.This test ensures that:
- New users start with a nonce of 0.
- The nonce is correctly incremented for a user's first transaction.
- Other users' nonces remain unaffected.
This is essential for maintaining proper transaction ordering and security, especially when introducing new users to the system.
Also applies to: 174-175
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
31-32: Approved: Addition of sender-specific nonce trackingThe introduction of
mapping(address => uint256) public senderNoncesenhances the nonce management by tracking nonces per sender, reducing risks associated with global nonces and reorgs.
| function test_nonce() public view { | ||
| // deprecated. should always return zero in FbV2. | ||
| assertEq(fastBridge.nonce(), 0); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Update the test_nonce() function to reflect the new per-sender nonce implementation
The FastBridgeV2.sol contract now implements per-sender nonces using the senderNonces mapping. However, the test_nonce() function in FastBridgeV2.Src.Base.t.sol asserts that fastBridge.nonce() is always zero and is marked as deprecated. This discrepancy can lead to inaccurate test coverage and potential misunderstandings of the contract's functionality.
- Action Items:
- Revise the
test_nonce()function to test thesenderNoncesmapping correctly, ensuring that nonces increment per sender as intended. - Remove or update the deprecated comment to clarify the current status and purpose of the
test_nonce()function. - Ensure all related tests are updated to align with the new nonce implementation to maintain consistent and reliable test coverage.
- Revise the
🔗 Analysis chain
Clarify the purpose of the test_nonce() function
The newly added test_nonce() function appears to be inconsistent with the PR objectives. While the PR aims to implement a per-sender nonce, this test function asserts that the nonce is always zero and is marked as deprecated. This raises several concerns:
- If the nonce is always zero, it doesn't align with the goal of implementing a per-sender nonce.
- The "deprecated" comment suggests this might be temporary or outdated code.
- The function name implies it's a test, but it's not using a testing framework assertion (e.g.,
assertinstead ofassertEq).
Consider the following actions:
- Clarify if this function is intended to be part of the final implementation or if it's a temporary placeholder.
- If it's meant to stay, update the function to properly test the new per-sender nonce behavior.
- If it's temporary, consider removing it or adding a TODO comment explaining its future purpose.
To better understand the context, let's check for other nonce-related implementations:
This will help us verify if the nonce implementation has been updated elsewhere in the contract.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for nonce-related implementations in the FastBridgeV2 contract
rg --type solidity -i "nonce" packages/contracts-rfq/src/FastBridgeV2.sol
Length of output: 106
Script:
#!/bin/bash
# Search for nonce-related implementations in the FastBridgeV2 contract without specifying file type
rg -i "nonce" packages/contracts-rfq/src/FastBridgeV2.sol
Length of output: 141
Script:
#!/bin/bash
# Search for FastBridgeV2.sol in the entire repository
fd "FastBridgeV2\.sol" . -t f
Length of output: 147
Script:
#!/bin/bash
# Search for nonce-related implementations in FastBridgeV2.sol
rg -i "nonce" packages/contracts-rfq/contracts/FastBridgeV2.sol
Length of output: 360
| @@ -158,7 +163,7 @@ | |||
| originFeeAmount: originFeeAmount, | |||
| sendChainGas: params.sendChainGas, | |||
| deadline: params.deadline, | |||
| nonce: nonce++, // increment nonce on every bridge | |||
| nonce: senderNonces[params.sender]++, // increment nonce on every bridge | |||
There was a problem hiding this comment.
Potential off-by-one error due to postfix increment operator
The use of the postfix increment operator senderNonces[params.sender]++ assigns the current nonce value and then increments it. This means the nonce included in the request is the pre-incremented value. If the intention is to include the incremented nonce value, consider using the prefix increment operator ++senderNonces[params.sender] to avoid potential off-by-one errors.
Suggested change:
- nonce: senderNonces[params.sender]++, // increment nonce on every bridge
+ nonce: ++senderNonces[params.sender], // increment nonce on every bridge📝 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.
| nonce: senderNonces[params.sender]++, // increment nonce on every bridge | |
| nonce: ++senderNonces[params.sender], // increment nonce on every bridge |
| /// @notice This function is deprecated and should not be used. | ||
| /// @dev Replaced by senderNonces | ||
| function nonce() external pure returns (uint256) { | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider modifying or removing the deprecated nonce() function
The nonce() function is deprecated and currently returns 0, which might lead to confusion or unintended behavior if external callers rely on its previous functionality. Consider either removing the function or updating it to revert with a clear deprecation message to prevent misuse.
Suggested change:
function nonce() external pure returns (uint256) {
- return 0;
+ revert("nonce() is deprecated and should not be used.");
}📝 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.
| /// @notice This function is deprecated and should not be used. | |
| /// @dev Replaced by senderNonces | |
| function nonce() external pure returns (uint256) { | |
| return 0; | |
| } | |
| /// @notice This function is deprecated and should not be used. | |
| /// @dev Replaced by senderNonces | |
| function nonce() external pure returns (uint256) { | |
| revert("nonce() is deprecated and should not be used."); | |
| } | |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
31-36: Improved nonce management with per-sender trackingThe introduction of
senderNoncesmapping enhances security by preventing potential replay attacks across different senders. The deprecatednoncevariable is correctly handled.Consider adding a comment explaining why the deprecated
nonceis kept as an immutable constant.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/contracts-rfq/.gitignore (1 hunks)
- packages/contracts-rfq/contracts/FastBridgeV2.sol (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/contracts-rfq/.gitignore
🔇 Additional comments (3)
packages/contracts-rfq/contracts/FastBridgeV2.sol (3)
163-163: Nonce increment implementation is correctThe use of
senderNonces[params.sender]++is appropriate here. It assigns the current nonce to the transaction and then increments it for the next use, which is the desired behavior for nonce management.Regarding the previous comment about a potential off-by-one error: The current implementation is correct. Using the postfix increment operator ensures that each transaction gets a unique, sequential nonce starting from the initial value (likely 0). This approach is standard in nonce management systems.
Line range hint
1-1: Summary of nonce management changesThe implementation of per-sender nonces in the
FastBridgeV2contract effectively addresses the PR objectives. The new system reduces the risk of reorgs leading to different nonces being used, enhancing the overall security of the bridge transactions.Key points:
- The
senderNoncesmapping provides unique nonce tracking per sender.- The
bridgefunction correctly implements the new nonce management system.- The old
noncevariable is properly deprecated.These changes align well with the goal of improving nonce management reliability in the system.
Line range hint
1-1: Verify the status of the deprecatednonce()functionThe previously mentioned deprecated
nonce()function is not visible in the provided code snippet. Please confirm if this function has been removed or modified as suggested in the previous review.If the function still exists unchanged, consider implementing the suggested modification to prevent potential misuse:
function nonce() external pure returns (uint256) { revert("nonce() is deprecated and should not be used."); }✅ Verification successful
Nonce() Function Removal Verified
The deprecated
nonce()function is no longer present inpackages/contracts-rfq/contracts/FastBridgeV2.sol. Current references tononcepertain to state variables and do not include the deprecated function.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the nonce() function in the contract rg --type solidity 'function\s+nonce\s*\(\s*\)\s*external' packages/contracts-rfq/contracts/FastBridgeV2.solLength of output: 143
Script:
#!/bin/bash # Search for the nonce() function in the contract without specifying file type rg 'function\s+nonce\s*\(\s*\)\s*external' packages/contracts-rfq/contracts/FastBridgeV2.solLength of output: 94
Script:
#!/bin/bash # Search for any nonce() function definitions in the contract ast-grep --lang solidity 'function nonce($_) { $$$ }' packages/contracts-rfq/contracts/FastBridgeV2.sol # Search for any references or comments mentioning nonce in the contract rg 'nonce' packages/contracts-rfq/contracts/FastBridgeV2.sol -A 2Length of output: 797
Bundle ReportBundle size has no change ✅ |
Description
Replace chain-scoped nonce with one that increments per-sender
Additional context
Reduces risk of reorgs resulting in a different nonce
Summary by CodeRabbit
New Features
Bug Fixes
Documentation