fix(contracts-rfq): FastBridge V2 prove race condition #3435
Conversation
WalkthroughThe changes in this pull request introduce significant enhancements to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Deploying sanguine-fe with
|
| Latest commit: |
6fdaec3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e318b026.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-fbv2-prove-race.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3435 +/- ##
============================================
Coverage ? 98.78296%
============================================
Files ? 9
Lines ? 493
Branches ? 0
============================================
Hits ? 487
Misses ? 6
Partials ? 0
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: 6
🧹 Outside diff range and nitpick comments (6)
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
Line range hint
23-37: Useuint256instead ofint256forquoteExclusivitySeconds.The
quoteExclusivitySecondsfield represents a duration and should not have negative values. Usinguint256ensures that only non-negative values are accepted, preventing potential errors due to negative durations.Apply this diff to correct the type:
struct BridgeParamsV2 { address quoteRelayer; - int256 quoteExclusivitySeconds; + uint256 quoteExclusivitySeconds; bytes quoteId; uint256 zapNative; bytes zapData; }packages/contracts-rfq/contracts/AdminV2.sol (1)
21-29: Optimize storage by packing struct variables efficiently.In the
ProverInfostruct, consider reordering the variables to optimize storage. Placing smaller types together can reduce the storage slots used.Apply this diff to reorder the struct fields:
struct ProverInfo { - uint16 id; - uint240 activeFromTimestamp; + uint240 activeFromTimestamp; + uint16 id; }This change can help in reducing gas costs associated with storage operations.
packages/contracts-rfq/test/FastBridgeV2.Management.t.sol (1)
16-18: Recommend adding comments explaining new constants.The constants
MIN_PROVER_TIMEOUTandDEFAULT_PROVER_TIMEOUThave been introduced. Adding comments explaining their purpose enhances code readability and maintainability.Apply this diff to add comments:
+// Minimum timeout duration for a prover uint256 public constant MIN_PROVER_TIMEOUT = 1 minutes; +// Default timeout duration for a prover uint256 public constant DEFAULT_PROVER_TIMEOUT = 30 minutes;packages/contracts-rfq/contracts/interfaces/IAdminV2Errors.sol (1)
7-10: Consolidate error declarations for clarity.Consider grouping related error declarations together and adding comments for better readability.
Apply this diff to group prover-related errors:
error CancelDelayBelowMin(); error FeeRateAboveMax(); +// Prover-related errors error ProverAlreadyActive(); error ProverCapacityExceeded(); error ProverNotActive(); error ProverTimeoutBelowMin();packages/contracts-rfq/contracts/interfaces/IAdminV2.sol (2)
14-18: Consider adding validation requirements in function documentationThe admin functions for adding/removing provers should document any address validation requirements (e.g., non-zero address checks).
38-44: Consider documenting return value behavior for edge casesThe view function documentation should specify the return values for edge cases (e.g., when prover address is zero or invalid).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
packages/contracts-rfq/contracts/AdminV2.sol(5 hunks)packages/contracts-rfq/contracts/FastBridgeV2.sol(4 hunks)packages/contracts-rfq/contracts/interfaces/IAdminV2.sol(2 hunks)packages/contracts-rfq/contracts/interfaces/IAdminV2Errors.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol(1 hunks)packages/contracts-rfq/foundry.toml(1 hunks)packages/contracts-rfq/test/FastBridgeV2.Management.t.sol(7 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol(2 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.t.sol(24 hunks)packages/contracts-rfq/test/FastBridgeV2.t.sol(2 hunks)packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol(1 hunks)packages/contracts-rfq/test/integration/TokenZapV1.t.sol(1 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-11-12T03:37:02.230Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
🔇 Additional comments (13)
packages/contracts-rfq/test/integration/TokenZapV1.t.sol (1)
48-48: LGTM! The change correctly uses the new prover management API.
The update from grantRole() to addProver() aligns with the new prover management system introduced in FastBridge V2.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
18-19: Verify the sufficiency of uint40 for proofBlockTimestamp and potential risks.
The proofBlockTimestamp type has been changed from uint56 to uint40, which reduces the maximum representable timestamp. Ensure that uint40 is sufficient to store timestamps for the expected lifespan of the contract to prevent any overflow issues in the future.
Run the following script to calculate the maximum date representable by uint40:
This will help confirm that the uint40 type is adequate for the application's needs.
✅ Verification successful
uint40 is sufficient for timestamp storage until year 36812
The uint40 type can store timestamps until February 20, 36812, which provides an extremely generous time buffer of over 34,000 years from now. This is more than sufficient for any practical blockchain application, making the reduction from uint56 a safe optimization that won't cause overflow issues in the foreseeable future.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Calculate the maximum date that can be stored in a uint40 (in years from Unix epoch)
# uint40 max value
MAX_UINT40=$((2**40 - 1))
# Calculate the maximum date
date -d "@$MAX_UINT40" +"%Y-%m-%d %H:%M:%S"
Length of output: 93
packages/contracts-rfq/contracts/AdminV2.sol (2)
93-109: Check for potential reentrancy in addProver function.
The addProver function updates state variables and emits events. Ensure that there are no external calls before state changes are completed to prevent reentrancy attacks.
198-212: Ensure safe arithmetic operations in _applyTimeoutPenalty.
When calculating newActiveFromTimestamp, make sure that the addition of proverTimeout to block.timestamp cannot overflow.
Since Solidity 0.8.x handles integer overflows with built-in checks, confirm that proverTimeout is within acceptable bounds to prevent any unexpected behavior.
packages/contracts-rfq/test/FastBridgeV2.Management.t.sol (1)
188-191: Handle edge cases in test_addProver_revertAlreadyActive.
Ensure that attempting to add an already active prover gracefully reverts with the appropriate error message.
Run tests to confirm that the correct error (ProverAlreadyActive) is thrown when expected.
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
356-367: Verify prover authentication in proveV2 function.
The proveV2 function now relies on getActiveProverID(msg.sender) instead of role-based access control. Ensure that this change does not introduce security vulnerabilities and that only valid, active provers can call this function.
Run the following script to check for any functions where inactive provers might bypass authentication:
Confirm that all functions interacting with provers correctly validate their active status.
packages/contracts-rfq/foundry.toml (1)
10-10: Consider using a more realistic gas limit
The current gas limit (2^63 - 1) is extremely high and could mask potential gas-related issues during testing. Consider using a value closer to real network limits (e.g., Ethereum mainnet's ~30M) to catch potential out-of-gas scenarios early.
packages/contracts-rfq/contracts/interfaces/IAdminV2.sol (2)
10-12: LGTM! Events provide good transparency
The events properly track prover lifecycle changes, enhancing transparency and auditability.
24-27: LGTM! Clear documentation of prover timeout mechanism
The documentation clearly explains the purpose and behavior of the prover timeout mechanism.
packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (2)
14-14: LGTM! Well-chosen test constant
The PROVER_TIMEOUT value (4.2 minutes) differs from default values, which helps verify correct configuration in tests.
30-38: LGTM! Proper test configuration
The configuration properly sets up the new prover management system and timeout mechanism. The changes align with the interface updates in IAdminV2.
packages/contracts-rfq/test/FastBridgeV2.t.sol (2)
12-12: LGTM: Import aligns with new prover management features
The addition of IAdminV2Errors import supports testing the new prover management functionality and error handling.
22-22: LGTM: Contract inheritance properly extended
The addition of IAdminV2Errors to the inheritance list enables testing of prover-related error cases, which is essential for validating the new prover timeout penalty mechanism.
packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function getActiveProverID(address prover) public view returns (uint16) { | ||
| // Aggregate the read operations from the same storage slot. | ||
| uint16 id = _proverInfos[prover].id; | ||
| uint256 activeFromTimestamp = _proverInfos[prover].activeFromTimestamp; | ||
| // Return zero if the prover has never been added or is no longer active. | ||
| if (activeFromTimestamp == 0 || activeFromTimestamp > block.timestamp) return 0; | ||
| return id; | ||
| } |
Check warning
Code scanning / Slither
Dangerous strict equalities Medium
|
@parodime sounds good, I updated the labeling and the PR description as well |
| function _applyDisputePenaltyTime(uint16 proverID) internal { | ||
| // Check that the prover exists. | ||
| if (proverID == 0 || proverID > _allProvers.length) return; | ||
| address prover = _allProvers[proverID - 1]; | ||
| ProverInfo storage $ = _proverInfos[prover]; | ||
| // No-op if the prover is already inactive. | ||
| if ($.activeFromTimestamp == 0) return; | ||
| uint256 newActiveFromTimestamp = block.timestamp + disputePenaltyTime; | ||
| // Update the activeFrom timestamp. | ||
| // Note: this is a storage write. | ||
| $.activeFromTimestamp = uint240(newActiveFromTimestamp); | ||
| emit DisputePenaltyTimeApplied(prover, newActiveFromTimestamp); | ||
| } |
Check warning
Code scanning / Slither
Dead-code Warning
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (1)
30-38: Ensure consistent test configuration across inherited test contractsThe addition of
addProverandsetDisputePenaltyTimein theconfigureFastBridgefunction modifies the base test setup. If other test contracts inherit fromFastBridgeV2SrcBaseTest, ensure they expect these configurations or consider encapsulating these setups within specific test contracts to prevent unintended side effects.packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
356-358: Consider additional validation for prover ID check.While the strict equality check
proverID == 0works for the current implementation, consider adding a require statement with a more descriptive error message for better debugging.- if (proverID == 0) revert ProverNotActive(); + require(proverID != 0, "FastBridgeV2: prover not active or has been removed");🧰 Tools
🪛 GitHub Check: Slither
[warning] 356-371: Dangerous strict equalities
FastBridgeV2.proveV2(bytes32,bytes32,address) (contracts/FastBridgeV2.sol#356-371) uses a dangerous strict equality:
- proverID == 0 (contracts/FastBridgeV2.sol#358)
[notice] 356-371: Block timestamp
FastBridgeV2.proveV2(bytes32,bytes32,address) (contracts/FastBridgeV2.sol#356-371) uses timestamp for comparisons
Dangerous comparisons:
- proverID == 0 (contracts/FastBridgeV2.sol#358)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
packages/contracts-rfq/contracts/AdminV2.sol(5 hunks)packages/contracts-rfq/contracts/FastBridgeV2.sol(4 hunks)packages/contracts-rfq/contracts/interfaces/IAdminV2.sol(2 hunks)packages/contracts-rfq/contracts/interfaces/IAdminV2Errors.sol(1 hunks)packages/contracts-rfq/test/FastBridgeV2.Management.t.sol(7 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol(2 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.t.sol(24 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (2)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-11-12T03:37:02.230Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/FastBridgeV2.sol
[warning] 356-371: Dangerous strict equalities
FastBridgeV2.proveV2(bytes32,bytes32,address) (contracts/FastBridgeV2.sol#356-371) uses a dangerous strict equality:
- proverID == 0 (contracts/FastBridgeV2.sol#358)
[notice] 356-371: Block timestamp
FastBridgeV2.proveV2(bytes32,bytes32,address) (contracts/FastBridgeV2.sol#356-371) uses timestamp for comparisons
Dangerous comparisons:
- proverID == 0 (contracts/FastBridgeV2.sol#358)
packages/contracts-rfq/contracts/AdminV2.sol
[warning] 112-118: Dangerous strict equalities
AdminV2.removeProver(address) (contracts/AdminV2.sol#112-118) uses a dangerous strict equality:
- getActiveProverID(prover) == 0 (contracts/AdminV2.sol#113)
[notice] 112-118: Block timestamp
AdminV2.removeProver(address) (contracts/AdminV2.sol#112-118) uses timestamp for comparisons
Dangerous comparisons:
- getActiveProverID(prover) == 0 (contracts/AdminV2.sol#113)
[warning] 189-196: Dangerous strict equalities
AdminV2.getActiveProverID(address) (contracts/AdminV2.sol#189-196) uses a dangerous strict equality:
- activeFromTimestamp == 0 || activeFromTimestamp > block.timestamp (contracts/AdminV2.sol#194)
[warning] 200-212: Dead-code
AdminV2._applyDisputePenaltyTime(uint16) (contracts/AdminV2.sol#200-212) is never used and should be removed
🔇 Additional comments (10)
packages/contracts-rfq/contracts/AdminV2.sol (1)
200-212: 🛠️ Refactor suggestion
Consider removing the unused function _applyDisputePenaltyTime
The internal function _applyDisputePenaltyTime is never called within the contract. Keeping unused code can lead to confusion and increases maintenance overhead. Consider removing this function if it is not intended for future use.
🧰 Tools
🪛 GitHub Check: Slither
[warning] 200-212: Dead-code
AdminV2._applyDisputePenaltyTime(uint16) (contracts/AdminV2.sol#200-212) is never used and should be removed
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (2)
376-387: Add a test case to verify prover reactivation after penalty time
The test_prove_revert_disputePenaltyTime function checks that a prover cannot prove during the penalty period. It would be beneficial to add a test confirming that the prover becomes active again and can successfully prove after DISPUTE_PENALTY_TIME has elapsed. This ensures the contract handles prover reactivation correctly.
517-527: Include a test for prover reactivation after penalty time in test_proveOther_revert_disputePenaltyTime
In test_proveOther_revert_disputePenaltyTime, the test verifies that a prover cannot prove during the penalty period after a dispute. Consider adding a test case to confirm that the prover can prove again once the DISPUTE_PENALTY_TIME has passed, ensuring proper reactivation logic.
packages/contracts-rfq/contracts/interfaces/IAdminV2Errors.sol (1)
7-10: LGTM
The addition of specific error types improves the contract's clarity and error handling. These errors provide precise feedback for different prover-related conditions.
packages/contracts-rfq/contracts/interfaces/IAdminV2.sol (1)
Line range hint 6-52: LGTM
The introduction of new events and functions for prover management enhances the contract's functionality and maintainability. The interface updates align with the changes in AdminV2.sol, facilitating proper interaction with provers and dispute penalty configurations.
packages/contracts-rfq/test/FastBridgeV2.Management.t.sol (3)
9-9: LGTM: Constants for dispute penalty time are well-defined.
The constants provide reasonable defaults:
MIN_DISPUTE_PENALTY_TIME = 1 minutes: Minimum penalty durationDEFAULT_DISPUTE_PENALTY_TIME = 30 minutes: Default penalty duration
Also applies to: 16-17
48-56: LGTM: Test helper functions for prover management.
The helper functions properly use vm.prank to test different caller scenarios.
103-179: LGTM: Comprehensive test coverage for prover management.
The test suite thoroughly covers:
- Basic prover addition and removal
- Re-adding provers after removal
- Prover info validation
- Edge cases and state transitions
packages/contracts-rfq/contracts/FastBridgeV2.sol (2)
111-114: LGTM: Prover timeout penalty mechanism implementation.
The implementation correctly:
- Extracts the prover ID from the disputed transaction
- Applies the timeout penalty to the prover
- Handles the case where the prover might have been removed
The comment on line 123 effectively documents the edge case handling.
Also applies to: 122-124
484-491: LGTM: Improved timestamp handling with clear documentation.
The implementation:
- Uses uint40 for gas optimization
- Correctly handles timestamp rollover
- Includes clear documentation explaining the design decisions
| function getProvers() external view returns (address[] memory provers) { | ||
| uint256 length = _allProvers.length; | ||
| // Calculate the number of active provers. | ||
| uint256 activeProversCount = 0; | ||
| for (uint256 i = 0; i < length; i++) { | ||
| if (getActiveProverID(_allProvers[i]) != 0) { | ||
| activeProversCount++; | ||
| } | ||
| } | ||
| // Do the second pass to populate the provers array. | ||
| provers = new address[](activeProversCount); | ||
| uint256 activeProversIndex = 0; | ||
| for (uint256 i = 0; i < length; i++) { | ||
| address prover = _allProvers[i]; | ||
| if (getActiveProverID(prover) != 0) { | ||
| provers[activeProversIndex++] = prover; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize getProvers function to reduce gas consumption
The getProvers function currently iterates over _allProvers twice: once to count active provers and again to populate the array. This can be optimized by using a single iteration and dynamically building the array, which would reduce gas costs.
Apply the following diff to optimize the function:
function getProvers() external view returns (address[] memory provers) {
uint256 length = _allProvers.length;
- uint256 activeProversCount = 0;
- for (uint256 i = 0; i < length; i++) {
- if (getActiveProverID(_allProvers[i]) != 0) {
- activeProversCount++;
- }
- }
- provers = new address[](activeProversCount);
- uint256 activeProversIndex = 0;
+ address[] memory activeProvers = new address[](length);
+ uint256 count = 0;
for (uint256 i = 0; i < length; i++) {
address prover = _allProvers[i];
if (getActiveProverID(prover) != 0) {
- provers[activeProversIndex++] = prover;
+ activeProvers[count++] = prover;
}
}
+ // Resize the array to the actual number of active provers
+ assembly {
+ mstore(activeProvers, count)
+ }
+ provers = activeProvers;
}📝 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 getProvers() external view returns (address[] memory provers) { | |
| uint256 length = _allProvers.length; | |
| // Calculate the number of active provers. | |
| uint256 activeProversCount = 0; | |
| for (uint256 i = 0; i < length; i++) { | |
| if (getActiveProverID(_allProvers[i]) != 0) { | |
| activeProversCount++; | |
| } | |
| } | |
| // Do the second pass to populate the provers array. | |
| provers = new address[](activeProversCount); | |
| uint256 activeProversIndex = 0; | |
| for (uint256 i = 0; i < length; i++) { | |
| address prover = _allProvers[i]; | |
| if (getActiveProverID(prover) != 0) { | |
| provers[activeProversIndex++] = prover; | |
| } | |
| } | |
| } | |
| function getProvers() external view returns (address[] memory provers) { | |
| uint256 length = _allProvers.length; | |
| address[] memory activeProvers = new address[](length); | |
| uint256 count = 0; | |
| for (uint256 i = 0; i < length; i++) { | |
| address prover = _allProvers[i]; | |
| if (getActiveProverID(prover) != 0) { | |
| activeProvers[count++] = prover; | |
| } | |
| } | |
| // Resize the array to the actual number of active provers | |
| assembly { | |
| mstore(activeProvers, count) | |
| } | |
| provers = activeProvers; | |
| } |
Description
This PR adds the concept of "dispute penalty time":
disputePenaltyTime.BridgeTxDetailsgrantRole().Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests